JSON fragments visible when typing double quotes into location bar

VERIFIED FIXED in Firefox 47

Status

()

Firefox
Location Bar
P1
normal
VERIFIED FIXED
a year ago
6 months ago

People

(Reporter: freddyb, Assigned: adw)

Tracking

(Blocks: 1 bug)

43 Branch
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 affected, firefox45 affected, firefox46 affected, firefox47 verified)

Details

(Whiteboard: [fxsearch][unifiedcomplete], URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8699916 [details]
json-injection-CWbUsJnWsAAc3Zb.png

As observed by Peter Jaric on Twitter.

STR:

1. Create a new profile, this affects Firefox 43 (current release) to 46 (current nightly).
2. Type something into the location bar that contains a double quote, e.g.,
> misdirect.ion.land/"
3. Observe how the location bar shows a JSON fragment instead of the actual URL.


The location bar should show the actual URL.
(Reporter)

Comment 1

a year ago
As discussed in the oingoing twitter thread, one can overwrite the display element to show a different URL:

This URL shows "Visit your parents" instead of the actual URL.

> http://www.jaric.org?/jaric.org/?/ ","url":"your parents


Injecting < or > does not breakr or create HTML/XML.
Interesting behavior indeed
Priority: -- → P1
Whiteboard: [fxsearch][unifiedcomplete]

Updated

a year ago
Assignee: nobody → adw
Blocks: 1206388

Comment 3

a year ago
At last this bug (reported several times) became interesting enough and got priority P1.
Blocks: 1187653, 1196874
(Assignee)

Comment 4

a year ago
Created attachment 8709795 [details] [diff] [review]
patch

This took a while...  The basic problem is simple though: we feed whatever you type into the urlbar into JSON.parse(), so you can break things by typing quotes and commas, like the URL in comment 1.  We should be encoding each property value in action.param objects.  Encode before stringifying, and decode after parsing.

But even though the problem is simple, there are lots of places in the code that make and parse moz-action URIs, and there's also at least one place that decodes all URIs in the results (_appendCurrentResult), which breaks moz-action URIs.  So while this patch seems to work, and the relevant tests that I could find pass, it's a little scary making these changes.

I was tempted to move all the moz-action URI making/parsing into one place, like I mentioned in another bug, but I didn't want to make this patch bigger than necessary since maybe we want to uplift it.
Attachment #8709795 - Flags: review?(mak77)
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab6600317a3
Summary: JSON fragments visible when typing double quotes into location ar → JSON fragments visible when typing double quotes into location bar
Comment on attachment 8709795 [details] [diff] [review]
patch

Review of attachment 8709795 [details] [diff] [review]:
-----------------------------------------------------------------

Could you please add a test covering the escape char and quotation mark in a location bar search?

I agree unEscapeURIForUI black magic is quite scary. My only doubt here is whether with this change we may not anymore invoke unEscapeURIForUI on URIs that are part of a moz-action uri and will be shown the user, for example switch-to-tab, remote-tab and visiturl have an url param that is shown as-is... I wonder if we'll show them badly after this change.
Maybe we still want to unEscapeURIForUI the url param of a moz-action url just before showing it to the user? I think that is where in autocomplete.xml we do displayUrl = action.params.url;
Would be nice to also have a test for this too, but I'd be happy with a followup bug if it's complicate enough.
Attachment #8709795 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 7

a year ago
Created attachment 8711233 [details] [diff] [review]
patch with test

Here's a test.  Re: showing escaped URLs badly, that's what the code under the "The fixup encoded the URI" comment in the UnifiedComplete.js change prevents.  If that change were not there, then you're right that URLs would be shown unescaped to the user.  I'm not 100% that unescaping like that in UnifiedComplete is OK and won't break other things... but the try run was good.  And actually this new test also happens to test that problem.  If the problem were present, then the shown URL during the test would be:

http://example.com/%20%22,%20%22url%22:%20%22bar

instead of:

http://example.com/ ", "url": "bar

And you can see that the test checks for the latter.
Attachment #8709795 - Attachment is obsolete: true
Attachment #8711233 - Flags: review?(mak77)
(Assignee)

Comment 8

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b42ba35f45b1
Blocks: 1242232
Comment on attachment 8711233 [details] [diff] [review]
patch with test

Review of attachment 8711233 [details] [diff] [review]:
-----------------------------------------------------------------

I still have some questions, but I'm not going to block if you think this is fine, having more real life testing is better than delaying.

::: browser/base/content/test/general/browser_urlbarJSON.js
@@ +3,5 @@
> +// This test makes sure you can't break the urlbar by typing particular JSON
> +// fragments into it.  See bug 1233672.
> +
> +add_task(function* () {
> +  let input = 'http://example.com/ ", "url": "bar';

could you please also test adding a backslash? it is currently breaking the UI too.

@@ +30,5 @@
> +               ["action", "heuristic", "visiturl"].toString(),
> +               "type");
> +
> +  Assert.equal(item._title.textContent, "Visit " + input, "Visible title");
> +});

