fix(security): shell-quote filesystem-derived paths in validation commands

[aaronjmars]

Summary

Several mappers interpolate filesystem-derived paths and package.json script keys directly into the command strings that the validation pipeline executes (clawpatch fix and clawpatch ci). Those command strings are run through runCommand (src/exec.ts), which uses spawn(..., { shell: true }) — i.e. /bin/sh -c "<command>". An attacker-controlled name flowed unquoted into that string, so shell metacharacters in a workspace package directory, a Swift package directory, an Elixir test filename, or a package.json script key were parsed as shell syntax.

Impact

When an operator runs clawpatch fix --finding <id> or clawpatch ci against a project they did not author, any of the following attacker-controlled names execute arbitrary shell commands on the operator's machine with their privileges:

• pnpm / yarn / bun / npm workspace package directories. scriptCommand and nodeScriptCommand produced pnpm --dir packages/$(id)-pkg test, yarn --cwd …, bun --cwd …, npm --prefix …. A workspace member whose directory name contained ;, $( … ), backticks, or | ran arbitrary commands during validation.
• package.json script keys. Same builders took script unquoted. A repo declaring a script named $(curl …|sh) ran that script when the validation pipeline mapped its target.
• Swift packages. prefixSwiftSeed and prefixTestRefs produced swift test --package-path $(id)-pkg. A SwiftPM package directory with shell metacharacters in its name landed on the shell.
• Elixir test filenames. associatedTests produced mix test test/$(id)_test.exs. A repo with a _test.exs file whose name contained shell metacharacters ran them at validation time.

Severity: high. Reachability is the operator-runs-clawpatch-against-untrusted-repo path that SECURITY.md already names ("repository feature mapping", "patch workflow state").

Location

• src/mappers/projects.ts — scriptCommand
• src/mappers/shared.ts — nodeScriptCommand
• src/mappers/elixir.ts — associatedTests (mix test <path>)
• src/mappers/swift.ts — prefixSwiftSeed and prefixTestRefs (swift test --package-path <pkg>)

Fix

Route each filesystem-derived or config-derived value through the existing shellQuotePath (src/shell.ts) before splicing it into the command string. shellQuotePath returns ordinary path-like values verbatim (whitelist of [A-Za-z0-9_./:@+-]) and wraps anything else in double quotes with $, backtick, and backslash escaped — so the value reaches the shell as a literal even under shell: true.

This is intentionally a local fix; it keeps runCommand as the validation execution path (where freeform pipelines are valid) and only hardens the few sites where the command string is built from attacker-reachable strings.

Verification

• pnpm typecheck — clean
• pnpm lint — clean (0 warnings, 0 errors)
• pnpm format:check — clean
• pnpm test — 720 passed, 1 skipped (up from 714 + 1 baseline; the new tests live in src/cmd-injection-regression.test.ts)

The regression suite covers two angles per builder:

1. With an attacker-controlled name ($(id)-pkg, $(id)), the produced command parses safely under /bin/sh -c (no unquoted $( … ), no unquoted ;, no && / || outside quotes).
2. With ordinary names (packages/ui, test), the produced command is byte-identical to the previous behavior — no over-quoting.

There is also a Swift end-to-end test that walks the full detectProject → mapFeatures → validationCommandsForFeature pipeline against a project whose package directory name contains $(id), and asserts the produced commands are shell-safe.

Detected by

Aeon — manual review against the maintainer's own SECURITY.md scope statement (the in-scope surface lists "repository feature mapping" and "patch workflow state"; the validation execution path falls under both). Class: CWE-78 (OS command injection via filesystem-derived strings) and CWE-88 (argument injection in --dir / --cwd / --prefix / --package-path flags).

Scanner pass (semgrep p/security-audit + p/owasp-top-ten + p/secrets + p/javascript + p/typescript + p/nodejs + p/rust + p/python, trufflehog filesystem + git, osv-scanner --recursive) returned 0 findings; the issue was reached entirely by reading the mappers against the operator-runs-validation trust boundary.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed May 26, 2026, 8:55 AM ET / 12:55 UTC.

