Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/features/instance/config/secrets/SecretGrantsEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}
Expand All @@ -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]);

Expand Down
25 changes: 25 additions & 0 deletions src/features/instance/secrets/SecretModals.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand All @@ -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(
<AddSecretModal
description="Add a secret."
existingKeys={[]}
onSubmit={vi.fn().mockRejectedValue(new SecretsOperationError(message))}
isModalOpen
setIsModalOpen={() => {}}
/>,
);
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(
<AddSecretModal
Expand Down
7 changes: 4 additions & 3 deletions src/features/instance/secrets/SecretModals.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { revealFieldErrorOnEnter } from '@/components/ui/form/revealFieldErrorOn
import { Input } from '@/components/ui/input';
import { Textarea } from '@/components/ui/textarea';
import { ENV_KEY_REGEX, ENV_VALUE_MASK } from '@/lib/env/envFile';
import { describeError } from '@/react-query/queryClient';
import { zodResolver } from '@hookform/resolvers/zod';
import { EyeIcon, Save, Trash2 } from 'lucide-react';
import { ReactNode, useCallback, useMemo, useState } from 'react';
Expand Down Expand Up @@ -117,7 +118,7 @@ export function AddSecretModal({
toast.success(`Secret "${formData.key}" saved.`);
setIsModalOpen(false);
} catch (error) {
toast.error(String(error));
toast.error(describeError(error).message);
}
},
[onSubmit, form, setIsModalOpen, delivery, tier, grants, resetDelivery],
Expand Down Expand Up @@ -281,7 +282,7 @@ export function EditSecretModal({
toast.success(`Secret "${name}" updated.`);
closeModal();
} catch (error) {
toast.error(String(error));
toast.error(describeError(error).message);
}
},
[onSave, name, closeModal, delivery, tier],
Expand All @@ -300,7 +301,7 @@ export function EditSecretModal({
toast.success(`Secret "${name}" deleted.`);
closeModal();
} catch (error) {
toast.error(String(error));
toast.error(describeError(error).message);
} finally {
setIsDeleting(false);
}
Expand Down
83 changes: 80 additions & 3 deletions src/integrations/api/instance/secrets/secrets.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ import type { AxiosInstance } from 'axios';
import { webcrypto } from 'node:crypto';
import { PropsWithChildren } from 'react';
import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest';
import { secretsPublicKeyQueryOptions, useDeleteSecret, useSetSecret } from './secrets';
import {
SecretsOperationError,
secretsPublicKeyQueryOptions,
useDeleteSecret,
useGrantSecret,
useRevokeSecret,
useSetSecret,
} from './secrets';

/*
Exercises the hdb_secret client flow against a mocked operations client and a REAL RSA keypair:
Expand Down Expand Up @@ -62,6 +69,17 @@ 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<unknown>, 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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
});
});
31 changes: 22 additions & 9 deletions src/integrations/api/instance/secrets/secrets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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);
}
}

Expand All @@ -218,7 +231,7 @@ async function mutateGrant(
const { data } = await instanceClient.post<GrantSecretResponse>('/', { operation, name, component });
return data;
} catch (error) {
throw new Error(operationErrorMessage(error));
throw operationError(error);
}
}

Expand Down
91 changes: 91 additions & 0 deletions src/integrations/datadog/beforeSend.sdk.test.ts
Original file line number Diff line number Diff line change
@@ -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<T extends Error>(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).');
});
});
Loading
Loading