Maybe better to call gURLBar.handleRevert(); at the end of the test to clean it up?

::: toolkit/components/places/UnifiedComplete.js
@@ +1307,5 @@
>      }
>  
> +    // The fixup encoded the URI.  Since the URI is displayed to the user,
> +    // decode it so it's prettier.
> +    let spec = decodeURI(uri.spec);

What I'm still worried about, is that idn names have some special handling in unescapeURIForUI (regarding network.IDN.blacklist_chars) and I don't think decodeURI handles that? Unfortunately I don't think we have many tests covering those.
I'm also not sure it's the right thing to return a decoded uri from the backend.
Would it be a problem to use unescapeURIForUI in autocomplete.xml when we set displayUrl in action handling code, as I suggested in comment 6? Do you think it is better to decode in the backend?For non-action results it happens here _appendCurrentResult.

::: toolkit/content/widgets/autocomplete.xml
@@ +1235,5 @@
> +            } else {
> +              // Unescape the URI spec for showing as an entry in the popup
> +              url = Components.classes["@mozilla.org/intl/texttosuburi;1"].
> +                getService(Components.interfaces.nsITextToSubURI).
> +                unEscapeURIForUI("UTF-8", originalURL);

nit: while here would you mind to reindent this on dots?
Attachment #8711233 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #9)
> For non-action results it
> happens in _appendCurrentResult.

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#1229
(Assignee)

Updated

a year ago
No longer blocks: 1242232
Duplicate of this bug: 1242232
(Assignee)

Comment 12

a year ago
Created attachment 8712342 [details] [diff] [review]
patch v3

OK, after looking at this more yesterday, I'm pretty confident in this patch.  UnifiedComplete embeds the escaped URL returned from fixup into the action URI.  So it's up to the front-end to unescape it if it wants.  The front-end, in autocomplete.xml, now unescapes the URL in one place, in _adjustAcItem.  If the URL is an action URI, then it unescapes the embedded URL.  Otherwise it unescapes the URL itself.

As a convenience, UnifiedComplete also passes an escaped version of the URL, as match.comment, because it's expected that the front-end will use the comment for displaying to the user at most.

This patch makes one more change.  While I was trying to figure out how params.url is used everywhere, I took a closer look at the params.originalUrl that was added in bug 1104165.  I don't think that was actually necessary to fix that bug.  The thing that fixed it was not calling losslessDecodeURI on action URIs.  And as far as I can tell the reason for calling losslessDecodeURI at all is to keep weird characters out of the textbox, but action URIs themselves are never put into the textbox.  browser_bug1104165-switchtab-decodeuri.js still passes with this change.
Attachment #8711233 - Attachment is obsolete: true
Attachment #8712342 - Flags: review?(mak77)
(Assignee)

Comment 13

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b41395487dc3
Comment on attachment 8712342 [details] [diff] [review]
patch v3

Review of attachment 8712342 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not 100% sure about losslessDecodeURI. I put it there cause the code was invoking losslessDecodeURI to whatever went through textValue, and url from actions would be skipping that.
Regardless, that change was not required for the fix to make switch to tab work on google maps, but mostly a try to make textValue behavior consistent: "val contains a url that went through losslessDecodeURI"
We are unlikely to have a test that checks urls inside action urls are "properly" decoded. It may cause urls to be less readable, it may just cause subtle differences. Looks like there's a whole history of bugs behind losslessDecodeURI that I don't know well.
Maybe you could look at bug 582186, bug 909264, bug 452979 (from losslessDecodeURI comments) and see if the change makes any difference for the user with visituri and switch to tab actions.

::: toolkit/components/places/UnifiedComplete.js
@@ +1317,5 @@
> +    let escapedURL = uri.spec;
> +    let displayURL =
> +      Components.classes["@mozilla.org/intl/texttosuburi;1"]
> +                .getService(Components.interfaces.nsITextToSubURI)
> +                .unEscapeURIForUI("UTF-8", uri.spec);

just textURIService.unEscapeURIForUI("UTF-8", uri.spec); since we already have a lazy getter for it

::: toolkit/content/widgets/autocomplete.xml
@@ +1398,5 @@
>              // so it must not contain anything that should not be user-facing.
>  
>              let parts = [
>                this.getAttribute("title"),
> +              this._displayUrl,

do you think it would be an overkill to use a displayUrl attribute?

@@ +1674,5 @@
> +        <body>
> +          <![CDATA[
> +          return Components.classes["@mozilla.org/intl/texttosuburi;1"]
> +                           .getService(Components.interfaces.nsITextToSubURI)
> +                           .unEscapeURIForUI("UTF-8", url);

would be nice to cache Components.classes["@mozilla.org/intl/texttosuburi;1"].getService(Components.interfaces.nsITextToSubURI) into a field since it's used often.

@@ +1809,5 @@
>              [title, searchEngine] = title.split(TITLE_SEARCH_ENGINE_SEPARATOR);
>              displayUrl = this._stringBundle.formatStringFromName("searchWithEngine", [searchEngine], 1);
>            }
>  
> +          displayUrl = displayUrl || this._unescapeUrl(url);

May we assign getAttribute("url") to originalUrl, remove url, and here just have a

if (!displayUrl) {
  displayUrl = typeof input.trimValue == "function" ?
    this._unescapeUrl(input.trimValue(originalUrl)) :
    this._unescapeUrl(originalUrl);
}

The reason I'd prefer this, is that we are starting having too many "url" variables around: url, originalUrl, displayUrl, emphasisUrl... it's starting being confusing.

On the other side, it would require all the actions to do
displayUrl = this._unescapeUrl(action.params.url);
but that looks a minor problem.
What do you think?
Attachment #8712342 - Flags: review?(mak77) → review+
(Assignee)

Comment 15

a year ago
Created attachment 8713431 [details] [diff] [review]
patch v4

You're right about the losslessDecodeURI for embedded URLs.  The previous patch broke those bugs that you mentioned.  Hopefully this is the final patch...

This patch still removes how the textValue setter was calling losslessDecodeURI on the embedded URL and then reconstructing the action URI.  But it replaces it with a new params.displayUrl that's created right when the action URI is parsed.

(In reply to Marco Bonardo [::mak] from comment #14)
> do you think it would be an overkill to use a displayUrl attribute?

I had the same thought, so I did it.  And actually that let me use it in urlbar-rich-result-popup's createResultLabel, which is nice.  (Otherwise I could use the new params.displayUrl.)

> would be nice to cache
> Components.classes["@mozilla.org/intl/texttosuburi;1"].getService(Components.
> interfaces.nsITextToSubURI) into a field since it's used often.

done

> May we assign getAttribute("url") to originalUrl, remove url, and here just
> have a

Sounds good, done.
Attachment #8712342 - Attachment is obsolete: true
Attachment #8713431 - Flags: review?(mak77)
(Assignee)

Comment 16

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=edd51f60eacf
Comment on attachment 8713431 [details] [diff] [review]
patch v4

Review of attachment 8713431 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thank you for adding even more tests.

::: browser/base/content/urlbarBindings.xml
@@ +869,5 @@
> +            try {
> +              uri = makeURI(action.params.url);
> +            } catch (e) {}
> +            action.params.displayUrl = uri ? losslessDecodeURI(uri)
> +                                           : action.params.url;

instead of using a less readable ternary you could assign in the try and in the catch. Also cause eslint rule (currently disabled) doesn't like empty bodies.

::: toolkit/content/widgets/autocomplete.xml
@@ +1807,5 @@
>              displayUrl = this._stringBundle.formatStringFromName("searchWithEngine", [searchEngine], 1);
>            }
>  
> +          if (!displayUrl) {
> +            displayUrl = this._unescapeUrl(

you forgot to move:
let input = this.parentNode.parentNode.input;

indeed tests are complaining

@@ +1811,5 @@
> +            displayUrl = this._unescapeUrl(
> +              typeof input.trimValue == "function" ?
> +              input.trimValue(originalUrl) :
> +              originalUrl
> +            );

I'd also be fine with a temp var, for readability. My problem is not with url variables that are short-lived.
Attachment #8713431 - Flags: review?(mak77) → review+
(Assignee)

Comment 18

a year ago
Created attachment 8713722 [details] [diff] [review]
patch v4.1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=862f328a591c
Attachment #8713431 - Attachment is obsolete: true
Attachment #8713722 - Flags: review+
(Assignee)

Comment 19

a year ago
Clear the urlbar and make sure the popup is closed after each add_task:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1393d3621d4
(Assignee)

Comment 20

a year ago
https://hg.mozilla.org/integration/fx-team/rev/2323e6b411c1

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2323e6b411c1
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
No longer blocks: 1206388
Duplicate of this bug: 1206388

Comment 23

a year ago
This bug also fixed suggestion for this string (earlier it was "a_%uFEFF_b - Search with Google")
a__b

Updated

a year ago
Duplicate of this bug: 1263788

Updated

a year ago
Duplicate of this bug: 1265379

Updated

a year ago
Duplicate of this bug: 1267160
Flags: qe-verify+
Confirming the fix across platform using Firefox 47.0b4, build ID 20160509171155.
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
Flags: qe-verify+

Updated

11 months ago
Duplicate of this bug: 1277151

Updated

11 months ago
Duplicate of this bug: 1277728
(Assignee)

Updated

10 months ago
Blocks: 1273521

Updated

10 months ago
No longer blocks: 1273521

Updated

6 months ago
Duplicate of this bug: 1187653
You need to log in before you can comment on or make changes to this bug.