fix(server): clearCookies({name}) should not transiently delete other cookies#40955
fix(server): clearCookies({name}) should not transiently delete other cookies#40955adityasingh2400 wants to merge 6 commits into
Conversation
… cookies BrowserContext.clearCookies(options) currently wipes every cookie via doClearCookies() and then re-adds the ones that did not match the filter. Pages that subscribe to cookieStore.change observe a transient deletion of the kept cookies during the gap between the wipe and the readd, which is enough to trip route-guards, useSyncExternalStore-style auth state machines, and similar. When a filter (name, domain, or path) is set, expire only the matching cookies in place by calling addCookies with expires=0; the no-filter path still delegates to doClearCookies() as before. No per-browser code is changed. Reported and diagnosed by @jasikpark in microsoft#40953.
|
@microsoft-github-policy-service agree |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@microsoft-github-policy-service agree |
…ned cookies Adds coverage for two cases the new filtered-clearCookies path in microsoft#40955 does not handle: __Secure- prefixed cookies (rejected by Chromium because the expire payload omits secure:true) and partitioned (CHIPS) cookies (silently survive because partitionKey is dropped from the expire payload).
Same root cause as the __Secure- regression: without secure/sameSite in the expire payload, Chromium rejects the cookie with "Invalid cookie fields".
…arCookies Spread the original cookie into the expire payload instead of picking name/domain/path. Without secure/sameSite/partitionKey/_crHasCrossSiteAncestor, Chromium rejects __Secure- and __Host- prefixed cookies with "Invalid cookie fields" and silently leaves partitioned (CHIPS) cookies in place.
|
Code looked good, I found some regressions that I covered with tests + fixed. @yury-s could you take a look since you last touched Cookie around CHIPS? |
|
Thanks for catching those regressions and covering them with tests, much appreciated. Looks good to me. |
| await page.goto(server.PREFIX); | ||
|
|
||
| const eventsHandle = await page.evaluateHandle(() => { | ||
| const events: { kind: string, name: string }[] = []; |
There was a problem hiding this comment.
only delete cookies are changed, no need to store kind
| it.describe('clearCookies with filter preserves cookie identity', () => { | ||
| it.use({ ignoreHTTPSErrors: true }); | ||
|
|
||
| it('should remove __Secure- prefixed cookies by name', async ({ context, httpsServer }) => { |
| expect(await page.evaluate('document.cookie')).toBe('keep_me=1'); | ||
| }); | ||
|
|
||
| it.describe('clearCookies with filter preserves cookie identity', () => { |
There was a problem hiding this comment.
the title doesn't seem to match the tests
| it('should remove partitioned cookies by name', async ({ context, httpsServer, browserName }) => { | ||
| it.skip(browserName !== 'chromium', 'Partitioned cookies (CHIPS) are Chromium-specific'); | ||
| await context.addCookies([ | ||
| { |
There was a problem hiding this comment.
also add another cookie that is preserved?
|
Thanks for the review. Happy to make these: drop storing the kind at line 187 where only deletions change, fix the title at line 204 so it matches what the test actually does, and add a preserved cookie alongside the deleted one at line 232. The star-prefixed tests at line 207 came in through Skn0tt's commits, so I will leave those to whatever you and Skn0tt land on and then push the rest in one go, to avoid clobbering his changes. |
Test results for "MCP"7181 passed, 1113 skipped Merge workflow run. |
Test results for "tests 1"2 flaky43952 passed, 863 skipped Merge workflow run. |
|
@yury-s those tests caught regression in the original change, I can drop them now. Since your feedback is about tests and not impl, I assume you don't see a problem with deleting via |
- Drop *-prefixed tests (__Secure-, __Host-) - Simplify partitioned-cookie test: inline newContext, add a preserved cookie - Simplify event log to plain strings for direct equality assertion
3 participants
Fixes #40953.
BrowserContext.clearCookies(options)currently wipes every cookie viadoClearCookies()and then re-adds the ones that did not match the filter. Pages that subscribe to thecookieStore.changeAPI observe a transient deletion of the kept cookies during the gap between the wipe and the readd, which is enough to trip route-guards,useSyncExternalStore-style auth state machines, and similar listeners. WithcookieStorenow Baseline 2025, this race window is observable from user code.When a filter (
name,domain, orpath) is set, this PR expires only the matching cookies in place by callingaddCookieswithexpires: 0; the no-filter path still delegates todoClearCookies()as before, so no per-browser code is touched.Credit to @jasikpark for the full root-cause analysis and the proposed fix shape in the issue.
Added a Chromium-only test in
tests/library/browsercontext-clearcookies.spec.tsthat adds two cookies, subscribes tocookieStore.changevia the page, then callsclearCookies({ name: 'delete_me' })and asserts the kept cookie never appears in a deletion event.