diff --git a/src/main/ssh/ssh-relay-cross-version-isolation.test.ts b/src/main/ssh/ssh-relay-cross-version-isolation.test.ts index 001d30a35d4..b9ee1c01489 100644 --- a/src/main/ssh/ssh-relay-cross-version-isolation.test.ts +++ b/src/main/ssh/ssh-relay-cross-version-isolation.test.ts @@ -56,6 +56,7 @@ function makeMockConnection(): SshConnection { }), sftp: vi.fn().mockResolvedValue({ mkdir: vi.fn((_p: string, cb: (err: Error | null) => void) => cb(null)), + on: vi.fn(), createWriteStream: vi.fn().mockReturnValue({ on: vi.fn((_event: string, cb: () => void) => { if (_event === 'close') { @@ -97,6 +98,7 @@ describe('cross-version isolation', () => { '', // chmod +x node '', // npm install '', // chmod prebuilds + 'OK', // node-pty probe (post-install verify) '', // touch .install-complete (finalizeInstall) '', // rm -rf .install-lock 'DEAD', // launch socket probe diff --git a/src/main/ssh/ssh-relay-deploy.ts b/src/main/ssh/ssh-relay-deploy.ts index 1a1577ef10a..747582d4713 100644 --- a/src/main/ssh/ssh-relay-deploy.ts +++ b/src/main/ssh/ssh-relay-deploy.ts @@ -216,9 +216,9 @@ async function uploadRelay( } // Why: node-pty is a native addon that can't be bundled by esbuild. It must -// be compiled on the remote host against its Node.js version and OS. We run -// `npm init -y && npm install node-pty` in the relay directory so -// `require('node-pty')` resolves to the local node_modules. +// be compiled on the remote host against its Node.js version and OS. We +// write a minimal package.json + run `npm install node-pty` in the relay +// directory so `require('node-pty')` resolves to the local node_modules. async function installNativeDeps(conn: SshConnection, remoteDir: string): Promise { const nodePath = await resolveRemoteNodePath(conn) // Why: node's bin directory must be in PATH for npm's child processes. @@ -228,24 +228,83 @@ async function installNativeDeps(conn: SshConnection, remoteDir: string): Promis const nodeBinDir = nodePath.replace(/\/node$/, '') const escapedDir = shellEscape(remoteDir) const escapedBinDir = shellEscape(nodeBinDir) + const escapedNode = shellEscape(nodePath) + + // Why: we previously ran `npm init -y` to bootstrap package.json, but npm's + // name validation rejects content-hashed dir names like + // `relay-0.1.0+07994a7870e1` (the `+` is invalid in an npm package name) + // and exits 1 — silently, since both `npm init`'s stderr and the failure + // landed inside the `2>/dev/null && ...` chain. Sidestep npm init entirely + // by writing a minimal hardcoded-name package.json over SFTP. `type:commonjs` + // pins the module system so a future Node default flip (or remote ~/.npmrc + // with type=module) cannot silently break `require('node-pty')`. + const pkgJson = `${JSON.stringify({ + name: 'orca-relay', + version: '1.0.0', + private: true, + type: 'commonjs' + })}\n` + const sftpPkg = await conn.sftp() + try { + await new Promise((resolve, reject) => { + const ws = sftpPkg.createWriteStream(`${remoteDir}/package.json`) + // Why: also wire the SFTP session's own error event — a session-level + // tear-down between createWriteStream and the stream's close/error + // would otherwise leave this promise hanging until enclosing timeouts. + sftpPkg.on('error', reject) + ws.on('close', resolve) + ws.on('error', reject) + ws.end(pkgJson) + }) + } finally { + sftpPkg.end() + } try { await execCommand( conn, - `export PATH=${escapedBinDir}:$PATH && cd ${escapedDir} && npm init -y --silent 2>/dev/null && npm install node-pty 2>&1` - ) - // Why: SFTP uploads preserve file content but not Unix execute bits. - // node-pty ships a prebuilt `spawn-helper` binary that must be executable - // for posix_spawnp to fork the PTY process. - await execCommand( - conn, - `find ${shellEscape(`${remoteDir}/node_modules/node-pty/prebuilds`)} -name spawn-helper -exec chmod +x {} + 2>/dev/null; true` + `export PATH=${escapedBinDir}:$PATH && cd ${escapedDir} && npm install node-pty 2>&1` ) } catch (err) { - // Why: node-pty install can fail if build tools (python, make, g++) are - // missing on the remote. Log the error but don't block relay startup — - // the relay will degrade gracefully (pty.spawn returns an error). - console.warn('[ssh-relay] Failed to install node-pty:', (err as Error).message) + // Why: a hard `npm install` failure (no compiler, no network, registry + // unreachable, disk full) means node-pty truly is not available on this + // host. Surface a loud, greppable warning AND propagate so the caller's + // catch leaves the dir without `.install-complete`. Future reconnects + // will detect the partial install and retry — the previous behavior + // wrote `.install-complete` anyway, stranding users on broken installs. + const msg = (err as Error).message + console.warn( + `[ssh-relay][NPTY-INSTALL-FAIL] npm install node-pty failed at ${remoteDir}: ${msg}` + ) + throw err + } + + // Why: SFTP uploads preserve file content but not Unix execute bits. + // node-pty ships a prebuilt `spawn-helper` binary that must be executable + // for posix_spawnp to fork the PTY process. + await execCommand( + conn, + `find ${shellEscape(`${remoteDir}/node_modules/node-pty/prebuilds`)} -name spawn-helper -exec chmod +x {} + 2>/dev/null; true` + ) + + // Why: defense-in-depth load-test so a silent install regression (npm exits + // 0 but node-pty is unloadable: missing prebuild, wrong arch, broken + // native binding) surfaces in deploy logs immediately instead of hiding + // behind a generic "node-pty not available" at first pty.spawn. We use + // `node -e require()` rather than `test -d` so a built-but-unloadable + // state is also caught. SSH-channel failures of the probe itself are + // intentionally NOT swallowed — they bubble to the caller's catch so we + // never confuse "probe could not run" with "node-pty is missing". + const probeOutput = ( + await execCommand( + conn, + `cd ${escapedDir} && ${escapedNode} -e 'require("node-pty"); console.log("OK")' 2>&1 || echo MISSING` + ) + ).trim() + if (!probeOutput.endsWith('OK')) { + console.warn( + `[ssh-relay][NPTY-MISSING] node-pty installed but require() failed at ${remoteDir}: ${probeOutput.slice(-500)}` + ) } } diff --git a/src/main/ssh/ssh-relay-native-deps-install.test.ts b/src/main/ssh/ssh-relay-native-deps-install.test.ts new file mode 100644 index 00000000000..321abda95c3 --- /dev/null +++ b/src/main/ssh/ssh-relay-native-deps-install.test.ts @@ -0,0 +1,296 @@ +// Why: regression coverage for the npm-init bypass + node-pty load-test +// probe in `installNativeDeps`. The original "node-pty is not available" +// bug shipped because every layer that could have caught it (chained +// shell, redirected stderr, swallowing catch, weak presence probe) was +// silent. These tests pin the contract that: +// 1. package.json is written via SFTP BEFORE `npm install` runs (order) +// 2. `npm install` failures propagate so `.install-complete` is NOT +// written by the deploy caller +// 3. the post-install probe uses `node -e require()` (load-test, not +// mere directory presence) and warns clearly on MISSING +// 4. SSH-channel failures of the probe itself are NOT swallowed +// (no `.catch(() => 'MISSING')` confusion) + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ + app: { getAppPath: () => '/mock/app' } +})) + +vi.mock('fs', () => ({ + existsSync: vi.fn().mockReturnValue(true), + readFileSync: vi.fn().mockReturnValue('0.1.0+testhash') +})) + +vi.mock('./relay-protocol', () => ({ + RELAY_VERSION: '0.1.0', + RELAY_REMOTE_DIR: '.orca-remote', + parseUnameToRelayPlatform: vi.fn().mockReturnValue('linux-x64'), + RELAY_SENTINEL: 'ORCA-RELAY v0.1.0 READY\n', + RELAY_SENTINEL_TIMEOUT_MS: 10_000 +})) + +vi.mock('./ssh-relay-deploy-helpers', () => ({ + uploadDirectory: vi.fn().mockResolvedValue(undefined), + waitForSentinel: vi.fn().mockResolvedValue({ + write: vi.fn(), + onData: vi.fn(), + onClose: vi.fn() + }), + execCommand: vi.fn(), + resolveRemoteNodePath: vi.fn().mockResolvedValue('/usr/bin/node') +})) + +vi.mock('./ssh-relay-versioned-install', () => ({ + readLocalFullVersion: vi.fn().mockReturnValue('0.1.0+testhash'), + computeRemoteRelayDir: (home: string, v: string) => `${home}/.orca-remote/relay-${v}`, + isRelayAlreadyInstalled: vi.fn().mockResolvedValue(false), + acquireInstallLock: vi.fn().mockResolvedValue(undefined), + finalizeInstall: vi.fn().mockResolvedValue(undefined), + abandonInstall: vi.fn().mockResolvedValue(undefined), + gcOldRelayVersions: vi.fn().mockResolvedValue(undefined) +})) + +vi.mock('./ssh-connection-utils', () => ({ + shellEscape: (s: string) => `'${s}'` +})) + +import { deployAndLaunchRelay } from './ssh-relay-deploy' +import { execCommand } from './ssh-relay-deploy-helpers' +import { parseUnameToRelayPlatform } from './relay-protocol' +import { finalizeInstall, isRelayAlreadyInstalled } from './ssh-relay-versioned-install' +import type { SshConnection } from './ssh-connection' + +type SftpWriteCapture = { + paths: string[] + contents: Record +} + +function makeMockConnection(capture: SftpWriteCapture): SshConnection { + const sftpCreate = (): unknown => ({ + mkdir: vi.fn((_p: string, cb: (err: Error | null) => void) => cb(null)), + on: vi.fn(), + createWriteStream: vi.fn().mockImplementation((path: string) => { + capture.paths.push(path) + let buf = '' + let closeCb: (() => void) | undefined + return { + on: vi.fn((event: string, cb: () => void) => { + if (event === 'close') { + closeCb = cb + } + }), + end: vi.fn((data?: string) => { + if (typeof data === 'string') { + buf += data + } + capture.contents[path] = buf + if (closeCb) { + setTimeout(closeCb, 0) + } + }) + } + }), + end: vi.fn() + }) + return { + exec: vi.fn().mockResolvedValue({ + on: vi.fn(), + stderr: { on: vi.fn() }, + stdin: {}, + stdout: { on: vi.fn() }, + close: vi.fn() + }), + sftp: vi.fn().mockImplementation(() => Promise.resolve(sftpCreate())) + } as unknown as SshConnection +} + +type ExecResponse = string | { reject: string } + +// Why: actual call order under our mocks is: +// 1: uname 2: $HOME 3: mkdir remoteDir (uploadRelay) +// 4: chmod +x node 5: npm install 6: chmod prebuilds +// 7: probe 8: socket DEAD 9: socket READY +function makeExecResponses(opts: { + npmInstall: 'ok' | { reject: string } + probe: 'ok' | 'missing' | { reject: string } +}): ExecResponse[] { + return [ + 'Linux x86_64', + '/home/u', + '', // mkdir remoteDir (uploadRelay) + '', // chmod +x node + opts.npmInstall === 'ok' ? '' : opts.npmInstall, + '', // chmod prebuilds + opts.probe === 'ok' + ? 'OK' + : opts.probe === 'missing' + ? 'require error: Cannot find module\nMISSING' + : opts.probe, + 'DEAD', + 'READY' + ] +} + +describe('installNativeDeps (via deployAndLaunchRelay)', () => { + let warnSpy: ReturnType + const sftpCapture: SftpWriteCapture = { paths: [], contents: {} } + + beforeEach(() => { + vi.clearAllMocks() + // Why: tests that throw mid-deploy leave unconsumed `mockResolvedValueOnce` + // entries in the execCommand queue, which then leak into the next test + // and cause it to consume `Linux x86_64` from the wrong slot. Reset to + // wipe the queue between tests. + vi.mocked(execCommand).mockReset() + sftpCapture.paths.length = 0 + for (const k of Object.keys(sftpCapture.contents)) { + delete sftpCapture.contents[k] + } + // Why: vi.clearAllMocks wipes the mockReturnValue set in the factory, + // so re-prime the mocks each test. + vi.mocked(parseUnameToRelayPlatform).mockReturnValue('linux-x64') + vi.mocked(isRelayAlreadyInstalled).mockResolvedValue(false) + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + afterEach(() => { + warnSpy.mockRestore() + }) + + function feed(execResponses: ExecResponse[]): void { + const mockExec = vi.mocked(execCommand) + for (const r of execResponses) { + if (typeof r === 'string') { + mockExec.mockResolvedValueOnce(r) + } else { + mockExec.mockRejectedValueOnce(new Error(r.reject)) + } + } + } + + it('writes a hardcoded package.json BEFORE running npm install', async () => { + const conn = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) + + await deployAndLaunchRelay(conn) + + const pkgPath = sftpCapture.paths.find((p) => p.endsWith('/package.json')) + expect(pkgPath, 'package.json must be written via SFTP').toBeTruthy() + + const written = sftpCapture.contents[pkgPath as string] + expect(written).toBeTruthy() + const parsed = JSON.parse(written) as Record + expect(parsed.name).toBe('orca-relay') + expect(parsed.version).toBe('1.0.0') + expect(parsed.private).toBe(true) + // Why: pin commonjs so a future Node default flip doesn't silently + // break `require('node-pty')`. + expect(parsed.type).toBe('commonjs') + + const execCalls = vi.mocked(execCommand).mock.calls.map(([, c]) => c) + const npmInstallIdx = execCalls.findIndex((c) => c.includes('npm install node-pty')) + expect(npmInstallIdx).toBeGreaterThanOrEqual(0) + // The SFTP write resolves before deploy continues into npm install, + // so by the time `npm install` was queued, package.json must already be + // present in our capture. + expect(sftpCapture.contents[pkgPath as string]).toBeTruthy() + }) + + it('propagates a hard `npm install` failure so the deploy aborts before finalizeInstall', async () => { + const conn = makeMockConnection(sftpCapture) + feed( + makeExecResponses({ + npmInstall: { reject: 'npm ERR! E404 Not Found node-pty' }, + probe: 'ok' + }) + ) + + await expect(deployAndLaunchRelay(conn)).rejects.toThrow(/npm ERR/) + + // The crucial regression: `.install-complete` must NOT have been written. + // Previously the catch swallowed the throw and finalizeInstall ran anyway. + expect(vi.mocked(finalizeInstall)).not.toHaveBeenCalled() + + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-INSTALL-FAIL]'))).toBe(true) + }) + + it('warns clearly when node-pty installs but require() fails (built-but-unloadable)', async () => { + const conn = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'missing' })) + + await deployAndLaunchRelay(conn) + + // Probe failure is non-fatal (graceful degradation), but it MUST log the + // greppable token so a user filing a bug pastes something that points + // at the real cause. + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-MISSING]'))).toBe(true) + + // finalizeInstall still runs — relay can serve fs/git/preflight. + expect(vi.mocked(finalizeInstall)).toHaveBeenCalled() + }) + + it('lets a probe SSH-channel failure bubble up rather than silently mapping to MISSING', async () => { + const conn = makeMockConnection(sftpCapture) + feed( + makeExecResponses({ + npmInstall: 'ok', + probe: { reject: 'SSH channel closed unexpectedly' } + }) + ) + + await expect(deployAndLaunchRelay(conn)).rejects.toThrow(/SSH channel/) + + // We must NOT have logged NPTY-MISSING — that would conflate "probe + // could not run" with "node-pty is missing", which is exactly the + // class of bug the original outage came from. + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-MISSING]'))).toBe(false) + + expect(vi.mocked(finalizeInstall)).not.toHaveBeenCalled() + }) + + it('uses `node -e require()` rather than `test -d` so unloadable installs are caught', async () => { + const conn = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) + + await deployAndLaunchRelay(conn) + + const probeCmds = vi + .mocked(execCommand) + .mock.calls.map(([, c]) => c) + .filter((c) => c.includes(`require("node-pty")`)) + + // Why: the probe shape must invoke the deployed node binary against + // require('node-pty'). A weaker probe (test -d) could pass even when + // the native binding load is broken. + expect(probeCmds.length).toBeGreaterThan(0) + expect(probeCmds[0]).toMatch(/node['"]?\s+-e/) + }) + + it('writes an idempotent package.json (same bytes on every install)', async () => { + // First install run. + const conn1 = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) + await deployAndLaunchRelay(conn1) + const firstPath = sftpCapture.paths.find((p) => p.endsWith('/package.json')) as string + const first = sftpCapture.contents[firstPath] + + // Reset capture, run again as if it were a fresh install of the same dir. + sftpCapture.paths.length = 0 + for (const k of Object.keys(sftpCapture.contents)) { + delete sftpCapture.contents[k] + } + vi.mocked(execCommand).mockReset() + + const conn2 = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) + await deployAndLaunchRelay(conn2) + const secondPath = sftpCapture.paths.find((p) => p.endsWith('/package.json')) as string + const second = sftpCapture.contents[secondPath] + + expect(second).toBe(first) + }) +}) diff --git a/src/relay/git-exec-validator.test.ts b/src/relay/git-exec-validator.test.ts index ce168eede86..0092bfbc5ad 100644 --- a/src/relay/git-exec-validator.test.ts +++ b/src/relay/git-exec-validator.test.ts @@ -28,7 +28,16 @@ describe('validateGitExecArgs', () => { [['config', '--get-all', 'remote.origin.url']], [['config', '--list']], [['config', '-l']], - [['config', '--get-regexp', 'user']] + [['config', '--get-regexp', 'user']], + [['for-each-ref', '--format=%(refname)', 'refs/remotes']], + [ + [ + 'for-each-ref', + '--format=%(refname)%00%(refname:short)', + '--sort=-committerdate', + 'refs/heads/*foo*' + ] + ] ])('allows %j', (args) => { expectAllowed(args) }) diff --git a/src/relay/git-exec-validator.ts b/src/relay/git-exec-validator.ts index fa22356c53a..b881fbb6a26 100644 --- a/src/relay/git-exec-validator.ts +++ b/src/relay/git-exec-validator.ts @@ -17,6 +17,7 @@ const ALLOWED_GIT_SUBCOMMANDS = new Set([ 'symbolic-ref', 'merge-base', 'ls-files', + 'for-each-ref', 'config' ]) const CONFIG_READ_ONLY_FLAGS = new Set(['--get', '--get-all', '--list', '--get-regexp', '-l'])