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

Cookie exception to unbreak Google logins #1748

Closed
wants to merge 1 commit into from

Conversation

mrzealot
Copy link

@mrzealot mrzealot commented Feb 21, 2019

Fixes brave/brave-browser#3534.

This PR wires in the already existing cookie exception checking method to the cookie blocking workflow, adds capability for 1st-party-independent exceptions (e.g., specific iframe hosts that should be allowed to access cookies no matter the hosting site), and adds an exception to https://accounts.google.com/o/oauth2/* to allow iframe-based Google login integration on many sites, as per our slack discussions.

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) on
    • Windows
    • macOS
    • Linux
  • 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:

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

@mrzealot mrzealot self-assigned this Feb 21, 2019
@mrzealot mrzealot changed the title Dban webcompat google Cookie exception to unbreak Google logins Feb 21, 2019
@diracdeltas
Copy link
Member

Please link in the PR to the issue this is fixing (creating an issue if one doesn't exist already) and squash commits into one commit

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

I would suggest adding two test cases in common/shield_exceptions_unittest.cc:

  • A positive test case:

    EXPECT_TRUE(IsWhitelistedCookieException(GURL("https://www.airbnb.com/"),
                                             GURL("<actual Google URL used AirBnB uses minus query string>")));
    
  • A negative test case:

    EXPECT_FALSE(IsWhitelistedCookieException(GURL("https://www.mozilla.org/"),
                                              GURL("https://www.googletagmanager.com/gtm.js")));
    

fmarier
fmarier previously approved these changes Feb 28, 2019
Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

The commit message (and this PR as @diracdeltas pointed out) is missing a link to the issue that it is fixing.

Other than that, it looks good to me.

@mrzealot
Copy link
Author

Oh sorry, I wasn't familiar with the formatting expectations and thought the above reference to brave/brave-browser#3115 was enough. Made a more generic issue to link this to, referenced it in the description, and also re-pushed the changes with a more appropriate message.

@fmarier
Copy link
Member

fmarier commented Mar 1, 2019

In the commit messages, the syntax also requires brave/brave-browser#1234 since these issues are not in the brave/brave-core repo.

@bbondy
Copy link
Member

bbondy commented Apr 18, 2019

This was added here ec009bb as part of an option that can be disabled. Closing this PR.

@bbondy bbondy closed this Apr 18, 2019
@bsclifton bsclifton deleted the dban_webcompat_google branch April 13, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some Google login integrations do not work
4 participants