Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new_audit: bring back redirects-http with passive https check #13548

Merged
merged 10 commits into from
Apr 9, 2024

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Jan 7, 2022

hot-take/needs-discussion PR: Bring back the redirects-http audit, but without the extra pass.

  • If requestedUrl is on http and
    • finalUrl is on https, pass
    • finalUrl isn't on https, fail
  • if requestedUrl is on any other scheme, notApplicable

redirects-http was removed in #12643 due to a history of debugger protocol issues, the massive slowness of an extra pass, and the fact that Chrome switched to https by default if a user doesn't supply a scheme.

However there are still sites out there that should be redirecting to https and Lighthouse should continue to nudge the remaining 10-20% of sites people spend time on to do so if it's feasible.

We already have this audit in git history ready to go with just a few changes, and switching to only alerting when someone is purposefully testing their http site should take user complaints down to zero (for instance, the old check required the http version of a site to respond, if only to redirect), which also allows making the audit essentially zero-overhead by passively observing what the URL artifact is collecting anyways.

Thoughts? :)

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

Sorry for late approval.

@@ -23,9 +23,14 @@ const config = {
*/
const expectations = {
lhr: {
requestedUrl: 'https://jakearchibald.github.io/svgomg/',
// Intentionally start out on http to test the redirect.
requestedUrl: 'http://jakearchibald.github.io/svgomg/',
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a few redirects smoke tests, should it go there instead?

Otherwise, please add an exclusion to these expectations so we can still run these smokes in devtools runner.

@connorjclark
Copy link
Collaborator

@brendankenny branch updated

Object.assign(/IndexedDB/, {_runner: 'devtools'}),
// DevTools can't simply test a redirect, because the browser will have already navigated before the Lighthouse panel can begin.
Object.assign(/redirected/, {_excludeRunner: 'devtools'}),
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤯

Copy link
Member

Choose a reason for hiding this comment

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

🤯

core/audits/redirects-http.js Show resolved Hide resolved
core/audits/redirects-http.js Outdated Show resolved Hide resolved
core/test/audits/redirects-http-test.js Outdated Show resolved Hide resolved
core/test/audits/redirects-http-test.js Show resolved Hide resolved
Object.assign(/IndexedDB/, {_runner: 'devtools'}),
// DevTools can't simply test a redirect, because the browser will have already navigated before the Lighthouse panel can begin.
Object.assign(/redirected/, {_excludeRunner: 'devtools'}),
],
Copy link
Member

Choose a reason for hiding this comment

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

🤯

@connorjclark
Copy link
Collaborator

It's been two years but it seems like this would still be useful.

@adamraine
Copy link
Member

Doesn't appear to be any downsides to this. I'll update for 12.0.

@adamraine adamraine self-assigned this Apr 1, 2024
@adamraine
Copy link
Member

Aaaaactually I'm pretty sure is-on-https already checks that the main document request is secure...

So maybe we just close this?

@brendankenny
Copy link
Member Author

Aaaaactually I'm pretty sure is-on-https already checks that the main document request is secure...

Conceptually they're different checks. is-on-https is "was everything, end to end, sent over https", while redirects-http was specifically about the redirect. Separating them allows the redirect to be communicated more clearly ("you had an insecure request to http://example.com" vs "you didn't redirect from http://example.com to https://example.com").

The context of this PR is that there is a difference, and it would be little effort (and none of the complexity that prompted deleting the audit in the first place would be required) to keep the audit, so why not keep it.

On the other hand, yes, a lot of overlap, and the path to being able to pass both if you start a LH run on an insecure URL is very narrow (just preloaded/cached HSTS these days? Any other approah?).

In #3417 there was a proposal to relax is-on-https basically as #15907 does, then introduce a new "susceptible to a downgrade attack" audit. Now I don't know why adding a third audit seemed like a good idea, but that was a long time ago :)

My quick opinion two years later: I think it's still good to warn users when they're not using HTTPS or not upgrading, both of which #15907 communicates (albeit less clearly without a dedicated description). I wish we could do more with giving advice about the connection upgrade etc, but that's not the direction best practices is going, so it's fine.

@adamraine adamraine merged commit 9bd33f8 into main Apr 9, 2024
27 checks passed
@adamraine adamraine deleted the resurrect-redirect branch April 9, 2024 23:08
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.

None yet

4 participants