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.

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 navigation referrers instead of spoofing #2070

Merged
merged 1 commit into from
Mar 26, 2019

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Mar 25, 2019

Fixes brave/brave-browser#3422.

This is the same as #1767, which was reverted in #2049, but with a small fix to the failing test.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) on
    • Windows
    • macOS
    • Linux
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

This can be manually tested using https://fmarier.github.io/brave-testing/referrer-spoofing.html.

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

@fmarier fmarier added this to the 0.64.x - Nightly milestone Mar 25, 2019
@fmarier fmarier self-assigned this Mar 25, 2019
@fmarier fmarier requested a review from iefremov March 25, 2019 23:40

// Same-origin navigations get full referrers.
const GURL kSameOriginRequest("http://document.com/different/path");
referrer = kReferrer;
Copy link
Member Author

Choose a reason for hiding this comment

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

That's the fix for the test failure.


// Special rule for extensions.
const GURL kExtensionUrl("chrome-extension://abc/path?query");
referrer = kReferrer;
Copy link
Member Author

Choose a reason for hiding this comment

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

That will ensure that this test doesn't start failing for obscure reasons in the future if we add another test before it.

@fmarier
Copy link
Member Author

fmarier commented Mar 25, 2019

@iefremov I've identified the two lines I added to this PR compared to what you reviewed in #1767.

@fmarier fmarier merged commit 501f4e0 into brave:master Mar 26, 2019
@fmarier fmarier deleted the issue3422 branch March 26, 2019 16:27
fmarier added a commit that referenced this pull request Apr 16, 2019
Fixes brave/brave-browser#3422.

This is based on the #2070 pull request which
was committed in 501f4e0 and
then reverted in 056ce15 because
of brave/brave-browser#3988.
fmarier added a commit that referenced this pull request Apr 17, 2019
Fixes brave/brave-browser#3422.

This is based on the #2070 pull request which
was committed in 501f4e0 and
then reverted in 056ce15 because
of brave/brave-browser#3988.
fmarier added a commit that referenced this pull request Apr 26, 2019
Fixes brave/brave-browser#3422.

This is based on the #2070 pull request which
was committed in 501f4e0 and
then reverted in 056ce15 because
of brave/brave-browser#3988.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Referrer spoofing could disable login CSRF protections on some sites
2 participants