fix(reborn): fingerprint credential setup in the auth gate-record key (#6299 follow-up) - #6304
ilblackdragon wants to merge 1 commit into
Conversation
…#6299 IronLoop) `stable_auth_gate_id` fingerprinted only `capability`, `required_secrets`, and per-requirement `provider:requester_extension:provider_scopes` — it omitted `RuntimeCredentialAuthRequirement.setup`. Since `GateRecord::Auth` is write-once and retained, changing a capability's setup (ManualToken → OAuth/Pairing, or an OAuth setup-scope change) derived the SAME `auth-{sha256}` key as the old flow and collided with the stale record; the runner then reloaded the obsolete requirements and presented the wrong auth flow on resume. Include a deterministic `setup` fingerprint in the per-requirement key via `auth_setup_fingerprint` (exhaustive match — a new setup variant fails the build rather than silently sharing a key; OAuth setup scopes sorted for order independence). Regression: `stable_auth_gate_id_distinguishes_credential_setup` asserts ManualToken vs OAuth, and OAuth setups with different setup scopes, derive distinct keys while an identical requirement stays stable — verified it fails before this fix. clippy (test-support,libsql) clean on ironclaw_host_runtime. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🔎 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. |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesAuth gate fingerprinting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
github-actions
Bot
added
size: M
|
Closing in favor of #6303 (@ilblackdragon), which lands the same fix — |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the stable_auth_gate_id fingerprint generation to include the credential auth setup, resolving issue #6299 where different auth flows could collide on the same gate-record key. It also introduces a helper function auth_setup_fingerprint and corresponding unit tests. The review feedback highlights potential delimiter collision vulnerabilities in both the main fingerprinting logic and the helper function, recommending the use of an injective length-prefixed encoding to guarantee unique and secure identifiers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| format!( | ||
| "credential={}:{}:{}", | ||
| "credential={}:{}:{}:{}", | ||
| requirement.provider.as_str(), | ||
| requirement.requester_extension.as_str(), | ||
| scopes.join(",") | ||
| scopes.join(","), | ||
| auth_setup_fingerprint(&requirement.setup), | ||
| ) |
There was a problem hiding this comment.
There is a potential delimiter collision vulnerability here. Since both provider_scopes and OAuth scopes can contain colons (:), joining them with commas and then formatting with colons can lead to identical fingerprints for different requirements. To eliminate the risk of separator-collision attacks or accidental collisions, use an injective length-prefixed encoding rather than simple concatenation with a delimiter.
[
requirement.provider.as_str(),
requirement.requester_extension.as_str(),
&scopes.join(","),
&auth_setup_fingerprint(&requirement.setup),
]
.iter()
.map(|s| format!("{}:{}", s.len(), s))
.collect::<String>()References
- When generating deterministic identifiers or hashes from multiple string components, use an injective length-prefixed encoding rather than simple concatenation with a delimiter to eliminate the risk of separator-collision attacks or accidental collisions.
| Setup::OAuth { scopes } => { | ||
| let mut scopes = scopes.clone(); | ||
| scopes.sort(); | ||
| format!("oauth:{}", scopes.join(",")) | ||
| } |
There was a problem hiding this comment.
To prevent delimiter collisions when OAuth scopes contain colons, use an injective length-prefixed encoding instead of joining with delimiters or using debug formatting. This ensures that the scopes are serialized with clear, unambiguous boundaries.
| Setup::OAuth { scopes } => { | |
| let mut scopes = scopes.clone(); | |
| scopes.sort(); | |
| format!("oauth:{}", scopes.join(",")) | |
| } | |
| Setup::OAuth { scopes } => { | |
| let mut scopes = scopes.clone(); | |
| scopes.sort(); | |
| let mut parts = vec!["oauth".to_string()]; | |
| parts.extend(scopes); | |
| parts.iter().map(|s| format!("{}:{}", s.len(), s)).collect::<String>() | |
| } |
References
- When generating deterministic identifiers or hashes from multiple string components, use an injective length-prefixed encoding rather than simple concatenation with a delimiter to eliminate the risk of separator-collision attacks or accidental collisions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_host_runtime/src/production.rs`:
- Around line 2130-2136: Update the fingerprint construction in the format block
and auth_setup_fingerprint so every provider, requester, scope element, and
setup component is structurally unambiguous. Replace bare colon/comma
concatenation with bounded serialization or equivalent length/escaping-aware
encoding, preserving scope and array boundaries so distinct inputs always
produce distinct gate-record keys.
🪄 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: 0345f702-438a-47a4-8540-49ad28f89157
📒 Files selected for processing (1)
crates/ironclaw_host_runtime/src/production.rs
| format!( | ||
| "credential={}:{}:{}", | ||
| "credential={}:{}:{}:{}", | ||
| requirement.provider.as_str(), | ||
| requirement.requester_extension.as_str(), | ||
| scopes.join(",") | ||
| scopes.join(","), | ||
| auth_setup_fingerprint(&requirement.setup), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Key collision via ambiguous delimiters in fingerprint.
Using bare : and , for concatenation creates collisions between adjacent fields, defeating the PR's core objective to guarantee distinct gate-record keys.
For example:
- Scope boundary collision:
provider_scopes=["read:oauth"]+setup=ManualTokenproduces...:read:oauth:manual_token, which identically collides withprovider_scopes=["read"]+setup=OAuth(["manual_token"]). - Array boundary collision:
scopes=["a,b", "c"]collides withscopes=["a", "b,c"]because both join toa,b,c.
Use unambiguous bounding (e.g., brackets [{}], or JSON serialization) instead of bare delimiters to preserve the domain structure.
🛡️ Proposed fix to unambiguously bound the fingerprint parts
format!(
- "credential={}:{}:{}:{}",
+ "credential={}:{}:[{}]-[{}]",
requirement.provider.as_str(),
requirement.requester_extension.as_str(),
- scopes.join(","),
+ scopes.join(" "),
auth_setup_fingerprint(&requirement.setup),
)And in auth_setup_fingerprint (lines 2159-2163):
Setup::OAuth { scopes } => {
let mut scopes = scopes.clone();
scopes.sort();
- format!("oauth:{}", scopes.join(","))
+ format!("oauth:[{}]", scopes.join(" "))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| format!( | |
| "credential={}:{}:{}", | |
| "credential={}:{}:{}:{}", | |
| requirement.provider.as_str(), | |
| requirement.requester_extension.as_str(), | |
| scopes.join(",") | |
| scopes.join(","), | |
| auth_setup_fingerprint(&requirement.setup), | |
| ) | |
| format!( | |
| "credential={}:{}:[{}]-[{}]", | |
| requirement.provider.as_str(), | |
| requirement.requester_extension.as_str(), | |
| scopes.join(" "), | |
| auth_setup_fingerprint(&requirement.setup), | |
| ) |
🤖 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_host_runtime/src/production.rs` around lines 2130 - 2136,
Update the fingerprint construction in the format block and
auth_setup_fingerprint so every provider, requester, scope element, and setup
component is structurally unambiguous. Replace bare colon/comma concatenation
with bounded serialization or equivalent length/escaping-aware encoding,
preserving scope and array boundaries so distinct inputs always produce distinct
gate-record keys.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | bd360381c5f6 |
Head: bd360381c5f63a6db2bc03fbd57e146bbcb1da89
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
Focused review of 1 Rust file (+78/-2) found one remaining gate-key collision in the new OAuth setup fingerprint.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Encode OAuth scope vectors unambiguously
Location: crates/ironclaw_host_runtime/src/production.rs:2162
Joining scope strings with commas is not injective: OAuth { scopes: vec!["a,b"] } and OAuth { scopes: vec!["a", "b"] } both fingerprint as oauth:a,b. Setup scopes are a Vec<String> without delimiter validation, so this configuration change retains the same write-once auth gate key and can reload the old GateRecord::Auth requirements. Use a canonical structured or length-prefixed encoding and add a regression assertion for this pair.
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.
| Setup::OAuth { scopes } => { | ||
| let mut scopes = scopes.clone(); | ||
| scopes.sort(); | ||
| format!("oauth:{}", scopes.join(",")) |
There was a problem hiding this comment.
Joining scope strings with commas is ambiguous: OAuth { scopes: vec!["a,b"] } and OAuth { scopes: vec!["a", "b"] } both yield oauth:a,b. That leaves a setup-only change on the same write-once gate key, so the stale auth record can still be reused. Please use an unambiguous structured or length-prefixed encoding and cover this collision.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.2% — 319539 / 370706 lines Per-crate breakdown (65 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)
|
This branch was successfully deployed
1 participant
Follow-up to a MEDIUM IronLoop finding that landed on
mainwhen #6299 was squash-merged before the fix was pushed (the fix was left on the deleted integration branch).The bug (now on
main)stable_auth_gate_id(ironclaw_host_runtime/src/production.rs) fingerprintscapability,required_secrets, and per-requirementprovider:requester_extension:provider_scopes— but omitsRuntimeCredentialAuthRequirement.setup. SinceGateRecord::Authis write-once and retained, changing a capability's setup (ManualToken → OAuth/Pairing, or an OAuth setup-scope change) derives the SAMEauth-{sha256}key as the old flow, collides with the stale record, and the runner reloads the obsolete requirements — presenting the wrong auth flow on resume.Fix
Include a deterministic
setupfingerprint in the per-requirement key viaauth_setup_fingerprint(exhaustive match — a new setup variant fails the build rather than silently sharing a key; OAuth setup scopes are sorted for order independence).Test
stable_auth_gate_id_distinguishes_credential_setupasserts ManualToken vs OAuth, and OAuth setups with different setup scopes, derive distinct keys while an identical requirement stays stable. Verified it fails before the fix. clippy (test-support,libsql) clean.🤖 Generated with Claude Code