HTTPS upgrades proposal

[carlosjoan91]

• At least two implementers are interested (and none opposed):
  • Chrome is proposing the change https://groups.google.com/a/chromium.org/g/blink-dev/c/cAS525en8XE/m/OdMMGgLXAgAJ
  • Mozilla: HTTPS Upgrades mozilla/standards-positions#800
• Tests are written and can be reviewed and commented upon at:
  • https://github.com/web-platform-tests/wpt/tree/master/https-upgrades/tentative
• Implementation bugs are filed:
  • Chromium: crbug.com/1394910
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1855558
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=262211
  • Deno (not for CORS changes): N/A
• MDN issue is filed: Not Applicable since this doesn't introduce any new API or change any existing one.

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[mikewest]

A few comments:

[yoavweiss]

A few more nits. Otherwise, non-authoritiative LGTM

[christhompson]

Thanks for all the help with editing this Yoav!

@annevk I think this PR should be in a good place now if you have time to take a look.

[christhompson]

I'm not sure why the CI build is failing -- the bikeshed error FATAL ERROR: Spurious / in <a> isn't reproducible when running make local locally.

[annevk]

It would help a lot if this was rebased as a single commit describing the changes it is making. (I attempted to rebase locally against main in order to address the CI issue but immediately ran into a merge conflict.)

[annevk]

Thanks for working on this. I've done an initial review that mainly focuses on editorial issues.

[meacer]

annevk: Friendly ping

[christhompson]

Pinging again -- let us know if there are any remaining concerns.

[annevk]

Apologies for the delay here. I blame European summer.

So this still applies:

It would help a lot if this was rebased as a single commit describing the changes it is making. (I attempted to rebase locally against main in order to address the CI issue but immediately ran into a merge conflict.)

In particular it seems Build (CI) hasn't even run for this change, which is a problem.

[meacer]

Thank you for the review! Rebased into a single commit, but the Build still doesn't seem to be running. I get a "1 workflow awaiting approval" warning above it, so not sure if a maintainer needs to specifically allow it to run.

[annevk]

Hey @meacer, thanks for the update. I left a bunch of inline comments including on some of the old threads. I think it would be good for you to resolve all the threads you consider resolved at this point and leave those open you still have questions on. Generally instead of writing "Done" hitting "Resolve conversation" is fine. That also helps the reviewer to know what to focus on. Also if you want any kind of live feedback I'd recommend https://whatwg.org/chat.

[meacer]

@annevk Apologies for the comment noise. I'm not the original author of the PR so I don't have the "Resolve conversation" option. I asked Carlos to resolve most of them (thanks Carlos!). I'll need to think about the service worker parts before I respond on the remaining threads. Thank you again.

[meacer]

@annevk I believe my last commit addresses all your comments. Could you please take a look when you have a chance? Thanks.

[annevk]

Mainly nits and still the fundamental question about why you're patching HTTP fetch rather than HTTP-network fetch.

Having a commit message somewhere explaining the rationale behind the changes would go a long way.

[simon-friedberger]

@simon-friedberger are you asking for a note? The current text already requires it.

Thanks @annevk, that is indeed what I was looking for. If you don't mind, could you point me at the right text? I checked again but still cannot find it.

[annevk]

It's subtle, but because only the scheme is changed the port remains unchanged. And when the scheme is "http" the port is either null or a non-80 integer (due to the URL parser). And when you go make a request and the scheme is "https" and the port is null, it uses 443. (This subtle behavior happens in multiple places though so I'm a bit hesitant to add notes all over, but maybe there's a way to make it work well.)

[simon-friedberger]

The explainer touched on loop handling behavior:

