Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/main/ssh/ssh-connection-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,14 @@ export function shellEscape(s: string): string {
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)}`
if (!command.includes('\n')) {
return `exec /bin/sh -c ${shellEscape(command)}`
}
// Why: csh/tcsh login shells re-parse each line of a quoted multiline
// argument and never reach /bin/sh (#8701). Collapse to one line and let
// /bin/sh rebuild the script; eval keeps stdin free for streaming commands.
const encoded = command.replace(/\\/g, '\\\\').replace(/\n/g, '\\n')
return `exec /bin/sh -c ${shellEscape(`eval "$(printf %b ${shellEscape(encoded)})"`)}`
}

export type SshExecOptions = {
Expand Down
81 changes: 81 additions & 0 deletions src/main/ssh/ssh-remote-command-wrapping.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { spawnSync } from 'node:child_process'
import { describe, expect, it } from 'vitest'
import { wrapRemoteCommandForPosixShell } from './ssh-connection-utils'

// Why: sshd hands the wrapped command string to the user's login shell with
// `-c`, so running `<shell> -c <wrapped>` locally reproduces the remote parse
// exactly — including the csh/tcsh multiline re-parse failure from #8701.
const LOGIN_SHELLS = ['/bin/sh', '/bin/bash', '/bin/zsh', '/bin/dash', '/bin/csh', '/bin/tcsh']
const availableShells =
process.platform === 'win32'
? []
: LOGIN_SHELLS.filter((shell) => spawnSync(shell, ['-c', 'exit 0']).status === 0)

function runViaLoginShell(
loginShell: string,
wrapped: string,
input?: string
): { stdout: string; status: number | null } {
const result = spawnSync(loginShell, ['-c', wrapped], {
encoding: 'utf8',
...(input !== undefined ? { input } : {}),
timeout: 5000
})
expect(result.error).toBeUndefined()
return { stdout: result.stdout, status: result.status }
}

describe('wrapRemoteCommandForPosixShell', () => {
it('keeps single-line commands in the plain /bin/sh -c form', () => {
expect(wrapRemoteCommandForPosixShell('echo "${SHELL:-/bin/sh}"')).toBe(
`exec /bin/sh -c 'echo "\${SHELL:-/bin/sh}"'`
)
})

it('emits a single-line wrapper for multiline scripts', () => {
const wrapped = wrapRemoteCommandForPosixShell('echo one\necho two\n')
expect(wrapped).not.toContain('\n')
expect(wrapped.startsWith('exec /bin/sh -c ')).toBe(true)
})

describe.each(availableShells)('under a %s login shell', (loginShell) => {
it('runs a multiline script with quotes, backslashes, and expansions intact', () => {
const script = [
'echo START',
`name='it'\\''s %s here'`,
'for cand in "$HOME" /nonexistent',
'do',
' [ -e "$cand" ] && echo "found: $cand \\\\ $name"',
'done',
'echo END'
].join('\n')

const { stdout, status } = runViaLoginShell(
loginShell,
wrapRemoteCommandForPosixShell(script)
)
expect(status).toBe(0)
expect(stdout).toContain('START')
expect(stdout).toContain(`found: ${process.env.HOME} \\ it's %s here`)
expect(stdout).toContain('END')
})

it('propagates the script exit code', () => {
const { status } = runViaLoginShell(
loginShell,
wrapRemoteCommandForPosixShell('echo one\nexit 42\n')
)
expect(status).toBe(42)
})

it('leaves stdin available to the script', () => {
const { stdout, status } = runViaLoginShell(
loginShell,
wrapRemoteCommandForPosixShell('read line\necho "got: $line"\n'),
'stream-me\n'
)
expect(status).toBe(0)
expect(stdout).toContain('got: stream-me')
})
})
})
31 changes: 31 additions & 0 deletions src/main/ssh/ssh-remote-node-resolution.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,37 @@ describe('resolveRemoteNodePath', () => {
)
})

it('uses -c and the csh which builtin for tcsh login shells', async () => {
execCommandMock
.mockResolvedValueOnce('\n') // path probe: empty
.mockResolvedValueOnce('/bin/tcsh') // $SHELL
.mockResolvedValueOnce('/home/u/.local/bin/node\n') // which node
.mockResolvedValueOnce('v20.20.2\n')

await expect(resolveRemoteNodePath(conn)).resolves.toBe('/home/u/.local/bin/node')

// Why: csh/tcsh reject the combined `-lc` flag and have no `command` builtin.
expect(execCommandMock).toHaveBeenNthCalledWith(3, conn, `'/bin/tcsh' -c 'which node'`, {
wrapCommand: false,
timeoutMs: 8_000
})
})

it('uses -c and the csh which builtin for csh login shells', async () => {
execCommandMock
.mockResolvedValueOnce('\n') // path probe: empty
.mockResolvedValueOnce('/bin/csh') // $SHELL
.mockResolvedValueOnce('/usr/local/bin/node\n') // which node
.mockResolvedValueOnce('v20.20.2\n')

await expect(resolveRemoteNodePath(conn)).resolves.toBe('/usr/local/bin/node')

expect(execCommandMock).toHaveBeenNthCalledWith(3, conn, `'/bin/csh' -c 'which node'`, {
wrapCommand: false,
timeoutMs: 8_000
})
})

it('uses /bin/sh when the remote shell expansion falls back to it', async () => {
execCommandMock
.mockResolvedValueOnce('\n') // path probe: empty
Expand Down
15 changes: 12 additions & 3 deletions src/main/ssh/ssh-remote-node-resolution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,11 @@ async function tryResolveViaLoginShell(
return null
}

// Why: csh/tcsh have no `command` builtin; `which` is their PATH resolver.
const probe = isCshFamilyShell(shell) ? 'which node' : 'command -v node'
const nodePath = await execCommand(
conn,
buildCommandInShell(shell, 'command -v node'),
buildCommandInShell(shell, probe),
commandOptions({ wrapCommand: false, timeoutMs: LOGIN_SHELL_PROBE_TIMEOUT_MS }, options)
)
const candidate = nodePath.trim().split('\n')[0]
Expand All @@ -171,11 +173,18 @@ async function tryResolveViaLoginShell(
return null
}

function isCshFamilyShell(shell: string): boolean {
const shellName = shell.split('/').at(-1)
return shellName === 'csh' || shellName === 'tcsh'
}

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'
// /bin/sh, prefer a portable command over login-shell semantics. csh/tcsh
// reject the combined `-lc` flag outright (#8701); their non-login shells
// still read .cshrc, where EDA/HPC farms set PATH.
const mode = shellName === 'sh' || shellName === 'dash' || isCshFamilyShell(shell) ? '-c' : '-lc'
Comment on lines +176 to +187

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use Node path utilities instead of assuming path separators.

As per coding guidelines, use Node path utilities for file paths and never assume / or \. Since this function operates on remote POSIX shell paths, path.posix.basename() is the proper utility and correctly handles edge cases (like trailing slashes) that manual splitting might miss.

Note: Please ensure path is imported at the top of the file (e.g., import path from 'node:path') if it isn't already.

♻️ Proposed refactor
-function isCshFamilyShell(shell: string): boolean {
-  const shellName = shell.split('/').at(-1)
+function isCshFamilyShell(shell: string): boolean {
+  const shellName = path.posix.basename(shell)
   return shellName === 'csh' || shellName === 'tcsh'
 }
 
 function buildCommandInShell(shell: string, command: string): string {
-  const shellName = shell.split('/').at(-1)
+  const shellName = path.posix.basename(shell)
   // Why: dash and POSIX sh do not require `-l`; when $SHELL falls back to
   // /bin/sh, prefer a portable command over login-shell semantics. csh/tcsh
   // reject the combined `-lc` flag outright (`#8701`); their non-login shells
   // still read .cshrc, where EDA/HPC farms set PATH.
   const mode = shellName === 'sh' || shellName === 'dash' || isCshFamilyShell(shell) ? '-c' : '-lc'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function isCshFamilyShell(shell: string): boolean {
const shellName = shell.split('/').at(-1)
return shellName === 'csh' || shellName === 'tcsh'
}
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'
// /bin/sh, prefer a portable command over login-shell semantics. csh/tcsh
// reject the combined `-lc` flag outright (#8701); their non-login shells
// still read .cshrc, where EDA/HPC farms set PATH.
const mode = shellName === 'sh' || shellName === 'dash' || isCshFamilyShell(shell) ? '-c' : '-lc'
function isCshFamilyShell(shell: string): boolean {
const shellName = path.posix.basename(shell)
return shellName === 'csh' || shellName === 'tcsh'
}
function buildCommandInShell(shell: string, command: string): string {
const shellName = path.posix.basename(shell)
// Why: dash and POSIX sh do not require `-l`; when $SHELL falls back to
// /bin/sh, prefer a portable command over login-shell semantics. csh/tcsh
// reject the combined `-lc` flag outright (`#8701`); their non-login shells
// still read .cshrc, where EDA/HPC farms set PATH.
const mode = shellName === 'sh' || shellName === 'dash' || isCshFamilyShell(shell) ? '-c' : '-lc'

Source: Coding guidelines

return `${shellEscape(shell)} ${mode} ${shellEscape(command)}`
}

Expand Down
Loading