fix(wallet): fully purge seed + key on wallet delete#621
fix(wallet): fully purge seed + key on wallet delete#621joshuakrueger-dfx wants to merge 4 commits into
Conversation
Why this PR shows a red ✗Two checks were red; one is fixed here, the other is pre-existing:
Functional checks (Analyze & Test, BitBox quirks audit) are green. |
|
Per global repo convention (
Please drop both kinds of references before merge: Issue refs:
Caller / siblng-method references (rot risk):
These name the caller / sibling method and rot the moment any of them is renamed. If the contrast matters at the code level, it's worth renaming the surviving primitive (e.g. Fix itself looks correct (proper split between user-facing purge and onboarding-regenerate account-only delete, tests cover both paths). |
|
Re API as Decision Authority ( No API call involved; nothing for the backend to know about — the wallet's local cryptographic identity is gone after purge, future API calls will simply auth as a fresh wallet. |
|
@TaprootFreak done — dropped the |
|
Repo-rule compliance (commit |
Status — ready for review
Open gates: formal maintainer approval ( |
The user-facing 'Delete Wallet' only cleared walletAccountInfos via WalletStorage.deleteWallet — the walletInfos row (the AES-GCM-encrypted seed) and the mnemonic encryption key in secure storage both survived, leaving the full mnemonic recoverable after a delete (resale / right-to-erasure risk). Add a distinct purge primitive used only by the user-facing delete: - WalletStorage.deleteWalletCompletely (accounts + the seed row, in a tx) - SecureStorage.deleteMnemonicKey - WalletRepository.purgeWallet = deleteWalletCompletely + deleteMnemonicKey - WalletService.deleteCurrentWallet now calls purgeWallet The account-only deleteWallet is left untouched for the onboarding-regenerate flow (which relies on the seed row surviving). Tests: purgeWallet removes the row AND the key; deleteWallet still leaves both; the service delete now verifies purgeWallet (never deleteWallet). Refs RealUnitCH#612 (S2)
The repository test mocks SecureStorage, so deleteMnemonicKey's real body was never executed → scoped line coverage fell to 99.9% and the 100% Coverage Floor Gate failed. Add a direct SecureStorage unit test that invokes it.
…te-purge comments
…gration-test Per CONTRIBUTING.md, new secure-storage (platform-channel) code paths must carry the // @no-integration-test annotation while no integration_test/ dir exists; the unit test only mocks the plugin.
23e04ef to
d132e0a
Compare
Summary
The user-facing "Delete Wallet" only cleared
walletAccountInfos(WalletStorage.deleteWallet). ThewalletInfosrow — which holds the AES-GCM-encrypted seed — and the mnemonic encryption key influtter_secure_storageboth survived. So after a user deletes their wallet, the full mnemonic remained recoverable on the device (resale / GDPR right-to-erasure risk); only the current-wallet pointer was cleared, masking the residue.Fix (non-breaking — the account-only
deleteWalletis preserved for the onboarding-regenerate flow, which deliberately relies on the seed row surviving):WalletStorage.deleteWalletCompletely(id)— deletes accounts and thewalletInfosseed row in a transaction (FK-safe).SecureStorage.deleteMnemonicKey()— removes the AES-GCM key.WalletRepository.purgeWallet(id)=deleteWalletCompletely+deleteMnemonicKey.WalletService.deleteCurrentWalletnow callspurgeWallet.Test
purgeWalletremoves the seed row and the mnemonic key.deleteWallet(account-only) still leaves the row + key intact (regenerate contract guard).purgeWalletis called anddeleteWalletis not.Verification (local, CI-equivalent, Flutter 3.41.6)
flutter analyzeon the 6 changed files: No issues found.deleteCurrentWalletis reverted todeleteWallet, confirming the regression is caught.Fixes the S2 item of #612.