diff --git a/.changeset/fix-runwrangler-shell-injection.md b/.changeset/fix-runwrangler-shell-injection.md new file mode 100644 index 000000000..47fe3be87 --- /dev/null +++ b/.changeset/fix-runwrangler-shell-injection.md @@ -0,0 +1,5 @@ +--- +"@opennextjs/cloudflare": patch +--- + +fix: harden `runWrangler` against shell injection and silence Node.js DEP0190. diff --git a/packages/cloudflare/src/cli/commands/deploy.ts b/packages/cloudflare/src/cli/commands/deploy.ts index b8a8f6ccc..1ced0b9a6 100644 --- a/packages/cloudflare/src/cli/commands/deploy.ts +++ b/packages/cloudflare/src/cli/commands/deploy.ts @@ -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 { @@ -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)}`] : []), ], { diff --git a/packages/cloudflare/src/cli/commands/populate-cache.spec.ts b/packages/cloudflare/src/cli/commands/populate-cache.spec.ts index ea43c4cba..766c845a8 100644 --- a/packages/cloudflare/src/cli/commands/populate-cache.spec.ts +++ b/packages/cloudflare/src/cli/commands/populate-cache.spec.ts @@ -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"); diff --git a/packages/cloudflare/src/cli/commands/populate-cache.ts b/packages/cloudflare/src/cli/commands/populate-cache.ts index b2d55f9cc..576584c8d 100644 --- a/packages/cloudflare/src/cli/commands/populate-cache.ts +++ b/packages/cloudflare/src/cli/commands/populate-cache.ts @@ -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"; @@ -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, @@ -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, @@ -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, diff --git a/packages/cloudflare/src/cli/commands/upload.ts b/packages/cloudflare/src/cli/commands/upload.ts index 095d0e76e..544e3fd23 100644 --- a/packages/cloudflare/src/cli/commands/upload.ts +++ b/packages/cloudflare/src/cli/commands/upload.ts @@ -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 { @@ -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" } diff --git a/packages/cloudflare/src/cli/commands/utils/helpers.spec.ts b/packages/cloudflare/src/cli/commands/utils/helpers.spec.ts new file mode 100644 index 000000000..2cc2880e8 --- /dev/null +++ b/packages/cloudflare/src/cli/commands/utils/helpers.spec.ts @@ -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(/(? { + 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"); + }); + }); +}); diff --git a/packages/cloudflare/src/cli/commands/utils/helpers.ts b/packages/cloudflare/src/cli/commands/utils/helpers.ts index b38ab57d9..cfcdd0ed7 100644 --- a/packages/cloudflare/src/cli/commands/utils/helpers.ts +++ b/packages/cloudflare/src/cli/commands/utils/helpers.ts @@ -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 ""` 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; } diff --git a/packages/cloudflare/src/cli/commands/utils/run-wrangler.ts b/packages/cloudflare/src/cli/commands/utils/run-wrangler.ts index 6859b4097..17ddd2b64 100644 --- a/packages/cloudflare/src/cli/commands/utils/run-wrangler.ts +++ b/packages/cloudflare/src/cli/commands/utils/run-wrangler.ts @@ -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"; @@ -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")[] = + 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.` while we should load `.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.` while we should load `.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() ?? "";