-
Notifications
You must be signed in to change notification settings - Fork 893
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 expiry fixes #14852
Cookie expiry fixes #14852
Conversation
5195e33
to
5d8ec11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security review not needed since this is maintaining the same policy as before, just fixing a regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, did you forget to git add
the browsertest you added?
Moved the test and didn't add it again 😅 |
@@ -14,6 +14,8 @@ | |||
|
|||
#define RemoveChangeListener \ | |||
NotUsed() const {} \ | |||
base::Time ModifyExpiration(const base::Time& expiry_date, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no need to make this a method on the class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong feelings on my end, just that it seemed cleanest to keep this method together with the existing ones?
a738534
to
98d31d2
Compare
do a rebase please before merging. |
Max expiry across all cookies (JS, HTTP) is 6 months
Move test Minor test fixes Change macro name
Add less-than-max tests for JS + HTTP cookie expiry Set HTTP cookie via header
8b91fd3
to
e4ce029
Compare
Resolves brave/brave-browser#15048
document.cookie
and Cookie Store APISubmitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Same test plan as #1905 (comment)