From 5425e038cebe4a9664f3049a1db10152d9b1b6eb Mon Sep 17 00:00:00 2001 From: Nicolas Borges Date: Wed, 24 Jun 2026 11:53:08 -0400 Subject: [PATCH 1/2] fix: tighten sanitize logic on payment secrets --- src/cli/aws/agentcore-payments.ts | 97 +++++++++++++------ .../__tests__/pre-deploy-payments.test.ts | 12 ++- .../operations/deploy/pre-deploy-identity.ts | 3 +- 3 files changed, 76 insertions(+), 36 deletions(-) diff --git a/src/cli/aws/agentcore-payments.ts b/src/cli/aws/agentcore-payments.ts index bbd9d6272..28e1c9d01 100644 --- a/src/cli/aws/agentcore-payments.ts +++ b/src/cli/aws/agentcore-payments.ts @@ -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 | 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); +} + async function signedRequest(options: { region: string; method: string; path: string; body?: string; + secretsToRedact?: Iterable; }): Promise { - const { region, method, path, body } = options; + const { region, method, path, body, secretsToRedact } = options; const endpoint = controlPlaneEndpoint(region); const url = new URL(path, endpoint); @@ -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 & { + code?: string; + }; try { const parsed = JSON.parse(errorBody) as Record; const code = parsed.code ?? parsed.__type; @@ -170,6 +203,8 @@ async function signedRequest(options: { function buildProviderConfigPayload(options: CreatePaymentCredentialProviderOptions): { credentialProviderVendor: string; providerConfigurationInput: Record; + /** Literal secret values from `options`, used only for DEBUG-mode redaction. */ + secrets: string[]; } { if (options.vendor === 'StripePrivy') { return { @@ -182,6 +217,7 @@ function buildProviderConfigPayload(options: CreatePaymentCredentialProviderOpti authorizationId: options.authorizationId, }, }, + secrets: [options.appId, options.appSecret, options.authorizationPrivateKey, options.authorizationId], }; } return { @@ -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 { - const { credentialProviderVendor, providerConfigurationInput } = buildProviderConfigPayload(options); + const { credentialProviderVendor, providerConfigurationInput, secrets } = buildProviderConfigPayload(options); const body = JSON.stringify({ name: options.name, credentialProviderVendor, @@ -212,6 +249,7 @@ export async function createPaymentCredentialProvider( method: 'POST', path: '/identities/CreatePaymentCredentialProvider', body, + secretsToRedact: secrets, })) as PaymentCredentialProviderApiResult; return { @@ -219,16 +257,14 @@ export async function createPaymentCredentialProvider( 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 { - const { credentialProviderVendor, providerConfigurationInput } = buildProviderConfigPayload(options); + const { credentialProviderVendor, providerConfigurationInput, secrets } = buildProviderConfigPayload(options); const body = JSON.stringify({ name: options.name, credentialProviderVendor, @@ -241,6 +277,7 @@ export async function updatePaymentCredentialProvider( method: 'POST', path: '/identities/UpdatePaymentCredentialProvider', body, + secretsToRedact: secrets, })) as PaymentCredentialProviderApiResult; return { @@ -248,9 +285,7 @@ export async function updatePaymentCredentialProvider( 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); } } @@ -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}`); } } @@ -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); } } @@ -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}`); } } @@ -316,8 +351,10 @@ async function signedDataPlaneRequest(options: { path: string; body?: string; extraHeaders?: Record; + /** See `signedRequest.secretsToRedact`. */ + secretsToRedact?: Iterable; }): Promise { - 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); @@ -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 { diff --git a/src/cli/operations/deploy/__tests__/pre-deploy-payments.test.ts b/src/cli/operations/deploy/__tests__/pre-deploy-payments.test.ts index 5b6bb2317..0849dbce9 100644 --- a/src/cli/operations/deploy/__tests__/pre-deploy-payments.test.ts +++ b/src/cli/operations/deploy/__tests__/pre-deploy-payments.test.ts @@ -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({ @@ -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({ diff --git a/src/cli/operations/deploy/pre-deploy-identity.ts b/src/cli/operations/deploy/pre-deploy-identity.ts index d69e722d1..12da31014 100644 --- a/src/cli/operations/deploy/pre-deploy-identity.ts +++ b/src/cli/operations/deploy/pre-deploy-identity.ts @@ -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}` ); From 8d093fbf5a8f6449e7ed6b03a935e357c17de999 Mon Sep 17 00:00:00 2001 From: Nicolas Borges Date: Mon, 29 Jun 2026 12:31:47 -0400 Subject: [PATCH 2/2] fix: always include code and message field in error msg output --- .../aws/__tests__/agentcore-payments.test.ts | 180 ++++++++++++++++++ src/cli/aws/agentcore-payments.ts | 62 +++--- src/lib/secrets/types/napi-keyring.d.ts | 7 + 3 files changed, 227 insertions(+), 22 deletions(-) create mode 100644 src/cli/aws/__tests__/agentcore-payments.test.ts create mode 100644 src/lib/secrets/types/napi-keyring.d.ts diff --git a/src/cli/aws/__tests__/agentcore-payments.test.ts b/src/cli/aws/__tests__/agentcore-payments.test.ts new file mode 100644 index 000000000..41061f39e --- /dev/null +++ b/src/cli/aws/__tests__/agentcore-payments.test.ts @@ -0,0 +1,180 @@ +import { redactSecrets, rethrowWithContext, sanitizeErrorBody } from '../agentcore-payments'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +describe('redactSecrets', () => { + it('replaces literal secret values with [REDACTED]', () => { + const text = 'key=sk_live_abc123 and wallet=w_secret_456'; + const result = redactSecrets(text, ['sk_live_abc123', 'w_secret_456']); + expect(result).toBe('key=[REDACTED] and wallet=[REDACTED]'); + }); + + it('handles secrets nested in JSON', () => { + const body = JSON.stringify({ nested: { deep: { value: 'my-secret-key' } } }); + expect(redactSecrets(body, ['my-secret-key'])).not.toContain('my-secret-key'); + expect(redactSecrets(body, ['my-secret-key'])).toContain('[REDACTED]'); + }); + + it('handles snake_cased server echo of secrets', () => { + const body = '{"api_key_secret": "sk_live_abc123", "error": "invalid api_key_secret: sk_live_abc123"}'; + const result = redactSecrets(body, ['sk_live_abc123']); + expect(result).not.toContain('sk_live_abc123'); + expect(result.match(/\[REDACTED\]/g)?.length).toBe(2); + }); + + it('handles secrets embedded in free-text messages', () => { + const body = 'Error: The provided key "super_secret_value" is invalid for account xyz'; + expect(redactSecrets(body, ['super_secret_value'])).toBe( + 'Error: The provided key "[REDACTED]" is invalid for account xyz' + ); + }); + + it('does not crash with empty secrets iterable', () => { + expect(redactSecrets('hello world', [])).toBe('hello world'); + }); + + it('does not crash with undefined secrets', () => { + expect(redactSecrets('hello world', undefined)).toBe('hello world'); + }); + + it('skips empty string secrets without infinite loop', () => { + expect(redactSecrets('hello world', ['', 'world'])).toBe('hello [REDACTED]'); + }); +}); + +describe('sanitizeErrorBody', () => { + let originalDebug: string | undefined; + + beforeEach(() => { + originalDebug = process.env.DEBUG; + }); + + afterEach(() => { + if (originalDebug !== undefined) process.env.DEBUG = originalDebug; + else delete process.env.DEBUG; + }); + + it('returns empty string for empty body regardless of DEBUG', () => { + process.env.DEBUG = '1'; + expect(sanitizeErrorBody('', ['secret'])).toBe(''); + delete process.env.DEBUG; + expect(sanitizeErrorBody('', ['secret'])).toBe(''); + }); + + describe('without DEBUG', () => { + beforeEach(() => { + delete process.env.DEBUG; + }); + + it('includes parsed code from response body', () => { + const body = JSON.stringify({ code: 'ValidationException', message: 'name is required' }); + const result = sanitizeErrorBody(body, []); + expect(result).toContain('ValidationException'); + }); + + it('includes parsed __type from response body', () => { + const body = JSON.stringify({ __type: 'ThrottlingException', message: 'rate exceeded' }); + const result = sanitizeErrorBody(body, []); + expect(result).toContain('ThrottlingException'); + }); + + it('includes redacted server message', () => { + const body = JSON.stringify({ + code: 'ValidationException', + message: 'Invalid key: sk_live_abc123', + }); + const result = sanitizeErrorBody(body, ['sk_live_abc123']); + expect(result).toContain('ValidationException'); + expect(result).toContain('Invalid key: [REDACTED]'); + expect(result).not.toContain('sk_live_abc123'); + }); + + it('handles Message field (capital M)', () => { + const body = JSON.stringify({ code: 'InternalError', Message: 'something went wrong' }); + const result = sanitizeErrorBody(body, []); + expect(result).toContain('something went wrong'); + }); + + it('handles errorMessage field', () => { + const body = JSON.stringify({ __type: 'ServiceException', errorMessage: 'oops' }); + const result = sanitizeErrorBody(body, []); + expect(result).toContain('oops'); + }); + + it('does not include the raw body', () => { + const body = JSON.stringify({ code: 'Err', message: 'msg', extra: 'data' }); + const result = sanitizeErrorBody(body, []); + expect(result).not.toContain('"extra"'); + }); + + it('returns empty string for non-JSON body with no parseable fields', () => { + const result = sanitizeErrorBody('Internal Server Error', []); + expect(result).toBe(''); + }); + }); + + describe('with DEBUG', () => { + beforeEach(() => { + process.env.DEBUG = '1'; + }); + + it('includes full redacted body', () => { + const body = JSON.stringify({ + code: 'ValidationException', + message: 'bad key: sk_live_abc123', + detail: { apiKeySecret: 'sk_live_abc123' }, + }); + const result = sanitizeErrorBody(body, ['sk_live_abc123']); + expect(result).toContain('ValidationException'); + expect(result).toContain('bad key: [REDACTED]'); + expect(result).not.toContain('sk_live_abc123'); + }); + + it('caps raw body at 500 chars after redaction', () => { + const body = JSON.stringify({ message: 'x'.repeat(600) }); + const result = sanitizeErrorBody(body, []); + const parts = result.split(' — '); + const rawBodyPart = parts[parts.length - 1]!; + expect(rawBodyPart.length).toBeLessThanOrEqual(500); + }); + + it('redacts secrets that straddle the 500-char boundary', () => { + const prefix = 'a'.repeat(495); + const secret = 'SECRET_VALUE_HERE'; + const body = JSON.stringify({ data: prefix + secret }); + const result = sanitizeErrorBody(body, [secret]); + expect(result).not.toContain(secret); + }); + + it('includes non-JSON body in debug excerpt', () => { + const result = sanitizeErrorBody('raw server error text', []); + expect(result).toContain('raw server error text'); + }); + }); +}); + +describe('rethrowWithContext', () => { + it('preserves .code from the inner error', () => { + const inner = new Error('something failed') as Error & { code?: string }; + inner.code = 'ValidationException'; + const wrapped = rethrowWithContext('Create failed', inner); + expect(wrapped.code).toBe('ValidationException'); + expect(wrapped.message).toBe('Create failed: something failed'); + }); + + it('works with non-Error values', () => { + const wrapped = rethrowWithContext('Operation failed', 'string error'); + expect(wrapped.message).toBe('Operation failed: string error'); + expect(wrapped.code).toBeUndefined(); + }); + + it('does not set .code when inner error has no code', () => { + const wrapped = rethrowWithContext('prefix', new Error('inner')); + expect(wrapped.code).toBeUndefined(); + }); + + it('ignores non-string code values', () => { + const inner = { message: 'hi', code: 42 }; + const wrapped = rethrowWithContext('prefix', inner); + expect(wrapped.code).toBeUndefined(); + }); +}); diff --git a/src/cli/aws/agentcore-payments.ts b/src/cli/aws/agentcore-payments.ts index 28e1c9d01..7e8f5fc48 100644 --- a/src/cli/aws/agentcore-payments.ts +++ b/src/cli/aws/agentcore-payments.ts @@ -86,7 +86,7 @@ interface PaymentManagerDetail { * 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 } { +export 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; @@ -95,27 +95,45 @@ function rethrowWithContext(prefix: string, err: unknown): Error & { code?: stri } /** - * 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. + * Redact every literal secret value from a string, replacing occurrences with + * `[REDACTED]`. Value-based redaction is robust to server-side reshaping */ -function sanitizeErrorBody(body: string, secrets: Iterable | undefined): string { - if (!process.env.DEBUG || !body) return ''; - let out = body; +export function redactSecrets(text: string, secrets: Iterable | undefined): string { + let out = text; for (const secret of secrets ?? []) { if (typeof secret === 'string' && secret.length > 0) { out = out.split(secret).join('[REDACTED]'); } } - return out.slice(0, 500); + return out; +} + +/** + * Build an error message excerpt from a non-2xx response body. + * + * - Always includes the parsed `code`/`__type` and `message` field + * (run through value-based redaction) so users get actionable context. + * - With DEBUG set: appends the full (redacted) body + */ +export function sanitizeErrorBody(body: string, secrets: Iterable | undefined): string { + if (!body) return ''; + + const parts: string[] = []; + try { + const parsed = JSON.parse(body) as Record; + const code = parsed.code ?? parsed.__type; + if (typeof code === 'string') parts.push(code); + const message = parsed.message ?? parsed.Message ?? parsed.errorMessage; + if (typeof message === 'string') parts.push(redactSecrets(message, secrets)); + } catch (_err) { + /* body is not JSON — fall through */ + } + + if (process.env.DEBUG) { + parts.push(redactSecrets(body, secrets).slice(0, 500)); + } + + return parts.join(' — '); } async function signedRequest(options: { @@ -178,8 +196,8 @@ async function signedRequest(options: { if (!response.ok) { 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 & { + const excerpt = sanitizeErrorBody(errorBody, secretsToRedact); + const error = new Error(excerpt ? `${baseMsg}: ${excerpt}` : baseMsg) as Error & { code?: string; }; try { @@ -305,7 +323,7 @@ export async function getPaymentCredentialProvider( const msg = err instanceof Error ? err.message : String(err); 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}`); + throw rethrowWithContext(`Failed to get payment credential provider "${options.name}"`, err); } } @@ -337,7 +355,7 @@ export async function getPaymentManager(options: GetPaymentManagerOptions): Prom const msg = err instanceof Error ? err.message : String(err); 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}`); + throw rethrowWithContext(`Failed to get payment manager "${options.paymentManagerId}"`, err); } } @@ -408,8 +426,8 @@ async function signedDataPlaneRequest(options: { if (!response.ok) { const errorBody = await response.text().catch(() => ''); const baseMsg = `Payment data plane API error (${response.status})`; - const debugExcerpt = sanitizeErrorBody(errorBody, secretsToRedact); - const error = new Error(debugExcerpt ? `${baseMsg}: ${debugExcerpt}` : baseMsg) as Error & { + const excerpt = sanitizeErrorBody(errorBody, secretsToRedact); + const error = new Error(excerpt ? `${baseMsg}: ${excerpt}` : baseMsg) as Error & { code?: string; }; try { diff --git a/src/lib/secrets/types/napi-keyring.d.ts b/src/lib/secrets/types/napi-keyring.d.ts new file mode 100644 index 000000000..c45c47549 --- /dev/null +++ b/src/lib/secrets/types/napi-keyring.d.ts @@ -0,0 +1,7 @@ +declare module '@napi-rs/keyring' { + export class Entry { + constructor(service: string, account: string); + getPassword(): string | null; + setPassword(password: string): void; + } +}