diff --git a/.changeset/deploy-positional-path-telemetry.md b/.changeset/deploy-positional-path-telemetry.md new file mode 100644 index 0000000000..9c265d9c5e --- /dev/null +++ b/.changeset/deploy-positional-path-telemetry.md @@ -0,0 +1,7 @@ +--- +"wrangler": minor +--- + +Categorise the positional path argument to `wrangler deploy` and `wrangler versions upload` in command telemetry + +Command telemetry now records a coarse category for the entry-point/assets positional (`wrangler deploy `) under `sanitizedArgs.path`, so we can understand whether people pass a file, a directory, or a relational reference such as `.` or `../example`. The possible values are `file`, `directory`, `current-dir`, `parent-relative`, and `not-found`, or `null` when no positional is provided. The raw path is never sent — only the category. diff --git a/packages/wrangler/src/__tests__/deploy/entry-points.test.ts b/packages/wrangler/src/__tests__/deploy/entry-points.test.ts index e3d3a6cd05..594d3e18ea 100644 --- a/packages/wrangler/src/__tests__/deploy/entry-points.test.ts +++ b/packages/wrangler/src/__tests__/deploy/entry-points.test.ts @@ -11,6 +11,7 @@ import * as esbuild from "esbuild"; import { http, HttpResponse } from "msw"; import dedent from "ts-dedent"; import { afterEach, assert, beforeEach, describe, it, vi } from "vitest"; +import { logger } from "../../logger"; import { clearOutputFilePath } from "../../output"; import { mockAccountId, mockApiToken } from "../helpers/mock-account-id"; import { mockConsoleMethods } from "../helpers/mock-console"; @@ -1275,4 +1276,71 @@ addEventListener('fetch', event => {});` }); }); }); + + describe("positional path telemetry categorisation", () => { + beforeEach(() => { + logger.loggerLevel = "debug"; + }); + afterEach(() => { + logger.resetLoggerLevel(); + }); + + it("records null when no positional is passed", async ({ expect }) => { + writeWranglerConfig({ main: "index.js" }); + fs.writeFileSync("index.js", "export default {};"); + + await runWrangler("deploy --dry-run --outdir out"); + + expect(std.debug).toContain('"sanitizedCommand":"deploy"'); + expect(std.debug).toContain('"path":null'); + }); + + it("categorises a file entry-point as 'file'", async ({ expect }) => { + writeWranglerConfig(); + fs.writeFileSync("index.js", "export default {};"); + + await runWrangler("deploy index.js --dry-run --outdir out"); + + expect(std.debug).toContain('"sanitizedCommand":"deploy"'); + expect(std.debug).toContain('"path":"file"'); + // The raw path is never sent in the categorised property. + expect(std.debug).not.toContain('"path":"index.js"'); + }); + + it("categorises a directory as 'directory'", async ({ expect }) => { + writeWranglerConfig({ name: "test-name" }); + fs.mkdirSync("public"); + fs.writeFileSync("public/index.html", "

Hello

"); + + await runWrangler("deploy public --dry-run --outdir out --no-autoconfig"); + + expect(std.debug).toContain('"path":"directory"'); + }); + + it("categorises the current directory reference as 'current-dir'", async ({ + expect, + }) => { + writeWranglerConfig({ name: "test-name" }); + fs.writeFileSync("index.html", "

Hello

"); + + await runWrangler("deploy . --dry-run --outdir out --no-autoconfig"); + + expect(std.debug).toContain('"path":"current-dir"'); + }); + + it("categorises a parent-relative reference as 'parent-relative'", async ({ + expect, + }) => { + writeWranglerConfig(); + + // The path does not need to resolve — the started event (which fires + // before the handler) already carries the category. The handler then + // fails because the entry-point cannot be found. + await expect( + runWrangler("deploy ../missing-entry.js --dry-run --outdir out") + ).rejects.toThrow(); + + expect(std.debug).toContain('"path":"parent-relative"'); + }); + }); }); diff --git a/packages/wrangler/src/__tests__/metrics/sanitization.test.ts b/packages/wrangler/src/__tests__/metrics/sanitization.test.ts index bb5307d905..a06e0ad3d6 100644 --- a/packages/wrangler/src/__tests__/metrics/sanitization.test.ts +++ b/packages/wrangler/src/__tests__/metrics/sanitization.test.ts @@ -1,6 +1,10 @@ +import * as fs from "node:fs"; +import { runInTempDir } from "@cloudflare/workers-utils/test-helpers"; import { describe, it } from "vitest"; import { ALLOW, + categoriseArgs, + categorisePositionalPath, COMMAND_ARG_ALLOW_LIST, getAllowedArgs, REDACT, @@ -136,6 +140,92 @@ describe("getAllowedArgs", () => { }); }); +describe("categorisePositionalPath", () => { + runInTempDir(); + + it("returns null when no value is provided", ({ expect }) => { + expect(categorisePositionalPath(undefined)).toBeNull(); + expect(categorisePositionalPath("")).toBeNull(); + expect(categorisePositionalPath(123)).toBeNull(); + }); + + it("categorises the current directory reference", ({ expect }) => { + expect(categorisePositionalPath(".")).toBe("current-dir"); + expect(categorisePositionalPath("./")).toBe("current-dir"); + }); + + it("categorises parent-relative references", ({ expect }) => { + expect(categorisePositionalPath("..")).toBe("parent-relative"); + expect(categorisePositionalPath("../example")).toBe("parent-relative"); + expect(categorisePositionalPath("../../dist")).toBe("parent-relative"); + }); + + it("categorises an existing directory", ({ expect }) => { + fs.mkdirSync("./public"); + expect(categorisePositionalPath("./public")).toBe("directory"); + expect(categorisePositionalPath("public")).toBe("directory"); + }); + + it("categorises an existing file", ({ expect }) => { + fs.writeFileSync("./index.js", "export default {};"); + expect(categorisePositionalPath("./index.js")).toBe("file"); + expect(categorisePositionalPath("index.js")).toBe("file"); + }); + + it("categorises a path that does not exist as not-found", ({ expect }) => { + expect(categorisePositionalPath("does-not-exist")).toBe("not-found"); + expect(categorisePositionalPath("src/missing.ts")).toBe("not-found"); + }); +}); + +describe("categoriseArgs", () => { + runInTempDir(); + + it("only categorises args whose allow-list entry is a categoriser", ({ + expect, + }) => { + fs.mkdirSync("./public"); + const allowedArgs: AllowedArgs = { + path: categorisePositionalPath, + force: ALLOW, + config: REDACT, + }; + const result = categoriseArgs( + { path: "./public", force: true, config: "wrangler.toml" }, + allowedArgs + ); + expect(result).toEqual({ path: "directory" }); + }); + + it("reads positional values straight from the full args object", ({ + expect, + }) => { + // `path` here mirrors the positional set by yargs, which sanitizeArgKeys + // would otherwise drop because it is not passed as `--path` in argv. + const result = categoriseArgs( + { path: "../example" }, + { path: categorisePositionalPath } + ); + expect(result).toEqual({ path: "parent-relative" }); + }); + + it("records null when a positional is absent", ({ expect }) => { + const result = categoriseArgs( + { path: undefined }, + { path: categorisePositionalPath } + ); + expect(result).toEqual({ path: null }); + }); + + it("omits args when the categoriser returns undefined", ({ expect }) => { + const result = categoriseArgs( + { path: "anything" }, + { path: () => undefined } + ); + expect(result).toEqual({}); + }); +}); + describe("COMMAND_ARG_ALLOW_LIST", () => { it("should pass boolean flag values through the full sanitisation pipeline for any command", ({ expect, diff --git a/packages/wrangler/src/__tests__/versions/versions.upload.test.ts b/packages/wrangler/src/__tests__/versions/versions.upload.test.ts index 1230b67a25..886e42c07b 100644 --- a/packages/wrangler/src/__tests__/versions/versions.upload.test.ts +++ b/packages/wrangler/src/__tests__/versions/versions.upload.test.ts @@ -16,6 +16,7 @@ import { http, HttpResponse } from "msw"; * TODO: remove this `expect` import */ import { beforeEach, describe, expect, it, test, vi } from "vitest"; +import { logger } from "../../logger"; import * as metrics from "../../metrics"; import { dedent } from "../../utils/dedent"; import { makeApiRequestAsserter } from "../helpers/assert-request"; @@ -1746,6 +1747,23 @@ describe("versions upload", () => { expect(std.out).toContain("MY_KV"); expect(std.out).toContain("--dry-run: exiting now."); }); + + test("categorises the positional path in command telemetry", async ({ + expect, + }) => { + logger.loggerLevel = "debug"; + writeWranglerConfig({ name: "test-name" }); + writeWorkerSource(); + + try { + await runWrangler("versions upload index.js --dry-run"); + } finally { + logger.resetLoggerLevel(); + } + + expect(std.debug).toContain('"sanitizedCommand":"versions upload"'); + expect(std.debug).toContain('"path":"file"'); + }); }); // --no-bundle, --var/--define/--alias, annotations, non-versioned fields, diff --git a/packages/wrangler/src/core/register-yargs-command.ts b/packages/wrangler/src/core/register-yargs-command.ts index 00842a5d9b..e5adc5a6c0 100644 --- a/packages/wrangler/src/core/register-yargs-command.ts +++ b/packages/wrangler/src/core/register-yargs-command.ts @@ -27,6 +27,7 @@ import { isNonInteractiveOrCI } from "../is-interactive"; import { logger } from "../logger"; import { getMetricsDispatcher } from "../metrics"; import { + categoriseArgs, COMMAND_ARG_ALLOW_LIST, getAllowedArgs, sanitizeArgKeys, @@ -295,10 +296,12 @@ function createHandler(def: InternalCommandDefinition, argv: string[]) { sanitizedCommand ); const argsWithSanitizedKeys = sanitizeArgKeys(args, argv); - const sanitizedArgs = sanitizeArgValues( - argsWithSanitizedKeys, - allowedArgs - ); + const sanitizedArgs = { + ...sanitizeArgValues(argsWithSanitizedKeys, allowedArgs), + // Categorised positional args (e.g. the deploy path) are added + // separately because positionals are excluded by sanitizeArgKeys. + ...categoriseArgs(args, allowedArgs), + }; const argsUsed = Object.keys(argsWithSanitizedKeys).sort(); dispatcher.sendCommandEvent( diff --git a/packages/wrangler/src/metrics/sanitization.ts b/packages/wrangler/src/metrics/sanitization.ts index 12e086b002..a4e27a4f46 100644 --- a/packages/wrangler/src/metrics/sanitization.ts +++ b/packages/wrangler/src/metrics/sanitization.ts @@ -1,3 +1,4 @@ +import { statSync } from "node:fs"; import { UserError } from "@cloudflare/workers-utils"; import { getErrorType } from "../core/handle-errors"; @@ -30,11 +31,51 @@ export function sanitizeArgKeys( export const REDACT = Symbol("REDACT"); /** Allow all values for this arg. */ export const ALLOW = Symbol("ALLOW"); +/** + * Transform this arg's value into a coarse, non-identifying category label + * instead of logging it raw. Return `null` to record that the arg was absent, + * or `undefined` to omit the arg entirely. + * + * Categorisers are the safe way to gather telemetry about high-cardinality or + * potentially sensitive values (such as file paths) — only the returned label + * is ever sent, never the original value. Unlike the other allow-list values, + * categorisers are also applied to positional args (see `categoriseArgs`). + */ +export type Categoriser = (value: unknown) => string | null | undefined; export type AllowedArgs = { - [arg: string]: unknown[] | typeof REDACT | typeof ALLOW; + [arg: string]: unknown[] | typeof REDACT | typeof ALLOW | Categoriser; }; export type AllowList = Record; +/** + * Categorises a positional path argument (e.g. the entry-point or assets path + * passed to `wrangler deploy` / `wrangler versions upload`) into a coarse, + * non-identifying label. + * + * The raw value is never returned — paths can leak sensitive information and + * are too high-cardinality to analyse. Relational references (`.`, `..`) are + * surfaced as their own categories; anything else is resolved against the + * filesystem to distinguish files from directories, mirroring how the deploy + * path positional is actually interpreted. Returns `null` when no positional + * was provided so the property is always present in telemetry. + */ +export function categorisePositionalPath(value: unknown): string | null { + if (typeof value !== "string" || value.length === 0) { + return null; + } + if (value === "." || value === "./" || value === ".\\") { + return "current-dir"; + } + if (value === ".." || value.startsWith("../") || value.startsWith("..\\")) { + return "parent-relative"; + } + try { + return statSync(value).isDirectory() ? "directory" : "file"; + } catch { + return "not-found"; + } +} + /** * A list of all the command args that are allowed. * @@ -42,10 +83,12 @@ export type AllowList = Record; * The top level "*" applies to all commands. * Specific commands can override or add to the allow list. * - * Each arg can have one of three values: + * Each arg can have one of four values: * - an array of strings: only those specific values are allowed * - REDACT: the arg value will always be redacted * - ALLOW: all values for that arg are allowed + * - a Categoriser function: the value is transformed into a coarse category + * label (also applied to positional args) */ export const COMMAND_ARG_ALLOW_LIST: AllowList = { // * applies to all commands. @@ -98,6 +141,12 @@ export const COMMAND_ARG_ALLOW_LIST: AllowList = { }, deploy: { containersRollout: ["immediate", "gradual"], + // Categorise the entry-point/assets positional without logging the raw path. + path: categorisePositionalPath, + }, + "versions upload": { + // `versions upload` shares the same positional path semantics as `deploy`. + path: categorisePositionalPath, }, }; @@ -155,6 +204,33 @@ export function sanitizeArgValues( return result; } +/** + * Applies categoriser transforms from the allow-list to the raw args. + * + * Unlike {@link sanitizeArgValues}, this reads from the *full* args object + * rather than the argv-filtered set produced by {@link sanitizeArgKeys}. That + * is what allows positional args — which are otherwise deliberately excluded + * from telemetry — to be safely categorised into a fixed label. Only args whose + * allow-list entry is a {@link Categoriser} are considered, and only the label + * it returns (never the raw value) is emitted. A `null` result is recorded as-is + * (the arg was absent); an `undefined` result omits the arg entirely. + */ +export function categoriseArgs( + args: Record, + allowedArgs: AllowedArgs +): Record { + const result: Record = {}; + for (const [key, allowed] of Object.entries(allowedArgs)) { + if (typeof allowed === "function") { + const category = allowed(args[key]); + if (category !== undefined) { + result[key] = category; + } + } + } + return result; +} + /** * Returns the canonical argument name for metrics reporting. */ diff --git a/packages/wrangler/src/metrics/types.ts b/packages/wrangler/src/metrics/types.ts index 88c64f8e7d..fbdf75fdf3 100644 --- a/packages/wrangler/src/metrics/types.ts +++ b/packages/wrangler/src/metrics/types.ts @@ -103,6 +103,12 @@ type CommandEventProperties = CommonCommandEventProperties & { * the sanitized args would only contain `{ format: "json" }` since that is the only allowed value. * The `` positional argument and `--namespace-id` flag would be omitted since they are not allowed. * + * Positional args are normally excluded entirely, but an allow-list entry may + * define a categoriser that maps a positional value to a coarse, non-identifying + * label (never the raw value). For example, `wrangler deploy ./dist` records + * `{ path: "directory" }`, while `wrangler deploy` with no positional records + * `{ path: null }`. + * * It is named `sanitizedArgs` to distinguish it from the historical `args` field which * may have contained sensitive data in older Wrangler versions. */