Skip to content

feat(signing): injected wallet provider + gate/resolve wiring (attested-signing PR7/10) - #3974

Closed
zmanian wants to merge 13 commits into
nearai:attested-signing-05-turns-resumefrom
zmanian:attested-signing-07-injected-provider
Closed

zmanian wants to merge 13 commits into
nearai:attested-signing-05-turns-resumefrom
zmanian:attested-signing-07-injected-provider

Conversation

@zmanian

@zmanian zmanian commented May 24, 2026 •

Copy link
Copy Markdown
Collaborator

Rebased onto current main (attested-signing cascade, 2026-07-23). The stack was 1184 commits behind (merge base 2026-05-24). Tracking issue: #6532.

Inline review threads may have re-anchored or orphaned from the force-push. Reviewers: the "Cascade changes" section states exactly what the rebase altered beyond the original work.

Reopened after the cascade. This PR was closed on 2026-07-20 for modifying the legacy top-level src/ web ingress. src/ has since been deleted entirely (b6da0272a, #6375), so the branch has been rebased with all src/ hunks excised and is now Reborn-only.

What this PR is

The browser injected-wallet signing provider (window.ethereum / window.solana) in ironclaw_wallet_external: the wallet attests to the bound ApprovedTxHash and verify_resume enforces hash binding → signer binding (EVM ecrecover / Solana ed25519) → one-shot grant CAS, fail-closed in that order.

Cascade changes

  • src/ excised: dropped the legacy src/channels/web/features/chat/attested.rs proof ingress and its wiring (+739 lines / 10 files, 8 of which were 1–7 line touches). The Reborn-side replacement lands in the gate/resolve ingress PR (feat(signing): reborn webui attested gate/resolve ingress (attested-signing PR11/12) #3995). Crate-side work is untouched.
  • Adopted the round-2 GrantError variants: InvalidTimestamp/InvalidExpiry map fail-closed to GrantClaimFailed (construction-time rejects; the distinction is not safe to leak).

Verification

20 crate tests pass; boundary test green — including wallet_external_crate_has_no_chain_sdk_secrets_or_chain_signing_dependency (this crate holds no key material and must stay pure); clippy clean.

zmanian and others added 2 commits May 23, 2026 17:33
…ng PR1/10)

Introduce ironclaw_signing_provider, the pure, provider-agnostic
SigningProvider trait crate at the base of the 10-PR attested-signing
substrate stack. It pins the binding model every downstream crate depends
on and carries zero chain/crypto/secrets dependencies (only async-trait,
serde, thiserror).

- ProviderId / TrustModel enums (wire-stable snake_case serde).
- SigningContext with strong identity newtypes (local for PR1; reconciled
  against ironclaw_turns vocabulary in PR5 to avoid a dependency cycle).
- Opaque DecodedTransaction / RenderedTx handles + fixed 32-byte
  ApprovedTxHash (concretes land in ironclaw_attestation / PR2).
- SigningProof / VerifiedProof and SigningProviderError (thiserror).
- #[async_trait] SigningProvider trait (provider_id, trust_model,
  initiate, verify_resume), object-safe via Arc<dyn SigningProvider>.

Add an architecture dependency-boundary test asserting the trait crate's
manifest carries no solana/near/alloy/k256/sha3/webauthn-rs/secrets/
chain_signing/attestation dependency, across all dependency kinds.

Includes the consolidated v3 plan doc with the security invariants, the
22-threat matrix, the dependency table, and the 10-PR stack.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…d-signing PR2/10)

Adds the `ironclaw_attestation` crate: the value-binding core of the
attested-signing substrate. Depends only on `ironclaw_signing_provider`,
`serde`, `thiserror`, and `sha2` (already vendored at 0.10.9; no new
transitive cost). No chain SDK, no secrets, no webauthn — those land in
PR4/PR6.

Modules:
- decoded_tx.rs: chain-tagged, chain-SDK-free DecodedTransaction (Evm/Solana/
  Near) over plain serde types + RenderingSchemaVersion newtype.
