Conversation
github-actions
Bot
added
scope: dependencies
![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 implements the crypto-free attested-signing continuation port for the WebUI facade, enabling the resolution of 'BlockedAttested' gates through external-wallet or custodial proofs. The changes introduce new DTOs, traits, and logic to handle the transition of turns to 'AttestedResolved' and drive the subsequent sign-and-broadcast continuation. Review feedback identifies a critical reliability issue where the turn state is committed before heavyweight verification, potentially leaving turns in a stuck state if the continuation fails. Additionally, a security concern was raised regarding unsafe string slicing in the hex parsing logic which could lead to panics on non-ASCII input; a more robust validation check and idiomatic implementation were suggested.
| let resume = self | ||
| .turn_coordinator | ||
| .resume_turn(ResumeTurnRequest { | ||
| scope: scope.clone(), | ||
| actor, | ||
| run_id, | ||
| gate_resolution_ref: gate_ref.clone(), | ||
| source_binding_ref: bounded_ref::<SourceBindingRef>("webui-gate-src", &binding_id)?, | ||
| reply_target_binding_ref: bounded_ref::<ReplyTargetBindingRef>( | ||
| "webui-gate-reply", | ||
| &binding_id, | ||
| )?, | ||
| idempotency_key: client_action_id, | ||
| attestation: Some(attestation), | ||
| }) | ||
| .await | ||
| .map_err(map_turn_error)?; | ||
|
|
||
| // The turn is `AttestedResolved`. Drive the deterministic sign + | ||
| // broadcast continuation. A continuation failure does NOT roll the turn | ||
| // back (the resume guard already consumed the one-shot); it surfaces as | ||
| // a sanitized error so the client can observe the failure category. | ||
| continuation | ||
| .continue_resolved_gate(&scope, run_id, &gate_ref, &claim) | ||
| .await | ||
| .map_err(map_attested_continuation_rejection)?; | ||
|
|
||
| Ok(RebornResolveGateResponse::Resumed(resume.into())) |
There was a problem hiding this comment.
The state transition in resume_turn occurs before the heavyweight verification in continue_resolved_gate. Since resume_turn consumes a one-shot guard (as noted in the comments), any failure in the subsequent continuation—such as ProofRejected due to an invalid signature or a LedgerGuard conflict—will leave the turn stuck in the AttestedResolved state without the intended side effects (signing and broadcasting) having occurred. This prevents the user from retrying the resolution with a corrected proof. Consider performing verification before or atomically with the state transition, or ensuring the turn state remains resolvable if the continuation fails.
References
- Operations that modify shared state (like turn transitions) should be atomic with their validation to prevent inconsistent states where a transition is committed but the associated action fails.
There was a problem hiding this comment.
Not actionable — this is already the design after commit 19e35a9 ("atomic verify-before-resume"). The FULL cryptographic verification AND the one-shot grant claim run in continuation.verify_and_claim(...) BEFORE resume_turn. resume_turn (which performs the BlockedAttested -> AttestedResolved transition and consumes the one-shot resume guard) is only reached after verification succeeds, so a ProofRejected/LedgerGuard failure leaves the turn in BlockedAttested with no state-machine transition, and the user can retry with a corrected proof. Line 327 is now just the response construction. Only broadcast_resolved runs after the transition, and by design a broadcast failure does not roll back (the one-shot is already consumed) — it surfaces as a sanitized error category; rolling back there would re-open a signed/possibly-broadcast gate.
| fn parse_hex(s: &str) -> Result<Vec<u8>, AttestedContinuationRejection> { | ||
| let s = s.strip_prefix("0x").unwrap_or(s); | ||
| if !s.len().is_multiple_of(2) { | ||
| return Err(AttestedContinuationRejection::MalformedProof); | ||
| } | ||
| (0..s.len()) | ||
| .step_by(2) | ||
| .map(|i| { | ||
| u8::from_str_radix(&s[i..i + 2], 16) | ||
| .map_err(|_| AttestedContinuationRejection::MalformedProof) | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
The parse_hex function uses byte-index slicing (s[i..i + 2]) on a &str. If the input string contains multi-byte UTF-8 characters, this will panic if a slice boundary falls inside a character, creating a potential Denial of Service (DoS) vector. To comply with repository safety rules, perform cheaper validation checks (like is_ascii and length) on the slice before performing more expensive operations or allocations. Additionally, while this function returns a Vec, be aware that for WASM performance, we should generally avoid unnecessary heap allocations and prefer iterators where possible. Using alloy_primitives::hex::decode would also be more idiomatic and efficient.
| fn parse_hex(s: &str) -> Result<Vec<u8>, AttestedContinuationRejection> { | |
| let s = s.strip_prefix("0x").unwrap_or(s); | |
| if !s.len().is_multiple_of(2) { | |
| return Err(AttestedContinuationRejection::MalformedProof); | |
| } | |
| (0..s.len()) | |
| .step_by(2) | |
| .map(|i| { | |
| u8::from_str_radix(&s[i..i + 2], 16) | |
| .map_err(|_| AttestedContinuationRejection::MalformedProof) | |
| }) | |
| .collect() | |
| } | |
| fn parse_hex(s: &str) -> Result<Vec<u8>, AttestedContinuationRejection> { | |
| let s = s.strip_prefix("0x").unwrap_or(s); | |
| if !s.is_ascii() || !s.len().is_multiple_of(2) { | |
| return Err(AttestedContinuationRejection::MalformedProof); | |
| } | |
| (0..s.len()) | |
| .step_by(2) | |
| .map(|i| { | |
| u8::from_str_radix(&s[i..i + 2], 16) | |
| .map_err(|_| AttestedContinuationRejection::MalformedProof) | |
| }) | |
| .collect() | |
| } |
References
- Perform cheaper validation checks (like length and character set) on a trimmed slice before performing more expensive operations or allocations.
- When slicing or truncating UTF-8 strings, use character-aware methods or validate boundaries (e.g., via is_ascii or is_char_boundary) to avoid panics on multi-byte characters.
- To improve performance in WASM, avoid unnecessary heap allocations by using iterators directly instead of collecting them into a Vec.
There was a problem hiding this comment.
Not actionable — the DoS premise is stale. The current parse_hex (attested_continuation.rs) no longer does &s[i..i+2] &str slicing; it strips the optional 0x, takes s.as_bytes(), checks even length, and decodes via chunks_exact(2) + a hex_nibble matcher, so non-ASCII / odd-length input fails closed as MalformedProof rather than panicking. That is functionally the iterator-over-validated-ASCII form the suggestion asks for. We can't switch to alloy_primitives::hex::decode here: this crate doesn't depend on alloy and we won't pull it in for a hash decode.
![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: 754c7674ad
ℹ️ 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 attestation = AttestationClaimRef::new(claim.approved_tx_hash_hex.clone()) | ||
| .map_err(|_| attested_invalid_field("attested_approved_tx_hash"))?; |
There was a problem hiding this comment.
The attested inbound parser explicitly allows attested_approved_tx_hash with an optional 0x prefix, but this value is passed unchanged into AttestationClaimRef and then compared by the resume port against the authoritative bound hash in canonical lowercase hex without 0x. As a result, valid requests that include the documented prefix are rejected as attestation binding mismatches, so WebUI clients using prefixed hashes cannot resolve attested gates even when the proof is otherwise correct.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 14ec604. The inbound parser now canonicalizes attested_approved_tx_hash (strips the optional 0x prefix, requires exactly 64 ASCII-hex digits, lowercases) before it becomes the AttestationClaimRef, so a documented 0x-prefixed (or uppercase) hash now matches the resume port's byte-exact comparison against the canonical bound hash instead of failing as a binding mismatch. Added parser tests for normalization and wrong-length rejection.
|
Merge-order coordination note (overlap with #4029 / #4031) This PR's webui gate/resolve ingress overlaps two in-flight reborn PRs:
Decided order: #4029/#4031 land first; this stack rebases onto reborn-integration afterward. On that rebase, fold the attested gate family into #4029's single |
Canonicalize the attested approved-tx-hash hex (strip optional 0x prefix, require exactly 64 ASCII-hex digits, lowercase) at the WebUI inbound parser so a documented 0x-prefixed or uppercase hash matches the resume port's byte-exact comparison against the canonical bound hash, instead of being rejected as a binding mismatch. Adds parser tests for normalization and wrong-length rejection. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Wire attested-signing gate lifecycle into reborn WebUI ingress with crypto-free facade and continuation port
Stats: 12 findings (from 18 raw) across 8 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 3
Security (2)
-
High Custodial path signs caller-supplied tx without verifying it matches approved decoded binding (
crates/ironclaw_attested_runtime/src/driver.rs:410-440, confidence 75) — anchor:crates/ironclaw_attested_runtime/src/driver.rs:410
The new sign_custodial accepts a caller-supplied EvmTx and passes it to CustodialSigner::sign_evm. There is no check that the supplied tx matches req.decoded.
Fix:Either rebuild the signable from binding.decoded in the driver, or add a check that the supplied tx matches req.decoded.
Also flagged by: security/Medium -
Low Zeroize of hex-encoded private key removed from KeyStore::bind (
crates/ironclaw_chain_signing/src/keystore.rs:244-256, confidence 50) — anchor:crates/ironclaw_chain_signing/src/keystore.rs:244(no diff position — body only)
The old code called hex_key.zeroize() after encrypting. This was removed.
Fix:Restore the zeroize call.
Bugs (2)
-
High Broadcast failure mapped to 400 ProofRejected instead of server error (
crates/ironclaw_reborn_composition/src/attested_continuation.rs:315-317, confidence 100) — anchor:crates/ironclaw_reborn_composition/src/attested_continuation.rs:315
ContinuationError::Broadcast is mapped to AttestedContinuationRejection::ProofRejected (400). A broadcast failure is a post-verification infrastructure failure.
Fix:Map ContinuationError::Broadcast to AttestedContinuationRejection::Unavailable (503).
Also flagged by: tests/Medium -
Medium InMemoryAttestedGateBindingStore::put silently drops binding on lock poisoning (
crates/ironclaw_attested_runtime/src/binding.rs:87-90, confidence 75) — anchor:crates/ironclaw_attested_runtime/src/binding.rs:88(no diff position — body only)
if let Ok(mut map) = self.bindings.lock() silently ignores a poisoned mutex.
Fix:Fail closed on lock poisoning.
Also flagged by: performance/Medium
Performance (1)
- High register_attested_gate has check-then-act TOCTOU on bindings with silent overwrite (
crates/ironclaw_reborn_composition/src/attested.rs:175-206, confidence 85) — anchor:crates/ironclaw_reborn_composition/src/attested.rs:175
register_attested_gate does bindings.get then bindings.put. Between the get and put, a concurrent caller can pass the same get check.
Fix:Add a try_put_if_absent method to AttestedGateBindingStore trait.
Also flagged by: security/Medium
Also flagged by: tests/Medium
Also flagged by: security/Medium
Tests (3)
-
Medium NearRedirect and WalletConnect proof decode paths have no test coverage (
crates/ironclaw_reborn_composition/src/attested_continuation.rs:161-198, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/attested_continuation.rs:161
The e2e test only exercises InjectedWallet proofs.
Fix:tests::attested_continuation::decode_near_redirect_and_walletconnect_proofs -
Medium attested_proof non-object JSON values not tested (
crates/ironclaw_product_workflow/src/webui_inbound.rs:340-346, confidence 75) — anchor:crates/ironclaw_product_workflow/src/webui_inbound.rs:341
parse_attested_resolution rejects non-object attested_proof but tests don't cover array/string/null.
Fix:tests::webui_inbound_contract::attested_resolution_rejects_non_object_proof -
Medium MissingBinding rejection not tested when no binding is registered (
crates/ironclaw_reborn_composition/tests/attested_gate_resolve_ingress.rs:1-712, confidence 75) — anchor:crates/ironclaw_reborn_composition/tests/attested_gate_resolve_ingress.rs:1
All e2e tests register a binding before resolving. The MissingBinding path is never exercised.
Fix:tests::attested_gate_resolve_ingress::resolve_gate_attested_with_no_binding_returns_missing
Local Patterns (2)
-
Low VerifiedContinuation field visibility inconsistent with SignerContinuationOutcome (
crates/ironclaw_attested_runtime/src/driver.rs:107-124, confidence 50) — anchor:crates/ironclaw_attested_runtime/src/driver.rs:78-86
SignerContinuationOutcome exposes all fields as pub while VerifiedContinuation makes all fields private.
Fix:Either make all fields pub or add getters for completeness. -
Medium normalize_approved_tx_hash_hex breaks the parse_ naming convention (
crates/ironclaw_product_workflow/src/webui_inbound.rs:520-529, confidence 75) — anchor:crates/ironclaw_product_workflow/src/webui_inbound.rs:378-407
All sibling functions use parse prefix but this one uses normalize_._
Fix:Rename to parse_approved_tx_hash_hex.
Maintainability (2)
-
Medium create_ledger_row conflates AlreadyExists with InvalidTransition (
crates/ironclaw_attested_runtime/src/driver.rs:308-322, confidence 75) — anchor:crates/ironclaw_attested_runtime/src/driver.rs:311
The helper maps LedgerError::AlreadyExists to LedgerError::InvalidTransition.
Fix:Propagate AlreadyExists as a distinct ContinuationError variant. -
Low factory.rs crosses 1000 lines due to attested-signing additions (
crates/ironclaw_reborn_composition/src/factory.rs:1-1012, confidence 100) — anchor:crates/ironclaw_reborn_composition/src/factory.rs:183(no diff position — body only)
This PR pushes factory.rs from 989 to 1012 lines.
Fix:Extract the attested-signing local-dev wiring into a private helper.
| //! proof through the bound provider. On any failure the turn is left | ||
| //! `BlockedAttested` (the facade never resumes). On success it returns an | ||
| //! opaque verified handle. | ||
| //! 2. [`AttestedGateContinuationPort::broadcast_resolved`] (AFTER `resume_turn` |
There was a problem hiding this comment.
High — Broadcast failure mapped to 400 ProofRejected instead of server error.
ContinuationError::Broadcast is mapped to AttestedContinuationRejection::ProofRejected (400). A broadcast failure is a post-verification infrastructure failure.
Fix: Map ContinuationError::Broadcast to AttestedContinuationRejection::Unavailable (503).
Also flagged by: tests/Medium: map_continuation_error ProviderMismatch path not exercised
There was a problem hiding this comment.
Fixed in a19b7c1. ContinuationError::Broadcast now maps to AttestedContinuationRejection::Unavailable (503), not ProofRejected (400). ChainSigning stays ProofRejected. Added broadcast_failure_maps_to_unavailable_not_proof_rejected and custodial_signing_failure_still_maps_to_proof_rejected.
| use ironclaw_signing_provider::SigningContext; | ||
| use ironclaw_signing_provider::{GateRef, SigningContext}; | ||
|
|
||
| /// Error from [`RebornAttestedComposition::register_attested_gate`]. Distinct |
There was a problem hiding this comment.
High — register_attested_gate has check-then-act TOCTOU on bindings with silent overwrite.
register_attested_gate does bindings.get then bindings.put. Between the get and put, a concurrent caller can pass the same get check.
Fix: Add a try_put_if_absent method to AttestedGateBindingStore trait.
Also flagged by: security/Medium: register_attested_gate does not validate approved-hash consistency of the bindin
Also flagged by: tests/Medium: RegisterAttestedGateError::Grant(other) error path not tested
Also flagged by: security/Medium: TOCTOU race in register_attested_gate: get-then-put is not atomic
There was a problem hiding this comment.
Fixed in a19b7c1. Added AttestedGateBindingStore::try_put_if_absent (existence check + insert under one lock, no TOCTOU window) and register_attested_gate now uses it. The grant seal CAS already serializes concurrent raises (it runs first); the atomic binding insert closes the silent-overwrite window and fails closed via a new BindingStoreError -> RegisterAttestedGateError::BindingStore.
| /// Signed`, | ||
| /// * the sealed grant was claimed exactly once (threat #1 — inside the | ||
| /// provider's `verify_resume` for the external-wallet path, or inside the | ||
| /// [`CustodialSigner`] for the custodial path), |
There was a problem hiding this comment.
High — Custodial path signs caller-supplied tx without verifying it matches approved decoded binding.
The new sign_custodial accepts a caller-supplied EvmTx and passes it to CustodialSigner::sign_evm. There is no check that the supplied tx matches req.decoded.
Fix: Either rebuild the signable from binding.decoded in the driver, or add a check that the supplied tx matches req.decoded.
Also flagged by: security/Medium: Binding chain-vs-decoded-tx mismatch check removed from driver
There was a problem hiding this comment.
Fixed in a19b7c1 — genuine approve-A/sign-B. The custodial path now re-derives the EXACT alloy signing digest (keccak256(rlp(unsigned))) from binding.decoded and refuses any caller tx whose bytes diverge. New ironclaw_chain_signing::evm::rebuild::assert_matches_binding is enforced in two places: in the driver's sign_custodial BEFORE any ledger row/grant claim (maps to ContinuationError::ApprovedHashMismatch), and at the custodial signer boundary BEFORE key consumption (ChainSigningError::ApprovedTxMismatch), ordered first so a divergent tx never burns the sealed grant. Rebuild round-trips through the existing decoders (round-trip test guards drift). New threat test threat_3_approve_a_sign_b_caller_tx_mismatch_rejected drives a different recipient+value through continue_after_resolved and asserts rejection.
| /// existing row for this `gate_ref` (a prior attempt) makes any re-entry fail | ||
| /// closed. Surfaced as an invalid-transition ledger error carrying the | ||
| /// existing state. | ||
| async fn create_ledger_row(&self, gate_ref: &GateRef) -> Result<(), ContinuationError> { |
There was a problem hiding this comment.
Medium — create_ledger_row conflates AlreadyExists with InvalidTransition.
The helper maps LedgerError::AlreadyExists to LedgerError::InvalidTransition.
Fix: Propagate AlreadyExists as a distinct ContinuationError variant.
There was a problem hiding this comment.
Fixed in a19b7c1. Added a distinct ContinuationError::LedgerRowExists { current } variant; create_ledger_row no longer fabricates an InvalidTransition with a synthetic to. It still maps to LedgerGuard (409) at the facade. Threat tests updated to accept either ledger-guard variant.
| }; | ||
| use ironclaw_turns::{GateRef, TurnRunId, TurnScope}; | ||
| use ironclaw_wallet_external::{ | ||
| InjectedProofPayload, InjectedScheme, NearAccessKeyScope, NearRedirectProofPayload, |
There was a problem hiding this comment.
Medium — NearRedirect and WalletConnect proof decode paths have no test coverage.
The e2e test only exercises InjectedWallet proofs.
Fix: tests::attested_continuation::decode_near_redirect_and_walletconnect_proofs
There was a problem hiding this comment.
Fixed in a19b7c1. Added decode_near_redirect_proof (FullAccess + FunctionCall scopes), decode_walletconnect_proof, decode_injected_wallet_proof, and decode_proof_rejects_malformed_payload to the composition crate's attested_continuation tests.
| /// `injected_wallet`, `near_redirect`, `wallet_connect`. Mirrors the legacy | ||
| /// monolith `GateResolutionPayload` proof variants. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub attested_proof_kind: Option<String>, |
There was a problem hiding this comment.
Medium — attested_proof non-object JSON values not tested.
parse_attested_resolution rejects non-object attested_proof but tests don't cover array/string/null.
Fix: tests::webui_inbound_contract::attested_resolution_rejects_non_object_proof
There was a problem hiding this comment.
Fixed in a19b7c1. Added attested_resolution_rejects_non_object_proof covering array/string/number/bool/null (null -> MissingField, the rest -> InvalidValue; all fail closed on the attested_proof field).
| // would be carried verbatim into the `AttestationClaimRef` and then fail the | ||
| // resume port's byte-exact comparison against the canonical bound hash — | ||
| // rejecting otherwise-valid proofs. | ||
| let approved_tx_hash_hex = normalize_approved_tx_hash_hex(&approved_tx_hash_raw)?; |
There was a problem hiding this comment.
Medium — normalize_approved_tx_hash_hex breaks the parse_ naming convention.
All sibling functions use parse_ prefix but this one uses normalize_.
Fix: Rename to parse_approved_tx_hash_hex.
There was a problem hiding this comment.
Fixed in a19b7c1. Renamed normalize_approved_tx_hash_hex -> parse_approved_tx_hash_hex to match the parse_ sibling convention.
| AgentId, InvocationId, ProjectId, ResourceScope, TenantId, ThreadId, UserId, | ||
| }; | ||
| use ironclaw_product_workflow::{ | ||
| AttestedContinuationOutcome, AttestedContinuationRejection, AttestedGateContinuationPort, |
There was a problem hiding this comment.
Medium — MissingBinding rejection not tested when no binding is registered.
All e2e tests register a binding before resolving. The MissingBinding path is never exercised.
Fix: tests::attested_gate_resolve_ingress::resolve_gate_attested_with_no_binding_returns_missing
There was a problem hiding this comment.
Fixed in a19b7c1. Added resolve_gate_attested_with_no_binding_fails_closed: blocks the attested gate but registers NO binding, then asserts the resolve fails closed and the continuation is never driven. (End-to-end the resume port rejects an unvalidatable claim before the driver's defense-in-depth MissingBinding is reached.)
| /// bytes never re-trigger verification or a second grant claim: the heavyweight | ||
| /// crypto runs exactly once, here. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct VerifiedContinuation { |
There was a problem hiding this comment.
Low — VerifiedContinuation field visibility inconsistent with SignerContinuationOutcome.
SignerContinuationOutcome exposes all fields as pub while VerifiedContinuation makes all fields private.
Fix: Either make all fields pub or add getters for completeness.
There was a problem hiding this comment.
Declining — this is intentional. VerifiedContinuation is a capability token whose signed/context fields must stay encapsulated (it is evidence consumed only by broadcast_signed_continuation); public getters already exist for the safe-to-read gate_ref/signer. SignerContinuationOutcome is a plain public DTO with a different role, so the visibility difference is by design. Exposing the signed bytes/context would weaken the token.
…xx mapping Addresses henrypark133 CHANGES_REQUESTED review on PR #3995. Security (High): WYSIWYS approve-A/sign-B. The custodial path signed the caller-supplied EvmTx after only checking (a) the binding's internal hash self-consistency and (b) that the signer key ecrecovers to the bound account — never that the tx bytes about to be signed match the approved binding. Add `ironclaw_chain_signing::evm::rebuild`, which re-derives the exact alloy signing digest (keccak256(rlp(unsigned))) from binding.decoded and rejects any divergent caller tx (ChainSigningError::ApprovedTxMismatch / ContinuationError::ApprovedHashMismatch). Enforced both in the driver (before any ledger row / grant claim) and at the custodial signer boundary (before key consumption, so a divergent tx never burns the sealed grant). Bug (High): broadcast failure mapped to 400 ProofRejected. A post-verification broadcast/RPC failure is server-side recoverable; remap ContinuationError::Broadcast -> AttestedContinuationRejection::Unavailable (503). Also: fail-closed binding store with atomic try_put_if_absent (closes the register_attested_gate TOCTOU + silent-overwrite); restore zeroize of the hex-encoded private key in KeyStore::bind; distinct LedgerRowExists error (was disguised as a synthetic InvalidTransition); rename normalize_approved_tx_hash_hex -> parse_approved_tx_hash_hex; extract local_dev_attested_turn_state helper from factory.rs. New tests cover the approve-A/sign-B attack, broadcast->503 mapping, Near/WalletConnect proof decode, non-object attested_proof, and the missing-binding resolve path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed henrypark133's review in a19b7c1 (force-pushed on top of the cargo-deny fix cb825ed). WYSIWYS finding (Security/High) was GENUINE and is now enforced. The custodial path previously signed the caller-supplied
Verification: |
2fd8c94 to
b5f0f6f
Compare
a19b7c1 to
502489d
Compare
| impl AttestedGateContinuationPort for RebornAttestedContinuation { | ||
| async fn verify_and_claim( | ||
| &self, | ||
| _scope: &TurnScope, |
There was a problem hiding this comment.
[High] Gate binding ownership not validated against calling user/tenant
The _scope and _run_id args on verify_and_claim are explicitly unused (underscore prefix). The driver's verify_and_sign fetches the binding by gate_ref alone — the binding's ResourceScope { tenant_id, user_id } and SigningContext { tenant, user } are never compared against the calling user's scope.
The thread-ownership probe (resolve_webui_thread_metadata) confirms the caller owns their own thread, not that they own the gate. For the custodial path (ProviderId::Custodial) the proof payload is ignored entirely; the driver reconstructs from the authoritative binding. A second tenant-member who learns gate_ref (returned in the gate-raise response) can call resolve_gate with their own thread_id to pass the ownership probe, then drive Alice's custodial signing continuation.
Fix: After the binding is loaded, assert binding.scope.tenant_id == scope.tenant_id and binding.context.user.as_str() == caller_user_id before calling driver.verify_and_sign. Add a cross-user IDOR test to attested_gate_resolve_ingress.rs.
There was a problem hiding this comment.
Fixed in cd41a5a. Activated the previously-unused caller identity: verify_and_claim now takes actor: &TurnActor, and the composition port calls a new AttestedSignerContinuationDriver::assert_binding_owner(gate_ref, BindingOwner { tenant_id, user_id }) BEFORE any decode / provider verify / custodial sign / grant claim. It compares the caller (tenant from scope, user from actor) against the binding's authoritative SigningContext { tenant, user }. On either a missing binding OR an owner divergence it returns the new ContinuationError::OwnerMismatch, which maps to MissingBinding -> 404 so a cross-user gate_ref is indistinguishable from a non-existent one (no existence oracle). Note: TurnScope carries only the tenant axis (no user_id), so the user identity is taken from TurnActor rather than scope as the suggested snippet implied. Added resolve_gate_attested_cross_user_fails_closed (Finding 4) driving RebornServices::resolve_gate: a same-tenant second user who owns their own thread (passing the ownership probe) cannot resolve user1's gate — it fails closed 404 with zero broadcasts, and the real owner can still resolve afterward.
PR #3995 — WebUI Attested Gate/Resolve Ingress — Security ReviewReviewed at: Summary Table
Finding 1 — High: Gate binding ownership not validated against calling user (IDOR)File:
let binding = self.bindings.get(gate_ref).await.ok_or(ContinuationError::MissingBinding)?;The binding carries The upstream Attack path: User B in the same tenant learns Alice's Fix: In if binding.scope.tenant_id.as_str() != scope.tenant_id.as_str()
|| binding.context.user.as_str() != scope /* owning_user_id */
{
return Err(AttestedContinuationRejection::ProofRejected);
}The Finding 2 — Medium:
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Requesting changes for High finding: gate binding ownership not validated against calling user in verify_and_claim (IDOR). See full review in issue comment. 1 High · 3 Medium · 1 Low · 2 Nit.
… proof ingress Addresses henrypark133's second-round CHANGES_REQUESTED review on PR #3995. Finding 1 (High, IDOR): verify_and_claim's scope/run_id were unused, so the driver fetched the authoritative binding by gate_ref alone and never checked the caller owns it. A second tenant member who learned another user's gate_ref could drive that user's signing continuation (most acutely the custodial path, which signs from the binding regardless of the proof). Fix: - New AttestedSignerContinuationDriver::assert_binding_owner + BindingOwner, comparing the caller (tenant_id from scope, user_id from actor) against the binding's authoritative SigningContext tenant/user BEFORE any decode / verify / custodial sign / grant claim. - New ContinuationError::OwnerMismatch, mapped to MissingBinding (404) so a cross-user gate_ref is indistinguishable from a non-existent one (no existence oracle). - Thread the caller actor into the AttestedGateContinuationPort::verify_and_claim contract and the composition impl. Finding 4 (Medium, required companion): added resolve_gate_attested_cross_user_fails_closed integration test driving the RebornServices::resolve_gate caller — a same-tenant second user who owns their own thread (passing the ownership probe) cannot resolve user1's gate; it fails closed 404 with no broadcast, and the real owner can still resolve afterward. Finding 2 (Medium, REPL/TUI): downgraded the background broadcast-failure recover_unknown warn! to debug! with a structured recoverable field, per CLAUDE.md (background tasks must not corrupt the interactive display). Finding 3 (Medium): bounded the untrusted proof_json blob to 16 KiB before any clone/parse work. Finding 5 (Low): bounded the WalletConnect session_topic to 256 chars. Finding 7 (Nit): added a PRODUCTION WARNING doc to NoopBroadcaster. Findings 6 and 8 (Low/Nit) accepted as deferred/test-only per the review. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed the second-round review at Finding 1 — High (IDOR), FIXED. Finding 4 — Medium (required companion), FIXED. Added Finding 2 — Medium (REPL/TUI), FIXED. Finding 3 — Medium (proof clone), FIXED. Finding 5 — Low (session_topic), FIXED. WalletConnect Finding 7 — Nit (NoopBroadcaster), ADDRESSED. Added an explicit Finding 6 — Low (flat attested_ fields), DECLINED (deferred).* Mirrors the legacy monolith wire contract as noted; tracked for the per-variant restructure as the surface stabilizes. Finding 8 — Nit (test helper), ACKNOWLEDGED. Preserved invariants: WYSIWYS sign-binding, broadcast-failure -> Unavailable/503, verify-before-advance/resume, sealed one-shot grant CAS, gate-bound signer, deterministic resume, KMS ship-gate, TenantId isolation, openssl-free. Verification (IRONCLAW_DISABLE_OS_KEYCHAIN=1): touched-crate tests pass — |
b5f0f6f to
106cbdf
Compare
106cbdf to
52dfc4a
Compare
cd41a5a to
fc27dd5
Compare
|
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 |
…r IDOR fix (#3995, ported)
fc27dd5 to
cf7053d
Compare
… (Phase C, slice 1) Lands §C1–§C3's decision logic ahead of any HTTP plumbing: who may look at an intent, who may submit a proof for it, and what a bare token gets you. **The governing rule, encoded:** the link token is an addressing convenience, never an authorization. Authorization to view is a session whose user equals the bound approver; to sign, the device ceremony; to advance the turn, the sealed grant CAS. The token adds only routeability, expiry, and unguessability. **Every refusal is the same refusal.** Unknown token, expired intent, already resolved, wrong user, wrong tenant — all collapse to `ReviewRejection::NotFound` for a uniform 404. `ReviewRejection` is deliberately single-variant so a distinguishable rejection cannot be added by accident; doing it would be a visible API change rather than a quiet leak. A 403 here would confirm to someone holding a forwarded link that the intent exists, whose it is, and whether it is still live — and transaction detail is exactly the reconnaissance worth denying. This is the #3995 cross-user IDOR lesson applied before the surface ships rather than after it leaked. The tenant axis is checked before the user axis, so a cross-tenant caller never reaches the user comparison and timing cannot separate the two failures. **GET is side-effect-free by construction**, not by discipline: every function takes `&IntentRecord` and returns a decision, so none of them *can* mutate state. Chat platforms fetch link previews with bot user-agents; a state-changing or one-shot-consuming GET would be burned by the preview fetch before the human ever clicked. The pre-session `resolve_token_landing` returns only an intent id — a test asserts the landing's rendering names neither the approver nor the tenant, so a preview bot learns nothing. The redirect target is composed server-side from that id; nothing request-derived contributes to it (no open-redirect class). `authorize_proof_submission` additionally requires a pending intent. That is for clear errors, not authority: the sealed-grant CAS refuses a replayed approval underneath regardless, and the doc says so, so nobody later mistakes this check for the one-shot guarantee. 8 tests, including the same-tenant-different-user IDOR case, the cross-tenant case, inclusive expiry at the boundary, dead-token non-redirect for all three terminal states, and a table asserting all four refusal paths are byte-identical. Still Phase C's to come: the HTTP route + SPA landing, and the pending -> approved/rejected projection driven by the resolve path that claims the grant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Superseded by #6769, part of consolidating the 20-PR Verified before closing: this branch is an ancestor of the old stack tip, so its content is carried forward in full. The consolidation also re-based everything onto current Closing to keep the queue honest. The review history stays on this PR and remains readable; reopen if the consolidation is rejected. |
2 participants
What
Wires the attested-signing gate lifecycle into the reborn WebUI ingress so attested signing works under reborn — not just the legacy monolith web channel. Today the attested proof ingress exists only in
src/channels/web/features/chat/attested.rs; under reborn there is no way to submit an attested proof. This PR closes that one real binary-coupling gap, building directly on PR10'sironclaw_attested_runtime.How it wires together
ironclaw_product_workflow(crypto-free facade)AttestedGateContinuationPorttrait + opaqueAttestedProofClaimDTO (strings/JSON only — no chain/crypto types).WebUiGateResolution::Attested { kind, approved_tx_hash_hex, proof_json }wire variant, mirroring the legacyGateResolutionPayloadinjected / NEAR-redirect / WalletConnect proof families.resolve_gateonAttested: buildsResumeTurnRequest { attestation: Some(claim) }(the claim is the proof's bound-hash hex), callsresume_turn— the injectedRuntimeAttestedResumePortruns the synchronous authoritative-binding re-check + one-shot resume guard and transitions the turn toAttestedResolved— then drives the deterministic sign + broadcast through the continuation port. Fails closed when no port is wired.ironclaw_reborn_composition(wiring layer)RebornAttestedContinuationimplements the port over PR10'sAttestedSignerContinuationDriver. It decodes the opaque proof JSON into the concreteSigningProof(hex wire contract mirroring the legacyproof_from_input/near_proof_from_input), then dispatchescontinue_after_resolved. No verification logic is duplicated — signer/hash binding, sealed-grant CAS, and ledger idempotency stay inironclaw_attested_runtime/providers.RebornAttestedComposition::register_attested_gateis the raise-side seam: persists the authoritativeAttestedGateBinding(gate_ref ∥ expectedApprovedTxHash∥ bound signer/account ∥ chain/tx-type) and seals the one-shot grant.ProviderMismatchuntil their ceremony config (state_secret/URLs, session-binding store) is wired.build_webui_serviceswires the continuation port into the facade.Invariants held
ironclaw_turnsstays crypto-free; the runtime stays outsidesrc/; no inverted deps on the binary.workspace_graph_is_openssl_freegreen)./api/chat/gate/resolvewith arequest_id; the legacy no-request_idv1 path is untouched.Tests
crates/ironclaw_reborn_composition/tests/attested_gate_resolve_ingress.rs) drives the callerRebornServices::resolve_gate: raise (persist binding + seal grant) → blockBlockedAttested→ POST attested injected-wallet proof → resume →AttestedResolved→ driver broadcast. Plus replay-fails-closed and no-continuation-port-fails-closed.webui_inbound_contract.rs).Verification
cargo fmt --all— cleancargo clippy --all --tests --examples --all-features -- -D warnings— exit 0cargo test -p ironclaw_reborn_webui_ingress -p ironclaw_reborn_composition -p ironclaw_attested_runtime -p ironclaw_architecture— greencargo tree -i openssl-sys(host +x86_64-unknown-linux-gnu) — emptyDeferred to PR12
🤖 Generated with Claude Code