From 411fda4098d061b01bc98055042e2d4379d35656 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 02:10:24 -0700 Subject: [PATCH] fix(ssh): refuse to publish a native-deps tree whose cloexec patch did not take --- src/main/ssh/ssh-relay-deploy.ts | 66 +++++++++++++-- src/main/ssh/ssh-relay-native-deps-cache.ts | 4 +- .../ssh-relay-native-deps-install-fixture.ts | 3 +- ...h-relay-pty-master-cloexec-install.test.ts | 83 +++++++++++++++++++ 4 files changed, 148 insertions(+), 8 deletions(-) diff --git a/src/main/ssh/ssh-relay-deploy.ts b/src/main/ssh/ssh-relay-deploy.ts index 3d4089232ce..fc65af35c0a 100644 --- a/src/main/ssh/ssh-relay-deploy.ts +++ b/src/main/ssh/ssh-relay-deploy.ts @@ -729,6 +729,29 @@ const NODE_PTY_VERSION = '1.1.0' const NODE_PTY_CONSOLE_LIST_PATCH_FILENAME = 'node-pty-1.1.0-console-list-agent-patch.cjs' const NODE_PTY_MASTER_CLOEXEC_PATCH_FILENAME = 'node-pty-1.1.0-master-cloexec-patch.cjs' const NODE_PTY_CLOEXEC_STATUS_PREFIX = 'ORCA-NPTY-CLOEXEC:' +/** + * Whether the tree the patch left behind still leaks the pty master into every later child. + * `fixed` is the only outcome a shared cache entry may be published from. + */ +type NodePtyMasterCloexecOutcome = 'fixed' | 'unfixed' +/** + * The statuses that leave a non-leaking tree. Deliberately an allowlist, not a `failed:` denylist: + * the script's `skipped:` family is mixed. `skipped:not-linux` is a platform that never leaks, but + * `skipped:earlier-attempt-failed`, `skipped:no-compiled-build`, `skipped:unexpected-source` and + * the two `skipped:` forms all mean the patch was refused and the leaky build is still on + * disk -- indistinguishable from `failed:` as far as what gets published. + */ +const NODE_PTY_CLOEXEC_FIXED_STATUSES: ReadonlySet = new Set([ + 'patched', + // The rebuild ran from patched source; only the isolation check could not observe the result. + // An unobservable check is not a failed patch, and treating it as one would disable the shared + // cache on every host without `lsof`. + 'patched-unverified', + 'already-patched', + // Unreachable while the platform gate below short-circuits first, but it is the one `skipped:` + // that means "nothing to fix" rather than "would not fix it". + 'skipped:not-linux' +]) // Exported for the relay-native-dependency-coverage test, which asserts every // native addon the relay bundle imports is either installed here or explicitly // declared as degrading without it. @@ -1220,14 +1243,30 @@ async function installNativeDeps( // published -- and by contract immutable -- shared cache entry. Patching afterwards would write // through the link, and `.deps-complete` would already have published an unpatched tree that // every later host links and skips. - if (probe.available) { - await applyNodePtyMasterCloexecPatch(conn, remoteDir, platform, hostPlatform, nodePath, signal) - } + const cloexec = probe.available + ? await applyNodePtyMasterCloexecPatch( + conn, + remoteDir, + platform, + hostPlatform, + nodePath, + signal + ) + : 'unfixed' // Why promotion is gated on the probe and not on npm's exit code: an entry is shared, so the // only evidence worth publishing is this host having loaded both addons out of that tree. + // Why it is gated on the patch too: a refused or rolled-back patch leaves the pre-patch leaky + // build in place, and the cache key hashes this patch's bytes -- so publishing it would hand + // every later host on the machine a tree that links, probes loadable, and skips patching. if (probe.available && cacheContext && cache) { - await promoteRelayNativeDepsCache(conn, cacheContext, cache.key) + if (cloexec === 'fixed') { + await promoteRelayNativeDepsCache(conn, cacheContext, cache.key) + } else { + console.warn( + `[ssh-relay][NPTY-CLOEXEC-UNSHARED] keeping the native deps at ${remoteDir} (${platform}) private; the tree still leaks the pty master, so it is not publishable as ${cache.key}` + ) + } } // MISSING is non-fatal by design: the relay still serves fs/git/preflight; only native-backed ops fail on hosts that can't build the addons. @@ -1251,6 +1290,9 @@ async function installNativeDeps( * * Why a shared cache entry never reaches here: the caller returns as soon as a linked tree probes * loadable, so this only ever rewrites a `node_modules` the relay directory still owns privately. + * + * Returns whether the tree that is left behind still leaks, which is what decides publishability. + * The script exits 0 on every outcome by design, so the status line is the only evidence there is. */ async function applyNodePtyMasterCloexecPatch( conn: SshConnection, @@ -1259,11 +1301,11 @@ async function applyNodePtyMasterCloexecPatch( hostPlatform: RemoteHostPlatform, nodePath: string, signal?: AbortSignal -): Promise { +): Promise { // Linux is the only relay platform that takes forkpty()'s no-O_CLOEXEC path; macOS and Windows // ship prebuilds, so forcing a rebuild there would add a first compile to fix nothing. if (isWindowsRemoteHost(hostPlatform) || !platform.startsWith('linux')) { - return + return 'fixed' } try { const command = commandWithNodePath( @@ -1282,7 +1324,16 @@ async function applyNodePtyMasterCloexecPatch( .map((line) => line.trim()) .find((line) => line.startsWith(NODE_PTY_CLOEXEC_STATUS_PREFIX)) ?.slice(NODE_PTY_CLOEXEC_STATUS_PREFIX.length) ?? 'no-status' + if (!NODE_PTY_CLOEXEC_FIXED_STATUSES.has(status)) { + // Warn, not log: the script exits 0 on a refusal too, so this line is the only thing that + // says the relay directory will leak a master into every child for its whole life. + console.warn( + `[ssh-relay][NPTY-CLOEXEC-UNFIXED] pty master still leaks at ${remoteDir} (${platform}): ${status}` + ) + return 'unfixed' + } console.log(`[ssh-relay][NPTY-CLOEXEC] ${remoteDir} (${platform}): ${status}`) + return 'fixed' } catch (err) { signal?.throwIfAborted() // Never fatal: the script restores the working build itself, and a leaky relay beats none. An @@ -1290,6 +1341,9 @@ async function applyNodePtyMasterCloexecPatch( console.warn( `[ssh-relay][NPTY-CLOEXEC-FAIL] pty master cloexec patch failed at ${remoteDir} (${platform}): ${(err as Error).message}` ) + // An exec that never answered cannot say which build is on disk, and a tree nobody can vouch + // for is exactly the one not to share. + return 'unfixed' } } diff --git a/src/main/ssh/ssh-relay-native-deps-cache.ts b/src/main/ssh/ssh-relay-native-deps-cache.ts index d8ec5e6d3e9..8400a467a91 100644 --- a/src/main/ssh/ssh-relay-native-deps-cache.ts +++ b/src/main/ssh/ssh-relay-native-deps-cache.ts @@ -26,7 +26,9 @@ * * Linux's `node-pty-1.1.0-master-cloexec-patch.cjs` also mutates in place, but it stays inside rule * 1: the deploy path runs it before promotion, and returns early on a linked entry, so it only ever - * touches a private tree. Its bytes are in the key, so a patched build never links a pre-patch entry. + * touches a private tree. Its bytes are in the key, so a patched build never links a pre-patch + * entry -- and a tree whose patch was refused or rolled back is not promoted at all, because under + * that same key it would publish the leak to every later host on the machine. */ import { createHash } from 'node:crypto' import { RELAY_REMOTE_DIR } from './relay-protocol' diff --git a/src/main/ssh/ssh-relay-native-deps-install-fixture.ts b/src/main/ssh/ssh-relay-native-deps-install-fixture.ts index d94bcc5855f..5cd20a03438 100644 --- a/src/main/ssh/ssh-relay-native-deps-install-fixture.ts +++ b/src/main/ssh/ssh-relay-native-deps-install-fixture.ts @@ -199,7 +199,8 @@ export function makeExecResponses(opts: { } // Publication is gated on the probe: only a tree this host actually loaded is shared. if (loadable) { - // The cloexec patch runs first, so what gets published is already patched. + // The cloexec patch runs first, and publication is gated on its status, so `patched` is what + // makes the promote exec below reachable at all. slots.push(`${NODE_PTY_CLOEXEC_STATUS_PREFIX}patched\n`) slots.push('') // promote the private tree into the shared native-deps cache } diff --git a/src/main/ssh/ssh-relay-pty-master-cloexec-install.test.ts b/src/main/ssh/ssh-relay-pty-master-cloexec-install.test.ts index 26bd360049e..66af01fa43c 100644 --- a/src/main/ssh/ssh-relay-pty-master-cloexec-install.test.ts +++ b/src/main/ssh/ssh-relay-pty-master-cloexec-install.test.ts @@ -131,6 +131,34 @@ describe('relay pty-master close-on-exec patch on the install path', () => { .filter((command) => command.includes(PATCH_ASSET)) } + /** Whether this deploy elected itself publisher of the shared entry. */ + function promoted(): boolean { + return vi + .mocked(execCommand) + .mock.calls.some(([, command]) => command.includes('mkdir "$cache"')) + } + + /** + * A cache-miss first install whose patch reports `status`. The promote slot is fed either way, + * so a run that wrongly promotes reads a valid response rather than falling off the end -- the + * assertion has to be the absence of the command itself, not a downstream crash. + */ + function firstInstallReporting(status: string): ExecResponse[] { + return [ + ...makeStagedFirstInstallExecPrefix(), + '', // npm install native deps + '', // chmod prebuilds + 'ORCA-NPTY-PROBE-OK\n', + '', // rm probe stderr + `ORCA-NPTY-CLOEXEC:${status}\n`, + '', // promote into the shared native-deps cache, if this deploy still gets that far + '', // clean stage root + 'DEAD', + '', // publish the per-launch credential + 'READY' + ] + } + it('runs the patch on a Linux relay once node-pty is proven loadable', async () => { const conn = makeMockConnection(sftpCapture) feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) @@ -160,6 +188,61 @@ describe('relay pty-master close-on-exec patch on the install path', () => { expect(patchAt).toBeLessThan(promoteAt) }) + it('does not publish a tree whose patch failed and rolled back', async () => { + // The script rolls `pty.cc` and `build/Release` back to the pre-patch, still-leaky build and + // reports `failed:` with exit 0, so nothing throws. Publishing that tree would be worse than + // the leak this PR closes: the key hashes the patch's bytes, so every later host on the + // machine links the entry, probes it loadable, and skips patching. Stay private instead. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const conn = makeMockConnection(sftpCapture) + feed(firstInstallReporting('failed:npm rebuild node-pty failed: gyp ERR! not found: make')) + + await deployAndLaunchRelay(conn) + + expect(patchCommands()).toHaveLength(1) + expect(promoted()).toBe(false) + expect(warn.mock.calls.map((args) => String(args[0] ?? '')).join('\n')).toContain( + '[ssh-relay][NPTY-CLOEXEC-UNSHARED]' + ) + } finally { + warn.mockRestore() + } + }) + + it('does not publish a tree the patch refused to touch', async () => { + // `skipped:` is not one verdict. Every form except `skipped:not-linux` means the patch was + // declined and the leaky build is still on disk, which is indistinguishable from `failed:` + // as far as what would get published. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const conn = makeMockConnection(sftpCapture) + feed(firstInstallReporting('skipped:earlier-attempt-failed')) + + await deployAndLaunchRelay(conn) + + expect(promoted()).toBe(false) + // A refusal exits 0, so the warn is the only signal that this host stayed leaky. + expect(warn.mock.calls.map((args) => String(args[0] ?? '')).join('\n')).toContain( + '[ssh-relay][NPTY-CLOEXEC-UNFIXED]' + ) + } finally { + warn.mockRestore() + } + }) + + it('still publishes a tree that was patched but whose isolation check could not run', async () => { + // `patched-unverified` rebuilt from patched source; only the check that watches a later child + // could not observe the result. An unobservable check is not a failed patch, and refusing to + // publish here would disable the shared cache on every host without `lsof`. + const conn = makeMockConnection(sftpCapture) + feed(firstInstallReporting('patched-unverified')) + + await deployAndLaunchRelay(conn) + + expect(promoted()).toBe(true) + }) + it('never patches through a symlink into an entry another relay already published', async () => { // A linked entry was built under a key that hashes this patch's bytes, so it is already // patched; re-running the patch would rebuild inside the shared tree.