From 40aa5a35e86fe4c46f19655c29984e93a1b20b5f Mon Sep 17 00:00:00 2001
From: Nidhi Jaju <nidhijaju@chromium.org>
Date: Thu, 13 Aug 2026 20:48:55 -0700
Subject: [PATCH] [Navigation] Move redirect check bypass state to NavigationHandle

Currently, bypass_redirect_checks is passed across process boundaries
in network::mojom::URLResponseHead. However, this flag is only ever
intended to be set by browser-process proxying URLLoaderFactories
(such as the WebRequest API proxying loader factory on the UI thread)
when authorizing a redirect.

This CL removes bypass_redirect_checks from URLResponseHead and instead
records the authorization state directly on the UI-thread
NavigationHandle/NavigationRequest identified by the navigation_id.
When NavigationURLLoaderImpl receives a redirect, it consumes the
in-memory flag from NavigationHandle rather than reading from
URLResponseHead.

Bug: 498732709
Change-Id: I62d17b56bac8a94f7be03772620149b9ab9ba9af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8254718
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Andrea Orru <andreaorru@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1679332}
---

diff --git a/content/browser/blob_storage/blob_url_browsertest.cc b/content/browser/blob_storage/blob_url_browsertest.cc
index 08c747b..5010588b 100644
--- a/content/browser/blob_storage/blob_url_browsertest.cc
+++ b/content/browser/blob_storage/blob_url_browsertest.cc
@@ -293,7 +293,6 @@
     head->headers = net::HttpResponseHeaders::TryToCreate(
         "HTTP/1.1 302 Found\r\nLocation: " + redirect_target_.spec() + "\r\n");
     head->encoded_data_length = 0;
-    head->bypass_redirect_checks = true;
     client_->OnReceiveRedirect(redirect_info, std::move(head));
   }
   void ReadSideData(ReadSideDataCallback) override { NOTREACHED(); }
diff --git a/content/browser/loader/navigation_url_loader_impl.cc b/content/browser/loader/navigation_url_loader_impl.cc
index 6c52418..bf56800 100644
--- a/content/browser/loader/navigation_url_loader_impl.cc
+++ b/content/browser/loader/navigation_url_loader_impl.cc
@@ -66,6 +66,7 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_utils.h"
 #include "content/public/browser/frame_accept_header.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/navigation_ui_data.h"
 #include "content/public/browser/network_service_instance.h"
 #include "content/public/browser/network_service_util.h"
@@ -1567,10 +1568,27 @@
                         resource_request().is_outermost_main_frame);
   net::Error error = net::OK;
 
-  bool bypass_redirect_checks =
-      base::FeatureList::IsEnabled(features::kBypassRedirectChecksPerRequest)
-          ? head->bypass_redirect_checks
-          : bypass_redirect_checks_;
+  bool bypass_redirect_checks = false;
+  if (base::FeatureList::IsEnabled(features::kBypassRedirectChecksPerRequest)) {
+    // A proxying URLLoaderFactory may authorize a redirect to bypass safety
+    // checks. This authorization is set directly on the NavigationRequest
+    // in the browser process. NavigationURLLoaderImpl doesn't have a direct
+    // pointer to the NavigationRequest, so we look it up via the
+    // FrameTreeNode. We check the navigation ID to ensure we don't apply the
+    // bypass to a different navigation in the same frame.
+    if (FrameTreeNode* frame_tree_node =
+            FrameTreeNode::GloballyFindByID(frame_tree_node_id_)) {
+      if (NavigationRequest* nav_request =
+              frame_tree_node->navigation_request()) {
+        if (nav_request->GetNavigationId() == request_info_->navigation_id) {
+          bypass_redirect_checks =
+              nav_request->ConsumeBypassRedirectChecksForNextRedirect();
+        }
+      }
+    }
+  } else {
+    bypass_redirect_checks = bypass_redirect_checks_;
+  }
 
   if (url_.SchemeIsBlob()) {
     // Loading a blob URL never produces a redirect.
diff --git a/content/browser/loader/navigation_url_loader_impl_unittest.cc b/content/browser/loader/navigation_url_loader_impl_unittest.cc
index d756567..df925305 100644
--- a/content/browser/loader/navigation_url_loader_impl_unittest.cc
+++ b/content/browser/loader/navigation_url_loader_impl_unittest.cc
@@ -30,6 +30,7 @@
 #include "content/public/browser/navigation_ui_data.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/buildflags.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/mock_client_hints_controller_delegate.h"
@@ -38,6 +39,7 @@
 #include "content/public/test/test_renderer_host.h"
 #include "content/test/test_navigation_url_loader_delegate.h"
 #include "content/test/test_web_contents.h"
+#include "mojo/public/cpp/bindings/remote.h"
 #include "net/base/load_flags.h"
 #include "net/base/mock_network_change_notifier.h"
 #include "net/proxy_resolution/configured_proxy_resolution_service.h"
@@ -200,10 +202,12 @@
             nullptr /* blob_url_loader_factory */,
             base::UnguessableToken::Create() /* devtools_navigation_token */,
             base::UnguessableToken::Create() /* devtools_frame_token */,
-            nullptr /* client_security_state */,
-            false /* is_pdf */, ChildProcessId() /* initiator_process_id */,
+            nullptr /* client_security_state */, false /* is_pdf */,
+            ChildProcessId() /* initiator_process_id */,
             std::nullopt /* initiator_document_token */,
-            false /* allow_cookies_from_browser */, 0 /* navigation_id */,
+            false /* allow_cookies_from_browser */,
+            pending_navigation_->GetNavigationHandle()
+                ->GetNavigationId() /* navigation_id */,
             is_ad_tagged /* is_ad_tagged */,
             false /* force_no_https_upgrade */));
 
