From 42ecabae97984e472af529bdcc828c3377ef7f98 Mon Sep 17 00:00:00 2001
From: Liam Brady <lbrady@google.com>
Date: Tue, 26 May 2026 15:27:08 -0700
Subject: [PATCH] [Disabled] Sanitize more commit params for cross-origin redirects.

The `NavigationRequest::SanitizeRedirectsForCommit` method was
introduced to prevent sensitive cross-site URL information (such as
query parameters) from making it to a committed navigation's renderer if
the navigation was the result of a redirect or redirect chain. This
method sanitized the commit params' `redirects` and `redirect_infos`
fields. However, it did not account for information being in either the
`original_url` field or in "Location" headers in `redirect_response`.
This CL sanitizes those fields.

This CL is behind two new default-disabled feature flags, so this will
essentially be a no-op. This is done because we expect some features to
break with the feature enabled. They will need to be modified to work
with these changes. That will be done in follow-up CLs. Landing this CL
will not actively break anything since the features will be turned off.

The end goal is to fully replace `original_url` with an Origin type
`original_origin` field, which will happen once all features are working
as expected with this change.

Bug: 495463654
Change-Id: I87a086725cf8b3acbdfea94cacbbb72e56033f23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7851593
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Liam Brady <lbrady@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1636524}
---

diff --git a/content/browser/renderer_host/navigation_controller_impl.cc b/content/browser/renderer_host/navigation_controller_impl.cc
index 1e23d33..d3b70f8 100644
--- a/content/browser/renderer_host/navigation_controller_impl.cc
+++ b/content/browser/renderer_host/navigation_controller_impl.cc
@@ -83,6 +83,7 @@
 #include "content/browser/site_instance_impl.h"
 #include "content/common/content_constants_internal.h"
 #include "content/common/content_navigation_policy.h"
+#include "content/common/features.h"
 #include "content/common/navigation_params_utils.h"
 #include "content/common/trace_utils.h"
 #include "content/public/browser/back_forward_transition_animation_manager.h"
@@ -4652,6 +4653,16 @@
           false /* is_history_navigation_in_new_child_frame */,
           params.input_start, network::mojom::RequestDestination::kEmpty);
 
