fix(server): authenticate before advancing the replay counter, and implement the handshake timeout the docs already claim - #356
Conversation
…authenticates `SessionState.acceptInboundSeq` (and the extension's `SessionEntry` mirror) checked and advanced `lastInbound` in one call, and all three receivers called it BEFORE the AES-GCM open. So anything that could put bytes on the socket could name a `seq` without holding the session key, and every genuine frame behind it — all carrying lower numbers — was then dropped as a replay: the peer and the extension were wedged for the life of the session with the socket still open and looking healthy, and the host took its in-flight frames down with the teardown the forged frame provoked. The gate is now two calls, which is what lets the caller place the commit where only it knows the frame authenticated: `isFreshInboundSeq` asks, `commitInboundSeq` records, and the single-call form is gone rather than left beside them. host.ts and peer.ts capture the session before the await so a renegotiation during the open cannot commit the seq against the new key; peer.ts commits on `validation-failed` too, since that frame decrypted under the live key and its seq is genuinely spent. Tests at each of the three sites drive a real handshake and send a frame that fails GCM authentication followed by a genuine one with a LOWER seq, asserting the genuine frame still lands — and that replaying it is still refused, so the counter has not simply stopped working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Tar4Eh59YtFuxQy4BpRBQ
…a peer may send docs/SECURITY.md §T2 defense 3 has promised since the threat model was written that "connections that don't send a valid hello frame within 15 seconds get closed", and nothing implemented it: a socket that reached the upgrade — a drive-by page, or any local process enumerating the port — held a connection open for as long as it liked. HANDSHAKE_TIMEOUT_MS (15 000, overridable so a suite need not wait out fifteen real seconds) now closes one with 1008, gated on the connection having IDENTIFIED rather than on bytes having arrived: the extension path sets `identified` only after an awaited pin read, so a flag set when a hello lands would spare a socket the trust decision is still refusing. `maxPayload` on the same server is the other half of the same reason. `ws` defaults to 100 MiB, which is process memory a local peer can make the host allocate before a byte is validated; 8 MiB is the extension's own 5 MiB body cap plus base64 expansion and the envelope, so no legitimate frame is near it. Capping it is what made a socket 'error' routine rather than exotic — `ws` emits WS_ERR_UNSUPPORTED_MESSAGE_LENGTH before closing with 1009 — and the host registered no 'error' listener, so one oversize frame was an uncaught exception that took the whole MCP process down. It now logs to stderr; the test suite reporting no unhandled error is the proof. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Tar4Eh59YtFuxQy4BpRBQ
github-actions
Bot
added
the
auto-review
|
Claude finished @chrischall's task in 4m 53s —— View job Review posted
Summary: the replay-counter fix is correct at all three sites (session captured before the Not done: I could not install dependencies in this checkout, so Verdict: fail |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * The largest frame the host will accept from a peer or the extension. | ||
| * | ||
| * `ws` defaults to 100 MiB, which is a lot of process memory a local peer can | ||
| * make the host allocate before a single byte is validated. 8 MiB is the | ||
| * extension's own 5 MiB body cap plus room for base64 expansion and the | ||
| * envelope around it, so no legitimate frame is near it. Over the cap, `ws` | ||
| * closes the socket with 1009 without buffering the rest. | ||
| */ | ||
| export const MAX_PAYLOAD_BYTES = 8 * 1024 * 1024; |
There was a problem hiding this comment.
🔴 The 8 MiB headroom calculation doesn't hold — a legitimate response can exceed it and take the shared extension socket down with 1009.
The comment reasons "5 MiB body cap plus room for base64 expansion and the envelope", but two things break that:
-
The wire frame is base64(AES-GCM(UTF-8(JSON(inner)))) —
sealInnerFrame(packages/protocol/src/seal.ts:26-34) JSON-stringifies the inner frame, UTF-8-encodes it, thentoB64s the ciphertext (packages/protocol/src/encoding.ts:16-20, standardbtoa). So the on-wire size is ≈ 4/3 × the UTF-8 byte length of the plaintext. The 8 MiB cap therefore allows only ~6.29 MB of plaintext. Meanwhile the extension's cap isbody.length > MAX_RESPONSE_BODY_BYTES(packages/extension-core/src/content.ts:695) — that's UTF-16 code units, not bytes. A 2.1 M-character CJK body is 6.3 MB of UTF-8, passes the extension's check comfortably, and produces a frame over the cap. JSON escaping of"/\inside the body pushes it the same direction. -
The other response verbs have no size cap at all.
readIndexedDb/readLocalStorage/readDomreturn whatever the page holds:$ rg -n "length >|too large" packages/extension-core/src/background/handlers/*.ts packages/extension-core/src/background/handlers/read-storage.ts:58: if (undeclared.length > 0) { packages/extension-core/src/background/handlers/read-dom.ts:43: if (undeclared.length > 0) { packages/extension-core/src/background/handlers/read-indexed-db.ts:55: if (undeclared.length > 0) {Only
undeclared-key checks — no byte guard. A 7 MB IndexedDB store is a plausiblereadIndexedDbresult.
Failure scenario: an MCP calls readIndexedDb on a site whose store serializes to ~7 MB (or fetches a ~2.5 M-char CJK/JSON page). The extension seals and sends a ~9.3 MiB frame; ws on the host refuses it with 1009 and closes the host's one extension socket — which serves every MCP on the concentrator, not just the caller. The new ws.on('error') handler turns the underlying error into a console.warn, so the whole bridge drops with one warning line and no per-request error. This worked before the cap (ws default is much larger).
Suggestions: size the cap from the real worst case (5 MiB chars × up to 3–4 bytes/char × 4/3 ≈ 21–28 MiB, so e.g. 32 MiB still bounds the allocation) and/or make the extension-side caps byte-accurate and extend them to the storage/DOM/IndexedDB verbs, so an oversize payload becomes a per-request error instead of a concentrator-wide disconnect.
| // anything able to reach this socket could name a seq without holding the | ||
| // key, and every genuine frame after it — all carrying lower numbers — was | ||
| // dropped as a replay while the socket stayed open and looked healthy. | ||
| entry.commitInboundSeq(frame.seq); |
There was a problem hiding this comment.
🟡 Nit: this site doesn't follow the rule peer.ts states for the same decision. In packages/server/src/peer.ts:407-413 the counter moves for both authenticated outcomes — "a validation failure decrypted under the live key, so its seq is genuinely spent and replaying it must still be refused". Here openEncryptedFrame throws on validation-failed too (seal.ts:47-54), so it lands in the catch above and the seq is never committed, leaving an authenticated-but-malformed frame replayable on a socket that stays open. Low impact — replaying a frame that fails validateInnerFrame does nothing on the second pass either — but it's the one live-socket site where the stated invariant doesn't hold (the host tears the socket down, so its equivalent gap is moot). Switching to openEncryptedFrameDetailed here would make the three sites agree.
Review — PR #356The replay-counter half of this is right, and the ask-then-commit split is the correct shape: One 🔴 finding, on the
One 🟡 nit: the extension Residuals the description already records (same-burst double-open window; the host still tearing the socket down on a frame it cannot open) look like fair scope calls, not blockers. Verification note: dependencies are not installed in this checkout and Verdict: fail |
|
🟡 Auto-review verdict: warn — The previous round's 🔴 same-burst replay window is properly closed by the synchronous claim/commit/release gate — verified at all three call sites that the claim precedes any await, with tests that deliver both duplicates in one read — and all three prior nits (peer 'error' listener, MCP-side producer frame-size refusal, double serialisation) are resolved. Two minor nits remain: the peer's client socket has no maxPayload, and the in-flight-bound refusal is silent. |
…rame, not the shared socket The 8 MiB `MAX_PAYLOAD_BYTES` added in 699afb0 sat BELOW the worst legitimate frame. The wire form is base64 of the JSON of the plaintext, so a frame's size is nothing like its body's: `content.ts` caps a relayed body at 5 MiB counted in UTF-16 code units, and one such unit becomes up to six bytes once JSON has escaped it and then grows by a third again through base64 — while `read_indexed_db`, `read_local_storage` and `read_dom` had no size cap at all, so no fixed number could have been "above every legitimate frame" in the first place. A payload over `maxPayload` is answered by `ws` CLOSING the socket with 1009, and on the concentrator that socket is the ONE the extension holds for every MCP on the host: one large storage read would have dropped all of them. So the cap is now stated where the size is known and the failure can be contained, and the receiver's is derived from it: - `MAX_FRAME_BYTES` (protocol, `seal.ts`) is derived from the 5 MiB body cap with the arithmetic written out beside it — 6 bytes/unit worst case, the rest of the inner frame, the GCM tag, base64, the envelope: 42,030,956 bytes, so 42 MiB. `sealedFrameWireBytes()` answers exactly what a frame will weigh without sealing it (a test pins it against the real sealed bytes). - The extension measures every outbound frame against that constant in `sendInner` — the one and only sender, which is what makes this bind every verb including the three uncapped readers — and, over it, answers that ONE request with `ok:false` naming the size and the cap. The socket, and every sibling MCP on it, is untouched. Measuring before sealing means the refusal takes the seq the original would have had, so there is no gap either. - `MAX_PAYLOAD_BYTES` is `MAX_FRAME_BYTES`. A conforming sender no longer reaches it at all; what is left is a backstop against a sender that is not this extension, and it still takes 58% off what `ws` would let an unidentified local peer allocate. Its test now injects a small cap, because the behaviour under test is the close and not the size of the constant. Second, the extension's replay counter now agrees with `peer.ts`: `openEncryptedFrame` throws for BOTH stages, so `onEncryptedFrame` treated an authenticated-but-malformed frame as if it had never arrived and left its seq replayable. It uses `openEncryptedFrameDetailed` and commits for every outcome except `decrypt-failed`, which is the rule `peer.ts` states — a frame that opened under the live session key was sent by whoever holds that key, so its seq is spent whatever validation then says. A validation failure is also now logged on its own channel rather than in the bucket every stale-key straggler lands in; no synthetic reply goes back, because the extension is the responder here and has no pending call of its own to fail. The refusal also keeps the original frame's `op` echo when what it replaces was already an `ok:false` response, which the first cut dropped — the `ok:true` branch preserved it, and narrowing the other told the peer less than the frame it stands in for did. `npm test` 125 files / 1622 tests, `npm run typecheck` and `npm run build` clean. Both fixes were written failing first and each was re-checked against a reverted fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Tar4Eh59YtFuxQy4BpRBQ
|
Claude finished @chrischall's task in 6m 37s —— View job Re-review — PR #356 (after the frame-cap commit)
Both findings from the last pass are properly resolved. One 🔴 remains, and it is one of the two residuals the description records as deliberate:
Three 🟡 nits, all inline: the peer's client socket still has no persistent The other recorded residual — the host still tearing the socket down on a frame it cannot open — reads as a fair scope call. Verification note: dependencies are not installed in this checkout ( Verdict: fail |
| this.lastInboundSeq = seq; | ||
| return true; | ||
| /** Would this seq be accepted right now? Asks only — changes nothing. */ | ||
| isFreshInboundSeq(seq: number): boolean { |
There was a problem hiding this comment.
🔴 The same-burst window the description records as "inherent to the split" is neither inherent nor pre-existing — it re-opens a replay hole the old atomic call closed, and at the extension that means a write verb runs twice.
The code this replaces was a check-and-set:
acceptInboundSeq(seq: number): boolean {
if (seq <= this.lastInbound) return false;
this.lastInbound = seq; // synchronous, before any await
return true;
}Two identical frames in one read: the first set lastInbound = N, the second saw N <= N and was dropped. After the split, neither has committed when the second is dispatched, because the commit sits behind await openEncryptedFrame* while 'message' dispatch is synchronous per frame — which is exactly the property this PR's own host test relies on and states (packages/server/tests/replay-counter-after-auth.test.ts:114-118): "One write, so both frames reach the host's receiver in the same pass and the second is dispatched while the first is still awaiting its (failing) decrypt." So this is not a narrow race you need luck to hit; duplicating the frame in one write hits it every time.
Failure scenario. A concentrator host (T3 / T-host-MITM) or anything else that can write to the link re-sends one captured request frame twice in a single write. Both pass isFreshInboundSeq (line 35 / extension-core/src/session-keys.ts:30), both authenticate under the live key, both reach handleRequest — and packages/extension-core/src/background/handlers/dispatch.ts has no per-id in-flight guard (rg -n "inFlight|dedup|seen" …/dispatch.ts returns nothing). A replayed write_cookies writes twice; a replayed non-GET fetch submits the POST twice. That is the thing docs/SECURITY.md §T9 — amended by this PR — still promises is impossible: "Receivers reject any frame whose seq is <= lastInbound … A replayed frame from earlier in the session is dropped."
And it is fixable without giving back what the split bought. Rolling lastInbound back on failure would reintroduce the original bug (an in-flight forged seq: 2**40 would reject the genuine frames behind it while it is still open). An in-flight set avoids both:
isFreshInboundSeq(seq: number): boolean {
return seq > this.lastInboundSeq && !this.inFlight.has(seq);
}
reserveInboundSeq(seq: number): void { this.inFlight.add(seq); }
commitInboundSeq(seq: number): void {
this.inFlight.delete(seq);
if (seq > this.lastInboundSeq) this.lastInboundSeq = seq;
}
releaseInboundSeq(seq: number): void { this.inFlight.delete(seq); }lastInbound still moves only after authentication, so the wedge this PR fixes stays fixed, and a duplicate in the same burst is refused again. Serialising per-session frame handling behind a promise chain is the other shape, and has the side benefit of restoring in-order delivery.
| // emitting here (WS_ERR_UNSUPPORTED_MESSAGE_LENGTH) before closing with | ||
| // 1009 — so a peer could kill the host by sending one big frame. `ws` | ||
| // closes the socket itself; there is nothing to do but say so. | ||
| ws.on('error', (e) => { |
There was a problem hiding this comment.
🟡 Nit: the reasoning in this comment — "unhandled, it is an uncaught exception that takes the whole MCP process down" — applies equally to the peer's client socket, which still has no persistent handler. The only 'error' listener on it is the connect-time one, and it is a once that the handshake consumes:
$ grep -an "'error'" packages/server/src/peer.ts
147: ws.once('error', reject);
After open resolves, that listener is gone, so any later emit on the peer's socket (an ECONNRESET from a host that exits, a frame ws considers malformed) is an unhandled 'error' on an EventEmitter. Pre-existing rather than introduced here, but it is the same crash class and the same two lines to close.
| * request instead. What is left here is the backstop for a sender that is not | ||
| * the extension. | ||
| */ | ||
| export const MAX_PAYLOAD_BYTES = MAX_FRAME_BYTES; |
There was a problem hiding this comment.
🟡 Nit: MAX_FRAME_BYTES is documented in seal.ts as "the largest frame a conforming end of this protocol may put on the wire", and the extension is now a conforming end — but the server's two senders are not measured against it:
$ rg -n "sealInnerFrame" packages/server/src packages/extension-core/src | grep -v tests
packages/server/src/peer.ts:496: const sealed = await sealInnerFrame(
packages/server/src/host.ts:672: const sealed = await sealInnerFrame(
packages/extension-core/src/background/send-inner.ts:94: const sealed = await sealInnerFrame(entry.sessionKey, mcpId, entry.nextOutboundSeq(), toSend);
host.ts:672 is harmless (the browser end has no configurable maxPayload), but peer.ts:496 now writes into a socket capped at 42 MiB, and MAX_REQUEST_BODY_BYTES is a content-script check — it bounds what the extension will relay, not what an MCP may put in a request frame. An MCP that tries to POST a >42 MiB body therefore gets its own peer link closed with 1009 plus the console.warn added above, rather than the per-request failure this PR gives the extension for the mirror case. Implausible body size, self-inflicted, and only that MCP's link — hence a nit — but the same sealedFrameWireBytes guard at peer.ts:496 would make the rule hold at both ends.
| // dropped frame. `Number.MAX_SAFE_INTEGER` stands in for the seq not yet | ||
| // claimed — it is the widest this session could ever reach, so the | ||
| // measurement is at or above the frame that actually goes out, never below. | ||
| const wireBytes = sealedFrameWireBytes(mcpId, Number.MAX_SAFE_INTEGER, inner); |
There was a problem hiding this comment.
🟡 Nit (efficiency): measuring here does the whole serialisation twice for every frame. sealedFrameWireBytes runs enc.encode(JSON.stringify(inner)) (packages/protocol/src/seal.ts:45), throws the bytes away, and then sealInnerFrame runs the identical enc.encode(JSON.stringify(inner)) (seal.ts:107). For the frame this cap exists for — a 5 MiB body — that is a second multi-megabyte JSON string plus a second Uint8Array of up to ~31 MB, allocated in an MV3 service worker, on the common path where the frame fits and nothing was wrong with it.
Having sealedFrameWireBytes (or a sibling) hand back the encoded plaintext so sealInnerFrame can take it instead of re-deriving it would keep the exactness the measurement needs — it is the same bytes either way — at one pass.
… not after The two-call inbound gate from cdc5da5 put an `await` between the question and the answer. `isFreshInboundSeq` changes nothing, so two identical frames read in ONE pass of the WS receiver both got their yes before either could commit, and both were processed — deterministic, not a race needing luck. On the host and the peer that is a duplicated inner frame; on the extension it is worse, because that end is the RESPONDER and `handlers/dispatch.ts` has no per-id guard, so a repeated `write_cookies` or non-GET `fetch` EXECUTED TWICE. The gate is now a synchronous CLAIM taken before any await, held in an in-flight `Set` beside the high-water mark: `claimInboundSeq` takes the seq out of circulation the instant the frame is read, `commitInboundSeq` spends it once the frame has authenticated, `releaseInboundSeq` gives it back when it has not — so the property the split exists for survives intact, a frame that never authenticated still leaves the counter where it was and the genuine frames behind it still land. Every claim is released or committed by the caller that made it, including from a `catch` around the open, and the set is bounded (1024) so frames that never open cannot grow it without limit; the bound commits nothing, so the seq it drops is still taken once the flood drains. `isFreshInboundSeq` is gone rather than left beside the claim. Tests at all three sites deliver the two copies with nothing awaited between them — one corked TCP write on the server sides, two synchronous emits on the extension's — and assert exactly one is processed; each was re-checked against a reverted fix and fails there. The unit tests pin the claim's exclusivity, the released claim, and the bound. Three nits from the same round: - `peer.ts` had no persistent `'error'` listener: the handshake's `once('error', reject)` stayed attached and swallowed the first later emit into a settled promise, and the SECOND was an unhandled EventEmitter error that kills the MCP process — the crash class `host.ts` already guards. Both handshake listeners now come off when either fires, and the persistent listener mirrors the host's. - The MCP side did not measure its own outbound frames, so an oversize request met `maxPayload` as a 1009 CLOSE — on the host's socket that is every MCP's bridge, on a peer's it is the only link — instead of the per-request failure the extension already gives. `server/src/frame-size.ts` refuses it before a seq is claimed, so nothing is spent and no gap is left. - `sendInner` serialised every frame twice (`sealedFrameWireBytes` built and threw away the bytes `sealInnerFrame` then rebuilt): a second multi-MB string plus up to ~31 MB of `Uint8Array` inside an MV3 service worker. `encodeInnerFrame` is now exported and both functions accept its output, so the bytes measured ARE the bytes encrypted — exactness is a property of construction rather than of two calls agreeing, and a protocol test pins the number unchanged for the object and the plaintext across every case. And three ways the suite itself was not reliably green — none of them a statement about the code under test, all of them worth a fix rather than a retry: - A case that seals a frame AT the cap moves ~42 MiB through JSON.stringify, AES-GCM and base64. That is 1-2 s of CPU alone and 5837 ms under the full suite's 129 workers, against vitest's default 5 s — a budget sized for a test that is WAITING, not one that is WORKING. The three such cases (one here, two in `frame-size-cap.test.ts`) carry a 30 s budget of their own, so every other test keeps the tight default and a 5 s test there still means something is wrong. - A test `WebSocketServer` bound the WILDCARD address. macOS lets a later, more specific bind take `127.0.0.1:<that port>` out from under it and win every loopback dial, so a peer test could sit watching another test file's server answer its own client: 'Unexpected server response: 200' where that server refuses the upgrade, a 5 s timeout where it simply never replies. Both were seen in full-suite runs. `loopbackWss()` binds the loopback address, so the port is genuinely taken on the address the client dials, and every construction site in the package goes through it. - `MockExtension.closed()` attached its `'close'` listener at the moment it was asked, so a close that had already arrived woke nobody. The host answers a forged `ready` by closing 1008 within ~5 ms, and this round's `completeHandshake` awaits an ECDH and an HKDF after the send, which is what started `extension-pin-host.test.ts` timing out under load. The close is latched instead, and a new test pins that asking late — twice — is still an answer. 45 consecutive full-suite runs clean after those three, where the run that opened this round failed 1 in 5. `npm test` 130 files / 1639 tests, `npm run typecheck` and `npm run build` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Tar4Eh59YtFuxQy4BpRBQ
|
Claude finished @chrischall's task in 5m 22s —— View job Re-review — PR #356 (after the seq-claim commit)
The 🔴 is resolved, and so are all three 🟡 nits. Two 🟡 nits, neither blocking: the peer's client socket still has no Not done: Verdict: warn |
| @@ -143,8 +144,35 @@ const enc = new TextEncoder(); | |||
| export async function startPeer(opts: PeerOpts): Promise<InternalPeerHandle> { | |||
| const ws = new WebSocket(`ws://${opts.host}:${opts.port}`); | |||
There was a problem hiding this comment.
🟡 Nit: the peer's client socket is constructed without maxPayload, so the host→peer direction is still at ws's 100 MiB default while peer→host is now capped at 42 MiB. The 'error' listener added just below was justified as "Mirrors host.ts"; the cap it exists to make survivable isn't mirrored:
$ grep -rn "maxPayload" packages/server/src
packages/server/src/host.ts:130: maxPayloadBytes?: number;
packages/server/src/host.ts:162: maxPayload: opts.maxPayloadBytes ?? MAX_PAYLOAD_BYTES,
packages/server/src/host.ts:689: // `maxPayload` as a 1009 close would drop every sibling's bridge to
packages/server/src/peer.ts:542: // `maxPayload` as a 1009 CLOSE, taking this peer's only link to the
packages/server/src/frame-size.ts:5: * `ws` answers a payload over its `maxPayload` by CLOSING the socket with
The host-side rationale — "ws defaults to 100 MiB, which is a lot of process memory a local peer can make the host allocate before a single byte is validated" (host.ts:59-60) — reads the same way in reverse: a concentrator host is another local process, and under T-host-MITM it is exactly the party the 2.0.0 signature change treats as potentially hostile. new WebSocket(url, { maxPayload: MAX_PAYLOAD_BYTES }) here would make the budget symmetric; the error listener directly below already handles the 1009 that follows. Not introduced by this PR (the peer never had a cap), which is why it's a nit rather than a regression.
| if (seq <= this.lastInboundSeq) return false; | ||
| this.lastInboundSeq = seq; | ||
| if (this.inflightInbound.has(seq)) return false; | ||
| if (this.inflightInbound.size >= MAX_INFLIGHT_INBOUND_SEQS) return false; |
There was a problem hiding this comment.
🟡 Nit: the bound's refusal is indistinguishable from a replay refusal at every call site, and none of them logs it. All three callers do if (!claimInboundSeq(frame.seq)) return; (host.ts:537, peer.ts:439, extension-core/src/background/socket.ts:274), so a frame dropped because the in-flight set was full looks exactly like a frame dropped as a replay — and unlike a replay, that one was probably genuine. The MCP waiting on it sees only its own request timeout.
Every other drop on these paths says something: console.warn('[fetchproxy] dropped malformed frame:', e) (socket.ts:248), console.warn('[fetchproxy] decrypt failed:', opened.error) (socket.ts:303), console.error('[fetchproxy] host: message handler error:', e) (host.ts:607). Returning a discriminated result ('taken' | 'replay' | 'saturated'), or logging once from inside claimInboundSeq when the size check is what refused, would keep the one drop that indicates load rather than attack diagnosable.
Agreed the bound is unreachable under real traffic — one AES-GCM open per claim and the set self-drains — which is what makes this a nit rather than a finding.
Re-review — PR #356 (after the seq-claim commit)The 🔴 from the last pass is resolved, and all three 🟡 nits with it. The property the split bought is intact and pinned separately: a released claim leaves The other three nits are all addressed, not just acknowledged: Spot-checks on the surrounding arithmetic: Two 🟡 nits, both inline, neither blocking:
The two residuals the description records — the host tearing the socket down on a frame it cannot open, and Verification note: Verdict: warn |
chrischall
added
the
ready-to-merge
…les, and profile writes that survive an interrupt (#360) PR H3 of the mcp-host single-tier plan, from `docs/beta-review-2026-09-10/fetchproxy-cli-supply-chain.md`. Three small closes, each with a correctness reason. **A session cookie no longer has to sit in a shell command.** `fpx write-cookies` took `name=value` pairs on the command line, so the value landed in shell history, in `ps` output, and in anything that reads `/proc/<pid>/cmdline`. It now accepts `name=@file` and `--from-stdin`. The file form strips exactly one trailing newline and says so, because a cookie carrying a stray newline fails in a way that is very hard to see. **The CLI and the server disagreed about the same profile.** `assertUrlOnProfile` compared the URL's `host`, which includes the port, while both real enforcers on the server compare `hostname`, which does not — so a profile declaring `example.com` refused `https://example.com:8443/…` on the CLI and accepted it on the server. The server's rule is the one the protocol enforces, so the CLI now follows it; I checked which was authoritative rather than assuming, and re-verified both enforcer call sites after merging main, since that merge touched `packages/server/src`. **An interrupted profile write destroyed the identity it held.** Writing in place truncates the file the moment the write begins, so a crash, a full disk or a `^C` left a truncated or empty profile. Every profile write path now goes through a temporary file in the same directory and a `rename`. `npm test` 131 files / 1665 tests, `npm run typecheck` and `npm run build` all green after merging #356, which the branch does not overlap. Two residuals recorded rather than fixed, both outside the three tasks: the stdin read has no deadline, so a non-TTY descriptor that never closes makes `--from-stdin` wait silently; and `tsx` is an undeclared dependency of `packages/cli` that resolves only through workspace hoisting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_015Tar4Eh59YtFuxQy4BpRBQ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Group A2 of the mcp-host beta-readiness backlog (
docs/beta-review-2026-09-10/fetchproxy-bridge.md), plus two rounds of auto-review fixes.The replay counter advanced before authentication at three sites — the concentrator host, the peer, and the extension's background socket. One forged frame carrying a high
seqwedged the session on two of them and forced a re-handshake on the third, with no key needed.Getting that right took two attempts, and the second is the one that matters. The first split the atomic
acceptInboundSeqinto ask-then-commit, which put anawaitbetween the freshness check and the counter move — so two identical frames arriving in one read both passed the check. That is deterministic, not a race needing luck, and at the extensiondispatch.tshas no per-request guard, so a duplicatedwrite_cookiesor non-GETfetchexecuted twice. Trading a wedge for a double write is not a fix.A seq is now CLAIMED synchronously, before any await, in an in-flight set beside the high-water mark. The claim is released on an authentication failure — that frame never happened, so its seq stays open — and converted into the high-water mark when the frame authenticates, whether or not it then validates. The tests deliver both copies in one real burst: the server sides cork the TCP socket so both frames land in one write, the extension emits two synchronous callbacks. Against the pre-fix code all three fail; the extension case was checked at the side effect rather than the reply, counting
chrome.cookies.set— two calls before, one after.A socket that never says hello now closes.
docs/SECURITY.mdhas claimed a 15-second handshake timeout since the concentrator shipped and nothing implemented it.HANDSHAKE_TIMEOUT_MScloses it with 1008; a socket that finishes the hello inside the window is untouched.The frame cap, derived rather than picked. The first cut capped
maxPayloadat 8 MiB, reasoning from the extension's 5 MiB body limit. Wrong twice: the limit counts UTF-16 code units, not bytes, andread_indexed_db,read_local_storage,read_session_storageandread_domhave no limit at all — so a large non-ASCII or storage answer exceeds 8 MiB legitimately. Worse,wsanswers an oversized payload by closing with 1009, and that is the one socket the extension holds for the whole concentrator: a single big DOM read would have dropped every MCP on it.So the number comes from written-down arithmetic — 5 MiB of UTF-16 units at the JSON worst case of six bytes per unit, plus inner overhead and the GCM tag, base64-expanded, plus the envelope, is 42,030,956 bytes, and
MAX_FRAME_BYTESis 44,040,192 — with a test that re-does the arithmetic rather than restating the constant. Enforcement moved to the producing end, on both sides: the extension'ssendInnerand the server's two outbound call sites measure before sealing and refuse that one request, leaving the socket open and every other MCP working. The refusal spends exactly one seq, so there is no gap. Measuring no longer serialises the frame twice, which on the common path was a second multi-megabyte string in an MV3 service worker.Also fixed: the peer's client socket had no persistent
'error'listener, so a post-handshake emit was an unhandled EventEmitter error that killed the MCP process.One behaviour change worth naming: a storage or DOM read larger than 42 MiB now fails that single request with
response too large for one bridge frame, where on released 2.11.3 — hostmaxPayloadat ws's 100 MiB default — it would have gone through.Two test-infrastructure fixes ride along because this PR's own tests exposed them: a wildcard
WebSocketServer({port: 0})on macOS can have its loopback address taken by a later, more specific bind, which then answers the dial; and the mock extension attached its close listener on the spot, too late for a 1008 the host now sends within milliseconds.npm test130 files / 1639 tests green across 45 consecutive runs,npm run typecheckandnpm run buildclean.Closes #357
🤖 Generated with Claude Code
https://claude.ai/code/session_015Tar4Eh59YtFuxQy4BpRBQ