Skip to content

Quote mapper-generated validation command arguments#111

Open
rohitjavvadi wants to merge 1 commit into
openclaw:mainfrom
rohitjavvadi:fix/quote-validation-commands
Open

Quote mapper-generated validation command arguments#111
rohitjavvadi wants to merge 1 commit into
openclaw:mainfrom
rohitjavvadi:fix/quote-validation-commands

Conversation

@rohitjavvadi
Copy link
Copy Markdown
Contributor

Summary

  • Quote repo-derived command arguments before mapper-generated validation commands are assembled.
  • Cover Node package roots/scripts, Nx targets, Turbo filters, Rust manifest paths, Swift package roots, and Elixir test paths.
  • Add regression coverage for shell metacharacters and command substitution syntax while keeping simple command output unchanged.

Why

Clawpatch later executes validation command strings through the shell. If a mapped package path or package name contains shell syntax, a command like pnpm --dir apps/web; touch INJECTED test can be interpreted as two shell commands instead of one literal package path.

Proof

I tested a fixture with a workspace package under apps/web; touch INJECTED. After this change, clawpatch map generated:

pnpm --dir "apps/web; touch INJECTED" test

Running that generated command did not create the INJECTED sentinel file, which confirms the semicolon stayed part of the package path instead of becoming shell syntax.

Testing

  • ./node_modules/.bin/oxfmt src/mapper.test.ts src/mappers/elixir.ts src/mappers/projects.ts src/mappers/rust.ts src/mappers/shared.ts src/mappers/swift.ts src/mappers/turbo.ts src/shell.test.ts --check
  • ./node_modules/.bin/oxlint . --config oxlint.json
  • ./node_modules/.bin/tsc -p tsconfig.json --noEmit
  • ./node_modules/.bin/tsc -p tsconfig.build.json
  • ./node_modules/.bin/vitest run src/shell.test.ts src/mapper.test.ts
  • ./node_modules/.bin/vitest run
  • live fixture proof with apps/web; touch INJECTED

Fixes #110

@rohitjavvadi rohitjavvadi requested a review from a team as a code owner May 24, 2026 07:07
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 24, 2026

Codex review: needs maintainer review before merge.

Latest ClawSweeper review: 2026-05-24 07:11 UTC / May 24, 2026, 3:11 AM ET.

Workflow note: Future ClawSweeper reviews update this same comment in place.

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.

Summary
The branch quotes repo-derived mapper validation command arguments for Node/Nx, Turbo, Rust, Swift, and Elixir and adds shell-metacharacter regression tests.

Reproducibility: yes. Source inspection shows current main can generate commands like an unquoted pnpm --dir <packageRoot> test and then execute those strings with shell: true; a workspace path containing ; touch INJECTED is a high-confidence reproduction path, though I did not run it in this read-only review.

PR rating
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Summary: Focused security hardening with good regression coverage and usable live fixture proof; the remaining uncertainty is only that I did not run the suite in this read-only pass.

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.

Real behavior proof
Sufficient (live_output): The PR body includes after-fix live fixture proof showing the generated quoted command for a semicolon-bearing workspace path and states that running it did not create the sentinel file.

Risk before merge

  • I did not execute the Vitest/typecheck suite during this read-only review; verification relies on source inspection, static diff checks, and the contributor-reported checks and live fixture proof.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused quoting hardening after normal maintainer review and CI, keeping the existing shellQuotePath helper as the shared command-argument escaping path.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge
No ClawSweeper repair lane is needed because the PR already contains the focused implementation and tests; maintainers should handle normal review and merge checks.

Security
Cleared: The patch narrows a command-injection surface using the existing quoting helper and does not add dependencies, generated code, CI changes, or broader execution permissions.

Review details

Best possible solution:

Land the focused quoting hardening after normal maintainer review and CI, keeping the existing shellQuotePath helper as the shared command-argument escaping path.

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

Yes. Source inspection shows current main can generate commands like an unquoted pnpm --dir <packageRoot> test and then execute those strings with shell: true; a workspace path containing ; touch INJECTED is a high-confidence reproduction path, though I did not run it in this read-only review.

Is this the best way to solve the issue?

