From 50dff058f0c710121b0454dbf024b2d881d60099 Mon Sep 17 00:00:00 2001
From: Kouhei Ueno <kouhei@chromium.org>
Date: Tue, 28 Jul 2026 03:47:52 -0700
Subject: [PATCH] [iOS] HTTPS-Only Mode: upgrade HTTP redirect targets at request

HttpsOnlyModeUpgradeTabHelper only enforced in ShouldAllowResponse,
which WKWebView calls for the final non-3xx response of a redirect
chain. A chain of HTTPS -> HTTP -> HTTPS would therefore send the
cleartext request and render the final HTTPS page without an
interstitial.

Override ShouldAllowRequest so that main-frame redirects to HTTP within
an in-progress navigation are cancelled before being sent and re-issued
through the existing upgrade state machine. A new
was_navigation_started_ flag distinguishes redirect hops, where
cancelling produces a matching DidFinishNavigation, from the initial
request of a new navigation (which is still handled at response time).
The previously unused kStoppedToFallback state is now set when an
already-upgraded navigation redirects to HTTP so that
DidFinishNavigation routes through the existing fallback path.

Add a unit test that drives ShouldAllowRequest before and after
DidStartNavigation, and an EG test that loads a faux-HTTPS page
redirecting to HTTP and checks that the HTTP server is never reached.

TAG=agy
CONV=4f6d8236-305b-4859-b9ef-29b97faef52f

Bug: 503736006
Change-Id: I657e0c3ad9bd49b1b1237a550bc2890d78a4eed0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8017366
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1669338}
---
diff --git a/ios/chrome/browser/https_upgrades/model/https_only_mode_egtest.mm b/ios/chrome/browser/https_upgrades/model/https_only_mode_egtest.mm
index 1ff8ae3..a8bcb0ff 100644
--- a/ios/chrome/browser/https_upgrades/model/https_only_mode_egtest.mm
+++ b/ios/chrome/browser/https_upgrades/model/https_only_mode_egtest.mm
@@ -402,6 +402,23 @@
   [self assertFailedUpgrade:1];
 }
 
+// Navigate to an HTTPS URL that redirects to an HTTP URL. The HTTP redirect
+// target should be upgraded to HTTPS before the request is sent so that the
+// HTTP server never receives a cleartext request.
+- (void)test_HTTPSRedirectsToHTTP_ShouldUpgradeRedirectTarget {
+  [HttpsUpgradeAppInterface setHTTPSPortForTesting:self.goodHTTPSServer->port()
+                                      useFakeHTTPS:true];
+
+  GURL targetURL = self.testServer->GetURL("/");
+  GURL testURL = self.goodHTTPSServer->GetURL("/?redirect=" + targetURL.spec());
+  [ChromeEarlGrey loadURL:testURL];
+  [ChromeEarlGrey waitForWebStateContainingText:"HTTPS_RESPONSE"];
+  [self assertSuccessfulUpgrade];
+
+  GREYAssertEqual(0, _HTTPResponseCounter,
+                  @"The HTTP server should not have been reached");
+}
+
 // Tests that prerendered navigations that should be upgraded are cancelled.
 // This test is adapted from testTapPrerenderSuggestions() in
 // prerender_egtest.mm.
diff --git a/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper.h b/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper.h
index e2bfeae..1ed3bb4 100644
--- a/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper.h
+++ b/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper.h
@@ -86,6 +86,10 @@
   void ResetState();
 
   // web::WebStatePolicyDecider implementation:
+  void ShouldAllowRequest(
+      NSURLRequest* request,
+      WebStatePolicyDecider::RequestInfo request_info,
+      web::WebStatePolicyDecider::PolicyDecisionCallback callback) override;
   void ShouldAllowResponse(
       NSURLResponse* response,
       WebStatePolicyDecider::ResponseInfo response_info,
@@ -115,6 +119,11 @@
   // Used to check if the navigation should be upgraded when a response is
   // received. Cleared when the current navigation finishes.
   bool navigation_is_post_ = false;
+  // Set to true when a main frame navigation has started but not yet finished.
+  // Used to distinguish ShouldAllowRequest calls for redirects within an
+  // in-progress navigation from those for the initial request of a new
+  // navigation.
+  bool was_navigation_started_ = false;
 
   base::OneShotTimer timer_;
 
diff --git a/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper.mm b/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper.mm
index 9fb8d628..98a1bd6 100644
--- a/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper.mm
+++ b/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper.mm
@@ -141,6 +141,75 @@
 }
 
 // web::WebStatePolicyDecider
