From 190ed7d4ed0e43f1e2e543435d4ee09ab40fb7ca Mon Sep 17 00:00:00 2001 From: Haohan Lin Date: Wed, 15 Jul 2026 17:38:40 +0800 Subject: [PATCH] fix(ssh): make POSIX relay wrapper survive csh/tcsh login shells (#8709) Reconstruct remote POSIX commands with bounded printf arguments so non-POSIX SSH login shells can forward them without requiring remote base64. Preserve relay and system-SSH stdin, and centralize login-shell flag selection for csh/tcsh compatibility.\n\nValidated against real csh and tcsh OpenSSH targets with built-in and system SSH, including cold relay deployment, stdin upload, PTY I/O, file mutation, and reconnect. --- src/main/ssh/ssh-connection-utils.ts | 40 +++++++- src/main/ssh/ssh-connection.test.ts | 14 ++- src/main/ssh/ssh-login-shell-command.ts | 11 +++ .../ssh/ssh-posix-command-wrapper.test.ts | 94 +++++++++++++++++++ .../ssh/ssh-remote-node-resolution.test.ts | 35 +++++++ src/main/ssh/ssh-remote-node-resolution.ts | 11 +-- src/main/ssh/ssh-system-fallback.test.ts | 11 ++- 7 files changed, 198 insertions(+), 18 deletions(-) create mode 100644 src/main/ssh/ssh-login-shell-command.ts create mode 100644 src/main/ssh/ssh-posix-command-wrapper.test.ts diff --git a/src/main/ssh/ssh-connection-utils.ts b/src/main/ssh/ssh-connection-utils.ts index 9e496ae0fa0..de9e0b6c63f 100644 --- a/src/main/ssh/ssh-connection-utils.ts +++ b/src/main/ssh/ssh-connection-utils.ts @@ -110,10 +110,44 @@ export function shellEscape(s: string): string { return `'${s.replace(/'/g, "'\\''")}'` } +const REMOTE_COMMAND_CHUNK_MAX_BYTES = 1_024 +const REMOTE_COMMAND_PRINTF_ESCAPED_BYTES = new Set([0x21, 0x27, 0x5c]) + +function encodeRemoteCommandForPrintf(command: string): string[] { + const chunks: string[] = [] + let chunk = '' + let chunkBytes = 0 + for (const character of command) { + const codePoint = character.codePointAt(0)! + const isSafePrintableAscii = + codePoint >= 0x20 && codePoint <= 0x7e && !REMOTE_COMMAND_PRINTF_ESCAPED_BYTES.has(codePoint) + const encodedCharacter = + codePoint > 0x7f || isSafePrintableAscii + ? character + : `\\0${codePoint.toString(8).padStart(3, '0')}` + const encodedBytes = codePoint > 0x7f ? Buffer.byteLength(character) : encodedCharacter.length + if (chunkBytes + encodedBytes > REMOTE_COMMAND_CHUNK_MAX_BYTES) { + chunks.push(chunk) + chunk = '' + chunkBytes = 0 + } + chunk += encodedCharacter + chunkBytes += encodedBytes + } + chunks.push(chunk) + return chunks +} + +/** Wrap a POSIX snippet into one line that non-POSIX SSH login shells can forward. */ export function wrapRemoteCommandForPosixShell(command: string): string { - // Why: sshd asks the user's login shell to parse exec commands. Orca emits - // POSIX sh snippets; `exec` avoids leaving that shell around for relay bridges. - return `exec /bin/sh -c ${shellEscape(command)}` + // Why: csh/tcsh split multiline SSH exec strings before /bin/sh sees them. + // POSIX printf rebuilds bounded argument chunks without consuming relay stdin. + const encodedChunks = encodeRemoteCommandForPrintf(command) + const decodeAndRun = + 'decoded=$(printf %b "$@" && printf _) || exit $?; ' + + 'decoded=${decoded%_}; exec /bin/sh -c "$decoded"' + const chunkArguments = encodedChunks.map(shellEscape).join(' ') + return `exec /bin/sh -c ${shellEscape(decodeAndRun)} orca-command ${chunkArguments}` } export type SshExecOptions = { diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index d78c2ac15fe..e8c5d71234c 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -657,15 +657,19 @@ describe('SshConnection', () => { } }) - it('wraps exec commands in /bin/sh so non-POSIX login shells do not parse relay snippets', async () => { + it('wraps exec commands as a single line that csh/tcsh login shells cannot break', async () => { const conn = new SshConnection(createTarget(), createCallbacks()) await conn.connect() - await conn.exec("cd '/tmp' && ('/usr/bin/node' -e 'console.log(1)' || echo MISSING)") + const original = "cd '/tmp' && ('/usr/bin/node' -e 'console.log(1)' || echo MISSING)" + await conn.exec(original) - expect(clientInstances[0].lastExecCommand).toBe( - "exec /bin/sh -c 'cd '\\''/tmp'\\'' && ('\\''/usr/bin/node'\\'' -e '\\''console.log(1)'\\'' || echo MISSING)'" - ) + const wrapped = clientInstances[0].lastExecCommand! + // Why: sshd lets the login shell parse this first, so raw newlines let + // csh/tcsh split the command before /bin/sh receives it (issue #8701). + expect(wrapped).not.toContain('\n') + expect(wrapped).toMatch(/^exec \/bin\/sh -c '.*printf %b .*' orca-command /) + expect(wrapped).not.toContain('base64') }) it('can execute native remote commands without the POSIX shell wrapper', async () => { diff --git a/src/main/ssh/ssh-login-shell-command.ts b/src/main/ssh/ssh-login-shell-command.ts new file mode 100644 index 00000000000..3b44bafbb49 --- /dev/null +++ b/src/main/ssh/ssh-login-shell-command.ts @@ -0,0 +1,11 @@ +import { shellEscape } from './ssh-connection-utils' + +const COMMAND_ONLY_SHELLS = new Set(['sh', 'dash', 'csh', 'tcsh']) + +/** Build a command using the startup mode supported by the configured login shell. */ +export function buildSshLoginShellCommand(shell: string, command: string): string { + const shellName = shell.split('/').at(-1) + // Why: csh/tcsh reject combined -lc, while sh/dash do not need login mode here. + const mode = shellName && COMMAND_ONLY_SHELLS.has(shellName) ? '-c' : '-lc' + return `${shellEscape(shell)} ${mode} ${shellEscape(command)}` +} diff --git a/src/main/ssh/ssh-posix-command-wrapper.test.ts b/src/main/ssh/ssh-posix-command-wrapper.test.ts new file mode 100644 index 00000000000..1497dba4d40 --- /dev/null +++ b/src/main/ssh/ssh-posix-command-wrapper.test.ts @@ -0,0 +1,94 @@ +import { existsSync } from 'node:fs' +import { spawnSync } from 'node:child_process' +import { describe, expect, it } from 'vitest' +import { shellEscape, wrapRemoteCommandForPosixShell } from './ssh-connection-utils' + +describe('wrapRemoteCommandForPosixShell', () => { + it('emits one physical line without requiring a remote decoder binary', () => { + const wrapped = wrapRemoteCommandForPosixShell("printf 'a\\n'\nprintf 'b\\n'") + expect(wrapped).not.toContain('\n') + expect(wrapped).toContain('printf %b "$@"') + expect(wrapped).not.toContain('base64') + }) + + it('does not expand long commands made only of parser-safe ASCII', () => { + const command = `: #${'x'.repeat(7_000)}` + expect(wrapRemoteCommandForPosixShell(command).length - command.length).toBeLessThan(200) + }) + + it('does not octal-expand literal UTF-8 near SSH command-length limits', () => { + const command = `: #${'☃'.repeat(7_000)}` + expect(wrapRemoteCommandForPosixShell(command).length - command.length).toBeLessThan(200) + }) + + it('does not octal-expand quoted shell metacharacters near command-length limits', () => { + const command = `: #${'$"`'.repeat(2_000)}` + expect(wrapRemoteCommandForPosixShell(command).length - command.length).toBeLessThan(200) + }) + + it.skipIf(process.platform === 'win32')( + 'reconstructs multiline UTF-8 and shell metacharacters exactly', + () => { + const expected = "line one\nline 'two' \"$`\\ ! ☃\n" + const command = `printf '%s' ${shellEscape(expected)}` + const result = runThroughShell('/bin/sh', command) + + expect(result.status).toBe(0) + expect(result.stderr).toBe('') + expect(result.stdout).toBe(expected) + } + ) + + it.skipIf(process.platform === 'win32')('keeps stdin free for relay protocol bytes', () => { + const command = 'IFS= read -r frame; printf \'frame:%s\' "$frame"' + const result = runThroughShell('/bin/sh', command, 'rpc-payload\n') + + expect(result.status).toBe(0) + expect(result.stdout).toBe('frame:rpc-payload') + }) + + it.skipIf(process.platform === 'win32' || !existsSync('/bin/tcsh'))( + 'keeps every login-shell word bounded for long metacharacter-heavy commands', + () => { + const expected = '$"`'.repeat(2_000) + const command = `printf '%s' ${shellEscape(expected)}` + const result = runThroughShell('/bin/tcsh', command) + + expect(result.status).toBe(0) + expect(result.stderr).toBe('') + expect(result.stdout).toBe(expected) + } + ) + + for (const shell of [ + '/bin/sh', + '/bin/bash', + '/bin/dash', + '/bin/zsh', + '/bin/ksh', + '/bin/csh', + '/bin/tcsh', + '/usr/bin/fish', + '/usr/local/bin/fish', + '/opt/homebrew/bin/fish' + ]) { + it.skipIf(process.platform === 'win32' || !existsSync(shell))( + `survives the real ${shell} parser`, + () => { + const expected = "first\nsecond '$`\\n\\\\ ! ☃\n" + const command = `printf '%s' ${shellEscape(expected)}` + const result = runThroughShell(shell, command) + expect(result.status).toBe(0) + expect(result.stderr).toBe('') + expect(result.stdout).toBe(expected) + } + ) + } +}) + +function runThroughShell(shell: string, command: string, input?: string) { + return spawnSync(shell, ['-c', wrapRemoteCommandForPosixShell(command)], { + encoding: 'utf8', + input + }) +} diff --git a/src/main/ssh/ssh-remote-node-resolution.test.ts b/src/main/ssh/ssh-remote-node-resolution.test.ts index 84d9b9b48e7..109725e549c 100644 --- a/src/main/ssh/ssh-remote-node-resolution.test.ts +++ b/src/main/ssh/ssh-remote-node-resolution.test.ts @@ -272,6 +272,41 @@ describe('resolveRemoteNodePath', () => { }) }) + it('uses -c (not -lc) for a csh login shell, which rejects combined -lc', async () => { + // Why: csh/tcsh reject `-lc` and mis-handle `-l` here ("Bad : modifier", + // issue #8701), so the login-shell probe must drop to plain `-c`. + execCommandMock + .mockResolvedValueOnce('\n') // path probe: empty + .mockResolvedValueOnce('/bin/csh\n') // $SHELL + .mockResolvedValueOnce('/usr/local/bin/node\n') + .mockResolvedValueOnce('v20.0.0\n') + + await expect(resolveRemoteNodePath(conn)).resolves.toBe('/usr/local/bin/node') + expect(execCommandMock).toHaveBeenNthCalledWith(3, conn, `'/bin/csh' -c 'command -v node'`, { + wrapCommand: false, + timeoutMs: 8_000 + }) + }) + + it('uses -c (not -lc) for a tcsh login shell', async () => { + execCommandMock + .mockResolvedValueOnce('\n') // path probe: empty + .mockResolvedValueOnce('/usr/bin/tcsh\n') // $SHELL + .mockResolvedValueOnce('/usr/local/bin/node\n') + .mockResolvedValueOnce('v20.0.0\n') + + await expect(resolveRemoteNodePath(conn)).resolves.toBe('/usr/local/bin/node') + expect(execCommandMock).toHaveBeenNthCalledWith( + 3, + conn, + `'/usr/bin/tcsh' -c 'command -v node'`, + { + wrapCommand: false, + timeoutMs: 8_000 + } + ) + }) + // ── Failure ─────────────────────────────────────────────────────────── it('throws when both strategies find no usable node', async () => { diff --git a/src/main/ssh/ssh-remote-node-resolution.ts b/src/main/ssh/ssh-remote-node-resolution.ts index 52baa610d3f..01d6a0c7d48 100644 --- a/src/main/ssh/ssh-remote-node-resolution.ts +++ b/src/main/ssh/ssh-remote-node-resolution.ts @@ -9,6 +9,7 @@ import { } from './ssh-remote-node-install-guidance' import { execCommand } from './ssh-relay-deploy-helpers' import { isSshSessionLimitError } from './ssh-session-limit-error' +import { buildSshLoginShellCommand } from './ssh-login-shell-command' // Why: the relay requires Node.js 18+. Version managers like nvm keep every // installed version on disk, so a naive "highest version" glob can hand back @@ -150,7 +151,7 @@ async function tryResolveViaLoginShell( const nodePath = await execCommand( conn, - buildCommandInShell(shell, 'command -v node'), + buildSshLoginShellCommand(shell, 'command -v node'), commandOptions({ wrapCommand: false, timeoutMs: LOGIN_SHELL_PROBE_TIMEOUT_MS }, options) ) const candidate = nodePath.trim().split('\n')[0] @@ -171,14 +172,6 @@ async function tryResolveViaLoginShell( return null } -function buildCommandInShell(shell: string, command: string): string { - const shellName = shell.split('/').at(-1) - // Why: dash and POSIX sh do not require `-l`; when $SHELL falls back to - // /bin/sh, prefer a portable command over login-shell semantics. - const mode = shellName === 'sh' || shellName === 'dash' ? '-c' : '-lc' - return `${shellEscape(shell)} ${mode} ${shellEscape(command)}` -} - // Returns true if `nodePath` runs and reports Node >= MIN_NODE_MAJOR. // Caches nothing — this runs at most a few times per resolution (one per // candidate), and the exec round-trip dominates. diff --git a/src/main/ssh/ssh-system-fallback.test.ts b/src/main/ssh/ssh-system-fallback.test.ts index 36698e92b2f..c7f297be9a6 100644 --- a/src/main/ssh/ssh-system-fallback.test.ts +++ b/src/main/ssh/ssh-system-fallback.test.ts @@ -441,9 +441,18 @@ describe('spawnSystemSsh', () => { it('spawns a remote command through the system ssh target', () => { spawnSystemSshCommand(createTarget({ configHost: 'fdpass-host' }), 'echo hello') + // Why: the remote command stays on one line so csh/tcsh login shells cannot + // split it before /bin/sh receives it. + const args = spawnMock.mock.calls[0][1] as string[] + expect(args).toContain('--') + expect(args).toContain('deploy@fdpass-host') + const wrapped = args.at(-1)! + expect(wrapped).not.toContain('\n') + expect(wrapped).toContain('printf %b "$@"') + expect(wrapped).not.toContain('base64') expect(spawnMock).toHaveBeenCalledWith( SYSTEM_SSH_PATH, - expect.arrayContaining(['--', 'deploy@fdpass-host', "exec /bin/sh -c 'echo hello'"]), + expect.any(Array), expect.objectContaining({ stdio: ['pipe', 'pipe', 'pipe'] }) ) })