[HTTPS-First Mode] Improve security state and navigation tracking

This adds a new "failed upgrade URLs" set to HttpsOnlyModeTabHelper to
track navigations that failed when being upgraded to HTTPS in each
tab. This lets us fix a bug where the upgrade interceptor would
trigger on back/forward navigations and cause extra history entries to
be added/other entries to be lost. Instead, the new implementation
skips the upgrade and directly triggers the HTTPS-First Mode
interstitial for these cases. (Other cases like reloading the tab will
still cause the navigation to be upgraded.)

This also clears the per-navigation HFM state from the TabHelper on a
new navigation starting. This prevents this state from carrying over
onto other pages, which previously could cause a downgraded security
indicator showing on actually secure pages.

This also removes some no-longer used code in HttpsOnlyTabHelper for
handling timeouts.

Bug: 1394910,1272781
Change-Id: If7b6c7ec8dc0d4d8ad835953f71933d9fc7844d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4261465
Commit-Queue: Chris Thompson <cthomp@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107847}

chrome/browser/ssl/https_only_mode_tab_helper.cc

@@ -4,29 +4,26 @@
 
 #include "chrome/browser/ssl/https_only_mode_tab_helper.h"
 
-#include "components/security_interstitials/content/https_only_mode_blocking_page.h"
-#include "components/security_interstitials/content/security_interstitial_tab_helper.h"
+#include "chrome/common/chrome_features.h"
 #include "content/public/browser/navigation_handle.h"
 
 HttpsOnlyModeTabHelper::~HttpsOnlyModeTabHelper() = default;
 
-void HttpsOnlyModeTabHelper::ReadyToCommitNavigation(
+void HttpsOnlyModeTabHelper::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (is_timer_interstitial()) {
-    set_is_timer_interstitial(false);
-    std::unique_ptr<security_interstitials::HttpsOnlyModeBlockingPage>
-        blocking_page = factory_->CreateHttpsOnlyModeBlockingPage(
-            navigation_handle->GetWebContents(), fallback_url());
-    security_interstitials::SecurityInterstitialTabHelper::
-        AssociateBlockingPage(navigation_handle, std::move(blocking_page));
+  // The original HTTPS-First Mode implementation expects these to stay set
+  // across navigations and handles clearing them separately. Only reset if
+  // the HFMv2 implementation is being used to avoid interfering with HFMv1.
+  if (base::FeatureList::IsEnabled(features::kHttpsFirstModeV2)) {
+    set_fallback_url(GURL());
+    set_is_navigation_fallback(false);
+    set_is_navigation_upgraded(false);
   }
 }
 
 HttpsOnlyModeTabHelper::HttpsOnlyModeTabHelper(
     content::WebContents* web_contents)
     : WebContentsObserver(web_contents),
-      content::WebContentsUserData<HttpsOnlyModeTabHelper>(*web_contents) {
-  factory_ = std::make_unique<ChromeSecurityBlockingPageFactory>();
-}
+      content::WebContentsUserData<HttpsOnlyModeTabHelper>(*web_contents) {}
 
 WEB_CONTENTS_USER_DATA_KEY_IMPL(HttpsOnlyModeTabHelper);

chrome/browser/ssl/https_only_mode_tab_helper.h

@@ -5,7 +5,6 @@
 #ifndef CHROME_BROWSER_SSL_HTTPS_ONLY_MODE_TAB_HELPER_H_
 #define CHROME_BROWSER_SSL_HTTPS_ONLY_MODE_TAB_HELPER_H_
 
-#include "chrome/browser/ssl/chrome_security_blocking_page_factory.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
@@ -15,8 +14,7 @@ class WebContents;
 }  // namespace content
 
 // A short-lived, per-tab helper for tracking HTTPS-Only Mode data about the
