feat(secrets): sandbox credential placeholder registry (unwired) - #6689
Merged
Merged
Conversation
The sandbox container must never see real secret material, even transiently. It now holds only an inert `icsbx_`-prefixed placeholder token, stable per (tenant, user, provider) and separate from the session store, so holding one grants nothing on its own. InMemoryCredentialBroker gains JIT minting (mint_on_first_use): a CredentialSession is minted only at actual first use of an (invocation, binding) pair, not staged up front, and bound to the placeholder so the egress proxy can find it. CredentialSessionLease guarantees the session is revoked exactly once regardless of how the dispatch ends -- success/error via explicit revoke(), timeout/panic via Drop during future-cancellation or unwind -- because a missed revoke path is a silent standing grant. W6 (egress proxy consumer) and W8 (obligation chokepoint) are not built yet; this lands the placeholder/session primitives unwired, per plan, for them to call into later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The upcoming credential firewall injects inert icsbx_-prefixed placeholders into the sandbox in place of real secrets; the egress proxy swaps them for the real credential at request time. Placeholders are stable and inert but must never cross the trust boundary into model output, logs, or transcripts. Sandbox exec output already runs through the leak detector, so only the pattern was missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-1 code review found two correctness gaps in the JIT credential- session skeleton: mint_on_first_use's cache-hit reuse handed out a second lease over an already-live session with no reference counting, so the first caller to revoke/drop its lease killed the session out from under a second caller still holding it; and sessions_by_placeholder was a single-valued map, so binding a second session to a stable placeholder (a second account under the same provider, or an overlapping invocation) silently clobbered the first binding instead of tracking both. Fixes: - Add a per-session lease refcount; a session is only actually revoked once its last outstanding lease releases it. - Make sessions_by_placeholder multi-valued (HashSet per token) and prune it, plus jit_minted and lease_refcounts, from revoke_session so a long-lived process doesn't accumulate stale entries. - revoke_session recovers from a poisoned lock instead of silently no-op'ing, matching the "a missed revoke can never leave a standing grant" invariant this module documents. - Add TryFrom<String>/AsRef<str>/into_inner() to CredentialPlaceholderToken and rename CredentialPlaceholderOwner::provider_id to provider_or_extension_id, matching this crate's existing newtype/naming conventions. - Pin the icsbx_ prefix shared between ironclaw_secrets and the ironclaw_safety leak-detector pattern with a dev-dependency-only regression test instead of a real crate dependency, so ironclaw_safety stays a dependency-light substrate (ruled via thermo-nuclear review). - Tighten cross-reference comments (W6 -> W6-EGRESS-PROXY, name the concrete sibling leak pattern) per local-patterns review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rset Round 2 review (performance/tests dimensions round 1 missed): - mint_on_first_use's cache-hit path validated a jit_minted session and only incremented its lease refcount afterward, as separate lock acquisitions. A concurrent release_lease on the last outstanding lease could revoke the session in that window, handing back a lease for an already-dead session. Fixed by incrementing the refcount first (only succeeds while a live entry exists) and validating after, backing out the reference if invalid — serializes the join against release_lease on the same lease_refcounts lock instead of racing across mutexes. - CredentialPlaceholderToken::validate only checked the prefix, unlike this crate's own validate_credential_id convention; a malformed suffix (e.g. carrying control characters) could reach a container's env/logs once this token round-trips through the not-yet-built egress proxy. Now enforces the same shape generate() produces. - Dropped acquire_lease_refcount's vestigial Result (it can't fail) and deduplicated mint_on_first_use's bind+lease-construction tail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6689
July 27, 2026 01:52
Destroyed
github-actions
Bot
added
scope: dependencies
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds validated ChangesCredential placeholder flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Sandbox
participant LeakDetector
participant CredentialPlaceholderRegistry
participant InMemoryCredentialBroker
participant CredentialSessionLease
Sandbox->>LeakDetector: scan output containing placeholder
LeakDetector->>LeakDetector: block or redact matching token
CredentialPlaceholderRegistry->>InMemoryCredentialBroker: provide validated placeholder
InMemoryCredentialBroker->>CredentialSessionLease: mint or reuse leased session
CredentialSessionLease->>InMemoryCredentialBroker: release on revoke or drop
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | 4bcc1ae28129 |
Head: 4bcc1ae28129738e96833d38f8d772bc6cd4c6f1
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Reviewed the normal, focused 6-file/1,329-line change. Found two lifecycle/concurrency defects in the new JIT session path.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [HIGH] Make JIT mint publication atomic
Location: crates/ironclaw_secrets/src/placeholder.rs:346-351
Concurrent first uses of the same (invocation, capability, account) can both miss jit_minted, create separate sessions, and overwrite the cache entry at record_jit_mint. The sessions then receive independent refcounts and leases, violating the documented single-session reuse guarantee; with max_uses: Some(1), this also permits multiple independently usable sessions for one binding. Protect cache lookup/create/publication/refcount acquisition with one synchronization boundary (or an in-flight entry) and add a concurrent regression test.
2. ❌ [HIGH] Do not rely on Drop as the only session-expiry mechanism
Location: crates/ironclaw_secrets/src/placeholder.rs:276-280
CredentialSessionLease can be safely passed to std::mem::forget, which suppresses Drop and leaves its broker session live indefinitely when the request supplies expires_at: None and max_uses: None. This contradicts the stated guarantee that missed revoke paths cannot leave a standing grant. Enforce a broker-side expiry/lease deadline (and cleanup) independent of the RAII guard, and test a deliberately forgotten lease.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| /// A missed revoke path would leave a standing grant, so this type has no | ||
| /// safe way to leak the session past its own lifetime: there is no `mem::forget`-safe | ||
| /// accessor, and the only way to keep the session alive is to hold the lease. | ||
| pub struct CredentialSessionLease { |
Contributor
There was a problem hiding this comment.
RAII alone cannot guarantee revocation: std::mem::forget(lease) is safe and prevents Drop. Since requests may set both expiry and max uses to None, that leaves a standing session indefinitely. Add broker-side expiry/cleanup independent of the guard.
Collaborator
Author
There was a problem hiding this comment.
Partially addressed, not closed. 10f65fa softened the doc overclaim (dropping/revoking the lease releases the reference; deliberately forgetting it does not), and 7b39800 caps session expiry at 30 minutes by default at creation, so a forgotten lease can no longer be an indefinite standing grant — it now expires. Proactive eviction of expired entries independent of the guard is deliberately deferred: ironclaw_secrets owns no runtime to hang a sweep task off, and nothing calls this path yet (the consumer, the egress proxy, isn't built). It lands with that wiring. Leaving this open rather than marking it resolved, since the broker-side cleanup you asked for isn't there yet.
![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: 4bcc1ae281
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_safety/src/leak_detector.rs`:
- Around line 1259-1292: Update
sandbox_credential_placeholder_prefix_matches_registry to also validate the
detector’s minimum suffix length against
ironclaw_secrets::placeholder::CredentialPlaceholderToken::MIN_SUFFIX_LEN.
Expose MIN_SUFFIX_LEN as a public constant if necessary, then construct and scan
a token using the minimum valid suffix length and assert it matches the
sandbox_credential_placeholder pattern, preserving the existing prefix
synchronization check.
- Around line 626-631: Update the sandbox_credential_placeholder regex in the
LeakPattern definition so matching is not blocked by underscores adjacent to the
credential prefix or suffix; use non-identifier left context or a substring
match while preserving the existing 16+ alphanumeric credential shape. Extend
the boundary test near the existing case to cover both a leading underscore and
a trailing underscore, ensuring both are blocked.
In `@crates/ironclaw_secrets/src/lib.rs`:
- Around line 473-474: The InvalidPlaceholderToken error must not expose the
untrusted value through its Display message. Update InvalidPlaceholderToken and
its construction or formatting so the user-facing error reports only the
actionable reason, or uses an established redacting wrapper such as RedactedJson
or CredentialSessionId when triage requires the value; preserve the existing
validation behavior.
In `@crates/ironclaw_secrets/src/placeholder.rs`:
- Around line 272-275: Update the documentation around the lease type to avoid
claiming that session lifetime cannot be extended via mem::forget. Describe the
intended behavior instead: dropping or explicitly revoking the lease releases
its reference, while deliberately forgetting the lease skips cleanup and can
keep the session alive indefinitely.
- Around line 205-231: Replace the separate by_owner and by_token mutexes in
CredentialPlaceholderRegistry with one Mutex<RegistryState> containing both
maps. Update get_or_create and resolve to lock the shared state and perform
lookup/insertion through its by_owner and by_token fields, keeping both mappings
updated before get_or_create returns.
- Around line 92-104: Update the suffix length validation in the placeholder
token validator to use character counting (`chars().count()`) instead of byte
length, matching the error message’s “characters” wording. Keep the existing
ASCII alphanumeric validation and InvalidPlaceholderToken error behavior
unchanged.
- Around line 344-352: Register the initial lease refcount before publishing the
session in the new-session path around create_session, record_jit_mint, and
finish_lease. Move acquire_lease_refcount(session_id) ahead of record_jit_mint,
then update the safety comment to reflect that the id is unpublished until its
refcount exists; preserve the existing reuse-path behavior.
- Around line 357-368: Update finish_lease to construct the
CredentialSessionLease before binding so a bind_placeholder_to_session error
drops the lease and releases the caller-held session reference. Modify
bind_placeholder_to_session to acquire sessions_by_placeholder through the
module’s lock_or_recover helper, preserving cleanup on poisoned-lock failures
and the existing successful binding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c0f92a05-1168-45e7-935e-278c74153eb7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (5)
crates/ironclaw_safety/Cargo.tomlcrates/ironclaw_safety/src/leak_detector.rscrates/ironclaw_secrets/Cargo.tomlcrates/ironclaw_secrets/src/lib.rscrates/ironclaw_secrets/src/placeholder.rs
|
🚅 Deployed to the ironclaw-pr-6689 environment in ironclaw-ci-preview
|
InvalidPlaceholderToken interpolated the raw, untrusted `value` verbatim into its Display impl, so a rejected sandbox-supplied token — which can carry control characters, ANSI escapes, or a real secret pasted into the wrong slot — propagated into every log line derived from the error. `reason` already carries all actionable information, so drop `value` entirely rather than sanitizing it (per the repo's boundary-mapping guideline for credential-adjacent errors, which wins over matching the sibling `InvalidAccountId` convention here since this value is attacker-controlled). Also cap the parsed token length: validation only enforced a minimum suffix length, so an arbitrarily long `icsbx_...` string handed back from the sandbox was accepted and then hashed/cloned/inserted into maps - unbounded work driven by untrusted input. Registry-issued tokens are always exactly 32 alphanumeric characters (a UUIDv4 `simple()` suffix), and nothing needs a variable-length suffix, so validation now requires that exact length instead of "at least 16". The length constant is exposed publicly (CREDENTIAL_PLACEHOLDER_SUFFIX_LEN) so the leak detector's cross-crate pinning test can assert against it instead of a bare literal (see the companion ironclaw_safety commit). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sandbox_credential_placeholder regex was \bicsbx_[A-Za-z0-9]{16,}\b.
`_` is a word character, so \b does not fire next to it: a single
leading or trailing byte (`_icsbx_...`, `icsbx_..._x`) defeated the one
pattern standing between a leaked placeholder and model output/logs.
Drop the word boundaries entirely — `icsbx_` plus 16+ alphanumerics is
a distinctive shape with no realistic false-positive risk, and
over-matching here fails safe. Extends the existing test rather than
adding a parallel file: added leading-underscore, trailing-underscore,
and leading-letter cases, and flipped the prior "substring of a longer
word is NOT flagged" test to assert it IS flagged now, since that
flip is the deliberate fail-safe tradeoff.
Also pins the length half of the sandbox_credential_placeholder /
ironclaw_secrets contract, not just the prefix: the cross-crate test
now asserts CREDENTIAL_PLACEHOLDER_SUFFIX_LEN stays at or above the
regex's own 16-char floor, and additionally constructs a minimum-shaped
token through the registry's public parse() API and asserts the
detector actually flags it - pinning behavior instead of a literal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6689
July 27, 2026 02:15
Destroyed
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_secrets/src/placeholder.rs (1)
1-1195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
placeholder.rsto keep the new Rust file under the size ceiling.
crates/ironclaw_secrets/src/placeholder.rsis newly added at 1,195 lines, with the tests spanning lines 607–1195. This violates the repo invariant keeping new Rust files below ~800 lines, and the 589-line test addition also exceeds the ~200-line inline-justification threshold. Move the#[cfg(test)] mod testsinto a sibling module, such ascrates/ironclaw_secrets/src/placeholder/tests.rswith#[path], or an explicit nestedmod tests { include!("placeholder/tests.rs"); }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_secrets/src/placeholder.rs` around lines 1 - 1195, Split the inline #[cfg(test)] mod tests from placeholder.rs into a sibling test module such as placeholder/tests.rs, wiring it from placeholder.rs with #[path] or include!. Preserve all existing tests and their access to private placeholder symbols, while reducing placeholder.rs below the repository’s size ceiling and keeping production code unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_secrets/src/placeholder.rs`:
- Around line 1-1195: Split the inline #[cfg(test)] mod tests from
placeholder.rs into a sibling test module such as placeholder/tests.rs, wiring
it from placeholder.rs with #[path] or include!. Preserve all existing tests and
their access to private placeholder symbols, while reducing placeholder.rs below
the repository’s size ceiling and keeping production code unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6687fd64-cf87-497b-bbd9-88eb34b9579a
📒 Files selected for processing (3)
crates/ironclaw_safety/src/leak_detector.rscrates/ironclaw_secrets/src/lib.rscrates/ironclaw_secrets/src/placeholder.rs
henrypark133
commented
Jul 27, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add an unwired sandbox credential placeholder registry with just-in-time leases, reliable revocation, and leak detection for inert tokens.
Stats: 4 net-new findings (from 7 raw reviewer findings, 6 after lane dedup) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0. Two performance findings were suppressed because the PR explicitly documents those O(n) deferrals; existing unresolved threads covering first-use serialization, RAII-forget cleanup, placeholder bounds/redaction, and binding cleanup were also suppressed as duplicates.
Bugs / lifecycle
- High Session leaks when JIT index publication fails (
crates/ironclaw_secrets/src/placeholder.rs:363-365, confidence 85) —create_sessioninserts the session before falliblerecord_jit_mint; a poisoned JIT index can therefore leave an unowned session indefinitely when no expiry is configured. Anchor:crates/ironclaw_secrets/src/placeholder.rs:365.
Conventions
- Medium Avoid unwrap in production leak-pattern initialization (
crates/ironclaw_safety/src/leak_detector.rs:637, confidence 100) — the new production regex initialization uses.unwrap(), contrary to the repository production rule. Anchor:AGENTS.md:103.
Tests
-
Medium Missing lookup tests for expired and exhausted candidates (
crates/ironclaw_secrets/src/placeholder.rs:458-471, confidence 90) — the lookup skips these states, but coverage does not prove a stale candidate cannot prevent a later valid binding from being found. Anchor:crates/ironclaw_secrets/src/placeholder.rs:463. -
Medium Placeholder redaction is not tested in model-visible details (
crates/ironclaw_safety/src/leak_detector.rs:307-315, confidence 80) —redact_all_secretslacks a placeholder-specific test confirming the token is removed while surrounding diagnostic context remains. Anchor:crates/ironclaw_safety/src/leak_detector.rs:307.
| // the two ever drift apart. | ||
| LeakPattern { | ||
| name: "sandbox_credential_placeholder".to_string(), | ||
| regex: Regex::new(r"icsbx_[A-Za-z0-9]{16,}").unwrap(), // safety: hardcoded literal |
Collaborator
Author
There was a problem hiding this comment.
Medium — Avoid unwrap in production leak-pattern initialization.
The new production pattern initialization calls Regex::new(...).unwrap(). This violates the repository rule forbidding .unwrap() and .expect() in production code, even though the regex is currently a hardcoded literal.
Fix: Propagate or otherwise handle the regex construction error without using unwrap().
Collaborator
Author
There was a problem hiding this comment.
Not changing this one. scripts/check_no_panics.py explicitly skips any added line containing // safety: (script line 309), and the script's own error output documents that as the intended suppression mechanism ('Suppress false positives with an inline...'). The new sandbox_credential_placeholder pattern uses that convention exactly like the 21 pre-existing hardcoded-literal patterns in the same default_patterns() function — none of those are LazyLock-wrapped either. Making this one line a LazyLock/fallible exception would be inconsistent with its neighbors for a literal that cannot fail to compile as a regex. Open to being convinced otherwise, but keeping it as-is for now.
mint_on_first_use looked up and recorded a JIT mint through jit_minted in two separate lock/unlock cycles, so two threads racing on the identical (invocation, capability, account) binding could both observe "nothing minted yet" and each mint their own session — defeating the one-session-per-binding guarantee and multiplying a max_uses: Some(1) budget. Hold jit_minted once across the whole lookup-or-mint sequence instead, and drop it before binding the result to a placeholder (that part only needs "at most one session per binding", not "at most one bind call"). This is the first nested-lock pattern in this module, so it documents an explicit lock-ordering rule (jit_minted outermost) and splits release_lease/revoke_session into jit_minted-touching and jit_minted-free variants: try_join_live_lease's failure path runs while mint_on_first_use already holds jit_minted, and its old cleanup route (via revoke_session) would have re-locked jit_minted on the same thread and deadlocked on the non-reentrant std::sync::Mutex. Adds a Barrier-aligned 32-thread regression test asserting every racing lease resolves to the same session id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CredentialSessionRequest.expires_at was a plain Option, and create_session stored it unchanged, so None meant a genuinely unbounded session. Combined with revocation that depends on Drop running, a lease held or leaked forever was a standing grant for the process lifetime. Default a None expiry to CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS out and clamp anything longer than CREDENTIAL_SESSION_MAX_TTL_SECONDS (30 minutes each, per the project's already-decided design: explicit revoke stays primary, this is only the backstop). Enforcement is the existing lazy check in validate_session; no new enforcement path. Deliberately no background sweep task: proactive eviction would be standing infrastructure for a path with zero live callers today, and this crate owns no runtime to host it on — deferred to whichever later PR gives this store an owning runtime (noted in the code). create_session has no callers outside this crate's own tests today (egress proxy not built yet), so the blast radius is contained here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6689
July 27, 2026 02:30
Destroyed
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6689
July 27, 2026 02:48
Destroyed
redact_all_secrets tested detection-adjacent secrets but never an icsbx_ sandbox credential placeholder, and never confirmed that redaction masks only the token value while surrounding context (path, status code) survives. A redaction that nuked the whole string would pass a detection-only check while destroying the diagnostic value of sandbox output. The unwrap() in default_patterns() flagged in review is unchanged: it matches the same `// safety: hardcoded literal` convention as the 21 sibling regex patterns in this function, and the "No panics in production code" CI job (scripts/check_no_panics.py, which explicitly exempts any line containing `// safety:`) passed on this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6689
July 27, 2026 03:02
Destroyed
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_safety/src/leak_detector.rs`:
- Around line 868-884: Strengthen the assertions in the redaction regression
test around the existing redacted value by asserting the complete expected
output, rather than only checking that the full token is absent. Ensure the
assertion proves no token fragment or suffix remains while preserving the
workspace path, HTTP 401 status, and [REDACTED] marker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c337e440-f919-4f32-87f7-5c019b24086b
📒 Files selected for processing (1)
crates/ironclaw_safety/src/leak_detector.rs
sessions, jit_minted, sessions_by_placeholder, and lease_refcounts were four separately-lockable HashMaps on InMemoryCredentialBroker, even though jit_minted and sessions_by_placeholder are just secondary indices over the same session records. That fragmentation forced mint_on_first_use to document a lock-ordering rule (jit_minted always outermost) and keep two "ignoring_jit_minted" twin methods purely to dodge self-deadlock on the non-reentrant std::sync::Mutex when a failure path needed to revoke while jit_minted was already held. Collapse all four into one Mutex<SessionState>, and fold the outstanding-lease count into CredentialSessionRecord as lease_count, replacing the parallel HashMap<SessionId, usize>. mint_on_first_use now holds session_state locked across the whole lookup-or-mint-and-publish sequence as plain field access, so there is no second acquisition left to deadlock against. The twins (release_lease_ignoring_jit_minted, revoke_session_ignoring_jit_minted) and the lock-ordering doc comment are deleted outright, not refactored: the hazard they existed to dodge no longer exists. accounts stays a separate Mutex — create_session (via the new build_session split) already drops it before touching session state, so nesting it inside an already-held session_state lock cannot deadlock and cannot widen its own critical section. lock_or_recover moves from placeholder.rs to lib.rs (crate root) since it is now needed by inherent methods on InMemoryCredentialBroker defined there, and a private item in a child module is not visible to its parent. All previously-pinned properties (race-free concurrent first use, increment-then-validate ordering, revoke on all four lease exit paths, reused-session survival, secondary-index pruning, no standing grant on error, poisoned-lock recovery, cross-user isolation) still pass unchanged; the poisoned-lock regression test is updated to poison the single merged session_state mutex instead of the now-gone sessions_by_placeholder field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
placeholder.rs's #[cfg(test)] mod was ~745 lines, pushing the file to 1436 lines and past the repo's 1000-line convention despite the production body itself being small. Move it to a sibling placeholder_tests.rs via #[path], zero behavior change: super:: references still resolve since the module tree is identical, only the file that backs it moves. Also merge lease_revokes_on_explicit_success_call and lease_revokes_on_explicit_error_call into one lease_revokes_on_explicit_call: both called the identical CredentialSessionLease::revoke API and asserted the identical postcondition, differing only in a narrative `if dispatch_result.is_err()` wrapper that touches no broker code path. The timeout (cancellation-drop) and panic (unwind-drop) tests stay separate since those exercise genuinely different Rust mechanisms. Add the missing lookup-coverage case: a stale (expired or use-exhausted) session bound to a placeholder under the same scope as a later, still-valid one must not prevent the valid one from being found by find_session_by_placeholder. The existing multi-session test only covered scope-mismatch skipping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6689
July 27, 2026 03:26
Destroyed
Collaborator
Author
Review follow-upReplied to and resolved the fixed inline threads (IronLoop, Codex, CodeRabbit, and the Leak-pattern evasion — the Session lifecycle / concurrency — three related races and one lifecycle gap:
Error hygiene — P0 simplification ( Deliberate deferrals, stated not buried (all fine while nothing calls this path — the consumer is the not-yet-built egress proxy):
One rejected finding: the 🤖 Generated with Claude Code |
Collaborator
Author
|
@ironloopai review |
placeholder_tests.rs didn't match scripts/check_no_panics.py's is_test_only_path() exemption (src/**/tests.rs or src/**/tests/*.rs), so its test-fixture .unwrap() calls were scanned as production code, failing "No panics in production code" CI with 13 violations. Move it to placeholder/tests.rs, which matches the tests.rs filename exemption, instead of loosening the shared repo-wide panic scanner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6689
July 27, 2026 03:37
Destroyed
henrypark133
enabled auto-merge
July 27, 2026 03:54
henrypark133
disabled auto-merge
July 27, 2026 03:55
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.55% — 307491 / 359422 lines Per-crate breakdown (60 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
Open
henrypark133
added a commit
that referenced
this pull request
Jul 28, 2026
…, W8) Two unwired primitives for the sandbox credential firewall. Neither has a production caller yet; W6 (proxy TLS termination + injection) is the named consumer for both. Landing them separately keeps the eventual PR series unstacked. W5 - sandbox_process/ca.rs: in-memory root key + short-lived per-host leaf certs via rcgen. The root private key never touches disk and is never reachable from a container: it lives only in a private field, no method returns it, Debug is hand-written to omit it, and root_certificate_pem() returns only the public trust anchor (test-pinned to contain no PRIVATE KEY block). Architecture review on the rcgen dependency edge: SHIP. W8 - sandbox_process/credential_firewall.rs: obligation staging keyed (tenant, user), because invocation identity is unrecoverable from the proxy's source IP. The fail-closed matrix uses distinct types so a caller cannot collapse grant-denial (Ok(NoGrant) - strip placeholder, forward bare) into connection-denial (Err - deny outright). stage() returns an RAII lease whose Drop revokes, mirroring ironclaw_secrets' CredentialSessionLease; caller discipline alone was rejected because it shipped a Critical standing-grant leak in that sibling mechanism (PR #6689). TTL is clamped to MAX_GRANT_TTL as the numeric backstop, with the lease as the primary bound. Expired entries are reclaimed on read rather than only filtered. Regression tests: 11 CA tests (chain-of-trust, per-host SAN isolation, TTL expiry, cache eviction, no key material in the container-facing artifact) and 14 firewall tests (cross-tenant and cross-user isolation, both denial categories, deadline and TTL expiry, lease-drop and drop-during-unwind revocation, TTL clamp, reclamation observed via map size). Ratchet baselines added for both new files at the counts the ratchet itself reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
29 tasks
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
…rai#6689) * feat(secrets): stable placeholder registry + JIT sessions The sandbox container must never see real secret material, even transiently. It now holds only an inert `icsbx_`-prefixed placeholder token, stable per (tenant, user, provider) and separate from the session store, so holding one grants nothing on its own. InMemoryCredentialBroker gains JIT minting (mint_on_first_use): a CredentialSession is minted only at actual first use of an (invocation, binding) pair, not staged up front, and bound to the placeholder so the egress proxy can find it. CredentialSessionLease guarantees the session is revoked exactly once regardless of how the dispatch ends -- success/error via explicit revoke(), timeout/panic via Drop during future-cancellation or unwind -- because a missed revoke path is a silent standing grant. W6 (egress proxy consumer) and W8 (obligation chokepoint) are not built yet; this lands the placeholder/session primitives unwired, per plan, for them to call into later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(safety): detect sandbox credential placeholders The upcoming credential firewall injects inert icsbx_-prefixed placeholders into the sandbox in place of real secrets; the egress proxy swaps them for the real credential at request time. Placeholders are stable and inert but must never cross the trust boundary into model output, logs, or transcripts. Sandbox exec output already runs through the leak detector, so only the pattern was missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): refcount JIT leases and multi-bind placeholder sessions Round-1 code review found two correctness gaps in the JIT credential- session skeleton: mint_on_first_use's cache-hit reuse handed out a second lease over an already-live session with no reference counting, so the first caller to revoke/drop its lease killed the session out from under a second caller still holding it; and sessions_by_placeholder was a single-valued map, so binding a second session to a stable placeholder (a second account under the same provider, or an overlapping invocation) silently clobbered the first binding instead of tracking both. Fixes: - Add a per-session lease refcount; a session is only actually revoked once its last outstanding lease releases it. - Make sessions_by_placeholder multi-valued (HashSet per token) and prune it, plus jit_minted and lease_refcounts, from revoke_session so a long-lived process doesn't accumulate stale entries. - revoke_session recovers from a poisoned lock instead of silently no-op'ing, matching the "a missed revoke can never leave a standing grant" invariant this module documents. - Add TryFrom<String>/AsRef<str>/into_inner() to CredentialPlaceholderToken and rename CredentialPlaceholderOwner::provider_id to provider_or_extension_id, matching this crate's existing newtype/naming conventions. - Pin the icsbx_ prefix shared between ironclaw_secrets and the ironclaw_safety leak-detector pattern with a dev-dependency-only regression test instead of a real crate dependency, so ironclaw_safety stays a dependency-light substrate (ruled via thermo-nuclear review). - Tighten cross-reference comments (W6 -> W6-EGRESS-PROXY, name the concrete sibling leak pattern) per local-patterns review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): close JIT lease reuse race and validate placeholder charset Round 2 review (performance/tests dimensions round 1 missed): - mint_on_first_use's cache-hit path validated a jit_minted session and only incremented its lease refcount afterward, as separate lock acquisitions. A concurrent release_lease on the last outstanding lease could revoke the session in that window, handing back a lease for an already-dead session. Fixed by incrementing the refcount first (only succeeds while a live entry exists) and validating after, backing out the reference if invalid — serializes the join against release_lease on the same lease_refcounts lock instead of racing across mutexes. - CredentialPlaceholderToken::validate only checked the prefix, unlike this crate's own validate_credential_id convention; a malformed suffix (e.g. carrying control characters) could reach a container's env/logs once this token round-trips through the not-yet-built egress proxy. Now enforces the same shape generate() produces. - Dropped acquire_lease_refcount's vestigial Result (it can't fail) and deduplicated mint_on_first_use's bind+lease-construction tail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): stop echoing raw placeholder tokens and cap parsed length InvalidPlaceholderToken interpolated the raw, untrusted `value` verbatim into its Display impl, so a rejected sandbox-supplied token — which can carry control characters, ANSI escapes, or a real secret pasted into the wrong slot — propagated into every log line derived from the error. `reason` already carries all actionable information, so drop `value` entirely rather than sanitizing it (per the repo's boundary-mapping guideline for credential-adjacent errors, which wins over matching the sibling `InvalidAccountId` convention here since this value is attacker-controlled). Also cap the parsed token length: validation only enforced a minimum suffix length, so an arbitrarily long `icsbx_...` string handed back from the sandbox was accepted and then hashed/cloned/inserted into maps - unbounded work driven by untrusted input. Registry-issued tokens are always exactly 32 alphanumeric characters (a UUIDv4 `simple()` suffix), and nothing needs a variable-length suffix, so validation now requires that exact length instead of "at least 16". The length constant is exposed publicly (CREDENTIAL_PLACEHOLDER_SUFFIX_LEN) so the leak detector's cross-crate pinning test can assert against it instead of a bare literal (see the companion ironclaw_safety commit). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(safety): close word-boundary bypass in placeholder leak pattern The sandbox_credential_placeholder regex was \bicsbx_[A-Za-z0-9]{16,}\b. `_` is a word character, so \b does not fire next to it: a single leading or trailing byte (`_icsbx_...`, `icsbx_..._x`) defeated the one pattern standing between a leaked placeholder and model output/logs. Drop the word boundaries entirely — `icsbx_` plus 16+ alphanumerics is a distinctive shape with no realistic false-positive risk, and over-matching here fails safe. Extends the existing test rather than adding a parallel file: added leading-underscore, trailing-underscore, and leading-letter cases, and flipped the prior "substring of a longer word is NOT flagged" test to assert it IS flagged now, since that flip is the deliberate fail-safe tradeoff. Also pins the length half of the sandbox_credential_placeholder / ironclaw_secrets contract, not just the prefix: the cross-crate test now asserts CREDENTIAL_PLACEHOLDER_SUFFIX_LEN stays at or above the regex's own 16-char floor, and additionally constructs a minimum-shaped token through the registry's public parse() API and asserts the detector actually flags it - pinning behavior instead of a literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): close concurrent-first-use race in JIT session minting mint_on_first_use looked up and recorded a JIT mint through jit_minted in two separate lock/unlock cycles, so two threads racing on the identical (invocation, capability, account) binding could both observe "nothing minted yet" and each mint their own session — defeating the one-session-per-binding guarantee and multiplying a max_uses: Some(1) budget. Hold jit_minted once across the whole lookup-or-mint sequence instead, and drop it before binding the result to a placeholder (that part only needs "at most one session per binding", not "at most one bind call"). This is the first nested-lock pattern in this module, so it documents an explicit lock-ordering rule (jit_minted outermost) and splits release_lease/revoke_session into jit_minted-touching and jit_minted-free variants: try_join_live_lease's failure path runs while mint_on_first_use already holds jit_minted, and its old cleanup route (via revoke_session) would have re-locked jit_minted on the same thread and deadlocked on the non-reentrant std::sync::Mutex. Adds a Barrier-aligned 32-thread regression test asserting every racing lease resolves to the same session id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): cap credential session expiry at creation CredentialSessionRequest.expires_at was a plain Option, and create_session stored it unchanged, so None meant a genuinely unbounded session. Combined with revocation that depends on Drop running, a lease held or leaked forever was a standing grant for the process lifetime. Default a None expiry to CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS out and clamp anything longer than CREDENTIAL_SESSION_MAX_TTL_SECONDS (30 minutes each, per the project's already-decided design: explicit revoke stays primary, this is only the backstop). Enforcement is the existing lazy check in validate_session; no new enforcement path. Deliberately no background sweep task: proactive eviction would be standing infrastructure for a path with zero live callers today, and this crate owns no runtime to host it on — deferred to whichever later PR gives this store an owning runtime (noted in the code). create_session has no callers outside this crate's own tests today (egress proxy not built yet), so the blast radius is contained here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): close standing-grant leak when placeholder bind fails finish_lease constructed the CredentialSessionLease only after the fallible bind_placeholder_to_session call, so a bind failure propagated `?` with the session already refcounted by the caller (mint or reuse path) but no lease ever built to drop and release that reference — an unrevocable standing grant surviving to expiry. Construct the lease first so any early return still drops it through the normal path, and switch bind_placeholder_to_session (and the sibling read, placeholder_session_ids) to lock_or_recover so a poisoned sessions_by_placeholder mutex is recovered instead of failing closed with a dangling reference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): collapse registry to one lock, tighten docs and length check CredentialPlaceholderRegistry::get_or_create wrote by_owner and by_token under two separate mutexes, so a concurrent get_or_create for the same triple could take the by_owner early-return before by_token caught up, and resolve() would answer None for a token already handed out. Collapse both maps into one Mutex<RegistryState> so every token resolves the instant get_or_create returns. Also: soften CredentialSessionLease's doc comment, which overclaimed "no safe way to leak the session" — std::mem::forget is safe, skips Drop, and strands the refcount; note the 30-minute expiry cap as the actual backstop. Tighten a stale comment in mint_on_first_use (the jit_minted-held-across-the-sequence fix already closed the double-mint window; the comment now says so precisely instead of leaving room to misread it as still open). And measure the placeholder suffix length in chars, matching the error message's "characters" wording, instead of bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(safety): pin placeholder redaction preserves diagnostic context redact_all_secrets tested detection-adjacent secrets but never an icsbx_ sandbox credential placeholder, and never confirmed that redaction masks only the token value while surrounding context (path, status code) survives. A redaction that nuked the whole string would pass a detection-only check while destroying the diagnostic value of sandbox output. The unwrap() in default_patterns() flagged in review is unchanged: it matches the same `// safety: hardcoded literal` convention as the 21 sibling regex patterns in this function, and the "No panics in production code" CI job (scripts/check_no_panics.py, which explicitly exempts any line containing `// safety:`) passed on this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): collapse session-lifecycle mutexes into one lock sessions, jit_minted, sessions_by_placeholder, and lease_refcounts were four separately-lockable HashMaps on InMemoryCredentialBroker, even though jit_minted and sessions_by_placeholder are just secondary indices over the same session records. That fragmentation forced mint_on_first_use to document a lock-ordering rule (jit_minted always outermost) and keep two "ignoring_jit_minted" twin methods purely to dodge self-deadlock on the non-reentrant std::sync::Mutex when a failure path needed to revoke while jit_minted was already held. Collapse all four into one Mutex<SessionState>, and fold the outstanding-lease count into CredentialSessionRecord as lease_count, replacing the parallel HashMap<SessionId, usize>. mint_on_first_use now holds session_state locked across the whole lookup-or-mint-and-publish sequence as plain field access, so there is no second acquisition left to deadlock against. The twins (release_lease_ignoring_jit_minted, revoke_session_ignoring_jit_minted) and the lock-ordering doc comment are deleted outright, not refactored: the hazard they existed to dodge no longer exists. accounts stays a separate Mutex — create_session (via the new build_session split) already drops it before touching session state, so nesting it inside an already-held session_state lock cannot deadlock and cannot widen its own critical section. lock_or_recover moves from placeholder.rs to lib.rs (crate root) since it is now needed by inherent methods on InMemoryCredentialBroker defined there, and a private item in a child module is not visible to its parent. All previously-pinned properties (race-free concurrent first use, increment-then-validate ordering, revoke on all four lease exit paths, reused-session survival, secondary-index pruning, no standing grant on error, poisoned-lock recovery, cross-user isolation) still pass unchanged; the poisoned-lock regression test is updated to poison the single merged session_state mutex instead of the now-gone sessions_by_placeholder field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(secrets): split placeholder tests out, merge redundant case placeholder.rs's #[cfg(test)] mod was ~745 lines, pushing the file to 1436 lines and past the repo's 1000-line convention despite the production body itself being small. Move it to a sibling placeholder_tests.rs via #[path], zero behavior change: super:: references still resolve since the module tree is identical, only the file that backs it moves. Also merge lease_revokes_on_explicit_success_call and lease_revokes_on_explicit_error_call into one lease_revokes_on_explicit_call: both called the identical CredentialSessionLease::revoke API and asserted the identical postcondition, differing only in a narrative `if dispatch_result.is_err()` wrapper that touches no broker code path. The timeout (cancellation-drop) and panic (unwind-drop) tests stay separate since those exercise genuinely different Rust mechanisms. Add the missing lookup-coverage case: a stale (expired or use-exhausted) session bound to a placeholder under the same scope as a later, still-valid one must not prevent the valid one from being found by find_session_by_placeholder. The existing multi-session test only covered scope-mismatch skipping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(secrets): move placeholder tests to conform to test-path convention placeholder_tests.rs didn't match scripts/check_no_panics.py's is_test_only_path() exemption (src/**/tests.rs or src/**/tests/*.rs), so its test-fixture .unwrap() calls were scanned as production code, failing "No panics in production code" CI with 13 violations. Move it to placeholder/tests.rs, which matches the tests.rs filename exemption, instead of loosening the shared repo-wide panic scanner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The sandbox container is given only a stable, inert token prefixed
icsbx_(keyed{tenant,user,provider}) instead of a real secret — the invariant being that secret material never enters the container, even transiently. ACredentialSessionis minted just-in-time per(invocation × binding)at actual first use; staging every binding up front would itself be a standing grant. Revocation uses an RAII lease so success, error, timeout, and panic paths all revoke — a missed revoke path would silently leave a standing grant. The leak detector gains anicsbx_pattern so a placeholder can never cross the boundary into model output or logs (sandbox output already runs through the detector, so this is pattern-only).🔴 Not wired to production
Nothing calls this yet. Grep for the new public symbols (
CredentialPlaceholderRegistry,CredentialPlaceholderToken,CredentialSessionLease,mint_on_first_use,find_session_by_placeholder,CREDENTIAL_PLACEHOLDER_PREFIX) across the workspace turns up matches only in three places:crates/ironclaw_secrets/src/lib.rs— re-exports and doc commentscrates/ironclaw_secrets/src/placeholder.rs— the definitions themselves and their unit testscrates/ironclaw_safety/src/leak_detector.rs— theicsbx_leak-pattern constant/comments (pattern-only; no calls into the registry/broker API)No production call site anywhere else in the workspace. Consumers land in later PRs, and the whole path will ultimately be gated behind a deployment profile.
Series context
First of several PRs landing the sandbox credential firewall incrementally. Deliberately not stacked — stacked PRs drift badly under review plus rebase. Each slice is independently reviewable off
mainand dormant until activated by profile.Review history
This slice went through two full review rounds before opening, which found real defects, not nits:
revoke_sessionsilently no-opping on a poisoned lock, contradicting the "never leave a standing grant" invariant;jit_mintedgrowth;Each has a regression test.
Test coverage
Tests pin these invariants:
icsbx_leak pattern matches what the registry actually generatesKnown deferrals (called out, not hidden)
revoke_sessionuses O(n)retain()scans and the store uses unsharded global mutexes — both fine while nothing calls this, both worth revisiting when the egress proxy wires in per-connection traffic. Per-account disambiguation by request target is deliberately left to that proxy.What is NOT in this PR
🤖 Generated with Claude Code