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

Set a limit on cookie expiration #1905

Merged
merged 1 commit into from
Mar 14, 2019
Merged

Set a limit on cookie expiration #1905

merged 1 commit into from
Mar 14, 2019

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Mar 9, 2019

The client-side cookie (i.e. document.cookie API) expiry limit is based off of the limit set by both Safari and Firefox:

https://webkit.org/blog/8613/intelligent-tracking-prevention-2-1/
https://groups.google.com/forum/#!msg/mozilla.dev.platform/lECBPeiGTy4/cPP52vyZAwAJ

whereas the server-side cookie (i.e. Set-Cookie header) limit was picked to avoid interfering in a noticeable way with user logins.

Resolves brave/brave-browser#3443.

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
  • 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:

Use the following test pages and examine the state of the cookie jar using the devtools:

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 9, 2019
@fmarier fmarier self-assigned this Mar 9, 2019
@fmarier fmarier requested a review from bridiver March 9, 2019 03:42
@fmarier
Copy link
Member Author

fmarier commented Mar 9, 2019

@bridiver I forgot to mark this as a draft PR, but basically, at this point, I just want to get feedback on whether or not you think this is a reasonable approach. I will add tests before landing.

@fmarier fmarier removed the request for review from bridiver March 11, 2019 22:32
@fmarier fmarier force-pushed the issue3443 branch 3 times, most recently from cecb60e to f9cc70b Compare March 13, 2019 21:04
The client-side cookie (i.e. document.cookie API) expiry limit is
based off of the limit set by both Safari and Firefox:

https://webkit.org/blog/8613/intelligent-tracking-prevention-2-1/
https://groups.google.com/forum/#!msg/mozilla.dev.platform/lECBPeiGTy4/cPP52vyZAwAJ

whereas the server-side cookie (i.e. Set-Cookie header) limit was
picked to avoid interfering in a noticeable way with user logins.
@fmarier fmarier requested a review from darkdh March 13, 2019 21:10
@fmarier
Copy link
Member Author

fmarier commented Mar 13, 2019

@darkdh I made all of the changes you requested and switched to an override approach.

I also added a unit test which covers the same test cases as my manual test page.

darkdh
darkdh previously approved these changes Mar 13, 2019
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.

++

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.

Limit JS set cookie lifetime to 7 days
2 participants