diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 365b67a18..7ab80c751 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -29,6 +29,7 @@ - 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. +- 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. ## [16.3.8] - 2026-07-05 diff --git a/packages/coding-agent/src/secrets/obfuscator.ts b/packages/coding-agent/src/secrets/obfuscator.ts index 00453f153..123edc58f 100644 --- a/packages/coding-agent/src/secrets/obfuscator.ts +++ b/packages/coding-agent/src/secrets/obfuscator.ts @@ -1087,7 +1087,9 @@ export class SecretObfuscator { // it; the secret still gets a bare (unprefixed) placeholder. const requestedFriendlyName = friendlyName ? sanitizeSecretFriendlyName(friendlyName) : undefined; const sanitizedFriendlyName = - requestedFriendlyName !== undefined && !this.#friendlyNameCollidesWithSecret(requestedFriendlyName) + requestedFriendlyName !== undefined && + friendlyName !== undefined && + !this.#friendlyNameCollidesWithSecret(requestedFriendlyName, friendlyName) ? requestedFriendlyName : undefined; const preferredBase = this.#resolvePreferredPlaceholderBase(baseKey); @@ -1158,21 +1160,27 @@ 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 - // sanitized (alnum-only, uppercased) form of a configured plain secret's - // value — the same normalization already applied to the label itself, so a + // alias guard in `#isGeneratedPlaceholder`. Reject when the SANITIZED label + // contains a sanitized (alnum-only, uppercased) form of a configured plain + // secret's value — comparing both sides through the same normalization 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 { + // separator noise — or when any configured regex pattern matches the RAW + // (pre-sanitization) label. The regex check uses the raw label, not the + // sanitized one: a regex describes what a real secret occurrence looks like + // verbatim in text, so the question is whether this label's own literal + // characters are a value the regex would have redacted, not whether some + // unrelated case/punctuation-stripped rendering of it happens to match a + // pattern that never described that rendering. Either check means the text + // is meant to be redacted, not stamped unredacted onto every use of this + // secret. + #friendlyNameCollidesWithSecret(sanitizedName: string, rawName: string): boolean { for (const secretValue of this.#configuredSecretValues) { const sanitizedSecret = secretValue.replace(/[^A-Za-z0-9]/g, "").toUpperCase(); - if (sanitizedSecret.length > 0 && name.includes(sanitizedSecret)) return true; + if (sanitizedSecret.length > 0 && sanitizedName.includes(sanitizedSecret)) return true; } for (const entry of this.#regexEntries) { entry.regex.lastIndex = 0; - const matches = entry.regex.test(name); + const matches = entry.regex.test(rawName); entry.regex.lastIndex = 0; if (matches) return true; } diff --git a/packages/coding-agent/test/secrets-obfuscator.test.ts b/packages/coding-agent/test/secrets-obfuscator.test.ts index b25ecf928..4b40c1de8 100644 --- a/packages/coding-agent/test/secrets-obfuscator.test.ts +++ b/packages/coding-agent/test/secrets-obfuscator.test.ts @@ -358,6 +358,30 @@ describe("SecretObfuscator friendlyName placeholders", () => { expect(obfuscator.deobfuscate(obfuscated)).toBe(input); }); + it("rejects a regex entry friendlyName that is itself a live match for its own pattern", () => { + // `#friendlyNameCollidesWithSecret` used to run each regex entry's pattern + // against the SANITIZED friendly name (uppercased, non-alphanumeric + // stripped) instead of the raw one. A case-sensitive/punctuated pattern + // like `tok_[a-z0-9]+` requires a literal lowercase underscore, so it could + // never match the sanitized label "TOKABC123" even though the raw + // friendlyName "tok_abc123" is itself a live match for that very pattern. + // That let a secret-shaped friendlyName slip through and get stamped + // (minus separators, uppercased) into every placeholder minted for it. The + // check now runs the pattern against the raw, pre-sanitization + // friendlyName, so this case is caught and the secret falls back to a bare + // placeholder. + const secret = "tok_abc123"; + const obfuscator = new SecretObfuscator([ + { type: "regex", content: "tok_[a-z0-9]+", friendlyName: "tok_abc123" }, + ]); + const input = `use ${secret} now`; + const obfuscated = obfuscator.obfuscate(input); + + expect(obfuscated).not.toMatch(/TOKABC123_/); + expect(obfuscated).toMatch(/^use #[A-Z0-9]+:L# now$/); + expect(obfuscator.deobfuscate(obfuscated)).toBe(input); + }); + it("does not replace plain secrets inside generated friendly placeholders", () => { const longSecret = "long-secret-token"; const prefixSecret = "TOKENABC";