Identify URL bar input without a scheme and allow upgrading those to HTTPS with a fallback
Categories
(Core :: DOM: Security, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox120 | --- | fixed |
People
(Reporter: daisuke, Assigned: maltejur)
References
(Blocks 3 open bugs)
Details
(Whiteboard: [domsecurity-active])
Attachments
(6 files, 1 obsolete file)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
When we pass a URL string without the protocol, the function returns URL with http protocol.
https://searchfox.org/mozilla-central/rev/cf3af6bb6657278880f8baf38435eeb8f2d5d86c/docshell/base/URIFixup.sys.mjs#960
However, we'd better use the value of browser.fixup.alternate.protocol
instead of it.
If the pref is empty, perhaps, we'd better use https instead of http.
Comment 1•2 years ago
|
||
We already have a pref for "alternate" uri fixup that is browser.fixup.alternate.protocol
that is used by the URIFixup module to generate, starting from mozilla.com
, something like https://www.mozilla.com/
.
Currently fixupURIProtocol
is hardcoding http://
in https://searchfox.org/mozilla-central/rev/3a13c5ad9c1dbd065e484a399af75eb98c235def/docshell/base/URIFixup.sys.mjs#957,960
The idea is to make it use whatever is defined in browser.fixup.alternate.protocol
(with a fallback to https if the pref is invalid).
Open questions:
- There is a
browser.fixup.fallback-to-https
that is used by the docshell if it tries to load http but that fails. If we change fixup to do https first, this path will be hit less often, and there's no fallback to http. Is this a problem? - URIFixup won't set alternateURI for https, only for http. I think the main reason behind this was privacy/security (changing the domain may inadvertantly send data to a different one). Defaulting to https will make this path hit less often. Is this a problem?
- Overall, while before we were trying http, and fallback to https, this will bring us to directly try https without a fallback, is this a problem?
Comment 2•2 years ago
|
||
Actually,
Freddy and I wanted to introduce something similar.
Maybe our ideas help for the open questions above:
-
We thought also to upgrade in fixupURIProtocol but @Gijs had some second thoughts about it. Since it gets called by more than only the URLbar functions it might lead to problems (@Gijs I hope I "recited" you correctly).
-
To ensure that the fallback to http is guaranteed we thought to introduce a flag in fixupURIProtocol that we would check here and use the HTTPSFirst fallback mechanism for that request.
But since we were unsure if starting in fixupURIProtocol is the correct call and also how to introduce a flag that reaches to MaybeHandleLoadErrorWithURIFixup we did not take further steps.
Comment 3•2 years ago
|
||
There's a point I want to make sure is emphasized clearly: If the address bar is able to transfer and hand down a flag that ends up in a loadState (and eventually loadInfo), we could tie this into the existing mechanisms for fallbacks.
Specifically, "HTTPS First", does two racing requests and falls back to HTTP if the page does not respond timely (already enabled in private browsing mode).
Comment 4•2 years ago
|
||
I think Marco can do a better job of answering the questions here than I can. :-)
Comment 5•2 years ago
•
|
||
(In reply to Tomer Yavor from comment #2)
- We thought also to upgrade in fixupURIProtocol but @Gijs had some second thoughts about it. Since it gets called by more than only the URLbar functions it might lead to problems (@Gijs I hope I "recited" you correctly).
Long term it's the right thing to do, but I concur with the fact UriFixup is used by a lot of consumers, sometimes even inappropriately. The work Gijs is doing in Bug 1810141 and its dependencies would help creating a list of those consumers that still rely on fixup and see how they'd react to this change. Plus it will remove some of the inappropriate uses of it, making potentially easier to migrate.
The main consumer remains the urlbar of course, and my concerns are the ones I expressed in comment 1, related to not having a fallback if https doesn't effectively work for a certain origin.
- To ensure that the fallback to http is guaranteed we thought to introduce a flag in fixupURIProtocol that we would check here and use the HTTPSFirst fallback mechanism for that request.
Is the plan to allow consumers to selectively opt-in to fixup to https, so you can examine one consumer at a time?
Long term we may reach a point where all consumers are working fine with it, and the added argument could be removed.
But since we were unsure if starting in fixupURIProtocol is the correct call
I think so, the alternative would be to either:
- change default but allow the user to revert, that was our initial suggestion to just rely on browser.fixup.alternate.protocol. This of course may break users expectations without a proper fallback
- intercept the load, that I assume is what https-first does, but this will still show http in the urlbar and it's not the ideal long term target
and also how to introduce a flag that reaches to MaybeHandleLoadErrorWithURIFixup we did not take further steps.
(In reply to Frederik Braun [:freddy] from comment #3)
There's a point I want to make sure is emphasized clearly: If the address bar is able to transfer and hand down a flag that ends up in a loadState (and eventually loadInfo), we could tie this into the existing mechanisms for fallbacks.
All the urlbar loads go through a _loadURL method that calls into openTrustedLinkIn and finally to openLinkIn
https://searchfox.org/mozilla-central/rev/9de332d5c8faac58dc1232b8a6383ce6cb1400f4/browser/components/urlbar/UrlbarInput.sys.mjs#2613,2719
https://searchfox.org/mozilla-central/rev/9de332d5c8faac58dc1232b8a6383ce6cb1400f4/browser/base/content/utilityOverlay.js#276
There's already multiple params there.
There it may either open a window or a load in a tab, so you may have to handle both cases.
The browser then goes to nsiWebNavigation.LoadURI
https://searchfox.org/mozilla-central/rev/9de332d5c8faac58dc1232b8a6383ce6cb1400f4/browser/base/content/tabbrowser.js#7059,7061
That ends in a nsDocShellLoadState::CreateFromLoadURIOptions, so there should be no problem adding additional flags to reach it.
Comment 6•2 years ago
|
||
Updated•1 year ago
|
Comment 7•1 year ago
|
||
Comment 8•1 year ago
|
||
Depends on D179794
Updated•1 year ago
|
Comment 9•1 year ago
|
||
Hey Adam, as a triage owner - who would be a good person to give first feedback?
I'm not entirely seeking a patch review yet, as I am going through test failures and making sure that this is 100% the intended behavior, but I hope it's somewhat. close.
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Comment 11•1 year ago
|
||
Depends on D179795
Updated•1 year ago
|
Assignee | ||
Comment 12•1 year ago
|
||
Depends on D181097
Assignee | ||
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 15•1 year ago
|
||
Assignee | ||
Comment 16•1 year ago
|
||
This is done by allowing url
to also be set to true
instead of a string, which will wait for any url to be loaded.
Depends on D181097
Comment 17•1 year ago
|
||
Comment 18•1 year ago
|
||
Backed out for causing browser_schemeless.js failures
- Backout link
- Push with failures
- Failure Log
- Failure line:
TEST-UNEXPECTED-FAIL | dom/security/test/https-first/browser_schemeless.js | Test timed out -
Assignee | ||
Updated•1 year ago
|
Comment 19•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9833b559342e
https://hg.mozilla.org/mozilla-central/rev/ef7219327a10
https://hg.mozilla.org/mozilla-central/rev/0f39aba7e4d5
https://hg.mozilla.org/mozilla-central/rev/fa6784f8b0f4
https://hg.mozilla.org/mozilla-central/rev/9c2a1ff6c90d
https://hg.mozilla.org/mozilla-central/rev/3d07a92324d0
Comment 20•1 year ago
|
||
Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/46308bfd7076
Comment 21•1 year ago
|
||
Comment 22•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/22b72253e171
https://hg.mozilla.org/mozilla-central/rev/cecb7eec7fa1
https://hg.mozilla.org/mozilla-central/rev/f09f7df8fd50
https://hg.mozilla.org/mozilla-central/rev/32e82c255be4
https://hg.mozilla.org/mozilla-central/rev/cc1e01d3e2ac
https://hg.mozilla.org/mozilla-central/rev/8cf0c99e2f50
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Updated•7 months ago
|
Description
•