-
Notifications
You must be signed in to change notification settings - Fork 45
fix: redact sensitive tokens from invoke CLI output #1419
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?
Changes from all 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 |
|---|---|---|
|
|
@@ -87,11 +87,20 @@ async function handleInvokeCLI(options: InvokeOptions, preloadedContext?: Invoke | |
| } | ||
| } | ||
|
|
||
| function redactSensitiveText(value: string): string { | ||
| return value | ||
| .replace(/(bearer\s+)[a-z0-9\-._~+/]+=*/gi, '$1[REDACTED]') | ||
| .replace(/(client[_-]?secret\s*[:=]\s*)([^,\s]+)/gi, '$1[REDACTED]') | ||
| .replace(/(token\s*[:=]\s*)([^,\s]+)/gi, '$1[REDACTED]'); | ||
| } | ||
|
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. No tests for This is a security-sensitive helper that's easy to get subtly wrong (see the JSON-shape comment above — a unit test would have caught it immediately). A small table-driven test in
would be enough to lock down the behavior and prevent regressions like the one this PR is fixing. |
||
|
|
||
| function printInvokeResult(result: InvokeResult, options: InvokeOptions): void { | ||
| if (options.json) { | ||
| console.log(JSON.stringify(serializeResult(result))); | ||
| const serialized = serializeResult(result); | ||
| if (typeof serialized.response === 'string') serialized.response = redactSensitiveText(serialized.response); | ||
| if (typeof serialized.error === 'string') serialized.error = redactSensitiveText(serialized.error); | ||
| console.log(JSON.stringify(serialized)); | ||
| } else if (options.stream) { | ||
| // Streaming already wrote to stdout, just show session and log path | ||
| if (result.sessionId) { | ||
| console.error(`\nSession: ${result.sessionId}`); | ||
| console.error(`To resume: agentcore invoke --session-id ${result.sessionId}`); | ||
|
|
@@ -100,11 +109,10 @@ function printInvokeResult(result: InvokeResult, options: InvokeOptions): void { | |
| console.error(`Log: ${result.logFilePath}`); | ||
| } | ||
| } else { | ||
| // Non-streaming, non-json: print provider info and response or error | ||
| if (result.success && result.response) { | ||
| console.log(result.response); | ||
| console.log(redactSensitiveText(result.response)); | ||
| } else if (!result.success && result.error) { | ||
| console.error(result.error.message); | ||
| console.error(redactSensitiveText(result.error.message)); | ||
| } | ||
| if (result.sessionId) { | ||
| console.error(`\nSession: ${result.sessionId}`); | ||
|
|
||
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.
Regexes don't match JSON-quoted forms — the most common shape this PR is meant to cover.
The
client_secretandtokenpatterns requiresecret/tokento be followed (after optional whitespace) by a literal:or=. In JSON output (the PR's stated motivating case —--json), the actual shape is:There's a
"between the key and the:, so\s*[:=]\s*does not match and these values are not redacted. Quick check:This means agent responses or upstream error bodies that echo headers/tokens as JSON (a realistic case for misbehaving harnesses or 4xx response bodies) still leak.
Options:
"(or any non-word char) between the key and the separator, e.g./(client[_-]?secret["']?\s*[:=]\s*["']?)([^"',\s}]+)/giand similarly fortoken.key=value/key: valueones.options.bearerTokenis set, do a literalreplaceAll(options.bearerToken, '[REDACTED]')in addition to the heuristic regexes). This is the most reliable for the bearer case and is independent of the surrounding syntax.Whichever path you take, please widen the value character class so the closing
"/}aren't consumed as part of the redacted value (the current[^,\s]+would swallow them when no comma/whitespace follows).