@@ -698,6 +702,78 @@
   bool should_redirect_ = true;
 };
 
+// A `NavigationLoaderInterceptor` that intercepts the request via
+// `MaybeCreateLoader()` and immediately issues a redirect to `redirect_url`
+// with `URLResponseHead::bypass_redirect_checks` set to the supplied value.
+class TestRedirectInterceptor final : public NavigationLoaderInterceptor {
+ public:
+  TestRedirectInterceptor(const GURL& redirect_url,
+                          bool bypass_redirect_checks,
+                          int64_t* navigation_id_ptr = nullptr,
+                          FrameTreeNodeId* frame_tree_node_id = nullptr)
+      : redirect_url_(redirect_url),
+        bypass_redirect_checks_(bypass_redirect_checks),
+        navigation_id_ptr_(navigation_id_ptr),
+        frame_tree_node_id_ptr_(frame_tree_node_id) {}
+  ~TestRedirectInterceptor() override = default;
+
+ private:
+  void MaybeCreateLoader(
+      const network::ResourceRequest& tentative_resource_request,
+      BrowserContext* browser_context,
+      LoaderCallback callback,
+      FallbackCallback fallback_callback) override {
+    auto factory = base::MakeRefCounted<network::SingleRequestURLLoaderFactory>(
+        base::BindOnce(&TestRedirectInterceptor::HandleRequest,
+                       base::Unretained(this)));
+    std::move(callback).Run(NavigationLoaderInterceptor::Result(
+        std::move(factory), SubresourceLoaderParams()));
+  }
+
+  bool MaybeCreateLoaderForResponse(
+      const network::URLLoaderCompletionStatus& status,
+      const network::ResourceRequest& request,
+      network::mojom::URLResponseHeadPtr* response_head,
+      mojo::ScopedDataPipeConsumerHandle* response_body,
+      mojo::PendingReceiver<network::mojom::URLLoaderClient>* client_receiver,
+      blink::ThrottlingURLLoader* url_loader,
+      bool* skip_other_interceptors) override {
+    return false;
+  }
+
+  void HandleRequest(
+      const network::ResourceRequest& request,
+      mojo::PendingReceiver<network::mojom::URLLoader> loader,
+      mojo::PendingRemote<network::mojom::URLLoaderClient> client) {
+    loader_receiver_ = std::move(loader);
+    client_.Bind(std::move(client));
+
+    auto head = network::mojom::URLResponseHead::New();
+    if (bypass_redirect_checks_ && navigation_id_ptr_ &&
+        *navigation_id_ptr_ != 0 && frame_tree_node_id_ptr_) {
+      NavigationHandle::SetBypassRedirectChecksForNextRedirect(
+          *frame_tree_node_id_ptr_, *navigation_id_ptr_);
+    }
+    net::RedirectInfo redirect_info = net::RedirectInfo::ComputeRedirectInfo(
+        request.method, request.url, request.site_for_cookies,
+        request.update_first_party_url_on_redirect
+            ? net::RedirectInfo::FirstPartyURLPolicy::UPDATE_URL_ON_REDIRECT
+            : net::RedirectInfo::FirstPartyURLPolicy::NEVER_CHANGE_URL,
+        request.referrer_policy, request.referrer.spec(),
+        request.request_initiator, net::HTTP_TEMPORARY_REDIRECT, redirect_url_,
+        /*referrer_policy_header=*/std::nullopt,
+        /*insecure_scheme_was_upgraded=*/false);
+    client_->OnReceiveRedirect(redirect_info, std::move(head));
+  }
+
+  const GURL redirect_url_;
+  const bool bypass_redirect_checks_;
+  const raw_ptr<int64_t> navigation_id_ptr_;
+  const raw_ptr<FrameTreeNodeId> frame_tree_node_id_ptr_;
+  mojo::PendingReceiver<network::mojom::URLLoader> loader_receiver_;
+  mojo::Remote<network::mojom::URLLoaderClient> client_;
+};
+
 // This sets the timeout timer but doesn't expect the timer is fired
 // automatically. If needed, the timer should be fired explicitly e.g. via
 // `TriggerTimeoutForTesting()`.
