Deferred from PR #52: SonarCloud D Security Rating on secureExecution shell path

[cryptoxdog]

Context

Deferred during PR remediation on #52 (commit 076901f).

SonarCloud's Quality Gate reports a D Security Rating on New Code. The source is:

packages/validation-executor/src/utils/secureExecution.ts — executeWithShell():

const result = spawnSync('sh', ['-c', command], { ... });

This shell path is taken only when requiresShellExecution(command) is true (pipes, redirects, chaining, globbing, substitution). It is guarded by a denylist sanitizer (sanitizeShellCommand) that throws on rm/dd/curl/wget/nc/bash/sh after separators and in command/backtick substitution.

Why this is deferred (not fixed in the cycle)

• Shell execution is an intentional, required capability — the validation executor must run spec-defined commands that legitimately use shell features. Removing it would break documented functionality.
• SonarCloud classifies this as a Security Hotspot; resolving the rating requires either a human "review as safe" decision in the SonarQube Cloud UI, or an architectural change. Neither is a mechanical code fix, and I did not attempt to trick the scanner.

Proposed resolutions (owner decision)

1. Review the hotspot as safe in SonarQube Cloud (documenting the denylist + spawn-based design), OR
2. Harden toward an allowlist: restrict executeWithShell to a vetted set of executables/operators instead of a denylist, OR
3. Split the API: keep executeDirectly (no shell) as the default and require explicit opt-in for shell commands with a narrowed grammar.

Scope

• packages/validation-executor/src/utils/secureExecution.ts
• Related tests: packages/validation-executor/test/unit/secureExecution.test.ts

[cryptoxdog]

Triage — requires an owner decision (not a mechanical fix)

Confirmed on main: packages/validation-executor/src/utils/secureExecution.ts still routes shell-feature commands through spawnSync('sh', ['-c', command]), guarded by the denylist sanitizeShellCommand (blocks rm/dd/curl/wget/nc/bash/sh after separators and in $(...)/backticks). SonarCloud's Security-Hotspot rating stands because a denylist is inherently incomplete (e.g. python -c, perl, >/etc/... redirects, env-var indirection, unicode/whitespace evasions all pass), and the scanner can't be satisfied by a code tweak alone.

This is a genuine security-design decision, so I've not unilaterally rewritten the sanitizer. Recommendation, in order of durability:

1. Preferred — narrow the shell surface (issue's option 3). Default every call to executeDirectly (no shell). Require explicit opt-in for the few specs that truly need shell features, and for those replace the denylist with an allowlist of permitted executables + operators. This is the only option that actually raises the security rating and removes the real risk. It's a real refactor with test updates (test/unit/secureExecution.test.ts) — I can implement it on your go-ahead.
2. Interim — "Review as safe" in SonarQube Cloud. If the shell path is considered acceptable given the denylist + non-shell:true spawn, an owner marks the hotspot reviewed in the SonarQube UI (documenting the rationale). This is a required human action — it can't be done from code/CI.
3. Keep the denylist but add fail-closed structural limits (single command, no separators/substitution at all) — partial, still likely flagged.

Required human action: choose (1) or (2). Tell me which and I'll either open the allowlist refactor against main or draft the review-as-safe rationale text. Leaving open pending that decision.

---

Generated by Claude Code