Bug 1184960 - Limit the usable text to get search suggestions. r=Mossop
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 28 Jul 2015 10:21:40 +0200
changeset 254844 4be4031b55e3a399023cdb164f88f51534ca7542
parent 254843 2316676f94d54df4a7651c7ede57fa255439f23a
child 254845 07132b9fbc109dbccf3e9bd967809b9e335aa64e
child 255010 9727cedcfa9b94867a2962161f579b7d693290ef
push id29122
push usercbook@mozilla.com
push dateTue, 28 Jul 2015 14:13:05 +0000
treeherdermozilla-central@07132b9fbc10 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMossop
bugs1184960
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1184960 - Limit the usable text to get search suggestions. r=Mossop
browser/app/profile/firefox.js
toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
toolkit/components/places/UnifiedComplete.js
toolkit/components/places/tests/unifiedcomplete/test_searchSuggestions.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -349,16 +349,20 @@ pref("browser.urlbar.suggest.history",  
 pref("browser.urlbar.suggest.bookmark",             true);
 pref("browser.urlbar.suggest.openpage",             true);
 #ifdef NIGHTLY_BUILD
 pref("browser.urlbar.suggest.searches",             true);
 #else
 pref("browser.urlbar.suggest.searches",             false);
 #endif
 
+// Limit the number of characters sent to the current search engine to fetch
+// suggestions.
+pref("browser.urlbar.maxCharsForSearchSuggestions", 20);
+
 // Restrictions to current suggestions can also be applied (intersection).
 // Typed suggestion works only if history is set to true.
 pref("browser.urlbar.suggest.history.onlyTyped",    false);
 
 pref("browser.urlbar.formatting.enabled", true);
 pref("browser.urlbar.trimURLs", true);
 
 pref("browser.altClickSave", false);
--- a/toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
+++ b/toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
@@ -128,17 +128,19 @@ const SearchAutocompleteProviderInternal
 
 function SearchSuggestionControllerWrapper(engine, searchToken,
                                            inPrivateContext, maxResults) {
   this._controller = new SearchSuggestionController();
   this._controller.maxLocalResults = 0;
   this._controller.maxRemoteResults = maxResults;
   let promise = this._controller.fetch(searchToken, inPrivateContext, engine);
   this._suggestions = [];
+  this._success = false;
   this._promise = promise.then(results => {
+    this._success = true;
     this._suggestions = (results ? results.remote : null) || [];
   }).catch(err => {
     // fetch() rejects its promise if there's a pending request.
   });
 }
 
 SearchSuggestionControllerWrapper.prototype = {
 
@@ -160,16 +162,24 @@ SearchSuggestionControllerWrapper.protot
    */
   consume() {
     return !this._suggestions.length ? [null, null] :
            [SearchAutocompleteProviderInternal.defaultMatch,
             this._suggestions.shift()];
   },
 
   /**
+   * Returns the number of fetched suggestions, or -1 if the fetching was
+   * incomplete or failed.
+   */
+  get resultsCount() {
+    return this._success ? this._suggestions.length : -1;
+  },
+
+  /**
    * Stops the fetch.
    */
   stop() {
     this._controller.stop();
   },
 };
 
 let gInitializationPromise = null;
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -33,16 +33,18 @@ const PREF_MATCH_TITLE =            [ "m
 const PREF_MATCH_URL =              [ "match.url",              "@" ];
 
 const PREF_SUGGEST_HISTORY =        [ "suggest.history",        true ];
 const PREF_SUGGEST_BOOKMARK =       [ "suggest.bookmark",       true ];
 const PREF_SUGGEST_OPENPAGE =       [ "suggest.openpage",       true ];
 const PREF_SUGGEST_HISTORY_ONLYTYPED = [ "suggest.history.onlyTyped", false ];
 const PREF_SUGGEST_SEARCHES =       [ "suggest.searches",       true ];
 
+const PREF_MAX_CHARS_FOR_SUGGEST =  [ "maxCharsForSearchSuggestions", 20];
+
 // Match type constants.
 // These indicate what type of search function we should be using.
 const MATCH_ANYWHERE = Ci.mozIPlacesAutoComplete.MATCH_ANYWHERE;
 const MATCH_BOUNDARY_ANYWHERE = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY_ANYWHERE;
 const MATCH_BOUNDARY = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY;
 const MATCH_BEGINNING = Ci.mozIPlacesAutoComplete.MATCH_BEGINNING;
 const MATCH_BEGINNING_CASE_SENSITIVE = Ci.mozIPlacesAutoComplete.MATCH_BEGINNING_CASE_SENSITIVE;
 
@@ -411,16 +413,17 @@ XPCOMUtils.defineLazyGetter(this, "Prefs
     store.restrictSearchesToken = prefs.get(...PREF_RESTRICT_SEARCHES);
     store.matchTitleToken = prefs.get(...PREF_MATCH_TITLE);
     store.matchURLToken = prefs.get(...PREF_MATCH_URL);
     store.suggestHistory = prefs.get(...PREF_SUGGEST_HISTORY);
     store.suggestBookmark = prefs.get(...PREF_SUGGEST_BOOKMARK);
     store.suggestOpenpage = prefs.get(...PREF_SUGGEST_OPENPAGE);
     store.suggestTyped = prefs.get(...PREF_SUGGEST_HISTORY_ONLYTYPED);
     store.suggestSearches = prefs.get(...PREF_SUGGEST_SEARCHES);
+    store.maxCharsForSearchSuggestions = prefs.get(...PREF_MAX_CHARS_FOR_SUGGEST);
 
     // If history is not set, onlyTyped value should be ignored.
     if (!store.suggestHistory) {
       store.suggestTyped = false;
     }
     store.defaultBehavior = types.concat("Typed").reduce((memo, type) => {
       let prefValue = store["suggest" + type];
       return memo | (prefValue &&
@@ -593,19 +596,21 @@ function makeActionURL(action, params) {
  *          possibly in permanent private-browsing mode.  The search
  *          should exclude privacy-sensitive results as appropriate.
  * @param autocompleteListener
  *        An nsIAutoCompleteObserver.
  * @param resultListener
  *        An nsIAutoCompleteSimpleResultListener.
  * @param autocompleteSearch
  *        An nsIAutoCompleteSearch.
+ * @param prohibitSearchSuggestions
+ *        Whether search suggestions are allowed for this search.
  */
 function Search(searchString, searchParam, autocompleteListener,
-                resultListener, autocompleteSearch) {
+                resultListener, autocompleteSearch, prohibitSearchSuggestions) {
   // We want to store the original string for case sensitive searches.
   this._originalSearchString = searchString;
   this._trimmedOriginalSearchString = searchString.trim();
   this._searchString = fixupSearchText(this._trimmedOriginalSearchString.toLowerCase());
 
   this._matchBehavior = Prefs.matchBehavior;
   // Set the default behavior for this search.
   this._behavior = this._searchString ? Prefs.defaultBehavior
@@ -627,16 +632,18 @@ function Search(searchString, searchPara
   // 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;
+
   this._listener = autocompleteListener;
   this._autocompleteSearch = autocompleteSearch;
 
   // Create a new result to add eventual matches.  Note we need a result
   // regardless having matches.
   let result = Cc["@mozilla.org/autocomplete/simple-result;1"]
                  .createInstance(Ci.nsIAutoCompleteSimpleResult);
   result.setSearchString(searchString);
@@ -904,28 +911,39 @@ Search.prototype = {
       }
     }
 
     // Ensure to fill any remaining space.
     yield Promise.all(this._remoteMatchesPromises);
   }),
 
   *_matchSearchSuggestions() {
-    if (!this.hasBehavior("searches") || this._inPrivateWindow) {
+    // Limit the string sent for search suggestions to a maximum length.
+    let searchString = this._searchTokens.join(" ")
+                           .substr(0, Prefs.maxCharsForSearchSuggestions);
+    // Avoid fetching suggestions if they are not required, private browsing
+    // mode is enabled, or the search string may expose sensitive information.
+    if (!this.hasBehavior("searches") || this._inPrivateWindow ||
+        this._prohibitSearchSuggestionsFor(searchString)) {
       return;
     }
 
     this._searchSuggestionController =
       PlacesSearchAutocompleteProvider.getSuggestionController(
-        this._searchTokens.join(" "),
+        searchString,
         this._inPrivateWindow,
         Prefs.maxRichResults
       );
     let promise = this._searchSuggestionController.fetchCompletePromise
       .then(() => {
+        if (this._searchSuggestionController.resultsCount >= 0 &&
+            this._searchSuggestionController.resultsCount < 2) {
+          // The original string is used to properly compare with the next search.
+          this._lastLowResultsSearchSuggestion = this._originalSearchString;
+        }
         while (this.pending && this._remoteMatchesCount < Prefs.maxRichResults) {
           let [match, suggestion] = this._searchSuggestionController.consume();
           if (!suggestion)
             break;
           // Don't include the restrict token, if present.
           let searchString = this._searchTokens.join(" ");
           this._addSearchEngineMatch(match, searchString, suggestion);
         }
@@ -935,16 +953,38 @@ Search.prototype = {
       // We're done if we're restricting to search suggestions.
       yield promise;
       this.stop();
     } else {
       this._remoteMatchesPromises.push(promise);
     }
   },
 
+  _prohibitSearchSuggestionsFor(searchString) {
+    if (this._prohibitSearchSuggestions)
+      return true;
+
+    // Suggestions for a single letter are unlikely to be useful.
+    if (searchString.length < 2)
+      return true;
+
+    let tokens = searchString.split(/\s/);
+
+    // The first token may be a whitelisted host.
+    if (REGEXP_SINGLEWORD_HOST.test(tokens[0]) &&
+        Services.uriFixup.isDomainWhitelisted(tokens[0], -1))
+      return true;
+
+    // Disallow fetching search suggestions for strings looking like URLs, to
+    // avoid disclosing information about networks or passwords.
+    return tokens.some(token => {
+      return ["/", "@", ":", "."].some(c => token.includes(c));
+    });
+  },
+
   _matchKnownUrl: function* (conn) {
     // Hosts have no "/" in them.
     let lastSlashIndex = this._searchString.lastIndexOf("/");
     // Search only URLs if there's a slash in the search string...
     if (lastSlashIndex != -1) {
       // ...but not if it's exactly at the end of the search string.
       if (lastSlashIndex < this._searchString.length - 1) {
         // We don't want to execute this query right away because it needs to
@@ -1696,18 +1736,25 @@ UnifiedComplete.prototype = {
     // Stop the search in case the controller has not taken care of it.
     if (this._currentSearch) {
       this.stopSearch();
     }
 
     // Note: We don't use previousResult to make sure ordering of results are
     //       consistent.  See bug 412730 for more details.
 
+    // If the previous search didn't fetch enough search suggestions, it's
+    // unlikely a longer text would do.
+    let prohibitSearchSuggestions =
+      this._lastLowResultsSearchSuggestion &&
+      searchString.length > this._lastLowResultsSearchSuggestion.length &&
+      searchString.startsWith(this._lastLowResultsSearchSuggestion);
+
     this._currentSearch = new Search(searchString, searchParam, listener,
-                                     this, this);
+                                     this, this, prohibitSearchSuggestions);
 
     // If we are not enabled, we need to return now.  Notice we need an empty
     // result regardless, so we still create the Search object.
     if (!Prefs.enabled) {
       this.finishSearch(true);
       return;
     }
 
@@ -1740,16 +1787,17 @@ UnifiedComplete.prototype = {
    *        Indicates if we should notify the AutoComplete listener about our
    *        results or not.
    */
   finishSearch: function (notify=false) {
     TelemetryStopwatch.cancel(TELEMETRY_1ST_RESULT, this);
     TelemetryStopwatch.cancel(TELEMETRY_6_FIRST_RESULTS, this);
     // Clear state now to avoid race conditions, see below.
     let search = this._currentSearch;
+    this._lastLowResultsSearchSuggestion = search._lastLowResultsSearchSuggestion;
     delete this._currentSearch;
 
     if (!notify)
       return;
 
     // There is a possible race condition here.
     // When a search completes it calls finishSearch that notifies results
     // here.  When the controller gets the last result it fires
--- a/toolkit/components/places/tests/unifiedcomplete/test_searchSuggestions.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_searchSuggestions.js
@@ -409,8 +409,76 @@ add_task(function* mixup_frecency() {
         title: "low frecency 1" },
       { uri: NetUtil.newURI("http://example.com/lo0"),
         title: "low frecency 0" },
     ],
   });
 
   yield cleanUpSuggestions();
 });
+
+add_task(function* prohibit_suggestions() {
+  Services.prefs.setBoolPref(SUGGEST_PREF, true);
+
+  yield check_autocomplete({
+    search: "localhost",
+    matches: [
+      {
+        uri: makeActionURI(("searchengine"), {
+          engineName: ENGINE_NAME,
+          input: "localhost foo",
+          searchQuery: "localhost",
+          searchSuggestion: "localhost foo",
+        }),
+        title: ENGINE_NAME,
+        style: ["action", "searchengine"],
+        icon: "",
+      },
+      {
+        uri: makeActionURI(("searchengine"), {
+          engineName: ENGINE_NAME,
+          input: "localhost bar",
+          searchQuery: "localhost",
+          searchSuggestion: "localhost bar",
+        }),
+        title: ENGINE_NAME,
+        style: ["action", "searchengine"],
+        icon: "",
+      },
+    ],
+  });
+  Services.prefs.setBoolPref("browser.fixup.domainwhitelist.localhost", true);
+  do_register_cleanup(() => {
+    Services.prefs.clearUserPref("browser.fixup.domainwhitelist.localhost");
+  });
+  yield check_autocomplete({
+    search: "localhost",
+    matches: [],
+  });
+
+  yield check_autocomplete({
+    search: "1.2.3.4",
+    matches: [],
+  });
+  yield check_autocomplete({
+    search: "[2001::1]:30",
+    matches: [],
+  });
+  yield check_autocomplete({
+    search: "user:pass@test",
+    matches: [],
+  });
+  yield check_autocomplete({
+    search: "test/test",
+    matches: [],
+  });
+  yield check_autocomplete({
+    search: "data:text/plain,Content",
+    matches: [],
+  });
+
+  yield check_autocomplete({
+    search: "a",
+    matches: [],
+  });
+
+  yield cleanUpSuggestions();
+});