sec (5/9): reject unsafe targets on the Windows host shell - #29
Conversation
6f1cade to
33a4013
Compare
82956cd to
5ecd550
Compare
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Codex review: needs maintainer review before merge. Reviewed July 25, 2026, 10:38 PM ET / July 26, 2026, 02:38 UTC. ClawSweeper reviewWhat this changesReject user-defined scanner targets containing Merge readinessKeep this PR open for maintainer review. It is a narrowly scoped security hardening for the merged user-defined-scanner path, and its member author plus the intentional Windows host compatibility change make automatic cleanup inappropriate. Priority: P1 Review scores
Verification
How this fits togetherUser-defined scanners are profile-defined commands that receive a scan target and emit raw JSON evidence. The runner resolves either a Docker/POSIX shell or the host shell, renders the target into the scanner command, then records the scanner result for the CLI output. flowchart LR
A[Profile-defined scanner] --> B[Scanner runner]
C[Scan target] --> B
B --> D{Resolved execution shell}
D -->|Docker or POSIX| E[Positional target argument]
D -->|Windows host shell| F[Reject unsafe target characters]
E --> G[Scanner command]
F --> H[Failed scanner result]
G --> I[Raw JSON evidence]
Decision needed
Why: The patch deliberately changes an existing host execution path from run to fail for certain filenames, so the security-versus-compatibility boundary needs maintainer acceptance. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the fail-closed Windows host-shell guard and have a maintainer explicitly accept the narrow compatibility trade-off, with the Docker sandbox remaining the supported escape path for affected targets. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction was established in this review because an actual Windows host-shell run was not available; the source path and focused helper cases make the affected interpolation path source-reproducible. Is this the best way to solve the issue? Yes, conditionally: rejecting untrusted target characters before AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5cfa90b129f4. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Reject target strings that cmd.exe cannot safely interpolate when Docker is disabled, preventing expansion or command injection through %, !, and quote characters.
5ecd550 to
dec077c
Compare
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: sec (5/9): reject unsafe targets on the Windows host shell This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
What changes
On the Windows host shell (
--sandbox offon Windows), a user-definedscanner target containing
%,", or!is now rejected instead of beinginterpolated into the
cmd.execommand line.Why it matters
cmd.exeexpands%VAR%even inside double quotes, expands!VAR!when delayedexpansion is on, and does not let backslash escape a
". So a target pathcontaining those characters could break out of the intended argument and inject
host commands. Since a target name/path can come from untrusted skill content,
that is a command-injection vector on Windows. Rejecting those characters closes
it. This affects only the Windows host shell path — the Docker sandbox (the
default) and POSIX hosts pass the target as a positional
"$1"argument and areunaffected.
Before / after
skill%PATH%.mdcmd.exeline — expansion/injection riskfailed, no command runplain-skill.mdThe guard is
unsafeWindowsShellTarget(target); it fires only when the resolvedshell is the Windows host shell, so Docker/POSIX behavior is untouched.
Verify
TestUnsafeWindowsShellTargetDetectsInjectionCharacterscovers%,",!(rejected) and safe targets (allowed). Cross-compiles clean:
GOOS=windows go build ./....