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