Summary
The PR shell-quotes repo-derived validation command fragments in Node/Nx/Turbo, Elixir, Swift, and Rust mappers and adds command-injection regression tests.

Reproducibility: yes. from source inspection: current main sends validation command strings to spawn(..., { shell: true }) while mapper builders interpolate repo-derived fragments unquoted. I did not execute a live throwaway repo, so this is source-reproducible rather than fully reproduced.

Review metrics: 2 noteworthy metrics.

• Changed surface: 7 files, 230 additions, 25 deletions. The patch spans six mapper files plus one regression test file, so review should focus on validation command construction.
• Base drift: 1 main-only commit; merge-tree clean. Current main moved after the PR base, but the read-only merge-tree check did not show conflicts.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• Add redacted terminal output or logs from a throwaway repo showing clawpatch ci or clawpatch fix does not execute malicious package, crate, or test names.
• Redact IP addresses, API keys, phone numbers, non-public endpoints, and other private details before posting proof.
• Update the PR body after adding proof; if no fresh review appears, ask a maintainer to comment @clawsweeper re-review.

Proof guidance:
Needs real behavior proof before merge: Proof is limited to claimed test output; before merge, add redacted terminal output or logs from a real throwaway repo showing malicious names are treated literally, then update the PR body for re-review and ask a maintainer for @clawsweeper re-review if needed.

Risk before merge

• No after-fix throwaway-repo terminal output or logs were posted; tests alone do not prove the real clawpatch ci or clawpatch fix path against malicious names.
• The PR body publicly documents an unmerged command-injection path despite SECURITY.md asking for private reporting of unpatched vulnerabilities, so maintainers should coordinate disclosure and release handling.

Maintainer options:

1. Decide the mitigation before merge
   Land the narrow quoting fix after contributor-supplied real behavior proof and maintainer security handling confirm the validation path is safe end to end.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge
Human/security review should verify disclosure handling and wait for contributor-supplied real behavior proof; no narrow automated repair remains after the Rust sink was added.

Security
Needs attention: The code diff hardens the shell boundary, but the public PR body contains unmerged vulnerability details that need maintainer security handling.

Review details

Best possible solution:

Land the narrow quoting fix after contributor-supplied real behavior proof and maintainer security handling confirm the validation path is safe end to end.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: current main sends validation command strings to spawn(..., { shell: true }) while mapper builders interpolate repo-derived fragments unquoted. I did not execute a live throwaway repo, so this is source-reproducible rather than fully reproduced.

Is this the best way to solve the issue?

Yes for the code shape: using the existing shellQuotePath helper at each repo-derived interpolation is the narrow maintainable fix and preserves ordinary command strings. Merge should still wait for real behavior proof and security-process handling.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against ed3d5750ff89.

Label changes

Label justifications:

• P0: The PR addresses a command-injection path in shell-interpreted validation commands run on operator machines.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Proof is limited to claimed test output; before merge, add redacted terminal output or logs from a real throwaway repo showing malicious names are treated literally, then update the PR body for re-review and ask a maintainer for @clawsweeper re-review if needed.

Evidence reviewed

Security concerns:

• [medium] Coordinate public vulnerability disclosure — SECURITY.md:5
  SECURITY.md asks reporters not to open public issues or PRs disclosing unpatched vulnerabilities; maintainers should decide whether to move details to a private advisory and coordinate release handling before merge.
  Confidence: 0.86

What I checked:

• Repository policy read: AGENTS.md was read in full; its TypeScript/Vitest and security-sensitive mapper guidance applies to this review. (AGENTS.md:1, ed3d5750ff89)
• Shell sink on current main: Current main runs validation command strings through spawn(command, { shell: true }), so mapper-built strings are interpreted by a shell. (src/exec.ts:26, ed3d5750ff89)
• PR quotes Node and Nx command fragments: At PR head, scriptCommand quotes package roots and script names, and nxCommand quotes repo-derived project names before composing shell command strings. (src/mappers/projects.ts:197, c272b53fd7f8)
• PR quotes remaining mapper sinks: At PR head, Turbo filters, Elixir test paths, Swift package roots, and Rust conventional-crate manifest paths are routed through shellQuotePath. (src/mappers/rust.ts:96, c272b53fd7f8)
• Regression coverage: The new regression test file checks malicious and ordinary inputs for Node script helpers, Turbo, Nx, Swift end-to-end mapping, and Rust end-to-end mapping. (src/cmd-injection-regression.test.ts:37, c272b53fd7f8)
• Clean merge shape: The PR is one commit behind current main, but git merge-tree showed a clean three-way merge with no conflicts in the touched files. (c272b53fd7f8)

