diff --git a/packages/error-sink/README.md b/packages/error-sink/README.md index 03718b473..e1d898b5f 100644 --- a/packages/error-sink/README.md +++ b/packages/error-sink/README.md @@ -3,7 +3,9 @@ The structured convention every catch block reports a failure to instead of swallowing it (CL-6496 — the owner ruling this package exists to enforce): one function, `reportError(error, context)`, that never throws and returns -a `refId` a person can quote to support. +a `refId` a person can quote to support. It redacts (see below) and +preserves the error's `.cause` chain, depth-capped so a cyclic or +unbounded chain can't blow up the logger. ## Why this isn't a second logger @@ -23,6 +25,54 @@ structured-error shape (`operation`, optional `tenantId`/`roomId`/ registered once via `@intx/log`'s own `configureSync`/`setup` — no call site of `reportError` changes when that happens. +New code should always go through `reportError` rather than calling +`log.error` directly. The repo's existing raw `log.error` call sites predate +this package and are grandfathered, not a pattern to copy — they don't get +the redaction pass below, the `refId`, or the structured context shape. + +## What redaction does and doesn't catch + +`redactText`/`redactExtra` (`src/redact.ts`) are a heuristic pass, not a +general-purpose secret scanner — the bar is "never ships an obvious secret +in a common shape," not "catches every possible one." + +Caught: + +- A `Bearer ` fragment or an `Authorization: ` header fragment + anywhere in a string. +- Known provider key prefixes: `sk-`/`pk-`/`rk-`/`ghp-`/`gho-`/`ghu-`/`ghs-` + and their underscore variants (`ghp_...`, as GitHub actually issues them). +- A raw JWT (`eyJ...eyJ...`. shape) even with no keyword nearby — the + base64url header is distinctive enough to key off directly. +- `token=`, `secret=`, `password=`, `api_key=`, and similar assignments, + whether in a URL's query string or in a free-text message — only the + value becomes `[redacted]`, the param/field name stays. +- `code=` and `key=` specifically **only** when they appear as a URL query + param (right after a literal `?` or `&`, e.g. an OAuth callback's + `?code=...`). Elsewhere these two names are common non-secret shapes + (`code=404`, logfmt's `code=DB_TIMEOUT`, a cache `key=user:1234:profile`) + and are deliberately left untouched — see below. +- Any object key that itself looks credential-shaped (`token`, `secret`, + `password`, `apiKey`, `cookie`, ...) is redacted wholesale, including when + its value is an array or a nested object — `redactExtra` recurses through + both. + +Not caught, by design: + +- A raw secret string with no recognizable prefix, keyword, or shape sitting + under an unrelated key (e.g. a bare AWS access key in a field named + `values`). There is no reliable heuristic for this that doesn't also flag + ordinary IDs; put such values behind a credential-shaped key instead. +- `code=`/`key=` outside a URL query string. These two names are too + ambiguous with everyday non-secret shapes (an HTTP status `code=404`, + logfmt's `code=DB_TIMEOUT`, a cache `key=...`) to redact in free text + without regularly destroying debugging context — a log nobody can read is + a log nobody uses. If a call site genuinely has a bare secret under one of + these names outside a URL, put it in `extra` under a credential-shaped + key instead of interpolating it into the message string. +- Secrets embedded in non-string values (numbers, binary blobs) or inside a + JSON-serialized string that isn't itself parsed back into an object. + ## Retiring `@corbits/client-log` Out of scope for this unit, but the plan: migrate `apps/web`'s four diff --git a/packages/error-sink/src/index.test.ts b/packages/error-sink/src/index.test.ts index 6f7e4826d..8027b9d16 100644 --- a/packages/error-sink/src/index.test.ts +++ b/packages/error-sink/src/index.test.ts @@ -85,6 +85,27 @@ describe("reportError context capture", () => { const loggedError = records[0]?.message; expect(String(loggedError)).not.toContain("abc.def.ghi"); }); + + test("preserves the error's cause chain, redacted", () => { + const inner = new Error("rejected Bearer abc.def.ghi"); + const outer = new Error("wrapped failure", { cause: inner }); + reportError(outer, { operation: "op" }); + + const properties = records[0]?.properties as Record; + const loggedError = properties.error as Error; + expect(loggedError.cause).toBeInstanceOf(Error); + const cause = loggedError.cause as Error; + expect(cause.message).not.toContain("abc.def.ghi"); + expect(cause.message).toContain("[redacted]"); + }); + + test("caps a cyclic cause chain instead of recursing forever", () => { + const a = new Error("a"); + const b = new Error("b", { cause: a }); + a.cause = b; + + expect(() => reportError(b, { operation: "op" })).not.toThrow(); + }); }); describe("reportError never throws", () => { diff --git a/packages/error-sink/src/index.ts b/packages/error-sink/src/index.ts index 7d731b678..54f20b106 100644 --- a/packages/error-sink/src/index.ts +++ b/packages/error-sink/src/index.ts @@ -29,6 +29,9 @@ import { redactExtra, redactText } from "./redact"; import { generateRefId } from "./ref-id"; const UNKNOWN_OPERATION = "unknown"; +// A cyclic or unbounded `.cause` chain must not make the logger recurse +// forever; this caps how many links get carried over. +const MAX_CAUSE_DEPTH = 5; const log = getLogger(["errors"]); @@ -36,9 +39,12 @@ function asError(error: unknown): Error { return error instanceof Error ? error : new Error(String(error)); } -function redactedCopyOf(error: Error): Error { +function redactedCopyOf(error: Error, depth = 0): Error { const redacted = new Error(redactText(error.message)); if (error.stack !== undefined) redacted.stack = redactText(error.stack); + if (depth < MAX_CAUSE_DEPTH && error.cause !== undefined) { + redacted.cause = redactedCopyOf(asError(error.cause), depth + 1); + } return redacted; } diff --git a/packages/error-sink/src/redact.test.ts b/packages/error-sink/src/redact.test.ts index 813dd6454..e2d9dbe50 100644 --- a/packages/error-sink/src/redact.test.ts +++ b/packages/error-sink/src/redact.test.ts @@ -25,6 +25,62 @@ describe("redactText", () => { "could not reach the hub", ); }); + + test("redacts sensitive query-param values in a URL while keeping it readable", () => { + expect( + redactText( + "callback failed: https://api.example.com/cb?access_token=SECRETVALUE123&code=abc&state=xyz", + ), + ).toBe( + "callback failed: https://api.example.com/cb?access_token=[redacted]&code=[redacted]&state=xyz", + ); + }); + + test("redacts token= style assignments in free text", () => { + expect(redactText("failed request token=abc123xyz retries=3")).toBe( + "failed request token=[redacted] retries=3", + ); + }); + + test("redacts a raw JWT with no keyword prefix", () => { + const jwt = + "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dGhpc2lzbm90YXJlYWxzaWc"; + expect(redactText(`session restore failed for ${jwt}`)).toBe( + "session restore failed for [redacted]", + ); + }); + + test("redacts code=/key= only in query-string position, not free text", () => { + expect(redactText('code=404 message="Not Found"')).toBe( + 'code=404 message="Not Found"', + ); + expect( + redactText('level=error msg="db timeout" code=DB_TIMEOUT retries=3'), + ).toBe('level=error msg="db timeout" code=DB_TIMEOUT retries=3'); + expect(redactText("cache miss for key=user:1234:profile")).toBe( + "cache miss for key=user:1234:profile", + ); + expect(redactText("at /routes/key=handler.ts:12:5)")).toBe( + "at /routes/key=handler.ts:12:5)", + ); + }); + + test("redacts code=/key= when they appear as a URL query param", () => { + expect( + redactText( + "https://api.example.com/authorize?client_id=abc&code=SECRETCODE", + ), + ).toBe("https://api.example.com/authorize?client_id=abc&code=[redacted]"); + expect( + redactText("https://api.example.com/data?key=APIKEYVALUE&format=json"), + ).toBe("https://api.example.com/data?key=[redacted]&format=json"); + }); + + test("does not let the value group run past a stack-frame's trailing text", () => { + expect( + redactText("auth failed token=abc123).authenticate() at line 4"), + ).toBe("auth failed token=[redacted]).authenticate() at line 4"); + }); }); describe("redactExtra", () => { @@ -64,4 +120,12 @@ describe("redactExtra", () => { test("passes undefined through unchanged", () => { expect(redactExtra(undefined)).toBeUndefined(); }); + + test("redacts a raw secret string inside an array under a non-secret key", () => { + const jwt = + "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dGhpc2lzbm90YXJlYWxzaWc"; + expect(redactExtra({ sessions: [jwt, "plain-session-id"] })).toEqual({ + sessions: ["[redacted]", "plain-session-id"], + }); + }); }); diff --git a/packages/error-sink/src/redact.ts b/packages/error-sink/src/redact.ts index 0669476fa..fd76667f5 100644 --- a/packages/error-sink/src/redact.ts +++ b/packages/error-sink/src/redact.ts @@ -10,14 +10,56 @@ const SECRET_KEY_PATTERN = const SECRET_VALUE_PATTERNS: readonly RegExp[] = [ /bearer\s+\S+/gi, /authorization\s*:\s*\S+/gi, - /\b(sk|pk|rk|ghp|gho|ghu|ghs)-[a-z0-9]{8,}\b/gi, + // Provider key prefixes use either a hyphen (OpenAI's `sk-...`) or an + // underscore (GitHub's `ghp_...`) -- both must match. + /\b(sk|pk|rk|ghp|gho|ghu|ghs)[-_][a-z0-9]{8,}\b/gi, + // A raw JWT (header.payload.signature) carries no keyword prefix at all, + // but its base64url header always starts with the literal `eyJ` (base64 + // of `{"`), which is distinctive enough to key off heuristically. + /\beyJ[\w-]{10,}\.[\w-]{10,}\.[\w-]{10,}\b/g, ]; +// A value's character set once past `name=`: covers hex/base64/JWT-shaped +// tokens without running past the value into unrelated trailing text (a +// closing paren, a stack-frame's `:12:5)`, ...). +const ASSIGNMENT_VALUE = "[\\w.+/=%-]+"; + +// `token=`, `access_token=`, ... assignments: covers both a raw OAuth +// callback URL's query string (`?access_token=...`) and the same shape +// typed into a free-text error message. Only the value is replaced so the +// param name -- and the rest of the URL/message -- stays readable for +// debugging. These names are unambiguous: outside of a credential they +// don't ordinarily show up as a bare `name=value` assignment at all. +const SENSITIVE_ASSIGNMENT_PATTERN = new RegExp( + `\\b(access_token|refresh_token|id_token|api[-_]?key|apikey|secret|password|passwd|token|credential)(\\s*=\\s*)(${ASSIGNMENT_VALUE})`, + "gi", +); + +// `code` and `key` are NOT unambiguous -- `code=404`, logfmt's +// `code=DB_TIMEOUT`, and `key=user:1234:profile` are everyday non-secret +// shapes. The only place they reliably mean "secret" is a URL query string +// (an OAuth `code`, a `key=` API credential passed as a param), so these +// two are scoped to right after a literal `?` or `&`. +const QUERY_PARAM_SENSITIVE_ASSIGNMENT_PATTERN = new RegExp( + `([?&])(code|key)(\\s*=\\s*)(${ASSIGNMENT_VALUE})`, + "gi", +); + export function redactText(text: string): string { let redacted = text; for (const pattern of SECRET_VALUE_PATTERNS) { redacted = redacted.replace(pattern, "[redacted]"); } + redacted = redacted.replace( + SENSITIVE_ASSIGNMENT_PATTERN, + (_match, name: string, separator: string) => + `${name}${separator}[redacted]`, + ); + redacted = redacted.replace( + QUERY_PARAM_SENSITIVE_ASSIGNMENT_PATTERN, + (_match, prefix: string, name: string, separator: string) => + `${prefix}${name}${separator}[redacted]`, + ); return redacted; }