feat(protocol): add write_cookies, the one verb that can repair a rotated session - #211
Conversation
github-actions
Bot
added
the
auto-review
|
Claude finished @chrischall's task in 5m 24s —— View job Review complete — PR #211
|
…ated session Every verb until now reads. That leaves a failure class consuming MCPs cannot fix: sites that ROTATE a credential cookie hand back a new value on every refresh, so when an MCP refreshes and keeps the result to itself, the copy in the browser's cookie jar is dead. The user is signed out of a tab they never touched and told it was "inactivity". Measured on creditkarma.com: two MCP-side refreshes were enough to log out an untouched, freshly signed-in tab, and reloading does not recover it — only a full sign-in does (chrischall/creditkarma-mcp#119). That MCP shipped the only mitigation available to it (refresh only when the token is genuinely spent) and it still cannot help when the browser has been idle past the access token's life, which is the scheduled-sync case. `writeCookies()` overwrites the value of cookies the MCP already declares. Four independent constraints, all enforced extension-side: - capability: `write_cookies`, approved at pair time as its own line and stored in the trust record, so adding it later forces a re-pair - read scope: every name must already be in declared `cookieKeys`, so a write can never reach a cookie the user did not approve reading - domain: origin gated against declared `domains`, decided on the bare origin before any path, exactly as the read path does - existence: the cookie must already exist — this refreshes a value in place and cannot author cookies, which is what stops it from being a cookie-injection primitive Reusing `cookieKeys` rather than adding a separate writable-key list is deliberate: it keeps the blast radius identical to the read scope the user already saw, at the cost of not being able to grant write on a cookie you cannot read. That seemed the right trade for a first write verb; a separate list can be added later without a wire break. Attributes are copied off the cookie being replaced. Passing only {url,name,value} looks like it works and quietly creates a SECOND cookie — Chrome derives a host-only, non-HttpOnly, default-path cookie from the URL, which does not replace a Domain=-scoped or HttpOnly original, and the site keeps reading the stale one. `domain` is omitted for host-only cookies and `expirationDate` for session cookies, since passing either would widen scope or lifetime. The popup labels it "Overwrite cookies it can already read (can change your signed-in session)" rather than as a sibling of the read verbs. docs/SECURITY.md gains §T-cookie-write with the residual risk. Closes #202 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo
3de01ba to
c87808a
Compare
Deferred: this has never touched a real cookie jarRecording this on the PR rather than leaving it in a chat log, because it is the one gap the test suite structurally cannot close. Every test here mocks Agreed to verify post-deployment. What to check, once the rebuilt extension is loaded from
(1) is the one that matters most: it is the difference between this working and appearing to work. Rebased onto 🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export interface InnerRequestWriteCookies { | ||
| type: 'request'; | ||
| id: number; | ||
| op: 'write_cookies'; | ||
| init: WriteCookiesInit; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 write_cookies requests are rejected at the wire boundary — the verb can never reach the extension.
packages/protocol/src/validate.ts was not touched by this PR:
$ git diff HEAD~1 --name-only
docs/SECURITY.md
packages/cli/src/args.ts
packages/cli/src/main.ts
packages/cli/src/profiles.ts
packages/cli/src/server-opts.ts
packages/cli/tests/args.test.ts
packages/extension-core/src/background.ts
packages/extension-core/src/popup/popup.ts
packages/extension-core/tests/write-cookies.test.ts
packages/protocol/src/frames.ts
packages/server/src/ws-server.ts
packages/server/tests/write-cookies.test.ts
and it contains zero occurrences of the new op (Grep for write_cookies in packages/protocol/src/validate.ts → No matches found).
validateInnerRequest switches on raw.op and falls through to a terminal throw for anything unrecognised — validate.ts:959:
inner.op: must be one of "fetch", "read_cookies", "read_local_storage", "read_session_storage", "capture_request_header", "capture_redirect", "read_indexed_db", "read_dom", "download", "graphql_query"; got ...
The receive path is not optional: sealInnerFrame does not validate, but openEncryptedFrameDetailed calls validateInnerFrame (seal.ts:118), and the extension decodes every inbound frame through openEncryptedFrame (background.ts:1344). The throw lands in that function's catch:
} catch (e) {
console.warn('[fetchproxy] decrypt failed:', e);
return;
}So a write_cookies request is silently dropped — no response frame at all — and FetchproxyServer.writeCookies() hangs until _withVerbTimeout fires. handleWriteCookiesRequest is unreachable in production.
The new server tests don't catch this because they stub the transport (packages/server/tests/write-cookies.test.ts:36 mocks sendInnerFrame), and the extension tests call resolveWriteCookiesRequest directly rather than going through the frame decode.
This is also the one thing CLAUDE.md calls out by name:
Don't merge feature work that adds protocol fields without updating
packages/protocol/src/validate.tsvalidators (every inbound frame is validated before dispatch).
Needs a raw.op === 'write_cookies' branch asserting init.origin is an HTTPS origin, init.path via assertCookiePath when present, init.cookies is a non-empty array of {name: string, value: string}, and rejecting unexpected init fields — matching the read_cookies branch's shape. Worth a round-trip test through sealInnerFrame/openEncryptedFrame so the gap can't reopen.
| export interface InnerResponseWriteCookiesOk { | ||
| type: 'response'; | ||
| id: number; | ||
| ok: true; | ||
| op: 'write_cookies'; | ||
| /** Names actually written, echoed so the caller can confirm rather than | ||
| * assume. Same order as requested. */ | ||
| written: string[]; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 The success response has no validator branch either — and this one tears down the concentrator's extension socket.
validateInnerResponse's ok === true path discriminates on op and ends in a terminal throw (validate.ts:1124):
throw new ProtocolError(
`inner.op: unknown success-response op ${JSON.stringify(raw.op)}`,
);There's no write_cookies case, so {ok: true, op: 'write_cookies', written: [...]} — the frame handleWriteCookiesRequest sends on success (background.ts:1929-1935) — fails validation on the MCP side.
Note the asymmetry: the failure path is already fine, because KNOWN_RESPONSE_OPS is built from KNOWN_CAPABILITIES (validate.ts:1007-1010) and this PR added write_cookies there. So every gate rejection round-trips correctly and only a successful write hits the throw — which is the worse ordering for noticing it.
The blast radius is the one the KNOWN_RESPONSE_OPS doc comment was written about:
Without this, every
ok:false, op:'graphql_query'response … fails validation here, which — via host.ts's message-handler catch-all — closes the extension WebSocket for every MCP on the concentrator.
Confirmed at packages/server/src/host.ts:342-351: the throw out of openEncryptedFrame (line 320) is caught by the message handler's catch-all, which does ws.close(1011, 'internal error') on the extension socket — dropping every other MCP multiplexed through the host, not just the one that wrote a cookie.
Same fix location as the request-side comment above; written wants a string[] assertion.
| await fail( | ||
| `cookies not present to overwrite: ${missing.join(', ')} — write_cookies refreshes an ` + | ||
| 'existing value, it does not create cookies', | ||
| ); | ||
| return; | ||
| } | ||
| const written: string[] = []; | ||
| for (const { decl, existing } of targets) { | ||
| try { |
There was a problem hiding this comment.
🟡 Nit — written reports success purely on "the promise didn't reject", never on the resolved value. The declared type is Promise<unknown> and the result is discarded.
Given this verb's entire thesis is that a silently stale cookie is the failure mode worth adding a write verb for, it seems worth checking the resolution too — Chrome's cookies.set contract is documented as yielding null when the set fails. A const set = await chrome.cookies.set(...); if (!set) { await fail(...); return; } (with the type narrowed to Promise<ChromeCookie | null>) would keep a no-op from being reported back as a completed rotation.
I couldn't execute this against a real Chrome, so treat the behavioural half as an open question rather than a confirmed defect — but the defensive check costs nothing either way.
Review —
|
|
🟡 Auto-review verdict: warn — The round-3 blocker is fixed — |
|
Claude finished @chrischall's task in 5m 32s —— View job Review complete — PR #211
Verdict: fail — one blocking finding. 🔴 🟡 Four nits, all inline: partial writes possible when The design itself is good — the four-constraint model, the No files were modified. |
| * cannot reach a cookie the MCP was not already trusted to read, which keeps | ||
| * the blast radius identical to the read scope the user already saw. | ||
| */ | ||
| export interface WriteCookiesInit { |
There was a problem hiding this comment.
🔴 write_cookies was added to frames.ts but never to validate.ts — the verb cannot work on the wire, and its success response would tear down the whole bridge.
CLAUDE.md, What to not do:
Don't merge feature work that adds protocol fields without updating
packages/protocol/src/validate.tsvalidators (every inbound frame is validated before dispatch).
write_cookies appears nowhere in that file:
$ grep -n "write_cookies" packages/protocol/src/validate.ts packages/protocol/tests/*.ts
(no output)
Two concrete consequences, both on the real path:
1. The request never reaches the handler. validateInnerRequest has no write_cookies branch, so it falls through to the unconditional throw at validate.ts:959:
throw new ProtocolError(
`inner.op: must be one of "fetch", "read_cookies", ... "graphql_query"; got ${JSON.stringify(raw.op)}`,
);Every inner frame goes through it — seal.ts:118 calls validateInnerFrame(parsed) inside openEncryptedFrame, and the extension opens frames at background.ts:1344. On throw, onEncryptedFrame logs and returns without sending a response:
} catch (e) {
console.warn('[fetchproxy] decrypt failed:', e);
return;
}So handleWriteCookiesRequest is unreachable in production, and the MCP-side writeCookies() promise hangs until _withVerbTimeout fires — surfacing as a bridge timeout, not as "unsupported op".
2. Even after fixing (1), the success response kills the socket for every MCP. validateInnerResponse ends at validate.ts:1124:
throw new ProtocolError(`inner.op: unknown success-response op ${JSON.stringify(raw.op)}`);and host.ts:344-352 catches any throw from openEncryptedFrame and closes the socket — the exact failure mode already documented in the KNOWN_RESPONSE_OPS comment at validate.ts:995-1006 ("closes the extension WebSocket for every MCP on the concentrator"). Note the ok:false path is fine, since KNOWN_RESPONSE_OPS spreads KNOWN_CAPABILITIES and this PR did add write_cookies there — which is why the gate-refusal tests would still look healthy.
The existing tests don't cover this: packages/server/tests/write-cookies.test.ts:31 mocks sendInnerFrame, so nothing in the new suite crosses the seal/validate boundary.
Needs a raw.op === 'write_cookies' branch in validateInnerRequest (assert origin via assertHttpsOriginOnly, optional path via assertCookiePath, a non-empty cookies array of {name, value} string pairs, and reject unexpected init fields like the read_cookies branch does) plus an op === 'write_cookies' branch in validateInnerResponse asserting written is a string array.
| const written: string[] = []; | ||
| for (const { decl, existing } of targets) { | ||
| try { | ||
| await chrome.cookies.set(cookieSetDetailsFor(url, decl, existing)); | ||
| written.push(decl.name); | ||
| } catch (e) { | ||
| await fail(`failed to write cookie ${decl.name}: ${String(e)}`); | ||
| return; | ||
| } | ||
| } | ||
| await sendInner(mcpId, { | ||
| type: 'response', |
There was a problem hiding this comment.
🟡 A chrome.cookies.set failure part-way through this loop does apply part of the request, which is the one outcome the doc block above (background.ts:1849-1856) says is refused:
Three gates, in order, all refusing the WHOLE request rather than applying part of it — a half-written rotation is worse than a failed one
The three gates hold that promise, but the write loop doesn't: with {SESSION, REFRESH}, if SESSION succeeds and REFRESH throws, SESSION is already rotated in the jar and the error response carries no written list — so the MCP sees only a failure string and has no way to know it must not retry SESSION, or that the browser is now half-rotated. The pre-flight existence check makes this unlikely, not impossible (the store can change between get and set).
Chrome gives no transaction to make this atomic, so the practical fix is to make the partial state legible — include the already-written names in the error, e.g.:
await fail(
`failed to write cookie ${decl.name}: ${String(e)}` +
(written.length > 0 ? ` — already written: ${written.join(', ')}` : ''),
);…and soften the doc block so it doesn't claim atomicity the write phase can't provide.
| sameSite: existing.sameSite, | ||
| ...(existing.expirationDate === undefined ? {} : { expirationDate: existing.expirationDate }), | ||
| storeId: existing.storeId, |
There was a problem hiding this comment.
🟡 sameSite and storeId are both declared optional on ChromeCookie (background.ts:1372-1376) yet are passed through unconditionally, so when they're absent on the source cookie this emits sameSite: undefined / storeId: undefined — exactly the pattern the function's own doc comment singles out as wrong for domain and expirationDate ("must be OMITTED rather than passed as undefined"). Conditional spreads would make the four fields consistent:
| sameSite: existing.sameSite, | |
| ...(existing.expirationDate === undefined ? {} : { expirationDate: existing.expirationDate }), | |
| storeId: existing.storeId, | |
| ...(existing.sameSite === undefined ? {} : { sameSite: existing.sameSite }), | |
| ...(existing.expirationDate === undefined ? {} : { expirationDate: existing.expirationDate }), | |
| ...(existing.storeId === undefined ? {} : { storeId: existing.storeId }), |
(the expirationDate line already below this range stays as-is if you'd rather only touch the two — this suggestion replaces lines 1846-1848, so drop the middle line if you keep the existing one).
| it('labels it as changing state, and warns', async () => { | ||
| const mod = await import('../src/popup/popup.js'); | ||
| const display = (mod as unknown as { | ||
| __testCapabilityDisplay?: Record<string, { label: string; warn: boolean }>; | ||
| }).__testCapabilityDisplay; | ||
| // Falls back to reading the source when the map isn't exported for tests. | ||
| const { readFileSync } = await import('node:fs'); | ||
| const src = display | ||
| ? JSON.stringify(display) | ||
| : readFileSync(new URL('../src/popup/popup.ts', import.meta.url), 'utf8'); | ||
| expect(src).toMatch(/write_cookies/); | ||
| expect(src).toMatch(/Overwrite cookies/); | ||
| expect(src).toMatch(/signed-in session/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 This asserts on the source text of popup.ts rather than on rendered output, so it passes as long as those strings appear anywhere in the file — including inside the comment this PR added right above the entry ("'write cookies' alone reads as a sibling", "signed-in session"). Delete the CAPABILITY_DISPLAY entry and keep the comment and the test still goes green.
The adjacent suite already does this properly by rendering — packages/extension-core/tests/popup.test.ts:518-535:
it('renders DOM selectors when read_dom declared', () => {
...
capabilities: ['fetch', 'read_dom'],
...
expect(container.textContent).toContain('Read DOM elements');
});Following that shape with capabilities: ['fetch', 'write_cookies'] and asserting on container.textContent (plus the warn marker, as read_cookies does at popup.test.ts:426) would actually pin the label the user sees. The __testCapabilityDisplay fallback branch is also dead — nothing exports that symbol.
|
|
||
| **Residual risk:** A user who approves a pair with `read_cookies` is giving the MCP a powerful read primitive for the declared domains. The popup tries to make that visible; the trust record forces re-approval on change. There is no further defense — if you don't trust the MCP, don't approve the pair. | ||
|
|
||
| ### T-cookie-write — `write_cookies` capability misuse |
There was a problem hiding this comment.
🟡 docs/PRIVACY.md §2.2 is the user-facing counterpart to this section and is framed entirely as reads — "Depending on the capabilities an MCP server declares and you approve at pair time, Transporter may read:" followed by a table of read_* / capture_request_header (PRIVACY.md:25-38). The first capability that changes browser state deserves a line there.
Partly pre-existing — read_dom, graphql, and download are missing from that table too — but write_cookies is the one that makes the section's read-only framing inaccurate rather than merely incomplete.
Review —
|
…cannot run Auto-review on #211 caught that `packages/protocol/src/validate.ts` was never updated. Every inbound frame is validated before dispatch, so: - requests fell through to the terminal "unknown op" throw. The extension's decode catch logs and returns, so the request was silently dropped, no response was ever sent, and writeCookies() hung to its timeout. handleWriteCookiesRequest was unreachable in production. - successful responses would throw "unknown success-response op" on the MCP side, which the concentrator host answers by closing the EXTENSION socket — taking down every other MCP on the bridge, not just the caller. Only successes broke: `ok:false` already validated, because KNOWN_RESPONSE_OPS derives from KNOWN_CAPABILITIES and the capability was added. So gate rejections round-tripped cleanly the whole time, which is exactly why nothing noticed. The new validators also enforce two things the extension would otherwise have accepted off the wire: a non-empty `cookies` array (an empty write was answered with `ok: true, written: []` — a success that wrote nothing), and an exact cookie name rather than a glob (declared keys may be globs; a pattern here would reach cookies the pair popup never displayed). Adds packages/protocol/tests/write-cookies-wire.test.ts, which round- trips request and response through sealInnerFrame/openEncryptedFrame. Verified it catches the original bug: with the validator reverted, 8 of its tests fail, including both round-trips. Nothing existing covered this — the server tests stub sendInnerFrame and the extension tests call the pure resolvers directly, so no test crossed the wire. Also from the review: chrome.cookies.set resolves null on failure WITHOUT throwing, so reporting `written` on "didn't throw" alone could claim a write that never landed — the same silent staleness this verb exists to prevent. Now checked. And the extension README documents that write_cookies rides the existing `cookies` permission while adding new chrome.* surface, matching how `graphql` and `download` are covered. Refs #202 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo
|
Claude finished @chrischall's task in 6m 39s —— View job Review —
|
| * | ||
| * So this module is the other half of `trustedMcps`: first contact is trusted | ||
| * and remembered (TOFU, matching what the extension does), and a DIFFERENT | ||
| * identity afterwards is refused rather than silently accepted. |
There was a problem hiding this comment.
🔴 This module is wired to nothing — the pin it describes is never read, written, or checked on any connection path.
199 lines of new security code (plus 156 lines of tests) arrived in commit 758b62e, whose message is entirely about validating write_cookies frames and never mentions it:
$ git show --stat 758b62e
fix(protocol): validate write_cookies frames, without which the verb cannot run
packages/extension-chrome/README.md | 1 +
packages/extension-core/src/background.ts | 12 +-
packages/protocol/src/validate.ts | 52 ++++++
packages/protocol/tests/write-cookies-wire.test.ts | 144 +++++++++++++++
packages/server/src/extension-trust.ts | 199 +++++++++++++++++++++
packages/server/src/identity.ts | 14 +-
packages/server/tests/extension-trust.test.ts | 156 ++++++++++++++++
Nothing imports it:
$ rg -n "extension-trust|decideExtensionTrust|readExtensionPin|writeExtensionPin|allowNewExtensionIdentity" --glob '!*.md' . | grep -v node_modules
./packages/server/src/extension-trust.ts:68:export const TRUST_NEW_EXTENSION_ENV = …
…
./packages/server/tests/extension-trust.test.ts:6: decideExtensionTrust,
Every hit is the module itself or its own test file. host.ts does not import it (rg -n "^import" packages/server/src/host.ts → identity.js, session.js, session-ready.js, build-server-hello.js, and the protocol/ws packages — no extension-trust.js), and packages/server/src/index.ts does not re-export it, while packages/server/package.json exposes exactly one entrypoint ("." → ./dist/index.js). So it is unreachable internally and unavailable to consumers.
Concretely, the doc comment above says:
So this module is the other half of
trustedMcps: first contact is trusted and remembered (TOFU, matching what the extension does), and a DIFFERENT identity afterwards is refused rather than silently accepted.
None of that happens. decideExtensionTrust is never called, so an extension presenting a different identity is still accepted exactly as it was before this PR — the #208 asymmetry is unchanged. Shipping it in this state means the npm package carries a security control that reads as active, under a squash-merge whose title is feat(protocol): add write_cookies.
Two clean resolutions:
- Wire it in — call
readExtensionPin/decideExtensionTrust/writeExtensionPinfrom the extension-hello path inhost.ts(around thesessionSigverification athost.ts:270-273), with tests through that path; or - Split it out — drop it from this PR and land it as its own
feat(security):PR for MCP side does not pin the extension's identity — freshness without continuity #208, where the design gets reviewed and released under a title that names it.
Either way it shouldn't ride along inside a fix(protocol): validate write_cookies frames commit.
| >(); | ||
| // 1.12.0+: write-cookies awaiters resolve the list of names actually | ||
| // written, so a caller can confirm rather than assume. | ||
| private pendingWriteCookies = new Map< |
There was a problem hiding this comment.
🔴 pendingWriteCookies is missing from both cleanup paths every sibling map is in — a disconnect mid-write never rejects the caller.
$ rg -n "pendingReadCookies|pendingWriteCookies|pendingCapture\b" packages/server/src/ws-server.ts
1608: this.pendingReadCookies.delete(id);
1610: this.pendingCapture.delete(id);
…
3122: for (const cb of this.pendingReadCookies.values()) {
3125: this.pendingReadCookies.clear();
3128: for (const { reject } of this.pendingCapture.values()) reject(err);
3129: this.pendingCapture.clear();
pendingWriteCookies appears only at its declaration, its .set (2201), its _withVerbTimeout hand-off (2204), and its response handler (3071-3073).
1. rejectAllPending (ws-server.ts:3111-3138) rejects pending, pendingReadCookies, pendingStorage, pendingCapture, pendingRedirect, pendingIdb, pendingDownload, pendingGraphql — all eight — and not pendingWriteCookies. It's called on extension disconnect (1309, 1345, 1369), on the extension re-entering pairing (1315, 1351, with this.pairingErrorMessage(code)), and from close() (3178).
Failure scenario: an MCP calls writeCookies(), the extension's MV3 service worker is evicted (or the user hits re-pair) before the response frame arrives. Every other verb settles immediately with extension disconnected — or, in the pairing case, the actionable "here is your pair code" message the class goes out of its way to surface. writeCookies() instead waits out the full fetchTimeoutMs and reports a FetchproxyTimeoutError against https://<host>, which classifyBridgeError will read as a slow bridge rather than a down one, pointing the consumer at the wrong remedy.
Worse, with fetchTimeoutMs: 0 — an explicitly supported opt-out, per ws-server.ts:729 ("0 means the caller explicitly opted out of the …") — _withVerbTimeout short-circuits at 1690-1691:
const timeoutMs = this.opts.fetchTimeoutMs;
if (timeoutMs === undefined || timeoutMs <= 0) return pending;No timer is armed, nothing else ever settles the promise, and writeCookies() hangs forever after a disconnect. That's an MCP tool call that never returns.
2. sendInnerFrame's catch (ws-server.ts:1604-1616) deletes the just-registered resolver from all eight sibling maps when the send throws, under a doc comment that states the invariant:
drop the just-registered pending resolver for this id (it lives in exactly one of the op maps — request ids are unique) so it doesn't leak until the server closes
For write_cookies that invariant is now false: a FetchproxySessionNotReadyError out of the send leaves the entry in pendingWriteCookies for the life of the server.
Both are one line each:
// in rejectAllPending, alongside the others:
for (const { reject } of this.pendingWriteCookies.values()) reject(err);
this.pendingWriteCookies.clear();
// in sendInnerFrame's catch:
this.pendingWriteCookies.delete(id);
| } | ||
| return raw as unknown as InnerFrame; | ||
| } | ||
| if (raw.op === 'write_cookies') { |
There was a problem hiding this comment.
🟡 Nit — the branch is right, but validateInnerRequest's terminal message (validate.ts:999-1001) still enumerates the pre-write_cookies op list:
throw new ProtocolError(
`inner.op: must be one of "fetch", "read_cookies", "read_local_storage", "read_session_storage", "capture_request_header", "capture_redirect", "read_indexed_db", "read_dom", "download", "graphql_query"; got ${JSON.stringify(raw.op)}`,
);It's the file's only human-readable inventory of valid ops, and it now under-reports by one. A typo'd write_cookie would produce an error listing nine ops that doesn't include the one the caller nearly typed correctly.
| sameSite: existing.sameSite, | ||
| ...(existing.expirationDate === undefined ? {} : { expirationDate: existing.expirationDate }), | ||
| storeId: existing.storeId, |
There was a problem hiding this comment.
🟡 Nit (carried over from the previous round, still open) — sameSite and storeId are declared optional on ChromeCookie (background.ts:1370-1374) but passed unconditionally, so an absent value is emitted as an explicit undefined — the exact pattern this function's own doc comment singles out as wrong two fields above:
Two fields must be OMITTED rather than passed as undefined
domain and expirationDate use conditional spreads; these two don't. Making all four consistent removes the question of whether Chrome distinguishes "absent" from "present and undefined" here, rather than relying on it not doing so:
| sameSite: existing.sameSite, | |
| ...(existing.expirationDate === undefined ? {} : { expirationDate: existing.expirationDate }), | |
| storeId: existing.storeId, | |
| ...(existing.sameSite === undefined ? {} : { sameSite: existing.sameSite }), | |
| ...(existing.expirationDate === undefined ? {} : { expirationDate: existing.expirationDate }), | |
| ...(existing.storeId === undefined ? {} : { storeId: existing.storeId }), |
| // staleness this verb exists to prevent. | ||
| const result = await chrome.cookies.set(cookieSetDetailsFor(url, decl, existing)); | ||
| if (!result) { | ||
| await fail(`failed to write cookie ${decl.name}: chrome.cookies.set returned null`); | ||
| return; | ||
| } | ||
| written.push(decl.name); | ||
| } catch (e) { | ||
| await fail(`failed to write cookie ${decl.name}: ${String(e)}`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Nit (carried over, still open — and the new result === null check adds a second way to hit it) — this loop can apply part of the request, which the doc block at background.ts:1849-1856 says it does not:
Three gates, in order, all refusing the WHOLE request rather than applying part of it — a half-written rotation is worse than a failed one
The three gates keep that promise; the write phase doesn't. With {SESSION, REFRESH}, if SESSION sets cleanly and REFRESH returns null or throws, SESSION is already rotated in the jar and the error response carries no written list — so the MCP sees only a failure string, can't tell the jar is half-rotated, and will likely retry the whole batch. The pre-flight existence pass makes this unlikely, not impossible (the store can change between get and set).
Chrome offers no transaction, so the fix is legibility rather than atomicity — name what already landed:
const partial = written.length > 0 ? ` — already written: ${written.join(', ')}` : '';
await fail(`failed to write cookie ${decl.name}: chrome.cookies.set returned null${partial}`);…and soften the doc block so it claims whole-request refusal for the gates rather than for the verb.
| const display = (mod as unknown as { | ||
| __testCapabilityDisplay?: Record<string, { label: string; warn: boolean }>; | ||
| }).__testCapabilityDisplay; | ||
| // Falls back to reading the source when the map isn't exported for tests. | ||
| const { readFileSync } = await import('node:fs'); | ||
| const src = display | ||
| ? JSON.stringify(display) | ||
| : readFileSync(new URL('../src/popup/popup.ts', import.meta.url), 'utf8'); | ||
| expect(src).toMatch(/write_cookies/); | ||
| expect(src).toMatch(/Overwrite cookies/); | ||
| expect(src).toMatch(/signed-in session/); | ||
| }); |
There was a problem hiding this comment.
🟡 Nit (carried over, still open) — this asserts on the source text of popup.ts, so it passes as long as those strings appear anywhere in the file, including inside the comment this PR added directly above the map entry (popup.ts:40-42: "'write cookies' alone reads as a sibling…", "can change your signed-in session"). Delete the CAPABILITY_DISPLAY entry, keep the comment, and this test stays green while the user sees nothing.
The adjacent suite already does it properly by rendering — packages/extension-core/tests/popup.test.ts:518-535:
it('renders DOM selectors when read_dom declared', () => {
…
capabilities: ['fetch', 'read_dom'],
…
expect(container.textContent).toContain('Read DOM elements');
});Following that with capabilities: ['fetch', 'write_cookies'] and asserting on container.textContent (plus the warn marker, as the read_cookies case does at popup.test.ts:426) would actually pin the label. The __testCapabilityDisplay branch is dead either way — rg -n "__testCapabilityDisplay" packages/extension-core/src returns nothing.
…cannot run Auto-review on #211 caught that `packages/protocol/src/validate.ts` was never updated. Every inbound frame is validated before dispatch, so: - requests fell through to the terminal "unknown op" throw. The extension's decode catch logs and returns, so the request was silently dropped, no response was ever sent, and writeCookies() hung to its timeout. handleWriteCookiesRequest was unreachable in production. - successful responses would throw "unknown success-response op" on the MCP side, which the concentrator host answers by closing the EXTENSION socket — taking down every other MCP on the bridge, not just the caller. Only successes broke: `ok:false` already validated, because KNOWN_RESPONSE_OPS derives from KNOWN_CAPABILITIES and the capability was added. So gate rejections round-tripped cleanly the whole time, which is exactly why nothing noticed. The new validators also enforce two things the extension would otherwise have accepted off the wire: a non-empty `cookies` array (an empty write was answered with `ok: true, written: []` — a success that wrote nothing), and an exact cookie name rather than a glob (declared keys may be globs; a pattern here would reach cookies the pair popup never displayed). Adds packages/protocol/tests/write-cookies-wire.test.ts, which round- trips request and response through sealInnerFrame/openEncryptedFrame. Verified it catches the original bug: with the validator reverted, 8 of its tests fail, including both round-trips. Nothing existing covered this — the server tests stub sendInnerFrame and the extension tests call the pure resolvers directly, so no test crossed the wire. Also from the review: chrome.cookies.set resolves null on failure WITHOUT throwing, so reporting `written` on "didn't throw" alone could claim a write that never landed — the same silent staleness this verb exists to prevent. Now checked. And the extension README documents that write_cookies rides the existing `cookies` permission while adding new chrome.* surface, matching how `graphql` and `download` are covered. Refs #202 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo
758b62e to
032774d
Compare
|
Claude finished @chrischall's task in 2m 46s —— View job Review —
|
Correct, and it was mine to cause
What happenedI committed with "Completely unwired" was exactly right, and the reason is worse than accidental inclusion: the wiring for that module ( The files are untouched and remain as working-tree changes in the local repo; I removed them from the commit rather than reverting them. VerificationBecause the working tree still holds that other work, a local test run cannot prove this branch stands alone. I built and tested it in a detached worktree containing only these commits: typecheck clean, 1188 tests pass. (The 1202 I quoted last round included the 14 tests from the swept-in file — so that figure was inflated, and I'd have kept quoting it.) Process fix
🤖 Generated with Claude Code |
| >(); | ||
| // 1.12.0+: write-cookies awaiters resolve the list of names actually | ||
| // written, so a caller can confirm rather than assume. | ||
| private pendingWriteCookies = new Map< |
There was a problem hiding this comment.
🔴 Still open from the last round — pendingWriteCookies is in neither cleanup path, unlike all eight sibling maps.
$ rg -n "pendingWriteCookies" packages/server/src/ws-server.ts
1030: private pendingWriteCookies = new Map<
2201: this.pendingWriteCookies.set(id, { resolve, reject });
2204: return this._withVerbTimeout(pending, this.pendingWriteCookies, id, `https://${host}`);
3071: const writeCookiesCb = this.pendingWriteCookies.get(inner.id);
3073: this.pendingWriteCookies.delete(inner.id);
Declaration, registration, timeout hand-off, response handler — and nothing else. Compare the sibling:
$ rg -n "pendingGraphql" packages/server/src/ws-server.ts
1057: private pendingGraphql = new Map<
1614: this.pendingGraphql.delete(id); ← sendInnerFrame catch
…
3136: for (const { reject } of this.pendingGraphql.values()) reject(err); ← rejectAllPending
3137: this.pendingGraphql.clear();
1. rejectAllPending (ws-server.ts:3111-3137) rejects pending, pendingReadCookies, pendingStorage, pendingCapture, pendingRedirect, pendingIdb, pendingDownload, pendingGraphql — and not pendingWriteCookies. It's the handler for extension disconnect, for the extension re-entering pairing (where it carries the actionable pair-code message), and for close().
Failure scenario: an MCP calls writeCookies(), the MV3 service worker is evicted (or the user hits re-pair) before the response arrives. Every other verb settles immediately with extension disconnected; writeCookies() instead waits out the full fetchTimeoutMs (default 30_000, ws-server.ts:1172) and reports a FetchproxyTimeoutError, which reads as a slow bridge rather than a down one.
With fetchTimeoutMs: 0 — an explicitly supported opt-out (ws-server.ts:729: "0 means the caller explicitly opted out") — _withVerbTimeout short-circuits at 1690-1691:
const timeoutMs = this.opts.fetchTimeoutMs;
if (timeoutMs === undefined || timeoutMs <= 0) return pending;No timer is armed and nothing else settles the promise, so writeCookies() never returns after a disconnect.
2. sendInnerFrame's catch (1604-1616) deletes the just-registered resolver from all eight sibling maps, under a doc comment stating the invariant:
drop the just-registered pending resolver for this id (it lives in exactly one of the op maps — request ids are unique) so it doesn't leak until the server closes
For write_cookies that's now false: a throw out of the send (e.g. FetchproxySessionNotReadyError) leaves the entry in pendingWriteCookies for the life of the server.
Two one-line additions:
// rejectAllPending, alongside the others:
for (const { reject } of this.pendingWriteCookies.values()) reject(err);
this.pendingWriteCookies.clear();
// sendInnerFrame's catch:
this.pendingWriteCookies.delete(id);
| } | ||
| return raw as unknown as InnerFrame; | ||
| } | ||
| if (raw.op === 'write_cookies') { |
There was a problem hiding this comment.
🟡 Nit (carried over, still open) — the new branch is right, but validateInnerRequest's terminal message still enumerates the pre-write_cookies op list (validate.ts:999-1001):
throw new ProtocolError(
`inner.op: must be one of "fetch", "read_cookies", "read_local_storage", "read_session_storage", "capture_request_header", "capture_redirect", "read_indexed_db", "read_dom", "download", "graphql_query"; got ${JSON.stringify(raw.op)}`,
);It's the file's only human-readable inventory of valid ops and now under-reports by one — a typo'd write_cookie produces an error listing nine ops that omits the one the caller nearly typed correctly.
| httpOnly: existing.httpOnly, | ||
| sameSite: existing.sameSite, | ||
| ...(existing.expirationDate === undefined ? {} : { expirationDate: existing.expirationDate }), | ||
| storeId: existing.storeId, |
There was a problem hiding this comment.
🟡 Nit (carried over, still open) — sameSite and storeId are declared optional on ChromeCookie (background.ts:1373-1375) but passed unconditionally, so an absent value is emitted as an explicit undefined — the exact pattern this function's own doc comment singles out as wrong two fields above:
Two fields must be OMITTED rather than passed as undefined
domain and expirationDate use conditional spreads; these two don't. Making all four consistent removes the reliance on Chrome not distinguishing "absent" from "present and undefined":
| storeId: existing.storeId, | |
| ...(existing.sameSite === undefined ? {} : { sameSite: existing.sameSite }), | |
| ...(existing.expirationDate === undefined ? {} : { expirationDate: existing.expirationDate }), | |
| ...(existing.storeId === undefined ? {} : { storeId: existing.storeId }), |
| } catch (e) { | ||
| await fail(`failed to write cookie ${decl.name}: ${String(e)}`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Nit (carried over, still open) — this loop can apply part of the request, which the doc block at background.ts:1849-1856 says it does not:
Three gates, in order, all refusing the WHOLE request rather than applying part of it — a half-written rotation is worse than a failed one
The three gates keep that promise; the write phase doesn't. With {SESSION, REFRESH}, if SESSION sets cleanly and REFRESH returns null or throws, SESSION is already rotated in the jar and the error response carries no written list — so the MCP sees a bare failure string, can't tell the jar is half-rotated, and will likely retry the whole batch. The pre-flight existence pass makes this unlikely, not impossible (the store can change between get and set).
Chrome offers no transaction, so the fix is legibility rather than atomicity — name what already landed:
const partial = written.length > 0 ? ` — already written: ${written.join(', ')}` : '';
await fail(`failed to write cookie ${decl.name}: chrome.cookies.set returned null${partial}`);…and soften the doc block so whole-request refusal is claimed for the gates rather than for the verb.
| expect(src).toMatch(/write_cookies/); | ||
| expect(src).toMatch(/Overwrite cookies/); | ||
| expect(src).toMatch(/signed-in session/); | ||
| }); |
There was a problem hiding this comment.
🟡 Nit (carried over, still open) — this asserts on the source text of popup.ts, so it passes as long as those strings appear anywhere in the file, including inside the comment this PR added directly above the map entry (popup.ts:40-42: "'write cookies' alone reads as a sibling…", "can change your signed-in session"). Delete the CAPABILITY_DISPLAY entry, keep the comment, and this test stays green while the user sees nothing.
The __testCapabilityDisplay branch is dead — the symbol exists only in this test file:
$ rg -n "__testCapabilityDisplay" packages/extension-core/src packages/extension-core/tests
packages/extension-core/tests/write-cookies.test.ts:127: __testCapabilityDisplay?: Record<string, { label: string; warn: boolean }>;
packages/extension-core/tests/write-cookies.test.ts:128: }).__testCapabilityDisplay;
The adjacent suite already does this properly by rendering (packages/extension-core/tests/popup.test.ts:518-535); following that with capabilities: ['fetch', 'write_cookies'] and asserting on container.textContent (plus the warn marker, as the read_cookies case does at popup.test.ts:426) would actually pin the label.
|
|
||
| **Residual risk:** A user who approves a pair with `read_cookies` is giving the MCP a powerful read primitive for the declared domains. The popup tries to make that visible; the trust record forces re-approval on change. There is no further defense — if you don't trust the MCP, don't approve the pair. | ||
|
|
||
| ### T-cookie-write — `write_cookies` capability misuse |
There was a problem hiding this comment.
🟡 Nit (carried over, still open) — docs/PRIVACY.md §2.2 is the user-facing counterpart to this section and is framed entirely as reads:
$ sed -n 25p docs/PRIVACY.md
Depending on the capabilities an MCP server declares and you approve at pair time, Transporter may **read**:
followed by a table of read_* / capture_request_header. write_cookies is the first capability that changes browser state, which makes that framing inaccurate rather than merely incomplete. (Partly pre-existing — read_dom, graphql, and download are missing from that table too.)
…ss of gap
`writeCookies()` registered an awaiter but was missing from both cleanup
paths every sibling map is in. On extension disconnect, re-pair, or
close(), an in-flight call waited out `fetchTimeoutMs` and reported a
timeout instead of "extension disconnected" — and with the supported
`fetchTimeoutMs: 0` opt-out, `_withVerbTimeout` arms no timer, so the
promise never settled at all.
Two one-line additions fix this instance. The more useful part is
`packages/server/tests/pending-maps-drained.test.ts`, which asserts the
invariant across every map by DISCOVERY rather than by name: it walks the
instance for `pending*` Maps, seeds each, and requires `rejectAllPending`
to settle-and-clear all of them and `sendInnerFrame`'s catch to drop the
id from all of them. The next verb to add a map is covered the moment it
exists, with no new test to remember. Verified it fails by name on the
unfixed code ("pendingWriteCookies was not cleared by rejectAllPending").
Per-verb tests could never have caught this — each exercises its own map,
so the one map nobody wired up is exactly the one nothing tests.
Also from review:
- validateInnerRequest's unknown-op message still listed nine ops
- cookieSetDetailsFor passed `sameSite`/`storeId` as explicit undefined
while `domain`/`expirationDate` used conditional spreads, against the
rule its own doc states; all four are conditional now
- a mid-batch write failure reported a bare error, so an MCP could not
tell a jar that is half-rotated from one untouched, and would retry
the whole batch. The gates are all-or-nothing but the writes cannot
be (there is no transaction over chrome.cookies), so the error now
names what already landed and the doc says so instead of implying
atomicity
- the popup test asserted on popup.ts SOURCE TEXT, so it passed on a
comment containing the right words. CAPABILITY_DISPLAY is exported
and the test now reads the actual label, plus asserts it does not
read as another "Read …" verb
- docs/PRIVACY.md said Transporter "may read"; the first
state-changing capability makes that inaccurate, not just incomplete
Refs #202
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo
|
Claude finished @chrischall's task in 5m 25s —— View job Review —
|
|
The blockerBoth cleanup paths now drain The more useful part is Verified it fails on the unfixed code, naming the culprit: It also asserts it found ≥9 maps, so it can't pass vacuously if the discovery ever breaks, and it checks awaiters are settled rather than merely dropped — clearing a map without settling hangs the caller just as badly. Worth naming why this survived three rounds: every per-verb test exercises its own map, so the one map nobody wired up is exactly the one nothing tests. Same shape as the validator gap — a per-op test suite can't see a per-op omission. Nits
VerificationBuilt and tested in a detached worktree containing only this branch's commits — typecheck clean, 1192 tests pass. Local runs are still unreliable for this branch because the working tree holds unrelated in-progress work, which is what produced the inflated 1202 figure two rounds ago. Staging is now explicit per path rather than Your two open questionsBoth go on the manual post-deploy checklist rather than being answered here, since neither is decidable from static reading:
Added both to the checklist comment above. |
| * this verb exists to prevent. `hostOnly` decides whether `domain` may be sent | ||
| * at all: Chrome rejects `domain` on a host-only cookie. | ||
| */ | ||
| /** |
There was a problem hiding this comment.
🟡 Nit (new in 3c12399) — inserting partialWriteError here orphaned the verb's doc block.
Lines 1857–1891 are the ~35-line JSDoc for handleWriteCookiesRequest (the three gates, the all-or-nothing note, the chrome.cookies.set shadowing trap). partialWriteError and its own JSDoc were spliced in between that block and the function it documents:
1891 */ ← end of handleWriteCookiesRequest's doc
1892 /** ← partialWriteError's doc starts
…
1899 function partialWriteError(…)
…
1906 async function handleWriteCookiesRequest( ← now has no doc comment attached
Two consecutive JSDoc blocks: the tooling-visible one for partialWriteError is the second, and everything above it now attaches to partialWriteError too (or to nothing). Hovering handleWriteCookiesRequest in an editor shows nothing, and the gate documentation — which is the security argument for this verb — reads as documentation for a string formatter.
Moving the partialWriteError definition (with its own doc) above line 1857 restores the pairing without touching any prose.
| // reading it. Labelled to say so outright — 'write cookies' alone reads | ||
| // as a sibling of the reads above, and it is not one. | ||
| write_cookies: { | ||
| label: 'Overwrite cookies it can already read (can change your signed-in session)', |
There was a problem hiding this comment.
🟡 Nit — this label is right, but the cookie names are still surfaced only under a heading that says "Read cookies", even when write_cookies is granted.
renderPendingPair itemises the scope at popup.ts:693:
appendScopeSubList(dl, 'Read cookies', pending.cookieKeys);appendScopeSubList renders on non-empty cookieKeys regardless of capability (popup.ts:206-218), so the list does appear for a write-declaring MCP — but it's labelled as a read, and it is the only place in the card the individual names appear. The user has to join the capability line here with a sub-list two sections down to work out which cookies are overwritable.
That matters more for this capability than for the reads, per the comment directly above that call (popup.ts:690-692):
the user approves the exact set of names, not just "this MCP can read storage" — so the pair popup MUST show them
Since the write scope is by construction identical to cookieKeys, the cheapest fix is to vary that heading:
appendScopeSubList(
dl,
pending.capabilities.includes('write_cookies') ? 'Read + overwrite cookies' : 'Read cookies',
pending.cookieKeys,
);Worth pinning with a rendering-based assertion in the style of popup.test.ts:518-535 rather than another map lookup.
chrischall
added
the
ready-to-merge
…itable cookies as writable (#215) Closes #212 — the two round-4 findings on #211, which merged before they could be addressed on that PR. ## Orphaned doc block `partialWriteError` was spliced in between `handleWriteCookiesRequest`'s doc block and the function, so ~40 lines documenting the verb's four gates, its all-or-nothing refusal and the cookie-shadowing trap ended up describing a five-line string helper. The helper moves above the block; nothing else changes. Worth naming the cause: I was editing source by matching a string anchor. The anchor matched, the edit applied, the build passed, the tests passed — and the result was still wrong, because nothing verifies that prose sits on the function it describes. ## Cookie names headed "Read cookies" while granting a write `popup.ts` listed `cookieKeys` under `'Read cookies'` unconditionally. That sub-list is the **only** place those names appear, so a user granting `write_cookies` saw the affected cookies filed under a read verb — directly contradicting the capability line above it, which says *"Overwrite cookies it can already read (can change your signed-in session)"*. It now reads **"Read and overwrite cookies"** when the capability is present. The user sees one consistent story at the moment they decide, which is the whole point of itemising the names in the first place. Pinned by a **rendering** test — `renderPopup` into a container, then read the actual `<dt>` — not a source-text match. The earlier popup test in this series asserted on source text and would have passed on a comment containing the right words; not worth repeating that mistake two PRs later. Verified the test fails against the unfixed heading before fixing it. ## Note on #212 I reopened it. `Closes #212` went into #211's body when only the round-3 items were resolved, and round 4 then added these two to the same issue — so the merge closed it with both outstanding. `Closes` belongs on a PR only once every item is genuinely done, and I added it a round early. 1194 tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [2.0.0](v1.11.0...v2.0.0) (2026-08-06) ### ⚠ BREAKING CHANGES * **protocol:** bind the ephemeral key into the ready signature ([#222](#222)) ### Features * **protocol:** add write_cookies, the one verb that can repair a rotated session ([#211](#211)) ([b2557c2](b2557c2)) * **protocol:** bind the ephemeral key into the ready signature ([#222](#222)) ([c13aeed](c13aeed)) * **server:** let a request name the tab that relays it ([#207](#207)) ([c5d3f4d](c5d3f4d)) * **server:** pin the extension's identity, and verify it on the peer path ([#213](#213)) ([0eeced7](0eeced7)) ### Bug Fixes * **cli:** let a real filesystem error be itself, not "no extension pin" ([#221](#221)) ([c87a864](c87a864)), closes [#220](#220) * **cli:** validate --via-tab before connecting, like the request URL ([#210](#210)) ([959fcc5](959fcc5)) * **extension:** reattach the write_cookies doc block, and name the writable cookies as writable ([#215](#215)) ([2730c4a](2730c4a)) * **extension:** use the guarded caps local for the cookie heading ([#217](#217)) ([f95c832](f95c832)) * **server:** release only our own extension claim, and stop guessing scoped names ([#219](#219)) ([3d90a64](3d90a64)), closes [#218](#218) * **server:** type no-tab rejections so they stop reading as version mismatches ([#205](#205)) ([dc30bd9](dc30bd9)) ### Refactor * **server:** drop the concatBytes imports the signature change orphaned ([#224](#224)) ([4985ba7](4985ba7)), closes [#223](#223) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Closes #202.
Why a write verb at all
Every verb until now reads, and that leaves a failure class consuming MCPs cannot fix locally. Sites that rotate a credential cookie hand back a new value on each refresh; when an MCP refreshes and keeps the result to itself, the copy in the browser's cookie jar is dead. The user is signed out of a tab they never touched, and told it was "inactivity".
Measured on creditkarma.com: two MCP-side refreshes logged out an untouched, freshly signed-in tab, and reloading doesn't recover it — only a full sign-in does. That MCP shipped the only mitigation available to it (refresh only when the token is genuinely spent, chrischall/creditkarma-mcp#121) and it still can't help once the browser has been idle past the access token's ~15-minute life — the scheduled-sync case, i.e. the common one.
The design decision I'd most like reviewed
Writes reuse the declared
cookieKeysrather than getting their own list.I flagged both options in #202 and picked the narrower one. It keeps the blast radius identical to the read scope the user already saw and approved, and avoids threading a second declaration array through the trust store, scope diff, popup, and CLI. The cost: you cannot grant write on a cookie you can't read. That seemed right for a first write verb, and a separate
cookieWriteKeyscan be added later without a wire break — say the word if you'd rather have it now.Constraints, all enforced extension-side
write_cookies, declared in the hello, approved at pair time as its own line, stored in the trust record. Adding it later forces a re-pair with the diff UI like any other capability change.cookieKeys. Granting writes cannot widen which cookies are in play, only what may be done to ones already listed.domains, decided on the bare origin before any path, exactly as the read path does.Any violation refuses the whole request rather than applying part of it — a half-written rotation leaves the session in a state neither side expects.
The trap this would otherwise have shipped with
chrome.cookies.set({url, name, value})looks like it works and quietly creates a second cookie. Chrome derives a host-only, non-HttpOnly, default-path cookie from the URL, which does not replace aDomain=-scoped or HttpOnly original — so the site keeps reading the stale one. That's precisely the silent failure this verb exists to prevent, so attributes are copied off the cookie being replaced, with two that must be omitted rather than passed as undefined:domainon a host-only cookie — Chrome reads its presence as "widen this into a domain cookie"expirationDateon a session cookie — passing undefined would turn a persistent cookie into a session oneBoth have their own tests.
Surfacing it to the user
The popup labels it "Overwrite cookies it can already read (can change your signed-in session)", not as a sibling of the read verbs, because it isn't one.
docs/SECURITY.mdgains §T-cookie-write with the four constraints and the residual risk stated plainly: an MCP with this capability can set a declared cookie to a value of its choosing, which grants no new access (it could already read and exfiltrate those cookies) but does add the ability to alter browser state for them.Tests
1171 pass (up from 1157). Server-side: capability gate, undeclared key, mixed batch, empty write, and the frame shape including the bare-origin/path invariant. Extension-side: the gate resolver and the attribute-copying builder, including both omission cases. Plus a check that the popup presents this as a write.
Following the repo's existing pattern, the security decisions are exported as pure functions (
resolveWriteCookiesRequest,cookieSetDetailsFor) so they're unit-testable without live WS/session state — same shape asresolveReadDomRequest.Not done
No CLI verb for writing. The CLI gains only
fpx profile declare --allow-cookie-writeso a profile can opt in; the write itself is an MCP-side operation and I didn't want to add a way to hand-set cookies from a shell without you asking for it.🤖 Generated with Claude Code
https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo
Review fixes (
758b62e) — closes #212. Auto-review failed this correctly:packages/protocol/src/validate.tswas never updated, so the verb could not execute at all. Requests were dropped at the frame-decode boundary andwriteCookies()hung to its timeout; a successful response would have thrown on the MCP side and closed the extension socket for every MCP on the concentrator. Only successes broke —ok:falsealready validated viaKNOWN_RESPONSE_OPS, which is why nothing noticed.My error was specific and worth naming: I grepped
validate.tsforread_cookies, got no hits, and concluded it didn't dispatch on ops. The file is treated as binary by grep, so the search was silently empty rather than genuinely negative.CLAUDE.mdnames that file as a merge blocker for new protocol fields and I walked straight past it.New
packages/protocol/tests/write-cookies-wire.test.tsround-trips throughsealInnerFrame/openEncryptedFrame. Verified it catches the original bug: with the validator reverted, 8 of its tests fail, including both round-trips. All six checklist items are done, plus two extras that fell out of tracing the findings — the validator refuses a glob in a cookie name (declared keys may be globs; a pattern would reach cookies the popup never showed) and refuses extra fields on a cookie entry (no smugglingdomain/expirationDatepast the in-place contract).1202 tests pass.
Closes #212