fix: add shell: true on Windows for execFileSync calls#742
fix: add shell: true on Windows for execFileSync calls#742evil1morty wants to merge 2 commits intocloudflare:mainfrom
Conversation
On Windows, `execFileSync` cannot execute `.bin/wrangler` (a shell script) or package manager commands like `npm`/`npx` (which are `.cmd` wrappers) without enabling the shell. This causes `ENOENT` errors during `vinext deploy`, `vinext build`, and `vinext lint`. Fix by adding `shell: process.platform === "win32"` to all `execFileSync` options. This only enables the shell on Windows where it's required, preserving the no-shell-injection guarantee on other platforms. Fixes `spawnSync ENOENT` on Windows for: - `runWranglerDeploy` (deploy.ts) - `installDeps` (deploy.ts) - React upgrade in `deploy()` (deploy.ts) - React upgrade in `buildApp()` (cli.ts) - Lint commands in `lint()` (cli.ts)
|
/bigbonk review |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix. The ENOENT on Windows for .cmd wrappers and shell scripts in .bin/ is a well-known Node.js footgun, and the shell: process.platform === "win32" pattern is the standard fix.
Verified:
- All 7
execFileSynccall sites incli.tsanddeploy.tsare covered - Arguments are still passed as arrays (not interpolated into a string), so shell injection risk on Windows is minimal — the shell is only used for
.cmdshim resolution, not for building command strings init.tsalready usesshell: trueunconditionally (line 262), so this PR is actually more conservative by limiting it to win32- No type issues —
ExecFileSyncOptions.shellacceptsboolean | string
One non-blocking nit: the existing comment in runWranglerDeploy at line 1228 says "Use execFileSync to avoid shell injection — args are passed as an array, never interpolated into a shell command string." — this is slightly misleading now that shell: true is enabled on Windows. The new comment at lines 1214-1216 explains the tradeoff well, but the original comment at 1228 could be updated to say something like: "execFileSync passes args as an array, avoiding shell injection on Unix. On Windows, shell: true is required for .cmd wrappers but the array form still prevents trivial injection."
Not a blocker — LGTM.
| // On Windows, .bin/wrangler is a .cmd wrapper; execFileSync can't run | ||
| // it without a shell. Enabling shell only on win32 keeps the | ||
| // no-shell-injection guarantee on other platforms. | ||
| shell: process.platform === "win32", |
There was a problem hiding this comment.
Nice — the comment here is clear and well-scoped. Just noting that the other comment a few lines below (line 1228: "Use execFileSync to avoid shell injection") is now slightly inaccurate on Windows. Consider updating that one too to acknowledge the platform split, since both comments describe the same call site.
|
Review submitted successfully. Here's a summary of what I reviewed: PR #742: fix: add shell: true on Windows for execFileSync calls -- Approved The fix is correct and well-scoped. On Windows, Key observations:
|
2 participants
Summary
vinext deploy,vinext build, andvinext lintfail on Windows withspawnSync ENOENTbecauseexecFileSynccannot execute.bin/wrangler(a shell script) or package manager commands likenpm/npx(.cmdwrappers) without enabling the shell.Fix
Add
shell: process.platform === "win32"to allexecFileSyncoptions indeploy.tsandcli.ts. This only enables the shell on Windows where it's required, preserving the no-shell-injection guarantee on other platforms.Affected call sites:
runWranglerDeploy()indeploy.ts— wrangler binaryinstallDeps()indeploy.ts— package managerdeploy()— package managerbuildApp()incli.ts— package managerlint()incli.ts— npx eslint/oxlintTest plan
npx wrangler deployworks as workaround on Windows 11vinext deployon Windows with this patchvinext buildon Windows (react upgrade path)vinext linton Windowsfalse)Environment: Windows 11, Node 22, wrangler 4.79.0