[fix] Use execFile instead of a shell string in commandExists() - #99
Merged
Conversation
commandExists() was the one function in the local-execution surface
that built a shell command string (exec(`command -v ${cmd}`)) instead
of using execFile/execFileSync with an argv array, unlike every other
command execution in this codebase - which has extensive commentary
explaining why shell strings are avoided wherever a value could trace
back to registry/package metadata. It's only ever called with literal
values today ('dpkg'/'rpm' in ManagerLocal.install()'s Linux branch),
so there's no live injection vector, but it was a landmine for the
next contributor who calls it with something dynamic. Flagged in an
internal spec-compliance audit.
Switched to execFile('which', [cmd], ...) - `which` is a real
executable (unlike `command`, a shell builtin with no standalone
binary), so this avoids a shell entirely rather than just working
around it. Also adds commandExists()'s first test coverage.
Co-Authored-By: Claude Sonnet 5 <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
1 participant
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.
Summary
commandExists()was the one function in the local-execution surface that built a shell command string (exec(\command -v ${cmd}`)) instead of usingexecFile/execFileSyncwith an argv array, unlike every other command execution in this codebase — which has extensive commentary explaining why shell strings are avoided wherever a value could trace back to registry/package metadata. It's only ever called with literal values today ('dpkg'/'rpm'inManagerLocal.install()`'s Linux branch), so there's no live injection vector, but it was a landmine for the next contributor who calls it with something dynamic. Flagged in an internal spec-compliance audit.execFile('which', [cmd], ...)—whichis a real executable (unlikecommand, a shell builtin with no standalone binary), so this avoids a shell entirely rather than just working around it.commandExists()'s first test coverage.Test plan
npm run checkpasses (format, lint, build, tests — 203/203)commandExists('node')→ true,commandExists('this-command-should-not-exist-xyz123')→ false (skipped on Windows, wherecommandExistsisn't actually used andwhichisn't a standalone command)🤖 Generated with Claude Code