Redirects to HTTP: If a navigation would result in a redirect to HTTP, that redirection should also get upgraded to HTTPS. This applies both to navigations that are initially to HTTP URLs which get upgraded to HTTPS (and then redirected to an HTTP URL) and to navigations that are initially to HTTPS URLs that are redirected to HTTP. If these upgrades result in a redirect loop (for example, https://http.badssl.com/ redirects to http://http.badssl.com/, which would be upgraded to https://http.badssl.com/, and so on), this should be considered a failed upgrade.

I don't see this in the PR and this WPT implies that the query string should be part of the data used for detecting loops. Should this be part of the spec and not just the test?

Also, I guess that the fragment should not be part of such a check, correct?

[annevk]

The only thing I don't see is "redirect loop" and I also don't see that being tested. However, I don't think we should add that as we already have a limit on 20 redirects that should suffice.

[simon-friedberger]

For context, Chrome seems to have explicit loop detection: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/chrome/browser/ssl/https_upgrades_interceptor.cc#519

The WPT is upgrading with new URL("http://{{host}}:{{ports[https][0]}}/fetch/api/resources/redirect.py?location=" + … which will trigger loop detection unless the query string is included.

What should happen on pages which redirect after a timeout like (www.bom.gov.au) which gets upgraded and then redirected to (http://www.bom.gov.au/akamai/https-redirect.html) which redirects using window.location.replace after 10 seconds. Using the 20 redirects limit would take over 3 minutes to get to the actual page.

[mozfreddyb]

I believe there is a disconnect about the redirect loops between what @annevk and @simon-friedberger understood.

The issue isn't reaching the maximum of allowed redirects, but whether a browser should look at previous URLs in the redirect chains to stop upgrading. E.g., for a page that supports HTTPS but redirects back to HTTP.

Specifically, how "far" does the browser look back? All 20? Which information does the browser use to consider something a loop? Just the domain? The site? With URL parameters?

If all browsers have explicit "loop breakout logic", we should specify it.

[annevk]

I thought I understood, but now various kinds of redirects are getting conflated and it's getting confusing. I'm not sure this PR is the best place to have that discussion. I don't think we should have loop detection for HTTP or (specified) internal redirects. Website-driven navigations that look like redirects are not covered here. If they should be that should also be discussed in a new issue.

[simon-friedberger]

Isn't there a problem specific to HTTP upgrades, though? If a website has a redirect loop, sure, the website needs to fix it. But if there is a redirect loop in the browser because it is upgrading HTTP to HTTPS it needs to be fixed in the browser. Anyway, just let me know if I should file a separate issue, please!

[simon-friedberger]

It's subtle, but because only the scheme is changed the port remains unchanged. And when the scheme is "http" the port is either null or a non-80 integer (due to the URL parser). And when you go make a request and the scheme is "https" and the port is null, it uses 443. (This subtle behavior happens in multiple places though so I'm a bit hesitant to add notes all over, but maybe there's a way to make it work well.)

Should this behavior be changed and custom ports be exempt from upgrades? Configurations which use the same port for HTTP and HTTPS seem rare (I don't have any data). In some cases this does cause bugs like the one linked above: aio-libs/aiohttp#8065 & python/cpython#109765

[mozfreddyb]

@meacer Are you still working on this? Have Chrome's plans for this changed somehow?

[meacer]

@meacer Are you still working on this? Have Chrome's plans for this changed somehow?

Apologies for my late reply. Chrome's plans for HTTPS Upgrades haven't changed. In fact HTTPS Upgrades are now enabled by default in Chrome, starting in Chrome 115. Therefore, this spec change is still a priority for us.

Regarding redirect loop detection: I don't think it's possible to break out of loops between https upgrades and http fallbacks without any sort of detection logic. The https://www.bom.gov.au/ example above is a good one where this is needed. Given that most browsers will need this, I agree it makes sense to add it to the spec.

[simon-friedberger]

It looks like there is a logic error in the fallback handling. This is inserted before the HSTS and HTTPS RR handling so a request will have its "is HTTPS upgrade" flag set and a downgrade will be attempted even if HSTS or HTTPS RR would have enforced HTTPS anyway.

[Dreamsorcerer]

Configurations which use the same port for HTTP and HTTPS seem rare (I don't have any data).

I would point out that it's actually quite complex to setup a server that supports this in most software (see for example: https://serverfault.com/questions/47876/handling-http-and-https-requests-using-a-single-port-with-nginx/). So, I'd agree this seems unlikely.

[meacer]

Hello,

I made a few changes to this PR:

1. Added an example for custom ports
2. Added an example for redirect loops
3. Moved the upgrade step after HSTS upgrades to avoid setting a fallback URL when an HSTS upgrade is done

(3) also makes the flow more in-line with the HTTPS Upgrades implementation in Chrome. Chrome does HTTPS upgrading before HSTS, but first checks if the URL would be upgraded by HSTS. If yes, it doesn't continue with the upgrade and lets HSTS handle the navigation.

This latest change should handle all of the concerns raised in the comment thread before. Please let me know if the latest diff looks good, thanks.

[simon-friedberger]

@meacer If I am reading this correctly you are saying "don't upgrade custom ports, but we're not specifying it". Why not specify it? ISTM that there is too much going on under is exempted from upgrades in an [implementation-defined](...) way.

[meacer]

@meacer If I am reading this correctly you are saying "don't upgrade custom ports, but we're not specifying it". Why not specify it?

That's fair. I think all implementations are going to have this exception, so I explicitly specify it now.

[simon-friedberger]

Afaict "HTTPS Upgrade" is the same as "HTTPS upgrade fallback URL" is non-null. Is this redundancy intentional? Algorithm 2.11.1 checks the latter but 2.11.2 checks the former which seems odd. Am I missing something?

[simon-friedberger]

I already filed an issue for redirect behavior a while ago: #1738

Only an example mentions it and it is fairly trivial:
http://a.com -> https://a.com -> http://a.com

There are plenty of common but more complicated cases like
http://a.com -> https://a.com -> https://www.a.com -> http://www.a.com

Chrome has implemented a quite thorough redirect URL list to detect arbitrarily complicated loops.
Anne suggested that this should simply be covered by the 20 redirect limit.

Some redirect detection should be standardized. The fact that we have WPTs for it is a good indicator that we want to align on the same behavior here.