dependabot-core icon indicating copy to clipboard operation
dependabot-core copied to clipboard

Address CodeQL warnings about http vs https

Open jeffwidman opened this issue 2 years ago • 1 comments

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.

jeffwidman avatar Jul 29 '22 05:07 jeffwidman

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 avatar Jul 29 '22 05:07 jeffwidman

@jeffwidman should this be closed out?

abdulapopoola avatar Mar 28 '23 22:03 abdulapopoola

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.

deivid-rodriguez avatar Mar 31 '23 10:03 deivid-rodriguez

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.

jeffwidman avatar Apr 20 '23 07:04 jeffwidman