From b5b2e7665f0c53e0d3b3f48f99e3d8ced8563254 Mon Sep 17 00:00:00 2001 From: konard Date: Sun, 6 Sep 2026 21:34:11 +0000 Subject: [PATCH 1/7] Initial commit with task details Adding .gitkeep for PR creation (default mode). This file will be removed when the task is complete. Issue: https://github.com/link-foundation/command-stream/issues/41 --- .gitkeep | 1 + 1 file changed, 1 insertion(+) create mode 100644 .gitkeep diff --git a/.gitkeep b/.gitkeep new file mode 100644 index 0000000..d523da0 --- /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 From 6634615d79d8f955408a96d3220a87083ed2e93d Mon Sep 17 00:00:00 2001 From: konard Date: Sun, 6 Sep 2026 21:55:15 +0000 Subject: [PATCH 2/7] Interpolate every value as one literal argument, like "$var" in sh Interpolation had two "already quoted" shortcuts: a value wrapped in matching quotes was spliced into the command as shell syntax instead of being quoted. So a path the caller had quoted lost its quotes, and a value like "it's" produced '"it's"' - an unterminated string the shell refuses to run. Worse, the shortcut also accepted unbalanced values, so "' ; touch /tmp/pwned ; '" was spliced in verbatim and the injected command executed. Values are now always quoted as literal text, so an interpolated path reaches the command as exactly one argument, spaces and quote characters included - the same guarantee as "$path" in sh, and the behavior of Bun's $, zx and execa (issue #41). The previous behavior stays available via shell.preQuotedPassthrough(), setPreQuotedPassthroughEnabled(true), or COMMAND_STREAM_PREQUOTED_PASSTHROUGH=1, and even then only balanced values are passed through, so the injection can no longer happen. Mirrored in the Rust crate (quote, is_pre_quoted_passthrough_enabled). --- experiments/issue-41-broken-quoting.mjs | 18 + experiments/issue-41-diff-sh.mjs | 52 +++ experiments/issue-41-injection.mjs | 6 + experiments/issue-41-matrix.mjs | 61 +++ experiments/issue-41-nested.mjs | 6 + experiments/issue-41-newline.mjs | 22 ++ experiments/issue-41-repro.mjs | 31 ++ js/.changeset/issue-41-paths-with-spaces.md | 15 + js/BEST-PRACTICES.md | 28 ++ js/README.md | 49 ++- js/src/$.mjs | 11 + js/src/$.quote.mjs | 102 +++-- js/tests/$.test.mjs | 11 +- js/tests/path-interpolation.test.mjs | 50 ++- js/tests/paths-with-spaces.test.mjs | 350 ++++++++++++++++++ js/tests/readme-examples.test.mjs | 6 +- rust/BEST-PRACTICES.md | 20 + .../20260906_120000_paths_with_spaces.md | 18 + rust/src/lib.rs | 3 +- rust/src/quote.rs | 104 +++++- rust/tests/paths_with_spaces.rs | 131 +++++++ rust/tests/utils.rs | 3 +- 22 files changed, 1013 insertions(+), 84 deletions(-) create mode 100644 experiments/issue-41-broken-quoting.mjs create mode 100644 experiments/issue-41-diff-sh.mjs create mode 100644 experiments/issue-41-injection.mjs create mode 100644 experiments/issue-41-matrix.mjs create mode 100644 experiments/issue-41-nested.mjs create mode 100644 experiments/issue-41-newline.mjs create mode 100644 experiments/issue-41-repro.mjs create mode 100644 js/.changeset/issue-41-paths-with-spaces.md create mode 100644 js/tests/paths-with-spaces.test.mjs create mode 100644 rust/changelog.d/20260906_120000_paths_with_spaces.md create mode 100644 rust/tests/paths_with_spaces.rs diff --git a/experiments/issue-41-broken-quoting.mjs b/experiments/issue-41-broken-quoting.mjs new file mode 100644 index 0000000..61c2e0a --- /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-diff-sh.mjs b/experiments/issue-41-diff-sh.mjs new file mode 100644 index 0000000..76aab32 --- /dev/null +++ b/experiments/issue-41-diff-sh.mjs @@ -0,0 +1,52 @@ +// 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" + const script = 'printf "[%s]\\n" "$1"'; + 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 0000000..3f00949 --- /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 0000000..cfda2a5 --- /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 0000000..71944c2 --- /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 0000000..9405fc4 --- /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 0000000..869e637 --- /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 0000000..94d8e6c --- /dev/null +++ b/js/.changeset/issue-41-paths-with-spaces.md @@ -0,0 +1,15 @@ +--- +'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`. diff --git a/js/BEST-PRACTICES.md b/js/BEST-PRACTICES.md index e7f7a1b..01fe97d 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 1c61007..34229d1 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/src/$.mjs b/js/src/$.mjs index 45bd984..bc629be 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/$.quote.mjs b/js/src/$.quote.mjs index 7e2201f..1b92829 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 8ed3461..cd50833 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 c5b3e0c..e710e4c 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 0000000..754585f --- /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/readme-examples.test.mjs b/js/tests/readme-examples.test.mjs index 76ad339..b725a74 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 423172f..5568f62 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 0000000..a390dbb --- /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 8476144..8791da8 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 7df15a1..cf9a18f 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 0000000..a88a1b0 --- /dev/null +++ b/rust/tests/paths_with_spaces.rs @@ -0,0 +1,131 @@ +//! 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). + +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", +]; + +#[cfg(unix)] +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. +#[cfg(unix)] +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\""), +]; + +#[cfg(unix)] +#[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); +} + +#[cfg(unix)] +#[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 3058417..9ba4455 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] From f7ad254093b2bb8dc875d1cb2aa4cae18a23251e Mon Sep 17 00:00:00 2001 From: konard Date: Sun, 6 Sep 2026 21:57:29 +0000 Subject: [PATCH 3/7] Add a runnable example for paths with spaces --- js/examples/README.md | 5 ++++ js/examples/paths-with-spaces.mjs | 39 +++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 js/examples/paths-with-spaces.mjs diff --git a/js/examples/README.md b/js/examples/README.md index 07bbf25..19f6c95 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 0000000..24459af --- /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 }); +} From 809611bbd547b7b248530df8c008beef830800bb Mon Sep 17 00:00:00 2001 From: konard Date: Sun, 6 Sep 2026 21:59:27 +0000 Subject: [PATCH 4/7] Restrict the Rust sh-parity test file to unix --- rust/tests/paths_with_spaces.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/rust/tests/paths_with_spaces.rs b/rust/tests/paths_with_spaces.rs index a88a1b0..56c4a03 100644 --- a/rust/tests/paths_with_spaces.rs +++ b/rust/tests/paths_with_spaces.rs @@ -2,6 +2,11 @@ //! 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; @@ -25,7 +30,6 @@ const VALUES: &[&str] = &[ "/tmp/new\nline/file.txt", ]; -#[cfg(unix)] fn sh_stdout(script: &str, value: Option<&str>) -> String { let mut command = std::process::Command::new("/bin/sh"); command.arg("-c").arg(script); @@ -38,14 +42,12 @@ fn sh_stdout(script: &str, value: Option<&str>) -> String { /// `printf` instead of `echo`: /bin/sh may be dash, whose `echo` expands /// backslash escapes and would corrupt Windows-style paths. -#[cfg(unix)] 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\""), ]; -#[cfg(unix)] #[test] fn interpolated_values_match_a_quoted_sh_variable() { for value in VALUES { @@ -113,7 +115,6 @@ async fn a_pre_quoted_path_is_not_reinterpreted_as_shell_syntax() { assert!(result.stdout.is_empty(), "stdout: {}", result.stdout); } -#[cfg(unix)] #[tokio::test] async fn an_interpolated_value_cannot_start_a_second_command() { let dir = tempfile::Builder::new() From f0c23d6b40ca2924eec565a1743b33816cfb2172 Mon Sep 17 00:00:00 2001 From: konard Date: Sun, 6 Sep 2026 22:03:16 +0000 Subject: [PATCH 5/7] Add a competitor comparison experiment for interpolated paths --- experiments/issue-41-competitors.mjs | 74 ++++++++++++++++++++++++++++ experiments/issue-41-diff-sh.mjs | 1 - 2 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 experiments/issue-41-competitors.mjs diff --git a/experiments/issue-41-competitors.mjs b/experiments/issue-41-competitors.mjs new file mode 100644 index 0000000..131d960 --- /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 index 76aab32..445a14b 100644 --- a/experiments/issue-41-diff-sh.mjs +++ b/experiments/issue-41-diff-sh.mjs @@ -24,7 +24,6 @@ const values = [ function shArgs(value) { // What a POSIX shell gives argv when you write: prog "$var" - const script = 'printf "[%s]\\n" "$1"'; return execFileSync('/bin/sh', ['-c', 'printf "[%s]\\n" "$V"'], { env: { ...process.env, V: value }, encoding: 'utf8', From ffa450b89b07a90e3b91a6fcdb184afb52e1ae8e Mon Sep 17 00:00:00 2001 From: konard Date: Sun, 6 Sep 2026 22:11:42 +0000 Subject: [PATCH 6/7] Ignore EPIPE when closing the stdin of an exited pipeline stage A pipeline stage can exit before the stage feeding it has finished writing (`source | grep -m1 ...`, or simply a race at the end of the pipe). Closing that stdin then raises EPIPE. The pump in pipeStreamToProcess guarded its writes but not the close in its `finally` block, and the pump promise itself was never awaited or caught, so the rejection escaped as an unhandled error and could fail an otherwise successful command - as it did on CI for a `printf ... | cat` parity case in tests/paths-with-spaces.test.mjs. Guard the close the same way the writes are guarded, and return the pump promise so the failure mode is testable. The Rust implementation already discards these errors (`let _ = stdin.write_all(...)` / `let _ = stdin.shutdown()`), so this brings JavaScript in line with it. --- js/.changeset/issue-41-pipeline-epipe.md | 10 ++++ js/src/$.process-runner-pipeline.mjs | 27 +++++++--- js/tests/pipeline-epipe.test.mjs | 67 ++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 7 deletions(-) create mode 100644 js/.changeset/issue-41-pipeline-epipe.md create mode 100644 js/tests/pipeline-epipe.test.mjs diff --git a/js/.changeset/issue-41-pipeline-epipe.md b/js/.changeset/issue-41-pipeline-epipe.md new file mode 100644 index 0000000..7caee98 --- /dev/null +++ b/js/.changeset/issue-41-pipeline-epipe.md @@ -0,0 +1,10 @@ +--- +'command-stream': patch +--- + +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/src/$.process-runner-pipeline.mjs b/js/src/$.process-runner-pipeline.mjs index 413d890..102fcc1 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/tests/pipeline-epipe.test.mjs b/js/tests/pipeline-epipe.test.mjs new file mode 100644 index 0000000..9fc584f --- /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(); +}); From 6e08ce4b032b12b44c4ff4395af11d3ba21c16c5 Mon Sep 17 00:00:00 2001 From: konard Date: Sun, 6 Sep 2026 22:15:44 +0000 Subject: [PATCH 7/7] Fold the EPIPE note into the single PR changeset The changeset validator requires exactly one changeset per pull request. --- js/.changeset/issue-41-paths-with-spaces.md | 7 +++++++ js/.changeset/issue-41-pipeline-epipe.md | 10 ---------- 2 files changed, 7 insertions(+), 10 deletions(-) delete mode 100644 js/.changeset/issue-41-pipeline-epipe.md diff --git a/js/.changeset/issue-41-paths-with-spaces.md b/js/.changeset/issue-41-paths-with-spaces.md index 94d8e6c..4166943 100644 --- a/js/.changeset/issue-41-paths-with-spaces.md +++ b/js/.changeset/issue-41-paths-with-spaces.md @@ -13,3 +13,10 @@ emitted the unterminated string `'"it\'s"'`, and a value like 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/.changeset/issue-41-pipeline-epipe.md b/js/.changeset/issue-41-pipeline-epipe.md deleted file mode 100644 index 7caee98..0000000 --- a/js/.changeset/issue-41-pipeline-epipe.md +++ /dev/null @@ -1,10 +0,0 @@ ---- -'command-stream': patch ---- - -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.