Skip to content

fix(test_wallet_backup): keep the mnemonic out of the bloc state - #2562

Open
ethicnology wants to merge 2 commits into
developfrom
fix/test-wallet-backup-secret-state
Open

fix(test_wallet_backup): keep the mnemonic out of the bloc state#2562
ethicnology wants to merge 2 commits into
developfrom
fix/test-wallet-backup-secret-state

Conversation

@ethicnology

Copy link
Copy Markdown
Member

TestWalletBackupState (freezed) held the full mnemonic and passphrase in bloc state. Freezed's generated toString() included both fields in clear text, so any state log, crash report, or debug session could expose the user's 12/24 words. This violates our two load-bearing rules: secrets are ephemeral (read from secure storage at the point of use, never cached in bloc state) and secrets never reach logs.

The rewrite also surfaced two pre-existing correctness flaws:

  • VerifyPhysicalBackupUsecase (then dead code) always verified against the default mainnet wallet, not the selected one.
  • VerifyMnemonicScreen pushed the success screen before the verification result was known.

What changes

  • State carries no secret. TestWalletBackupState is now statusError, wallets, selectedWallet, verificationStatus — the regenerated toString() is clean by construction.
  • Sealed reads at the point of use. Both screens fetch the mnemonic/passphrase through TestWalletBackupBloc.loadSelectedWalletMnemonic(), a presentation-layer method that forwards the use-case result without storing it. The word-order game lives in ephemeral widget state (rule: local widget state stays in StatefulWidget).
  • Verification re-reads the seed. VerifyPhysicalBackupUsecase (previously orphaned) is now wired and takes the selected wallet's fingerprint; it compares and returns only a bool. Success navigation happens only on verificationStatus.success.
  • The hardcoded English error string is replaced by the existing testBackupErrorIncorrectOrder l10n key.
  • Two orphaned planning documents removed from the repo root (separate commit).

What it deliberately does not change

  • import_mnemonic's cubit also holds the mnemonic in state — same class of issue, left for a dedicated PR.
  • Remaining dev-facing English error strings in the bloc ('Failed to load wallets: $e') — pre-existing rule-Change address must inherint label of outbound transaction label #11 violation, follow-up.
  • CompletePhysicalBackupVerificationUsecase semantics are unchanged (still marks default wallets).

Alternatives considered

  • Keeping the mnemonic in a private bloc field (not in state): rejected — it still caches key material for the bloc's whole lifetime, which the "secrets are ephemeral" rule forbids.
  • Screens calling the use case directly via get_it: rejected — it breaks the ui → presentation → usecase layering; the bloc method keeps the chain while never persisting the secret.

Reading order

  1. chore: remove orphaned planning documents from the repo root — independent cleanup.
  2. fix(test_wallet_backup): keep the mnemonic out of the bloc state — the fix, with its tests.

The freezed state held the full mnemonic and passphrase, and its generated toString() exposed both in clear text to any state log, crash report or debug session.

Secrets now stay ephemeral: screens read them at the point of use through a bloc method that never stores them, the word-order game lives in widget state, and VerifyPhysicalBackupUsecase re-reads the seed to compare, returning only a bool.

This also corrects two pre-existing flaws made visible by the rewrite: verification compared against the default mainnet wallet instead of the selected one, and the success screen opened before the verification result was known.
@ethicnology ethicnology self-assigned this Aug 6, 2026
@ethicnology

Copy link
Copy Markdown
Member Author

@wired-pasteque can you test physical backup flow?

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 7m 44s —— View job


Review of PR #2562 — keep the mnemonic out of the bloc state

  • Gather context (diff vs base, bloc observer, provider version, l10n keys)
  • Review bloc/state/event changes
  • Review use case + locator + router changes
  • Review UI screens (show/verify mnemonic)
  • Review new tests
  • Post review findings

The security goal is achieved: TestWalletBackupState no longer carries any secret, VerifyPhysicalBackupUsecase now verifies against the selected wallet's fingerprint, and navigation is gated on the actual verification result. The new tests (including the toString() leak test) are a nice touch. I found one likely blocker and a few smaller issues.

Note: I can't post formal inline review comments, so findings are listed here with file/line references.

