fix: harden runWrangler against shell injection and silence DEP0190#1248
fix: harden runWrangler against shell injection and silence DEP0190#1248kirkouimet wants to merge 7 commits into
Conversation
runWrangler called spawnSync with shell:true while interpolating flag values into single-string array entries. With shell:true, Node.js joins the args into a shell command string without escaping, allowing values containing shell metacharacters to break out of their argument boundary. Node.js 22 added DEP0190 specifically to flag this pattern. Each flag/value pair is now passed as separate array entries, and shell is set to false on POSIX. shell:true is retained on Windows so the package manager's .cmd shim resolves via cmd.exe, but values are no longer interpolated, closing the injection vector on every platform. Closes opennextjs#1182. Verified locally: - pnpm code:checks (prettier, eslint, tsc) passes - pnpm -r test (32 files, 326 tests) passes - End-to-end: spawnSync('pnpm', ['exec', 'wrangler', '--version']) on Node 24.14.1 returns successfully with no DEP0190 emitted - Security: malicious value 'staging$(echo INJECTED > /tmp/pwned.txt)' no longer executes the injected subshell (writes literal string to args, file is not created)
🦋 Changeset detectedLatest commit: 5a5a768 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
With shell:false on POSIX, the OS no longer word-splits combined strings like "kv bulk put" or "--binding NAME" into separate process arguments. Update every caller of runWrangler to pass each token as a distinct array entry so wrangler receives them as separate argv elements: - populate-cache.ts: "kv bulk put" -> "kv","bulk","put"; --binding/--preview flag-value pairs split; "d1 execute" -> "d1","execute"; --command/--preview pairs split (drops the now-stray inner quotes around the SQL string). - deploy.ts, upload.ts: "versions upload" -> "versions","upload"; --var pair split; quoteShellMeta wrapper dropped (shell:false means args are passed verbatim, so shell escaping would inject literal characters). - Drop quoteShellMeta imports from the three call sites. Caught by Devin Review on the original PR.
|
Great catch from Devin Review — that was a real bug. Pushed 5df6c61 splitting every multi-word arg and
Re-verified locally: |
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I have added some inline comments.
@james-elicx has done a lot of work on the CLI, I'd love to get some insights from him too
| // Each flag value is now a separate array entry, so `shell: true` is no longer | ||
| // needed for value interpolation and would only re-introduce the injection vector | ||
| // flagged by Node.js DEP0190 (and the related security advisory). `shell: true` is | ||
| // still required on Windows so that the package manager's `.cmd` shim is resolved | ||
| // via cmd.exe; with each arg as a separate entry, Node escapes them correctly even | ||
| // when the shell is involved. | ||
| shell: process.platform === "win32", |
There was a problem hiding this comment.
Could we still use shell: true now that args are separated?
There was a problem hiding this comment.
Oh now I understand this would trigger DEP0190.
Could we please reword the comment to something like:
Setting shell to false to avoid DEP0190
See https://nodejs.org/api/deprecations.html#dep0190-passing-args-to-nodechild-process-execfilespawn-with-shell-option
Note that shell: true is required on Windows so that the package manager's .cmd shim is resolved
| import { DEPLOYMENT_MAPPING_ENV_NAME } from "../templates/skew-protection.js"; | ||
| import { populateCache, withPopulateCacheOptions } from "./populate-cache.js"; | ||
| import { getDeploymentMapping } from "./skew-protection.js"; | ||
| import { getEnvFromPlatformProxy, quoteShellMeta } from "./utils/helpers.js"; |
There was a problem hiding this comment.
is quoteShellMeta still use anywhere? If not, we can remove that
| "--preview", | ||
| String(populateCacheOptions.shouldUsePreviewId), |
There was a problem hiding this comment.
What about:
| "--preview", | |
| String(populateCacheOptions.shouldUsePreviewId), | |
| populateCacheOptions.shouldUsePreviewId ? "--preview" : "--no-preview", |
| "--preview", | ||
| String(populateCacheOptions.shouldUsePreviewId), |
| fix: harden `runWrangler` against shell injection and silence Node.js DEP0190. | ||
|
|
||
| `runWrangler` previously called `spawnSync` with `shell: true` while interpolating | ||
| flag values into single-string array entries (e.g. `` `--env ${wranglerOpts.environment}` ``). | ||
| With `shell: true`, Node.js joins the arguments into a shell command string without | ||
| escaping, so a value containing shell metacharacters (`;`, `&&`, `$(...)`, backticks, | ||
| etc.) could break out of its intended argument boundary. Node.js 22 added `DEP0190` | ||
| specifically to flag this pattern, and that warning surfaced on every `opennextjs-cloudflare deploy` | ||
| invoked through `wrangler-action` on Node 22+. | ||
|
|
||
| Each flag/value pair is now passed as separate array entries, and `shell` is set to | ||
| `false` on POSIX (where the package manager binary can be invoked directly). `shell: true` | ||
| is retained on Windows so the package manager's `.cmd` shim still resolves via `cmd.exe`, | ||
| but values are no longer interpolated into the shell command, so the injection vector | ||
| is closed on every platform. | ||
|
|
||
| Closes [#1182](https://github.com/opennextjs/opennextjs-cloudflare/issues/1182). |
There was a problem hiding this comment.
This is used for the changelog.
I don't think there is a need to give details as the changes in this PR will not affect users (the behavior stays the same).
As long as the changes are described in the PR, we should be good.
Thanks!
| fix: harden `runWrangler` against shell injection and silence Node.js DEP0190. | |
| `runWrangler` previously called `spawnSync` with `shell: true` while interpolating | |
| flag values into single-string array entries (e.g. `` `--env ${wranglerOpts.environment}` ``). | |
| With `shell: true`, Node.js joins the arguments into a shell command string without | |
| escaping, so a value containing shell metacharacters (`;`, `&&`, `$(...)`, backticks, | |
| etc.) could break out of its intended argument boundary. Node.js 22 added `DEP0190` | |
| specifically to flag this pattern, and that warning surfaced on every `opennextjs-cloudflare deploy` | |
| invoked through `wrangler-action` on Node 22+. | |
| Each flag/value pair is now passed as separate array entries, and `shell` is set to | |
| `false` on POSIX (where the package manager binary can be invoked directly). `shell: true` | |
| is retained on Windows so the package manager's `.cmd` shim still resolves via `cmd.exe`, | |
| but values are no longer interpolated into the shell command, so the injection vector | |
| is closed on every platform. | |
| Closes [#1182](https://github.com/opennextjs/opennextjs-cloudflare/issues/1182). | |
| fix: harden `runWrangler` against shell injection and silence Node.js DEP0190. |
commit: |
- populate-cache.ts: switch all three --preview call sites from
--preview ${bool} to --preview / --no-preview. Idiomatic yargs
boolean negation pattern, suggested by @vicb.
- helpers.ts: remove now-unused quoteShellMeta function. With
shell:false on POSIX, shell-escaping injects literal characters
into argument values, so the function has no remaining call sites.
- populate-cache.spec.ts: drop the corresponding mock entry.
|
Thanks for the review! Pushed 227371e addressing all four points. Re-verified locally:
|
|
@kirkouimet thank you so much! shell: true with separated argsIIUC 2 says Maybe also linked to your last point? Probably a point to discuss more before merging this PR quoteShellMeta👍 --preview / --no-previewConsistency is the best, thanks! |
| if (process.platform === "win32") { | ||
| if (arg.length === 0) { | ||
| return '""'; | ||
| } | ||
| const needsEscaping = /[&|<>^()%!"]/; | ||
| const needsQuotes = /\s/.test(arg) || needsEscaping.test(arg); | ||
| let escaped = arg.replace(/"/g, '""'); | ||
| if (/[&|<>^()%!]/.test(arg)) { | ||
| escaped = escaped.replace(/[&|<>^()%!]/g, "^$&"); | ||
| } | ||
| return needsQuotes ? `"${escaped}"` : escaped; | ||
| } |
There was a problem hiding this comment.
we'll likely want to keep this given that it still uses shell: true on win32
| `--command "CREATE TABLE IF NOT EXISTS revalidations (tag TEXT NOT NULL, revalidatedAt INTEGER NOT NULL, stale INTEGER, expire INTEGER default NULL, UNIQUE(tag) ON CONFLICT REPLACE);"`, | ||
| `--preview ${populateCacheOptions.shouldUsePreviewId}`, | ||
| "--command", | ||
| "CREATE TABLE IF NOT EXISTS revalidations (tag TEXT NOT NULL, revalidatedAt INTEGER NOT NULL, stale INTEGER, expire INTEGER default NULL, UNIQUE(tag) ON CONFLICT REPLACE);", |
There was a problem hiding this comment.
i think the sql statements might still need to be quoted for windows?
| but values are no longer interpolated into the shell command, so the injection vector | ||
| is closed on every platform. |
There was a problem hiding this comment.
isn't it still left open on windows?
Per @james-elicx: restore quoteShellMeta and use it at the call sites where values can contain shell metacharacters or whitespace. Without it, Windows users would have regressed since runWrangler still uses shell: true on Windows for .cmd shim resolution, and cmd.exe would tokenize unquoted SQL --command values and unquoted --var JSON values. - helpers.ts: restore quoteShellMeta with updated docstring noting it should only be applied when process.platform === "win32". - populate-cache.ts: quote chunkPath, CREATE TABLE SQL, and ALTER TABLE SQL on Windows. - deploy.ts, upload.ts: quote the --var JSON value on Windows. - populate-cache.spec.ts: restore the corresponding mock entry. Per @vicb: reword the shell:false comment in run-wrangler.ts to his suggested text linking to the DEP0190 docs, and trim the changeset description to a one-liner since the change is internal.
Per Node.js docs (https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows), the recommended way to invoke a .cmd/.bat shim is to spawn cmd.exe directly with /c rather than relying on shell:true. This silences DEP0190 on Windows while keeping the .cmd shim resolution working. The Windows path now mirrors how cross-spawn handles non-.exe commands: - Pre-escape every token with quoteShellMeta - Join into a single quoted command line - spawnSync('cmd.exe', ['/d', '/s', '/c', '"<cmd line>"'], { shell: false, windowsVerbatimArguments: true }) Encapsulating this inside runWrangler means call sites no longer need to know about the Windows quirk: populate-cache.ts, deploy.ts, and upload.ts revert to passing plain string arrays without conditional quoting. Note: I cannot verify the Windows path on real Windows hardware (no Windows CI in this repo). The escaping output has been verified by simulation, and the pattern matches cross-spawn's well-tested approach, but please validate on Windows before merging.
|
Took another swing at this. Pushed 382e5d1 which silences DEP0190 on Windows too, plus 9f39728 addressing the rest of the review feedback. End result: DEP0190 is silenced on every platform, the injection vector is closed everywhere, and call sites don't need to know about Windows. Windows DEP0190 (the asymmetry @vicb flagged)Per Node.js docs (Spawning .bat and .cmd files on Windows), the recommended pattern is to spawn spawnSync("cmd.exe", ["/d", "/s", "/c", `"${escapedCommandLine}"`], {
shell: false,
windowsVerbatimArguments: true,
...
});Where POSIX path is unchanged from the previous round:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The previous implementation caret-escaped cmd.exe metacharacters AND wrapped the value in double quotes, producing strings like "CREATE TABLE foo ^(x INT^);". cmd.exe treats ^ as literal inside double quotes, so those carets corrupted argument values. Caught by Devin Review. Replace with cross-spawn's escapeArgument algorithm, which: 1. Doubles backslash sequences that precede a quote (Windows argv parser quirk). 2. Wraps the value in "...". 3. Caret-escapes every metacharacter, INCLUDING the wrapping quotes. The carets are needed for the outer cmd /d /s /c "..." parse, which consumes them and reveals normally-quoted args to the receiving program. Verified output matches cross-spawn byte-for-byte across 8 representative inputs (SQL with parens, JSON with quotes, paths with spaces, empty strings, etc.). Add helpers.spec.ts with regression tests for the SQL case and coverage for both Windows and POSIX branches.
|
Excellent catch from Devin Review. The bug
The fixPushed 229c45f replacing the Windows branch with
The wrapping carets pair with the outer Verification
Caveat from earlier still applies: the new Windows escaping is verified by simulation (output identical to cross-spawn's, which IS production-tested on Windows), but the actual |
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
The latest version looks much simpler hence better. Thanks for the updates.
We need to test on:
- WSL
- Windows
Before merging
|
|
||
| /** | ||
| * Escape shell metacharacters. | ||
| * Escape a single argument so it survives the `cmd.exe /d /s /c "<command line>"` invocation |
There was a problem hiding this comment.
The implementation seems to works for POSIX and windows, could you please update the comment to reflect that
| if (arg.length === 0) { | ||
| return '""'; | ||
| if (process.platform !== "win32") { | ||
| // POSIX fallback (preserved for any external callers): standard shell quoting. |
There was a problem hiding this comment.
| // POSIX fallback (preserved for any external callers): standard shell quoting. | |
| // POSIX shell quoting. |
| // Setting `shell` to `false` to avoid DEP0190. | ||
| // See https://nodejs.org/api/deprecations.html#dep0190-passing-args-to-nodechild-process-execfilespawn-with-shell-option |
There was a problem hiding this comment.
Move this up as it is used for both win and non win ands merge with l135/136
| ]), | ||
| ]; | ||
|
|
||
| const stdio: ("inherit" | "ignore" | "pipe")[] = |
There was a problem hiding this comment.
Could you please re-add the former comment on this
Per @vicb's review: - helpers.ts: rewrite quoteShellMeta docstring so it describes both the Windows and POSIX branches with equal weight, rather than treating POSIX as a footnote. The function works on both platforms. - helpers.ts: simplify the inner POSIX comment to "POSIX shell quoting" per the suggestion. - run-wrangler.ts: hoist the shell:false / DEP0190 explanation to the top-level comment block so it covers both branches, then describe the POSIX path and the Windows path in turn. Drop the now-redundant inner comment in the POSIX spawn call. - run-wrangler.ts: restore the stdio explanatory comment that was lost when stdio was hoisted to a const in an earlier refactor. No behavioral changes.
|
Pushed 5a5a768 addressing the four inline comments. All comment-only changes, no behavioral diff:
|
3 participants
Summary
Closes #1182.
runWranglerinpackages/cloudflare/src/cli/commands/utils/run-wrangler.tspreviously calledspawnSyncwithshell: truewhile interpolating flag values into single-string array entries:With
shell: true, Node.js joins the args array into a shell command string without escaping, so a value containing shell metacharacters (;,&&,$(...), backticks, etc.) can break out of its intended argument boundary and execute arbitrary commands. Node.js 22 addedDEP0190specifically to flag this pattern, and that warning surfaces on everyopennextjs-cloudflare deployinvoked throughwrangler-actionon Node 22+:Fix
["--env", value],["--config", value], etc.) so values are never interpolated into a shell string.shellis set tofalseon POSIX, where the package manager binary can be invoked directly. This silencesDEP0190and removes the injection vector entirely.shell: trueis retained on Windows so the package manager's.cmdshim still resolves viacmd.exe. With each value as a separate array entry, Node escapes them correctly even when the shell is involved, so the injection vector is closed on every platform.Test plan
pnpm code:checks(prettier + eslint + tsc) passespnpm -r test— 32 files, 326 tests, all greenrunWranglerto callwrangler --versionagainst a real project on Node 24.14.1 returns4.87.0with noDEP0190emitted (previously emitted)staging$(echo INJECTED > /tmp/pwned.txt)no longer executes the injected subshell — the string is passed verbatim as an argument and the file is never created (with the old code, the file IS created)