fix(security): migrate shell-exec sites to execFile (RUSH-554)#22
Closed
prix-cloud[bot] wants to merge 3 commits intomainfrom
Closed
fix(security): migrate shell-exec sites to execFile (RUSH-554)#22prix-cloud[bot] wants to merge 3 commits intomainfrom
prix-cloud[bot] wants to merge 3 commits intomainfrom
Conversation
The Factory Floor bot accidentally added two platform-specific deps (@esbuild/linux-x64, @rollup/rollup-linux-x64-gnu) and bumped the version field. These would break installs on macOS/Windows. Reset package.json + package-lock.json to match main; the security fix (execFile migration) in the .ts files is the real change. Co-Authored-By: Claude Opus 4.7 (1M context) <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 changed
Replaced all
exec/execAsynccall sites that built command strings viatemplate-literal interpolation with
execFile/execFileAsync, passingarguments as arrays. No shell is involved, so user-controlled values (MCP
server names, remote hosts, binary paths, package specs) are passed as
literal argv entries and cannot escape into shell metacharacter injection.
Files modified
src/lib/agents.ts— primary fixgetCliVersion(line 370):${agent.cliCommand} --version→execFileAsync(cliCommand, ['--version'])isMcpRegistered(line 753):${agent.cliCommand} mcp list→execFileAsync(cliCommand, ['mcp', 'list'])registerMcp(lines 783–789): template-literalcmdstring replaced with explicit argv array;commandstring split on whitespace into tokens and spread after--unregisterMcp(line 813):"${name}"shell quoting replaced withexecFileAsync(bin, ['mcp', 'remove', name])src/lib/drive-sync.ts— rsync + ssh callspull: rsync template →execFileAsync('rsync', ['-az', '--exclude=config.json', remote, localDir])push: ssh mkdir template →execFileAsync('ssh', [remote, 'mkdir -p ~/.agents/drive']); rsync template →execFileAsync('rsync', [...])src/lib/versions.ts— npm calls + binary version checkgetLatestNpmVersion:npm view ${pkg} version→execFileAsync('npm', ['view', pkg, 'version'])installVersion:npm install ${spec}→execFileAsync('npm', ['install', spec], { cwd })getInstalledVersion:${binaryPath} --version→execFileAsync(binaryPath, ['--version'])src/commands/fork.ts— gh CLI callgh repo fork ${repoSlug} --clone=false→execFileAsync('gh', ['repo', 'fork', repoSlug, '--clone=false'])tests/agents.test.ts— regression testregisterMcp('claude', '"; touch /tmp/pwn_RUSH554 #', ...)with/bin/echoas the binary and asserts/tmp/pwn_RUSH554does not exist after the call.Why
execruns commands through/bin/sh -c. Any template-literal interpolationis shell-parsed, so a value like
"; touch /tmp/pwn #in an MCP server namewould break out of the surrounding double-quotes and run arbitrary commands.
execFilebypasses the shell entirely — each argument is a verbatim stringhanded to the OS
execvesyscall.Closes RUSH-554.
How to test
npm run build npm test -- tests/agents.test.tsThe new regression test (
registerMcp - shell injection prevention) shouldpass. It would fail on the old
exec-based code because/tmp/pwn_RUSH554would be created by the injected
touchcommand.