From fe323be4c46cc9178b70ee7f66805dbdbb222aae Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Sat, 27 Jun 2026 21:30:43 +0530 Subject: [PATCH] fix(secrets): preserve alias-aware regex redactions --- .../coding-agent/src/secrets/obfuscator.ts | 38 ++++++++++--------- .../test/secrets-obfuscator.test.ts | 31 +++++++++++++++ 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/packages/coding-agent/src/secrets/obfuscator.ts b/packages/coding-agent/src/secrets/obfuscator.ts index 7854d93f5..982032837 100644 --- a/packages/coding-agent/src/secrets/obfuscator.ts +++ b/packages/coding-agent/src/secrets/obfuscator.ts @@ -80,9 +80,9 @@ function findNonMatchingReplacement(value: string, regex: RegExp): string | unde const len = value.length; if (len === 0) return undefined; const base = NONMATCHING_REPLACEMENT_CHARS.length; - // 91^2 = 8281 candidates cover every 1–2 char value (the only realistic - // trigger) exhaustively; the cap also bounds longer astronomical collisions. - const maxAttempts = 4096; + // Exhaust every 1–2 char candidate (the only realistic trigger). Longer + // collisions stay bounded to avoid pathological sweeps. + const maxAttempts = Math.max(4096, len <= 2 ? base ** len : 0); const chars = new Array(len); for (let n = 0; n < maxAttempts; n++) { let q = n; @@ -302,6 +302,16 @@ function placeholderWithoutFriendlyName(placeholder: string): string | undefined return match ? `#${match[1]}#` : undefined; } +function lookupFriendlyPlaceholderAlias( + deobfuscateMap: ReadonlyMap, + placeholder: string, +): { secret: string; recursive: boolean } | undefined { + const direct = deobfuscateMap.get(placeholder); + if (direct !== undefined) return direct; + const unprefixed = placeholderWithoutFriendlyName(placeholder); + return unprefixed !== undefined ? deobfuscateMap.get(unprefixed) : undefined; +} + const PENDING_PLACEHOLDER_SUFFIX_RE = /#(?:[A-Z0-9]+_)?[A-Z0-9]*(?::[ULCM]?)?$/; // Withhold a trailing run that could be the start of a placeholder from streamed @@ -592,18 +602,10 @@ export class SecretObfuscator { for (;;) { let shouldContinue = false; const next = result.replace(PLACEHOLDER_RE, match => { - const direct = this.#deobfuscateMap.get(match); - if (direct !== undefined) { - shouldContinue ||= direct.recursive; - return direct.secret; - } - const unprefixed = placeholderWithoutFriendlyName(match); - if (unprefixed) { - const mapped = this.#deobfuscateMap.get(unprefixed); - if (mapped !== undefined) { - shouldContinue ||= mapped.recursive; - return mapped.secret; - } + const mapped = lookupFriendlyPlaceholderAlias(this.#deobfuscateMap, match); + if (mapped !== undefined) { + shouldContinue ||= mapped.recursive; + return mapped.secret; } if (allowLegacy) { const legacy = this.#legacyDeobfuscateMap.get(match); @@ -777,7 +779,7 @@ export class SecretObfuscator { } #isGeneratedPlaceholder(placeholder: string): boolean { - return this.#generatedPlaceholders.has(placeholder); + return lookupFriendlyPlaceholderAlias(this.#deobfuscateMap, placeholder) !== undefined; } // Replace `search` with `replacement` outside known generated placeholders while @@ -1293,7 +1295,7 @@ function buildReplaceRegexScan( for (const range of ranges) { appendSegment(text.slice(cursor, range.start), cursor, range.start, false, false); const placeholder = text.slice(range.start, range.end); - const mapping = deobfuscateMap.get(placeholder); + const mapping = lookupFriendlyPlaceholderAlias(deobfuscateMap, placeholder); appendSegment(mapping?.secret ?? placeholder, range.start, range.end, true, mapping?.recursive ?? false); cursor = range.end; } @@ -1392,7 +1394,7 @@ function deobfuscateGeneratedPlaceholderRanges( const overlapEnd = Math.min(range.end, end); result += text.slice(cursor, overlapStart); const placeholder = text.slice(overlapStart, overlapEnd); - const mapping = deobfuscateMap.get(placeholder); + const mapping = lookupFriendlyPlaceholderAlias(deobfuscateMap, placeholder); result += mapping?.secret ?? placeholder; recursive ||= mapping?.recursive ?? false; cursor = overlapEnd; diff --git a/packages/coding-agent/test/secrets-obfuscator.test.ts b/packages/coding-agent/test/secrets-obfuscator.test.ts index dc3005616..62dc2dbab 100644 --- a/packages/coding-agent/test/secrets-obfuscator.test.ts +++ b/packages/coding-agent/test/secrets-obfuscator.test.ts @@ -662,6 +662,26 @@ describe("SecretObfuscator friendlyName placeholders", () => { expect(obf.obfuscate(out)).toBe(out); }); + it("redacts bounded regex remainders around renamed friendly placeholders", () => { + const sharedKey = "J".repeat(43); + const before = new SecretObfuscator([{ type: "plain", content: "abcdefgh", friendlyName: "old" }], sharedKey); + const oldToken = before.obfuscate("abcdefgh"); + const after = new SecretObfuscator( + [ + { type: "plain", content: "abcdefgh", friendlyName: "new" }, + { type: "regex", mode: "replace", content: "api_key=[A-Za-z0-9]{11}" }, + ], + sharedKey, + ); + + const out = after.obfuscate(`api_key=${oldToken}XYZ`); + + expect(out).toContain(oldToken); + expect(out).not.toContain("api_key="); + expect(out).not.toContain("XYZ"); + expect(after.obfuscate(out)).toBe(out); + }); + it("redacts default replace raw suffixes after prior placeholders", () => { const obf = new SecretObfuscator( [ @@ -758,6 +778,17 @@ describe("SecretObfuscator friendlyName placeholders", () => { expect(obf.obfuscate(out)).toBe(out); }); + it("exhausts two-character fallback candidates before keeping the sentinel", () => { + const obf = new SecretObfuscator([{ type: "regex", mode: "replace", content: "[A-Za-z0-9]." }], "Q".repeat(43)); + + const out = obf.obfuscate("ZZ"); + + expect(out).not.toBe("ZZ"); + expect(out).toHaveLength(2); + expect(/[A-Za-z0-9]./.test(out)).toBe(false); + expect(obf.obfuscate(out)).toBe(out); + }); + it("keeps the sentinel only when no same-length value avoids the regex", () => { // A match-everything regex has no nonmatching same-length redaction, so the // search exhausts and the sentinel is kept as the sole fixed point. Such a