+void HttpsOnlyModeUpgradeTabHelper::ShouldAllowRequest(
+    NSURLRequest* request,
+    WebStatePolicyDecider::RequestInfo request_info,
+    base::OnceCallback<void(web::WebStatePolicyDecider::PolicyDecision)>
+        callback) {
+  GURL url = net::GURLWithNSURL(request.URL);
+  // Only handle main frame redirects to HTTP within an in-progress navigation.
+  // The initial request of a navigation is handled in ShouldAllowResponse(),
+  // because cancelling it here does not produce the DidFinishNavigation()
+  // callback that drives the upgrade state machine.
+  if (!was_navigation_started_ || !request_info.target_frame_is_main ||
+      !url.SchemeIs(url::kHttpScheme) || service_->IsFakeHTTPSForTesting(url) ||
+      IsHttpAllowedForUrl(url) || state_ == State::kFallbackStarted) {
+    std::move(callback).Run(
+        web::WebStatePolicyDecider::PolicyDecision::Allow());
+    return;
+  }
+
+  if ((!base::FeatureList::IsEnabled(
+           security_interstitials::features::kHttpsUpgrades) &&
+       !(prefs_ && prefs_->GetBoolean(prefs::kHttpsOnlyModeEnabled))) ||
+      service_->IsLocalhost(url) || navigation_is_post_) {
+    std::move(callback).Run(
+        web::WebStatePolicyDecider::PolicyDecision::Allow());
+    return;
+  }
+
+  web::NavigationItem* item_pending =
+      web_state()->GetNavigationManager()->GetPendingItem();
+  web::HttpsUpgradeType upgrade_type = item_pending
+                                           ? item_pending->GetHttpsUpgradeType()
+                                           : web::HttpsUpgradeType::kNone;
+
+  // Omnibox upgrade failures are handled in TypedNavigationUpgradeTabHelper.
+  // Ignore them here.
+  if (upgrade_type != web::HttpsUpgradeType::kNone &&
+      upgrade_type != web::HttpsUpgradeType::kHttpsOnlyMode) {
+    std::move(callback).Run(
+        web::WebStatePolicyDecider::PolicyDecision::Allow());
+    return;
+  }
+
+  // If the tab is being prerendered, cancel the prerender instead of upgrading.
+  if (PrerenderTabHelper::FromWebState(web_state())) {
+    RecordUMA(Event::kPrerenderCancelled);
+    ResetState();
+    return std::move(callback)
+        .Then(base::BindOnce(&CancelPrerender, web_state()->GetWeakPtr()))
+        .Run(web::WebStatePolicyDecider::PolicyDecision::Cancel());
+  }
+
+  if (upgrade_type == web::HttpsUpgradeType::kHttpsOnlyMode) {
+    // The previously upgraded navigation has redirected to HTTP. Stop it and
+    // let DidFinishNavigation() trigger the fallback navigation.
+    timer_.Stop();
+    state_ = State::kStoppedToFallback;
+    http_url_ = url;
+    std::move(callback).Run(
+        web::WebStatePolicyDecider::PolicyDecision::Cancel());
+    return;
+  }
+
+  // The navigation has redirected to an HTTP URL. Cancel the request before it
+  // is sent and let DidFinishNavigation() start the upgraded navigation.
+  StopToUpgrade(url,
+                item_pending ? item_pending->GetReferrer() : web::Referrer(),
+                std::move(callback));
+}
+
 void HttpsOnlyModeUpgradeTabHelper::ShouldAllowResponse(
     NSURLResponse* response,
     WebStatePolicyDecider::ResponseInfo response_info,
@@ -263,15 +332,9 @@
   }
 
   // The navigation was already upgraded but landed on an HTTP URL, possibly
-  // through redirects (e.g. upgraded HTTPS -> HTTP). In this case, show the
-  // interstitial.
-  // Note that this doesn't handle HTTP URLs in the middle of redirects such as
-  // HTTPS -> HTTP -> HTTPS. The alternative is to do this check in
-  // ShouldAllowRequest(), but we don't have enough information there to ensure
-  // whether the HTTP URL is part of the redirect chain or a completely new
-  // navigation.
-  // This is a divergence from the desktop implementation of this feature which
-  // relies on a redirect loop triggering a net error.
+  // through redirects (e.g. upgraded HTTPS -> HTTP). HTTP redirect targets are
+  // normally cancelled in ShouldAllowRequest() before being sent, but this can
+  // still be reached when the navigation uses POST. Show the interstitial.
   DCHECK(state_ == State::kUpgraded || state_ == State::kNone);
   timer_.Stop();
   state_ = State::kDone;