- fields.rs: the single shared field projection both the renderer and the
  canonical encoder consume — structurally prevents "approve view A, sign
  bytes B".
- rendered.rs: render() derives the human view from that projection.
- canonical.rs: hand-rolled domain-separated, length-prefixed canonical
  signing bytes (no CBOR dependency, per conservative-deps rule).
- approved_tx_hash.rs: domain-separated SHA-256 over render ∥ canonical bytes
  ∥ signer/account ∥ chain/network ∥ tx-type ∥ schema-version.

Tests (TDD) prove the anti-field-smuggling property: changing ANY component
changes the hash; determinism across calls and serde round-trips; per-chain
render coverage; cross-chain domain separation. Extends the architecture
boundary test to assert the crate carries no chain/secrets/webauthn dep.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions github-actions Bot added scope: channel/web Web gateway channel scope: dependencies Dependency updates size: XL 500+ changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels May 24, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ed3697e02

ℹ️ 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".

Comment thread src/channels/web/features/chat/mod.rs Outdated
… full Solana/NEAR message modeling (attested-signing PR2)

Addresses four critical findings in the ironclaw_attestation canonical /
ApprovedTxHash core:

1. Wrong signer bound: removed DecodedTransaction::signer_account() (a
   heuristic returning the EVM recipient / account[0]). The approved hash
   now binds the explicit, trusted signer from SigningContext via the new
   safe public API approved_tx_hash_for(tx, signer_account, schema). The
   low-level component hasher is crate-private (test-internals feature only)
   so render and canonical inputs can't be mismatched.

2. deny_unknown_fields: added #[serde(deny_unknown_fields)] to
   DecodedTransaction and every nested decoded struct (EVM tx + access-list,
   Solana header/instruction/lookup/tx, NEAR public-key/access-key/action/tx).

3. Solana non-injective projection: replaced the lossy resolved-pubkey model
   with the full versioned-message model — version (legacy/v0), header
   (3 counts), static account keys, recent blockhash, compiled instructions
   (program/account indices), and address-table lookups. canonical bytes now
   embed the EXACT signed message via hand-rolled shortvec encoding.

4. NEAR incomplete projection: added the transaction public_key and a typed
   NearAction enum covering all actions (CreateAccount, DeployContract,
   FunctionCall, Transfer, Stake, AddKey w/ permission+allowance+receiver+
   methods, DeleteKey, DeleteAccount, Delegate). canonical bytes embed the
   exact borsh-signed transaction.

Full message modeling is hand-rolled (wire.rs) — no chain SDK pulled; the
architecture boundary test is unchanged (publish=false added, no new deps).
Strengthened binding.rs with wrong-signer, extra-field-rejection (per nested
struct), same-render/different-bytes, Solana version/ALT distinctness, NEAR
per-action distinctness, and a real field-count-equality check (fixing the
previously mislabeled non-empty-only assertion).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
zmanian and others added 4 commits May 25, 2026 16:24
Widen canonical/approved-tx length prefixes and field counts from u32 to
u64 in the hand-rolled length-prefixed encoders (canonical.rs and
approved_tx_hash.rs). usize->u64 is infallible on all supported platforms,
removing any theoretical truncation/collision on the canonical signing
pre-image. Updated module docs and the binding-test parser to match.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ai#3960)

Addresses henrypark133 CHANGES_REQUESTED review on nearai#3960.

High (forgery): VerifiedProof derived Deserialize and its public new()
took only the proof, so any caller — or any untrusted wire payload — could
mint a "verified" trust token without verification ever running. Drop
Deserialize (locked in by a compile_fail doctest) so it can never be
rehydrated off the wire, and bind provider_id + approved_tx_hash through
new() so a verified proof cannot be re-pointed at a different provider or
transaction. Construction stays pub because the real verifiers live in the
downstream provider/attestation crates (PR4/PR7-9); the seam is now an
un-deserializable token only a verifier can mint.

