Omnibox - Do Not Allow HTTP/HTTPS Equivalence if User Explicitly Entered A Scheme

components/omnibox/autocomplete_input.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/omnibox/autocomplete_input.h"
 
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/metrics/proto/omnibox_event.pb.h"
 #include "components/omnibox/autocomplete_scheme_classifier.h"
 #include "components/url_fixer/url_fixer.h"
 #include "net/base/net_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "url/url_canon_ip.h"
 #include "url/url_util.h"
 
 namespace {
 
 // Hardcode constant to avoid any dependencies on content/.
 const char kViewSourceScheme[] = "view-source";
 
 void AdjustCursorPositionIfNecessary(size_t num_leading_chars_removed,
                                      size_t* cursor_position) {
   if (*cursor_position == base::string16::npos)
     return;
   if (num_leading_chars_removed < *cursor_position)
     *cursor_position -= num_leading_chars_removed;
   else
     *cursor_position = 0;
 }
 
+// Finds all terms in |text| that start with http:// or https:// and puts the
+// text after the prefix in |http_following_terms|.  Each term has to include
+// at least one additional character after the prefix.

[Peter Kasting, 2015/06/09 20:36:39]

Nit: Could say "...or https:// plus at least one more character and puts..." and
then drop the second sentence entirely.

[Mark P, 2015/06/10 23:38:34]

Done.

+void PopulateHttpTerms(const base::string16& text,

[Peter Kasting, 2015/06/09 20:36:38]

Nit: If you change the member name, change this too -- maybe
PopulateTermsFollowingScheme() or FindTermsWithExplicitScheme() or something.

[Mark P, 2015/06/10 23:38:34]

Selected PopulateTermsPrefixedByScheme().

+                       std::vector<base::string16>* http_following_terms) {
+  const base::string16& http_scheme =

[Peter Kasting, 2015/06/09 20:36:38]

Rather than converting these to string16s, I think we should convert |text| to
UTF-8 (likely on the caller side).  The code in autocomplete_match.cc has UTF-8
GURLs anyway, so this will avoid the conversions there.

This means the type of |http_following_terms| (here and as a data member) will
change to std::vector<std::string>.

[Mark P, 2015/06/10 23:38:34]

I don't like this idea.  Everywhere in the autocomplete code when dealing with
user input we use string16.  Anything that gets compared with user input goes to
the UTF-16 space.  This is a widespread convention, and I'm nervous about
breaking this pattern when there's no particularly good reason.  I don't recall
ever seeing the input converted to a std::string even when it would be more
convenient to do so.  To be concrete, much of this code parallels logic in
url_prefix, which uses the same strategy (converting the URL to UTF16 to compare
to the user input, which remains as string16).

[Peter Kasting, 2015/06/11 00:07:38]

This isn't an intentional pattern.  The intentional pattern is to minimize
conversions and code complexity, which is often accomplished by leaving input as
UTF-16, but not in this case.  (There are other places we convert input, or
parts of it, to UTF-8 already; AutocompleteResult::SortAndCull(),
ShortcutsProvider::ShortcutToACMatch(), HistoryURLProvider::DoAutocomplete(),
etc.)

I don't see any downsides to this.  UTF-8 can represent everything UTF-16 can
represent and it's impossible to confuse the two because they use different char
and string types.  So I see no risk of loss of functionality or introducing
bugs.

[Mark P, 2015/06/11 22:11:41]

Okay, I did it.  Easy enough.

+      base::UTF8ToUTF16(url::kHttpScheme) +
+      base::UTF8ToUTF16(url::kStandardSchemeSeparator);
+  const base::string16& https_scheme =
+      base::UTF8ToUTF16(url::kHttpsScheme) +
+      base::UTF8ToUTF16(url::kStandardSchemeSeparator);
+  std::vector<base::string16> words;

[Peter Kasting, 2015/06/09 20:36:38]

Nit: words -> terms (for consistency)?

[Mark P, 2015/06/10 23:38:34]

Done.

+  base::SplitString(text, ' ', &words);

[Peter Kasting, 2015/06/09 20:36:38]

Nit: Consider a comment about why we don't use ICU breaking functions to split
into words.

[Mark P, 2015/06/10 23:38:34]

Done.

+  for (auto it : words) {

[Peter Kasting, 2015/06/09 20:36:38]

Nit: const auto&, to avoid a copy

[Mark P, 2015/06/10 23:38:34]

Done.

+    if (StartsWith(it, http_scheme, false) &&

[Peter Kasting, 2015/06/09 20:36:38]

Nit: This is shorter overall and no less efficient:

  std::vector<std::string> terms;
  base::SplitString(text, ' ', &terms);
  const std::string separator(url::kStandardSchemeSeparator);
  for (const auto& term : terms) {
    static const char* kSchemes[2] = { url::kHttpScheme, url::kHttpsScheme };
    for (const char* scheme : kSchemes) {
      const std::string prefix(scheme + separator);
      if (StartsWith(term, prefix, false) && (term.length() > prefix.length()))
        http_following_terms->push_back(term.substr(prefix.length()));
    }
  }

[Mark P, 2015/06/10 23:38:34]

Done.  (well, the string16 version of the above)

+        (it.length() > http_scheme.length())) {
+      http_following_terms->push_back(it.substr(http_scheme.length()));
+    } else if (StartsWith(it, https_scheme, false) &&
+               (it.length() > https_scheme.length())) {
+      http_following_terms->push_back(it.substr(https_scheme.length()));
+    }
+  }
+}
+
 }  // namespace
 
 AutocompleteInput::AutocompleteInput()
     : cursor_position_(base::string16::npos),
       current_page_classification_(metrics::OmniboxEventProto::INVALID_SPEC),
       type_(metrics::OmniboxInputType::INVALID),
       prevent_inline_autocomplete_(false),
       prefer_keyword_(false),
       allow_exact_keyword_match_(true),
       want_asynchronous_matches_(true) {
@@ skipping 23 matching lines @@
   // None of the providers care about leading white space so we always trim it.
   // Providers that care about trailing white space handle trimming themselves.
   if ((base::TrimWhitespace(text, base::TRIM_LEADING, &text_) &
        base::TRIM_LEADING) != 0)
     AdjustCursorPositionIfNecessary(text.length() - text_.length(),
                                     &cursor_position_);
 
   GURL canonicalized_url;
   type_ = Parse(text_, desired_tld, scheme_classifier, &parts_, &scheme_,
                 &canonicalized_url);
+  PopulateHttpTerms(text_, &http_following_terms_);
 
   if (type_ == metrics::OmniboxInputType::INVALID)
     return;
 
   if (((type_ == metrics::OmniboxInputType::UNKNOWN) ||
        (type_ == metrics::OmniboxInputType::URL)) &&
       canonicalized_url.is_valid() &&
       (!canonicalized_url.IsStandard() || canonicalized_url.SchemeIsFile() ||
        canonicalized_url.SchemeIsFileSystem() ||
        !canonicalized_url.host().empty()))
@@ skipping 440 matching lines @@
   current_url_ = GURL();
   current_page_classification_ = metrics::OmniboxEventProto::INVALID_SPEC;
   type_ = metrics::OmniboxInputType::INVALID;
   parts_ = url::Parsed();
   scheme_.clear();
   canonicalized_url_ = GURL();
   prevent_inline_autocomplete_ = false;
   prefer_keyword_ = false;
   allow_exact_keyword_match_ = false;
   want_asynchronous_matches_ = true;
+  http_following_terms_.clear();
 }