1922064 - make nsStandardURL::GetDisplayHost() always return "" for file URLs

Status: RESOLVED WONTFIX

(Core :: Networking, task, P2)

Description

[edgul]

Note also: The good news is this automatically fixes displayHostPort, but the bad news is a ton more places that could be wrong (investigate?)

Comment 1

[edgul]

Easy mScheme check here.

But I'm still not clear on a ton more places that could be wrong, are we referring to other nsStandardURL methods?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

I don't think we should make GetDisplayHost lie about the host.
There are a few issues with that:

• GetDisplayHost would conflict with GetDisplaySpec.
• If we make DisplaySpec also lie about the host, then you could get an error when loading file://host/ but see a file:/// URL in the error display string.

I think we should audit the uses of displayHost, and make sure it's not used for anything critical (it's only for display, so it already shouldn't be used to make any decisions).

Comment 3

[edgul]

A quick summary of our audit on GetDisplayHost: we should be able to proceed with this as the below calls are either low impact or we have opened bugs with the respective component. Details follow:

C++ uses

GetDisplayHostPort call should be handled by the bug 1922062

JS uses

1. PageInfoChild’s getWindowInfo Should be okay

2. Add on install (?)

• This instance uses displayHost to set an html element’s context and shown to user when an add-on is blocked: https://searchfox.org/mozilla-central/rev/7fb746f0be47ce0135af7bca9fffdb5cd1c4b1d5/browser/base/content/browser-addons.js#938,944
• Probably fine. We can’t install from a file channel with a host.

3. URIFixup

• Goes into notification label and a button label:
• Triggered by UrlBarInput’s pickResult
• Seems fine.

4. UrlBarInput maybeUntrimUrl uses displayHost to update the selection

• Fine because the URL display is using the same displayHost

5. UrlbarValueFormatter's getUrlMetaData

• This compares an http scheme. Should be no issue here

6 and 7. SmartBlock getAPI and another.

• Handled by: bug 1936834

8. URIFixup just checks that it looks like a URI. Should be safe.

9 and 10. More tracking protection use & here as well

• Android code similar to smartblock above
• Handled by: bug 1936834

11. Password manager

• Seems like a buggy callsite. Should be using asciiHost or host instead as ETLD service doesn't normalize the domain.
• filed bug 1935395

12. Add on manager

• could installInfo be triggered by file url with hostname? Probably not, this is fine

Comment 4

[edgul]

If we aren't going to make GetDisplayHost lie about the host, then what is the remaining work for this bug?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Considering we've audited all potentially risky uses of displayHost, I think we can close this bug.
DisplayHost should always return the actual host of the URL.