Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Exclude cookies from google.com domain #12171

Merged
merged 1 commit into from
Feb 8, 2018
Merged

Exclude cookies from google.com domain #12171

merged 1 commit into from
Feb 8, 2018

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Dec 4, 2017

fix #11401

Auditors: @bsclifton, @bbondy

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).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

  1. .google.com cookie mismatch after import #11401 (comment)

  2. Redo exception to cover Chrome's case #11647 (comment)

  3. .google.com cookie mismatch after import #11401 (comment)

4.a Clear cookies on FF and Chrome, login to gmail account 1 and account 2
4.b Close FF and Chrome, Open Brave (clean profile)
4.c Import cookies only from FF and Chrome
4.d Navigate to mail.google.com --> you have to enter password for
account 1 and account 2 (but no 2FA)
4.e Import cookies from FF and Chrome again, refresh gmail page
(you will have to enter password again to login both accounts)
4.f Logout and you will see login page with both accounts loggout
(no cookie mismatch error)

  1. unit test
    npm run unittest -- --grep="shouldSkipCookie"

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@darkdh darkdh self-assigned this Dec 4, 2017
@darkdh darkdh requested review from bbondy and bsclifton December 4, 2017 22:12
@codecov-io
Copy link

codecov-io commented Dec 4, 2017

Codecov Report

Merging #12171 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #12171      +/-   ##
==========================================
- Coverage   56.21%   56.21%   -0.01%     
==========================================
  Files         280      280              
  Lines       27519    27521       +2     
  Branches     4486     4486              
==========================================
+ Hits        15470    15471       +1     
- Misses      12049    12050       +1
Flag Coverage Δ
#unittest 56.21% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
app/importer.js 37.11% <100%> (+0.13%) ⬆️

@bsclifton bsclifton modified the milestones: 0.21.x (Developer Channel), 0.20.x Hotfix 1 Jan 8, 2018
@alexwykoff alexwykoff modified the milestones: 0.20.x Hotfix 3 (Ledger improvments), 0.21.x (Beta Channel) Feb 6, 2018
fix #11401

Auditors: @bsclifton, @bbondy

Test Plan:
1. #11401 (comment)
2. #11647 (comment)
3. #11401 (comment)

4.
4.a Clear cookies on FF and Chrome, login to gmail account 1 and account 2
4.b Close FF and Chrome, Open Brave (clean profile)
4.c Import cookies only from FF and Chrome
4.d Navigate to mail.google.com --> you have to enter password for
account 1 and account 2 (but no 2FA)
4.e Import cookies from FF and Chrome again, refresh gmail page
(you will have to enter password again to login both accounts)
4.f Logout and you will see login page with both accounts loggout
(no cookie mismatch error)

5. unit test
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! 😄 👍 I did get prompted for 2FA after import, but I wonder if that's because I clicked Deny on the keychain prompt that was shown?

@bsclifton bsclifton merged commit 59e2b83 into master Feb 8, 2018
@bsclifton bsclifton deleted the issue-11401 branch February 8, 2018 21:43
bsclifton added a commit that referenced this pull request Feb 8, 2018
Exclude cookies from google.com domain
bsclifton added a commit that referenced this pull request Feb 8, 2018
Exclude cookies from google.com domain
@bsclifton
Copy link
Member

master 59e2b83
0.22.x 7c5ceba
0.21.x a220321

@darkdh
Copy link
Member Author

darkdh commented Feb 8, 2018

yeah, it must be that because you can't read the encrypted cookie value

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.google.com cookie mismatch after import
4 participants