diff --git a/docs/remote-sessions.md b/docs/remote-sessions.md index dc46bb45..e6becde6 100644 --- a/docs/remote-sessions.md +++ b/docs/remote-sessions.md @@ -166,6 +166,15 @@ Key points: prerequisite probe's `command -v tmux` needs a `command` executable: macOS and the BSDs ship one, Debian and Ubuntu do not, so linking a tcsh host there is refused with `could not verify tmux`. +- **File reads never leave a quote for the login shell** (`remoteLoginShellLine()` in + `remote-files.ts`). Previews, downloads, folder grids and Edit history are multi-line + POSIX scripts holding a path someone else may have named. fish reads `\'` inside + single quotes as a quote, so the shell-quoted text flips in and out of quotes, and + csh and tcsh end a command at a newline even inside quotes, so both ran part of a path + as a command (measured). The script therefore crosses the login shell as one word of + `printf` text with those bytes in octal, and only `sh` ever reads it. Plain csh caps + a word at about 8 KB and answers `Word too long` for a larger read; tcsh has no such + cap. - Connection options come from the **same `buildSshConnectionArgs(remote)`** as the prereq probe; `-t` is inserted right after `ssh -o BatchMode=yes`, preserving historical token order. diff --git a/src/remote-files.ts b/src/remote-files.ts index 1a006d76..f06313ee 100644 --- a/src/remote-files.ts +++ b/src/remote-files.ts @@ -17,9 +17,9 @@ * * ⚠️ The path is the injection surface: it arrives from the browser (`?path=`). It is * always interpolated as a single `shellescape`d token, and the whole remote command - * is itself shellescaped into the ssh line, so the local shell and the remote shell - * each see one opaque argument. Never build a command here by concatenating a raw - * path into the string. + * is {@link remoteLoginShellLine} shellescaped into the ssh line, so the local shell + * and the remote login shell each see one opaque argument, whichever shell that is. + * Never build a command here by concatenating a raw path into the string. * * Read-only by design: previews, text reads and streaming. Writing to a remote file * is deliberately NOT implemented (docs/file-viewer-edit-plan.md §6), nor are the @@ -109,16 +109,42 @@ export class RemoteFileAccessError extends Error { } } +/** + * A POSIX script as one line that every remote login shell reads the same way, and hands + * to `sh`. + * + * ⚠️ sshd gives the command to the user's LOGIN shell, and {@link shellescape} quotes for a + * POSIX one. Inside single quotes fish takes `\'` as a literal quote, so `'\''` stops + * closing and reopening the string, and csh and tcsh end a command at a newline even there + * and run the script line by line. Both let a path act as a command (measured: a path + * holding `';touch x;'` ran `touch x` under fish, and one holding a newline ran the next + * line under tcsh). So the script travels as `printf` text with the bytes those shells + * treat specially written as octal: no quote, backslash pair, newline or `!` for the login + * shell to read, and `%` so `printf` keeps its hands off too. Only `sh` sees the script. + */ +export function remoteLoginShellLine(script: string): string { + const text = Array.from(Buffer.from(script), (byte) => { + const char = String.fromCharCode(byte); + return byte < 0x20 || byte > 0x7e || "'\\%!".includes(char) ? `\\${byte.toString(8).padStart(3, '0')}` : char; + }).join(''); + return `sh -c 'eval "$(printf "$1")"' sh '${text}'`; +} + /** * Wrap a remote shell command in the shared, shellescaped ssh line. * * The single entry point for "run this on the remote host": connection args (port, * identity, jump host, SOCKS ProxyCommand, extra `-o`) all come from - * `buildSshConnectionArgs`, and the command is ONE shellescaped token, so a path with - * spaces, quotes or `$(…)` cannot escape into the ssh command line. + * `buildSshConnectionArgs`, and the command is ONE shellescaped token holding + * {@link remoteLoginShellLine}, so a path with spaces, quotes or `$(…)` cannot escape + * into the ssh command line or the remote login shell. */ export function buildRemoteFileCommand(remote: SessionRemote, shellCommand: string): string { - return [...buildSshConnectionArgs(remote), remoteSshTarget(remote), shellescape(shellCommand)].join(' '); + return [ + ...buildSshConnectionArgs(remote), + remoteSshTarget(remote), + shellescape(remoteLoginShellLine(shellCommand)), + ].join(' '); } /** @@ -442,9 +468,9 @@ const EDIT_SOURCE_HEADER_SLACK_BYTES = 1024 * 1024; /** * Subagent requests per ssh round trip of an edit-history read. Like * {@link REMOTE_PROBE_CHUNK_SIZE}: the script is ONE argument of the ssh line, Linux caps a - * single argv string at 128 KiB, and each request adds a `case` line of 125 bytes once - * shellescaped twice (about 370 with the longest name a filesystem allows; the test pins that - * worst case). See {@link planRemoteEditSourceRounds}. + * single argv string at 128 KiB, and each request adds a `case` line of 139 bytes once + * encoded for the login shell (376 with the longest name a filesystem allows; the test pins + * that worst case). See {@link planRemoteEditSourceRounds}. */ const EDIT_SOURCE_REQUESTS_PER_ROUND_TRIP = 300; diff --git a/test/remote-edit-sources.test.ts b/test/remote-edit-sources.test.ts index ca3b6de6..211c40db 100644 --- a/test/remote-edit-sources.test.ts +++ b/test/remote-edit-sources.test.ts @@ -321,7 +321,7 @@ describe('planRemoteEditSourceRounds', () => { it('keeps every ssh line under the 128 KiB argv limit, longest names and offsets included', () => { // 255 bytes is the longest file name a filesystem allows; a request without `len` is the - // longer one, because its `''` costs 13 bytes per quote once shellescaped twice. + // longer one, because its `''` costs 16 bytes per quote once escaped and encoded. const requests: EditSourceRequest[] = Array.from({ length: 5000 }, (_, i) => ({ relPath: `subagents/agent-${String(i).padStart(6, '0')}${'x'.repeat(237)}.jsonl`, offset: Number.MAX_SAFE_INTEGER, diff --git a/test/remote-files.test.ts b/test/remote-files.test.ts index d0198c6a..160146a7 100644 --- a/test/remote-files.test.ts +++ b/test/remote-files.test.ts @@ -23,6 +23,7 @@ import { RemoteFileAccessError, buildRemoteFileCommand, buildRemoteProbeCommand, + remoteLoginShellLine, buildRemoteReadCommand, parseRemoteProbeRecord, parseRemoteProbeOutput, @@ -55,6 +56,34 @@ function remoteFixture(overrides: Partial = {}): SessionRemote { }; } +describe('remoteLoginShellLine', () => { + const run = (command: string) => execFileSync('sh', ['-c', command]).toString(); + + it('runs the script in sh, byte for byte', () => { + const script = [ + `printf '%s\\n' "it's 100% done!"`, + `printf 'back\\\\slash\\n'`, + `echo "zürich $((6 * 7))"`, + `for x in a b; do echo "$x"; done`, + ].join('\n'); + + expect(run(remoteLoginShellLine(script))).toBe(run(script)); + expect(run(remoteLoginShellLine(script))).toContain("it's 100% done!\nback\\slash\nzürich 42\na\nb\n"); + }); + + it('gives the login shell nothing it could read differently', () => { + // sshd runs this in the user's login shell, and fish and csh do not parse single quotes + // the way sh does. Neither is on CI, so this pins what makes their answer the same as + // sh's: one fixed wrapper around a word with no quote, newline, `!` or backslash pair. + const script = buildRemoteProbeCommand(["/srv/it's\n#!/bin/\\'100%.txt"]); + const match = /^sh -c 'eval "\$\(printf "\$1"\)"' sh '([\s\S]*)'$/.exec(remoteLoginShellLine(script)); + + expect(match).not.toBeNull(); + expect(match![1]).toMatch(/^(?:[\x20-\x7e]|\\[0-7]{3})*$/); + expect(match![1].replace(/\\[0-7]{3}/g, '')).not.toMatch(/['\\%!]/); + }); +}); + describe('buildRemoteFileCommand', () => { it('builds the ssh line from the shared connection args and one shellescaped command', () => { const argv = shellArgv(buildRemoteFileCommand(remoteFixture(), 'cat /etc/hostname')); @@ -65,7 +94,7 @@ describe('buildRemoteFileCommand', () => { expect(argv).toContain('ConnectTimeout=10'); expect(argv).toContain('j@192.0.2.10'); // The remote command is ONE argument, whatever it contains. - expect(argv[argv.length - 1]).toBe('cat /etc/hostname'); + expect(argv[argv.length - 1]).toBe(remoteLoginShellLine('cat /etc/hostname')); expect(argv[argv.length - 2]).toBe('j@192.0.2.10'); }); @@ -95,7 +124,7 @@ describe('buildRemoteFileCommand', () => { const command = "cat '/tmp/it''s here' ; rm -rf ~ #"; const argv = shellArgv(buildRemoteFileCommand(remoteFixture(), command)); - expect(argv[argv.length - 1]).toBe(command); + expect(argv[argv.length - 1]).toBe(remoteLoginShellLine(command)); expect(argv).not.toContain('rm'); expect(argv).not.toContain('-rf'); }); @@ -115,7 +144,7 @@ describe('buildRemoteProbeCommand', () => { const script = buildRemoteProbeCommand([nasty]); expect(script).toContain(`probe 0 '/srv/case/it'\\''s $(touch /tmp/pwned).txt'`); - expect(shellArgv(buildRemoteFileCommand(remoteFixture(), script)).at(-1)).toBe(script); + expect(shellArgv(buildRemoteFileCommand(remoteFixture(), script)).at(-1)).toBe(remoteLoginShellLine(script)); }); });