From 7d3a3a23a358c3c8ef553a05e2d53b0a60ed0ca9 Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Mon, 6 Jul 2026 05:05:26 +0530 Subject: [PATCH] fix(coding-agent): reject unresolvable regex fallback and sanitize friendly-name collision check Two Codex P2 findings on commit 732f725: - A default (no custom replacement) mode: "replace" regex that cannot escape a 1-2 char match (e.g. ".", "[\\s\\S]", "[\\s\\S]{2}") had its key-derived same-length fallback marker returned without checking it against the matched value. Since that marker is drawn from an alphabet the regex has already proven to match exhaustively, a real 1-2 byte secret coinciding with it would ship unredacted. Such entries are now rejected: dropped with a warning when loaded from secrets.yml, dropped silently as a construction-time backstop otherwise. - #friendlyNameCollidesWithSecret compared the sanitized (uppercased, alnum-only) friendly name against each secret's raw value, so a friendlyName that was a lowercase or punctuated variant of its own secret slipped through and stamped most of the secret into the placeholder. The secret value is now sanitized the same way before comparing. --- packages/coding-agent/CHANGELOG.md | 2 + packages/coding-agent/src/secrets/index.ts | 13 ++- .../coding-agent/src/secrets/obfuscator.ts | 70 +++++++++++++-- .../test/secrets-obfuscator.test.ts | 90 +++++++++++++------ 4 files changed, 141 insertions(+), 34 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 8a52007ac..365b67a18 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -27,6 +27,8 @@ - Fixed a configured secret's `friendlyName` being able to bake another live secret's literal value straight into every placeholder minted for it (e.g. `{ content: "ABCDEFGH", friendlyName: "LEAKTOKEN" }` alongside a secret covering `LEAKTOKEN`), which then read as an already-generated placeholder on an exact match and was never scanned. `obfuscate()` now drops the friendly-name prefix for a given secret whenever the sanitized name contains a configured plain secret's literal or is matched by a configured regex, independent of `entries[]` order (regex patterns are now compiled before any placeholder is minted). - Fixed a default (no custom `replacement`) `mode: "replace"` regex with no same-length candidate able to escape it (a pathological match-everything config such as `.`/`[\s\S]`) emitting a 1–2 byte matched value unchanged when it was exactly `Z` or `ZZ`: the fallback reused `#generateReplacement`'s shared `Z`/`ZZ` sentinel for such short values, and a raw match of exactly that sentinel round-tripped to the same bytes, reaching the provider unredacted (plain `mode: "replace"` secrets already avoided this via `ensureDistinctReplacement`, but the regex fallback did not). The fallback now uses a same-length, key-derived run instead of the sentinel for <=2 char values — still a fixed point under re-obfuscation (depends only on the per-install key and the value's length, never its content, so re-matching and re-redacting it reproduces the identical marker), but no longer a public, install-independent constant a regex config could be tuned to bypass. - Fixed `getSecretPlaceholderKey()`/`getExistingSecretPlaceholderKey()` defaulting their `keyDir` parameter to `getConfigRootDir()` (`~/.omp`) while `createAgentSession()` always passes the profile-scoped `agentDir` (`~/.omp/agent`, per `docs/secrets.md`) explicitly. A caller relying on the default read or minted a key file at a different path than live sessions use, so placeholders it created were never stable against SDK sessions. Both helpers now default to `getAgentDir()`. +- Fixed a default (no custom `replacement`) `mode: "replace"` regex that cannot escape a 1–2 byte match (e.g. `.`, `[\s\S]`, `[\s\S]{2}`) still risking an unredacted round-trip: the previous key-derived same-length fallback marker was returned without checking it against the matched value, and since that marker is drawn from an alphabet the regex has already proven to match exhaustively, a real 1–2 byte secret that happened to equal it would ship to the provider unchanged. Such regex entries are now rejected outright — dropped with a warning when loaded from `secrets.yml`, dropped silently as a construction-time backstop otherwise — since no same-length marker can be guaranteed distinct from every possible match once the regex is proven to match every candidate in that alphabet. +- Fixed a plain secret's `friendlyName` being accepted as a placeholder prefix when it was a lowercase or punctuated variant of the secret's own value (e.g. `friendlyName: "github_pat_abc123"` for a secret of the same content): the collision check compared the already-sanitized (uppercased, alphanumeric-only) friendly name against the secret's raw, case-sensitive value, so a case/punctuation variant slipped through and stamped most of the secret into every placeholder (`#GITHUBPATABC123_…#`). The secret value is now sanitized the same way before the comparison. ## [16.3.8] - 2026-07-05 diff --git a/packages/coding-agent/src/secrets/index.ts b/packages/coding-agent/src/secrets/index.ts index fe02e8457..ff3cb0455 100644 --- a/packages/coding-agent/src/secrets/index.ts +++ b/packages/coding-agent/src/secrets/index.ts @@ -3,7 +3,7 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; import { getAgentDir, isEnoent, logger } from "@oh-my-pi/pi-utils"; import { YAML } from "bun"; -import { type SecretEntry, sanitizeSecretFriendlyName } from "./obfuscator"; +import { regexHasUnresolvableShortMatchFallback, type SecretEntry, sanitizeSecretFriendlyName } from "./obfuscator"; import { compileSecretRegex } from "./regex"; const PLACEHOLDER_KEY_RE = /^[A-Za-z0-9_-]{43}$/; @@ -216,8 +216,9 @@ function validateEntry(entry: unknown, filePath: string, index: number): entry i return false; } if (e.type === "regex") { + let regex: RegExp; try { - compileSecretRegex(e.content as string, e.flags as string | undefined); + regex = compileSecretRegex(e.content as string, e.flags as string | undefined); } catch (error) { logger.warn(`secrets.yml[${index}]: invalid regex pattern`, { path: filePath, @@ -226,6 +227,14 @@ function validateEntry(entry: unknown, filePath: string, index: number): entry i }); return false; } + const mode = (e.mode as "obfuscate" | "replace" | undefined) ?? "obfuscate"; + if (mode === "replace" && e.replacement === undefined && regexHasUnresolvableShortMatchFallback(regex)) { + logger.warn( + `secrets.yml[${index}]: regex matches every 1-2 character candidate with no custom replacement, so a match can never be redacted distinctly from itself`, + { path: filePath, pattern: e.content }, + ); + return false; + } } return true; } diff --git a/packages/coding-agent/src/secrets/obfuscator.ts b/packages/coding-agent/src/secrets/obfuscator.ts index 2bdbf6d7f..00453f153 100644 --- a/packages/coding-agent/src/secrets/obfuscator.ts +++ b/packages/coding-agent/src/secrets/obfuscator.ts @@ -206,6 +206,33 @@ function findWhitespaceFallbackReplacement( return undefined; } +/** + * Whether a default (no custom `replacement`) replace-mode regex can never + * safely redact a 1-2 char match: `findNonMatchingReplacement`'s bounded + * search — the same search `#generateRegexReplacement` runs at match time — + * finds no candidate the regex fails to re-match. This holds independent of + * any actual per-install key: the search already exhausts every character in + * `REPLACEMENT_CHARS` (the alphabet `buildKeyedReplacementRun` draws its + * fallback marker from) plus punctuation and whitespace, so if none of those + * escape the regex, no key-derived marker drawn from the same alphabet can + * either — the marker is guaranteed to re-match too, making every such match + * unresolvable: the fallback could only ever emit the raw matched text + * unchanged. Probed with a value (`"\0".repeat(length)`) the bounded search + * never treats as a real candidate, so the result depends only on the + * regex's own matching behavior, not on this specific probe. + */ +export function regexHasUnresolvableShortMatchFallback(regex: RegExp): boolean { + return ([1, 2] as const).some(length => { + const probe = "\u0000".repeat(length); + const savedLastIndex = regex.lastIndex; + try { + return findNonMatchingReplacement(probe, regex, { text: probe, start: 0, end: length }) === undefined; + } finally { + regex.lastIndex = savedLastIndex; + } + }); +} + // ═══════════════════════════════════════════════════════════════════════════ // Placeholder format // ═══════════════════════════════════════════════════════════════════════════ @@ -586,9 +613,24 @@ export class SecretObfuscator { continue; } try { + const regex = compileSecretRegex(entry.content, entry.flags); + const mode = entry.mode ?? "obfuscate"; + // A default (no custom `replacement`) replace-mode regex that can + // never redact a 1-2 char match distinctly from itself (see + // `regexHasUnresolvableShortMatchFallback`) is dropped rather than + // risk a real secret round-tripping unredacted; `secrets/index.ts` + // warns loudly for the `secrets.yml`-loaded path — this is the + // silent backstop for direct construction. + if ( + mode === "replace" && + entry.replacement === undefined && + regexHasUnresolvableShortMatchFallback(regex) + ) { + continue; + } this.#regexEntries.push({ - regex: compileSecretRegex(entry.content, entry.flags), - mode: entry.mode ?? "obfuscate", + regex, + mode, replacement: entry.replacement, friendlyName: entry.friendlyName, }); @@ -938,6 +980,16 @@ export class SecretObfuscator { * than a universal constant — the same class of accepted risk * `generateDeterministicReplacement`'s hash collision already carries for longer * values. + * A regex that is UNCONDITIONALLY pathological for a length <= 2 — matching + * literally every candidate in isolation, independent of context, like `.` + * or `[\s\S]` — is now rejected at construction/config-load time instead + * (see `regexHasUnresolvableShortMatchFallback`), since for that narrow + * case the residual risk above is fully avoidable rather than merely + * unlikely. This branch, and the residual risk above, still applies to a + * length > 2 pathological config and to a length <= 2 pattern that is only + * pathological in a SPECIFIC match's surrounding context (the + * construction-time check tests the regex in isolation, not every context + * it could appear in). */ #generateRegexReplacement(value: string, regex: RegExp, context: RegexMatchContext): string { let replacement = generateDeterministicReplacement(value); @@ -1106,13 +1158,17 @@ export class SecretObfuscator { // verbatim, model-visible prefix on every placeholder minted for THIS // secret, baked in via an exact `#deobfuscateMap` entry rather than the // alias fallback — so it needs its own check independent of the scan-skip - // alias guard in `#isGeneratedPlaceholder`. Reject when the label contains - // a configured plain secret's literal value, or when any configured regex - // pattern matches the label — either means that text is meant to be - // redacted, not stamped unredacted onto every use of this secret. + // alias guard in `#isGeneratedPlaceholder`. Reject when the label contains a + // sanitized (alnum-only, uppercased) form of a configured plain secret's + // value — the same normalization already applied to the label itself, so a + // lowercase or punctuated secret cannot smuggle itself through case or + // separator noise — or when any configured regex pattern matches the label + // — either means that text is meant to be redacted, not stamped unredacted + // onto every use of this secret. #friendlyNameCollidesWithSecret(name: string): boolean { for (const secretValue of this.#configuredSecretValues) { - if (secretValue.length > 0 && name.includes(secretValue)) return true; + const sanitizedSecret = secretValue.replace(/[^A-Za-z0-9]/g, "").toUpperCase(); + if (sanitizedSecret.length > 0 && name.includes(sanitizedSecret)) return true; } for (const entry of this.#regexEntries) { entry.regex.lastIndex = 0; diff --git a/packages/coding-agent/test/secrets-obfuscator.test.ts b/packages/coding-agent/test/secrets-obfuscator.test.ts index bc1842835..b25ecf928 100644 --- a/packages/coding-agent/test/secrets-obfuscator.test.ts +++ b/packages/coding-agent/test/secrets-obfuscator.test.ts @@ -317,6 +317,36 @@ describe("SecretObfuscator friendlyName placeholders", () => { expect(obfuscator.deobfuscate(obfuscated)).toBe(input); }); + it("rejects a friendlyName that is a case/punctuation variant of its own secret, but still applies unrelated friendly names", () => { + // `#friendlyNameCollidesWithSecret` used to compare the sanitized (uppercased, + // alnum-only) friendly name against each secret's RAW value, so a friendlyName + // that was merely a case/punctuation variant of its own secret (e.g. + // "GitHub_Pat_Abc123" labeling "github_pat_abc123") slipped through: sanitizing + // the label produced "GITHUBPATABC123", which never literally appears inside the + // lowercase, underscored raw secret string. The fix sanitizes the secret value + // the same way before comparing, so this exact-content-under-normalization case + // is now caught and the secret falls back to a bare placeholder — while a + // genuinely unrelated friendly name is untouched and still gets its prefix. + const collidingSecret = "github_pat_abc123"; + const collidingObfuscator = new SecretObfuscator([ + { type: "plain", content: collidingSecret, friendlyName: "GitHub_Pat_Abc123" }, + ]); + const collidingObfuscated = collidingObfuscator.obfuscate(collidingSecret); + + expect(collidingObfuscated).not.toMatch(/GITHUBPATABC123_/); + expect(collidingObfuscated).toMatch(/^#[A-Z0-9]+:L#$/); + expect(collidingObfuscator.deobfuscate(collidingObfuscated)).toBe(collidingSecret); + + const distinctSecret = "github_pat_xyz789"; + const distinctObfuscator = new SecretObfuscator([ + { type: "plain", content: distinctSecret, friendlyName: "GitHub Token" }, + ]); + const distinctObfuscated = distinctObfuscator.obfuscate(distinctSecret); + + expect(distinctObfuscated).toMatch(/^#GITHUBTOKEN_[A-Z0-9]+:L#$/); + expect(distinctObfuscator.deobfuscate(distinctObfuscated)).toBe(distinctSecret); + }); + it("uses regex entry friendly names for discovered matches", () => { const secret = "tok_abc123"; const obfuscator = new SecretObfuscator([{ type: "regex", content: "tok_[a-z0-9]+", friendlyName: "API Key" }]); @@ -1413,36 +1443,26 @@ describe("SecretObfuscator friendlyName placeholders", () => { expect(obf.obfuscate(out)).toBe(out); }); - it("never emits a <=2 char match unchanged when no same-length value avoids the regex", () => { - // A match-everything regex has no nonmatching same-length redaction, so the - // search exhausts. The fallback must still differ from a <=2 char value that - // happens to equal `#generateReplacement`'s own `Z`/`ZZ` sentinel (else the - // raw secret would ship unchanged) while staying a fixed point under - // re-obfuscation. Such a config redacts every character and is pathological - // by construction. - for (const content of [".", "[\\s\\S]"]) { + it("drops a replace regex entirely when it can never redact a 1-2 char match distinctly from itself", () => { + // A match-everything regex has no nonmatching same-length redaction for a + // 1-2 char value, so `findNonMatchingReplacement` would exhaust even against + // an isolated probe drawn from the fallback's own alphabet — the regex has + // already proven it matches literally every candidate that alphabet could + // produce. Accepting the entry would mean the only "redaction" available is + // the raw match returned unchanged, which is worse than not registering the + // entry at all. The constructor now drops such entries silently at + // construction (`regexHasUnresolvableShortMatchFallback`), so with no other + // configured secret, `hasSecrets()` is false and the input passes through + // completely unobfuscated. + for (const content of [".", "[\\s\\S]", "[\\s\\S]{2}"]) { const obf = new SecretObfuscator([{ type: "regex", mode: "replace", content }], "Q".repeat(43)); + const input = content === "[\\s\\S]{2}" ? "ZZ" : "Z"; - const out = obf.obfuscate("Z"); - - expect(out).not.toBe("Z"); - expect(out).toHaveLength(1); - expect(obf.obfuscate(out)).toBe(out); + expect(obf.hasSecrets()).toBe(false); + expect(obf.obfuscate(input)).toBe(input); } }); - it("never emits a 2-char match unchanged when no same-length value avoids the regex", () => { - // Same pathology as above, sized to hit the `ZZ` half of `#generateReplacement`'s - // sentinel rather than the `Z` half. - const obf = new SecretObfuscator([{ type: "regex", mode: "replace", content: "[\\s\\S]{2}" }], "Q".repeat(43)); - - const out = obf.obfuscate("ZZ"); - - expect(out).not.toBe("ZZ"); - expect(out).toHaveLength(2); - expect(obf.obfuscate(out)).toBe(out); - }); - it("resolves a three-character match-everything regex without the removed exhaustive sweep", () => { // Regression: `findNonMatchingReplacement` used to special-case `len <= 3` // with a fully exhaustive search over every `base**length` candidate @@ -2473,6 +2493,26 @@ describe("SecretObfuscator friendlyName placeholders", () => { await fs.rm(root, { recursive: true, force: true }); } }); + + it("rejects a secrets.yml replace regex entry that can never redact a 1-2 char match distinctly from itself", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "omp-secret-friendly-")); + try { + const project = path.join(root, "project"); + const agentDir = path.join(root, "agent"); + await fs.mkdir(path.join(project, ".omp"), { recursive: true }); + await fs.mkdir(agentDir, { recursive: true }); + await fs.writeFile( + path.join(project, ".omp", "secrets.yml"), + '- type: regex\n mode: replace\n content: "."\n', + ); + + const entries = await loadSecrets(project, agentDir); + + expect(entries).toEqual([]); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); }); describe("SecretObfuscator cross-turn cache stability", () => {