Tests: extend payload_accessor coverage to all four SigningProof variants
(WalletConnect + NearRedirect were missing) and assert the serialize-binding
/ non-deserializable invariant.

Conventions: add publish = false to Cargo.toml (matches 55/59 workspace crates).

Purity invariant preserved: no new dependencies; the architecture
dependency-boundary test stays green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…es (nearai#3961 follow-up)

Address henrypark133 review on nearai#3961 (merged): the hand-rolled Solana/NEAR
wire encoders in ironclaw_attestation silently truncated length prefixes,
which breaks the what-you-see-is-what-you-sign (WYSIWYS) binding by letting
an attacker desync a length from its payload.

Critical (compact-u16): push_compact_bytes cast bytes.len() as u16 behind a
debug_assert (compiled out in release). A slice > u16::MAX truncated the
shortvec prefix while appending the full slice, so trailing bytes could be
reinterpreted as extra instructions/accounts (field/instruction smuggling).
Now every Solana shortvec length goes through checked_short_vec_len and
returns AttestationError::SolanaShortVecOverflow instead of truncating.

High (NEAR borsh u32): push_borsh_bytes cast len as u32, truncating on 64-bit
platforms for slices > u32::MAX. Now routed through borsh_len_le, returning
AttestationError::NearBorshLengthOverflow.

High (NEAR u128): u128_from_be_minimal used wrapping shifts, silently
discarding high bytes for inputs > 16 bytes — the rendered (full) value would
diverge from the signed (wrapped) value. Now rejects >16-byte inputs with
AttestationError::NearU128Overflow; <=16 bytes (incl. leading-zero padding)
still parse.

High (NEAR Delegate): the Delegate action omitted the NEP-366 Vec<Action>
field, so a deserializer would read nonce as the actions-vector length. Now
serializes an explicit empty actions vector (borsh 0u32) in the correct
position between receiver_id and nonce.

The encoders, fields::project, render, canonical_signing_bytes, and
approved_tx_hash_for are now fallible (Result<_, AttestationError>). Solana
and NEAR wire bytes are also serialized ONCE and reused for both the human
value and the canonical commitment (was twice each), addressing the
duplicate-serialization findings.

Adds wire unit tests (overflow boundaries) and binding integration tests that
drive the safe public API for each rejection path plus the Delegate layout.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review (multi-agent)

Intent: Implement browser injected wallet provider as a SigningProvider backend and wire web /api/chat/gate/resolve ingress for attested signing
Stats: 12 findings (from 18 raw) across 5 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 4

Security (1)

  1. Medium No user-to-gate ownership check before injected-proof resolution (src/channels/web/features/chat/attested.rs:137-176, confidence 75) — anchor: src/channels/web/features/chat/attested.rs:137 (no diff position — body only)
    resolve_injected_wallet_proof receives user_id and gate_request_id but never verifies the authenticated user owns the gate. When PR10 wires the gate-store lookup, an attacker who knows another user's gate_request_id could submit a proof against that gate.
    Fix: In PR10, before calling verify_injected_proof, look up the persisted gate by gate_request_id scoped to user_id and reject with 403/404 if the gate does not belong to the requesting user.
    Also flagged by: maintainability/Medium
    Also flagged by: security/Low

Performance (2)

  1. Low Double JSON serialization: proof_from_input encodes then verify_resume decodes (src/channels/web/features/chat/attested.rs:69-107, confidence 80) — anchor: src/channels/web/features/chat/attested.rs:106 (no diff position — body only)
    proof_from_input builds an InjectedProofPayload, serializes it to JSON, then verify_resume immediately deserializes it back. This round-trip allocates a Vec and runs two JSON parse/serialize cycles.
    Fix: Pass the structured InjectedProofPayload directly through the call chain instead of serializing to opaque bytes and re-parsing.
    Also flagged by: pattern-refactor/Medium
    Also flagged by: maintainability/Low

  2. Medium std::sync::Mutex in async claim() can block tokio worker under contention (crates/ironclaw_attestation/src/grant.rs:204-222, confidence 70) — anchor: crates/ironclaw_attestation/src/grant.rs:181 (no diff position — body only)
    InMemorySealedGrantStore::claim uses std::sync::Mutex to guard the grant map. Under burst concurrent claims, this serializes all access unnecessarily.
    Fix: Replace std::sync::Mutex with tokio::sync::Mutex, or use dashmap::DashMap for lock-free concurrent access.

Tests (6)

  1. High Solana missing public_key in proof has no test (crates/ironclaw_wallet_external/src/injected/mod.rs:164-170, confidence 100) — anchor: crates/ironclaw_wallet_external/src/injected/mod.rs:164
    verify_resume returns ProofInvalid when scheme is Solana and public_key is None. All existing Solana tests include public_key: Some(...).
    Fix: tests::verify_resume::solana_missing_public_key_is_proof_invalid
    Also flagged by: performance/High

  2. Medium map_grant_error Backend variant mapping has no test (crates/ironclaw_wallet_external/src/injected/mod.rs:188-196, confidence 75) — anchor: crates/ironclaw_wallet_external/src/injected/mod.rs:195
    map_grant_error maps GrantError::Backend to SigningProviderError::Provider. No test exercises the Backend error path.
    Fix: tests::verify_resume::backend_grant_error_maps_to_provider_error

  3. Medium hex_bytes deserializer rejects odd-length hex strings with no test (crates/ironclaw_wallet_external/src/injected/mod.rs:228-246, confidence 75) — anchor: crates/ironclaw_wallet_external/src/injected/mod.rs:233
    hex_bytes::hex_decode rejects odd-length hex strings. No test exercises the odd-length hex rejection in the serde deserializer path.
    Fix: tests::verify_resume::decode_injected_proof_rejects_odd_length_hex
    Also flagged by: maintainability/Medium

  4. High EVM signature length validation (non-65-byte) has no test (crates/ironclaw_wallet_external/src/injected/evm.rs:28-32, confidence 100) — anchor: crates/ironclaw_wallet_external/src/injected/evm.rs:28
    verify_signer_over_hash returns ProofInvalid when signature.len() != 65, but all existing EVM tests pass valid 65-byte signatures.
    Fix: tests::verify_resume::evm_wrong_signature_length_is_proof_invalid

  5. Medium recovery_id_from_v invalid v-byte rejection has no test (crates/ironclaw_wallet_external/src/injected/evm.rs:76-89, confidence 75) — anchor: crates/ironclaw_wallet_external/src/injected/evm.rs:76
    recovery_id_from_v rejects v values outside {0,1,27,28,>=35}. No test exercises this invalid-v rejection path.
    Fix: tests::verify_resume::evm_invalid_recovery_v_byte_is_proof_invalid

  6. High Solana signature length validation (non-64-byte) has no test (crates/ironclaw_wallet_external/src/injected/solana.rs:23-27, confidence 100) — anchor: crates/ironclaw_wallet_external/src/injected/solana.rs:23
    verify_signer_over_hash returns ProofInvalid when signature.len() != 64, but all existing Solana tests pass valid 64-byte signatures.
    Fix: tests::verify_resume::solana_wrong_signature_length_is_proof_invalid

Local Patterns (2)

  1. Medium Duplicate hex_digit and hex-decode loop already exists in parent mod.rs hex_bytes module (crates/ironclaw_wallet_external/src/injected/evm.rs:105-136, confidence 75) — anchor: crates/ironclaw_wallet_external/src/injected/mod.rs:216
    evm.rs:127 defines a private hex_digit() byte-for-byte identical to solana.rs:88 and duplicates mod.rs:242. parse_evm_address reimplements the hex-to-bytes loop that hex_bytes::hex_decode already provides.
    Fix: Have parse_evm_address call super::hex_bytes::hex_decode from mod.rs, then .try_into() with .map_err().

  2. Medium Duplicate hex_digit and hex-decode loop already exists in parent mod.rs hex_bytes module (crates/ironclaw_wallet_external/src/injected/solana.rs:66-97, confidence 75) — anchor: crates/ironclaw_wallet_external/src/injected/mod.rs:216
    solana.rs:88 defines a private hex_digit() byte-for-byte identical to evm.rs:127 and duplicates mod.rs:242.
    Fix: Have parse_solana_pubkey call super::hex_bytes::hex_decode from mod.rs.

Maintainability (1)

  1. Low verify_injected_proof is a thin wrapper only used in tests (src/channels/web/features/chat/attested.rs:119-131, confidence 50) — anchor: src/channels/web/features/chat/attested.rs:119 (no diff position — body only)
    verify_injected_proof creates an InjectedSigningProvider and calls verify_resume, discarding the result.
    Fix: Remove verify_injected_proof. PR10 callers should construct InjectedSigningProvider directly.

Comment thread crates/ironclaw_wallet_external/src/injected/mod.rs
Comment thread crates/ironclaw_wallet_external/src/injected/evm.rs
Comment thread crates/ironclaw_wallet_external/src/injected/solana.rs
Comment thread crates/ironclaw_wallet_external/src/injected/mod.rs
Comment thread crates/ironclaw_wallet_external/src/injected/mod.rs
Comment thread crates/ironclaw_wallet_external/src/injected/evm.rs
Comment thread crates/ironclaw_wallet_external/src/injected/evm.rs
Comment thread crates/ironclaw_wallet_external/src/injected/solana.rs
zmanian added a commit that referenced this pull request May 26, 2026
Add the six fail-closed rejection tests henrypark133 flagged as missing on
PR #3974, all driving the full verify_resume path behind the SigningProvider
trait:

- solana_missing_public_key_is_proof_invalid (PRESERVE: proof must carry the
  signer public key; guards the Solana None branch)
- evm_wrong_signature_length_is_proof_invalid (non-65-byte rejection)
- evm_invalid_recovery_v_byte_is_proof_invalid (v outside {0,1,27,28,>=35})
- solana_wrong_signature_length_is_proof_invalid (non-64-byte rejection)
- decode_injected_proof_rejects_odd_length_hex (serde hex_bytes deserializer)
- backend_grant_error_maps_to_provider_error (map_grant_error Backend arm ->
  SigningProviderError::Provider, via a BackendErrorGrantStore mock)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@zmanian

zmanian commented May 26, 2026

Copy link
Copy Markdown
Collaborator Author

Review response (henrypark133 CHANGES_REQUESTED)

Pushed 7fd6252c8 — adds the six missing fail-closed rejection tests; no production logic changed (PRESERVE invariants intact: proof carries/verifies the signer public key with missing rejected, ApprovedTxHash binding, openssl-free, deterministic resume). All 20 verify_resume tests pass; cargo fmt --check and cargo clippy -p ironclaw_wallet_external --all-features clean (zero warnings).

Tests (6) — all FIXED in 7fd6252

Finding Test added
Solana missing public_key (High) solana_missing_public_key_is_proof_invalid
EVM non-65-byte sig (High) evm_wrong_signature_length_is_proof_invalid
Solana non-64-byte sig (High) solana_wrong_signature_length_is_proof_invalid
map_grant_error Backend arm (Med) backend_grant_error_maps_to_provider_error (new BackendErrorGrantStore mock)
odd-length hex deserializer (Med) decode_injected_proof_rejects_odd_length_hex
invalid recovery v byte (Med) evm_invalid_recovery_v_byte_is_proof_invalid

Security (1) — Medium: no user-to-gate ownership check — DEFERRED BY DESIGN (not a PR7 regression)

In PR7 resolve_injected_wallet_proof fails closed at the attested_grant_store guard (returns 503) and never reaches proof verification or any gate lookup — there is no authoritative binding to verify against yet. The ownership check (look up the persisted gate scoped to user_id, reject with 403/404 on mismatch) belongs in PR10 where the gate-store lookup is wired; this is already called out in the module docs and the // PR10: handoff note. No state mutation is reachable on the current head, so there is nothing exploitable to fix here. Tracking for PR10.

Performance (2)

  1. Double JSON serialization (Low) — declining for this PR. SigningProof::InjectedProof(Vec<u8>) is an intentional opaque-bytes contract: the proof rides through the persisted gate as opaque bytes and is re-decoded at the verification boundary, which is where untrusted input must be re-validated regardless. Threading a structured InjectedProofPayload through SigningProof would leak a backend-specific type into the provider-agnostic proof enum. The round-trip is one small JSON encode/decode per resume (not a hot path). Can revisit in PR10 if profiling shows it matters.
  2. std::sync::Mutex in grant.rs::claim (Med) — out of PR7 diff scope (ironclaw_attestation, untouched here). The critical section is a single in-memory HashMap CAS with no .await held across the guard, so it cannot block a tokio worker on an await point; contention is bounded to map access. The trait contract requires the seal-check + mark-claimed to be one atomic critical section (the one-shot CAS), which the std Mutex satisfies cleanly. Not changing the attestation crate in this PR.

Maintainability (1) — Low: verify_injected_proof thin wrapper "only used in tests" — DECLINE

It is the documented PR10 entry point, intentionally marked #[cfg_attr(not(test), allow(dead_code))]. It is the reusable crypto-real core PR10 will call once the gate store supplies the authoritative binding; removing it now would just be re-added in PR10. Keeping it (with the explanatory doc + // PR10: note).

Local Patterns (2) — duplicate hex_digit — DECLINE (low value)

Replied inline: the per-scheme helpers return SigningProviderError with scheme-specific reasons while mod.rs::hex_bytes returns Result<_, String> for the serde path; collapsing them forces an error re-wrap or leaks serde-shaped strings into the provider error taxonomy for ~10 lines of pure hex decode with no security logic.

zmanian and others added 2 commits May 25, 2026 23:11
…-signing PR3)