+  // It is safe to convert GURL to an Origin and back in the code below because
+  // we only want to discard the rest of the URL (e.g., path and params). The
+  // actual underlying Origin is not needed, which could be inherited or opaque
+  // in sandbox cases.
+  const GURL original_url_for_renderer =
+      base::FeatureList::IsEnabled(
+          features::kSanitizeOriginalUrlDuringNavigation)
+          ? common_params->url.DeprecatedGetOriginAsURL()
+          : common_params->url;
+
   blink::mojom::CommitNavigationParamsPtr commit_params =
       blink::mojom::CommitNavigationParams::New(
           url::Origin(),
@@ -4660,7 +4671,7 @@
           blink::StorageKey(), override_user_agent, params.redirect_chain,
           std::vector<network::mojom::URLResponseHeadPtr>(),
           std::vector<net::RedirectInfo>(), params.post_content_type,
-          common_params->url, common_params->method,
+          original_url_for_renderer, common_params->method,
           params.can_load_local_resources, page_state_data,
           entry->GetUniqueID(), entry->GetSubframeUniqueNames(node),
           /*intended_as_new_entry=*/true,
@@ -4859,12 +4870,22 @@
   common_params->is_history_navigation_in_new_child_frame =
       is_history_navigation_in_new_child_frame;
 
+  // It is safe to convert GURL to an Origin and back in the code below because
+  // we only want to discard the rest of the URL (e.g., path and params). The
+  // actual underlying Origin is not needed, which could be inherited or opaque
+  // in sandbox cases.
+  const GURL original_url_for_renderer =
+      base::FeatureList::IsEnabled(
+          features::kSanitizeOriginalUrlDuringNavigation)
+          ? common_params->url.DeprecatedGetOriginAsURL()
+          : common_params->url;
+
   // TODO(clamy): |intended_as_new_entry| below should always be false once
   // Reload no longer leads to this being called for a pending NavigationEntry
   // of index -1.
   blink::mojom::CommitNavigationParamsPtr commit_params =
       entry->ConstructCommitNavigationParams(
-          *frame_entry, common_params->url, common_params->method,
+          *frame_entry, original_url_for_renderer, common_params->method,
           entry->GetSubframeUniqueNames(frame_tree_node),
           GetPendingEntryIndex() == -1 /* intended_as_new_entry */,
           GetIndexOfEntry(entry), GetLastCommittedEntryIndex(), GetEntryCount(),
@@ -5733,7 +5754,15 @@
 
   blink::mojom::CommitNavigationParamsPtr commit_params =
       blink::CreateCommitNavigationParams();
-  commit_params->original_url = common_params->url;
+  // It is safe to convert GURL to an Origin and back in the code below because
+  // we only want to discard the rest of the URL (e.g., path and params). The
+  // actual underlying Origin is not needed, which could be inherited or opaque
+  // in sandbox cases.
+  commit_params->original_url =
+      base::FeatureList::IsEnabled(
+          features::kSanitizeOriginalUrlDuringNavigation)
+          ? common_params->url.DeprecatedGetOriginAsURL()
+          : common_params->url;
 
   // TODO(arthursonzogni): Consider providing the minimal capabilities to the
   // error pages.
diff --git a/content/browser/renderer_host/navigation_entry_impl.cc b/content/browser/renderer_host/navigation_entry_impl.cc
index 5de8aef..f637f70f 100644
--- a/content/browser/renderer_host/navigation_entry_impl.cc
+++ b/content/browser/renderer_host/navigation_entry_impl.cc
@@ -12,6 +12,7 @@
 #include <utility>
 
 #include "base/containers/queue.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/i18n/rtl.h"
 #include "base/memory/ptr_util.h"
@@ -28,6 +29,7 @@
 #include "content/browser/renderer_host/navigation_entry_restore_context_impl.h"
 #include "content/browser/renderer_host/navigation_request.h"
 #include "content/common/content_constants_internal.h"
+#include "content/common/features.h"
 #include "content/public/browser/reload_type.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/common/url_constants.h"
@@ -1032,6 +1034,12 @@
     current_length_to_send = 0;
   }
 
+  const GURL original_url_for_renderer =
+      base::FeatureList::IsEnabled(
+          features::kSanitizeOriginalUrlDuringNavigation)
+          ? original_url.DeprecatedGetOriginAsURL()
+          : original_url;
+
   blink::mojom::CommitNavigationParamsPtr commit_params =
       blink::mojom::CommitNavigationParams::New(
           url::Origin(),
@@ -1039,12 +1047,12 @@
           // navigation.
           blink::StorageKey(), GetIsOverridingUserAgent(), redirects,
           std::vector<network::mojom::URLResponseHeadPtr>(),
-          std::vector<net::RedirectInfo>(), std::string(), original_url,
-          original_method, GetCanLoadLocalResources(),
-          frame_entry.page_state().ToEncodedData(), GetUniqueID(),
-          subframe_unique_names, intended_as_new_entry, pending_index_to_send,
-          current_index_to_send, current_length_to_send, false,
-          IsViewSourceMode(), should_clear_history_list(),
+          std::vector<net::RedirectInfo>(), std::string(),
+          original_url_for_renderer, original_method,
+          GetCanLoadLocalResources(), frame_entry.page_state().ToEncodedData(),
+          GetUniqueID(), subframe_unique_names, intended_as_new_entry,
+          pending_index_to_send, current_index_to_send, current_length_to_send,
+          false, IsViewSourceMode(), should_clear_history_list(),
           blink::mojom::NavigationTiming::New(),
           blink::mojom::WasActivatedOption::kUnknown,
           base::UnguessableToken::Create(),
diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index b50c2691..23237a4 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -1391,6 +1391,16 @@
   common_params->request_destination =
       GetDestinationFromFrameTreeNode(frame_tree_node);
 
+  GURL original_url = common_params->url;
+  if (base::FeatureList::IsEnabled(
+          features::kSanitizeOriginalUrlDuringNavigation)) {
+    // It is safe to convert GURL to an Origin and back in the code below
+    // because we only want to discard the rest of the URL (e.g., path and
+    // params). The actual underlying Origin is not needed, which could be
+    // inherited or opaque in sandbox cases.
+    original_url = common_params->url.DeprecatedGetOriginAsURL();
+  }
+
   // TODO(clamy): See if the navigation start time should be measured in the
   // renderer and sent to the browser instead of being measured here.
   blink::mojom::CommitNavigationParamsPtr commit_params =
@@ -1403,7 +1413,7 @@
           /*redirect_response=*/
           std::vector<network::mojom::URLResponseHeadPtr>(),
           /*redirect_infos=*/std::vector<net::RedirectInfo>(),
-          /*post_content_type=*/std::string(), common_params->url,
+          /*post_content_type=*/std::string(), original_url,
           common_params->method,
           /*can_load_local_resources=*/false,
           /*page_state=*/std::string(),
@@ -1553,6 +1563,15 @@
   // by navigations that went through the browser (e.g. page_state is only
   // set in CommitNavigationParams of history navigations) or these values are
   // not used by the browser after commit.
+  // It is safe to convert GURL to an Origin and back in the code below because
+  // we only want to discard the rest of the URL (e.g., path and params). The

diff --git a/content/browser/renderer_host/navigation_request_unittest.cc b/content/browser/renderer_host/navigation_request_unittest.cc
index 0defdb6..8a8acf35 100644
--- a/content/browser/renderer_host/navigation_request_unittest.cc
+++ b/content/browser/renderer_host/navigation_request_unittest.cc
@@ -15,6 +15,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "content/browser/renderer_host/navigation_throttle_runner.h"
+#include "content/common/features.h"
 #include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/origin_trials_controller_delegate.h"
 #include "content/public/browser/process_selection_user_data.h"
@@ -890,9 +891,216 @@
   EXPECT_EQ(GURL("https://c.com"), commit_params->redirects[2]);
 }
 
+// Test to ensure that relative Location headers are handled correctly during
+// sanitization (not cleared if same-origin, and sanitized to origin if
+// cross-origin).
+TEST_F(NavigationRequestTest, SanitizeRedirectsForCommitRelativeLocation) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      /*enabled_features=*/{features::kSanitizeLocationHeadersDuringNavigation,
+                            features::kSanitizeOriginalUrlDuringNavigation},
+      /*disabled_features=*/{});
+  const GURL start_url("https://a.com/start");
+  const GURL url_2("https://a.com/foo");
+  const GURL url_3("https://b.com/bar");
+  const GURL url_4("https://b.com/baz");
+  const GURL final_url("https://b.com/final");
+
+  std::unique_ptr<NavigationSimulator> navigation =
+      NavigationSimulator::CreateRendererInitiated(start_url, main_test_rfh());
+  navigation->Start();
+
+  // 1. Redirect to same-site (relative). Cross-origin to final URL.
+  auto headers1 =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 302 Found");
+  headers1->SetHeader("Location", "/foo");
+  navigation->SetRedirectHeaders(headers1);
+  navigation->Redirect(url_2);
+
+  // 2. Redirect to cross-site (absolute). Same-origin to final URL.
+  auto headers2 =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 302 Found");
+  headers2->SetHeader("Location", "https://b.com/bar");
+  navigation->SetRedirectHeaders(headers2);
+  navigation->Redirect(url_3);
+
+  // 3. Redirect to same-site (relative). Same-origin to final URL.
+  auto headers3 =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 302 Found");
+  headers3->SetHeader("Location", "/baz");
+  navigation->SetRedirectHeaders(headers3);
+  navigation->Redirect(url_4);
+
+  // Final navigation to D.
+  navigation->Redirect(final_url);
+
+  NavigationRequest* request =
+      NavigationRequest::From(navigation->GetNavigationHandle());
+  auto commit_params = request->commit_params().Clone();
+
+  request->SanitizeRedirectsForCommit(commit_params);
+
+  EXPECT_EQ(4u, commit_params->redirect_response.size());
+
+  size_t iter = 0;
+  std::optional<std::string_view> location;
+
+  // 1. "Location: /foo" resolves to cross-origin URL. Should be sanitized to
+  // origin.
+  location = commit_params->redirect_response[0]->headers->EnumerateHeader(
+      &iter, "Location");
+  ASSERT_TRUE(location.has_value());
+  EXPECT_EQ("https://a.com/", location.value());
+
+  // 2. "Location: https://b.com/bar" is same-origin to final URL. Should be
+  // left alone.
+  iter = 0;
+  location = commit_params->redirect_response[1]->headers->EnumerateHeader(
+      &iter, "Location");
+  ASSERT_TRUE(location.has_value());
+  EXPECT_EQ("https://b.com/bar", location.value());
+
+  // 3. "Location: /baz" resolves to same-origin URL. Should be left alone as
+  // relative URL.
+  iter = 0;
+  location = commit_params->redirect_response[2]->headers->EnumerateHeader(
+      &iter, "Location");
+  ASSERT_TRUE(location.has_value());
+  EXPECT_EQ("/baz", location.value());
+
+  // The original navigation URL should be sanitized to origin when
+  // kSanitizeOriginalUrlDuringNavigation is enabled.
+  EXPECT_EQ(GURL("https://a.com/"), commit_params->original_url);
+}
+
+// Test to ensure that relative Location headers on non-standard schemes are
+// handled correctly during sanitization.
+TEST_F(NavigationRequestTest, SanitizeRedirectsForCommitNonStandardRelative) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      /*enabled_features=*/{features::kSanitizeLocationHeadersDuringNavigation,
+                            features::kSanitizeOriginalUrlDuringNavigation},
+      /*disabled_features=*/{});
+
+  url::ScopedSchemeRegistryForTests scoped_registry;
+  url::AddStandardScheme("chrome-foo", url::SCHEME_WITH_HOST);
+
+  const GURL start_url("chrome-foo://history/start");
+  const GURL url_2("chrome-foo://history/foo");
+  const GURL url_3("chrome-foo://newtab/bar");
+  const GURL final_url("chrome-foo://newtab/final");
+
+  std::unique_ptr<NavigationSimulator> navigation =
+      NavigationSimulator::CreateRendererInitiated(start_url, main_test_rfh());
+  navigation->Start();
+
+  // 1. Redirect to same-site (relative). Cross-origin to final URL.
+  auto headers1 =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 302 Found");
+  headers1->SetHeader("Location", "/foo");
+  navigation->SetRedirectHeaders(headers1);
+  navigation->Redirect(url_2);
+
+  // 2. Redirect to cross-site (absolute). Same-origin to final URL.
+  auto headers2 =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 302 Found");
+  headers2->SetHeader("Location", "chrome-foo://newtab/bar");
+  navigation->SetRedirectHeaders(headers2);
+  navigation->Redirect(url_3);
+
+  // Final navigation to D.
+  navigation->Redirect(final_url);
+
+  NavigationRequest* request =
+      NavigationRequest::From(navigation->GetNavigationHandle());
+  auto commit_params = request->commit_params().Clone();
+
+  request->SanitizeRedirectsForCommit(commit_params);
+
+  EXPECT_EQ(3u, commit_params->redirect_response.size());
+
+  size_t iter = 0;
+  std::optional<std::string_view> location;
+
+  // 1. "Location: /foo" resolves to cross-origin URL. Should be sanitized to
+  // origin.
+  location = commit_params->redirect_response[0]->headers->EnumerateHeader(
+      &iter, "Location");
+  ASSERT_TRUE(location.has_value());
+  EXPECT_EQ("chrome-foo://history/", location.value());
+
+  // 2. "Location: chrome-foo://newtab/bar" is same-origin to final URL.
+  // Should be left alone.
+  iter = 0;
+  location = commit_params->redirect_response[1]->headers->EnumerateHeader(
+      &iter, "Location");
+  ASSERT_TRUE(location.has_value());
+  EXPECT_EQ("chrome-foo://newtab/bar", location.value());
+
+  // The original navigation URL should be sanitized to origin when
+  // kSanitizeOriginalUrlDuringNavigation is enabled.
+  EXPECT_EQ(GURL("chrome-foo://history/"), commit_params->original_url);
+}
+
+// Test to ensure that hostless non-standard schemes (like data:) are handled
+// safely and treated as cross-origin during sanitization.
+TEST_F(NavigationRequestTest, SanitizeRedirectsForCommitHostlessNonStandard) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      /*enabled_features=*/{features::kSanitizeLocationHeadersDuringNavigation,
+                            features::kSanitizeOriginalUrlDuringNavigation},
+      /*disabled_features=*/{});
+
+  const GURL start_url("https://a.com/start");
+  const GURL url_2("data:text/html,foo");
+  const GURL final_url("https://a.com/final");
+
+  std::unique_ptr<NavigationSimulator> navigation =
+      NavigationSimulator::CreateRendererInitiated(start_url, main_test_rfh());
+  navigation->Start();
+
+  // 1. Redirect to data: URL.
+  auto headers =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 302 Found");
+  headers->SetHeader("Location", "data:text/html,foo");
+  navigation->SetRedirectHeaders(headers);
+  navigation->Redirect(url_2);
+
+  // Final navigation to D.
+  navigation->Redirect(final_url);
+
+  NavigationRequest* request =
+      NavigationRequest::From(navigation->GetNavigationHandle());
+  auto commit_params = request->commit_params().Clone();
+
+  request->SanitizeRedirectsForCommit(commit_params);
+
+  EXPECT_EQ(2u, commit_params->redirect_response.size());
+
+  size_t iter = 0;
+  std::optional<std::string_view> location;
+
+  // "Location: data:text/html,foo" resolves to cross-origin URL (since data:
+  // has no origin). Should be sanitized to empty string because
+  // GetOriginForSanitization returns empty!
+  location = commit_params->redirect_response[0]->headers->EnumerateHeader(
+      &iter, "Location");
+  ASSERT_TRUE(location.has_value());
+  EXPECT_EQ("", location.value());
+
+  // The original navigation URL should be sanitized to origin when
+  // kSanitizeOriginalUrlDuringNavigation is enabled.
+  EXPECT_EQ(GURL("https://a.com/"), commit_params->original_url);
+}
+
 // Test to ensure that SanitizeRedirectsForCommit is called when a navigation
 // fails and commits an error page.
 TEST_F(NavigationRequestTest, SanitizeRedirectsForCommitErrorPage) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      /*enabled_features=*/{features::kSanitizeOriginalUrlDuringNavigation},
+      /*disabled_features=*/{});
+
   const GURL start_url("https://a.com?param=1");
   const GURL url_2("https://b.com?param=2#foo");
   const GURL final_url("https://d.com?param=4");
@@ -921,6 +1129,10 @@
   EXPECT_EQ(2u, commit_params.redirect_infos.size());
   EXPECT_EQ(GURL("https://b.com"), commit_params.redirect_infos[0].new_url);
   EXPECT_EQ(final_url, commit_params.redirect_infos[1].new_url);
+
+  // The original navigation URL should be sanitized to origin when
+  // kSanitizeOriginalUrlDuringNavigation is enabled.
+  EXPECT_EQ(GURL("https://a.com/"), commit_params.original_url);
 }
 
 TEST_F(NavigationRequestTest, AbortsDeletedNavigationInProgress) {