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
The deterministic-replacement collision search truncated the surrounding
text to a 512-byte window before testing whether a candidate redaction
would be re-matched in place. A replace-mode regex whose lookbehind or
lookahead reaches beyond that window (e.g. `(?<=A{600})[AZ]`) then had its
assertion context dropped, so the check falsely accepted a candidate the
pattern DOES re-match once the full prefix is present. The redaction
oscillated back to the raw matched value on alternating obfuscate() passes,
leaking it to the provider every other turn.
The probe now substitutes the candidate into the full text so lookbehind /
lookahead always evaluate against complete context, and the scan starts a
bounded distance left of the span and stops once a match begins at/after the
span end, keeping per-candidate cost independent of total text length.
Refs #2465
The partial-placeholder-cut skip dropped the whole straddling match, but the regex iterator had already advanced through the expanded bytes on the far side of the placeholder, so a real wholly-outside match was missed — e.g. plain ABCDEFGH + replace [A-Z]{8} on YYBBABCDEFGHSECRETUV left the trailing SECRETUV un-redacted before it reached the provider. mapReplaceRegexMatch now reports a resume index (just past the last overlapping placeholder); on a cut, #collectRegexMatches rewinds the scan there instead of recording the cut, so the cut secret stays as its existing placeholder and trailing wholly-outside content is matched fresh (obfuscated or redacted on its own). Removes the now-dead obfuscate/replace skip branches and the partialPlaceholderCut match field. Round-trip stays exact, the secret never leaks, and re-obfuscation is a fixed point.
Refs #2465
The obfuscate-mode partialPlaceholderCut skip was not applied to replace-mode regexes, so a replace match whose boundary cuts into a generated placeholder's expanded value still redacted the bytes outside the snapped token. That deterministic scramble is not a fixed point across re-obfuscation passes (e.g. plain ABCDEFGH + replace [A-Z]{8} on YYBBABCDEFGHSECRETUV: ZZgK#…#ZZk2ETUV drifts to ZZgZ#…#ZZk2ETUV), drifting provider history / prompt-cache prefixes. The replace branch now skips partial-placeholder cuts as well: the cut secret stays obfuscated as its placeholder, the surrounding non-secret bytes are left untouched, and re-obfuscation is stable. Adds a regression test; the secret never leaks.
Refs #2465
The key-need probe tested whether a replace fragment survives the rest of the phase, but still assumed the passthrough bytes it tiles into are untouched. A later (shorter-content) replacement can rewrite those surrounding bytes too: AA -> SEC forms SEC+RET12, then R -> X turns the freshly formed SECRET12 into SECXET12, and direct SECRET12 is shadowed to SAFE — so no output ever contains SECRET12, yet the probe required secret-placeholder.key and could fail startup in an unwritable agent config dir. The tiling check now also requires the obfuscate content to be stable under the later replacements; this only drops false positives (a formation that genuinely survives is still caught at the index that produces it). Adds a regression test.
Refs #2465
An obfuscate-mode regex whose match boundary falls inside a prior placeholder's expanded value had the span snapped out to the whole #…# token, so two such matches around one placeholder mapped to overlapping source ranges that clobbered each other on apply and dropped bytes — e.g. a plain ABCDEFGH secret plus [A-Z]{8} turned YYBBABCDEFGHSECRETUV into a placeholder restoring as YYBBABCDEFGHETUV. mapReplaceRegexMatch now reports partial placeholder cuts and the obfuscate path skips them (before the preserve-input-placeholders branch, so re-obfuscation stays a fixed point), leaving the already-obfuscated secret and surrounding bytes untouched. Adds a regression test.
Refs #2465
The sub-threshold guard for obfuscate-mode regex matches ran after the preserve-input-placeholders branch, so on a re-obfuscation pass a short match straddling a prior #…# placeholder rewrote its surrounding context (XX/YY) into fresh placeholders. obfuscate() then stopped being a fixed point and provider-visible history / prompt-cache prefixes drifted across passes even though round-trip deobfuscation still recovered the original. Move the scanMatchLength guard ahead of the preservation branch so a match shorter than MIN_OBFUSCATE_SECRET_LEN is skipped regardless of placeholder overlap, keeping re-obfuscation idempotent. Extends the regression test to assert the fixed point.
Refs #2465
An obfuscate-mode secret regex whose match is shorter than MIN_OBFUSCATE_SECRET_LEN but straddles a previously generated #…# placeholder had its range extended across the whole token, so the short-match guard (which measured the rewritten source span) let it re-placeholder across the token and corrupt reversible deobfuscation — e.g. a plain SECRETUV secret plus [A-Z]{6} turned XXSECRETUVYY into a single placeholder that restored as XXSECRETUV. Guard on the regex's own match length in the placeholder-expanded scan view instead, so sub-threshold matches are skipped and surrounding literals round-trip intact.
Refs #2465
The full-width whitespace fallback still leaked for a default replace regex that also matches all-space/all-tab runs (e.g. (?:\S{5}| {5}|\t{5})), since the space/tab runs matched too. Also try a single whitespace byte among non-whitespace filler (' AAAA'), whose lone whitespace breaks every fixed-length run. A regex matching every non-line-terminator stays in the documented ./[\s\S] sentinel-keeping case.
Refs #2465
secretEntriesNeedPlaceholderKey conservatively required the persisted secret-placeholder.key whenever a replace-mode output could seed an obfuscate content, ignoring that a later (shorter-content) replacement may always erase that fragment (e.g. AA->SEC then S->X turns every SEC into XEC). Test each replacement output in the form it survives the rest of the replace phase; surrounding bytes stay modeled as arbitrary passthrough so this only drops false positives and never under-approximates a real key need.
Refs #2465
A default mode:"replace" regex matching every non-whitespace candidate (e.g. \S{n}) shipped the raw secret unchanged when its deterministic replacement collided with the value: every alphanumeric/punctuation candidate still matched. Add a same-length whitespace (space/tab) fallback tier to the redaction search, which \S-style patterns never re-match, so the secret is redacted to a stable nonmatching value. A match-everything regex (./[\s\S]) matches whitespace too and still keeps the sentinel.
Refs #2465
The distinctness guard for default replace-mode regex matches tried only the
single A/B perturbation; when that one alternative also matched the regex (e.g.
`Z|A`, `[AZ]`) it kept the raw sentinel and shipped the secret to the provider.
Bounded-search same-length candidates for one the regex does not match — a stable
fixed point under re-obfuscation — and keep the sentinel only when none exists
(a pathological match-everything regex).
Refs #2735
The replace phase tiles its output from passthrough bytes and whole replacement
outputs, so an obfuscate content can be reconstructed not only by a replacement
that contains it but also by a replacement prefix/suffix that abuts surrounding
passthrough (e.g. A->SEC turns ARET12 into SECRET12). The key-need check now
treats a replacement as able to form a content when it is an interior substring,
a wholesale superstring, OR shares a prefix/suffix border with the content
(replacementCanFormContent). A plain obfuscate entry requires the key when its
content survives the simulated replace phase or any effective replacement can so
form it. Same-content default shadows with no other interacting replacement stay
key-free. Adds fragment-join regression coverage.
Replace the per-case shadow heuristics with a ground-truth simulation of the
plain replace phase obfuscate() actually runs (content-keyed mappings, later
duplicate wins, applied in descending content-length order). A plain obfuscate
entry needs the key when its content survives that phase OR any effective
replacement string contains the content (a sound guard against context- and
chain-triggered reintroduction a bare-content probe cannot surface, e.g.
SECRET->ALIAS then ALIAS->SECRET). This handles direct shadowing, reintroduction,
duplicate ordering, and transitive chains uniformly; regex obfuscate entries stay
conservatively key-requiring. Adds transitive-chain regression coverage.
SecretObfuscator stores plain replace mappings in a content-keyed Map, so among
duplicate same-content replace entries only the LAST applies in obfuscate(). The
shadow check populated its set from any replace entry, so a safe earlier
duplicate could mask a later reintroducing one (custom replacement containing the
secret) and wrongly suppress the key need — the reversible placeholder then used
the process-random fallback and could not be deobfuscated after restart. The
check now resolves the effective replacement per content (later duplicate wins,
mirroring the constructor) before deciding shadowing. Adds both-ordering
regression coverage.
The shadow check that skips key creation for a same-content replace-mode entry
was too broad: if the replace entry has a custom replacement that still contains
the secret value, obfuscate()'s later plain-obfuscate pass re-scans the inserted
replacement text and emits a reversible placeholder, which then needs the
persisted key (otherwise it is keyed with the process-random fallback and cannot
deobfuscate after restart). The shadow now only applies when the replacement
omits the content (custom replacement not containing it, or the default
deterministic length-preserving replacement which never can). Extends the
regression test with the reintroduction case.
An auto-collected env secret that is also declared as a plain mode:replace
entry with the same content still triggered creation of the persisted
secret-placeholder.key. Replace mappings run before obfuscate mappings in
obfuscate() and only rewrite text outside existing spans, so the obfuscate
entry is one-way replaced first and never emits a reversible placeholder; it
does not need the key. Add secretEntriesNeedPlaceholderKey() which excludes
same-content replace-shadowed obfuscate entries, and use it in sdk.ts so an
effectively replace-only secret set no longer requires/writes the key file or
fails startup on an unwritable agent config dir.
When the only obfuscate entries were short (<8 char) plain secrets, the
key-creation gate still saw them as obfuscate-mode and wrote/persisted
`secret-placeholder.key`, yet SecretObfuscator tones those entries down so
hasSecrets() is false and provider-output redaction is disabled. The
persisted key was then readable via a tool (leaking it to the model) and
reused for later placeholders. Gate key creation on whether an entry can
actually produce a reversible placeholder via a shared predicate
(secretEntryNeedsPlaceholderKey), single-sourcing the tone-down threshold
as MIN_OBFUSCATE_SECRET_LEN.
The default replace-mode regex path emitted the raw match verbatim when
its deterministic replacement equalled the `Z`/`ZZ` sentinel (e.g. a
literal regex matching "ZZ"). Route it through a regex-aware distinctness
guard: perturb to a length-preserving distinct value, but only when the
regex does not re-match the perturbed output. For a self-matching short
regex (e.g. `Z+`, `[A-Z]{2}`) any non-sentinel 2-char value is re-matched
and would oscillate, re-leaking on alternate passes, so the sentinel is
kept to preserve the obfuscate() fixed-point invariant. Such configs are
pathological. Plain replace secrets remain unconditionally perturbed.
A plain replace-mode secret (or the redacted key) whose entire value is
exactly the deterministic sentinel (`Z`/`ZZ`) was emitted unchanged,
shipping the raw value to the provider. Scope a distinctness guard to
whole configured-secret replacements so the output differs from the input
while staying length-preserving and deterministic; a plain secret only
matches its own literal, so the perturbed output remains a fixed point
under re-obfuscation. Per-chunk remainder redaction keeps the sentinel
fixed-point for cross-restart idempotence.
Mathews-Tom