@@ -297,6 +360,7 @@
   if (navigation_context->IsSameDocument()) {
     return;
   }
+  was_navigation_started_ = true;
   if (state_ == State::kUpgraded) {
     DCHECK(!timer_.IsRunning());
     // `timer_` is deleted when the tab helper is deleted, so it's safe to use
@@ -323,6 +387,7 @@
     return;
   }
   navigation_is_post_ = false;
+  was_navigation_started_ = false;
   if (state_ == State::kNone) {
     return;
   }
diff --git a/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper_unittest.mm b/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper_unittest.mm
index 097f513..5ffac52 100644
--- a/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper_unittest.mm
+++ b/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper_unittest.mm
@@ -17,6 +17,7 @@
 #import "ios/components/security_interstitials/https_only_mode/https_upgrade_service.h"
 #import "ios/components/security_interstitials/https_only_mode/https_upgrade_test_util.h"
 #import "ios/web/public/navigation/web_state_policy_decider.h"
+#import "ios/web/public/test/fakes/fake_navigation_context.h"
 #import "ios/web/public/test/fakes/fake_navigation_manager.h"
 #import "ios/web/public/test/fakes/fake_web_state.h"
 #import "ios/web/public/test/web_task_environment.h"
@@ -132,6 +133,32 @@
     return policy_decision;
   }
 
+  // Helper function that calls into WebState::ShouldAllowRequest with the
+  // given `url` and `for_main_frame`, waits for the callback with the decision
+  // to be called, and returns the decision.
+  web::WebStatePolicyDecider::PolicyDecision ShouldAllowRequestUrl(
+      const GURL& url,
+      bool for_main_frame) {
+    NSURLRequest* request =
+        [NSURLRequest requestWithURL:net::NSURLWithGURL(url)];
+    __block bool callback_called = false;
+    __block web::WebStatePolicyDecider::PolicyDecision policy_decision =
+        web::WebStatePolicyDecider::PolicyDecision::Allow();
+    auto callback =
+        base::BindOnce(^(web::WebStatePolicyDecider::PolicyDecision decision) {
+          policy_decision = decision;
+          callback_called = true;
+        });
+    web::WebStatePolicyDecider::RequestInfo request_info(
+        ui::PAGE_TRANSITION_CLIENT_REDIRECT, for_main_frame,
+        /*target_frame_is_cross_origin=*/false,
+        /*target_window_is_cross_origin=*/false,
+        /*is_user_initiated=*/false, /*user_tapped_recently=*/false);
+    web_state_.ShouldAllowRequest(request, request_info, std::move(callback));
+    EXPECT_TRUE(callback_called);
+    return policy_decision;
+  }
+
   base::HistogramTester histogram_tester_;
   web::FakeWebState web_state_;
 
@@ -142,9 +169,7 @@
 };
 
 // Tests that ShouldAllowResponse properly upgrades navigations and
-// ignores subframe navigations. ShouldAllowRequest should always allow
-// the navigation.
-// Also tests that UMA records correctly.
+// ignores subframe navigations. Also tests that UMA records correctly.
 TEST_P(HttpsOnlyModeUpgradeTabHelperTest, ShouldAllowResponse) {
   // Create a navigation item.
   auto fake_navigation_manager_ =
@@ -186,6 +211,61 @@
                   .ShouldAllowNavigation());
 }
 