🔴 1. context.select inside didChangeDependencies — assertion failure in debug builds

verify_mnemonic_screen.dart:35 and show_mnemonic_screen.dart:103

context.select is provider's SelectContext.select (re-exported by flutter_bloc; provider 6.1.5+1 in the lockfile), and it asserts that it's only called during build:

Tried to use context.select outside of the build method of a widget.

didChangeDependencies runs before build (including on first mount, right after initState), so opening either screen should throw this assertion in any debug build. Release builds strip the assert, which is why this can slip through — but every dev/QA build of the backup-test flow would crash. All 25 other context.select call sites in the repo are inside build methods; only these two new ones aren't. Please verify by opening the flow in a debug build.

Suggested fix: do the initial load with context.read<TestWalletBackupBloc>().state.selectedWallet?.masterFingerprint (in initState or a post-frame callback), and reload on wallet switches via a BlocListener with listenWhen: (p, c) => p.selectedWallet?.masterFingerprint != c.selectedWallet?.masterFingerprint. Fix this →

🟡 2. Stale-response race in _loadSecret when switching wallets

verify_mnemonic_screen.dart:44-61

_fingerprint is updated before _loadSecret() is awaited, so if the user switches wallets (via the app-bar picker) while a load is in flight, two loads race and the last one to complete wins — which can be the older wallet. The game would then check taps against wallet A's words while VerifyPhysicalBackup verifies against wallet B's seed, making verification impossible to pass. Capture the fingerprint at call time and discard the result if it no longer matches _fingerprint after the await. (The show screen is safe: its FutureBuilder always listens to the newest _secretFuture.)

🟡 3. Silent failure leaves an empty, unrecoverable game

verify_mnemonic_screen.dart:57-60

If loadSelectedWalletMnemonic() throws, catch (_) only clears the spinner — the user sees an empty word grid with no error and no retry. The show screen handles the same failure with oopsSomethingWentWrong; this screen should surface an error too. Fix this + #2 →

🟢 4. VerifyPhysicalBackup event carries the fully ordered mnemonic — safe today, by convention only

test_wallet_backup_event.dart:16-19

reorderedWords is the complete mnemonic in correct order. Today this is log-safe because the event is a plain class (default toString()Instance of 'VerifyPhysicalBackup'), and AppBlocObserver.onEvent logs $event only behind the hardcoded _showConsoleLogs = false. But the PR's own standard is "clean by construction" — one Equatable/freezed migration or a debug-logging flip away from leaking. An explicit redacting toString() override would lock it in. Fix this →

🟢 5. Verified wallet ≠ recorded wallet (acknowledged, but now more visible)

CompletePhysicalBackupVerificationUsecase still stamps isPhysicalBackupTested/latestPhysicalBackup on the default wallets (complete_physical_backup_verification_usecase.dart:26-34), while verification now genuinely runs against the selected wallet. Verifying a non-default wallet records the default wallet as tested and leaves the verified one unrecorded. The PR body says this is deliberately out of scope — fine, but worth a tracked follow-up since the success screen now confirms a verification the record attributes to a different wallet.

Nits

  • After a successful verification, the game state isn't reset — popping back from BackupTestSuccessScreen shows a fully-selected grid that can't be replayed without switching wallets.
  • Bloc test gap: no test for VerifyPhysicalBackup when selectedWallet is null (the 'No wallet selected' path).
  • show_mnemonic_screen.dart: when selectedWallet is null, _secretFuture stays null and the FutureBuilder shows a spinner forever (only reachable if LoadWallets fails, which also surfaces statusError).

What looks good

  • The state redesign is clean by construction; the toString() leak test pins it down.
  • VerifyPhysicalBackupUsecase fingerprint parameterization fixes the old always-mainnet-default bug, and re-reading the seed at verification time is good defense in depth.
  • Navigation now correctly waits for verificationStatus.success — the old premature push is gone.
  • The word-game logic in ephemeral widget state handles duplicate words correctly (value comparison per position).

I did not run flutter analyze/flutter test — dependencies aren't installed in this environment and network-fetching commands aren't in my allowed tools, so finding #1 is based on reading provider 6.1.5's documented select constraint; please confirm with a debug run.


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.

1 participant