Skip to content

fix(wallet): expire pending signature requests after network or accou… - #1305

Open
Tamziycode wants to merge 1 commit into
Nanle-code:masterfrom
Tamziycode:fix-issue-1190
Open

Tamziycode wants to merge 1 commit into
Nanle-code:masterfrom
Tamziycode:fix-issue-1190

Conversation

@Tamziycode

Copy link
Copy Markdown

fix(wallet): expire pending signature requests after network or account changes

Summary

A signature request in Transaction Signer is now bound to the wallet, account and network that were active when the user confirmed it. If any of them change before the wallet is asked to sign, or the request is open for more than 5 minutes, it is cancelled. The review and confirmation panels close, an alert explains why, and a wallet.sign.rejected audit event is recorded. A signature that comes back from the wallet after the context changed is discarded and never shown.

Why: nothing tied a pending request to the context it was reviewed in. A user could start signing on one network, switch network, account or wallet while the confirmation panel, biometric check or wallet prompt was open, and the flow carried on against the new context. The XDR-level network check does not catch this (see Findings).

Closes #1190

What changed

  • New src/lib/wallet/signingRequests.ts: framework-free rules (context snapshot, expiry reason, lifetime, typed SigningRequestExpiredError).
  • New src/hooks/useSigningRequestGuard.ts: binds a request to the live wallet state and owns the lifetime timer.
  • New src/components/security/SigningExpiryNotice.tsx: role="alert" notice with a keyboard-reachable Dismiss button.
  • src/components/dashboard/TransactionSigner.tsx: starts, checks and ends the request.
  • Docs: new docs/security/wallet-signing-request-expiry.md; updates to wallet-threat-model.md, WALLET_ADAPTERS.md, SECURITY.md, CHANGELOG.md.

Behaviour

Reason When
wallet_disconnected Wallet disconnects, locks or is revoked
wallet_changed A different wallet connects
account_changed Active public key differs
network_changed Dashboard network differs
timed_out Lifetime reached (5 min default, capped at 30 min; the boundary instant is already expired)
  • If several apply at once, the first in the order above is reported.
  • The request starts when the user confirms the sign action, after the mainnet write guard. It is bound to the context of the render where the user acted, so a mainnet dialog left open while the network changes expires immediately.
  • If the wallet prompt is already open, the wallet cannot be told to abandon it. The dashboard re-checks when it returns and discards the signature if the context changed. A wallet error that arrives after an expiry is suppressed so the user sees one cause.
  • A request that finishes or that the user cancels never reports an expiry.
  • An invalid lifetime (0, negative, NaN, Infinity, non-number) falls back to the default, so a request can never be created that does not expire.

Related changes in TransactionSigner.tsx beyond the issue title

  1. Wallet errors are now displayed. error was set but never rendered, so every wallet error, including adapter network-mismatch errors, was invisible. Minimal role="alert" render added.
  2. The mainnet review step ends its request. _proceedToSign returns there without calling the wallet, so without this the orphaned request would later raise a spurious timeout notice.
  3. Delayed biometric steps are tracked and cleared on expiry and unmount (previously they could not be cancelled).

Security, privacy, accessibility, network safety

  • Security: fails closed on any wallet, account, network or connection change; the result is re-checked after the wallet returns; requireSigningNetwork in the adapter is untouched and remains the last line of defence.
  • Privacy: the audit event carries only { reason, requestId, walletType }, the signer public key as actor and the network as target. Never the XDR, a signature or key material (asserted in a test). A failing audit sink is swallowed.
  • Accessibility: role="alert" announcement, native button for Dismiss.
  • Network safety: no new network calls, storage or dependencies. Reuses the existing WALLET_SIGN_REJECTED event type; securityEvents.ts is not modified.

Scope and limits

  • Transaction Signer only. SignatureCollector signs with a pasted key against the network stored on its session, so wallet and network changes do not apply to it. Stored multisig sessions are not expired.
  • Cancelling does not close an open extension popup or Ledger prompt; it only guarantees the result is not used.
  • A change is observed once the dashboard has re-rendered with it; the adapter check covers that window.

How was this tested?

49 new tests, all passing:

