diff --git a/browser/net/brave_site_hacks_network_delegate_helper.cc b/browser/net/brave_site_hacks_network_delegate_helper.cc index e7f9ce51f6e1..ab178c1c7780 100644 --- a/browser/net/brave_site_hacks_network_delegate_helper.cc +++ b/browser/net/brave_site_hacks_network_delegate_helper.cc @@ -30,9 +30,11 @@ namespace brave { namespace { -const std::vector query_string_trackers = { - "fbclid", "gclid", "msclkid", "mc_eid" -}; +const std::vector& GetQueryStringTrackers() { + static const base::NoDestructor> trackers( + {"fbclid", "gclid", "msclkid", "mc_eid"}); + return *trackers; +} bool ApplyPotentialReferrerBlock(std::shared_ptr ctx) { DCHECK_CURRENTLY_ON(BrowserThread::IO); @@ -55,17 +57,14 @@ bool ApplyPotentialReferrerBlock(std::shared_ptr ctx) { return false; } -bool ApplyPotentialQueryStringFilter(std::shared_ptr ctx) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - if (!ctx->request_url.has_query()) { - return false; - } - - std::string new_query = ctx->request_url.query(); +void ApplyPotentialQueryStringFilter(const GURL& request_url, + std::string* new_url_spec) { + std::string new_query = request_url.query(); bool modified = false; - for (auto tracker : query_string_trackers) { - size_t key_size, key_start; + for (auto tracker : GetQueryStringTrackers()) { + size_t key_size = 0; + size_t key_start = std::string::npos; // Look for the tracker anywhere in the query string // (e.g. ?...fbclid=1234...). @@ -125,12 +124,8 @@ bool ApplyPotentialQueryStringFilter(std::shared_ptr ctx) { replacements.SetQuery(new_query.c_str(), url::Component(0, new_query.size())); } - ctx->new_url_spec = - ctx->request_url.ReplaceComponents(replacements).spec(); - return true; + *new_url_spec = request_url.ReplaceComponents(replacements).spec(); } - - return false; } } // namespace @@ -139,7 +134,10 @@ int OnBeforeURLRequest_SiteHacksWork( const ResponseCallback& next_callback, std::shared_ptr ctx) { ApplyPotentialReferrerBlock(ctx); - ApplyPotentialQueryStringFilter(ctx); + + if (ctx->request_url.has_query()) { + ApplyPotentialQueryStringFilter(ctx->request_url, &ctx->new_url_spec); + } return net::OK; } diff --git a/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc b/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc index aef25e7f0291..3477f19371b3 100644 --- a/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc +++ b/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc @@ -269,85 +269,84 @@ TEST_F(BraveSiteHacksNetworkDelegateHelperTest, } TEST_F(BraveSiteHacksNetworkDelegateHelperTest, QueryStringUntouched) { - std::vector urls({ - GURL("https://example.com/"), - GURL("https://example.com/?"), - GURL("https://example.com/?+%20"), - GURL("https://user:pass@example.com/path/file.html?foo=1#fragment"), - GURL("http://user:pass@example.com/path/file.html?foo=1&bar=2#fragment"), - GURL("https://example.com/?file=https%3A%2F%2Fexample.com%2Ftest.pdf"), - GURL("https://example.com/?title=1+2&caption=1%202"), - GURL("https://example.com/?foo=1&&bar=2#fragment"), - GURL("https://example.com/?foo&bar=&#fragment"), - GURL("https://example.com/?foo=1&fbcid=no&gcid=no&mc_cid=no&bar=&#frag"), - GURL("https://example.com/?fbclid=&gclid&=mc_eid&msclkid="), - GURL("https://example.com/?value=fbclid=1¬-gclid=2&foo+mc_eid=3"), - GURL("https://example.com/?+fbclid=1"), - GURL("https://example.com/?%20fbclid=1"), - GURL("https://example.com/#fbclid=1"), + const std::vector urls({ + "https://example.com/", + "https://example.com/?", + "https://example.com/?+%20", + "https://user:pass@example.com/path/file.html?foo=1#fragment", + "http://user:pass@example.com/path/file.html?foo=1&bar=2#fragment", + "https://example.com/?file=https%3A%2F%2Fexample.com%2Ftest.pdf", + "https://example.com/?title=1+2&caption=1%202", + "https://example.com/?foo=1&&bar=2#fragment", + "https://example.com/?foo&bar=&#fragment", + "https://example.com/?foo=1&fbcid=no&gcid=no&mc_cid=no&bar=&#frag", + "https://example.com/?fbclid=&gclid&=mc_eid&msclkid=", + "https://example.com/?value=fbclid=1¬-gclid=2&foo+mc_eid=3", + "https://example.com/?+fbclid=1", + "https://example.com/?%20fbclid=1", + "https://example.com/#fbclid=1", }); - std::for_each(urls.begin(), urls.end(), [this](GURL url){ + for (const auto& url : urls) { net::TestDelegate test_delegate; - std::unique_ptr request = - context()->CreateRequest(url, net::IDLE, &test_delegate, - TRAFFIC_ANNOTATION_FOR_TESTS); + std::unique_ptr request = context()->CreateRequest( + GURL(url), net::IDLE, &test_delegate, TRAFFIC_ANNOTATION_FOR_TESTS); - std::shared_ptr - brave_request_info(new brave::BraveRequestInfo()); + std::shared_ptr brave_request_info( + new brave::BraveRequestInfo()); brave::BraveRequestInfo::FillCTXFromRequest(request.get(), - brave_request_info); + brave_request_info); brave::ResponseCallback callback; - int ret = brave::OnBeforeURLRequest_SiteHacksWork(callback, - brave_request_info); + int ret = + brave::OnBeforeURLRequest_SiteHacksWork(callback, brave_request_info); EXPECT_EQ(ret, net::OK); // new_url should not be set EXPECT_TRUE(brave_request_info->new_url_spec.empty()); - EXPECT_EQ(request->url(), url); - }); + EXPECT_EQ(request->url(), GURL(url)); + } } TEST_F(BraveSiteHacksNetworkDelegateHelperTest, QueryStringFiltered) { - std::vector > urls({ - // { original url, expected url after filtering } - { GURL("https://example.com/?fbclid=1234"), GURL("https://example.com/") }, - { GURL("https://example.com/?fbclid=1234&"), GURL("https://example.com/") }, - { GURL("https://example.com/?&fbclid=1234"), GURL("https://example.com/") }, - { GURL("https://example.com/?gclid=1234"), GURL("https://example.com/") }, - { GURL("https://example.com/?fbclid=0&gclid=1&msclkid=a&mc_eid=a1"), - GURL("https://example.com/") }, - { GURL("https://example.com/?fbclid=&foo=1&bar=2&gclid=abc"), - GURL("https://example.com/?fbclid=&foo=1&bar=2") }, - { GURL("https://example.com/?fbclid=&foo=1&gclid=1234&bar=2"), - GURL("https://example.com/?fbclid=&foo=1&bar=2") }, - { GURL("http://u:p@example.com/path/file.html?foo=1&fbclid=abcd#fragment"), - GURL("http://u:p@example.com/path/file.html?foo=1#fragment") }, - // Obscure edge cases that break most parsers: - { GURL("https://example.com/?fbclid&foo&&gclid=2&bar=&%20"), - GURL("https://example.com/?fbclid&foo&&bar=&%20") }, - { GURL("https://example.com/?fbclid=1&1==2&=msclkid&foo=bar&&a=b=c&"), - GURL("https://example.com/?1==2&=msclkid&foo=bar&&a=b=c&") }, - { GURL("https://example.com/?fbclid=1&=2&?foo=yes&bar=2+"), - GURL("https://example.com/?=2&?foo=yes&bar=2+") }, - { GURL("https://example.com/?fbclid=1&a+b+c=some%20thing&1%202=3+4"), - GURL("https://example.com/?a+b+c=some%20thing&1%202=3+4") }, - }); - std::for_each(urls.begin(), urls.end(), [this](std::pair url){ + const std::vector> urls( + { + // { original url, expected url after filtering } + {"https://example.com/?fbclid=1234", "https://example.com/"}, + {"https://example.com/?fbclid=1234&", "https://example.com/"}, + {"https://example.com/?&fbclid=1234", "https://example.com/"}, + {"https://example.com/?gclid=1234", "https://example.com/"}, + {"https://example.com/?fbclid=0&gclid=1&msclkid=a&mc_eid=a1", + "https://example.com/"}, + {"https://example.com/?fbclid=&foo=1&bar=2&gclid=abc", + "https://example.com/?fbclid=&foo=1&bar=2"}, + {"https://example.com/?fbclid=&foo=1&gclid=1234&bar=2", + "https://example.com/?fbclid=&foo=1&bar=2"}, + {"http://u:p@example.com/path/file.html?foo=1&fbclid=abcd#fragment", + "http://u:p@example.com/path/file.html?foo=1#fragment"}, + // Obscure edge cases that break most parsers: + {"https://example.com/?fbclid&foo&&gclid=2&bar=&%20", + "https://example.com/?fbclid&foo&&bar=&%20"}, + {"https://example.com/?fbclid=1&1==2&=msclkid&foo=bar&&a=b=c&", + "https://example.com/?1==2&=msclkid&foo=bar&&a=b=c&"}, + {"https://example.com/?fbclid=1&=2&?foo=yes&bar=2+", + "https://example.com/?=2&?foo=yes&bar=2+"}, + {"https://example.com/?fbclid=1&a+b+c=some%20thing&1%202=3+4", + "https://example.com/?a+b+c=some%20thing&1%202=3+4"}, + }); + for (const auto& pair : urls) { net::TestDelegate test_delegate; std::unique_ptr request = - context()->CreateRequest(url.first, net::IDLE, &test_delegate, + context()->CreateRequest(GURL(pair.first), net::IDLE, &test_delegate, TRAFFIC_ANNOTATION_FOR_TESTS); - std::shared_ptr - brave_request_info(new brave::BraveRequestInfo()); + std::shared_ptr brave_request_info( + new brave::BraveRequestInfo()); brave::BraveRequestInfo::FillCTXFromRequest(request.get(), - brave_request_info); + brave_request_info); brave::ResponseCallback callback; - int ret = brave::OnBeforeURLRequest_SiteHacksWork(callback, - brave_request_info); + int ret = + brave::OnBeforeURLRequest_SiteHacksWork(callback, brave_request_info); EXPECT_EQ(ret, net::OK); - EXPECT_STREQ(brave_request_info->new_url_spec.c_str(), - url.second.spec().c_str()); - }); + EXPECT_EQ(brave_request_info->new_url_spec, pair.second); + } } } // namespace