From 32f95890ffdc45c3f129caa32bd992d366a001b8 Mon Sep 17 00:00:00 2001
From: Nasko Oskov <[email protected]>
Date: Mon, 02 Jun 2025 13:45:08 -0700
Subject: [PATCH] Fix redirect info sanitization to avoid a copy and actually sanitize.

The initial fix to sanitize URLs had a flaw where it made a local copy
for each iteration of the for loop, then operated on the copy and it was
discarded when moving to the next item. Effectively, the original CL
was a noop.

This change updates the code to actually sanitize properly and also
account for the fact that the last element in the redirect_infos vector
is the actual URL to commit, which should not be sanitized.

Bug: 40095391
Change-Id: I6c94bc9d794c98b3ef66a99231b763c40b16f412
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6512516
Commit-Queue: Nasko Oskov <[email protected]>
Reviewed-by: Charlie Reis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1468327}
---

diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index a0cf9e8..ac5a84d 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -6718,18 +6718,11 @@
   // consistently upheld condition.
   DUMP_WILL_BE_CHECK(commit_params->redirect_response.size() ==
                      commit_params->redirect_infos.size());
-  if (base::FeatureList::IsEnabled(kSanitizeRedirectUrlsDuringNavigation)) {
-    // Before sending the commit parameters to the renderer process, sanitize
-    // the redirect URLs to avoid leaking pontentially sensitive data into
-    // processes which are cross-site. There is no dependency on the
-    // cross-site-ness, therefore just sanitize unilaterally.
-    for (auto redirect : commit_params->redirect_infos) {
-      redirect.new_url = redirect.new_url.DeprecatedGetOriginAsURL();
-    }
-    for (auto redirect : commit_params->redirects) {
-      redirect = redirect.DeprecatedGetOriginAsURL();
-    }
-  }
+  // Before sending the commit parameters to the renderer process, sanitize
+  // the redirect URLs to avoid leaking potentially sensitive data into
+  // processes which are cross-site. There is no dependency on the
+  // cross-site-ness, therefore just sanitize unilaterally.
+  SanitizeRedirectsForCommit(commit_params);
 
   GetRenderFrameHost()->CommitNavigation(
       this, std::move(common_params), std::move(commit_params),
@@ -7515,6 +7508,31 @@
       navigation_controller.GetEntryCount();
 }
 
+void NavigationRequest::SanitizeRedirectsForCommit(
+    blink::mojom::CommitNavigationParamsPtr& commit_params) {
+  if (!base::FeatureList::IsEnabled(kSanitizeRedirectUrlsDuringNavigation)) {
+    return;
+  }
+  // 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.
+  for (GURL& redirect : commit_params->redirects) {
+    redirect = redirect.DeprecatedGetOriginAsURL();
+  }
+
+  // In the redirect_infos vector, the last entry is the URL we are going to
+  // commit after following all redirects. We should not be sanitizing it, as
+  // we need to commit the real URL as part of the navigation.
+  if (!commit_params->redirect_infos.empty()) {
+    auto redirect_infos_span = base::span(commit_params->redirect_infos);
+    for (net::RedirectInfo& redirect :
+         redirect_infos_span.first(redirect_infos_span.size() - 1)) {
+      redirect.new_url = redirect.new_url.DeprecatedGetOriginAsURL();
+    }
+  }
+}
+
 void NavigationRequest::RendererRequestedNavigationCancellationForTesting() {
   OnNavigationClientDisconnected(0, "");
 }
diff --git a/content/browser/renderer_host/navigation_request.h b/content/browser/renderer_host/navigation_request.h
index 3e84acf..e73e2a591 100644
--- a/content/browser/renderer_host/navigation_request.h
+++ b/content/browser/renderer_host/navigation_request.h
@@ -1722,6 +1722,7 @@
 
  private:
   friend class NavigationRequestTest;
+  FRIEND_TEST_ALL_PREFIXES(NavigationRequestTest, SanitizeRedirectsForCommit);
 
   struct ConsoleMessage {
     blink::mojom::ConsoleMessageLevel level;
@@ -2006,6 +2007,11 @@
   // renderer process.
   void UpdateHistoryParamsInCommitNavigationParams();
 
+  // Helper method to sanitize URLs for redirects before the commit IPC is sent
+  // to the renderer process. Must be called right before sending the IPC.
+  void SanitizeRedirectsForCommit(
+      blink::mojom::CommitNavigationParamsPtr& commit_params);
+
   // The disconnect handler for the NavigationClient Mojo interface; used as a
   // signal to potentially cancel navigations, e.g. when the renderer replaces
   // an existing NavigationClient connection with a new one or when the renderer
diff --git a/content/browser/renderer_host/navigation_request_unittest.cc b/content/browser/renderer_host/navigation_request_unittest.cc
index b41ae0a..f78672b 100644
--- a/content/browser/renderer_host/navigation_request_unittest.cc
+++ b/content/browser/renderer_host/navigation_request_unittest.cc
@@ -35,6 +35,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/common/navigation/navigation_params.h"
+#include "third_party/blink/public/common/navigation/navigation_params_mojom_traits.h"
 #include "third_party/blink/public/common/origin_trials/scoped_test_origin_trial_policy.h"
 #include "third_party/blink/public/common/runtime_feature_state/runtime_feature_state_context.h"
 #include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom.h"
@@ -927,6 +928,40 @@
   navigation->Commit();
 }
 
+// Test to ensure that the SanitizeRedirectsForCommit method correctly removes
+// the query parameters parts of the URL that can contain sensitive information.
+TEST_F(NavigationRequestTest, SanitizeRedirectsForCommit) {
+  const GURL start_url("https://a.com?param=1");
+  const GURL url_2("https://b.com?param=2#foo");
+  const GURL url_3("https://c.com?param=3");
+  const GURL final_url("https://d.com?param=4");
+  std::unique_ptr<NavigationSimulator> navigation =
+      NavigationSimulator::CreateRendererInitiated(start_url, main_test_rfh());
+  navigation->Start();
+  navigation->Redirect(url_2);
+  navigation->Redirect(url_3);
+  navigation->Redirect(final_url);
+
+  NavigationRequest* request =
+      NavigationRequest::From(navigation->GetNavigationHandle());
+  auto commit_params = request->commit_params().Clone();
+  request->SanitizeRedirectsForCommit(commit_params);
+
+  // redirect_infos contains entries for B, C, and D, but not the starting URL.
+  // Ensure that the full URL for D is preserved.
+  EXPECT_EQ(3, commit_params->redirect_infos.size());
+  EXPECT_EQ(GURL("https://b.com"), commit_params->redirect_infos[0].new_url);
+  EXPECT_EQ(GURL("https://c.com"), commit_params->redirect_infos[1].new_url);
+  EXPECT_EQ(final_url, commit_params->redirect_infos[2].new_url);
+
+  // In contrast, redirects contains A, B, and C (i.e., the starting URL but not
+  // the final URL).
+  EXPECT_EQ(3, commit_params->redirects.size());
+  EXPECT_EQ(GURL("https://a.com"), commit_params->redirects[0]);
+  EXPECT_EQ(GURL("https://b.com"), commit_params->redirects[1]);
+  EXPECT_EQ(GURL("https://c.com"), commit_params->redirects[2]);
+}
+
 // Test that the required CSP of every frame is computed/inherited correctly and
 // that the Sec-Required-CSP header is set.
 class CSPEmbeddedEnforcementUnitTest : public NavigationRequestTest {

diff --git a/content/browser/renderer_host/navigation_request_unittest.cc b/content/browser/renderer_host/navigation_request_unittest.cc
index b41ae0a..f78672b 100644
--- a/content/browser/renderer_host/navigation_request_unittest.cc
+++ b/content/browser/renderer_host/navigation_request_unittest.cc
@@ -35,6 +35,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/common/navigation/navigation_params.h"
+#include "third_party/blink/public/common/navigation/navigation_params_mojom_traits.h"
 #include "third_party/blink/public/common/origin_trials/scoped_test_origin_trial_policy.h"
 #include "third_party/blink/public/common/runtime_feature_state/runtime_feature_state_context.h"
 #include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom.h"
@@ -927,6 +928,40 @@
   navigation->Commit();
 }
 
+// Test to ensure that the SanitizeRedirectsForCommit method correctly removes
+// the query parameters parts of the URL that can contain sensitive information.
+TEST_F(NavigationRequestTest, SanitizeRedirectsForCommit) {
+  const GURL start_url("https://a.com?param=1");
+  const GURL url_2("https://b.com?param=2#foo");
+  const GURL url_3("https://c.com?param=3");
+  const GURL final_url("https://d.com?param=4");
+  std::unique_ptr<NavigationSimulator> navigation =
+      NavigationSimulator::CreateRendererInitiated(start_url, main_test_rfh());
+  navigation->Start();
+  navigation->Redirect(url_2);
+  navigation->Redirect(url_3);
+  navigation->Redirect(final_url);
+
+  NavigationRequest* request =
+      NavigationRequest::From(navigation->GetNavigationHandle());
+  auto commit_params = request->commit_params().Clone();
+  request->SanitizeRedirectsForCommit(commit_params);
+
+  // redirect_infos contains entries for B, C, and D, but not the starting URL.
+  // Ensure that the full URL for D is preserved.
+  EXPECT_EQ(3, commit_params->redirect_infos.size());
+  EXPECT_EQ(GURL("https://b.com"), commit_params->redirect_infos[0].new_url);
+  EXPECT_EQ(GURL("https://c.com"), commit_params->redirect_infos[1].new_url);
+  EXPECT_EQ(final_url, commit_params->redirect_infos[2].new_url);
+
+  // In contrast, redirects contains A, B, and C (i.e., the starting URL but not
+  // the final URL).
+  EXPECT_EQ(3, commit_params->redirects.size());
+  EXPECT_EQ(GURL("https://a.com"), commit_params->redirects[0]);
+  EXPECT_EQ(GURL("https://b.com"), commit_params->redirects[1]);
+  EXPECT_EQ(GURL("https://c.com"), commit_params->redirects[2]);
+}
+
 // Test that the required CSP of every frame is computed/inherited correctly and
 // that the Sec-Required-CSP header is set.
 class CSPEmbeddedEnforcementUnitTest : public NavigationRequestTest {