+// Tests that ShouldAllowRequest allows the initial request of a navigation so
+// that ShouldAllowResponse can handle the upgrade, but cancels HTTP redirect
+// targets in the middle of an in-progress main frame navigation so that the
+// redirect chain does not transit cleartext.
+TEST_P(HttpsOnlyModeUpgradeTabHelperTest, ShouldAllowRequest) {
+  auto fake_navigation_manager = std::make_unique<web::FakeNavigationManager>();
+  std::unique_ptr<web::NavigationItem> pending_item =
+      web::NavigationItem::Create();
+  fake_navigation_manager->SetPendingItem(pending_item.release());
+  web_state_.SetNavigationManager(std::move(fake_navigation_manager));
+
+  GURL https_url("https://example.com/");
+  GURL http_url("http://example.com/");
+
+  // Before the navigation has started, the initial request should be allowed
+  // regardless of scheme; ShouldAllowResponse handles upgrades for the first
+  // hop.
+  EXPECT_TRUE(ShouldAllowRequestUrl(http_url, /*main_frame=*/true)
+                  .ShouldAllowNavigation());
+  EXPECT_TRUE(ShouldAllowRequestUrl(https_url, /*main_frame=*/true)
+                  .ShouldAllowNavigation());
+
+  // Start an HTTPS main frame navigation. Subsequent ShouldAllowRequest calls
+  // simulate server redirects.
+  web::FakeNavigationContext context;
+  context.SetUrl(https_url);
+  web_state_.OnNavigationStarted(&context);
+
+  // Redirects to HTTPS should always be allowed.
+  EXPECT_TRUE(ShouldAllowRequestUrl(https_url, /*main_frame=*/true)
+                  .ShouldAllowNavigation());
+  // Subframe HTTP requests should be allowed.
+  EXPECT_TRUE(ShouldAllowRequestUrl(http_url, /*main_frame=*/false)
+                  .ShouldAllowNavigation());
+  // Allowlisted hosts shouldn't be blocked.
+  ProfileIOS* profile =
+      ProfileIOS::FromBrowserState(web_state_.GetBrowserState());
+  HttpsUpgradeService* service =
+      HttpsUpgradeServiceFactory::GetForProfile(profile);
+  service->AllowHttpForHost("allowlisted.com");
+  EXPECT_TRUE(ShouldAllowRequestUrl(GURL("http://allowlisted.com/"),
+                                    /*main_frame=*/true)
+                  .ShouldAllowNavigation());
+
+  // If either HTTPS-Only Mode or HTTPS-Upgrades is enabled, redirects to HTTP
+  // in the main frame should be cancelled before the request is sent.
+  if (GetParam() != HttpsUpgradesTestType::kNone) {
+    EXPECT_FALSE(ShouldAllowRequestUrl(http_url, /*main_frame=*/true)
+                     .ShouldAllowNavigation());
+  } else {
+    EXPECT_TRUE(ShouldAllowRequestUrl(http_url, /*main_frame=*/true)
+                    .ShouldAllowNavigation());
+  }
+}
+
 TEST_P(HttpsOnlyModeUpgradeTabHelperTest, GetUpgradedHttpsUrl) {
   ProfileIOS* profile =
       ProfileIOS::FromBrowserState(web_state_.GetBrowserState());

diff --git a/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper_unittest.mm b/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper_unittest.mm
index 097f513..5ffac52 100644
--- a/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper_unittest.mm
+++ b/ios/chrome/browser/https_upgrades/model/https_only_mode_upgrade_tab_helper_unittest.mm
@@ -17,6 +17,7 @@
 #import "ios/components/security_interstitials/https_only_mode/https_upgrade_service.h"
 #import "ios/components/security_interstitials/https_only_mode/https_upgrade_test_util.h"
 #import "ios/web/public/navigation/web_state_policy_decider.h"
+#import "ios/web/public/test/fakes/fake_navigation_context.h"
 #import "ios/web/public/test/fakes/fake_navigation_manager.h"
 #import "ios/web/public/test/fakes/fake_web_state.h"
 #import "ios/web/public/test/web_task_environment.h"
@@ -132,6 +133,32 @@
     return policy_decision;
   }
 
+  // Helper function that calls into WebState::ShouldAllowRequest with the
+  // given `url` and `for_main_frame`, waits for the callback with the decision
+  // to be called, and returns the decision.
+  web::WebStatePolicyDecider::PolicyDecision ShouldAllowRequestUrl(
+      const GURL& url,
+      bool for_main_frame) {
+    NSURLRequest* request =
+        [NSURLRequest requestWithURL:net::NSURLWithGURL(url)];
+    __block bool callback_called = false;
+    __block web::WebStatePolicyDecider::PolicyDecision policy_decision =
+        web::WebStatePolicyDecider::PolicyDecision::Allow();
+    auto callback =
+        base::BindOnce(^(web::WebStatePolicyDecider::PolicyDecision decision) {
+          policy_decision = decision;
+          callback_called = true;
+        });
+    web::WebStatePolicyDecider::RequestInfo request_info(
+        ui::PAGE_TRANSITION_CLIENT_REDIRECT, for_main_frame,
+        /*target_frame_is_cross_origin=*/false,
+        /*target_window_is_cross_origin=*/false,
+        /*is_user_initiated=*/false, /*user_tapped_recently=*/false);
+    web_state_.ShouldAllowRequest(request, request_info, std::move(callback));
+    EXPECT_TRUE(callback_called);
+    return policy_decision;
+  }
+
   base::HistogramTester histogram_tester_;
   web::FakeWebState web_state_;
 
