fix: tighten sanitize logic on payment secrets#1631
Conversation
github-actions
Bot
added
the
agentcore-harness-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
Package TarballHow to installgh release download pr-1631-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.20.2.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
agentcore-cli-automation
left a comment
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for tightening this up — value-based redaction is a clear improvement over the key-name regex, and rethrowWithContext is a nice helper. A few things worth addressing before merge, mainly around the default (non-DEBUG) UX and a couple of consistency gaps. Nothing fundamental; happy to re-review once you take a look.
| const errorBody = await response.text().catch(() => ''); | ||
| const baseMsg = `Payment API error (${response.status})`; | ||
| const debugExcerpt = sanitizeErrorBody(errorBody, secretsToRedact); | ||
| const error = new Error(debugExcerpt ? `${baseMsg}: ${debugExcerpt}` : baseMsg) as Error & { |
There was a problem hiding this comment.
Default-mode error message loses all human-readable detail.
With DEBUG unset (the common case for end users), sanitizeErrorBody returns '' and the thrown error's .message becomes just Payment API error (400) — no validation reason, no server-side message, nothing actionable. The PR description says the user will see "base errorMsg + error code", but error.code lives on the .code property and is not surfaced in .message, so unless the consuming UI specifically reads .code (today, setupPaymentCredentialProviders just pushes the raw Error into result.errors, and status/action.ts calls getErrorMessage(error)), users will see a status code with no hint as to what went wrong. Compared to the existing behavior — which at least showed the (partially redacted) server message — this is a real diagnostic regression for the most common path.
A few ways to address this (any one would help, can combine):
- Always append the parsed
code/__typeto the base message. The code itself is not sensitive (e.g.ValidationException,ThrottlingException) and immediately tells the user the failure class. Something likePayment API error (400) ValidationExceptioneven when DEBUG is off. - Extract
message/Message/errorMessagefrom the JSON body and include it (run through the same value-based redactor) even when DEBUG is off. Server-provided messages typically contain the actionable detail ("name must match pattern …", "resource already exists", etc.) and the value-based redaction guarantees no echoed secret slips through. - Gate only the raw body excerpt behind DEBUG, but always include the parsed
code+messagefield. Best of both — full body only with DEBUG, but structured/redacted message always shown.
| if (msg.includes('(404)') || msg.includes('ResourceNotFoundException')) return null; | ||
| const code = (err as { code?: unknown }).code; | ||
| if (code === 'ResourceNotFoundException' || msg.includes('(404)')) return null; | ||
| throw new Error(`Failed to get payment credential provider "${options.name}": ${msg}`); |
There was a problem hiding this comment.
.code is dropped on rethrow here (and at line 340 in getPaymentManager).
The PR description says rethrowWithContext was added to "ensure call sites who rethrow errors maintain the error code provided from original exception," but these two non-404 rethrow paths still use throw new Error(\...: ${msg}`)which discardserr.code. For consistency (and so downstream callers can rely on the code being present regardless of which payment function threw), these should also go through rethrowWithContext`:
throw rethrowWithContext(`Failed to get payment credential provider "${options.name}"`, err);No current caller depends on .code from these two functions, so it's not a live bug — just a contract gap that future callers will trip on.
| } | ||
| } | ||
| return out.slice(0, 500); | ||
| } |
There was a problem hiding this comment.
Given how security-sensitive sanitizeErrorBody is (a regression here means secrets leak), it would be worth adding direct unit tests for agentcore-payments.ts covering at minimum:
DEBUGunset → returns''regardless of body / secretsDEBUGset, secret appears verbatim, snake_cased server echo, nested in JSON, embedded in free-text → all replaced with[REDACTED]- Empty/undefined secrets iterable → no crash, body still capped at 500 chars
- Truncation happens after redaction (so a secret straddling the 500-char boundary still gets redacted)
There are currently no unit tests for this file at all (only the mocked tests in pre-deploy-payments.test.ts), so the redactor's correctness is effectively untested. A small focused test would let future refactors here move with confidence.
github-actions
Bot
removed
the
agentcore-harness-reviewing
Coverage Report
|
2 participants
Description
Update payment error messaging in order to strengthen the redaction logic of secret values. Previously, we sanitized based on key names in key:value pairs. This meant that the secret being nested in any unexpected way or shape changes would lead to secrets slipping through in logging.
This PR updates this logic in the following ways:
sanitizeErrorBodyhelper method.rethrowWithContexthelper method. Ensures that call sites who rethrow errors maintain the error code provided from original exception.Related Issue
Closes #
Documentation PR
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.