Repository navigation
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: XL
🔎 Review · PR #6769
2 actionable findings →Reviewed the complete trusted base-to-head comparison. The new attested-signing runtime has a blocking authorization bypass in custodial signing and a registration atomicity defect that can permanently strand gates. Manifest, lockfile, runtime accessor, secret-key generation, snapshot, and external-wallet dependency changes were also inspected. Automatic · PR opened · attempt 1 of 3 · completed in 2m 34s Run details
|
![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: 7c98ec31ce
ℹ️ 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".
| let verified = self | ||
| .driver | ||
| .verify_and_sign(&signing_gate_ref, &proof) | ||
| .await | ||
| .map_err(map_continuation_error)?; |
There was a problem hiding this comment.
If resume_turn fails or the process stops after this call succeeds, verify_and_sign has already claimed the one-shot grant, advanced the signing ledger, and produced a handle that exists only in memory, while the turn remains BlockedAttested. A retry then fails on the claimed grant or existing ledger row, permanently wedging the gate; persist a prepared continuation and couple it transactionally/CAS-safely to the turn transition, or defer the consuming operations until a recoverable post-transition stage.
AGENTS.md reference: AGENTS.md:L189-L190
Useful? React with 👍 / 👎.
| Ok(VerifiedContinuation { | ||
| gate_ref: gate_ref.clone(), | ||
| context: binding.context.clone(), | ||
| signed: verified.proof().payload().to_vec(), |
There was a problem hiding this comment.
When a real broadcaster is wired for Injected, WalletConnect, or NEAR, these bytes are the provider's encoded proof blob, not a chain-native serialized signed transaction: the payload contains attestation/session metadata and a signature over the approved hash or signing payload. Passing it to an eth_sendRawTransaction/Solana/NEAR broadcaster will be rejected after the grant has been claimed and the ledger advanced, so the verified signature must be attached to the authoritative transaction and encoded in the chain's broadcast format first.
Useful? React with 👍 / 👎.
| Ok(VerifiedContinuation { | ||
| gate_ref: gate_ref.clone(), | ||
| context: binding.context.clone(), | ||
| signed: outcome.signature, | ||
| signer: outcome.signer, |
There was a problem hiding this comment.
For the custodial EVM path, CustodialSignOutcome::signature is only the raw signature, whereas the broadcaster contract requires the complete RLP-encoded signed transaction. Once a submitting broadcaster is installed, every custodial submission will therefore fail after consuming the grant and ledger row; attach the signature to the rebuilt transaction and encode the signed envelope before creating VerifiedContinuation.
Useful? React with 👍 / 👎.
|
|
||
| /// The assembled signer-continuation driver dispatched when a turn reaches | ||
| /// `AttestedResolved`. | ||
| pub fn driver(&self) -> &Arc<InMemoryContinuationDriver> { |
There was a problem hiding this comment.
Any downstream composition consumer can obtain this raw driver and call verify_and_sign or continue_after_resolved directly, bypassing RebornAttestedContinuation's binding-owner check and the intended turn-status/run checks; the custodial path even ignores the supplied proof and signs from the binding. A leaked gate reference can therefore drive another user's signature through an internal caller, so expose only the owner-checking service-shaped port rather than the lower-level driver handle.
AGENTS.md reference: crates/ironclaw_reborn_composition/AGENTS.md:L21-L27
Useful? React with 👍 / 👎.
| match self | ||
| .grants |
There was a problem hiding this comment.
If bindings.put subsequently rejects the binding—for example because validate_binding reports ChainMismatch—or the process stops between these two writes, the grant remains sealed without an authoritative binding. Retrying a corrected registration with the same grant key then returns DuplicateBinding, leaving the gate unusable; validate first and commit both records with a shared bounded CAS/transaction or compensate the grant write on failure.
AGENTS.md reference: AGENTS.md:L189-L190
Useful? React with 👍 / 👎.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Review complete · PR #6769
Reviewed the complete trusted base-to-head comparison. The new attested-signing runtime has a blocking authorization bypass in custodial signing and a registration atomicity defect that can permanently strand gates. Manifest, lockfile, runtime accessor, secret-key generation, snapshot, and external-wallet dependency changes were also inspected.
Findings
- 🔴 High · Custodial continuation bypasses WebAuthn proof verification —
crates/ironclaw_attested_runtime/src/driver.rs:415-419
Details are attached to the relevant diff. - 🟠 Medium · Gate registration can seal a grant without recording its binding —
crates/ironclaw_reborn_composition/src/attested.rs:223-246
Details are attached to the relevant diff.
Validation and technical details
- Compared refs/ironloop/base (9bacf20) through refs/ironloop/head (7c98ec3), covering all 22 changed files.
- Traced custodial continuation through
verify_and_sign,sign_custodial,CustodialSignerLike, and the underlying chain-signingCustodialSigner; none validates the supplied WebAuthn proof. - Confirmed the new composition test signs successfully using an empty WebAuthn proof payload.
- Inspected binding validation, sealed-grant CAS semantics, resume guards, ledger transitions, broadcast recovery, proof decoding and bounds, ownership checks, transaction rebuilding, runtime optional wiring, manifests, lockfile, and pub-use snapshot.
git diff --check refs/ironloop/base...refs/ironloop/headcompleted without whitespace errors.- Targeted Rust tests could not be executed because
cargois unavailable in the review environment (/bin/bash: cargo: command not found). - Base:
attested-g3-wallet-providers - Head:
attested-g4-runtime-ingressat7c98ec3 - Run:
1c120e45-ac9e-43e6-8f93-e1729b4ba5df
| match binding.provider_id { | ||
| ProviderId::Custodial => self.sign_custodial(gate_ref, &binding).await, | ||
| external => { | ||
| self.verify_external_wallet(gate_ref, external, &binding, proof) | ||
| .await |
There was a problem hiding this comment.
🔴 High · Custodial continuation bypasses WebAuthn proof verification
For an authoritative Custodial binding, verify_and_sign calls sign_custodial without passing or inspecting proof. That path claims the sealed grant and signs, but never validates the required WebAuthn assertion. Consequently, a binding owner can submit an empty or unrelated proof and trigger use of the custodial key. The tests currently confirm this behavior by expecting success with WebAuthnAssertionProof(vec![]). Require the custodial proof variant and run it through the WebAuthn verifier before claiming the grant, advancing the ledger, or accessing key material.
| let grant_key = GrantKey::from_context(&binding.context, binding.approved_tx_hash); | ||
| match self | ||
| .grants | ||
| .seal( | ||
| AttestedSigningGrant::new(grant_key, created_at_ms, expiry_ms) | ||
| .map_err(RegisterAttestedGateError::Grant)?, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(()) => {} | ||
| Err(GrantError::AlreadySealed) => { | ||
| return Err(RegisterAttestedGateError::DuplicateBinding); | ||
| } | ||
| Err(other) => return Err(RegisterAttestedGateError::Grant(other)), | ||
| } | ||
|
|
||
| // Insert-only, ATOMIC + VALIDATED: the store's `put` is insert-only (the | ||
| // existence check and the insert happen under a single critical section, | ||
| // closing the check-then-act TOCTOU window) and fully validates the | ||
| // binding (`gate_ref`/hash/chain/signer self-consistency) before | ||
| // persisting. An existing binding for this gate fails closed with | ||
| // `AlreadyExists` — treat it as a duplicate, consistent with the grant | ||
| // CAS above; any other validation/backend error fails closed too. | ||
| match self.bindings.put(gate_ref, binding).await { |
There was a problem hiding this comment.
🟠 Medium · Gate registration can seal a grant without recording its binding
Registration performs two separately committed writes: it seals the one-shot grant first, then inserts and validates the binding. If binding validation or storage fails, the sealed grant remains. A retry with the corrected binding then receives AlreadySealed and is reported as a duplicate, permanently stranding that gate in this composition. Validate before sealing and commit both records atomically (or provide a safe rollback/CAS transaction) so partial failure cannot consume the gate identity.
9bacf20 to
af1744c
Compare
7c98ec3 to
57213b8
Compare
Group 4 of the attested-signing consolidation. Lands the ironclaw_attested_runtime crate (resume port, signer-continuation driver, custodial ship-gate) and the composition seam that reaches it. Ported onto restructured main, which moved a great deal under this work: - The `attested_signing` composition threads stores -> RebornRuntime -> accessor. Main flattened RebornServices out of RebornRuntime, so it is a field rather than a nested lookup. Production wires None: with no durable attested backend the gate ingress has nothing to dispatch to and refuses, rather than half-resolving a signature. - production_turn_state_store takes an optional AttestedResumePort. Without a port the store never admits an attested resume -- the fail-closed default, not a silent no-op. - factory.rs was NOT merged. Git aligned its conflicts against unrelated regions (one 1,390-line hunk paired flow-record wiring against test-support attachment ports), because main restructured that file wholesale. The real change is 73 insertions, so it was applied by hand at the correct anchors instead. Two ratchets, both genuine: - ironclaw_attested_runtime declared no [package.metadata.ironclaw] layer. Declared kernel: it depends on ironclaw_turns, so it sits above the attestation/chain-signing substrates it drives and below the composition that wires it. - LocalDevContinuationDriver / LocalDevCustodialSigner tripped the LocalDev typename ratchet, which bans mode-shaped type names because a deployment mode is a DeploymentConfig value. Renamed InMemoryContinuationDriver / InMemoryCustodialSigner -- named for the stores they pin, which is also the accurate distinction: a durable composition differs by its stores, not by which environment runs it. build_attested_composition is deliberately NOT included. Main moved local-dev composition out of this crate, so nothing here would call it; carrying it as dead code in a signing path is worse than its absence. It lands with the group that wires the durable backends. The composition pub-use snapshot is regenerated for the two new attested exports -- an intentional public service change. [skip-regression-check] Both ratchets are the regression tests and both pass; the rest is porting existing covered behaviour onto renamed main APIs, verified by the crates' existing suites.
57213b8 to
3c60bfb
Compare
Review-response summary (g4 — signer-continuation runtime + composition seam)Force-pushed, so inline anchors have moved. Consolidated response to all 7 findings. Changed since review: the branch commit was amended to fix a test that the upstream That break is worth flagging on its own: I force-pushed g4–g8 once without compiling them, and this test build was red as a result. I caught it only when the stack tip failed to build. All nine branches have since been compiled individually with Genuinely open — not addressedNone of the seven findings on this PR are fixed in this push. Listing them plainly rather than leaving them ambiguous:
Related change landing downstreamThis branch's Verification for this branch: |
The WebAuthn proof in this stack has no verifierFollowing up on the review finding "Custodial continuation bypasses WebAuthn proof verification" ( At the stack tip, Nothing else. And searching the whole workspace (excluding So the custodial path carries a WebAuthn assertion as an opaque byte payload that nothing ever checks. The proof type is plumbed; the verification is absent. That is worse than a bypass in one branch, because the shape of the code suggests verification exists somewhere. Where the verifier actually is#3964 — "durable one-shot challenge store + WebAuthn verifier + fail-closed audit (attested-signing PR4/10)". It is APPROVED and not merged, and I confirmed it is not an ancestor of this stack: When the 20-PR stack was consolidated into 8 PRs against restructured Consequence for the custodial trust modelWorth being explicit about what this means, since the custodial path is the one that signs with a key we hold:
This is also why the fail-closed state introduced by #6822 is the right posture for now: with gate raising disabled pending #6860, the custodial resolve path is not reachable from the agent loop. Suggested sequencing
Doing 3 before 1–2 would re-open a path to a custodial signature whose authorization artifact is unverified, so the order matters. I have not done any of this — flagging it because "the verifier exists but isn't called" and "the verifier isn't in the codebase" call for very different fixes, and the review comment reads as the former. |
This branch has not been deployed
1 participant
What
Group 4 of 8. Stacks on #6755. The
ironclaw_attested_runtimecrate — resume port, signer-continuation driver, custodial mainnet ship-gate — plus the composition seam that reaches it.Scope change worth flagging
This group was planned as "runtime + WebUI ingress". The ingress is deferred to its own PR. Main deleted
webui_inboundentirely (zero references) and replaced it with a capability/command-constant dispatch model. So the attested gate/resolve ingress needs re-expressing in that vocabulary, not porting — andRESOLVE_GATE_COMMANDalready exists, so the attested resolve is a gate resolution carrying a claim rather than a new command. That is design work with an authorization boundary in it (the cross-user IDOR contract lives there), and it does not belong bundled with runtime plumbing.What lands here is the plumbing the ingress will attach to, complete and green.
Ported onto restructured main
attested_signingthreads stores →RebornRuntime→ accessor. Main flattenedRebornServicesout ofRebornRuntime, so it is a field rather than a nested lookup. Production wiresNone: with no durable attested backend, the gate ingress has nothing to dispatch to and refuses, rather than half-resolving a signature.production_turn_state_storetakes an optionalAttestedResumePort. Without a port the store never admits an attested resume — the fail-closed default, not a silent no-op.factory.rswas not merged. Git aligned its conflicts against unrelated regions — one 1,390-line hunk paired flow-record wiring against test-support attachment ports — because main restructured that file wholesale. The actual change is 73 insertions, so it was applied by hand at the correct anchors.Two ratchets, both genuine
ironclaw_attested_runtimedeclared no layer. Nowkernel: it depends onironclaw_turns, so it sits above the substrates it drives and below the composition that wires it.LocalDevContinuationDriver/LocalDevCustodialSignertripped theLocalDevtypename ratchet, which bans mode-shaped type names because a deployment mode is aDeploymentConfigvalue. RenamedInMemoryContinuationDriver/InMemoryCustodialSigner— named for the stores they pin, which is also the accurate distinction: a durable composition differs by its stores, not by which environment runs it.Deliberately not included
build_attested_composition. Main moved local-dev composition out of this crate, so nothing here would call it, and dead code in a signing path is worse than its absence. It lands with the group that wires the durable backends.Verification
ironclaw_architecture100% (both ratchets green),ironclaw_attested_runtimesuite green, whole workspace compiles including tests,fmt,clippy --tests(zero warnings),check_no_panics.py,check-hermetic-env.sh— all against currentmain. The composition pub-use snapshot is regenerated for the two new attested exports.Supersedes
#3994 (reborn runtime). #3995's ingress content moves to the follow-up PR described above.