-// navigation and for creating the blocking page for the early-timeout code
-// path.
+// navigation.
 class HttpsOnlyModeTabHelper
     : public content::WebContentsObserver,
       public content::WebContentsUserData<HttpsOnlyModeTabHelper> {
@@ -26,7 +24,7 @@ class HttpsOnlyModeTabHelper
   ~HttpsOnlyModeTabHelper() override;
 
   // content::WebContentsObserver:
-  void ReadyToCommitNavigation(
+  void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
 
   // HTTPS-Only Mode metadata getters and setters:
@@ -40,22 +38,20 @@ class HttpsOnlyModeTabHelper
   }
   bool is_navigation_fallback() const { return is_navigation_fallback_; }
 
-  void set_is_timer_interstitial(bool fallback) {
-    is_timer_interstitial_ = fallback;
-  }
-  bool is_timer_interstitial() const { return is_timer_interstitial_; }
-
   void set_fallback_url(const GURL& fallback_url) {
     fallback_url_ = fallback_url;
   }
   GURL fallback_url() const { return fallback_url_; }
 
+  bool has_failed_upgrade(const GURL& url) {
+    return failed_upgrade_urls_.contains(url);
+  }
+  void add_failed_upgrade(const GURL& url) { failed_upgrade_urls_.insert(url); }
+
  private:
   explicit HttpsOnlyModeTabHelper(content::WebContents* web_contents);
   friend class content::WebContentsUserData<HttpsOnlyModeTabHelper>;
 
-  std::unique_ptr<ChromeSecurityBlockingPageFactory> factory_;
-
   // TODO(crbug.com/1218526): Track upgrade status per-navigation rather than
   // per-WebContents, in case multiple navigations occur in the WebContents and
   // the metadata is not cleared. This may be tricky however as the Interceptor
@@ -67,13 +63,19 @@ class HttpsOnlyModeTabHelper
   // Set to true if the current navigation is a fallback to HTTP.
   bool is_navigation_fallback_ = false;
 
-  // Set to true if an interstitial triggered due to an HTTPS timeout is about
-  // to be shown.
-  bool is_timer_interstitial_ = false;
-
   // HTTP URL that the current navigation should fall back to on failure.
   GURL fallback_url_;
 
+  // Holds the set of URLs that have failed to be upgraded to HTTPS in this
+  // WebContents. This is used to immediately show the HTTP interstitial without
+  // re-trying to upgrade the navigation -- currently this is only applied to
+  // back/forward navigations as they interact badly with interceptors, and this
+  // acts as the browser "remembering" the navigation state.
+  //
+  // In the case of HTTPS Upgrades, without HTTPS-First Mode enabled, these
+  // hostnames will also be on the HTTP allowlist, bypassing upgrade attempts.
+  std::set<GURL> failed_upgrade_urls_;
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 };
 

chrome/browser/ssl/https_upgrades_browsertest.cc

