fix(deploy): prevent Windows shell injection

[james-elicx]

Summary

Fixes Windows command injection in vinext deploy --env by removing the command shell from Wrangler execution.

Vulnerability

On Windows, vinext resolved the package-manager .cmd shim and called:

execFileSync(wranglerBin, args, { shell: true })

With shell: true, an attacker-controlled --env value was interpreted by cmd.exe rather than remaining one argument.

Previous POC

vinext deploy --env 'preview & whoami > vinext-pwned.txt & rem'

This could execute whoami on the developer workstation or CI runner.

Impact

A malicious repository command, copied deployment command, or attacker-controlled CI parameter could execute commands with the deploy process's privileges, including access to Cloudflare credentials and other CI secrets. This affects the build/deploy plane on Windows, not deployed Worker request handling.

Fix

• Resolve Wrangler's JavaScript bin entry from its package manifest, including hoisted and Yarn Plug'n'Play resolution.
• Execute the entrypoint with process.execPath and shell: false on every platform.
• Keep --env as a discrete argument, preserving Wrangler-compatible names while rejecting null bytes and excessive lengths.
• Validate the resolved environment before any deploy-side filesystem or subprocess side effects.
• Audited the other shell: true calls in deploy.ts; they only receive framework-generated package-manager dependency arguments, not --env or other direct deploy CLI strings.

Regression coverage

• Verifies shell metacharacters remain one literal argv value.
• Runs a real subprocess reproduction and proves no injected output file is created.
• Asserts runWranglerDeploy() uses process.execPath with shell: false.
• Covers hoisted package resolution, string/object package bin forms, and a Plug'n'Play-style resolver.
• Verifies invalid environment input is rejected before project mutation.

Tests

vp test run tests/deploy.test.ts
266 tests passed

vp check packages/vinext/src/deploy.ts tests/deploy.test.ts
format, lint, and type checks passed

Review

An independent Codex review agent reviewed the change three times. Initial findings around PnP resolution, environment compatibility, validation ordering, and subprocess-level reproduction were addressed. The final review reported no actionable findings.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@1946

npm i https://pkg.pr.new/vinext@1946

commit: da865bc

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(deploy): prevent Windows shell injection

Reviewed the full diff, traced both commits, verified bin resolution against the real pnpm-installed wrangler@4.80.0, ran vp test run tests/deploy.test.ts (266/266 pass), and proved the build-env refactor is behaviorally equivalent to the prior code.

Verdict: correct, and the security goal is achieved. Executing the resolved JavaScript entrypoint via process.execPath with shell: false on all platforms eliminates the Windows cmd.exe injection vector while keeping --env a discrete argv value. I confirmed the two remaining shell: true sites (installDeps, React upgrade) only receive framework-generated package-manager args, never the attacker-controlled --env — the audit claim in the PR body holds.

Findings (minor / non-blocking)

1. Stale PR description. The body still states the fix is "rejecting null bytes and excessive lengths," but commit da865bc deliberately removed the 255-char limit. validateWranglerEnvName now only rejects null bytes. Worth updating the description so the listed behavior matches the merged code.

2. Resolution path — verified robust, just noting it. createRequire(path.join(root, 'package.json')).resolve('wrangler/package.json') fails when wrangler is not a direct dependency of the resolving package.json (reproduced from this repo's root). The _findInNodeModules fallback correctly recovers under pnpm's symlinked layout (verified by resolving from examples/hackernews), so real deploys resolve fine. The Yarn PnP case is covered only via an injected resolver in the test rather than a real PnP runtime — acceptable as a test seam, but real PnP returns a virtual package.json path that fs.readFileSync may not be able to read. Not blocking, but a candidate for a follow-up if PnP support is a hard requirement.

Confirmed equivalences

• Build-env logic (deployEnv === 'production' && !options.env ? undefined : deployEnv) matches the old options.env || (options.preview ? 'preview' : undefined) across all cases including env: 'production' and env: ''. Build and deploy env stay in sync.
• Dropping preview from the runWranglerDeploy call is safe: preview is only consulted by buildWranglerDeployArgs when env is undefined, and the new call already resolves preview→'preview' into env.
• Env validation runs at the top of deploy() before any filesystem/subprocess side effects, matching the regression test.

LGTM.

[ask-bonk]

Review posted to PR #1946.

Conclusion

The fix is correct and achieves its security goal. Switching to process.execPath + shell: false on all platforms removes the Windows cmd.exe injection vector while keeping --env a single literal argv value. I verified:

• 266/266 deploy tests pass
• Wrangler bin resolution works against the real pnpm-installed wrangler@4.80.0 (fallback recovers when createRequire resolution fails)
• The two remaining shell: true sites never receive --env — only framework-generated args
• Build-env logic is exactly equivalent to the old code across all edge cases
• Env validation runs before any side effects

Two minor, non-blocking findings posted: (1) the PR description's "excessive lengths" claim is stale (the length limit was removed in da865bc), and (2) the Yarn PnP path is covered only via an injected test resolver, not a real PnP runtime where fs.readFileSync on a virtual package.json could fail — a possible follow-up if PnP is a hard requirement.

github run