fix(coding-agent): normalize plain-secret alias checks, preserve raw friendlyName, guard deobfuscate against forged aliases
Codex P2 findings on commit 029e838:
- secrets/index.ts:189: loadFriendlyName pre-sanitized the friendlyName
before storing it on the SecretEntry, silently defeating the raw-label
regex collision check for every secrets.yml-loaded entry. The loader now
preserves the original, unsanitized string (still validating it sanitizes
to something non-empty).
- obfuscator.ts:1232: the forged-alias guard (isGeneratedPlaceholder)
compared the dropped prefix against RAW plain-secret values, so a
lowercase/punctuated secret's normalized rendering slipped through. Both
the plain-secret-value and obfuscateMappings loops now normalize the
compared value the same way the prefix is already constrained to.
Self-discovered while verifying the above: deobfuscate()'s bare-alias
fallback had NO prefix validation at all (unlike obfuscate()'s guard),
so a forged token wrapping any real placeholder's hash suffix in a
secret-shaped prefix would restore to that secret's raw value on the
live provider-output/tool-call-argument path -- strictly worse than the
obfuscate-direction leak. Extracted the shared check into
#prefixIsSecretShaped and reused it in a new #lookupLiveAlias gate for
deobfuscate(), verified a genuine friendly-name rename still round-trips.
This commit is contained in:
Mathews-Tom
@@ -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_<observed-suffix>#` 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_<suffix-copied-from-any-real-placeholder>#` 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
|
||||
|
||||
|
||||
@@ -175,18 +175,24 @@ async function loadSecretsFile(filePath: string): Promise<SecretEntry[]> {
|
||||
}
|
||||
}
|
||||
|
||||
// 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 {
|
||||
|
||||
@@ -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_<suffix-copied-from-any-real-placeholder>#`
|
||||
// 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_<observed-suffix>#` 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]}#`);
|
||||
}
|
||||
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
Reference in New Issue
Block a user