File Tests Covers
src/lib/wallet/__tests__/signingRequests.test.ts 18 Snapshotting, every reason and their precedence, lifetime boundary (ttl-1 valid, ttl expired), invalid and capped lifetimes, clock moving backwards, typed error
src/hooks/__tests__/useSigningRequestGuard.test.tsx 13 Expiry on each context change, stale-snapshot begin, timer boundary, report-once, ended requests never expire, cleanup on unmount
tests/integration/TransactionSigner.requestExpiry.test.jsx 18 End to end through the real component
  • Primary flow: signing completes with the context the request started in.
  • Boundary: a store update with unchanged wallet context does not expire the request; TTL minus 1 ms is valid, TTL expires.
  • Failure: a signature returned after a network change is discarded; a wallet rejection after an expiry is suppressed (one alert); an ordinary wallet error with no context change is still shown; the audit sink throwing does not break the UI.
  • Recovery: after dismissing the notice the user can sign again in the new context.

I also broke the implementation on purpose to confirm the tests catch it. Removing the post-wallet re-check, and removing the end-of-request on the mainnet halt, each fail a test. Removing the extra check inside the biometric timer callback does not, because onExpire already clears those timers; that check is defense-in-depth.

Run:

pnpm install
pnpm --filter @stellar-dev-dashboard/core build   # the integration test needs the core package built
pnpm test requestExpiry signingRequests useSigningRequestGuard
pnpm run governance:check

Manual: connect a wallet on testnet, paste an XDR, click Sign so the confirmation panel opens, then switch network or account. The panel should close with an alert and no wallet prompt should open.

Not run: Playwright, accessibility and bundle-budget checks.

CI status

CI is not fully green on this PR. The failures are in files this PR does not touch:

  • pnpm run type-check reports errors across files outside this diff (for example AuditLog.tsx, routes.ts, capabilitySandbox.ts). The files added or changed here type-check cleanly in isolation.
  • Several existing tests fail without this change too, including adapters, passkey, smartWallet, useSidebarArrowNav and TransactionSigner.riskReview (its freighter mock lacks isFreighterInstalled). I confirmed this by running them on an unmodified checkout.
  • docs:validate-drift reports the same 13 errors with and without this PR. governance:check passes.

The issue says required checks must not be bypassed. I am not claiming an exception; I am asking the maintainer to decide how to handle the existing failures, and I will fix anything in this PR's own files that a check flags.

Findings (pre-existing, not fixed here; may deserve their own issues)

  1. Mainnet signing from this panel appears to dead-end: after the mainnet confirmation, _proceedToSign sets showMainnetReview and returns, and nothing renders or consumes it, so the wallet is never asked to sign. Confirmed with a mocked jsdom test.
  2. validateTransactionNetwork does not reject a mismatched passphrase: TransactionBuilder.fromXDR parses fine under the wrong passphrase (only the hash differs), while wallet-threat-model.md says this check prevents the prompt. Confirmed against the SDK. I did not change that doc claim.
  3. beginRiskReview is never called in TransactionSigner, so the risk-review panel cannot open from this flow.

Merge requirements

A PR is merged only when every box below is true. See
Merge requirements for the full policy.

  • All required CI checks pass on the latest commit (not just an earlier push).
  • No required checks are failing, pending, or skipped — re-run or fix them; do not ask for a merge while any are outstanding.
  • The branch has no merge conflicts with the target branch (rebase or merge master if GitHub shows "This branch has conflicts").
  • Tests were added or updated for the change (primary flow, a boundary case, and a failure case).
  • Docs were updated where behaviour, configuration, or security posture changed.

Security-sensitive changes

  • This PR touches a path covered by .github/CODEOWNERS (wallet, auth, cryptography, CI) and a code owner has been requested for review.
  • I described any change to key handling, signing, session lifetime, or trusted endpoints above.

@vercel

vercel Bot commented Oct 19, 2026

Copy link
Copy Markdown

@Tamziycode is attempting to deploy a commit to the nanle-code's projects Team on Vercel.

A member of the Team first needs to authorize it.

@drips-wave

drips-wave Bot commented Oct 19, 2026

Copy link
Copy Markdown

@Tamziycode Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[2026 Wallet] Expire pending signature requests after network or account changes

1 participant