1554984 - URL elision in Fenix

Status: RESOLVED FIXED

(Firefox for Android :: Toolbar, defect)

Description

[Frederik Braun [:freddy]]

There's a low spoofing risks on websites with long URLs pretending to belong to some other URLs by abusing the elision at the end with ... for overly long URLs.

I suggest we elide in a way that alwys ensure we show the registerable domain. See also Chrome's excellent Guidelines on URL Display on this specific section (and generally, really).

This bugs affects (at least) the following areas and might need sub-bugs for the individual fixes

• Permission prompts
• JS Modal dialogs
• quick settings (when tapping the lock icon)
• overview of open tabs (home screen?)
• collections

Comment 1

[Frederik Braun [:freddy]]

Attachment: This screenshot shows how the current elision errs for IP addresses

Example screenshot for IPv4 addresses being shortened to the left-most bytes.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This will be covered by https://github.com/mozilla-mobile/fenix/issues/2648

Comment 3

[Chris Peterson [:cpeterson]]

Adding [geckoview:fenix:m6] whiteboard tag because mozilla-mobile/fenix#2648 has the "MVP Blocker" label.

Comment 4

[Colin Lee]

Does this fix seem acceptable as a resolution? We matched what desktop was doing for URL elision.

https://github.com/mozilla-mobile/fenix/commit/45c509fbce1604f07e89e45ef710199eff6b2ba3

Comment 5

[Frederik Braun [:freddy]]

If I read https://github.com/mozilla-mobile/fenix/issues/2648#issuecomment-499257993 correctly, it seems that we are removing the whole eTLD? That seems not to increase spoofing risks, as anyone can register the combination of a high-profile .com website with an obscure TLD and the interface would just show the high-profile brand name without it. Furthermore, I could get my website "evil.example" added to the public suffix list and then have "google.evil.example" show up as "google"?

NB: I will need to defer the actual re-testing and leave the needinfo in place. I have trouble connecting to the main process of Fenix Preview (self-made debug build from Android Studio) through Firefox Nightly about:debugging, which is what I usually do to add a custom certificate and proxy for improved network inspection. I'm assuming this is an intermittent issue or a regression that will get fixed, so I will take another look next week.

Comment 6

[Colin Lee]

Thanks, I believe you read correctly that we hide the TLD from the app's home screen, which also functions right now as its tab switcher. While the user is on the site, they can view the entire URL on the URL bar, starting from the schema. Tapping on the URL bar provides the ability to view, scroll, or change the entire URL.

UX perhaps was thinking more about ease of use than potential exploits. Should I add a patch to keep the eTLD on the tab switcher screen?

I believe one may debug by enabling remote debugging via USB in Firefox Preview Settings.

Comment 7

[Frederik Braun [:freddy]]