Yes. Quoting dynamic arguments where mapper validation commands are assembled is the narrow maintainable fix, and the PR uses the existing helper rather than adding a parallel escaping implementation.

Label changes:

  • add P1: The PR addresses a security-sensitive validation command injection path in mapper-generated shell commands that can affect real CLI users reviewing hostile or unusual repositories.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix live fixture proof showing the generated quoted command for a semicolon-bearing workspace path and states that running it did not create the sentinel file.
  • add rating: 🦞 diamond lobster: Current PR rating is 🦞 diamond lobster because proof is 🦞 diamond lobster, patch quality is 🦞 diamond lobster, and Focused security hardening with good regression coverage and usable live fixture proof; the remaining uncertainty is only that I did not run the suite in this read-only pass.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix live fixture proof showing the generated quoted command for a semicolon-bearing workspace path and states that running it did not create the sentinel file.

Label justifications:

  • P1: The PR addresses a security-sensitive validation command injection path in mapper-generated shell commands that can affect real CLI users reviewing hostile or unusual repositories.
  • rating: 🦞 diamond lobster: Current PR rating is 🦞 diamond lobster because proof is 🦞 diamond lobster, patch quality is 🦞 diamond lobster, and Focused security hardening with good regression coverage and usable live fixture proof; the remaining uncertainty is only that I did not run the suite in this read-only pass.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix live fixture proof showing the generated quoted command for a semicolon-bearing workspace path and states that running it did not create the sentinel file.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix live fixture proof showing the generated quoted command for a semicolon-bearing workspace path and states that running it did not create the sentinel file.

What I checked:

  • Current main still interpolates repo-derived Node command parts: On current main, scriptCommand builds package-manager validation command strings with raw packageRoot and script values such as pnpm --dir ${packageRoot} ${script} and npm --prefix ${packageRoot} run ${script}. (src/mappers/projects.ts:196, 857d854ac8d0)
  • Validation commands run through a shell: Feature validation commands are passed to runCommand, and runCommandRaw uses child_process.spawn with shell: true, so unquoted command metacharacters can be interpreted by the shell. (src/exec.ts:26, 857d854ac8d0)
  • PR quotes the central mapper command builders: The PR commit adds shellQuotePath to scriptCommand, nodeScriptCommand, Nx, Turbo, Rust manifest, Swift package-root, and Elixir test-path command assembly while preserving simple unquoted arguments. (src/mappers/projects.ts:197, 71ad22437f9f)
  • Regression tests cover the reported injection shape: The PR adds tests for Node package roots/scripts, Turbo filters, Rust manifest paths, Swift package roots, Elixir test paths, and POSIX command substitution escaping with semicolon or $() payloads. (src/mapper.test.ts:14, 71ad22437f9f)
  • Diff scope is focused and format-clean by static check: The PR changes only mapper implementation/tests and shellQuotePath tests; git diff --check between base and head produced no whitespace errors. (71ad22437f9f)
  • Feature history points to the release-introduced mapper surface: Blame on the affected mapper command builders and shellQuotePath points to commit cdd58ac, the v0.4.0 release commit that introduced the current mapper surface in this checkout. (src/mappers/projects.ts:196, cdd58ac59213)

Likely related people:

  • Peter Steinberger: Blame and -S history for the affected mapper command builders and shellQuotePath in this checkout point to the v0.4.0 release commit authored by Peter Steinberger; the checkout is grafted there, so finer-grained pre-release provenance is not available locally. (role: introduced behavior / likely follow-up owner; confidence: medium; commits: cdd58ac59213; files: src/mappers/projects.ts, src/mappers/shared.ts, src/mappers/turbo.ts)

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

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P1 Urgent regression or broken agent/channel workflow affecting real users now. labels May 24, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 24, 2026

ClawSweeper PR egg

✨ Hatched: 🌱 uncommon Velvet Patch Peep

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🌱 uncommon.
Trait: polishes edge cases.
Image traits: location green-check meadow; accessory review stamp; palette rose quartz and slate; mood watchful; pose curling around a status light; shell brushed metal shell; lighting warm desk-lamp glow; background delicate sparkle particles.
Share on X: post this hatch
Copy: My PR egg hatched a 🌱 uncommon Velvet Patch Peep in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Quote mapper-generated validation command arguments

1 participant