fix(B19): allowlist binaries in verify-changes, add --allow-arbitrary-tool, npx --no-install (#22)

[bigknoxy]

What this fixes

Issue #22 (B19) — verify-changes executes arbitrary target-repo-chosen binaries. Score 50, P1, area:security.

The problem

Bun.spawn received untrusted string arguments from three sources:

1. Pass-through flags: --formatter, --linter, --test-runner values handed directly to spawn
2. npx installs from the network: A typo or attacker-chosen runner name was a supply-chain execution since npm registry downloads + execute packages on first use
3. Auto-detect scans target repo's package.json: Repo being edited chose what code executes

What changed

1. Binary allowlist (default: reject)

• ~25 known verification tools whitelisted; anything else rejected with clear error listing allowed binaries and the override flag
• Tests use allowArbitraryTool: true for mock tools (echo, sleep) since they aren't testing security

2. npx --no-install

• TEST_RUNNER_MAP now uses npx --no-install vitest run and npx --no-install jest
• Missing runner is a clean error instead of a network fetch

3. Shell injection is inert

• Already the case (Bun.spawn with argv array, never shell: true)
• Added regression tests asserting $(), backticks, and pipes are treated as literal arguments

4. Resolved command line logged to stderr before execution
Every spawned command prints [verify-changes] running: <binary> <full args> so the caller can see exactly what about to execute.

5. Auto-detected tool flagged on stderr
When auto-detect fills in a tool from project config (not explicit flag), it announces this on stderr.

Testing

• 35 tests pass (25 existing + 9 new B19 regression tests)
• CLI verified: --linter "curl http://evil.example.com" rejected without --allow-arbitrary-tool
• All 9 new security tests cover allowlist rejection, shell metacharacter safety, npx --no-install

Files changed

File	Change
src/core/verify.ts	Allowlist + validation, npx --no-install, resolved logging
src/cli.ts	--allow-arbitrary-tool flag added
tests/verify.test.ts	9 B19 regression tests
docs/CLI-QUICKREF.md	Auto-regenerated to match new flag

Fixes #22

[bigknoxy]

Hermes Agent Code Review — PR #69

Verdict: Comment (security-critical fix, well-tested; minor suggestions only)

OK Looks Good

• Binary allowlist (ALLOWED_BINARIES) is the right fix for the B19 supply-chain vulnerability: rejects arbitrary binaries by default, with an explicit opt-in via --allow-arbitrary-tool.
• npx --no-install in TEST_RUNNER_MAP eliminates the network-fetch attack vector for vitest/jest.
• resolveCommand extracts the basename from path arguments (e.g. ./node_modules/.bin/x is resolved), preventing path-based bypasses.
• runTool logs the resolved command line to stderr before execution — excellent for auditability.
• 9 new regression tests cover: allowlist rejection, --allow-arbitrary-tool=true override, npx --no-install enforcement, shell metacharacter safety (canary file test for $(touch ...)), and auto-detected tool logging. The test suite is comprehensive for a security boundary.
• 35 tests pass (25 existing + 9 new).

Warnings / Suggestions

• resolveCommand splits on whitespace (cmd.split(/\s+/)) — this breaks if a binary path contains spaces. The allowlist check uses binary.split(/[\\/]/).pop() to get the basename, but if the binary itself has a space in its path, the split will mangle it. This is an edge case but worth noting in a comment.
• Allowlist is hard-coded in source — adding new tools (e.g., next, svelte-check, astro) requires a code change + release. Consider whether this should be configurable via env var for projects with unusual toolchains. The PR description says ~25 tools; this may grow over time.
• The test for shell metacharacters (echo $(touch ${canary})) is clever, but note that echo with $(...) as a literal argument still prints the string — it just doesn't execute it. This is the correct behavior with shell: false, but verify Bun.spawn doesn't have any shell fallback.
• go vet is missing from EXT_TOOLS — .go files get go vet typecheck from the extension fallback, but go is in the allowlist. However, go vet passes through resolveCommand and go is allowlisted, so this is fine — just confirming the chain works.

Suggestions

• Consider a test asserting that a path-traversal attempt (e.g., --formatter "../../bin/evil") is rejected. The basename extraction would resolve to evil which is not on the allowlist, so it should fail — worth an explicit test.
• The --allow-arbitrary-tool flag warns on each use — consider whether the CLI help text makes the security tradeoff clear enough.

Reviewed by Hermes Agent

[bigknoxy]

Code Review Summary

Verdict: Comment — strong P1 security fix. I verified all 35 tests pass locally (including 9 new B19 regression tests). One minor hardening nudge.

Critical

(None — the security holes are fixed, not introduced.)

Warnings

• src/core/verify.ts — allowlist approach is sound, but consider documenting the threat model. The PR correctly fixes three vectors: binary allowlist (default deny), npx --no-install to kill supply-chain fetches, and argv-array spawning (already inert to shell metacharacters). The regression test for $()/ canary confirms metacharacters stay literal (no canary file created on my run). One follow-up worth filing: the allowlist checks the basename of the binary. A symlink with a benign name pointing to a malicious binary would pass. This is low-likelihood (requires write access to a path on PATH or cwd), but worth a short note in the allowlist comment for future audit readers.

Suggestions

• resolveCommand: consider logging the rejected binary name too. Currently runTool logs the resolved command line on success but the allowlist rejection returns { passed: false, output: "security: ..." } without the canonical [verify-changes] running: line. For observability parity, you could emit [verify-changes] blocked: <binaryName> (allowlist) to stderr. Minor.
• The echo mock binary now requires allowArbitraryTool: true in several existing tests (echo is not on the allowlist). This is correct and the PR updates those call sites. Good catch keeping them passing.
• Consider extending the shell-metacharacter regression to backtick variants (``) and | pipes explicitly, though the argv-spawn fix already covers them structurally.

Looks Good

• Default-deny allowlist design with an explicit --allow-arbitrary-tool escape hatch is the correct security posture.
• npx --no-install on both vitest and jest entries closes the supply-chain vector cleanly.
• Clear [verify-changes] running: <cmd> logging on stderr before execution — great for auditability.
• Auto-detected tool announcement on stderr gives the caller visibility into target-repo-chosen tools.
• Test naming ("B19 — verify-changes security hardening") makes the security intent explicit.

Reviewed by Hermes Agent.