From 85ec17cdc2227a2270e9109004db1ff78575cb74 Mon Sep 17 00:00:00 2001 From: Drew Willcoxon Date: Fri, 29 Jan 2016 14:02:15 -0800 Subject: [PATCH] Bug 1233672 - Properly encode/decode moz-action URIs. r=mak --- browser/base/content/test/general/browser.ini | 1 + .../general/browser_action_searchengine.js | 2 +- .../test/general/browser_urlbarDecode.js | 97 +++++++++++++++++++ browser/base/content/test/general/head.js | 6 +- browser/base/content/urlbarBindings.xml | 42 ++++---- toolkit/components/places/UnifiedComplete.js | 20 +++- .../unifiedcomplete/head_autocomplete.js | 12 ++- .../test_keyword_search_actions.js | 5 +- toolkit/content/widgets/autocomplete.xml | 55 +++++++---- 9 files changed, 188 insertions(+), 52 deletions(-) create mode 100644 browser/base/content/test/general/browser_urlbarDecode.js diff --git a/browser/base/content/test/general/browser.ini b/browser/base/content/test/general/browser.ini index 3a4b46eaf430..f942d843d634 100644 --- a/browser/base/content/test/general/browser.ini +++ b/browser/base/content/test/general/browser.ini @@ -470,6 +470,7 @@ skip-if = e10s # Bug 1100700 - test relies on unload event firing on closed tabs [browser_urlHighlight.js] [browser_urlbarAutoFillTrimURLs.js] [browser_urlbarCopying.js] +[browser_urlbarDecode.js] [browser_urlbarDelete.js] [browser_urlbarEnter.js] [browser_urlbarEnterAfterMouseOver.js] diff --git a/browser/base/content/test/general/browser_action_searchengine.js b/browser/base/content/test/general/browser_action_searchengine.js index 3ccf23370808..6d87a87bf8f9 100644 --- a/browser/base/content/test/general/browser_action_searchengine.js +++ b/browser/base/content/test/general/browser_action_searchengine.js @@ -35,7 +35,7 @@ add_task(function* () { isnot(result, null, "Should have a result"); is(result.getAttribute("url"), - `moz-action:searchengine,{"engineName":"MozSearch","input":"open a search","searchQuery":"open a search"}`, + `moz-action:searchengine,{"engineName":"MozSearch","input":"open%20a%20search","searchQuery":"open%20a%20search"}`, "Result should be a moz-action: for the correct search engine"); is(result.hasAttribute("image"), false, "Result shouldn't have an image attribute"); diff --git a/browser/base/content/test/general/browser_urlbarDecode.js b/browser/base/content/test/general/browser_urlbarDecode.js new file mode 100644 index 000000000000..8577e056d598 --- /dev/null +++ b/browser/base/content/test/general/browser_urlbarDecode.js @@ -0,0 +1,97 @@ +"use strict"; + +// This test makes sure (1) you can't break the urlbar by typing particular JSON +// or JS fragments into it, (2) urlbar.textValue shows URLs unescaped, and (3) +// the urlbar also shows the URLs embedded in action URIs unescaped. See bug +// 1233672. + +add_task(function* injectJSON() { + let inputStrs = [ + 'http://example.com/ ", "url": "bar' , + 'http://example.com/\\' , + 'http://example.com/"' , + 'http://example.com/","url":"evil.com' , + 'http://mozilla.org/\\u0020' , + 'http://www.mozilla.org/","url":1e6,"some-key":"foo' , + 'http://www.mozilla.org/","url":null,"some-key":"foo' , + 'http://www.mozilla.org/","url":["foo","bar"],"some-key":"foo' , + ]; + for (let inputStr of inputStrs) { + yield checkInput(inputStr); + } + gURLBar.value = ""; + gURLBar.handleRevert(); + gURLBar.blur(); +}); + +add_task(function losslessDecode() { + let url = "http://example.com/\u30a2\u30a4\u30a6\u30a8\u30aa"; + gURLBar.textValue = url; + Assert.equal(gURLBar.inputField.value, url, + "The string displayed in the textbox should not be escaped"); + gURLBar.value = ""; + gURLBar.handleRevert(); + gURLBar.blur(); +}); + +add_task(function* actionURILosslessDecode() { + let urlNoScheme = "example.com/\u30a2\u30a4\u30a6\u30a8\u30aa"; + let url = "http://" + urlNoScheme; + yield promiseAutocompleteResultPopup(url); + + // At this point the heuristic result is selected but the urlbar's value is + // simply `url`. Key down and back around until the heuristic result is + // selected again, and at that point the urlbar's value should be a visiturl + // moz-action. + + do { + gURLBar.controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN); + } while (gURLBar.popup.selectedIndex != 0); + + let [, type, params] = gURLBar.value.match(/^moz-action:([^,]+),(.*)$/); + Assert.equal(type, "visiturl", + "visiturl action URI should be in the urlbar"); + + Assert.equal(gURLBar.inputField.value, urlNoScheme, + "The string displayed in the textbox should not be escaped"); + + gURLBar.value = ""; + gURLBar.handleRevert(); + gURLBar.blur(); +}); + +function* checkInput(inputStr) { + yield promiseAutocompleteResultPopup(inputStr); + + let item = gURLBar.popup.richlistbox.firstChild; + Assert.ok(item, "Should have a result"); + + // visiturl matches have their param.urls fixed up. + let fixupInfo = Services.uriFixup.getFixupURIInfo(inputStr, + Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS | + Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP + ); + let expectedVisitURL = fixupInfo.fixedURI.spec; + + let type = "visiturl"; + let params = { + url: expectedVisitURL, + input: inputStr, + }; + for (let key in params) { + params[key] = encodeURIComponent(params[key]); + } + let expectedURL = "moz-action:" + type + "," + JSON.stringify(params); + Assert.equal(item.getAttribute("url"), expectedURL, "url"); + + Assert.equal(item.getAttribute("title"), inputStr, "title"); + Assert.equal(item.getAttribute("text"), inputStr, "text"); + + let itemTypeStr = item.getAttribute("type"); + let itemTypes = itemTypeStr.split(" ").sort(); + Assert.equal(itemTypes.toString(), + ["action", "heuristic", "visiturl"].toString(), + "type"); + + Assert.equal(item._title.textContent, "Visit " + inputStr, "Visible title"); +} diff --git a/browser/base/content/test/general/head.js b/browser/base/content/test/general/head.js index 2f20399e19a1..184b95f8b0c4 100644 --- a/browser/base/content/test/general/head.js +++ b/browser/base/content/test/general/head.js @@ -916,7 +916,11 @@ function assertMixedContentBlockingState(tabbrowser, states = {}) { } function makeActionURI(action, params) { - let url = "moz-action:" + action + "," + JSON.stringify(params); + let encodedParams = {}; + for (let key in params) { + encodedParams[key] = encodeURIComponent(params[key]); + } + let url = "moz-action:" + action + "," + JSON.stringify(encodedParams); return NetUtil.newURI(url); } diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml index 48a147034be6..a4beaf8fccef 100644 --- a/browser/base/content/urlbarBindings.xml +++ b/browser/base/content/urlbarBindings.xml @@ -159,7 +159,7 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. case "switchtab": // Fall through. case "remotetab": // Fall through. case "visiturl": { - returnValue = action.params.url; + returnValue = action.params.displayUrl; break; } case "keyword": // Fall through. @@ -351,9 +351,9 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. if (this.hasAttribute("actiontype")) { this.handleRevert(); let prevTab = gBrowser.selectedTab; - if (switchToTabHavingURI(action.params.originalUrl || url) && - isTabEmpty(prevTab)) + if (switchToTabHavingURI(url) && isTabEmpty(prevTab)) { gBrowser.removeTab(prevTab); + } return; } } else if (action.type == "remotetab") { @@ -799,15 +799,10 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. } catch (ex) {} if (uri) { - let action = this._parseActionUrl(val); - if (action) { - if (action.params.url) { - // Store the original URL in the action URL. - action.params.originalUrl = action.params.url; - action.params.url = losslessDecodeURI(makeURI(action.params.url)); - val = "moz-action:" + action.type + "," + JSON.stringify(action.params); - } - } else { + // Do not touch moz-action URIs at all. They depend on being + // properly encoded and decoded and will break if decoded + // unexpectedly. + if (!this._parseActionUrl(val)) { val = losslessDecodeURI(uri); } } @@ -856,6 +851,9 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. try { action.params = JSON.parse(params); + for (let key in action.params) { + action.params[key] = decodeURIComponent(action.params[key]); + } } catch (e) { // If this failed, we assume that params is not a JSON object, and // is instead just a flat string. This will happen when @@ -863,18 +861,16 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. // a URL. action.params = { url: params, - } - return action; + }; } - for (let key of [ - "engineName", - "input", - "searchQuery", - "searchSuggestion", - ]) { - if (action.params[key]) { - action.params[key] = decodeURIComponent(action.params[key]); + if ("url" in action.params) { + let uri; + try { + uri = makeURI(action.params.url); + action.params.displayUrl = losslessDecodeURI(uri); + } catch (e) { + action.params.displayUrl = action.params.url; } } @@ -1506,7 +1502,7 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. case "remotetab": parts = [ item.getAttribute("title"), - action.params.url, + item.getAttribute("displayurl"), ]; break; } diff --git a/toolkit/components/places/UnifiedComplete.js b/toolkit/components/places/UnifiedComplete.js index 47ce40e805b8..c0e65dbec30a 100644 --- a/toolkit/components/places/UnifiedComplete.js +++ b/toolkit/components/places/UnifiedComplete.js @@ -582,9 +582,11 @@ function stripHttpAndTrim(spec) { * @return String representation of the built moz-action: URL */ function makeActionURL(action, params) { - let url = "moz-action:" + action + "," + JSON.stringify(params); - // Make a nsIURI out of this to ensure it's encoded properly. - return NetUtil.newURI(url).spec; + let encodedParams = {}; + for (let key in params) { + encodedParams[key] = encodeURIComponent(params[key]); + } + return "moz-action:" + action + "," + JSON.stringify(encodedParams); } /** @@ -1307,14 +1309,22 @@ Search.prototype = { return false; } + // getFixupURIInfo() escaped the URI, so it may not be pretty. Embed the + // escaped URL in the action URI since that URL should be "canonical". But + // pass the pretty, unescaped URL as the match comment, since it's likely + // to be displayed to the user, and in any case the front-end should not + // rely on it being canonical. + let escapedURL = uri.spec; + let displayURL = textURIService.unEscapeURIForUI("UTF-8", uri.spec); + let value = makeActionURL("visiturl", { - url: uri.spec, + url: escapedURL, input: this._originalSearchString, }); let match = { value: value, - comment: uri.spec, + comment: displayURL, style: "action visiturl", frecency: 0, }; diff --git a/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js b/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js index 7551e83134ae..138b417a03f2 100644 --- a/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js +++ b/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js @@ -346,7 +346,11 @@ function stripPrefix(spec) } function makeActionURI(action, params) { - let url = "moz-action:" + action + "," + JSON.stringify(params); + let encodedParams = {}; + for (let key in params) { + encodedParams[key] = encodeURIComponent(params[key]); + } + let url = "moz-action:" + action + "," + JSON.stringify(encodedParams); return NetUtil.newURI(url); } @@ -360,7 +364,11 @@ function makeSearchMatch(input, extra = {}) { engineName: extra.engineName || "MozSearch", input, searchQuery: "searchQuery" in extra ? extra.searchQuery : input, - alias: extra.alias, // may be undefined which is expected. + }; + if ("alias" in extra) { + // May be undefined, which is expected, but in that case make sure it's not + // included in the params of the moz-action URL. + params.alias = extra.alias; } let style = [ "action", "searchengine" ]; if (extra.heuristic) { diff --git a/toolkit/components/places/tests/unifiedcomplete/test_keyword_search_actions.js b/toolkit/components/places/tests/unifiedcomplete/test_keyword_search_actions.js index 51c710ab08c2..0a0759cd67da 100644 --- a/toolkit/components/places/tests/unifiedcomplete/test_keyword_search_actions.js +++ b/toolkit/components/places/tests/unifiedcomplete/test_keyword_search_actions.js @@ -43,10 +43,13 @@ add_task(function* test_keyword_search() { }); do_print("Unescaped term in query"); + // ... but note that UnifiedComplete calls encodeURIComponent() on the query + // string when it builds the URL, so the expected result will have the + // ユニコード substring encoded in the URL. yield check_autocomplete({ search: "key ユニコード", searchParam: "enable-actions", - matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=ユニコード", input: "key ユニコード"}), title: "abc", style: [ "action", "keyword", "heuristic" ] } ] + matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=" + encodeURIComponent("ユニコード"), input: "key ユニコード"}), title: "abc", style: [ "action", "keyword", "heuristic" ] } ] }); do_print("Keyword that happens to match a page"); diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml index 027eb6fdcd06..c3b483d34a89 100644 --- a/toolkit/content/widgets/autocomplete.xml +++ b/toolkit/content/widgets/autocomplete.xml @@ -1228,10 +1228,7 @@ extends="chrome://global/content/bindings/popup.xml#popup"> // trim the leading/trailing whitespace var trimmedSearchString = controller.searchString.replace(/^\s+/, "").replace(/\s+$/, ""); - // Unescape the URI spec for showing as an entry in the popup - let url = Components.classes["@mozilla.org/intl/texttosuburi;1"]. - getService(Components.interfaces.nsITextToSubURI). - unEscapeURIForUI("UTF-8", controller.getValueAt(this._currentIndex)); + let url = controller.getValueAt(this._currentIndex); if (this._currentIndex < existingItemsCount) { // re-use the existing item @@ -1402,7 +1399,7 @@ extends="chrome://global/content/bindings/popup.xml#popup"> let parts = [ this.getAttribute("title"), - this.getAttribute("url"), + this.getAttribute("displayurl"), ]; let label = parts.filter(str => str).join(" ") @@ -1672,19 +1669,29 @@ extends="chrome://global/content/bindings/popup.xml#popup"> + null + + + + + + + // If the type includes an action, set up the item appropriately. if (initialTypes.has("action")) { - let action = this._parseActionUrl(url); + let action = this._parseActionUrl(originalUrl); this.setAttribute("actiontype", action.type); if (action.type == "switchtab") { this.classList.add("overridable-action"); - displayUrl = action.params.url; + displayUrl = this._unescapeUrl(action.params.url); let desc = this._stringBundle.GetStringFromName("switchToTab"); this._setUpDescription(this._action, desc, true); } else if (action.type == "remotetab") { - displayUrl = action.params.url; + displayUrl = this._unescapeUrl(action.params.url); let desc = action.params.deviceName; this._setUpDescription(this._action, desc, true); } else if (action.type == "searchengine") { @@ -1781,14 +1788,12 @@ extends="chrome://global/content/bindings/popup.xml#popup"> } } else if (action.type == "visiturl") { emphasiseUrl = false; - displayUrl = action.params.url; - + displayUrl = this._unescapeUrl(action.params.url); let sourceStr = this._stringBundle.GetStringFromName("visitURL"); title = this._generateEmphasisPairs(sourceStr, [ [displayUrl, "match"], ]); } - } // Check if we have a search engine name @@ -1802,6 +1807,15 @@ extends="chrome://global/content/bindings/popup.xml#popup"> displayUrl = this._stringBundle.formatStringFromName("searchWithEngine", [searchEngine], 1); } + if (!displayUrl) { + let input = this.parentNode.parentNode.input; + let url = typeof(input.trimValue) == "function" ? + input.trimValue(originalUrl) : + originalUrl; + displayUrl = this._unescapeUrl(url); + } + this.setAttribute("displayurl", displayUrl); + // Check if we have an auto-fill URL if (initialTypes.has("autofill")) { emphasiseUrl = false; @@ -1884,7 +1898,7 @@ extends="chrome://global/content/bindings/popup.xml#popup"> if (title == "") { title = displayUrl; try { - let uri = Services.io.newURI(url, null, null); + let uri = Services.io.newURI(originalUrl, null, null); // Not all valid URLs have a domain. if (uri.host) title = uri.host; @@ -1923,6 +1937,9 @@ extends="chrome://global/content/bindings/popup.xml#popup"> try { action.params = JSON.parse(params); + for (let key in action.params) { + action.params[key] = decodeURIComponent(action.params[key]); + } } catch (e) { // If this failed, we assume that params is not a JSON object, and // is instead just a flat string. This will happen when