13464: Unrestrict referrer hiding for top-level navigations. #7591

iefremov · 2021-01-13T21:39:26Z

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

Resolves

Submitter Checklist:

There is a ticket for my issue.
Used Github auto-closing keywords in the commit message.
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed).
Requested a security/privacy review as needed.

Reviewer Checklist:

New files have MPL-2.0 license header.
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Use the following test pages to confirm that all test cases are working as expected:

components/brave_shields/browser/brave_shields_util.cc

fmarier · 2021-01-14T01:08:25Z

I've updated my test pages for this:

fmarier · 2021-01-14T01:11:54Z

chromium_src/net/url_request/redirect_util.cc

-                                            "X-Brave-Clear-Referer")) {
-      const_cast<RedirectInfo&>(redirect_info).new_referrer.clear();
+                                            "X-Brave-Cap-Referer")) {
+      url::Origin referrer = url::Origin::Create(


Does this take into account HTTPS -> HTTP transitions where the referrer needs to be empty?

huh, good catch - should be fixed now

fmarier · 2021-01-14T01:15:21Z

components/content_settings/renderer/brave_content_settings_agent_impl_browsertest.cc

  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());


You could use expected_referrer here.

fmarier · 2021-01-14T01:15:29Z

components/content_settings/renderer/brave_content_settings_agent_impl_browsertest.cc

  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());


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

iefremov · 2021-01-14T21:51:10Z

@mkarolin please take a look, there are only simple changes

mkarolin · 2021-01-15T01:50:40Z

chromium_src/net/url_request/redirect_util.cc

-                                            removed_headers->end(),
-                                            "X-Brave-Clear-Referer")) {
-      const_cast<RedirectInfo&>(redirect_info).new_referrer.clear();
+    if (base::Contains(*removed_headers, "X-Brave-Cap-Referer")) {


Nit: X-Brave-Cap-Referer -> ReferRer? (if so, also in browser/net/brave_site_hacks_network_delegate_helper.cc)

mkarolin

chromium_src changes LGTM

iefremov · 2021-01-15T21:54:55Z

@fmarier PTAL

fmarier

Looks good to me.

I've added a test plan. Please go through it before merging to make sure we didn't forget anything in the automated tests.

iefremov · 2021-01-18T09:20:17Z

Many thanks @fmarier for the test plan - I've verified everyhing. FWIW, we block referrer.js on https://dev-pages.brave.software/referrer/default.html so I had to disable adblocking to do the checks.

iefremov · 2021-01-18T09:21:29Z

@fmarier mind updating https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)#modified-features-and-functionality ?

brave/brave-core#7591 (comment)

fmarier · 2021-01-18T23:55:36Z

FWIW, we block referrer.js on https://dev-pages.brave.software/referrer/default.html so I had to disable adblocking to do the checks.

Thanks for the heads up. Hopefully this will get fixed by brave-experiments/qa-test-pages#1.

mind updating https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)#modified-features-and-functionality ?

Done: https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)/_compare/b1b6ef2a3fb5bc2a5d5d65d3228ccdbc40421a8b...cfc022cc8fe7a01b7d722d5d9423f02f0076b813

kjozwiak · 2021-01-19T02:20:19Z

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.21.10 Chromium: 88.0.4324.89 (Official Build) nightly (64-bit)
-- | --
Revision | 4534be84786f5269fb52e9bf82643af61e2fedaf-refs/branch-heads/4324@{#1721}
OS | Windows 10 OS Version 2009 (Build 19042.746)

https://dev-pages.brave.software/referrer/index.html - PASSED
https://fmarier.github.io/brave-testing/referrer-spoofing.html - PASSED
https://referrer.fmarier.org/redirects.html - PASSED
https://referrer.fmarier.org/samesite.html - PASSED

iefremov requested a review from a team as a code owner January 13, 2021 21:39

iefremov requested review from fmarier and pes10k January 13, 2021 21:39

pes10k reviewed Jan 13, 2021

View reviewed changes

components/brave_shields/browser/brave_shields_util.cc Show resolved Hide resolved

fmarier reviewed Jan 14, 2021

View reviewed changes

iefremov force-pushed the ie_referrer_cap branch from e2e9022 to 15fd25a Compare January 14, 2021 21:28

13464: Unrestrict referrer hiding for top-level navigations.

739093e

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

iefremov force-pushed the ie_referrer_cap branch from 15fd25a to 739093e Compare January 14, 2021 21:44

mkarolin reviewed Jan 15, 2021

View reviewed changes

mkarolin approved these changes Jan 15, 2021

View reviewed changes

13464: Whitespace and typo fix.

69438d8

fmarier approved these changes Jan 15, 2021

View reviewed changes

iefremov merged commit 6999e42 into master Jan 18, 2021

iefremov deleted the ie_referrer_cap branch January 18, 2021 09:18

brave-builds pushed a commit that referenced this pull request Jan 18, 2021

Uplift of #7591 (squashed) to beta

97f100f

brave-builds pushed a commit that referenced this pull request Jan 18, 2021

Uplift of #7591 (squashed) to release

4d51b2c

This was referenced Jan 18, 2021

13464: Unrestrict referrer hiding for top-level navigations. (uplift to 1.20.x) #7638

Merged

13464: Unrestrict referrer hiding for top-level navigations. (uplift to 1.19.x) #7639

Merged

iefremov self-assigned this Jan 18, 2021

iefremov modified the milestones: 1.19.x - Release, 1.21.x - Nightly Jan 18, 2021

fmarier added a commit to fmarier/qa-test-pages that referenced this pull request Jan 18, 2021

According to @iefremov referrer.js gets blocked by Shields.

43498a6

brave/brave-core#7591 (comment)

fmarier mentioned this pull request Jan 18, 2021

According to @iefremov referrer.js gets blocked by Shields. brave-experiments/qa-test-pages#1

Merged

GeetaSarvadnya mentioned this pull request Jan 19, 2021

Strip referer to strict-origin-when-cross-origin in all cases brave/brave-browser#13464

Closed

13464: Unrestrict referrer hiding for top-level navigations. #7591

13464: Unrestrict referrer hiding for top-level navigations. #7591

Uh oh!

Conversation

iefremov commented Jan 13, 2021 • edited by fmarier Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Uh oh!

Uh oh!

fmarier commented Jan 14, 2021

Uh oh!

fmarier Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

iefremov Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

fmarier Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

iefremov Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

fmarier Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

iefremov commented Jan 14, 2021

Uh oh!

mkarolin Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

mkarolin left a comment

Choose a reason for hiding this comment

Uh oh!

iefremov commented Jan 15, 2021

Uh oh!

fmarier left a comment

Choose a reason for hiding this comment

Uh oh!

iefremov commented Jan 18, 2021

Uh oh!

iefremov commented Jan 18, 2021

Uh oh!

fmarier commented Jan 18, 2021

Uh oh!

kjozwiak commented Jan 19, 2021

Uh oh!

Uh oh!

iefremov commented Jan 13, 2021 •

edited by fmarier

Loading