From f5cee3fbf495fb2b685ff6f832a3752d64bbcf88 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 01:19:30 -0700 Subject: [PATCH] fix(relay): do not let a peer's version claim crash the daemon that logs it --- src/relay/protocol.ts | 2 ++ src/relay/relay-handshake-roundtrip.test.ts | 30 +++++++++++++++++++++ src/relay/relay-handshake.ts | 7 ++--- src/shared/relay-protocol-version.test.ts | 17 ++++++++++++ src/shared/relay-protocol-version.ts | 11 ++++++++ 5 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/relay/protocol.ts b/src/relay/protocol.ts index 57305616d55..3f5db21e57d 100644 --- a/src/relay/protocol.ts +++ b/src/relay/protocol.ts @@ -27,6 +27,7 @@ export type { DecodedFrame, FrameDecoderOptions } from './relay-frame-decoder' import { MIN_RELAY_PROTOCOL_VERSION, RELAY_PROTOCOL_VERSION, + describeRelayProtocolVersion, relayProtocolOffer, relayProtocolOfferAdmits, type RelayHandshakeCapabilities, @@ -36,6 +37,7 @@ import { export { MIN_RELAY_PROTOCOL_VERSION, RELAY_PROTOCOL_VERSION, + describeRelayProtocolVersion, relayProtocolOffer, relayProtocolOfferAdmits } diff --git a/src/relay/relay-handshake-roundtrip.test.ts b/src/relay/relay-handshake-roundtrip.test.ts index b57300337e4..68148f1ef8b 100644 --- a/src/relay/relay-handshake-roundtrip.test.ts +++ b/src/relay/relay-handshake-roundtrip.test.ts @@ -354,6 +354,36 @@ describe('handshake round-trip over a real Socket pair', () => { bridgeSock.destroy() }) + // The refusal path logs the peer's claim, and `JSON.parse` yields objects a template literal + // cannot stringify. A throw there is inside the frame-decoder callback, so it would kill the + // daemon — and every PTY it still holds — on an unauthenticated frame. + it('refuses a hostile protocolVersion claim without taking the daemon down', async () => { + const { accepted } = await startDaemon('0.1.0+server-version') + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + const reply = readDaemonReply(bridgeSock) + + bridgeSock.write( + encodeHandshakeFrame( + JSON.parse( + '{"type":"orca-relay-handshake","version":"0.1.0+different","protocolVersion":{"toString":1}}' + ) as HandshakeMessage + ) + ) + + await expect(reply).resolves.toMatchObject({ type: 'orca-relay-handshake-mismatch' }) + + // Still serving: a second, well-formed client is admitted after the hostile one. + const good = connect(sockPath) + await new Promise((r) => good.once('connect', () => r())) + runConnectHandshake(good, '0.1.0+server-version', { onAccepted: vi.fn() }) + await accepted + + bridgeSock.destroy() + good.destroy() + }) + // Tolerance replaces a compatibility gate, never the auth gate: the credential file lives inside // the incumbent's own install dir, so presenting it is what proves the caller may reach it. it('still refuses a cross-build bridge that cannot present the endpoint credential', async () => { diff --git a/src/relay/relay-handshake.ts b/src/relay/relay-handshake.ts index ecfe6a0ff54..27505a6fa06 100644 --- a/src/relay/relay-handshake.ts +++ b/src/relay/relay-handshake.ts @@ -10,6 +10,7 @@ import { FrameDecoder, encodeHandshakeFrame, parseHandshakeMessage, + describeRelayProtocolVersion, relayProtocolOffer, relayProtocolOfferAdmits, type DecodedFrame, @@ -140,7 +141,7 @@ function handleDaemonHandshakeFrame( if (!negotiated) { relayLogLine( `[relay] Handshake mismatch: own=${launchVersion}/p${RELAY_PROTOCOL_VERSION}, ` + - `client=${msg.version}/p${msg.protocolVersion ?? 'none'}; closing socket` + `client=${msg.version}/p${describeRelayProtocolVersion(msg.protocolVersion)}; closing socket` ) try { sock.write( @@ -224,7 +225,7 @@ export function runConnectHandshake( // Why both numbers: with negotiation the daemon's build can legitimately differ from this // bridge's, so the version alone no longer says which relay answered. process.stderr.write( - `[relay-connect] Handshake OK at version=${msg.version} protocol=${msg.protocolVersion ?? 'none'}\n` + `[relay-connect] Handshake OK at version=${msg.version} protocol=${describeRelayProtocolVersion(msg.protocolVersion)}\n` ) handshakeDone = true const leftover = decoder.drain() @@ -236,7 +237,7 @@ export function runConnectHandshake( // Why: exit inside the write callback; stderr is async on pipe transports, so exiting early drops the version detail. process.stderr.write( `[relay-connect] Handshake mismatch: expected=${msg.expected}, daemon=${msg.got}, ` + - `daemonProtocol=${msg.protocolVersion ?? 'none'}, ours=${RELAY_PROTOCOL_VERSION}; ` + + `daemonProtocol=${describeRelayProtocolVersion(msg.protocolVersion)}, ours=${RELAY_PROTOCOL_VERSION}; ` + `exiting ${EXIT_CODE_VERSION_MISMATCH}\n`, () => { sock.destroy() diff --git a/src/shared/relay-protocol-version.test.ts b/src/shared/relay-protocol-version.test.ts index 80de5c67957..0fcfeba228d 100644 --- a/src/shared/relay-protocol-version.test.ts +++ b/src/shared/relay-protocol-version.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { MIN_RELAY_PROTOCOL_VERSION, RELAY_PROTOCOL_VERSION, + describeRelayProtocolVersion, relayProtocolOffer, relayProtocolOfferAdmits } from './relay-protocol-version' @@ -71,4 +72,20 @@ describe('relay protocol version', () => { expect(relayProtocolOfferAdmits({ protocolVersion: 1, minProtocolVersion: 5 }, 1)).toBe(false) expect(relayProtocolOfferAdmits({ protocolVersion: 1, minProtocolVersion: 5 }, 5)).toBe(false) }) + + // A relay daemon logs the peer's claim on the refusal path, and `JSON.parse` can produce an + // object a template literal cannot stringify. Throwing there is inside the frame-decoder + // callback, which would take the daemon and every PTY it still holds down with it. + it('renders a peer version claim that a template literal would throw on', () => { + const hostile = JSON.parse('{"protocolVersion": {"toString": 1}}').protocolVersion + expect(() => `${hostile}`).toThrow() + expect(describeRelayProtocolVersion(hostile)).toBe('none') + }) + + it('renders real numbers and refuses every other shape', () => { + expect(describeRelayProtocolVersion(7)).toBe('7') + for (const bad of [undefined, null, '3', {}, [], Number.NaN, Number.POSITIVE_INFINITY]) { + expect(describeRelayProtocolVersion(bad), JSON.stringify(bad ?? null)).toBe('none') + } + }) }) diff --git a/src/shared/relay-protocol-version.ts b/src/shared/relay-protocol-version.ts index 14bb0bbaf59..abeb4db63e3 100644 --- a/src/shared/relay-protocol-version.ts +++ b/src/shared/relay-protocol-version.ts @@ -50,6 +50,17 @@ function readVersion(value: unknown): number | null { : null } +/** + * Renders a peer's claimed version for a log line without interpolating it. + * + * `JSON.parse` can hand back `{ "toString": 1 }`, and a template literal on that throws + * `Cannot convert object to primitive value` — inside the frame-decoder callback, which would + * take the daemon and every PTY it holds down with it. + */ +export function describeRelayProtocolVersion(value: unknown): string { + return typeof value === 'number' && Number.isFinite(value) ? String(value) : 'none' +} + /** This build's own offer, sent on every handshake and reply. */ export function relayProtocolOffer(): Required { return {