Squashed for stack integration (feat + PR3 review fixes: poisoned-lock Backend
tests, concurrent one-shot/transition contract cases, contract-tests feature).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…attested-signing PR5)

Squashed for stack integration (feat + PR5 review fixes: lock-free verify split,
deterministic resume, cancel-transition coverage).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@zmanian
zmanian force-pushed the attested-signing-05-turns-resume branch from 1f49ceb to 449c401 Compare May 26, 2026 06:17
@zmanian
zmanian requested a review from henrypark133 May 26, 2026 14:26
Comment thread crates/ironclaw_wallet_external/src/injected/mod.rs
Comment thread crates/ironclaw_attestation/src/grant.rs
Comment thread src/channels/web/features/chat/attested.rs
Comment thread crates/ironclaw_wallet_external/src/injected/evm.rs
@henrypark133

Copy link
Copy Markdown
Collaborator

Architectural note: expiry enforcement contract for durable backends

The sealed_grant_store_contract_cases! macro is the canonical compliance suite that every backend (in-memory today, durable PG/libSQL in follow-up PRs) must pass. The contract currently has no case for expiry-based claim rejection. Since expiry_ms is already in the wire shape and marked as stable, the enforcement semantics need to be locked into the contract macro before the durable backend arrives — otherwise the durable backend will silently diverge from the in-memory one on expired grants. The recommended contract addition is: sealed_grant_store_expired_grant_is_not_claimable — seal a grant with expiry_ms = 1 (past), attempt claim, assert Err(GrantError::NotFound) or a new Err(GrantError::Expired). If you add GrantError::Expired, update map_grant_error in ironclaw_wallet_external to collapse it to GrantClaimFailed (fail-closed, not leaked to callers). This is not a PR-blocking finding since the durable backend is explicitly out-of-scope here, but the contract gap should be closed in the same PR that introduces the first durable backend.

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of #3974: feat(signing): injected wallet provider + gate/resolve wiring (attested-signing PR7/10)