@@ -1036,10 +1036,8 @@ IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest, PreferHstsOverHttpsFirstMode) {
 // Regression test for crbug.com/1272781. Previously, performing back/forward
 // navigations around the HTTPS-First Mode interstitial could cause history
 // entries to dropped.
-// TODO(crbug.com/1272781): Broken when BFCache is disabled (like on the
-// linux-bfcache-rel bots).
-IN_PROC_BROWSER_TEST_F(HttpsUpgradesBrowserTest,
-                       DISABLED_InterstitialFallbackMaintainsHistory) {
+IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
+                       InterstitialFallbackMaintainsHistory) {
   // This test only applies to HTTPS-First Mode.
   if (!IsHttpInterstitialEnabled()) {
     return;
@@ -1088,18 +1086,16 @@ IN_PROC_BROWSER_TEST_F(HttpsUpgradesBrowserTest,
   // upgraded to HTTPS and fail, triggering the HTTPS-First Mode
   // interstitial.
   content::NavigateToURLBlockUntilNavigationsComplete(contents,
-                                                      downgrading_http_url, 2);
+                                                      downgrading_http_url, 1);
   EXPECT_EQ(downgrading_http_url, contents->GetLastCommittedURL());
   EXPECT_TRUE(chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
       contents));
 
   // Simulate clicking the browser "back" button.
-  // TODO(crbug.com/1394910): The incorrect WARNING security state is retained
-  // from the interstitial page.
   EXPECT_TRUE(content::HistoryGoBack(contents));
   EXPECT_EQ(good_https_url, contents->GetLastCommittedURL());
   auto* helper = SecurityStateTabHelper::FromWebContents(contents);
-  EXPECT_EQ(security_state::WARNING, helper->GetSecurityLevel());
+  EXPECT_EQ(security_state::SECURE, helper->GetSecurityLevel());
 
   // Simulate clicking the browser "forward" button. The HistoryGoForward()
   // call returns `false` because it is an error page.

chrome/browser/ssl/https_upgrades_interceptor.cc

@@ -15,6 +15,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/security_interstitials/content/stateful_ssl_host_state_delegate.h"
 #include "components/security_interstitials/core/https_only_mode_metrics.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/url_loader_request_interceptor.h"
 #include "content/public/browser/web_contents.h"
@@ -34,6 +35,7 @@
 #include "services/network/public/mojom/network_context.mojom.h"
 #include "services/network/public/mojom/url_loader.mojom.h"
 #include "services/network/public/mojom/url_response_head.mojom.h"
+#include "ui/base/page_transition_types.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
@@ -85,8 +87,7 @@ bool ShouldCreateLoader(const network::ResourceRequest& resource_request,
   if (resource_request.is_outermost_main_frame &&
       resource_request.method == "GET" &&
       !net::IsLocalhost(resource_request.url) &&
-      resource_request.url.SchemeIs(url::kHttpScheme) &&
-      !tab_helper->is_navigation_fallback()) {
+      resource_request.url.SchemeIs(url::kHttpScheme)) {
     return true;
   }
   return false;
@@ -256,6 +257,30 @@ void HttpsUpgradesInterceptor::MaybeCreateLoader(
     return;
   }
 
+  // If this is a back/forward navigation to a failed upgrade, then don't
+  // intercept to upgrade the navigation. Other forms of re-visiting a URL
+  // that previously failed to be upgraded to HTTPS *should* be intercepted so
+  // the upgrade can be attempted again (e.g., the user reloading the tab, the
+  // user navigating around and ending back on this URL in the same tab, etc.).
+  //
+  // This effectively "caches" the HTTPS-First Mode interstitial for the
+  // history entry of a failed upgrade for the lifetime of the tab. This means
+  // that it is possible for a user to come back much later (say, a week later),
+  // after a site has fixed its HTTPS configuration, and still see the
+  // interstitial for that URL.
+  //
+  // Without this check, resetting the HTTPS-Upgrades flags in
+  // HttpsOnlyModeTabHelper::DidStartNavigation() means the Interceptor would
+  // fire on back/forward navigation to the interstitial, which causes an
+  // "extra" interstitial entry to be added to the history list and lose other
+  // entries.
+  auto* entry = web_contents->GetController().GetPendingEntry();
+  if (entry && entry->GetTransitionType() & ui::PAGE_TRANSITION_FORWARD_BACK &&
+      tab_helper->has_failed_upgrade(tentative_resource_request.url)) {
+    std::move(callback).Run({});
+    return;
+  }
+
   if (!ShouldCreateLoader(tentative_resource_request, tab_helper)) {
     std::move(callback).Run({});
     return;
@@ -377,8 +402,9 @@ bool HttpsUpgradesInterceptor::MaybeCreateLoaderForResponse(
 
   tab_helper->set_is_navigation_upgraded(false);
   tab_helper->set_is_navigation_fallback(true);
+  tab_helper->add_failed_upgrade(tab_helper->fallback_url());
 
-  // `client_` may have been previously boudn from handling the initial upgrade
+  // `client_` may have been previously bound from handling the initial upgrade
   // in MaybeCreateLoader(), so reset it before re-binding it to handle this
   // response.
   client_.reset();

chrome/browser/ssl/https_upgrades_navigation_throttle.cc

@@ -23,6 +23,7 @@
 #include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/net_errors.h"
+#include "ui/base/page_transition_types.h"
 
 using security_interstitials::https_only_mode::Event;
 
@@ -91,6 +92,42 @@ HttpsUpgradesNavigationThrottle::~HttpsUpgradesNavigationThrottle() = default;
 
 content::NavigationThrottle::ThrottleCheckResult
 HttpsUpgradesNavigationThrottle::WillStartRequest() {
+  // If the navigation is fallback to HTTP, trigger the HTTP interstitial (if
+  // enabled). The interceptor creates a redirect for the fallback navigation,
+  // which will trigger MaybeCreateLoader() in the interceptor for the redirect
+  // but *doesn't* trigger WillStartRequest() because it's all part of the same
+  // request. Here, we skip directly to showing the HTTP interstitial if this
+  // is:
+  //   (1) a back/forward navigation, and
+  //   (2) the URL already failed upgrades before.
+  // This lets us avoid triggering the Interceptor during a back/forward
+  // navigation (which breaks history state) and acts like the browser
+  // "remembering" the state of the tab as being on the interstitial for that
+  // URL.
+  //
+  // Other cases for starting a navigation to a URL that previously failed
+  // to be upgraded should go through the full upgrade flow -- better to assume
+  // that something may have changed in the time since. For example: a user
+  // reloading the tab showing the interstitial should re-try the upgrade.
+  auto* handle = navigation_handle();
+  auto* contents = handle->GetWebContents();
+  auto* tab_helper = HttpsOnlyModeTabHelper::FromWebContents(contents);
+  if ((handle->GetPageTransition() & ui::PAGE_TRANSITION_FORWARD_BACK &&
+       tab_helper->has_failed_upgrade(handle->GetURL())) &&
+      !handle->GetURL().SchemeIsCryptographic() && http_interstitial_enabled_) {
+    // Mark this as a fallback HTTP navigation and trigger the interstitial.
+    tab_helper->set_is_navigation_fallback(true);
+    std::unique_ptr<security_interstitials::HttpsOnlyModeBlockingPage>
+        blocking_page = blocking_page_factory_->CreateHttpsOnlyModeBlockingPage(
+            contents, handle->GetURL());
+    std::string interstitial_html = blocking_page->GetHTMLContents();
+    security_interstitials::SecurityInterstitialTabHelper::
+        AssociateBlockingPage(handle, std::move(blocking_page));
+    return content::NavigationThrottle::ThrottleCheckResult(
+        content::NavigationThrottle::CANCEL, net::ERR_BLOCKED_BY_CLIENT,
+        interstitial_html);
+  }
+
   // Navigation is HTTPS or an initial HTTP navigation (which will get
   // upgraded by the interceptor). Fallback HTTP navigations are handled in
   // WillRedirectRequest().
@@ -106,11 +143,11 @@ HttpsUpgradesNavigationThrottle::WillFailRequest() {
 
 content::NavigationThrottle::ThrottleCheckResult
 HttpsUpgradesNavigationThrottle::WillRedirectRequest() {
-  // If the navigation is fallback to HTTP, trigger the HTTP interstitial (if
-  // enabled). The interceptor creates a redirect for the fallback navigation,
-  // which will trigger MaybeCreateLoader() in the interceptor for the redirect
-  // but *doesn't* trigger WillStartRequest() because it's all part of the same
-  // request.
+  // If the navigation is doing a fallback redirect to HTTP, trigger the HTTP
+  // interstitial (if enabled). The interceptor creates a redirect for the
+  // fallback navigation, which will trigger MaybeCreateLoader() in the
+  // interceptor for the redirect but *doesn't* trigger WillStartRequest()
+  // because it's all part of the same request.
   auto* handle = navigation_handle();
   auto* contents = handle->GetWebContents();
   auto* tab_helper = HttpsOnlyModeTabHelper::FromWebContents(contents);

components/security_state/core/security_state.cc

@@ -66,18 +66,18 @@ SecurityLevel GetSecurityLevel(
     return DANGEROUS;
   }
 
-  // If the navigation was upgraded to HTTPS because of HTTPS-Only Mode, but did
-  // not succeed (either currently showing the HTTPS-Only Mode interstitial, or
-  // the navigation fell back to HTTP), set the security level to WARNING. The
-  // HTTPS-Only Mode interstitial warning is considered "less serious" than the
-  // general certificate error interstitials.
+  // If the navigation was upgraded to HTTPS because of HTTPS-First Mode, but
+  // did not succeed and is showing the HTTPS-First Mode interstitial, set the
+  // security level to WARNING. The HTTPS-First Mode interstitial warning is
+  // considered "less serious" than the general certificate error interstitials.
   //
   // This check must come before the checks for `connection_info_initialized`
-  // (because the HTTPS-Only Mode intersitital can trigger if the HTTPS version
-  // of the page does not commit) and certificate errors (because the HTTPS-Only
-  // Mode interstitial takes precedent if the certificate error occurred due to
-  // an upgraded main-frame navigation).
-  if (visible_security_state.is_https_only_mode_upgraded) {
+  // (because the HTTPS-First Mode intersitital can trigger if the HTTPS version
+  // of the page does not commit) and certificate errors (because the
+  // HTTPS-First Mode interstitial takes precedent if the certificate error
+  // occurred due to an upgraded main-frame navigation).
+  if (visible_security_state.is_https_only_mode_upgraded &&
+      visible_security_state.is_error_page) {
     return WARNING;
   }
 

components/security_state/core/security_state_unittest.cc

@@ -474,6 +474,7 @@ TEST(SecurityStateTest, HttpsOnlyModeOverridesCertificateError) {
   helper.set_cert_status(net::CERT_STATUS_SHA1_SIGNATURE_PRESENT |
                          net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION);
   EXPECT_TRUE(helper.HasMajorCertificateError());
+  helper.set_is_error_page(true);
   helper.set_is_https_only_mode_upgraded(true);
   EXPECT_EQ(SecurityLevel::WARNING, helper.GetSecurityLevel());
 }
@@ -483,6 +484,7 @@ TEST(SecurityStateTest, MaliciousContentOverridesHttpsOnlyMode) {
   TestSecurityStateHelper helper;
   helper.set_malicious_content_status(
       MALICIOUS_CONTENT_STATUS_SOCIAL_ENGINEERING);
+  helper.set_is_error_page(true);
   helper.set_is_https_only_mode_upgraded(true);
   EXPECT_EQ(DANGEROUS, helper.GetSecurityLevel());
 }