mission(biometric-vault): biometric-first native vault (mission branch)#2
Open
LiranCohen wants to merge 5 commits into
Open
mission(biometric-vault): biometric-first native vault (mission branch)#2LiranCohen wants to merge 5 commits into
LiranCohen wants to merge 5 commits into
Conversation
LiranCohen
added a commit
that referenced
this pull request
Apr 24, 2026
…ings PR #2 review surfaced 8 findings across security, data integrity, UX, and CI. This commit lands the implementation fixes plus 32 new regression tests pinning the new behavior so future refactors can't silently regress. Finding 1 (Critical) — recovery phrase can be permanently lost before backup confirmation. Add a persisted `isPendingFirstBackup` flag to session-store so a relaunch / auto-lock between "native secret provisioned" and "user confirmed phrase" routes back to RecoveryPhrase instead of the unlock gate. Add `commitSetupInitialized()` for atomic two-flag persistence. Add `agent-store.resumePendingBackup()` that prompts biometrics once via agent.start({}) and re-derives the mnemonic from the vault's in-memory entropy via the new `BiometricVault.getMnemonic()`. `hydrate()` now detects an orphan native-secret (committed onboarding + uncommitted identity + native secret on disk) and promotes it to the pending-backup state so a crash mid-setup also recovers cleanly. Finding 2 (High) — lock paths did not zero in-memory key material. `agent-store.teardown()` now calls `vault.lock()` (which zeroes _secretBytes / _rootSeed / _contentEncryptionKey synchronously) before dropping the store reference. The manual "Lock wallet" path in app-navigator.tsx now calls both sessionStore.lock() and agentStore.teardown() so manual lock matches auto-lock semantics. Finding 3 (High) — Android first-frame screenshot race. Apply WindowManager.LayoutParams.FLAG_SECURE in MainActivity.onCreate() BEFORE super.onCreate(), so the first frame of every screen (including RecoveryPhrase) is protected. The per-screen JS enable/disable shim is kept as defense-in-depth. Finding 4 (High) — CI artifact leak on FLAG_SECURE regression. emulator-debug-flow.py now defines `SENSITIVE_SCREEN_NAMES = {"recovery-phrase"}` and `screencap()` short-circuits for any name in that set: it skips the adb screencap call entirely and writes the placeholder PNG, so a FLAG_SECURE regression cannot leak a mnemonic through the unconditionally-uploaded /tmp/emulator-ui-artifacts/** archive. Finding 5 (High) — iOS silently stored random bytes for malformed secretHex. RCTNativeBiometricVault.mm `generateAndStoreSecret` now explicitly rejects when secretHex is non-nil but its length != 64 or it contains invalid hex, matching the Android module's behavior. The pre-fix fall-through-to-random path was a silent JS/native secret mismatch waiting to happen on restore. Finding 6 (Medium) — iOS misrouted retryable auth failure. codeForGetSecretOSStatus is now a pure passthrough to codeForOSStatus; errSecAuthFailed consistently maps to retryable kErrAuthFailed instead of being escalated to KEY_INVALIDATED based on a stale LAContext probe. Users no longer get pushed into recovery-restore on a single Face ID / Touch ID retry. Finding 7 (Medium) — restore deleted the existing native secret before validating the mnemonic. agent-store.restoreFromMnemonic() is now a two-phase action: Phase 1 runs validateMnemonic() with zero state mutation and zero native I/O; Phase 2 only runs after Phase 1 has confirmed a valid BIP-39 phrase. An invalid mnemonic preserves both the prior native secret and the prior in-memory agent / vault / identities references — the action is a true no-op on validation failure. Finding 8 (Medium) — emulator hardening checks were not triggered by the files they validate. debug-emulator.yml `paths:` trigger expanded to include scripts/**, android/**, ios/**, and the workflow file itself, so a change to the emulator driver, the native vault, or the workflow definition runs the validation flow. Test coverage (32 new regression tests, 437 total): - session-store.test.ts: 8 tests for isPendingFirstBackup (default, hydrate round-trip, persist, commitSetupInitialized atomicity, orphan-secret promotion, defense-in-depth gate) - agent-store.teardown.test.ts: 3 tests for vault.lock() invocation during teardown including the rejection path - agent-store.restore-identities.test.ts: 4 tests for validate-before-wipe (empty / wrong-length / bad-checksum / prior-state-preserved) - agent-store.recoveryPhrase.test.ts: 5 tests for resumePendingBackup() (re-derive mnemonic, no native touch, no SecureStorage leak, cancellation, key-invalidated) - biometric-vault.lock.test.ts: 5 tests for getMnemonic() (round-trip, no biometric prompt, lock+unlock round-trip, locked / never-initialized error paths) - recovery-phrase-screen.resume.test.tsx (new file): 7 tests for the resume CTA UI (gating, single-invoke, in-flight label + double-tap suppression, error surfacing, retry, grid-rendering precedence, legacy empty-grid behaviour) - no-leakage-flow.test.tsx: assertion update for the new isPendingFirstBackup key in the persisted session payload Verification: bun run test (437/437 pass), bun run lint (clean), bun run typecheck (clean). Made-with: Cursor
LiranCohen
added a commit
that referenced
this pull request
Apr 27, 2026
…s per-window block, not focus-marker cross-ref Round-9 follow-up #2 — surfaced after the round-9 follow-up #1 hex parser fix landed and the CI was STILL failing with the same "focused org.enbox.mobile window does NOT carry FLAG_SECURE" error against the real emulator (https://github.com/enboxorg/mobile/actions/runs/25019000636). Root cause: ``extractWindowBlockByDescriptor`` used ``String.prototype.match`` (no ``g`` flag) which returned the FIRST occurrence of ``Window{<id>...}`` in the dumpsys output. The canonical ``adb shell dumpsys window`` emits MANY ``Window{<id>...}`` references, in this order: WINDOW MANAGER POLICY STATE mTopFullscreenOpaqueWindowState=Window{<id>...} mInputFocus=Window{<id>...} mLastFocus=Window{<id>...} WINDOW MANAGER WINDOWS Window #N Window{<id>...}: ← ACTUAL per-window block mAttrs={... fl=#85812100 ...} WINDOW MANAGER GLOBAL STATE mCurrentFocus=Window{<id>...} Pre-fix, the regex matched the FIRST occurrence (a focus-marker cross-reference at the top of the dump), then the capture group ``[\s\S]*?(?=Window\{|$)`` grabbed only the few bytes between that marker and the NEXT ``Window{`` (typically another focus marker). The captured slice never contained the per-window block's ``mAttrs=...fl=#<hex>...``, so the round-9 follow-up #1 hex parser correctly observed "no FLAG_SECURE" — on a 0-byte fragment. The result: every CI run threw the privacy-gate assertion even though MainActivity correctly sets ``FLAG_SECURE``. Self-tests passed because their fixtures only emitted the focus marker ONCE — at the bottom of the synthetic dumpsys, AFTER the per- window block. So the fixtures triggered the regex on the correct block, while real CI never did. Fix: 1. ``extractWindowBlockByDescriptor`` now enumerates ALL ``Window{<id>...}`` occurrences with the ``g`` flag, captures the slice from each header to the NEXT ``Window{`` (any id), and scores candidates on whether the slice contains ``mAttrs=`` (only the actual per-window block emits that key). The longest ``mAttrs``-bearing candidate wins; if none have ``mAttrs`` we fall back to the longest candidate so the diagnostic dump still surfaces something useful. 2. ``assertFlagSecureOnForeground`` now writes a diagnostic dump (``flag-secure-diag-<context>.txt``) to the artifact directory whenever the assertion would throw, capturing: - the context name - the raw focus line - the parsed focused descriptor - the matched window block (4KB cap) - the first 64KB of the raw dumpsys window output This means any future round-N+1 regression in the dumpsys parser is debuggable from the very first failed CI run, instead of requiring a second diagnostic round-trip. 3. ``ci-debug-emulator-runner.sh`` now unconditionally snapshots ``adb shell dumpsys window`` to ``/tmp/emulator-ui-artifacts/dumpsys-window.txt`` during the ALWAYS-RUN capture phase. Tail-bounded to MAX_LOGCAT_BYTES (10 MiB). Provides the source-of-truth window-manager output for any post-mortem regardless of whether the in-driver diagnostic was able to persist. Self-test grew 3 new cases (h.21-h.23): h.21 — production-shape dumpsys with focus markers BEFORE the per-window block; per-window block has FLAG_SECURE set. Pre-fix returned false; post-fix MUST return true. This is the EXACT shape that failed the round-9 follow-up #1 CI run. h.22 — same as h.21 but FLAG_SECURE bit CLEAR. Post-fix MUST return false (regression branch: extractor must still respect the bit value once it picks the right block). h.23 — two app-owned windows (MainActivity FLAG_SECURE + insecure overlay), focus on the insecure overlay. Post-fix MUST return false — the multi-occurrence enumeration must pick the FOCUSED id, not just any mAttrs slice owned by the package. Pins Round-7 F5 semantics under the new extractor. Verify: 48 suites, 499 tests, (h.1–h.23) + (i.1–i.5) self-test cases — all green. Made-with: Cursor
LiranCohen
added a commit
that referenced
this pull request
Apr 27, 2026
…elimited symbolic flags (real google_apis API 31 emulator format) Round-9 follow-up #3 — surfaced after rounds #1 (hex parser) and #2 (per-window-block extractor) landed and CI was STILL failing (https://github.com/enboxorg/mobile/actions/runs/25020408460). Root cause uncovered via the round-9 follow-up #2 diagnostic dump (``flag-secure-diag-recovery-phrase.txt``) captured for the first time on the failed CI run. The dumped block correctly belongs to ``org.enbox.mobile/.MainActivity`` (focused descriptor matched, extractor picked the right block, mAttrs= present), AND the mAttrs.fl= line DOES contain SECURE. Excerpt: mAttrs={(0,0)(fillxfill) sim={adjust=resize} ty=BASE_APPLICATION wanim=0x10302fe fl=LAYOUT_IN_SCREEN FORCE_NOT_FULLSCREEN SECURE LAYOUT_INSET_DECOR SPLIT_TOUCH HARDWARE_ACCELERATED DRAWS_SYSTEM_BAR_BACKGROUNDS pfl=NO_MOVE_ANIMATION ...} But ``windowBlockHasFlagSecure`` returned false. Why? The actual flag format on the API 31+ google_apis emulator is SPACE-DELIMITED symbolic flag names with NO ``FLAG_`` prefix — neither the hex form ``fl=#<hex>`` (round #1) nor the pipe-delimited ``fl=...|SECURE|...`` (the symbolic fallback added in round #1) appears here. The symbolic regex ``[|=]SECURE(?:[|}\s])`` required a leading delimiter of ``|`` or ``=`` — but the leading delimiter in the production format is whitespace. Fix: widen the symbolic matcher to accept whitespace as a leading delimiter on top of ``|`` and ``=``: Before: /[|=]SECURE(?:[|}\s])/ After: /[\s|=]SECURE(?:[\s|}])/ Critically, kept case-sensitive uppercase ``SECURE`` so we don't false-match on KeyguardServiceDelegate's ``secure=true`` / ``mSimSecure=false`` lines that share the dumpsys window block on some Android builds — those would silently mask a real MainActivity FLAG_SECURE regression. Verified end-to-end against the actual CI dumpsys-window.txt artifact (run 25020408460): the new parser returns true on the focused org.enbox.mobile MainActivity block (1014 bytes, fl=...SECURE...) — the production happy path is now unblocked. Self-test grew 4 new cases (h.24-h.27): h.24 — production-shape space-delimited mAttrs with SECURE token (the EXACT shape extracted from CI run 25020408460 via the round-#2 diagnostic dump) MUST match. Pre-fix returned false; post-fix MUST return true. h.25 — same shape WITHOUT SECURE token MUST NOT match. Regression branch — guards against an over-permissive matcher (e.g. plain ``includes("SECURE")``) that would silently pass any dumpsys whose block happens to contain the substring. h.26 — block with ONLY lowercase ``secure=true`` / ``mSimSecure=`` (Keyguard state) and NO SECURE in fl= MUST NOT match. Case-sensitivity is structural, not cosmetic — these Keyguard fields appear inside our app's window block on some Android builds. h.27 — End-to-end IO wrapper on the production space-delimited shape MUST NOT throw. The production happy path that this fix unblocks. Verify: 48 suites, 499 tests, (h.1–h.27) + (i.1–i.5) self-test cases — all green. Made-with: Cursor
LiranCohen
force-pushed
the
mission/biometric-vault
branch
from
a00eb4d to
ffef41e
Compare
Fix mobile restore parity and native vault integration
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
1 participant
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.
Summary
Biometric-first native wallet for the mobile app. Replaces the JS-side
Web5IdentityVaultwith a hardware-backedBiometricVaultwhose root secret lives in the OS keystore (Android Keystore / iOS Keychain) and is unlocked exclusively through a biometric prompt. No passwords. No JS-resident root key.Highlights:
BIOMETRIC_STRONG. Recovery phrase (BIP-39) is shown ONCE and re-derivable from the stored entropy viaresumePendingBackup.useAutoLockruns teardown even whenisLocked=trueif any wallet material is still resident (round-13 F1).useAgentStore.reset()persists four retry sentinels (vault / LevelDB / auth / session) before any wipe and clears each only after the corresponding step succeeds. Cleanup helpers retry every sentinel before the next agent-init opens any handle.STORAGE_KEYSso delegate / session / revocation / registration material does not leak across wallets (round-12 F3).aesGcmEncrypt/aesGcmDecryptproduce / consume a 5-segment RFC-7516 compact JWE with the protected header bound as AAD; legacy concatenated-tag shapes are explicitly rejected (round-13 F4).FLAG_SECUREfor biometric / recovery-phrase / settings screens; the emulator self-test pins the focused-window matcher across every AOSP variant we have seen on CI.bun run verify(lint + typecheck + 570 tests + emulator self-test) plus separatebuild-android,build-ios, anddebug-androidjobs.Review history
This PR has been through 14 rounds of external review. The full set of findings + fixes lives in the commit log under the
fix-pr2-review-round*prefix; the most recent (round14) addressed two HIGH-severity issues:SecureStorageAdapter.trackKey()failed.SESSION_RESET_PENDING_KEYsentinel closes the ghost-state misroute that could trap users onBiometricUnlockagainst a wiped vault ifSESSION_KEYdeletion failed mid-reset.Test plan
bun run verify(lint + tsc + jest 570/570 + emulator self-test) — passes locally and in CI.build-android/build-ios/debug-androidworkflows — all green on the latest push.KEY_INVALIDATEDand post-reset).