fix(wrangler): enforce single value on non-array arguments (#7005)

edmundhung · web-flow · commit 6131ef5a3d16 · 2024-10-22T12:53:50.000+01:00
diff --git a/.changeset/sour-buses-develop.md b/.changeset/sour-buses-develop.md
@@ -0,0 +1,5 @@
+---
+"wrangler": patch
+---
+
+fix: prevent users from passing multiple arguments to non array options
diff --git a/packages/wrangler/src/__tests__/core/command-registration.test.ts b/packages/wrangler/src/__tests__/core/command-registration.test.ts
@@ -484,6 +484,17 @@ describe("Command Registration", () => {
 				`[Error: Missing namespace definition for 'wrangler missing-namespace']`
 			);
 		});
+
+		test("throws upon duplicated arguments on non-array options", async () => {
+			await expect(
+				runWrangler(
+					"my-test-command --str foo --str bar --num 123 --bool --arr first second --str baz"
+				)
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`[Error: The argument "--str" expects a single value, but received multiple: ["foo","bar","baz"].]`
+			);
+		});
+
 		test("throws upon alias to undefined command", async () => {
 			defineAlias({
 				command: "wrangler my-alias-command",
diff --git a/packages/wrangler/src/__tests__/index.test.ts b/packages/wrangler/src/__tests__/index.test.ts
@@ -140,6 +140,22 @@ describe("wrangler", () => {
 		});
 	});
 
+	describe("global options", () => {
+		it("should display an error if duplicated --env or --config arguments are provided", async () => {
+			await expect(
+				runWrangler("--env prod -e prod")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`[Error: The argument "--env" expects a single value, but received multiple: ["prod","prod"].]`
+			);
+
+			await expect(
+				runWrangler("--config=wrangler.toml -c example")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`[Error: The argument "--config" expects a single value, but received multiple: ["wrangler.toml","example"].]`
+			);
+		});
+	});
+
 	describe("preview", () => {
 		it("should throw an error if the deprecated command is used with positional arguments", async () => {
 			await expect(runWrangler("preview GET")).rejects
diff --git a/packages/wrangler/src/core/helpers.ts b/packages/wrangler/src/core/helpers.ts
@@ -0,0 +1,48 @@
+import { CommandLineArgsError } from "../errors";
+
+/**
+ * A helper to demand one of a set of options
+ * via https://github.com/yargs/yargs/issues/1093#issuecomment-491299261
+ */
+export function demandOneOfOption(...options: string[]) {
+	return function (argv: { [key: string]: unknown }) {
+		const count = options.filter((option) => argv[option]).length;
+		const lastOption = options.pop();
+
+		if (count === 0) {
+			throw new CommandLineArgsError(
+				`Exactly one of the arguments ${options.join(
+					", "
+				)} and ${lastOption} is required`
+			);
+		} else if (count > 1) {
+			throw new CommandLineArgsError(
+				`Arguments ${options.join(
+					", "
+				)} and ${lastOption} are mutually exclusive`
+			);
+		}
+
+		return true;
+	};
+}
+
+/**
+ * A helper to ensure that an argument only receives a single value.
+ *
+ * This is a workaround for a limitation in yargs where non-array arguments can still receive multiple values
+ * even though the inferred type is not an array.
+ *
+ * @see https://github.com/yargs/yargs/issues/1318
+ */
+export function demandSingleValue(key: string) {
+	return function (argv: { [key: string]: unknown }) {
+		if (Array.isArray(argv[key])) {
+			throw new CommandLineArgsError(
+				`The argument "--${key}" expects a single value, but received multiple: ${JSON.stringify(argv[key])}.`
+			);
+		}
+
+		return true;
+	};
+}
diff --git a/packages/wrangler/src/core/index.ts b/packages/wrangler/src/core/index.ts
@@ -1 +1,2 @@
 export { defineAlias, defineCommand, defineNamespace } from "./define-command";
+export { demandOneOfOption, demandSingleValue } from "./helpers";
diff --git a/packages/wrangler/src/core/register-commands.ts b/packages/wrangler/src/core/register-commands.ts
@@ -6,6 +6,7 @@ import { FatalError, UserError } from "../errors";
 import { logger } from "../logger";
 import { printWranglerBanner } from "../update-check";
 import { CommandRegistrationError, DefinitionTreeRoot } from "./define-command";
+import { demandSingleValue } from "./helpers";
 import type { CommonYargsArgv, SubHelp } from "../yargs-types";
 import type {
 	Command,
@@ -125,6 +126,17 @@ function walkTreeAndRegister(
 			if (def.type === "command") {
 				yargs.options(def.args);
 
+				// Yargs offers an `array: true` option that will always coerces the value to an array
+				// e.g. `--name foo` becomes `{ name: ["foo"] }` instead of `{ name: "foo" }`
+				// However, non-array arguments can still receives multiple values
+				// e.g. `--name foo --name bar` becomes `{ name: ["foo", "bar"] }` regardless of the `array` setting
+				// @see https://github.com/yargs/yargs/issues/1318
+				for (const [key, opt] of Object.entries(def.args)) {
+					if (!opt.array) {
+						yargs.check(demandSingleValue(key));
+					}
+				}
+
 				for (const key of def.positionalArgs ?? []) {
 					yargs.positional(key, def.args[key]);
 				}
diff --git a/packages/wrangler/src/errors.ts b/packages/wrangler/src/errors.ts
@@ -28,6 +28,8 @@ export class FatalError extends UserError {
 	}
 }
 
+export class CommandLineArgsError extends UserError {}
+
 /**
  * JsonFriendlyFatalError is used to output JSON when wrangler crashes, useful for --json mode.
  *
diff --git a/packages/wrangler/src/generate/index.ts b/packages/wrangler/src/generate/index.ts
@@ -2,9 +2,9 @@ import fs from "node:fs";
 import path from "node:path";
 import { execa } from "execa";
 import { getC3CommandFromEnv } from "../environment-variables/misc-variables";
-import { UserError } from "../errors";
+import { CommandLineArgsError, UserError } from "../errors";
 import { cloneIntoDirectory, initializeGit } from "../git-client";
-import { CommandLineArgsError, printWranglerBanner } from "../index";
+import { printWranglerBanner } from "../index";
 import { initHandler } from "../init";
 import { logger } from "../logger";
 import { getPackageManager } from "../package-manager";
diff --git a/packages/wrangler/src/index.ts b/packages/wrangler/src/index.ts
@@ -38,12 +38,17 @@ import {
 import { devHandler, devOptions } from "./dev";
 import { workerNamespaceCommands } from "./dispatch-namespace";
 import { docsHandler, docsOptions } from "./docs";
-import { JsonFriendlyFatalError, UserError } from "./errors";
+import {
+	CommandLineArgsError,
+	JsonFriendlyFatalError,
+	UserError,
+} from "./errors";
 import { generateHandler, generateOptions } from "./generate";
 import { hyperdrive } from "./hyperdrive/index";
 import { initHandler, initOptions } from "./init";
 import "./kv";
 import "./workflows";
+import { demandSingleValue } from "./core";
 import { logBuildFailure, logger, LOGGER_LEVELS } from "./logger";
 import * as metrics from "./metrics";
 import { mTlsCertificateCommands } from "./mtls-certificate/cli";
@@ -159,35 +164,6 @@ export function getLegacyScriptName(
 		: args.name ?? config.name;
 }
 
-/**
- * A helper to demand one of a set of options
- * via https://github.com/yargs/yargs/issues/1093#issuecomment-491299261
- */
-export function demandOneOfOption(...options: string[]) {
-	return function (argv: { [key: string]: unknown }) {
-		const count = options.filter((option) => argv[option]).length;
-		const lastOption = options.pop();
-
-		if (count === 0) {
-			throw new CommandLineArgsError(
-				`Exactly one of the arguments ${options.join(
-					", "
-				)} and ${lastOption} is required`
-			);
-		} else if (count > 1) {
-			throw new CommandLineArgsError(
-				`Arguments ${options.join(
-					", "
-				)} and ${lastOption} are mutually exclusive`
-			);
-		}
-
-		return true;
-	};
-}
-
-export class CommandLineArgsError extends UserError {}
-
 export function createCLIParser(argv: string[]) {
 	const experimentalGradualRollouts =
 		// original flag -- using internal product name (Gradual Rollouts) -- kept for temp back-compat
@@ -230,12 +206,14 @@ export function createCLIParser(argv: string[]) {
 			type: "string",
 			requiresArg: true,
 		})
+		.check(demandSingleValue("config"))
 		.option("env", {
 			alias: "e",
 			describe: "Environment to use for operations and .env files",
 			type: "string",
 			requiresArg: true,
 		})
+		.check(demandSingleValue("env"))
 		.option("experimental-json-config", {
 			alias: "j",
 			describe: `Experimental: support wrangler.json`,
diff --git a/packages/wrangler/src/init.ts b/packages/wrangler/src/init.ts
@@ -12,7 +12,7 @@ import { readConfig } from "./config";
 import { getDatabaseInfoFromId } from "./d1/utils";
 import { confirm, select } from "./dialogs";
 import { getC3CommandFromEnv } from "./environment-variables/misc-variables";
-import { FatalError, UserError } from "./errors";
+import { CommandLineArgsError, FatalError, UserError } from "./errors";
 import { getGitVersioon, initializeGit, isInsideGitRepo } from "./git-client";
 import { logger } from "./logger";
 import { getPackageManager } from "./package-manager";
@@ -21,7 +21,7 @@ import { getBasePath } from "./paths";
 import { requireAuth } from "./user";
 import { createBatches } from "./utils/create-batches";
 import * as shellquote from "./utils/shell-quote";
-import { CommandLineArgsError, printWranglerBanner } from "./index";
+import { printWranglerBanner } from "./index";
 import type { RawConfig } from "./config";
 import type {
 	CustomDomainRoute,
diff --git a/packages/wrangler/src/kv/index.ts b/packages/wrangler/src/kv/index.ts
@@ -2,10 +2,14 @@ import { Blob } from "node:buffer";
 import { arrayBuffer } from "node:stream/consumers";
 import { StringDecoder } from "node:string_decoder";
 import { readConfig } from "../config";
-import { defineAlias, defineCommand, defineNamespace } from "../core";
+import {
+	defineAlias,
+	defineCommand,
+	defineNamespace,
+	demandOneOfOption,
+} from "../core";
 import { confirm } from "../dialogs";
-import { UserError } from "../errors";
-import { CommandLineArgsError, demandOneOfOption } from "../index";
+import { CommandLineArgsError, UserError } from "../errors";
 import { logger } from "../logger";
 import * as metrics from "../metrics";
 import { parseJSON, readFileSync, readFileSyncToBuffer } from "../parse";
diff --git a/packages/wrangler/src/pubsub/pubsub-commands.ts b/packages/wrangler/src/pubsub/pubsub-commands.ts
@@ -1,6 +1,6 @@
 import { readConfig } from "../config";
 import { confirm } from "../dialogs";
-import { CommandLineArgsError } from "../index";
+import { CommandLineArgsError } from "../errors";
 import { logger } from "../logger";
 import * as metrics from "../metrics";
 import { parseHumanDuration } from "../parse";
diff --git a/packages/wrangler/src/queues/cli/commands/consumer/http-pull/add.ts b/packages/wrangler/src/queues/cli/commands/consumer/http-pull/add.ts
@@ -1,5 +1,5 @@
 import { readConfig } from "../../../../../config";
-import { CommandLineArgsError } from "../../../../../index";
+import { CommandLineArgsError } from "../../../../../errors";
 import { logger } from "../../../../../logger";
 import { postConsumer } from "../../../../client";
 import type {
diff --git a/packages/wrangler/src/queues/cli/commands/consumer/worker/add.ts b/packages/wrangler/src/queues/cli/commands/consumer/worker/add.ts
@@ -1,5 +1,5 @@
 import { readConfig } from "../../../../../config";
-import { CommandLineArgsError } from "../../../../../index";
+import { CommandLineArgsError } from "../../../../../errors";
 import { logger } from "../../../../../logger";
 import { postConsumer } from "../../../../client";
 import type {
diff --git a/packages/wrangler/src/queues/cli/commands/create.ts b/packages/wrangler/src/queues/cli/commands/create.ts
@@ -1,5 +1,5 @@
 import { readConfig } from "../../../config";
-import { CommandLineArgsError } from "../../../index";
+import { CommandLineArgsError } from "../../../errors";
 import { logger } from "../../../logger";
 import { createQueue } from "../../client";
 import { handleFetchError } from "../../utils";
diff --git a/packages/wrangler/src/r2/index.ts b/packages/wrangler/src/r2/index.ts
@@ -5,8 +5,8 @@ import * as stream from "node:stream";
 import { ReadableStream } from "node:stream/web";
 import prettyBytes from "pretty-bytes";
 import { readConfig } from "../config";
-import { FatalError, UserError } from "../errors";
-import { CommandLineArgsError, printWranglerBanner } from "../index";
+import { CommandLineArgsError, FatalError, UserError } from "../errors";
+import { printWranglerBanner } from "../index";
 import { logger } from "../logger";
 import * as metrics from "../metrics";
 import { requireAuth } from "../user";
diff --git a/packages/wrangler/src/type-generation/index.ts b/packages/wrangler/src/type-generation/index.ts
@@ -6,8 +6,7 @@ import { getNodeCompat } from "miniflare";
 import { findWranglerToml, readConfig } from "../config";
 import { getEntry } from "../deployment-bundle/entry";
 import { getVarsForDev } from "../dev/dev-vars";
-import { UserError } from "../errors";
-import { CommandLineArgsError } from "../index";
+import { CommandLineArgsError, UserError } from "../errors";
 import { logger } from "../logger";
 import { parseJSONC } from "../parse";
 import { printWranglerBanner } from "../update-check";

Original file line number	Diff line number	Diff line change
`@@ -1 +1,2 @@`
`1`	`1`	`export { defineAlias, defineCommand, defineNamespace } from "./define-command";`
	`2`	`+export { demandOneOfOption, demandSingleValue } from "./helpers";`
Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,8 @@ export class FatalError extends UserError {`
`28`	`28`	`}`
`29`	`29`	`}`
`30`	`30`
	`31`	`+export class CommandLineArgsError extends UserError {}`
	`32`	`+`
`31`	`33`	`/**`
`32`	`34`	`* JsonFriendlyFatalError is used to output JSON when wrangler crashes, useful for --json mode.`
`33`	`35`	`*`