From 480392e193c510a732d3369e0eec65dbf3863ecf Mon Sep 17 00:00:00 2001 From: Matt Reichhoff Date: Thu, 24 Mar 2022 16:49:19 +0000 Subject: [PATCH] [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 Reviewed-by: Avi Drissman Commit-Queue: Matt Reichhoff Cr-Commit-Position: refs/heads/main@{#984887} --- chrome/browser/BUILD.gn | 2 + .../browser/chrome_content_browser_client.cc | 4 + .../render_view_context_menu.cc | 5 +- .../url_param_filter/cross_otr_observer.cc | 57 +++- .../url_param_filter/cross_otr_observer.h | 22 +- .../cross_otr_observer_unittest.cc | 104 +++++++- .../url_param_filter_browsertest.cc | 237 ++++++++++++++++- .../url_param_filter_throttle.cc | 72 +++++ .../url_param_filter_throttle.h | 49 ++++ .../url_param_filter_throttle_unittest.cc | 249 ++++++++++++++++++ .../url_param_filter/url_param_filterer.cc | 11 +- .../url_param_filterer_unittest.cc | 41 +++ chrome/test/BUILD.gn | 1 + 13 files changed, 836 insertions(+), 18 deletions(-) create mode 100644 chrome/browser/url_param_filter/url_param_filter_throttle.cc create mode 100644 chrome/browser/url_param_filter/url_param_filter_throttle.h create mode 100644 chrome/browser/url_param_filter/url_param_filter_throttle_unittest.cc diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn index a07110b2c87860..2655e56118acfe 100644 --- a/chrome/browser/BUILD.gn +++ b/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", diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc index 22b999853b06ba..993c9614909cbf 100644 --- a/chrome/browser/chrome_content_browser_client.cc +++ b/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(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()); diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu.cc b/chrome/browser/renderer_context_menu/render_view_context_menu.cc index c037f9714c26cd..f7d16b3bb1252c 100644 --- a/chrome/browser/renderer_context_menu/render_view_context_menu.cc +++ b/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 */); diff --git a/chrome/browser/url_param_filter/cross_otr_observer.cc b/chrome/browser/url_param_filter/cross_otr_observer.cc index 2613591eb1c121..36ab753b8d762a 100644 --- a/chrome/browser/url_param_filter/cross_otr_observer.cc +++ b/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(*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()); diff --git a/chrome/browser/url_param_filter/cross_otr_observer.h b/chrome/browser/url_param_filter/cross_otr_observer.h index 6a7839e7b61452..d8ec2424b7ac03 100644 --- a/chrome/browser/url_param_filter/cross_otr_observer.h +++ b/chrome/browser/url_param_filter/cross_otr_observer.h @@ -12,7 +12,14 @@ 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 { public: @@ -20,12 +27,17 @@ class CrossOtrObserver : public content::WebContentsObserver, // 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(); }; diff --git a/chrome/browser/url_param_filter/cross_otr_observer_unittest.cc b/chrome/browser/url_param_filter/cross_otr_observer_unittest.cc index 39a7754cff9784..349deaa3827002 100644 --- a/chrome/browser/url_param_filter/cross_otr_observer_unittest.cc +++ b/chrome/browser/url_param_filter/cross_otr_observer_unittest.cc @@ -138,7 +138,10 @@ TEST_F(CrossOtrObserverTest, FinishedNavigation) { scoped_refptr response = base::MakeRefCounted("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>(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 response = base::MakeRefCounted("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 response = base::MakeRefCounted("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,11 +286,16 @@ TEST_F(CrossOtrObserverTest, MultipleRefreshesAfterNavigation) { scoped_refptr response = base::MakeRefCounted("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. @@ -285,6 +303,7 @@ TEST_F(CrossOtrObserverTest, MultipleRefreshesAfterNavigation) { 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 response = base::MakeRefCounted("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( "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 handle = + std::make_unique>(contents); + + scoped_refptr response = + base::MakeRefCounted("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 handle = + std::make_unique>(contents); + + scoped_refptr response = + base::MakeRefCounted("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 diff --git a/chrome/browser/url_param_filter/url_param_filter_browsertest.cc b/chrome/browser/url_param_filter/url_param_filter_browsertest.cc index e2e1a009db8a82..f9fca077a8abcc 100644 --- a/chrome/browser/url_param_filter/url_param_filter_browsertest.cc +++ b/chrome/browser/url_param_filter/url_param_filter_browsertest.cc @@ -15,7 +15,9 @@ #include "content/public/browser/web_contents.h" #include "content/public/test/browser_test.h" #include "content/public/test/browser_test_utils.h" +#include "net/base/escape.h" #include "net/http/http_util.h" +#include "third_party/blink/public/common/input/web_mouse_event.h" class ContextMenuIncognitoFilterBrowserTest : public InProcessBrowserTest { public: @@ -31,7 +33,8 @@ class ContextMenuIncognitoFilterBrowserTest : public InProcessBrowserTest { // or a source of: foo.com with outgoing param plzblock1 std::string encoded_classification = url_param_filter:: CreateBase64EncodedFilterParamClassificationForTesting( - {{"foo.com", {"plzblock1"}}}, {{"", {"plzblock"}}}); + {{"foo.com", {"plzblock1"}}, {"", {"plzblockredirect"}}}, + {{"", {"plzblock"}}}); scoped_feature_list_.InitAndEnableFeatureWithParameters( features::kIncognitoParamFilterEnabled, {{"classifications", encoded_classification}}); @@ -80,11 +83,241 @@ IN_PROC_BROWSER_TEST_F(ContextMenuIncognitoFilterBrowserTest, ASSERT_EQ(expected, tab->GetLastCommittedURL()); // The response was a 200, and the navigation went from normal-->OTR browsing. - histogram_tester.ExpectUniqueSample( + // Because we intervened, an artificial redirect was injected, so we also + // expect a 307. + histogram_tester.ExpectBucketCount( + kCrossOtrResponseMetricName, + net::HttpUtil::MapStatusCodeForHistogram(307), 1); + histogram_tester.ExpectBucketCount( kCrossOtrResponseMetricName, net::HttpUtil::MapStatusCodeForHistogram(200), 1); } +// Enable "Open Link in Incognito Window" URL parameter filtering, and ensure +// that it filters as expected. +IN_PROC_BROWSER_TEST_F( + ContextMenuIncognitoFilterBrowserTest, + OpenIncognitoUrlParamFilterClientRedirectAfterActivation) { + base::HistogramTester histogram_tester; + + ui_test_utils::AllBrowserTabAddedWaiter add_tab; + + ASSERT_TRUE(embedded_test_server()->Start()); + GURL test_root(embedded_test_server()->GetURL( + "/empty.html?plzblock=1&nochanges=2&plzblock1=2")); + + // Go to a |page| with a link to a URL that has associated filtering rules. + GURL page("data:text/html,link"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), page)); + + // Set up the source URL to an eTLD+1 that also has a filtering rule. + const GURL kSource("http://foo.com/test"); + + // Set up menu with link URL. + content::ContextMenuParams context_menu_params; + context_menu_params.page_url = kSource; + context_menu_params.link_url = test_root; + + // Select "Open Link in Incognito Window" and wait for window to be added. + TestRenderViewContextMenu menu( + *browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(), + context_menu_params); + menu.Init(); + menu.ExecuteCommand(IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD, 0); + + content::WebContents* tab = add_tab.Wait(); + EXPECT_TRUE(content::WaitForLoadStop(tab)); + + // Verify that it loaded the filtered URL. + GURL expected(embedded_test_server()->GetURL("/empty.html?nochanges=2")); + ASSERT_EQ(expected, tab->GetLastCommittedURL()); + + // Trigger user activation, at which point client redirects are no longer + // protected. + content::SimulateMouseClick(tab, 0, blink::WebMouseEvent::Button::kLeft); + content::LoadStopObserver client_redirect_load_observer(tab); + std::string script = "document.location.href=\"" + test_root.spec() + "\""; + ASSERT_TRUE(content::ExecJs(tab, script)); + client_redirect_load_observer.Wait(); + ASSERT_EQ(test_root, tab->GetLastCommittedURL()); +} + +// Enable "Open Link in Incognito Window" URL parameter filtering, and ensure +// that it filters as expected when server redirects are encountered. +IN_PROC_BROWSER_TEST_F(ContextMenuIncognitoFilterBrowserTest, + OpenIncognitoUrlParamFilterServerRedirect) { + base::HistogramTester histogram_tester; + + ui_test_utils::AllBrowserTabAddedWaiter add_tab; + + ASSERT_TRUE(embedded_test_server()->Start()); + // `plzblockredirect` is blocked on navigations from IP source domains; + // `plzblock` is blocked when an IP is the destination domain. + GURL test_root(embedded_test_server()->GetURL( + "/empty.html?plzblock=1&nochanges=2&plzblockredirect=2")); + GURL redirect_page(embedded_test_server()->GetURL( + "/server-redirect?" + + net::EscapeQueryParamValue(test_root.spec(), false))); + + // Go to a |page| with a link to a URL that has associated filtering rules. + GURL page("data:text/html,link"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), page)); + + // Set up the source URL to an eTLD+1 that also has a filtering rule. + const GURL kSource("http://foo.com/test"); + + // Set up menu with link URL. + content::ContextMenuParams context_menu_params; + context_menu_params.page_url = kSource; + context_menu_params.link_url = redirect_page; + + // Select "Open Link in Incognito Window" and wait for window to be added. + TestRenderViewContextMenu menu( + *browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(), + context_menu_params); + menu.Init(); + menu.ExecuteCommand(IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD, 0); + + content::WebContents* tab = add_tab.Wait(); + EXPECT_TRUE(content::WaitForLoadStop(tab)); + + // Verify that it loaded the filtered URL. + GURL expected(embedded_test_server()->GetURL("/empty.html?nochanges=2")); + ASSERT_EQ(expected, tab->GetLastCommittedURL()); + + // The response was a 301-->200, and the navigation went from normal-->OTR + // browsing. Because we intervened, an artificial redirect was injected, so we + // also expect a 307. + histogram_tester.ExpectBucketCount( + kCrossOtrResponseMetricName, + net::HttpUtil::MapStatusCodeForHistogram(301), 1); + histogram_tester.ExpectBucketCount( + kCrossOtrResponseMetricName, + net::HttpUtil::MapStatusCodeForHistogram(307), 1); + histogram_tester.ExpectBucketCount( + kCrossOtrResponseMetricName, + net::HttpUtil::MapStatusCodeForHistogram(200), 1); +} + +// Enable "Open Link in Incognito Window" URL parameter filtering, and ensure +// that it filters as expected when client redirects are encountered. +IN_PROC_BROWSER_TEST_F(ContextMenuIncognitoFilterBrowserTest, + OpenIncognitoUrlParamFilterClientRedirect) { + base::HistogramTester histogram_tester; + + ui_test_utils::AllBrowserTabAddedWaiter add_tab; + + ASSERT_TRUE(embedded_test_server()->Start()); + // `plzblock1` is blocked only on navs from foo.com. Because analysis will see + // this as a separate navigation, the source domain of the client redirect + // will be localhost. + GURL test_root( + embedded_test_server()->GetURL("/empty.html?plzblock=1&nochanges=2")); + GURL redirect_page(embedded_test_server()->GetURL( + "/client-redirect?" + + net::EscapeQueryParamValue(test_root.spec(), false))); + + // Go to a |page| with a link to a URL that has associated filtering rules. + GURL page("data:text/html,link"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), page)); + + // Set up the source URL to an eTLD+1 that also has a filtering rule. + const GURL kSource("http://foo.com/test"); + + // Set up menu with link URL. + content::ContextMenuParams context_menu_params; + context_menu_params.page_url = kSource; + context_menu_params.link_url = redirect_page; + + // Select "Open Link in Incognito Window" and wait for window to be added. + TestRenderViewContextMenu menu( + *browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(), + context_menu_params); + menu.Init(); + menu.ExecuteCommand(IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD, 0); + + content::WebContents* tab = add_tab.Wait(); + EXPECT_TRUE(content::WaitForLoadStop(tab)); + // The prior load stop succeeds for the initial response; we now wait for the + // client redirect to occur. + content::LoadStopObserver client_redirect_load_observer(tab); + client_redirect_load_observer.Wait(); + // Verify that it loaded the filtered URL. + GURL expected(embedded_test_server()->GetURL("/empty.html?nochanges=2")); + ASSERT_EQ(expected, tab->GetLastCommittedURL()); + + // The response was a 200-->client redirect-->200, and the navigation went + // from normal-->OTR browsing. Because we intervened, an artificial redirect + // was injected, so we also expect a 307. + histogram_tester.ExpectBucketCount( + kCrossOtrResponseMetricName, + net::HttpUtil::MapStatusCodeForHistogram(307), 1); + histogram_tester.ExpectBucketCount( + kCrossOtrResponseMetricName, + net::HttpUtil::MapStatusCodeForHistogram(200), 2); +} + +// Enable "Open Link in Incognito Window" URL parameter filtering, and ensure +// that it filters as expected when client redirects are encountered. +IN_PROC_BROWSER_TEST_F(ContextMenuIncognitoFilterBrowserTest, + OpenIncognitoUrlParamFilterClientRedirectThenRefresh) { + base::HistogramTester histogram_tester; + + ui_test_utils::AllBrowserTabAddedWaiter add_tab; + + ASSERT_TRUE(embedded_test_server()->Start()); + GURL test_root( + embedded_test_server()->GetURL("/empty.html?plzblock=1&nochanges=2")); + GURL redirect_page(embedded_test_server()->GetURL( + "/client-redirect?" + + net::EscapeQueryParamValue(test_root.spec(), false))); + + // Go to a |page| with a link to a URL that has associated filtering rules. + GURL page("data:text/html,link"); + ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), page)); + + // Set up the source URL to an eTLD+1 that also has a filtering rule. + const GURL kSource("http://foo.com/test"); + + // Set up menu with link URL. + content::ContextMenuParams context_menu_params; + context_menu_params.page_url = kSource; + context_menu_params.link_url = redirect_page; + + // Select "Open Link in Incognito Window" and wait for window to be added. + TestRenderViewContextMenu menu( + *browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(), + context_menu_params); + menu.Init(); + menu.ExecuteCommand(IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD, 0); + + content::WebContents* tab = add_tab.Wait(); + EXPECT_TRUE(content::WaitForLoadStop(tab)); + // The prior load stop succeeds for the initial response; we now wait for the + // client redirect to occur. + content::LoadStopObserver client_redirect_load_observer(tab); + client_redirect_load_observer.Wait(); + // Verify that it loaded the filtered URL. + GURL expected(embedded_test_server()->GetURL("/empty.html?nochanges=2")); + ASSERT_EQ(expected, tab->GetLastCommittedURL()); + + tab->GetController().Reload(content::ReloadType::NORMAL, false); + EXPECT_TRUE(content::WaitForLoadStop(tab)); + tab->Close(); + + // The response was a 200-->client redirect-->200, and the navigation went + // from normal-->OTR browsing. Because we intervened, an artificial redirect + // was injected, so we also expect a 307. + histogram_tester.ExpectBucketCount( + kCrossOtrResponseMetricName, + net::HttpUtil::MapStatusCodeForHistogram(307), 1); + histogram_tester.ExpectBucketCount( + kCrossOtrResponseMetricName, + net::HttpUtil::MapStatusCodeForHistogram(200), 2); + histogram_tester.ExpectTotalCount(kCrossOtrRefreshCountMetricName, 1); + ASSERT_EQ(histogram_tester.GetTotalSum(kCrossOtrRefreshCountMetricName), 1); +} + // Verify that appropriate metrics are written when redirects are encountered. IN_PROC_BROWSER_TEST_F(ContextMenuIncognitoFilterBrowserTest, OpenIncognitoUrlParamFilterRedirect) { diff --git a/chrome/browser/url_param_filter/url_param_filter_throttle.cc b/chrome/browser/url_param_filter/url_param_filter_throttle.cc new file mode 100644 index 00000000000000..1fb5a2808abc4f --- /dev/null +++ b/chrome/browser/url_param_filter/url_param_filter_throttle.cc @@ -0,0 +1,72 @@ +// Copyright 2022 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/url_param_filter/url_param_filter_throttle.h" + +#include + +#include "chrome/browser/url_param_filter/cross_otr_observer.h" +#include "chrome/browser/url_param_filter/url_param_filterer.h" +#include "content/public/browser/browser_thread.h" +#include "net/url_request/redirect_info.h" +#include "services/network/public/cpp/resource_request.h" + +namespace url_param_filter { + +void UrlParamFilterThrottle::MaybeCreateThrottle( + content::WebContents* web_contents, + const network::ResourceRequest& request, + std::vector>* throttle_list) { + if (!web_contents) { + return; + } + // Only main frame navigations are in scope. We do not modify other + // navigations. + if (!request.is_main_frame) { + return; + } + CrossOtrObserver* observer = CrossOtrObserver::FromWebContents(web_contents); + if (observer && observer->IsCrossOtrState()) { + throttle_list->push_back( + std::make_unique(request.request_initiator)); + } +} + +UrlParamFilterThrottle::UrlParamFilterThrottle( + const absl::optional& request_initiator_origin) { + last_hop_initiator_ = request_initiator_origin.has_value() + ? request_initiator_origin->GetURL() + : GURL(); +} +UrlParamFilterThrottle::~UrlParamFilterThrottle() = default; + +void UrlParamFilterThrottle::DetachFromCurrentSequence() {} + +void UrlParamFilterThrottle::WillStartRequest(network::ResourceRequest* request, + bool* defer) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + request->url = FilterUrl(last_hop_initiator_, request->url); + last_hop_initiator_ = request->url; +} + +void UrlParamFilterThrottle::WillRedirectRequest( + net::RedirectInfo* redirect_info, + const network::mojom::URLResponseHead& response_head, + bool* defer, + std::vector* to_be_removed_request_headers, + net::HttpRequestHeaders* modified_request_headers, + net::HttpRequestHeaders* modified_cors_exempt_request_headers) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + redirect_info->new_url = + FilterUrl(last_hop_initiator_, redirect_info->new_url); + // Future redirects should use the redirect's domain as the navigation source. + last_hop_initiator_ = redirect_info->new_url; +} + +bool UrlParamFilterThrottle::makes_unsafe_redirect() { + // Scheme changes are not possible with this throttle. Only URL params are + // modified. + return false; +} +} // namespace url_param_filter diff --git a/chrome/browser/url_param_filter/url_param_filter_throttle.h b/chrome/browser/url_param_filter/url_param_filter_throttle.h new file mode 100644 index 00000000000000..199b6e2894f2bd --- /dev/null +++ b/chrome/browser/url_param_filter/url_param_filter_throttle.h @@ -0,0 +1,49 @@ +// Copyright 2022 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_URL_PARAM_FILTER_URL_PARAM_FILTER_THROTTLE_H_ +#define CHROME_BROWSER_URL_PARAM_FILTER_URL_PARAM_FILTER_THROTTLE_H_ + +#include + +#include "content/public/browser/web_contents.h" +#include "third_party/abseil-cpp/absl/types/optional.h" +#include "third_party/blink/public/common/loader/url_loader_throttle.h" +#include "url/origin.h" + +namespace url_param_filter { +class UrlParamFilterThrottle : public blink::URLLoaderThrottle { + public: + // Create the throttle if conditions warrant doing so, and add it to + // `throttle_list` if created. Otherwise, leave `throttle_list` unchanged and + // do nothing. + static void MaybeCreateThrottle( + content::WebContents* web_contents, + const network::ResourceRequest& request, + std::vector>* throttle_list); + explicit UrlParamFilterThrottle( + const absl::optional& request_initiator_origin); + ~UrlParamFilterThrottle() override; + + UrlParamFilterThrottle(const UrlParamFilterThrottle&) = delete; + UrlParamFilterThrottle& operator=(const UrlParamFilterThrottle&) = delete; + + // blink::URLLoaderThrottle implementation. + void DetachFromCurrentSequence() override; + void WillStartRequest(network::ResourceRequest* request, + bool* defer) override; + void WillRedirectRequest( + net::RedirectInfo* redirect_info, + const network::mojom::URLResponseHead& response_head, + bool* defer, + std::vector* to_be_removed_request_headers, + net::HttpRequestHeaders* modified_request_headers, + net::HttpRequestHeaders* modified_cors_exempt_request_headers) override; + bool makes_unsafe_redirect() override; + + private: + GURL last_hop_initiator_; +}; +} // namespace url_param_filter +#endif // CHROME_BROWSER_URL_PARAM_FILTER_URL_PARAM_FILTER_THROTTLE_H_ diff --git a/chrome/browser/url_param_filter/url_param_filter_throttle_unittest.cc b/chrome/browser/url_param_filter/url_param_filter_throttle_unittest.cc new file mode 100644 index 00000000000000..bafec6b7949635 --- /dev/null +++ b/chrome/browser/url_param_filter/url_param_filter_throttle_unittest.cc @@ -0,0 +1,249 @@ +// Copyright 2022 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/url_param_filter/url_param_filter_throttle.h" + +#include "base/test/scoped_feature_list.h" +#include "chrome/browser/ui/browser_navigator_params.h" +#include "chrome/browser/url_param_filter/cross_otr_observer.h" +#include "chrome/browser/url_param_filter/url_param_filter_test_helper.h" +#include "chrome/common/chrome_features.h" +#include "chrome/test/base/chrome_render_view_host_test_harness.h" +#include "net/http/http_request_headers.h" +#include "services/network/public/mojom/url_response_head.mojom.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "third_party/abseil-cpp/absl/types/optional.h" +#include "third_party/blink/public/common/loader/url_loader_throttle.h" +#include "url/origin.h" + +namespace url_param_filter { + +// Tests the UrlParamFilterThrottle, which is currently a very thin wrapper +// around the url_param_filter::FilterUrl() function. Coverage is accordingly +// somewhat less thorough than that seen in url_param_filterer_unittest. +class UrlParamFilterThrottleTest : public ChromeRenderViewHostTestHarness { + public: + UrlParamFilterThrottleTest() { + std::string encoded_classification = + CreateBase64EncodedFilterParamClassificationForTesting( + {{"source.xyz", {"plzblock"}}, + {"redirect.abc", {"plzblockredirect"}}, + {"redirect2.abc", {"plzblockredirect2"}}}, + {{"destination.xyz", {"plzblock1"}}}); + // With the flag set, the URL should be filtered. + scoped_feature_list_.InitAndEnableFeatureWithParameters( + features::kIncognitoParamFilterEnabled, + {{"classifications", encoded_classification}}); + } + + void CreateCrossOtrState() { + 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); + } + + private: + base::test::ScopedFeatureList scoped_feature_list_; +}; + +TEST_F(UrlParamFilterThrottleTest, ShouldCreateThrottleNullContents) { + network::ResourceRequest resource_request; + resource_request.is_main_frame = true; + std::vector> result; + + UrlParamFilterThrottle::MaybeCreateThrottle(nullptr, resource_request, + &result); + + ASSERT_EQ(result.size(), 0u); +} + +TEST_F(UrlParamFilterThrottleTest, ShouldCreateThrottleNotCrossOtr) { + network::ResourceRequest resource_request; + resource_request.is_main_frame = true; + std::vector> result; + + UrlParamFilterThrottle::MaybeCreateThrottle(web_contents(), resource_request, + &result); + + ASSERT_EQ(result.size(), 0u); +} + +TEST_F(UrlParamFilterThrottleTest, ShouldCreateThrottleNotMainFrame) { + CreateCrossOtrState(); + network::ResourceRequest resource_request; + resource_request.is_main_frame = false; + std::vector> result; + + UrlParamFilterThrottle::MaybeCreateThrottle(web_contents(), resource_request, + &result); + + ASSERT_EQ(result.size(), 0u); +} + +TEST_F(UrlParamFilterThrottleTest, ShouldCreateThrottleTrueCase) { + CreateCrossOtrState(); + network::ResourceRequest resource_request; + resource_request.is_main_frame = true; + std::vector> result; + + UrlParamFilterThrottle::MaybeCreateThrottle(web_contents(), resource_request, + &result); + + ASSERT_EQ(result.size(), 1u); +} + +TEST_F(UrlParamFilterThrottleTest, WillStartRequestNullInitiatorNoChanges) { + std::unique_ptr resource_request = + std::make_unique(); + GURL expected_url = GURL("https://no-rule.xyz?asdf=1"); + UrlParamFilterThrottle throttle = UrlParamFilterThrottle(absl::nullopt); + resource_request->url = expected_url; + + bool defer = false; + + throttle.WillStartRequest(resource_request.get(), &defer); + + ASSERT_EQ(resource_request->url, expected_url); + ASSERT_FALSE(defer); +} + +TEST_F(UrlParamFilterThrottleTest, WillStartRequestInitiatorChanges) { + std::unique_ptr resource_request = + std::make_unique(); + GURL destination_url = GURL("https://no-rule.xyz?asdf=1&plzblock=1"); + GURL expected_url = GURL("https://no-rule.xyz?asdf=1"); + absl::optional initiator = + absl::make_optional(url::Origin::Create(GURL("https://source.xyz"))); + UrlParamFilterThrottle throttle = UrlParamFilterThrottle(initiator); + resource_request->url = destination_url; + + bool defer = false; + + throttle.WillStartRequest(resource_request.get(), &defer); + + ASSERT_EQ(resource_request->url, expected_url); + ASSERT_FALSE(defer); +} +TEST_F(UrlParamFilterThrottleTest, + WillStartRequestNoInitiatorDestinationChanges) { + std::unique_ptr resource_request = + std::make_unique(); + GURL destination_url = GURL("https://destination.xyz?asdf=1&plzblock1=1"); + GURL expected_url = GURL("https://destination.xyz?asdf=1"); + UrlParamFilterThrottle throttle = UrlParamFilterThrottle(absl::nullopt); + resource_request->url = destination_url; + + bool defer = false; + + throttle.WillStartRequest(resource_request.get(), &defer); + + ASSERT_EQ(resource_request->url, expected_url); + ASSERT_FALSE(defer); +} + +TEST_F(UrlParamFilterThrottleTest, WillRedirectRequestNullInitiatorNoChanges) { + std::unique_ptr redirect_info = + std::make_unique(); + GURL expected_url = GURL("https://no-rule.xyz?asdf=1"); + UrlParamFilterThrottle throttle = UrlParamFilterThrottle(absl::nullopt); + redirect_info->new_url = expected_url; + + bool defer = false; + net::HttpRequestHeaders headers; + std::vector removed_headers; + auto response_head = network::mojom::URLResponseHead::New(); + + throttle.WillRedirectRequest(redirect_info.get(), *response_head, &defer, + &removed_headers, &headers, &headers); + + ASSERT_EQ(redirect_info->new_url, expected_url); + ASSERT_FALSE(defer); +} + +TEST_F(UrlParamFilterThrottleTest, WillRedirectRequestInitiatorChanges) { + std::unique_ptr redirect_info = + std::make_unique(); + GURL destination_url = GURL("https://no-rule.xyz?asdf=1&plzblock=1"); + GURL expected_url = GURL("https://no-rule.xyz?asdf=1"); + absl::optional initiator = + absl::make_optional(url::Origin::Create(GURL("https://source.xyz"))); + UrlParamFilterThrottle throttle = UrlParamFilterThrottle(initiator); + redirect_info->new_url = destination_url; + + bool defer = false; + net::HttpRequestHeaders headers; + std::vector removed_headers; + auto response_head = network::mojom::URLResponseHead::New(); + + throttle.WillRedirectRequest(redirect_info.get(), *response_head, &defer, + &removed_headers, &headers, &headers); + + ASSERT_EQ(redirect_info->new_url, expected_url); + ASSERT_FALSE(defer); +} +TEST_F(UrlParamFilterThrottleTest, + WillRedirectRequestNoInitiatorDestinationChanges) { + std::unique_ptr redirect_info = + std::make_unique(); + GURL destination_url = GURL("https://destination.xyz?asdf=1&plzblock1=1"); + GURL expected_url = GURL("https://destination.xyz?asdf=1"); + UrlParamFilterThrottle throttle = UrlParamFilterThrottle(absl::nullopt); + redirect_info->new_url = destination_url; + + bool defer = false; + net::HttpRequestHeaders headers; + std::vector removed_headers; + auto response_head = network::mojom::URLResponseHead::New(); + + throttle.WillRedirectRequest(redirect_info.get(), *response_head, &defer, + &removed_headers, &headers, &headers); + + ASSERT_EQ(redirect_info->new_url, expected_url); + ASSERT_FALSE(defer); +} + +TEST_F(UrlParamFilterThrottleTest, MultipleRedirects) { + std::unique_ptr redirect_info = + std::make_unique(); + std::unique_ptr resource_request = + std::make_unique(); + + // The chain is: source.xyz-->redirect.abc-->redirect2.abc-->destination.xyz. + resource_request->url = GURL("https://redirect.abc?plzblock=1"); + GURL expected_first_intermediate_url = GURL("https://redirect.abc"); + GURL redirect_url = GURL("https://redirect2.abc?plzblockredirect=1"); + GURL expected_second_intermediate_url = GURL("https://redirect2.abc"); + GURL destination_url = + GURL("https://destination.xyz?asdf=1&plzblockredirect2=1"); + GURL expected_url = GURL("https://destination.xyz?asdf=1"); + UrlParamFilterThrottle throttle = + UrlParamFilterThrottle(url::Origin::Create(GURL("https://source.xyz"))); + redirect_info->new_url = redirect_url; + + bool defer = false; + net::HttpRequestHeaders headers; + std::vector removed_headers; + auto response_head = network::mojom::URLResponseHead::New(); + + throttle.WillStartRequest(resource_request.get(), &defer); + ASSERT_EQ(resource_request->url, expected_first_intermediate_url); + + throttle.WillRedirectRequest(redirect_info.get(), *response_head, &defer, + &removed_headers, &headers, &headers); + ASSERT_EQ(redirect_info->new_url, expected_second_intermediate_url); + // The new source should be redirect.abc; the new destination includes + // plzblockredirect, which should be filtered. + redirect_info->new_url = destination_url; + throttle.WillRedirectRequest(redirect_info.get(), *response_head, &defer, + &removed_headers, &headers, &headers); + + ASSERT_EQ(redirect_info->new_url, expected_url); + ASSERT_FALSE(defer); +} + +} // namespace url_param_filter diff --git a/chrome/browser/url_param_filter/url_param_filterer.cc b/chrome/browser/url_param_filter/url_param_filterer.cc index 508ae1271641c2..553d2ac73f4b1d 100644 --- a/chrome/browser/url_param_filter/url_param_filterer.cc +++ b/chrome/browser/url_param_filter/url_param_filterer.cc @@ -49,6 +49,11 @@ FilterResult FilterUrl(const GURL& source_url, GURL result = GURL{destination_url}; int filtered_params_count = 0; + // If there's no query string, we can short-circuit immediately. + if (!destination_url.has_query()) { + return FilterResult{destination_url, filtered_params_count}; + } + std::string source_etld_plus1 = GetEtldPlusOne(source_url); std::string destination_etld_plus1 = GetEtldPlusOne(destination_url); @@ -106,7 +111,11 @@ FilterResult FilterUrl(const GURL& source_url, std::string new_query = base::JoinString(query_parts, "&"); GURL::Replacements replacements; - replacements.SetQueryStr(new_query); + if (new_query == "") { + replacements.ClearQuery(); + } else { + replacements.SetQueryStr(new_query); + } result = result.ReplaceComponents(replacements); return FilterResult{result, filtered_params_count}; } diff --git a/chrome/browser/url_param_filter/url_param_filterer_unittest.cc b/chrome/browser/url_param_filter/url_param_filterer_unittest.cc index f99e50f970d666..5d29849679e8c6 100644 --- a/chrome/browser/url_param_filter/url_param_filterer_unittest.cc +++ b/chrome/browser/url_param_filter/url_param_filterer_unittest.cc @@ -338,6 +338,47 @@ TEST_F(UrlParamFiltererTest, FeatureDeactivated) { ASSERT_EQ(result, expected); } +TEST_F(UrlParamFiltererTest, FeatureActivatedNoQueryString) { + GURL source = GURL{"http://source.xyz"}; + GURL destination = GURL{"https://destination.xyz"}; + + std::string encoded_classification = + CreateBase64EncodedFilterParamClassificationForTesting( + {{"source.xyz", {"plzblock"}}}, {{"destination.xyz", {"plzblock1"}}}); + + base::test::ScopedFeatureList scoped_feature_list; + // With the flag set, the URL should be filtered. + scoped_feature_list.InitAndEnableFeatureWithParameters( + features::kIncognitoParamFilterEnabled, + {{"classifications", encoded_classification}}); + + GURL expected = GURL{"https://destination.xyz"}; + GURL result = FilterUrl(source, destination); + + ASSERT_EQ(result, expected); +} + +TEST_F(UrlParamFiltererTest, FeatureActivatedAllRemoved) { + GURL source = GURL{"http://source.xyz"}; + GURL destination = + GURL{"https://destination.xyz?plzblock=adf&plzblock1=asdffdsa"}; + + std::string encoded_classification = + CreateBase64EncodedFilterParamClassificationForTesting( + {{"source.xyz", {"plzblock"}}}, {{"destination.xyz", {"plzblock1"}}}); + + base::test::ScopedFeatureList scoped_feature_list; + // With the flag set, the URL should be filtered. + scoped_feature_list.InitAndEnableFeatureWithParameters( + features::kIncognitoParamFilterEnabled, + {{"classifications", encoded_classification}}); + + GURL expected = GURL{"https://destination.xyz"}; + GURL result = FilterUrl(source, destination); + + ASSERT_EQ(result, expected); +} + TEST_F(UrlParamFiltererTest, FeatureActivatedSourceAndDestinationRemoval) { GURL source = GURL{"http://source.xyz"}; GURL destination = diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 4d86fa8ef7030a..d291726fb6a3b5 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -5195,6 +5195,7 @@ test("unit_tests") { "../browser/update_client/chrome_update_query_params_delegate_unittest.cc", "../browser/url_param_filter/cross_otr_observer_unittest.cc", "../browser/url_param_filter/url_param_classifications_loader_unittest.cc", + "../browser/url_param_filter/url_param_filter_throttle_unittest.cc", "../browser/url_param_filter/url_param_filterer_unittest.cc", "../browser/visibility_timer_tab_helper_unittest.cc", "../browser/vr/vr_tab_helper_unittest.cc",