1747230 - In HTTPS-Only mode, redirect loop is too strict, resulting on NS_ERROR_REDIRECT_LOOP on valid redirects.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Kacper Michajłow [:kasper93]]

On sites where some redirects point back to http address, Firefox will tell that "Secure Site Not Available" with "NS_ERROR_REDIRECT_LOOP".

Looks like the detection is too strict and when there is legitimate redirect it will fail, probably thinking that there is loop redirecting back to http...

Arguably this is misconfigured forum that uses http, while https is fully supported, but this is the exact job for HTTPS-Only mode.

Example:
https://forum.metpage.org/index.php?act=search&CODE=getnew
It does 302 redirect to
http://forum.metpage.org/index.php?act=Search&nav=lv&CODE=show&searchid=<someid>&search_in=topics&result_type=topics&lastdate=0

And at this point Firefox bails-out with NS_ERROR_REDIRECT_LOOP giving the option to "Continue to HTTP Site", while the proper solution is to manually change http to https in address bar and carry on.

I suspect this is because there is a check to prevent loop from downgrading/upgrading in loop, but I think it should try twice, not once as it does now. Because as we can see it mistakes normal redirect with downgrade, while it should be upgraded by HTTPS-Only mode. So it probably should check if it get twice the same redirect to error out with loop.

Comment 1

[fantasyfate]

If dom.security.https_only_mode_break_upgrade_downgrade_endless_loop is set to false, this issue will be worked around, and maximum number of redirection depends on network.http.redirection-limit. However, we still need a real fix.

Comment 2

[Manuel Bucher [:manuel]]

Fixing this is harder than I initially though, because we don't keep the query string in the redirect chain since Bug 1715785.

Passing the necessary info down here should work:

• https://searchfox.org/mozilla-central/rev/48b6992e03fa66f77ac9688ba61c95d31a451bc1/netwerk/protocol/http/nsHttpChannel.cpp#5141
• https://searchfox.org/mozilla-central/rev/48b6992e03fa66f77ac9688ba61c95d31a451bc1/netwerk/protocol/http/HttpBaseChannel.cpp#5895

We can only compare the last redirect, rather than the full redirect chain. The patch also needs to consider that we are getting called for HTTPS-RR upgrade mode. Some https-only/-first tests need to be updated, since they depend on this behavior.

Note for self: Recorded a pernosco session (with my not yet ready fix) https://pernos.co/debug/uatOyORk3GuG-6V3zlknPA/index.html To debug: Breaking on nsHTTPSOnlyUtils::IsUpgradeDowngradeEndlessLoop and print the url via print aURI->GetSpecOrDefault().get()

Comment 3

[Manuel Bucher [:manuel]]

Attachment: Bug 1747230 - Fix IsUpgradeDowngradeEndlessLoop blocking legitimate redirects when redirecting to different query parameters r=#necko

This changes where the IsUpgradeDowngradeEndlessLoop check triggers.
Before this patch, it triggered during the redirect caused by the https
upgrade. With this patch, it triggers during the downgrade redirect to
the same url, except with the scheme http instead of https.

Different query parameters normally lead to different responses by web servers.
Don't consider the '#ref' part of the uri, because it doesn't get send to
the server and therefore can't change the server response.

We can't use the redirect chain anymore, because the query parameters
are trimmed since Bug 1715785.

Comment 4

[[:estemal]]

So, what's the holdup with this?

Comment 5

[Manuel Bucher [:manuel]]

Waiting on review. There was some discussion about removing the pref dom.security.https_only_check_path_upgrade_downgrade_endless_loop, because we might want that behavior in HTTPS-first mode. And HTTPS-first mode is also affected by the pref.

@Freddy: Do you think that patch could land if I add another one with a pref dom.security.https_first_redirect_downgrade_same_origin to only look at the host part when redirect happen in HTTP-first mode? And allow servers to downgrade to the HTTP version if they have a same origin redirect?

For example, when we get redirected from https://example.com to http://example.com/other_path that we load the HTTP version instead of the HTTPS version with the pref enabled?

In my understanding, that is the holdup of the patch landing. Do I understand the concerns correctly, or are there other holdups?

