feat(rn_cli_wallet): encrypt MMKV secret storage with keychain-backed key#566
Draft
ignaciosantise wants to merge 10 commits into
Draft
feat(rn_cli_wallet): encrypt MMKV secret storage with keychain-backed key#566ignaciosantise wants to merge 10 commits into
ignaciosantise wants to merge 10 commits into
Conversation
… key Wallet mnemonics/private keys were persisted as plaintext in MMKV. Encrypt the MMKV store at rest on native using MMKV's built-in encryptionKey, with a random key generated once and kept in the iOS Keychain / Android Keystore via expo-secure-store. All secret access is funneled through a single chokepoint (getEncryptionKey), so every chain is protected at once and the design is ready for optional biometric gating later. - add expo-secure-store dependency - new src/utils/secureEncryptionKey.ts owns the key lifecycle (native-only; web returns undefined and stays best-effort/unencrypted by design) - storage.ts lazily opens an encrypted MMKV; on first run it recrypts the existing plaintext store in place so wallets aren't reset on upgrade - reword now-inaccurate "stored unencrypted" dev warnings in the chain utils Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @ignaciosantise's task in 2m 33s —— View job Code Review in Progress
|
There was a problem hiding this comment.
Pull request overview
Introduces at-rest encryption for wallets/rn_cli_wallet secret persistence by generating an MMKV encryption key once and storing it in OS-backed secure storage, then opening MMKV with that key (with an in-place migration path for existing plaintext installs).
Changes:
- Add
expo-secure-storedependency to persist an MMKV encryption key in iOS Keychain / Android Keystore. - Introduce
getEncryptionKey()helper and updatestorage.tsto lazily open MMKV with encryption + recrypt existing plaintext data on first run. - Update dev-only security warning strings in several chain wallet utils to reflect the new storage behavior.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wallets/rn_cli_wallet/package.json | Adds expo-secure-store dependency for key persistence. |
| wallets/rn_cli_wallet/yarn.lock | Lockfile updates for expo-secure-store. |
| wallets/rn_cli_wallet/src/utils/secureEncryptionKey.ts | New helper to generate/store/memoize the MMKV encryption key. |
| wallets/rn_cli_wallet/src/utils/storage.ts | Lazily initializes MMKV with encryption and performs first-run recrypt migration. |
| wallets/rn_cli_wallet/src/utils/SolanaWalletUtil.ts | Updates dev warning text to reflect encrypted MMKV on native. |
| wallets/rn_cli_wallet/src/utils/SuiWalletUtil.ts | Updates dev warning text to reflect encrypted MMKV on native. |
| wallets/rn_cli_wallet/src/utils/TonWalletUtil.ts | Updates dev warning text to reflect encrypted MMKV on native. |
| wallets/rn_cli_wallet/src/utils/TronWalletUtil.ts | Updates dev warning text to reflect encrypted MMKV on native. |
| wallets/rn_cli_wallet/src/utils/CantonWalletUtil.ts | Updates dev warning text to reflect encrypted MMKV on native. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+16
to
+20
| if (!key) { | ||
| if (__DEV__) { | ||
| console.log('[storage] MMKV opened UNENCRYPTED (web / no key available)'); | ||
| } | ||
| return new MMKV(); |
Comment on lines
+14
to
+21
| // MMKV's encryption key cannot exceed 16 bytes, so we generate 8 random bytes | ||
| // and hex-encode them into a 16-character key. | ||
| function generateKey(): string { | ||
| const bytes = crypto.getRandomValues(new Uint8Array(8)); | ||
| return Array.from(bytes) | ||
| .map(b => b.toString(16).padStart(2, '0')) | ||
| .join(''); | ||
| } |
…tance
Encrypting the DEFAULT MMKV store broke the app: SettingsStore, WalletStore
and LogStore also open `new MMKV()` (default, no key), so once the default
store was encrypted their reads returned garbage and WalletKit init threw
"Value is undefined, expected a String" — the app hung on the splash screen
(flagged by Copilot on the PR).
- storage.ts now uses a dedicated MMKV id ('wallet-secure') with the
encryptionKey, so wallet secrets + WalletConnect Core data are encrypted in
isolation and never touch the default store other modules rely on
- drop the recrypt-in-place migration (no longer encrypting the default store)
- bump key entropy to 96 bits (12 random bytes → 16-char base64) within MMKV's
16-byte key limit, per PR review feedback
Verified on Android (debug + internal): fresh install, cold restart, and
already-encrypted persisted state all initialize WalletKit successfully.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…to fix/encrypt-mnemonic-storage
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… unavailable Unsigned iOS builds (our E2E simulator archive is built with CODE_SIGNING_ALLOWED=NO) have no application-identifier entitlement, so expo-secure-store's Keychain access throws (`getValueWithKeyAsync ... failed`). getEncryptionKey didn't catch it, so the rejected promise propagated through storage into WalletKit init and the app hung on the splash screen (iOS E2E only; Android Keystore works unsigned). Wrap the SecureStore calls in try/catch and fall back to an unencrypted store so the app always initializes. Encryption stays active on real signed builds (TestFlight/App Store); only unsigned test builds degrade to unencrypted. Verified by reproducing CI's exact unsigned Release archive locally: the app now loads the wallet home instead of hanging, and logs the fallback warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Keychain fallback stopped the deterministic splash hang, but iOS E2E was still flaky: on the slower CI simulator the unsigned build's SecureStore call intermittently stalls on the securityd/KeychainMigrator path. Because that call sits on the app-init critical path (storage awaits the key before opening), init sometimes didn't finish within Maestro's wait and the app stayed on the splash screen — failing ~most pay flows with "input-paste-url not found". Race the SecureStore get/set against a 4s timeout; on timeout, fall back to unencrypted storage so init never blocks on a slow/hanging Keychain. On real signed builds SecureStore resolves in milliseconds, so the timeout never fires and encryption stays active. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This PR encrypts wallet secrets via expo-secure-store, which needs the iOS Keychain. PR #563 made the Maestro iOS build unsigned (no application-identifier entitlement), so SecureStore fails/stalls there — leaving the app on the splash screen and failing the pay suite (the app-level fallback + timeout reduced but didn't eliminate the flakiness, since the stalled Keychain call sits on the init critical path). Re-wire the match Development signing secrets into the e2e-ios job (the exact 6 lines #563 dropped). The Fastfile build_for_simulator lane already gates signing on MATCH_GIT_URL, so this flips it from unsigned back to signed and the Keychain becomes available — init behaves like main again and the encrypted storage path is actually exercised in CI. Trade-off: reintroduces the match clone + keychain setup on every iOS E2E run (the build speed #563 reclaimed, which was only needed for the since-removed Universal Link test). The app-level SecureStore fallback/timeout stays as defensive code; it's inert on signed builds where the Keychain resolves immediately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…in works" This reverts commit 192bbdf.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Problem
wallets/rn_cli_walletpersisted every chain's mnemonic / private key / secret key as plaintext in MMKV (new MMKV()with noencryptionKey). Anyone with filesystem or backup access to the device could read the seed phrases. The code itself warned about this ([SECURITY] … stored unencrypted).Fix
Encrypt the whole MMKV store at rest on native using MMKV's built-in
encryptionKey, with a random key generated once and kept in the iOS Keychain / Android Keystore viaexpo-secure-store. The key that protects the secrets never sits in plaintext.Chosen deliberately over moving secrets into SecureStore per-chain: all secret access is funneled through a single chokepoint (
getEncryptionKey), so every chain is protected at once, the 7 copy-pasted-from-web chain utils stay untouched, and it leaves a clean seam for optional biometric gating later.Changes
package.json/yarn.lock— addexpo-secure-store@~56.0.4(matches the SDK-56 unified versioning track).src/utils/secureEncryptionKey.ts(new) — owns the encryption-key lifecycle. Native-only; generates a 16-char key (8 random bytes hex, respecting MMKV's ≤16-byte limit) via the globalcrypto.getRandomValues, stores it in SecureStore, and memoizes. Web returnsundefined(no native secure storage in a browser).src/utils/storage.ts— MMKV is now opened lazily (the key fetch is async; thestorage.*methods were already async so signatures are unchanged). On first run it recrypts the existing plaintext store in place so wallets aren't reset on upgrade; afterwards it opens withencryptionKey. Dev-onlyconsole.logs report which path ran.stored unencrypteddev warnings.Platform behavior
localStorageshim, unencrypted by design (documented limitation).How to verify
cd wallets/rn_cli_wallet && yarn install, rebuild, clear Metro/gradle caches (encryption changes the on-disk format).MMKV recrypted in place …(first run) thenMMKV opened ENCRYPTED …on subsequent launches.Notes
requireAuthentication: trueinsecureEncryptionKey.ts+ theexpo-secure-storeconfig plugin forNSFaceIDUsageDescription.node_modulesnot installed here) — verification above is the intended path.🤖 Generated with Claude Code