Bug 1339497 - Location bar may suggest an incorrect url when input url contains % encoded components. r=standard8

MozReview-Commit-ID: 7dmtSN41H71 --HG-- extra : rebase_source : b0596872d16dfd747064819e745956468740b591
2017-02-21 18:50:16 +01:00 · 2017-02-21 18:50:16 +01:00 · c05b0ff17d
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@ -585,18 +585,6 @@ XPCOMUtils.defineLazyGetter(this, "ProfileAgeCreatedPromise", () => {

 // Helper functions

-/**
- * Used to unescape encoded URI strings and drop information that we do not
- * care about.
- *
- * @param spec
- *        The text to unescape and modify.
- * @return the modified spec.
- */
-function fixupSearchText(spec) {
-  return textURIService.unEscapeURIForUI("UTF-8", stripPrefix(spec));
-}
-
 /**
 * Generates the tokens used in searching from a given string.
 *
@ -640,16 +628,18 @@ function stripPrefix(spec) {
 *
 * @param spec
 *        The text to modify.
+ * @param trimSlash
+ *        Whether to trim the trailing slash.
 * @return the modified spec.
 */
-function stripHttpAndTrim(spec) {
+function stripHttpAndTrim(spec, trimSlash = true) {
  if (spec.startsWith("http://")) {
    spec = spec.slice(7);
  }
  if (spec.endsWith("?")) {
    spec = spec.slice(0, -1);
  }
-  if (spec.endsWith("/")) {
+  if (trimSlash && spec.endsWith("/")) {
    spec = spec.slice(0, -1);
  }
  return spec;
@ -742,7 +732,16 @@ function Search(searchString, searchParam, autocompleteListener,
  // We want to store the original string for case sensitive searches.
  this._originalSearchString = searchString;
  this._trimmedOriginalSearchString = searchString.trim();
-  this._searchString = fixupSearchText(this._trimmedOriginalSearchString.toLowerCase());
+  let strippedOriginalSearchString =
+    stripPrefix(this._trimmedOriginalSearchString.toLowerCase());
+  this._searchString =
+    textURIService.unEscapeURIForUI("UTF-8", strippedOriginalSearchString);
+
+  // The protocol and the host are lowercased by nsIURI, so it's fine to
+  // lowercase the typed prefix, to add it back to the results later.
+  this._strippedPrefix = this._trimmedOriginalSearchString.slice(
+    0, this._trimmedOriginalSearchString.length - strippedOriginalSearchString.length
+  ).toLowerCase();

  this._matchBehavior = Prefs.matchBehavior;
  // Set the default behavior for this search.
@ -762,19 +761,6 @@ function Search(searchString, searchParam, autocompleteListener,

  this._searchTokens =
    this.filterTokens(getUnfilteredSearchTokens(this._searchString));
-  // The protocol and the host are lowercased by nsIURI, so it's fine to
-  // lowercase the typed prefix, to add it back to the results later.
-  this._strippedPrefix = this._trimmedOriginalSearchString.slice(
-    0, this._trimmedOriginalSearchString.length - this._searchString.length
-  ).toLowerCase();
-  // The URIs in the database are fixed-up, so we can match on a lowercased
-  // host, but the path must be matched in a case sensitive way.
-  let pathIndex =
-    this._trimmedOriginalSearchString.indexOf("/", this._strippedPrefix.length);
-  this._autofillUrlSearchString = fixupSearchText(
-    this._trimmedOriginalSearchString.slice(0, pathIndex).toLowerCase() +
-    this._trimmedOriginalSearchString.slice(pathIndex)
-  );

  this._prohibitSearchSuggestions = prohibitSearchSuggestions;

@ -1089,7 +1075,6 @@ Search.prototype = {
      if (site.uri.host.startsWith(this._searchString)) {
        let match = {
          value: stripPrefix(site.uri.spec),
-          comment: site.title,
          style: "autofill",
          finalCompleteValue: site.uri.spec,
          frecency: FRECENCY_DEFAULT,
@ -1625,6 +1610,20 @@ Search.prototype = {
    if (!this.pending)
      return;

+    // For autofill entries, the comment field must be a stripped version
+    // of the final destination url, so that the user will definitely know
+    // where he is going to end up. For example, if the user is visiting a
+    // secure page, we'll leave the https on it, to let him know that.
+    // This must happen before generating the dedupe key.
+    if (match.hasOwnProperty("style") && match.style.includes("autofill")) {
+      // We fallback to match.value, as that's what autocomplete does if
+      // finalCompleteValue is null.
+      // Trim only if the value looks like a domain, we want to retain the
+      // trailing slash if we're completing a url to the next slash.
+      match.comment = stripHttpAndTrim(match.finalCompleteValue || match.value,
+                                       !this._searchString.includes("/"));
+    }
+
    // Must check both id and url, cause keywords dynamically modify the url.
    let urlMapKey = makeKeyForURL(match);
    if ((match.placeId && this._usedPlaceIds.has(match.placeId)) ||
@ -1698,28 +1697,19 @@ Search.prototype = {

  _processHostRow(row) {
    let match = {};
-    let trimmedHost = row.getResultByIndex(QUERYINDEX_URL);
-    let untrimmedHost = row.getResultByIndex(QUERYINDEX_TITLE);
+    let strippedHost = row.getResultByIndex(QUERYINDEX_URL);
+    let unstrippedHost = row.getResultByIndex(QUERYINDEX_TITLE);
    let frecency = row.getResultByIndex(QUERYINDEX_FRECENCY);
    let faviconUrl = row.getResultByIndex(QUERYINDEX_ICONURL);

-    // If the untrimmed value doesn't preserve the user's input just
+    // If the unfixup value doesn't preserve the user's input just
    // ignore it and complete to the found host.
-    if (untrimmedHost &&
-        !untrimmedHost.toLowerCase().includes(this._trimmedOriginalSearchString.toLowerCase())) {
-      untrimmedHost = null;
+    if (!unstrippedHost.toLowerCase().includes(this._trimmedOriginalSearchString.toLowerCase())) {
+      unstrippedHost = null;
    }

-    match.value = this._strippedPrefix + trimmedHost;
-    match.finalCompleteValue = untrimmedHost;
-
-    // The comment should be the user's final destination so that the user
-    // will definitely know where he is going to end up. For example, if the
-    // user is visiting a secure page, we'll leave the https on it, so that
-    // they know it'll be secure.
-    // We fallback to match.value, as that's what autocomplete does if
-    // finalCompleteValue is null.
-    match.comment = stripHttpAndTrim(match.finalCompleteValue || match.value);
+    match.value = this._strippedPrefix + strippedHost;
+    match.finalCompleteValue = unstrippedHost;

    if (faviconUrl) {
      match.icon = PlacesUtils.favicons
@ -1733,44 +1723,43 @@ Search.prototype = {
  },

  _processUrlRow(row) {
-    let match = {};
-    let value = row.getResultByIndex(QUERYINDEX_URL);
-    let url = fixupSearchText(value);
+    let url = row.getResultByIndex(QUERYINDEX_URL);
+    let strippedUrl = stripPrefix(url);
+    let prefix = url.substr(0, url.length - strippedUrl.length);
    let frecency = row.getResultByIndex(QUERYINDEX_FRECENCY);
    let faviconUrl = row.getResultByIndex(QUERYINDEX_ICONURL);

-    let prefix = value.slice(0, value.length - stripPrefix(value).length);
-
    // We must complete the URL up to the next separator (which is /, ? or #).
-    let separatorIndex = url.slice(this._searchString.length)
-                            .search(/[\/\?\#]/);
+    let searchString = stripPrefix(this._trimmedOriginalSearchString);
+    let separatorIndex = strippedUrl.slice(searchString.length)
+                                    .search(/[\/\?\#]/);
    if (separatorIndex != -1) {
-      separatorIndex += this._searchString.length;
-      if (url[separatorIndex] == "/") {
+      separatorIndex += searchString.length;
+      if (strippedUrl[separatorIndex] == "/") {
        separatorIndex++; // Include the "/" separator
      }
-      url = url.slice(0, separatorIndex);
+      strippedUrl = strippedUrl.slice(0, separatorIndex);
    }

-    // If the untrimmed value doesn't preserve the user's input just
-    // ignore it and complete to the found url.
-    let untrimmedURL = prefix + url;
-    if (untrimmedURL &&
-        !untrimmedURL.toLowerCase().includes(this._trimmedOriginalSearchString.toLowerCase())) {
-      untrimmedURL = null;
-     }
+    let match = {
+      value: this._strippedPrefix + strippedUrl,
+      // Although this has a frecency, this query is executed before any other
+      // queries that would result in frecency matches.
+      frecency,
+      style: "autofill"
+    };

-    match.value = this._strippedPrefix + url;
-    match.comment = url;
-    match.finalCompleteValue = untrimmedURL;
    if (faviconUrl) {
      match.icon = PlacesUtils.favicons
                              .getFaviconLinkForIcon(NetUtil.newURI(faviconUrl)).spec;
    }
-    // Although this has a frecency, this query is executed before any other
-    // queries that would result in frecency matches.
-    match.frecency = frecency;
-    match.style = "autofill";
+
+    // Complete to the found url only if its untrimmed value preserves the
+    // user's input.
+    if (url.toLowerCase().includes(this._trimmedOriginalSearchString.toLowerCase())) {
+      match.finalCompleteValue = prefix + strippedUrl;
+    }
+
    return match;
  },

@ -2019,9 +2008,16 @@ Search.prototype = {
    // We expect this to be a full URL, not just a host. We want to extract the
    // host and use that as a guess for whether we'll get a result from a URL
    // query.
-    let slashIndex = this._autofillUrlSearchString.indexOf("/");
-    let revHost = this._autofillUrlSearchString.substring(0, slashIndex).toLowerCase()
-                      .split("").reverse().join("") + ".";
+    // The URIs in the database are fixed-up, so we can match on a lowercased
+    // host, but the path must be matched in a case sensitive way.
+    let pathIndex = this._trimmedOriginalSearchString.indexOf("/", this._strippedPrefix.length);
+    let revHost = this._trimmedOriginalSearchString
+                      .substring(this._strippedPrefix.length, pathIndex)
+                      .toLowerCase().split("").reverse().join("") + ".";
+    let searchString = stripPrefix(
+      this._trimmedOriginalSearchString.slice(0, pathIndex).toLowerCase() +
+      this._trimmedOriginalSearchString.slice(pathIndex)
+    );

    let typed = Prefs.autofillTyped || this.hasBehavior("typed");
    let bookmarked = this.hasBehavior("bookmark") && !this.hasBehavior("history");
@ -2037,7 +2033,7 @@ Search.prototype = {

    query.push({
      query_type: QUERYTYPE_AUTOFILL_URL,
-      searchString: this._autofillUrlSearchString,
+      searchString,
      revHost
    });

--- a/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
+++ b/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@ -374,7 +374,7 @@ function makeSearchMatch(input, extra = {}) {
    params.alias = extra.alias;
  }
  let style = [ "action", "searchengine" ];
-  if (Array.isArray(extra.style)) {
+  if ("style" in extra && Array.isArray(extra.style)) {
    style.push(...extra.style);
  }
  if (extra.heuristic) {
--- a/toolkit/components/places/tests/unifiedcomplete/test_encoded_urls.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_encoded_urls.js
@ -0,0 +1,71 @@
+add_task(function* test_encoded() {
+  do_print("Searching for over encoded url should not break it");
+  yield PlacesTestUtils.addVisits({
+    uri: NetUtil.newURI("https://www.mozilla.com/search/top/?q=%25%32%35"),
+    title: "https://www.mozilla.com/search/top/?q=%25%32%35",
+    transition: TRANSITION_TYPED
+  });
+  yield check_autocomplete({
+    search: "https://www.mozilla.com/search/top/?q=%25%32%35",
+    matches: [ { uri: NetUtil.newURI("https://www.mozilla.com/search/top/?q=%25%32%35"),
+                 title: "https://www.mozilla.com/search/top/?q=%25%32%35",
+                 style: [ "autofill", "heuristic" ] }],
+    autofilled: "https://www.mozilla.com/search/top/?q=%25%32%35",
+    completed: "https://www.mozilla.com/search/top/?q=%25%32%35"
+  });
+  yield cleanup();
+});
+
+add_task(function* test_encoded_trimmed() {
+  do_print("Searching for over encoded url should not break it");
+  yield PlacesTestUtils.addVisits({
+    uri: NetUtil.newURI("https://www.mozilla.com/search/top/?q=%25%32%35"),
+    title: "https://www.mozilla.com/search/top/?q=%25%32%35",
+    transition: TRANSITION_TYPED
+  });
+  yield check_autocomplete({
+    search: "mozilla.com/search/top/?q=%25%32%35",
+    matches: [ { uri: NetUtil.newURI("https://www.mozilla.com/search/top/?q=%25%32%35"),
+                 title: "https://www.mozilla.com/search/top/?q=%25%32%35",
+                 style: [ "autofill", "heuristic" ] }],
+    autofilled: "mozilla.com/search/top/?q=%25%32%35",
+    completed: "https://www.mozilla.com/search/top/?q=%25%32%35"
+  });
+  yield cleanup();
+});
+
+add_task(function* test_encoded_partial() {
+  do_print("Searching for over encoded url should not break it");
+  yield PlacesTestUtils.addVisits({
+    uri: NetUtil.newURI("https://www.mozilla.com/search/top/?q=%25%32%35"),
+    title: "https://www.mozilla.com/search/top/?q=%25%32%35",
+    transition: TRANSITION_TYPED
+  });
+  yield check_autocomplete({
+    search: "https://www.mozilla.com/search/top/?q=%25",
+    matches: [ { uri: NetUtil.newURI("https://www.mozilla.com/search/top/?q=%25%32%35"),
+                 title: "https://www.mozilla.com/search/top/?q=%25%32%35",
+                 style: [ "autofill", "heuristic" ] }],
+    autofilled: "https://www.mozilla.com/search/top/?q=%25%32%35",
+    completed: "https://www.mozilla.com/search/top/?q=%25%32%35"
+  });
+  yield cleanup();
+});
+
+add_task(function* test_encoded_path() {
+  do_print("Searching for over encoded url should not break it");
+  yield PlacesTestUtils.addVisits({
+    uri: NetUtil.newURI("https://www.mozilla.com/%25%32%35/top/"),
+    title: "https://www.mozilla.com/%25%32%35/top/",
+    transition: TRANSITION_TYPED
+  });
+  yield check_autocomplete({
+    search: "https://www.mozilla.com/%25%32%35/t",
+    matches: [ { uri: NetUtil.newURI("https://www.mozilla.com/%25%32%35/top/"),
+                 title: "https://www.mozilla.com/%25%32%35/top/",
+                 style: [ "autofill", "heuristic" ] }],
+    autofilled: "https://www.mozilla.com/%25%32%35/top/",
+    completed: "https://www.mozilla.com/%25%32%35/top/"
+  });
+  yield cleanup();
+});
--- a/toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
+++ b/toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
@ -22,6 +22,7 @@ support-files =
 [test_dupe_urls.js]
 [test_empty_search.js]
 [test_enabled.js]
+[test_encoded_urls.js]
 [test_escape_self.js]
 [test_extension_matches.js]
 [test_ignore_protocol.js]