From cbd25644a0382a45330b8837c485693a7cebf777 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 21:39:39 -0400 Subject: [PATCH 1/5] fix(datadog): withhold an error whose cause the redaction would change The RUM SDK flattens an error's `cause` chain into `error.causes[]` and sends it, and `beforeSend` never looked at it, so a cause's message and stack bypassed every redaction layer (#1665). Rewriting the causes in place is not possible: `error.causes` is not on the SDK's modifiable-field allowlist, and `limitModification` silently restores any write to it. So `beforeSend` now checks each cause against the same rules the top-level text gets (URL-path and credential redaction, and the relayed-type withhold, which a relayed error passes down its chain) and drops the event when any cause would change, or is in a shape other than the SDK's own. Clean causes are sent as before; the four cause-bearing events in the last 30 days of production RUM all are. Closes #1665 Co-Authored-By: Claude Opus 5.5 --- .../datadog/beforeSend.sdk.test.ts | 76 +++++++++++++++++ src/integrations/datadog/beforeSend.test.ts | 85 +++++++++++++++++++ src/integrations/datadog/beforeSend.ts | 40 ++++++++- .../datadog/redactRelayedMessage.ts | 2 +- src/integrations/datadog/shouldKeepEvent.ts | 2 + 5 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 src/integrations/datadog/beforeSend.sdk.test.ts diff --git a/src/integrations/datadog/beforeSend.sdk.test.ts b/src/integrations/datadog/beforeSend.sdk.test.ts new file mode 100644 index 000000000..bd17ec70b --- /dev/null +++ b/src/integrations/datadog/beforeSend.sdk.test.ts @@ -0,0 +1,76 @@ +import { createRequire } from 'node:module'; +import { describe, expect, it } from 'vitest'; +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 thrown(name: string, message: string, cause?: Error) { + const error = new Error(message, { cause }); + error.name = name; + error.stack = `${name}: ${message}\n at fb (https://fabric.harper.fast/assets/index-Dcf_w9Ns.js:26:1557)`; + return 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'); + }); +}); diff --git a/src/integrations/datadog/beforeSend.test.ts b/src/integrations/datadog/beforeSend.test.ts index 60642d99d..62fe3c960 100644 --- a/src/integrations/datadog/beforeSend.test.ts +++ b/src/integrations/datadog/beforeSend.test.ts @@ -313,6 +313,91 @@ describe('beforeSend', () => { expect(event.view?.name).toBe('/o/c/config/users//'); }); + describe('error.causes', () => { + // The production shape: every cause-bearing event in 30 days of RUM looked like this. + const abortCause = { + 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); + }); + + 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); + }); + + // The SDK only ever emits an array of `{ message: string, stack?: string, type?: string }`. + // Anything else can't be checked, and can't be rewritten, so it must not be sent. + 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' } }]], + ])('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..59bcce410 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,38 @@ export function beforeSend(event: DatadogErrorEvent) { function redactErrorAndParams(text: string) { return redactCredentialParams(redactErrorText(text)); } + +/** + * `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; + } + const { message, stack, type: causeType } = cause as Record; + if (typeof message !== 'string') { + return false; + } + if (stack !== undefined && typeof stack !== 'string') { + return false; + } + if (causeType !== undefined && typeof causeType !== 'string') { + 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.ts b/src/integrations/datadog/redactRelayedMessage.ts index d279926cc..e9777c229 100644 --- a/src/integrations/datadog/redactRelayedMessage.ts +++ b/src/integrations/datadog/redactRelayedMessage.ts @@ -26,7 +26,7 @@ const RELAYED_ERROR_TYPES = new Set(['SSEOperationError', 'ImportJobError', 'Dep 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; From 1e05581d7baf66b598e3ff70f4e6dc5789b0d93c Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 21:49:59 -0400 Subject: [PATCH 2/5] fix(datadog): withhold a cause carrying a field the redaction has no rule for A cause can only be checked, not rewritten, so a field the SDK starts recording later would ship unchecked. Accept only the fields browser-core's computeErrorBase records per cause (source, type, message, stack), each a string or absent; anything else withholds the event, which the SDK-pipeline test then reports on the bump that adds it. Refs #1665 Co-Authored-By: Claude Opus 5.5 --- src/integrations/datadog/beforeSend.test.ts | 13 ++++++++++--- src/integrations/datadog/beforeSend.ts | 16 +++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/integrations/datadog/beforeSend.test.ts b/src/integrations/datadog/beforeSend.test.ts index 62fe3c960..cf018d725 100644 --- a/src/integrations/datadog/beforeSend.test.ts +++ b/src/integrations/datadog/beforeSend.test.ts @@ -314,8 +314,8 @@ describe('beforeSend', () => { }); describe('error.causes', () => { - // The production shape: every cause-bearing event in 30 days of RUM looked like this. const abortCause = { + source: 'console', type: 'AbortError', message: 'BodyStreamBuffer was aborted', stack: @@ -341,6 +341,12 @@ describe('beforeSend', () => { 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}` }], @@ -369,8 +375,7 @@ describe('beforeSend', () => { expect(beforeSend(withCauses(chain.slice(0, 9)))).toBe(true); }); - // The SDK only ever emits an array of `{ message: string, stack?: string, type?: string }`. - // Anything else can't be checked, and can't be rewritten, so it must not be sent. + // 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], @@ -380,6 +385,8 @@ describe('beforeSend', () => { ['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(); diff --git a/src/integrations/datadog/beforeSend.ts b/src/integrations/datadog/beforeSend.ts index 59bcce410..480b3d2d7 100644 --- a/src/integrations/datadog/beforeSend.ts +++ b/src/integrations/datadog/beforeSend.ts @@ -67,6 +67,9 @@ 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 @@ -82,14 +85,13 @@ function causesNeedNoRedaction(causes: unknown, type: string | undefined) { if (typeof cause !== 'object' || cause === null) { return false; } - const { message, stack, type: causeType } = cause as Record; - if (typeof message !== 'string') { - return false; - } - if (stack !== undefined && typeof stack !== 'string') { - return false; + for (const [field, value] of Object.entries(cause)) { + if (!CAUSE_FIELDS.has(field) || (value !== undefined && typeof value !== 'string')) { + return false; + } } - if (causeType !== undefined && typeof causeType !== 'string') { + const { message, stack, type: causeType } = cause as Record; + if (message === undefined) { return false; } relayed ||= isRelayed(causeType); From b2fd93f9abb0bb5dde0bdd112dd574816c27669b Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 22:03:29 -0400 Subject: [PATCH 3/5] fix(datadog): withhold failed secrets-operation text, which quotes secret names A failed set/delete/grant/revoke secret rejected with a plain Error carrying Harper's response text, and React Query's global handler passes that to console.error, which the RUM SDK reports. Harper composes those messages by interpolating the secret and component names (harper-pro core/components/secretOperations.ts), so they reached Error Tracking as written (#1664). All three throw sites now throw a named SecretsOperationError, and that name joins RELAYED_ERROR_TYPES, so beforeSend withholds the message and the stack header the same way it does for SSEOperationError and ImportJobError. The class carries no cause: under #1665 a cause on a relayed error would withhold the whole event, losing the failure rate. The secrets dialogs and grants editor toasted String(error), which would now read "SecretsOperationError: ..."; they toast describeError's message instead, as the global handler does. The shared SecretModals also serve the env-var editor, whose toasts now show Harper's response text (or axios's own message when there is none) rather than "AxiosError: Request failed with status code ...". Closes #1664 Co-Authored-By: Claude Opus 5.5 --- .../config/secrets/SecretGrantsEditor.tsx | 5 ++- .../instance/secrets/SecretModals.test.tsx | 25 +++++++++++ .../instance/secrets/SecretModals.tsx | 7 +-- .../api/instance/secrets/secrets.test.tsx | 44 ++++++++++++++++--- .../api/instance/secrets/secrets.ts | 14 ++++-- .../datadog/beforeSend.sdk.test.ts | 19 +++++++- .../datadog/redactRelayedMessage.test.ts | 10 +++++ .../datadog/redactRelayedMessage.ts | 4 +- 8 files changed, 110 insertions(+), 18 deletions(-) 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( { }); const { result } = renderHook(() => useSetSecret(), { wrapper }); - await expect(result.current.mutateAsync({ ...params, name: 'API_KEY', value: 'x' })).rejects.toThrow( - 'secrets custody is not initialized on this node', - ); + await expect(result.current.mutateAsync({ ...params, name: 'API_KEY', value: 'x' })).rejects.toMatchObject({ + name: 'SecretsOperationError', + message: 'secrets custody is not initialized on this node', + }); }); it('fetches the key from central-manager for managed clusters (central-manager#409)', async () => { @@ -196,8 +203,31 @@ 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( - "No secret found with name 'NOPE'", - ); + await expect(result.current.mutateAsync({ ...params, name: 'NOPE' })).rejects.toMatchObject({ + name: 'SecretsOperationError', + message: "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 expect(result.current.mutateAsync({ ...params, name: 'NOPE', component: 'app' })).rejects.toMatchObject({ + name: 'SecretsOperationError', + message: "No secret found with name 'NOPE'", + }); }); }); diff --git a/src/integrations/api/instance/secrets/secrets.ts b/src/integrations/api/instance/secrets/secrets.ts index 9c00faf93..ae08093ef 100644 --- a/src/integrations/api/instance/secrets/secrets.ts +++ b/src/integrations/api/instance/secrets/secrets.ts @@ -54,6 +54,14 @@ export interface ListSecretsResponse { custody_fingerprint: string | null; } +/** 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 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; @@ -171,7 +179,7 @@ async function encryptAndSetSecret(queryClient: QueryClient, args: SetSecretArgs queryClient.removeQueries({ queryKey: secretsPublicKeyQueryOptions(args).queryKey }); return encryptAndSetSecret(queryClient, args, true); } - throw new Error(message); + throw new SecretsOperationError(message); } } @@ -191,7 +199,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 new SecretsOperationError(operationErrorMessage(error)); } } @@ -218,7 +226,7 @@ async function mutateGrant( const { data } = await instanceClient.post('/', { operation, name, component }); return data; } catch (error) { - throw new Error(operationErrorMessage(error)); + throw new SecretsOperationError(operationErrorMessage(error)); } } diff --git a/src/integrations/datadog/beforeSend.sdk.test.ts b/src/integrations/datadog/beforeSend.sdk.test.ts index bd17ec70b..42eae991b 100644 --- a/src/integrations/datadog/beforeSend.sdk.test.ts +++ b/src/integrations/datadog/beforeSend.sdk.test.ts @@ -1,5 +1,6 @@ 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'; @@ -37,11 +38,16 @@ function sent(error: Error, hook: (event: SentEvent) => boolean) { } /** 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; - error.stack = `${name}: ${message}\n at fb (https://fabric.harper.fast/assets/index-Dcf_w9Ns.js:26:1557)`; - return error; + return withStudioStack(error); } const idle = 'The stream went idle before completing.'; @@ -73,4 +79,13 @@ describe('beforeSend in the RUM SDK pipeline', () => { 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/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 e9777c229..96c93bfc3 100644 --- a/src/integrations/datadog/redactRelayedMessage.ts +++ b/src/integrations/datadog/redactRelayedMessage.ts @@ -21,8 +21,10 @@ * * `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. + * `SecretsOperationError` for a failed secrets operation, whose text quotes the secret's and + * component's names (harper-pro `core/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).'; From b592d97cdecf8c78448c96eede38b89be20939aa Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 22:18:30 -0400 Subject: [PATCH 4/5] fix(secrets): leave a failure Harper did not explain as it was thrown Wrapping every rejection in SecretsOperationError also withheld failures that carry no Harper text at all: a network error or a bug reached Error Tracking only as the withheld placeholder. Only a response body with Harper's message now becomes a SecretsOperationError; anything else is rethrown unchanged, keeping its own type and message. That also stops a structured body from turning into "[object Object]". Cite the OSS harper path for the interpolated secrets messages. Refs #1664 Co-Authored-By: Claude Opus 5.5 --- .../api/instance/secrets/secrets.test.tsx | 59 +++++++++++++++---- .../api/instance/secrets/secrets.ts | 23 +++++--- .../datadog/redactRelayedMessage.ts | 2 +- 3 files changed, 62 insertions(+), 22 deletions(-) diff --git a/src/integrations/api/instance/secrets/secrets.test.tsx b/src/integrations/api/instance/secrets/secrets.test.tsx index a33c9420b..19599eb12 100644 --- a/src/integrations/api/instance/secrets/secrets.test.tsx +++ b/src/integrations/api/instance/secrets/secrets.test.tsx @@ -9,6 +9,7 @@ import { webcrypto } from 'node:crypto'; import { PropsWithChildren } from 'react'; import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { + SecretsOperationError, secretsPublicKeyQueryOptions, useDeleteSecret, useGrantSecret, @@ -68,6 +69,18 @@ function harness() { return { post, queryClient, wrapper, params }; } +/** Datadog reads the `name`, so match it as well as the class: dropping the assignment keeps `instanceof`. */ +async function expectSecretsOperationError(pending: Promise, message: string) { + const error = await pending.then(() => undefined, (rejection: unknown) => rejection); + expect(error).toBeInstanceOf(SecretsOperationError); + expect(error).toMatchObject({ name: 'SecretsOperationError', message }); +} + +/** What axios rejects with when the request never got a response. */ +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(); @@ -140,10 +153,24 @@ describe('useSetSecret', () => { }); const { result } = renderHook(() => useSetSecret(), { wrapper }); - await expect(result.current.mutateAsync({ ...params, name: 'API_KEY', value: 'x' })).rejects.toMatchObject({ - name: 'SecretsOperationError', - message: 'secrets custody is not initialized on this node', - }); + 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 () => { @@ -203,10 +230,14 @@ 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.toMatchObject({ - name: 'SecretsOperationError', - message: "No secret found with name 'NOPE'", - }); + 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); }); }); @@ -225,9 +256,13 @@ describe.each( expect(post).toHaveBeenCalledWith('/', { operation, name: 'API_KEY', component: 'app' }); post.mockRejectedValueOnce({ response: { data: { error: "No secret found with name 'NOPE'" } } }); - await expect(result.current.mutateAsync({ ...params, name: 'NOPE', component: 'app' })).rejects.toMatchObject({ - name: 'SecretsOperationError', - message: "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 ae08093ef..e278e94a5 100644 --- a/src/integrations/api/instance/secrets/secrets.ts +++ b/src/integrations/api/instance/secrets/secrets.ts @@ -62,10 +62,15 @@ export class SecretsOperationError extends Error { } } -/** 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); +/** + * 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 = body?.error ?? body?.message; + return typeof message === 'string' ? new SecretsOperationError(message) : error; } /** @@ -171,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 SecretsOperationError(message); + throw failure; } } @@ -199,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 SecretsOperationError(operationErrorMessage(error)); + throw operationError(error); } } @@ -226,7 +231,7 @@ async function mutateGrant( const { data } = await instanceClient.post('/', { operation, name, component }); return data; } catch (error) { - throw new SecretsOperationError(operationErrorMessage(error)); + throw operationError(error); } } diff --git a/src/integrations/datadog/redactRelayedMessage.ts b/src/integrations/datadog/redactRelayedMessage.ts index 96c93bfc3..b51b50940 100644 --- a/src/integrations/datadog/redactRelayedMessage.ts +++ b/src/integrations/datadog/redactRelayedMessage.ts @@ -22,7 +22,7 @@ * `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. * `SecretsOperationError` for a failed secrets operation, whose text quotes the secret's and - * component's names (harper-pro `core/components/secretOperations.ts`). + * component's names (harper `components/secretOperations.ts`). */ const RELAYED_ERROR_TYPES = new Set(['SSEOperationError', 'ImportJobError', 'DeployRolloutError', 'SecretsOperationError']); From 9d5fe201481eef377fc3948bee74fab3ba1474e6 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 22:24:47 -0400 Subject: [PATCH 5/5] fix(secrets): take Harper's string message when the error field is structured A body with a structured `error` and a string `message` short-circuited on the object and fell through to the raw AxiosError, so the string never became a SecretsOperationError. Prefer whichever field is a string. Refs #1664 Co-Authored-By: Claude Opus 5.5 --- .../api/instance/secrets/secrets.test.tsx | 14 +++++++++++++- src/integrations/api/instance/secrets/secrets.ts | 2 +- src/integrations/datadog/redactRelayedMessage.ts | 13 +++++++++---- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/integrations/api/instance/secrets/secrets.test.tsx b/src/integrations/api/instance/secrets/secrets.test.tsx index 19599eb12..cb5e310f4 100644 --- a/src/integrations/api/instance/secrets/secrets.test.tsx +++ b/src/integrations/api/instance/secrets/secrets.test.tsx @@ -76,7 +76,6 @@ async function expectSecretsOperationError(pending: Promise, message: s expect(error).toMatchObject({ name: 'SecretsOperationError', message }); } -/** What axios rejects with when the request never got a response. */ function networkError() { return Object.assign(new Error('Network Error'), { name: 'AxiosError', code: 'ERR_NETWORK' }); } @@ -239,6 +238,19 @@ describe('useDeleteSecret', () => { 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( diff --git a/src/integrations/api/instance/secrets/secrets.ts b/src/integrations/api/instance/secrets/secrets.ts index e278e94a5..5957c6f72 100644 --- a/src/integrations/api/instance/secrets/secrets.ts +++ b/src/integrations/api/instance/secrets/secrets.ts @@ -69,7 +69,7 @@ export class SecretsOperationError extends Error { */ function operationError(error: unknown): unknown { const body = (error as { response?: { data?: { error?: unknown; message?: unknown } } })?.response?.data; - const message = body?.error ?? body?.message; + const message = typeof body?.error === 'string' ? body.error : body?.message; return typeof message === 'string' ? new SecretsOperationError(message) : error; } diff --git a/src/integrations/datadog/redactRelayedMessage.ts b/src/integrations/datadog/redactRelayedMessage.ts index b51b50940..8f6286c6b 100644 --- a/src/integrations/datadog/redactRelayedMessage.ts +++ b/src/integrations/datadog/redactRelayedMessage.ts @@ -19,12 +19,17 @@ * `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. - * `SecretsOperationError` for a failed secrets operation, whose text quotes the secret's and + * 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', 'SecretsOperationError']); +const RELAYED_ERROR_TYPES = new Set([ + 'SSEOperationError', + 'ImportJobError', + 'DeployRolloutError', + 'SecretsOperationError', +]); const WITHHELD = 'Harper reported an operation failure (server message withheld).';