Comment 6

[[:estemal]]

I applied the patch locally and got the "HTTPS-Only Mode Alert//Secure Site Not Available" error instead. The site is definitely available by HTTPS and the URL in question is available too. Refreshing the page just gets me there without adding an exception. Shouldn't it just silently get there in such cases without complaining?

Comment 7

[Manuel Bucher [:manuel]]

Hi, thanks for testing out the patch. Which website did you test against?

Comment 8

[[:estemal]]

A small website, that had a bug that caused it to redirect from HTTPS to HTTP (with 302 response). The bug has been fixed since, so I won't be able to test this again.

Comment 9

[Manuel Bucher [:manuel]]

Attachment: Bug 1747230 - Add Pref to not upgrade same origin redirects in HTTPS-First mode r=freddyb

This reintroduces a pref that affected both HTTPS-First and HTTPS-Only
mode. In HTTPS-Only mode this pref doesn't make sense and got removed.
To keep allowing this behavior in HTTPS-First mode this patch introduces
it again.

Website breakages for websites misconfigured like in Bug 1725646. If
those are more common, we might want to flip the pref.

Or the Pref could be removed in the future if we don't find use for it.

Comment 10

[Kershaw Chang [:kershaw]]

Talked to Manuel about this.
I think we can move this to DOM: Security, since the change in this patch is mostly in dom/security/nsHTTPSOnlyUtils.cpp.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b234ba179483
Fix IsUpgradeDowngradeEndlessLoop blocking legitimate redirects when redirecting to different query parameters r=necko-reviewers,kershaw,simonf,maltejur

Comment 12

[tszentpeteri]

Backed out for causing mochitest failures on browser_target_blank.js

• Backout link
• Push with failures
• Failure Log:

[task 2024-06-06T16:13:17.174Z] 16:13:17     INFO - TEST-START | devtools/client/responsive/test/browser/browser_target_blank.js
[task 2024-06-06T16:13:17.604Z] 16:13:17     INFO - Entering test bound 
[task 2024-06-06T16:13:17.606Z] 16:13:17     INFO - Adding a new tab with URL: data:text/html,<a%20href="http://example.com/"%20target="_blank">Click%20me</a>
[task 2024-06-06T16:13:17.616Z] 16:13:17     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 7fa3a5cd8000 == 2 [pid = 3774] [id = 25]
[task 2024-06-06T16:13:17.616Z] 16:13:17     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 3 (7fa3a5c6a3e0) [pid = 3774] [serial = 59] [outer = 0]
[task 2024-06-06T16:13:17.618Z] 16:13:17     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 4 (7fa3a5cd8400) [pid = 3774] [serial = 60] [outer = 7fa3a5c6a3e0]
[task 2024-06-06T16:13:17.698Z] 16:13:17     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 5 (7fa3a5cd9400) [pid = 3774] [serial = 61] [outer = 7fa3a5c6a3e0]
[task 2024-06-06T16:13:17.770Z] 16:13:17     INFO - Console message: [JavaScript Warning: "This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”." {file: "data:text/html,<a%20href="http://example.com/"%20target="_blank">Click%20me</a>" line: 0}]
[task 2024-06-06T16:13:17.949Z] 16:13:17     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7fbfa20a3000 == 6 [pid = 3670] [id = 65] [url = about:robots]
[task 2024-06-06T16:13:17.997Z] 16:13:17     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 26 (7fbfbd9b1400) [pid = 3670] [serial = 136] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:17.999Z] 16:13:17     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 25 (7fbfa20c2000) [pid = 3670] [serial = 122] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:18.000Z] 16:13:17     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 24 (7fbfa20c3800) [pid = 3670] [serial = 126] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:18.001Z] 16:13:17     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 23 (7fbfa546ac00) [pid = 3670] [serial = 124] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:18.001Z] 16:13:17     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 22 (7fbfa55f3000) [pid = 3670] [serial = 128] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:18.002Z] 16:13:17     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 21 (7fbfa52ae800) [pid = 3670] [serial = 120] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:18.003Z] 16:13:18     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 20 (7fbfa55f1000) [pid = 3670] [serial = 130] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:18.003Z] 16:13:18     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 19 (7fbfa20ac400) [pid = 3670] [serial = 132] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:18.012Z] 16:13:18     INFO - Tab added and finished loading
[task 2024-06-06T16:13:18.014Z] 16:13:18     INFO - Opening responsive design mode
[task 2024-06-06T16:13:18.018Z] 16:13:18     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 7fbfa209f000 == 7 [pid = 3670] [id = 68]
[task 2024-06-06T16:13:18.019Z] 16:13:18     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 20 (7fbfbd9f1980) [pid = 3670] [serial = 143] [outer = 0]
[task 2024-06-06T16:13:18.020Z] 16:13:18     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 21 (7fbfa20a7c00) [pid = 3670] [serial = 144] [outer = 7fbfbd9f1980]
[task 2024-06-06T16:13:18.354Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "CHANGE_DISPLAY_PIXEL_RATIO"
[task 2024-06-06T16:13:18.359Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "ADD_VIEWPORT"
[task 2024-06-06T16:13:18.502Z] 16:13:18     INFO - Responsive design mode opened
[task 2024-06-06T16:13:18.521Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "LOAD_DEVICE_LIST_START"
[task 2024-06-06T16:13:18.537Z] 16:13:18     INFO - Responsive design initialized
[task 2024-06-06T16:13:18.538Z] 16:13:18     INFO - Waiting for state predicate "state => state.viewports.length == 1"
[task 2024-06-06T16:13:18.540Z] 16:13:18     INFO - Found state predicate "state => state.viewports.length == 1"
[task 2024-06-06T16:13:18.568Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "ADD_DEVICE_TYPE"
[task 2024-06-06T16:13:18.586Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "ADD_DEVICE"
[task 2024-06-06T16:13:18.606Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "ADD_DEVICE"
[task 2024-06-06T16:13:18.614Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "ADD_DEVICE"
[task 2024-06-06T16:13:18.627Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "ADD_DEVICE"
[task 2024-06-06T16:13:18.637Z] 16:13:18     INFO - GECKO(3670) | console.log: "[DISPATCH] action type:" "ADD_DEVICE"
<...>
[task 2024-06-06T16:13:24.161Z] 16:13:24     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 14 (7fbfcf4ee800) [pid = 3670] [serial = 142] [outer = 0] [url = about:robots]
[task 2024-06-06T16:13:24.625Z] 16:13:24     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 13 (7fbfbd9f13e0) [pid = 3670] [serial = 138] [outer = 0] [url = chrome://devtools/content/responsive/toolbar.xhtml]
[task 2024-06-06T16:13:25.213Z] 16:13:25     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7fa3a5cd6800 == 2 [pid = 3774] [id = 24] [url = about:blank]
[task 2024-06-06T16:13:25.214Z] 16:13:25     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7fa3a5cdc000 == 1 [pid = 3774] [id = 26] [url = about:blank]
[task 2024-06-06T16:13:25.257Z] 16:13:25     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 7 (7fa3a5c6a200) [pid = 3774] [serial = 57] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:25.258Z] 16:13:25     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 6 (7fa3a5c6a7a0) [pid = 3774] [serial = 62] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:28.772Z] 16:13:28     INFO - GECKO(3670) | [Parent 3670: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 12 (7fbfa20ac000) [pid = 3670] [serial = 139] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:29.353Z] 16:13:29     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 5 (7fa3a5cdc800) [pid = 3774] [serial = 64] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:29.354Z] 16:13:29     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 4 (7fa3a5cdc400) [pid = 3774] [serial = 63] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:29.355Z] 16:13:29     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 3 (7fa3a5cd8400) [pid = 3774] [serial = 60] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:29.355Z] 16:13:29     INFO - GECKO(3670) | [Child 3774: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 2 (7fa3a5cd7000) [pid = 3774] [serial = 58] [outer = 0] [url = about:blank]
[task 2024-06-06T16:13:32.294Z] 16:13:32     INFO - GECKO(3670) | [Child 5410: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 2 (7f51db57a400) [pid = 5410] [serial = 2] [outer = 0] [url = about:blank]
[task 2024-06-06T16:14:51.995Z] 16:14:51     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2024-06-06T16:16:25.002Z] 16:16:25     INFO - TEST-INFO | started process screentopng
[task 2024-06-06T16:16:25.363Z] 16:16:25     INFO - TEST-INFO | screentopng: exit 0
[task 2024-06-06T16:16:25.363Z] 16:16:25     INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive/test/browser/browser_target_blank.js | Test timed out - 
[task 2024-06-06T16:16:25.369Z] 16:16:25     INFO - GECKO(3670) | Completed ShutdownLeaks collections in process 3670
[task 2024-06-06T16:16:25.369Z] 16:16:25     INFO - TEST-START | Shutdown

Comment 13

[Manuel Bucher [:manuel]]

It times out even before my commit with http3. Added skip-if = ["http3"] to the test case now.

./mach mochitest --use-http3-server devtools/client/responsive/test/browser/browser_target_blank.js --headless

Comment 14

[Manuel Bucher [:manuel]]

It also times out with http2. Disabling the test there too.

Not sure why it didn't(?) fail for the commit before mine. Maybe it didn't run with http3 there.

./mach mochitest --use-http2-server devtools/client/responsive/test/browser/browser_target_blank.js --headless also times out.

Landing again with these unrelated test failures resolved by disabling them.

Comment 15

[Manuel Bucher [:manuel]]

Moving devtools part to Bug 1901210 due to requiring new review.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ced283567ab
Fix IsUpgradeDowngradeEndlessLoop blocking legitimate redirects when redirecting to different query parameters r=necko-reviewers,kershaw,simonf,maltejur

Comment 18

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/5ced283567ab

Comment 20

[Phabricator Automation]

Comment on attachment 9380825 [details]
Bug 1747230 - Add Pref to not upgrade same origin redirects in HTTPS-First mode r=freddyb

Revision D202147 was moved to bug 1901725. Setting attachment 9380825 [details] to obsolete.

Comment 22

[Manuel Bucher [:manuel]]

Attachment: Bug 1747230 - Fix IsUpgradeDowngradeEndlessLoop blocking legitimate redirects when redirecting to different query parameters r=#necko

This changes where the IsUpgradeDowngradeEndlessLoop check triggers.
Before this patch, it triggered during the redirect caused by the https
upgrade. With this patch, it triggers during the downgrade for http
redirects. META and JS redirect are still detected during upgrade.
This should be fixed as a follow up (See Bug 1896691).
Downgrade in this context means same url, except with the scheme http
instead of https.

Different query parameters normally lead to different responses by web servers.
Don't consider the '#ref' part of the uri, because it doesn't get send to
the server and therefore can't change the server response.

We can't use the redirect chain anymore, because the query parameters
are trimmed since Bug 1715785.

This also removes the config option dom.security.https_only_check_path_upgrade_downgrade_endless_loop,
because it adds unnecessary complexity. Removing it for this patch is
easier.

https-only, https-first and httpssvc_https_upgrade tests had to be
modified, because they depended on the incorrect handling of query
strings in loop detection.

Original Revision: https://phabricator.services.mozilla.com/D193672

Comment 23

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Webcompat bugs in https-first mode enabled by default in private browsing mode. (in esr for up to one year). Would request uplift to 128 esr in a few releases if deemed to risky for beta due to already being mid-cycle. Prefering beta-uplift to not have different behaviors in one esr release.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: -
• Risk associated with taking this patch: low
• Explanation of risk level: very good test coverage. Already tested in Nightly. Only one tier-2 regression bug, engineering considers non-blocking for beta uplift.
• String changes made/needed: -
• Is Android affected?: yes

Comment 24

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Webcompat bugs in https-first mode enabled by default in private browsing mode. (in esr for up to one year). Would request uplift to 128 esr in a few releases if deemed to risky for beta due to already being mid-cycle. Prefering beta-uplift to not have different behaviors in one esr release.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: -
• Risk associated with taking this patch: low
• Explanation of risk level: very good test coverage. Already tested in Nightly. Only one tier-2 regression bug (Bug 1901623), engineering considers non-blocking for beta uplift.
• String changes made/needed: -
• Is Android affected?: yes

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/c36ce41bc12f