From 9208797737e96ac6d00624d7df20d5a881781db3 Mon Sep 17 00:00:00 2001 From: Francois Marier Date: Fri, 4 Sep 2020 18:10:41 -0700 Subject: [PATCH] Exempt same-site redirects from query string filter. --- .../net/brave_proxying_url_loader_factory.cc | 21 +++++++-- browser/net/brave_proxying_web_socket.cc | 9 ++-- ...rave_site_hacks_network_delegate_helper.cc | 40 +++++++++++------ ..._hacks_network_delegate_helper_unittest.cc | 45 +++++++++++++++++++ browser/net/url_context.cc | 9 ++++ browser/net/url_context.h | 4 ++ 6 files changed, 109 insertions(+), 19 deletions(-) diff --git a/browser/net/brave_proxying_url_loader_factory.cc b/browser/net/brave_proxying_url_loader_factory.cc index 0bcc0cd6d6e4..fdbfa4eb396d 100644 --- a/browser/net/brave_proxying_url_loader_factory.cc +++ b/browser/net/brave_proxying_url_loader_factory.cc @@ -143,10 +143,11 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { base::BindRepeating(&InProgressRequest::ContinueToBeforeSendHeaders, weak_factory_.GetWeakPtr()); redirect_url_ = GURL(); + std::shared_ptr old_ctx = ctx_; ctx_ = std::make_shared(); brave::BraveRequestInfo::FillCTX(request_, render_process_id_, frame_tree_node_id_, request_id_, - browser_context_, ctx_); + browser_context_, old_ctx, ctx_); int result = factory_->request_handler_->OnBeforeURLRequest( ctx_, continuation, &redirect_url_); @@ -231,6 +232,8 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::OnReceiveRedirect( const net::RedirectInfo& redirect_info, network::mojom::URLResponseHeadPtr head) { current_response_ = std::move(head); + DCHECK(ctx_); + ctx_->internal_redirect = false; HandleResponseOrRedirectHeaders( base::BindRepeating(&InProgressRequest::ContinueToBeforeRedirect, weak_factory_.GetWeakPtr(), redirect_info)); @@ -337,6 +340,8 @@ void BraveProxyingURLLoaderFactory::InProgressRequest:: head->encoded_data_length = 0; current_response_ = std::move(head); + DCHECK(ctx_); + ctx_->internal_redirect = true; ContinueToBeforeRedirect(redirect_info, net::OK); } @@ -404,10 +409,11 @@ void BraveProxyingURLLoaderFactory::InProgressRequest:: auto continuation = base::BindRepeating( &InProgressRequest::ContinueToSendHeaders, weak_factory_.GetWeakPtr()); + std::shared_ptr old_ctx = ctx_; ctx_ = std::make_shared(); brave::BraveRequestInfo::FillCTX(request_, render_process_id_, frame_tree_node_id_, request_id_, - browser_context_, ctx_); + browser_context_, old_ctx, ctx_); int result = factory_->request_handler_->OnBeforeStartTransaction( ctx_, continuation, &request_.headers); @@ -530,6 +536,8 @@ void BraveProxyingURLLoaderFactory::InProgressRequest:: proxied_client_receiver_.reset(); target_loader_.reset(); + DCHECK(ctx_); + ctx_->internal_redirect = true; ContinueToBeforeRedirect(redirect_info, net::OK); return; } @@ -549,6 +557,12 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeRedirect( if (proxied_client_receiver_.is_bound()) proxied_client_receiver_.Resume(); + if (ctx_->internal_redirect) { + ctx_->redirect_source = GURL(); + } else { + ctx_->redirect_source = request_.url; + } + target_client_->OnReceiveRedirect(redirect_info, std::move(current_response_)); request_.url = redirect_info.new_url; @@ -579,10 +593,11 @@ void BraveProxyingURLLoaderFactory::InProgressRequest:: net::CompletionRepeatingCallback copyable_callback = base::AdaptCallbackForRepeating(std::move(continuation)); if (request_.url.SchemeIsHTTPOrHTTPS()) { + std::shared_ptr old_ctx = ctx_; ctx_ = std::make_shared(); brave::BraveRequestInfo::FillCTX(request_, render_process_id_, frame_tree_node_id_, request_id_, - browser_context_, ctx_); + browser_context_, old_ctx, ctx_); int result = factory_->request_handler_->OnHeadersReceived( ctx_, copyable_callback, current_response_->headers.get(), &override_headers_, &redirect_url_); diff --git a/browser/net/brave_proxying_web_socket.cc b/browser/net/brave_proxying_web_socket.cc index dba8258953c2..a589e8c4cd76 100644 --- a/browser/net/brave_proxying_web_socket.cc +++ b/browser/net/brave_proxying_web_socket.cc @@ -85,10 +85,11 @@ void BraveProxyingWebSocket::Start() { weak_factory_.GetWeakPtr()); } + std::shared_ptr old_ctx = ctx_; ctx_ = std::make_shared(); brave::BraveRequestInfo::FillCTX(request_, process_id_, frame_tree_node_id_, request_id_, - browser_context_, ctx_); + browser_context_, old_ctx, ctx_); int result = request_handler_->OnBeforeURLRequest( ctx_, continuation, &redirect_url_); // TODO(bridiver) - need to handle general case for redirect_url @@ -156,10 +157,11 @@ void BraveProxyingWebSocket::ContinueToHeadersReceived() { auto continuation = base::BindRepeating( &BraveProxyingWebSocket::OnHeadersReceivedComplete, weak_factory_.GetWeakPtr()); + std::shared_ptr old_ctx = ctx_; ctx_ = std::make_shared(); brave::BraveRequestInfo::FillCTX(request_, process_id_, frame_tree_node_id_, request_id_, - browser_context_, ctx_); + browser_context_, old_ctx, ctx_); int result = request_handler_->OnHeadersReceived( ctx_, continuation, response_.headers.get(), &override_headers_, &redirect_url_); @@ -267,10 +269,11 @@ void BraveProxyingWebSocket::OnBeforeSendHeadersCompleteFromProxy( &BraveProxyingWebSocket::OnBeforeSendHeadersComplete, weak_factory_.GetWeakPtr()); + std::shared_ptr old_ctx = ctx_; ctx_ = std::make_shared(); brave::BraveRequestInfo::FillCTX(request_, process_id_, frame_tree_node_id_, request_id_, - browser_context_, ctx_); + browser_context_, old_ctx, ctx_); int result = request_handler_->OnBeforeStartTransaction( ctx_, continuation, &request_.headers); diff --git a/browser/net/brave_site_hacks_network_delegate_helper.cc b/browser/net/brave_site_hacks_network_delegate_helper.cc index 7f7e53bbf72b..586dc4840d52 100644 --- a/browser/net/brave_site_hacks_network_delegate_helper.cc +++ b/browser/net/brave_site_hacks_network_delegate_helper.cc @@ -74,20 +74,35 @@ DECLARE_LAZY_MATCHER(tracker_appended_matcher, #undef DECLARE_LAZY_MATCHER -void ApplyPotentialQueryStringFilter(const GURL& initiator_url, - const GURL& request_url, - std::string* new_url_spec) { - DCHECK(new_url_spec); +void ApplyPotentialQueryStringFilter(std::shared_ptr ctx) { SCOPED_UMA_HISTOGRAM_TIMER("Brave.SiteHacks.QueryFilter"); - // Same-site requests are exempted from the filter. - if (net::registry_controlled_domains::SameDomainOrHost( - initiator_url, request_url, - net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) { + if (!ctx->initiator_url.is_valid()) { + // Direct navigations (e.g. omnibar, bookmarks) are exempted. return; } - std::string new_query = request_url.query(); + if (ctx->redirect_source.is_valid()) { + if (ctx->internal_redirect) { + // Ignore internal redirects since we trigger them. + return; + } + + if (net::registry_controlled_domains::SameDomainOrHost( + ctx->redirect_source, ctx->request_url, + net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) { + // Same-site redirects are exempted. + return; + } + } else if (net::registry_controlled_domains::SameDomainOrHost( + ctx->initiator_url, ctx->request_url, + net::registry_controlled_domains:: + INCLUDE_PRIVATE_REGISTRIES)) { + // Same-site requests are exempted. + return; + } + + std::string new_query = ctx->request_url.query(); // Note: the ordering of these replacements is important. const int replacement_count = re2::RE2::GlobalReplace(&new_query, tracker_appended_matcher.Get(), "") + @@ -102,7 +117,7 @@ void ApplyPotentialQueryStringFilter(const GURL& initiator_url, replacements.SetQuery(new_query.c_str(), url::Component(0, new_query.size())); } - *new_url_spec = request_url.ReplaceComponents(replacements).spec(); + ctx->new_url_spec = ctx->request_url.ReplaceComponents(replacements).spec(); } } @@ -134,9 +149,8 @@ bool ApplyPotentialReferrerBlock(std::shared_ptr ctx) { int OnBeforeURLRequest_SiteHacksWork(const ResponseCallback& next_callback, std::shared_ptr ctx) { ApplyPotentialReferrerBlock(ctx); - if (ctx->request_url.has_query() && ctx->initiator_url.is_valid()) { - ApplyPotentialQueryStringFilter(ctx->initiator_url, ctx->request_url, - &ctx->new_url_spec); + if (ctx->request_url.has_query()) { + ApplyPotentialQueryStringFilter(ctx); } 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 e534213ba257..521dcb15b0eb 100644 --- a/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc +++ b/browser/net/brave_site_hacks_network_delegate_helper_unittest.cc @@ -203,6 +203,37 @@ TEST(BraveSiteHacksNetworkDelegateHelperTest, QueryStringExempted) { // new_url should not be set EXPECT_TRUE(brave_request_info->new_url_spec.empty()); } + + // Internal redirect + { + auto brave_request_info = + std::make_shared(tracking_url); + brave_request_info->initiator_url = + GURL("https://example.net"); // cross-site + brave_request_info->internal_redirect = true; + brave_request_info->redirect_source = + GURL("https://example.org"); // cross-site + int rc = brave::OnBeforeURLRequest_SiteHacksWork(ResponseCallback(), + brave_request_info); + EXPECT_EQ(rc, net::OK); + // new_url should not be set + EXPECT_TRUE(brave_request_info->new_url_spec.empty()); + } + + // Same-site redirect + { + auto brave_request_info = + std::make_shared(tracking_url); + brave_request_info->initiator_url = + GURL("https://example.net"); // cross-site + brave_request_info->redirect_source = + GURL("https://sub.example.com"); // same-site + int rc = brave::OnBeforeURLRequest_SiteHacksWork(ResponseCallback(), + brave_request_info); + EXPECT_EQ(rc, net::OK); + // new_url should not be set + EXPECT_TRUE(brave_request_info->new_url_spec.empty()); + } } TEST(BraveSiteHacksNetworkDelegateHelperTest, QueryStringFiltered) { @@ -241,4 +272,18 @@ TEST(BraveSiteHacksNetworkDelegateHelperTest, QueryStringFiltered) { EXPECT_EQ(rc, net::OK); EXPECT_EQ(brave_request_info->new_url_spec, pair.second); } + + // Cross-site redirect + { + auto brave_request_info = std::make_shared( + GURL("https://example.com/?fbclid=1")); + brave_request_info->initiator_url = + GURL("https://example.com"); // same-origin + brave_request_info->redirect_source = + GURL("https://example.net"); // cross-site + 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/"); + } } diff --git a/browser/net/url_context.cc b/browser/net/url_context.cc index 42b0b61f14f4..9e9d5877dadd 100644 --- a/browser/net/url_context.cc +++ b/browser/net/url_context.cc @@ -58,6 +58,7 @@ void BraveRequestInfo::FillCTX(const network::ResourceRequest& request, int frame_tree_node_id, uint64_t request_identifier, content::BrowserContext* browser_context, + std::shared_ptr old_ctx, std::shared_ptr ctx) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); ctx->request_identifier = request_identifier; @@ -124,6 +125,14 @@ void BraveRequestInfo::FillCTX(const network::ResourceRequest& request, prefs->GetInteger(kIPFSResolveMethod)) == ipfs::IPFSResolveMethodTypes::IPFS_LOCAL; #endif + + // TODO(fmarier): remove this once the hacky code in + // brave_proxying_url_loader_factory.cc is refactored. See + // BraveProxyingURLLoaderFactory::InProgressRequest::UpdateRequestInfo(). + if (old_ctx) { + ctx->internal_redirect = old_ctx->internal_redirect; + ctx->redirect_source = old_ctx->redirect_source; + } } } // namespace brave diff --git a/browser/net/url_context.h b/browser/net/url_context.h index 657fb7afc5b9..230a861322ea 100644 --- a/browser/net/url_context.h +++ b/browser/net/url_context.h @@ -61,6 +61,9 @@ struct BraveRequestInfo { GURL tab_url; GURL initiator_url; + bool internal_redirect = false; + GURL redirect_source; + GURL referrer; net::ReferrerPolicy referrer_policy = net::ReferrerPolicy::CLEAR_ON_TRANSITION_FROM_SECURE_TO_INSECURE; @@ -109,6 +112,7 @@ struct BraveRequestInfo { int frame_tree_node_id, uint64_t request_identifier, content::BrowserContext* browser_context, + std::shared_ptr old_ctx, std::shared_ptr ctx); private: