Bug 1233672 - Properly encode/decode moz-action URIs. r=mak

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]

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");
 

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");
+}

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);
 }
 

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) &&
+                if (switchToTabHavingURI(url) && isTabEmpty(prevTab)) {
-                    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);
+            // Do not touch moz-action URIs at all.  They depend on being
-            if (action) {
+            // properly encoded and decoded and will break if decoded
-              if (action.params.url) {
+            // unexpectedly.
-                // Store the original URL in the action URL.
+            if (!this._parseActionUrl(val)) {
-                action.params.originalUrl = action.params.url;
-                action.params.url = losslessDecodeURI(makeURI(action.params.url));
-                val = "moz-action:" + action.type + "," + JSON.stringify(action.params);
-              }
-            } else {
               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 [
+          if ("url" in action.params) {
-            "engineName",
+            let uri;
-            "input",
+            try {
-            "searchQuery",
+              uri = makeURI(action.params.url);
-            "searchSuggestion",
+              action.params.displayUrl = losslessDecodeURI(uri);
-          ]) {
+            } catch (e) {
-            if (action.params[key]) {
+              action.params.displayUrl = action.params.url;
-              action.params[key] = decodeURIComponent(action.params[key]);
             }
           }
 
@@ -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;
               }

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);
+  let encodedParams = {};
-  // Make a nsIURI out of this to ensure it's encoded properly.
+  for (let key in params) {
-  return NetUtil.newURI(url).spec;
+    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,
     };

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) {

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");

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 = controller.getValueAt(this._currentIndex);
-            let url = Components.classes["@mozilla.org/intl/texttosuburi;1"].
-              getService(Components.interfaces.nsITextToSubURI).
-              unEscapeURIForUI("UTF-8", 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">
         </body>
       </method>
 
+      <field name="_textToSubURI">null</field>
+      <method name="_unescapeUrl">
+        <parameter name="url"/>
+        <body>
+          <![CDATA[
+          if (!this._textToSubURI) {
+            this._textToSubURI =
+              Components.classes["@mozilla.org/intl/texttosuburi;1"]
+                        .getService(Components.interfaces.nsITextToSubURI);
+          }
+          return this._textToSubURI.unEscapeURIForUI("UTF-8", url);
+          ]]>
+        </body>
+      </method>
+
       <method name="_adjustAcItem">
         <body>
           <![CDATA[
-          let url = this.getAttribute("url");
+          let originalUrl = this.getAttribute("url");
           let title = this.getAttribute("title");
           let type = this.getAttribute("type");
 
-          let displayUrl = url;
+          let displayUrl;
-
-          let input = this.parentNode.parentNode.input;
-          if (typeof input.trimValue == "function")
-            displayUrl = input.trimValue(url);
-
           let emphasiseTitle = true;
           let emphasiseUrl = true;
 
@@ -1716,16 +1723,16 @@ extends="chrome://global/content/bindings/popup.xml#popup">
 
           // 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