Skip to content

Commit

Permalink
13464: Unrestrict referrer hiding for top-level navigations.
Browse files Browse the repository at this point in the history
To solve webcompat problems we replace forcing "no-referrer"
for cross-site top-level navigations with capping with
"strict-origin-when-cross-origin".

Fix brave/brave-browser#13464
  • Loading branch information
iefremov committed Jan 14, 2021
1 parent 4afee1e commit 739093e
Show file tree
Hide file tree
Showing 10 changed files with 71 additions and 87 deletions.
13 changes: 1 addition & 12 deletions browser/brave_content_browser_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,6 @@ void BraveContentBrowserClient::MaybeHideReferrer(
content::BrowserContext* browser_context,
const GURL& request_url,
const GURL& document_url,
bool is_main_frame,
const std::string& method,
blink::mojom::ReferrerPtr* referrer) {
DCHECK(referrer && !referrer->is_null());
#if BUILDFLAG(ENABLE_EXTENSIONS)
Expand All @@ -368,18 +366,9 @@ void BraveContentBrowserClient::MaybeHideReferrer(
HostContentSettingsMapFactory::GetForProfile(profile),
document_url);

// Some top-level navigations get empty referrers (brave/brave-browser#3422).
network::mojom::ReferrerPolicy policy = (*referrer)->policy;
if (is_main_frame &&
brave_shields::ShouldCleanReferrerForTopLevelNavigation(method,
(*referrer)->url,
request_url)) {
policy = network::mojom::ReferrerPolicy::kNever;
}

content::Referrer new_referrer;
if (brave_shields::MaybeChangeReferrer(
allow_referrers, shields_up, (*referrer)->url, request_url, policy,
allow_referrers, shields_up, (*referrer)->url, request_url,
&new_referrer)) {
(*referrer)->url = new_referrer.url;
(*referrer)->policy = new_referrer.policy;
Expand Down
2 changes: 0 additions & 2 deletions browser/brave_content_browser_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ class BraveContentBrowserClient : public ChromeContentBrowserClient {
void MaybeHideReferrer(content::BrowserContext* browser_context,
const GURL& request_url,
const GURL& document_url,
bool is_main_frame,
const std::string& method,
blink::mojom::ReferrerPtr* referrer) override;

GURL GetEffectiveURL(content::BrowserContext* browser_context,
Expand Down
23 changes: 8 additions & 15 deletions browser/brave_content_browser_client_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -496,49 +496,42 @@ IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientReferrerTest,
blink::mojom::ReferrerPtr kReferrer = blink::mojom::Referrer::New(
kDocumentUrl, network::mojom::ReferrerPolicy::kDefault);

// Cross-origin navigations don't get a referrer.
// Cross-origin navigations get an origin.
blink::mojom::ReferrerPtr referrer = kReferrer.Clone();
client()->MaybeHideReferrer(browser()->profile(), kRequestUrl, kDocumentUrl,
true, "GET", &referrer);
EXPECT_EQ(referrer->url, GURL());

// Cross-origin navigations get a truncated referrer if method is not "GET" or
// "HEAD".
referrer = kReferrer.Clone();
client()->MaybeHideReferrer(browser()->profile(), kRequestUrl, kDocumentUrl,
true, "POST", &referrer);
&referrer);
EXPECT_EQ(referrer->url, kDocumentUrl.GetOrigin());

// Same-origin navigations get full referrers.
referrer = kReferrer.Clone();
client()->MaybeHideReferrer(browser()->profile(), kSameOriginRequestUrl,
kDocumentUrl, true, "GET", &referrer);
kDocumentUrl, &referrer);
EXPECT_EQ(referrer->url, kDocumentUrl);

// Same-site navigations get truncated referrers.
referrer = kReferrer.Clone();
client()->MaybeHideReferrer(browser()->profile(), kSameSiteRequestUrl,
kDocumentUrl, true, "GET", &referrer);
kDocumentUrl, &referrer);
EXPECT_EQ(referrer->url, kDocumentUrl.GetOrigin());

// Cross-origin iframe navigations get origins.
referrer = kReferrer.Clone();
client()->MaybeHideReferrer(browser()->profile(), kRequestUrl, kDocumentUrl,
false, "GET", &referrer);
&referrer);
EXPECT_EQ(referrer->url, kDocumentUrl.GetOrigin().spec());

