← Back to PRs

#18143: fix(windows): wrap shell builtins with cmd.exe /c for proper execution

by brandonwise open 2026-02-16 15:05 View on GitHub →
size: S trusted-contributor
Fixes #17806 ## What Wrap Windows shell builtins (echo, dir, type, etc.) with `cmd.exe /c` when spawning processes, so they execute properly without requiring the unsafe `shell: true` flag. ## Why The security hardening in v2026.2.15 (commit a7eb0dd9a) disabled `shell: true` to prevent command injection on Windows. However, this broke Windows shell builtins which are implemented by cmd.exe, not as separate executables. **Before (broken):** `echo hello` → Node.js looks for `echo.exe` → not found → empty output **After (fixed):** `echo hello` → wrapped to `cmd.exe /c echo hello` → works correctly ## How Instead of re-enabling the unsafe `shell: true` flag, this fix: 1. Defines a set of 50+ known Windows shell builtins 2. Detects when a command is a builtin via `isWindowsShellBuiltin()` 3. Wraps only those commands with `cmd.exe /c` in the argv array via `wrapWindowsBuiltinCommand()` 4. Preserves security by NOT enabling `shell: true` globally This is exactly the approach recommended in the original security comment: > "If you need a shell, use an explicit shell-wrapper argv (e.g. `cmd.exe /c ...`)" ## Testing - [x] Added 10 unit tests for `wrapWindowsBuiltinCommand()` - [x] All 15 tests in `exec.test.ts` pass - [x] `oxlint`: 0 warnings, 0 errors - [x] `pnpm build` succeeds ## AI-Assisted 🤖 This PR was built with AI assistance (Claude via OpenClaw). - [x] Code reviewed and understood - [x] Tested via unit tests (Windows execution requires manual verification) - [x] Session available on request <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR fixes Windows shell builtins (echo, dir, type, etc.) that broke after security hardening disabled `shell: true` in v2026.2.15. Instead of re-enabling the unsafe `shell: true` flag, it wraps known builtins with `cmd.exe /c` in the argv array — matching the recommended approach from the original security comment. - Defines a set of 50+ known Windows shell builtins and wraps only those commands with `cmd.exe /c` when spawning on win32 - Preserves security by keeping `shell: true` disabled globally - Only applies wrapping in `runCommandWithTimeout`; `runExec` is unaffected (its callers use standalone executables, not builtins) - The interaction with `resolveCommand` is correct — `cmd.exe` already has a `.exe` extension and passes through unchanged - 10 well-structured unit tests cover platform gating, case-insensitive matching, and passthrough of non-builtins <h3>Confidence Score: 4/5</h3> - This PR is safe to merge — it follows the security-recommended pattern and only touches the Windows code path with known builtins. - Score of 4 reflects: correct security approach (wrapping known builtins instead of re-enabling shell: true), proper interaction with existing resolveCommand function, good test coverage, and no functional regressions for non-Windows platforms. Prior thread items (missing /d flag, comment wording) have already been addressed. The one remaining concern — the absence of the /d flag — was raised in a prior thread and is not a blocking issue but a hardening opportunity. - No files require special attention beyond the prior review feedback on `/d` flag hardening in `src/process/exec.ts`. <sub>Last reviewed commit: 0a73f42</sub> <!-- greptile_other_comments_section --> <!-- /greptile_comment -->

Most Similar PRs