Skip to content
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.

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

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

Merged
merged 6 commits into from
Oct 22, 2024
Merged
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/sour-buses-develop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"wrangler": patch
---

fix: prevent users from passing multiple arguments to non array options
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
16 changes: 16 additions & 0 deletions packages/wrangler/src/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
48 changes: 48 additions & 0 deletions packages/wrangler/src/core/helpers.ts
Original file line number Diff line number Diff line change
@@ -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;
};
}
1 change: 1 addition & 0 deletions packages/wrangler/src/core/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export { defineAlias, defineCommand, defineNamespace } from "./define-command";
export { demandOneOfOption, demandSingleValue } from "./helpers";
12 changes: 12 additions & 0 deletions packages/wrangler/src/core/register-commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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));
}
}
Comment on lines +129 to +138
Copy link
Contributor

Choose a reason for hiding this comment

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

Good stuff! This util is intended for us to be more opinionated than yargs 👏


for (const key of def.positionalArgs ?? []) {
yargs.positional(key, def.args[key]);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/wrangler/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/generate/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
38 changes: 8 additions & 30 deletions packages/wrangler/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 {}
Copy link
Member Author

@edmundhung edmundhung Oct 21, 2024

Choose a reason for hiding this comment

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

I moved it to errors.ts since both demandOneOfOption and demandSingleValue need this error class.


export function createCLIParser(argv: string[]) {
const experimentalGradualRollouts =
// original flag -- using internal product name (Gradual Rollouts) -- kept for temp back-compat
Expand Down Expand Up @@ -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`,
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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,
Expand Down
10 changes: 7 additions & 3 deletions packages/wrangler/src/kv/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/pubsub/pubsub-commands.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/queues/cli/commands/create.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/r2/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
3 changes: 1 addition & 2 deletions packages/wrangler/src/type-generation/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Loading