// Same-origin iframe navigations get full referrers.
referrer = kReferrer.Clone();
client()->MaybeHideReferrer(browser()->profile(), kSameOriginRequestUrl,
kDocumentUrl, false, "GET", &referrer);
kDocumentUrl, &referrer);
EXPECT_EQ(referrer->url, kDocumentUrl);

// Special rule for extensions.
const GURL kExtensionUrl("chrome-extension://abc/path?query");
referrer = kReferrer.Clone();
referrer->url = kExtensionUrl;
client()->MaybeHideReferrer(browser()->profile(), kRequestUrl, kExtensionUrl,
true, "GET", &referrer);
&referrer);
EXPECT_EQ(referrer->url, kExtensionUrl);

// Allow referrers for certain URL.
Expand All @@ -548,6 +541,6 @@ IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientReferrerTest,
CONTENT_SETTING_ALLOW);
referrer = kReferrer.Clone();
client()->MaybeHideReferrer(browser()->profile(), kRequestUrl, kDocumentUrl,
true, "GET", &referrer);
&referrer);
EXPECT_EQ(referrer->url, kDocumentUrl);
}
16 changes: 7 additions & 9 deletions browser/net/brave_site_hacks_network_delegate_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,8 @@ bool ApplyPotentialReferrerBlock(std::shared_ptr<BraveRequestInfo> ctx) {

content::Referrer new_referrer;
if (brave_shields::MaybeChangeReferrer(
ctx->allow_referrers, ctx->allow_brave_shields,
GURL(ctx->referrer), ctx->request_url,
blink::ReferrerUtils::NetToMojoReferrerPolicy(ctx->referrer_policy),
&new_referrer)) {
ctx->allow_referrers, ctx->allow_brave_shields, GURL(ctx->referrer),
ctx->request_url, &new_referrer)) {
ctx->new_referrer = new_referrer.url;
return true;
}
Expand Down Expand Up @@ -179,14 +177,14 @@ int OnBeforeStartTransaction_SiteHacksWork(
// will affect performance).
// Note that this code only affects "Referer" header sent via network - we
// handle document.referer in content::NavigationRequest (see also
// BraveContentBrowserClient::MaybeHideReferrer).
// |BraveContentBrowserClient::MaybeHideReferrer|).
if (!ctx->allow_referrers && ctx->allow_brave_shields &&
ctx->redirect_source.is_valid() &&
ctx->resource_type == blink::mojom::ResourceType::kMainFrame &&
brave_shields::ShouldCleanReferrerForTopLevelNavigation(
ctx->method, ctx->redirect_source, ctx->request_url)) {
// This is hack that notifies the patched code in net::URLRequest.
ctx->removed_headers.insert("X-Brave-Clear-Referer");
!brave_shields::IsSameOriginNavigation(ctx->redirect_source,
ctx->request_url)) {
// This is a hack that notifies the network layer.
ctx->removed_headers.insert("X-Brave-Cap-Referer");
}
return net::OK;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,12 @@ GURL GetTopDocumentGURL(content::FrameTreeNode* frame_tree_node) {
frame_tree_node_->navigator().GetController()->GetBrowserContext(); \
GetContentClient()->browser()->MaybeHideReferrer( \
browser_context, common_params_->url, \
GetTopDocumentGURL(frame_tree_node_), frame_tree_node_->IsMainFrame(), \
common_params_->method, &common_params_->referrer);
GetTopDocumentGURL(frame_tree_node_), &common_params_->referrer);

#define BRAVE_ONSTARTCHECKSCOMPLETE_MAYBEHIDEREFERRER \
GetContentClient()->browser()->MaybeHideReferrer( \
browser_context, common_params_->url, \
GetTopDocumentGURL(frame_tree_node_), frame_tree_node_->IsMainFrame(), \
common_params_->method, &common_params_->referrer);
GetTopDocumentGURL(frame_tree_node_), &common_params_->referrer);

#include "../../../../../content/browser/renderer_host/navigation_request.cc"

Expand Down
3 changes: 1 addition & 2 deletions chromium_src/content/public/browser/content_browser_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@
#define BRAVE_CONTENT_BROWSER_CLIENT_H \
virtual void MaybeHideReferrer( \
BrowserContext* browser_context, const GURL& request_url, \
const GURL& document_url, bool is_main_frame, \
const std::string& method, \
const GURL& document_url, \
blink::mojom::ReferrerPtr* referrer) {}

