Skip to content

fix: harden runWrangler against shell injection and silence DEP0190 #1248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kirkouimet wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: harden runWrangler against shell injection and silence DEP0190 #1248

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
726e3ef
fix: harden runWrangler against shell injection and silence DEP0190
kirkouimet May 4, 2026
5df6c61
fix: split multi-word args at every runWrangler call site
kirkouimet May 4, 2026
227371e
fix: address review feedback on runWrangler shell injection PR
kirkouimet May 5, 2026
9f39728
fix: address review feedback round 2 from @vicb and @james-elicx
kirkouimet May 5, 2026
382e5d1
fix: silence DEP0190 on Windows too via cmd.exe /c invocation
kirkouimet May 5, 2026
229c45f
fix: correct quoteShellMeta Windows escaping algorithm
kirkouimet May 5, 2026
5a5a768
docs: address review feedback on comments
kirkouimet May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .changeset/fix-runwrangler-shell-injection.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
"@opennextjs/cloudflare": patch
---

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.

@james-elicx james-elicx May 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it still left open on windows?


Closes [#1182](https://github.com/opennextjs/opennextjs-cloudflare/issues/1182).

@vicb vicb May 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Suggested change
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.

4 changes: 2 additions & 2 deletions packages/cloudflare/src/cli/commands/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type yargs from "yargs";
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";

@vicb vicb May 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is quoteShellMeta still use anywhere? If not, we can remove that

import { getEnvFromPlatformProxy } from "./utils/helpers.js";
import { runWrangler } from "./utils/run-wrangler.js";
import type { WithWranglerArgs } from "./utils/utils.js";
import {
Expand Down Expand Up @@ -58,7 +58,7 @@ export async function deployCommand(args: WithWranglerArgs<{ cacheChunkSize?: nu
"deploy",
...args.wranglerArgs,
...(deploymentMapping
? [`--var ${DEPLOYMENT_MAPPING_ENV_NAME}:${quoteShellMeta(JSON.stringify(deploymentMapping))}`]
? ["--var", `${DEPLOYMENT_MAPPING_ENV_NAME}:${JSON.stringify(deploymentMapping)}`]
: []),
],
{
Expand Down
32 changes: 21 additions & 11 deletions packages/cloudflare/src/cli/commands/populate-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import {
import { ensureR2Bucket } from "../utils/ensure-r2-bucket.js";
import { normalizePath } from "../utils/normalize-path.js";
import type { R2Response } from "../workers/r2-cache-types.js";
import { getEnvFromPlatformProxy, quoteShellMeta, type WorkerEnvVar } from "./utils/helpers.js";
import { getEnvFromPlatformProxy, type WorkerEnvVar } from "./utils/helpers.js";
import type { WranglerTarget } from "./utils/run-wrangler.js";
import { runWrangler } from "./utils/run-wrangler.js";
import type { WithWranglerArgs } from "./utils/utils.js";
Expand Down Expand Up @@ -530,10 +530,14 @@ async function populateKVIncrementalCache(
const result = runWrangler(
buildOpts,
[
"kv bulk put",
quoteShellMeta(chunkPath),
`--binding ${KV_CACHE_BINDING_NAME}`,
`--preview ${populateCacheOptions.shouldUsePreviewId}`,
"kv",
"bulk",
"put",
chunkPath,
"--binding",
KV_CACHE_BINDING_NAME,
"--preview",
String(populateCacheOptions.shouldUsePreviewId),
],
{
target: populateCacheOptions.target,
Expand Down Expand Up @@ -570,15 +574,18 @@ function populateD1TagCache(
const result = runWrangler(
buildOpts,
[
"d1 execute",
"d1",
"execute",
D1_TAG_BINDING_NAME,
// Columns:
// tag - The cache tag.
// revalidatedAt - Timestamp (ms) when the tag was last revalidated.
// stale - Timestamp (ms) when the cached entry becomes stale. Added in v1.19.
// expire - Timestamp (ms) when the cached entry expires. NULL means no expire. Added in v1.19.
`--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);",

@james-elicx james-elicx May 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the sql statements might still need to be quoted for windows?

"--preview",
String(populateCacheOptions.shouldUsePreviewId),

@vicb vicb May 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

Suggested change
"--preview",
String(populateCacheOptions.shouldUsePreviewId),
populateCacheOptions.shouldUsePreviewId ? "--preview" : "--no-preview",

ref: https://yargs.js.org/docs/#api-reference-booleankey

],
{
target: populateCacheOptions.target,
Expand All @@ -598,10 +605,13 @@ function populateD1TagCache(
runWrangler(
buildOpts,
[
"d1 execute",
"d1",
"execute",
D1_TAG_BINDING_NAME,
`--command "ALTER TABLE revalidations ADD COLUMN stale INTEGER; ALTER TABLE revalidations ADD COLUMN expire INTEGER default NULL"`,
`--preview ${populateCacheOptions.shouldUsePreviewId}`,
"--command",
"ALTER TABLE revalidations ADD COLUMN stale INTEGER; ALTER TABLE revalidations ADD COLUMN expire INTEGER default NULL",
"--preview",
String(populateCacheOptions.shouldUsePreviewId),

@vicb vicb May 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

],
{
target: populateCacheOptions.target,
Expand Down
7 changes: 4 additions & 3 deletions packages/cloudflare/src/cli/commands/upload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type yargs from "yargs";
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";
import { getEnvFromPlatformProxy } from "./utils/helpers.js";
import { runWrangler } from "./utils/run-wrangler.js";
import type { WithWranglerArgs } from "./utils/utils.js";
import {
Expand Down Expand Up @@ -55,10 +55,11 @@ export async function uploadCommand(args: WithWranglerArgs<{ cacheChunkSize?: nu
const result = runWrangler(
buildOpts,
[
"versions upload",
"versions",
"upload",
...args.wranglerArgs,
...(deploymentMapping
? [`--var ${DEPLOYMENT_MAPPING_ENV_NAME}:${quoteShellMeta(JSON.stringify(deploymentMapping))}`]
? ["--var", `${DEPLOYMENT_MAPPING_ENV_NAME}:${JSON.stringify(deploymentMapping)}`]
: []),
],
{ logging: "all" }
Expand Down
25 changes: 14 additions & 11 deletions packages/cloudflare/src/cli/commands/utils/run-wrangler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,22 @@ export function runWrangler(
[
options.packager === "bun" ? "x" : "exec",
"wrangler",
...injectPassthroughFlagForArgs(
options,
[
...args,
wranglerOpts.environment && `--env ${wranglerOpts.environment}`,
wranglerOpts.configPath && `--config ${wranglerOpts.configPath}`,
wranglerOpts.target === "remote" && "--remote",
wranglerOpts.target === "local" && "--local",
].filter((v): v is string => !!v)
),
...injectPassthroughFlagForArgs(options, [
...args,
...(wranglerOpts.environment ? ["--env", wranglerOpts.environment] : []),
...(wranglerOpts.configPath ? ["--config", wranglerOpts.configPath] : []),
...(wranglerOpts.target === "remote" ? ["--remote"] : []),
...(wranglerOpts.target === "local" ? ["--local"] : []),
]),
],
{
shell: true,
// 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",
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Outdated

@vicb vicb May 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we still use shell: true now that args are separated?

@vicb vicb May 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Always pipe stderr so that we can capture it for inspection.
// Keep stdin and stdout as "inherit" when not piping logs to maintain TTY detection
// (wrangler checks `process.stdin.isTTY && process.stdout.isTTY` for interactive mode).
Expand Down
Loading