From 78a17bb24de085992edeb38ce47f6b71227e97da Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 17 Sep 2026 21:21:27 -0700 Subject: [PATCH] fix(relay): one malformed pre-auth handshake frame closes its connection, not the daemon (#19879) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(relay): one malformed pre-auth handshake frame closes its connection, not the daemon parseHandshakeMessage returned whatever JSON.parse produced, and the daemon interpolates the peer's version into a log line before any credential check. A version that is an object with a non-callable toString throws TypeError there, inside the frame-decoder callback. FrameDecoder.drainTurn wrapped its synchronous dispatch in try/finally with no catch, so the throw escaped feed(), escaped the socket data handler, and reached uncaughtException: the relay daemon exited and every PTY and agent session it held died with it. Two layers, because only the second closes the class: - parseHandshakeMessage now requires the string fields each arm carries (version; expected/got) and rejects a non-object payload. Both readers share the parser, so neither side can interpolate a non-string again. - FrameDecoder contains a frame owner that throws on the synchronous turn the same way it already contained one on a continuation turn: reset the residue and report one FrameDecoderContinuationError to onError. Every owner's onError already closes its own connection, so any future throw of this shape costs one connection instead of the process. The relay CLI channel gains an explicit onError so a malformed reply still ends that one-shot command instead of parking it. * fix(relay): keep the diagnostic the refusal path exists to produce Two error paths that destroy their own evidence. `parseHandshakeMessage`'s unknown-type refusal interpolated `String(t)` on a peer-supplied value: `{"type":{"toString":1}}` makes String() throw "Cannot convert object to primitive value", so the refusal arrives without naming what was refused. `describeRelayProtocolVersion` guards this exact hazard two files away; the sibling was missed. `runRelayOrcaCliChannel`'s new `onDecodeError` wrote to stderr and then exited synchronously. stderr is async on a pipe transport, so the one line recording why the command died could be dropped — the reason relay-handshake.ts already exits inside its write callback. * fix(relay): prove the optional handshake field too, not just the required ones The parser refuses a non-string `version`, `expected` and `got`, then returns the object with `endpointCredential` unproved — the most pre-auth field on the frame. It is safe today only by accident: its one reader compares it, and a non-string loses that comparison. Nothing holds that shape in place, and the next reader to put it in a log line reinstates the template-literal throw this function exists to stop. Present-but-not-a-string is now refused at the parser. Absent stays absent: a bridge presenting no credential is the common case, and refusing it would close every unauthenticated-endpoint connection. Wire-visible delta, deliberate: a peer sending a non-string credential used to get `orca-relay-handshake-credential-mismatch` and exit 43; it now gets a bare close. No first-party client can reach it — `runConnectHandshake` types the parameter `string` and omits it when falsy — and a bare close is the right answer to a frame that was malformed before any credential was checked. * fix(relay): carry the SAFETY: rationale main's casting gate now requires Main gained a `typescript/consistent-type-assertions` scan while this branch sat 432 commits behind, so every `as` the branch touches lands as a new finding. The parser is the one place the handshake shape is proved, so each cast names the check that earns it, and the hostile-frame cast in the round-trip test names the fact that it is a deliberate lie the type system cannot describe. * test(relay): annotate the hostile handshake frame instead of suppressing a cast JSON.parse answers `any`, so a typed const expresses the same deliberate lie the assertion did and the casting gate has nothing to flag. One fewer suppression. --- .../ssh/relay-protocol-backpressure.test.ts | 34 ++++++++ src/relay/protocol-backpressure.test.ts | 35 +++++++++ src/relay/protocol-handshake.test.ts | 78 +++++++++++++++++++ src/relay/protocol.ts | 64 ++++++++++++--- src/relay/relay-handshake-roundtrip.test.ts | 66 ++++++++++++++++ src/relay/relay-orca-cli-channel.ts | 13 +++- src/shared/relay-frame-decoder.ts | 22 ++++-- 7 files changed, 295 insertions(+), 17 deletions(-) diff --git a/src/main/ssh/relay-protocol-backpressure.test.ts b/src/main/ssh/relay-protocol-backpressure.test.ts index a1ee62655e8..45a869af16e 100644 --- a/src/main/ssh/relay-protocol-backpressure.test.ts +++ b/src/main/ssh/relay-protocol-backpressure.test.ts @@ -200,6 +200,40 @@ describe('FrameDecoder bounded turns', () => { expect(seen).toEqual([1, 4]) }) + // feed() runs straight from a transport data handler. A frame owner that threw on the first + // turn used to escape feed() and reach uncaughtException. The continuation path was already + // contained; the synchronous path must match it. + it('contains a frame owner that throws on the synchronous turn and reports one typed error', () => { + const seen: number[] = [] + const onError = vi.fn() + const pause = vi.fn() + const resume = vi.fn() + const decoder = new FrameDecoder( + (decoded) => { + if (decoded.id === 2) { + throw new Error('frame owner failed') + } + seen.push(decoded.id) + }, + onError, + { pause, resume } + ) + + expect(() => decoder.feed(Buffer.concat([frame(1), frame(2), frame(3)]))).not.toThrow() + + expect(seen).toEqual([1]) + expect(onError).toHaveBeenCalledExactlyOnceWith(expect.any(FrameDecoderContinuationError)) + expect(onError.mock.calls[0]?.[0]).toMatchObject({ + cause: expect.objectContaining({ message: 'frame owner failed' }) + }) + expect(decoder.drain()).toHaveLength(0) + expect(pause).not.toHaveBeenCalled() + expect(resume).not.toHaveBeenCalled() + + decoder.feed(frame(4)) + expect(seen).toEqual([1, 4]) + }) + it('keeps reads active for partial frames and incrementally discards oversized payloads', () => { const errors: Error[] = [] const seen: DecodedFrame[] = [] diff --git a/src/relay/protocol-backpressure.test.ts b/src/relay/protocol-backpressure.test.ts index fa4724ff960..f2693fe941f 100644 --- a/src/relay/protocol-backpressure.test.ts +++ b/src/relay/protocol-backpressure.test.ts @@ -200,6 +200,41 @@ describe('relay FrameDecoder bounded turns', () => { expect(seen).toEqual([1, 4]) }) + // feed() runs straight from a socket 'data' handler. A frame owner that threw on the first + // turn used to escape feed() and reach uncaughtException, which in the relay daemon means every + // PTY and agent session it holds dies with it. The continuation path was already contained. + it('contains a frame owner that throws on the synchronous turn and reports one typed error', () => { + const seen: number[] = [] + const onError = vi.fn() + const pause = vi.fn() + const resume = vi.fn() + const decoder = new FrameDecoder( + (decoded) => { + if (decoded.id === 2) { + throw new Error('frame owner failed') + } + seen.push(decoded.id) + }, + onError, + { pause, resume } + ) + + expect(() => decoder.feed(Buffer.concat([frame(1), frame(2), frame(3)]))).not.toThrow() + + expect(seen).toEqual([1]) + expect(onError).toHaveBeenCalledExactlyOnceWith(expect.any(FrameDecoderContinuationError)) + expect(onError.mock.calls[0]?.[0]).toMatchObject({ + cause: expect.objectContaining({ message: 'frame owner failed' }) + }) + // Residue after the bad frame is dropped rather than replayed, and no pause epoch is leaked. + expect(decoder.drain()).toHaveLength(0) + expect(pause).not.toHaveBeenCalled() + expect(resume).not.toHaveBeenCalled() + + decoder.feed(frame(4)) + expect(seen).toEqual([1, 4]) + }) + it('keeps reads active for partial frames and incrementally discards oversized payloads', () => { const errors: Error[] = [] const seen: DecodedFrame[] = [] diff --git a/src/relay/protocol-handshake.test.ts b/src/relay/protocol-handshake.test.ts index 822fea3e20b..4b4695877b7 100644 --- a/src/relay/protocol-handshake.test.ts +++ b/src/relay/protocol-handshake.test.ts @@ -61,6 +61,84 @@ describe('handshake framing', () => { expect(() => parseHandshakeMessage(bogus)).toThrow(/Unknown handshake type/) }) + // `type` is peer-supplied, so it can be an object whose String() conversion throws — which + // replaced the one diagnostic this refusal exists to produce with a primitive-conversion error. + it('still names the refusal when the peer type cannot be stringified', () => { + const hostile = Buffer.from(JSON.stringify({ type: { toString: 1 } })) + expect(() => parseHandshakeMessage(hostile)).toThrow(/Unknown handshake type: object/) + }) + + // The daemon logs the peer's version before any credential check, and `JSON.parse` can hand + // back a value a template literal throws on. The parser is the one place every reader shares. + it('rejects a version that is not a string on both arms that carry one', () => { + for (const type of ['orca-relay-handshake', 'orca-relay-handshake-ok']) { + for (const version of [{ toString: 1 }, 7, null, undefined, ['0.1.0']]) { + const payload = Buffer.from(JSON.stringify({ type, version })) + expect( + () => parseHandshakeMessage(payload), + `${type} version=${JSON.stringify(version)}` + ).toThrow(/Handshake field version is not a string/) + } + } + }) + + it('rejects a mismatch reply whose expected or got is not a string', () => { + const type = 'orca-relay-handshake-mismatch' + expect(() => + parseHandshakeMessage(Buffer.from(JSON.stringify({ type, expected: {}, got: 'b' }))) + ).toThrow(/Handshake field expected is not a string/) + expect(() => + parseHandshakeMessage(Buffer.from(JSON.stringify({ type, expected: 'a', got: 1 }))) + ).toThrow(/Handshake field got is not a string/) + }) + + it('rejects payloads that are not objects', () => { + for (const payload of ['null', '"orca-relay-handshake"', '42']) { + expect(() => parseHandshakeMessage(Buffer.from(payload)), payload).toThrow( + /Handshake payload is not an object/ + ) + } + }) + + // endpointCredential is the one optional field, and it is the most pre-auth thing on the frame. + // Its only reader compares it, so a non-string refuses today by inequality rather than by type — + // which is luck, not a guarantee. Prove it at the parser, where every reader shares it. + it('rejects a present endpointCredential that is not a string', () => { + for (const endpointCredential of [{ toString: 1 }, 7, null, ['secret'], true]) { + const payload = Buffer.from( + JSON.stringify({ type: 'orca-relay-handshake', version: '0.1.0', endpointCredential }) + ) + expect( + () => parseHandshakeMessage(payload), + `endpointCredential=${JSON.stringify(endpointCredential)}` + ).toThrow(/Handshake field endpointCredential is not a string/) + } + }) + + // Absent must stay absent: a bridge that legitimately presents no credential is the common case, + // and refusing it here would close every unauthenticated-endpoint connection in the fleet. + it('still accepts a handshake with no endpointCredential, and one with a string', () => { + const bare = Buffer.from(JSON.stringify({ type: 'orca-relay-handshake', version: '0.1.0' })) + expect(parseHandshakeMessage(bare)).toEqual({ type: 'orca-relay-handshake', version: '0.1.0' }) + const withCredential = Buffer.from( + JSON.stringify({ type: 'orca-relay-handshake', version: '0.1.0', endpointCredential: 'sec' }) + ) + expect(parseHandshakeMessage(withCredential)).toEqual({ + type: 'orca-relay-handshake', + version: '0.1.0', + endpointCredential: 'sec' + }) + }) + + it('still accepts a credential-mismatch reply, which carries no fields', () => { + const payload = Buffer.from( + JSON.stringify({ type: 'orca-relay-handshake-credential-mismatch' }) + ) + expect(parseHandshakeMessage(payload)).toEqual({ + type: 'orca-relay-handshake-credential-mismatch' + }) + }) + it('handshake frames use a distinct MessageType from Regular and KeepAlive', () => { expect(MessageType.Handshake).not.toBe(MessageType.Regular) expect(MessageType.Handshake).not.toBe(MessageType.KeepAlive) diff --git a/src/relay/protocol.ts b/src/relay/protocol.ts index 0f31b448f55..84656fed3cb 100644 --- a/src/relay/protocol.ts +++ b/src/relay/protocol.ts @@ -50,18 +50,62 @@ export function encodeHandshakeFrame(msg: HandshakeMessage): Buffer { return encodeFrame(MessageType.Handshake, 0, 0, payload) } +// Why the fields are checked and not just the type: this frame arrives before any credential, and +// both sides interpolate its version fields into log lines. `JSON.parse` can produce values a +// template literal throws on, so anything that reaches a reader must already be a string. +const HANDSHAKE_STRING_FIELDS: Readonly> = { + 'orca-relay-handshake': ['version'], + 'orca-relay-handshake-ok': ['version'], + 'orca-relay-handshake-mismatch': ['expected', 'got'], + 'orca-relay-handshake-credential-mismatch': [] +} + +// Optional fields are peer-supplied too, so the parser only proves the type of what it returns if +// it refuses a present-but-wrong one. `endpointCredential` survives today only because its single +// reader compares it and never interpolates it; the next reader to log it would restore the bug +// this function exists to stop. Absent stays absent — refusing that would break a bridge that +// legitimately presents no credential. +const HANDSHAKE_OPTIONAL_STRING_FIELDS: Readonly< + Record +> = { + 'orca-relay-handshake': ['endpointCredential'], + 'orca-relay-handshake-ok': [], + 'orca-relay-handshake-mismatch': [], + 'orca-relay-handshake-credential-mismatch': [] +} + export function parseHandshakeMessage(payload: Buffer): HandshakeMessage { - const msg = JSON.parse(payload.toString('utf-8')) as HandshakeMessage - const t = (msg as { type?: string }).type - if ( - t !== 'orca-relay-handshake' && - t !== 'orca-relay-handshake-ok' && - t !== 'orca-relay-handshake-mismatch' && - t !== 'orca-relay-handshake-credential-mismatch' - ) { - throw new Error(`Unknown handshake type: ${t}`) + const parsed: unknown = JSON.parse(payload.toString('utf-8')) + if (typeof parsed !== 'object' || parsed === null) { + throw new Error('Handshake payload is not an object') } - return msg + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the typeof/null guard directly above is exactly what makes this an index-able object; every read below still proves its own field. + const msg = parsed as Record + const t = msg.type + const required = + typeof t === 'string' && Object.hasOwn(HANDSHAKE_STRING_FIELDS, t) + ? // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: reached only when Object.hasOwn proved t is a key of this record, on the same line. + HANDSHAKE_STRING_FIELDS[t as HandshakeMessage['type']] + : null + if (required === null) { + // Why typeof and not String(t): a peer-supplied `{ "type": { "toString": 1 } }` makes String() + // itself throw "Cannot convert object to primitive value", replacing the one diagnostic this + // line exists to produce. + throw new Error(`Unknown handshake type: ${typeof t === 'string' ? t : typeof t}`) + } + for (const field of required) { + if (typeof msg[field] !== 'string') { + throw new Error(`Handshake field ${field} is not a string`) + } + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the required === null bail above already refused every t that is not one of the four keys. + for (const field of HANDSHAKE_OPTIONAL_STRING_FIELDS[t as HandshakeMessage['type']]) { + if (msg[field] !== undefined && typeof msg[field] !== 'string') { + throw new Error(`Handshake field ${field} is not a string`) + } + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: this is the one place the shape is proved: the type is one of the four literals and every field the union declares has been checked to be a string. + return msg as unknown as HandshakeMessage } export const KEEPALIVE_SEND_MS = 5_000 diff --git a/src/relay/relay-handshake-roundtrip.test.ts b/src/relay/relay-handshake-roundtrip.test.ts index 0713d62295e..bfc345e8366 100644 --- a/src/relay/relay-handshake-roundtrip.test.ts +++ b/src/relay/relay-handshake-roundtrip.test.ts @@ -14,6 +14,7 @@ import { encodeJsonRpcFrame, FrameDecoder, type DecodedFrame, + type HandshakeMessage, MessageType } from './protocol' import { relayTestSocketPath } from './relay-test-socket-path' @@ -246,4 +247,69 @@ describe('handshake round-trip over a real Socket pair', () => { bridgeSock.destroy() }) + + // The daemon reads one handshake frame before any credential check, so every field on it is + // untrusted input. `JSON.parse` hands back objects a template literal cannot stringify, and the + // frame callback runs inside the decoder: a throw there used to escape the socket's data + // handler and take the daemon — and every PTY and agent session it held — down with it. + it('closes a connection whose handshake version is not a string and keeps serving', async () => { + const { accepted } = await startDaemon('0.1.0+server-version') + + const hostile = connect(sockPath) + await new Promise((r) => hostile.once('connect', () => r())) + const hostileClosed = new Promise((r) => hostile.once('close', () => r())) + // The annotation is deliberately a lie: this is the frame a hostile peer sends, and + // HandshakeMessage cannot describe it. JSON.parse answers `any`, so it needs no assertion. + const hostileFrame: HandshakeMessage = JSON.parse( + '{"type":"orca-relay-handshake","version":{"toString":1}}' + ) + hostile.write(encodeHandshakeFrame(hostileFrame)) + await hostileClosed + + const good = connect(sockPath) + await new Promise((r) => good.once('connect', () => r())) + const acceptedCb = vi.fn<(leftover: Buffer) => void>() + runConnectHandshake(good, '0.1.0+server-version', { onAccepted: acceptedCb }) + await accepted + await vi.waitFor(() => expect(acceptedCb).toHaveBeenCalledTimes(1)) + + good.destroy() + }) + + // Same class, different instance: `onAccepted` runs inside the frame callback too, so a throw + // from the accept path must cost that one connection and nothing else. + it('closes only the connection whose accept path throws', async () => { + let connections = 0 + const acceptedSockets: Socket[] = [] + server = createServer((sock) => { + trackServerSocket(sock) + connections += 1 + const failThisOne = connections === 1 + setupDaemonHandshake(sock, { + launchVersion: '0.1.0+server-version', + onAccepted: (s) => { + if (failThisOne) { + throw new Error('accept path failed') + } + acceptedSockets.push(s) + } + }) + }) + await new Promise((r) => server.listen(sockPath, () => r())) + + const first = connect(sockPath) + await new Promise((r) => first.once('connect', () => r())) + const firstClosed = new Promise((r) => first.once('close', () => r())) + runConnectHandshake(first, '0.1.0+server-version', { onAccepted: vi.fn() }) + await firstClosed + + const second = connect(sockPath) + await new Promise((r) => second.once('connect', () => r())) + const acceptedCb = vi.fn<(leftover: Buffer) => void>() + runConnectHandshake(second, '0.1.0+server-version', { onAccepted: acceptedCb }) + await vi.waitFor(() => expect(acceptedCb).toHaveBeenCalledTimes(1)) + expect(acceptedSockets).toHaveLength(1) + + second.destroy() + }) }) diff --git a/src/relay/relay-orca-cli-channel.ts b/src/relay/relay-orca-cli-channel.ts index 2afbd363fe5..1d528c63280 100644 --- a/src/relay/relay-orca-cli-channel.ts +++ b/src/relay/relay-orca-cli-channel.ts @@ -151,6 +151,17 @@ export async function runRelayOrcaCliChannel( } } + // Why an explicit error path: the decoder contains a throwing frame owner instead of letting + // it escape, so a malformed relay reply must still end this one-shot command, not park it. + const onDecodeError = (error: Error): void => { + // Why exit inside the write callback: stderr is async on pipe transports, so exiting early + // drops the only evidence this failure ever produces — the same reason relay-handshake.ts + // writes its mismatch line this way. + process.stderr.write(`[orca-cli] Relay protocol error: ${error.message}\n`, () => { + sock.destroy() + process.exit(1) + }) + } const decoder = new FrameDecoder((frame: DecodedFrame) => { if (frame.id > highestReceivedSeq) { highestReceivedSeq = frame.id @@ -194,7 +205,7 @@ export async function runRelayOrcaCliChannel( } sendPostOutput(result.postOutput) }) - }) + }, onDecodeError) const connectTimeout = setTimeout(() => { process.stderr.write(`[orca-cli] Relay connection timed out after ${CONNECT_TIMEOUT_MS}ms\n`) diff --git a/src/shared/relay-frame-decoder.ts b/src/shared/relay-frame-decoder.ts index 22a1c348185..e2b012593e3 100644 --- a/src/shared/relay-frame-decoder.ts +++ b/src/shared/relay-frame-decoder.ts @@ -143,12 +143,22 @@ export class FrameDecoder { const framed = this.buffer.take(totalLength) frames += 1 bytes += totalLength - this.onFrame({ - type: framed[0], - id: framed.readUInt32BE(1), - ack: framed.readUInt32BE(5), - payload: framed.subarray(HEADER_LENGTH, totalLength) - }) + // Why contain here and not in the caller: feed() runs straight from a socket 'data' + // handler, so a frame owner that throws on the first turn would escape as an + // uncaughtException and take the whole process — and every connection it serves — down. + // The continuation path already contains this; the synchronous path must match it, so + // one bad frame costs one connection (the owner's onError closes it), never the process. + try { + this.onFrame({ + type: framed[0], + id: framed.readUInt32BE(1), + ack: framed.readUInt32BE(5), + payload: framed.subarray(HEADER_LENGTH, totalLength) + }) + } catch (error) { + // reset() bumps the generation, which ends this turn and drops the residue. + containFrameDecoderContinuation(() => this.reset(), this.onError, error) + } } } finally { this.draining = false