2070848 - Overeager visual URL-decoding of delimiters in StatusPanel

Status: NEW

(Firefox :: Tabbed Browser, defect)

Description

[Nicolas Chevobbe [:nchevobbe]]

(cloned from Bug 2069873 so we have dedicated bug for each impacted components)

Steps to reproduce:

1. Take the following example links:
   1.1. https://example.com/?action=edit&username=%26action%3Ddelete
   1.2. https://example.com/X%23Y
   1.3. https://example.com/X%2FY%3FZ

2. Observe over-eager visual URL-decoding, when:
   2.1. Hovering over the links above, checking the URL preview in the bottom of the desktop browser

Example: https://example.com/?action=edit&username=%26action%3Ddelete is displayed as https://example.com/?action=edit&username=&action=delete, changing the semantic meaning from two query parameters (action, username) to three query parameters (action, username, action).

Actual results:

URL-encoded URL (sub)delimiters are overeagerly URL-decoded in the visual output in multiple components like link previews.

Semantic meaning as seen by the end user or developer is different than the actual semantics of the URL.

The actual bytes sent to the server seem consistent with the actual meaning of the URL, so it seems to be only a visual bug.

Expected results:

URL-encoded URL (sub)delimiters (reserved characters), including %2F (/) , %3F (?), %26 (&), %23 (#), should not be visually URL-decoded.

Comment 1

[Dão Gottwald [:dao]]

IIRC it's intentional that we decode loose and lossy because it's only for presentation, so unlike the address bar or devtools showing URLs this doesn't need to survive a copy-paste-submit round-trip. That said, it probably shouldn't be actively misleading as to what clicking the link will send.

Comment 2

[Stephen Thompson [:sthompson]]

Status panel uses its own sanitization https://searchfox.org/firefox-main/rev/6c74efe2fcddf84b6f320959064a66946b4a1759/browser/base/content/browser.js#2018-2055

Last year, there was a discussion about the different URL sanitization methods in the codebase. There was a proposal to standardize on the most mature implementation from the URL bar https://searchfox.org/firefox-main/rev/6c74efe2fcddf84b6f320959064a66946b4a1759/browser/components/urlbar/content/UrlbarInputBase.mjs#6310-6413

:Gijs pointed out UrlbarUtils.unEscapeURIForUI as well. :mak said this at the time:

Whether we could use losslessDecodeURI everywhere, it may be a good idea as that list is by far more updated and uses more modern unicode aware regex. Though at that point I'd wonder if unEscapeURIForUI makes still sense, as it may become a pitfall for new features if we don't trust it?
...
The urlbar started using character groups exactly to have a better long term behavior also with introduction of new unicode in the future.
...
Please file a follow-up bug to analyze whether unEscapeURIForUI should be replaced everywhere or still has valuable uses, and share losslessDecodeURI from Toolkit.

I don't see a followup on file. Based on comment 1 I can see that we have some different use cases and we may not want a single implementation for all use cases.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Stephen Thompson [:sthompson] from comment #2)

Whether we could use losslessDecodeURI everywhere, it may be a good idea as that list is by far more updated and uses more modern unicode aware regex. Though at that point I'd wonder if unEscapeURIForUI makes still sense, as it may become a pitfall for new features if we don't trust it?
...
The urlbar started using character groups exactly to have a better long term behavior also with introduction of new unicode in the future.
...
Please file a follow-up bug to analyze whether unEscapeURIForUI should be replaced everywhere or still has valuable uses, and share losslessDecodeURI from Toolkit.

I don't see a followup on file. Based on comment 1 I can see that we have some different use cases and we may not want a single implementation for all use cases.

I think the address bar use case is the crucial one. LinkTargetDisplay pretty much only prioritizes readability, but again if it's misleading then what's the point? :) From a user's perspective it doesn't seem to make much sense that we preview something, you click it, and then we show something different in the address bar. The address bar does also prioritize readability but not at the cost of ambiguity, so that seems like the sane default behavior. As the original author of both decoding choices which were deliberately different at the time, I'm in favor of unifying in the direction of losslessDecodeURI (now losslessDecodeURL / losslessDecodeDisplaySpec).