diff --git a/.gitkeep b/.gitkeep new file mode 100644 index 00000000..d523da0e --- /dev/null +++ b/.gitkeep @@ -0,0 +1 @@ +# .gitkeep file auto-generated at 2026-09-06T21:34:11.215Z for PR creation at branch issue-41-448ca60fbc42 for issue https://github.com/link-foundation/command-stream/issues/41 \ No newline at end of file diff --git a/experiments/issue-41-broken-quoting.mjs b/experiments/issue-41-broken-quoting.mjs new file mode 100644 index 00000000..61c2e0a6 --- /dev/null +++ b/experiments/issue-41-broken-quoting.mjs @@ -0,0 +1,18 @@ +import { $ } from '../js/src/$.mjs'; +for (const v of [`"it's"`, `''`, `'; touch /tmp/pwned-41; '`]) { + const c = $({ mirror: false })`printf ${{ raw: '"[%s]\\n"' }} ${v}`; + console.log(JSON.stringify(v), 'built=', JSON.stringify(c.spec.command)); + try { + const r = await c; + console.log( + ' out=', + JSON.stringify(r.stdout), + 'code=', + r.code, + 'err=', + JSON.stringify(r.stderr.slice(0, 80)) + ); + } catch (e) { + console.log(' THREW', e.message); + } +} diff --git a/experiments/issue-41-competitors.mjs b/experiments/issue-41-competitors.mjs new file mode 100644 index 00000000..131d960d --- /dev/null +++ b/experiments/issue-41-competitors.mjs @@ -0,0 +1,74 @@ +// Competitor comparison for issue #41: how does an interpolated path with +// spaces (or quotes) reach the child process in each library? +// +// Reference: `prog "$V"` in /bin/sh - the value is always one argument. +// Run with: bun experiments/issue-41-competitors.mjs +import { execFileSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { $ } from '../js/src/$.mjs'; + +const PRINTER = fileURLToPath( + new URL('../js/tests/fixtures/argprint.mjs', import.meta.url) +); + +const VALUES = [ + '/Users/john/My Documents/report.txt', + "'/tmp/pre single quoted/f.txt'", + '"/tmp/pre double quoted/f.txt"', + "/tmp/it's a dir/f.txt", + '/tmp/$HOME dir/f.txt', +]; + +const parse = (stdout) => + [...stdout.matchAll(/^ARG\[([\s\S]*?)\]$/gm)].map((m) => m[1]); + +const shReference = (value) => + parse( + execFileSync('/bin/sh', ['-c', `node ${PRINTER} "$V"`], { + env: { ...process.env, V: value }, + encoding: 'utf8', + }) + ); + +const commandStream = async (value) => + parse((await $({ mirror: false })`node ${PRINTER} ${value}`).stdout); + +async function bunShell(value) { + if (typeof Bun === 'undefined') { + return null; + } + const { $: bun$ } = await import('bun'); + return parse( + (await bun$`node ${PRINTER} ${value}`.quiet()).stdout.toString() + ); +} + +async function execaRun(value) { + try { + const { execa } = await import('execa'); + return parse((await execa`node ${PRINTER} ${value}`).stdout + '\n'); + } catch { + return null; // not installed + } +} + +for (const value of VALUES) { + const expected = shReference(value); + const rows = { + 'sh "$V"': expected, + 'command-stream': await commandStream(value), + 'bun $': await bunShell(value), + execa: await execaRun(value), + }; + console.log(`\nvalue ${JSON.stringify(value)}`); + for (const [name, args] of Object.entries(rows)) { + if (args === null) { + console.log(` ${name.padEnd(14)} (not available here)`); + continue; + } + const same = JSON.stringify(args) === JSON.stringify(expected); + console.log( + ` ${name.padEnd(14)} ${same ? 'same as sh' : 'DIFFERS '} ${JSON.stringify(args)}` + ); + } +} diff --git a/experiments/issue-41-diff-sh.mjs b/experiments/issue-41-diff-sh.mjs new file mode 100644 index 00000000..445a14b1 --- /dev/null +++ b/experiments/issue-41-diff-sh.mjs @@ -0,0 +1,51 @@ +// Differential test: command-stream interpolation vs POSIX sh "$var" +import { $ } from '../js/src/$.mjs'; +import { execFileSync } from 'child_process'; + +const values = [ + '/Users/john/My Documents/report.txt', + "/tmp/it's a dir/file.txt", + '/tmp/quoted "name"/f.txt', + "'/already/single quoted/path'", + '"/already/double quoted/path"', + '/tmp/back\\slash dir/f.txt', + '/tmp/$HOME dir/f.txt', + '/tmp/tab\there/f.txt', + '/tmp/new\nline/f.txt', + ' leading and trailing ', + '', + '/tmp/glob*dir/f.txt', + '/tmp/~tilde dir/f.txt', + '/tmp/semi;colon dir/f.txt', + '/tmp/(paren) dir/f.txt', + '/tmp/emoji 🚀 dir/f.txt', + 'C:\\Program Files\\App\\app.exe', +]; + +function shArgs(value) { + // What a POSIX shell gives argv when you write: prog "$var" + return execFileSync('/bin/sh', ['-c', 'printf "[%s]\\n" "$V"'], { + env: { ...process.env, V: value }, + encoding: 'utf8', + }); +} + +let fails = 0; +for (const v of values) { + const expected = shArgs(v); + const built = $({ mirror: false })`printf ${{ raw: '"[%s]\\n"' }} ${v}`; + let actual; + try { + actual = (await built).stdout; + } catch (e) { + actual = 'THREW ' + e.message; + } + const ok = actual === expected; + if (!ok) { + fails++; + } + console.log( + `${ok ? 'OK ' : 'FAIL'} value=${JSON.stringify(v)}\n built=${JSON.stringify(built.spec.command)}\n sh =${JSON.stringify(expected)}\n cs =${JSON.stringify(actual)}` + ); +} +console.log('failures:', fails, '/', values.length); diff --git a/experiments/issue-41-injection.mjs b/experiments/issue-41-injection.mjs new file mode 100644 index 00000000..3f009494 --- /dev/null +++ b/experiments/issue-41-injection.mjs @@ -0,0 +1,6 @@ +import { $ } from '../js/src/$.mjs'; +const evil = `"' ; touch /tmp/pwned-41b ; '"`; +const c = $({ mirror: false })`printf ${{ raw: '"[%s]\\n"' }} ${evil}`; +console.log('built=', JSON.stringify(c.spec.command)); +const r = await c; +console.log('code', r.code, JSON.stringify(r.stdout), JSON.stringify(r.stderr)); diff --git a/experiments/issue-41-matrix.mjs b/experiments/issue-41-matrix.mjs new file mode 100644 index 00000000..cfda2a52 --- /dev/null +++ b/experiments/issue-41-matrix.mjs @@ -0,0 +1,61 @@ +import { $ } from '../js/src/$.mjs'; +import fs from 'fs'; + +const dir = '/tmp/space test dir'; +const filePath = `${dir}/report file.txt`; +fs.mkdirSync(dir, { recursive: true }); +fs.writeFileSync(filePath, 'hello content\n'); + +async function t(name, fn) { + try { + const r = await fn(); + console.log( + `[${name}] code=${r.code} out=${JSON.stringify(r.stdout)} err=${JSON.stringify((r.stderr || '').slice(0, 120))}` + ); + } catch (e) { + console.log(`[${name}] THREW ${e.message}`); + } +} + +// redirection to a path with spaces +await t( + 'redirect out', + () => $({ mirror: false })`echo hi > ${dir}/out file.txt` +); +await t( + 'redirect out interp full path', + () => $({ mirror: false })`echo hi > ${dir + '/out2.txt'}` +); +await t('read back', () => $({ mirror: false })`cat ${dir + '/out2.txt'}`); +// virtual/builtin commands +await t('cd virtual', () => $({ mirror: false })`cd ${dir}`); +await t('pwd after cd', () => $({ mirror: false })`cd ${dir} && pwd`); +await t('echo builtin', () => $({ mirror: false })`echo ${'a b'} ${'c d'}`); +// pipeline with spaces +await t( + 'pipe grep', + () => $({ mirror: false })`cat ${filePath} | grep ${'hello content'}` +); +// array interpolation +await t('array args', () => $({ mirror: false })`echo ${['a b', 'c d']}`); +// sh -c inner +await t('sh -c', () => $({ mirror: false })`sh -c "cat '${filePath}'"`); +await t( + 'sh -c double', + () => $({ mirror: false })`sh -c "cat \"${filePath}\""` +); +// trailing/leading spaces value +await t( + 'value with quotes literal', + () => $({ mirror: false })`echo ${"'quoted'"}` +); +await t('value with tab', () => $({ mirror: false })`echo ${'a\tb'} | cat -A`); +// backslash in path +await t( + 'backslash path', + () => $({ mirror: false })`echo ${'/tmp/back\\slash'}` +); +// env var in path should not expand +await t('dollar path', () => $({ mirror: false })`echo ${'/tmp/$HOME/x'}`); +// glob dir with space +await t('ls glob', () => $({ mirror: false })`ls ${dir}/*.txt`); diff --git a/experiments/issue-41-nested.mjs b/experiments/issue-41-nested.mjs new file mode 100644 index 00000000..71944c28 --- /dev/null +++ b/experiments/issue-41-nested.mjs @@ -0,0 +1,6 @@ +import { $ } from '../js/src/$.mjs'; +const filePath = '/tmp/space test dir/report file.txt'; +const cmd = $({ mirror: false })`sh -c "cat \"${filePath}\""`; +console.log('BUILT:', JSON.stringify(cmd.spec.command)); +const r = await cmd; +console.log('code', r.code, JSON.stringify(r.stdout), JSON.stringify(r.stderr)); diff --git a/experiments/issue-41-newline.mjs b/experiments/issue-41-newline.mjs new file mode 100644 index 00000000..9405fc43 --- /dev/null +++ b/experiments/issue-41-newline.mjs @@ -0,0 +1,22 @@ +import { $ } from '../js/src/$.mjs'; +import { fileURLToPath } from 'node:url'; +const P = fileURLToPath( + new URL('../js/tests/fixtures/argprint.mjs', import.meta.url) +); +const v = '/tmp/new\nline/f.txt'; +for (const build of [ + () => $({ mirror: false })`node ${P} ${v}`, + () => $({ mirror: false })`printf "[%s]\n" ${v}`, +]) { + const c = build(); + console.log('BUILT', JSON.stringify(c.spec.command)); + const r = await c; + console.log( + ' code', + r.code, + 'out', + JSON.stringify(r.stdout), + 'err', + JSON.stringify(r.stderr) + ); +} diff --git a/experiments/issue-41-repro.mjs b/experiments/issue-41-repro.mjs new file mode 100644 index 00000000..869e6373 --- /dev/null +++ b/experiments/issue-41-repro.mjs @@ -0,0 +1,31 @@ +import { $ } from '../js/src/$.mjs'; + +const dir = '/tmp/space test dir'; +const filePath = `${dir}/report file.txt`; + +async function t(name, fn) { + try { + const r = await fn(); + console.log( + `[${name}] code=${r.code} stdout=${JSON.stringify(r.stdout)} stderr=${JSON.stringify(r.stderr)}` + ); + } catch (e) { + console.log(`[${name}] THREW ${e.message}`); + } +} + +await t('cat unquoted interp', () => $({ mirror: false })`cat ${filePath}`); +await t('cat quoted interp', () => $({ mirror: false })`cat "${filePath}"`); +await t('ls dir', () => $({ mirror: false })`ls ${dir}`); +await t('echo path', () => $({ mirror: false })`echo ${filePath}`); +await t('cd + pwd', () => $({ mirror: false })`cd ${dir} && pwd`); +await t( + 'builtin cat via virtual?', + () => $({ mirror: false })`cat ${filePath} | head -1` +); +await t( + 'test -f', + () => $({ mirror: false })`test -f ${filePath} && echo EXISTS` +); +await t('cp', () => $({ mirror: false })`cp ${filePath} ${dir}/copy\ file.txt`); +console.log('cmd:', $({ mirror: false })`cat ${filePath}`.spec.command); diff --git a/js/.changeset/issue-41-paths-with-spaces.md b/js/.changeset/issue-41-paths-with-spaces.md new file mode 100644 index 00000000..4166943a --- /dev/null +++ b/js/.changeset/issue-41-paths-with-spaces.md @@ -0,0 +1,22 @@ +--- +'command-stream': minor +--- + +Interpolate every value as exactly one literal argument, like `"$var"` in a +POSIX shell. `quote()` no longer treats a value that starts and ends with a +matching quote as ready-made shell syntax: those quote characters are part of +the value, so a path such as `/My Documents/report.txt` (or a pre-quoted one) +reaches the command intact (issue #41). This matches `sh`, Bun's `$`, zx and +execa, and it removes two defects of the old heuristic - `quote('"it\'s"')` +emitted the unterminated string `'"it\'s"'`, and a value like +`"' ; touch /tmp/pwned ; '"` was spliced in as shell syntax and executed. The +previous behavior is available for balanced values only, via +`shell.preQuotedPassthrough(true)`, `setPreQuotedPassthroughEnabled(true)`, or +`COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1`. + +Also ignore `EPIPE` when a pipeline stage closes the stdin of a process that +has already exited. Closing (or writing to) that pipe is a normal race in a +pipeline - a shell ignores it - but the streaming pipeline let the rejection +escape as an unhandled error, which could fail an otherwise successful +command. This matches the Rust implementation, which already discards those +write and shutdown errors. diff --git a/js/BEST-PRACTICES.md b/js/BEST-PRACTICES.md index e7f7a1b5..01fe97dc 100644 --- a/js/BEST-PRACTICES.md +++ b/js/BEST-PRACTICES.md @@ -115,6 +115,34 @@ await $`bash -c "${script}"`; To restore the old always-quote behavior, call `shell.quoteContext(false)` or set `COMMAND_STREAM_QUOTE_CONTEXT=0`. +### Paths With Spaces + +Interpolate the path as-is. An interpolated value always becomes exactly one +argument, so spaces, apostrophes, and other special characters need no help +from you - the same guarantee as `"$path"` in a shell script: + +```javascript +const file = '/Users/john/My Documents/report.txt'; + +await $`cat ${file}`; // one argument, spaces included +await $`cp ${file} ${'/tmp/My Backups/'}`; +``` + +Never pre-quote the value. Quote characters you add become part of the file +name, exactly as `sh` would treat them: + +```javascript +// WRONG: looks for a file whose name starts and ends with a quote +await $`cat ${"'" + file + "'"}`; + +// RIGHT +await $`cat ${file}`; +``` + +Before v0.21 a value that started and ended with a matching quote was spliced +in as shell syntax. Set `COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1` or call +`shell.preQuotedPassthrough(true)` if you still depend on that. + ### Using raw() for Trusted Commands Only use `raw()` with trusted, hardcoded command strings: diff --git a/js/README.md b/js/README.md index 1c610072..34229d13 100644 --- a/js/README.md +++ b/js/README.md @@ -222,14 +222,57 @@ await $`echo ${pathWithSpaces}`; // pathWithSpaces = "/my path/file" → echo '/ // Special characters that trigger auto-quoting: // Spaces, $, ;, |, &, >, <, `, *, ?, [, ], {, }, (, ), !, #, and others -// User-provided quotes are preserved +// Quote characters inside a value are data, never shell syntax const quotedPath = "'/path with spaces/file'"; -await $`cat ${quotedPath}`; // → cat '/path with spaces/file' (no double-quoting!) +await $`cat ${quotedPath}`; // → cat with the argument: '/path with spaces/file' const doubleQuoted = '"/path with spaces/file"'; -await $`cat ${doubleQuoted}`; // → cat '"/path with spaces/file"' (preserves intent) +await $`cat ${doubleQuoted}`; // → cat with the argument: "/path with spaces/file" ``` +### Paths With Spaces + +An interpolated value always becomes **exactly one argument**, spaces and all — +the same guarantee you get from `"$path"` in a shell script, and the same +behavior as Bun's `$`, zx, and execa: + +```javascript +const file = '/Users/john/My Documents/report.txt'; + +await $`cat ${file}`; // one argument: /Users/john/My Documents/report.txt +await $`cp ${file} ${'/tmp/My Backups/'}`; // both paths stay intact +await $`ls -la ${'/Applications/Visual Studio Code.app'}`; +``` + +Do **not** pre-quote the path yourself. Quote characters you put in the value +are literal characters of the file name, exactly as `sh` treats them: + +```javascript +// ❌ looks for a file whose name literally starts and ends with a quote +await $`cat ${`'${file}'`}`; + +// ✅ just interpolate the path +await $`cat ${file}`; +``` + +**Opting out.** Before v0.21 a value that started and ended with a matching +quote was spliced into the command as shell syntax instead of being quoted. +That diverged from `sh` and could produce unrunnable commands: the value +`"it's"` was emitted as `'"it's"'`, an unterminated string. If you depend on +the old behavior: + +```javascript +import { shell, setPreQuotedPassthroughEnabled } from 'command-stream'; + +shell.preQuotedPassthrough(true); // or: setPreQuotedPassthroughEnabled(true) +shell.preQuotedPassthrough(false); // back to sh-like quoting +setPreQuotedPassthroughEnabled(null); // follow the environment again +``` + +Or set `COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1` for a whole process. Even then +only _balanced_ values are passed through, so a value like `"a" ; rm -rf / ; "b"` +is still quoted rather than executed. + ### Interpolating Inside Your Own Quotes Quoting is context-aware: an interpolated value is quoted only where a quote is diff --git a/js/examples/README.md b/js/examples/README.md index 07bbf25c..19f6c956 100644 --- a/js/examples/README.md +++ b/js/examples/README.md @@ -158,6 +158,11 @@ The simplest examples to get started: - `ctrl-c-virtual-command.mjs` - Virtual command CTRL+C - `ctrl-c-concurrent-processes.mjs` - Multiple concurrent processes +### 🔤 Quoting and Paths + +- `paths-with-spaces.mjs` - File paths with spaces need no manual quoting (GitHub issue #41) +- `quote-context-bash-c.mjs` - Interpolating inside your own quotes (GitHub issue #49) + ### 🔧 Syntax Comparisons **Feature Comparisons:** diff --git a/js/examples/paths-with-spaces.mjs b/js/examples/paths-with-spaces.mjs new file mode 100644 index 00000000..24459afa --- /dev/null +++ b/js/examples/paths-with-spaces.mjs @@ -0,0 +1,39 @@ +#!/usr/bin/env node +// Working with file paths that contain spaces (issue #41). +// An interpolated value always becomes exactly one argument - the same +// guarantee as "$path" in a POSIX shell - so paths need no manual quoting. +import { $ } from '../src/$.mjs'; +import { mkdtempSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +const $q = $({ mirror: false }); + +const dir = mkdtempSync(join(tmpdir(), 'my documents ')); +const report = join(dir, 'annual report 2026.txt'); +const backup = join(dir, 'annual report 2026.backup.txt'); +writeFileSync(report, 'quarterly numbers\n'); + +try { + // Read a file whose path contains spaces - no quotes in the template. + console.log((await $q`cat ${report}`).stdout.trim()); + + // Several paths with spaces in one command. + await $q`cp ${report} ${backup}`; + console.log((await $q`ls ${backup}`).stdout.trim()); + + // Redirection and pipelines keep the path in one piece too. + await $q`echo appended >> ${backup}`; + console.log((await $q`cat ${backup} | wc -l`).stdout.trim()); + + // Enter a directory whose name contains spaces. + console.log((await $q`cd ${dir} && pwd`).stdout.trim()); + + // Do NOT pre-quote: the quotes become part of the file name, exactly as + // `cat "'$path'"` behaves in sh, so the file is not found. + const preQuoted = `'${report}'`; + const missing = await $q`cat ${preQuoted}`; + console.log(`pre-quoted path fails as in sh: code=${missing.code}`); +} finally { + rmSync(dir, { recursive: true, force: true }); +} diff --git a/js/src/$.mjs b/js/src/$.mjs index 45bd9848..bc629be7 100755 --- a/js/src/$.mjs +++ b/js/src/$.mjs @@ -12,11 +12,13 @@ import { } from './$.state.mjs'; import { buildShellCommand, + isPreQuotedPassthroughEnabled, isQuoteContextEnabled, quote, quoteForContext, quoteLiteral, raw, + setPreQuotedPassthroughEnabled, setQuoteContextEnabled, } from './$.quote.mjs'; import { @@ -323,6 +325,13 @@ const shell = { // the historical behaviour of always single-quoting interpolated values, // or null to follow COMMAND_STREAM_QUOTE_CONTEXT again. quoteContext: (enable = true) => setQuoteContextEnabled(enable), + + // Legacy pre-quoted passthrough (off by default): when enabled, a value that + // is already wrapped in matching quotes is spliced in as shell syntax instead + // of being treated as literal text. Pass null to follow + // COMMAND_STREAM_PREQUOTED_PASSTHROUGH again. + preQuotedPassthrough: (enable = true) => + setPreQuotedPassthroughEnabled(enable), }; // Virtual command registration API @@ -436,6 +445,8 @@ export { quoteLiteral, isQuoteContextEnabled, setQuoteContextEnabled, + isPreQuotedPassthroughEnabled, + setPreQuotedPassthroughEnabled, create, raw, literal, diff --git a/js/src/$.process-runner-pipeline.mjs b/js/src/$.process-runner-pipeline.mjs index 413d8905..102fcc19 100644 --- a/js/src/$.process-runner-pipeline.mjs +++ b/js/src/$.process-runner-pipeline.mjs @@ -308,15 +308,16 @@ function createStringStream(data) { * Pipe stream to process stdin * @param {ReadableStream} stream - Input stream * @param {object} proc - Process + * @returns {Promise|undefined} Resolves when the pump has finished */ -function pipeStreamToProcess(stream, proc) { +export function pipeStreamToProcess(stream, proc) { if (!stream || !proc.stdin) { - return; + return undefined; } const reader = stream.getReader(); const writer = proc.stdin.getWriter ? proc.stdin.getWriter() : proc.stdin; - (async () => { + const promise = (async () => { try { while (true) { const { done, value } = await reader.read(); @@ -347,13 +348,25 @@ function pipeStreamToProcess(stream, proc) { } } finally { reader.releaseLock(); - if (writer.close) { - await writer.close(); - } else if (writer.end) { - writer.end(); + // The downstream process may already have exited and closed its stdin, + // in which case closing the writer raises EPIPE. That is a normal + // pipeline race, not an error worth propagating as an unhandled + // rejection. + try { + if (writer.close) { + await writer.close(); + } else if (writer.end) { + writer.end(); + } + } catch (error) { + StreamUtils.handleStreamError(error, 'stream writer close', false); } } })(); + + return promise.catch((error) => { + StreamUtils.handleStreamError(error, 'stream pipe', false); + }); } /** diff --git a/js/src/$.quote.mjs b/js/src/$.quote.mjs index 7e2201f6..1b928290 100644 --- a/js/src/$.quote.mjs +++ b/js/src/$.quote.mjs @@ -3,8 +3,74 @@ import { trace } from './$.trace.mjs'; +// --------------------------------------------------------------------------- +// Pre-quoted passthrough (legacy, off by default) +// +// Older versions treated a value that happened to start and end with a quote +// character as "already quoted" and spliced it into the command as shell +// syntax, so the value `'/My Documents/x'` reached the command as +// `/My Documents/x` - the quotes vanished. sh does the opposite: `"$var"` +// always yields the value verbatim, quote characters included, which is also +// what Bun's $, zx and execa do. Worse, the heuristic could hand the shell +// unbalanced quotes: `"' ; touch pwned ; '"` was spliced in as-is and the +// injected command ran (issue #41). +// +// The heuristic is therefore off by default. Set +// COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1 (or call +// setPreQuotedPassthroughEnabled(true)) to restore it for code that relied on +// hand-quoted values; even then only values that stay balanced are passed +// through, so the injection above can no longer happen. +// --------------------------------------------------------------------------- + +let preQuotedPassthroughEnabled = null; + +/** + * Enable or disable the legacy pre-quoted passthrough heuristic. + * @param {boolean|null} enabled - true/false to force, null to follow the env + * @returns {boolean} The effective setting after the change + */ +export function setPreQuotedPassthroughEnabled(enabled) { + preQuotedPassthroughEnabled = enabled === null ? null : Boolean(enabled); + return isPreQuotedPassthroughEnabled(); +} + +/** + * Whether the legacy pre-quoted passthrough heuristic is active. + * @returns {boolean} true when enabled + */ +export function isPreQuotedPassthroughEnabled() { + if (preQuotedPassthroughEnabled !== null) { + return preQuotedPassthroughEnabled; + } + return process.env.COMMAND_STREAM_PREQUOTED_PASSTHROUGH === '1'; +} + +// Alphanumerics plus the punctuation a POSIX shell leaves alone; anything else +// (spaces above all) has to be quoted. +const SAFE_UNQUOTED_PATTERN = /^[a-zA-Z0-9_\-./=,+@:]+$/; + /** - * Quote a value for safe shell interpolation + * Whether a value can be spliced into the command as-is under the legacy + * pre-quoted passthrough heuristic, i.e. it is wrapped in matching quotes and + * contains none of that quote character inside. + * @param {string} value - Raw value + * @returns {boolean} true when the value is balanced, quoted shell syntax + */ +function isBalancedQuotedValue(value) { + const quoteChar = value[0]; + if ((quoteChar !== "'" && quoteChar !== '"') || value.length < 2) { + return false; + } + const inner = value.slice(1, -1); + return value.endsWith(quoteChar) && !inner.includes(quoteChar); +} + +/** + * Quote a value for safe shell interpolation. + * + * The value is always treated as literal text - exactly one argument, spaces + * and quote characters included - which is what `"$var"` does in sh. + * * @param {*} value - Value to quote * @returns {string} Safely quoted string */ @@ -22,35 +88,18 @@ export function quote(value) { return "''"; } - // If the value is already properly quoted and doesn't need further escaping, - // check if we can use it as-is or with simpler quoting - if (value.startsWith("'") && value.endsWith("'") && value.length >= 2) { - // If it's already single-quoted and doesn't contain unescaped single quotes in the middle, - // we can potentially use it as-is - const inner = value.slice(1, -1); - if (!inner.includes("'")) { - // The inner content has no single quotes, so the original quoting is fine - return value; - } - } - - if (value.startsWith('"') && value.endsWith('"') && value.length > 2) { - // If it's already double-quoted, wrap it in single quotes to preserve it - return `'${value}'`; + if (isPreQuotedPassthroughEnabled() && isBalancedQuotedValue(value)) { + // Legacy: the caller quoted the value themselves, so use it as shell syntax. + return value; } - // Check if the string needs quoting at all - // Safe characters: alphanumeric, dash, underscore, dot, slash, colon, equals, comma, plus - // This regex matches strings that DON'T need quoting - const safePattern = /^[a-zA-Z0-9_\-./=,+@:]+$/; - - if (safePattern.test(value)) { + if (SAFE_UNQUOTED_PATTERN.test(value)) { // The string is safe and doesn't need quoting return value; } - // Default behavior: wrap in single quotes and escape any internal single quotes - // This handles spaces, special shell characters, etc. + // Wrap in single quotes and escape any internal single quotes. This handles + // spaces, quote characters, and every other shell metacharacter. return `'${value.replace(/'/g, "'\\''")}'`; } @@ -475,10 +524,7 @@ export function quoteLiteral(value) { } // Check if the string needs quoting at all - // Safe characters: alphanumeric, dash, underscore, dot, slash, colon, equals, comma, plus - const safePattern = /^[a-zA-Z0-9_\-./=,+@:]+$/; - - if (safePattern.test(value)) { + if (SAFE_UNQUOTED_PATTERN.test(value)) { return value; } diff --git a/js/tests/$.test.mjs b/js/tests/$.test.mjs index 8ed34612..cd508335 100644 --- a/js/tests/$.test.mjs +++ b/js/tests/$.test.mjs @@ -186,8 +186,10 @@ describe('Utility Functions', () => { expect(quote(true)).toBe('true'); // Safe boolean string, no quotes needed }); - test('should preserve user-provided quotes', () => { - expect(quote("'already quoted'")).toBe("'already quoted'"); + test('should treat user-provided quotes as data', () => { + // Quote characters in a value are literal data, exactly like "$var" in + // sh - they never quote the value itself (issue #41). + expect(quote("'already quoted'")).toBe("''\\''already quoted'\\'''"); expect(quote('"double quoted"')).toBe('\'"double quoted"\''); }); }); @@ -324,9 +326,8 @@ describe('ProcessRunner - Classic Await Pattern', () => { const dangerous = "'; rm -rf /; echo '"; const result = await $`echo ${dangerous}`; - // The dangerous string is safely quoted, so the echo outputs it without the outer quotes - // Single quotes in the output are handled by shell - expect(result.stdout.trim()).toBe('; rm -rf /; echo'); + // The whole value reaches echo as one literal argument, quotes included. + expect(result.stdout.trim()).toBe(dangerous); }); }); diff --git a/js/tests/path-interpolation.test.mjs b/js/tests/path-interpolation.test.mjs index c5b3e0c9..e710e4c1 100644 --- a/js/tests/path-interpolation.test.mjs +++ b/js/tests/path-interpolation.test.mjs @@ -29,8 +29,9 @@ test('path interpolation - path already wrapped in single quotes', () => { const path = "'/path/to/command'"; const cmd = $({ mirror: false })`${path} hello`; - // With the fix, already-quoted paths should be used as-is when they don't contain internal quotes - expect(cmd.spec.command).toBe("'/path/to/command' hello"); + // The quotes are part of the value, so they are escaped like any other + // character - the same result "$path" would give in sh (issue #41). + expect(cmd.spec.command).toBe("''\\''/path/to/command'\\''' hello"); }); test('path interpolation - environment variable inheritance works', () => { @@ -94,19 +95,16 @@ test('path interpolation - command building works correctly', () => { expect(cmd.spec.mode).toBe('shell'); }); -test('path interpolation - fixed escaping for simple pre-quoted paths', () => { - // This test verifies the fix for excessive escaping of pre-quoted paths - // The issue was that paths like "'/path/to/command'" would get double-escaped - +test('path interpolation - pre-quoted paths keep their quotes', () => { + // Values are never re-interpreted as shell syntax: a path that already + // carries quotes keeps them as literal characters (issue #41). const preQuotedPath = "'/path/to/command'"; // Already has single quotes const cmd = $({ mirror: false })`${preQuotedPath} --version`; - // Fixed behavior: no excessive escaping for simple pre-quoted paths const generated = cmd.spec.command; - expect(generated).not.toContain("\\'"); // Should NOT contain escaped quotes - expect(generated).toBe("'/path/to/command' --version"); // Should use path as-is + expect(generated).toBe("''\\''/path/to/command'\\''' --version"); - // The generated command should be valid shell syntax + // The generated command is still valid shell syntax. expect(generated).toMatch(/^'.*' --version$/); }); @@ -142,16 +140,12 @@ test('path interpolation - environment variable scenario from GitHub issue', () } }); -test('path interpolation - improved handling of pre-quoted paths', () => { - // Test that the improved quoting logic handles pre-quoted paths better +test('path interpolation - pre-quoted paths are one literal argument', () => { const preQuotedPath = "'/path/to/claude'"; // Already has single quotes const cmd = $({ mirror: false })`${preQuotedPath} --version`; - // With the fix, already-quoted paths should be used as-is when they don't contain internal quotes - expect(cmd.spec.command).toBe("'/path/to/claude' --version"); - - // Should not contain excessive escaping - expect(cmd.spec.command).not.toContain("\\'"); + // Quote characters are data, so they survive into the argument itself. + expect(cmd.spec.command).toBe("''\\''/path/to/claude'\\''' --version"); expect(cmd.spec.mode).toBe('shell'); }); @@ -164,10 +158,10 @@ test('path interpolation - handles complex quoting edge cases', () => { // This should still use escaping because it has internal quotes expect(cmd1.spec.command).toContain("\\'"); - // Case 2: Empty quotes should be handled + // Case 2: Empty quotes are two literal quote characters const emptyQuoted = "''"; const cmd2 = $({ mirror: false })`echo ${emptyQuoted}`; - expect(cmd2.spec.command).toBe("echo ''"); + expect(cmd2.spec.command).toBe("echo ''\\'''\\'''"); // Case 3: Just quotes with no content const justQuotes = "'"; @@ -299,10 +293,10 @@ test('double-quoting prevention - user quotes with spaces', () => { // User quotes a path that actually needs quotes (has spaces) const pathWithSpaces = '/path with spaces/cmd'; - // User provides single quotes + // User provides single quotes - they become part of the argument const singleQuoted = `'${pathWithSpaces}'`; const cmd1 = $({ mirror: false })`${singleQuoted} --test`; - expect(cmd1.spec.command).toBe("'/path with spaces/cmd' --test"); + expect(cmd1.spec.command).toBe("''\\''/path with spaces/cmd'\\''' --test"); // User provides double quotes const doubleQuoted = `"${pathWithSpaces}"`; @@ -314,10 +308,10 @@ test('double-quoting prevention - user quotes with special chars', () => { // User quotes a string with special characters const dangerous = 'test; echo INJECTED'; - // User provides single quotes + // User provides single quotes - the value stays a single literal argument const singleQuoted = `'${dangerous}'`; const cmd1 = $({ mirror: false })`echo ${singleQuoted}`; - expect(cmd1.spec.command).toBe("echo 'test; echo INJECTED'"); + expect(cmd1.spec.command).toBe("echo ''\\''test; echo INJECTED'\\'''"); // User provides double quotes const doubleQuoted = `"${dangerous}"`; @@ -329,10 +323,10 @@ test('double-quoting prevention - user unnecessarily quotes safe strings', () => // User quotes a safe string that doesn't need quotes const safe = 'hello'; - // User provides single quotes (unnecessary) + // User provides single quotes (unnecessary) - echo prints them const singleQuoted = `'${safe}'`; const cmd1 = $({ mirror: false })`echo ${singleQuoted}`; - expect(cmd1.spec.command).toBe("echo 'hello'"); + expect(cmd1.spec.command).toBe("echo ''\\''hello'\\'''"); // User provides double quotes (unnecessary) const doubleQuoted = `"${safe}"`; @@ -345,7 +339,7 @@ test('double-quoting prevention - mixed scenarios', () => { { desc: 'Already single-quoted safe string', input: "'safe'", - expected: "echo 'safe'", + expected: "echo ''\\''safe'\\'''", }, { desc: 'Already double-quoted safe string', @@ -355,7 +349,7 @@ test('double-quoting prevention - mixed scenarios', () => { { desc: 'Already single-quoted dangerous string', input: "'rm -rf /'", - expected: "echo 'rm -rf /'", + expected: "echo ''\\''rm -rf /'\\'''", }, { desc: 'Already double-quoted dangerous string', @@ -365,7 +359,7 @@ test('double-quoting prevention - mixed scenarios', () => { { desc: 'Single-quoted path with spaces', input: "'/usr/local bin/app'", - expected: "echo '/usr/local bin/app'", + expected: "echo ''\\''/usr/local bin/app'\\'''", }, { desc: 'Double-quoted path with spaces', diff --git a/js/tests/paths-with-spaces.test.mjs b/js/tests/paths-with-spaces.test.mjs new file mode 100644 index 00000000..754585f1 --- /dev/null +++ b/js/tests/paths-with-spaces.test.mjs @@ -0,0 +1,350 @@ +// Paths with spaces (issue #41). +// +// Interpolating a value must behave like referencing a quoted variable in sh: +// `$`cat ${file}`` is `cat "$file"`, so the value stays one argument no matter +// which characters it contains. These tests pin that for paths with spaces +// across quoting contexts, compare a battery of cases against /bin/sh, and +// exercise real file operations in a directory whose name contains spaces. + +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { test, expect, afterEach, beforeAll, afterAll } from 'bun:test'; +import { + $, + disableVirtualCommands, + enableVirtualCommands, + quote, + setPreQuotedPassthroughEnabled, +} from '../src/$.mjs'; +import './test-helper.mjs'; // Automatically sets up beforeEach/afterEach cleanup + +const PRINTER = fileURLToPath( + new URL('./fixtures/argprint.mjs', import.meta.url) +); + +const isWindows = process.platform === 'win32'; + +// The fixture prints one `ARG[...]` block per argument; a block may span +// several lines when the argument itself contains a newline. +function argsOf(stdout) { + return [...stdout.matchAll(/^ARG\[([\s\S]*?)\]$/gm)].map((m) => m[1]); +} + +// Build a template literal object from a plain string containing one `\0` +// marker where the value should be interpolated, so one string can describe +// both the command-stream template and the sh reference script. +function templateFrom(text) { + const parts = text.split('\0'); + return Object.assign(parts, { raw: parts }); +} + +afterEach(() => { + setPreQuotedPassthroughEnabled(null); + enableVirtualCommands(); +}); + +// --- command building ------------------------------------------------------ + +test('a path with spaces becomes a single quoted argument', () => { + const filePath = '/Users/john/My Documents/report.txt'; + const cmd = $({ mirror: false })`cat ${filePath}`; + expect(cmd.spec.command).toBe("cat '/Users/john/My Documents/report.txt'"); +}); + +test('quote() treats every path as literal text', () => { + expect(quote('/Users/john/My Documents/report.txt')).toBe( + "'/Users/john/My Documents/report.txt'" + ); + expect(quote('C:\\Program Files\\App\\app.exe')).toBe( + "'C:\\Program Files\\App\\app.exe'" + ); + // Quote characters in the value are data, exactly like "$var" in sh - they + // are not treated as quoting the value (issue #41). + expect(quote("'/My Documents/report.txt'")).toBe( + "''\\''/My Documents/report.txt'\\'''" + ); + expect(quote('"/My Documents/report.txt"')).toBe( + '\'"/My Documents/report.txt"\'' + ); + // A value that needs no quoting at all is still passed through untouched. + expect(quote('/Users/john/report.txt')).toBe('/Users/john/report.txt'); +}); + +test('quote() keeps quotes balanced for values mixing both quote kinds', () => { + // The old "already double-quoted" shortcut emitted '"it's"', which the shell + // rejects as an unterminated string (issue #41). + expect(quote('"it\'s"')).toBe("'\"it'\\''s\"'"); +}); + +test('a path with spaces inside author quotes is spliced in literally', () => { + const filePath = '/Users/john/My Documents/report.txt'; + expect($({ mirror: false })`cat "${filePath}"`.spec.command).toBe( + 'cat "/Users/john/My Documents/report.txt"' + ); + expect($({ mirror: false })`cat '${filePath}'`.spec.command).toBe( + "cat '/Users/john/My Documents/report.txt'" + ); +}); + +test('an array of paths with spaces becomes one argument each', () => { + const files = ['/tmp/My Documents/a.txt', '/tmp/b.txt']; + expect($({ mirror: false })`cat ${files}`.spec.command).toBe( + "cat '/tmp/My Documents/a.txt' /tmp/b.txt" + ); +}); + +// --- argv fidelity --------------------------------------------------------- + +const ARGV_VALUES = [ + ['spaces', '/Users/john/My Documents/report.txt'], + ['windows path', 'C:\\Program Files\\App\\app.exe'], + ['apostrophe', "/tmp/it's a dir/file.txt"], + ['double quotes', '/tmp/quoted "name"/f.txt'], + ['single-quoted value', "'/tmp/My Documents/f.txt'"], + ['double-quoted value', '"/tmp/My Documents/f.txt"'], + ['dollar sign', '/tmp/$HOME dir/f.txt'], + ['glob', '/tmp/glob* dir/f.txt'], + ['leading and trailing spaces', ' /tmp/spaced '], + ['backslash', '/tmp/back\\slash dir/f.txt'], + ['tab', '/tmp/tab\there/f.txt'], + ['newline', '/tmp/new\nline/f.txt'], + ['emoji', '/tmp/emoji 🚀 dir/f.txt'], + ['shell operators', '/tmp/a; echo pwned | b && c/f.txt'], + ['command substitution text', '/tmp/$(echo pwned)/f.txt'], +]; + +const ARGV_CONTEXTS = [ + ['unquoted', `node "${PRINTER}" \0`], + ['double-quoted', `node "${PRINTER}" "\0"`], + ['single-quoted', `node "${PRINTER}" '\0'`], +]; + +for (const [contextName, script] of ARGV_CONTEXTS) { + for (const [valueName, value] of ARGV_VALUES) { + test.skipIf(isWindows)( + `${contextName} interpolation keeps "${valueName}" as one argument`, + async () => { + const result = await $({ mirror: false })(templateFrom(script), value); + expect(argsOf(result.stdout)).toEqual([value]); + } + ); + } +} + +test.skipIf(isWindows)( + 'a path with spaces stays one argument for a real binary too', + async () => { + disableVirtualCommands(); + const value = '/Users/john/My Documents/report.txt'; + const result = await $({ mirror: false })`node ${PRINTER} ${value}`; + expect(argsOf(result.stdout)).toEqual([value]); + } +); + +// --- parity with /bin/sh --------------------------------------------------- + +// Each case pairs a command-stream template (with `\0` at the interpolation +// point) with the sh script it must behave like. Interpolation corresponds to +// a *quoted* variable reference, which is why the reference uses "$V" wherever +// the template interpolates outside quotes. +// `printf '%s\\n'` rather than `echo`, because /bin/sh's echo expands +// backslash escapes on some systems (dash does, bash does not) - a difference +// between echo implementations, not a difference in how the path is passed. +const PARITY_CASES = [ + ['unquoted path', "printf '%s\\n' \0", `printf '%s\\n' "$V"`], + ['double-quoted path', `printf '%s\\n' "\0"`, `printf '%s\\n' "$V"`], + ['single-quoted path', "printf '%s\\n' '\0'", `printf '%s\\n' "$V"`], + [ + 'path inside a sentence', + `printf '%s\\n' "file: \0 done"`, + `printf '%s\\n' "file: $V done"`, + ], + [ + 'path as one of several args', + "printf '[%s]\\n' a \0 b", + `printf '[%s]\\n' a "$V" b`, + ], + [ + 'path with a suffix appended', + "printf '%s\\n' \0.bak", + `printf '%s\\n' "$V".bak`, + ], + [ + 'path in a pipeline', + "printf '%s\\n' \0 | cat", + `printf '%s\\n' "$V" | cat`, + ], + [ + 'path in a subcommand', + `sh -c "printf '%s\\n' \0"`, + `sh -c "printf '%s\\n' $V"`, + ], +]; + +const PARITY_VALUES = ARGV_VALUES; + +for (const [caseName, script, reference] of PARITY_CASES) { + for (const [valueName, value] of PARITY_VALUES) { + test.skipIf(isWindows)( + `matches /bin/sh: ${caseName} with ${valueName}`, + async () => { + const expected = spawnSync('/bin/sh', ['-c', reference], { + env: { ...process.env, V: value }, + encoding: 'utf8', + }); + const result = await $({ mirror: false })(templateFrom(script), value); + expect(result.stdout).toBe(expected.stdout); + expect(result.code).toBe(expected.status); + } + ); + } +} + +// --- real file operations -------------------------------------------------- + +let workDir; +let filePath; + +beforeAll(() => { + workDir = fs.mkdtempSync(path.join(os.tmpdir(), 'my documents ')); + filePath = path.join(workDir, 'report file.txt'); + fs.writeFileSync(filePath, 'hello content\n'); +}); + +afterAll(() => { + fs.rmSync(workDir, { recursive: true, force: true }); +}); + +test.skipIf(isWindows)('cat reads a file whose path has spaces', async () => { + const result = await $({ mirror: false })`cat ${filePath}`; + expect(result.code).toBe(0); + expect(result.stdout).toBe('hello content\n'); +}); + +test.skipIf(isWindows)( + 'cat reads the file with author-written quotes too', + async () => { + expect((await $({ mirror: false })`cat "${filePath}"`).stdout).toBe( + 'hello content\n' + ); + expect((await $({ mirror: false })`cat '${filePath}'`).stdout).toBe( + 'hello content\n' + ); + } +); + +test.skipIf(isWindows)( + 'cat reads the file without virtual commands', + async () => { + disableVirtualCommands(); + const result = await $({ mirror: false })`cat ${filePath}`; + expect(result.code).toBe(0); + expect(result.stdout).toBe('hello content\n'); + } +); + +test.skipIf(isWindows)('sync execution handles paths with spaces', () => { + const result = $({ mirror: false })`cat ${filePath}`.sync(); + expect(result.code).toBe(0); + expect(result.stdout).toBe('hello content\n'); +}); + +test.skipIf(isWindows)( + 'ls lists a directory whose name has spaces', + async () => { + const result = await $({ mirror: false })`ls ${workDir}`; + expect(result.code).toBe(0); + expect(result.stdout).toContain('report file.txt'); + } +); + +test.skipIf(isWindows)('cp and mv work on paths with spaces', async () => { + const copy = path.join(workDir, 'copy of report.txt'); + const moved = path.join(workDir, 'moved report.txt'); + expect((await $({ mirror: false })`cp ${filePath} ${copy}`).code).toBe(0); + expect(fs.readFileSync(copy, 'utf8')).toBe('hello content\n'); + expect((await $({ mirror: false })`mv ${copy} ${moved}`).code).toBe(0); + expect(fs.existsSync(copy)).toBe(false); + expect(fs.readFileSync(moved, 'utf8')).toBe('hello content\n'); + expect((await $({ mirror: false })`rm ${moved}`).code).toBe(0); + expect(fs.existsSync(moved)).toBe(false); +}); + +test.skipIf(isWindows)('mkdir creates a nested path with spaces', async () => { + const nested = path.join(workDir, 'a b', 'c d'); + expect((await $({ mirror: false })`mkdir -p ${nested}`).code).toBe(0); + expect(fs.statSync(nested).isDirectory()).toBe(true); +}); + +test.skipIf(isWindows)('redirection writes to a path with spaces', async () => { + const target = path.join(workDir, 'redirected output.txt'); + const result = await $({ mirror: false })`echo redirected > ${target}`; + expect(result.code).toBe(0); + expect(fs.readFileSync(target, 'utf8')).toBe('redirected\n'); +}); + +test.skipIf(isWindows)('a pipeline keeps the path in one piece', async () => { + const result = await $({ mirror: false })`cat ${filePath} | grep hello`; + expect(result.code).toBe(0); + expect(result.stdout).toBe('hello content\n'); +}); + +test.skipIf(isWindows)( + 'cd enters a directory whose name has spaces', + async () => { + const result = await $({ mirror: false })`cd ${workDir} && pwd`; + expect(result.code).toBe(0); + expect(result.stdout.trim()).toBe(fs.realpathSync(workDir)); + } +); + +test.skipIf(isWindows)('test -f finds a path with spaces', async () => { + const result = await $({ mirror: false })`test -f ${filePath} && echo found`; + expect(result.code).toBe(0); + expect(result.stdout).toBe('found\n'); +}); + +// --- injection safety ------------------------------------------------------ + +test.skipIf(isWindows)( + 'a value wrapped in quotes cannot inject a command (issue #41)', + async () => { + const marker = path.join(workDir, 'pwned.txt'); + const evil = `"' ; touch ${marker} ; '"`; + const result = await $({ mirror: false })`node ${PRINTER} ${evil}`; + expect(argsOf(result.stdout)).toEqual([evil]); + expect(fs.existsSync(marker)).toBe(false); + } +); + +// --- legacy pre-quoted passthrough ----------------------------------------- + +test('pre-quoted passthrough can be re-enabled', () => { + setPreQuotedPassthroughEnabled(true); + // Opted in, a hand-quoted value is spliced in as shell syntax again. + expect(quote("'/My Documents/report.txt'")).toBe( + "'/My Documents/report.txt'" + ); + expect(quote('"/My Documents/report.txt"')).toBe( + '"/My Documents/report.txt"' + ); + setPreQuotedPassthroughEnabled(false); + expect(quote("'/My Documents/report.txt'")).toBe( + "''\\''/My Documents/report.txt'\\'''" + ); +}); + +test('pre-quoted passthrough never emits unbalanced quotes', () => { + setPreQuotedPassthroughEnabled(true); + // A value whose own quoting is unbalanced falls back to literal quoting, so + // the injection the old heuristic allowed stays impossible: the old code + // wrapped `"a" ; touch pwned ; "b"` in single quotes and the shell then read + // the value's quotes as syntax. + expect(quote('"a" ; touch pwned ; "b"')).toBe('\'"a" ; touch pwned ; "b"\''); + expect(quote("'a' ; touch pwned ; 'b'")).toBe( + "''\\''a'\\'' ; touch pwned ; '\\''b'\\'''" + ); +}); diff --git a/js/tests/pipeline-epipe.test.mjs b/js/tests/pipeline-epipe.test.mjs new file mode 100644 index 00000000..9fc584f8 --- /dev/null +++ b/js/tests/pipeline-epipe.test.mjs @@ -0,0 +1,67 @@ +import { test, expect } from 'bun:test'; +import './test-helper.mjs'; // Automatically sets up beforeEach/afterEach cleanup +import { pipeStreamToProcess } from '../src/$.process-runner-pipeline.mjs'; + +/** + * A process whose stdin behaves like a pipe whose reader has already gone + * away: closing it (and, optionally, writing to it) raises EPIPE. + * @param {object} options - Which operations should fail + * @returns {object} A fake process with a writable stdin + */ +function procWithBrokenStdin({ failWrites = false } = {}) { + const written = []; + const epipe = () => + Object.assign(new Error('broken pipe'), { code: 'EPIPE' }); + return { + written, + stdin: { + async write(chunk) { + if (failWrites) { + throw epipe(); + } + written.push(chunk); + }, + async close() { + throw epipe(); + }, + }, + }; +} + +/** + * @param {string[]} chunks - Chunks the stream yields + * @returns {ReadableStream} A stream over the encoded chunks + */ +function streamOf(chunks) { + return new ReadableStream({ + start(controller) { + for (const chunk of chunks) { + controller.enqueue(new TextEncoder().encode(chunk)); + } + controller.close(); + }, + }); +} + +// Regression test: a downstream process can exit before its stdin is closed, +// and then closing that stdin raises EPIPE. A shell ignores that, so the +// pipeline must ignore it too instead of letting the rejection escape. +test('closing the stdin of an exited process does not reject', async () => { + const proc = procWithBrokenStdin(); + await expect( + pipeStreamToProcess(streamOf(['line 0\n']), proc) + ).resolves.toBeUndefined(); + expect(proc.written).toHaveLength(1); +}); + +test('an EPIPE on write does not reject either', async () => { + const proc = procWithBrokenStdin({ failWrites: true }); + await expect( + pipeStreamToProcess(streamOf(['line 0\n', 'line 1\n']), proc) + ).resolves.toBeUndefined(); + expect(proc.written).toHaveLength(0); +}); + +test('a process without stdin is left alone', () => { + expect(pipeStreamToProcess(streamOf(['x']), {})).toBeUndefined(); +}); diff --git a/js/tests/readme-examples.test.mjs b/js/tests/readme-examples.test.mjs index 76ad339d..b725a740 100644 --- a/js/tests/readme-examples.test.mjs +++ b/js/tests/readme-examples.test.mjs @@ -382,12 +382,14 @@ describe('README Examples and Use Cases', () => { expect(testCmd2.spec.command).toBe("echo '/my path/file'"); }); - test('user-provided quotes are preserved', () => { + test('user-provided quotes are kept as data', () => { const quotedPath = "'/path with spaces/file'"; const doubleQuoted = '"/path with spaces/file"'; const testCmd1 = $({ mirror: false })`cat ${quotedPath}`; - expect(testCmd1.spec.command).toBe("cat '/path with spaces/file'"); + expect(testCmd1.spec.command).toBe( + "cat ''\\''/path with spaces/file'\\'''" + ); const testCmd2 = $({ mirror: false })`cat ${doubleQuoted}`; expect(testCmd2.spec.command).toBe('cat \'"/path with spaces/file"\''); diff --git a/rust/BEST-PRACTICES.md b/rust/BEST-PRACTICES.md index 423172f1..5568f62f 100644 --- a/rust/BEST-PRACTICES.md +++ b/rust/BEST-PRACTICES.md @@ -137,6 +137,26 @@ assert_eq!(quote_for_context("it's", QuoteContext::Single), "it'\\''s"); Set `COMMAND_STREAM_QUOTE_CONTEXT=0` to restore the previous behavior of always quoting every interpolated value. +### Paths With Spaces + +Interpolate the path as-is. An interpolated value always becomes exactly one +argument, so spaces and other special characters need no help from you - the +same guarantee as `"$path"` in a shell script: + +```rust +use command_stream::quote::quote; + +assert_eq!( + quote("/Users/john/My Documents/report.txt"), + "'/Users/john/My Documents/report.txt'" +); +``` + +Never pre-quote the value: quote characters you add become part of the file +name, exactly as `sh` would treat them. Before v0.18 a value that started and +ended with a matching quote was spliced in as shell syntax; set +`COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1` if you still depend on that. + ### When Quoting is Applied ```rust diff --git a/rust/changelog.d/20260906_120000_paths_with_spaces.md b/rust/changelog.d/20260906_120000_paths_with_spaces.md new file mode 100644 index 00000000..a390dbb6 --- /dev/null +++ b/rust/changelog.d/20260906_120000_paths_with_spaces.md @@ -0,0 +1,18 @@ +--- +bump: minor +--- + +### Fixed + +- Interpolate every value as exactly one literal argument, like `"$var"` in a + POSIX shell. `quote` no longer treats a value that starts and ends with a + matching quote as ready-made shell syntax, so paths with spaces (and + pre-quoted paths) reach the command intact (issue #41). This also fixes + `quote("\"it's\"")`, which used to emit the unterminated string `'"it's"'`, + and closes an injection where a value like `"' ; touch /tmp/pwned ; '"` was + spliced into the command and executed. + +### Added + +- `is_pre_quoted_passthrough_enabled` and `COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1` + restore the previous pre-quoted passthrough for balanced values only. diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 8476144a..8791da82 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -95,7 +95,8 @@ pub use events::{EventData, EventType, StreamEmitter}; pub use pipeline::{Pipeline, PipelineBuilder, PipelineExt}; pub use quote::{ escape_for_double_quotes, escape_for_single_quotes, has_shell_escapes, - is_quote_context_enabled, quote, quote_for_context, scan_quote_context, QuoteContext, + is_pre_quoted_passthrough_enabled, is_quote_context_enabled, quote, quote_for_context, + scan_quote_context, QuoteContext, }; pub use state::{ get_shell_settings, global_state, reset_global_state, set_shell_option, unset_shell_option, diff --git a/rust/src/quote.rs b/rust/src/quote.rs index 7df15a10..cf9a18f1 100644 --- a/rust/src/quote.rs +++ b/rust/src/quote.rs @@ -6,10 +6,45 @@ use std::collections::HashSet; use std::sync::{Mutex, OnceLock}; +/// Whether the legacy pre-quoted passthrough heuristic is active. +/// +/// Older versions treated a value that happened to start and end with a quote +/// character as "already quoted" and spliced it into the command as shell +/// syntax, so the value `'/My Documents/x'` reached the command as +/// `/My Documents/x` - the quotes vanished. sh does the opposite: `"$var"` +/// always yields the value verbatim, quote characters included, which is also +/// what Bun's $, zx and execa do. Worse, the heuristic could hand the shell +/// unbalanced quotes, and an injected command ran (issue #41). +/// +/// The heuristic is therefore off by default; set +/// `COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1` to restore it for code that relies +/// on hand-quoted values. Even then only values that stay balanced are passed +/// through, so the injection above can no longer happen. +pub fn is_pre_quoted_passthrough_enabled() -> bool { + matches!( + std::env::var("COMMAND_STREAM_PREQUOTED_PASSTHROUGH"), + Ok(ref value) if value == "1" + ) +} + +/// Whether a value is wrapped in matching quotes that contain none of that +/// quote character inside, i.e. it is balanced shell syntax on its own. +fn is_balanced_quoted_value(value: &str) -> bool { + let quote_char = match value.chars().next() { + Some(c @ ('\'' | '"')) => c, + _ => return false, + }; + if value.chars().count() < 2 || !value.ends_with(quote_char) { + return false; + } + let inner = &value[quote_char.len_utf8()..value.len() - quote_char.len_utf8()]; + !inner.contains(quote_char) +} + /// Quote a value for safe shell usage /// -/// This function quotes strings appropriately for use in shell commands, -/// handling special characters and edge cases. +/// The value is always treated as literal text - exactly one argument, spaces +/// and quote characters included - which is what `"$var"` does in sh. /// /// # Examples /// @@ -23,6 +58,9 @@ use std::sync::{Mutex, OnceLock}; /// // Special characters are quoted /// assert_eq!(quote("hello world"), "'hello world'"); /// +/// // Paths with spaces stay a single argument +/// assert_eq!(quote("/My Documents/report.txt"), "'/My Documents/report.txt'"); +/// /// // Single quotes in strings are escaped /// assert_eq!(quote("it's"), "'it'\\''s'"); /// @@ -34,17 +72,9 @@ pub fn quote(value: &str) -> String { return "''".to_string(); } - // If already properly quoted with single quotes, check if we can use as-is - if value.starts_with('\'') && value.ends_with('\'') && value.len() >= 2 { - let inner = &value[1..value.len() - 1]; - if !inner.contains('\'') { - return value.to_string(); - } - } - - // If already double-quoted, wrap in single quotes - if value.starts_with('"') && value.ends_with('"') && value.len() > 2 { - return format!("'{}'", value); + // Legacy: the caller quoted the value themselves, so use it as shell syntax. + if is_pre_quoted_passthrough_enabled() && is_balanced_quoted_value(value) { + return value.to_string(); } // Check if the string needs quoting at all @@ -55,7 +85,7 @@ pub fn quote(value: &str) -> String { return value.to_string(); } - // Default behavior: wrap in single quotes and escape any internal single quotes + // Wrap in single quotes and escape any internal single quotes. // The shell escape sequence for a single quote inside single quotes is: '\'' // This ends the single quote, adds an escaped single quote, and starts single quotes again format!("'{}'", value.replace('\'', "'\\''")) @@ -482,9 +512,51 @@ mod tests { } #[test] - fn test_quote_already_quoted() { - assert_eq!(quote("'already quoted'"), "'already quoted'"); + fn test_quote_treats_quote_characters_as_data() { + // Quote characters inside a value are data, exactly like "$var" in sh - + // they never quote the value itself (issue #41). + assert_eq!(quote("'already quoted'"), "''\\''already quoted'\\'''"); assert_eq!(quote("\"double quoted\""), "'\"double quoted\"'"); + // The old "already double-quoted" shortcut emitted '"it's"', which the + // shell rejects as an unterminated quoted string. + assert_eq!(quote("\"it's\""), "'\"it'\\''s\"'"); + } + + #[test] + fn test_quote_paths_with_spaces() { + assert_eq!( + quote("/Users/john/My Documents/report.txt"), + "'/Users/john/My Documents/report.txt'" + ); + assert_eq!( + quote("C:\\Program Files\\App\\app.exe"), + "'C:\\Program Files\\App\\app.exe'" + ); + assert_eq!(quote(" /tmp/spaced "), "' /tmp/spaced '"); + assert_eq!( + quote("/tmp/it's a dir/f.txt"), + "'/tmp/it'\\''s a dir/f.txt'" + ); + } + + #[test] + fn test_pre_quoted_passthrough_disabled_by_default() { + // The opt-in is read from the environment on every call, so with the + // variable unset the sh-like literal behaviour must be in effect. + if std::env::var("COMMAND_STREAM_PREQUOTED_PASSTHROUGH").is_err() { + assert!(!is_pre_quoted_passthrough_enabled()); + } + } + + #[test] + fn test_balanced_quoted_value_detection() { + assert!(is_balanced_quoted_value("'/My Documents/f.txt'")); + assert!(is_balanced_quoted_value("\"/My Documents/f.txt\"")); + // Unbalanced quoting is what made the old heuristic injectable. + assert!(!is_balanced_quoted_value("\"a\" ; touch pwned ; \"b\"")); + assert!(!is_balanced_quoted_value("'a' ; touch pwned ; 'b'")); + assert!(!is_balanced_quoted_value("/plain/path")); + assert!(!is_balanced_quoted_value("'")); } #[test] diff --git a/rust/tests/paths_with_spaces.rs b/rust/tests/paths_with_spaces.rs new file mode 100644 index 00000000..56c4a038 --- /dev/null +++ b/rust/tests/paths_with_spaces.rs @@ -0,0 +1,132 @@ +//! Paths with spaces (and other shell metacharacters) must reach the command +//! as exactly one literal argument, like `"$path"` in a POSIX shell. +//! +//! Mirrors `js/tests/paths-with-spaces.test.mjs` (issue #41). +//! +//! Unix only: the assertions compare against `/bin/sh`, whose quoting rules +//! differ from `cmd.exe`. The platform-independent part of the behaviour is +//! covered by the unit tests in `src/quote.rs`. +#![cfg(unix)] + +use command_stream::cmd; +use command_stream::quote::quote; + +/// Tricky path values, each of which must survive interpolation unchanged. +const VALUES: &[&str] = &[ + "/Users/john/My Documents/report.txt", + "/tmp/two spaces/file.txt", + " /tmp/leading and trailing ", + "/tmp/it's a dir/file.txt", + "/tmp/say \"hi\"/file.txt", + "'/tmp/pre single quoted/file.txt'", + "\"/tmp/pre double quoted/file.txt\"", + "/tmp/$HOME dir/file.txt", + "/tmp/back`tick`/file.txt", + "/tmp/semi;colon/file.txt", + "/tmp/pipe|and&/file.txt", + "/tmp/star*glob?/file.txt", + "/tmp/paren(s)/file.txt", + "C:\\Program Files\\App\\app.exe", + "/tmp/new\nline/file.txt", +]; + +fn sh_stdout(script: &str, value: Option<&str>) -> String { + let mut command = std::process::Command::new("/bin/sh"); + command.arg("-c").arg(script); + if let Some(value) = value { + command.env("V", value); + } + let output = command.output().expect("failed to run /bin/sh"); + String::from_utf8_lossy(&output.stdout).into_owned() +} + +/// `printf` instead of `echo`: /bin/sh may be dash, whose `echo` expands +/// backslash escapes and would corrupt Windows-style paths. +const SCRIPTS: &[(&str, &str)] = &[ + ("printf '%s\\n' {}", "printf '%s\\n' \"$V\""), + ("printf '%s\\n' {} tail", "printf '%s\\n' \"$V\" tail"), + ("printf '[%s]\\n' {}", "printf '[%s]\\n' \"$V\""), +]; + +#[test] +fn interpolated_values_match_a_quoted_sh_variable() { + for value in VALUES { + for (template, reference) in SCRIPTS { + let built = template.replace("{}", "e(value)); + assert_eq!( + sh_stdout(&built, None), + sh_stdout(reference, Some(value)), + "value {value:?} in script {template:?} built as {built:?}" + ); + } + } +} + +#[tokio::test] +async fn a_path_with_spaces_stays_one_argument() { + let dir = tempfile::Builder::new() + .prefix("my documents ") + .tempdir() + .unwrap(); + let file = dir.path().join("annual report 2026.txt"); + std::fs::write(&file, "hello content\n").unwrap(); + let file = file.to_str().unwrap(); + + let result = cmd!("cat {}", file).await.unwrap(); + assert!(result.is_success(), "stderr: {}", result.stderr); + assert_eq!(result.stdout, "hello content\n"); +} + +#[tokio::test] +async fn copying_between_directories_with_spaces_works() { + let dir = tempfile::Builder::new() + .prefix("my documents ") + .tempdir() + .unwrap(); + let source = dir.path().join("source file.txt"); + let target = dir.path().join("target file.txt"); + std::fs::write(&source, "copy me\n").unwrap(); + + let result = cmd!( + "cp {} {}", + source.to_str().unwrap(), + target.to_str().unwrap() + ) + .await + .unwrap(); + assert!(result.is_success(), "stderr: {}", result.stderr); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "copy me\n"); +} + +#[tokio::test] +async fn a_pre_quoted_path_is_not_reinterpreted_as_shell_syntax() { + // The quotes are part of the value, so the file is not found - exactly + // what `cat "'$path'"` does in sh. + let dir = tempfile::Builder::new() + .prefix("my documents ") + .tempdir() + .unwrap(); + let file = dir.path().join("report.txt"); + std::fs::write(&file, "hello\n").unwrap(); + let pre_quoted = format!("'{}'", file.to_str().unwrap()); + + let result = cmd!("cat {}", pre_quoted).await.unwrap(); + assert!(!result.is_success()); + assert!(result.stdout.is_empty(), "stdout: {}", result.stdout); +} + +#[tokio::test] +async fn an_interpolated_value_cannot_start_a_second_command() { + let dir = tempfile::Builder::new() + .prefix("injection ") + .tempdir() + .unwrap(); + let marker = dir.path().join("pwned"); + // The value that made the old pre-quoted heuristic injectable. + let evil = format!("' ; touch {} ; '", marker.to_str().unwrap()); + + let result = cmd!("printf '%s\\n' {}", evil).await.unwrap(); + assert!(result.is_success(), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{evil}\n")); + assert!(!marker.exists(), "the injected command ran"); +} diff --git a/rust/tests/utils.rs b/rust/tests/utils.rs index 30584171..9ba4455c 100644 --- a/rust/tests/utils.rs +++ b/rust/tests/utils.rs @@ -260,7 +260,8 @@ fn test_quote_with_single_quote() { #[test] fn test_quote_already_single_quoted() { - assert_eq!(quote("'hello'"), "'hello'"); + // Quote characters are data, like "$var" in sh (issue #41). + assert_eq!(quote("'hello'"), "''\\''hello'\\'''"); } #[test]