Default omnibox navigations to https: Add tests for redirects

This CL adds a test case where the upgraded HTTPS URL redirects to
another HTTPS URL. The test simply checks if an upgraded HTTPS URL
that redirects to another good HTTPS URL successfully loads and records
a histogram entry.

There are additional interesting cases involving redirects, however
it's not yet fully clear how the feature should behave in those cases.
For example, an upgraded HTTPS URL could redirect to a broken HTTPS URL.
It's TBD whether this should fall back to HTTP or not. For that
reason, these cases aren't yet tested.

Presently, the autocomplete code does the upgrade from HTTP to HTTPS and
it only upgrades URLs with default ports. As a result, browser tests
need to use URLs with default ports, so they use
URLLoaderInterceptor instead of EmbeddedTestServer.
(URLLoaderInterceptor can return responses for URLs with a default
port (e.g. https://example.com) whereas EmbeddedTestServer can't.)

I wasn't able to simulate a redirect using URLLoaderInterceptor, so
this CL modifies the autocomplete code to accept a non-default port as
well. It also adds a new test class that uses EmbeddedTestServer to
serve the http and https URLs. The rest of the browser tests are
unchanged and still use URLLoaderInterceptor so that the codepath that
uses the default ports is still exercised.

Bug: 1141691
Change-Id: Ic0d0638758bf457fad508ef028144681a7034cbe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2621964
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Chris Thompson <cthomp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845054}

chrome/browser/ssl/typed_navigation_upgrade_throttle.cc

@@ -35,6 +35,8 @@ constexpr base::FeatureParam<base::TimeDelta> kFallbackDelay{
     omnibox::kDefaultTypedNavigationsToHttpsTimeoutParam,
     base::TimeDelta::FromSeconds(3)};
 
+int g_https_port_for_testing = 0;
+
 bool IsNavigationUsingHttpsAsDefaultScheme(content::NavigationHandle* handle) {
   content::NavigationUIData* ui_data = handle->GetNavigationUIData();
   // UI data can be null in the case of navigations to interstitials.
@@ -173,6 +175,12 @@ TypedNavigationUpgradeThrottle::WillFailRequest() {
   return content::NavigationThrottle::CANCEL_AND_IGNORE;
 }
 
+content::NavigationThrottle::ThrottleCheckResult
+TypedNavigationUpgradeThrottle::WillRedirectRequest() {
+  RecordUMA(Event::kRedirected);
+  return content::NavigationThrottle::PROCEED;
+}
+
 content::NavigationThrottle::ThrottleCheckResult
 TypedNavigationUpgradeThrottle::WillProcessResponse() {
   DCHECK_EQ(url::kHttpsScheme, navigation_handle()->GetURL().scheme());
@@ -196,6 +204,17 @@ bool TypedNavigationUpgradeThrottle::
          IsNavigationUsingHttpsAsDefaultScheme(handle);
 }
 
+// static
+int TypedNavigationUpgradeThrottle::GetHttpsPortForTesting() {
+  return g_https_port_for_testing;
+}
+
+// static
+void TypedNavigationUpgradeThrottle::SetHttpsPortForTesting(
+    int https_port_for_testing) {
+  g_https_port_for_testing = https_port_for_testing;
+}
+
 TypedNavigationUpgradeThrottle::TypedNavigationUpgradeThrottle(
     content::NavigationHandle* handle)
     : content::NavigationThrottle(handle),

chrome/browser/ssl/typed_navigation_upgrade_throttle.h

@@ -35,7 +35,10 @@ class TypedNavigationUpgradeThrottle : public content::NavigationThrottle {
     // Failed to load the upgraded HTTPS URL within the timeout window, fell
     // back to the HTTP URL.
     kHttpsLoadTimedOut,
-    kMaxValue = kHttpsLoadTimedOut,
+    // Received a redirect. This doesn't necessarily imply that the HTTPS load
+    // succeeded or failed.
+    kRedirected,
+    kMaxValue = kRedirected,
   };
 
   static std::unique_ptr<content::NavigationThrottle> MaybeCreateThrottleFor(
@@ -46,6 +49,8 @@ class TypedNavigationUpgradeThrottle : public content::NavigationThrottle {
   // content::NavigationThrottle:
   content::NavigationThrottle::ThrottleCheckResult WillStartRequest() override;
   content::NavigationThrottle::ThrottleCheckResult WillFailRequest() override;
+  content::NavigationThrottle::ThrottleCheckResult WillRedirectRequest()
+      override;
   content::NavigationThrottle::ThrottleCheckResult WillProcessResponse()
       override;
   const char* GetNameForLogging() override;
@@ -56,6 +61,12 @@ class TypedNavigationUpgradeThrottle : public content::NavigationThrottle {
   static bool ShouldIgnoreInterstitialBecauseNavigationDefaultedToHttps(
       content::NavigationHandle* handle);
 
+  // Sets the port used by the embedded https server. This is used to determine
+  // the correct port while upgrading URLs to https if the original URL has a
+  // non-default port.
+  static void SetHttpsPortForTesting(int https_port_for_testing);
+  static int GetHttpsPortForTesting();
+
   static const char kHistogramName[];
 
  private:

chrome/browser/ui/omnibox/chrome_omnibox_client.cc

@@ -33,6 +33,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_metrics.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/ssl/typed_navigation_upgrade_throttle.h"
 #include "chrome/browser/translate/chrome_translate_client.h"
 #include "chrome/browser/ui/bookmarks/bookmark_stats.h"
 #include "chrome/browser/ui/browser.h"
@@ -169,6 +170,10 @@ bool ChromeOmniboxClient::ShouldDefaultTypedNavigationsToHttps() const {
   return base::FeatureList::IsEnabled(omnibox::kDefaultTypedNavigationsToHttps);
 }
 
+int ChromeOmniboxClient::GetHttpsPortForTesting() const {
+  return TypedNavigationUpgradeThrottle::GetHttpsPortForTesting();
+}
+
 gfx::Image ChromeOmniboxClient::GetIconIfExtensionMatch(
     const AutocompleteMatch& match) const {
   TemplateURLService* service =

chrome/browser/ui/omnibox/chrome_omnibox_client.h

@@ -49,6 +49,7 @@ class ChromeOmniboxClient : public OmniboxClient {
   const AutocompleteSchemeClassifier& GetSchemeClassifier() const override;
   AutocompleteClassifier* GetAutocompleteClassifier() override;
   bool ShouldDefaultTypedNavigationsToHttps() const override;
+  int GetHttpsPortForTesting() const override;
   gfx::Image GetIconIfExtensionMatch(
       const AutocompleteMatch& match) const override;
   gfx::Image GetSizedIcon(const gfx::VectorIcon& vector_icon_type,

components/omnibox/browser/autocomplete_input.cc

@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -89,31 +90,36 @@ AutocompleteInput::AutocompleteInput()
       keyword_mode_entry_method_(metrics::OmniboxEventProto::INVALID),
       want_asynchronous_matches_(true),
       should_use_https_as_default_scheme_(false),
-      added_default_scheme_to_typed_url_(false) {}
+      added_default_scheme_to_typed_url_(false),
+      https_port_for_testing_(0) {}
 
 AutocompleteInput::AutocompleteInput(
     const base::string16& text,
     metrics::OmniboxEventProto::PageClassification current_page_classification,
     const AutocompleteSchemeClassifier& scheme_classifier,
-    bool should_use_https_as_default_scheme)
+    bool should_use_https_as_default_scheme,
+    int https_port_for_testing)
     : AutocompleteInput(text,
                         std::string::npos,
                         current_page_classification,
                         scheme_classifier,
-                        should_use_https_as_default_scheme) {}
+                        should_use_https_as_default_scheme,
+                        https_port_for_testing) {}
 
 AutocompleteInput::AutocompleteInput(
     const base::string16& text,
     size_t cursor_position,
     metrics::OmniboxEventProto::PageClassification current_page_classification,
     const AutocompleteSchemeClassifier& scheme_classifier,
-    bool should_use_https_as_default_scheme)
+    bool should_use_https_as_default_scheme,
+    int https_port_for_testing)
     : AutocompleteInput(text,
                         cursor_position,
                         "",
                         current_page_classification,
                         scheme_classifier,
-                        should_use_https_as_default_scheme) {}
+                        should_use_https_as_default_scheme,
+                        https_port_for_testing) {}
 
 AutocompleteInput::AutocompleteInput(
     const base::string16& text,
@@ -121,12 +127,14 @@ AutocompleteInput::AutocompleteInput(
     const std::string& desired_tld,
     metrics::OmniboxEventProto::PageClassification current_page_classification,
     const AutocompleteSchemeClassifier& scheme_classifier,
-    bool should_use_https_as_default_scheme)
+    bool should_use_https_as_default_scheme,
+    int https_port_for_testing)
     : AutocompleteInput() {
   cursor_position_ = cursor_position;
   current_page_classification_ = current_page_classification;
   desired_tld_ = desired_tld;
   should_use_https_as_default_scheme_ = should_use_https_as_default_scheme;
+  https_port_for_testing_ = https_port_for_testing;
   Init(text, scheme_classifier);
 }
 
@@ -156,7 +164,7 @@ void AutocompleteInput::Init(
       !base::StartsWith(text_, scheme_, base::CompareCase::INSENSITIVE_ASCII) &&
       !url::HostIsIPAddress(canonicalized_url.host()) &&
       !net::IsHostnameNonUnique(base::UTF16ToUTF8(text)) &&
-      canonicalized_url.port().empty()) {
+      (canonicalized_url.port().empty() || https_port_for_testing_)) {
     // Use HTTPS as the default scheme for URLs that are typed without a scheme.
     // Inputs of type UNKNOWN can still be valid URLs, but these will be mainly
     // intranet hosts which we don't to upgrade to HTTPS so we only check the
@@ -174,6 +182,14 @@ void AutocompleteInput::Init(
     scheme_ = base::ASCIIToUTF16(url::kHttpsScheme);
     GURL::Replacements replacements;
     replacements.SetSchemeStr(url::kHttpsScheme);
+    // This needs to be in scope when ReplaceComponents() is called:
+    const std::string port_str = base::NumberToString(https_port_for_testing_);
+    if (https_port_for_testing_) {
+      // We'll only get here in tests. Tests should always have a non-default
+      // port on the input text.
+      DCHECK(!canonicalized_url.port().empty());
+      replacements.SetPortStr(port_str);
+    }
     canonicalized_url = canonicalized_url.ReplaceComponents(replacements);
     // We changed the scheme from http to https. Offset remaining components
     // by one.
@@ -633,6 +649,7 @@ void AutocompleteInput::Clear() {
   focus_type_ = OmniboxFocusType::DEFAULT;
   terms_prefixed_by_http_or_https_.clear();
   query_tile_id_.reset();
+  https_port_for_testing_ = false;
 }
 
 size_t AutocompleteInput::EstimateMemoryUsage() const {

components/omnibox/browser/autocomplete_input.h

@@ -37,7 +37,8 @@ class AutocompleteInput {
                     metrics::OmniboxEventProto::PageClassification
                         current_page_classification,
                     const AutocompleteSchemeClassifier& scheme_classifier,
-                    bool should_use_https_as_default_scheme = false);
+                    bool should_use_https_as_default_scheme = false,
+                    int https_port_for_testing = 0);
   // This constructor adds |cursor_position|, related to |text|.
   // |cursor_position| represents the location of the cursor within the
   // query |text|. It may be set to base::string16::npos if the input
@@ -47,7 +48,8 @@ class AutocompleteInput {
                     metrics::OmniboxEventProto::PageClassification
                         current_page_classification,
                     const AutocompleteSchemeClassifier& scheme_classifier,
-                    bool should_use_https_as_default_scheme = false);
+                    bool should_use_https_as_default_scheme = false,
+                    int https_port_for_testing = 0);
 
   // This constructor adds |desired_tld|, related to |text|. |desired_tld|
   // is the user's desired TLD, if one is not already present in the text to
@@ -60,7 +62,8 @@ class AutocompleteInput {
                     metrics::OmniboxEventProto::PageClassification
                         current_page_classification,
                     const AutocompleteSchemeClassifier& scheme_classifier,
-                    bool should_use_https_as_default_scheme = false);
+                    bool should_use_https_as_default_scheme = false,
+                    int https_port_for_testing = 0);
 
   AutocompleteInput(const AutocompleteInput& other);
   ~AutocompleteInput();
@@ -299,6 +302,12 @@ class AutocompleteInput {
   // Flags for OmniboxDefaultNavigationsToHttps feature.
   bool should_use_https_as_default_scheme_;
   bool added_default_scheme_to_typed_url_;
+  // Port used by the embedded https server in tests. This is used to determine
+  // the correct port while upgrading URLs to https if the original URL has a
+  // non-default port.
+  // TODO(crbug.com/1168371): Remove when URLLoaderInterceptor can simulate
+  // redirects.
+  int https_port_for_testing_;
 };
 
 #endif  // COMPONENTS_OMNIBOX_BROWSER_AUTOCOMPLETE_INPUT_H_

components/omnibox/browser/autocomplete_input_unittest.cc

@@ -376,11 +376,13 @@ TEST(AutocompleteInputTest, InputTypeWithCursorPosition) {
 }
 
 TEST(AutocompleteInputTest, UpgradeTypedNavigationsToHttps) {
-  struct test_data {
+  struct TestData {
     const base::string16 input;
     const GURL expected_url;
     bool expected_added_default_scheme_to_typed_url;
-  } input_cases[]{
+  };
+
+  const TestData test_cases[] = {
       {ASCIIToUTF16("example.com"), GURL("https://example.com"), true},
       // If the hostname has a port specified, the URL shouldn't be upgraded
       // to HTTPS because we can't assume that the HTTPS site is served over the
@@ -408,14 +410,56 @@ TEST(AutocompleteInputTest, UpgradeTypedNavigationsToHttps) {
       {ASCIIToUTF16("HTTP://EXAMPLE.COM:8080"), GURL("http://example.com:8080"),
        false},
   };
-  for (const test_data& input_case : input_cases) {
-    AutocompleteInput input(input_case.input, base::string16::npos,
+  for (const TestData& test_case : test_cases) {
+    AutocompleteInput input(test_case.input, base::string16::npos,
                             metrics::OmniboxEventProto::OTHER,
                             TestSchemeClassifier(),
                             /*should_use_https_as_default_scheme=*/true);
-    EXPECT_EQ(input_case.expected_url, input.canonicalized_url())
-        << input_case.input;
-    EXPECT_EQ(input_case.expected_added_default_scheme_to_typed_url,
+    EXPECT_EQ(test_case.expected_url, input.canonicalized_url())
+        << test_case.input;
+    EXPECT_EQ(test_case.expected_added_default_scheme_to_typed_url,
+              input.added_default_scheme_to_typed_url());
+  }
+
+  // Try the same test cases with a non-zero HTTPS port passed to
+  // AutocompleteInput. When a non-zero HTTPS port is used, AutoCompleteInput
+  // should use that port to replace the port of the HTTP URL when upgrading
+  // the URL.
+  // We don't check the default port 80 being upgraded in these test case,
+  // because the default port will be dropped by GURL and we'll end up with
+  // example.com. A hostname without a port is not a valid input when using a
+  // non-zero value for https_port_for_testing.
+  int https_port_for_testing = 12345;
+  const TestData test_cases_non_default_port[] = {
+      {ASCIIToUTF16("example.com:8080"), GURL("https://example.com:12345"),
+       true},
+      // Non-URL inputs shouldn't be upgraded.
+      {ASCIIToUTF16("example query"), GURL(), false},
+      // IP addresses shouldn't be upgraded.
+      {ASCIIToUTF16("127.0.0.1"), GURL("http://127.0.0.1"), false},
+      {ASCIIToUTF16("127.0.0.1:80"), GURL("http://127.0.0.1:80"), false},
+      {ASCIIToUTF16("127.0.0.1:8080"), GURL("http://127.0.0.1:8080"), false},
+      // Non-unique hostnames shouldn't be upgraded.
+      {ASCIIToUTF16("site.test"), GURL("http://site.test"), false},
+      // // Fully typed URLs shouldn't be upgraded.
+      {ASCIIToUTF16("http://example.com"), GURL("http://example.com"), false},
+      {ASCIIToUTF16("HTTP://EXAMPLE.COM"), GURL("http://example.com"), false},
+      {ASCIIToUTF16("http://example.com:80"), GURL("http://example.com"),
+       false},
+      {ASCIIToUTF16("HTTP://EXAMPLE.COM:80"), GURL("http://example.com"),
+       false},
+      {ASCIIToUTF16("http://example.com:8080"), GURL("http://example.com:8080"),
+       false},
+      {ASCIIToUTF16("HTTP://EXAMPLE.COM:8080"), GURL("http://example.com:8080"),
+       false}};
+  for (const TestData& test_case : test_cases_non_default_port) {
+    AutocompleteInput input(
+        test_case.input, base::string16::npos,
+        metrics::OmniboxEventProto::OTHER, TestSchemeClassifier(),
+        /*should_use_https_as_default_scheme=*/true, https_port_for_testing);
+    EXPECT_EQ(test_case.expected_url, input.canonicalized_url())
+        << test_case.input;
+    EXPECT_EQ(test_case.expected_added_default_scheme_to_typed_url,
               input.added_default_scheme_to_typed_url());
   }
 }
\ No newline at end of file

components/omnibox/browser/omnibox_client.cc

@@ -65,6 +65,10 @@ bool OmniboxClient::ShouldDefaultTypedNavigationsToHttps() const {
   return false;
 }
 
+int OmniboxClient::GetHttpsPortForTesting() const {
+  return 0;
+}
+
 gfx::Image OmniboxClient::GetIconIfExtensionMatch(
     const AutocompleteMatch& match) const {
   return gfx::Image();

components/omnibox/browser/omnibox_client.h

@@ -85,6 +85,13 @@ class OmniboxClient {
   virtual const AutocompleteSchemeClassifier& GetSchemeClassifier() const = 0;
   virtual AutocompleteClassifier* GetAutocompleteClassifier();
   virtual bool ShouldDefaultTypedNavigationsToHttps() const = 0;
+  // Returns the port used by the embedded https server in tests. This is used
+  // to determine the correct port while upgrading typed URLs to https if the
+  // original URL has a non-default port. Only meaningful if
+  // ShouldDefaultTypedNavigationsToHttps() returns true.
+  // TODO(crbug.com/1168371): Remove when URLLoaderInterceptor can simulate
+  // redirects.
+  virtual int GetHttpsPortForTesting() const = 0;
 
   // Returns the icon corresponding to |match| if match is an extension match
   // and an empty icon otherwise.

components/omnibox/browser/omnibox_edit_model.cc

@@ -539,7 +539,8 @@ void OmniboxEditModel::StartAutocomplete(bool has_selected_text,
   input_ =
       AutocompleteInput(input_text, cursor_position, GetPageClassification(),
                         client_->GetSchemeClassifier(),
-                        client_->ShouldDefaultTypedNavigationsToHttps());
+                        client_->ShouldDefaultTypedNavigationsToHttps(),
+                        client_->GetHttpsPortForTesting());
   input_.set_current_url(client_->GetURL());
   input_.set_current_title(client_->GetTitle());
   input_.set_prevent_inline_autocomplete(
@@ -762,7 +763,8 @@ void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
   // to create an alternate navigational match.
   AutocompleteInput alternate_input(
       input_text, GetPageClassification(), client_->GetSchemeClassifier(),
-      client_->ShouldDefaultTypedNavigationsToHttps());
+      client_->ShouldDefaultTypedNavigationsToHttps(),
+      client_->GetHttpsPortForTesting());
   // Somehow we can occasionally get here with no active tab.  It's not
   // clear why this happens.
   alternate_input.set_current_url(client_->GetURL());
@@ -1144,7 +1146,8 @@ void OmniboxEditModel::StartZeroSuggestRequest(
   // match can be wrong. The full page URL is anyways in set_current_url().
   input_ = AutocompleteInput(view_->GetText(), GetPageClassification(),
                              client_->GetSchemeClassifier(),
-                             client_->ShouldDefaultTypedNavigationsToHttps());
+                             client_->ShouldDefaultTypedNavigationsToHttps(),
+                             client_->GetHttpsPortForTesting());
   input_.set_current_url(client_->GetURL());
   input_.set_current_title(client_->GetTitle());
   input_.set_focus_type(user_clobbered_permanent_text

components/omnibox/browser/test_omnibox_client.cc

@@ -102,6 +102,10 @@ bool TestOmniboxClient::ShouldDefaultTypedNavigationsToHttps() const {
   return false;
 }
 
+int TestOmniboxClient::GetHttpsPortForTesting() const {
+  return 0;
+}
+
 gfx::Image TestOmniboxClient::GetSizedIcon(
     const gfx::VectorIcon& vector_icon_type,
     SkColor vector_icon_color) const {

components/omnibox/browser/test_omnibox_client.h

@@ -44,6 +44,7 @@ class TestOmniboxClient : public OmniboxClient {
   const AutocompleteSchemeClassifier& GetSchemeClassifier() const override;
   AutocompleteClassifier* GetAutocompleteClassifier() override;
   bool ShouldDefaultTypedNavigationsToHttps() const override;
+  int GetHttpsPortForTesting() const override;
   gfx::Image GetSizedIcon(const gfx::VectorIcon& vector_icon_type,
                           SkColor vector_icon_color) const override;
   gfx::Image GetFaviconForPageUrl(

ios/chrome/browser/ui/omnibox/chrome_omnibox_client_ios.h

@@ -38,6 +38,7 @@ class ChromeOmniboxClientIOS : public OmniboxClient {
   const AutocompleteSchemeClassifier& GetSchemeClassifier() const override;
   AutocompleteClassifier* GetAutocompleteClassifier() override;
   bool ShouldDefaultTypedNavigationsToHttps() const override;
+  int GetHttpsPortForTesting() const override;
   gfx::Image GetIconIfExtensionMatch(
       const AutocompleteMatch& match) const override;
   bool ProcessExtensionKeyword(const TemplateURL* template_url,

ios/chrome/browser/ui/omnibox/chrome_omnibox_client_ios.mm

@@ -108,6 +108,10 @@ bool ChromeOmniboxClientIOS::ShouldDefaultTypedNavigationsToHttps() const {
   return false;
 }
 
+int ChromeOmniboxClientIOS::GetHttpsPortForTesting() const {
+  return 0;
+}
+
 gfx::Image ChromeOmniboxClientIOS::GetIconIfExtensionMatch(
     const AutocompleteMatch& match) const {
   // Extensions are not supported on iOS.

tools/metrics/histograms/enums.xml

@@ -75077,6 +75077,9 @@ Full version information for the fingerprint enum values:
   <int value="4"
       label="Failed to load the upgraded HTTPS URL because of a net error,
              fell back to the HTTP URL."/>
+  <int value="5"
+      label="Received a redirect. This doesn't necessarily imply that the
+             HTTPS load succeeded or failed."/>
 </enum>
 
 <enum name="TypeOfVideoInFrame">