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 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..8f6286c6b 100644 --- a/src/integrations/datadog/redactRelayedMessage.ts +++ b/src/integrations/datadog/redactRelayedMessage.ts @@ -19,10 +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. + * 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).';