#include "../../../../../content/public/browser/content_browser_client.h"
Expand Down
16 changes: 11 additions & 5 deletions chromium_src/net/url_request/redirect_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@

#include "net/url_request/redirect_util.h"

#include "base/stl_util.h"
#include "net/url_request/url_request_job.h"

#define UpdateHttpRequest UpdateHttpRequest_ChromiumImpl
#include "../../../../net/url_request/redirect_util.cc"
#undef UpdateHttpRequest
Expand All @@ -26,12 +29,15 @@ void RedirectUtil::UpdateHttpRequest(
modified_headers,
request_headers,
should_clear_upload);
// Hack for dropping referrer at the network layer.
// Hack for capping referrers at the network layer.
if (removed_headers) {
if (removed_headers->end() != std::find(removed_headers->begin(),
removed_headers->end(),
"X-Brave-Clear-Referer")) {
const_cast<RedirectInfo&>(redirect_info).new_referrer.clear();
if (base::Contains(*removed_headers, "X-Brave-Cap-Referer")) {
GURL capped_referrer = URLRequestJob::ComputeReferrerForPolicy(
ReferrerPolicy::REDUCE_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN,
GURL(redirect_info.new_referrer),
redirect_info.new_url);
const_cast<RedirectInfo&>(redirect_info).new_referrer =
capped_referrer.spec();
}
}
}
Expand Down
28 changes: 12 additions & 16 deletions components/brave_shields/browser/brave_shields_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -414,41 +414,37 @@ void DispatchBlockedEvent(const GURL& request_url,
#endif
}

bool ShouldCleanReferrerForTopLevelNavigation(const std::string& method,
const GURL& referrer_url,
const GURL& request_url) {
// See https://github.com/brave/brave-browser/issues/8696
return ((method == "GET" || method == "HEAD") &&
!net::registry_controlled_domains::SameDomainOrHost(
referrer_url, request_url,
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
bool IsSameOriginNavigation(const GURL& referrer, const GURL& target_url) {
const url::Origin original_referrer = url::Origin::Create(referrer);
const url::Origin target_origin = url::Origin::Create(target_url);

return original_referrer.IsSameOriginWith(target_origin);
}

bool MaybeChangeReferrer(
bool allow_referrers,
bool shields_up,
const GURL& current_referrer,
const GURL& target_url,
network::mojom::ReferrerPolicy policy,
Referrer* output_referrer) {
DCHECK(output_referrer);
if (allow_referrers || !shields_up || current_referrer.is_empty()) {
return false;
}

const url::Origin original_referrer = url::Origin::Create(current_referrer);
const url::Origin target_origin = url::Origin::Create(target_url);

if (original_referrer.IsSameOriginWith(target_origin)) {
if (IsSameOriginNavigation(current_referrer, target_url)) {
// Do nothing for same-origin requests. This check also prevents us from
// sending referrer from HTTPS to HTTP.
return false;
}

// Cap referrer to "strict-origin-when-cross-origin" or anything more
// restrictive according do given policy.
// Cap the referrer to "strict-origin-when-cross-origin". More restrictive
// policies should be already applied.
// See https://github.com/brave/brave-browser/issues/13464
*output_referrer = Referrer::SanitizeForRequest(
target_url, Referrer(current_referrer.GetOrigin(), policy));
target_url,
Referrer(current_referrer.GetOrigin(),
network::mojom::ReferrerPolicy::kStrictOriginWhenCrossOrigin));

return true;
}
Expand Down
5 changes: 1 addition & 4 deletions components/brave_shields/browser/brave_shields_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,16 +93,13 @@ void DispatchBlockedEvent(const GURL& request_url,
int frame_tree_node_id,
const std::string& block_type);

bool ShouldCleanReferrerForTopLevelNavigation(const std::string& method,
const GURL& referrer_url,
const GURL& request_url);
bool IsSameOriginNavigation(const GURL& referrer, const GURL& target_url);

bool MaybeChangeReferrer(
bool allow_referrers,
bool shields_up,
const GURL& current_referrer,
const GURL& target_url,
network::mojom::ReferrerPolicy policy,
content::Referrer* output_referrer);


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -622,10 +622,11 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsAgentImplBrowserTest,
link_url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(same_site_url()), link_url().GetOrigin().spec());

// Cross-site navigations get no referrer.
// Cross-site navigations should follow the default referrer policy.
NavigateDirectlyToPageWithLink(cross_site_url());
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), "");
EXPECT_EQ(GetLastReferrer(cross_site_url()), "");
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()),
link_url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(cross_site_url()), link_url().GetOrigin().spec());
}

