From 7563a2740570ebf5addb7e4040714cc748fbfa8d Mon Sep 17 00:00:00 2001 From: epicexcelsior Date: Wed, 10 Jun 2026 09:26:15 -0800 Subject: [PATCH] =?UTF-8?q?fix(wallet):=20close=20last=20QA-20=20hole=20?= =?UTF-8?q?=E2=80=94=20strict=20marker=20read=20in=20exists()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA-20's fix (88bfd5e) stopped hasLocalWallet()/createLocal() deleting on a keychain read error, but both gate on LocalWallet.exists(), whose marker read still used the error-swallowing secureGet. A transient keystore outage (device just unlocked, cross-process lock) read as 'no wallet': cold start routed to onboarding, and tapping create would overwrite the stored secret of a real, funded wallet. - exists() reads the marker with secureGetStrict and throws on read failure - hasLocalWallet() treats a failed marker read as 'wallet present' so the unlock path surfaces a retryable error instead of routing to create - createLocal() refuses to create when existence can't be verified — nothing is written over unknown state - connect() distinguishes a failed pubkey read (retryable, keys intact) from verified absence — 'recreate your wallet' was exactly the wrong advice during a transient outage --- .../src/infrastructure/wallet/LocalWallet.ts | 25 ++++++++++++++++--- .../infrastructure/wallet/WalletFactory.ts | 24 ++++++++++++++++-- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/mobile_app/src/infrastructure/wallet/LocalWallet.ts b/mobile_app/src/infrastructure/wallet/LocalWallet.ts index 295e27c..5b6fa5c 100644 --- a/mobile_app/src/infrastructure/wallet/LocalWallet.ts +++ b/mobile_app/src/infrastructure/wallet/LocalWallet.ts @@ -6,7 +6,7 @@ import * as LocalAuthentication from 'expo-local-authentication'; import { TurboModuleRegistry, type TurboModule } from 'react-native'; import { SecureKeys, PrefKeys, - secureGet, secureGetStrict, secureSet, secureDeleteAll, + secureGetStrict, secureSet, secureDeleteAll, prefGet, } from '@/src/storage'; import type { IWalletAdapter, WalletMode } from './types'; @@ -120,7 +120,16 @@ export class LocalWallet implements IWalletAdapter { if (!auth.success) throw new Error('Authentication cancelled'); } - const stored = await secureGet(SecureKeys.WALLET_PUBKEY); + // Strict read: a transient keystore failure must surface as a retryable + // error, NOT as "no wallet" — the absent message tells the user to + // recreate, which is exactly the wrong advice while their keys are intact. + let stored: string | null; + try { + stored = await secureGetStrict(SecureKeys.WALLET_PUBKEY); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + throw new Error(`Keychain read failed: ${msg} — your wallet is still on this device. Try again in a moment.`); + } if (!stored) throw new Error('No local wallet found — please recreate your wallet'); this._publicKey = new PublicKey(stored); } @@ -133,8 +142,18 @@ export class LocalWallet implements IWalletAdapter { return (await readAndDecrypt()).secretKey; } + /** + * True when the wallet marker is verifiably present. + * + * Reads with secureGetStrict, so a Keychain/Keystore read FAILURE throws + * instead of returning false. The conflation was the last QA-20 hole: a + * transient keystore outage (device just unlocked, cross-process lock) made + * exists() report "no wallet", which routed the user to onboarding where + * create() would overwrite the real, funded keypair. Callers must treat a + * throw as "unknown — wallet may exist" and never create/delete on it. + */ static async exists(): Promise { - return (await secureGet(SecureKeys.WALLET_MARKER)) === 'true'; + return (await secureGetStrict(SecureKeys.WALLET_MARKER)) === 'true'; } /** diff --git a/mobile_app/src/infrastructure/wallet/WalletFactory.ts b/mobile_app/src/infrastructure/wallet/WalletFactory.ts index 359db0b..0db1381 100644 --- a/mobile_app/src/infrastructure/wallet/WalletFactory.ts +++ b/mobile_app/src/infrastructure/wallet/WalletFactory.ts @@ -14,7 +14,17 @@ export const WalletFactory = { async hasLocalWallet(): Promise { if (DeviceDetector.isSolanaMobileDevice()) return MWAWallet.hasCachedToken(); - if (!await LocalWallet.exists()) return false; + let exists: boolean; + try { + exists = await LocalWallet.exists(); + } catch { + // Marker read FAILED — wallet presence is unknown, which must never be + // reported as "no wallet": that routes the user to onboarding, where + // create() would overwrite a real, funded keypair (QA-20's last hole). + // Report "present" and let connect() surface the retryable read error. + return true; + } + if (!exists) return false; const integrity = await LocalWallet.isFullyIntact(); // ONLY delete when the keys are verifiably absent (marker present but // secret/AES key genuinely gone — e.g. cross-build keychain access-group @@ -45,7 +55,17 @@ export const WalletFactory = { async createLocal(): Promise { // Guard: reconnect if wallet already exists rather than overwriting keypair. - if (await LocalWallet.exists()) { + // exists() throws on a keychain read failure — when we cannot VERIFY there + // is no wallet, creating one would overwrite the stored secret of a wallet + // that may well exist. Fail the create instead; nothing is written. + let exists: boolean; + try { + exists = await LocalWallet.exists(); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + throw new Error(`Keychain read failed: ${msg} — can't verify whether a wallet already exists, so nothing was created or changed. Try again in a moment.`); + } + if (exists) { const integrity = await LocalWallet.isFullyIntact(); // Only recreate when storage is verifiably partial — otherwise the // wallet cannot export or sign and re-onboarding is the only fix.