refactor(wrangler): add safe command/args handling for telemetry #12153
Conversation
🦋 Changeset detectedLatest commit: 34c0bfc 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: |
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
Nice simplification!
I added a couple comments inline
|
Rewritten the sanitizedArgs code to make it less scary and more clear where the sanitization should happen. PTAL @vicb |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
Nice, thanks!
Some of my comments could probably be addressed in a follow up PR.
Could you please create/update an issue for the minor comments you decide not to address here?
| argsWithSanitizedKeys, | ||
| allowedArgs | ||
| ); | ||
| const argsUsed = Object.keys(argsWithSanitizedKeys).sort(); |
There was a problem hiding this comment.
I am not fan of "argsUsed" because I find this too generic.
What about "userSuppliedArgs" or something similar.
I would also be nice to figure out a place to document it, either in the JSDoc of sendCommandEvent or a newly introduce type?
There was a problem hiding this comment.
argsUsed is the property sent in the metrics event.
I don't think this refactor is the right place to discuss that.
#12192
There was a problem hiding this comment.
Reopening...
argsUsed is the property sent in the metrics event.
It doesn't mean we can not use a clearer and meaningful name in the code, does it?
The only place where we "have to" use this name is when sending the payload
There was a problem hiding this comment.
It can be dealt with in the linked issue.
There was a problem hiding this comment.
Reopening...
Sorry but isn't this newly introduced code?
I think it's fair to fix existing code later but I don't think introducing new "not great" code is.
There was a problem hiding this comment.
@vicb you concern at the start of this thread above is merely a naming/documentation one, which is itself entirely subjective.
The previous code looked like this:
const sanitizedArgs = sanitizeArgKeys(
properties.args ?? {},
options.argv
);
const sanitizedArgsKeys = Object.keys(sanitizedArgs).sort();
const commonEventProperties: CommonEventProperties = {
...
argsUsed: sanitizedArgsKeys,
argsCombination: sanitizedArgsKeys.join(", "),
...
};So you can see that all that has effectively happened is that sanitizedArgsKeys variable is now argsUsed.
This PR is therefore not new code, and is not reducing the quality of the code in any tangible meaningful way.
There was a problem hiding this comment.
We can discuss further on the linked issue, if you feel strongly about the naming of this variable.
There was a problem hiding this comment.
Your call here.
My point is that it is a shame to introduce code and create an issue to change it at the same time - knowing that the update would take approximately 30s.
petebacondarwin
force-pushed
the
pbd/wrangler/sanitized-metrics
branch
from
aea552b to
5e7c1f5
Compare
- Renames telemetry event fields so that we can distinguish them in the metric reporting tools: - 'command' -> 'safeCommand' (without 'wrangler ' prefix and containing no positional args at all) - 'args' -> 'safeArgs' (only including allowed args, with sanitized values) - Moves Sentry breadcrumbs for command start/error into the metrics dispatcher to ensure they don't include positional args.
Moved the sanitization to the command handling machinery rather than the metrics dispatcher. This makes it clear that the values passed to be dispatched must have already been sanitized.
… might be overwritten
petebacondarwin
force-pushed
the
pbd/wrangler/sanitized-metrics
branch
from
5e7c1f5 to
34c0bfc
Compare
2 participants
Sanitize commands and arguments in telemetry to prevent accidentally capturing sensitive information.
Changes:
command/argstosanitizedCommand/sanitizedArgsto distinguish from historical fields that may have contained sensitive data in older versionsCOMMAND_ARG_ALLOW_LISTReplaces fix(wrangler): add safe command/args handling for telemetry #12063, which added the redundant
logArgs: falsebehaviour. See fix(wrangler): add safe command/args handling for telemetry #12063 (comment)A picture of a cute animal (not mandatory, but encouraged)