fix(proc): pin shell:false after opts spread in run() and dockerExec() - #2
Open
anupamme wants to merge 1 commit into
Open
fix(proc): pin shell:false after opts spread in run() and dockerExec()#2anupamme wants to merge 1 commit into
anupamme wants to merge 1 commit into
Conversation
Move safety-critical options (encoding, shell:false, maxBuffer) after the caller-supplied ...opts spread in both helpers so no caller can silently override them. Adds test/proc.test.js with regression coverage for the shell invariant and full path coverage for dockerExec(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Two places in
lib/proc.jsbuilt the options object forchild_processcalls with safety-critical flags before the caller-suppliedoptsspread:This meant a caller could pass
{ shell: true }(or overrideencoding/maxBuffer) and silently defeat the no-shell contract of both helpers. No current caller does this, but the invariant was unenforced at the call site.Moving the pinned flags after the spread closes the gap:
Legitimate caller opts (
cwd,env,stdio, etc.) still pass through; only the safety-critical keys are pinned. (shell: falseis inert onexecFileSync, which ignores it, but makes the intent explicit and consistent withspawnSync.)What this is not
This is not a known-exploitable injection vulnerability. PR #1 was a false positive — Semgrep flagged
...optsreaching exec, but no untrusted data flows throughoptsin any current caller, and all call sites already use argv-array form with noshell: true. This follow-up addresses the defensive-programming gap you identified: that theshelloverride avenue existed at all.Testing
Added
test/proc.test.js— Node built-innode:test+node:assert/strict, no new dependencies. Uses the samerequire.cachepre-seeding stub pattern already intest/lib.test.js.The suite covers:
shell: falseis the effective value even when a caller passes{ shell: true }(bothrun()anddockerExec()); same forencodingandmaxBufferSC_DOCKER_SUDO=1forced sudo,SC_DOCKER_SUDO=0no-fallback, auto-fallback onpermission deniedandcannot connect to the docker daemoncontainerNamevalidation,res.errorrethrow, non-zero exit throwAll 134 tests pass (
node --test "test/**/*.test.js").Closes the hardening gap identified in #1. Thanks again for the clear review.