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;
}
Comment on lines +70 to 74

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The current implementation of operationError only extracts the error message if body.error is a string, or falls back to body.message. However, if body.error is a structured object, it can fail to extract the message and return the original error unwrapped. This prevents the error from being classified as a SecretsOperationError, causing Datadog to skip redaction and potentially leak sensitive secret names. To prevent displaying generic error messages or rendering '[object Object]', we should use a shared helper (like errorText) to extract the nested string message or fall back to JSON stringification.

Suggested change
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;
}
function operationError(error: unknown): unknown {
const body = (error as { response?: { data?: { error?: unknown; message?: unknown } } })?.response?.data;
const message = errorText(body?.error) ?? errorText(body?.message);
return message ? new SecretsOperationError(message) : error;
}
References
  1. When handling error messages that can be either strings or structured/nested objects, use a shared helper (like errorText) to extract the nested string message or fall back to JSON stringification. This prevents displaying generic error messages or rendering '[object Object]' in the UI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining. I checked the leak path and the response shape, and neither supports the finding:

  • Nothing leaks from the unwrapped error. The SDK reports a console.error(axiosError) as `${name}: ${message}`, which here is axios's own AxiosError: Request failed with status code N (@datadog/browser-core 7.15.0, cjs/domain/console/consoleObservable.js:66). computeRawError copies only the stack, cause chain, dd_fingerprint and dd_context (cjs/domain/error/error.js:39-66). The response body never reaches the event, so a secret name inside it can't either.
  • These operations don't send a structured error. Every failure in harper components/secretOperations.ts is a ClientError with a string message, and the operations error handler sends that back as { error: error.message } (harper server/serverHelpers/serverHandlers.js:83).
  • The suggestion would also break a case the PR already handles. errorText({ code: 'NOT_FOUND' }) returns that object's JSON, not undefined. So errorText(body?.error) ?? … would show the JSON instead of a string message next to it, which is the case 8884dee fixed and secrets.test.tsx "takes the string message when the error field is structured" tests.

🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.


/**
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
19 changes: 17 additions & 2 deletions src/integrations/datadog/beforeSend.sdk.test.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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<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;
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.';
Expand Down Expand Up @@ -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).');
});
});
10 changes: 10 additions & 0 deletions src/integrations/datadog/redactRelayedMessage.test.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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.',
Expand Down
13 changes: 10 additions & 3 deletions src/integrations/datadog/redactRelayedMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).';

Expand Down
Loading