IN_PROC_BROWSER_TEST_F(BraveContentSettingsAgentImplBrowserTest,
Expand Down Expand Up @@ -655,10 +656,12 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsAgentImplBrowserTest,
url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(cross_site_url()), url().GetOrigin().spec());

// Cross-site navigations get no referrer.
// Cross-site navigations should follow the default referrer policy.
RedirectToPageWithLink(redirect_to_cross_site_url(), cross_site_url());
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), "");
EXPECT_EQ(GetLastReferrer(cross_site_url()), "");
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()),
redirect_to_cross_site_url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(cross_site_url()),
redirect_to_cross_site_url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(redirect_to_cross_site_url()),
link_url().spec());
}
Expand Down Expand Up @@ -696,28 +699,28 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsAgentImplBrowserTest,
url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(cross_site_url()), url().GetOrigin().spec());

// Same-origin navigations get the original page origin as the referrer.
// Same-origin navigations get the original page URL as the referrer.
NavigateDirectlyToPageWithLink(same_origin_url());
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), link_url().spec());
EXPECT_EQ(GetLastReferrer(same_origin_url()), link_url().spec());

// Same-site but cross-origin navigations get the original page origin as the
// referrer.
const std::string expected_referrer = link_url().GetOrigin().spec();
NavigateDirectlyToPageWithLink(same_site_url());
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()),
link_url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(same_site_url()), link_url().GetOrigin().spec());
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), expected_referrer);
EXPECT_EQ(GetLastReferrer(same_site_url()), expected_referrer);

// Cross-site navigations get no referrer.
// Cross-site navigations should follow the default referrer policy.
NavigateDirectlyToPageWithLink(cross_site_url());
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), "");
EXPECT_EQ(GetLastReferrer(cross_site_url()), "");
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), expected_referrer);
EXPECT_EQ(GetLastReferrer(cross_site_url()), expected_referrer);

// Check that a less restrictive policy is not respected.
NavigateDirectlyToPageWithLink(cross_site_url(),
"no-referrer-when-downgrade");
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), "");
EXPECT_EQ(GetLastReferrer(cross_site_url()), "");
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), expected_referrer);
EXPECT_EQ(GetLastReferrer(cross_site_url()), expected_referrer);

// Check that "no-referrer" policy is respected as more restrictive.
NavigateDirectlyToPageWithLink(same_origin_url(), "no-referrer");
Expand All @@ -727,6 +730,11 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsAgentImplBrowserTest,
NavigateDirectlyToPageWithLink(cross_site_url(), "no-referrer");
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), "");
EXPECT_EQ(GetLastReferrer(cross_site_url()), "");

// Check that "same-origin" policy is respected as more restrictive.
NavigateDirectlyToPageWithLink(cross_site_url(), "same-origin");
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), "");
EXPECT_EQ(GetLastReferrer(cross_site_url()), "");
}

IN_PROC_BROWSER_TEST_F(BraveContentSettingsAgentImplBrowserTest,
Expand All @@ -752,10 +760,12 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsAgentImplBrowserTest,
url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(cross_site_url()), url().GetOrigin().spec());

// Cross-site navigations get no referrer.
// Cross-site navigations should follow the default referrer policy.
RedirectToPageWithLink(redirect_to_cross_site_url(), cross_site_url());
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()), "");
EXPECT_EQ(GetLastReferrer(cross_site_url()), "");
EXPECT_EQ(ExecScriptGetStr(kReferrerScript, contents()),
redirect_to_cross_site_url().GetOrigin().spec());
EXPECT_EQ(GetLastReferrer(cross_site_url()),
redirect_to_cross_site_url().GetOrigin().spec());
// Intermidiate same-origin navigation gets full referrer.
EXPECT_EQ(GetLastReferrer(redirect_to_cross_site_url()),
link_url().spec());
Expand Down

0 comments on commit 739093e

Please sign in to comment.