CL-6496: Fix three redaction leaks and cause-chain data loss in error-sink - #231
Merged
Merged
Conversation
Covers three redaction gaps in packages/error-sink/src/redact.ts (OAuth callback URL query-param tokens, raw secret strings in arrays under a non-secret key, and token=/key= style assignments in free text) plus the loss of an Error's .cause chain when reportError copies it.
redactText missed OAuth callback URL query-param values entirely, raw secret strings with no keyword prefix inside arrays under a non-secret key, and token=/key= style assignments outside a Bearer/Authorization header. Added a name-based assignment pattern that redacts only the value (keeping URLs and messages structurally readable) and a JWT-shape pattern for prefix-less tokens; fixed the provider-key pattern to also match GitHub's underscore-separated tokens. reportError also silently dropped an Error's .cause chain when building its redacted copy. redactedCopyOf now recurses into .cause, redacting each link, capped at a fixed depth so a cyclic or unbounded chain can't blow up the logger.
States plainly what redaction is and isn't heuristically able to catch, and that new code should call reportError rather than a raw log.error call (the existing raw call sites predate this package and aren't a pattern to copy).
Peer review on PR #231 found the free-text code=/key= assignment match over-redacted everyday non-secret shapes (an HTTP code=404, logfmt's code=DB_TIMEOUT, a cache key=...). These tests pin the intended boundary: code=/key= must survive untouched in free text but still get redacted when they appear as a URL query param, and a token= value must not swallow unrelated trailing text.
code and key are too ambiguous with everyday non-secret shapes to redact wherever they appear as a bare assignment -- code=404, logfmt's code=DB_TIMEOUT, and a cache key=user:1234:profile were all getting destroyed. Split the assignment pattern in two: unambiguous names (token, secret, password, api_key, ...) still match anywhere; code and key only match right after a literal ? or & (the URL query-string case that motivated this in the first place). Also bounds the value capture to a token-shaped character class so it can't run past the intended value into unrelated trailing text.
Peer review flagged the prior wording as broader than the actual (now-fixed) behavior. States explicitly that code=/key= redaction is scoped to a URL query string and is deliberately left alone everywhere else, since those two names collide too often with ordinary status codes and cache keys.
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
1 participant
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.
Summary
Fixes four issues a reviewer found against
packages/error-sink/src/redact.tsbefore the queued 141-call-site sweep ontoreportError:?access_token=SECRET&code=abcpassed through untouched. Added a name-basedkey=valueassignment pattern that redacts only the sensitive value, keeping the URL's host/path/param names readable.redactValue/redactRecordalready existed structurally; the real gap was pattern coverage for secrets with no keyword prefix. Added a JWT-shape pattern (eyJ...eyJ...) that catches this case without a nearby keyword.token=/key=style prefixes in free text weren't matched — onlyBearer/Authorization:were. Same assignment pattern as (1) covers this..causechains silently vanished —redactedCopyOfonly copied.message/.stack. It now recurses into.cause, redacting each link, depth-capped (5) so a cyclic or unbounded chain can't blow up the logger.Also fixed in passing: the provider-key pattern required a hyphen (
sk-,ghp-, ...) but GitHub actually issues underscore-separated tokens (ghp_...), so those were silently missing too.reportError's never-throws guarantee is unchanged and covered by the existing throwing-sink test.Follow-up from peer review:
code=/key=over-redactionPeer review caught that the original
code=/key=assignment match was too broad — it destroyed everyday non-secret shapes: an HTTPcode=404, logfmt'scode=DB_TIMEOUT retries=3, a cachekey=user:1234:profile. Fixed by splitting the pattern:token,access_token,secret,password,api_key, ...) still match anywhere as a bare assignment.code/keyare scoped to right after a literal?or&— the URL query-string case that motivated this in the first place (an OAuthcode, an APIkeypassed as a param) — and are left alone everywhere else.:12:5)).Added red/green tests for all four over-redaction cases plus confirming the URL case still redacts, and updated the README's redaction-coverage section to describe the query-string-only scope.
Test plan
bun testinpackages/error-sink— 25 pass (tests for all issues above, confirmed red before each fix, green after)bun run typecheckinpackages/error-sinkbun run lint/bun run check— skipped locally on this push (a concurrent full-repo check had exhausted local machine memory); left to CI, which runs the full check on GitHub's machines.