feat(wallet): bind prepared transaction quotes to originating chat session#2708
Conversation
Adds private QuoteOwner type and a current_owner() helper that reads the APPROVAL_CHAT_CONTEXT task-local set by the web chat channel around the agent tool loop. No behavior change yet — the helper is unused; the next commits wire it into the prepared-quote lifecycle so a quote prepared in one chat thread cannot be executed from another.
Add a private `owner: Option<QuoteOwner>` field to PreparedTransaction
(serialised only when set, so the no-context wire shape stays stable) and
populate it from `current_owner()` in all three prepare entry points —
`prepare_transfer`, `prepare_swap`, `prepare_contract_call`.
Chain-test literal constructions in `wallet/chains/{btc,solana,tron}.rs`
get `owner: None` to keep their fixtures compiling (these are direct
struct literals in the same crate, not external constructors).
Pure data plumbing in this commit — execute_prepared still ignores the
owner field. The gate is wired in the next commit.
…ote_for Replaces `take_quote(id)` with `take_quote_for(id, caller_owner)`. Read the caller's chat-thread owner from APPROVAL_CHAT_CONTEXT once at execute_prepared entry, then require the stored quote's owner to match. Mismatch returns the exact same "quote '…' not found" string the missing-id branch returns, so a co-channel attacker who has scraped a quote_id from the prompt broadcast cannot distinguish wrong-owner from no-such-quote — no enumeration oracle. Owner check runs *before* removal, so a mismatched caller cannot poison the store by consuming someone else's quote. The existing retry-restore path already does `quote.clone()` so the owner survives broadcast failure and retry naturally. Callers with no chat context (CLI, direct JSON-RPC, background turns) can only execute quotes also prepared with no chat context — intentional, since those flows have no shared channel from which a quote_id could leak. Tests added in the next commit.
…hape invariant Five new tests pinning down the prepare/execute owner gate: 1. `execute_prepared_rejects_cross_owner_execution` — Alice prepares, Bob tries to execute → error returned, quote remains in the store so Alice can still execute. Mismatched callers can't poison the store. 2. `execute_prepared_allows_same_owner_execution` — Alice prepares + Alice executes inside the same APPROVAL_CHAT_CONTEXT scope → past the owner gate (chain code may error later for unrelated mock reasons, but the failure is asserted *not* to be the not-found oracle). 3. `execute_prepared_allows_no_context_flows` — prepare + execute outside any scope → success. Keeps CLI / direct JSON-RPC usable. 4. `execute_prepared_rejects_chat_quote_from_no_context_caller` — prepare under owner A, execute with no scope → reject. Prevents privilege-drop into background / triage / cron flows that wouldn't surface UI confirmation. 5. `execute_prepared_owner_mismatch_error_matches_not_found_shape` — explicit byte-equality assertion locking the no-enumeration-oracle invariant. Regressions here would re-open the leak. Owner-stamped fixtures are built via insert_owned_quote() to avoid needing the full wallet-setup + mock-RPC stack for gate-only assertions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughPreparedTransaction gains an optional ChangesQuote Owner Gating
Sequence DiagramsequenceDiagram
participant Caller
participant PrepareEndpoint
participant CurrentOwner
participant QuoteStore
participant ExecutePrepared
Caller->>PrepareEndpoint: prepare_transfer(params)
PrepareEndpoint->>CurrentOwner: current_owner()
CurrentOwner-->>PrepareEndpoint: Option<QuoteOwner>
PrepareEndpoint->>QuoteStore: store quote with owner
QuoteStore-->>PrepareEndpoint: quote_id
PrepareEndpoint-->>Caller: PreparedTransaction{quote_id, owner: Some(...)}
Caller->>ExecutePrepared: execute_prepared(quote_id, ...)
ExecutePrepared->>CurrentOwner: current_owner()
CurrentOwner-->>ExecutePrepared: Option<QuoteOwner>
ExecutePrepared->>QuoteStore: take_quote_for(quote_id, caller_owner)
QuoteStore-->>ExecutePrepared: quote (owner matches) / not found
ExecutePrepared-->>Caller: execution result / error
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
the
feature
![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
🧹 Nitpick comments (1)
src/openhuman/wallet/execution.rs (1)
1289-1316: ⚡ Quick winAdd one owner-gating test that goes through a real
prepare_*path.These tests inject
ownerwithinsert_owned_quote(...), so they never verify thatcurrent_owner()is actually stamped during prepare. If task-local propagation regresses, the gate tests still stay green while chat-scoped quotes quietly fall back toowner: None. Please cover at least oneprepare_* -> execute_preparedflow insideAPPROVAL_CHAT_CONTEXT.scope(...).Also applies to: 1339-1484
🤖 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 `@src/openhuman/wallet/execution.rs` around lines 1289 - 1316, Add a test that exercises a real prepare -> execute_prepared flow inside APPROVAL_CHAT_CONTEXT.scope(...) instead of injecting owner via insert_owned_quote, so the owner is stamped by task-local context; specifically, create a test that calls one of the real prepare_* functions (e.g., the prepare for the transfer path used in your suite) within APPROVAL_CHAT_CONTEXT.scope(...) and then calls execute_prepared (or execute_prepared_transaction) on the produced PreparedTransaction and assert that PreparedTransaction.owner (or current_owner() on the executed result) is Some(expected_owner); this ensures task-local propagation is exercised instead of bypassing prepare_* with insert_owned_quote.
🤖 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 `@src/openhuman/wallet/execution.rs`:
- Around line 128-135: PreparedTransaction.owner (type QuoteOwner) currently
gets serialized when Some(...) and leaks chat thread/client IDs; update the
field to never be included in RPC responses by annotating it to be skipped by
Serde (e.g., replace or add #[serde(skip)] / #[serde(skip_serializing)] on the
owner field in the PreparedTransaction struct), leaving internal usages
(prepare_transfer/prepare_swap/prepare_contract_call and
execute_prepared/ExecutionResult.transaction) unchanged so the field remains
available in-process but is never emitted over the wire; reference the
PreparedTransaction.owner, QuoteOwner, current_owner(),
prepare_transfer/prepare_swap/prepare_contract_call and execute_prepared symbols
when making the change.
---
Nitpick comments:
In `@src/openhuman/wallet/execution.rs`:
- Around line 1289-1316: Add a test that exercises a real prepare ->
execute_prepared flow inside APPROVAL_CHAT_CONTEXT.scope(...) instead of
injecting owner via insert_owned_quote, so the owner is stamped by task-local
context; specifically, create a test that calls one of the real prepare_*
functions (e.g., the prepare for the transfer path used in your suite) within
APPROVAL_CHAT_CONTEXT.scope(...) and then calls execute_prepared (or
execute_prepared_transaction) on the produced PreparedTransaction and assert
that PreparedTransaction.owner (or current_owner() on the executed result) is
Some(expected_owner); this ensures task-local propagation is exercised instead
of bypassing prepare_* with insert_owned_quote.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c547791-77d7-4c9c-b267-4088301c7853
📒 Files selected for processing (4)
src/openhuman/wallet/chains/btc.rssrc/openhuman/wallet/chains/solana.rssrc/openhuman/wallet/chains/tron.rssrc/openhuman/wallet/execution.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@oxoxDev nice security work here — the two-step owner-binding approach is clean, the error-shape invariant (mismatch returns byte-equal "not found") is properly locked down in tests, and the None == None round-trip for non-chat callers is exactly right.
CI is failing on Rust Core Tests and Frontend Unit Tests, so I'm holding off on a full sign-off until those are green. That said, a couple things to address while you're at it:
CodeRabbit flagged the main one: the #[serde(skip_serializing_if = "Option::is_none")] annotation on PreparedTransaction.owner leaks thread_id/client_id as threadId/clientId in any RPC response where a chat context is present. That field should be #[serde(skip_serializing)] — internal gate data, never wire-visible.
One thing they didn't catch: QuoteOwner itself derives Serialize with #[serde(rename_all = "camelCase")]. After the field annotation is fixed, that derive is dead weight — and a latent trap. Any future struct that embeds QuoteOwner without an explicit skip on the field would re-expose session identifiers without warning. Since QuoteOwner is purely an internal gate type with no serialization use case, drop Serialize from its derive list entirely.
Fix the CI, apply CodeRabbit's suggestion, drop Serialize from QuoteOwner, and this is good to go.
|
@oxoxDev unresolved review feedback — please address before we review. |
|
Unresolved review feedback from coderabbitai[bot] — please address before we review. |
…e-path integration test - Change `PreparedTransaction.owner` from `skip_serializing_if = "Option::is_none"` to `skip_serializing` so chat thread/client IDs are never leaked in RPC responses (addresses @coderabbitai on execution.rs:135 and @graycyrus review). - Drop `Serialize` + `serde(rename_all)` from `QuoteOwner` — purely internal gate type with no serialization use case (addresses @graycyrus review). - Add `prepare_stamps_owner_via_task_local` integration test that exercises a real `prepare_transfer` inside `APPROVAL_CHAT_CONTEXT.scope(...)` to verify task-local propagation (addresses @coderabbitai nitpick on lines 1289-1316).
|
@graycyrus — CodeRabbit's serde concern on the owner field is resolved by c63ac3f (which Steven pushed directly): |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Continuation review — two new commits since the last pass.
c63ac3f addresses both the CodeRabbit thread and my earlier note: #[serde(skip_serializing)] replaces the conditional skip, and Serialize / rename_all are gone from QuoteOwner entirely. Session identifiers no longer reach the wire under any code path. The new prepare_stamps_owner_via_task_local test exercises the full task-local propagation end-to-end, which is the right level of confidence for that // SAFETY: invariant.
CI is green across all 30 checks. Code is clean. Approving.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #2708 — feat(wallet): bind prepared transaction quotes to originating chat session
Walkthrough
Defense-in-depth follow-up to #2331. After per-sender session isolation landed, the QUOTE_STORE remained keyed only by quote_id with no ownership gate. Since the prepared-tx summary is broadcast to the full channel, a co-member could read the quote_id and call wallet_execute_prepared from their own (now isolated) agent session to trigger the original sender's transaction. This PR closes that by stamping a QuoteOwner {thread_id, client_id} on every quote at prepare time, then enforcing equality at execute time — with the mismatch error deliberately identical to the genuine not-found error to avoid an enumeration oracle.
Changes
| File | Summary |
|---|---|
src/openhuman/wallet/execution.rs |
QuoteOwner struct + current_owner() task-local reader; owner field on PreparedTransaction; take_quote → take_quote_for; three prepare_* fns stamped; 6 new tests |
src/openhuman/wallet/chains/btc.rs |
Test struct literals updated with owner: None |
src/openhuman/wallet/chains/solana.rs |
Test struct literals updated with owner: None |
src/openhuman/wallet/chains/tron.rs |
Test struct literals updated with owner: None |
Actionable comments (0)
None — the implementation is correct across all axes.
Verified / looks good
- No TOCTOU in
take_quote_for: the mutex is held for the full function — position lookup, owner check, andstore.removeare atomic with respect to other callers. ✓ - Store not poisoned on mismatch: owner check runs before
store.remove(pos), so a rejected caller cannot consume someone else's quote. Explicitly commented and covered byexecute_prepared_rejects_cross_owner_execution(which asserts the quote is still present after rejection). ✓ - Error indistinguishability: both genuine not-found and owner-mismatch use the same
not_found()closure → byte-equal strings. Locked byexecute_prepared_owner_mismatch_error_matches_not_found_shape. ✓ #[serde(skip_serializing)]is load-bearing:QuoteOwnerdoes not deriveSerialize, so without this attribute thePreparedTransactionderive would fail to compile. The attribute is not merely documenting intent — it is required. ✓- Owner preserved through restore path:
restorable = quote.clone()carries theownerfield, so a quote restored after a chain-layer failure (store_quote(refreshed)) retains the original owner and remains executable only by its original session. ✓ None == Noneround-trip: CLI / direct-RPC callers have noAPPROVAL_CHAT_CONTEXT, socurrent_owner()returnsNoneon both prepare and execute.Option::PartialEqmakesNone == Nonepass the gate. Covered byexecute_prepared_allows_no_context_flows. ✓- Privilege-drop blocked: a context-scoped quote (
Some(owner)) cannot be executed by a no-context caller (None). Covered byexecute_prepared_rejects_chat_quote_from_no_context_caller. ✓ - Task-local wiring tested end-to-end:
prepare_stamps_owner_via_task_localgoes through the fullprepare_transfercode path inside anAPPROVAL_CHAT_CONTEXTscope and asserts the stamped owner matches. ✓ - All 25 CI checks pass, including Rust coverage gate (≥ 80% on changed lines). ✓
- No merge conflicts with current main. ✓
4 participants
Summary
PreparedTransactionatwallet_prepare_*time.wallet_execute_preparedso the caller's session must match the prepared quote's stamped owner."quote '<id>' not found"error verbatim (no enumeration oracle).Problem
#2331 hardened the channel inbound path so two distinct senders in the same shared channel no longer collapse into one cached agent session. The wallet broadcast path was not aware of that session boundary:
QUOTE_STOREis process-global keyed only byquote_id, andexecute_preparedonly checksconfirmed: true.In a shared multi-member channel where the agent's confirmation prompt is broadcast back to every channel member, a co-channel observer can read the prepared
quote_idand then pass it intowallet_execute_preparedfrom their own (now session-isolated) agent run, triggering the original sender's prepared transaction.Solution
QuoteOwner { thread_id, client_id }derived from theAPPROVAL_CHAT_CONTEXTtask-local that already scopes the agent tool loop (channels/providers/web.rs::run_chat_task).Option<QuoteOwner>onto eachPreparedTransactionat prepare time —prepare_transfer,prepare_swap,prepare_contract_call.execute_preparedreadscurrent_owner()once on entry and gates retrieval throughtake_quote_for(id, caller_owner). Owner mismatch returns the same not-found error string the genuine not-found branch returns, so error-string diffing cannot enumerate other sessions' quote ids.Option<QuoteOwner>preserves single-session flows:None == Noneround-trips successfully so CLI, direct JSON-RPC, single-DM Telegram, single-user Discord channels are unchanged.A
// SAFETY:doc oncurrent_owner()flags reliance on the inline.awaitchain inrun_chat_task. A future refactor that detaches the wallet handler onto a freshly spawned task would landcaller_owner = Noneagainst aSome(owner)quote → reject. The safe default.Submission Checklist
wallet/execution.rs:execute_prepared_rejects_cross_owner_executionexecute_prepared_allows_same_owner_executionexecute_prepared_allows_no_context_flowsexecute_prepared_rejects_chat_quote_from_no_context_callerexecute_prepared_owner_mismatch_error_matches_not_found_shapecargo fmt --all -- --checkcleancargo check --libcleancargo clippy --lib— zero new warnings on changed linescargo test --lib openhuman::wallet::execution— 18 passed / 0 failed (13 pre-existing + 5 new)Impact
owner = None(noAPPROVAL_CHAT_CONTEXT), so the equality check isNone == Noneand the existing flow keeps working.owneris#[serde(skip_serializing_if = "Option::is_none")]. LegacyNonequotes serialize byte-identically to today; only quotes prepared inside a chat scope add the new field.owner.thread_idderives from the sameAPPROVAL_CHAT_CONTEXT.thread_idthatderive_inbound_thread_idalready special-cases viachannel_is_telegram. No separate plumbing.Related
send_channel_reply) — privacy-only residual after the execution path is gated; consider sensitive-reply DM redirect on multi-member shared channels.current_owner()from a task-local read to explicit-argument plumbing so a future refactor cannot silently no-op the gate by detaching the wallet handler onto a fresh task.AI Authored PR Metadata
Summary by CodeRabbit
Bug Fixes
Tests