1855734 - The HTTPS Only error page repeats forever in view-source:http://IP-addresses when privacy.firstparty.isolate is enabled

Status: VERIFIED FIXED

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

Description

[Pier Angelo Vendrame]

Bug 1647829 solved an issue for which the HTTPS Only error page was shown in loop with privacy.firstparty.isolate enabled.
But that condition still happens with view-source:http://IP-address.

Steps to reproduce:

1. Enable privacy.firstparty.isolate and HTTPS Only
2. Open view-source:http://128.31.0.34/ in the URL bar
3. Get the HTTPS Only error
4. Click on Continue to HTTP Site

Comment 1

[Daniel Veditz [:dveditz]]

Very little of this code seems to deal with nested nsIURIs, of which "view-source:" would be the most relevant example. We still use "jar:" internally but don't allow it for remote content, but it looks like some of the https-only code does still have to deal with it (internal chrome pages, I guess). It looks like "wyciwyg:" is gone), but in the past that was used to reference pages from the bfcache ("What You Cache Is What You Get") -- dunno what we use now.

The code for saving the exception patched in bug 1647829 already dealt with view-source:, and that patch only had to add the first-party-domain attribute.

bug 1

The Site Identity panel code in browser/base/content/browser-siteIdentity.js doesn't seem to deal with it correctly. Stripping view-source off the URL will work, but means every kind of nested URL has to be dealt with individually and indeed, this code also has a special-case "jar" section (that won't handle nested jar files, which can happen!)
https://searchfox.org/mozilla-central/rev/37d9d6f0a77147292a87ab2d7f5906a62644f455/browser/base/content/browser-siteIdentity.js#1125-1127,1155-1166

Instead it should have done something like this up at the top of setURI()

    if (uri instanceof Ci.nsINestedURI)
      uri = uri.QueryInterface(Ci.nsINestedURI).innerMostURI;
    }

Fixing the above won't fix this loop, but it's related.

bug 2

openWebsiteInsecure() in AboutHttpsOnlyErrorParent.jsm (the file patched by bug 1647829) isn't quite right, either. It's good it's using innerURI instead of syntactically rewriting the URL, but it shouldn't special-case "view-source". Instead check if it's a nsINestedURI, and if it is grab .innermostURI, not just .innerURI. Wasn't going to mention this, but seeing that browser-siteIdentity.js does have to deal with jar: URIs then maybe we do have to make this code right.

This might be the reporter's bug

origin attributes without explicit first party setting is done here:
https://searchfox.org/mozilla-central/rev/37d9d6f0a77147292a87ab2d7f5906a62644f455/dom/ipc/WindowGlobalParent.cpp#1342,1354-1357

I'm guessing a similar problem to the original "about:" bug, but in this case it's getting a "view-source:" first-party from the original principal. But code that turns a URI into an origin (e.g. for same-origin checks) should already be de-nesting these. Maybe that code is broken, too?

Comment 2

[Daniel Veditz [:dveditz]]

Fixing the above won't fix this loop, but it's related.

To clarify the ambiguous pronouns, I meant fixing the browser-siteIdentity.js problems won't fix the view-source: loop described in comment 0

I'm guessing a similar problem to the original "about:" bug [...]

I stress guessing. It would be easy to prove or disprove in a debugger, which I didn't check in (what was supposed to be) a quick "triage" pass. Start looking here, but I could be wrong.

Comment 3

[Malte Jürgens [:maltejur]]

As far as I can tell, the code linked in WindowGlobalParent::RecvReloadWithHttpsOnlyException is doing its job correctly. At least when I visit the exceptions UI in the settings, I see that a temporary exception for http://128.31.0.34^firstPartyDomain=128.31.0.34 is correctly added. So the place where the problem lies is probably rather the code that is checking if a exception permission for the current principal is set, so nsHTTPSOnlyUtils::TestSitePermissionAndPotentiallyAddExemption. That function is getting the principal here. I'll try to figure out what goes wrong there.

Comment 4

[Malte Jürgens [:maltejur]]

Attachment: Bug 1855734 - Use innermost nested URI in `PopulateTopLevelInfoFromURI` r?freddyb

Comment 5

[Tom Ritter [:tjr] (OOTO until mid-August)]

