Codex P2 finding on commit 9632ed5: #friendlyNameCollidesWithSecret tested
a regex-entry friendlyName's RAW spelling directly against the pattern,
which can never match a label that is already normalized (uppercased,
separators stripped) even when that label IS the normalized rendering of
a value the regex actually discovers -- e.g. friendlyName: "TOKABC123"
for content: "tok_[a-z0-9]+" discovering literal tok_abc123. Nothing
compared the label against the actual matched value either. The check
now also compares the sanitized label against the sanitized value of the
secret currently being minted (reusing #prefixIsSecretShaped), catching
this on the secret's first mint before it's recorded as previously
discovered.
Codex P2 finding on commit 9b2e14d: #friendlyNameCollidesWithSecret
compared a secret's full sanitized value against the already 32-char-capped
(sanitizeSecretFriendlyName) friendly name, so a secret whose sanitized
form exceeds MAX_FRIENDLY_NAME_LEN could never be fully contained in the
truncated label -- the collision went undetected and the secret's first
32 sanitized characters leaked as an accepted placeholder prefix. The
collision check now runs against the full, untruncated sanitized label
(sanitizeForCollisionCheck(friendlyName)); the 32-char cap is applied
only afterward, to the label actually used for display.
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.
Codex P2 finding on commit dff2a8d: #isGeneratedPlaceholder's forged-alias
guard only checked a dropped friendly-name prefix against exact previously-
discovered secret strings, recorded in whatever casing they first turned
up in. A case-insensitive (or other flag-variant) regex only ever records
the one casing it actually discovered, so a forged token wrapping a
differently-cased occurrence of that secret-shaped text around a real
bare-alias suffix matched neither exact-string check and sailed through
as an already-redacted placeholder, leaking the secret-shaped text
verbatim. The guard now also tests the dropped prefix directly against
every configured regex pattern.
Codex P2 finding on commit 7d3a3a2: #friendlyNameCollidesWithSecret ran a
configured regex entry's pattern against the already-sanitized (uppercased,
separator-stripped) friendly name, so a case-sensitive/punctuated pattern
like tok_[a-z0-9]+ never matched the sanitized label even when the raw
friendlyName was itself a live match for that regex — letting a
secret-shaped label slip through and stamp into every placeholder minted
for it. The regex check now runs against the raw, pre-sanitization label,
matching how the regex would encounter that text verbatim.
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.
- Fix #generateRegexReplacement's pathological (match-everything) fallback
emitting a 1-2 byte matched value unchanged when it was exactly `Z`/`ZZ`,
the shared sentinel #generateReplacement uses for such short values.
Falls back to a same-length, key-derived run instead, which stays a fixed
point under re-obfuscation without being a public, guessable constant.
- Default getSecretPlaceholderKey()/getExistingSecretPlaceholderKey() to
getAgentDir() instead of getConfigRootDir(), matching the directory
createAgentSession() actually passes.
- Isolate the getSecretPlaceholderKey test suite under a fresh $HOME/temp
agent dir instead of the real homedir, fixing an EACCES failure in
sandboxed review environments.
- Revert an unrelated gc session-ordering tie-breaker bundled into this
branch's history; out of scope for the secrets/friendly-name feature.
- Relocate this PR's CHANGELOG.md entries out of already-released sections
(16.3.0, 16.3.5) into [Unreleased], where a stale merge had left them,
and drop a duplicate blank line and duplicate serverSideFallback/
softRequestBudgetNotice entries the same merge introduced.
A configured secret's friendlyName is baked verbatim into every
placeholder minted for it via an exact deobfuscateMap entry - not
through the alias-fallback path fixed earlier. If the sanitized label
happens to contain another configured secret's literal (or is matched
by a configured regex), that secret is smuggled into every use of the
unrelated placeholder and is never scanned, since the token is
recognized as already-generated on an exact match.
createPlaceholder() now drops the friendly-name prefix for a mint
whenever it collides with a configured plain secret or a configured
regex pattern. Regex entries are now compiled in a dedicated
constructor pass before any placeholder is minted, so the collision
check sees the full picture regardless of entries[] order.
Two follow-up fixes on the friendly-placeholder alias check:
- isGeneratedPlaceholder() now also rejects a forged prefix that
contains any regex-discovered secret's value (tracked in
#obfuscateMappings), not just statically configured plain secrets,
closing the same smuggling path for type: "regex" entries.
- The regex match collector's placeholder-prefix clamp branch now
keeps the full original match length for the short-match guard
instead of the clamped prefix's shorter length, so a full-size
match whose wholly-outside prefix is under MIN_OBFUSCATE_SECRET_LEN
is no longer wrongly skipped as noise.
Untrusted text could wrap a real secret's plaintext in a fabricated
friendly-name prefix around a hash suffix borrowed from any OTHER
already-obfuscated secret. obfuscate() treated the whole forged token
- including the exposed secret literal standing in for the prefix - as
already redacted, letting it reach the provider untouched.
isGeneratedPlaceholder() now refuses the friendly-name-independent
alias fallback whenever the dropped prefix contains a configured
secret's literal value, while still accepting a stale prefix left over
from a legitimate friendly-name rename (same secret, same key).
A bounded default-mode regex (e.g. [A-Z]{9}) whose greedy reach spans
a placeholdered secret plus a short trailing raw chunk resolved
differently depending on whether the leading portion of that span was
still raw text (first obfuscate() call) or already a placeholder
(later calls): a cut-resolution resume point that landed exactly on
the start of another already-generated placeholder was handed
straight to a fresh regex.exec attempt instead of skipping past it,
exposing a shorter/different match to a LATER call that the first
call never attempted (e.g. leaving a trailing byte raw on pass one,
then sweeping it into a brand-new placeholder on pass two), churning
provider-visible history and prompt-cache prefixes across that
transition.
Added extendPastAdjacentPlaceholders, which chains a cut-resolution
resume point through every immediately-adjacent generated-placeholder
segment (no raw gap) before allowing a fresh regex attempt, applied at
all three resume points in the partialPlaceholderCut handling block.
Both calls now land on the same next scan position and agree on the
same (conservative) resolution from the first pass onward.
The earlier fix made #generateRegexReplacement's no-stable-candidate
fallback (a pathological match-everything default replace-mode regex,
e.g. [\s\S]{8}) key-derived via #generateReplacement, so it stays a
fixed point across restarts \u2014 but only if the key itself is stable.
secretEntriesNeedPlaceholderKey previously treated every replace-mode
entry as never needing the persisted key, so a fresh install with ONLY
such a regex (no other entry needing a persisted key) never got one:
SecretObfuscator fell back to defaultPlaceholderKey(), a process-random
key regenerated on every restart, so the fallback marker churned across
restarts anyway.
secretEntryNeedsPlaceholderKey now also returns true for a regex entry
in a replace-mode shape with no custom `replacement` \u2014 exactly the
shape that can reach the key-derived fallback. A regex WITH a custom
replacement (always emits the literal configured string) and a plain
replace secret (pure content-hash) are unaffected.
findNonMatchingReplacement special-cased value.length <= 3 with a fully
exhaustive search over every base**length candidate (base = 90, so up to
729000 for a 3-character match). A default replace-mode regex of that
length matching every candidate (e.g. `[\s\S]{3}`) burned the entire
sweep \u2014 measured ~70ms \u2014 on every single match before reaching the
whitespace/keyed fallback, stalling provider requests on modest tool
output.
All lengths now use the same bounded single-position-substitution
search already used for values longer than 3 chars: O(length * 90)
instead of O(90^length). Measured ~1ms for the same case post-fix.
Correctness is unaffected \u2014 any regex whose rejection depends on a
single position (the realistic case: character classes, literal
matches, bounded repeats) is still found; this is the same bounded-
search tradeoff already accepted for longer values.
When a default replace-mode regex has no same-length candidate that
escapes it (a pathological match-everything config such as `[\s\S]{8}`),
`findNonMatchingReplacement` returns undefined. The fallback kept the
content-hash-derived replacement, but that is not a fixed point: the
regex re-matches it on the next obfuscate() pass, and since the value
is hashed from its own bytes (not the original secret), each pass
rehashes it into a different marker \u2014 churning the redaction and
drifting the provider prompt-cache prefix it anchors, both across
re-obfuscation passes and across obfuscator restarts.
Fall back to the same key+length-only marker already used for
per-chunk remainder redactions, which depends on nothing but the
per-install key and the value's length, so re-matching and
re-redacting it always reproduces the identical marker.
Codex flagged (PR #2735): the placeholder conflict check only
consulted the internal deobfuscation map, not the full set of
configured secret literals. If secret A's minted placeholder happened
to equal secret B's raw configured value, obfuscating text containing
A leaked B's literal value verbatim: B's own plain-secret redaction
pass (sorted by length, once per obfuscate() call) already completed
before A's placeholder existed in the text, so the newly-minted
placeholder was never caught and redacted as B's secret.
Adds a #configuredSecretValues set populated with every configured
plain-secret literal (both obfuscate and replace mode, plus the
placeholder key) before any placeholder is minted. #placeholderConflicts
now also rejects a candidate (and its friendly-name-unprefixed alias)
if it equals any OTHER configured secret's value, forcing the
fallback-base retry loop to pick a different token.
Adds a regression test covering both entry orderings.
Codex flagged (PR #2735): when a replace-mode regex match fully
overlapped a placeholder freshly created earlier in the SAME
obfuscate() call, the redaction call site blanket-tagged the entire
redacted span's origin as 'I' (prior-call), including the preserved
placeholder bytes. A later regex entry over the same result then
wrongly treated that fresh placeholder as prior-call, applying the
greedy-spillover fixed-point skip and leaving genuinely new adjacent
content (e.g. SECRET in ABCDEFGHSECRET) provider-visible.
Threads the origin tag string through redactRegexMatchOutsidePlaceholders
and redactWithFixedReplacementOutsidePlaceholders (both now return
{text, origin} via a new shared transformOutsidePlaceholdersTracked
helper), so a placeholder preserved inside a redacted span keeps its
own existing origin tag instead of being overwritten.
Adds a regression test reproducing the exact reviewer scenario in a
single obfuscate() call across three chained entries.
Codex flagged (PR #2735): when re-obfuscating text with a prior-call
placeholder, the outside-chunk independence check
(outsidePlaceholderRangesAnyIndependentlyMatch) tested only the literal
'#...#' placeholder token text. A regex like
'ABCDEFGHSECRET|(?<=ABCDEFGH)SECRET|ABCDEFGH' combined with a prior
placeholder for ABCDEFGH left a trailing SECRET provider-visible,
because the lookbehind (?<=ABCDEFGH) never matches the literal
placeholder token — only the expanded secret value satisfies it.
Fix tests each outside chunk in BOTH the literal placeholder-token
context (needed when the token's own non-word boundary completes a
boundary-sensitive pattern like \b[A-Z]{8}\b) and the expanded scan
context (needed when a lookbehind/lookahead only resolves once the
neighboring placeholder is deobfuscated), unioning the result.
Adds a regression test covering the scan-context-only case, distinct
from the three existing adjacent tests which are all satisfied by
literal-context alone.
Testing an outside-placeholder chunk in isolation (a sliced substring)
broke context-sensitive regex patterns -- lookbehind, lookahead, and
\b -- that depend on bytes actually adjacent to the chunk in the
source text but outside its own range. `(?<=api=)[0-9]{8}` over
`api=12345678` plus a trailing placeholder failed when `12345678`
was tested standalone, since the isolated slice has no `api=`
immediately before it, wrongly treating the digits as spillover and
leaking them unredacted.
Each outside chunk is now tested at its real position in the source
text via a new chunkMatchesInSourceContext helper, so lookbehind and
lookahead assertions see the actual surrounding bytes. A match that
spans into the placeholder itself still does not count as an
independent match of the outside chunk alone.
The spillover independence check concatenated the outside-placeholder
chunks before testing whether they satisfy the regex on their own,
which erased the placeholder-token boundary between them. With
`\b[A-Z]{8}\b|[A-Z]{17}` and a placeholder for SECRETUV flanked by
prefix ABCDEFGH (independently matches) and suffix I (does not), the
concatenated ABCDEFGHI no longer matched either alternative, so
ABCDEFGH was wrongly treated as spillover and left unredacted in
provider-visible output.
Each outside chunk is now tested independently against the regex, so
a chunk that genuinely satisfies it on its own is redacted even when
concatenating it with its neighbor would hide the match.
The default replace-mode redaction had the same greedy-spillover class as
the obfuscate branch: a match straddling a prior-call placeholder whose
own value already satisfies the regex still re-scrambled the short
nonmatching surrounding bytes on each pass (e.g. `…#…#ZZJ5sotJ` →
`…#…#ZZpvsotJ`), drifting provider-visible history and prompt-cache
prefixes. Both branches now skip the spillover rewrite when the
placeholder value alone matches the regex and the outside bytes do not
independently match; structurally-required or independently-matching
outside bytes are still redacted.
Refs: #2465
An obfuscate-mode secret regex whose match straddles a previously
generated placeholder still obfuscated short surrounding raw bytes the
regex never needed, minting fresh placeholders on re-obfuscation and
drifting the obfuscate() fixed point plus provider-visible history and
prompt-cache prefixes. When the placeholder's own deobfuscated value
already satisfies the regex, the surrounding chunk is greedy spillover
and now stays verbatim; outside bytes are still obfuscated when the
placeholder value alone cannot match (e.g. a required api_key= prefix).
Refs: #2465
A regex match that starts in outside text and ends inside a previously
generated `#…#` placeholder's expanded value was skipped wholesale by
resuming the scan past the placeholder. An independently-matching outside
prefix was therefore left provider-visible — e.g. `[A-Z0-9]{8,12}` greedily
spanning `SECRETUV` into an `ABCDEFGH` placeholder returned `SECRETUV#…#`
even though `SECRETUV` satisfies the regex on its own.
The cut handling now re-runs the regex bounded to just before the
placeholder (full left context kept, so lookbehind still evaluates) and, when
the prefix forms a standalone match, redacts it — to its own reversible
placeholder in obfuscate mode, or a one-way redaction in replace mode — while
the cut secret stays as its existing placeholder. The replace default
redaction's fixed point is now verified against the placeholder-expanded view
re-obfuscation actually scans, so it does not drift when the adjacent
placeholder expands and connects to the redaction's trailing bytes.
Refs #2465
Mathews-Tom