1563039 - Incorrect domain and tld highlighting (urlbar.formatting)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[bugcon]

Attachment: android.png

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

Visit https://one.one.one.one/ on Android

Actual results:

The subdomains (The first 2 "one.one"'s) are highlighted as the domain and tld

Expected results:

The domain and tld (The last 2 "one.one"'s) should be highlighted

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Does not need to be a security bug.

Comment 2

[bugcon]

(In reply to Kevin Brosnan [:kbrosnan] from comment #1)

Does not need to be a security bug.

I (perhaps obviously) disagree as the intent of the feature is to prevent subdomain-style domain spoofing attacks - whereas this bug in the feature would actually highlight the domain being spoofed as the legitimate site(!)

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

This looks like a specific case of one site. Playing around with similar domains such as https://www.one.one or https://google.one.one are highlighted as expected.

Comment 4

[Jan Henning [:JanH]]

Whoops, I guess https://dxr.mozilla.org/mozilla-central/rev/fd2778a2ce8ba1284cba867f4358f9fcaf795568/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#335 actually needs to use lastIndexOf().

Comment 5

[Sorina Florean [:sflorean]]

Reproducible also on 69.0a1 and 68 Fennec. I will set the flags to reflect the affected versions.

Comment 6

[bugcon]

Tested with v41 (~2015) which was also affected. :sflorean did you mean to un-assign :JanH?

Comment 7

[Sorina Florean [:sflorean]]

Sorry Jan, when I posted the comment I unassigned you. Thanks bugcon for the notice.

Comment 8

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #4)

Whoops, I guess https://dxr.mozilla.org/mozilla-central/rev/fd2778a2ce8ba1284cba867f4358f9fcaf795568/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#335 actually needs to use lastIndexOf().

Actually not quite as simple, because otherwise we could mistakenly get matches in the path part of the domain.

Comment 9

[Jan Henning [:JanH]]

Attachment: Bug 1563039 - Fix erroneous base domain highlighting. r?VladBaicu

Domain highlighting needs to find the last instance of the base domain within
the domain part of the URL. Otherwise, there's a chance we mistakenly highlight
(parts of) a subdomain if it matches the base domain, too.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67f71ec8a8a4
Fix erroneous base domain highlighting. r=VladBaicu

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 9076624 [details]
Bug 1563039 - Fix erroneous base domain highlighting. r?VladBaicu

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug fix for URL bar domain highlighting
• User impact if declined: If the base domain (eTLD + 1) appears within the subdomain as well, we mistakenly highlight (and scroll to) that part of the subdomain instead of the proper base domain.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only small change in the highlighting code required, so we correctly find the last appearance of the base domain inside the domain segment of the URL.
• String or UUID changes made by this patch: none

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/67f71ec8a8a4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9076624 [details]
Bug 1563039 - Fix erroneous base domain highlighting. r?VladBaicu

Fixes erroneous domain highlighting, approved for Fennec 68.1b2 (and Beta too just to cover bases).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/20015237f3bf

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/39abdfd19eae

Comment 16

[Stefan Deiac]

Verified as fixed on Nightly 70.0a1 (2019-07-18), Beta 69.0b6 and Beta ESR 68.1b2.
Devices:

• Motorola Moto G6 (Android 8);
• Samsung Galaxy S8 (Android 9);
• Google Pixel (Android Q).