@@ -142,9 +169,7 @@
 };
 
 // Tests that ShouldAllowResponse properly upgrades navigations and
-// ignores subframe navigations. ShouldAllowRequest should always allow
-// the navigation.
-// Also tests that UMA records correctly.
+// ignores subframe navigations. Also tests that UMA records correctly.
 TEST_P(HttpsOnlyModeUpgradeTabHelperTest, ShouldAllowResponse) {
   // Create a navigation item.
   auto fake_navigation_manager_ =
@@ -186,6 +211,61 @@
                   .ShouldAllowNavigation());
 }
 
+// Tests that ShouldAllowRequest allows the initial request of a navigation so
+// that ShouldAllowResponse can handle the upgrade, but cancels HTTP redirect
+// targets in the middle of an in-progress main frame navigation so that the
+// redirect chain does not transit cleartext.
+TEST_P(HttpsOnlyModeUpgradeTabHelperTest, ShouldAllowRequest) {
+  auto fake_navigation_manager = std::make_unique<web::FakeNavigationManager>();
+  std::unique_ptr<web::NavigationItem> pending_item =
+      web::NavigationItem::Create();
+  fake_navigation_manager->SetPendingItem(pending_item.release());
+  web_state_.SetNavigationManager(std::move(fake_navigation_manager));
+
+  GURL https_url("https://example.com/");
+  GURL http_url("http://example.com/");
+
+  // Before the navigation has started, the initial request should be allowed
+  // regardless of scheme; ShouldAllowResponse handles upgrades for the first
+  // hop.
+  EXPECT_TRUE(ShouldAllowRequestUrl(http_url, /*main_frame=*/true)
+                  .ShouldAllowNavigation());
+  EXPECT_TRUE(ShouldAllowRequestUrl(https_url, /*main_frame=*/true)
+                  .ShouldAllowNavigation());
+
+  // Start an HTTPS main frame navigation. Subsequent ShouldAllowRequest calls
+  // simulate server redirects.
+  web::FakeNavigationContext context;
+  context.SetUrl(https_url);
+  web_state_.OnNavigationStarted(&context);
+
+  // Redirects to HTTPS should always be allowed.
+  EXPECT_TRUE(ShouldAllowRequestUrl(https_url, /*main_frame=*/true)
+                  .ShouldAllowNavigation());
+  // Subframe HTTP requests should be allowed.
+  EXPECT_TRUE(ShouldAllowRequestUrl(http_url, /*main_frame=*/false)
+                  .ShouldAllowNavigation());
+  // Allowlisted hosts shouldn't be blocked.
+  ProfileIOS* profile =
+      ProfileIOS::FromBrowserState(web_state_.GetBrowserState());
+  HttpsUpgradeService* service =
+      HttpsUpgradeServiceFactory::GetForProfile(profile);
+  service->AllowHttpForHost("allowlisted.com");
+  EXPECT_TRUE(ShouldAllowRequestUrl(GURL("http://allowlisted.com/"),
+                                    /*main_frame=*/true)
+                  .ShouldAllowNavigation());
+
+  // If either HTTPS-Only Mode or HTTPS-Upgrades is enabled, redirects to HTTP
+  // in the main frame should be cancelled before the request is sent.
+  if (GetParam() != HttpsUpgradesTestType::kNone) {
+    EXPECT_FALSE(ShouldAllowRequestUrl(http_url, /*main_frame=*/true)
+                     .ShouldAllowNavigation());
+  } else {
+    EXPECT_TRUE(ShouldAllowRequestUrl(http_url, /*main_frame=*/true)
+                    .ShouldAllowNavigation());
+  }
+}
+
 TEST_P(HttpsOnlyModeUpgradeTabHelperTest, GetUpgradedHttpsUrl) {
   ProfileIOS* profile =
       ProfileIOS::FromBrowserState(web_state_.GetBrowserState());