diff --git a/src/features/instance/config/secrets/SecretGrantsEditor.tsx b/src/features/instance/config/secrets/SecretGrantsEditor.tsx index 4490e8274..9525adb40 100644 --- a/src/features/instance/config/secrets/SecretGrantsEditor.tsx +++ b/src/features/instance/config/secrets/SecretGrantsEditor.tsx @@ -9,6 +9,7 @@ import { Badge } from '@/components/ui/badge'; import { useInstanceClientIdParams } from '@/config/useInstanceClient'; import { ComponentGrantCombobox } from '@/features/instance/secrets/ComponentGrantCombobox'; import { useGrantSecret, useRevokeSecret } from '@/integrations/api/instance/secrets/secrets'; +import { describeError } from '@/react-query/queryClient'; import { XIcon } from 'lucide-react'; import { useCallback, useState } from 'react'; import { toast } from 'sonner'; @@ -40,7 +41,7 @@ export function SecretGrantsEditor({ setGrants(response.grants); onChanged?.(); } catch (error) { - toast.error(String(error)); + toast.error(describeError(error).message); // Rethrow so the combobox keeps the typed text for a retry instead of clearing it. throw error; } @@ -52,7 +53,7 @@ export function SecretGrantsEditor({ setGrants(response.grants); onChanged?.(); } catch (error) { - toast.error(String(error)); + toast.error(describeError(error).message); } }, [revokeSecret, instanceParams, name, onChanged]); diff --git a/src/features/instance/secrets/SecretModals.test.tsx b/src/features/instance/secrets/SecretModals.test.tsx index a86f12ee6..f70717e61 100644 --- a/src/features/instance/secrets/SecretModals.test.tsx +++ b/src/features/instance/secrets/SecretModals.test.tsx @@ -6,6 +6,8 @@ import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; vi.mock('sonner', () => ({ toast: { error: vi.fn(), success: vi.fn() } })); +import { SecretsOperationError } from '@/integrations/api/instance/secrets/secrets'; +import { toast } from 'sonner'; import { AddSecretModal, EditSecretModal } from './SecretModals'; beforeAll(() => { @@ -31,6 +33,29 @@ async function clearAndLeave(field: HTMLElement) { } describe('AddSecretModal', () => { + // A named error class is how Datadog knows to withhold the text; the toast must not show the name. + it("toasts a failed save's message without its error class", async () => { + const message = "Secret 'ACME_STRIPE_KEY' cannot be both processEnv and grant-scoped"; + render( + {}} + />, + ); + await act(async () => { + fireEvent.change(screen.getByRole('textbox', { name: /key/i }), { target: { value: 'ACME_STRIPE_KEY' } }); + fireEvent.change(screen.getByRole('textbox', { name: /value/i }), { target: { value: 'sk_live' } }); + }); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: /add secret/i })); + }); + + expect(toast.error).toHaveBeenCalledWith(message); + }); + it('asks for a key and a value once each is left empty', async () => { render( , message: string) { + const error = await pending.then(() => undefined, (rejection: unknown) => rejection); + expect(error).toBeInstanceOf(SecretsOperationError); + expect(error).toMatchObject({ name: 'SecretsOperationError', message }); +} + +function networkError() { + return Object.assign(new Error('Network Error'), { name: 'AxiosError', code: 'ERR_NETWORK' }); +} + describe('useSetSecret', () => { it('encrypts client-side and posts an envelope sealed with the served key', async () => { const { post, wrapper, params } = harness(); @@ -134,11 +152,26 @@ describe('useSetSecret', () => { }); const { result } = renderHook(() => useSetSecret(), { wrapper }); - await expect(result.current.mutateAsync({ ...params, name: 'API_KEY', value: 'x' })).rejects.toThrow( + await expectSecretsOperationError( + result.current.mutateAsync({ ...params, name: 'API_KEY', value: 'x' }), 'secrets custody is not initialized on this node', ); }); + it('leaves a failure Harper did not explain as it was thrown', async () => { + const { post, wrapper, params } = harness(); + const key = await generateKey(); + const failure = networkError(); + post.mockImplementation((_url: string, body: { operation: string }) => + body.operation === 'get_secrets_public_key' + ? Promise.resolve({ data: { public_key: key.pem, fingerprint: key.fingerprint } }) + : Promise.reject(failure) + ); + + const { result } = renderHook(() => useSetSecret(), { wrapper }); + await expect(result.current.mutateAsync({ ...params, name: 'API_KEY', value: 'x' })).rejects.toBe(failure); + }); + it('fetches the key from central-manager for managed clusters (central-manager#409)', async () => { const { post, wrapper, params } = harness(); const key = await generateKey(); @@ -196,8 +229,52 @@ describe('useDeleteSecret', () => { expect(post).toHaveBeenCalledWith('/', { operation: 'delete_secret', name: 'API_KEY' }); post.mockRejectedValueOnce({ response: { data: { error: "No secret found with name 'NOPE'" } } }); - await expect(result.current.mutateAsync({ ...params, name: 'NOPE' })).rejects.toThrow( + await expectSecretsOperationError( + result.current.mutateAsync({ ...params, name: 'NOPE' }), "No secret found with name 'NOPE'", ); + + const failure = networkError(); + post.mockRejectedValueOnce(failure); + await expect(result.current.mutateAsync({ ...params, name: 'API_KEY' })).rejects.toBe(failure); + }); + + it('takes the string message when the error field is structured', async () => { + const { post, wrapper, params } = harness(); + post.mockRejectedValueOnce({ + response: { data: { error: { code: 'NOT_FOUND' }, message: "No secret found with name 'NOPE'" } }, + }); + + const { result } = renderHook(() => useDeleteSecret(), { wrapper }); + await expectSecretsOperationError( + result.current.mutateAsync({ ...params, name: 'NOPE' }), + "No secret found with name 'NOPE'", + ); + }); +}); + +describe.each( + [ + ['useGrantSecret', useGrantSecret, 'grant_secret'], + ['useRevokeSecret', useRevokeSecret, 'revoke_secret'], + ] as const, +)('%s', (_hook, useHook, operation) => { + it(`posts ${operation} and surfaces the body error message on failure`, async () => { + const { post, wrapper, params } = harness(); + post.mockResolvedValueOnce({ data: { name: 'API_KEY', grants: ['app'], changed: true } }); + + const { result } = renderHook(() => useHook(), { wrapper }); + await result.current.mutateAsync({ ...params, name: 'API_KEY', component: 'app' }); + expect(post).toHaveBeenCalledWith('/', { operation, name: 'API_KEY', component: 'app' }); + + post.mockRejectedValueOnce({ response: { data: { error: "No secret found with name 'NOPE'" } } }); + await expectSecretsOperationError( + result.current.mutateAsync({ ...params, name: 'NOPE', component: 'app' }), + "No secret found with name 'NOPE'", + ); + + const failure = networkError(); + post.mockRejectedValueOnce(failure); + await expect(result.current.mutateAsync({ ...params, name: 'API_KEY', component: 'app' })).rejects.toBe(failure); }); }); diff --git a/src/integrations/api/instance/secrets/secrets.ts b/src/integrations/api/instance/secrets/secrets.ts index 9c00faf93..5957c6f72 100644 --- a/src/integrations/api/instance/secrets/secrets.ts +++ b/src/integrations/api/instance/secrets/secrets.ts @@ -54,10 +54,23 @@ export interface ListSecretsResponse { custody_fingerprint: string | null; } -/** Operations API errors carry the message in the response body; surface that, not the Axios noise. */ -function operationErrorMessage(error: unknown): string { - const body = (error as { response?: { data?: { error?: string; message?: string } } })?.response?.data; - return body?.error ?? body?.message ?? String(error); +/** A secrets operation Harper rejected. Datadog withholds its message by this `name`. */ +export class SecretsOperationError extends Error { + constructor(message: string) { + super(message); + this.name = 'SecretsOperationError'; + } +} + +/** + * Operations API errors carry Harper's message in the response body; surface that, not the Axios + * noise. A failure with no such message (the network, a bug) is not Harper's text, so it stays as + * thrown and Error Tracking keeps its type and message. + */ +function operationError(error: unknown): unknown { + const body = (error as { response?: { data?: { error?: unknown; message?: unknown } } })?.response?.data; + const message = typeof body?.error === 'string' ? body.error : body?.message; + return typeof message === 'string' ? new SecretsOperationError(message) : error; } /** @@ -163,15 +176,15 @@ async function encryptAndSetSecret(queryClient: QueryClient, args: SetSecretArgs }); return data; } catch (error) { - const message = operationErrorMessage(error); + const failure = operationError(error); // A kid mismatch means our cached public key is stale (the custody key rotated). Drop the // cached key (ensureQueryData returns stale entries, so invalidation isn't enough) and // re-encrypt once; anything else (or a second mismatch) surfaces to the caller. - if (!isRetry && isKidMismatchError(message)) { + if (!isRetry && failure instanceof SecretsOperationError && isKidMismatchError(failure.message)) { queryClient.removeQueries({ queryKey: secretsPublicKeyQueryOptions(args).queryKey }); return encryptAndSetSecret(queryClient, args, true); } - throw new Error(message); + throw failure; } } @@ -191,7 +204,7 @@ async function deleteSecret({ instanceClient, name }: DeleteSecretArgs): Promise const { data } = await instanceClient.post('/', { operation: 'delete_secret', name }); return data; } catch (error) { - throw new Error(operationErrorMessage(error)); + throw operationError(error); } } @@ -218,7 +231,7 @@ async function mutateGrant( const { data } = await instanceClient.post('/', { operation, name, component }); return data; } catch (error) { - throw new Error(operationErrorMessage(error)); + throw operationError(error); } } diff --git a/src/integrations/datadog/beforeSend.sdk.test.ts b/src/integrations/datadog/beforeSend.sdk.test.ts new file mode 100644 index 000000000..42eae991b --- /dev/null +++ b/src/integrations/datadog/beforeSend.sdk.test.ts @@ -0,0 +1,91 @@ +import { createRequire } from 'node:module'; +import { describe, expect, it } from 'vitest'; +import { SecretsOperationError } from '../api/instance/secrets/secrets'; +import { beforeSend } from './beforeSend'; +import { DatadogErrorEvent } from './shouldKeepEvent'; + +/** + * Runs `beforeSend` inside the installed SDK's own error collection and assembly, because which of + * its writes survive is decided there (`limitModification`), not by the hook. The modules are + * internal, so an SDK bump that moves them fails here first: re-check the allowlist, don't just + * repoint the paths. + */ +const sdkRequire = createRequire(createRequire(import.meta.url).resolve('@datadog/browser-rum')); +const rumCore = (path: string) => sdkRequire(`@datadog/browser-rum-core/cjs/domain/${path}`); + +interface SentEvent extends DatadogErrorEvent { + error: { message: string; causes?: { message: string; stack?: string }[] }; +} + +function sent(error: Error, hook: (event: SentEvent) => boolean) { + const { LifeCycle, LifeCycleEventType } = rumCore('lifeCycle.js'); + const lifeCycle = new LifeCycle(); + const events: SentEvent[] = []; + lifeCycle.subscribe(LifeCycleEventType.RUM_EVENT_COLLECTED, (event: SentEvent) => events.push(event)); + rumCore('assembly.js').startRumAssembly( + { beforeSend: hook }, + lifeCycle, + rumCore('hooks.js').createHooks(), + () => {}, + 100, + ); + rumCore('error/errorCollection.js').doStartErrorCollection(lifeCycle).addError({ + error, + context: {}, + startClocks: { relative: 0, timeStamp: Date.now() }, + }); + return events; +} + +/** A stack the SDK parses into Studio-hosted frames, so only the text under test can need redaction. */ +function withStudioStack(error: T) { + error.stack = + `${error.name}: ${error.message}\n at fb (https://fabric.harper.fast/assets/index-Dcf_w9Ns.js:26:1557)`; + return error; +} + +function thrown(name: string, message: string, cause?: Error) { + const error = new Error(message, { cause }); + error.name = name; + return withStudioStack(error); +} + +const idle = 'The stream went idle before completing.'; + +describe('beforeSend in the RUM SDK pipeline', () => { + it('sends an error whose cause needs no redaction, cause and all', () => { + const events = sent( + thrown('SSEInconclusiveError', idle, thrown('AbortError', 'BodyStreamBuffer was aborted')), + beforeSend, + ); + expect(events).toHaveLength(1); + expect(events[0].error.causes?.[0].message).toBe('BodyStreamBuffer was aborted'); + }); + + it('sends nothing for an error whose cause would need redaction', () => { + const cause = thrown('Error', 'Failed to clone https://github.com/acme-corp/billing-service.git'); + expect(sent(thrown('SSEInconclusiveError', idle, cause), beforeSend)).toEqual([]); + }); + + // The premise of dropping rather than rewriting. If this fails, the SDK now keeps a rewritten + // cause, and `beforeSend` could redact causes in place instead of withholding their event. + it('undoes a beforeSend rewrite of a cause', () => { + const cause = thrown('Error', 'Failed to clone acme-corp/billing'); + const events = sent(thrown('Error', 'Deploy failed', cause), (event) => { + event.error.message = 'rewritten'; + event.error.causes![0].message = 'rewritten'; + return true; + }); + expect(events[0].error.message).toBe('rewritten'); + expect(events[0].error.causes?.[0].message).toBe('Failed to clone acme-corp/billing'); + }); + + it('withholds a failed secrets operation by the type the SDK reads off the thrown class', () => { + const events = sent( + withStudioStack(new SecretsOperationError("No secret found with name 'ACME_STRIPE_KEY'")), + beforeSend, + ); + expect(events).toHaveLength(1); + expect(events[0].error.message).toBe('Harper reported an operation failure (server message withheld).'); + }); +}); diff --git a/src/integrations/datadog/beforeSend.test.ts b/src/integrations/datadog/beforeSend.test.ts index 60642d99d..cf018d725 100644 --- a/src/integrations/datadog/beforeSend.test.ts +++ b/src/integrations/datadog/beforeSend.test.ts @@ -313,6 +313,98 @@ describe('beforeSend', () => { expect(event.view?.name).toBe('/o/c/config/users//'); }); + describe('error.causes', () => { + const abortCause = { + source: 'console', + type: 'AbortError', + message: 'BodyStreamBuffer was aborted', + stack: + 'AbortError: BodyStreamBuffer was aborted\n at @ https://fabric.harper.fast/assets/index-Dcf_w9Ns.js:26:1557', + }; + const repoUrl = 'https://github.com/acme-corp/billing-service.git'; + + function withCauses(causes: unknown, type = 'Error'): DatadogErrorEvent { + return { + type: 'error', + error: { type, message: 'The stream went idle before completing.', causes }, + } as DatadogErrorEvent; + } + + it('keeps an event whose causes need no redaction, and still redacts its own text', () => { + const event = errorEvent({ message: `Failed to deploy ${repoUrl}`, causes: [abortCause] }); + expect(beforeSend(event)).toBe(true); + expect(event.error?.message).toBe('Failed to deploy https://github.com/'); + expect(event.error?.causes).toEqual([abortCause]); + }); + + it('keeps an event with an empty cause chain', () => { + expect(beforeSend(withCauses([]))).toBe(true); + }); + + // How the SDK records a cause that was not an Error: serialised, with no type or stack. + it('keeps a non-Error cause whose serialised text needs no redaction', () => { + const cause = { source: 'console', type: undefined, message: '{"code":"ECONNRESET"}', stack: undefined }; + expect(beforeSend(withCauses([cause]))).toBe(true); + }); + + it.each([ + ['message', { type: 'Error', message: `Failed to clone ${repoUrl}` }], + ['stack', { type: 'Error', message: 'Failed to clone', stack: `Error: Failed to clone\n at x @ ${repoUrl}` }], + ['credential', { type: 'Error', message: 'GET https://api.harper.fast/User/?token=abc.def failed' }], + ])('drops an event whose cause %s the redaction would change', (_field, cause) => { + expect(beforeSend(withCauses([abortCause, cause]))).toBe(false); + }); + + it('drops the causes of a relayed error, whose text is the server chain', () => { + expect(beforeSend(withCauses([abortCause], 'SSEOperationError'))).toBe(false); + }); + + it('drops a relayed cause', () => { + expect(beforeSend(withCauses([abortCause, { type: 'ImportJobError', message: 'row 3: acme' }]))).toBe(false); + }); + + // The relayed link has no text of its own, so only the propagation can drop this. + it('drops a cause below a relayed one', () => { + const relayed = { type: 'SSEOperationError', message: '' }; + expect(beforeSend(withCauses([relayed, { type: 'Error', message: 'acme billing' }]))).toBe(false); + }); + + it('checks every link of a chain as deep as the SDK records', () => { + const chain = [...Array(9).fill(abortCause), { type: 'Error', message: repoUrl }]; + expect(beforeSend(withCauses(chain))).toBe(false); + expect(beforeSend(withCauses(chain.slice(0, 9)))).toBe(true); + }); + + // Anything but the SDK's own shape can't be checked, and can't be rewritten either. + it.each([ + ['a string', 'Failed to clone acme-corp/billing'], + ['null', null], + ['a null entry', [null]], + ['a string entry', ['acme-corp/billing']], + ['an entry with no message', [{ stack: 'Error\n at x @ https://fabric.harper.fast/a.js:1:1' }]], + ['a non-string message', [{ message: { detail: repoUrl } }]], + ['a non-string stack', [{ message: 'x', stack: [repoUrl] }]], + ['a non-string type', [{ message: 'x', type: { name: 'SSEOperationError' } }]], + ['a non-string source', [{ message: 'x', source: { url: repoUrl } }]], + ['an entry with a field the SDK does not record', [{ message: 'x', detail: repoUrl }]], + ])('drops, without throwing, an event whose causes are %s', (_shape, causes) => { + const event = withCauses(causes); + expect(() => beforeSend(event)).not.toThrow(); + expect(beforeSend(event)).toBe(false); + }); + + it('drops, without throwing, an event whose cause throws when read', () => { + const cause = { + get message(): string { + throw new Error('hostile getter'); + }, + }; + const event = withCauses([cause]); + expect(() => beforeSend(event)).not.toThrow(); + expect(beforeSend(event)).toBe(false); + }); + }); + it('leaves a parameterised view name untouched', () => { const event: DatadogErrorEvent = { type: 'view', diff --git a/src/integrations/datadog/beforeSend.ts b/src/integrations/datadog/beforeSend.ts index 35219ea8e..480b3d2d7 100644 --- a/src/integrations/datadog/beforeSend.ts +++ b/src/integrations/datadog/beforeSend.ts @@ -1,5 +1,5 @@ import { redactErrorText } from './redactErrorText'; -import { redactRelayedMessage, redactRelayedStack } from './redactRelayedMessage'; +import { isRelayed, redactRelayedMessage, redactRelayedStack } from './redactRelayedMessage'; import { redactCredentialParams, redactSensitiveParams } from './redactSensitiveParams'; import { type DatadogErrorEvent, shouldKeepEvent } from './shouldKeepEvent'; @@ -38,6 +38,9 @@ export function beforeSend(event: DatadogErrorEvent) { } const error = event.error; if (error) { + if (error.causes !== undefined && !causesNeedNoRedaction(error.causes, error.type)) { + return false; + } const relayedMessage = typeof error.message === 'string' ? error.message : ''; if (typeof error.message === 'string') { error.message = redactErrorAndParams(redactRelayedMessage(error.type, error.message)); @@ -63,3 +66,40 @@ export function beforeSend(event: DatadogErrorEvent) { function redactErrorAndParams(text: string) { return redactCredentialParams(redactErrorText(text)); } + +/** What browser-core's `computeErrorBase` records per cause. A field added later has no rule here. */ +const CAUSE_FIELDS = new Set(['source', 'type', 'message', 'stack']); + +/** + * `error.causes` is off the SDK's modifiable-field allowlist (`MODIFIABLE_FIELD_PATHS_BY_EVENT`, + * browser-rum-core `domain/assembly.ts`): a rewrite here is silently undone and the cause ships as + * thrown. So a cause the rules above would change, or one in a shape they can't read, withholds its + * whole event instead. A relayed error's causes are the server's chain, so they are withheld too. + */ +function causesNeedNoRedaction(causes: unknown, type: string | undefined) { + if (!Array.isArray(causes)) { + return false; + } + let relayed = isRelayed(type); + for (const cause of causes) { + if (typeof cause !== 'object' || cause === null) { + return false; + } + for (const [field, value] of Object.entries(cause)) { + if (!CAUSE_FIELDS.has(field) || (value !== undefined && typeof value !== 'string')) { + return false; + } + } + const { message, stack, type: causeType } = cause as Record; + if (message === undefined) { + return false; + } + relayed ||= isRelayed(causeType); + for (const text of [message, stack]) { + if (text && (relayed || redactErrorAndParams(text) !== text)) { + return false; + } + } + } + return true; +} diff --git a/src/integrations/datadog/redactRelayedMessage.test.ts b/src/integrations/datadog/redactRelayedMessage.test.ts index da530c0aa..57e6e6e5c 100644 --- a/src/integrations/datadog/redactRelayedMessage.test.ts +++ b/src/integrations/datadog/redactRelayedMessage.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { DeployRolloutError } from '../api/instance/applications/restartJob'; import { ImportJobError } from '../api/instance/database/getJob'; +import { SecretsOperationError } from '../api/instance/secrets/secrets'; import { SSEOperationError } from '../api/sse/errors'; import { redactRelayedMessage, redactRelayedStack } from './redactRelayedMessage'; @@ -80,6 +81,15 @@ describe('redactRelayedMessage', () => { expect(redacted).toBe(WITHHELD + frames); }); + it("withholds a failed secrets operation, whose message quotes the secret's name", () => { + const error = new SecretsOperationError("No secret found with name 'ACME_STRIPE_KEY'"); + expect(error.name).toBe('SecretsOperationError'); + expect(redactRelayedMessage(error.name, error.message)).toBe(WITHHELD); + const frames = '\n at deleteSecret @ https://fabric.harper.fast/assets/index-A1b2C3d4.js:5:1234'; + expect(redactRelayedStack(error.name, error.message, `SecretsOperationError: ${error.message}${frames}`)) + .toBe(WITHHELD + frames); + }); + it('removes exactly the message and keeps what follows', () => { const message = [ 'SSEOperationError: Failed to clone package github:gh repo clone acme-corp/billing: fatal: Too many arguments.', diff --git a/src/integrations/datadog/redactRelayedMessage.ts b/src/integrations/datadog/redactRelayedMessage.ts index d279926cc..8f6286c6b 100644 --- a/src/integrations/datadog/redactRelayedMessage.ts +++ b/src/integrations/datadog/redactRelayedMessage.ts @@ -19,14 +19,21 @@ * `SSEOperationError`'s `code` and `phase`, which reach the browser but are not on the RUM event; * reporting them deliberately would restore that detail without the message. * - * `ImportJobError` is the same relay for a failed load job, whose text quotes the rejected rows, and - * `DeployRolloutError` for a rolling deploy's failed restart job, whose text quotes each node's error. + * The same relay covers `ImportJobError` for a failed load job, whose text quotes the rejected rows; + * `DeployRolloutError` for a rolling deploy's failed restart job, whose text quotes each node's error; + * and `SecretsOperationError` for a failed secrets operation, whose text quotes the secret's and + * component's names (harper `components/secretOperations.ts`). */ -const RELAYED_ERROR_TYPES = new Set(['SSEOperationError', 'ImportJobError', 'DeployRolloutError']); +const RELAYED_ERROR_TYPES = new Set([ + 'SSEOperationError', + 'ImportJobError', + 'DeployRolloutError', + 'SecretsOperationError', +]); const WITHHELD = 'Harper reported an operation failure (server message withheld).'; -function isRelayed(type: string | undefined) { +export function isRelayed(type: string | undefined) { return type !== undefined && RELAYED_ERROR_TYPES.has(type); } diff --git a/src/integrations/datadog/shouldKeepEvent.ts b/src/integrations/datadog/shouldKeepEvent.ts index da7f185ca..9a91f951e 100644 --- a/src/integrations/datadog/shouldKeepEvent.ts +++ b/src/integrations/datadog/shouldKeepEvent.ts @@ -13,6 +13,8 @@ export interface DatadogErrorEvent { /** The thrown value's `name`; absent when a non-Error was thrown. */ type?: string; resource?: { url?: string }; + /** The `cause` chain, nearest first. Read-only to `beforeSend`, which can only drop it. */ + causes?: { message?: string; stack?: string; type?: string }[]; }; view?: { name?: string;