fix(xbull): always close the bridge so rejected flows don't leak window listeners - #110
Open
mihaieremia wants to merge 1 commit into
Open
fix(xbull): always close the bridge so rejected flows don't leak window listeners#110mihaieremia wants to merge 1 commit into
mihaieremia wants to merge 1 commit into
Conversation
…ow listeners Every xBullWalletConnect instance registers a permanent window 'message' listener and a fresh nacl keypair. closeConnections() only ran on the success path, so a rejected connect/sign/signMessage (e.g. the user closes the popup) leaked the listener for the rest of the session. The next action's XBULL_INITIAL_RESPONSE is encrypted for the new bridge's keypair; the leaked listener's box.open then fails and the bridge throws 'Decrypted message is null' as an unhandled error inside its RxJS subscriber. Move closeConnections() into a finally in all three flows.
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.
The bug
Every
xBullWalletConnectinstance registers a permanentwindowmessagelistener and generates a fresh nacl keypair in its constructor. Inxbull.module.ts,bridge.closeConnections()is only reached on the success path — whenconnect()/sign()/signMessage()rejects (most commonly because the user closes the xBull popup without approving), thecatchrethrows and the listener leaks for the rest of the browser session.On the next xBull action, a new bridge is created and the wallet's
XBULL_INITIAL_RESPONSEis encrypted for the new bridge's public key. The leaked listener also receives the message,nacl.box.openwith its stale secret key returnsnull, and@creit.tech/xbull-wallet-connectthrows:…inside an RxJS subscriber, so it surfaces as an unhandled error in the host application. We see these regularly in production error reports at xoxno.com from users retrying an xBull connect/sign after dismissing the popup.
The fix
Move
closeConnections()into afinallyin all three flows (getAddress,signTransaction,signMessage), so the listener and the popup-poll timer are torn down whether the flow resolves or rejects.closeConnections()completes the bridge's subjects and removes themessagelistener, so calling it on the error path is exactly its intended use.No behavior change on the success path — the same call just moves from the
trybody intofinally.We're currently shipping this as a
patch-packagepatch on@creit.tech/stellar-wallets-kit@2.3.0and would love to drop it in favor of an upstream release.