Skip to content

Refuse to open a journal on a build without SQLCipher - #139

Closed
Jberma23 wants to merge 8 commits into
mainfrom
feature/require-sqlcipher-at-runtime
Closed

Refuse to open a journal on a build without SQLCipher#139
Jberma23 wants to merge 8 commits into
mainfrom
feature/require-sqlcipher-at-runtime

Conversation

@Jberma23

Copy link
Copy Markdown
Collaborator

What this does

Adds the runtime guard half of #130: the app now refuses to open the journal on a build that has no SQLCipher, instead of silently writing a plaintext one.

PRAGMA key cannot tell you whether SQLCipher is present. SQLite ignores pragmas it does not recognise rather than erroring, so on a stock build the key statement is accepted and does nothing, the schema read after it succeeds against a plaintext file, and the app writes an unencrypted medical journal with no error, no warning, and nothing observable that distinguishes it from the encrypted case. Verified against stock SQLite:

PRAGMA key on plain SQLite: ACCEPTED SILENTLY (no error)
sqlite_master probe still works: {"c":0}
PRAGMA cipher_version -> null

That build is easy to be running. useSQLCipher is applied by a config plugin at prebuild, so Expo Go has never had it, and neither has a stale ios/ or android/ directory generated before the flag was set — and until #129 lands the README still tells contributors to use Expo Go.

PRAGMA cipher_version returns a version string on a SQLCipher build and no row on stock SQLite, which makes it the one cheap way to tell them apart. It asks the library rather than the database, so it is safe ahead of the key — and asking first means a build that cannot encrypt fails against an empty file rather than filling one with plaintext. The error names the remedy (npx expo prebuild + a development build), since whoever hits it is the person who most needs to read it.

0017 refuses an unencrypted fallback on web for exactly this reason: a silent downgrade makes the privacy promise untrue in the way nobody notices until it matters. This applies the same rule to a build that lost SQLCipher, and reports it as SQLCipherUnavailableError rather than a generic open failure — the problem is the binary, not the database, and "could not open the journal database" would send someone looking in the wrong place.

Issue

Addresses #130

Not closing it. #130 has two halves and this is only one: whether a SQLCipher build can read an unkeyed plaintext catalog.db still needs a device, and is untouched here.

Testing

  • Covered by tests

cd mobile && npm test11 suites, 114 tests, all pass (109 before). npx tsc --noEmit and npm run lint clean, on Node 22.20.0.

Five new tests, mutation-checked: commenting out the assertSQLCipher call fails all five and nothing else, so they are genuinely load-bearing rather than passing by coincidence. They cover the refusal itself, that the check happens before PRAGMA key so nothing is written to the file, that the message names the remedy, that the handle is closed, and that it is not reported as a generic DatabaseUnavailableError.

Two existing ordering assertions became index-based casualties of inserting a statement — they now locate PRAGMA key rather than assuming position 0, which is what they actually meant.

Cannot be tested here: that a real SQLCipher build returns a version string. That is the device half of #130.

Notes for review

PaulgSmith and others added 8 commits August 13, 2026 02:32
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
Two gaps between what 0015 decided and what the code does.

The first is the case 0015 designs for. It keeps the key
WHEN_UNLOCKED_THIS_DEVICE_ONLY precisely so it does not travel in an
iCloud backup - but journal.db lives in Documents and does. Restoring a
backup onto a new phone, which is what people do when they replace a
handset, brings the file back without the key.

Nothing handled that. SecureStore returns null both for "nothing stored
yet" and for "the entry was invalidated", key.ts read null as a first run
and minted a fresh key, and database.ts asked for the key before opening
the file so it could not know better. The result was a generic "Could not
open the journal database" on that launch and every launch after, with no
explanation and nowhere to go.

getOrCreateDatabaseKey now reports whether it minted the key, and
database.ts uses that: a key we just created cannot fail to read a file we
just created, so if the decrypt probe fails the file was already there and
belonged to a key that is gone. That is now UnrecoverableJournalError
rather than a generic failure, and the useless key is deleted so the next
launch reaches the same branch instead of degrading to something
undiagnosable. The journal really is unrecoverable - 0007 commits to
saying so during onboarding - but "made on a different phone" is a true
thing to say and leaves the user somewhere to go. The UI for starting over
is not built; this is the mechanism it needs.

The second gap is that the unlock gate locked the view and nothing else.
database.ts held a decrypted handle and the key stayed in memory for the
life of the process, which made WHEN_UNLOCKED_THIS_DEVICE_ONLY a property
of the first launch and nothing after it, because the keychain was never
asked again. The gate now closes the database on the same background
event that re-locks.

Also corrects two comments in key.ts that were wrong rather than merely
thin. keychainAccessible is iOS-only, so THIS_DEVICE_ONLY keeps the key
out of iCloud but does nothing about Google backup - that comes from the
expo-secure-store config plugin's backup rules, which makes the bare
plugin entry in app.json load-bearing. And WHEN_PASSCODE_SET_THIS_DEVICE_ONLY
is recorded as considered and rejected: it looks like a free upgrade, but
it cannot store a key at all on the devices 0018 is about, and it turns
removing a passcode into a data-loss event.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects in the repeatable-entry layer that #118 and #49 are both built
on. Neither was caught by the tests, and neither is caught by TypeScript.

update() reached its sibling method through `this`, which is only bound
when it is called as repo.update(...). `const { update } = repo` and
`onPress={repo.update}` are both ordinary React and both threw
"TypeError: find is not a function" at runtime. find is now a plain
function above the returned object, so nothing in the repository depends
on how it was called.

