fix(ext/node): handle shell redirections in child_process exec by fraidev · Pull Request #32087 · denoland/deno

fraidev · 2026-02-06T20:25:47Z

When child_process.exec() runs a shell command like "deno" "script.js" < "input.txt", transformDenoShellCommand failed to add run -A because shell redirections (<, >, |) were included in the args passed to the CLI parser, triggering the metacharacter safety bail-out.

Enables node_compat tests: test-stdin-from-file.js and test-stdin-from-file-spawn.js.

Copilot

Pull request overview

Improves Deno’s Node-compat child_process.exec() shell-command translation so Deno-invoking commands containing shell redirections/pipes can still be rewritten (eg adding run -A) without shell operators confusing the CLI arg parser. Also adds the legacy process.openStdin() API and enables the related Node-compat stdin tests.

Changes:

Add splitShellSuffix() and integrate it into transformDenoShellCommand() to separate shell operators from CLI args before translation, then re-append the suffix.
Expand the shell variable detection regex to match "$VAR" in addition to "${VAR}" / ${VAR} / $VAR.
Implement process.openStdin() and enable stdin-from-file node_compat tests in config.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
tests/node_compat/config.jsonc	Enables two stdin-from-file Node-compat tests.
ext/node/polyfills/process.ts	Adds `process.openStdin()` implementation for legacy API compatibility.
ext/node/polyfills/internal/child_process.ts	Adds shell-suffix splitting and improves env-var Deno-path detection to allow arg translation with redirections/pipes present.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ext/node/polyfills/internal/child_process.ts

bartlomieju · 2026-02-09T14:40:57Z

ext/node/polyfills/process.ts

+/** https://nodejs.org/api/process.html#processopenStdin */
+process.openStdin = () => {
+  process.stdin.resume();
+  return process.stdin;
+};


Is there a separate test to enable for this one?

Actually, test-stdin-from-file.js uses it, because it uses the script echo-close-check.js that uses process.openStdin.

Do you think we need one test only for it?

That's fine

bartlomieju

I asked @dsherret to take a look at this PR - feels to me we might want to use deno_task_shell to perform this parsing instead of hand-rolling it.

bartlomieju

Discussed with David and while it's not necessary to use deno_task_shell, I got Claude to highlight a few potential issues in the current implementation:

  Potential issues:

  1. No backslash escape handling. splitShellSuffix doesn't handle \< or \>. A command like echo hello \< world would
  incorrectly split at the escaped <. This is likely rare in child_process.exec() usage but worth noting.
  2. & as first operator vs &&. If the command is script.js & echo done, the function splits at & — which is correct (it's a
   shell operator). But consider script.js --flag=a&b (no spaces around &): this would incorrectly split at &. Again,
  uncommon in practice since shell commands typically have spaces around operators.
  3. Semicolons in URLs or arguments. A command like deno run script.ts --url="http://example.com" is fine (semicolons
  inside quotes are handled), but deno run script.ts --separator=; without quotes would split at ;. This is actually the
  correct shell behavior though — an unquoted ; is a shell operator.

  Shell variable regex fix

  // Old: /^(?:"\$\{([^}]+)\}"|\$\{([^}]+)\}|\$([A-Za-z_][A-Za-z0-9_]*))/
  // New: /^(?:"\$\{([^}]+)\}"|\"\$([A-Za-z_][A-Za-z0-9_]*)\"|\$\{([^}]+)\}|\$([A-Za-z_][A-Za-z0-9_]*))/

  Adds matching for "$VARNAME" (quoted, no braces). Group indices updated correctly from 3 to 4.

So maybe worth trying out with deno_task_shell parsing?

fraidev · 2026-02-17T04:04:53Z

Discussed with David and while it's not necessary to use deno_task_shell, I got Claude to highlight a few potential issues in the current implementation:

  Potential issues:

  1. No backslash escape handling. splitShellSuffix doesn't handle \< or \>. A command like echo hello \< world would
  incorrectly split at the escaped <. This is likely rare in child_process.exec() usage but worth noting.
  2. & as first operator vs &&. If the command is script.js & echo done, the function splits at & — which is correct (it's a
   shell operator). But consider script.js --flag=a&b (no spaces around &): this would incorrectly split at &. Again,
  uncommon in practice since shell commands typically have spaces around operators.
  3. Semicolons in URLs or arguments. A command like deno run script.ts --url="http://example.com" is fine (semicolons
  inside quotes are handled), but deno run script.ts --separator=; without quotes would split at ;. This is actually the
  correct shell behavior though — an unquoted ; is a shell operator.

  Shell variable regex fix

  // Old: /^(?:"\$\{([^}]+)\}"|\$\{([^}]+)\}|\$([A-Za-z_][A-Za-z0-9_]*))/
  // New: /^(?:"\$\{([^}]+)\}"|\"\$([A-Za-z_][A-Za-z0-9_]*)\"|\$\{([^}]+)\}|\$([A-Za-z_][A-Za-z0-9_]*))/

  Adds matching for "$VARNAME" (quoted, no braces). Group indices updated correctly from 3 to 4.

So maybe worth trying out with deno_task_shell parsing?

Sure, I added a commit using deno_task_shell, also moving the split part to Rust (not sure that it makes more sense, though.)

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ext/node/ops/shell.rs

ext/node/polyfills/internal/child_process.ts

ext/node/Cargo.toml

ext/node/ops/shell.rs

dsherret

LGTM

dsherret · 2026-02-19T15:03:15Z

ext/node/ops/shell.rs

