-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: allowlist some types defaults #8130
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "wrangler": patch | ||
| --- | ||
|
|
||
| Include default values for wrangler types --path and --x-include-runtime in telemetry | ||
|
|
||
| User provided strings are still left redacted as always. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,12 +32,15 @@ export function getMetricsDispatcher(options: MetricsConfigOptions) { | |
| let amplitude_event_id = 0; | ||
|
|
||
| /** We redact strings in arg values, unless they are named here */ | ||
| const allowList = { | ||
| const allowList: Record<string, AllowedValues> & { "*": AllowedValues } = { | ||
| // applies to all commands | ||
| // use camelCase version | ||
| "*": ["format", "logLevel"], | ||
| // specific commands | ||
| tail: ["status"], | ||
| "*": { format: "*", logLevel: "*" }, | ||
| "wrangler tail": { status: "*" }, | ||
| "wrangler types": { | ||
| xIncludeRuntime: [".wrangler/types/runtime.d.ts"], | ||
| path: ["worker-configuration.d.ts"], | ||
| }, | ||
| }; | ||
|
|
||
| return { | ||
|
|
@@ -253,35 +256,47 @@ const sanitiseUserInput = ( | |
| return result; | ||
| }; | ||
|
|
||
| type AllowedValues = Record<string, string[] | "*">; | ||
| const getAllowedArgs = ( | ||
| allowList: Record<string, string[]> & { "*": string[] }, | ||
| allowList: Record<string, AllowedValues> & { "*": AllowedValues }, | ||
| key: string | ||
| ) => { | ||
| const commandSpecific = allowList[key] ?? []; | ||
| return [...commandSpecific, ...allowList["*"]]; | ||
| return { ...commandSpecific, ...allowList["*"] }; | ||
| }; | ||
| export const redactArgValues = ( | ||
| args: Record<string, unknown>, | ||
| allowedKeys: string[] | ||
| allowedValues: AllowedValues | ||
| ) => { | ||
| const result: Record<string, unknown> = {}; | ||
|
|
||
| for (const [k, value] of Object.entries(args)) { | ||
| const key = normalise(k); | ||
| for (let [key, value] of Object.entries(args)) { | ||
| key = normalise(key); | ||
| // the default is not set by yargs :/ | ||
| if (key === "xIncludeRuntime" && value === "") { | ||
| value = ".wrangler/types/runtime.d.ts"; | ||
| } | ||
| const allowedValuesForArg = allowedValues[key] ?? []; | ||
| if (exclude.has(key)) { | ||
| continue; | ||
| } | ||
| if ( | ||
| typeof value === "number" || | ||
| typeof value === "boolean" || | ||
| allowedKeys.includes(normalise(key)) | ||
| allowedValuesForArg.includes(normalise(key)) | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ) { | ||
| result[key] = value; | ||
| } else if (typeof value === "string") { | ||
| } else if ( | ||
| typeof value === "string" && | ||
| !(allowedValuesForArg === "*" || allowedValuesForArg.includes(value)) | ||
| ) { | ||
| result[key] = "<REDACTED>"; | ||
| } else if (Array.isArray(value)) { | ||
| result[key] = value.map((v) => | ||
| typeof v === "string" ? "<REDACTED>" : v | ||
| typeof v === "string" && | ||
| !(allowedValuesForArg === "*" || allowedValuesForArg.includes(v)) | ||
| ? "<REDACTED>" | ||
| : v | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add some comments explaining this logic? It's gotten pretty hairy—but be nice if there was a way to simplify
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah i'd like to tidy all of this up, because the sanitise + redact + normalise functions have gotten very redundant and messy. Would you be okay with leaving it for now so I can start collecting metrics, and doing that in a fast follow this afternooon?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. depends how long until the release really 😅
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good--let's get this in as-is for the current release |
||
| ); | ||
| } else { | ||
| result[key] = value; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.