Add encrypted storage and biometric unlock foundation - #128
Conversation
expo-sqlite, expo-secure-store, expo-local-authentication and expo-crypto, all on the SDK 57 line, plus the config plugins they need. useSQLCipher is what makes the journal database encrypted at rest. It is a build-level flag rather than a runtime one, so it requires `npx expo prebuild` and a development build, and it ends Expo Go for local development. That cost is real and is recorded in 0016 rather than left for the next person to discover. enableFTS is set explicitly even though it already defaults to true. 0013's drug search is SQLite FTS5, so an edit that switched it off would break the Medicine Diary while looking like a storage-layer change — not somewhere anyone would think to look. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The encrypted local database and the unlock gate that the rest of Phase 1 sits on. Neither existed: the decision for both lives in architecture-plan.md and was referenced by 0003 and 0007, but was never turned into implementation work, so every Phase 1 list screen was blocked on something with no ticket. src/lib/db holds the storage layer. key.ts generates a 256-bit key and keeps it in Keychain/Keystore; database.ts opens the file, keys it before anything else touches the connection, and migrates; migrations.ts runs each step in its own exclusive transaction with the user_version bump, so an interrupted upgrade leaves a version that matches the schema; and repository.ts is the repeatable-entry data layer that contacts, providers, allergies and the rest are all variants of, per 0008. The key is deliberately stored without SecureStore's requireAuthentication. That option invalidates the stored value when biometric enrollment changes, which on a device holding the only copy of the data (0001) turns adding a fingerprint into silent, permanent loss of someone's journal. The biometric gate is applied separately at the app shell instead, which is what 0007 actually asks for. The trade is written up in 0015 and there is a test asserting the option stays off. The database refuses to open on web rather than falling back to plain SQLite, since a quietly unencrypted medical journal is worse than a loud failure. No domain tables ship here — the first one belongs to the first feature that needs it, #118. 95 tests. The repository and migration suites run real SQL through node:sqlite rather than asserting on generated strings, so ordering, constraints and rollback are genuinely exercised. What none of it proves is that the file is actually encrypted; that needs hardware and is the gap #101 tracks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four choices surfaced while building the storage foundation. None were settled anywhere, and three of them are the kind that look like implementation detail in a diff and like a policy decision six months later, so they are recorded rather than left in the code. 0015 — Where the database key lives, and why it is stored without SecureStore's requireAuthentication. Expo's docs are explicit that a value stored that way becomes inaccessible when biometric settings change, and with no server copy (0001) that makes adding a fingerprint destroy the journal. 0007 accepts "lose the phone, lose the journal" and requires telling users so; nobody has told anyone about the fingerprint case. Needs technical-lead sign-off, and states plainly what the trade costs. 0016 — Development builds are now required and Expo Go no longer runs the app. Records the alternative that would have preserved it (app-level field encryption) and why it loses. Also notes that useSQLCipher is build-wide rather than per-database, so 0013's public catalog.db gets opened by a SQLCipher build with no key set — ordinary SQLCipher behaviour, but undocumented by Expo and untested here, so it is flagged for on-device verification before the Medicine Diary depends on it. 0017 — Journal data is native-only; the database fails loudly on web instead of falling back to an unencrypted one. Scoped to patient data: 0013's public drug catalog is explicitly unaffected. 0018 — Unlock behaviour on a device with no lock screen, left Open. 0007 was written about lockout and assumes there is a device lock to fall back to. Four options with their tradeoffs, and the current "explain and continue" behaviour marked as a placeholder rather than a decision. Needs a named owner, like 0009. The README's "Related, not duplicated here" note claimed local database encryption lived only in architecture-plan.md. The direction still does; what that document left unspecified is now in-repo, so the note points at 0015-0017. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* update brakeman version * switch to double qoutes * bump sqlite
| * THIS_DEVICE_ONLY additionally keeps the key out of iCloud/Google backups, so | ||
| * a restored backup on a new phone cannot decrypt a copied database file. That | ||
| * is the same boundary 0003 draws and what issue #115 asks for. |
There was a problem hiding this comment.
keychainAccessible is @platform ios in expo-secure-store, so this option does nothing on Android — the Google-backup half of this claim isn't achieved by it. Android is covered, but by the config plugin's backup rules, which makes the bare "expo-secure-store" entry in app.json load-bearing rather than just registration. Worth saying, so nobody removes it while tidying the plugin list.
| * THIS_DEVICE_ONLY additionally keeps the key out of iCloud/Google backups, so | |
| * a restored backup on a new phone cannot decrypt a copied database file. That | |
| * is the same boundary 0003 draws and what issue #115 asks for. | |
| * THIS_DEVICE_ONLY keeps the key out of iCloud backups, so a restored backup on | |
| * a new phone cannot decrypt a copied database file. That is the same boundary | |
| * 0003 draws and what issue #115 asks for. | |
| * | |
| * This option is iOS-only - `keychainAccessible` is @platform ios. The Android | |
| * equivalent comes from the expo-secure-store config plugin, whose backup rules | |
| * exclude the SecureStore shared prefs from Auto Backup. That makes the bare | |
| * "expo-secure-store" entry in app.json load-bearing, not just registration. |
| * | ||
| * THIS_DEVICE_ONLY additionally keeps the key out of iCloud/Google backups, so | ||
| * a restored backup on a new phone cannot decrypt a copied database file. That | ||
| * is the same boundary 0003 draws and what issue #115 asks for. |
There was a problem hiding this comment.
Worth recording the other option that looks like a free upgrade, so it doesn't get "improved" in later by someone who reads only the requireAuthentication paragraph.
| * is the same boundary 0003 draws and what issue #115 asks for. | |
| * is the same boundary 0003 draws and what issue #115 asks for. | |
| * | |
| * WHEN_PASSCODE_SET_THIS_DEVICE_ONLY is the upgrade that looks free and is not. | |
| * It is stronger, and unlike requireAuthentication it does not bind to biometric | |
| * enrollment - but expo's documentation is that "the user must have set a | |
| * passcode in order to store an entry. If the user removes their passcode, the | |
| * entry will be deleted." It therefore cannot store a key at all on the devices | |
| * 0018 is about, and it turns removing a passcode into a data-loss event: the | |
| * same catastrophe as the fingerprint case, with a rarer trigger. |
| // Touch the schema to force SQLCipher to actually decrypt a page. Without | ||
| // this, a wrong key is not discovered until the first real query, which | ||
| // could be several screens away from the thing that caused it. |
There was a problem hiding this comment.
This comment claims more than the probe delivers. It catches a wrong key; it does not catch a missing SQLCipher, because stock SQLite ignores unknown pragmas and this read succeeds against a plaintext file. Correcting the comment is free even before the cipher_version guard lands.
| // Touch the schema to force SQLCipher to actually decrypt a page. Without | |
| // this, a wrong key is not discovered until the first real query, which | |
| // could be several screens away from the thing that caused it. | |
| // Touch the schema so a *wrong* key is discovered here rather than several | |
| // screens away at the first real query. | |
| // | |
| // What this does not catch: on a build without SQLCipher, `PRAGMA key` is | |
| // silently ignored and this read succeeds against a plaintext file. That | |
| // needs `PRAGMA cipher_version` - see #130. |
| }); | ||
| } | ||
|
|
||
| if (existing) return existing; |
There was a problem hiding this comment.
The try/catch above is the right instinct and the comment explaining it is the best in the file — but it guards the throw path, and the case that matters arrives as null. Expo's doc for getItemAsync: resolves to null "if there is no entry for the given key or if the key has been invalidated." One value, two very different meanings, and this line reads it as "first run."
Compose that with database.ts:48, where the key is fetched before openDatabaseAsync, so it can't know a database file already exists. Now run the scenario 0015 explicitly designs for — restore an iPhone backup onto a new phone. journal.db comes back (it's in Documents/, iCloud-backed); the key does not (THIS_DEVICE_ONLY, by design and correctly). First launch mints a fresh key, PRAGMA key sets it, the sqlite_master probe fails to decrypt, and the user gets Could not open the journal database. On every launch after that too, with no explanation and no way out.
That's not an edge case — it's what happens when someone replaces a phone, which is the event this ADR is reasoning about. The ADR gets the design right and then the code turns the intended outcome into a brick.
Suggested handling: check whether the database file exists before minting. File present + no key is a distinct, nameable state — say the true thing ("this journal was created on a different phone and can't be opened here") and offer to start fresh. 0007 already commits to warning people about exactly this during onboarding, so the honest message is consistent with a promise that's been made. Roughly fifteen lines between a bricked app and a sad-but-clear one.
Happy to open this as an issue instead if you'd rather keep #128 to the foundation.
| } catch (cause) { | ||
| // Reading can fail on a locked device or a corrupted keychain entry. It is | ||
| // important that this throws rather than falling through to generating a | ||
| // fresh key: a new key against an existing database means every read fails |
There was a problem hiding this comment.
Nothing calls this except destroyJournalDatabase. So the key is read once and then lives in the JS heap for the life of the process, with openPromise (:33) holding a decrypted handle open. UnlockGate re-locks the view; the data layer keeps serving.
The consequence for 0015: WHEN_UNLOCKED_THIS_DEVICE_ONLY is effectively a first-launch-only property. After that read, whether the device is locked protects nothing, because nobody asks the keychain again. The ADR's central claim — "the key is protected by the device lock" — holds at read time, once, and then it's a string in memory.
Calling closeJournalDatabase() from the same AppState handler that re-locks the gate fixes both halves: the accessibility class starts doing real work, and the gate's re-lock becomes true at the data layer instead of cosmetic. No ADR revision needed — it makes the ADR's existing reasoning true.
|
Reviewed this properly rather than skimming — the ADRs made that unusually easy. Verification. I ran the full suite on Node 22.20.0, which closes both of your "not run locally" caveats: npx jest → 10 suites, 95 tests, all pass; npx tsc --noEmit clean; npx expo lint clean. The arithmetic lines up with your run — the two suites you couldn't execute hold 9 + 17 = 26 tests, and 69 + 26 = 95. So migrations.test.ts and repository.test.ts do pass on 22.x. The 0015 trade. I'd ratify it. Expo's types confirm your mechanism exactly — requireAuthentication maps to biometryCurrentSet on iOS, which invalidates on any enrollment change. For a local-only journal with no recovery path, trading that confidentiality property against a silent, permanent, user-triggered data-loss bomb is the right way round, and I wouldn't relitigate it. Where I'd push is that the ADR reasons carefully about choosing the trade and then doesn't follow through on what it implies. Two of those are line comments above (the restore-to-new-phone path, and the key living in the heap for process lifetime). Two more are ADR text:
On #133: I'd fold the lazy-require fix in here. It's four lines, and leaving it means anyone on Node 20, which engines permits and sees two red suites on the PR that introduces them. #134 is the one I'd consider pulling into this PR rather than leaving as a follow-up: it's ~20 lines, and #118 builds directly on that layer, so fixing it here means the broken update() never exists on main. |
|
Added some fixes for the things I commented on here #138 |
The README got someone as far as a simulator and stopped. For this app that is not far enough: the unlock path in 0015 cannot be checked in a simulator at all, because simulators do not enforce biometric authentication when retrieving a stored secret - expo-secure-store's own documentation says so - and nothing in the test suite shows the database is encrypted either, since node:sqlite is stock SQLite. A green simulator run and a green test run together still leave the two things this foundation exists for unverified. So: the device path, and the fact that a free Apple ID is enough for it. There is nothing here needing a paid account - no push, no app groups, no associated domains - and that is worth saying, because assuming otherwise is the kind of thing that quietly stops a volunteer testing on the phone in their pocket. Signing, trusting the certificate, going wireless after the first cable build, the seven-day expiry, and expo-dev-client for when the Mac's address on the network keeps moving. Also warns that prebuild stops and asks for a bundle identifier, because app.json does not set one. Whatever gets typed at that prompt is written to app.json and becomes the app's permanent identity, so the README now says not to invent one. Better still would be setting it deliberately in app.json, which belongs with #128 rather than here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
What this does
Encrypts the journal database with SQLCipher and puts a biometric gate in front of the app. This is the storage foundation the Phase 1 screens sit on — key management, schema migrations, a typed repository layer, and the unlock gate — plus four decision records for the choices building it forced.
0015 — where the SQLCipher key lives. A 256-bit CSPRNG key in Keychain/Keystore via
expo-secure-storewithWHEN_UNLOCKED_THIS_DEVICE_ONLY, deliberately withoutrequireAuthentication, and the biometric prompt applied separately at the app shell.requireAuthenticationbinds the key to biometric enrollment, so adding a fingerprint or re-enrolling Face ID makes it unreadable — and per 0001 there is no server copy, so that is not an inconvenience but the permanent loss of someone's medical journal. (0007) accepts "lose the phone, lose the journal" and requires telling people about it; nobody has told anyone "add a fingerprint, lose the journal". The trade this makes — key protected by the device lock rather than bound in the secure element — is stated plainly in the record and is the part most worth a second pair of eyes.0016 — development builds required. SQLCipher is a native fork of SQLite, so
npx expo prebuildand a dev build are now the way to run this app; Expo Go is not. The alternative that preserved Expo Go was field-level encryption over plain SQLite, rejected because it is a hand-rolled scheme protecting medical data maintained by rotating volunteers, and because encrypted values cannot be indexed or sorted.0017 — journal data is native-only. SQLCipher has no web build, so the journal refuses to open in the browser rather than silently falling back to unencrypted browser storage on whatever machine someone happened to use. Web stays the landing surface and the unlock gate passes it through.
0018 — deliberately left Open. A phone with neither biometric nor passcode has nothing to prompt with, and 0007 has no answer for it. The code explains the situation and lets the user continue; that is a placeholder chosen so the gap is visible, not a decision, and it needs a named owner.
Issue
Addresses #26 and #19 — 0015 resolves the key-storage half of the local-database-encryption question that both reference as living in
architecture-plan.md.Not closing either: all four records are Proposed or Open and need sign-off.
Follow-ups the records flag, now filed:
mobile/README.mdstill tells contributors to use Expo Go. 0016 asks for the docs to change with it and that has not happened yet, so on merge the onboarding instructions describe something that no longer works (mobile/README.md:13,:31,:53), for exactly the newcomers 0008 designed a gentle first ticket for. Worth landing with or close behind this PR.useSQLCipheris a build-wide flag, socatalog.db(0013) gets opened by a SQLCipher build with noPRAGMA keyset. That is ordinary SQLCipher behaviour but is untested here, and cheap to check now versus expensive to discover late. 0016 names Manually test the app on a simulator or device #101 as the home for this; it is split out so it does not wait on a full manual QA pass, and Manually test the app on a simulator or device #101 is cross-referenced.WHEN_UNLOCKED_THIS_DEVICE_ONLYkeeps the key out of iCloud/Google backups; the database file itself still needs the equivalent.Testing
cd mobile && npm test— 69 tests pass across 8 suites. Of the new ones, that covers key generation and persistence, database open/close, the unlock flow, and the unlock gate component.Two caveats worth stating rather than burying:
migrations.test.tsandrepository.test.tsdid not run locally. They usenode:sqlite, which needs Node 22.5+; this machine is on Node 18. CI is onnode-version: 22.x, so they run there — this PR is also the first real exercise of that (#99).Related, and a genuine rough edge:
jest/in-memory-sqlite.tsexports aHAS_NODE_SQLITEguard, but the staticimport { DatabaseSync } from 'node:sqlite'throws before the guard is ever evaluated. So on any Node below 22.5 those two suites error out instead of skipping — including Node 20, whichenginescurrently permits (>=20.19.4). Filed as #133 rather than fixed here, to keep this PR to the storage foundation — happy to fold the lazy-require fix in instead if reviewers prefer.Nothing here tests SQLCipher.
node:sqliteis stock SQLite, so the tests verify schema, queries, ordering, constraints and transaction rollback against a real engine — but encryption has no Node build and needs hardware. That is Manually test the app on a simulator or device #101, and the helper says so at the top so nobody mistakes a green suite for verified encryption.Not run locally:
npm run lintandnpm run typecheckalso want a newer Node than this machine has (#100 covers the lint half of that), so CI is the check for both.