+    if !cfg!(windows) && ch == b'\\' && !in_single {
+      i += 2;
+      continue;
+    }


Some users on Windows might have the shell configured to be something else. For example, the "shell" argument can be a path to the specific shell to use. I wonder if we need to handle that.

I added a commit that adds an argument "is_cmd" for windows 63f1758

- Fix `child_process` shell command translation to handle shell redirections (`1>&-`, `< file`), piped commands (`cmd1 | cmd2`), and shell variable prefixes (`"$NODE"`, `${NODE}`). Previously shell operators were passed to the CLI arg translator, causing parse failures or incorrect output. - Remove duplicate `transformDenoShellCommand` call from `normalizeSpawnArguments` — it was already called in `buildCommand`, and the double transformation corrupted POSIX `'\''` quoting. - Add `wrap_eval` parameter to `op_node_translate_cli_args` so shell command contexts skip eval code wrapping (which introduces metacharacters unsafe for shell embedding). - Map `BrokenPipe` to `EPIPE` (was incorrectly mapped to `EBADF`) in `stream_wrap`, and catch `BrokenPipe` in `process.stdout`/ `process.stderr` `writeSync` to emit it as an async error event. - Add minimal `_handle` with `ref()`/`unref()`/`close()` to PIPE stdin. Intentionally omits `readStart`/`readStop`/`reading` — adding those resets `_readableState.reading`, which triggers duplicate `_read()` calls with orphaned reffed promises that prevent process exit. - Ref `io.stdin` in `_read()` and unref on pause when no `readStop` exists, matching Node.js event loop ref-counting behavior. - Add platform-specific quoting in `transformDenoShellCommand`: POSIX uses single quotes with `'\''` escaping, Windows uses double quotes with `\\` doubling before `"`. - Add Windows `cmd.exe` detection (`/d /s /c` pattern) in `buildCommand` for shell command transformation. - Deduplicate `--unstable-*` flags in `fork()` between CLI translator and bootstrap args. Enables 3 new node_compat tests: - `test-stdio-closed.js` - `test-stdout-close-catch.js` - `test-stdout-close-unref.js` Needs: - [x] #32086 - [x] #32087

fraidev changed the title ~~Fix/node child process shell redirections~~ fix(ext/node): handle shell redirections in child_process exec Feb 6, 2026

bartlomieju requested a review from Copilot February 9, 2026 11:50

Copilot started reviewing on behalf of bartlomieju February 9, 2026 11:51 View session

Copilot AI reviewed Feb 9, 2026

View reviewed changes

ext/node/polyfills/internal/child_process.ts Outdated Show resolved Hide resolved

fraidev mentioned this pull request Feb 9, 2026

fix(ext/node): enable test-stdio-closed tests #32089

Closed

2 tasks

fraidev force-pushed the fix/node-child-process-shell-redirections branch from bdcf8f6 to ba52ee9 Compare February 9, 2026 12:38

bartlomieju reviewed Feb 9, 2026

View reviewed changes

bartlomieju reviewed Feb 12, 2026

View reviewed changes

fraidev force-pushed the fix/node-child-process-shell-redirections branch from f419a1e to 395d76b Compare February 16, 2026 23:34

fraidev requested a review from Copilot February 17, 2026 04:05

Copilot started reviewing on behalf of fraidev February 17, 2026 04:05 View session

Copilot AI reviewed Feb 17, 2026

View reviewed changes

ext/node/ops/shell.rs Outdated Show resolved Hide resolved

ext/node/polyfills/internal/child_process.ts Show resolved Hide resolved

ext/node/Cargo.toml Outdated Show resolved Hide resolved

ext/node/ops/shell.rs Outdated Show resolved Hide resolved

fraidev force-pushed the fix/node-child-process-shell-redirections branch 2 times, most recently from 838ff76 to c9115e7 Compare February 17, 2026 15:20

fraidev added 7 commits February 17, 2026 16:15

fix(ext/node): handle shell redirections in child_process exec

c6ddb5e

improve splitShellSuffix

3a2594c

use deno_task_shell instead

afda5d6

fix quotes

a5ebcb1

escaped posix shell vars

5d42788

fix windows

d787bb4

format

0602057

fraidev force-pushed the fix/node-child-process-shell-redirections branch from c9115e7 to 0602057 Compare February 17, 2026 19:15

fraidev requested a review from bartlomieju February 17, 2026 20:11

remove deno_task_shell again

af2f3b2

fraidev requested a review from dsherret February 19, 2026 02:53

dsherret reviewed Feb 19, 2026

View reviewed changes

ext/node/ops/shell.rs Outdated Show resolved Hide resolved

use &str instead

66cd0b8

dsherret approved these changes Feb 19, 2026

View reviewed changes

fraidev added 2 commits February 19, 2026 13:45

support more windows shells

63f1758

revert deno_task_shell Cargo.toml changes

205e298

fraidev merged commit 9642885 into denoland:main Feb 19, 2026
111 checks passed

fraidev mentioned this pull request Feb 19, 2026

fix(ext/node): enable test-stdio-closed tests #32237

Merged

2 tasks

Conversation

fraidev commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

bartlomieju Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

fraidev Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

bartlomieju Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

bartlomieju left a comment

Choose a reason for hiding this comment

Uh oh!

bartlomieju left a comment

Choose a reason for hiding this comment

Uh oh!

fraidev commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dsherret left a comment

Choose a reason for hiding this comment

Uh oh!

dsherret Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

fraidev Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fraidev commented Feb 6, 2026 •

edited

Loading