Skip to content

fix: tighten sanitize logic on payment secrets #1631

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.

Sign up for GitHub

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

Open
nborges-aws wants to merge 1 commit into
base: main
Choose a base branch
from
+76 −36
Open

fix: tighten sanitize logic on payment secrets #1631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 65 additions & 32 deletions src/cli/aws/agentcore-payments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,50 @@ interface PaymentManagerDetail {
// HTTP signing helper
// ============================================================================

/**
* Wrap an inner error with a contextual prefix while preserving its
* structured `.code` (the parsed `__type` / `code` from the server response).
*/
function rethrowWithContext(prefix: string, err: unknown): Error & { code?: string } {
const innerMsg = err instanceof Error ? err.message : String(err);
const wrapped = new Error(`${prefix}: ${innerMsg}`) as Error & { code?: string };
const innerCode = (err as { code?: unknown })?.code;
if (typeof innerCode === 'string') wrapped.code = innerCode;
return wrapped;
}

/**
* Build a debug-only excerpt of a non-2xx response body, with every literal
* secret value the CLI just sent stripped out.
*
* Default (no DEBUG): returns `''` so callers can omit the body from
* `Error.message` entirely. The structured `code`/`__type` extracted by the
* caller is what programmatic consumers should use.
*
* With DEBUG set: returns the body with each `secret` substring replaced by
* `[REDACTED]` and capped at 500 chars. Value-based redaction is robust to
* server-side reshaping (snake_case, nesting, free-text echoes) in a way the
* old key-name regex was not.
*/
function sanitizeErrorBody(body: string, secrets: Iterable<string> | undefined): string {
if (!process.env.DEBUG || !body) return '';
let out = body;
for (const secret of secrets ?? []) {
if (typeof secret === 'string' && secret.length > 0) {
out = out.split(secret).join('[REDACTED]');
}
}
return out.slice(0, 500);
}

@agentcore-cli-automation agentcore-cli-automation Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • DEBUG unset → returns '' regardless of body / secrets
  • DEBUG set, 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.


async function signedRequest(options: {
region: string;
method: string;
path: string;
body?: string;
secretsToRedact?: Iterable<string>;
}): Promise<unknown> {
const { region, method, path, body } = options;
const { region, method, path, body, secretsToRedact } = options;
const endpoint = controlPlaneEndpoint(region);
const url = new URL(path, endpoint);

Expand Down Expand Up @@ -139,16 +176,12 @@ async function signedRequest(options: {
}

if (!response.ok) {
const errorBody = await response.text();
// Sanitize error body -- API validation errors may echo request fields containing secrets
const sanitized = errorBody
.replace(
/("apiKeySecret"|"walletSecret"|"apiKeyId"|"appId"|"appSecret"|"authorizationPrivateKey"|"authorizationId")\s*:\s*"[^"]*"/g,
'$1:"[REDACTED]"'
)
.slice(0, 500);

const error = new Error(`Payment API error (${response.status}): ${sanitized}`) as Error & { code?: string };
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 & {

@agentcore-cli-automation agentcore-cli-automation Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  1. Always append the parsed code/__type to the base message. The code itself is not sensitive (e.g. ValidationException, ThrottlingException) and immediately tells the user the failure class. Something like Payment API error (400) ValidationException even when DEBUG is off.
  2. Extract message/Message/errorMessage from 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.
  3. Gate only the raw body excerpt behind DEBUG, but always include the parsed code + message field. Best of both — full body only with DEBUG, but structured/redacted message always shown.

code?: string;
};
try {
const parsed = JSON.parse(errorBody) as Record<string, unknown>;
const code = parsed.code ?? parsed.__type;
Expand All @@ -170,6 +203,8 @@ async function signedRequest(options: {
function buildProviderConfigPayload(options: CreatePaymentCredentialProviderOptions): {
credentialProviderVendor: string;
providerConfigurationInput: Record<string, unknown>;
/** Literal secret values from `options`, used only for DEBUG-mode redaction. */
secrets: string[];
} {
if (options.vendor === 'StripePrivy') {
return {
Expand All @@ -182,6 +217,7 @@ function buildProviderConfigPayload(options: CreatePaymentCredentialProviderOpti
authorizationId: options.authorizationId,
},
},
secrets: [options.appId, options.appSecret, options.authorizationPrivateKey, options.authorizationId],
};
}
return {
Expand All @@ -193,13 +229,14 @@ function buildProviderConfigPayload(options: CreatePaymentCredentialProviderOpti
walletSecret: options.walletSecret,
},
},
secrets: [options.apiKeyId, options.apiKeySecret, options.walletSecret],
};
}

export async function createPaymentCredentialProvider(
options: CreatePaymentCredentialProviderOptions
): Promise<PaymentCredentialProviderApiResult> {
const { credentialProviderVendor, providerConfigurationInput } = buildProviderConfigPayload(options);
const { credentialProviderVendor, providerConfigurationInput, secrets } = buildProviderConfigPayload(options);
const body = JSON.stringify({
name: options.name,
credentialProviderVendor,
Expand All @@ -212,23 +249,22 @@ export async function createPaymentCredentialProvider(
method: 'POST',
path: '/identities/CreatePaymentCredentialProvider',
body,
secretsToRedact: secrets,
})) as PaymentCredentialProviderApiResult;

return {
credentialProviderArn: data.credentialProviderArn,
status: data.status,
};
} catch (err) {
throw new Error(
`Failed to create payment credential provider "${options.name}": ${err instanceof Error ? err.message : String(err)}`
);
throw rethrowWithContext(`Failed to create payment credential provider "${options.name}"`, err);
}
}

export async function updatePaymentCredentialProvider(
options: UpdatePaymentCredentialProviderOptions
): Promise<PaymentCredentialProviderApiResult> {
const { credentialProviderVendor, providerConfigurationInput } = buildProviderConfigPayload(options);
const { credentialProviderVendor, providerConfigurationInput, secrets } = buildProviderConfigPayload(options);
const body = JSON.stringify({
name: options.name,
credentialProviderVendor,
Expand All @@ -241,16 +277,15 @@ export async function updatePaymentCredentialProvider(
method: 'POST',
path: '/identities/UpdatePaymentCredentialProvider',
body,
secretsToRedact: secrets,
})) as PaymentCredentialProviderApiResult;

return {
credentialProviderArn: data.credentialProviderArn,
status: data.status,
};
} catch (err) {
throw new Error(
`Failed to update payment credential provider "${options.name}": ${err instanceof Error ? err.message : String(err)}`
);
throw rethrowWithContext(`Failed to update payment credential provider "${options.name}"`, err);
}
}

Expand All @@ -268,7 +303,8 @@ export async function getPaymentCredentialProvider(
return data;
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
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}`);

@agentcore-cli-automation agentcore-cli-automation Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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.

}
}
Expand All @@ -282,9 +318,7 @@ export async function deletePaymentCredentialProvider(options: { region: string;
body: JSON.stringify({ name: options.name }),
});
} catch (err) {
throw new Error(
`Failed to delete payment credential provider "${options.name}": ${err instanceof Error ? err.message : String(err)}`
);
throw rethrowWithContext(`Failed to delete payment credential provider "${options.name}"`, err);
}
}

Expand All @@ -301,7 +335,8 @@ export async function getPaymentManager(options: GetPaymentManagerOptions): Prom
})) as PaymentManagerDetail;
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
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 manager "${options.paymentManagerId}": ${msg}`);
}
}
Expand All @@ -316,8 +351,10 @@ async function signedDataPlaneRequest(options: {
path: string;
body?: string;
extraHeaders?: Record<string, string>;
/** See `signedRequest.secretsToRedact`. */
secretsToRedact?: Iterable<string>;
}): Promise<unknown> {
const { region, method, path, body, extraHeaders } = options;
const { region, method, path, body, extraHeaders, secretsToRedact } = options;
const endpoint = dataPlaneEndpoint(region);
const url = new URL(path, endpoint);

Expand Down Expand Up @@ -370,13 +407,9 @@ async function signedDataPlaneRequest(options: {

if (!response.ok) {
const errorBody = await response.text().catch(() => '');
const sanitized = errorBody
.replace(
/("apiKeySecret"|"walletSecret"|"apiKeyId"|"appId"|"appSecret"|"authorizationPrivateKey"|"authorizationId")\s*:\s*"[^"]*"/g,
'$1:"[REDACTED]"'
)
.slice(0, 500);
const error = new Error(`Payment data plane API error (${response.status}): ${sanitized}`) as Error & {
const baseMsg = `Payment data plane API error (${response.status})`;
const debugExcerpt = sanitizeErrorBody(errorBody, secretsToRedact);
const error = new Error(debugExcerpt ? `${baseMsg}: ${debugExcerpt}` : baseMsg) as Error & {
code?: string;
};
try {
Expand Down
12 changes: 9 additions & 3 deletions src/cli/operations/deploy/__tests__/pre-deploy-payments.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,9 @@ describe('cleanupPaymentCredentialProviders', () => {
});

it('ignores 404 errors gracefully without throwing', async () => {
mockDeletePaymentCredentialProvider.mockRejectedValue(new Error('Payment API error (404): resource not found'));
mockDeletePaymentCredentialProvider.mockRejectedValue(
new Error('Failed to delete payment credential provider "my-cdp-cred": Payment API error (404)')
);

await expect(
cleanupPaymentCredentialProviders({
Expand All @@ -405,8 +407,12 @@ describe('cleanupPaymentCredentialProviders', () => {
).resolves.toBeUndefined();
});

it('ignores NotFound errors gracefully without throwing', async () => {
mockDeletePaymentCredentialProvider.mockRejectedValue(new Error('ResourceNotFoundException: not found'));
it('ignores ResourceNotFoundException errors gracefully without throwing', async () => {
const notFoundErr = new Error(
'Failed to delete payment credential provider "my-cdp-cred": Payment API error (400)'
) as Error & { code?: string };
notFoundErr.code = 'ResourceNotFoundException';
mockDeletePaymentCredentialProvider.mockRejectedValue(notFoundErr);

await expect(
cleanupPaymentCredentialProviders({
Expand Down
3 changes: 2 additions & 1 deletion src/cli/operations/deploy/pre-deploy-identity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,8 @@ export async function cleanupPaymentCredentialProviders(options: {
await deletePaymentCredentialProvider({ region, name: credName });
} catch (credErr) {
const msg = credErr instanceof Error ? credErr.message : String(credErr);
if (!msg.includes('404') && !msg.includes('NotFound')) {
const code = (credErr as { code?: unknown })?.code;
if (code !== 'ResourceNotFoundException' && !msg.includes('404')) {
console.warn(
`Failed to delete credential provider for connector '${connName}' (payment '${name}'): ${msg}`
);
Expand Down
Loading