fix(browser): report malformed relay CDP frames#102070
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 4:02 AM ET / 08:02 UTC. Summary PR surface: Source +29, Tests +28. Total +57 across 2 files. Reproducibility: yes. Current main has a high-confidence source-level reproduction path: attach a CDP client through ExtensionRelayBridge.attachCdpClientSocket and send malformed JSON or an object without numeric id plus string method; the handler returns before sending a response. Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this focused relay-boundary fix, or an equivalent one, for #102057 and then close or supersede the overlapping candidate at #102071. Do we have a high-confidence way to reproduce the issue? Yes. Current main has a high-confidence source-level reproduction path: attach a CDP client through ExtensionRelayBridge.attachCdpClientSocket and send malformed JSON or an object without numeric id plus string method; the handler returns before sending a response. Is this the best way to solve the issue? Yes. The PR fixes the implicated CDP client socket boundary, preserves id/sessionId in invalid-request errors, and leaves normal handleCdpRequest routing unchanged. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4ae8d735bf54. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +29, Tests +28. Total +57 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Added the missing real behavior proof for this exact head. Head verified: Environment and safety notes:
Command shape, from repo root: # Inline Node harness piped to node from PowerShell.
# The harness asserted:
# git rev-parse upstream/pr/102070 == ae2425f414fdf2a6fd84a4841a0b7853a78c9ddf
# It then loaded the bridge source with:
# git show ae2425f414fdf2a6fd84a4841a0b7853a78c9ddf:extensions/browser/src/browser/extension-relay/relay-bridge.ts
# It started a loopback HTTP upgrade server wired to ExtensionRelayBridge.attachCdpClientSocket,
# connected a raw TCP WebSocket client, sent the two bad text frames, and asserted each
# response arrived within 500ms with the expected JSON-RPC id/code/message.Redacted transcript: {
"proof": "live loopback WebSocket /cdp client against ExtensionRelayBridge.attachCdpClientSocket",
"head": "ae2425f414fdf2a6fd84a4841a0b7853a78c9ddf",
"node": "v22.20.0",
"timeoutBudgetMs": 500,
"transcript": [
{
"case": "malformed JSON",
"sent": "<malformed JSON frame, redacted body>",
"responseMs": 16,
"response": {
"id": null,
"error": {
"code": -32700,
"message": "Parse error"
}
}
},
{
"case": "invalid request frame",
"sent": "<JSON object missing method, params redacted>",
"responseMs": 15,
"response": {
"id": 42,
"error": {
"code": -32600,
"message": "Invalid request"
}
}
}
]
}This proves the malformed JSON and invalid request frames now receive immediate JSON-RPC errors instead of waiting for a client-side timeout. I only looked at #102071 enough to confirm it is still an overlapping open PR for the same issue; I did not touch it. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: sufficient
ae2425f to
fc389e1
Compare
|
Maintainer follow-up complete. I kept the contributor's regression coverage, collapsed the parser abstraction into one relay validation path, and preserved numeric request ids plus flat CDP Verification:
This fixes the hang without changing relay authentication, frame-size limits, extension routing, or valid CDP behavior. Ready to merge. |
|
Merged via squash.
|
* fix(browser): report malformed relay CDP frames * refactor(browser): streamline relay frame validation --------- Co-authored-by: Peter Steinberger <[email protected]>
2 participants
Closes #102057
What Problem This Solves
Fixes an issue where browser extension relay CDP clients would keep waiting when they sent malformed JSON or a structurally invalid CDP request frame.
Why This Change Was Made
The relay now validates inbound CDP client frames at the socket boundary and returns JSON-RPC parse or invalid-request errors instead of silently dropping those frames. Valid CDP requests continue through the existing request handler unchanged.
User Impact
Automation clients connected through the browser extension relay now receive immediate protocol errors for malformed client frames instead of timing out without a response.
Evidence
extensions/browser/src/browser/extension-relay/relay-bridge.tspreviously returned silently whenJSON.parse(raw)failed or when the parsed request lacked numericid/ stringmethod.node scripts/run-vitest.mjs extensions/browser/src/browser/extension-relay/relay-bridge.test.tspassed in the first issue worktree after dependency hydration: 1 file, 12 tests.git diff --check HEAD~1..HEADpassed.pnpm dlx [email protected] --check --threads=1 extensions/browser/src/browser/extension-relay/relay-bridge.ts extensions/browser/src/browser/extension-relay/relay-bridge.test.tspassed.python .agents\skills\autoreview\scripts\autoreview --mode localreported no accepted/actionable findings before commit.