Skip to content

fix(datadog): keep error causes that would need redaction out of Error Tracking - #1799

Merged
dawsontoth merged 5 commits into
stagefrom
claude/1665-redact-error-causes
Oct 12, 2026
Merged

dawsontoth merged 5 commits into
stagefrom
claude/1665-redact-error-causes

Conversation

@dawsontoth

@dawsontoth dawsontoth commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

A raccoon postmaster holding back letters whose notes hold secrets

📦 This PR now carries #1806 too

Rebased into 5 linear, signed commits on current stage, with no merge commits: #1799 (keep error causes that would need redaction out of Error Tracking, Closes #1665) and #1806 (withhold failed secrets-operation text, #1664).

Rebase: the only conflict was RELAYED_ERROR_TYPES in redactRelayedMessage.ts, plus its test. stage had added DeployRolloutError (from #1868) to the same set where #1806 adds SecretsOperationError. The set now holds all four types, the doc comment names each one, and both tests stay. Every other file's changes are identical to what was reviewed. Gate on the final tree: tsc -b, oxlint, dprint check, vitest 474/474 files.


⊙ Problem

The RUM SDK flattens an error's cause chain into error.causes[] and sends it. beforeSend never read that field, so a cause's message and stack skipped every redaction layer: URL-path redaction, credential params, and the relayed-type withhold (#1665). Nothing leaks today. In the last 30 days, 4 of 4,407 production error events carried causes, and all four were an AbortError with no customer text in it. But a future { cause } that wraps server or customer text would have gone to Error Tracking as written.

Closes #1665.

❓ Your call: Is defense-in-depth worth a change to beforeSend when nothing leaks today? I think it is: Harper's error guidelines tell us to chain causes, so new ones will appear, and #1664 (stacked on this) adds another relayed type. The alternative is to close #1665 as accepted risk. This is easy to reverse.

💡 Solution

The issue suggests walking causes[] and redacting each entry. That can't work. error.causes is not on the SDK's modifiable-field allowlist, and the SDK's limitModification puts back whatever beforeSend writes there. The new SDK-pipeline test shows the rewrite being undone.

So beforeSend now checks each cause instead of rewriting it. It withholds the whole event when any of these is true:

  • A cause's message or stack would change under the rules the top-level text already gets (redactErrorText and redactCredentialParams).
  • The error, or an earlier link in its chain, is a relayed type (SSEOperationError, ImportJobError). The flag is seeded from the error's own type and only ever set further down the chain. A relayed error's causes are the server's chain, so they are withheld just like its message.
  • A cause is not in the SDK's own shape. That shape (CAUSE_FIELDS) is source, type, message and stack, each a string or absent, with message required. If a future SDK starts recording a new field, no rule here covers it, so the event is withheld. The SDK-pipeline test then fails on that bump.

Clean causes ship unchanged. I replayed the 4 production cause-bearing events through the new beforeSend: all 4 were kept, with their causes untouched. The check runs after shouldKeepEvent and inside the existing try/catch. A malformed or hostile cause therefore returns false instead of escaping into the SDK's fail-open wrapper. An event with no causes costs one property read.

⚠️ Look hardest: the check withholds the whole event, not just the cause. That is the only option the SDK leaves, but it means such an error is not counted in Error Tracking at all.

⚖️ Alternatives

  • Strip cause at Studio's throw sites. This misses unhandled errors and third-party errors that never pass through Studio code. It also contradicts the team's cause-chaining convention.
  • pnpm patch the SDK allowlist so causes can be rewritten. The SDK's lockfile entry has changed 12 times since July. A patch is pinned to one version, and the repo's last vendor patch (router-core) had to be retargeted and then removed.
  • Drop every event that has causes. The planning review suggested this as the do-less option. It would lose the 4 clean SSEInconclusiveError events and protect nothing more.

❓ Your call: Should we ask Datadog to add error.causes[].message and .stack to the beforeSend allowlist? That would let this become a rewrite that keeps the event. I have not filed it, because it would be a public post on DataDog/browser-sdk.

🔧 Changes

  • src/integrations/datadog/beforeSend.ts: adds the call that runs the cause check only when causes is present, and causesNeedNoRedaction, which holds the field allowlist, the shape checks, and the relayed flag that passes down the chain.
  • src/integrations/datadog/redactRelayedMessage.ts: exports isRelayed, so the relayed-type decision stays in one module.
  • src/integrations/datadog/shouldKeepEvent.ts: adds causes to DatadogErrorEvent.

✅ Verification

  • End-to-end route: an SDK-pipeline integration test. The new beforeSend.sdk.test.ts runs the installed SDK 7.15.0's real error collection, assembly and limitModification through to RUM_EVENT_COLLECTED. It shows that an error with a clean cause is sent, that an error with an unsafe cause sends nothing, and that a beforeSend rewrite of a cause is undone. That last case pins the premise of the whole fix.
  • Unit tests in beforeSend.test.ts cover a message, a stack, or a credential that needs redaction; a relayed error, a relayed cause, and a cause below a relayed one; a 10-link chain; empty and non-Error causes being kept; and ten malformed shapes plus a throwing getter. Each malformed shape returns false and none throws.
  • Fails on base: run against origin/stage's beforeSend, 17 of the new tests fail, including the SDK-pipeline test.
  • Mutation checks: I reverted each guard separately (the call, the field allowlist, the value-type check, the required message, the relayed seed, and the relayed propagation). Every revert turned at least one test red.
  • Corpus replay: the 4 cause-bearing production events from the last 30 days (pulled with pup) all went through the real beforeSend and were kept, with causes unchanged. That evidence is off-repo.
  • Gates: the full vitest suite passed (396 files, 3,687 tests, run by the pre-commit hook). tsc -b, oxlint and dprint check all exited 0.
  • Not covered: the planning review suggested a stage rollout check (emit a synthetic clean cause and an unsafe one on stage, then confirm in RUM which one arrives). That needs a deploy.

Cross-model review. A planning round (codex) returned chosen-approach-sound. I adopted its two refinements: malformed shapes fail closed, and the SDK-pipeline test is committed. Two implementation rounds ran with codex and gemini. Two legs failed for infrastructure reasons: the domain adjudicator (Claude OAuth expired) and Cursor (the 1Password SSH agent refused to sign). With no adjudicator, I triaged the outside findings myself:

  • Taken: the field allowlist (gemini, major) and removal of a dated test comment.
  • Rejected:
    • gemini's "blocker" that a throwing getter escapes beforeSend. The check runs inside the existing try/catch, and the throwing-getter test passes.
    • The .ts import-extension nit. Imports in this repo are extensionless.
    • The per-cause array allocation. It only happens on the rare path, at most 10 links.
    • The test-comment nits.

Stacked: the #1664 PR builds on this branch.

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

🤖 Generated with Claude Code

Related PRs: #1666 overlaps (merged; added the relayed-type withhold this extends to causes)
Complexity: medium

Review-Coverage: authored=claude; ran=codex,gemini; blocked=cursor-composer(no-receipt),domain(auth); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=2; full=1 @ e3eaa8c

Review-Attention: read ~3m (raised: degraded review) @ e3eaa8c

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces validation for the error.causes chain within the Datadog integration's beforeSend hook. Because error.causes is unmodifiable in the SDK, any event containing a cause that requires redaction or is relayed is dropped entirely to prevent leaking sensitive information. The changes include implementing the causesNeedNoRedaction helper, updating the DatadogErrorEvent interface, and adding comprehensive unit and integration tests. There are no review comments, so I have no feedback to provide.

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 73.35% 12161 / 16578
🔵 Statements 73.44% 12953 / 17637
🔵 Functions 68.35% 3240 / 4740
🔵 Branches 69.07% 9576 / 13864
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/features/instance/config/secrets/SecretGrantsEditor.tsx 0% 0% 0% 0% 30-74
src/features/instance/secrets/SecretModals.tsx 91.66% 94.11% 92.3% 91.66% 128-130, 285, 304
src/integrations/api/instance/secrets/secrets.ts 90.9% 88.23% 85% 90.47% 133-140
src/integrations/datadog/beforeSend.ts 100% 100% 100% 100%
src/integrations/datadog/redactRelayedMessage.ts 93.75% 92.85% 100% 93.75% 76
src/integrations/datadog/shouldKeepEvent.ts 100% 100% 100% 100%
Generated in workflow #2279 for commit 9d5fe20 by the Vitest Coverage Report Action

dawsontoth and others added 5 commits October 12, 2026 16:38
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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
…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>
@dawsontoth
dawsontoth added this pull request to stack #1921 October 12, 2026 20:40
@dawsontoth
dawsontoth marked this pull request as ready for review October 12, 2026 20:40
@dawsontoth
dawsontoth requested a review from a team as a code owner October 12, 2026 20:40
@dawsontoth
dawsontoth force-pushed the claude/1665-redact-error-causes branch from e3eaa8c to 1e05581 Compare October 12, 2026 20:42
@dawsontoth
dawsontoth merged commit bf6dcaa into stage Oct 12, 2026
7 checks passed
@dawsontoth
dawsontoth deleted the claude/1665-redact-error-causes branch October 12, 2026 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RUM] error.causes[] bypasses every redaction layer in beforeSend

1 participant