(In reply to Colin Lee from comment #6)

UX perhaps was thinking more about ease of use than potential exploits. Should I add a patch to keep the eTLD on the tab switcher screen?
I totally understand the background, but I'd rather have us be explicit here.

I believe one may debug by enabling remote debugging via USB in Firefox Preview Settings.

Yeah, that works for tabs (content processes), but not the main process. As I said, I'll leave the needinfo and will test again, when I got some more spare cycles.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

https://github.com/mozilla-mobile/fenix/issues/4844

Added a Fenix issue for this. As issue 2648 did not fix this.

Comment 9

[Colin Lee]

Freddy, how should this behave exactly? Drop path, subdomain, and keep TLD? Since we did this incorrectly the first time, I want to be certain we get this right. Keeping NI.

Comment 10

[Chris Peterson [:cpeterson]]

Removing [geckoview:fenix:m6] whiteboard tag because this is a Fenix bug, not a GV bug.

Comment 11

[Frederik Braun [:freddy]]

Attachment: top-level still missing

(In reply to Colin Lee from comment #9)

Freddy, how should this behave exactly? Drop path, subdomain, and keep TLD? Since we did this incorrectly the first time, I want to be certain we get this right. Keeping NI.

Please excuse the embarrassingly long wait. This is still wrong, as can be seen on this screenshot.
This seems to be cutting of the eTLD, regardless of whether this is an IP address.

Tested in this screenshot:

mozfreddyb.github.io -> mozfreddyb (fail)
web.security.plumbing -> web.security (fail)
about:buildconfig -> about:buildconfig (pass)
192.168.85.1 -> 192.168.85 (fail)

Comment 12

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Sebastian, I think this code currently lives in Fenix. What do you think of pulling it into A-C? This is something that other products probably also need, so A-C would be a good place.

Comment 13

[Chenxia Liu [:liuche]]

We need to pick up the code that we're using in Fennec or Desktop. Part of this bug will probably involve talking to Desktop in #firefox to see what they do (and if we can pick those changes up), or implement this in Fenix new.

Needs test cases.

Comment 14

[Mira Lobontiu (Android QA)]

Hi all!

Since the issue is not fixed yet, I am not able to verify it.
I'll update this status on https://github.com/mozilla-mobile/fenix/issues/4844 also.

Thanks,

Comment 15

[:severin Rudie]

re: how to test this.

I asked around about this, and I'm not sure we have any good way to verify it. Every edge case that we were able to come up with now has an automated test in this file. Down the line it would be good to have a security review on this, but for now if it's alright with you, I think we can mark this qa:not-needed.

Sorry about the miscommunication!

Comment 16

[Frederik Braun [:freddy]]

There are a lot of great test cases. I just skimmed them. Thank you!
Just one minor thing. In https://github.com/mozilla-mobile/fenix/blob/4cfbaf2dc4f7526892a1663039685721755b12b8/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt#L188 you seem to be testing that the file protocol is hidden.

    @Test
    fun `should return not the protocol for file`() {
        "file:///foo/bar.txt" shortenedShouldBecome "/foo/bar.txt"
    }

This is surprising to me. Can you help me understand why?

Comment 17

[Mira Lobontiu (Android QA)]

Thank you, Severin!
I'll remove my NI. I've removed the qa-needed label from https://github.com/mozilla-mobile/fenix/issues/4844 and added the qa:not-needed label.

Comment 18

[:severin Rudie]

(In reply to Frederik Braun [:freddyb] from comment #16)

There are a lot of great test cases. I just skimmed them. Thank you!

Thanks for taking the time to look them over!

you seem to be testing that the file protocol is hidden.... Can you help me understand why?

This behavior (and this test, actually) was borrowed from desktop code for 'Top Sites', which seemed to me to be a similar use case. I'm happy to follow up if this isn't desired behavior here. Collecting requirements for this story was difficult and I'm certain I made at least a few errors, so I'm open to suggestions.

Comment 19

[Frederik Braun [:freddy]]

(In reply to Severin Rudie from comment #18)

This behavior (and this test, actually) was borrowed from desktop code for 'Top Sites', which seemed to me to be a similar use case. I'm happy to follow up if this isn't desired behavior here. Collecting requirements for this story was difficult and I'm certain I made at least a few errors, so I'm open to suggestions.

Yikes. I was not aware it is like that on desktop (I'm admitting I have a blank new tab page). I understand it's super tricky to get it right and we should find out what we can do to make all of our "show a URL" code align, regardless of whether it's in the address bar or in an overview.

The solution we should align on (this is the minimum bar) is the whatwg URL spec's section on URL rendering: https://url.spec.whatwg.org/#url-rendering

Severin, please ensure we show the URL scheme for file URLs and I'll file a duplicate of this bug for desktop.

Comment 20

[:severin Rudie]

(In reply to Frederik Braun [:freddyb] from comment #19)

...we should find out what we can do to make all of our "show a URL" code align....

Completely agreed. When Fenix settles down a little bit, I would love to wrap this into a Rust component that we can share across products.

Severin, please ensure we show the URL scheme for file URLs and I'll file a duplicate of this bug for desktop.

thumbs up. Opened https://github.com/mozilla-mobile/fenix/pull/7036 with a fix.

Comment 21

[Chenxia Liu [:liuche]]

We've landed a fix in Fenix and it's been QA-ed by our QA team, so I'm going to close this bugzilla issue as well.