Repository navigation
fix(datadog): withhold failed secrets-operation text, which quotes secret names - #1806
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new SecretsOperationError class and updates error handling for secrets operations to ensure that sensitive secret names are redacted in Datadog. It also replaces generic error stringifications with describeError(error).message for user-facing toast notifications and adds corresponding tests. Feedback was provided regarding a potential sensitive data leak in operationError; if body.error is a structured object, it may fail to be wrapped in SecretsOperationError, thereby bypassing Datadog redaction. A suggestion was made to use a helper like errorText to safely extract the 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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
- 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.
There was a problem hiding this comment.
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 ownAxiosError: Request failed with status code N(@datadog/browser-core7.15.0,cjs/domain/console/consoleObservable.js:66).computeRawErrorcopies only the stack,causechain,dd_fingerprintanddd_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 harpercomponents/secretOperations.tsis aClientErrorwith a string message, and the operations error handler sends that back as{ error: error.message }(harperserver/serverHelpers/serverHandlers.js:83). - The suggestion would also break a case the PR already handles.
errorText({ code: 'NOT_FOUND' })returns that object's JSON, notundefined. SoerrorText(body?.error) ?? …would show the JSON instead of a stringmessagenext to it, which is the case 8884dee fixed andsecrets.test.tsx"takes the string message when the error field is structured" tests.
🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.
…cret 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…ructured 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 <noreply@anthropic.com>
8884dee to
9d5fe20
Compare
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Stacked on #1799 (studio#1665). The base is
claude/1665-redact-error-causes, so this diff shows only the #1664 change. Merge #1799 first, then retarget this PR tostage.⊙ Problem
A failed secrets operation (set, delete, grant or revoke) rejected with a plain
Errorcarrying Harper's response text. React Query's global handler passes that toconsole.error, and the RUM SDK reports it, so Harper's text reached Error Tracking, where it is retained and searchable. Harper builds those messages by interpolating the secret's and component's names:No secret found with name '<name>', andSecret '<name>' cannot be both processEnv and grant-scoped(harpercomponents/secretOperations.ts). When #1664 was filed, there were no such events in the previous 7 days of production. The exposure is latent, the same class #1768 fixed for import jobs.Closes #1664.
💡 Solution
This follows #1768's
ImportJobErrorpattern:SecretsOperationErrorfrom all three throw sites:set_secret,delete_secret, and grant/revoke. Itsnameis set as a literal, so minification can't change it.RELAYED_ERROR_TYPES, sobeforeSendwithholds the message and the stack header, exactly as it does forSSEOperationErrorandImportJobError. Studio's own frames are kept.error, ormessageiferroris not a string) becomes aSecretsOperationError. A network failure, a bug, or a non-string body is rethrown unchanged, so Error Tracking keeps its own type and message. The kid-mismatch self-heal still reads Harper's message before the throw.cause. Under fix(datadog): keep error causes that would need redaction out of Error Tracking #1799, a cause on a relayed error would withhold the whole event, and with it the failure rate the relay type exists to keep.Also changed: the secrets dialogs and the grants editor toasted
String(error), which would now readSecretsOperationError: …. All five toasts now showdescribeError(error).message, the same text the global handler shows (SecretModals,SecretGrantsEditor).SecretModalsis shared with the env-var editor, whose failures are rawAxiosErrors. Its toasts change too: they now show Harper's response text (or axios's own message when there is none) instead ofAxiosError: Request failed with status code ….🔧 Changes
src/integrations/api/instance/secrets/secrets.ts: addsSecretsOperationErrorandoperationError, which decides between Harper's text and the original failure, and calls them at the three throw sites (set, delete, grant/revoke).src/integrations/datadog/redactRelayedMessage.ts: adds the type toRELAYED_ERROR_TYPESand documents why it belongs there.src/features/instance/secrets/SecretModals.tsx(add-secret toast; the edit and delete toasts are the same one-line change) andsrc/features/instance/config/secrets/SecretGrantsEditor.tsx(grant toast, and revoke): the five toasts now show the message without the class name.✅ Verification
SecretsOperationErrorgoes through the installed SDK's real collection and assembly with Studio'sbeforeSend(beforeSend.sdk.test.ts). The event is sent with the message withheld, which proves the typebeforeSendkeys on is the one the SDK reads off the thrown class.secrets.test.tsx: set, delete, grant and revoke each reject with aSecretsOperationErrorcarrying Harper's text, checked byinstanceofand byname. A network error is rethrown unchanged on every path. A structurederrorfield with a stringmessagestill yields the string.redactRelayedMessage.test.ts: the message and stack header are withheld, and the frames are kept.SecretModals.test.tsx: a failed save toasts the bare message.nameassignment, the pass-through, and the toast extractor. The grants-editor toasts use the same one-line extractor but have no test of their own.tsc -b,oxlintanddprint checkall exited 0. The pre-commit hook's full suite passed at the head commit (396 files, 3,694 tests). One intermediate commit (af418d6) was made with--no-verify. On that run the hook's full suite failed twice on two unrelated timeouts,editorApi.test.tsandNewApplication.test.tsx, while the machine's load average was about 86. Both files pass in isolation. I ran that commit's other hook steps (oxlint, staged dprint, commitlint) by hand, and they passed.Cross-model review. Three rounds ran against base
claude/1665-redact-error-causes, with codex and gemini in each. Codex's verdict on the final round was LGTM, with no findings. The domain adjudicator failed on auth (Claude OAuth expired), and Cursor failed when the 1Password SSH agent refused to sign. I triaged the findings myself:instanceofas well asname.AxiosError's response body, so the old path did not leak, but the classification was wrong.describeErrormight return no.message. It always returns all three fields, falling back to "We had some trouble!".🤖 Generated by Claude Code (Anthropic Claude); posted via @dawsontoth.
🤖 Generated with Claude Code
Related PRs: #1799 overlaps (this PR is stacked on it; same SDK test and relayed-type module), #1768 overlaps (merged; the ImportJobError pattern this mirrors), #1666 overlaps (merged; introduced the relayed-type withhold)
Complexity: medium
Review-Coverage: authored=claude; ran=gemini,codex; blocked=cursor-grok(no-receipt),cursor-composer(no-receipt),domain(auth); declined=cursor-kimi,cursor-muse; rounds=3; full=1 @ 8884dee
Review-Attention: study ~15m (sensitive: SecretGrantsEditor.tsx, SecretModals.tsx +1; raised: degraded review) @ 8884dee