1773298 - about:* pages are broken if ABOUT is written in uppercase letters

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Ardelean Oana]

Attachment: Screen Recording 2022-06-08 at 17.09.26.mov

Affected versions

• Firefox 102
• Nightly 103

Affected platforms

• Ubuntu 18.04
• Windows 10
• macOS 11

Steps to reproduce

1. Open Firefox.
2. In the adress bar, type ABOUT:LOGINS and hit enter.

Expected result

• User is redirected to a functional about:logins page.

Actual result

• User is redirected to a broken about:logins page.

Regression range

• Will look for one ASAP.

Attachments

• There is a video showing the problem in more detail.

Notes
This is only reproducible using uppercase letters or a combination between uppercase and lowercase letters(spellings such as aBouT:LoGIns). Whenever one writes about:LOGINS, ABOUT:logins, they are redirected to a functional page.

Comment 1

[Sergey Galich [:serg]]

Interesting! Same problem with a few other ABOUT: pages (I've tried ABOUT:PROTECTIONS and ABOUT:NEWTAB)

Comment 2

[Ardelean Oana]

mozregression bisection points to:
-last good: 2022-05-05
-first bad: 2022-05-06

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7bb9efa58526a297b185bfcb1d64dfa4dee020f9&tochange=2e478e0b255228880e524b00296f95ff7839e96c

Most likely bug 1560676.

Comment 3

[Release mgmt bot [:suhaib / :marco/ :calixte]]

:oana.ardelean, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 4

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1560676

Comment 5

[Release mgmt bot [:suhaib / :marco/ :calixte]]

:jteow, since you are the author of the regressor, bug 1560676, could you take a look?
For more information, please visit auto_nag documentation.

Comment 6

[Drew Willcoxon :adw]

Given the regressing bug and the fact this happens for multiple about pages, let's move this to address bar. I can't imagine that something in the urlbar code is actually affecting how these pages load, but I wonder if it's something as simple as some code somewhere doing a case-sensitive comparison for "about", like this, and failing because you've actually loaded "ABOUT". But that wouldn't explain how mozregression found bug 1560676.

Comment 7

[Release mgmt bot [:suhaib / :marco/ :calixte]]

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 8

[Drew Willcoxon :adw]

Thanks Tom, based on those other bugs this sounds like a longstanding problem. I'd still like to understand why mozregression found bug 1560676 though. I'm guessing it affects the STR that were used and it's probably not actually a regressor, but I'd like to verify that.

Comment 9

[James Teow [:jteow]]

Drew, I'm wondering if there's two separate issues:

1. About pages act inconsistently depending on case
2. Previous to my change, URL bar would show the "correct" lowercase about page as the heuristic result even if the user typed all caps, but post-change, if ABOUT is all caps, then it doesn't recognize ABOUT as a protocol.

The error is likely happening because I neglected to set the prefix to lowercase.

I feel like the fix here is for me to add .toLowerCase() to prefix in the includes check and then add a test expecting only one result when typing an about page. Prefixes should be case insensitive and the canonical form of a prefix is lowercase so it stands to make sense that it should be lower-cased before checking. What do you think?

There's still ways for users to fool the mechanism making the heuristic result show the corrected about result, namely typing an all caps protocol and a wrong path (e.g. ABOUT:PRIVATEBROWSINGS) and then deleting characters, but that seems like a super edge case.

Comment 10

[Sergey Galich [:serg]]

(In reply to James Teow [:jteow] from comment #9)

The error is likely happening because I neglected to set the prefix to lowercase.

I tried that and it still failed on about:LOGINS. I'm curious what is so special about it, because it opens about:logins, but then something is stuck and no data is loaded.

Comment 11

[Drew Willcoxon :adw]

Before bug 1560676 landed, typing ABOUT:LOGINS and pressing enter would load about:logins -- all lowercase. (I verified this with a Nightly build from May 5, a day before that bug landed.)

After that bug, it loads about:LOGINS -- the part after the colon remains in uppercase.

Bug 1560676 made it easier to reproduce the problem, but before that bug it was still possible to load about:LOGINS by typing ABOUT:LOGINSX and then backspacing over the X, like you noted in your comment, James.

So you're right that there are two separate problems here. I think for now we should use this bug to make your suggested fix of changing this line to:

      !UrlbarUtils.PROTOCOLS_WITHOUT_AUTHORITY.includes(prefix.toLowerCase())

With that change, we're back to the status quo before bug 1560676, where typing ABOUT:LOGINS loads about:logins.

For the second problem -- when about:LOGINS is actually loaded, regardless of how the user manages to load it -- I can think of a few possible fixes and I'm not sure which one is best. It probably requires discussion among different teams.

• Force the urlbar to load about-pages using lowercase URIs
• Force some deeper part of Firefox's page-loading code path to load about-pages using lowercase URIs (docshell? AboutRedirector?)
• Don't try to load about:logins at all when attempting to load about:LOGINS. Instead show an error page
• Fix whatever problem is causing about:logins to be blank when about:LOGINS is loaded

Comment 12

[Tom S [:evilpie]]

I am not sure if that is still the case, but I think I remember that someone mentioned that, JSWindowActor matches is case-sensitive: So when going to about:LOGINS the appropriate JSM file isn't being loaded. (This is totally unverified, but I thought it might be useful to at least rule out)

Comment 13

[Drew Willcoxon :adw]

Adding "about:LOGINS" to that array fixes it, so looks like you're right. :-) I still think it's not clear if that's the right fix. It would mean we'd need to add uppercase versions of all about-pages to that AboutLogins object. It seems better to either fix the problem at a more fundamental layer that applies to all about-pages or just show an error page.

Comment 14

[James Teow [:jteow]]

Attachment: Bug 1773298 - Convert prefix to lowercase before lookup in stripURLPrefix - r?adw

Comment 15

[James Teow [:jteow]]

(In reply to Sergey Galich [:serg] from comment #10)

(In reply to James Teow [:jteow] from comment #9)

The error is likely happening because I neglected to set the prefix to lowercase.

I tried that and it still failed on about:LOGINS. I'm curious what is so special about it, because it opens about:logins, but then something is stuck and no data is loaded.

My proposed fix won't fix the issue of about pages acting strangely when one navigates to a capitalized version of the path, but it should fix the URL bar from not having the fixed up version of the URI as the first result when users start typing the protocol with a non-lowercase character. That's unfortunately what's happening in the video provided by Ardelean. The fix up to stripURLPrefix is something I should have be doing in the first place since protocols should be treated case-insensitive.

IMO, the larger issue of about pages acting strangely depending on the path containing uppercase characters is something that is beyond the scope of the URL bar and should be fixed in the open bugs others have linked in this bug.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b50db30781f7
Convert prefix to lowercase before lookup in stripURLPrefix - r=adw

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/b50db30781f7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR102 approval when you get a chance.

Comment 19

[James Teow [:jteow]]

Comment on attachment 9281741 [details]
Bug 1773298 - Convert prefix to lowercase before lookup in stripURLPrefix - r?adw

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The bug could impact ESR 102 users ability to navigate to an about page.
• User impact if declined: Without this, users in ESR 102 typing an uppercase in the "about" protocol could go to an about page that acts irregularly. This doesn't fix the issue of navigating to an odd about page if a user goes to an uppercase version of it (e.g. about:PRIVATEBROWSING) as that is a separate bug, but it will re-enable auto-suggestion of the all lower case version regardless of the case used in typing about:.
• Fix Landed on Version: firefox103
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change to the application code is a one-line change and would only affect a small subset of protocols, specifically found at: https://searchfox.org/mozilla-central/rev/2946e9b450cb35afaf8dad927a8d187975dcd74d/browser/components/urlbar/UrlbarUtils.jsm#207

The pre-patch version of the code only matched if the input protocol was all lowercase. However, the protocol should be case insensitive, so the patch just converts the protocol as all lowercase before matching against the list of safe protocols.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9281741 [details]
Bug 1773298 - Convert prefix to lowercase before lookup in stripURLPrefix - r?adw

Approved for 102.1esr.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/e0d6eee37ac3

Comment 22

[Adrian Florinescu [:aflorinescu]]

Although the verification comes a bit late,here's the results, as follows:

1. There are differences in behaviour betwen the latest 104+beta 103 and the ESR 102.0.1 see [video](https://bugzilla.mozilla.org/attachment.cgi?id=9286069)
   - latest 104 + beta 103 an input like ABOUT:LOGINS or about:LOGINS will always force about:logins navigation
   - ESR 102.0.1 an input like ABOUT:LOGINS will force toLOWER for about, resulting in about:LOGINS navigation, which is broken
2. Edit string with UPPER will allow the access to the edited address: (already mentioned in comment 9)

• about:logins edited into about:logINs will navigate to about:logINs which doesn't work (latest 104, beta 103 and ESR 102.0.1)

3. Copy/paste string will result into navigation to an invalid string copy ABOUT:LOGINS and address bar will navigate to about:LOGINS (latest 104, beta 103 and ESR 102.0.1)

James, for point 1, I tried with mozregression to find fix to see if something changed betwen 103 and 102 but mozregression did only point to this patch. Any idea why ESR behaves differently than 103b and 104 Nightly; was the patch applied incomplete?

Taking this bug verification as purely toLower for about protocol we might consider it as verified, but the above scenarios would still affect the users who would navigate to a broken page using one of the 2nd or 3rd scenario (e.g. about:HOME, about:hoMe or about:LOGIN) and although this might seem like an edge case, I think this would be rather the main usage case, in which you type about:logns and then the I that's missing would be a caps I.
I'm also a bit confused why some of the protocol pages from about:about long list seem affected by the 2-3 scenarios, while others seem to be doing just fine, no mater what capitalization are in.

Finally, just my two cents nitpicking on the approach here. While the toLower for about protocol makes sense, IMO, it's editing user input, which I don't see as a best practice: I would've liked more an approach in which the user input in in address bar remains unchanged, while on backend we would toLOWER the entire string loading the target right about:page or something like that.

Comment 23

[Adrian Florinescu [:aflorinescu]]

Attachment: aboutLOGINS.mp4

Comment 24

[Adrian Florinescu [:aflorinescu]]

Comment on attachment 9286069 [details]
aboutLOGINS.mp4

Please ignore the above comment22 stiken part of the comment and video, I did a mistake verifying 102.0.1ESR which doesn't have the patch vs verifying the 102.1.0ESR which has it.

Comment 25

[James Teow [:jteow]]

Hi Adrian,

Yes, unfortunately this fix doesn't completely negate one of the issues, namely about pages acting strangely with uppercase letters with some working as expected while some act oddly. And indeed, there are ways users can encounter these pages like you pointed out. However, I think that should be handled at the root of whatever is causing this issue which is outside the address bar.

Ultimately, my fix for this regression was to make sure that the helper worked correctly. It doesn't solve all the ways users can end up on those pages, but that's also because the URL bar code works differently depending on how you enter the text, e.g. typing it out, pasting text, modifying existing text... that's where I think fixing the about pages is key and avoids having to write extra custom code in the URL bar to accommodate the underlying bug.

To your point about avoiding modifying a users input, I typically agree and we are moving away from that. But in this case, I think it's okay to temporarily modify a users input in the context of checking a value against a set of stored values. Based on the standard, schemes should be case-insensitive and lowercase is the canonical form of a protocol. The change to the user input is to a copy of it and is only used to check against a protocol safelist. If the protocol is recognized in the helper function, it will return the protocol exactly how the user typed it.

For example, if a user types "ABOUT:newtab", the helper extracts ABOUT:, lower cases it to check it against a safelist, and if it belongs, it is then distributed into the array `["ABOUT:", "newtab"].

Comment 26

[Adrian Florinescu [:aflorinescu]]

Thanks for the explanation James.

Given comment 22 and comment 25, I've verified the fix on:

• Windows 10
• Ubuntu 22.04
• Mac 11.04

with

• 104.0a1 2022-07-19
• 103.0b9
• 102.1.0esr