fix(wrangler): add safe command/args handling for telemetry#12063
fix(wrangler): add safe command/args handling for telemetry#12063petebacondarwin wants to merge 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 10b86ae The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
| let dispatcher: ReturnType<typeof getMetricsDispatcher> | undefined; | ||
|
|
||
| // Register middleware to capture command info for fallback telemetry | ||
| const wranglerWithTelemetry = wranglerWithMiddleware.middleware((args) => { | ||
| // Capture command and args for potential fallback telemetry | ||
| // (used when yargs validation errors occur before handler runs) | ||
| command = `wrangler ${args._.join(" ")}`; | ||
| metricsArgs = args; | ||
| safeCommand = args._.join(" "); |
There was a problem hiding this comment.
@MattieTK - do we want this just to be "" since we cannot guarantee that the args don't contain something sensitive?
3fd5325 to
7e9344a
Compare
bd603d8 to
2e40a31
Compare
a073093 to
db843d1
Compare
3ad4dd8 to
5a78386
Compare
7dee593 to
1360eb5
Compare
5a78386 to
8897b68
Compare
54b538b to
84bf670
Compare
vicb
left a comment
There was a problem hiding this comment.
Nice.
I added some comments.
I also see that Devin has comments, have those been addressed?
| * The command that was used, e.g. `dev`. | ||
| * Does not include the "wrangler" prefix. |
There was a problem hiding this comment.
future proof this in case wrangler is renamed
| * The command that was used, e.g. `dev`. | |
| * Does not include the "wrangler" prefix. | |
| * The command that was used, e.g. `dev` without the binary name |
There was a problem hiding this comment.
wrangler is used throughout this file, including the names of the events; so it will need a full overhaul if we change the name. "binary" is less clear.
| * When logArgs is false, positional arguments are stripped to prevent | ||
| * accidentally capturing secrets in telemetry. |
There was a problem hiding this comment.
It's nice to see what's stripped, could we also maybe describe what's kept?
| * When logArgs is false, positional arguments are stripped to prevent | |
| * accidentally capturing secrets in telemetry. | |
| * When `logArgs` is `false`, positional arguments are stripped to prevent | |
| * accidentally capturing secrets in telemetry. |
| * Named `safeArgs` to distinguish from historical `args` field which | ||
| * may have contained sensitive data in older Wrangler versions. |
There was a problem hiding this comment.
IMO this is noise and not needed
| * Named `safeArgs` to distinguish from historical `args` field which | |
| * may have contained sensitive data in older Wrangler versions. |
There was a problem hiding this comment.
I think this is actually really useful context for why we are not just using command and args here.
| * Named `safeCommand` to distinguish from historical `command` field which | ||
| * may have contained sensitive positional arguments in older Wrangler versions. |
There was a problem hiding this comment.
IMO this is noise and not needed
| * Named `safeCommand` to distinguish from historical `command` field which | |
| * may have contained sensitive positional arguments in older Wrangler versions. |
| * Named `safeCommand` to distinguish from historical `command` field which | ||
| * may have contained sensitive positional arguments in older Wrangler versions. | ||
| */ | ||
| safeCommand: string; |
There was a problem hiding this comment.
Telemetry is the last thing that comes to mind about a "safe" command.
Would "sanitized" be a better prefix?
There was a problem hiding this comment.
I agree. I'll change the names.
| * The args and flags that were passed in when running the command. | ||
| * All user-inputted string values are redacted, except for some cases where there are set options. |
There was a problem hiding this comment.
Maybe add an example to ease the understanding?
I see const safeArgs = logArgs ? args : {}; in register-yargs-command.ts
So if my understanding is correct:
- "When logArgs is false, this is an empty object." that's true
- WHen logArs is true, some sanitization is going on, can we add ref to the relevant code/config (
@see ...)
| * Named `safeArgs` to distinguish from historical `args` field which | ||
| * may have contained sensitive data in older Wrangler versions. | ||
| */ | ||
| safeArgs: Record<string, unknown>; |
There was a problem hiding this comment.
Same comment for "safe" that for safeCommand
| * If true, arguments for this command will be included in telemetry. | ||
| * | ||
| * @default false - Arguments are not logged by default. | ||
| * Set to `true` to explicitly include this command's args in telemetry. |
There was a problem hiding this comment.
| * If true, arguments for this command will be included in telemetry. | |
| * | |
| * @default false - Arguments are not logged by default. | |
| * Set to `true` to explicitly include this command's args in telemetry. | |
| * Whether arguments for this command will be posted to telemetry. | |
| * | |
| * @default false - Arguments are not logged by default. |
| // Truncate login commands to just "login" to avoid capturing tokens | ||
| if (properties.safeCommand?.startsWith("login")) { | ||
| properties.safeCommand = "login"; | ||
| } | ||
| // Don't send metrics for telemetry/metrics disable commands | ||
| if ( | ||
| properties.command === "wrangler telemetry disable" || | ||
| properties.command === "wrangler metrics disable" | ||
| properties.safeCommand === "telemetry disable" || | ||
| properties.safeCommand === "metrics disable" | ||
| ) { | ||
| return; | ||
| } | ||
| // Show metrics banner for certain commands | ||
| if ( | ||
| properties.command === "wrangler deploy" || | ||
| properties.command === "wrangler dev" || | ||
| properties.safeCommand === "deploy" || | ||
| properties.safeCommand === "dev" || | ||
| // for testing purposes | ||
| properties.command === "wrangler docs" | ||
| properties.safeCommand === "docs" | ||
| ) { |
1360eb5 to
8676957
Compare
6e57b40 to
b033763
Compare
8676957 to
1775cf4
Compare
9b91d8a to
f99030b
Compare
| description: "Write a single key/value pair to the given namespace", | ||
| status: "stable", | ||
| owner: "Product: KV", | ||
| logArgs: true, | ||
| }, |
There was a problem hiding this comment.
🔴 Sensitive KV write commands now opt-in to telemetry args logging via metadata.logArgs: true
kv key put and kv bulk put are write operations that accept secret values (inline value or a JSON file containing values) and file paths. This PR’s stated goal is to avoid capturing sensitive data by requiring explicit opt-in for arg logging, but these commands are explicitly opted in.
Actual: Wrangler will include argsUsed/argsCombination and may include allowed args values depending on allow-list; at minimum it records that flags like --path, --metadata, etc. were used, and it can accidentally include values if they become allow-listed later.
Expected: write/secret-bearing commands should keep metadata.logArgs unset/false so telemetry args are always {}.
Impact: privacy/security regression—telemetry collection is re-enabled for commands that often deal with secrets and sensitive file paths.
Click to expand
Relevant opt-in additions:
kvKeyPutCommandsetslogArgs: truein metadata:packages/wrangler/src/kv/index.ts:409-414kvBulkPutCommandsetslogArgs: truein metadata:packages/wrangler/src/kv/index.ts:880-885
Telemetry plumbing includes args when logArgs is true: packages/wrangler/src/core/register-yargs-command.ts:223-232
(Refers to lines 409-414)
Recommendation: Remove logArgs: true from KV write commands (at least kv key put and kv bulk put). Only opt in for read-only/non-sensitive commands, and consider adding a test asserting logArgs is false and sanitizedArgs is {} for these commands.
Was this helpful? React with 👍 or 👎 to provide feedback.
| description: "Enable Sippy on an R2 bucket", | ||
| status: "stable", | ||
| owner: "Product: R2", | ||
| logArgs: true, | ||
| }, |
There was a problem hiding this comment.
🔴 r2 bucket sippy enable opts in to telemetry args despite taking access keys and credentials
wrangler r2 bucket sippy enable takes highly sensitive credentials (--access-key-id, --secret-access-key, --r2-access-key-id, --r2-secret-access-key, and GCS private key material). This PR intends to prevent accidental capture of secrets, but the command is explicitly opted in to arg logging.
Actual: Since metadata.logArgs is true, Wrangler will send argsUsed/argsCombination derived from the args and may include values if allow-listed later.
Expected: This command should not log args (leave logArgs unset/false).
Impact: privacy/security regression; telemetry starts recording usage of secret-bearing flags for Sippy enable.
Click to expand
Opt-in added here: packages/wrangler/src/r2/sippy.ts:21-26.
The command defines credential flags immediately below (access-key-id, secret-access-key, etc.): packages/wrangler/src/r2/sippy.ts:51-80.
(Refers to lines 21-26)
Recommendation: Remove logArgs: true from r2BucketSippyEnableCommand (and any other commands that accept credentials/secret material). Add/extend tests to ensure logArgs remains false for these commands.
Was this helpful? React with 👍 or 👎 to provide feedback.
| owner: "Workers: Authoring and Testing", | ||
| status: "stable", | ||
| category: "Account", | ||
| logArgs: true, | ||
| }, |
There was a problem hiding this comment.
🔴 wrangler login is opted in to telemetry args logging contrary to privacy requirements
This PR’s description explicitly calls out wrangler login as a command that should no longer collect telemetry on argument usage. However, loginCommand is marked with metadata.logArgs: true.
Actual: register-yargs-command will pass full parsed args as sanitizedArgs when logArgs is true (packages/wrangler/src/core/register-yargs-command.ts:223-232). While metrics-dispatcher truncates sanitizedCommand to "login", it still processes and sends sanitized args/flags usage for the login command.
Expected: wrangler login should have logArgs unset/false so sanitizedArgs is always {}.
Impact: privacy regression: argument usage telemetry is still recorded for login.
Click to expand
Opt-in added in login metadata: packages/wrangler/src/user/commands.ts:24-30.
(Refers to lines 24-30)
Recommendation: Remove logArgs: true from loginCommand metadata. Consider adding a unit test verifying sanitizedArgs is {}/argsCombination is empty for wrangler login events.
Was this helpful? React with 👍 or 👎 to provide feedback.
- Rename 'command' to 'safeCommand' (without 'wrangler ' prefix) and 'args' to 'safeArgs' in telemetry events - Add 'logArgs' boolean to control whether command arguments are included in telemetry - Commands must explicitly opt-in via metadata.logArgs: true to log arguments - Safe commands (list, info, get, etc.) that don't handle sensitive data opt-in to logging - Sensitive commands (secret put/delete/bulk, hyperdrive create) intentionally do not opt-in - Update fallback telemetry in index.ts to use new format with logArgs: false
… telemetry Move addBreadcrumb call into the command handler where we have access to the safe commandName from the command definition, rather than deriving it from args._ which may contain sensitive user-supplied positional arguments. Remove safeCommand variable from fallback telemetry since we cannot safely derive it from args._. The dispatchGenericCommandErrorEvent now uses empty string for safeCommand.
f99030b to
10b86ae
Compare
| owner: "Workers: Authoring and Testing", | ||
| status: "stable", | ||
| category: "Account", | ||
| logArgs: true, | ||
| }, |
There was a problem hiding this comment.
🔴 Sensitive wrangler login command is incorrectly opted-in to argument telemetry via logArgs: true
wrangler login was intended (per PR description/changeset) to not collect any argument usage telemetry, but it is explicitly opted-in by setting metadata.logArgs: true.
Actual behavior: when wrangler login runs, createHandler() will set logArgs = true and pass the full args object to telemetry (sanitizedArgs = args) (packages/wrangler/src/core/register-yargs-command.ts:223-232). The metrics dispatcher then derives argsUsed/argsCombination from the sanitized arg keys, meaning argument usage is still collected (even if values are later dropped/redacted).
Expected behavior: wrangler login should have metadata.logArgs unset/false so that sanitizedArgs is {} and no argument usage is collected.
Impact: privacy/telemetry policy regression for a security-sensitive auth command; could leak presence of particular flags/options and violates the explicit policy described in the PR.
Click to expand
Relevant code:
loginCommandopts in:packages/wrangler/src/user/commands.ts:25-30- Opt-in causes args to be sent into telemetry pipeline:
packages/wrangler/src/core/register-yargs-command.ts:223-232
(Refers to lines 25-30)
Recommendation: Remove logArgs: true from loginCommand metadata (or explicitly set logArgs: false) so wrangler login sends empty sanitizedArgs and collects no argument usage.
Was this helpful? React with 👍 or 👎 to provide feedback.
| description: "Upload an mTLS certificate", | ||
| owner: "Product: SSL", | ||
| status: "stable", | ||
| logArgs: true, | ||
| }, |
There was a problem hiding this comment.
🔴 wrangler mtls-certificate upload is incorrectly opted-in to argument telemetry (captures file-path flag usage)
mTlsCertificateUploadCommand sets metadata.logArgs: true even though it takes certificate/private-key file paths (--cert, --key). The changeset states that commands with file paths that could reveal sensitive structure should not collect argument usage telemetry.
Actual behavior: with logArgs enabled, Wrangler will include argsUsed/argsCombination for this command, indicating the use of file path flags. While values may be redacted/dropped later, the command is still opted into argument usage telemetry.
Expected behavior: logArgs should be false so telemetry does not capture argument usage for file-path-bearing commands.
Impact: privacy regression (argument usage telemetry for filesystem-path-bearing commands).
Click to expand
Relevant code:
- Opt-in:
packages/wrangler/src/mtls-certificate/cli.ts:14-20
(Refers to lines 14-20)
Recommendation: Remove logArgs: true from mTlsCertificateUploadCommand metadata so it does not collect argument usage telemetry.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Looking at this code after the refactorings, we don't actually need the |
This PR improves telemetry safety by introducing explicit control over which command arguments are included in telemetry events, preventing accidental capture of sensitive data like secrets and credentials.
Replaces #11856
Builds on top of #12069 and Builds on top of #12071
Renamed telemetry fields for clarity:
Added logArgs control:
Opted-in safe commands (~100 commands) that don't handle sensitive input:
Intentionally excluded sensitive commands:
Tests
Public documentation
A picture of a cute animal (not mandatory, but encouraged)