Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip xorigin top-level navigation referrers instead of spoofing #2260

Merged
merged 2 commits into from
May 9, 2019
Merged

Strip xorigin top-level navigation referrers instead of spoofing #2260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea9ae21
Strip xorigin top-level navigation referrers instead of spoofing
fmarier Feb 22, 2019
737ba14
Fix lint errors
fmarier Apr 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion browser/brave_content_browser_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ void BraveContentBrowserClient::MaybeHideReferrer(
content::BrowserContext* browser_context,
const GURL& request_url,
const GURL& document_url,
bool is_main_frame,
content::Referrer* referrer) {
DCHECK(referrer);
if (document_url.SchemeIs(kChromeExtensionScheme)) {
Expand All @@ -295,9 +296,15 @@ void BraveContentBrowserClient::MaybeHideReferrer(
const bool shields_up = brave_shields::IsAllowContentSettingsForProfile(
profile, document_url, GURL(), CONTENT_SETTINGS_TYPE_PLUGINS,
brave_shields::kBraveShields);
// Top-level navigations get empty referrers (brave/brave-browser#3422).
GURL replacement_referrer_url;
if (!is_main_frame) {
// But iframe navigations get spoofed instead (brave/brave-browser#3988).
replacement_referrer_url = request_url.GetOrigin();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iefremov This is the same as #2070, which you've already reviewed, except for this extra bit. We no longer strip URLs for cross-origin iframe navigations (we continue to spoof instead). This means that the sum total of this change is to strip referrers only on top-level navigations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmarier Fine, I'm ok with the change. Just curious, does it weaken user privacy after all?

}
brave_shields::ShouldSetReferrer(
allow_referrers, shields_up, referrer->url, document_url, request_url,
request_url.GetOrigin(), referrer->policy, referrer);
replacement_referrer_url, referrer->policy, referrer);
}

GURL BraveContentBrowserClient::GetEffectiveURL(
Expand Down
1 change: 1 addition & 0 deletions browser/brave_content_browser_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class BraveContentBrowserClient : public ChromeContentBrowserClient {
void MaybeHideReferrer(content::BrowserContext* browser_context,
const GURL& request_url,
const GURL& document_url,
bool is_main_frame,
content::Referrer* referrer) override;

GURL GetEffectiveURL(content::BrowserContext* browser_context,
Expand Down
31 changes: 27 additions & 4 deletions browser/brave_content_browser_client_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,22 +474,45 @@ IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientReferrerTest,
DefaultBehaviour) {
const GURL kRequestUrl("http://request.com/path?query");
const GURL kDocumentUrl("http://document.com/path?query");
const GURL kSameOriginRequestUrl("http://document.com/different/path");

content::Referrer kReferrer(kDocumentUrl,
network::mojom::ReferrerPolicy::kDefault);

// Should be hidden by default.
// Cross-origin navigations don't get a referrer.
content::Referrer referrer = kReferrer;
client()->MaybeHideReferrer(browser()->profile(),
kRequestUrl, kDocumentUrl,
kRequestUrl, kDocumentUrl, true,
&referrer);
EXPECT_EQ(referrer.url, GURL());

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

// Cross-origin iframe navigations get a spoofed referrer.
referrer = kReferrer;
client()->MaybeHideReferrer(browser()->profile(),
kRequestUrl, kDocumentUrl, false,
&referrer);
EXPECT_EQ(referrer.url, kRequestUrl.GetOrigin());

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

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

Expand All @@ -501,7 +524,7 @@ IN_PROC_BROWSER_TEST_F(BraveContentBrowserClientReferrerTest,
brave_shields::kReferrers, CONTENT_SETTING_ALLOW);
referrer = kReferrer;
client()->MaybeHideReferrer(browser()->profile(),
kRequestUrl, kDocumentUrl,
kRequestUrl, kDocumentUrl, true,
&referrer);
EXPECT_EQ(referrer.url, kDocumentUrl);
}
5 changes: 3 additions & 2 deletions patches/content-browser-frame_host-navigation_request.cc.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index ff845a4963826736b6030d7179a70dcd9d74d241..466cc388c20feafd1cc7ff53accef95e68a97911 100644
index ff845a4963826736b6030d7179a70dcd9d74d241..72860a693c51b25e8dc06c841806d2501bb3e9c0 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -1593,6 +1593,11 @@ void NavigationRequest::OnStartChecksComplete(
@@ -1593,6 +1593,12 @@ void NavigationRequest::OnStartChecksComplete(
headers.MergeFrom(navigation_handle_->TakeModifiedRequestHeaders());
begin_params_->headers = headers.ToString();

+ GetContentClient()->browser()->MaybeHideReferrer(browser_context,
+ common_params_.url,
+ top_document_url,
+ frame_tree_node_->IsMainFrame(),
+ &common_params_.referrer);
+
loader_ = NavigationURLLoader::Create(
Expand Down
7 changes: 4 additions & 3 deletions patches/content-public-browser-content_browser_client.h.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
index 009991aa57e8f543d2fdb7095b2fe2c863373d71..00ab100aa8c17e4614f808ac2c0e2eea709c8243 100644
index 009991aa57e8f543d2fdb7095b2fe2c863373d71..fddf6126c84e95c057301d69d7d297180c618050 100644
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -1516,6 +1516,13 @@ class CONTENT_EXPORT ContentBrowserClient {
@@ -1516,6 +1516,14 @@ class CONTENT_EXPORT ContentBrowserClient {
virtual ui::AXMode GetAXModeForBrowserContext(
BrowserContext* browser_context);

+ // Brave-speicific: allows the embedder to modify the referrer string
+ // Brave-specific: allows the embedder to modify the referrer string
+ // according to user preferences.
+ virtual void MaybeHideReferrer(BrowserContext* browser_context,
+ const GURL& request_url,
+ const GURL& document_url,
+ bool is_main_frame,
+ Referrer* referrer) {}
+
#if defined(OS_ANDROID)
Expand Down
Loading