Merge pull request #10830 from accumulator/qml_trustedcoin_wallet_scope
What the developer wrote
Authored by ghost43
Merge pull request #10830 from accumulator/qml_trustedcoin_wallet_scope
qml: wallet, tx and callbacks were incorrectly scoped to class level for trustedcoin plugin.
What changed, and why it matters
This change fixes a programming mistake in Electrum's mobile/QML user interface where the trustedcoin two-factor authentication (2FA) plugin stored wallet, transaction, and callback information as shared class-level data instead of per-use data. In practice, this could mean that if multiple wallets or transactions requested an OTP at the same time, the wrong wallet or transaction could be signed, or a stale callback could run. The patch scopes those values to each individual OTP request so they cannot collide or leak across requests.
Treat as a bug-fix patch that improves state isolation for the QML 2FA signing flow. No immediate emergency response is indicated, but users relying on the QML/Android build with 2FA wallets should update to a release containing this fix. Review whether any similar class-level state exists in other GUI plugins.
Security signals we found
Class-level mutable state for security-critical objects (wallet, transaction, success/failure callbacks) replaced with per-request closure
Potential cross-request state collision in 2FA OTP signing flow
Removal of plugin-global self.wallet/self.tx/self.on_success/self.on_failure
Callback storage now instance-level on QEWallet with explicit type annotation
Evidence from the diff
The trustedcoin QML plugin previously kept mutable state (self.wallet, self.tx, self.on_success, self.on_failure) on the Plugin instance. prompt_user_for_otp overwrote these fields for each new OTP request, creating a race/collision window and possible use of stale references. The patch removes the class-level fields, passes wallet/tx/callbacks via functools.partial into qewallet.request_otp, and makes on_otp a pure function of its arguments. It also removes the now-unused otpSubmit pyqtSlot from common_qt and adds an explicit _otp_on_submit field on QEWallet. The fix is a correctness/state-isolation improvement rather than a clear-cut remote exploit, but it removes a class of confused-deputy/stale-state bugs in 2FA signing.
Changed components
electrum/plugins/trustedcoin/qml.pyelectrum/plugins/trustedcoin/common_qt.pyelectrum/gui/qml/qewallet.pyInspect captured patch +29 / −29
### electrum/gui/qml/qewallet.py
@@ -114,6 +114,8 @@ def __init__(self, wallet: 'Abstract_Wallet', parent=None):
self._seed = ''
self._seed_passphrase = ''
+ self._otp_on_submit = None # type: Callable[[str], None]
+
self.tx_notification_queue = queue.Queue()
self.tx_notification_last_time = 0
@@ -599,7 +601,7 @@ def on_sign_failed(self, cb: Callable[[], None] = None, error: str = None):
if cb:
cb()
- def request_otp(self, on_submit):
+ def request_otp(self, on_submit: Callable[[str], None]):
self._otp_on_submit = on_submit
self.otpRequested.emit()
### electrum/plugins/trustedcoin/common_qt.py
@@ -63,10 +63,6 @@ def otpSecret(self):
def shortId(self):
return self._shortId
- @pyqtSlot(str)
- def otpSubmit(self, otp):
- self._plugin.on_otp(otp)
-
@pyqtProperty(str, notify=remoteKeyStateChanged)
def remoteKeyState(self):
return self._remoteKeyState
### electrum/plugins/trustedcoin/qml.py
@@ -1,3 +1,4 @@
+from functools import partial
from typing import TYPE_CHECKING, Callable
from electrum.i18n import _
@@ -8,26 +9,20 @@
from electrum.gui.qml.qedaemon import QEDaemon
from .common_qt import TrustedcoinPluginQObject
-from .trustedcoin import TrustedCoinPlugin, TrustedCoinException
+from .trustedcoin import TrustedCoinPlugin, TrustedCoinException, Wallet_2fa
if TYPE_CHECKING:
from electrum.gui.qml import ElectrumQmlApplication
from electrum.wallet import Abstract_Wallet
from electrum.wizard import NewWalletWizard
from electrum.transaction import PartialTransaction
- from .trustedcoin import Wallet_2fa
-
class Plugin(TrustedCoinPlugin):
def __init__(self, *args):
super().__init__(*args)
self._app = None # type: ElectrumQmlApplication
- self.wallet = None # type: Wallet_2fa
self.so = None # type: TrustedcoinPluginQObject
- self.on_success = None # type: Callable
- self.on_failure = None # type: Callable
- self.tx = None # type: PartialTransaction
@hook
def load_wallet(self, wallet: 'Abstract_Wallet'):
@@ -107,39 +102,46 @@ def extend_wizard(self, wizard: 'NewWalletWizard'):
def prompt_user_for_otp(
self,
- wallet: 'Wallet_2fa',
+ wallet: Wallet_2fa,
tx: 'PartialTransaction',
- on_success: Callable,
- on_failure: Callable
+ on_success: Callable[['PartialTransaction'], None],
+ on_failure: Callable[[str], None],
):
self.logger.debug('prompt_user_for_otp')
- self.on_success = on_success
- self.on_failure = on_failure if on_failure else lambda x: self.logger.error(x)
- self.wallet = wallet
- self.tx = tx
qewallet = QEWallet.getInstanceFor(wallet)
- qewallet.request_otp(self.on_otp)
-
- def on_otp(self, otp):
+ qewallet.request_otp(partial(self.on_otp, wallet, tx, on_success=on_success, on_failure=on_failure))
+
+ def on_otp(
+ self,
+ wallet: Wallet_2fa,
+ tx: 'PartialTransaction',
+ otp,
+ *,
+ on_success: Callable[['PartialTransaction'], None],
+ on_failure: Callable[[str], None] = None
+ ):
self.logger.debug('on_otp')
+ assert wallet and isinstance(wallet, Wallet_2fa)
+
+ on_failure = on_failure if on_failure else lambda x: self.logger.error(x)
if not otp:
- self.on_failure(_('No auth code'))
+ on_failure(_('No auth code'))
return
try:
- self.wallet.on_otp(self.tx, otp)
+ wallet.on_otp(tx, otp)
except UserFacingException as e:
- self.on_failure(_('Invalid one-time password.'))
+ on_failure(_('Invalid one-time password.'))
except TrustedCoinException as e:
if e.status_code == 400: # invalid OTP
- self.on_failure(_('Invalid one-time password.'))
+ on_failure(_('Invalid one-time password.'))
else:
- self.on_failure(_('Service Error') + ':\n' + str(e))
+ on_failure(_('Service Error') + ':\n' + str(e))
except Exception as e:
- self.on_failure(_('Error') + ':\n' + str(e))
+ on_failure(_('Error') + ':\n' + str(e))
else:
- self.on_success(self.tx)
+ on_success(tx)
def billing_info_retrieved(self, wallet):
self.logger.info('billing_info_retrieved')Why this scored 42/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.