Skip to content

Commit

Permalink
Exempt same-site redirects from query string filter.
Browse files Browse the repository at this point in the history
  • Loading branch information
@fmarier
fmarier committed Sep 22, 2020
1 parent 0cd9882 commit 9208797
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 19 deletions.
21 changes: 18 additions & 3 deletions browser/net/brave_proxying_url_loader_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,11 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::RestartInternal() {
base::BindRepeating(&InProgressRequest::ContinueToBeforeSendHeaders,
weak_factory_.GetWeakPtr());
redirect_url_ = GURL();
std::shared_ptr<brave::BraveRequestInfo> old_ctx = ctx_;
ctx_ = std::make_shared<brave::BraveRequestInfo>();
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_);

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -404,10 +409,11 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::
auto continuation = base::BindRepeating(
&InProgressRequest::ContinueToSendHeaders, weak_factory_.GetWeakPtr());

std::shared_ptr<brave::BraveRequestInfo> old_ctx = ctx_;
ctx_ = std::make_shared<brave::BraveRequestInfo>();
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);

Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -579,10 +593,11 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::
net::CompletionRepeatingCallback copyable_callback =
base::AdaptCallbackForRepeating(std::move(continuation));
if (request_.url.SchemeIsHTTPOrHTTPS()) {
std::shared_ptr<brave::BraveRequestInfo> old_ctx = ctx_;
ctx_ = std::make_shared<brave::BraveRequestInfo>();
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_);
Expand Down
9 changes: 6 additions & 3 deletions browser/net/brave_proxying_web_socket.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,11 @@ void BraveProxyingWebSocket::Start() {
weak_factory_.GetWeakPtr());
}

std::shared_ptr<brave::BraveRequestInfo> old_ctx = ctx_;
ctx_ = std::make_shared<brave::BraveRequestInfo>();
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
Expand Down Expand Up @@ -156,10 +157,11 @@ void BraveProxyingWebSocket::ContinueToHeadersReceived() {
auto continuation = base::BindRepeating(
&BraveProxyingWebSocket::OnHeadersReceivedComplete,
weak_factory_.GetWeakPtr());
std::shared_ptr<brave::BraveRequestInfo> old_ctx = ctx_;
ctx_ = std::make_shared<brave::BraveRequestInfo>();
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_);
Expand Down Expand Up @@ -267,10 +269,11 @@ void BraveProxyingWebSocket::OnBeforeSendHeadersCompleteFromProxy(
&BraveProxyingWebSocket::OnBeforeSendHeadersComplete,
weak_factory_.GetWeakPtr());

std::shared_ptr<brave::BraveRequestInfo> old_ctx = ctx_;
ctx_ = std::make_shared<brave::BraveRequestInfo>();
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);

Expand Down
40 changes: 27 additions & 13 deletions browser/net/brave_site_hacks_network_delegate_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<BraveRequestInfo> 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(), "") +
Expand All @@ -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();
}
}

Expand Down Expand Up @@ -134,9 +149,8 @@ bool ApplyPotentialReferrerBlock(std::shared_ptr<BraveRequestInfo> ctx) {
int OnBeforeURLRequest_SiteHacksWork(const ResponseCallback& next_callback,
std::shared_ptr<BraveRequestInfo> 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;
}
Expand Down
45 changes: 45 additions & 0 deletions browser/net/brave_site_hacks_network_delegate_helper_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<brave::BraveRequestInfo>(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<brave::BraveRequestInfo>(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) {
Expand Down Expand Up @@ -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<brave::BraveRequestInfo>(
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/");
}
}
9 changes: 9 additions & 0 deletions browser/net/url_context.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<brave::BraveRequestInfo> old_ctx,
std::shared_ptr<brave::BraveRequestInfo> ctx) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
ctx->request_identifier = request_identifier;
Expand Down Expand Up @@ -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
4 changes: 4 additions & 0 deletions browser/net/url_context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -109,6 +112,7 @@ struct BraveRequestInfo {
int frame_tree_node_id,
uint64_t request_identifier,
content::BrowserContext* browser_context,
std::shared_ptr<brave::BraveRequestInfo> old_ctx,
std::shared_ptr<brave::BraveRequestInfo> ctx);

private:
Expand Down

0 comments on commit 9208797

Please sign in to comment.