Merge pull request #6774 from brave/pr6741_fix-direct-navigations-que…

…ry-filter-11924_1.16.x Do not exempt direct navigations from the query filter (uplift to 1.16.x)
brave · Oct 8, 2020 · fdf0283 · fdf0283
2 parents 82d85d1 + e62b4dc
commit fdf0283
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 7 deletions.
diff --git a/browser/net/brave_site_hacks_network_delegate_helper.cc b/browser/net/brave_site_hacks_network_delegate_helper.cc
@@ -79,11 +79,6 @@ DECLARE_LAZY_MATCHER(tracker_appended_matcher,
 void ApplyPotentialQueryStringFilter(std::shared_ptr<BraveRequestInfo> ctx) {
   SCOPED_UMA_HISTOGRAM_TIMER("Brave.SiteHacks.QueryFilter");
 
-  if (!ctx->initiator_url.is_valid()) {
-    // Direct navigations (e.g. omnibar, bookmarks) are exempted.
-    return;
-  }
-
   if (ctx->redirect_source.is_valid()) {
     if (ctx->internal_redirect) {
       // Ignore internal redirects since we trigger them.
@@ -96,7 +91,8 @@ void ApplyPotentialQueryStringFilter(std::shared_ptr<BraveRequestInfo> ctx) {
       // Same-site redirects are exempted.
       return;
     }
-  } else if (net::registry_controlled_domains::SameDomainOrHost(
+  } else if (ctx->initiator_url.is_valid() &&
+             net::registry_controlled_domains::SameDomainOrHost(
                  ctx->initiator_url, ctx->request_url,
                  net::registry_controlled_domains::
                      INCLUDE_PRIVATE_REGISTRIES)) {

diff --git a/browser/net/brave_site_hacks_network_delegate_helper_browsertest.cc b/browser/net/brave_site_hacks_network_delegate_helper_browsertest.cc
@@ -204,3 +204,31 @@ IN_PROC_BROWSER_TEST_F(BraveSiteHacksNetworkDelegateBrowserTest,
         landing_url(inputs[i], simple_landing_url()));
   }
 }
+
+IN_PROC_BROWSER_TEST_F(BraveSiteHacksNetworkDelegateBrowserTest,
+                       QueryStringFilterDirectNavigation) {
+  const std::string inputs[] = {
+      "",
+      "abc=1",
+      "fbclid=1",
+  };
+  const std::string outputs[] = {
+      // URLs without trackers should be untouched.
+      "",
+      "abc=1",
+      // URLs with trackers should have those removed.
+      "",
+  };
+
+  constexpr size_t input_count = base::size(inputs);
+  static_assert(input_count == base::size(outputs),
+                "Inputs and outputs must have the same number of elements.");
+
+  for (size_t i = 0; i < input_count; i++) {
+    // Direct navigations go through the query filter.
+    GURL input = landing_url(inputs[i], simple_landing_url());
+    GURL output = landing_url(outputs[i], simple_landing_url());
+    ui_test_utils::NavigateToURL(browser(), input);
+    EXPECT_EQ(contents()->GetLastCommittedURL(), output);
+  }
+}
diff --git a/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc b/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc
@@ -188,7 +188,6 @@ TEST(BraveSiteHacksNetworkDelegateHelperTest, QueryStringExempted) {
   const GURL tracking_url("https://example.com/?fbclid=1");
 
   const std::string initiators[] = {
-      "",                              // Direct navigation
       "https://example.com/path",      // Same-origin
       "https://sub.example.com/path",  // Same-site
   };
@@ -286,4 +285,15 @@ TEST(BraveSiteHacksNetworkDelegateHelperTest, QueryStringFiltered) {
     EXPECT_EQ(rc, net::OK);
     EXPECT_EQ(brave_request_info->new_url_spec, "https://example.com/");
   }
+
+  // Direct navigation
+  {
+    auto brave_request_info = std::make_shared<brave::BraveRequestInfo>(
+        GURL("https://example.com/?fbclid=2"));
+    brave_request_info->initiator_url = GURL();
+    int rc = brave::OnBeforeURLRequest_SiteHacksWork(ResponseCallback(),
+                                                     brave_request_info);
+    EXPECT_EQ(rc, net::OK);
+    EXPECT_EQ(brave_request_info->new_url_spec, "https://example.com/");
+  }
 }