fix(secrets): keyed replace-mode replacement derivation (#4166) - #4170
Conversation
08c98af to
8685dad
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08c98af2b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (block === undefined || blockOffset >= REPLACEMENT_DIGEST_BYTES) { | ||
| counterView.setUint32(4, blockIndex, false); | ||
| block = createHmac("sha256", key).update(REPLACEMENT_DOMAIN).update(counter).update(secret, "utf8").digest(); |
There was a problem hiding this comment.
When a replace-mode regex captures a long value, every 32-byte output block reruns HMAC over the entire secret, making replacement generation Θ(secret.length²) rather than linear as documented. This runs synchronously during obfuscate(); an isolated 200,000-character match took about five seconds in this checkout, so broad regexes over large messages can stall the CLI repeatedly. Hash the secret once into a keyed seed and perform counter expansion over that fixed-size seed instead.
Useful? React with 👍 / 👎.
secrets.yml entries with mode "replace" and no explicit replacement derived their substitute from an unkeyed public Bun.hash, so an observer who saw one replacement could confirm candidate secrets offline and read the secret's exact length (same-length by construction). The process key was already in scope at both call sites but unused. Replace the derivation with a keyed, domain-separated HMAC-SHA256 construction (replacement.v1 domain, distinct from the placeholder domain), expanded counter-style with uniform rejection sampling. Same-process determinism, same-length output, and alphanumeric character behavior are preserved; explicit replacement values are untouched. Derived replacements are now unpredictable without the process key; they intentionally differ across processes or after key rotation, matching placeholder behavior. Lore-id: 4166-keyed-secret-replacement Constraint: derived replacement must stay deterministic per process, same length, and alphanumeric Constraint: explicit replacement values in secrets.yml stay byte-identical Rejected: unkeyed Bun.hash | offline confirmation oracle and exact-length leakage Rejected: global static key | would make all installations share one confirmation key Confidence: high Scope-risk: low Reversibility: source-revert Tested: key A/B divergence, offline-confirmation oracle, mutation sensitivity, same-length/alphabet, explicit replacement, regex replace, empty/Unicode/very-long secrets Not-tested: cross-process replacement stability (deliberately out of scope; explicit replacement is the stable option)
8685dad to
dfab11c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfab11c18c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const byte = block[blockOffset++]!; | ||
| if (byte >= REPLACEMENT_REJECT_THRESHOLD) continue; | ||
| chars.push(REPLACEMENT_CHARS[byte % REPLACEMENT_CHARS.length]!); |
There was a problem hiding this comment.
For short alphanumeric secrets, the sampled output can equal the input, leaving the configured secret unchanged in outbound text. For example, with the test key Uint8Array.from({ length: 32 }, (_, i) => i), both the plain and regex replace paths derive "F" for secret "F"; with a random process key, any one-character alphanumeric secret has a 1/62 chance of exposure per process. Detect equality and deterministically derive another candidate before returning.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
NEEDS-WORK — the construction is correct, but the test that guards its central property does not work.
Disclosure: I wrote a competing fix for #4166 and closed it (#4171) in favour of this one, specifically because this eliminates a modulo bias mine had. So I went looking for reasons this is worse than I assumed rather than confirming my preference. The construction held up; the test did not.
The cryptographic construction is right
I checked the arithmetic rather than trusting the comment:
256 % 62 = 8, threshold248 = 62 × 4, andbyte >= 248rejects exactly248–255. Correct, no off-by-one.- Rejected bytes are consumed, not retried. Exhausting a 32-byte block increments the counter and derives a fresh HMAC block, so the loop cannot spin.
- Block construction (
obfuscator.ts:98-108):HMAC-SHA256(key, REPLACEMENT_DOMAIN || uint64be(counter) || UTF8(secret)). The counter is fixed-width big-endian, so the concatenation is unambiguous — this is the detail I most expected to be wrong and it is right. - Domains are genuinely distinct:
gjc.secret-obfuscation.replacement.v1\0vs...placeholder.v1\0. A placeholder and a replacement for the same secret cannot collide. - Determinism, UTF-16-identical length, and
[A-Za-z0-9]output are all preserved. - Explicit
replacementvalues stay authoritative viaentry.replacement ?? derive(...)on both the plain and regex paths (obfuscator.ts:196-198,250-252). - Tests assert key divergence rather than pinning a digest — the right call.
Rebases onto current dev with no conflicts. Focused suite 39 pass / 0 fail; neighbours green (memories redaction 3/0, session silent-abort/redaction 5/0); check:types exit 0. Mutation on the keyed derivation: 39 → 35 pass / 4 fail, restored 39/0. So the keying is properly pinned.
Finding 1 — the modulo-bias test cannot detect modulo bias
packages/coding-agent/test/secrets-obfuscator.test.ts:587-610.
I disabled rejection sampling entirely in production:
// obfuscator.ts:111
- if (byte >= REPLACEMENT_REJECT_THRESHOLD) continue;
+ // mutated: accept every byte (reintroduces modulo bias)That restores the exact byte % 62 bias this PR exists to remove. Result:
- the bias test alone: 1 pass / 0 fail
- the whole file: 39 pass / 0 fail
Nothing catches it. The tolerance is expected * 0.5 to expected * 1.5, while the actual bias is ~25% overrepresentation of the first eight characters — comfortably inside a ±50% band. With 6400 samples across 62 buckets the statistical test is also underpowered for that effect size.
This matters because bias-freedom is the one property distinguishing this PR from the simpler fix. Right now that property is unguarded, and a future refactor could silently drop rejection sampling with the suite still green.
Replace it with deterministic boundary coverage instead of a statistical one: prove byte 247 is accepted, 248 and 255 are rejected, and that sampling continues into fresh bytes/blocks after a rejection. That fails immediately under the mutation above.
Finding 2 — the reproduction artifacts have no generator
artifacts/issue-4166-reproduction-pre-fix.txt and -post-fix.txt are committed as evidence, but grepping scripts/ and packages/ finds nothing that produces them. They are hand-written.
Same pattern I blocked #4152 for earlier today, so I am applying it consistently: committed evidence that cannot be regenerated from the tree is worse than no evidence, because the next reader takes it as machine-verified fact.
Either add a deterministic producer that generates and verifies both files, or drop them — the tests already carry the proof.
Net
Neither finding touches the shipped behaviour, which is correct and an improvement on what I had written. Fix the bias test and resolve the artifacts, and this is an approve.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
MERGE_READY — owner hold 해제. Exact-head CI가 terminal green이며 현재 확인된 unresolved blocker가 없습니다. Merge/release는 owner-controlled 단계로 남깁니다.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
2 participants
Closes #4166
Summary
secrets.ymlentries withmode: "replace"and no explicitreplacementderived their substitute from an unkeyed public function (Bun.hash(secret)). An observer who saw one replacement could confirm candidate secrets offline (the algorithm is public and keyless) and read the secret's exact length (the replacement is same-length by construction).This PR replaces the derivation with a keyed, domain-separated HMAC-SHA256 construction over the existing process key (
PROCESS_SECRET_OBFUSCATION_KEY), expanded counter-style with uniform rejection sampling. The construction mirrors the already-keyed placeholder path and uses a distinct domain (gjc.secret-obfuscation.replacement.v1).Reproduction evidence (synthetic secrets only — no real credentials)
artifacts/issue-4166-reproduction-pre-fix.txt— pre-fix: replace output identical across key A/B; offline oracle recoversacme-staging-2024from one observed replacement with no key; length disclosure 17 == 17.artifacts/issue-4166-reproduction-post-fix.txt— post-fix: replace output diverges across keys; offline oracle finds nothing; same-length/alphanumeric preserved.Compatibility contract (preserved)
result.length === secret.length(UTF-16 code units, unchanged semantics).[A-Za-z0-9]only.replacementvalues insecrets.ymlare authoritative and byte-identical — the derived path runs only whenreplacementis undefined.deobfuscate()never reverses derived replacements.Decisions
replacement.obfuscate()runs per complete payload, so streamed chunks see the same deterministic replacement within a process.Tests
New
SecretObfuscator keyed deterministic replacementblock inpackages/coding-agent/test/secrets-obfuscator.test.ts:Verified:
bun test packages/coding-agent/test/secrets-obfuscator.test.ts packages/coding-agent/test/memories-redact-secrets.test.ts(42 pass) andbun --cwd=packages/coding-agent run check(biome + tsc clean).Notes