dependabot-core
dependabot-core copied to clipboard
Address CodeQL warnings about http vs https
CodeQL threw a bunch of warnings a bit ago about some of our dependency
download URLs using http
and not https
, examples:
- https://github.com/dependabot/dependabot-core/security/code-scanning/29
- https://github.com/dependabot/dependabot-core/security/code-scanning/30
At the time, we closed these as wontfix since they're only used in tests.
While that is completely true, the problem is that down the road a future developer will copy/paste/tweak the fixture for another test. And that will generate a new CodeQL alert.
So instead, let's get ahead of it by fixing the relevant URLs to use
https
.
Besides, both of these root domains, rubygems.org and github.com
redirect all http
traffic to https
, so this is simply pulling those
301's into our fixtures.
Note that we don't lose test coverage on whether we properly handle
non-https urls... I still saw tests checking that
http://github.com/<some-dep-url>
works, so we still cover that case.
As best I could tell, these are not autogenerated files, so it's safe to manually edit them.
But if I'm misunderstanding something about how Gemfiles/bundler works, and I have to either programmatically regenerate, or simply can't edit at all, then please enlighten me...
@jeffwidman should this be closed out?
For what it's worth, I think this is a good change. There should not be coverage loss since as @jeffwidman points out there are still tests checking for http. Actually, I see it as the opposite, by using https by default, we represent real usage better. For example, hardly anyone uses http://rubygems.org
in the Gemfile
these days, and that may be discontinued any time.
I think this is really "six" one way and "half-a-dozen" the other...
I still think the convenience of pre-empting copy/paste generating CodeQL warnings makes sense, especially since not doing it, while technically more protective against http
scenarios, pragmatically doesn't matter much since http
is rarely used... and it's easy enough to add a one-off test if we discover later we overlooked something. That would be a dedicated test, which as Landon pointed out would be better than the incidental tests that we have now.
Seems no one feels too strongly, and most feel slightly toward merging, I'm going to ship.