Likely related people:

• Peter Steinberger: Git blame shows the shell runner, shellQuotePath helper, and affected mapper command builders in the v0.4.0 release commit on current main. (role: introduced behavior / adjacent owner; confidence: high; commits: cdd58ac59213; files: src/exec.ts, src/shell.ts, src/mappers/projects.ts)

What the crustacean ranks mean

• 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
• 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
• 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
• 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
• 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
• 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
• 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

[clawsweeper]

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?

• The egg game starts only after the PR passes the real-behavior proof check.
• Before that, no creature or rarity is rolled. The treat waits for real proof.
• This is still just collectible flavor: proof affects review readiness, not creature quality.

[aaronjmars]

Good catch — both gaps were real and in-scope, now fixed in a902287.

What was wrong: turboCommand (src/mappers/turbo.ts) interpolated the --filter value, and nxCommand (src/mappers/projects.ts) interpolated the project name, both unquoted. The trace confirms they hit the same sink this PR already hardens elsewhere:

• filter ← turboFilter() = package.json name, or ./${project.root} (on-disk dir) — repo-controlled.
• projectName ← Nx project.json name / package.json name / dir basename — repo-controlled.
• Both flow through projectTargetCommand → validationCommandsForFeature → app.ts runCommand → exec.ts spawn(command, { shell: true }). With shell: true the whole string goes to /bin/sh -c, so an unquoted $(id) / ` / ; in a package or project name executes.

Fix: both now route the repo-derived value through shellQuotePath (same helper as scriptCommand/nodeScriptCommand). task/target stay unquoted since they come from the validationTaskNames allowlist (["test","build","lint","typecheck","format"]), so no over-quoting.

Proof: added regression cases to src/cmd-injection-regression.test.ts mirroring the existing ones — $(id) as a package name, ./packages/$(id)-pkg as a Turbo root, and $(id) as an Nx project name across all package managers, asserting isShellSafe; plus "ordinary inputs unchanged" cases to guard against over-quoting. Full suite: 724 passed, 1 skipped.

[aaronjmars]

Addressed the remaining sink you flagged — the unquoted Rust manifest-path — in c272b53.

What was wrong: the Rust mapper's conventional crate discovery (src/mappers/rust.ts) reads crates/* directly via readdir and built cargo test --manifest-path ${member}/Cargo.toml with the directory name interpolated unquoted. A crate directory passes only the inside-root check, so a folder literally named $(id) survives and — because runCommand (src/exec.ts) uses spawn(_, { shell: true }) — executes under /bin/sh -c. Same class this PR hardens for the Node/Nx/Turbo/Elixir/Swift mappers; this one was missed.

Fix: route the manifest path through shellQuotePath (the same helper used by the other mappers). Ordinary crate names stay unquoted (no over-quoting); names with shell metacharacters are quoted and escaped.

Behavior proof: added an end-to-end regression test that builds a workspace containing a crates/$(id)-crate directory, runs the real detectProject → mapFeatures → validationCommandsForFeature pipeline, and asserts every produced command is shell-safe. It also asserts a --manifest-path command is actually produced (so the test can't silently pass on an empty set).

Before the fix (reverting only rust.ts, keeping the test):

× rust end-to-end: malicious conventional crate directory cannot inject into cargo --manifest-path
AssertionError: unsafe command produced: cargo test --manifest-path crates/$(id)-crate/Cargo.toml: expected false to be true
 Test Files  1 failed (1)

After the fix:

 Test Files  1 passed (1)
      Tests  11 passed (11)

Full suite green (725 passed / 1 skipped), tsc and oxlint clean, oxfmt --check clean.