Verdict: Approved with Medium findings (no Critical/High)

The security core is sound. Three-check fail-closed order (hash binding → signer binding → one-shot CAS) is correctly implemented and tested. The injected-proof arm in the gate/resolve handler is properly isolated — it cannot trigger a mission resume (enforced via mission_outcome_for_resolution returning None for InjectedWalletProof + a dedicated regression test), fails closed 503 without composition, and rejects malformed proofs at the wire boundary with 400 before touching any crypto. Boundary test in attested_signing_boundaries.rs enforces that ironclaw_wallet_external carries no key-custody or chain-signing dependencies.

Findings posted:

# Severity Location Summary
1 Medium ironclaw_wallet_external/src/injected/mod.rs:74 encode_injected_proof uses .unwrap_or_default() — violates no-unwrap rule; silent empty proof though safe
2 Medium ironclaw_attestation/src/grant.rs:100 expiry_ms is stored but not enforced at claim time; contract gap must be closed before durable backend
3 Medium src/channels/web/features/chat/attested.rs:139 _user_id gate ownership check deferred to PR10 without a PR10 security note; cross-user gate claim risk if omitted
4 Low ironclaw_wallet_external/src/injected/evm.rs:76 v=27/28 and v≥35 are accepted but never tested as valid success paths; large EIP-155 chain IDs silently truncate in u8
A Arch issue comment sealed_grant_store_contract_cases! has no expiry case; must be added before durable backend arrives