@@ -1370,6 +1446,68 @@

diff --git a/content/browser/blob_storage/blob_url_browsertest.cc b/content/browser/blob_storage/blob_url_browsertest.cc
index 08c747b..5010588b 100644
--- a/content/browser/blob_storage/blob_url_browsertest.cc
+++ b/content/browser/blob_storage/blob_url_browsertest.cc
@@ -293,7 +293,6 @@
     head->headers = net::HttpResponseHeaders::TryToCreate(
         "HTTP/1.1 302 Found\r\nLocation: " + redirect_target_.spec() + "\r\n");
     head->encoded_data_length = 0;
-    head->bypass_redirect_checks = true;
     client_->OnReceiveRedirect(redirect_info, std::move(head));
   }
   void ReadSideData(ReadSideDataCallback) override { NOTREACHED(); }
diff --git a/content/browser/loader/navigation_url_loader_impl_unittest.cc b/content/browser/loader/navigation_url_loader_impl_unittest.cc
index d756567..df925305 100644
--- a/content/browser/loader/navigation_url_loader_impl_unittest.cc
+++ b/content/browser/loader/navigation_url_loader_impl_unittest.cc
@@ -30,6 +30,7 @@
 #include "content/public/browser/navigation_ui_data.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/buildflags.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/mock_client_hints_controller_delegate.h"
@@ -38,6 +39,7 @@
 #include "content/public/test/test_renderer_host.h"
 #include "content/test/test_navigation_url_loader_delegate.h"
 #include "content/test/test_web_contents.h"
+#include "mojo/public/cpp/bindings/remote.h"
 #include "net/base/load_flags.h"
 #include "net/base/mock_network_change_notifier.h"
 #include "net/proxy_resolution/configured_proxy_resolution_service.h"
@@ -200,10 +202,12 @@
             nullptr /* blob_url_loader_factory */,
             base::UnguessableToken::Create() /* devtools_navigation_token */,
             base::UnguessableToken::Create() /* devtools_frame_token */,
-            nullptr /* client_security_state */,
-            false /* is_pdf */, ChildProcessId() /* initiator_process_id */,
+            nullptr /* client_security_state */, false /* is_pdf */,
+            ChildProcessId() /* initiator_process_id */,
             std::nullopt /* initiator_document_token */,
-            false /* allow_cookies_from_browser */, 0 /* navigation_id */,
+            false /* allow_cookies_from_browser */,
+            pending_navigation_->GetNavigationHandle()
+                ->GetNavigationId() /* navigation_id */,
             is_ad_tagged /* is_ad_tagged */,
             false /* force_no_https_upgrade */));
 
@@ -698,6 +702,78 @@
   bool should_redirect_ = true;
 };
 
+// A `NavigationLoaderInterceptor` that intercepts the request via
+// `MaybeCreateLoader()` and immediately issues a redirect to `redirect_url`
+// with `URLResponseHead::bypass_redirect_checks` set to the supplied value.
+class TestRedirectInterceptor final : public NavigationLoaderInterceptor {
+ public:
+  TestRedirectInterceptor(const GURL& redirect_url,
+                          bool bypass_redirect_checks,
+                          int64_t* navigation_id_ptr = nullptr,
+                          FrameTreeNodeId* frame_tree_node_id = nullptr)
+      : redirect_url_(redirect_url),
+        bypass_redirect_checks_(bypass_redirect_checks),
+        navigation_id_ptr_(navigation_id_ptr),
+        frame_tree_node_id_ptr_(frame_tree_node_id) {}
+  ~TestRedirectInterceptor() override = default;
+
+ private:
+  void MaybeCreateLoader(
+      const network::ResourceRequest& tentative_resource_request,
+      BrowserContext* browser_context,
+      LoaderCallback callback,
+      FallbackCallback fallback_callback) override {
+    auto factory = base::MakeRefCounted<network::SingleRequestURLLoaderFactory>(
+        base::BindOnce(&TestRedirectInterceptor::HandleRequest,
+                       base::Unretained(this)));
+    std::move(callback).Run(NavigationLoaderInterceptor::Result(
+        std::move(factory), SubresourceLoaderParams()));
+  }
+
+  bool MaybeCreateLoaderForResponse(
+      const network::URLLoaderCompletionStatus& status,
+      const network::ResourceRequest& request,
+      network::mojom::URLResponseHeadPtr* response_head,
+      mojo::ScopedDataPipeConsumerHandle* response_body,
+      mojo::PendingReceiver<network::mojom::URLLoaderClient>* client_receiver,
+      blink::ThrottlingURLLoader* url_loader,
+      bool* skip_other_interceptors) override {
+    return false;
+  }
+
+  void HandleRequest(
+      const network::ResourceRequest& request,
+      mojo::PendingReceiver<network::mojom::URLLoader> loader,
+      mojo::PendingRemote<network::mojom::URLLoaderClient> client) {
+    loader_receiver_ = std::move(loader);
+    client_.Bind(std::move(client));
+
+    auto head = network::mojom::URLResponseHead::New();
+    if (bypass_redirect_checks_ && navigation_id_ptr_ &&
+        *navigation_id_ptr_ != 0 && frame_tree_node_id_ptr_) {
+      NavigationHandle::SetBypassRedirectChecksForNextRedirect(
+          *frame_tree_node_id_ptr_, *navigation_id_ptr_);
+    }
+    net::RedirectInfo redirect_info = net::RedirectInfo::ComputeRedirectInfo(
+        request.method, request.url, request.site_for_cookies,
+        request.update_first_party_url_on_redirect
+            ? net::RedirectInfo::FirstPartyURLPolicy::UPDATE_URL_ON_REDIRECT
+            : net::RedirectInfo::FirstPartyURLPolicy::NEVER_CHANGE_URL,
+        request.referrer_policy, request.referrer.spec(),
+        request.request_initiator, net::HTTP_TEMPORARY_REDIRECT, redirect_url_,
+        /*referrer_policy_header=*/std::nullopt,
+        /*insecure_scheme_was_upgraded=*/false);
+    client_->OnReceiveRedirect(redirect_info, std::move(head));
+  }
+
+  const GURL redirect_url_;
+  const bool bypass_redirect_checks_;
+  const raw_ptr<int64_t> navigation_id_ptr_;
+  const raw_ptr<FrameTreeNodeId> frame_tree_node_id_ptr_;
+  mojo::PendingReceiver<network::mojom::URLLoader> loader_receiver_;
+  mojo::Remote<network::mojom::URLLoaderClient> client_;
+};
+
 // This sets the timeout timer but doesn't expect the timer is fired
 // automatically. If needed, the timer should be fired explicitly e.g. via
 // `TriggerTimeoutForTesting()`.
@@ -1370,6 +1446,68 @@
       Optional(std::string("Value3")));
 }
 
