HTTPS upgrades proposal #1655
HTTPS upgrades proposal #1655
Conversation
|
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. |
|
I'm not sure why the CI build is failing -- the bikeshed error |
|
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: Friendly ping |
|
Pinging again -- let us know if there are any remaining concerns. |
There was a problem hiding this comment.
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
force-pushed
the
httpsupgrades
branch
from
68081a0
to
c5eefe7
Compare
|
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. |
meacer
force-pushed
the
httpsupgrades
branch
from
c5eefe7
to
36258fd
Compare
|
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. |
|
@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
force-pushed
the
httpsupgrades
branch
2 times, most recently
from
54a3d49
to
74247e3
Compare
|
@annevk I believe my last commit addresses all your comments. Could you please take a look when you have a chance? Thanks. |
meacer
force-pushed
the
httpsupgrades
branch
from
fa21599
to
82cd088
Compare
Yes. The browser navigates to Frankly, it's not immediately obvious to me where redirects are in Fetch (is this the "http network fetch"?) and if this is implicitly taken care of in this pull request by setting and persisting the upgrade flag at a a higher layer. Regardless of the answer to my question, I want to emphasize that there should be tests. |
There was a problem hiding this comment.
@mozfreddyb that would result in an upgrade. Once we see a redirect response we go back into main fetch. That also applies to navigation, although that is a bit more involved.
@meacer are there tests for that scenario?
meacer
force-pushed
the
httpsupgrades
branch
2 times, most recently
from
2b475bf
to
7d00e74
Compare
|
Thank you for the reviews! I also updated the commit message.
No, but I can add a WPT or two. Example scenarios:
|
|
I pushed some nits. Once the tests are written I think this is ready to be merged, assuming you're okay with the commit I just pushed. |
|
Quick update: Redirect tests have landed: https://github.com/web-platform-tests/wpt/tree/master/https-upgrades/tentative I haven't had a chance to look into the main fetch bits yet though. |
|
What's the status of this pull request? |
|
The latest status is that we wanted WPTs for the interaction of upgrades and referrer interaction. I landed the tests at web-platform-tests/wpt#43676. I also opened #1727 for a broader discussion, but I don't think it should be a blocker for this PR. @annevk I think this is ready for another round of review, wdyt? |
HTTPS Upgrading automatically and optimistically upgrades all HTTP navigations to HTTPS, with a fallback to HTTP. If an upgraded navigation fails, the user agent will fallback to the original HTTP navigation. Tests: web-platform-tests/wpt/tree/HEAD/https-upgrades
|
Text still looks good. Here is what remains in my view:
|
| <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">scheme</a> is not | ||
| "<code>http</code>"; or | ||
|
|
||
| <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">origin</a> is exempted from |
There was a problem hiding this comment.
So why isn't the HTTPS Upgrade opt-out mechanism defined in this spec or uselessly vauge? What if a site serves 2 different views or groups of assets, for clear vs TLS?
There was a problem hiding this comment.
Generally that's considered a bug in the website at this point.
There was a problem hiding this comment.
In https://groups.google.com/a/chromium.org/g/blink-dev/c/cAS525en8XE/m/jAAG2CIPBgAJ a user reported a Polish LTE ISP, offers the UA subscriber's ISP specific GUID (lets ignore privacy) through a clear HTTP 1.1 ISP addon req header (MITM clear proxy), so a site has to downgrade to clear, capture the customer's ISP subscriber GUID, then redirect back to HTTPS, then on backend, submit LTE subscriber GUID+server's 3rd party api key->bill UA's LTE account for 3rd party service. No app stores, no native apps, no browser specific local or cloud storage of UA auto-logins, and no asking a UA to type in his telephone number. How would this continue to work if a clear HTTP is prohibited by spec globally? Someone demonstrated a business need to capture req headers from a clear MITM proxy.
There was a problem hiding this comment.
ISPs broadcasting user identifiers in plain text is exactly why universal HTTPS matters.
|
I think this should specify what happens for custom ports. I believe, ports which work for both HTTP and HTTPS are very rare, even though nginx apparently supports it now. My view is that as long as the fallback works attempting to upgrade anyway is fine but I think it would be worth spelling out to get consistent behavior. In case it's not obvious, the issue is that port 80 gets upgraded to 443 but if there is a different port there is no known mapping. When trying to access |
|
@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. |
|
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.) |
|
The explainer touched on loop handling behavior:
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? |
|
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. |
|
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 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 |
|
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. |
|
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. |
|
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! |
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 |
(See WHATWG Working Mode: Changes for more details.)
💥 Error: 405 Method Not Allowed 💥
PR Preview failed to build. (Last tried on Feb 1, 2024, 11:31 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.