Repository navigation
fix(wallet): expire pending signature requests after network or accou… - #1305
Open
Tamziycode wants to merge 1 commit into
Open
Tamziycode wants to merge 1 commit into
Tamziycode wants to merge 1 commit into
Conversation
|
@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. |
|
@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! 🚀 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.rejectedaudit 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
src/lib/wallet/signingRequests.ts: framework-free rules (context snapshot, expiry reason, lifetime, typedSigningRequestExpiredError).src/hooks/useSigningRequestGuard.ts: binds a request to the live wallet state and owns the lifetime timer.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/security/wallet-signing-request-expiry.md; updates towallet-threat-model.md,WALLET_ADAPTERS.md,SECURITY.md,CHANGELOG.md.Behaviour
wallet_disconnectedwallet_changedaccount_changednetwork_changedtimed_out0, 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.tsxbeyond the issue titleerrorwas set but never rendered, so every wallet error, including adapter network-mismatch errors, was invisible. Minimalrole="alert"render added._proceedToSignreturns there without calling the wallet, so without this the orphaned request would later raise a spurious timeout notice.Security, privacy, accessibility, network safety
requireSigningNetworkin the adapter is untouched and remains the last line of defence.{ reason, requestId, walletType }, the signer public key asactorand the network astarget. Never the XDR, a signature or key material (asserted in a test). A failing audit sink is swallowed.role="alert"announcement, native button for Dismiss.WALLET_SIGN_REJECTEDevent type;securityEvents.tsis not modified.Scope and limits
SignatureCollectorsigns 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.How was this tested?
49 new tests, all passing:
src/lib/wallet/__tests__/signingRequests.test.tsttl-1valid,ttlexpired), invalid and capped lifetimes, clock moving backwards, typed errorsrc/hooks/__tests__/useSigningRequestGuard.test.tsxbegin, timer boundary, report-once, ended requests never expire, cleanup on unmounttests/integration/TransactionSigner.requestExpiry.test.jsxI 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
onExpirealready clears those timers; that check is defense-in-depth.Run:
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-checkreports errors across files outside this diff (for exampleAuditLog.tsx,routes.ts,capabilitySandbox.ts). The files added or changed here type-check cleanly in isolation.adapters,passkey,smartWallet,useSidebarArrowNavandTransactionSigner.riskReview(its freighter mock lacksisFreighterInstalled). I confirmed this by running them on an unmodified checkout.docs:validate-driftreports the same 13 errors with and without this PR.governance:checkpasses.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)
_proceedToSignsetsshowMainnetReviewand returns, and nothing renders or consumes it, so the wallet is never asked to sign. Confirmed with a mocked jsdom test.validateTransactionNetworkdoes not reject a mismatched passphrase:TransactionBuilder.fromXDRparses fine under the wrong passphrase (only the hash differs), whilewallet-threat-model.mdsays this check prevents the prompt. Confirmed against the SDK. I did not change that doc claim.beginRiskReviewis never called inTransactionSigner, 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.
masterif GitHub shows "This branch has conflicts").Security-sensitive changes
.github/CODEOWNERS(wallet, auth, cryptography, CI) and a code owner has been requested for review.