+// `URLResponseHead::bypass_redirect_checks` is delivered over the
+// `URLLoaderClient` pipe and must not by itself allow a redirect to a target
+// that fails `IsSafeRedirectTarget()`. The per-request bit is only honored when
+// the loader factory in use was created with `bypass_redirect_checks` set
+// (i.e., a browser-process proxy is responsible for the redirect).
+TEST_F(NavigationURLLoaderImplTest,
+       PerRequestBypassRedirectChecksRequiresFactoryFlag) {
+  base::test::ScopedFeatureList feature_list{
+      features::kBypassRedirectChecksPerRequest};
+  ASSERT_TRUE(http_test_server_.Start());
+
+  for (bool bypass : {true, false}) {
+    SCOPED_TRACE(testing::Message() << "bypass_redirect_checks=" << bypass);
+    TestNavigationURLLoaderDelegate delegate;
+    std::vector<std::unique_ptr<NavigationLoaderInterceptor>> interceptors;
+    interceptors.push_back(std::make_unique<TestRedirectInterceptor>(
+        GURL("file:///"), /*bypass_redirect_checks=*/bypass));
+    auto loader =
+        CreateTestLoader(http_test_server_.GetURL("/echo"), std::string(),
+                         "GET", &delegate, blink::NavigationDownloadPolicy(),
+                         /*is_main_frame=*/true,
+                         /*upgrade_if_insecure=*/false,
+                         /*is_ad_tagged=*/false, std::move(interceptors));
+    loader->Start();
+    delegate.WaitForRequestFailed();
+    EXPECT_EQ(delegate.on_redirect_handled_counter(), 0);
+    EXPECT_EQ(delegate.on_request_handled_counter(), 1);
+    EXPECT_EQ(net::ERR_UNSAFE_REDIRECT, delegate.net_error());
+  }
+}
+
+// When a browser-process proxy authorizes bypassing redirect checks via
+// authorizes bypassing redirect checks via
+// `NavigationHandle::SetBypassRedirectChecksForNextRedirect()`, the redirect is
+// allowed even if `IsSafeRedirectTarget()` would normally fail.
+TEST_F(NavigationURLLoaderImplTest,
+       PerRequestBypassRedirectChecksSucceedsWhenAuthorizedByProxy) {
+  base::test::ScopedFeatureList feature_list{
+      features::kBypassRedirectChecksPerRequest};
+  ASSERT_TRUE(http_test_server_.Start());
+
+  int64_t navigation_id = 0;
+  FrameTreeNodeId frame_tree_node_id;
+  TestNavigationURLLoaderDelegate delegate;
+  std::vector<std::unique_ptr<NavigationLoaderInterceptor>> interceptors;
+  interceptors.push_back(std::make_unique<TestRedirectInterceptor>(
+      GURL("file:///"), /*bypass_redirect_checks=*/true, &navigation_id,
+      &frame_tree_node_id));
+  auto loader =
+      CreateTestLoader(http_test_server_.GetURL("/echo"), std::string(), "GET",
+                       &delegate, blink::NavigationDownloadPolicy(),
+                       /*is_main_frame=*/true,
+                       /*upgrade_if_insecure=*/false,
+                       /*is_ad_tagged=*/false, std::move(interceptors));
+  navigation_id = pending_navigation_->GetNavigationHandle()->GetNavigationId();
+  frame_tree_node_id =
+      pending_navigation_->GetNavigationHandle()->GetFrameTreeNodeId();
+  loader->Start();
+  delegate.WaitForRequestRedirected();
+  EXPECT_EQ(delegate.on_redirect_handled_counter(), 1);
+}
+
 // Tests that the Upgrade If Insecure flag is obeyed.
 TEST_F(NavigationURLLoaderImplTest, UpgradeIfInsecureTest) {
   ASSERT_TRUE(http_test_server_.Start());
diff --git a/content/public/test/mock_navigation_handle.h b/content/public/test/mock_navigation_handle.h
index 155acda..3389103 100644
--- a/content/public/test/mock_navigation_handle.h
+++ b/content/public/test/mock_navigation_handle.h
@@ -280,6 +280,8 @@
               ());
   MOCK_METHOD(bool, NeedsUrlLoader, ());
   MOCK_METHOD(bool, IsInitialWebUINavigation, ());
+  MOCK_METHOD(void, SetBypassRedirectChecksForNextRedirect, (bool));
+  MOCK_METHOD(bool, ConsumeBypassRedirectChecksForNextRedirect, ());
 
 #if BUILDFLAG(IS_ANDROID)
   MOCK_METHOD(const base::android::JavaRef<jobject>&,