- Separated deterministic replacement generation, placeholder derivation,
placeholder-range scanning and message-tree transforms out of the 2647-line
module; obfuscator.ts now holds the types and SecretObfuscator.
- ephemeralPlaceholderKey stays a single instance and both global regexes stay
beside the code that resets their lastIndex, so placeholder stability and
the security argument in the moved comments are preserved verbatim.
- Repointed every importer at the real modules rather than leaving a re-export
shim; the public ./secrets barrel exports the same 15 names as before.
Moved the obfuscator assembly (secrets.yml + env entries + built-in
credential patterns, placeholder-key minting rules, redaction-only
fallback) from createAgentSessionScoped into the secrets module so other
entrypoints can build the same obfuscator.
These four paths bypassed DirResolver's XDG-aware rootSubdir/agentSubdir
hooks, resolving directly against getConfigRootDir()/getAgentDir() and
ignoring XDG state/data layout. Add XDG-aware path helpers in dirs.ts
and route all four through them:
- secret-placeholder.key → $XDG_STATE_HOME/omp/ (state, agent flattened)
- marketplaces.json → $XDG_DATA_HOME/omp/ (data)
- run/daemons/<hash>/ → $XDG_STATE_HOME/omp/run/ (state)
- run/provider-inflight/ → $XDG_STATE_HOME/omp/run/ (state)
omp config init-xdg migrates secret-placeholder.key and marketplaces.json
from their legacy locations; run/ is ephemeral and rebuilds on restart.
Appending builtinCredentialSecretEntries() unconditionally made every
secrets.enabled session carry a regex obfuscate entry, so
secretEntriesNeedPlaceholderKey was always true and startup always
created secret-placeholder.key — nullifying the replace-only/no-secret
key-avoidance path and failing headless runs on an unwritable config
root for a feature they never use.
Only configured entries now force startup key creation. The built-in
credential pattern matches dynamically, so SecretObfuscator accepts a
key provider resolved once on the first actual credential match, via
the new getSecretPlaceholderKeySync (never throws: degrades to a
process-ephemeral key with a warning when the key file is unwritable).
Also moves both CHANGELOG entries from the released 17.1.7 sections to
[Unreleased].
With secrets.enabled (Hide Secrets), a credential-shaped token not present
in secrets.yml or the environment fell through to pi-ai's irreversible
[*_token_redacted] rewrite in transform-messages. The model echoed that
placeholder into edit-tool old_text, which could never match the real
bytes on disk, breaking exact-match edits of any file containing such a
token.
Route the same token shapes (SENSITIVE_TOKEN_RE, now exported from
pi-ai) through the SecretObfuscator as a built-in obfuscate-mode regex
entry: the provider sees reversible keyed placeholders, and
deobfuscateToolArguments restores the real bytes before tool execution.
Hide Secrets' contract is unchanged — credential bytes still never
reach the provider; pi-ai's redaction stays as the backstop for hosts
without an obfuscator.
Fixes#6968
- Buffered only a lone trailing dollar or a $$-introduced body during streaming.
- Let $HOME/$100-style single-dollar text stream through immediately.
- Added regression coverage for the passthrough.
- Removed hash-delimited placeholder parsing and stored-session aliases.
- Simplified replay and display restoration to the double-dollar format.
- Replaced legacy compatibility tests with an inert-token regression.
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.
can1357