feat(graphql): route declared GraphQL ops through the tab's own Apollo client - #178
Conversation
…o client Adds a new opt-in `graphql` capability so an MCP can invoke a page-declared GraphQL operation through the matched tab's own window.__APOLLO_CLIENT__ in the MAIN world, instead of the isolated-world fetch() path. Some endpoints (OpenTable's RestaurantsAvailability) reject the isolated-world fetch at the edge because bot-detection telemetry (Akamai) lives inside the page's own Apollo link chain, not on window.fetch — routing through the real client clears it, verified live against opentable.com. The extension carries no hardcoded query text or persisted-query hash: it captures the live DocumentNode the page's Apollo client already observed for a declared operationName and reuses it, so it auto-adapts when a site revises its query. Gated by capability + a declared graphqlOps allowlist (approved and diffed at pair time, like every other capability) + the existing domain allowlist and host-or-subdomain tab match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9163f09 to
d9a0313
Compare
github-actions
Bot
added
the
auto-review
|
Claude finished @chrischall's task in 5m 26s —— View job Code review — PR #178 (
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| query: doc, | ||
| variables: safeVars, | ||
| fetchPolicy: 'no-cache', | ||
| errorPolicy: 'all', |
There was a problem hiding this comment.
🔴 Important — a GraphQL-level error tears down the whole bridge, not just this call.
errorPolicy: 'all' is specifically the policy under which Apollo does not throw on GraphQL errors — client.query resolves with { data: null | undefined, errors: [...] } (e.g. an expired session, or any error on a non-nullable root field). That path lands in the ok: true branch below:
post(win, { ok: true, data: res?.data })→dataisnull/undefinedcontent.tsrunGraphqlQueryaccepts it (JSON.stringify(null)passes the size guard) →{ ok: true, data: null }background.tshandleGraphqlQueryRequest→sendInner({ ok: true, op: 'graphql_query', data: null })- server
openEncryptedFrame→validateInnerResponse→packages/protocol/src/validate.ts:1052→assertObject(raw.data)rejectsnull(andundefinedhits theinner.data: missingbranch) host.ts:344catches theProtocolErrorand doesws.close(1011, 'internal error')— the extension WebSocket is torn down for every MCP on the concentrator, not just the caller.
This also contradicts the doc added in this PR (docs/PROTOCOL.md): "a GraphQL-level error surfaces as an ok: false protocol failure instead" — as written it surfaces as ok: true, data: null and kills the socket.
Suggested fix: handle it here, where the errors array is still in hand — if res.errors?.length or data == null, post { ok: false, error: <serialized errors> }. That also fixes the related loss noted below: res.errors is currently dropped entirely, so a partial-data response reaches the MCP with no indication anything failed.
There's no test coverage for this — capture-logger-graphql.test.ts:128 uses makeClient(null) but pairs it with mockRejectedValueOnce, so the resolved-null path is never exercised.
| capture_request_header: { label: 'Capture request header', warn: true }, | ||
| read_indexed_db: { label: 'Read IndexedDB', warn: true }, | ||
| read_dom: { label: 'Read DOM elements', warn: true }, | ||
| graphql: { label: 'Run declared GraphQL queries', warn: true }, |
There was a problem hiding this comment.
🔴 Important — the popup never renders graphqlOps, but three docs in this PR say it does.
This capability label is the only popup change. There is no appendGraphqlOpsSubList alongside appendDomSelectorsSubList(dl, pending.domSelectors) (line 647), and no graphqlOps entry in appendDiffSummary's diffLists block (lines ~280-320). Two consequences:
-
Pair-time consent is blind. The user sees
Run declared GraphQL queries ⚠️and approves — but never sees which operations. That contradicts the file's own stated invariant at line ~636: "the user approves the exact set of names, not just 'this MCP can read storage' — so the pair popup MUST show them", and the docs added in this PR:docs/SECURITY.md§T-graphql-misuse defense fix(extension): chrome.alarms keepalive so MV3 SW doesn't sleep between MCP calls #2: "the popup surfaces the exactoperationNamevalues verbatim so the user sees precisely what will run"docs/PROTOCOL.md: "the popup lists every declaredoperationNameverbatim" / "the popup shows the declared operations verbatim"packages/protocol/src/frames.ts: "Elevated — surfaces the declared operations in the pair popup"
-
The scope-update diff is empty when only
graphqlOpschanges.scopeHashandisScopeSubsetdo covergraphqlOps(lib/scope.ts), so adding an operation correctly fires the re-pair/scope-update prompt — but sinceappendDiffSummarydoesn't diff it, the user gets "wants to expand its access" with nothing at all in "Now requesting (new)". An unreviewable consent prompt is worse than no prompt.
The approval payload itself is fine (approvedPair: pending writes the raw storage record, and normalisePendingPair passes unknown fields through), so this is purely the display layer — but it's the layer the whole capability's security story rests on.
| }); | ||
|
|
||
| if (!tryWrap()) { | ||
| const timer = win.setInterval(() => { | ||
| if (tryWrap()) win.clearInterval(timer); | ||
| }, APOLLO_POLL_INTERVAL_MS); | ||
| } |
There was a problem hiding this comment.
🟡 Nit — the Apollo poll never gives up, on every page in every tab.
capture-logger.js is registered at <all_urls>, so on the vast majority of pages __APOLLO_CLIENT__ never appears and this interval fires every 500ms for the lifetime of the tab. The existing setInterval(syncCsrf, 2000) sets a precedent for a forever-timer, but this one is 4× the cadence and is pure waste on non-Apollo pages. Consider capping attempts (e.g. stop after ~60s) or backing off, and/or using document.readyState/a load deadline as the give-up signal.
Related: recordDocsFromLink monkey-patches client.link.request on every page that exposes an Apollo client, regardless of whether any MCP has declared the graphql capability. The captured DocumentNodes stay in an in-process Map and aren't exfiltrated, so this isn't a data-leak, but it does mean the extension mutates page internals on sites no MCP is scoped to — a wider footprint than the read-only CSRF sync this file previously did. Worth a note in docs/SECURITY.md §T-graphql-misuse at minimum.
|
🟡 Auto-review verdict: warn — The new commit 93d883c (peer.ts decrypt-vs-validation split via openEncryptedFrameDetailed) is correct, well-scoped, and properly tested, and all four prior blocking findings remain fixed on HEAD. One new nit plus three carried-over nits; nothing blocking. |
…er declared ops in the pair popup Addresses PR #178's auto-review fail verdict (two important findings): 1. capture-logger.ts's handleGraphqlRequest used errorPolicy:'all', under which Apollo resolves {data:null, errors} instead of throwing on a GraphQL-level error. That null data was posted as ok:true, which fails validateInnerResponse's assertObject server-side and, via host.ts's catch-all, closed the extension WebSocket for every MCP on the concentrator. Now resolved-with-errors (or resolved-with-no-data) posts ok:false with the serialized GraphQL errors instead. Fixes the PR's own doc claim ("a GraphQL-level error surfaces as an ok:false protocol failure") and stops errors from being silently dropped. 2. popup.ts never rendered the declared graphqlOps allowlist: no appendGraphqlOpsSubList, and no graphqlOps entry in appendDiffSummary's diff. Users approved an unseen operation list, and a graphqlOps-only scope-update showed an empty diff despite scopeHash/isScopeSubset already gating on it. Threaded graphqlOps through PreviousScope, ScopeSnapshot, PendingPair, and the two local pending-record types; added the sub-list renderer and diff entries, mirroring appendDomSelectorsSubList exactly. Left open (tracked in the follow-up issue, non-blocking nit): the Apollo poll and link-wrap run on every page regardless of whether any MCP declared graphql — noted in SECURITY.md §T-graphql-misuse as a known, non-security-relevant limitation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @chrischall's task in 4m 18s —— View job Code review — PR #178 (
|
| assertStringMap(raw.values, 'inner.values'); | ||
| return raw as unknown as InnerFrame; | ||
| } | ||
| if (op === 'graphql_query') { |
There was a problem hiding this comment.
🔴 Important — every ok: false graphql_query response fails validation and tears down the whole bridge.
The success branch was extended for op: 'graphql_query' (here), but the error branch below (validate.ts:1092-1101) still gates raw.op on KNOWN_CAPABILITIES:
if (raw.ok === false) {
assertString(raw.error, 'inner.error');
if (raw.op !== undefined) {
if (typeof raw.op !== 'string' || !KNOWN_CAPABILITIES.has(raw.op as Capability)) {
throw new ProtocolError(`inner.op: unknown response op ${JSON.stringify(raw.op)}`);
}
}KNOWN_CAPABILITIES (frames.ts:128-139) gained 'graphql' in this PR — not 'graphql_query'. But the extension sends op: 'graphql_query' on every graphql failure path:
background.ts:1367— capability gate (op: req.op, andreq.opisgraphql_query)background.tshandleGraphqlQueryRequest— gate failure,no-tab,throw, and the bridge's ownok:falserelay: all four literallyop: 'graphql_query'
So the chain is exactly the failure mode fixed in 5609045 for data: null, just relocated to the error path:
- extension →
{ ok: false, op: 'graphql_query', error: 'operation RestaurantsAvailability not yet observed on this tab — open a restaurant page and retry' } - server
openEncryptedFrame(seal.ts:49) →validateInnerFrame→ProtocolError host.ts:342-351catch-all →ws.close(1011, 'internal error')— the extension WebSocket dies for every MCP on the concentrator
This isn't an exotic path: the "not yet observed on this tab" error is the documented first-run case (capture-logger.ts:160 tells the user to open a restaurant page and retry). The first retry-worthy error kills the bridge.
frames.ts:832 already widened the type (op?: Capability | 'graphql_query') for precisely this — the runtime validator just wasn't widened to match, which is the CLAUDE.md rule ("Don't merge feature work that adds protocol fields without updating packages/protocol/src/validate.ts validators").
Suggested fix: accept 'graphql_query' in the error branch (e.g. a KNOWN_RESPONSE_OPS set = KNOWN_CAPABILITIES ∪ {'graphql_query'}) so it mirrors the widened type.
Note the one existing test here asserts the wrong string — validate.test.ts:2340 uses op: 'graphql', which nothing on the wire ever sends, so it passes while the real shape is uncovered. Worth adding both: op: 'graphql_query' accepted, plus an integration case where the mock extension replies ok:false, op:'graphql_query' and graphqlQuery() rejects with the bridge error while the socket stays open.
…can kill the whole bridge Audit requested after PR #178's graphql fix: host.ts's ws.on('message', ...) handler wraps its ENTIRE body in one try/catch. openEncryptedFrame calls validateInnerFrame on every decrypted inner frame for every op (not just graphql_query) — any ProtocolError it throws propagates to that catch-all, which closes the whole extension WebSocket for every MCP on the concentrator, not just the offending call. This is the same failure class as the graphql bug, just triggered by a different op. Found one other concrete trigger: chrome.downloads.DownloadItem.fileSize is documented to be -1 when the size is unknown (e.g. no Content-Length from the server). handleDownloadRequest's succeed() path forwarded it unconditionally as bytes:-1, and the download response validator requires bytes >= 0 — the same "extension forwards an unchecked external sentinel as ok:true, strict validator rejects it, whole bridge dies" pattern. Clamped to 0 in downloadValueFromItem (the file did save; only its size is unreported), with a regression test and an updated DownloadResult doc comment. Every other op's extension-side handler was audited and confirmed safe: fetch/read_cookies/read_local_storage/read_session_storage/ capture_request_header/capture_redirect/read_indexed_db/read_dom either guarantee well-typed values structurally or explicitly guard before resolving ok:true. peer.ts is architecturally NOT vulnerable to this failure mode — its openEncryptedFrame call has its own inner try/catch that silently drops a bad frame instead of closing the connection (a different, quieter risk: a real validation bug there would swallow a legitimate response rather than error loudly — not fixed here, out of scope of "kill the whole bridge"). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @chrischall's task in 3m 57s —— View job Code review — PR #178 (
|
| // are already-parsed JSON on the receiving side. | ||
| assertObject(raw.data, 'inner.data'); | ||
| return raw as unknown as InnerFrame; | ||
| } |
There was a problem hiding this comment.
🔴 Important — still unfixed on 8883bd0: every ok:false graphql_query response fails validation and closes the shared WebSocket. (re-raise of #discussion_r3678280467 — the new commit audited the download op instead, so this one is untouched.)
The success branch above learned op: 'graphql_query'. The error branch (validate.ts:1092-1101, verified on HEAD) did not:
if (raw.ok === false) {
assertString(raw.error, 'inner.error');
if (raw.op !== undefined) {
if (typeof raw.op !== 'string' || !KNOWN_CAPABILITIES.has(raw.op as Capability)) {
throw new ProtocolError(`inner.op: unknown response op ${JSON.stringify(raw.op)}`);
}
}KNOWN_CAPABILITIES (frames.ts:128-139) gained 'graphql' in this PR — not 'graphql_query'. The extension sends op: 'graphql_query' on all five graphql failure paths, verified on HEAD:
background.ts:1367— capability gate (op: req.op, wherereq.op === 'graphql_query')background.ts:2517gate failure ·:2545no-tab·:2555throw·:2576bridge-relayedok:false— all four literalop: 'graphql_query'
Chain: {ok:false, op:'graphql_query', error} → openEncryptedFrame → validateInnerFrame → ProtocolError → host.ts:342-351 catch-all → ws.close(1011). That is exactly the failure class 8883bd0's own commit message describes ("any ProtocolError it throws propagates to that catch-all, which closes the whole extension WebSocket for every MCP on the concentrator") — the graphql error path is the instance that message was written about, and it's still open.
It's also the documented first-run route: capture-logger.ts:160's operation … not yet observed on this tab — open a restaurant page and retry is an ok:false with that op. The retryable error kills the bridge instead of returning a retryable message.
frames.ts:832 already widened the type (op?: Capability | 'graphql_query'); the runtime validator wasn't widened to match — CLAUDE.md: "Don't merge feature work that adds protocol fields without updating packages/protocol/src/validate.ts validators (every inbound frame is validated before dispatch)."
Fix: a KNOWN_RESPONSE_OPS set (KNOWN_CAPABILITIES ∪ {'graphql_query'}) used in this branch.
Test gap unchanged: validate.test.ts:2340 (accepts a graphql error response (ok:false, op graphql)) asserts op: 'graphql', a string nothing on the wire ever sends, so it passes while the real shape is uncovered.
| finish({ ok: false, error: `graphql response too large: ${serialized.length} bytes` }); | ||
| return; | ||
| } | ||
| finish({ ok: true, data: data.data }); |
There was a problem hiding this comment.
🟡 Nit — the isolated-world relay doesn't re-guard data == null before resolving ok: true.
5609045 correctly closed this in capture-logger.ts (the honest MAIN-world bridge now posts ok:false when res.data is null/undefined), so the practical route is gone. But this side accepts ok: true with whatever data the message carried — JSON.stringify(null) → "null", which passes the size guard — and background.ts:2568 then relays ok:true, data:null to the server, where assertObject(raw.data) throws and host.ts closes the bridge for every MCP.
The remaining trigger is a page script (any script running in the tab's MAIN world, not just the site's own) posting {__fetchproxy:'graphql-res', reqId:1, ok:true, data:null}. That's a deliberate spoof, not an accident, and such a script already controls __APOLLO_CLIENT__ — but a concentrator-wide bridge kill is reach beyond that one tab, and 8883bd0's own audit standard for this class is "explicitly guard before resolving ok:true". A one-line if (data.data === null || data.data === undefined) { finish({ok:false, error:'graphql bridge returned no data'}); return; } here makes the invariant hold independently of the MAIN-world peer, the same way the size guards already don't trust it.
…rResponse The re-review on 8883bd0 found the real blocker: graphql_query is the one op whose wire op string ('graphql_query') differs from its governing capability name ('graphql') — InnerResponseError.op's type was already widened for this (op?: Capability | 'graphql_query'), but the runtime validator still gated on KNOWN_CAPABILITIES alone, which doesn't contain 'graphql_query'. So every ok:false graphql_query response — including the documented, expected-on-first-run "operation not yet observed on this tab" error — failed validation and, via host.ts's message-handler catch-all, closed the extension WebSocket for every MCP on the concentrator. Same failure class as the two bugs already fixed in this PR, on the op the whole PR is about. Fixed with a KNOWN_RESPONSE_OPS set (KNOWN_CAPABILITIES + 'graphql_query'), per the review's exact suggestion. Replaced the one existing test that looked like coverage but asserted the WRONG string (op:'graphql', which nothing on the wire sends — it passed only because 'graphql' happens to already be in KNOWN_CAPABILITIES) with a test asserting the REAL wire shape, plus a regression test for a genuinely unknown op. Also fixed the accompanying nit: content.ts's isolated-world listener trusted ANY same-window postMessage matching the graphql-res shape, including a spoofed ok:true with null/undefined data from a page script sharing the same message bus — which would hit the same assertObject(raw.data) rejection and bridge-wide close. Added the same null/undefined guard capture-logger.ts already has, with a regression test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @chrischall's task in 3m 46s —— View job Code review — PR #178 (
|
| Round | Finding | Status |
|---|---|---|
| 1 | GraphQL-level error (errorPolicy:'all' → resolved {data:null, errors}) tearing down the bridge |
fixed in 5609045 — capture-logger.ts:174-205 branches on res.errors?.length then data == null, both with tests |
| 1 | Popup never rendering graphqlOps |
fixed in 5609045 — appendGraphqlOpsSubList at popup.ts:470, wired at :692, diffed in kept/added/removed at :374/:398/:424, threaded through all four scope types, tested |
| 1 | Uncapped Apollo poll + unconditional link.request wrap (nit) |
deferred, documented in SECURITY.md §T-graphql-misuse, tracked in #179 |
| 2/3 | ok:false op:'graphql_query' failing validation |
fixed in efc8d4e |
| 3 | content.ts not re-guarding data == null (nit) |
fixed in efc8d4e (see nit 1 below for the remaining edge) |
🟡 Nits (non-blocking, tracked)
1. The new spoof guard covers null/undefined but not the other shapes assertObject rejects. (inline — content.ts:213) assertObject (validate.ts:57-60) also rejects arrays and non-object primitives, so a spoofed {ok:true, data: []} / data:"x" still reaches the same ws.close(1011). Same reachability as the original nit (deliberate same-origin page script); the fix is one condition wider.
2. Vendor-specific instruction in a generic error string. (inline — capture-logger.ts:162) "open a restaurant page and retry" is the only OpenTable-specific string in extension-core's runtime output — every other vendor reference in the package is a comment — and it's relayed verbatim into any MCP's tool output.
3. Integration suite still only exercises ok: true. (inline — graphql-query.test.ts:45) The unit-level shape is now pinned, but no end-to-end case asserts graphqlQuery() rejects on ok:false while the socket stays open — the assertion with actual regression value, since validator / openEncryptedFrame / host.ts catch-all are three layers that must stay aligned.
Checked and fine
assertGraphqlOpsArray(validate.ts:435-469):SCOPE_KEY_REonname,GRAPHQL_OP_NAME_RE(GraphQLNamegrammar, ≤128) onoperationName, uniqueness, unexpected-field rejection; wired intovalidateHelloat:574. Request-sidegraphql_queryvalidator requires a plain-objectvariablesand rejects unexpectedinitfields; the "must be one of" error string was updated.- Gate order in
resolveGraphqlQueryRequest(capability → declaredname→ optionaltabUrldomain),graphqlTabMatcherhost-or-subdomain fallback, and the fact that only the resolvedoperationName— never the raw declaredname— crosses to the tab. scope.ts:sameGraphqlOps/normGraphqlOpuse the same\x00-joined canonical key as the adjacent decls; covered inscopeHash,intersectScope,isScopeSubset, with add/remove/reorder tests.trust-store.tsmigration defaultsgraphqlOpsto[]on legacy records;host.ts/peer.ts/build-server-hello.tsthread it through with the conditional-spread convention.recordDocsFromLinkidempotent (__fetchproxyWrapped), always calls through, best-effort recording in a try/catch;handleGraphqlRequestnever rejects;post()swallows a failed post-back.- No new
chrome.*API; theiifesplit +content-scripts-classic.test.tspinning still in place; no manual version bumps.
npm test / npm run build — npm ci isn't in this job's allowed tools and node_modules is absent on this checkout, so my verification is static reading only. The PR body reports 1042/1042 green, and the earlier commits' CI runs are on the PR.
Verdict: warn
· claude/zen-pasteur-858985
Completes the "kill the whole bridge" audit: host.ts's failure mode (one bad response closes the WS for every MCP) has a mirror-image risk on peer.ts's side that's just as real, only quieter. peer.ts's own openEncryptedFrame call had ONE catch treating two very different failures identically: a decrypt failure (wrong/stale session key — nothing recoverable, correctly dropped silently) and a validation failure AFTER a successful decrypt (the ciphertext authenticated under the CURRENT session key, proving the frame is from the live, legitimate host — but the plaintext fails the wire schema, e.g. the download bytes:-1 class of bug found earlier in this audit). Both were silently dropped with zero diagnostic signal, leaving whichever pending call awaited that response's id to hang until its own timeout instead of failing fast with a clear error. Added `openEncryptedFrameDetailed` in packages/protocol/src/seal.ts — a non-throwing sibling of openEncryptedFrame that returns which stage failed (decrypt-failed vs validation-failed) and, for the latter, recovers the malformed frame's numeric `id` when possible. Deliberately scoped: the existing throw-only openEncryptedFrame is UNCHANGED, so host.ts and the extension's own request-decode path keep their current behavior — only peer.ts adopts the detailed variant, since it's the one place a silent, undiagnosable swallow was happening. peer.ts now logs validation failures loudly and, when the id is recoverable, routes a synthetic ok:false through the normal id-keyed onInner dispatch so that ONE call fails immediately — without tearing down the connection, which decryption just proved is legitimate. Tests: seal.test.ts covers all four stages (ok / decrypt-failed / validation-failed-with-id / validation-failed-without-id, including non-JSON and array-top-level plaintext). peer.test.ts drives a REAL handshake (actual ECDH + HKDF derivation, matching the extension's side exactly) so the malformed frame genuinely decrypts, then asserts the targeted ok:false arrives via onInner AND that a subsequent valid frame still delivers normally — proving this degrades gracefully rather than tearing down the session. Documented in SECURITY.md's T-host-MITM section, including the honest residual: host.ts's blast radius (close-the-whole-socket on ANY validation failure) is unchanged by this commit — this PR's concrete triggers for that were each fixed at the source instead. A future op-specific bug could still trip it; known, accepted, not closed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| // would otherwise reach the server's assertObject(raw.data), | ||
| // which rejects null — and via host.ts's catch-all, that closes | ||
| // the extension WebSocket for every MCP on the concentrator. | ||
| if (data.data === null || data.data === undefined) { |
There was a problem hiding this comment.
🟡 Nit — the new spoof guard covers null/undefined but not the other shapes assertObject rejects.
The stated intent of this guard (and of 8883bd0's audit standard) is that the isolated world shouldn't trust the MAIN-world peer to produce a shape the server will accept. assertObject in validateInnerResponse (validate.ts:57-60) rejects arrays and every non-object primitive as well as null:
if (typeof x !== 'object' || x === null || Array.isArray(x)) throw new ProtocolError(...)So a spoofed {__fetchproxy:'graphql-res', reqId, ok:true, data: []} (or data: "x", data: 0) still passes this guard, passes the JSON.stringify size check, gets relayed verbatim by background.ts:2568, and lands on the same assertObject(raw.data) → ProtocolError → host.ts ws.close(1011) — the concentrator-wide kill this guard was added to prevent.
Same practical reachability as the original nit (deliberate same-origin page script, no privilege gain), so still a nit — but the fix is one condition wider:
| if (data.data === null || data.data === undefined) { | |
| if ( | |
| data.data === null || | |
| typeof data.data !== 'object' || | |
| Array.isArray(data.data) | |
| ) { |
The honest bridge never produces those shapes (GraphQL data is an object or null), so this costs nothing on the real path.
| __fetchproxy: RES_MARKER, | ||
| reqId, | ||
| ok: false, | ||
| error: `operation ${operationName} not yet observed on this tab — open a restaurant page and retry`, |
There was a problem hiding this comment.
🟡 Nit — vendor-specific instruction in a generic user-facing error string.
"open a restaurant page and retry" is the only OpenTable-specific string in extension-core's runtime output — every other vendor reference in this package (capture-logger.ts:14/18/23/28, content.ts:159, url-match.ts:46/59, ensure-domain-tab.ts:7) is in a comment, and every other error string the extension produces is vendor-neutral. This one is relayed verbatim through background.ts:2576 → sendInner → FetchproxyProtocolError → the calling MCP's tool output, so a HoneyBook or Canvas MCP hitting the same cold-start case tells its user to open a restaurant page.
Since the bridge already knows the operation name, something like operation ${operationName} not yet observed on this tab — load a page that runs it, then retry keeps the actionable part without the vendor coupling.
| await new Promise((r) => setTimeout(r, 50)); | ||
| }); | ||
|
|
||
| it('routes graphqlQuery through the bridge, pins graphqlOps, returns data', async () => { |
There was a problem hiding this comment.
🟡 Nit — the integration suite still only exercises the ok: true reply.
efc8d4e correctly fixed the validator and pinned the real wire shape at the unit level (validate.test.ts now asserts op: 'graphql_query', plus an unknown-op regression). What's still missing is the end-to-end assertion that closes the loop on the actual failure mode: this is the only it() in the file, and it only covers a successful reply.
The bug that shipped three commits deep was precisely "an ok:false graphql frame kills the socket", and nothing here would have caught it — a case where the mock extension replies {ok:false, op:'graphql_query', error:'operation … not yet observed on this tab'}, then asserts (a) graphqlQuery() rejects with that message and (b) the extension WebSocket is still open afterwards, would. That second assertion is the one with regression value, since the validator, openEncryptedFrame, and host.ts's catch-all are three separate layers that all have to stay aligned.
chrischall
added
the
ready-to-merge
|
Claude finished @chrischall's task in 3m 33s —— View job Code review — PR #178 (
|
| Round | Finding | Status on 93d883c |
|---|---|---|
| 1 | errorPolicy:'all' resolved {data:null, errors} tearing down the bridge |
fixed — capture-logger.ts:176-205 branches on res.errors?.length then data == null |
| 1 | Popup never rendering graphqlOps |
fixed — appendGraphqlOpsSubList, wired + diffed in kept/added/removed, tested |
| 2/3 | ok:false op:'graphql_query' failing validation |
fixed — KNOWN_RESPONSE_OPS at validate.ts:988, used at :1112 |
| 3 | content.ts not guarding data == null |
fixed — content.ts:213-216 (see nit 2) |
No new commit touched those files; verified the fixes are still present.
🟡 Nits (non-blocking)
1. openEncryptedFrameDetailed's two fromB64 calls sit outside every try. (inline — seal.ts:93-94) fromB64 is documented as throwing on invalid base64, and BASE64_RE (validate.ts:20) doesn't enforce a length multiple of 4 — so iv: "A" passes validateEncrypted and then throws out of a function whose contract is "never throws" and whose result type has no stage for it. Harmless today (peer.ts's outer catch + post-ready no-op rejectFirstReady drops it exactly as before), but invisible to a future caller. The suggested fix — move both fromB64 calls inside the decrypt try, then reduce openEncryptedFrame to a rethrow-wrapper — also removes the now-duplicated open/parse/validate sequence the two functions carry.
Carried over, still open (all already tracked in #179, unchanged by this commit):
2. content.ts:213's spoof guard covers null/undefined but not the arrays/primitives assertObject also rejects.
3. capture-logger.ts:162's "open a restaurant page and retry" — the only OpenTable-specific string in extension-core's runtime output, relayed verbatim to any MCP.
4. graphql-query.test.ts still has one it(), covering only the ok:true reply — no end-to-end case asserting graphqlQuery() rejects on ok:false while the socket stays open.
npm ci isn't in this job's allowed tools and node_modules is absent on this checkout, so I could not run npm test / npm run build. The PR body reports 1042/1042 green and CI ran on the earlier commits.
Verdict: warn
· claude/zen-pasteur-858985
| const iv = fromB64(frame.iv); | ||
| const ct = fromB64(frame.ciphertext); | ||
| let pt: Uint8Array; | ||
| try { | ||
| pt = await aesGcmOpen(sessionKey, iv, ct); | ||
| } catch (error) { | ||
| return { stage: 'decrypt-failed', error }; | ||
| } |
There was a problem hiding this comment.
🟡 Nit — the two fromB64 calls sit outside every try, so "never throws" isn't quite true.
fromB64 is documented as "Throws on invalid base64 (DOMException via atob)" (encoding.ts:22), and BASE64_RE (validate.ts:20, /^[A-Za-z0-9+/]*={0,2}$/) does not enforce a length that's a multiple of 4 — so {iv: "A", ciphertext: "A"} passes validateEncrypted's assertBase64 and then makes atob throw here. That contradicts this function's own contract ("Like openEncryptedFrame, but never throws") and the OpenFrameResult type, which has no stage for it.
Practical impact today is nil: peer.ts's only call site is inside the outer try, and post-ready rejectFirstReady is a no-op, so the frame is dropped exactly as before. But the escape hatch is invisible to any future caller reading the contract.
The tidy fix also resolves a second, smaller thing: openEncryptedFrame (lines 40-49) and this function now carry two copies of the same open/parse/validate sequence, so a future change to open semantics has to be made in both. Moving the fromB64 pair inside the decrypt try (a bad-base64 input is a decrypt-failed, which is exactly how it's already treated in practice) and then reducing openEncryptedFrame to a thin rethrow-wrapper keeps one implementation and preserves both existing behaviours:
export async function openEncryptedFrame(
sessionKey: Uint8Array,
frame: EncryptedFrame,
): Promise<InnerFrame> {
const r = await openEncryptedFrameDetailed(sessionKey, frame);
if (r.stage !== 'ok') throw r.error;
return r.inner;
}A seal.test.ts case with iv: 'A' would pin the contract.
#180) ## Summary Follow-up to #178 (merged) — resolves every item left open in the auto-review-followup issue. 1. **`openEncryptedFrameDetailed`'s `fromB64` calls sat outside every `try`** — an `iv` like `"A"` passes `BASE64_RE` (which doesn't enforce length % 4) but still makes `atob` throw, out of a function documented as "never throws". Moved both calls inside the decrypt `try`, and reduced `openEncryptedFrame` to a thin rethrow-wrapper over `openEncryptedFrameDetailed` so the decrypt/parse/validate sequence exists in exactly one place. 2. **`content.ts`'s spoofed-`graphql-res` guard covered `null`/`undefined` but not arrays/primitives** that `assertObject` also rejects — a spoofed `data:[]` or `data:"x"` from a same-window page script still reached `assertObject(raw.data)` and closed the bridge. Widened the guard to match `assertObject`'s own object/array/null check, with parametrized tests over null, undefined, array, string, number, boolean. 3. **`capture-logger.ts`'s error string was OpenTable-specific** ("open a restaurant page and retry") — the only vendor-specific string in `extension-core`'s runtime output, relayed verbatim to every MCP using the `graphql` capability. Made generic. 4. **`graphql-query.test.ts` only exercised `ok:true`** — the actual bug the auto-review found (`op:'graphql_query'` failing `validateInnerResponse`) could only ever show up on the `ok:false` path, so this suite could not have caught it. Added an end-to-end case driving the real WS + encryption + validation + dispatch stack, asserting `graphqlQuery()` rejects, the extension WebSocket stays open, and a second call over the same connection still succeeds. Closes #179 ## Test plan - [x] `npm test` — 1064/1064 passing - [x] `npm run build` — clean across all 7 workspaces - [x] `npm run typecheck` — clean - [x] No version bumps 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [1.7.0](v1.6.2...v1.7.0) (2026-07-29) ### Features * **graphql:** route declared GraphQL ops through the tab's own Apollo client ([#178](#178)) ([0c3fdf4](0c3fdf4)) ### Bug Fixes * **extension-chrome:** build content scripts as classic IIFE so Chrome injects them ([#175](#175)) ([f4a3728](f4a3728)) * **graphql:** address all four tracked nits from PR [#178](#178 auto-review ([#180](#180)) ([9d88ac9](9d88ac9)) --- 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>
…raphql capability (#128) ## Summary `opentable_find_slots` has been failing with a 409 on the `RestaurantsAvailability` endpoint: OpenTable's Akamai Bot Manager rejects the isolated-world `fetch()` path fetchproxy uses for every other endpoint, because the bot-detection sensor telemetry lives inside the page's own Apollo link chain, not on `window.fetch`. Verified live ([fetchproxy#178](chrischall/fetchproxy#178)): routing the same query through the bridged tab's own `window.__APOLLO_CLIENT__` instead returns 200 with real slots, using this MCP's existing (unchanged) variable-building logic. `@fetchproxy/server` 1.7.0 added the `graphql` capability for exactly this. Switched `find_slots` to it end to end: - `transport.ts` — added `graphqlQuery` to the `OpenTableTransport` interface. - `transport-fetchproxy.ts` — declares `capabilities:['fetch','graphql']` + `graphqlOps:[{name:'availability', operationName:'RestaurantsAvailability'}]`, implements `graphqlQuery()` as a thin delegate to `inner.server.graphqlQuery()`. No persisted-query hash declared — the extension reuses the live `DocumentNode` the tab's Apollo client already observed for the operation, so it auto-adapts if OpenTable revises the query (retiring the `RESTAURANTS_AVAILABILITY_HASH` re-capture chore). - `transport-mcp-chrome.ts` — `graphqlQuery()` throws a clear "not supported" error — mcp-chrome has no equivalent to the MAIN-world Apollo bridge, so failing loudly beats a confusing silent fallback. - `client.ts` — added a thin `graphqlQuery(name, variables)` pass-through — no HTTP-status/sign-in-page mapping applies to this verb. - `reservations.ts` — `find_slots` now calls `client.graphqlQuery` instead of building a persisted-query body and POSTing it via `fetchJson`. Removed the now-dead `RESTAURANTS_AVAILABILITY_HASH` and `AVAILABILITY_PATH` constants. `parseAvailabilityResponse`'s contract is unchanged. Bumped `@fetchproxy/server` to `^1.7.0` — opentable-mcp's own direct dependency. Its type re-exports resolve against the **consumer's** peer-dep copy, so this did not need to wait on [chrischall/mcp-utils#106](chrischall/mcp-utils#106) to merge/publish first — verified by a clean build against the currently-published `@chrischall/mcp-utils`. ## Test plan - [x] `npm test` — 178/178 passing (added coverage for every new/changed surface: `client.graphqlQuery` pass-through + error propagation, `FetchproxyTransport`'s capability declaration + delegation, `McpChromeTransport`'s clear unsupported error, and rewrote the four `find_slots` tests that exercised the old raw-fetch path) - [x] `npm run build` — clean - [ ] Live re-verification against a real restaurant (La Belle Hélène, restaurant_id 1175428, 2026-07-31 17:00, party 2) — out of band from this diff, needs a live signed-in browser session 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Adds a new opt-in
graphqlcapability so an MCP can invoke a page-declared GraphQL operation through the matched tab's ownwindow.__APOLLO_CLIENT__in the MAIN world, instead of the isolated-worldfetch()path.Why: some endpoints (OpenTable's
RestaurantsAvailability) reject the isolated-worldfetchat the edge — the bot-detection telemetry (Akamai) lives inside the page's own Apollo link chain, not onwindow.fetch. Routing through the real client clears it. Verified live against a signed-in opentable.com tab (La Belle Hélène, restaurant_id 1175428): the isolated-world path 403/409s;client.query(...)through the page's own Apollo client returns 200 with real slots, using the MCP's existing (unchanged) variable shape.Design: the extension carries no hardcoded query text or persisted-query hash. It captures the live
DocumentNodethe page's Apollo client already observed for a declaredoperationNameand reuses it — so it auto-adapts when a site revises its query. Gated by capability + a declaredgraphqlOpsallowlist (approved and diffed at pair time, like every other capability) + the existing domain allowlist and host-or-subdomain tab match. Full design + the live PoC findings:docs/superpowers/specs/2026-07-29-graphql-page-apollo-capability.md.Caught in review: the extension-bridge task initially shipped the MAIN-world bridge with a top-level ES
export, which MV3 silently refuses to inject as a classic content script (this branch was missing the classic-IIFE build split from #175). Fixed by rebuildingextension-chrome/build.tswith thecontent/capture-logger→format:'iife'split, with a regression test (content-scripts-classic.test.ts) pinning no top-level export in either bundled script.Also added size guards on
graphql_queryrequestvariables/ responsedata, mirroring the existingfetchop'sMAX_REQUEST_BODY_BYTES/MAX_RESPONSE_BODY_BYTESconvention.No version bumps (release-please owns that).
Test plan
npm test— 1042/1042 passing across 75 filesnpm run build— clean across all 7 workspacesnpm run typecheck— cleanexportin bundledcontent.js/capture-logger.js@fetchproxy/serverinmcp-utils, then switchopentable-mcp'sfind_slotsto the new capability and re-verify live🤖 Generated with Claude Code