Positive findings: EIP-191 personal-sign digest is correct; k256 ecrecover + ed25519-dalek paths both have wrong-signer, tampered-hash, replay, and malformed-input unit tests; hex decoding works over &[u8] bytes throughout (panic-free for multi-byte UTF-8 inputs); InjectedWalletProof variant is correctly excluded from the mission-resume classifier; !unsafe_code is set in lib.rs; architecture boundary test enforces the no-secrets/no-chain-signing dep constraint.

zmanian and others added 4 commits May 27, 2026 06:29
…ed-signing PR7/10)

PR7 of the attested-signing substrate: the browser injected-provider
SigningProvider backend plus the web /api/chat/gate/resolve ingress that
accepts an injected-wallet proof and runs verification.

A. crates/ironclaw_wallet_external — InjectedSigningProvider
  - provider_id() = Injected, trust_model() = ExternalWallet; holds no keys.
  - initiate() returns ReadyForProof: the wallet renders + signs natively
    (true wallet-side WYSIWYS); no server-issued directive.
  - verify_resume() is the fail-closed security core, in order:
      1. hash binding (threat nearai#3): proof's approved-tx hash must equal the
         bound ApprovedTxHash;
      2. signer binding (threat nearai#5): EVM signer recovered via k256 ecrecover
         over the EIP-191 personal-sign digest / Solana ed25519 (vendored
         ed25519-dalek) verified against the connected key, required to equal
         the bound account else SignerMismatch;
      3. one-shot grant (threat #1): claim the sealed AttestedSigningGrant via
         the atomic CAS; replay / missing / lost-CAS collapse to
         GrantClaimFailed.
    Only on all three passing does it return VerifiedProof.
  - Deps limited to k256 / ed25519-dalek / sha3 / sha2 / signing_provider /
    attestation. No solana-sdk / near-primitives / secrets / chain_signing.

B. Web wiring — /api/chat/gate/resolve injected-proof arm
  - New GateResolutionPayload::InjectedWalletProof variant on the existing v2
    request_id gate-resolve handler (single dispatch path; legacy no-request_id
    pending_auth path untouched).
  - src/channels/web/features/chat/attested.rs deserializes the wire payload
    into SigningProof::InjectedProof and exposes verify_injected_proof, the
    crypto-real reusable core driving InjectedSigningProvider::verify_resume.
  - The handler fails closed (503) until the PR10 composition layer wires the
    sealed-grant store (state.attested_grant_store) and persists the
    authoritative BlockedAttested binding; malformed proofs reject 400 at the
    wire boundary regardless.

PR10 boundary: broadcasting the wallet-signed tx (ironclaw_chain_signing) and
building ResumeTurnRequest { attestation: Some(..) } through the existing
AttestedResumePort gate-resolve path are deferred and marked with // PR10:
notes; this PR stops at the verified-proof boundary.

Updates the architecture dependency-boundary test to assert
ironclaw_wallet_external carries no chain SDK / secrets / chain-signing
dependency, keeping the lower-crate assertions intact. Object-safety covered
via Arc<dyn SigningProvider>.

Stacks on PR5 (attested-signing-05-turns-resume).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s (attested-signing PR7)

cargo-deny's wildcards="deny" with allow-wildcard-paths=true does not
exempt path-dep wildcards on publishable crates (crates.io disallows path
deps), so the new ironclaw_attestation and ironclaw_wallet_external crates
tripped error[wildcard]. Both are internal workspace crates, so mark them
publish=false (matching ironclaw_turns). This clears cargo-deny, and since
the aggregate "Code Style (fmt + clippy)" gate depends on deny-check, it
clears that gate too. No separate fmt/clippy violation existed.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…sume + panic-free hex parsing (attested-signing PR7)

Finding 1 (fail-open mission resume): in chat_gate_resolve_handler the
mission auto-resume hook ran unconditionally after the resolution match,
so a malformed/unverified/PR7-disabled InjectedWalletProof that returned
Err(400/503) could still transition a paused mission Paused -> Active.
Injected proofs no longer pre-classify as Approved (extracted
mission_outcome_for_resolution returns None for them), and the resume
hook now fires only on a successful (Ok) resolution. The injected-proof
failure path is fully inert w.r.t. mission/gate state.

Finding 2 (panic on attacker hex): the four hex parsers sliced &str by
byte offsets, so a valid JSON string with even-byte-length non-ASCII
panicked (500/info leak). All four now decode over &[u8] with a
panic-free hex_digit helper that rejects non-ASCII/non-hex cleanly as
ProofInvalid/400.

Tests: paused-mission + classifier regressions on /api/chat/gate/resolve;
Unicode-input regressions for signature, approved_tx_hash, public_key,
and the bound account at the caller level.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add the six fail-closed rejection tests henrypark133 flagged as missing on
PR nearai#3974, all driving the full verify_resume path behind the SigningProvider
trait:

- solana_missing_public_key_is_proof_invalid (PRESERVE: proof must carry the
  signer public key; guards the Solana None branch)
- evm_wrong_signature_length_is_proof_invalid (non-65-byte rejection)
- evm_invalid_recovery_v_byte_is_proof_invalid (v outside {0,1,27,28,>=35})
- solana_wrong_signature_length_is_proof_invalid (non-64-byte rejection)
- decode_injected_proof_rejects_odd_length_hex (serde hex_bytes deserializer)
- backend_grant_error_maps_to_provider_error (map_grant_error Backend arm ->
  SigningProviderError::Provider, via a BackendErrorGrantStore mock)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copy link
Copy Markdown
Member

Closing this because it updates the legacy top-level src/ web ingress/types implementation (src/channels/web/*, plus legacy integration tests). Product work has moved to the Reborn workspace under crates/, so this PR is outdated as an implementation path.

I opened #6318 to track attested injected-wallet and NEAR-redirect provider support as Reborn-native work with implementation notes.

@zmanian

zmanian commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #6755, part of consolidating the 20-PR attested-signing-* stack into 8 PRs against current main.

The content of this PR is carried forward in full, re-based on current main rather than on the old stack. That re-basing also fixed the CI failures that had been red here — including the missing [package.metadata.ironclaw] layer declarations, which were blocking the bottom of the stack and therefore every PR above it.

Closing to keep the queue honest. The review history stays on this PR and remains readable; reopen if the consolidation is rejected.

@zmanian zmanian closed this Jul 28, 2026
zmanian added a commit that referenced this pull request Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: medium Business logic, config, or moderate-risk modules scope: channel/web Web gateway channel scope: dependencies Dependency updates scope: docs Documentation size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants