diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 748de0bf3..07b2006ce 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -31,6 +31,8 @@ - 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. - Fixed a regex-discovered secret's `friendlyName` collision check testing the already-sanitized (uppercased, separator-stripped) label against the configured regex instead of the label as written, so a case-sensitive or punctuated pattern (e.g. `tok_[a-z0-9]+`) missed a `friendlyName` that was itself a live match for that regex (e.g. `"tok_abc123"`), stamping the matched token — minus separators — into every placeholder (`#TOKABC123_…#`). The regex check now runs against the raw, pre-sanitization label, matching how the regex would actually encounter that text verbatim. - Fixed the forged friendly-name-alias placeholder guard missing a case- or flag-variant occurrence of a regex-discovered secret: it only checked the dropped alias prefix against exact previously-discovered secret strings, so a differently-cased match of a case-insensitive pattern (e.g. `content: "tok[a-z0-9]+", flags: "i"` discovering lowercase `tokabc123`) never landed in the exact-match set under its uppercase form, letting a forged `#TOKABC123_#` be waved through as already-redacted and leave `TOKABC123` provider-visible. The guard now also tests the dropped prefix directly against every configured regex pattern. +- Fixed `secrets.yml`-loaded regex `friendlyName` entries pre-sanitizing the label before it reached `#friendlyNameCollidesWithSecret`'s raw-label regex check (the fix above), silently defeating it for every config-file-loaded entry — only entries constructed programmatically with a raw string were actually protected. The loader now preserves the original, unsanitized `friendlyName` string (still validating that it sanitizes to something non-empty), deferring sanitization to the obfuscator as before. +- Fixed the forged friendly-name-alias guard comparing a normalized (alnum-only, uppercased) dropped prefix against RAW plain-secret values, so a lowercase or punctuated configured secret's normalized rendering (e.g. `GITHUBPATABC123` for `github_pat_abc123`) slipped past both the obfuscate-direction guard and, more severely, an equivalent unguarded fallback in `deobfuscate()` — restoring a forged `#GITHUBPATABC123_#` straight to that OTHER secret's raw value on live provider-output/tool-call-argument paths, with no check at all. Both guards now normalize the compared secret values the same way before accepting the alias fallback, and `deobfuscate()` gained the same secret-shaped-prefix check `obfuscate()` already had. ## [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 ff3cb0455..6b53be2f5 100644 --- a/packages/coding-agent/src/secrets/index.ts +++ b/packages/coding-agent/src/secrets/index.ts @@ -175,18 +175,24 @@ async function loadSecretsFile(filePath: string): Promise { } } +// Validates the friendlyName but returns it UNSANITIZED: `SecretObfuscator`'s +// own `#createPlaceholder` sanitizes it again and, critically, needs the raw +// string for `#friendlyNameCollidesWithSecret`'s regex-collision check — a +// case-sensitive/punctuated regex pattern (e.g. `tok_[a-z0-9]+`) only matches +// the label as it was actually written, not an already-uppercased, +// separator-stripped rendering of it. Pre-sanitizing here would silently +// defeat that check for every `secrets.yml`-loaded entry. function loadFriendlyName(entry: RawSecretEntry, filePath: string, index: number): string | undefined { if (entry.friendlyName === undefined) return undefined; if (typeof entry.friendlyName !== "string") { logger.warn(`secrets.yml[${index}]: friendlyName must be a string`, { path: filePath }); return undefined; } - const friendlyName = sanitizeSecretFriendlyName(entry.friendlyName); - if (!friendlyName) { + if (sanitizeSecretFriendlyName(entry.friendlyName) === undefined) { logger.warn(`secrets.yml[${index}]: friendlyName must contain at least one letter or digit`, { path: filePath }); return undefined; } - return friendlyName; + return entry.friendlyName; } function validateEntry(entry: unknown, filePath: string, index: number): entry is RawSecretEntry { diff --git a/packages/coding-agent/src/secrets/obfuscator.ts b/packages/coding-agent/src/secrets/obfuscator.ts index 9f6821c1a..a2dd998ce 100644 --- a/packages/coding-agent/src/secrets/obfuscator.ts +++ b/packages/coding-agent/src/secrets/obfuscator.ts @@ -274,6 +274,18 @@ export function sanitizeSecretFriendlyName(name: string): string | undefined { return sanitized.length > 0 ? sanitized : undefined; } +/** + * Normalize a secret value into the same alnum-only, uppercased shape a + * friendly-name label or placeholder prefix is sanitized into, so comparing a + * raw (possibly lowercase/punctuated) secret value against already-sanitized, + * model-visible text does not miss a case- or separator-only variant. Unlike + * `sanitizeSecretFriendlyName` this never truncates and never signals "empty" + * via `undefined` — callers already guard on `.length > 0` before comparing. + */ +function sanitizeForCollisionCheck(value: string): string { + return value.replace(/[^A-Za-z0-9]/g, "").toUpperCase(); +} + /** * Whether an entry needs the persisted placeholder key: either because it can * produce a reversible (keyed) obfuscate-mode placeholder, or because a default @@ -863,13 +875,33 @@ export class SecretObfuscator { return this.#deobfuscate(text, true); } + // Reverse-direction counterpart to `#isGeneratedPlaceholder`'s guard: the + // bare-alias fallback below intentionally accepts ANY prefix so a + // placeholder minted under a renamed friendly name still deobfuscates + // (see `#prefixIsSecretShaped`'s docstring for why), but unconditionally + // stripping and ignoring an attacker-authored prefix would let a forged + // token like `#GITHUBPATABC123_#` + // restore to that OTHER secret's raw value with no check at all — worse + // than the obfuscate-direction leak, since deobfuscation is what feeds + // tool-call arguments and provider-output restoration. Refuse the + // fallback (leave the token as opaque, unresolved text) when the prefix + // is itself secret-shaped; an exact full-token match is unaffected, since + // that was minted by this instance and carries no forgery risk. + #lookupLiveAlias(placeholder: string): { secret: string; recursive: boolean } | undefined { + const direct = this.#deobfuscateMap.get(placeholder); + if (direct !== undefined) return direct; + const match = /^#([A-Z0-9]+)_([A-Z0-9]{4,}(?::[ULCM])?)#$/.exec(placeholder); + if (match === null || this.#prefixIsSecretShaped(match[1]!)) return undefined; + return this.#deobfuscateMap.get(`#${match[2]}#`); + } + #deobfuscate(text: string, allowLegacy: boolean): string { if (!this.#hasAny || !text.includes("#")) return text; let result = text; for (;;) { let shouldContinue = false; const next = result.replace(PLACEHOLDER_RE, match => { - const mapped = lookupFriendlyPlaceholderAlias(this.#deobfuscateMap, match); + const mapped = this.#lookupLiveAlias(match); if (mapped !== undefined) { shouldContinue ||= mapped.recursive; return mapped.secret; @@ -1175,7 +1207,7 @@ export class SecretObfuscator { // secret. #friendlyNameCollidesWithSecret(sanitizedName: string, rawName: string): boolean { for (const secretValue of this.#configuredSecretValues) { - const sanitizedSecret = secretValue.replace(/[^A-Za-z0-9]/g, "").toUpperCase(); + const sanitizedSecret = sanitizeForCollisionCheck(secretValue); if (sanitizedSecret.length > 0 && sanitizedName.includes(sanitizedSecret)) return true; } for (const entry of this.#regexEntries) { @@ -1201,45 +1233,52 @@ export class SecretObfuscator { } } - // Exact match always qualifies. Otherwise fall back to the friendly-name- - // independent bare alias: needed so a placeholder minted under a NOW- - // renamed friendly name (same secret, same key, different `secrets.yml` - // label) still round-trips when older provider-visible text is re-scanned - // by a renamed-config instance — the hash suffix is a keyed digest of the - // secret VALUE alone, so a same-key instance recomputes it identically - // regardless of the label. The dropped prefix is otherwise unconstrained - // text, though: an attacker who has observed ANY live placeholder's hash - // suffix elsewhere in the transcript could wrap it around a DIFFERENT - // real secret's plaintext to make the whole token look pre-redacted and - // smuggle that secret through untouched. Refuse the alias fallback when the - // dropped prefix contains a configured plain secret's literal value, any - // regex-discovered secret's value this instance has ever minted a - // placeholder for (`#obfuscateMappings` — regex secrets are found - // dynamically, so they are never in `#configuredSecretValues`), OR is - // itself matched by a configured regex pattern directly — a case- or - // flag-variant occurrence (e.g. `content: "tok[a-z0-9]+", flags: "i"` - // discovering lowercase `tokabc123`) never lands in `#obfuscateMappings` - // under its differently-cased form, but the pattern itself still flags it, - // so a forged `#TOKABC123_#` must not be waved through as - // already-redacted. Either check means the plain-secret/regex pass still - // catches it instead of skipping the whole span. - #isGeneratedPlaceholder(placeholder: string): boolean { - if (this.#deobfuscateMap.has(placeholder)) return true; - const match = /^#([A-Z0-9]+)_([A-Z0-9]{4,}(?::[ULCM])?)#$/.exec(placeholder); - if (match === null) return false; - const prefix = match[1]!; + // Whether an alnum-only, uppercase friendly-name-shaped prefix dropped from + // a candidate placeholder token is itself something that should have been + // redacted, rather than an arbitrary label: a sanitized form of a + // configured plain secret's value, a sanitized form of any regex- + // discovered secret's value this instance has ever minted a placeholder + // for, or text a configured regex pattern matches directly. Shared by the + // obfuscate-direction guard below and the deobfuscate-direction bare-alias + // guard in `#deobfuscate` — both fall back to a friendly-name-independent + // alias keyed only by the hash suffix, so both need the same defense + // against a forged/attacker-chosen prefix. + #prefixIsSecretShaped(prefix: string): boolean { for (const secretValue of this.#configuredSecretValues) { - if (secretValue.length > 0 && prefix.includes(secretValue)) return false; + const sanitizedSecret = sanitizeForCollisionCheck(secretValue); + if (sanitizedSecret.length > 0 && prefix.includes(sanitizedSecret)) return true; } for (const { secret } of this.#obfuscateMappings.values()) { - if (secret.length > 0 && prefix.includes(secret)) return false; + const sanitizedSecret = sanitizeForCollisionCheck(secret); + if (sanitizedSecret.length > 0 && prefix.includes(sanitizedSecret)) return true; } for (const entry of this.#regexEntries) { entry.regex.lastIndex = 0; const matches = entry.regex.test(prefix); entry.regex.lastIndex = 0; - if (matches) return false; + if (matches) return true; } + return false; + } + + // A placeholder is an exact match, or the friendly-name-independent bare + // alias: needed so a placeholder minted under a NOW-renamed friendly name + // (same secret, same key, different `secrets.yml` label) still round-trips + // when older provider-visible text is re-scanned by a renamed-config + // instance — the hash suffix is a keyed digest of the secret VALUE alone, + // so a same-key instance recomputes it identically regardless of the + // label. The dropped prefix is otherwise unconstrained text, though: an + // attacker who has observed ANY live placeholder's hash suffix elsewhere + // in the transcript could wrap it around a DIFFERENT real secret's + // plaintext (or a normalized rendering of one) to make the whole token + // look pre-redacted and smuggle that secret through untouched. + // `#prefixIsSecretShaped` above guards against that, shared with the + // deobfuscate-direction check in `#deobfuscate`. + #isGeneratedPlaceholder(placeholder: string): boolean { + if (this.#deobfuscateMap.has(placeholder)) return true; + const match = /^#([A-Z0-9]+)_([A-Z0-9]{4,}(?::[ULCM])?)#$/.exec(placeholder); + if (match === null) return false; + if (this.#prefixIsSecretShaped(match[1]!)) return false; return this.#deobfuscateMap.has(`#${match[2]}#`); } diff --git a/packages/coding-agent/test/secrets-obfuscator.test.ts b/packages/coding-agent/test/secrets-obfuscator.test.ts index edd9a0127..fc069b9e9 100644 --- a/packages/coding-agent/test/secrets-obfuscator.test.ts +++ b/packages/coding-agent/test/secrets-obfuscator.test.ts @@ -2244,6 +2244,48 @@ describe("SecretObfuscator friendlyName placeholders", () => { expect(out).not.toContain("TOKABC123"); }); + it("refuses deobfuscate()'s bare-alias fallback for a forged secret-shaped prefix, but still honors a genuine rename", () => { + // Regression: `#lookupLiveAlias`'s bare-alias fallback (`#PREFIX_HASH#` → + // strip prefix → look up bare `#HASH#`) used to accept ANY prefix + // unconditionally as long as the bare hash suffix belonged to a real + // placeholder. On the live provider-output / tool-call-argument restore + // path, an attacker who observed any real placeholder's bare hash suffix + // elsewhere in a transcript could wrap it in a forged prefix that is a + // sanitized/normalized rendering of a configured secret's own value (or a + // different secret's), and deobfuscate() would restore that OTHER + // secret's raw value in its place — worse than the obfuscate-direction + // leak defended against above, since this is the path that reconstitutes + // real secrets for outbound tool calls. The fix reuses + // `#prefixIsSecretShaped` (shared with `#isGeneratedPlaceholder`) to + // refuse the fallback whenever the dropped prefix is itself + // secret-shaped, while a genuine friendly-name rename — a prefix that + // matches no configured secret value or pattern — still round-trips. + const secret = "github_pat_abc123"; + const obfuscator = new SecretObfuscator([{ type: "plain", content: secret }]); + + const real = obfuscator.obfuscate(secret); + const suffix = /#([A-Z0-9]{4,}(?::[ULCM])?)#/.exec(real)?.[1]; + expect(suffix).toBeDefined(); + + // Forge a prefix that is the sanitized rendering of the SAME configured + // secret's own value, wrapped around the real bare-alias suffix. + const forged = `run tool with #GITHUBPATABC123_${suffix}# now`; + const restored = obfuscator.deobfuscate(forged); + expect(restored).not.toContain(secret); + expect(restored).toContain("#GITHUBPATABC123_"); + + // A genuine rename is unaffected: the OLD friendly-name prefix is not + // itself secret-shaped, so the bare-alias fallback still resolves it to + // the same secret under the RENAMED (equally non-secret-shaped) prefix. + const renameObfuscator = new SecretObfuscator([ + { type: "plain", content: "some-other-secret-value", friendlyName: "OldName" }, + ]); + const currentToken = renameObfuscator.obfuscate("some-other-secret-value"); + expect(currentToken).toMatch(/^#OLDNAME_[A-Z0-9]+:L#$/); + const renameSuffix = currentToken.replace(/^#OLDNAME/, ""); + expect(renameObfuscator.deobfuscate(`#NEWNAME${renameSuffix}`)).toBe("some-other-secret-value"); + }); + it("drops a friendly name that contains another configured secret's literal value", () => { // Regression: a friendlyName is baked verbatim into every placeholder // minted for its secret (`#NAME_hash:hint#`) via an EXACT @@ -2570,6 +2612,40 @@ describe("SecretObfuscator friendlyName placeholders", () => { await fs.rm(root, { recursive: true, force: true }); } }); + + it("preserves the raw friendlyName from secrets.yml so a regex entry's self-collision check still catches it", async () => { + // Regression: `loadFriendlyName` used to return the SANITIZED (uppercased, + // separator-stripped) friendlyName, which then got stored verbatim as + // `entry.friendlyName` in the loaded `SecretEntry`. That defeated the + // regex-entry self-collision check above (see "rejects a regex entry + // friendlyName that is itself a live match for its own pattern"), which + // must test the RAW label against the configured pattern: a + // case-sensitive/punctuated pattern like `tok_[a-z0-9]+` can never match + // an already-uppercased, separator-stripped rendering of itself. The fix + // still validates the friendlyName but returns it unsanitized. + 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 content: "tok_[a-z0-9]+"\n friendlyName: "tok_abc123"\n', + ); + + const entries = await loadSecrets(project, agentDir); + expect(entries[0]?.friendlyName).toBe("tok_abc123"); + + const obfuscator = new SecretObfuscator(entries); + const obfuscated = obfuscator.obfuscate("use tok_abc123 now"); + + expect(obfuscated).not.toMatch(/TOKABC123_/); + expect(obfuscated).toMatch(/^use #[A-Z0-9]+:L# now$/); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); }); describe("SecretObfuscator cross-turn cache stability", () => {