Withhold server-composed error text from Datadog Error Tracking - #1666
Open
dawsontoth wants to merge 6 commits into
Open
Withhold server-composed error text from Datadog Error Tracking#1666dawsontoth wants to merge 6 commits into
dawsontoth wants to merge 6 commits into
Conversation
A customer's private repository owner and name reached Datadog inside a deploy failure. Harper composes those messages by interpolating the reference the customer entered, so the identifier arrives in whatever shape the import field accepts -- including free text with spaces -- and the RUM SDK repeats the whole message above the frames of the stack, where two kilobytes of git usage text came with it. Redaction rules cannot bound that: each one is a guess about where the reference ends, and the reference is whatever was typed. The error's type is the reliable signal instead. SSEOperationError is by construction relayed from the server, so none of its text is Studio's to publish, and all of it goes. The event is kept. Its rate is how an instance-side failure stays visible, and the stack keeps the frames that are ours. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A relayed stderr line can be shaped like a frame -- "at build step @ acme/svc" -- and the bare owner/repo in its tail is not URL-shaped, so redactErrorText does not backstop it. Anchor the tail the way FRAME_URL already does. Also pin the name SSEOperationError carries, since a mismatch there withholds nothing and every other test stays green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ped lines Selecting the lines that look like frames leaves any the server put there: a customer pastes a newline and "at x @ https://..." into the import field, Harper interpolates it, and a line-by-line rule keeps it. Removing exactly the message leaves nothing to decide, and drops the whole stack when the shape is unfamiliar. Verified against the 22 SSEOperationError events in the last week of production RUM: every stack begins with its message, and nothing but Studio's own frames survives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
startsWith('') is true, so an absent message took the branch meant to fail
closed and emitted the whole raw stack.
The SDK prefixes the error name on the handled path but not on the unhandled
one, so accept either header. Without this an unhandled deploy rejection kept
the event but lost every Studio frame with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A message that is itself a prefix of the error name matched the bare form first and left the rest of the name -- and the message -- in the retained stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces redaction logic for relayed error messages and stack traces (specifically SSEOperationError) before they are sent to Datadog, preventing sensitive customer-supplied inputs from being leaked. It includes comprehensive unit tests for these changes. The reviewer suggested normalizing line endings (CRLF to LF) in the stack and message before performing prefix checks to avoid mismatches on different platforms.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
dawsontoth
marked this pull request as ready for review
August 28, 2026 21:41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A customer's private GitHub repository owner and name reached Datadog Error Tracking, in a deploy failure Studio can do nothing about. Found in the 2026-08-28 daily RUM review.
The leak
Harper composes its operation errors by interpolating the reference the customer entered —
Failed to clone package ${packageIdentifier}: ${cloneStderr}and its siblings in harpercomponents/Application.js.derivePackageIdentifierrewrites any bareowner/repotyped into the import field asgithub:owner/repofirst, so the most ordinary entry there is comes back quoted in the failure.redactErrorTextclassifies references by URL token and by scp-style remote. The shorthand is neither, so it passed through. And the RUM SDK writes the message above the frames of the stack, so the same text arrived twice — one production stack carried the repository name and two kilobytes ofgit cloneusage output.Why this doesn't pattern-match the message
The first attempt did, and it was the wrong shape of solution. Every rule of that kind is a guess about where the reference ends, and the reference is whatever the customer typed — free text with spaces included. Successive review rounds each broke a different input: a quoted reference lost its delimiter, a branch name containing
)leaked its suffix, a compact JSON payload lost its neighbouring field, a reference carrying its own colon leaked the remainder. Tightening the boundary moved the failure rather than removing it.The error's type is the reliable signal.
SSEOperationErroris by construction relayed from the server (errors.ts:44 sets the name RUM records aserror.type), so everything it carries is either the server's text or one of Studio's own constants for the same failure. None of it is ours to publish, so all of it goes — including shapes Harper has never emitted yet.The stack is handled by removing exactly the message span, not by keeping the lines that look like frames: server text can be shaped like a frame, and any rule deciding line-by-line keeps it.
For the human reviewer
Error Tracking can no longer tell these causes apart. A forge outage, a full disk, and a typo'd repository now look identical — same message, grouped as one issue. The event is deliberately kept so the rate still moves, but the discriminating detail is gone.
SSEOperationErroralready carriescodeandphase, which would restore it without the prose; they are not on the RUM event today, and wiring them throughaddErroris more than a privacy fix should carry. If you'd rather have that first, this should wait for it.An unrecognised stack shape loses Studio's frames. When the stack doesn't begin with its message the whole thing is withheld, so the event keeps no file or line. That is the safe direction and it is fail-closed, but it is a real debuggability cost on a path no test can prove absent.
The gate is one string literal. If the SDK stops mapping
nametoerror.type, redaction silently becomes a no-op with every test still green. A test pinsnew SSEOperationError('').nameagainst the literal, which covers the half of that risk living in this repo; the SDK half is inherent.Two pre-existing siblings are filed, not fixed here — #1664 (
secrets.tsrelays Harper's response text through a plainError, outside the gate) and #1665 (error.causes[]bypasses every redaction layer).Verification
Route: unit tests, plus replaying real production events through
beforeSend. Not observable end to end — the filter runs inside the RUM SDK and the failure needs a customer repository.All 22
SSEOperationErrorevents from the last 7 days of production RUM were replayed throughbeforeSend; nothing survives but Studio's own source-mapped frames. That replay is also what caught my own wrong assumption: RUM'serror.messagealready carries theSSEOperationError:prefix, so an earlier version that rebuilt the header as`${type}: ${message}`matched 0 of 22 real events while its unit tests passed.Every guard is mutation-checked — removing it, or reverting it to a previous round's form, turns a specific test red:
withholds the whole stack when the message is emptyremoves the whole header when the message is a prefix of the error nameremoves the message when the SDK prefixed only the stackGates green on every commit via the pre-commit hook: 331 test files, 2,763 tests,
tsc, oxlint, dprint, commitlint.Scale
5 events from 1 session in 24 h, 22 in 7 days. This is a privacy fix, not a noise fix — the volume is small and the exposure is not.
Complexity: moderate — one new module and a two-line wiring change, but the reasoning behind the design is the substance.
Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=11 @ 6cfde78
Human-Review-Need: 4 @ 6cfde78