If the system lies about protocol handler, use internal checks and search if not available
Categories
(Firefox :: Address Bar, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox105 | --- | fixed |
People
(Reporter: mak, Assigned: daisuke, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: papercut, Whiteboard: [snt-scrubbed][search-papercut])
Attachments
(2 files)
In bug 1618094 we have a case where, due to flatpak, any request to externalProtocolHandlerExists returns true, regardless of the protocol name.
See https://searchfox.org/mozilla-central/rev/3c9369510cb883b9f08d5f9641e1233d2142f025/docshell/base/URIFixup.jsm#315-316
This means even a made up protocol like "bacon:" will end up trying to visit a non-existing url, instead of searching.
We can implement a detection system that passes 2 randomly built strings to externalProtocolHandlerExists, and if those return true, we assume to be in "wildcard" mode and we stop trusting it.
In those cases we can use HandlerService.exists() to check if the protocol is known internally, that would do a better job.
A test should somehow mock externalProtocolHandlerExists to always return true and check we search for something like "bacon:tasty".
Reporter | ||
Updated•2 years ago
|
Reporter | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Reporter | ||
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
•
|
||
Hi Marco.
I investigated the results of HandlerService.exists()
with like the following code.
lazy.handlerService.exists(
lazy.externalProtocolService.getProtocolHandlerInfo(scheme)
);
The results of comparison with externalProtocolService.externalProtocolHandlerExists(scheme)
is below.
scheme | HandlerService.exists() | externalProtocolService.externalProtocolHandlerExists() |
---|---|---|
invalid | false | false |
http | true | false |
https | true | false |
ftp | true | false |
about | false | false |
mailto | true | true |
From this result, it seems that we need to do something for HandlerService.exists()
to use it.
For example, when initializing URIFixup
, investigate whether or not we can trust externalProtocolService.externalProtocolHandlerExists()
. Then if not trusted, get all protocol handler info from externalProtocolService somehow. Then we store them into HandlerService
, we may be able to use.
But, if we do so, it seems that HandlerService
may affect other places as well, and I think that we can manage it here without using HandlerService
.
What do you think?
Assignee | ||
Comment 2•2 years ago
|
||
Ah, sorry, the table is wrong. Completely opposite.
scheme | externalProtocolService.externalProtocolHandlerExists() | HandlerService.exists() |
---|---|---|
invalid | false | false |
http | true | false |
https | true | false |
ftp | true | false |
about | false | false |
mailto | true | true |
Reporter | ||
Comment 3•2 years ago
|
||
(In reply to Daisuke Akatsuka (:daisuke) from comment #1)
For example, when initializing
URIFixup
, investigate whether or not we can trustexternalProtocolService.externalProtocolHandlerExists()
.
Yes, we should surely do this, it's the scope of this bug. passing a couple random generated strings and checking both calls return true would do.
Then if not trusted, get all protocol handler info from externalProtocolService somehow. Then we store them into
HandlerService
, we may be able to use.
But, if we do so, it seems thatHandlerService
may affect other places as well, and I think that we can manage it here without usingHandlerService
.
Yes, we should not store anything in handlerService, that would indeed affect other code.
The results you got seem ok.
Protocols Firefox can handle like about, view-source and so on are covered by the Services.io.getProtocolHandler(scheme) check in URIFixup
http, file and https are handled by the isCommonProtocol check for perf reasons, but they'd also pass the above defaultProtocolHandler check
So the only things hitting the last check is protocols not handled by Firefox, for which we ask the OS. Since we can't trust the OS, we just check if the protocol is registered in HandlerService.
Here we assume things will not be perfect, a protocol handler will have to be registered into handlers.json to be recognized. But it's better than recognizing any random string as a valid protocol.
What is your concern with the results from comment 2?
Assignee | ||
Comment 4•2 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #3)
(In reply to Daisuke Akatsuka (:daisuke) from comment #1)
For example, when initializing
URIFixup
, investigate whether or not we can trustexternalProtocolService.externalProtocolHandlerExists()
.Yes, we should surely do this, it's the scope of this bug. passing a couple random generated strings and checking both calls return true would do.
Then if not trusted, get all protocol handler info from externalProtocolService somehow. Then we store them into
HandlerService
, we may be able to use.
But, if we do so, it seems thatHandlerService
may affect other places as well, and I think that we can manage it here without usingHandlerService
.Yes, we should not store anything in handlerService, that would indeed affect other code.
The results you got seem ok.
Protocols Firefox can handle like about, view-source and so on are covered by the Services.io.getProtocolHandler(scheme) check in URIFixup
http, file and https are handled by the isCommonProtocol check for perf reasons, but they'd also pass the above defaultProtocolHandler check
So the only things hitting the last check is protocols not handled by Firefox, for which we ask the OS. Since we can't trust the OS, we just check if the protocol is registered in HandlerService.
Here we assume things will not be perfect, a protocol handler will have to be registered into handlers.json to be recognized. But it's better than recognizing any random string as a valid protocol.
Thank you very much, Marco!
It seems that I can implement it now.
What is your concern with the results from comment 2?
Ah, no concern. I just mistook writing the content of the result table in comment 1.
Reporter | ||
Updated•2 years ago
|
Assignee | ||
Comment 5•2 years ago
|
||
Pushed by dakatsuka.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d744fed63a45 Fallback to nsIHandlerService if don't trust nsIExternalProtocolService. r=mak
Comment 7•2 years ago
|
||
Backed out changeset d744fed63a45 (bug 1744243) for causing xpc failures in test_URIFixup_external_protocol_fallback.
Backout link: https://hg.mozilla.org/integration/autoland/rev/7a05657845fdaa46d64666421835015ca10ade7c
Pushed by dakatsuka.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aef29279d32f Fallback to nsIHandlerService if don't trust nsIExternalProtocolService. r=mak
Assignee | ||
Updated•2 years ago
|
Comment 9•2 years ago
|
||
bugherder |
Comment 10•2 years ago
|
||
Sorry to be a pain, but this test runs in Thunderbird's CI, and Thunderbird doesn't have mailto handlers, because it is the handler. So the new test fails. I'll make a patch.
Comment 11•2 years ago
|
||
Comment 12•2 years ago
|
||
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/integration/autoland/rev/a7c93bfcf67e follow-up - Fix test_URIFixup_external_protocol_fallback.js when run in Thunderbird. r=daisuke
Comment 13•2 years ago
|
||
bugherder |
Comment 14•2 years ago
|
||
A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)
Description
•