Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/fix-runwrangler-shell-injection.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@opennextjs/cloudflare": patch
---

fix: harden `runWrangler` against shell injection and silence Node.js DEP0190.
4 changes: 2 additions & 2 deletions packages/cloudflare/src/cli/commands/deploy.ts
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 @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ vi.mock("./utils/run-wrangler.js", () => ({

vi.mock("./utils/helpers.js", () => ({
getEnvFromPlatformProxy: vi.fn(async () => ({})),
quoteShellMeta: vi.fn((s) => s),
}));

vi.mock("../utils/ensure-r2-bucket.js");
Expand Down
29 changes: 18 additions & 11 deletions packages/cloudflare/src/cli/commands/populate-cache.ts
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,13 @@ 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,
populateCacheOptions.shouldUsePreviewId ? "--preview" : "--no-preview",
],
{
target: populateCacheOptions.target,
Expand Down Expand Up @@ -570,15 +573,17 @@ 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);",
populateCacheOptions.shouldUsePreviewId ? "--preview" : "--no-preview",
],
{
target: populateCacheOptions.target,
Expand All @@ -598,10 +603,12 @@ 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",
populateCacheOptions.shouldUsePreviewId ? "--preview" : "--no-preview",
],
{
target: populateCacheOptions.target,
Expand Down
7 changes: 4 additions & 3 deletions packages/cloudflare/src/cli/commands/upload.ts
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
83 changes: 83 additions & 0 deletions packages/cloudflare/src/cli/commands/utils/helpers.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { afterEach, beforeEach, describe, expect, it } from "vitest";

import { quoteShellMeta } from "./helpers.js";

describe("quoteShellMeta", () => {
describe("on Windows", () => {
let originalPlatform: PropertyDescriptor | undefined;

beforeEach(() => {
originalPlatform = Object.getOwnPropertyDescriptor(process, "platform");
Object.defineProperty(process, "platform", { value: "win32" });
});

afterEach(() => {
if (originalPlatform) {
Object.defineProperty(process, "platform", originalPlatform);
}
});

it("escapes empty strings", () => {
expect(quoteShellMeta("")).toBe('^"^"');
});

it("escapes simple alphanumeric values", () => {
expect(quoteShellMeta("simple-value")).toBe('^"simple-value^"');
});

it("does not leave bare carets in front of metacharacters inside the wrapping quotes", () => {
// Regression: an earlier implementation produced `"...^(...^)..."` which made the
// carets literal characters inside cmd.exe quotes, corrupting SQL with parens.
// The correct algorithm caret-escapes the wrapping quotes themselves, so the inner
// metacharacters stay un-escaped.
const sql = "CREATE TABLE foo (x INT);";
const escaped = quoteShellMeta(sql);
expect(escaped).toBe('^"CREATE^ TABLE^ foo^ ^(x^ INT^)^;^"');
// No bare `^(` or `^)` sitting inside an un-caret-escaped `"..."`.
expect(escaped).not.toMatch(/(?<!\^)"[^"]*\^[()][^"]*(?<!\^)"/);
});

it("escapes paths containing spaces", () => {
expect(quoteShellMeta("C:\\Users\\Some User\\chunk.json")).toBe(
'^"C:\\Users\\Some^ User\\chunk.json^"'
);
});

it("escapes JSON values containing quotes and braces", () => {
expect(quoteShellMeta('KEY:{"foo":"bar baz"}')).toBe('^"KEY:{\\^"foo\\^":\\^"bar^ baz\\^"}^"');
});

it("doubles backslashes that precede a quote", () => {
// A backslash immediately before a quote needs doubling so the receiving program's
// argument parser sees a single literal backslash followed by an escaped quote.
expect(quoteShellMeta('\\"')).toBe('^"\\\\\\^"^"');
});
});

describe("on POSIX", () => {
let originalPlatform: PropertyDescriptor | undefined;

beforeEach(() => {
originalPlatform = Object.getOwnPropertyDescriptor(process, "platform");
Object.defineProperty(process, "platform", { value: "linux" });
});

afterEach(() => {
if (originalPlatform) {
Object.defineProperty(process, "platform", originalPlatform);
}
});

it("single-quotes values containing whitespace", () => {
expect(quoteShellMeta("hello world")).toBe("'hello world'");
});

it("double-quotes values containing both single and double quotes", () => {
expect(quoteShellMeta(`it's "quoted"`)).toBe(`"it's \\"quoted\\""`);
});

it("backslash-escapes shell metacharacters in bare values", () => {
expect(quoteShellMeta("a;b")).toBe("a\\;b");
});
});
});
58 changes: 39 additions & 19 deletions packages/cloudflare/src/cli/commands/utils/helpers.ts
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -73,33 +73,53 @@ export async function getEnvFromPlatformProxy(options: GetPlatformProxyOptions,
}

/**
* Escape shell metacharacters.
* Escape a single argument for shell-style invocation. Cross-platform.
*
* When `spawnSync` is invoked with `shell: true`, metacharacters need to be escaped.
* On Windows, `runWrangler` invokes `cmd.exe /d /s /c "<command line>"` with
* `windowsVerbatimArguments: true`. The algorithm matches `cross-spawn`'s `escapeArgument`:
* 1. Wrap the value in `"..."`, doubling backslash sequences that precede a quote per the
* Windows command-line argument rules.
* 2. Caret-escape every cmd.exe metacharacter, including the wrapping quotes themselves.
* The wrapping carets are needed for the OUTER `cmd /c "..."` parse: cmd.exe consumes them
* and the receiving program sees a normally-quoted argument. Crucially, `^` must NOT be
* applied inside an un-caret-escaped quoted region, because cmd.exe treats `^` as literal
* inside double quotes (this was the bug an earlier shell-quote-style implementation hit).
*
* Based on https://github.com/ljharb/shell-quote/blob/main/quote.js
* On POSIX, the value is quoted using standard POSIX shell rules (single quotes by default,
* double quotes when the value contains a single quote, backslash-escapes for bare values).
* `runWrangler` itself does not invoke a shell on POSIX, but this branch is preserved so the
* function behaves usefully for any direct caller.
*
* References: https://github.com/moxystudio/node-cross-spawn/blob/master/lib/util/escape.js
* https://qntm.org/cmd
*
* @param arg
* @returns escaped arg
*/
export function quoteShellMeta(arg: string) {
if (process.platform === "win32") {
if (arg.length === 0) {
return '""';
if (process.platform !== "win32") {
// POSIX shell quoting.
if (/["\s]/.test(arg) && !/'/.test(arg)) {
return `'${arg.replace(/(['\\])/g, "\\$1")}'`;
}
const needsEscaping = /[&|<>^()%!"]/;
const needsQuotes = /\s/.test(arg) || needsEscaping.test(arg);
let escaped = arg.replace(/"/g, '""');
if (/[&|<>^()%!]/.test(arg)) {
escaped = escaped.replace(/[&|<>^()%!]/g, "^$&");
if (/["'\s]/.test(arg)) {
return `"${arg.replace(/(["\\$`!])/g, "\\$1")}"`;
}
return needsQuotes ? `"${escaped}"` : escaped;
}
if (/["\s]/.test(arg) && !/'/.test(arg)) {
return `'${arg.replace(/(['\\])/g, "\\$1")}'`;
}
if (/["'\s]/.test(arg)) {
return `"${arg.replace(/(["\\$`!])/g, "\\$1")}"`;
return arg.replace(/([A-Za-z]:)?([#!"$&'()*,:;<=>?@[\\\]^`{|}])/g, "$1\\$2");
}
return arg.replace(/([A-Za-z]:)?([#!"$&'()*,:;<=>?@[\\\]^`{|}])/g, "$1\\$2");

// Windows: cross-spawn-style escaping for `cmd.exe /d /s /c "..."` invocation.
const metaChars = /([()\][%!^"`<>&|;, *?])/g;

// Double up backslashes that precede a `"` (so the pre-existing backslashes survive
// as literals after the quote-doubling below).
let escaped = arg.replace(/(?=(\\+?)?)\1"/g, '$1$1\\"');
// Same for backslashes at the end of the string (which will be followed by the wrapping `"`).
escaped = escaped.replace(/(?=(\\+?)?)\1$/, "$1$1");
// Wrap the whole thing in quotes.
escaped = `"${escaped}"`;
// Caret-escape every meta char, including the wrapping quotes themselves.
escaped = escaped.replace(metaChars, "^$1");

return escaped;
}
93 changes: 62 additions & 31 deletions packages/cloudflare/src/cli/commands/utils/run-wrangler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import path from "node:path";

import { compareSemver } from "@opennextjs/aws/build/helper.js";

import { quoteShellMeta } from "./helpers.js";

export type PackagerDetails = {
/** The name of the package manager. */
packager: "npm" | "pnpm" | "yarn" | "bun";
Expand Down Expand Up @@ -96,39 +98,68 @@ export function runWrangler(
): WranglerCommandResult {
const noLogs = wranglerOpts.logging === "none";
const shouldPipeLogs = wranglerOpts.logging === "error";
const isWin = process.platform === "win32";

const packagerArgs = [
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"] : []),
]),
];

// 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).
const stdio: ("inherit" | "ignore" | "pipe")[] =
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 you please re-add the former comment on this

shouldPipeLogs || noLogs ? ["ignore", "pipe", "pipe"] : ["inherit", "inherit", "pipe"];

const env = {
...process.env,
...wranglerOpts.env,
// `.env` files are handled by the adapter.
// Wrangler would load `.env.<wrangler env>` while we should load `.env.<process.env.NEXTJS_ENV>`
// See https://opennext.js.org/cloudflare/howtos/env-vars
CLOUDFLARE_LOAD_DEV_VARS_FROM_DOT_ENV: "false",
};

const result = spawnSync(
options.packager,
[
options.packager === "bun" ? "x" : "exec",
"wrangler",
...injectPassthroughFlagForArgs(
options,
// We use `shell: false` everywhere to avoid DEP0190.
// See https://nodejs.org/api/deprecations.html#dep0190-passing-args-to-nodechild-process-execfilespawn-with-shell-option
//
// On POSIX we can spawn the package manager binary directly: each value is passed verbatim
// as its own argv entry.
//
// On Windows the package manager is typically a `.cmd` shim. 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` without `shell: true` is to spawn `cmd.exe`
// directly with `/c`. We pre-escape every token with `quoteShellMeta` and pass the joined
// command line as a single quoted string with `windowsVerbatimArguments: true` so Node
// does not re-escape it. This mirrors how `cross-spawn` handles non-`.exe` commands.
const result = isWin
? spawnSync(
"cmd.exe",
[
...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)
),
],
{
shell: true,
// 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).
stdio: shouldPipeLogs || noLogs ? ["ignore", "pipe", "pipe"] : ["inherit", "inherit", "pipe"],
env: {
...process.env,
...wranglerOpts.env,
// `.env` files are handled by the adapter.
// Wrangler would load `.env.<wrangler env>` while we should load `.env.<process.env.NEXTJS_ENV>`
// See https://opennext.js.org/cloudflare/howtos/env-vars
CLOUDFLARE_LOAD_DEV_VARS_FROM_DOT_ENV: "false",
},
}
);
"/d",
"/s",
"/c",
`"${[options.packager, ...packagerArgs].map((token) => quoteShellMeta(token)).join(" ")}"`,
],
{
shell: false,
windowsVerbatimArguments: true,
stdio,
env,
}
)
: spawnSync(options.packager, packagerArgs, {
shell: false,
stdio,
env,
});

const success = result.status === 0;
const stdout = result.stdout?.toString() ?? "";
Expand Down
Loading