-
Notifications
You must be signed in to change notification settings - Fork 653
refactor(sdk): remove credential redaction #322
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
dd61f0d
552c382
e8b9589
bf34227
c2d786b
68b11bf
a75ea4a
4de6057
179b234
ae623dc
6577fa6
9051e66
9fec366
be6012e
ef23ce1
1461d02
9e2a6e5
a444919
de6293a
124a84e
09a5e0b
a40e8c0
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 |
|---|---|---|
|
|
@@ -66,7 +66,7 @@ import { | |
| OutputDirectoryError, | ||
| OutputInsideProtectedRootError, | ||
| PluginPythonUnavailableError, | ||
| redactedErrorMessage, | ||
| errorMessage, | ||
| ScanCostLimitExceededError, | ||
| ScanInterruptedError, | ||
| } from "./errors.js"; | ||
|
|
@@ -715,7 +715,7 @@ export async function main( | |
| try { | ||
| return await select(await dependencies.runWorkbench(args)); | ||
| } catch (error) { | ||
| errorOutput.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| errorOutput.write(`codex-security: ${errorMessage(error)}\n`); | ||
| exitCode = 2; | ||
| return undefined; | ||
| } | ||
|
|
@@ -921,7 +921,7 @@ export async function main( | |
| scanArguments = scanArgumentsFromRecipe(recipe, args.scanId); | ||
| scanArguments.verbose = options.verbose; | ||
| } catch (error) { | ||
| const message = redactedErrorMessage(error); | ||
| const message = errorMessage(error); | ||
| errorOutput.write(`codex-security: ${message}\n`); | ||
| exitCode = 2; | ||
| return incurError({ | ||
|
|
@@ -984,7 +984,7 @@ export async function main( | |
| format, | ||
| ); | ||
| } catch (error) { | ||
| errorOutput.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| errorOutput.write(`codex-security: ${errorMessage(error)}\n`); | ||
| exitCode = 2; | ||
| return undefined; | ||
| } | ||
|
|
@@ -1037,7 +1037,7 @@ export async function main( | |
| verbose: z | ||
| .boolean() | ||
| .default(false) | ||
| .describe("Print redacted scan diagnostics to stderr."), | ||
| .describe("Print scan diagnostics to stderr."), | ||
| path: z | ||
| .array(optionValue("--path")) | ||
| .default([]) | ||
|
|
@@ -1298,7 +1298,7 @@ export async function main( | |
| failOnSeverity: options.failOnSeverity, | ||
| }; | ||
| } catch (error) { | ||
| errorOutput.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| errorOutput.write(`codex-security: ${errorMessage(error)}\n`); | ||
| exitCode = 2; | ||
| return undefined; | ||
| } | ||
|
|
@@ -1460,7 +1460,7 @@ export async function main( | |
| onProgress: ({ repository, status, attempt, error, warning }) => { | ||
| const detail = error ?? warning; | ||
| errorOutput.write( | ||
| `codex-security: ${repository} ${status} (attempt ${attempt})${detail === undefined ? "" : `: ${redactedErrorMessage(detail)}`}\n`, | ||
| `codex-security: ${repository} ${status} (attempt ${attempt})${detail === undefined ? "" : `: ${errorMessage(detail)}`}\n`, | ||
| ); | ||
| }, | ||
| }); | ||
|
|
@@ -1474,7 +1474,7 @@ export async function main( | |
| (error instanceof Error && error.name === "ExitPromptError" | ||
| ? 130 | ||
| : 2); | ||
| errorOutput.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| errorOutput.write(`codex-security: ${errorMessage(error)}\n`); | ||
| } finally { | ||
| dependencies.removeSignalListener("SIGINT", onInterrupt); | ||
| dependencies.removeSignalListener("SIGTERM", onTerminate); | ||
|
|
@@ -1578,7 +1578,7 @@ export async function main( | |
| ); | ||
| } catch (error) { | ||
| exitCode = 2; | ||
| errorOutput.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| errorOutput.write(`codex-security: ${errorMessage(error)}\n`); | ||
| } | ||
| }, | ||
| }) | ||
|
|
@@ -1614,7 +1614,7 @@ export async function main( | |
| ); | ||
| } catch (error) { | ||
| exitCode = 2; | ||
| errorOutput.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| errorOutput.write(`codex-security: ${errorMessage(error)}\n`); | ||
| } | ||
| }, | ||
| }) | ||
|
|
@@ -1803,7 +1803,7 @@ export async function main( | |
| if (frameworkExit !== undefined) { | ||
| if (exitCode !== 0) return exitCode; | ||
| errorOutput.write( | ||
| `codex-security: ${redactedErrorMessage(incurErrorMessage(frameworkOutput))}\n`, | ||
| `codex-security: ${errorMessage(incurErrorMessage(frameworkOutput))}\n`, | ||
| ); | ||
| return 2; | ||
| } | ||
|
|
@@ -1812,7 +1812,7 @@ export async function main( | |
| await writeCliOutput(output, renderedHistory ?? frameworkOutput); | ||
| return exitCode; | ||
| } catch (error) { | ||
| errorOutput.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| errorOutput.write(`codex-security: ${errorMessage(error)}\n`); | ||
| return 2; | ||
| } | ||
| } | ||
|
|
@@ -2533,20 +2533,18 @@ async function runExport( | |
| } | ||
| return 0; | ||
| } catch (error) { | ||
| errorOutput.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| errorOutput.write(`codex-security: ${errorMessage(error)}\n`); | ||
| return 2; | ||
| } | ||
| } | ||
|
|
||
| type VerboseDiagnosticValue = string | number | boolean | null | undefined; | ||
|
|
||
| function sanitizeDiagnosticValue(value: unknown): string { | ||
| return redactedErrorMessage(value) | ||
| .replaceAll( | ||
| /(\b(?:tenant(?:[_-]?id)?|org(?:anization)?(?:[_-]?id)?|project(?:[_-]?id)?|(?:x[_-]?)?(?:request|trace|correlation)[_-]?id)\b(?:\\*["'])?\s*[:=]\s*)(?!\[redacted\])(?:(\\*)(['"])(?:(?!(?<!\\)\2\3)(?:\\.|[^\\]))*(?:(?<!\\)\2\3|$)|[^\s"',;&}\]]+)/giu, | ||
| "$1$2$3[redacted]$2$3", | ||
| ) | ||
| .replaceAll(/[\u0000-\u001F\u007F\u0085\u2028\u2029]/gu, " "); | ||
| function diagnosticValue(value: unknown): string { | ||
| return errorMessage(value).replaceAll( | ||
| /[\u0000-\u001F\u007F\u0085\u2028\u2029]/gu, | ||
| " ", | ||
| ); | ||
| } | ||
|
|
||
| async function runScan( | ||
|
|
@@ -2589,7 +2587,7 @@ async function runScan( | |
| value === undefined | ||
| ? [] | ||
| : [ | ||
| `${name}=${JSON.stringify(typeof value === "string" ? sanitizeDiagnosticValue(value) : value)}`, | ||
| `${name}=${JSON.stringify(typeof value === "string" ? diagnosticValue(value) : value)}`, | ||
| ], | ||
| ); | ||
| writeAboveProgress(() => { | ||
|
|
@@ -2758,7 +2756,6 @@ async function runScan( | |
| : { maxCostUsd: arguments_.maxCostUsd }), | ||
| clock: dependencies, | ||
| color: dependencies.environment["NO_COLOR"] === undefined, | ||
|
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. [P1] Preserve credential sanitization in the default interactive dashboard Removing the dashboard's |
||
| sanitize: redactedErrorMessage, | ||
| input: process.stdin, | ||
| onInterrupt, | ||
| }); | ||
|
|
@@ -2858,13 +2855,13 @@ async function runScan( | |
| diagnostic("scan.output_archived", { archive_dir: archiveDir }); | ||
| if (dashboard !== null) { | ||
| dashboard.note( | ||
| `Moved existing results to: ${redactedErrorMessage(archiveDir)}`, | ||
| `Moved existing results to: ${errorMessage(archiveDir)}`, | ||
| ); | ||
| return; | ||
| } | ||
| progress?.stopTimer(); | ||
| errorOutput.write( | ||
| `Moved existing results to: ${redactedErrorMessage(archiveDir)}\n`, | ||
| `Moved existing results to: ${errorMessage(archiveDir)}\n`, | ||
| ); | ||
| }, | ||
| signal: preparationAbortController.signal, | ||
|
|
@@ -3017,7 +3014,7 @@ async function runScan( | |
| progress.startTimer(runningMessage()); | ||
| }, | ||
| onWarning: (warning, details) => { | ||
| const message = sanitizeDiagnosticValue(warning); | ||
| const message = diagnosticValue(warning); | ||
| if (details?.kind === "target_changed") { | ||
| targetWarnings.push(message); | ||
| } | ||
|
|
@@ -3031,7 +3028,7 @@ async function runScan( | |
| observer, | ||
| classification: classifyConnectionFailure(error), | ||
| }); | ||
| const warning = `${observer} observer failed: ${sanitizeDiagnosticValue(error)}`; | ||
| const warning = `${observer} observer failed: ${diagnosticValue(error)}`; | ||
| if (dashboard === null) { | ||
| writeAboveProgress(() => { | ||
| errorOutput.write(`codex-security: warning: ${warning}\n`); | ||
|
|
@@ -3089,7 +3086,7 @@ async function runScan( | |
| failure instanceof ScanCostLimitExceededError ? failure : undefined; | ||
| const message = | ||
| failure instanceof OutputInsideProtectedRootError | ||
| ? redactedErrorMessage(protectedRootErrorMessage(failure)) | ||
| ? errorMessage(protectedRootErrorMessage(failure)) | ||
| : scanFailureMessage(failure, selectedAuthentication); | ||
| diagnostic("scan.failed", { | ||
| classification: | ||
|
|
@@ -3108,7 +3105,7 @@ async function runScan( | |
| } | ||
| if (scanDir !== null) { | ||
| errorOutput.write( | ||
| `Partial output was kept at ${redactedErrorMessage(scanDir)}.\n`, | ||
| `Partial output was kept at ${errorMessage(scanDir)}.\n`, | ||
| ); | ||
| } | ||
| return { exitCode: 2, error: message }; | ||
|
|
@@ -3245,7 +3242,7 @@ function scanFailureMessage( | |
| // appending it. That is deliberate: upstream authentication and authorization | ||
| // errors can name the organization or project, which must not reach stderr or | ||
| // the JSON error field. | ||
| if (isLocalScanFailure(error)) return sanitizeDiagnosticValue(error); | ||
| if (isLocalScanFailure(error)) return diagnosticValue(error); | ||
| switch (classifyConnectionFailure(error)) { | ||
| case "unauthorized": | ||
| if (authentication?.method === "aws_credentials") { | ||
|
|
@@ -3277,7 +3274,7 @@ function scanFailureMessage( | |
| case "network_error": | ||
| case "timeout": | ||
| case "unknown": | ||
| return sanitizeDiagnosticValue(error); | ||
| return diagnosticValue(error); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -3291,9 +3288,7 @@ function scanScope(arguments_: ScanArguments): string | null { | |
| portable.startsWith("//") | ||
| ? portable.split("/").at(-1) ?? portable | ||
| : portable; | ||
| return redactedErrorMessage( | ||
| scoped.replaceAll(/[\u0000-\u001F\u007F]/gu, " "), | ||
| ); | ||
| return errorMessage(scoped.replaceAll(/[\u0000-\u001F\u007F]/gu, " ")); | ||
| }); | ||
| return `${displayed.join(", ")}${arguments_.paths.length > displayed.length ? `, +${arguments_.paths.length - displayed.length} more` : ""}`; | ||
| } | ||
|
|
@@ -3359,7 +3354,7 @@ function printScanSummary( | |
| ? 33 | ||
| : 36; | ||
| errorOutput.write( | ||
| `\n ${paint("REPORT", "1;36")} ${paint(redactedErrorMessage(result.reportPath), 4)}\n\n` + | ||
| `\n ${paint("REPORT", "1;36")} ${paint(errorMessage(result.reportPath), 4)}\n\n` + | ||
| ` ${paint("FINDINGS", 1)} ${paint(`${findingCount}${severitySummary === "" ? "" : ` (${severitySummary})`}`, findingColor)}\n` + | ||
| ` ${paint("COVERAGE", 1)} ${result.coverage.completeness}\n` + | ||
| ` ${paint("ELAPSED", 1)} ${duration}\n`, | ||
|
|
@@ -3375,7 +3370,7 @@ function printScanSummary( | |
| ); | ||
| } | ||
| errorOutput.write( | ||
| ` ${paint("RESULTS", 1)} ${redactedErrorMessage(result.scanDir)}\n`, | ||
| ` ${paint("RESULTS", 1)} ${errorMessage(result.scanDir)}\n`, | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -3710,7 +3705,7 @@ function interruptedExit( | |
| errorOutput.write( | ||
| scanDir === null | ||
| ? "codex-security: No partial output was kept.\n" | ||
| : `codex-security: Partial output was kept at ${redactedErrorMessage(scanDir)}.\n`, | ||
| : `codex-security: Partial output was kept at ${errorMessage(scanDir)}.\n`, | ||
| ); | ||
| return ctrlC ? 130 : 143; | ||
| } | ||
|
|
@@ -3736,7 +3731,7 @@ if (invokedAsMain()) { | |
| process.exitCode = exitCode; | ||
| }, | ||
| (error: unknown) => { | ||
| process.stderr.write(`codex-security: ${redactedErrorMessage(error)}\n`); | ||
| process.stderr.write(`codex-security: ${errorMessage(error)}\n`); | ||
| process.exitCode = 2; | ||
| }, | ||
| ); | ||
|
|
||
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.
[P1] Redact credentials before persisting failed scan messages
Passing the original upstream error directly to
fail-scan --messagepermanently stores API keys, client secrets, authorization values, and proxy credentials inworkbench.sqlite3, then returns them through scan-history commands. The changed test explicitly checks that synthetic credential text is present in the database andget-scanresponse. This contradicts both repositoryAGENTS.mdinstructions to preserve credential protections andSECURITY.md's existing boundary against disclosing credentials through logs or reports. Redact before truncating and persisting the failure message.