8696: Change referrer blocking in Brave #5613

iefremov · 2020-05-21T15:14:36Z

Resolves brave/brave-browser#8696

Submitter Checklist:

Test Plan:

Manual test pages:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

iefremov · 2020-05-21T15:16:59Z

app/brave_main_delegate_browsertest.cc

@@ -61,6 +61,8 @@ IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, EnabledFeatures) {
  const base::Feature* enabled_features[] = {
    &omnibox::kSimplifyHttpsIndicator,
    &password_manager::features::kPasswordImport,
+    &features::kReducedReferrerGranularity,


This switches us to strict-origin-when-cross-origin

iefremov · 2020-05-21T15:18:13Z

app/brave_main_delegate_browsertest.cc

@@ -61,6 +61,8 @@ IN_PROC_BROWSER_TEST_F(BraveMainDelegateBrowserTest, EnabledFeatures) {
  const base::Feature* enabled_features[] = {
    &omnibox::kSimplifyHttpsIndicator,
    &password_manager::features::kPasswordImport,
+    &features::kReducedReferrerGranularity,
+    &network::features::kCapReferrerToOriginOnCrossOrigin,


This satisfies the following

Requested policy of no-referrer, same-origin, or strict-origin? If yes, change effective policy to the requested policy.

iefremov · 2020-05-21T15:43:40Z

renderer/brave_content_settings_agent_impl_browsertest.cc

-  EXPECT_EQ(ExecScriptGetStr(create_image_script(), contents()),
-            image_url().spec());
-  EXPECT_EQ(GetLastReferrer(image_url()), iframe_url().GetOrigin().spec());
+  // Same-site sub-resources within the page get the page origin as referrer.


@fmarier There is some confusion, since the the spec defines "same-site" as equal domains. This way, navigations to subdomains are not same-site, so we probably should rewrite these comments.

Do you mean same-origin?

Because I'm using "same-site" here as defined in the Fetch spec since our policy is specifically looser (for webcompat reasons).

yup, sorry, you are correct!

fmarier

One of the side effects of relaxing the restrictions and leaning more heavily on the built-in referrer policy code is that we're actually become more strict in some cases because for sub-resources, we're moving away from "same-site" comparisons toward "same-origin" comparisons.

Therefore, I think it makes sense to change the existing "same-site" sub-resource tests to be all based on "same-origin". There's no point in testing the "same-site" case if it's the same as the "cross-origin" case.

For navigations, there is a difference and we should ideally test all three possible cases:

same-origin navigations: full referrer
same-site navigations: origin only
cross-origin navigations: no referrer

fmarier · 2020-05-27T01:54:05Z

browser/brave_content_browser_client_browsertest.cc

  referrer = kReferrer.Clone();
  client()->MaybeHideReferrer(browser()->profile(),
                              kRequestUrl, kDocumentUrl, false,
                              &referrer);
-  EXPECT_EQ(referrer->url, kRequestUrl.GetOrigin());
+  EXPECT_EQ(referrer->url, kDocumentUrl);


I think it should be the origin only in this case, no?

iframes are not handled by this method, this is only needed to fix document.referrer in main frames

I'm confused by this test case. Is the comment wrong? Is this not a cross-origin iframe navigation?

If it is, then I don't think it should be getting the full referrer info like the same-origin top-level navigation test case just before it.

Is this DefaultBehaviour test not testing the default Brave behaviour?

fmarier · 2020-05-27T02:00:57Z

renderer/brave_content_settings_agent_impl_browsertest.cc

-  EXPECT_EQ(GetLastReferrer(same_site_url()), url().spec());
+  EXPECT_EQ(ExecScriptGetStr(kReferrerScript, child_frame()),
+                             url().GetOrigin().spec());
+  EXPECT_EQ(GetLastReferrer(same_site_url()), url().GetOrigin().spec());

  // Cross-site iframe navigations get their referrer spoofed.


Not spoofed anymore.

fmarier · 2020-05-27T02:02:50Z

renderer/brave_content_settings_agent_impl_browsertest.cc

  EXPECT_EQ(
      ExecScriptGetStr(create_image_script(cross_site_image_url()), contents()),
      cross_site_image_url().spec());
-  EXPECT_EQ(GetLastReferrer(cross_site_image_url()), url().spec());
+  EXPECT_EQ(GetLastReferrer(cross_site_image_url()), url().GetOrigin().spec());

  // A cross-origin iframe navigation gets the URL of the first one as


"gets the ORIGIN of the first one"

fmarier · 2020-05-27T02:13:52Z

One of the key differences between our implementation and what Chrome is planning on shipping is that we not only default to strict-origin-when-cross-origin, but we also ignore pages that request a looser policy.

We should add some browsertests to make sure that a referrer policy specified by the page cannot override ours.

Also, these two tests will need to be updated for the new behavior and ideally enabled (see brave/brave-browser#7933):

DISABLED_BlockReferrerByDefaultRedirects
DISABLED_BlockReferrerRedirects

fmarier · 2020-05-27T02:14:23Z

Does the implementation also cover this case?

Requested policy of origin?
If yes, change effective policy to strict-origin

iefremov · 2020-05-28T17:19:27Z

I think I've found a solution for proper referrer masking during redirects, will push it here soon (instead of a separate PR).

iefremov · 2020-06-02T13:00:10Z

@fmarier I've updated the existing case and I think addressed #5613 (review)

iefremov · 2020-06-02T13:02:51Z

I've decided to postpone fixing redirects, since it will take some time and can be done as a follow-up.

We should add some browsertests to make sure that a referrer policy specified by the page cannot override ours.

I think I can port some existing chromium tests in the next commit

Previously we used to spoof referrers for iframe and subresource fetches and this has been causing some webcompat problems. This PR removes this machinery, makes 'strict-origin-when-cross-origin' the default policy, prevents this policy from downgrading and keeps referrer removal for top-level navigations.

I had to restore referrer blocking in site hacks, because it is needed for subresource queries (this does not help us with redirects though).

bridiver · 2020-06-13T14:06:21Z

components/brave_shields/browser/brave_shields_util.cc

    return false;
  }
+
+  // TODO(iefremov): Get rid of the whitelist once our webcompat is OK.


I guess that will also get rid of the layering violation for g_brave_browser_process so I won't worry about it here

concerns addressed and discussed in DM

iefremov · 2020-06-15T08:51:17Z

CI failure on MacOS seems unrelated, merging

iefremov requested review from jumde and pes10k May 21, 2020 15:14

iefremov commented May 21, 2020

View reviewed changes

iefremov requested a review from bbondy May 21, 2020 15:23

iefremov commented May 21, 2020

View reviewed changes

iefremov force-pushed the ie_referrer_ftw branch from b09c19c to 2ab743c Compare May 22, 2020 08:42

fmarier self-requested a review May 26, 2020 01:29

bbondy requested a review from bridiver May 26, 2020 18:08

fmarier previously requested changes May 27, 2020

View reviewed changes

iefremov force-pushed the ie_referrer_ftw branch from 2ab743c to 3f5a6ec Compare May 27, 2020 07:23

iefremov force-pushed the ie_referrer_ftw branch from cab334f to 76c8361 Compare June 2, 2020 13:01

pes10k mentioned this pull request Jun 3, 2020

Remove not currently used lists from local dat file brave/brave-core-crx-packager#130

Closed

iefremov force-pushed the ie_referrer_ftw branch from c07e5e6 to a2a2f6c Compare June 8, 2020 19:38

iefremov added 4 commits June 12, 2020 23:12

8696: Update and imporve tests.

2e4a483

8696: Properly tie referrer blocking to shields.

78ac2f9

I had to restore referrer blocking in site hacks, because it is needed for subresource queries (this does not help us with redirects though).

8696: Improve referrer hiding tests.

f7b1051

iefremov force-pushed the ie_referrer_ftw branch from f32bc3c to f7b1051 Compare June 12, 2020 16:13

iefremov requested a review from fmarier June 12, 2020 16:13

bridiver reviewed Jun 13, 2020

View reviewed changes

bridiver approved these changes Jun 13, 2020

View reviewed changes

8696: Pleasing linter.

2dcdfcc

iefremov added this to the 1.12.x - Nightly milestone Jun 15, 2020

iefremov self-assigned this Jun 15, 2020

iefremov merged commit b93dc48 into master Jun 15, 2020

iefremov deleted the ie_referrer_ftw branch June 15, 2020 08:51

fmarier mentioned this pull request Jun 15, 2020

[Desktop] Can't sign in with firebase website with shields up brave/brave-browser#5075

Closed

bsclifton mentioned this pull request Jun 18, 2020

Site-specific cookies setting does not override default for (Google API) HTTP referrer brave/brave-browser#6657

Closed

LaurenWags mentioned this pull request Aug 6, 2020

Change sub-resource referrer policy to report 3p origin brave/brave-browser#8696

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8696: Change referrer blocking in Brave #5613

8696: Change referrer blocking in Brave #5613

iefremov commented May 21, 2020 •

edited by fmarier

Loading

iefremov May 21, 2020

iefremov May 21, 2020

iefremov May 21, 2020

fmarier May 21, 2020

iefremov May 21, 2020

fmarier left a comment

fmarier May 27, 2020

iefremov Jun 2, 2020

fmarier Jun 2, 2020 •

edited

Loading

fmarier May 27, 2020

iefremov Jun 2, 2020

fmarier May 27, 2020

iefremov Jun 2, 2020

fmarier commented May 27, 2020

fmarier commented May 27, 2020

iefremov commented May 28, 2020

iefremov commented Jun 2, 2020

iefremov commented Jun 2, 2020

bridiver Jun 13, 2020

iefremov commented Jun 15, 2020

8696: Change referrer blocking in Brave #5613

8696: Change referrer blocking in Brave #5613

Conversation

iefremov commented May 21, 2020 • edited by fmarier Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarier Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarier commented May 27, 2020

fmarier commented May 27, 2020

iefremov commented May 28, 2020

iefremov commented Jun 2, 2020

iefremov commented Jun 2, 2020

Choose a reason for hiding this comment

iefremov commented Jun 15, 2020

iefremov commented May 21, 2020 •

edited by fmarier

Loading

fmarier Jun 2, 2020 •

edited

Loading