Bug 1758558 has some digging into Nest URIs and some potential bugs lurking in the codebase.

Comment 6

[Malte Jürgens [:maltejur]]

(In reply to Tom Ritter [:tjr] from comment #5)

Bug 1758558 has some digging into Nest URIs and some potential bugs lurking in the codebase.

Can you CC me on that bug? I currently don't have access to it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54dd41cecdd0
Use innermost nested URI in `PopulateTopLevelInfoFromURI` r=freddyb,timhuang

Comment 8

[Iulian Moraru]

Backed out for causing bc failures on browser_fpi_nested_uri.js.

• Push with failures

• Failure log

• Backout link

[task 2023-11-06T10:32:45.490Z] 10:32:45     INFO - TEST-START | toolkit/components/httpsonlyerror/tests/browser/browser_fpi_nested_uri.js
[task 2023-11-06T10:32:45.619Z] 10:32:45     INFO - GECKO(3434) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpgbhdjkuv.mozrunner/runtests_leaks_tab_pid4172.log
[task 2023-11-06T10:32:45.647Z] 10:32:45     INFO - GECKO(3434) | [Child 4172, Main Thread] WARNING: could not set real-time limit in CubebUtils::InitLibrary: file /builds/worker/checkouts/gecko/dom/media/CubebUtils.cpp:657
[task 2023-11-06T10:32:45.882Z] 10:32:45     INFO - GECKO(3434) | [WARN  rkv::backend::impl_safe::environment] `load_ratio()` is irrelevant for this storage backend.
[task 2023-11-06T10:32:45.906Z] 10:32:45     INFO - GECKO(3434) | console.error: (new Error("Polling for changes failed: Unexpected content-type \"text/plain;charset=US-ASCII\".", "resource://services-settings/remote-settings.sys.mjs", 321))
[task 2023-11-06T10:32:46.039Z] 10:32:46     INFO - GECKO(3434) | [Child 3532: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 2 (7f75ed232400) [pid = 3532] [serial = 6] [outer = 0] [url = about:blank]
[task 2023-11-06T10:32:46.040Z] 10:32:46     INFO - GECKO(3434) | [Child 3532: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7f75ed22a000 == 0 [pid = 3532] [id = 2] [url = about:blank]
[task 2023-11-06T10:32:48.294Z] 10:32:48     INFO - GECKO(3434) | [Child 3631: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7fb9e446a800 == 2 [pid = 3631] [id = 3] [url = about:privatebrowsing]
[task 2023-11-06T10:32:48.295Z] 10:32:48     INFO - GECKO(3434) | [Child 3631: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7fb9e454ac00 == 1 [pid = 3631] [id = 2] [url = about:privatebrowsing]
[task 2023-11-06T10:32:48.391Z] 10:32:48     INFO - GECKO(3434) | [Child 3631: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 7 (7fb9e44f53e0) [pid = 3631] [serial = 10] [outer = 0] [url = about:privatebrowsing]
[task 2023-11-06T10:32:48.392Z] 10:32:48     INFO - GECKO(3434) | [Child 3631: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 6 (7fb9eb6042e0) [pid = 3631] [serial = 7] [outer = 0] [url = about:privatebrowsing]
[task 2023-11-06T10:32:48.600Z] 10:32:48     INFO - GECKO(3434) | [Parent 3434: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7fb3202e6400 == 5 [pid = 3434] [id = 5] [url = chrome://browser/content/browser.xhtml]
[task 2023-11-06T10:32:48.918Z] 10:32:48     INFO - GECKO(3434) | [Parent 3434, Main Thread] WARNING: '!top', file /builds/worker/checkouts/gecko/dom/xul/MenuBarListener.cpp:99
[task 2023-11-06T10:32:48.931Z] 10:32:48     INFO - GECKO(3434) | [Parent 3434: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 13 (7fb324486b60) [pid = 3434] [serial = 13] [outer = 0] [url = chrome://browser/content/browser.xhtml]
[task 2023-11-06T10:32:48.931Z] 10:32:48     INFO - GECKO(3434) | [Parent 3434: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 12 (7fb332dc9f20) [pid = 3434] [serial = 15] [outer = 0] [url = about:blank]
[task 2023-11-06T10:32:50.148Z] 10:32:50     INFO - GECKO(3434) | [Child 3532: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 1 (7f75ec62a200) [pid = 3532] [serial = 7] [outer = 0] [url = about:blank]
[task 2023-11-06T10:32:52.672Z] 10:32:52     INFO - GECKO(3434) | [Child 3631: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 5 (7fb9e454b000) [pid = 3631] [serial = 8] [outer = 0] [url = about:blank]
[task 2023-11-06T10:32:52.672Z] 10:32:52     INFO - GECKO(3434) | [Child 3631: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 4 (7fb9e454c400) [pid = 3631] [serial = 9] [outer = 0] [url = about:privatebrowsing]
[task 2023-11-06T10:32:52.673Z] 10:32:52     INFO - GECKO(3434) | [Child 3631: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 3 (7fb9e446bc00) [pid = 3631] [serial = 12] [outer = 0] [url = about:privatebrowsing]
[task 2023-11-06T10:32:52.674Z] 10:32:52     INFO - GECKO(3434) | [Child 3631: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 2 (7fb9e446ac00) [pid = 3631] [serial = 11] [outer = 0] [url = about:blank]
[task 2023-11-06T10:32:52.913Z] 10:32:52     INFO - GECKO(3434) | [Parent 3434: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 11 (7fb326463000) [pid = 3434] [serial = 16] [outer = 0] [url = about:blank]
[task 2023-11-06T10:32:52.913Z] 10:32:52     INFO - GECKO(3434) | [Parent 3434: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 10 (7fb32034f800) [pid = 3434] [serial = 14] [outer = 0] [url = about:blank]
[task 2023-11-06T10:32:54.228Z] 10:32:54     INFO - GECKO(3434) | [Child 3532: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 0 (7f75ed22b400) [pid = 3532] [serial = 8] [outer = 0] [url = about:blank]
[task 2023-11-06T10:34:15.901Z] 10:34:15     INFO - TEST-INFO | started process screentopng
[task 2023-11-06T10:34:16.243Z] 10:34:16     INFO - TEST-INFO | screentopng: exit 0
[task 2023-11-06T10:34:16.245Z] 10:34:16     INFO - Buffered messages logged at 10:32:45
[task 2023-11-06T10:34:16.246Z] 10:34:16     INFO - Entering setup bound 
[task 2023-11-06T10:34:16.247Z] 10:34:16     INFO - Leaving setup bound 
[task 2023-11-06T10:34:16.247Z] 10:34:16     INFO - Entering test bound 
[task 2023-11-06T10:34:16.249Z] 10:34:16     INFO - Console message: [JavaScript Warning: "HTTPS-Only Mode: Upgrading insecure request “http://192.168.0.0/” to use “https”." {file: "http://192.168.0.0/" line: 0}]
[task 2023-11-06T10:34:16.249Z] 10:34:16     INFO - Buffered messages finished
[task 2023-11-06T10:34:16.250Z] 10:34:16     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/httpsonlyerror/tests/browser/browser_fpi_nested_uri.js | Test timed out - 
[task 2023-11-06T10:34:16.252Z] 10:34:16     INFO - GECKO(3434) | [Child 4128: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 7f8c8ac34400 == 1 [pid = 4128] [id = 0]
[task 2023-11-06T10:34:16.253Z] 10:34:16     INFO - GECKO(3434) | [Child 4128: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (7f8c8b6b5020) [pid = 4128] [serial = 1] [outer = 0]
[task 2023-11-06T10:34:16.253Z] 10:34:16     INFO - GECKO(3434) | [Child 4128: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (7f8c8ac34c00) [pid = 4128] [serial = 2] [outer = 7f8c8b6b5020]
[task 2023-11-06T10:34:16.254Z] 10:34:16     INFO - GECKO(3434) | [Parent 3434, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111 (NS_ERROR_NOT_AVAILABLE): file /builds/worker/checkouts/gecko/caps/BasePrincipal.cpp:1535
[task 2023-11-06T10:34:16.255Z] 10:34:16     INFO - GECKO(3434) | [Parent 3434, Main Thread] WARNING: Failed to preload local storage!: file /builds/worker/checkouts/gecko/dom/ipc/ContentParent.cpp:6435
[task 2023-11-06T10:34:16.256Z] 10:34:16     INFO - Console message: [JavaScript Error: "HTTPS-Only Mode: Upgrading insecure request “https://192.168.0.0/” failed. (M6-C14)" {file: "https://192.168.0.0/" line: 0}]
[task 2023-11-06T10:34:16.257Z] 10:34:16     INFO - GECKO(3434) | [Child 4128: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 3 (7f8c8ac37400) [pid = 4128] [serial = 3] [outer = 7f8c8b6b5020]
[task 2023-11-06T10:34:16.257Z] 10:34:16     INFO - GECKO(3434) | [Child 4128, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, NS_OK) failed with result 0x80004005 (NS_ERROR_FAILURE): file /builds/worker/checkouts/gecko/caps/BasePrincipal.cpp:1150
[task 2023-11-06T10:34:16.258Z] 10:34:16     INFO - GECKO(3434) | JavaScript error: resource://gre/modules/XULStore.sys.mjs, line 60: Error: Can't find profile directory.
[task 2023-11-06T10:34:16.258Z] 10:34:16     INFO - GECKO(3434) | [Child 4128, Main Thread] WARNING: '!mLocalStore', file /builds/worker/checkouts/gecko/dom/xul/XULPersist.cpp:146
[task 2023-11-06T10:34:16.275Z] 10:34:16     INFO - Console message: [JavaScript Error: "Error: Can't find profile directory." {file: "resource://gre/modules/XULStore.sys.mjs" line: 60}]
[task 2023-11-06T10:34:16.276Z] 10:34:16     INFO - load@resource://gre/modules/XULStore.sys.mjs:60:15
[task 2023-11-06T10:34:16.277Z] 10:34:16     INFO - XULStore@resource://gre/modules/XULStore.sys.mjs:17:10
[task 2023-11-06T10:34:16.277Z] 10:34:16     INFO - 
[task 2023-11-06T10:34:16.597Z] 10:34:16     INFO - GECKO(3434) | MEMORY STAT | vsize 3193MB | residentFast 397MB | heapAllocated 215MB

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/544dcbd8979a
Use innermost nested URI in `PopulateTopLevelInfoFromURI` r=freddyb,timhuang

Comment 10

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/544dcbd8979a

Comment 11

[Pier Angelo Vendrame]

Thanks for the fix!
Would it be possible to uplift it to the ESR?

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:mjurgens, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox120 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Malte Jürgens [:maltejur]]

Attachment: Bug 1855734 - Use innermost nested URI in `PopulateTopLevelInfoFromURI`

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

Comment 14

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: no
• Code covered by automated testing: yes
• Risk associated with taking this patch: Low/Medium
• User impact if declined: Tor Browser users and ESR users with privacy.firstparty.isolate enabled will see a infinite error loop when viewing the source code of a page visited by IP-address until the next major ESR release
• Fix verified in Nightly: yes
• Explanation of risk level: This is a simple behavioural change which is limited to a single function, but it does touch relatively low-level code
• Needs manual QE test: no
• String changes made/needed: No
• Steps to reproduce for manual QE testing: https://bugzilla.mozilla.org/show_bug.cgi?id=1855734#c0

Comment 15

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: no
• Code covered by automated testing: yes
• Risk associated with taking this patch: Low/Medium
• User impact if declined: Tor Browser users and ESR users with privacy.firstparty.isolate and HTTPS-Only enabled will see a infinite error loop when viewing the source code of a page visited by IP-address until the next major ESR release
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Explanation of risk level: This is a simple behavioural change which is limited to a single function, but it does touch relatively low-level code
• String changes made/needed: No
• Steps to reproduce for manual QE testing: https://bugzilla.mozilla.org/show_bug.cgi?id=1855734#c0

Comment 16

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9362976 [details]
Bug 1855734 - Use innermost nested URI in PopulateTopLevelInfoFromURI

Approved for 115.6esr.

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/6f850e14e9dc

Comment 18

[Alexandru Trif, Desktop QA [:atrif]]

Hello! Reproduced the issue using STR from comment 0 on Windows 10x64. The HTTPS-only error page is displayed again and again after clicking to continue to the HTTP site.
The issue is verified fixed with Firefox 121.0 and 115.6esr on Windows 10x64, macOS 12 and Ubuntu 22.1. Clicking the Continue to the HTTP site button will now open the page as expected.