Closed Bug 1744243 Opened 2 years ago Closed 2 years ago

If the system lies about protocol handler, use internal checks and search if not available

Categories

(Firefox :: Address Bar, defect, P3)

defect

Tracking

()

RESOLVED FIXED
105 Branch
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".

Mentor: mak
Whiteboard: [snt-triaged][search-papercut]
Whiteboard: [snt-triaged][search-papercut] → [snt-scrubbed][search-papercut]
Assignee: nobody → mak
Status: NEW → ASSIGNED

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?

Flags: needinfo?(mak)

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

(In reply to Daisuke Akatsuka (:daisuke) from comment #1)

For example, when initializing URIFixup, investigate whether or not we can trust externalProtocolService.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 that HandlerService may affect other places as well, and I think that we can manage it here without using HandlerService.

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?

Flags: needinfo?(mak) → needinfo?(daisuke)

(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 trust externalProtocolService.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 that HandlerService may affect other places as well, and I think that we can manage it here without using HandlerService.

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.

Flags: needinfo?(daisuke)
Assignee: mak → daisuke
Pushed by dakatsuka.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d744fed63a45
Fallback to nsIHandlerService if don't trust nsIExternalProtocolService. r=mak

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

Push with failures

Failure log

Flags: needinfo?(daisuke)
Pushed by dakatsuka.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aef29279d32f
Fallback to nsIHandlerService if don't trust nsIExternalProtocolService. r=mak
Flags: needinfo?(daisuke)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch

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.

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

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)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: