[UrlParamFilter] Add param filtering to server and client redirects

Previous behavior filtered only the URL seen by the context menu code
and not any redirects. This CL protects the full chain, including client
redirects fired without user interaction, while maintaining our existing
status code and refresh count metrics.

Bug: 1265734
Change-Id: Ie3c03938284348325f1680c869dedaacf4068774
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3542510
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Matt Reichhoff <mreichhoff@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984887}

chrome/browser/BUILD.gn

@@ -1795,6 +1795,8 @@ static_library("browser") {
     "url_param_filter/cross_otr_observer.h",
     "url_param_filter/url_param_classifications_loader.cc",
     "url_param_filter/url_param_classifications_loader.h",
+    "url_param_filter/url_param_filter_throttle.cc",
+    "url_param_filter/url_param_filter_throttle.h",
     "url_param_filter/url_param_filterer.cc",
     "url_param_filter/url_param_filterer.h",
     "usb/frame_usb_services.cc",

chrome/browser/chrome_content_browser_client.cc

@@ -143,6 +143,7 @@
 #include "chrome/browser/ui/webui/chrome_web_ui_controller_factory.h"
 #include "chrome/browser/ui/webui/log_web_ui_url.h"
 #include "chrome/browser/universal_web_contents_observers.h"
+#include "chrome/browser/url_param_filter/url_param_filter_throttle.h"
 #include "chrome/browser/usb/frame_usb_services.h"
 #include "chrome/browser/vr/vr_tab_helper.h"
 #include "chrome/browser/web_applications/policy/web_app_policy_manager.h"
@@ -4638,6 +4639,9 @@ ChromeContentBrowserClient::CreateURLLoaderThrottles(
   ChromeNavigationUIData* chrome_navigation_ui_data =
       static_cast<ChromeNavigationUIData*>(navigation_ui_data);
 
+  url_param_filter::UrlParamFilterThrottle::MaybeCreateThrottle(
+      wc_getter.Run(), request, &result);
+
 #if BUILDFLAG(SAFE_BROWSING_AVAILABLE)
   bool matches_enterprise_allowlist = safe_browsing::IsURLAllowlistedByPolicy(
       request.url, *profile->GetPrefs());

chrome/browser/renderer_context_menu/render_view_context_menu.cc

@@ -91,7 +91,6 @@
 #include "chrome/browser/ui/web_applications/app_browser_controller.h"
 #include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
 #include "chrome/browser/ui/webui/history/foreign_session_handler.h"
-#include "chrome/browser/url_param_filter/url_param_filterer.h"
 #include "chrome/browser/web_applications/system_web_apps/system_web_app_delegate.h"
 #include "chrome/browser/web_applications/system_web_apps/system_web_app_manager.h"
 #include "chrome/browser/web_applications/web_app_helpers.h"
@@ -2442,9 +2441,7 @@ void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) {
     case IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD:
       // Pass along the |referring_url| so we can show it in browser UI. Note
       // that this won't and shouldn't be sent via the referrer header.
-      OpenURLWithExtraHeaders(url_param_filter::FilterUrl(
-                                  GetDocumentURL(params_), params_.link_url),
-                              GetDocumentURL(params_),
+      OpenURLWithExtraHeaders(params_.link_url, GetDocumentURL(params_),
                               WindowOpenDisposition::OFF_THE_RECORD,
                               ui::PAGE_TRANSITION_LINK, "" /* extra_headers */,
                               true /* started_from_context_menu */);

chrome/browser/url_param_filter/cross_otr_observer.cc

@@ -14,6 +14,7 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
+#include "ui/base/page_transition_types.h"
 
 namespace url_param_filter {
 
@@ -40,14 +41,41 @@ CrossOtrObserver::CrossOtrObserver(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       content::WebContentsUserData<CrossOtrObserver>(*web_contents) {}
 
+bool CrossOtrObserver::IsCrossOtrState() {
+  return protecting_navigations_;
+}
+
+void CrossOtrObserver::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // If we've already observed the end of a navigation, and the navigation is in
+  // the primary main frame, and it is not the result of a client redirect,
+  // we've finished the cross-OTR case. Note that observing user activation
+  // would also serve to stop the protecting_navigations_ case. Note that
+  // refreshes after page load also trigger this, and thus are not at risk of
+  // being considered part of the cross-OTR case.
+  if (observed_response_ && navigation_handle->IsInPrimaryMainFrame() &&
+      !(navigation_handle->GetPageTransition() &
+        ui::PAGE_TRANSITION_CLIENT_REDIRECT)) {
+    protecting_navigations_ = false;
+  }
+}
+
 void CrossOtrObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  // We only want the first navigation to be counted; after that point, no
+  if (!navigation_handle->IsInPrimaryMainFrame() ||
+      navigation_handle->IsSameDocument()) {
+    // We only are concerned with top-level, non-same doc navigations.
+    return;
+  }
+
+  // We only want the first navigation, including client redirects occurring
+  // without having observed user activation, to be counted; after that, no
   // response codes should be tracked. The observer is left in place to track
   // refreshes on the first page.
-  if (!wrote_response_metric_) {
-    wrote_response_metric_ = true;
+  if (protecting_navigations_) {
+    observed_response_ = true;
     const net::HttpResponseHeaders* headers =
         navigation_handle->GetResponseHeaders();
     if (headers) {
@@ -57,9 +85,7 @@ void CrossOtrObserver::DidFinishNavigation(
     }
   } else if (navigation_handle->GetReloadType() != content::ReloadType::NONE) {
     refresh_count_++;
-  } else if (navigation_handle->IsInPrimaryMainFrame() &&
-             !navigation_handle->IsSameDocument() &&
-             navigation_handle->HasCommitted()) {
+  } else if (navigation_handle->HasCommitted() && !protecting_navigations_) {
     Detach();
     // DO NOT add code past this point. `this` is destroyed.
   }
@@ -68,11 +94,18 @@ void CrossOtrObserver::DidFinishNavigation(
 void CrossOtrObserver::DidRedirectNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!navigation_handle->IsInPrimaryMainFrame() ||
+      navigation_handle->IsSameDocument()) {
+    // We only are concerned with top-level, non-same doc navigations.
+    return;
+  }
+
   const net::HttpResponseHeaders* headers =
       navigation_handle->GetResponseHeaders();
-  // After the first navigation has committed, we no longer want to track
+  // After the first full navigation has committed, including any client
+  // redirects that occur without user activation, we no longer want to track
   // redirects.
-  if (!wrote_response_metric_ && headers) {
+  if (protecting_navigations_ && headers) {
     base::UmaHistogramSparse(
         kCrossOtrResponseCodeMetricName,
         net::HttpUtil::MapStatusCodeForHistogram(headers->response_code()));
@@ -86,6 +119,14 @@ void CrossOtrObserver::WebContentsDestroyed() {
   // DO NOT add code past this point. `this` is destroyed.
 }
 
+void CrossOtrObserver::FrameReceivedUserActivation(
+    content::RenderFrameHost* render_frame_host) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // Anytime the user activates a frame in the web contents, we cease to
+  // consider the case cross-OTR.
+  protecting_navigations_ = false;
+}
+
 void CrossOtrObserver::Detach() {
   base::UmaHistogramCounts100(kCrossOtrRefreshCountMetricName, refresh_count_);
   web_contents()->RemoveUserData(CrossOtrObserver::UserDataKey());

chrome/browser/url_param_filter/cross_otr_observer.h

@@ -12,20 +12,32 @@
 namespace url_param_filter {
 
 // Observes navigations that originate in normal browsing and move into OTR
-// browsing.
+// browsing. This class can be thought of as a state machine:
+// start-->blocking-->monitoring-->detached
+// where the initial cross-OTR navigation moves to blocking; user activation or
+// the start of a second navigation not initiated via client redirect moves to
+// monitoring; and the next completed non-refresh navigation after that point,
+// regardless of cause, detaches. Note that for our purposes, navigation above
+// refers to top-level, main frame navigations only; we do not consider e.g.,
+// subframe loads.
 class CrossOtrObserver : public content::WebContentsObserver,
                          public content::WebContentsUserData<CrossOtrObserver> {
  public:
   // Attaches the observer in cases where it should do so; leaves `web_contents`
   // unchanged otherwise.
   static void MaybeCreateForWebContents(content::WebContents* web_contents,
                                         const NavigateParams& params);
+  bool IsCrossOtrState();
   // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   void DidRedirectNavigation(
       content::NavigationHandle* navigation_handle) override;
   void WebContentsDestroyed() override;
+  void FrameReceivedUserActivation(
+      content::RenderFrameHost* render_frame_host) override;
 
  private:
   explicit CrossOtrObserver(content::WebContents* web_contents);
@@ -39,11 +51,17 @@ class CrossOtrObserver : public content::WebContentsObserver,
   // Drives state machine logic; we write the cross-OTR response code metric
   // only for the first navigation, which is that which would have parameters
   // filtered.
-  bool wrote_response_metric_ = false;
+  bool observed_response_ = false;
   // Tracks refreshes observed, which could point to an issue with param
   // filtering causing unexpected behavior for the user.
   int refresh_count_ = 0;
 
+  // Whether top-level navigations should have filtering applied. Starts true,
+  // and switched to false once a navigation completes and then either:
+  // user interaction is observed or a new navigation starts that is not a
+  // client redirect.
+  bool protecting_navigations_ = true;
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 };
 

chrome/browser/url_param_filter/cross_otr_observer_unittest.cc

@@ -138,7 +138,10 @@ TEST_F(CrossOtrObserverTest, FinishedNavigation) {
   scoped_refptr<net::HttpResponseHeaders> response =
       base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 200 OK");
   handle->set_response_headers(response);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
+
+  ASSERT_TRUE(observer->IsCrossOtrState());
   histogram_tester.ExpectTotalCount(kResponseCodeMetricName, 1);
   histogram_tester.ExpectUniqueSample(
       kResponseCodeMetricName, net::HttpUtil::MapStatusCodeForHistogram(200),
@@ -177,6 +180,7 @@ TEST_F(CrossOtrObserverTest, BadNavigationResponse) {
       std::make_unique<NiceMock<content::MockNavigationHandle>>(contents);
 
   handle->set_response_headers(nullptr);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
   histogram_tester.ExpectTotalCount(kResponseCodeMetricName, 0);
 
@@ -202,9 +206,11 @@ TEST_F(CrossOtrObserverTest, RefreshedAfterNavigation) {
   scoped_refptr<net::HttpResponseHeaders> response =
       base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 200 OK");
   handle->set_response_headers(response);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
 
   handle->set_reload_type(content::ReloadType::NORMAL);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
   observer->WebContentsDestroyed();
 
@@ -232,7 +238,9 @@ TEST_F(CrossOtrObserverTest, UncommittedNavigationWithRefresh) {
   scoped_refptr<net::HttpResponseHeaders> response =
       base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 200 OK");
   handle->set_response_headers(response);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
+  ASSERT_TRUE(observer->IsCrossOtrState());
 
   // Finish a non-reload navigation, but one that isn't committed (so no actual
   // navigation away from the monitored page)
@@ -241,10 +249,15 @@ TEST_F(CrossOtrObserverTest, UncommittedNavigationWithRefresh) {
   handle->set_is_same_document(false);
   handle->set_has_committed(false);
 
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
+  // We just observed another navigation not due to a client redirect, so should
+  // no longer be in the cross-OTR state.
+  ASSERT_FALSE(observer->IsCrossOtrState());
 
   // After that uncommitted navigation, trigger a redirect, then destroy.
   handle->set_reload_type(content::ReloadType::NORMAL);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
   observer->WebContentsDestroyed();
 
@@ -273,18 +286,24 @@ TEST_F(CrossOtrObserverTest, MultipleRefreshesAfterNavigation) {
   scoped_refptr<net::HttpResponseHeaders> response =
       base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 200 OK");
   handle->set_response_headers(response);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
 
   // Reload twice and ensure the count is persisted.
   handle->set_reload_type(content::ReloadType::NORMAL);
+  observer->DidStartNavigation(handle.get());
+  // With the refresh navigation started, we are no longer in cross-OTR mode.
+  ASSERT_FALSE(observer->IsCrossOtrState());
   observer->DidFinishNavigation(handle.get());
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
 
   // Navigating away means no more observer.
   handle->set_reload_type(content::ReloadType::NONE);
   handle->set_is_in_primary_main_frame(true);
   handle->set_is_same_document(false);
   handle->set_has_committed(true);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
 
   ASSERT_EQ(CrossOtrObserver::FromWebContents(contents), nullptr);
@@ -313,10 +332,20 @@ TEST_F(CrossOtrObserverTest, RedirectsAfterNavigation) {
   scoped_refptr<net::HttpResponseHeaders> response =
       base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 200 OK");
   handle->set_response_headers(response);
+  observer->DidStartNavigation(handle.get());
   observer->DidFinishNavigation(handle.get());
+  // The first navigation has finished, but we remain cross-OTR until either
+  // user activation or a non-client redirect navigation begins
+  ASSERT_TRUE(observer->IsCrossOtrState());
 
-  // Redirects observed after the first navigation has committed should not
+  // Redirects observed on navigations after the first should not
   // write responses.
+  observer->DidStartNavigation(handle.get());
+
+  // A new, non-client redirect navigation began, so we should no longer be
+  // filtering.
+  ASSERT_FALSE(observer->IsCrossOtrState());
+
   response = base::MakeRefCounted<net::HttpResponseHeaders>(
       "HTTP/1.1 302 Moved Temporarily");
   handle->set_response_headers(response);
@@ -327,4 +356,77 @@ TEST_F(CrossOtrObserverTest, RedirectsAfterNavigation) {
       kResponseCodeMetricName, net::HttpUtil::MapStatusCodeForHistogram(200),
       1);
 }
+TEST_F(CrossOtrObserverTest, ClientRedirectCrossOtr) {
+  base::HistogramTester histogram_tester;
+  NavigateParams params(profile(), GURL("https://www.foo.com"),
+                        ui::PAGE_TRANSITION_LINK);
+
+  params.started_from_context_menu = true;
+  params.privacy_sensitivity = NavigateParams::PrivacySensitivity::CROSS_OTR;
+  content::WebContents* contents = web_contents();
+  CrossOtrObserver::MaybeCreateForWebContents(contents, params);
+  CrossOtrObserver* observer = CrossOtrObserver::FromWebContents(contents);
+  ASSERT_NE(observer, nullptr);
+  std::unique_ptr<content::MockNavigationHandle> handle =
+      std::make_unique<NiceMock<content::MockNavigationHandle>>(contents);
+
+  scoped_refptr<net::HttpResponseHeaders> response =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 200 OK");
+  handle->set_response_headers(response);
+  observer->DidStartNavigation(handle.get());
+  observer->DidFinishNavigation(handle.get());
+  // The first navigation has finished, but we remain cross-OTR until either
+  // user activation or a non-client redirect navigation begins
+  ASSERT_TRUE(observer->IsCrossOtrState());
+
+  handle->set_page_transition(ui::PAGE_TRANSITION_CLIENT_REDIRECT);
+  observer->DidStartNavigation(handle.get());
+  ASSERT_TRUE(observer->IsCrossOtrState());
+  observer->DidFinishNavigation(handle.get());
+  ASSERT_TRUE(observer->IsCrossOtrState());
+
+  handle->set_page_transition(ui::PAGE_TRANSITION_AUTO_BOOKMARK);
+  observer->DidStartNavigation(handle.get());
+  // A new, non-client redirect navigation began, so we should no longer be
+  // filtering.
+  ASSERT_FALSE(observer->IsCrossOtrState());
+}
+TEST_F(CrossOtrObserverTest, ClientRedirectAfterActivationNotCrossOtr) {
+  base::HistogramTester histogram_tester;
+  NavigateParams params(profile(), GURL("https://www.foo.com"),
+                        ui::PAGE_TRANSITION_LINK);
+
+  params.started_from_context_menu = true;
+  params.privacy_sensitivity = NavigateParams::PrivacySensitivity::CROSS_OTR;
+  content::WebContents* contents = web_contents();
+  CrossOtrObserver::MaybeCreateForWebContents(contents, params);
+  CrossOtrObserver* observer = CrossOtrObserver::FromWebContents(contents);
+  ASSERT_NE(observer, nullptr);
+  std::unique_ptr<content::MockNavigationHandle> handle =
+      std::make_unique<NiceMock<content::MockNavigationHandle>>(contents);
+
+  scoped_refptr<net::HttpResponseHeaders> response =
+      base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 200 OK");
+  handle->set_response_headers(response);
+  observer->DidStartNavigation(handle.get());
+  observer->DidFinishNavigation(handle.get());
+  // The first navigation has finished, but we remain cross-OTR until either
+  // user activation or a non-client redirect navigation begins
+  ASSERT_TRUE(observer->IsCrossOtrState());
+
+  handle->set_page_transition(ui::PAGE_TRANSITION_CLIENT_REDIRECT);
+  observer->DidStartNavigation(handle.get());
+  ASSERT_TRUE(observer->IsCrossOtrState());
+  observer->DidFinishNavigation(handle.get());
+  ASSERT_TRUE(observer->IsCrossOtrState());
+
+  observer->FrameReceivedUserActivation(nullptr);
+  // Receiving user activation means we leave the cross-OTR state and instead
+  // allow client redirects to occur unfiltered.
+  ASSERT_FALSE(observer->IsCrossOtrState());
+  handle->set_page_transition(ui::PAGE_TRANSITION_AUTO_BOOKMARK);
+  observer->DidStartNavigation(handle.get());
+  // The client redirect should not reset the OTR state.
+  ASSERT_FALSE(observer->IsCrossOtrState());
+}
 }  // namespace url_param_filter