Skip to content

core(is-on-https): pass if http redirects to https#15907

Closed
adamraine wants to merge 4 commits intomainfrom
is-on-https-redirect
Closed

core(is-on-https): pass if http redirects to https#15907
adamraine wants to merge 4 commits intomainfrom
is-on-https-redirect

Conversation

@adamraine
Copy link
Copy Markdown
Contributor

@adamraine adamraine commented Apr 2, 2024

Halfway through updating #13548, I realized this audit already covers the passively checking http/https case. However, it will still flag insecure requests even if they redirect to a secure protocol.

This PR creates an exception for insecure main document redirects.

const devtoolsLogs = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const networkRecords = await NetworkRecords.request(devtoolsLogs, context);
const insecureURLs = networkRecords
.filter(record => !record.redirectDestination)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be limited to the initial document request chain and not allow any other resources that redirected.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also don't want to allow https -> http -> https redirects?

Copy link
Copy Markdown
Contributor Author

@adamraine adamraine Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be limited to the initial document request chain and not allow any other resources that redirected.

sgtm, but should we allow iframe documents as well or just the main document?

Also don't want to allow https -> http -> https redirects?

The impl in #13548 doesn't seem to care about intermediate redirects. Is there a strong reason for this?

Copy link
Copy Markdown
Contributor

@brendankenny brendankenny Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm, but should we allow iframe documents as well or just the main document?

IMO the only leeway should be if you put in an http URL to test, so not including iframes, yeah. Like you can't control what URL a user starts with (can only redirect them), but you can control the URLs of any iframes the page loads.

Also don't want to allow https -> http -> https redirects?

The impl in #13548 doesn't seem to care about intermediate redirects. Is there a strong reason for this?

Yeah, not included there, and I added the question mark because I'm not sure if it's worth failing on. I was just trying to think through possible failure modes.

Seems bad? Starting secure but letting an intermediate response do any number of things that could be intercepted/altered. OTOH is there any increased risk vs starting with an insecure request? OTOOH, kind of ridiculous if that's the redirect chain for your site?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting secure but letting an intermediate response do any number of things that could be intercepted/altered.

This has me leaning more towards #13548 and closing this PR haha. This sounds like a real security problem that we would want to flag in any situation. It's just a more severe problem if the site didn't at least redirect to https.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, well, up to you :)

Couldn't resist checking, and there are about 100k/0.4% of sites in a recent HTTP Archive crawl that did this (went at least https->http->https, ending on an https).

Interestingly ~99% of them end up on the same origin they started on by the end of things. A lot of them are of the form

  • https://example.com
  • http://example.com/en
  • https://example.com/en

so it does seem like a real error and something they can/should fix

@adamraine
Copy link
Copy Markdown
Contributor Author

Going back to reviving #13548

@adamraine adamraine closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants