From 945516d12f64e8c37af284365bb8785e4ed41a5a Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 00:47:36 -0700 Subject: [PATCH 1/6] feat(relay): negotiate a semantic protocol version in the handshake --- src/relay/protocol.ts | 39 ++++- src/relay/relay-handshake-roundtrip.test.ts | 155 +++++++++++++++++++- src/relay/relay-handshake.ts | 53 +++++-- src/shared/relay-protocol-version.test.ts | 68 +++++++++ src/shared/relay-protocol-version.ts | 85 +++++++++++ 5 files changed, 387 insertions(+), 13 deletions(-) create mode 100644 src/shared/relay-protocol-version.test.ts create mode 100644 src/shared/relay-protocol-version.ts diff --git a/src/relay/protocol.ts b/src/relay/protocol.ts index 0f31b448f55..57305616d55 100644 --- a/src/relay/protocol.ts +++ b/src/relay/protocol.ts @@ -24,6 +24,25 @@ export { } export type { DecodedFrame, FrameDecoderOptions } from './relay-frame-decoder' +import { + MIN_RELAY_PROTOCOL_VERSION, + RELAY_PROTOCOL_VERSION, + relayProtocolOffer, + relayProtocolOfferAdmits, + type RelayHandshakeCapabilities, + type RelayProtocolOffer +} from '../shared/relay-protocol-version' + +export { + MIN_RELAY_PROTOCOL_VERSION, + RELAY_PROTOCOL_VERSION, + relayProtocolOffer, + relayProtocolOfferAdmits +} +export type { RelayHandshakeCapabilities, RelayProtocolOffer } + +// Why frozen at 0.1.0: the deploy path carries a content hash of the bundle instead, so nothing +// ever needed this to move. `RELAY_PROTOCOL_VERSION` is the number that describes the wire. export const RELAY_VERSION = '0.1.0' export const RELAY_SENTINEL = `ORCA-RELAY v${RELAY_VERSION} READY\n` @@ -37,10 +56,24 @@ export const MessageType = { // reads exactly one Handshake frame before attaching the JSON-RPC dispatcher, // to refuse mismatched-version --connect bridges that would otherwise drive a // stale daemon. +// Why optional on every arm: `parseHandshakeMessage` throws on an unknown `type` and the throw +// closes the socket, so this envelope can only ever grow by optional fields on the arms that +// already ship. `endpointCredential` is the precedent. See +// docs/reference/remote-wire-compatibility.md Rule 1. export type HandshakeMessage = - | { type: 'orca-relay-handshake'; version: string; endpointCredential?: string } - | { type: 'orca-relay-handshake-ok'; version: string } - | { type: 'orca-relay-handshake-mismatch'; expected: string; got: string } + | ({ + type: 'orca-relay-handshake' + version: string + endpointCredential?: string + } & RelayProtocolOffer) + | ({ + type: 'orca-relay-handshake-ok' + version: string + capabilities?: RelayHandshakeCapabilities + } & RelayProtocolOffer) + // `protocolVersion` here tells a client whose offered range missed *what* it missed, which a + // content hash cannot: the two builds may be wire-compatible and merely differ in bytes. + | ({ type: 'orca-relay-handshake-mismatch'; expected: string; got: string } & RelayProtocolOffer) // Why a distinct reply: the bridge exits with its own code so the client can tell a refused // credential from a crashed relay. Old bridges reject the unknown type and exit 1 pre-sentinel. | { type: 'orca-relay-handshake-credential-mismatch' } diff --git a/src/relay/relay-handshake-roundtrip.test.ts b/src/relay/relay-handshake-roundtrip.test.ts index 0713d62295e..b57300337e4 100644 --- a/src/relay/relay-handshake-roundtrip.test.ts +++ b/src/relay/relay-handshake-roundtrip.test.ts @@ -13,9 +13,14 @@ import { encodeHandshakeFrame, encodeJsonRpcFrame, FrameDecoder, + parseHandshakeMessage, + MIN_RELAY_PROTOCOL_VERSION, + RELAY_PROTOCOL_VERSION, type DecodedFrame, + type HandshakeMessage, MessageType } from './protocol' +import { PTY_CONSUMER_SESSION_PROTOCOL_VERSION } from '../shared/pty-consumer-session-contract' import { relayTestSocketPath } from './relay-test-socket-path' // Why: --connect normally calls process.exit on mismatch / fatal handshake @@ -210,9 +215,86 @@ describe('handshake round-trip over a real Socket pair', () => { bridgeSock.destroy() }) - it('exits with EXIT_CODE_VERSION_MISMATCH when the daemon reports a mismatch', async () => { + // Reads the daemon's single handshake reply off a raw socket, so a test can assert what the + // envelope carries rather than only whether the bridge accepted it. + function readDaemonReply(sock: Socket): Promise { + return new Promise((resolve) => { + const decoder = new FrameDecoder((frame) => { + if (frame.type === MessageType.Handshake) { + resolve(parseHandshakeMessage(frame.payload)) + } + }) + sock.on('data', (chunk: Buffer) => decoder.feed(chunk)) + }) + } + + it('refuses a cross-build bridge that offers no protocol range at all', async () => { + // Why raw bytes: runConnectHandshake now always offers a range. Every relay already deployed + // sends none, and for those the build hash stays the only gate — this is that fallback. 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({ type: 'orca-relay-handshake', version: '0.1.0+different' }) + ) + + // The refusal now names the daemon's protocol version, which a content hash cannot express. + await expect(reply).resolves.toMatchObject({ + type: 'orca-relay-handshake-mismatch', + expected: '0.1.0+server-version', + got: '0.1.0+different', + protocolVersion: RELAY_PROTOCOL_VERSION, + minProtocolVersion: MIN_RELAY_PROTOCOL_VERSION + }) + + bridgeSock.destroy() + }) + + it('refuses a cross-build bridge whose offered range excludes this daemon', async () => { + 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({ + type: 'orca-relay-handshake', + version: '0.1.0+different', + protocolVersion: RELAY_PROTOCOL_VERSION + 5, + minProtocolVersion: RELAY_PROTOCOL_VERSION + 5 + }) + ) + + await expect(reply).resolves.toMatchObject({ type: 'orca-relay-handshake-mismatch' }) + + bridgeSock.destroy() + }) + + it('exits with EXIT_CODE_VERSION_MISMATCH when the daemon reports a mismatch', async () => { + server = createServer((sock) => { + trackServerSocket(sock) + const decoder = new FrameDecoder((frame) => { + if (frame.type !== MessageType.Handshake) { + return + } + sock.write( + encodeHandshakeFrame({ + type: 'orca-relay-handshake-mismatch', + expected: '0.1.0+server-version', + got: '0.1.0+different', + protocolVersion: RELAY_PROTOCOL_VERSION + 5, + minProtocolVersion: RELAY_PROTOCOL_VERSION + 5 + }) + ) + }) + sock.on('data', (chunk: Buffer) => decoder.feed(chunk)) + }) + await new Promise((r) => server.listen(sockPath, () => r())) + const bridgeSock = connect(sockPath) await new Promise((r) => bridgeSock.once('connect', () => r())) @@ -226,6 +308,77 @@ describe('handshake round-trip over a real Socket pair', () => { bridgeSock.destroy() }) + // #13852: after an app update the client's bundle hashes differently, so the incumbent relay + // holding every live PTY refused it and that work became permanently unreachable. + it('admits a bridge from a different build once it offers a range this daemon falls in', async () => { + const { accepted } = await startDaemon('0.1.0+incumbent-holding-live-ptys') + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + + const acceptedCb = vi.fn<(leftover: Buffer) => void>() + runConnectHandshake(bridgeSock, '0.1.0+freshly-updated-client', { onAccepted: acceptedCb }) + + await accepted + await vi.waitFor(() => expect(acceptedCb).toHaveBeenCalledTimes(1)) + expect(exitSpy).not.toHaveBeenCalled() + + bridgeSock.destroy() + }) + + it('answers a cross-build bridge with the protocol version and capabilities it must speak', async () => { + await startDaemon('0.1.0+incumbent-holding-live-ptys') + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + const reply = readDaemonReply(bridgeSock) + + bridgeSock.write( + encodeHandshakeFrame({ + type: 'orca-relay-handshake', + version: '0.1.0+freshly-updated-client', + protocolVersion: RELAY_PROTOCOL_VERSION + 3, + minProtocolVersion: MIN_RELAY_PROTOCOL_VERSION + }) + ) + + await expect(reply).resolves.toMatchObject({ + type: 'orca-relay-handshake-ok', + // The build the client actually reached, which is no longer its own. + version: '0.1.0+incumbent-holding-live-ptys', + protocolVersion: RELAY_PROTOCOL_VERSION, + minProtocolVersion: MIN_RELAY_PROTOCOL_VERSION, + capabilities: { ptyConsumerSession: PTY_CONSUMER_SESSION_PROTOCOL_VERSION } + }) + + bridgeSock.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 () => { + const { accepted } = await startDaemon('0.1.0+incumbent', 'secret-credential') + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + const closed = new Promise((r) => bridgeSock.once('close', () => r())) + + runConnectHandshake( + bridgeSock, + '0.1.0+freshly-updated-client', + { onAccepted: vi.fn() }, + 'wrong-credential' + ) + + await closed + await expect( + Promise.race([ + accepted.then(() => 'accepted'), + new Promise((r) => setTimeout(() => r('closed'), 20)) + ]) + ).resolves.toBe('closed') + }) + it('does not call onAccepted before any handshake-ok frame arrives', async () => { // Why: silent server that never replies. acceptedCb must stay // un-invoked even though the bridge has flushed its handshake frame. diff --git a/src/relay/relay-handshake.ts b/src/relay/relay-handshake.ts index ed76baf024d..ecfe6a0ff54 100644 --- a/src/relay/relay-handshake.ts +++ b/src/relay/relay-handshake.ts @@ -5,14 +5,25 @@ import { existsSync, readFileSync, realpathSync } from 'node:fs' import type { Socket } from 'node:net' import { RELAY_VERSION, + RELAY_PROTOCOL_VERSION, MessageType, FrameDecoder, encodeHandshakeFrame, parseHandshakeMessage, - type DecodedFrame + relayProtocolOffer, + relayProtocolOfferAdmits, + type DecodedFrame, + type RelayHandshakeCapabilities } from './protocol' +import { PTY_CONSUMER_SESSION_PROTOCOL_VERSION } from '../shared/pty-consumer-session-contract' import { relayLogLine } from './relay-diagnostic-log' +// Contracts that turn over independently of the handshake integer, so a client cannot infer them +// from `protocolVersion` and must not have to infer them from the build hash either. +function relayHandshakeCapabilities(): RelayHandshakeCapabilities { + return { ptyConsumerSession: PTY_CONSUMER_SESSION_PROTOCOL_VERSION } +} + // Why: clients treat this exit code as non-retryable; other non-zero exits are transient. export const EXIT_CODE_VERSION_MISMATCH = 42 // Why distinct from 42: a refused credential is a live daemon saying no, which the client must @@ -121,16 +132,23 @@ function handleDaemonHandshakeFrame( sock.destroy() return false } - if (msg.version !== launchVersion) { + // Why the build hash is still first: it is the only gate every already-deployed relay has, and + // it stays the fallback for peers that offer no protocol range at all. Negotiation is what lets + // a relay stranded by an app update keep serving the PTYs it already owns (#13852) — the hash + // differs by construction there, while the wire is unchanged. + const negotiated = msg.version === launchVersion || relayProtocolOfferAdmits(msg) + if (!negotiated) { relayLogLine( - `[relay] Handshake mismatch: own=${launchVersion}, client=${msg.version}; closing socket` + `[relay] Handshake mismatch: own=${launchVersion}/p${RELAY_PROTOCOL_VERSION}, ` + + `client=${msg.version}/p${msg.protocolVersion ?? 'none'}; closing socket` ) try { sock.write( encodeHandshakeFrame({ type: 'orca-relay-handshake-mismatch', expected: launchVersion, - got: msg.version + got: msg.version, + ...relayProtocolOffer() }) ) } catch { @@ -150,8 +168,17 @@ function handleDaemonHandshakeFrame( sock.end() return false } - process.stderr.write(`[relay] Handshake OK from version=${msg.version}\n`) - sock.write(encodeHandshakeFrame({ type: 'orca-relay-handshake-ok', version: launchVersion })) + process.stderr.write( + `[relay] Handshake OK from version=${msg.version} (${msg.version === launchVersion ? 'same build' : `cross-build, protocol ${RELAY_PROTOCOL_VERSION}`})\n` + ) + sock.write( + encodeHandshakeFrame({ + type: 'orca-relay-handshake-ok', + version: launchVersion, + ...relayProtocolOffer(), + capabilities: relayHandshakeCapabilities() + }) + ) return true } @@ -194,7 +221,11 @@ export function runConnectHandshake( process.exit(1) } if (msg.type === 'orca-relay-handshake-ok') { - process.stderr.write(`[relay-connect] Handshake OK at version=${msg.version}\n`) + // 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` + ) handshakeDone = true const leftover = decoder.drain() sock.removeAllListeners('data') @@ -204,7 +235,9 @@ export function runConnectHandshake( if (msg.type === 'orca-relay-handshake-mismatch') { // 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}; exiting ${EXIT_CODE_VERSION_MISMATCH}\n`, + `[relay-connect] Handshake mismatch: expected=${msg.expected}, daemon=${msg.got}, ` + + `daemonProtocol=${msg.protocolVersion ?? 'none'}, ours=${RELAY_PROTOCOL_VERSION}; ` + + `exiting ${EXIT_CODE_VERSION_MISMATCH}\n`, () => { sock.destroy() process.exit(EXIT_CODE_VERSION_MISMATCH) @@ -243,7 +276,9 @@ export function runConnectHandshake( encodeHandshakeFrame({ type: 'orca-relay-handshake', version: myVersion, - ...(endpointCredential ? { endpointCredential } : {}) + ...(endpointCredential ? { endpointCredential } : {}), + // A relay that predates the offer ignores these keys and compares the hash as before. + ...relayProtocolOffer() }) ) } diff --git a/src/shared/relay-protocol-version.test.ts b/src/shared/relay-protocol-version.test.ts new file mode 100644 index 00000000000..27b020e0d9f --- /dev/null +++ b/src/shared/relay-protocol-version.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest' +import { + MIN_RELAY_PROTOCOL_VERSION, + RELAY_PROTOCOL_VERSION, + relayProtocolOffer, + relayProtocolOfferAdmits +} from './relay-protocol-version' + +describe('relay protocol version', () => { + it('is a whole number at or above its own floor', () => { + expect(Number.isSafeInteger(RELAY_PROTOCOL_VERSION)).toBe(true) + expect(Number.isSafeInteger(MIN_RELAY_PROTOCOL_VERSION)).toBe(true) + expect(RELAY_PROTOCOL_VERSION).toBeGreaterThanOrEqual(MIN_RELAY_PROTOCOL_VERSION) + }) + + it('offers its own range, so a same-build peer is admitted by negotiation as well as by hash', () => { + expect(relayProtocolOfferAdmits(relayProtocolOffer())).toBe(true) + }) + + // The whole point of Release N: the stranded relay is always the OLDER side, and it must be + // able to say yes on the first frame with no earlier round trip to downgrade in. + it('admits a newer client whose range reaches back to this relay', () => { + expect(relayProtocolOfferAdmits({ protocolVersion: 9, minProtocolVersion: 1 }, 1)).toBe(true) + expect(relayProtocolOfferAdmits({ protocolVersion: 9, minProtocolVersion: 5 }, 1)).toBe(false) + }) + + it('admits a client older than this relay when the relay still falls in its range', () => { + expect(relayProtocolOfferAdmits({ protocolVersion: 4, minProtocolVersion: 2 }, 3)).toBe(true) + expect(relayProtocolOfferAdmits({ protocolVersion: 2, minProtocolVersion: 1 }, 3)).toBe(false) + }) + + it('treats a lone protocolVersion as a range of exactly one', () => { + expect(relayProtocolOfferAdmits({ protocolVersion: 3 }, 3)).toBe(true) + expect(relayProtocolOfferAdmits({ protocolVersion: 3 }, 2)).toBe(false) + expect(relayProtocolOfferAdmits({ protocolVersion: 3 }, 4)).toBe(false) + }) + + // A peer that sends no offer predates negotiation; admitting it would hand every unversioned + // caller the cross-build path that the build hash is supposed to gate. + it('never admits an absent or empty offer', () => { + expect(relayProtocolOfferAdmits(undefined, 1)).toBe(false) + expect(relayProtocolOfferAdmits({}, 1)).toBe(false) + expect(relayProtocolOfferAdmits({ minProtocolVersion: 1 }, 1)).toBe(false) + }) + + it('rejects offers that are not plausible integers', () => { + for (const bad of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY, 2_000_000]) { + expect( + relayProtocolOfferAdmits({ protocolVersion: bad, minProtocolVersion: 1 }, 1), + `protocolVersion=${bad}` + ).toBe(false) + } + for (const bad of ['1', null, {}, []]) { + expect( + relayProtocolOfferAdmits( + { protocolVersion: 1, minProtocolVersion: bad as unknown as number }, + 1 + ), + `minProtocolVersion=${JSON.stringify(bad)}` + ).toBe(false) + } + }) + + it('rejects an inverted range instead of silently reordering it', () => { + expect(relayProtocolOfferAdmits({ protocolVersion: 1, minProtocolVersion: 5 }, 1)).toBe(false) + expect(relayProtocolOfferAdmits({ protocolVersion: 1, minProtocolVersion: 5 }, 5)).toBe(false) + }) +}) diff --git a/src/shared/relay-protocol-version.ts b/src/shared/relay-protocol-version.ts new file mode 100644 index 00000000000..5f15df030d3 --- /dev/null +++ b/src/shared/relay-protocol-version.ts @@ -0,0 +1,85 @@ +/** + * Semantic wire version for the relay handshake. + * + * `RELAY_VERSION` ('0.1.0') has never moved, so the deploy path is namespaced by a content hash + * of the bundle instead (`config/scripts/build-relay.mjs`). That hash changes on nearly every + * release, which makes every install directory — and the socket inside it — unreachable to the + * next build, stranding the PTYs and agents the incumbent still owns (#13852). + * + * This integer is the escape: it names what the *wire* can do, independent of what the bytes + * hash to, exactly as `PROTOCOL_VERSION` does for the local terminal daemon + * (`src/main/daemon/daemon-protocol-version.ts`). Bump it only when a client and a relay of + * adjacent versions genuinely cannot drive each other; a rebuild is not a reason. + */ +export const RELAY_PROTOCOL_VERSION = 1 + +/** + * Oldest peer protocol this build can still speak. Raising it strands every relay below the new + * floor for good, so it moves only when serving a version is actually impossible. + */ +export const MIN_RELAY_PROTOCOL_VERSION = 1 + +/** Rejects a peer's absurd or non-integer claim before it reaches a comparison. */ +const MAX_PLAUSIBLE_RELAY_PROTOCOL_VERSION = 1_000_000 + +/** + * Contract versions a peer cannot derive from `protocolVersion` alone, because they turn over + * independently of the handshake. + */ +export type RelayHandshakeCapabilities = { + /** `pty.openClient` grant contract this relay mints (`PTY_CONSUMER_SESSION_PROTOCOL_VERSION`). */ + readonly ptyConsumerSession?: number +} + +/** + * The protocol range a peer says it can speak. Both fields are optional on the wire: a relay or + * bridge that predates this negotiation sends neither, which is the signal to fall back to + * comparing build hashes. + */ +export type RelayProtocolOffer = { + readonly protocolVersion?: number + readonly minProtocolVersion?: number +} + +function readVersion(value: unknown): number | null { + return typeof value === 'number' && + Number.isSafeInteger(value) && + value >= 1 && + value <= MAX_PLAUSIBLE_RELAY_PROTOCOL_VERSION + ? value + : null +} + +/** This build's own offer, sent on every handshake and reply. */ +export function relayProtocolOffer(): Required { + return { + protocolVersion: RELAY_PROTOCOL_VERSION, + minProtocolVersion: MIN_RELAY_PROTOCOL_VERSION + } +} + +/** + * Whether a peer offering `offer` can be served by a relay speaking `ownVersion`. + * + * The peer states a range rather than a single number so an *older* relay can admit a *newer* + * client on the first frame — there is no earlier round trip in which the client could learn + * what to downgrade to, and the stranded relay is by construction the older side. + * + * An offer with no `protocolVersion` is not a negotiation at all and never admits: that is a + * pre-negotiation peer, and the build-hash comparison stays its only gate. + */ +export function relayProtocolOfferAdmits( + offer: RelayProtocolOffer | undefined, + ownVersion: number = RELAY_PROTOCOL_VERSION +): boolean { + const max = readVersion(offer?.protocolVersion) + if (max === null) { + return false + } + // Why default to `max`: a peer that names one version speaks exactly that one. + const min = offer?.minProtocolVersion === undefined ? max : readVersion(offer.minProtocolVersion) + if (min === null || min > max) { + return false + } + return ownVersion >= min && ownVersion <= max +} From 0b568e6a07c0b08b1365cf4a45abae2419bc6d87 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 00:54:17 -0700 Subject: [PATCH 2/6] test(relay): pin the zero-version rejection the range band cannot catch --- src/shared/relay-protocol-version.test.ts | 6 ++++++ src/shared/relay-protocol-version.ts | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/shared/relay-protocol-version.test.ts b/src/shared/relay-protocol-version.test.ts index 27b020e0d9f..80de5c67957 100644 --- a/src/shared/relay-protocol-version.test.ts +++ b/src/shared/relay-protocol-version.test.ts @@ -43,6 +43,12 @@ describe('relay protocol version', () => { expect(relayProtocolOfferAdmits({ minProtocolVersion: 1 }, 1)).toBe(false) }) + // Zero is the one bad value the band check cannot catch on its own: a peer claiming protocol 0 + // would be admitted by a relay whose own version was also read as 0. + it('rejects a zero protocol version rather than treating it as a real version', () => { + expect(relayProtocolOfferAdmits({ protocolVersion: 0, minProtocolVersion: 0 }, 0)).toBe(false) + }) + it('rejects offers that are not plausible integers', () => { for (const bad of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY, 2_000_000]) { expect( diff --git a/src/shared/relay-protocol-version.ts b/src/shared/relay-protocol-version.ts index 5f15df030d3..14bb0bbaf59 100644 --- a/src/shared/relay-protocol-version.ts +++ b/src/shared/relay-protocol-version.ts @@ -78,8 +78,10 @@ export function relayProtocolOfferAdmits( } // Why default to `max`: a peer that names one version speaks exactly that one. const min = offer?.minProtocolVersion === undefined ? max : readVersion(offer.minProtocolVersion) - if (min === null || min > max) { + if (min === null) { return false } + // An inverted range admits nothing rather than being reordered into something the peer never + // offered: `min > max` simply fails this band. return ownVersion >= min && ownVersion <= max } From f5cee3fbf495fb2b685ff6f832a3752d64bbcf88 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 01:19:30 -0700 Subject: [PATCH 3/6] 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 { From 9d843f1705786110a9d21fa94664c4796bba6f31 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 01:23:20 -0700 Subject: [PATCH 4/6] fix(ssh): keep the daemon version intact now that the mismatch line has trailing fields --- .../ssh/ssh-relay-handshake-mismatch.test.ts | 48 +++++++++++++++++++ src/main/ssh/ssh-relay-handshake-mismatch.ts | 4 +- 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 src/main/ssh/ssh-relay-handshake-mismatch.test.ts diff --git a/src/main/ssh/ssh-relay-handshake-mismatch.test.ts b/src/main/ssh/ssh-relay-handshake-mismatch.test.ts new file mode 100644 index 00000000000..2fae7cc5716 --- /dev/null +++ b/src/main/ssh/ssh-relay-handshake-mismatch.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from 'vitest' +import { buildRelayHandshakeRefusalError } from './ssh-relay-handshake-mismatch' +import { + RelayVersionMismatchError, + RELAY_EXIT_CODE_VERSION_MISMATCH +} from './ssh-relay-version-mismatch-error' +import { + RelayCredentialMismatchError, + RELAY_EXIT_CODE_CREDENTIAL_MISMATCH +} from './ssh-relay-credential-mismatch-error' + +describe('buildRelayHandshakeRefusalError', () => { + // The bridge's mismatch line now carries `daemonProtocol=` and `ours=` after the version, so a + // `daemon=` capture that merely stops at `;` swallows the separating comma into the version. + it('names the daemon version without the comma that follows it', () => { + const error = buildRelayHandshakeRefusalError( + RELAY_EXIT_CODE_VERSION_MISMATCH, + '[relay-connect] Handshake mismatch: expected=0.1.0+aaa, daemon=0.1.0+bbb, ' + + 'daemonProtocol=6, ours=1; exiting 42\n' + ) + expect(error).toBeInstanceOf(RelayVersionMismatchError) + expect(error).toMatchObject({ expected: '0.1.0+aaa', got: '0.1.0+bbb' }) + }) + + // A relay from before the protocol fields still emits the short form. + it('still reads the pre-negotiation form of the line', () => { + const error = buildRelayHandshakeRefusalError( + RELAY_EXIT_CODE_VERSION_MISMATCH, + '[relay-connect] Handshake mismatch: expected=0.1.0+aaa, daemon=0.1.0+bbb; exiting 42\n' + ) + expect(error).toMatchObject({ expected: '0.1.0+aaa', got: '0.1.0+bbb' }) + }) + + it('reports an unparseable mismatch line without inventing versions', () => { + const error = buildRelayHandshakeRefusalError(RELAY_EXIT_CODE_VERSION_MISMATCH, 'nothing here') + expect(error).toMatchObject({ expected: undefined, got: undefined }) + }) + + it('keeps a refused credential distinct from a version skew', () => { + expect( + buildRelayHandshakeRefusalError(RELAY_EXIT_CODE_CREDENTIAL_MISMATCH, 'refused') + ).toBeInstanceOf(RelayCredentialMismatchError) + }) + + it('returns nothing for an exit code that encodes no refusal', () => { + expect(buildRelayHandshakeRefusalError(1, 'crashed')).toBeNull() + }) +}) diff --git a/src/main/ssh/ssh-relay-handshake-mismatch.ts b/src/main/ssh/ssh-relay-handshake-mismatch.ts index 90841675e3f..646a14ff9c4 100644 --- a/src/main/ssh/ssh-relay-handshake-mismatch.ts +++ b/src/main/ssh/ssh-relay-handshake-mismatch.ts @@ -24,11 +24,13 @@ export function buildRelayHandshakeRefusalError( // Why: extract the expected/got version pair from --connect's stderr line // "Handshake mismatch: expected=, daemon=" so diagnostics name both versions. +// Why a comma is excluded from `daemon`: the line grew trailing `daemonProtocol=`/`ours=` +// fields, so the version is no longer the last thing before the `;`. function parseHandshakeMismatchStderr(stderr: string): { expected: string | undefined got: string | undefined } { - const match = /expected=([^,\s]+),\s*daemon=([^\s;]+)/.exec(stderr) + const match = /expected=([^,\s]+),\s*daemon=([^\s;,]+)/.exec(stderr) if (!match) { return { expected: undefined, got: undefined } } From ac3606397c316ef34f835f725e84fd6386492691 Mon Sep 17 00:00:00 2001 From: Neil Date: Fri, 11 Sep 2026 00:18:58 -0700 Subject: [PATCH 5/6] docs(relay): say plainly that the protocol negotiation is not reachable yet relay-protocol-version.ts documents a negotiation no live path can reach, and reads as the fix for #13852. It is not. The deploy path namespaces the relay directory by the CLIENT'S OWN build hash -- remoteInstallDirName is `relay-` -- and the daemon socket lives inside it. Every runConnectHandshake caller takes its path from that one deploy result, so a bridge can only ever meet a daemon of its own build, `msg.version === launchVersion` short-circuits first, and relayProtocolOfferAdmits never decides anything on any live path. The stranded incumbent this exists for sits in `relay-/`, which nothing dials. Keeping it is right -- the mechanism is correct and hostile-input-safe, and it is the part that has to exist first. What was missing is the statement of what else has to land with it, written where someone editing this file will see it. Carried from a dead-code and unread-field sweep stranded on the un-PR'd branch nwparker/adv2-map-reconciled (94ba0b9f052). The lingerMs half of that sweep is a coordinator concern and went to #20061 instead. No behaviour change. --- src/shared/relay-protocol-version.ts | 31 ++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/shared/relay-protocol-version.ts b/src/shared/relay-protocol-version.ts index abeb4db63e3..b4345ebb8a3 100644 --- a/src/shared/relay-protocol-version.ts +++ b/src/shared/relay-protocol-version.ts @@ -13,9 +13,40 @@ */ export const RELAY_PROTOCOL_VERSION = 1 +/** + * ## This negotiation is not reachable yet. Do not read it as fixing #13852. + * + * Measured, not inferred: the deploy path namespaces the relay directory by the CLIENT'S OWN build + * hash — `remoteInstallDirName` is `relay-` — and the daemon socket lives inside that + * directory (`ssh-relay-deploy.ts`, `--connect --sock-path ~/.orca-remote/relay-/…`; + * the short-socket fallback derives its segment from the same version dir). Every + * `runConnectHandshake` caller takes its path from that one deploy result. So a bridge can only + * ever meet a daemon of its own build, `msg.version === launchVersion` short-circuits first, and + * `relayProtocolOfferAdmits` never decides anything on any live path. The stranded incumbent this + * exists for sits in `relay-/`, which nothing dials. + * + * What ships today is therefore three log lines: `describeRelayProtocolVersion` in + * `relay-handshake.ts`. `MIN_RELAY_PROTOCOL_VERSION` and the `minProtocolVersion` wire field have + * no live reader at all. + * + * Keep it — the mechanism is correct and hostile-input-safe, and it is the part that has to exist + * first. Making it live needs two more changes, both of which must land together: + * 1. route the bridge to the incumbent's socket rather than to its own version directory; + * 2. stop `validateGrant` (`ssh-pty-consumer-session.ts`) refusing on `serverBuildId`. Its + * premise, "client and relay ship in one build", is still TRUE today and becomes false the + * moment (1) lands — it is a second gate that would refuse what the handshake just admitted. + * + * Until both land, a change here cannot be validated by any end-to-end test, only by the + * handshake's own unit suite. + */ + /** * Oldest peer protocol this build can still speak. Raising it strands every relay below the new * floor for good, so it moves only when serving a version is actually impossible. + * + * No live reader — see the note above. It is serialized onto every handshake and reply so that the + * field exists on the wire before any peer needs it (Rule 1, docs/reference/remote-wire- + * compatibility.md); an older peer ignoring it today is the point. */ export const MIN_RELAY_PROTOCOL_VERSION = 1 From bedc94c5ecbece4e594a1ff23defd0d4fbeeb193 Mon Sep 17 00:00:00 2001 From: Neil Date: Fri, 11 Sep 2026 00:19:21 -0700 Subject: [PATCH 6/6] docs(relay): the negotiation needs three conditions to become reachable, not two The note the previous commit carried lists two prerequisites. There are three, and the missing one is the one a reader cannot see from this file. validateGrant's refusal is a single `if` with two disjuncts. Deleting the `serverBuildId` clause -- prerequisite (2) -- leaves the other standing: `grant.protocolVersion !== PTY_CONSUMER_SESSION_PROTOCOL_VERSION`, a different constant from this file's RELAY_PROTOCOL_VERSION, compared for exact equality with no range, no floor and no fallback. Two peers that just negotiated a compatible relay protocol are still refused if their PTY-session constants differ by one. And there is no channel to resolve it with. Verified rather than assumed: `capabilities` on orca-relay-handshake-ok is written at exactly one site (relay-handshake.ts) and read by nothing outside tests. It is a published field with no consumer, so (3) is not 'delete another clause' -- it is 'give that field a reader first'. The error text compounds it: the refusal interpolates only the build ids, so a pure protocol-version mismatch reports as 'expected build X, got X' with two identical ids, sending the next debugger at the gate that is not the problem. Anyone implementing N+1 from the two-item list ships a broken negotiation and debugs it at the wrong gate. Recorded here because this file is what they will read first. --- src/shared/relay-protocol-version.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/shared/relay-protocol-version.ts b/src/shared/relay-protocol-version.ts index b4345ebb8a3..0a4c6303d9e 100644 --- a/src/shared/relay-protocol-version.ts +++ b/src/shared/relay-protocol-version.ts @@ -30,13 +30,27 @@ export const RELAY_PROTOCOL_VERSION = 1 * no live reader at all. * * Keep it — the mechanism is correct and hostile-input-safe, and it is the part that has to exist - * first. Making it live needs two more changes, both of which must land together: + * first. Making it live needs THREE more changes, all of which must land together. Landing only + * the first two ships a negotiation that still refuses, later and less legibly: * 1. route the bridge to the incumbent's socket rather than to its own version directory; * 2. stop `validateGrant` (`ssh-pty-consumer-session.ts`) refusing on `serverBuildId`. Its * premise, "client and relay ship in one build", is still TRUE today and becomes false the * moment (1) lands — it is a second gate that would refuse what the handshake just admitted. + * 3. give that same refusal a way to survive a protocol-version difference. It is ONE `if` with + * two disjuncts, and deleting the `serverBuildId` clause leaves the other one standing: + * `grant.protocolVersion !== PTY_CONSUMER_SESSION_PROTOCOL_VERSION` — a DIFFERENT constant + * from this file's `RELAY_PROTOCOL_VERSION`, compared for exact equality, with no range, no + * floor and no fallback. Two peers that just negotiated a compatible relay protocol are still + * refused if their PTY-session constants differ by one. And there is nothing to negotiate it + * with: `capabilities` on `orca-relay-handshake-ok` is written at exactly one site + * (`relay-handshake.ts`) and read by nothing outside tests, so (3) means giving that field a + * reader before it can carry the peer's PTY-session protocol. * - * Until both land, a change here cannot be validated by any end-to-end test, only by the + * Budget a debugging session for the error text too: the refusal interpolates only the build ids, + * so a pure protocol-version mismatch reports as "expected build X, got X" with two IDENTICAL ids, + * which points at the gate that is not the problem. + * + * Until all three land, a change here cannot be validated by any end-to-end test, only by the * handshake's own unit suite. */