From d2d5010aec64fc48efc45f6b80a766124fa5035f Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 27 May 2026 03:34:44 -0400 Subject: [PATCH] fix: reject partial numeric CLI options --- src/cli/nodes-cli.coverage.test.ts | 60 ++++++++++++++++++++++++++ src/cli/nodes-cli/register.camera.ts | 20 +++++---- src/cli/nodes-cli/register.invoke.ts | 14 ++++-- src/cli/nodes-cli/register.location.ts | 24 +++++++---- src/cli/nodes-cli/register.notify.ts | 14 ++++-- src/cli/nodes-cli/register.screen.ts | 18 +++++--- src/cli/nodes-cli/rpc.ts | 33 ++++++++++++++ src/cli/plugins-cli.ts | 3 +- src/cli/plugins-search-command.test.ts | 13 ++++++ src/cli/program/helpers.test.ts | 8 ++++ src/cli/program/helpers.ts | 10 ++++- src/cli/skills-cli.commands.test.ts | 7 +++ src/cli/skills-cli.ts | 3 +- 13 files changed, 195 insertions(+), 32 deletions(-) diff --git a/src/cli/nodes-cli.coverage.test.ts b/src/cli/nodes-cli.coverage.test.ts index cc9d1956066..f8795b37cd1 100644 --- a/src/cli/nodes-cli.coverage.test.ts +++ b/src/cli/nodes-cli.coverage.test.ts @@ -186,4 +186,64 @@ describe("nodes-cli coverage", () => { }); expect(invoke.params?.timeoutMs).toBe(6000); }); + + it.each([ + { + args: ["nodes", "location", "get", "--node", "mac-1", "--max-age", "1000ms"], + flag: "--max-age", + }, + { + args: ["nodes", "location", "get", "--node", "mac-1", "--location-timeout", "5s"], + flag: "--location-timeout", + }, + { + args: ["nodes", "location", "get", "--node", "mac-1", "--invoke-timeout", "6s"], + flag: "--invoke-timeout", + }, + { + args: ["nodes", "camera", "snap", "--node", "mac-1", "--max-width", "1024px"], + flag: "--max-width", + }, + { + args: ["nodes", "camera", "snap", "--node", "mac-1", "--delay-ms", "20ms"], + flag: "--delay-ms", + }, + { + args: ["nodes", "camera", "snap", "--node", "mac-1", "--invoke-timeout", "20s"], + flag: "--invoke-timeout", + }, + { + args: ["nodes", "camera", "clip", "--node", "mac-1", "--invoke-timeout", "90s"], + flag: "--invoke-timeout", + }, + { + args: ["nodes", "screen", "record", "--node", "mac-1", "--screen", "1x"], + flag: "--screen", + }, + { + args: ["nodes", "screen", "record", "--node", "mac-1", "--invoke-timeout", "120s"], + flag: "--invoke-timeout", + }, + { + args: ["nodes", "notify", "--node", "mac-1", "--title", "Ping", "--invoke-timeout", "15s"], + flag: "--invoke-timeout", + }, + { + args: [ + "nodes", + "invoke", + "--node", + "mac-1", + "--command", + "canvas.eval", + "--invoke-timeout", + "15s", + ], + flag: "--invoke-timeout", + }, + ])("rejects partial numeric option for $args", async ({ args, flag }) => { + await expect(sharedProgram.parseAsync(args, { from: "user" })).rejects.toThrow("__exit__:1"); + expect(runtimeErrors.at(-1)).toContain(`${flag} must be`); + expect(lastNodeInvokeCall).toBeNull(); + }); }); diff --git a/src/cli/nodes-cli/register.camera.ts b/src/cli/nodes-cli/register.camera.ts index 15f5abc908c..9c3564828f8 100644 --- a/src/cli/nodes-cli/register.camera.ts +++ b/src/cli/nodes-cli/register.camera.ts @@ -20,6 +20,8 @@ import { buildNodeInvokeParams, callGatewayCli, nodesCallOpts, + parseOptionalNodeNonNegativeInteger, + parseOptionalNodePositiveInteger, resolveNode, resolveNodeId, } from "./rpc.js"; @@ -131,16 +133,17 @@ export function registerNodesCameraCommands(nodes: Command) { ); })(); - const maxWidth = opts.maxWidth ? Number.parseInt(opts.maxWidth, 10) : undefined; + const maxWidth = parseOptionalNodePositiveInteger(opts.maxWidth, "--max-width"); const quality = opts.quality ? Number.parseFloat(opts.quality) : undefined; - const delayMs = opts.delayMs ? Number.parseInt(opts.delayMs, 10) : undefined; + const delayMs = parseOptionalNodeNonNegativeInteger(opts.delayMs, "--delay-ms"); const deviceId = normalizeOptionalString(opts.deviceId); if (deviceId && facings.length > 1) { throw new Error("facing=both is not allowed when --device-id is set"); } - const timeoutMs = opts.invokeTimeout - ? Number.parseInt(opts.invokeTimeout, 10) - : undefined; + const timeoutMs = parseOptionalNodePositiveInteger( + opts.invokeTimeout, + "--invoke-timeout", + ); const results: Array<{ facing: CameraFacing; @@ -216,9 +219,10 @@ export function registerNodesCameraCommands(nodes: Command) { const facing = parseFacing(opts.facing ?? "front"); const durationMs = parseDurationMs(opts.duration ?? "3000"); const includeAudio = opts.audio !== false; - const timeoutMs = opts.invokeTimeout - ? Number.parseInt(opts.invokeTimeout, 10) - : undefined; + const timeoutMs = parseOptionalNodePositiveInteger( + opts.invokeTimeout, + "--invoke-timeout", + ); const deviceId = normalizeOptionalString(opts.deviceId); const invokeParams = buildNodeInvokeParams({ diff --git a/src/cli/nodes-cli/register.invoke.ts b/src/cli/nodes-cli/register.invoke.ts index fa1191009c1..bbc1d9eb1de 100644 --- a/src/cli/nodes-cli/register.invoke.ts +++ b/src/cli/nodes-cli/register.invoke.ts @@ -6,7 +6,12 @@ import { normalizeOptionalString, } from "../../shared/string-coerce.js"; import { getNodesTheme, runNodesCommand } from "./cli-utils.js"; -import { callGatewayCli, nodesCallOpts, resolveNodeId } from "./rpc.js"; +import { + callGatewayCli, + nodesCallOpts, + parseOptionalNodePositiveInteger, + resolveNodeId, +} from "./rpc.js"; import type { NodesRpcOpts } from "./types.js"; const BLOCKED_NODE_INVOKE_COMMANDS = new Set(["system.run", "system.run.prepare"]); @@ -37,9 +42,10 @@ export function registerNodesInvokeCommands(nodes: Command) { ); } const params = JSON.parse(opts.params ?? "{}") as unknown; - const timeoutMs = opts.invokeTimeout - ? Number.parseInt(opts.invokeTimeout, 10) - : undefined; + const timeoutMs = parseOptionalNodePositiveInteger( + opts.invokeTimeout, + "--invoke-timeout", + ); const invokeParams: Record = { nodeId, diff --git a/src/cli/nodes-cli/register.location.ts b/src/cli/nodes-cli/register.location.ts index 44df9a89636..278be407409 100644 --- a/src/cli/nodes-cli/register.location.ts +++ b/src/cli/nodes-cli/register.location.ts @@ -3,7 +3,13 @@ import { randomIdempotencyKey } from "../../gateway/call.js"; import { defaultRuntime } from "../../runtime.js"; import { normalizeOptionalLowercaseString } from "../../shared/string-coerce.js"; import { runNodesCommand } from "./cli-utils.js"; -import { callGatewayCli, nodesCallOpts, resolveNodeId } from "./rpc.js"; +import { + callGatewayCli, + nodesCallOpts, + parseOptionalNodeNonNegativeInteger, + parseOptionalNodePositiveInteger, + resolveNodeId, +} from "./rpc.js"; import type { NodesRpcOpts } from "./types.js"; export function registerNodesLocationCommands(nodes: Command) { @@ -24,7 +30,7 @@ export function registerNodesLocationCommands(nodes: Command) { .action(async (opts: NodesRpcOpts) => { await runNodesCommand("location get", async () => { const nodeId = await resolveNodeId(opts, opts.node ?? ""); - const maxAgeMs = opts.maxAge ? Number.parseInt(opts.maxAge, 10) : undefined; + const maxAgeMs = parseOptionalNodeNonNegativeInteger(opts.maxAge, "--max-age"); const desiredAccuracyRaw = normalizeOptionalLowercaseString(opts.accuracy); const desiredAccuracy = desiredAccuracyRaw === "coarse" || @@ -32,12 +38,14 @@ export function registerNodesLocationCommands(nodes: Command) { desiredAccuracyRaw === "precise" ? desiredAccuracyRaw : undefined; - const timeoutMs = opts.locationTimeout - ? Number.parseInt(opts.locationTimeout, 10) - : undefined; - const invokeTimeoutMs = opts.invokeTimeout - ? Number.parseInt(opts.invokeTimeout, 10) - : undefined; + const timeoutMs = parseOptionalNodePositiveInteger( + opts.locationTimeout, + "--location-timeout", + ); + const invokeTimeoutMs = parseOptionalNodePositiveInteger( + opts.invokeTimeout, + "--invoke-timeout", + ); const invokeParams: Record = { nodeId, diff --git a/src/cli/nodes-cli/register.notify.ts b/src/cli/nodes-cli/register.notify.ts index 85de63c2e7c..1bf0c702e85 100644 --- a/src/cli/nodes-cli/register.notify.ts +++ b/src/cli/nodes-cli/register.notify.ts @@ -3,7 +3,12 @@ import { randomIdempotencyKey } from "../../gateway/call.js"; import { defaultRuntime } from "../../runtime.js"; import { normalizeOptionalString } from "../../shared/string-coerce.js"; import { getNodesTheme, runNodesCommand } from "./cli-utils.js"; -import { callGatewayCli, nodesCallOpts, resolveNodeId } from "./rpc.js"; +import { + callGatewayCli, + nodesCallOpts, + parseOptionalNodePositiveInteger, + resolveNodeId, +} from "./rpc.js"; import type { NodesRpcOpts } from "./types.js"; export function registerNodesNotifyCommand(nodes: Command) { @@ -26,9 +31,10 @@ export function registerNodesNotifyCommand(nodes: Command) { if (!title && !body) { throw new Error("missing --title or --body"); } - const invokeTimeout = opts.invokeTimeout - ? Number.parseInt(opts.invokeTimeout, 10) - : undefined; + const invokeTimeout = parseOptionalNodePositiveInteger( + opts.invokeTimeout, + "--invoke-timeout", + ); const invokeParams: Record = { nodeId, command: "system.notify", diff --git a/src/cli/nodes-cli/register.screen.ts b/src/cli/nodes-cli/register.screen.ts index 7f57343a36b..61f170f6027 100644 --- a/src/cli/nodes-cli/register.screen.ts +++ b/src/cli/nodes-cli/register.screen.ts @@ -8,7 +8,14 @@ import { } from "../nodes-screen.js"; import { parseDurationMs } from "../parse-duration.js"; import { runNodesCommand } from "./cli-utils.js"; -import { buildNodeInvokeParams, callGatewayCli, nodesCallOpts, resolveNodeId } from "./rpc.js"; +import { + buildNodeInvokeParams, + callGatewayCli, + nodesCallOpts, + parseOptionalNodeNonNegativeInteger, + parseOptionalNodePositiveInteger, + resolveNodeId, +} from "./rpc.js"; import type { NodesRpcOpts } from "./types.js"; export function registerNodesScreenCommands(nodes: Command) { @@ -31,11 +38,12 @@ export function registerNodesScreenCommands(nodes: Command) { await runNodesCommand("screen record", async () => { const nodeId = await resolveNodeId(opts, opts.node ?? ""); const durationMs = parseDurationMs(opts.duration ?? ""); - const screenIndex = Number.parseInt(opts.screen ?? "0", 10); + const screenIndex = parseOptionalNodeNonNegativeInteger(opts.screen ?? "0", "--screen"); const fps = Number.parseFloat(opts.fps ?? "10"); - const timeoutMs = opts.invokeTimeout - ? Number.parseInt(opts.invokeTimeout, 10) - : undefined; + const timeoutMs = parseOptionalNodePositiveInteger( + opts.invokeTimeout, + "--invoke-timeout", + ); const invokeParams = buildNodeInvokeParams({ nodeId, diff --git a/src/cli/nodes-cli/rpc.ts b/src/cli/nodes-cli/rpc.ts index ba594b3fe9f..4de93b1d6f5 100644 --- a/src/cli/nodes-cli/rpc.ts +++ b/src/cli/nodes-cli/rpc.ts @@ -1,6 +1,10 @@ import { randomUUID } from "node:crypto"; import type { Command } from "commander"; import type { OperatorScope } from "../../gateway/method-scopes.js"; +import { + parseStrictNonNegativeInteger, + parseStrictPositiveInteger, +} from "../../infra/parse-finite-number.js"; import { createLazyImportLoader } from "../../shared/lazy-promise.js"; import { resolveNodeFromNodeList } from "../../shared/node-resolve.js"; import { normalizeLowercaseStringOrEmpty } from "../../shared/string-coerce.js"; @@ -63,6 +67,35 @@ export function buildNodeInvokeParams(params: { return invokeParams; } +function hasOptionalValue(value: unknown): boolean { + return value !== undefined && value !== null && value !== ""; +} + +export function parseOptionalNodePositiveInteger(value: unknown, flag: string): number | undefined { + if (!hasOptionalValue(value)) { + return undefined; + } + const parsed = parseStrictPositiveInteger(value); + if (parsed === undefined) { + throw new Error(`${flag} must be a positive integer.`); + } + return parsed; +} + +export function parseOptionalNodeNonNegativeInteger( + value: unknown, + flag: string, +): number | undefined { + if (!hasOptionalValue(value)) { + return undefined; + } + const parsed = parseStrictNonNegativeInteger(value); + if (parsed === undefined) { + throw new Error(`${flag} must be a non-negative integer.`); + } + return parsed; +} + export function unauthorizedHintForMessage(message: string): string | null { const haystack = normalizeLowercaseStringOrEmpty(message); if ( diff --git a/src/cli/plugins-cli.ts b/src/cli/plugins-cli.ts index 9e9685b666a..80d80d06c35 100644 --- a/src/cli/plugins-cli.ts +++ b/src/cli/plugins-cli.ts @@ -3,6 +3,7 @@ import { formatDocsLink } from "../terminal/links.js"; import { theme } from "../terminal/theme.js"; import type { PluginInspectOptions } from "./plugins-inspect-command.js"; import type { PluginsListOptions } from "./plugins-list-command.js"; +import { parseStrictPositiveIntOption } from "./program/helpers.js"; import { applyParentDefaultHelpAction } from "./program/parent-default-help.js"; export type PluginUpdateOptions = { @@ -75,7 +76,7 @@ export function registerPluginsCli(program: Command) { .command("search") .description("Search ClawHub plugin packages") .argument("[query...]", "Search query") - .option("--limit ", "Max results", (value) => Number.parseInt(value, 10)) + .option("--limit ", "Max results", (value) => parseStrictPositiveIntOption(value, "--limit")) .option("--json", "Print JSON", false) .action(async (queryParts: string[], opts: PluginSearchOptions) => { const { runPluginsSearchCommand } = await import("./plugins-search-command.js"); diff --git a/src/cli/plugins-search-command.test.ts b/src/cli/plugins-search-command.test.ts index f0015fa5923..855910fa3cc 100644 --- a/src/cli/plugins-search-command.test.ts +++ b/src/cli/plugins-search-command.test.ts @@ -1,3 +1,4 @@ +import { Command } from "commander"; import { beforeEach, describe, expect, it, vi } from "vitest"; const mocks = vi.hoisted(() => { @@ -35,6 +36,7 @@ vi.mock("../infra/clawhub.js", () => ({ })); const { runPluginsSearchCommand } = await import("./plugins-search-command.js"); +const { registerPluginsCli } = await import("./plugins-cli.js"); describe("plugins search command", () => { beforeEach(() => { @@ -107,4 +109,15 @@ describe("plugins search command", () => { expect(mocks.runtime.writeJson).toHaveBeenCalledWith({ results: [] }, 2); }); + + it("rejects partial numeric search limits", async () => { + const program = new Command(); + program.exitOverride(); + registerPluginsCli(program); + + await expect( + program.parseAsync(["plugins", "search", "calendar", "--limit", "10ms"], { from: "user" }), + ).rejects.toThrow("--limit must be a positive integer."); + expect(mocks.searchClawHubPackages).not.toHaveBeenCalled(); + }); }); diff --git a/src/cli/program/helpers.test.ts b/src/cli/program/helpers.test.ts index 0d7b7fa3890..d40f606302f 100644 --- a/src/cli/program/helpers.test.ts +++ b/src/cli/program/helpers.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import { collectOption, parsePositiveIntOrUndefined, + parseStrictPositiveIntOption, parseStrictPositiveIntOrUndefined, resolveActionArgs, resolveCommandOptionArgs, @@ -53,6 +54,13 @@ describe("program helpers", () => { expect(parseStrictPositiveIntOrUndefined(value)).toBe(expected); }); + it("parseStrictPositiveIntOption rejects partial numeric strings", () => { + expect(parseStrictPositiveIntOption("10", "--limit")).toBe(10); + expect(() => parseStrictPositiveIntOption("10ms", "--limit")).toThrow( + "--limit must be a positive integer.", + ); + }); + it("resolveActionArgs returns args when command has arg array", () => { const command = new Command(); (command as Command & { args?: string[] }).args = ["one", "two"]; diff --git a/src/cli/program/helpers.ts b/src/cli/program/helpers.ts index 453d04f0a22..077ca54f3dc 100644 --- a/src/cli/program/helpers.ts +++ b/src/cli/program/helpers.ts @@ -1,4 +1,4 @@ -import type { Command } from "commander"; +import { InvalidArgumentError, type Command } from "commander"; import { parseStrictPositiveInteger } from "../../infra/parse-finite-number.js"; export function collectOption(value: string, previous: string[] = []): string[] { @@ -30,6 +30,14 @@ export function parseStrictPositiveIntOrUndefined(value: unknown): number | unde return parseStrictPositiveInteger(value); } +export function parseStrictPositiveIntOption(value: string, flag: string): number { + const parsed = parseStrictPositiveInteger(value); + if (parsed === undefined) { + throw new InvalidArgumentError(`${flag} must be a positive integer.`); + } + return parsed; +} + export function resolveActionArgs(actionCommand?: Command): string[] { if (!actionCommand) { return []; diff --git a/src/cli/skills-cli.commands.test.ts b/src/cli/skills-cli.commands.test.ts index 6f5366abf96..04e3d95dc3a 100644 --- a/src/cli/skills-cli.commands.test.ts +++ b/src/cli/skills-cli.commands.test.ts @@ -289,6 +289,13 @@ describe("skills cli commands", () => { ).toBe(true); }); + it("rejects partial numeric search limits", async () => { + await expect(runCommand(["skills", "search", "calendar", "--limit", "10ms"])).rejects.toThrow( + "--limit must be a positive integer.", + ); + expect(searchSkillsFromClawHubMock).not.toHaveBeenCalled(); + }); + it("installs a skill from ClawHub into the active workspace", async () => { installSkillFromClawHubMock.mockResolvedValue({ ok: true, diff --git a/src/cli/skills-cli.ts b/src/cli/skills-cli.ts index bc0f699ee7c..9512489b009 100644 --- a/src/cli/skills-cli.ts +++ b/src/cli/skills-cli.ts @@ -21,6 +21,7 @@ import { formatDocsLink } from "../terminal/links.js"; import { theme } from "../terminal/theme.js"; import { CONFIG_DIR } from "../utils.js"; import { resolveOptionFromCommand } from "./cli-utils.js"; +import { parseStrictPositiveIntOption } from "./program/helpers.js"; import { formatSkillInfo, formatSkillsCheck, formatSkillsList } from "./skills-cli.format.js"; export type { @@ -123,7 +124,7 @@ export function registerSkillsCli(program: Command) { .command("search") .description("Search ClawHub skills") .argument("[query...]", "Optional search query") - .option("--limit ", "Max results", (value) => Number.parseInt(value, 10)) + .option("--limit ", "Max results", (value) => parseStrictPositiveIntOption(value, "--limit")) .option("--json", "Output as JSON", false) .action(async (queryParts: string[], opts: { limit?: number; json?: boolean }) => { try {