From d6696cf02b0120ddb69779d9895eb783ebe603dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 9 May 2026 16:05:53 +0200 Subject: [PATCH] fix(logging): redactSecrets regex requires non-alphanumeric prefix to avoid false positives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original `/sk-(?:ant-)?[A-Za-z0-9_-]+/g` redacted any word ending in `sk-…`, including common English compounds in error messages and PR descriptions like `task-force`, `risk-management`, `disk-io`. Server logs ended up reading "ta[REDACTED] ABC" / "ri[REDACTED] note" / "di[REDACTED] 1234", which is unhelpful even though over-redaction is the safer-than-leaking failure mode. Tightens the regex to require either the start of the string OR a non-alphanumeric prefix character (whitespace, `=`, `{`, `,`, `:`, etc.) so the `sk-` token must actually start a secret, mirroring the v1.4.6 P12 idempotency-cache guard. Adds a minimum tail length of 8 characters to defend against `sk-1`-style false-positives in code identifiers. Capture group 1 is the leading separator (preserved in the substitution) so the redaction reads "apiKey=[REDACTED]" rather than "apiKey[REDACTED]". Adds tests covering the three v1.4.6-QA false-positive examples (task-force, risk-management, disk-io) plus mid-word negatives, and extends the positive coverage to confirm `sk-1234567890abcdef` and `sk-ant-1234567890abcdef` still redact correctly with various preceding separators. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/logging/__tests__/redact.test.ts | 33 ++++++++++++++++++++++++ src/lib/logging/redact.ts | 13 +++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/lib/logging/__tests__/redact.test.ts b/src/lib/logging/__tests__/redact.test.ts index b2783d170..45fcaf264 100644 --- a/src/lib/logging/__tests__/redact.test.ts +++ b/src/lib/logging/__tests__/redact.test.ts @@ -52,6 +52,39 @@ describe("redactSecrets", () => { ); }); + // v1.4.6 P11 hardening: the original `/sk-(?:ant-)?[A-Za-z0-9_-]+/` + // regex over-matched any word ending in `sk-…`, redacting common + // English compounds in error messages and PR descriptions. v1.5 + // tightens the regex to require a non-alphanumeric prefix so the + // `sk-` token must actually start a secret. + it("does NOT redact common English compounds containing 'sk-'", () => { + expect(redactSecrets("task-force ABC")).toBe("task-force ABC"); + expect(redactSecrets("risk-management note")).toBe("risk-management note"); + expect(redactSecrets("disk-io 1234")).toBe("disk-io 1234"); + // Mid-word matches must also be ignored (the `sk-` is preceded by a + // letter / digit, so it's part of an identifier, not a secret). + expect(redactSecrets("workflow:risk-management")).toBe( + "workflow:risk-management", + ); + expect(redactSecrets("e2e_disk-io_check")).toBe("e2e_disk-io_check"); + }); + + it("still redacts genuine sk- and sk-ant- API keys", () => { + // Start of string. + expect(redactSecrets("sk-1234567890abcdef")).toBe("[REDACTED]"); + expect(redactSecrets("sk-ant-1234567890abcdef")).toBe("[REDACTED]"); + // Preceded by whitespace, equals, brace, comma, etc. — the + // non-alphanumeric-prefix requirement. + expect(redactSecrets("apiKey=sk-1234567890abcdef end")).toBe( + "apiKey=[REDACTED] end", + ); + expect(redactSecrets("OPENAI_API_KEY=sk-prod-abcDEF123456789012")).toBe( + "OPENAI_API_KEY=[REDACTED]", + ); + // Trailing context preserved, leading separator preserved. + expect(redactSecrets(",sk-1234567890abcdef,")).toBe(",[REDACTED],"); + }); + it("handles multiple secrets in one string", () => { // Bearer matches `\S+` so it greedily consumes `hlk_x;` — that's // safe (over-redaction is fine when the alternative is leaking). diff --git a/src/lib/logging/redact.ts b/src/lib/logging/redact.ts index 6018dfa57..40218e9f7 100644 --- a/src/lib/logging/redact.ts +++ b/src/lib/logging/redact.ts @@ -26,7 +26,18 @@ export function redactSecrets(input: string): string { return input .replace(/Bearer\s+\S+/gi, "Bearer [REDACTED]") .replace(/bot\d+:[A-Za-z0-9_-]+/g, "bot[REDACTED]") - .replace(/sk-(?:ant-)?[A-Za-z0-9_-]+/g, "[REDACTED]") + // `sk-` is a common English-prefix substring (task-force, risk- + // management, disk-io). Without a word-boundary, the original + // `/sk-(?:ant-)?[A-Za-z0-9_-]+/g` over-matched and made server logs + // unreadable. Require the `sk-` token to be at the start of the + // string OR preceded by a non-alphanumeric character (whitespace, + // `=`, `{`, `,`, `:`, etc.). The minimum length-8 tail keeps a + // tiny safety margin against `sk-1`-style false-positives in code + // identifiers. Capture group 1 = the leading separator (preserved). + .replace( + /(^|[^A-Za-z0-9])sk-(?:ant-)?[A-Za-z0-9_-]{8,}/g, + "$1[REDACTED]", + ) .replace( /([?&])(secret|code|token|api[_-]?key)=[^&\s]+/gi, "$1$2=[REDACTED]",