-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[wrangler] Sanitize sensitive commands in telemetry to prevent capturing secrets #11856
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 9e51120 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: |
| // Sanitize sensitive commands to prevent accidentally capturing secrets | ||
| // or credentials that users may have pasted as arguments | ||
| const sensitiveCommandPrefixes = [ | ||
| "wrangler login", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem with this implementation is that we duplicate the info in the command definition and here.
Meaning that if we tweak the command format or add a new command, we also have to update this code and there are chances we will miss that.
IMO a better implementation would be to move that to the command definition itself in some form.
Also we should probably remove "wrangler" from properties.command. Who knows, it might change some day ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Vic! That's a good point. So your idea would be that each command would have a property that marks the following args as potentially sensitive and skips them, and make the analytics aware of that?
Agree with ridding ourselves of wrangler in the properties.command, but the challenge with all logging data is that once it's there it's there and you kinda just have to move forward with your mistakes else you'll be forever adding additional filters to catch users on older versions, and then you'll forget a filter etc etc. Though I guess we're going to see that if the command changes anyway...
Instead of maintaining a hardcoded list of sensitive command prefixes in the metrics dispatcher, this adds a sensitiveArgs property to the command definition metadata. This ensures: - No duplication between command definitions and metrics code - New sensitive commands only need sensitiveArgs: true in their definition - The command registry is the single source of truth Changes: - Add sensitiveArgs to Metadata type in core/types.ts - Add findCommandDefinition() to CommandRegistry for looking up commands - Pass sensitiveArgs flag from command definition to sendCommandEvent() - Remove hardcoded prefix list from metrics-dispatcher.ts - Add sensitiveArgs: true to login, secret put/bulk, pages secret put/bulk, and versions secret put/bulk commands
| expect(std.debug).toContain('"command":"wrangler secret put'); | ||
| expect(std.debug).toContain('"sensitiveArgs":true'); | ||
| // Ensure the accidentally pasted secret is not in the debug output | ||
| expect(std.debug).not.toContain("accidentallyPastedSecret"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to make sure i'm clear on what is an arg and what is a flag and what is a value
what would happen in the test case npx wrangler secret put ACTUAL_SECRET RANDOM_POSITIONAL --foo=bar? do we still get --foo=bar somehow in telemetry or is that dropped too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we drop everything in this case. I will live to regret this but I think it's more important that we drop possibly useful data than capture something that we shouldn't capture. In the future if we want to add explicit checks for specific arguments in we can do that.
…nsitivity default - Renamed 'command' to 'safe_command' and 'args' to 'safe_args' to allow server-side differentiation from historical potentially-leaked data - Changed sensitiveArgs default from false to true (opt-in for sending args) - Fixed CommandRegistry.findCommandDefinition() to follow command aliases - Marked ~80 safe commands with sensitiveArgs: false (list, info, get, etc.) - Commands handling secrets remain sensitive by default (secret put, etc.)
- Strip 'wrangler' prefix from safe_command for future-proofing (e.g., 'secret put' instead of 'wrangler secret put') - Update COMMAND_ARG_ALLOW_LIST keys accordingly - Remove temporary audit document
| - Commands with file paths that could reveal sensitive structure | ||
| - Any command not explicitly marked as safe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure it hasn't because -c (config file path) is a global flag
Co-authored-by: emily-shen <69125074+emily-shen@users.noreply.github.com>
| // or credentials that users may have pasted as arguments. | ||
| // The sensitiveArgs flag is set from the command definition's metadata. | ||
| if (properties.sensitiveArgs) { | ||
| argv = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emily-shen I think this is the line that should remove global flags also if the sensitiveArgs property is false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
soz what i meant is that every command is a command that could have file paths, given -c is a global arg. so we shouldn't include that in the changeset list (also, i think its an llm implementation detail, what we should include is the actual commands affected)
Claude went a bit over the top here and committed classically hyperbolic language, so I'm toning it down.
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my inline comments.
I'm pretty sure there is better way that findCommandDefinition, let me know if you want me to look at it.
| * Returns the command definition and the number of args segments that form the command | ||
| * (excluding positional args), or undefined if no command is found. | ||
| */ | ||
| findCommandDefinition( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIS?
How are other Metadata retrieved?
packages/wrangler/src/core/types.ts
Outdated
| * @default true - Commands are treated as sensitive by default (args stripped). | ||
| * Set to `false` to explicitly mark a command as safe and include its args in telemetry. | ||
| */ | ||
| sensitiveArgs?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logArgs might be easier to understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, I've got a new version that swaps to logArgs with false being the default
Co-authored-by: lrapoport-cf <107272160+lrapoport-cf@users.noreply.github.com>
Summary
Sanitizes sensitive commands in telemetry to prevent accidentally capturing secrets or credentials that users may have pasted as arguments.
wrangler loginsanitization into a unified approach with other sensitive commandswrangler secret put,wrangler secret bulk, and theirpagesandversionsequivalents