1233672 - JSON fragments visible when typing double quotes into location bar

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Frederik Braun [:freddy]]

Attachment: 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.

Comment 1

[Frederik Braun [:freddy]]

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.

Comment 2

[Marco Bonardo [:mak]]

Interesting behavior indeed

Comment 3

[arni2033]

At last this bug (reported several times) became interesting enough and got priority P1.

Comment 4

[Drew Willcoxon :adw]

Attachment: 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.

Comment 5

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab6600317a3

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Drew Willcoxon :adw]

Attachment: 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.

Comment 8

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b42ba35f45b1

Comment 9

[Marco Bonardo [:mak]]

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?

Comment 10

[Marco Bonardo [:mak]]

(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

Comment 12

[Drew Willcoxon :adw]

Attachment: 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.

Comment 13

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b41395487dc3

Comment 14

[Marco Bonardo [:mak]]

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?

Comment 15

[Drew Willcoxon :adw]

Attachment: 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.

Comment 16

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edd51f60eacf

Comment 17

[Marco Bonardo [:mak]]

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.

Comment 18

[Drew Willcoxon :adw]

Attachment: patch v4.1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=862f328a591c

Comment 19

[Drew Willcoxon :adw]

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

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

Comment 20

[Drew Willcoxon :adw]

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

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2323e6b411c1

Comment 23

[arni2033]

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

Comment 27

[Cornel Ionce [:noni] [Hubs QA]]

Confirming the fix across platform using Firefox 47.0b4, build ID 20160509171155.