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

Issue 7044: Do not import cookies from Chrome and Firefox. #4121

Merged
merged 3 commits into from
Dec 13, 2019

Conversation

iefremov
Copy link
Contributor

@iefremov iefremov commented Dec 3, 2019

Note: we do not import cookies from Safari, so nothing to disable.

Fix brave/brave-browser#7044

Submitter Checklist:

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

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 iefremov requested a review from bridiver as a code owner December 3, 2019 13:50
@iefremov iefremov self-assigned this Dec 3, 2019
@iefremov iefremov requested review from bbondy, darkdh and fmarier December 3, 2019 13:52
@iefremov iefremov force-pushed the ie_dont_import_cookies branch from bbde210 to 7fc1130 Compare December 3, 2019 13:54
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

we also need to remove cookie option from importer_list.cc which means patch for Firefox and cookies detection in ChromeImporterCanImport for Chrome to prevent option shows up in import dialog

@iefremov iefremov force-pushed the ie_dont_import_cookies branch from 7fc1130 to b0bd76c Compare December 3, 2019 14:33
@iefremov
Copy link
Contributor Author

iefremov commented Dec 3, 2019

@darkdh thanks, fixed

@iefremov iefremov requested a review from darkdh December 4, 2019 14:06
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

lgtm

@iefremov
Copy link
Contributor Author

iefremov commented Dec 4, 2019

@bridiver please take a look

@iefremov
Copy link
Contributor Author

iefremov commented Dec 9, 2019

Agreed with @tomlowenthal to do not touch BraveImporter in this PR

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

Changes look good, just checking to make sure we are ok with dropping all cookie import vs just dropping 3rd-party cookie import

@fmarier
Copy link
Member

fmarier commented Dec 9, 2019

Just to confirm, we are talking about dropping all cookies (first and third-party).

This also came up at the Initiatives meeting where Brendan and David were of the opinion that we should just remove that functionality entirely (as opposed to disabling it by default and letting users enable it if they want) to avoid the privacy and compatibility problems we have seen with these.

@bsclifton
Copy link
Member

@fmarier correct- we want to remove the ability to import cookies completely 👍 (per @BrendanEich, don't even show the checkbox during manual import)

@bsclifton
Copy link
Member

Will try to review soon! Important to try both manual import and also import via the brave://welcome wizard

@bsclifton
Copy link
Member

bsclifton commented Dec 9, 2019

@iefremov we got the go-ahead to remove Muon import, if you wanted to do that also 👍

Either here or in a separate PR (I'd be OK including with this one, as a distinct commit)

@bsclifton bsclifton force-pushed the ie_dont_import_cookies branch from b0bd76c to b0b67e1 Compare December 10, 2019 16:43
Note: we do not import cookies from Safari, so nothing to disable.
@bsclifton bsclifton force-pushed the ie_dont_import_cookies branch from b0b67e1 to 0b1a2da Compare December 11, 2019 04:55
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Works great! Tested with both Chrome and Firefox... also tested regular import case (from settings) and then import from Wizard. Everything worked as expected 😄

@bsclifton
Copy link
Member

@iefremov only one problem: getting unit test failure for:

1 test failed:
    ChromeImporterTest.ImportCookies (../../brave/utility/importer/chrome_importer_unittest.cc:173)

I think we can delete that, push latest, and watch CI 😄

@iefremov
Copy link
Contributor Author

@bsclifton I decided to land BraveImporter removal as a follow-up, since there are too many changes involving removing patches, etc.

@bsclifton
Copy link
Member

All builds + tests passed; just one lint error. Gonna fix that up and merge 😄

@bsclifton bsclifton merged commit c9f6bee into master Dec 13, 2019
@bsclifton bsclifton deleted the ie_dont_import_cookies branch December 13, 2019 06:10
brave-builds pushed a commit that referenced this pull request Dec 13, 2019
brave-builds pushed a commit that referenced this pull request Dec 13, 2019
brave-builds pushed a commit that referenced this pull request Dec 13, 2019
bsclifton added a commit that referenced this pull request Dec 16, 2019
Issue 7044: Do not import cookies from Chrome and Firefox.
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.

Chrome/Firefox importers should not import cookies
5 participants