fix: close #485 scope gaps — _onHidden race + onboarding mnemonic#490
Conversation
Blume1977
force-pushed
the
feat/close-485-scope-gaps
branch
from
5ecfde4 to
3b42758
Compare
RealUnitCH#485 dropped the mnemonic on app-hidden via `WalletService.lockCurrentWallet()`, but a narrow race remained: when the lifecycle hook fired between a sign-flow's `ensureCurrentWalletUnlocked` starting and its DB-read + AES-GCM decrypt resolving, [AppStore.wallet] was still a [SoftwareViewWallet] at lock time (so `_lockWalletInPlace` no-op'd), then the in-flight unlock resolved and wrote the unlocked [SoftwareWallet] back. The mnemonic briefly resurfaced in memory until either the 60s safety-net timer or the sign-flow `finally lock` caught it. The 60s safety net is best-effort under iOS isolate suspension — that's the gap RealUnitCH#485 set out to close at the source for the foreground case. This commit extends the same principle to the in-flight unlock window: when `lockCurrentWallet` decrements the last holder, invalidate `_unlockInFlight` (both `ignore()` to silence any later Future.error and `= null` to break the identity check). The `ensureCurrentWalletUnlocked` write to `_appStore.wallet` is now gated on the in-flight token still being current — so a still-pending unlock that the lock has invalidated does not resurface the mnemonic. The in-flight sign-flow itself fails by design: it captured `credentials = appStore.wallet.currentAccount.primaryAddress` before its ensure awaited, the lock-during-flight invalidates the unlock, and `appStore.wallet` stays a [SoftwareViewWallet]. The captured `credentials` is the *old* `_LockedCredentials` from the view-wallet — `signToSignature` throws `StateError('SoftwareViewWallet credentials reached a sign call…')` (and the `assert(false, …)` trips in dev) which is caught by the existing `finally lock` in the sign flow. Security over usability: the mid-sign attempt fails, the *next* sign re-decrypts cleanly. The existing test `lock between two in-flight ensures preserves the unlocked wallet` continues to pass because in that scenario the counter goes from 2 to 1 (still > 0) and `lockCurrentWallet` returns before the invalidation, keeping the dedup contract intact for genuine concurrent holders. Only the single-holder-at-lock-time path (counter goes to 0) invalidates — exactly the case the lifecycle hook hits. While here, gate the post-unlock 60s timer on the same in-flight identity check: when the lock invalidated our write, arming the timer points at a [SoftwareViewWallet] and `_lockWalletInPlace` would safely no-op via `is! SoftwareWallet` — not a correctness bug, just dead work. The `landedInStore` local skips the arm in that case. New Completer-gated test pins the sequence: ensure-mid-flight → lock → resolve unlock → [AppStore.wallet] stays a [SoftwareViewWallet]. Reuses the gated-repository pattern from the neighbouring in-flight tests. The test asserts both the outcome (`stored.last is SoftwareViewWallet`) and the mechanism (`verifyNever(() => appStore.wallet = isA<SoftwareWallet>())`), so a future refactor cannot regress the gate while still passing by clearing the mnemonic via some other write — the suppressed write itself is the contract. Closes RealUnitCH#488.
…dden RealUnitCH#485 dropped the mnemonic stored in `AppStore.wallet` via `WalletService.lockCurrentWallet()` on app-hidden. During onboarding the freshly generated mnemonic lives in [CreateWalletState.wallet] (cubit state), NOT in [AppStore.wallet] — so the service-level lock no-op'd on this path via its `!isWalletLoaded` guard, leaving the just-generated seed resident in cubit memory if iOS suspended the isolate while the user was on the create-wallet screen. This is precisely the moment the mnemonic is most vulnerable: freshly generated, not yet backed up. A phone call coming in, or the user pulling down the notification shade, hands iOS the chance to snapshot the isolate. Option A from the issue: [CreateWalletCubit] owns its own [AppLifecycleListener] and resets state on `hidden`, dropping the wallet reference (and with it the mnemonic). The cubit remains the owner of its state — no extra indirection through a service-level cleanup hook. Re-fire `createWallet()` after the clear. The cubit is built once via `BlocProvider(create: (_) => CreateWalletCubit(...)..createWallet())` in `CreateWalletPage`, so the constructor cascade fires `createWallet()` exactly once. Without re-firing inside `_dropMnemonic`, the user would resume to `state.wallet == null` and the view's `BlocBuilder` would render `CupertinoActivityIndicator` indefinitely (escapable only via the AppBar back button). The fix: after `emit(const CreateWalletState())` clears the state, kick off a fresh `createWallet()` synchronously so the next emission replaces the cleared state. The screen briefly flashes the loading indicator, then re-renders with a NEW mnemonic — the prior in-memory seed is already gone before the new one is generated. The re-fire widens the async-tail window for `createWallet()`: user hides → `_dropMnemonic` re-fires generation → user resumes → user taps the AppBar back before the regenerated `createSeedWallet` resolves → cubit `close()` runs → the pending `emit(state.copyWith(wallet:))` would throw `StateError('Cannot emit new states after calling close')`. Guard with `if (isClosed) return;` at the async tail before the emit, matching the pattern used in `connect_bitbox_cubit` and `kyc_cubit`. The fire-and-forget `warmAuthSignature` does not emit on the cubit, so no guard is needed there. Why `hidden` (and not `paused`): same reasoning as the service path — fires earlier than `paused` on iOS (multitasking switcher + notification drag-down), Android raises it through the unified lifecycle pipeline, and `PinAuthCubit` + `WalletService.lockCurrentWallet()` (RealUnitCH#485) already use `hidden` for the same purpose. The listener is `dispose()`'d in `close()` so the cubit's lifecycle stays self-contained — no leaks if the user navigates away before generation completes. Tests cover: * hidden → first emission is the cleared state (mnemonic dropped), asserted via an emissions log to survive the synchronous regenerate that follows. * hidden with no wallet generated yet → no emission (`same` ref). * hidden → resumed re-generates a fresh wallet; emissions log pins cleared-then-new-wallet ordering and asserts `createSeedWallet` was called twice. * inactive / resumed → state preserved. Parameterised so a future refactor (e.g. switching to a `switch` with a default-clear) can't silently regress the contract for non-`hidden` lifecycle states. `paused` / `detached` are unreachable from `resumed` without going through `hidden` per Flutter's `AppLifecycleListener` state machine — those paths exercise the same clear-and-regenerate cycle anyway, so the dedicated `hidden` tests pin them. VerifySeedCubit (`lib/screens/verify_seed/`) also receives a [SoftwareWallet] from the create flow and holds it for the seed-quiz step. It is not addressed here because (1) the issue scopes Option A to [CreateWalletCubit] specifically, and (2) `VerifySeedCubit` consumes a wallet owned by its parent — dropping it independently would break the seed-quiz flow on transient hides. Worth a follow-up if the threat model demands it. Closes RealUnitCH#489.
Blume1977
force-pushed
the
feat/close-485-scope-gaps
branch
from
3b42758 to
051360f
Compare
Internal review loop completedBefore marking this PR ready for review, it went through 3 iterations of a two-agent loop (independent implementation + independent critical review), for a total of 6 agent interactions:
Each review pass used a fresh agent context with no prior implementation knowledge, so the findings represent independent reads of the code against issue acceptance criteria + repo conventions. |
Review-Findings (Defense-in-Depth + Privacy-Surface)Zwei Reviews durchgelaufen — Logik + Privacy. Der Core-Fix ( Blocker1. DB-Row-Leak durch Pre-PR: O(1) Orphan-Row pro abgebrochenem Onboarding. Verstärkt durch Pre-Existing-Bug in Future<int> deleteWallet(int walletId) =>
(delete(walletAccountInfos)..where((row) => row.wallet.equals(walletId))).go();Löscht nur aus Vorschlag (Option B, sauber):
Vorschlag (Option A, minimal): Der Empfehlungen2. PR-Description: VerifySeedCubit-Scope-Out-Begründung technisch falsch 3. Der Fix schliesst den WRITE-Pfad an die Quelle, NICHT den Decrypt selbst. Honest Acknowledgement in der PR-Description wert: "the lock invalidates the WRITE but cannot cancel the in-flight DECRYPT; the unlocked SoftwareWallet still briefly exists in a local variable until GC". Dart hat keine Zeroization — das ist eine Defence-in-Depth-Grenze, kein Bug. Aber die jetzige Formulierung "closes it at the source" überverkauft. 4. DFX-Auth: 1 HTTP GET pro Hide-Zyklus 5. Test-Fragilität: expect(emissions, hasLength(2),
reason: 'hidden must emit cleared-then-regenerated, in that order');Bricht silently bei jedem zusätzlichen Emit (z.B. Loading-Flag, Error-State, oder ein Refactor mit Zwischen-State). Die nachfolgenden Assertions ( Was solide ist
|
Batch-Re-Review (alle 3 offenen PRs)Nach den ersten 3 Iterationen (siehe vorherigen Comment) wurde der PR im Rahmen einer Batch-Verifikation aller 3 offenen PRs (#490 + #491 + #492) noch einmal von einem unabhängigen Reviewer durchgegangen, bevor alle drei gemeinsam auf Ready gestellt wurden. Iteration 4 (Batch-Verifikation): CLEAN — no issues found Verifiziert: #488 race fix hat beide Pieces ( Gesamt-Interaktions-Bilanz für diesen PR: 7 Agent-Interaktionen (3 Impl + 4 Review). |
Splits createSeedWallet into generateUncommittedSeedWallet (in-memory) + commitGeneratedWallet (DB write). CreateWalletCubit now generates fresh mnemonics per hide-cycle without writing to walletInfos; only the verified mnemonic ever lands on disk. Closes the DB-row accumulation regression introduced by the earlier _dropMnemonic re-fire — N hide-cycles now produce 0 DB rows instead of N+1.
Covers: - generateUncommittedSeedWallet returns an id=0 draft + does NOT write to walletInfos, fresh entropy per call - commitGeneratedWallet writes exactly one row per call, preserves seed - createSeedWallet remains the generate+commit convenience - CreateWalletCubit verifies generateUncommittedSeedWallet is called and commitGeneratedWallet is NEVER called from the cubit (including through the _dropMnemonic regenerate path) - VerifySeedCubit.verify commits BEFORE setCurrentWallet, uses the committed id, and skips both on a wrong word
|
Ready for review. Multi-agent loop summary: Implementation + review iterations: 3 + post-merge blocker remediation
Total agent interactions: ~14 (implementation + review across 4 phases). Separate issue filed: #498 ( |
2 participants
Closes #488
Closes #489
Three follow-up fixes for residual mnemonic-in-memory windows that PR #485 deliberately deferred plus a disk-side regression flagged in post-merge review. Two of the in-memory windows were already closed atomically (#488, #489); the third commit adds an Option B refactor of
WalletServiceso the onboarding regenerate path no longer accumulates orphan rows inwalletInfos.What changed
Three atomic commits.
1.
fix(wallet-service): close _onHidden vs _unlockInFlight race— closes #488A single sign-flow ensure that's still in flight when
_onHiddenfires used to leave the wallet asSoftwareViewWalletat lock time (no-op), then the unlock resolved and wrote the unlockedSoftwareWalletback toAppStore.wallet. The mnemonic briefly resurfaced until the 60s safety net or the sign-flowfinally lockcaught it.The fix: in
lockCurrentWallet(), when the last holder releases, invalidate_unlockInFlight(ignore()+= null). InensureCurrentWalletUnlocked(), gate the post-resolve write toAppStore.walleton the in-flight token still matching — so a pending unlock that the lock has invalidated does not resurface the mnemonic. This closes theAppStore.walletresurfacing window: the heap window is bounded by the in-flight DB-read latency, not zero (the mnemonic still lives in the resolved future's locals until the GC reclaims it), but the only path that lets it land back into observable app state is now gated.The in-flight sign-flow itself fails by design: it captured
credentials = appStore.wallet.currentAccount.primaryAddressbefore its ensure awaited, the lock-during-flight invalidates the unlock, andappStore.walletstays aSoftwareViewWallet. The capturedcredentialsis the old_LockedCredentialsfrom the view-wallet —signToSignaturethrowsStateError('SoftwareViewWallet credentials reached a sign call…')(and theassert(false, …)trips in dev), which is caught by the existingfinally lockin the sign flow. Security over usability: the mid-sign attempt fails, the next sign re-decrypts cleanly.While here, gate the post-unlock 60s timer on the same in-flight identity check (
landedInStore): when the lock invalidated our write, arming the timer would point at aSoftwareViewWalletand_lockWalletInPlacewould safely no-op viais! SoftwareWallet— not a correctness bug, just dead work.The existing test
lock between two in-flight ensures preserves the unlocked walletstill passes — the invalidation only runs when the counter goes to 0, which is precisely the lifecycle-hook scenario, not the genuine-concurrent-holders scenario.New Completer-gated test pins the sequence: ensure-mid-flight → lock → resolve →
AppStore.walletstaysSoftwareViewWallet. Reuses the gated-repository pattern from the neighbouring in-flight tests.2.
feat(create-wallet): drop & re-generate onboarding mnemonic on app-hidden— closes #489The mnemonic generated by
CreateWalletCubit.createWallet()lives inCreateWalletState.wallet, not inAppStore.wallet— soWalletService.lockCurrentWallet()no-op'd on this path (its!isWalletLoadedguard).Option A from the issue:
CreateWalletCubitowns its ownAppLifecycleListenerand resets state onhidden. The cubit is disposed-clean: the listener isdispose()'d in the cubit'sclose().Re-fire
createWallet()after the clear so the view recovers.BlocProvider.createruns once, so the constructor cascade..createWallet()fires exactly once — without re-firing inside_dropMnemonic, resume would leavestate.wallet == nullandBlocBuilderwould renderCupertinoActivityIndicatorindefinitely (escapable only via the AppBar back button). Afteremit(const CreateWalletState()), the cubit immediately callscreateWallet(); the screen briefly flashes the loading indicator, then re-renders with a NEW mnemonic. The prior in-memory seed is gone before the new one is generated.Why
hidden: same reasoning as #485. Fires earlier thanpausedon iOS, Android raises it via the unified pipeline, and the rest of the app (PinAuthCubit,WalletService.lockCurrentWallet()) already useshiddenfor the same purpose.Tests cover: hidden-clears (via emissions log so the synchronous regenerate doesn't hide the intermediate cleared state), hidden-no-op-when-no-wallet, hidden→resumed regenerates a fresh wallet, and
inactive/resumedno-op (parameterised regression-pin).3.
fix(create-wallet): defer mnemonic persistence to verify-seed confirm— disk-side regressionSurfaced in post-merge review of commit 2. The regenerate-on-hidden loop fixed #489 in memory but introduced a disk-side problem: each
createWallet()call wrote a row towalletInfos(an AES-GCM-encrypted mnemonic), andWalletStorage.deleteWalletonly deletes fromwalletAccountInfos. N+1 orphan encrypted-seed rows accumulated per onboarding session with N hide-cycles, none deletable.Option B refactor of
WalletService:generateUncommittedSeedWallet(String name) → Future<SoftwareWallet>— generates the mnemonic, builds aSoftwareWalletwithid == 0sentinel. No DB write.commitGeneratedWallet(SoftwareWallet draft) → Future<SoftwareWallet>— takes the draft, writes via_repository.createWallet, returns the id-bearing wallet. Assertsdraft.id == 0to catch double-commit at the boundary.createSeedWallet(String name)kept as a thingenerate → commitconvenience —restoreWalletstill uses the underlying_persistSoftwareWalletdirectly (typed seed → immediate persistence is the legitimate happy path; there's no verify quiz on restore).Wiring:
CreateWalletCubit.createWallet()callsgenerateUncommittedSeedWalletinstead ofcreateSeedWallet. No DB row per regenerate.VerifySeedCubit.verify()commits the draft viacommitGeneratedWalletBEFOREsetCurrentWallet, then uses the committed id. The user only reaches that branch by typing the four requested seed words correctly, so the seed that lands on disk is the seed the user kept.warmAuthSignaturecontinues to fire oncreateWallet()— the signature is derived from the primary address, which is deterministic from the mnemonic, so the warm-up is valid for the eventual committed wallet.New tests pin the contract:
WalletService:generateUncommittedSeedWalletreturns id=0 + does NOT write to the repository + fresh entropy per call;commitGeneratedWalletwrites exactly one row, preserves the draft seed;createSeedWalletstill works as the generate+commit convenience.CreateWalletCubit: the cubit callsgenerateUncommittedSeedWalletand nevercommitGeneratedWallet(including across the_dropMnemonicregenerate path).VerifySeedCubit:verify()commits BEFOREsetCurrentWallet, uses the committed id (not the0sentinel), and skips both calls on a wrong word.Why
hiddeneverywhereMirroring #485 — the threat is iOS isolate suspension before the user notices anything is wrong.
hiddenis the earliest hook that gives us a chance to drop the secret.Scope
VerifySeedCubit(the second screen in the onboarding flow) also receives theSoftwareWalletfrom the create flow and holds it for the seed-quiz step. This PR does NOT add a lifecycle drop there because (1) the issue scopes Option A toCreateWalletCubitspecifically, and (2)VerifySeedCubitconsumes a wallet owned by its parent — dropping it independently would break the seed-quiz on transient hides (e.g. notification drag-down). With Option B, the draft is still in-memory only at that stage, so the disk-orphan window doesn't exist for verify either. The in-memory window between create and verify is worth a follow-up if the threat model demands it._postUnlockLockTimer) remains in place — it's defence-in-depth for the "user signs once then leaves the app foregrounded" case (no_onHiddento trigger the lock).WalletStorage.deleteWallet'swalletAccountInfos-only behaviour is a separate issue tracked elsewhere — Option B sidesteps it by simply not writing the orphan rows in the first place.Test plan
flutter analyze— cleanflutter test— 1437 / 1437 green (1430 baseline before commit 1 + 7 new across the three commits: 1 in-flight-unlock-race, 5 cubit-lifecycle, plus the Option B additions inwallet_service_test/verify_seed_cubit_testnet out to +1 with the existingcreateSeedWallettest reshuffled intogenerate+commitgroups)flutter pub get→dart run tool/generate_localization.dart→dart run tool/generate_release_info.dart→flutter pub run build_runner build→flutter analyze→flutter test— all green_onHiddenvs._unlockInFlightlets mnemonic resurface briefly after hidden #488): start a sign flow that's slow enough to interleave (e.g. throttled network) → background the app while the unlock is in flight → return → the in-flight sign throwsStateError(programmer-error path: locked credentials reached a sign call) and is caught by the existingfinally lock; the next sign re-decrypts cleanly. NoAppStore.walletresurfacing toSoftwareWalletafter the lock. Hard to engineer reliably on a real device — the test pins the contract.CreateWalletState.walletis not dropped on app-hidden #489): fresh install → onboarding → reach the create-wallet screen with mnemonic visible → swipe up to multitasking → return → the screen briefly shows the loading indicator, then re-renders with a NEW mnemonic. No stale seed visible. Repeat on Android multitasker.walletInfosrows). Hard to introspect SQLite directly on a real device — the unit-test pin is the contract.Honest limitations
_onHiddenvs._unlockInFlightlets mnemonic resurface briefly after hidden #488 fix relies on the_unlockInFlightidentity check — if a future refactor changes how the in-flight slot is managed, the check could silently drift. The new test pins the exact sequence, but conceptually it tests behaviour, not the implementation. A reviewer extendingensureCurrentWalletUnlocked()should re-check the invariant.CreateWalletState.walletis not dropped on app-hidden #489 fix clears the cubit state onhiddenand immediately re-firescreateWallet()to recover the view. When the user returns, the view re-renders theCupertinoActivityIndicatorwhile a fresh mnemonic is generated. UX-wise this is acceptable for the onboarding context (the user hasn't committed to anything yet) but it's a behaviour change worth flagging — the mnemonic the user saw before backgrounding is NOT the same one shown on resume.id == 0sentinel for uncommitted drafts. It's asserted at the commit boundary, but a future refactor that hands the draft to any code path expecting a persisted id (e.g.getWalletById(0)) will fail at runtime. Confined to the create→verify hop today.