fix(companion): prevent withheld keys leaking through SSE on scrub failure

[DevChiniwala]

Summary

scrubEvent in companion/src/wire.ts had a single try-catch around both JSON.parse and scrub(). When scrub() threw — scrub() recurses, and a body nested a few thousand deep produces a RangeError that JSON.parse handles fine — the catch treated it as "not JSON" and returned the original unscrubbed line. That line still contained resumeCursors and sshAlias: the keys this file exists to withhold from devices.

The proxy's JSON response path already handles this exact scenario (proxy.ts lines 520–548), with a comment calling it "not hypothetical." The SSE path had the same exposure but lacked the split.

What changed

• Split the single try-catch into two: parse failure passes through (not JSON, nothing to scrub); scrub failure replaces the data line with data: {} (JSON but unscrubable, must not go through unscrubbed).
• Added a regression test that constructs a deeply nested payload with resumeCursors inside, feeds it through the SSE scrubber, and asserts the withheld keys do not appear in the output. The test fails on the old code and passes on the fix.

Test plan

• New test: replaces unscrubable JSON data instead of leaking withheld keys — confirmed to fail on the old code and pass on the fix
• All 16 wire tests pass
• All 207 companion tests pass (13 suites, 3 skipped platform-specific)
• pnpm typecheck clean
• pnpm check:brand clean

[Copilot]

🟢 Approval recommended

The change correctly eliminates a confirmed withheld-key leak scenario in SSE and includes a focused regression test to prevent regressions.

Pull request overview

This PR fixes a security-sensitive edge case in the companion SSE scrubbing path: when JSON parsing succeeds but scrub() throws (e.g., extreme nesting causing RangeError), the scrubber must not fall back to forwarding the original data: line with withheld keys intact.

Changes:

• Split SSE data: handling into separate parse vs. scrub error paths so scrub failures emit data: {} instead of passing through.
• Added a regression test ensuring withheld keys never appear in output when scrub fails due to deep nesting.

File summaries

File	Description
companion/src/wire.ts	Separates JSON parse failures (pass through) from scrub failures (replace with empty JSON) to prevent withheld-key leaks over SSE.
companion/test/wire.test.ts	Adds a regression test that forces scrub failure via deep nesting and asserts withheld keys are not emitted.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[DivyamTalwar]

@DevChiniwala — thank you for this. It's a genuinely good first contribution, and this is the strongest of your three: a real fail-open on a security path, found by reasoning about the code rather than by pattern-matching a linter. Welcome.

I reproduced the bug and the fix holds up. Detail below.

The bug is real

The original wrapped JSON.parse and scrub() in one try, so any throw from either landed in a catch whose comment says "not JSON" and whose behaviour is return line — the original, unscrubbed. Your read is right: that returns exactly what the scrubber exists to withhold.

It matters because scrub() (companion/src/wire.ts:26) recurses with no depth cap and no cycle guard:

if (Array.isArray(value)) return value.map(scrub);
...
out[key] = scrub(inner);

Worth noting server/redact.ts guards the same shape of traversal with MAX_REDACTION_DEPTH = 12 and a seen WeakSet. The companion scrubber has neither, so it's the one that can throw.

Reproduced

JSON.parse is iterative in V8 and survives far deeper than scrub() does:

nesting	JSON.parse	scrub()
2,000	ok	ok
5,000	ok	RangeError
12,000	ok	RangeError
20,000	ok	RangeError

So the window where parse succeeds and scrub throws is wide and reliably reachable — your test's 12,000 sits well inside it with roughly 2x headroom, not on a knife edge. I'd flagged possible flakiness before measuring; there isn't any. companion/test/wire.test.ts passes 16/16 locally.

Fail-closed is the right call

data: {} over the original line is correct. On a scrubber, silence beats a leak.

One optional follow-up (not blocking)

This fixes the symptom correctly. The root cause is that scrub() has no depth bound. A cap mirroring MAX_REDACTION_DEPTH would let a legitimately deep frame through scrubbed rather than flattened to {}. Happy either way — separate concern, separate PR if you want it.

No cycle-safety concern on this path, since JSON.parse output can't be cyclic.

Status

Looks correct to me and I'd like to take it. Two process notes:

• CI hasn't run yet — GitHub gates workflow runs for first-time contributors pending maintainer approval. I'll approve the run.
• mergeable is currently reporting UNKNOWN; I'll re-check once CI has been through.

Nice find. Please do send more.