create() built its return value from the caller's input rather than from
what it wrote. A field the caller omitted was stored as NULL and returned
as absent, so create() and find() disagreed about the same row; and keys
that were never columns were dropped on insert but echoed back, making
them look saved. It now returns the declared fields and only those, with
omissions turned into the NULL that actually goes to the database.

Nothing consumes createRepository yet, so this is not a regression - but
the cost of fixing it rises with every screen written against the old
behaviour, and #118 is next.

Fixes #134

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HAS_NODE_SQLITE was meant to let the SQLite-backed suites skip on a Node
without node:sqlite. It could never do that: the static import is resolved
before any module code runs, so on Node below 22.5 the module threw at
import time and the try/catch around the guard was never reached. The two
suites errored out instead of skipping, on a version engines still permits
(>=20.19.4). CI runs 22.x, which is exactly why it could sit unnoticed.

The require is now lazy, so the guard evaluates, and createInMemoryDatabase
fails with a message naming the Node version it needs rather than a
resolution error from inside node_modules.

Adds a test for the helper itself. Making the require throw is the only
way to exercise the old-Node path from a new Node, and without it nothing
in CI would notice this regressing back - which is how it got here.

Fixes #133

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`PRAGMA key` cannot tell you whether SQLCipher is there. SQLite ignores
pragmas it does not recognise rather than erroring, so on a plain build the
key statement is accepted and does nothing, the schema read after it
succeeds against a plaintext file, and the app writes an unencrypted
medical journal with no error, no warning, and nothing that distinguishes
it from the encrypted case.

That build is easy to be running. `useSQLCipher` is applied by a config
plugin at prebuild, so Expo Go has never had it, and neither has a stale
ios/ or android/ directory generated before the flag was set. Until #129
lands the README still tells contributors to use Expo Go.

`PRAGMA cipher_version` returns a version string on a SQLCipher build and
no row at all on stock SQLite, which makes it the one cheap way to tell
them apart. It asks the library rather than the database, so it is safe
ahead of the key - and asking first means a build that cannot encrypt
fails against an empty file instead of filling one with plaintext.

0017 refuses an unencrypted fallback on web for exactly this reason: a
silent downgrade makes the privacy promise untrue in the way nobody
notices until it matters. This applies the same rule to a build that lost
SQLCipher, and reports it as its own error rather than a generic open
failure, because the problem is the binary and not the database.

Addresses #130. The other half of that issue - whether a SQLCipher build
can read an unkeyed plaintext catalog.db - still needs a device and is
untouched here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Base automatically changed from feature/address-encrypted-storage-unlock-foundation to feature/encrypted-storage-unlock-foundation August 18, 2026 01:43
@PaulgSmith
PaulgSmith force-pushed the feature/encrypted-storage-unlock-foundation branch from 9edfed6 to 8350d03 Compare August 20, 2026 02:11
Base automatically changed from feature/encrypted-storage-unlock-foundation to main August 20, 2026 02:14
@PaulgSmith

Copy link
Copy Markdown
Collaborator

Looked at rebasing this onto main now that #128 has merged, and I don't think it should be — the fix landed with #128 already, in 37161a2 ("Detect a build with no SQLCipher, and stop destroy racing close"). Rebasing would mean reverting that to reinstate an earlier version of the same idea.

main's open() does the same thing this PR does, by the same mechanism:

async function assertEncrypted(db: SQLiteDatabase): Promise<void> {
  const row = await db.getFirstAsync<{ cipher_version?: string }>('PRAGMA cipher_version');
  if (!row?.cipher_version) {
    throw new DatabaseUnavailableError(
      'This build has no SQLCipher, so the journal would be stored unencrypted. Rebuild with `useSQLCipher` ...'
    );
  }
}

Same discriminator, same absence-is-the-signal reasoning, same remedy named in the message.

The test coverage on main is a superset of the five tests here. It has the refusal, the remedy in the message, the ordering guarantee, and the connection being closed — plus two this PR does not have: that a stored key is neither blamed nor deleted over a build misconfiguration, and an integration test that runs the real open path against real stock SQLite via node:sqlite, so PRAGMA key is accepted-and-ignored for real rather than because a mock was told to say so. That last one is the strongest evidence available off-device for exactly the build #130 describes.

Two differences are real but, I think, not worth reverting main for:

  1. Ordering. This PR checks before PRAGMA key; main checks after. The stated rationale here was that a broken build should fail against an empty file rather than one filled with plaintext — but on stock SQLite PRAGMA key is an unrecognised pragma that writes nothing, and openDatabaseAsync has already created the (empty) file by then either way. So the outcomes are identical, and main's order preserves the "key is the first statement to touch the database" invariant, which is the more conservative choice on a build that does have SQLCipher.

  2. Error type. This PR raises a distinct SQLCipherUnavailableError on the argument that the problem is the binary, not the database. main raises DatabaseUnavailableError with a message that names the binary and the remedy. That's a genuine design question and the argument here is decent — but nothing branches on the type today, so it's a refactor rather than a fix. If it's still wanted it's a small standalone PR, and easier to review as one.

So my suggestion is to close this as superseded rather than rebase it. #130 should stay open regardless: this only ever addressed half of it, and the other half — whether a SQLCipher build can read an unkeyed plaintext catalog.db — still needs a device and is untouched by anything on main.

Not closing it myself since it's your PR — say the word, or close it whenever suits.

@PaulgSmith

Copy link
Copy Markdown
Collaborator

Closing as superseded: the fix landed with #128 as commit 37161a2, and main's version is a superset of this one (see the analysis above).

Leaving #130 open — this only ever addressed half of it, and the other half (whether a SQLCipher build can read an unkeyed plaintext catalog.db) still needs a device.

The branch is not deleted, so nothing here is lost if the SQLCipherUnavailableError idea is worth picking up as its own PR later.

@PaulgSmith PaulgSmith closed this Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants