From 451dcaf77454493d162e5efa1c911fa60088a3d2 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Thu, 13 Aug 2026 02:32:43 +0000 Subject: [PATCH 01/13] Add SQLCipher, secure storage, and biometric dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- mobile/app.json | 16 +++++++++++- mobile/package-lock.json | 54 ++++++++++++++++++++++++++++++++++++++++ mobile/package.json | 4 +++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/mobile/app.json b/mobile/app.json index f8f2e9f..0be0e57 100644 --- a/mobile/app.json +++ b/mobile/app.json @@ -32,7 +32,21 @@ "image": "./assets/images/splash-icon.png", "imageWidth": 76 } - ] + ], + [ + "expo-sqlite", + { + "useSQLCipher": true, + "enableFTS": true + } + ], + [ + "expo-local-authentication", + { + "faceIDPermission": "Along With You uses Face ID to unlock your journal." + } + ], + "expo-secure-store" ], "experiments": { "typedRoutes": true, diff --git a/mobile/package-lock.json b/mobile/package-lock.json index 1cccc71..fe381d5 100644 --- a/mobile/package-lock.json +++ b/mobile/package-lock.json @@ -21,13 +21,17 @@ "clsx": "^2.1.1", "expo": "~57.0.14", "expo-constants": "~57.0.8", + "expo-crypto": "~57.0.1", "expo-device": "~57.0.1", "expo-font": "~57.0.1", "expo-glass-effect": "~57.0.1", "expo-image": "~57.0.3", "expo-linking": "~57.0.6", + "expo-local-authentication": "~57.0.2", "expo-router": "~57.0.13", + "expo-secure-store": "~57.0.1", "expo-splash-screen": "~57.0.6", + "expo-sqlite": "~57.0.1", "expo-status-bar": "~57.0.1", "expo-symbols": "~57.0.1", "expo-system-ui": "~57.0.2", @@ -5676,6 +5680,12 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/await-lock": { + "version": "2.2.2", + "resolved": "https://registry.npmjs.org/await-lock/-/await-lock-2.2.2.tgz", + "integrity": "sha512-aDczADvlvTGajTDjcjpJMqRkOF6Qdz3YbPZm/PyW6tKPkx2hlYBzxMhEywM/tU72HrVZjgl5VCdRuMlA7pZ8Gw==", + "license": "MIT" + }, "node_modules/babel-jest": { "version": "29.7.0", "resolved": "https://registry.npmjs.org/babel-jest/-/babel-jest-29.7.0.tgz", @@ -8168,6 +8178,15 @@ "react-native": "*" } }, + "node_modules/expo-crypto": { + "version": "57.0.1", + "resolved": "https://registry.npmjs.org/expo-crypto/-/expo-crypto-57.0.1.tgz", + "integrity": "sha512-xwegXQw3ATgeL1ZuqbSNrGzOeG+zNeh6Z6DSJk825Qpa3TEQQ1kG3ioE1p3g/SNF373BAVz2iBKUTSytlIbBRA==", + "license": "MIT", + "peerDependencies": { + "expo": "*" + } + }, "node_modules/expo-device": { "version": "57.0.1", "resolved": "https://registry.npmjs.org/expo-device/-/expo-device-57.0.1.tgz", @@ -8259,6 +8278,18 @@ "react-native": "*" } }, + "node_modules/expo-local-authentication": { + "version": "57.0.2", + "resolved": "https://registry.npmjs.org/expo-local-authentication/-/expo-local-authentication-57.0.2.tgz", + "integrity": "sha512-8K4zcrQ5wZkRS1rwEWY8qHbeGVMNh35e9D1VTVKlMHz1O47itW+pW6iBVuqwGFLozN4fRBeEDQH8hu9Gt58YuQ==", + "license": "MIT", + "dependencies": { + "invariant": "^2.2.4" + }, + "peerDependencies": { + "expo": "*" + } + }, "node_modules/expo-modules-autolinking": { "version": "57.0.10", "resolved": "https://registry.npmjs.org/expo-modules-autolinking/-/expo-modules-autolinking-57.0.10.tgz", @@ -8378,6 +8409,15 @@ } } }, + "node_modules/expo-secure-store": { + "version": "57.0.1", + "resolved": "https://registry.npmjs.org/expo-secure-store/-/expo-secure-store-57.0.1.tgz", + "integrity": "sha512-tLa1VmSadOq19mA/dwkl99RbHyjLE0T1qqBYMY3/OsguZTI+rlrDy/DDJjupqlVtmr95hD7o1pYqx5aL+B4YMA==", + "license": "MIT", + "peerDependencies": { + "expo": "*" + } + }, "node_modules/expo-server": { "version": "57.0.3", "resolved": "https://registry.npmjs.org/expo-server/-/expo-server-57.0.3.tgz", @@ -8401,6 +8441,20 @@ "expo": "*" } }, + "node_modules/expo-sqlite": { + "version": "57.0.1", + "resolved": "https://registry.npmjs.org/expo-sqlite/-/expo-sqlite-57.0.1.tgz", + "integrity": "sha512-I6KoUvfGIiROTKxr5D3H+jRIGA/iEvWEtqHK4XMukAA7tVTinz/YdS8zOz6/DdG6vgrNmvF7gyOcbhLinlfxzQ==", + "license": "MIT", + "dependencies": { + "await-lock": "^2.2.2" + }, + "peerDependencies": { + "expo": "*", + "react": "*", + "react-native": "*" + } + }, "node_modules/expo-status-bar": { "version": "57.0.1", "resolved": "https://registry.npmjs.org/expo-status-bar/-/expo-status-bar-57.0.1.tgz", diff --git a/mobile/package.json b/mobile/package.json index 31c7129..2d8ac44 100644 --- a/mobile/package.json +++ b/mobile/package.json @@ -19,13 +19,17 @@ "clsx": "^2.1.1", "expo": "~57.0.14", "expo-constants": "~57.0.8", + "expo-crypto": "~57.0.1", "expo-device": "~57.0.1", "expo-font": "~57.0.1", "expo-glass-effect": "~57.0.1", "expo-image": "~57.0.3", "expo-linking": "~57.0.6", + "expo-local-authentication": "~57.0.2", "expo-router": "~57.0.13", + "expo-secure-store": "~57.0.1", "expo-splash-screen": "~57.0.6", + "expo-sqlite": "~57.0.1", "expo-status-bar": "~57.0.1", "expo-symbols": "~57.0.1", "expo-system-ui": "~57.0.2", From a70ed2e176463030265e17b310d2871530dfcbb2 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Thu, 13 Aug 2026 02:32:56 +0000 Subject: [PATCH 02/13] Add encrypted storage and biometric unlock foundation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- mobile/jest/in-memory-sqlite.ts | 80 ++++++++ mobile/jest/node-sqlite.d.ts | 32 +++ mobile/src/app/_layout.tsx | 8 +- mobile/src/components/unlock-gate.test.tsx | 177 +++++++++++++++++ mobile/src/components/unlock-gate.tsx | 167 ++++++++++++++++ mobile/src/lib/auth/unlock.test.ts | 104 ++++++++++ mobile/src/lib/auth/unlock.ts | 98 ++++++++++ mobile/src/lib/db/database.test.ts | 165 ++++++++++++++++ mobile/src/lib/db/database.ts | 105 ++++++++++ mobile/src/lib/db/key.test.ts | 103 ++++++++++ mobile/src/lib/db/key.ts | 135 +++++++++++++ mobile/src/lib/db/migrations.test.ts | 110 +++++++++++ mobile/src/lib/db/migrations.ts | 104 ++++++++++ mobile/src/lib/db/repository.test.ts | 217 +++++++++++++++++++++ mobile/src/lib/db/repository.ts | 188 ++++++++++++++++++ 15 files changed, 1792 insertions(+), 1 deletion(-) create mode 100644 mobile/jest/in-memory-sqlite.ts create mode 100644 mobile/jest/node-sqlite.d.ts create mode 100644 mobile/src/components/unlock-gate.test.tsx create mode 100644 mobile/src/components/unlock-gate.tsx create mode 100644 mobile/src/lib/auth/unlock.test.ts create mode 100644 mobile/src/lib/auth/unlock.ts create mode 100644 mobile/src/lib/db/database.test.ts create mode 100644 mobile/src/lib/db/database.ts create mode 100644 mobile/src/lib/db/key.test.ts create mode 100644 mobile/src/lib/db/key.ts create mode 100644 mobile/src/lib/db/migrations.test.ts create mode 100644 mobile/src/lib/db/migrations.ts create mode 100644 mobile/src/lib/db/repository.test.ts create mode 100644 mobile/src/lib/db/repository.ts diff --git a/mobile/jest/in-memory-sqlite.ts b/mobile/jest/in-memory-sqlite.ts new file mode 100644 index 0000000..8498874 --- /dev/null +++ b/mobile/jest/in-memory-sqlite.ts @@ -0,0 +1,80 @@ +// An in-memory stand-in for an open expo-sqlite database, for tests. +// +// The alternative was asserting on generated SQL strings, which passes happily +// while the query is wrong. This runs the real thing against real SQLite - +// node:sqlite, built into Node - so ordering, constraints, transaction rollback +// and `changes` counts are the genuine article rather than a fake's opinion. +// +// It covers only the handful of SQLiteDatabase methods this codebase calls. It +// is not, and should not grow into, a general expo-sqlite polyfill: what it +// cannot tell you is anything about SQLCipher, which has no Node build and +// needs a device. Encryption is verified on hardware (issue #101), not here. + +import { DatabaseSync } from 'node:sqlite'; +import type { SQLiteDatabase } from 'expo-sqlite'; + +/** True when the running Node has node:sqlite (22.5+). */ +export const HAS_NODE_SQLITE = (() => { + try { + return typeof DatabaseSync === 'function'; + } catch { + return false; + } +})(); + +/** + * expo-sqlite accepts either `run(sql, a, b)` or `run(sql, [a, b])`, and this + * codebase uses both. Collapse them into one array. + */ +function normaliseParams(params: unknown[]): unknown[] { + if (params.length === 1 && Array.isArray(params[0])) return params[0] as unknown[]; + return params; +} + +export type InMemoryDatabase = SQLiteDatabase & { readonly raw: DatabaseSync }; + +export function createInMemoryDatabase(): InMemoryDatabase { + const raw = new DatabaseSync(':memory:'); + + const db = { + raw, + + async execAsync(sql: string) { + raw.exec(sql); + }, + + async runAsync(sql: string, ...params: unknown[]) { + const result = raw.prepare(sql).run(...(normaliseParams(params) as never[])); + return { + lastInsertRowId: Number(result.lastInsertRowid), + changes: Number(result.changes), + }; + }, + + async getAllAsync(sql: string, ...params: unknown[]) { + return raw.prepare(sql).all(...(normaliseParams(params) as never[])); + }, + + async getFirstAsync(sql: string, ...params: unknown[]) { + // node:sqlite returns undefined for no rows; expo-sqlite returns null. + return raw.prepare(sql).get(...(normaliseParams(params) as never[])) ?? null; + }, + + async withExclusiveTransactionAsync(task: (txn: unknown) => Promise) { + raw.exec('BEGIN EXCLUSIVE'); + try { + await task(db); + raw.exec('COMMIT'); + } catch (error) { + raw.exec('ROLLBACK'); + throw error; + } + }, + + async closeAsync() { + raw.close(); + }, + }; + + return db as unknown as InMemoryDatabase; +} diff --git a/mobile/jest/node-sqlite.d.ts b/mobile/jest/node-sqlite.d.ts new file mode 100644 index 0000000..51a4359 --- /dev/null +++ b/mobile/jest/node-sqlite.d.ts @@ -0,0 +1,32 @@ +// A minimal declaration for the part of node:sqlite that jest/in-memory-sqlite.ts +// uses. +// +// @types/node ships its own, but nothing loads it here: Expo's tsconfig sets +// moduleResolution "bundler", which skips `node:`-prefixed specifiers as though +// they were URIs unless "node" is listed in compilerOptions.types. Adding that +// field would turn off the automatic inclusion of every other @types package - +// jest's globals among them - so this declares the handful of members in play +// instead and leaves the project's type configuration alone. +// +// If a future tsconfig does add "node" to types, delete this file; the two +// declarations would collide. + +declare module 'node:sqlite' { + export interface StatementResultingChanges { + changes: number | bigint; + lastInsertRowid: number | bigint; + } + + export class StatementSync { + run(...params: unknown[]): StatementResultingChanges; + all(...params: unknown[]): unknown[]; + get(...params: unknown[]): unknown; + } + + export class DatabaseSync { + constructor(path: string); + exec(sql: string): void; + prepare(sql: string): StatementSync; + close(): void; + } +} diff --git a/mobile/src/app/_layout.tsx b/mobile/src/app/_layout.tsx index 0f852e9..251baf4 100644 --- a/mobile/src/app/_layout.tsx +++ b/mobile/src/app/_layout.tsx @@ -10,6 +10,7 @@ import { useColorScheme } from 'react-native'; import { AnimatedSplashOverlay } from '@/components/animated-icon'; import AppTabs from '@/components/app-tabs'; +import { UnlockGate } from '@/components/unlock-gate'; import { NAV_THEME } from '@/lib/theme'; SplashScreen.preventAutoHideAsync(); @@ -19,7 +20,12 @@ export default function TabLayout() { return ( - + {/* Inside ThemeProvider so the lock screen is themed like everything + else, and around AppTabs rather than around the whole tree so it + gates the app without gating the splash overlay above it. */} + + + {/* Where everything rendered through a Portal ends up - dialogs, dropdown menus, tooltips, popovers. It has to be the last child of the providers to sit on top of the rest of the tree. diff --git a/mobile/src/components/unlock-gate.test.tsx b/mobile/src/components/unlock-gate.test.tsx new file mode 100644 index 0000000..62b5bb2 --- /dev/null +++ b/mobile/src/components/unlock-gate.test.tsx @@ -0,0 +1,177 @@ +import { act, render, screen, userEvent, waitFor } from '@testing-library/react-native'; +import { AppState, Platform, Text as RNText } from 'react-native'; + +import { UnlockGate } from './unlock-gate'; +import { checkUnlockAvailability, requestUnlock } from '@/lib/auth/unlock'; + +jest.mock('@/lib/auth/unlock', () => ({ + checkUnlockAvailability: jest.fn(), + requestUnlock: jest.fn(), +})); + +const availability = checkUnlockAvailability as jest.Mock; +const unlock = requestUnlock as jest.Mock; + +const originalOS = Platform.OS; +function setPlatform(os: typeof Platform.OS) { + (Platform as { OS: typeof Platform.OS }).OS = os; +} + +function Journal() { + return Journal contents; +} + +beforeEach(() => { + jest.clearAllMocks(); + setPlatform('ios'); + availability.mockResolvedValue({ kind: 'biometric' }); + unlock.mockResolvedValue({ status: 'unlocked' }); +}); + +afterEach(() => setPlatform(originalOS)); + +describe('UnlockGate', () => { + it('shows nothing of what it wraps until the device says yes', async () => { + let release: (value: { status: string }) => void = () => {}; + unlock.mockReturnValue( + new Promise<{ status: string }>((resolve) => { + release = resolve; + }) + ); + + await render( + + + + ); + + expect(screen.queryByText('Journal contents')).toBeNull(); + + await act(async () => release({ status: 'unlocked' })); + expect(await screen.findByText('Journal contents')).toBeVisible(); + }); + + it('renders its children once unlocked', async () => { + await render( + + + + ); + + expect(await screen.findByText('Journal contents')).toBeVisible(); + }); + + it('offers a way back in after the prompt is dismissed', async () => { + const user = userEvent.setup(); + unlock.mockResolvedValueOnce({ status: 'cancelled' }); + + await render( + + + + ); + + expect(await screen.findByText('Your journal is locked')).toBeVisible(); + expect(screen.queryByText('Journal contents')).toBeNull(); + + unlock.mockResolvedValueOnce({ status: 'unlocked' }); + await user.press(screen.getByRole('button', { name: 'Unlock' })); + + expect(await screen.findByText('Journal contents')).toBeVisible(); + }); + + it('does not blame the user when authentication fails', async () => { + unlock.mockResolvedValue({ status: 'failed', reason: 'authentication_failed' }); + + await render( + + + + ); + + const message = await screen.findByText("That didn't work. You can try again."); + expect(message).toBeVisible(); + // Screen readers should hear it, not just sighted users see it. + expect(message.props.accessibilityLiveRegion).toBe('polite'); + }); + + it('explains a lockout rather than repeating a generic failure', async () => { + unlock.mockResolvedValue({ status: 'locked-out' }); + + await render( + + + + ); + + expect(await screen.findByText(/try again in a moment/)).toBeVisible(); + }); + + it('lets someone through on a phone with no lock, and says so', async () => { + const user = userEvent.setup(); + availability.mockResolvedValue({ kind: 'none' }); + + await render( + + + + ); + + expect(await screen.findByText('Your phone has no lock set')).toBeVisible(); + expect(unlock).not.toHaveBeenCalled(); + + await user.press(screen.getByRole('button', { name: 'Continue' })); + expect(await screen.findByText('Journal contents')).toBeVisible(); + }); + + it('re-locks when the app goes to the background', async () => { + const listeners: ((state: string) => void)[] = []; + jest.spyOn(AppState, 'addEventListener').mockImplementation((( + _event: string, + handler: (state: string) => void + ) => { + listeners.push(handler); + return { remove: jest.fn() }; + }) as never); + + await render( + + + + ); + + expect(await screen.findByText('Journal contents')).toBeVisible(); + + // Without this the gate is a splash screen, not a lock. + await act(async () => listeners.forEach((handler) => handler('background'))); + + await waitFor(() => expect(screen.queryByText('Journal contents')).toBeNull()); + expect(screen.getByText('Your journal is locked')).toBeVisible(); + + jest.restoreAllMocks(); + }); + + it('does not gate the web build, which holds no journal data', async () => { + setPlatform('web'); + + await render( + + + + ); + + expect(screen.getByText('Journal contents')).toBeVisible(); + expect(availability).not.toHaveBeenCalled(); + }); + + it('can be skipped explicitly', async () => { + await render( + + + + ); + + expect(screen.getByText('Journal contents')).toBeVisible(); + expect(availability).not.toHaveBeenCalled(); + }); +}); diff --git a/mobile/src/components/unlock-gate.tsx b/mobile/src/components/unlock-gate.tsx new file mode 100644 index 0000000..ba53a1f --- /dev/null +++ b/mobile/src/components/unlock-gate.tsx @@ -0,0 +1,167 @@ +// The app shell's lock screen. +// +// 0007 asks for the journal to sit behind the device's own biometric, with the +// OS passcode as the fallback. This is that gate: nothing it wraps renders +// until the device has said yes. +// +// On the copy: someone opening this app may be having a hard day, and a lock +// screen is a bad place to be brusque. It explains rather than demands, a +// refusal is never phrased as the user's fault, and "Not now" is a real option +// rather than a dead end. + +import { useCallback, useEffect, useRef, useState } from 'react'; +import type { ReactNode } from 'react'; +import { ActivityIndicator, AppState, Platform, View } from 'react-native'; + +import { Button } from '@/components/ui/button'; +import { Text } from '@/components/ui/text'; +import { checkUnlockAvailability, requestUnlock } from '@/lib/auth/unlock'; + +type GateState = + | { status: 'checking' } + | { status: 'locked'; message: string | null } + | { status: 'unlocked' } + /** Native device with no lock configured - see the note in unlock.ts. */ + | { status: 'no-device-lock' }; + +export type UnlockGateProps = { + children: ReactNode; + /** Escape hatch for tests and Storybook-style previews. */ + skip?: boolean; +}; + +export function UnlockGate({ children, skip = false }: UnlockGateProps) { + // Web has no biometric API and, per db/database.ts, no journal data either - + // it is the landing surface. Gating it would lock people out of a marketing + // page to protect nothing. + const passthrough = skip || Platform.OS === 'web'; + + const [state, setState] = useState( + passthrough ? { status: 'unlocked' } : { status: 'checking' } + ); + + // Guards against setting state after unmount, and against two prompts racing + // if the user backgrounds the app mid-authentication. + const mounted = useRef(true); + const prompting = useRef(false); + + useEffect(() => { + mounted.current = true; + return () => { + mounted.current = false; + }; + }, []); + + const attemptUnlock = useCallback(async () => { + if (passthrough || prompting.current) return; + prompting.current = true; + + try { + const availability = await checkUnlockAvailability(); + if (!mounted.current) return; + + if (availability.kind === 'unsupported' || availability.kind === 'none') { + setState({ status: 'no-device-lock' }); + return; + } + + const outcome = await requestUnlock(); + if (!mounted.current) return; + + switch (outcome.status) { + case 'unlocked': + setState({ status: 'unlocked' }); + break; + case 'cancelled': + setState({ status: 'locked', message: null }); + break; + case 'locked-out': + setState({ + status: 'locked', + message: 'Too many attempts. Your phone will let you try again in a moment.', + }); + break; + default: + setState({ + status: 'locked', + message: "That didn't work. You can try again.", + }); + } + } finally { + prompting.current = false; + } + }, [passthrough]); + + useEffect(() => { + void attemptUnlock(); + }, [attemptUnlock]); + + // Re-lock when the app leaves the foreground. Without this the gate is a + // one-time splash rather than a lock: hand someone an already-open phone and + // the journal is simply there. + useEffect(() => { + if (passthrough) return; + + const subscription = AppState.addEventListener('change', (next) => { + if (next === 'background') { + setState({ status: 'locked', message: null }); + } + }); + + return () => subscription.remove(); + }, [passthrough]); + + if (state.status === 'unlocked') return <>{children}; + + if (state.status === 'checking') { + return ( + + + + ); + } + + return ( + + {state.status === 'no-device-lock' ? ( + <> + Your phone has no lock set + + Your journal is still encrypted on this phone. Setting a passcode or fingerprint in + your phone's settings adds another layer, and we'd recommend it. + + + + ) : ( + <> + Your journal is locked + + Unlock with your phone to open it. + + {state.message ? ( + // Announced by a screen reader when it appears, rather than only + // being visible to someone looking at the screen. + + {state.message} + + ) : null} + + + )} + + ); +} diff --git a/mobile/src/lib/auth/unlock.test.ts b/mobile/src/lib/auth/unlock.test.ts new file mode 100644 index 0000000..e294a3e --- /dev/null +++ b/mobile/src/lib/auth/unlock.test.ts @@ -0,0 +1,104 @@ +import * as LocalAuthentication from 'expo-local-authentication'; +import { Platform } from 'react-native'; + +import { checkUnlockAvailability, requestUnlock } from './unlock'; + +jest.mock('expo-local-authentication', () => ({ + hasHardwareAsync: jest.fn(), + getEnrolledLevelAsync: jest.fn(), + authenticateAsync: jest.fn(), + SecurityLevel: { NONE: 0, SECRET: 1, BIOMETRIC_WEAK: 2, BIOMETRIC_STRONG: 3 }, +})); + +const hasHardwareAsync = LocalAuthentication.hasHardwareAsync as jest.Mock; +const getEnrolledLevelAsync = LocalAuthentication.getEnrolledLevelAsync as jest.Mock; +const authenticateAsync = LocalAuthentication.authenticateAsync as jest.Mock; + +const originalOS = Platform.OS; +function setPlatform(os: typeof Platform.OS) { + (Platform as { OS: typeof Platform.OS }).OS = os; +} + +beforeEach(() => { + jest.clearAllMocks(); + setPlatform('ios'); + hasHardwareAsync.mockResolvedValue(true); + getEnrolledLevelAsync.mockResolvedValue(LocalAuthentication.SecurityLevel.BIOMETRIC_STRONG); + authenticateAsync.mockResolvedValue({ success: true }); +}); + +afterAll(() => setPlatform(originalOS)); + +describe('checkUnlockAvailability', () => { + it('reports biometric when one is enrolled', async () => { + await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'biometric' }); + }); + + it('reports biometric for a weak biometric too', async () => { + getEnrolledLevelAsync.mockResolvedValue(LocalAuthentication.SecurityLevel.BIOMETRIC_WEAK); + await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'biometric' }); + }); + + it('reports passcode when only a PIN or pattern is set', async () => { + getEnrolledLevelAsync.mockResolvedValue(LocalAuthentication.SecurityLevel.SECRET); + await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'passcode' }); + }); + + it('distinguishes a device with no lock set from one that cannot lock at all', async () => { + getEnrolledLevelAsync.mockResolvedValue(LocalAuthentication.SecurityLevel.NONE); + + hasHardwareAsync.mockResolvedValue(true); + await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'none' }); + + hasHardwareAsync.mockResolvedValue(false); + await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'unsupported' }); + }); + + it('is unsupported on web', async () => { + setPlatform('web'); + await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'unsupported' }); + expect(hasHardwareAsync).not.toHaveBeenCalled(); + }); +}); + +describe('requestUnlock', () => { + it('unlocks on success', async () => { + await expect(requestUnlock()).resolves.toEqual({ status: 'unlocked' }); + }); + + it('leaves the OS passcode fallback enabled, as 0007 requires', async () => { + await requestUnlock(); + + const [options] = authenticateAsync.mock.calls[0]; + expect(options.disableDeviceFallback).toBeUndefined(); + expect(options.promptMessage).toBeTruthy(); + }); + + it.each(['user_cancel', 'app_cancel', 'system_cancel'])( + 'treats %s as a cancellation rather than an error', + async (error) => { + authenticateAsync.mockResolvedValue({ success: false, error }); + await expect(requestUnlock()).resolves.toEqual({ status: 'cancelled' }); + } + ); + + it('reports lockout separately so the UI can say to wait', async () => { + authenticateAsync.mockResolvedValue({ success: false, error: 'lockout' }); + await expect(requestUnlock()).resolves.toEqual({ status: 'locked-out' }); + }); + + it('reports any other error as a failure, keeping the reason', async () => { + authenticateAsync.mockResolvedValue({ success: false, error: 'authentication_failed' }); + await expect(requestUnlock()).resolves.toEqual({ + status: 'failed', + reason: 'authentication_failed', + }); + }); + + it('does not prompt when the device has nothing to prompt with', async () => { + getEnrolledLevelAsync.mockResolvedValue(LocalAuthentication.SecurityLevel.NONE); + + await expect(requestUnlock()).resolves.toEqual({ status: 'failed', reason: 'none' }); + expect(authenticateAsync).not.toHaveBeenCalled(); + }); +}); diff --git a/mobile/src/lib/auth/unlock.ts b/mobile/src/lib/auth/unlock.ts new file mode 100644 index 0000000..510ab7e --- /dev/null +++ b/mobile/src/lib/auth/unlock.ts @@ -0,0 +1,98 @@ +// The app lock. +// +// 0007 decided this: no username, no password, no in-app PIN of our own - the +// device's own biometric with the OS's passcode fallback, because it is a +// mechanism people already understand from every other app on their phone, and +// because "keep technology in a supporting role" rules out inventing a second +// one. +// +// Note what this is and is not. It gates the *app*, not the database key (see +// the long comment in db/key.ts for why those are deliberately separate). A +// locked app with an extractable key is a weaker guarantee than binding the key +// to biometric enrollment, and that is a trade taken knowingly there. + +import * as LocalAuthentication from 'expo-local-authentication'; +import { Platform } from 'react-native'; + +export type UnlockAvailability = + /** A biometric is enrolled; the OS will offer the passcode as a fallback. */ + | { readonly kind: 'biometric' } + /** No biometric, but a device passcode/pattern is set and can be prompted. */ + | { readonly kind: 'passcode' } + /** + * The device has no lock at all. There is nothing for us to prompt, and the + * data is protected only by the fact that the file is encrypted with a key + * sitting in an unlocked keystore. + * + * 0007 does not say what to do here, because it was written about lockout + * rather than about a device with no lock screen. Someone needs to decide + * whether the app refuses to open, opens with a plain warning, or asks the + * user to set a device passcode - and that is a product call, so this type + * surfaces the case honestly rather than picking one silently. + */ + | { readonly kind: 'none' } + /** No supported hardware or platform - the web build, mainly. */ + | { readonly kind: 'unsupported' }; + +export type UnlockOutcome = + | { readonly status: 'unlocked' } + /** The user dismissed the prompt. Not an error; do not show one. */ + | { readonly status: 'cancelled' } + /** Too many failed attempts; the OS has temporarily disabled the prompt. */ + | { readonly status: 'locked-out' } + | { readonly status: 'failed'; readonly reason: string }; + +/** What kind of lock, if any, this device can offer us. */ +export async function checkUnlockAvailability(): Promise { + if (Platform.OS === 'web') return { kind: 'unsupported' }; + + const hasHardware = await LocalAuthentication.hasHardwareAsync(); + const level = await LocalAuthentication.getEnrolledLevelAsync(); + + if (level === LocalAuthentication.SecurityLevel.NONE) { + // No biometric *and* no passcode. Distinguish "this phone could never do + // it" from "this phone could, but nothing is set up" - only the second is + // something the user can fix from Settings. + return hasHardware ? { kind: 'none' } : { kind: 'unsupported' }; + } + + if (level === LocalAuthentication.SecurityLevel.SECRET) return { kind: 'passcode' }; + + return { kind: 'biometric' }; +} + +/** + * Prompts for unlock. + * + * `disableDeviceFallback` is left at its default of false on purpose: that is + * what maps to iOS's DeviceOwnerAuthentication policy and gives the passcode + * fallback 0007 asks for. Setting it true would mean building our own fallback, + * which 0007 explicitly rejected. + */ +export async function requestUnlock(): Promise { + const availability = await checkUnlockAvailability(); + + if (availability.kind === 'unsupported' || availability.kind === 'none') { + return { status: 'failed', reason: availability.kind }; + } + + const result = await LocalAuthentication.authenticateAsync({ + promptMessage: 'Unlock your journal', + // Plain, and not a threat. Someone opening this app may be having a bad + // day already. + cancelLabel: 'Not now', + }); + + if (result.success) return { status: 'unlocked' }; + + switch (result.error) { + case 'user_cancel': + case 'app_cancel': + case 'system_cancel': + return { status: 'cancelled' }; + case 'lockout': + return { status: 'locked-out' }; + default: + return { status: 'failed', reason: result.error }; + } +} diff --git a/mobile/src/lib/db/database.test.ts b/mobile/src/lib/db/database.test.ts new file mode 100644 index 0000000..9807630 --- /dev/null +++ b/mobile/src/lib/db/database.test.ts @@ -0,0 +1,165 @@ +import * as SQLite from 'expo-sqlite'; +import { Platform } from 'react-native'; + +import { + closeJournalDatabase, + DatabaseUnavailableError, + destroyJournalDatabase, + getJournalDatabase, +} from './database'; +import { deleteDatabaseKey, getOrCreateDatabaseKey } from './key'; +import { migrate } from './migrations'; + +jest.mock('expo-sqlite', () => ({ + openDatabaseAsync: jest.fn(), + deleteDatabaseAsync: jest.fn(), +})); +jest.mock('./key', () => ({ + getOrCreateDatabaseKey: jest.fn(), + deleteDatabaseKey: jest.fn(), + rawKeyPragma: (hex: string) => `PRAGMA key = "x'${hex}'"`, +})); +jest.mock('./migrations', () => ({ migrate: jest.fn() })); + +const openDatabaseAsync = SQLite.openDatabaseAsync as jest.Mock; +const deleteDatabaseAsync = SQLite.deleteDatabaseAsync as jest.Mock; +const getKey = getOrCreateDatabaseKey as jest.Mock; +const deleteKey = deleteDatabaseKey as jest.Mock; +const migrateMock = migrate as jest.Mock; + +const KEY = 'a'.repeat(64); + +const originalOS = Platform.OS; +function setPlatform(os: typeof Platform.OS) { + (Platform as { OS: typeof Platform.OS }).OS = os; +} + +/** Records the statements run against it, in order. */ +function fakeDatabase() { + const statements: string[] = []; + return { + statements, + execAsync: jest.fn(async (sql: string) => { + statements.push(sql); + }), + getFirstAsync: jest.fn(async (sql: string) => { + statements.push(sql); + return { 'count(*)': 0 }; + }), + closeAsync: jest.fn(async () => undefined), + }; +} + +let db: ReturnType; + +beforeEach(async () => { + jest.clearAllMocks(); + setPlatform('ios'); + db = fakeDatabase(); + openDatabaseAsync.mockResolvedValue(db); + getKey.mockResolvedValue(KEY); + migrateMock.mockResolvedValue(0); + deleteDatabaseAsync.mockResolvedValue(undefined); +}); + +afterEach(async () => { + await closeJournalDatabase().catch(() => undefined); + setPlatform(originalOS); +}); + +describe('getJournalDatabase', () => { + it('keys the connection before doing anything else with it', async () => { + await getJournalDatabase(); + + // If any statement precedes the key, SQLCipher has already decided the file + // is not a database and the rest is noise. + expect(db.statements[0]).toBe(`PRAGMA key = "x'${KEY}'"`); + }); + + it('forces a decrypt before handing the connection out', async () => { + await getJournalDatabase(); + expect(db.statements[1]).toMatch(/FROM sqlite_master/); + }); + + it('sets WAL and foreign keys, then migrates', async () => { + await getJournalDatabase(); + + expect(db.statements).toEqual( + expect.arrayContaining(['PRAGMA journal_mode = WAL', 'PRAGMA foreign_keys = ON']) + ); + expect(migrateMock).toHaveBeenCalledWith(db); + }); + + it('opens once however many callers ask', async () => { + const [first, second] = await Promise.all([getJournalDatabase(), getJournalDatabase()]); + + expect(first).toBe(second); + expect(openDatabaseAsync).toHaveBeenCalledTimes(1); + }); + + it('refuses to run on web rather than falling back to an unencrypted database', async () => { + setPlatform('web'); + + await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + expect(openDatabaseAsync).not.toHaveBeenCalled(); + }); + + it('closes the handle and allows a retry when opening fails', async () => { + migrateMock.mockRejectedValueOnce(new Error('migration exploded')); + + await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + expect(db.closeAsync).toHaveBeenCalled(); + + // The failed attempt must not be cached, or the app can never recover + // without a restart. + migrateMock.mockResolvedValue(0); + await expect(getJournalDatabase()).resolves.toBeDefined(); + expect(openDatabaseAsync).toHaveBeenCalledTimes(2); + }); + + it('surfaces a key that cannot be read', async () => { + getKey.mockRejectedValue(new Error('keychain locked')); + await expect(getJournalDatabase()).rejects.toThrow(); + expect(openDatabaseAsync).not.toHaveBeenCalled(); + }); +}); + +describe('closeJournalDatabase', () => { + it('closes an open handle', async () => { + await getJournalDatabase(); + await closeJournalDatabase(); + expect(db.closeAsync).toHaveBeenCalledTimes(1); + }); + + it('does nothing when nothing is open', async () => { + await expect(closeJournalDatabase()).resolves.toBeUndefined(); + expect(db.closeAsync).not.toHaveBeenCalled(); + }); +}); + +describe('destroyJournalDatabase', () => { + it('deletes the database file before the key', async () => { + const order: string[] = []; + deleteDatabaseAsync.mockImplementation(async () => { + order.push('file'); + }); + deleteKey.mockImplementation(async () => { + order.push('key'); + }); + + await getJournalDatabase(); + await destroyJournalDatabase(); + + // Interrupted between the two, this order leaves an unreadable file rather + // than a readable one whose key is gone. + expect(order).toEqual(['file', 'key']); + expect(db.closeAsync).toHaveBeenCalled(); + }); + + it('still removes the key when the file was already gone', async () => { + deleteDatabaseAsync.mockRejectedValue(new Error('no such file')); + + await expect(destroyJournalDatabase()).resolves.toBeUndefined(); + expect(deleteKey).toHaveBeenCalled(); + }); +}); diff --git a/mobile/src/lib/db/database.ts b/mobile/src/lib/db/database.ts new file mode 100644 index 0000000..402a9a5 --- /dev/null +++ b/mobile/src/lib/db/database.ts @@ -0,0 +1,105 @@ +// Opening the encrypted journal database. +// +// Everything a user writes lives here and nowhere else (0001), encrypted at +// rest with SQLCipher under a key from key.ts. +// +// Requires a development build. SQLCipher is a native fork of SQLite that +// expo-sqlite compiles in only when `useSQLCipher` is set in app.json, so it +// cannot work in Expo Go and the project needs `npx expo prebuild` and a dev +// client. That is a real change to how you run the app locally, and it is the +// price of the database being encrypted at all. + +import * as SQLite from 'expo-sqlite'; +import type { SQLiteDatabase } from 'expo-sqlite'; +import { Platform } from 'react-native'; + +import { migrate } from './migrations'; +import { deleteDatabaseKey, getOrCreateDatabaseKey, rawKeyPragma } from './key'; + +const DATABASE_NAME = 'journal.db'; + +export class DatabaseUnavailableError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = 'DatabaseUnavailableError'; + } +} + +/** + * Cached as the in-flight promise, not the resolved handle, so that two screens + * mounting at once share one open rather than racing to create two. Cleared on + * failure so a retry is not permanently poisoned by one bad open. + */ +let openPromise: Promise | null = null; + +async function open(): Promise { + // expo-sqlite runs on web via wasm, but SQLCipher does not: the docs list it + // for Android, iOS and macOS only. Falling back to plain SQLite here would + // give us a web build quietly writing an unencrypted medical journal to + // origin-private storage, which is exactly the kind of silent downgrade that + // makes a privacy promise untrue. Web is the marketing and landing surface + // for this app; journal data does not belong there. + if (Platform.OS === 'web') { + throw new DatabaseUnavailableError( + 'The journal database is not available on web: SQLCipher has no web build, and an unencrypted fallback is not acceptable for this data.' + ); + } + + const key = await getOrCreateDatabaseKey(); + const db = await SQLite.openDatabaseAsync(DATABASE_NAME); + + try { + // Has to be the first statement executed against the connection. + await db.execAsync(rawKeyPragma(key)); + + // 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. + await db.getFirstAsync('SELECT count(*) FROM sqlite_master'); + + await db.execAsync('PRAGMA journal_mode = WAL'); + await db.execAsync('PRAGMA foreign_keys = ON'); + + await migrate(db); + } catch (cause) { + // Do not leave a half-configured handle behind for the next caller. + await db.closeAsync().catch(() => undefined); + throw new DatabaseUnavailableError('Could not open the journal database.', { cause }); + } + + return db; +} + +/** The shared database handle, opening and migrating it on first call. */ +export function getJournalDatabase(): Promise { + openPromise ??= open().catch((error: unknown) => { + openPromise = null; + throw error; + }); + return openPromise; +} + +/** Closes the handle if one is open. Safe to call when none is. */ +export async function closeJournalDatabase(): Promise { + const pending = openPromise; + if (!pending) return; + openPromise = null; + + const db = await pending.catch(() => null); + await db?.closeAsync(); +} + +/** + * Erases the journal: the database file first, then the key. + * + * This is the mechanism behind the "Delete all my data" control 0007 requires + * and issue #116 tracks. File before key, so that an interruption between the + * two leaves an unreadable database rather than a readable one with no key - + * either way there is nothing recoverable, but only one of those orders is + * still true to the promise if the process dies half-way. + */ +export async function destroyJournalDatabase(): Promise { + await closeJournalDatabase(); + await SQLite.deleteDatabaseAsync(DATABASE_NAME).catch(() => undefined); + await deleteDatabaseKey(); +} diff --git a/mobile/src/lib/db/key.test.ts b/mobile/src/lib/db/key.test.ts new file mode 100644 index 0000000..21351a6 --- /dev/null +++ b/mobile/src/lib/db/key.test.ts @@ -0,0 +1,103 @@ +import * as SecureStore from 'expo-secure-store'; +import * as Crypto from 'expo-crypto'; + +import { + DatabaseKeyError, + deleteDatabaseKey, + getOrCreateDatabaseKey, + rawKeyPragma, +} from './key'; + +jest.mock('expo-secure-store', () => ({ + getItemAsync: jest.fn(), + setItemAsync: jest.fn(), + deleteItemAsync: jest.fn(), + WHEN_UNLOCKED_THIS_DEVICE_ONLY: 'when-unlocked-this-device-only', +})); + +jest.mock('expo-crypto', () => ({ + getRandomBytesAsync: jest.fn(), +})); + +const getItemAsync = SecureStore.getItemAsync as jest.Mock; +const setItemAsync = SecureStore.setItemAsync as jest.Mock; +const deleteItemAsync = SecureStore.deleteItemAsync as jest.Mock; +const getRandomBytesAsync = Crypto.getRandomBytesAsync as jest.Mock; + +/** 32 bytes, 0x00..0x1f, i.e. 64 hex characters. */ +const BYTES = Uint8Array.from({ length: 32 }, (_, i) => i); +const HEX = '000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f'; + +beforeEach(() => { + jest.clearAllMocks(); + getRandomBytesAsync.mockResolvedValue(BYTES); +}); + +describe('getOrCreateDatabaseKey', () => { + it('generates, stores and returns a key the first time', async () => { + getItemAsync.mockResolvedValue(null); + + await expect(getOrCreateDatabaseKey()).resolves.toBe(HEX); + + expect(getRandomBytesAsync).toHaveBeenCalledWith(32); + expect(setItemAsync).toHaveBeenCalledWith('journal.database.key', HEX, expect.any(Object)); + }); + + it('returns the stored key without generating a new one', async () => { + getItemAsync.mockResolvedValue(HEX); + + await expect(getOrCreateDatabaseKey()).resolves.toBe(HEX); + + expect(getRandomBytesAsync).not.toHaveBeenCalled(); + expect(setItemAsync).not.toHaveBeenCalled(); + }); + + it('stores the key without requireAuthentication, and device-only', async () => { + // This is the decision documented at length in key.ts. If someone turns + // requireAuthentication on, adding a fingerprint silently destroys every + // journal in the field, so it is worth a test that says so out loud. + getItemAsync.mockResolvedValue(null); + await getOrCreateDatabaseKey(); + + const [, , options] = setItemAsync.mock.calls[0]; + expect(options).not.toHaveProperty('requireAuthentication', true); + expect(options.keychainAccessible).toBe(SecureStore.WHEN_UNLOCKED_THIS_DEVICE_ONLY); + }); + + it('throws rather than minting a replacement when the read fails', async () => { + // The dangerous bug this guards: treating an unreadable keychain as "no key + // yet" and generating a fresh one, which orphans a database that was only + // temporarily unreachable. + getItemAsync.mockRejectedValue(new Error('keychain locked')); + + await expect(getOrCreateDatabaseKey()).rejects.toThrow(DatabaseKeyError); + expect(getRandomBytesAsync).not.toHaveBeenCalled(); + expect(setItemAsync).not.toHaveBeenCalled(); + }); + + it('reports a failure to store the new key', async () => { + getItemAsync.mockResolvedValue(null); + setItemAsync.mockRejectedValue(new Error('keychain full')); + + await expect(getOrCreateDatabaseKey()).rejects.toThrow(/Could not store/); + }); +}); + +describe('deleteDatabaseKey', () => { + it('removes the keychain entry', async () => { + await deleteDatabaseKey(); + expect(deleteItemAsync).toHaveBeenCalledWith('journal.database.key', expect.any(Object)); + }); +}); + +describe('rawKeyPragma', () => { + it('uses SQLCipher raw-key syntax so no KDF runs over an already-random key', () => { + expect(rawKeyPragma(HEX)).toBe(`PRAGMA key = "x'${HEX}'"`); + }); + + it('rejects anything that is not exactly 32 bytes of hex', () => { + expect(() => rawKeyPragma('abc')).toThrow(DatabaseKeyError); + expect(() => rawKeyPragma('z'.repeat(64))).toThrow(DatabaseKeyError); + expect(() => rawKeyPragma(HEX + '00')).toThrow(/64 hex characters/); + }); +}); diff --git a/mobile/src/lib/db/key.ts b/mobile/src/lib/db/key.ts new file mode 100644 index 0000000..7d01ca9 --- /dev/null +++ b/mobile/src/lib/db/key.ts @@ -0,0 +1,135 @@ +// The SQLCipher key for the journal database: where it comes from, where it is +// kept, and - the part worth reading before changing anything here - what it is +// deliberately *not* protected with. +// +// Per 0001 (local-only architecture) there is no server copy of anything a user +// writes. That single fact drives every choice below: a key that becomes +// unreadable is not an inconvenience, it is the permanent loss of someone's +// medical journal, with no support path that can recover it. + +import * as Crypto from 'expo-crypto'; +import * as SecureStore from 'expo-secure-store'; + +/** SQLCipher takes a 256-bit key. */ +const KEY_BYTES = 32; + +/** Keychain (iOS) / Keystore (Android) entry holding the key. */ +const KEYCHAIN_ENTRY = 'journal.database.key'; + +/** + * How the key entry is protected. + * + * WHEN_UNLOCKED_THIS_DEVICE_ONLY, and specifically *not* `requireAuthentication: + * true`, which is the option you would reach for first and which we are turning + * down on purpose. + * + * expo-secure-store's own documentation is explicit that an entry written with + * `requireAuthentication: true` "will become inaccessible if there are changes + * to the user's biometric settings, such as adding a new fingerprint". On a + * device holding the only copy of the data, that turns an ordinary bit of phone + * housekeeping - adding a fingerprint because your other thumb is in a bandage, + * re-enrolling Face ID after new glasses - into silent, total, unrecoverable + * loss of the journal. 0007 accepts "lose the phone, lose the journal" as a + * risk it has told the user about; it does not accept, and nobody has told the + * user about, "add a fingerprint, lose the journal". + * + * So the biometric gate lives one level up instead, in src/lib/auth/unlock.ts, + * as an explicit authentication prompt at app open. That satisfies what 0007 + * actually asks for - a device-biometric lock with the OS passcode fallback - + * while leaving the key's survival independent of enrollment state. + * + * The tradeoff, stated plainly: the key is protected by the device lock rather + * than bound to biometric enrollment in the secure element, so an attacker with + * an unlocked device, or with a rooted/jailbroken one and the patience to read + * the keystore, can reach it. Against that we are weighing a failure mode that + * is silent, permanent, and triggered by something users do routinely. For a + * journal whose worst case is disclosure and whose *other* worst case is total + * loss, this is the better side of the trade - but it is a product decision as + * much as a technical one and it deserves a signed-off ADR of its own. + * + * 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. + */ +const KEYCHAIN_OPTIONS: SecureStore.SecureStoreOptions = { + keychainAccessible: SecureStore.WHEN_UNLOCKED_THIS_DEVICE_ONLY, +}; + +/** Thrown when the keychain is reachable but refuses to hand the key back. */ +export class DatabaseKeyError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = 'DatabaseKeyError'; + } +} + +function toHex(bytes: Uint8Array): string { + return Array.from(bytes, (b) => b.toString(16).padStart(2, '0')).join(''); +} + +/** + * Returns the database key, generating and storing one the first time. + * + * The hex string this returns is a raw key, not a passphrase - see + * `rawKeyPragma` for why that distinction matters at the PRAGMA. + */ +export async function getOrCreateDatabaseKey(): Promise { + let existing: string | null; + try { + existing = await SecureStore.getItemAsync(KEYCHAIN_ENTRY, KEYCHAIN_OPTIONS); + } 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 + // and, worse, an unguarded "recovery" that recreated the file would destroy + // data that was merely temporarily unreadable. + throw new DatabaseKeyError('Could not read the database key from secure storage.', { + cause, + }); + } + + if (existing) return existing; + + const key = toHex(await Crypto.getRandomBytesAsync(KEY_BYTES)); + + try { + await SecureStore.setItemAsync(KEYCHAIN_ENTRY, key, KEYCHAIN_OPTIONS); + } catch (cause) { + throw new DatabaseKeyError('Could not store the database key in secure storage.', { + cause, + }); + } + + return key; +} + +/** + * Removes the key. + * + * On its own this is not "delete my data" (issue #116) - it strands the + * database file rather than erasing it. Deleting the file is the other half and + * lives in database.ts; call both, file first. + */ +export async function deleteDatabaseKey(): Promise { + await SecureStore.deleteItemAsync(KEYCHAIN_ENTRY, KEYCHAIN_OPTIONS); +} + +/** + * Builds the `PRAGMA key` statement for a hex key. + * + * SQLCipher reads a plain string as a *passphrase* and runs 256k rounds of + * PBKDF2 over it at every open. The `x'...'` form instead supplies the 32 raw + * key bytes directly and skips derivation entirely. Since the key here is + * already 256 bits of CSPRNG output, derivation would add startup latency and + * no security, so the raw form is the right one - but it is only correct + * *because* of where the key comes from. Hand this a user-chosen passphrase and + * you have thrown away the KDF that made it safe. + */ +export function rawKeyPragma(hexKey: string): string { + if (!/^[0-9a-f]+$/i.test(hexKey) || hexKey.length !== KEY_BYTES * 2) { + throw new DatabaseKeyError( + `Expected ${KEY_BYTES * 2} hex characters for a raw SQLCipher key.` + ); + } + return `PRAGMA key = "x'${hexKey}'"`; +} diff --git a/mobile/src/lib/db/migrations.test.ts b/mobile/src/lib/db/migrations.test.ts new file mode 100644 index 0000000..21e3c5d --- /dev/null +++ b/mobile/src/lib/db/migrations.test.ts @@ -0,0 +1,110 @@ +import { createInMemoryDatabase, HAS_NODE_SQLITE } from '../../../jest/in-memory-sqlite'; +import type { InMemoryDatabase } from '../../../jest/in-memory-sqlite'; +import { currentVersion, migrate, MigrationError, MIGRATIONS } from './migrations'; +import type { Migration } from './migrations'; + +const describeSql = HAS_NODE_SQLITE ? describe : describe.skip; + +const CREATE_CONTACTS: Migration = { + id: 1, + name: 'create contacts', + up: 'CREATE TABLE contacts (id TEXT PRIMARY KEY NOT NULL, name TEXT NOT NULL)', +}; + +const ADD_PHONE: Migration = { + id: 2, + name: 'add phone to contacts', + up: 'ALTER TABLE contacts ADD COLUMN phone TEXT', +}; + +describeSql('migrate', () => { + let db: InMemoryDatabase; + + beforeEach(() => { + db = createInMemoryDatabase(); + }); + + afterEach(async () => { + await db.closeAsync(); + }); + + async function tableNames(): Promise { + const rows = await db.getAllAsync<{ name: string }>( + "SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name" + ); + return rows.map((row) => row.name); + } + + it('reports version 0 for a fresh database', async () => { + await expect(currentVersion(db)).resolves.toBe(0); + }); + + it('applies every migration and records the version', async () => { + await expect(migrate(db, [CREATE_CONTACTS, ADD_PHONE])).resolves.toBe(2); + + await expect(currentVersion(db)).resolves.toBe(2); + expect(await tableNames()).toContain('contacts'); + + const columns = await db.getAllAsync<{ name: string }>('PRAGMA table_info(contacts)'); + expect(columns.map((column) => column.name)).toEqual(['id', 'name', 'phone']); + }); + + it('skips migrations that have already run', async () => { + await migrate(db, [CREATE_CONTACTS]); + + // Re-running the full list must not re-run migration 1, which would throw + // "table contacts already exists". + await expect(migrate(db, [CREATE_CONTACTS, ADD_PHONE])).resolves.toBe(2); + await expect(currentVersion(db)).resolves.toBe(2); + }); + + it('is a no-op when everything has already been applied', async () => { + await migrate(db, [CREATE_CONTACTS, ADD_PHONE]); + await expect(migrate(db, [CREATE_CONTACTS, ADD_PHONE])).resolves.toBe(2); + }); + + it('rejects a list that is not numbered contiguously from 1', async () => { + await expect(migrate(db, [CREATE_CONTACTS, { ...ADD_PHONE, id: 3 }])).rejects.toThrow( + MigrationError + ); + await expect(migrate(db, [{ ...CREATE_CONTACTS, id: 0 }])).rejects.toThrow( + /contiguously from 1/ + ); + }); + + it('refuses to run against a database from a newer build', async () => { + await db.execAsync('PRAGMA user_version = 7'); + + await expect(migrate(db, [CREATE_CONTACTS])).rejects.toThrow(/newer version of the app/); + // Still 7 - it did not quietly downgrade anything. + await expect(currentVersion(db)).resolves.toBe(7); + }); + + it('rolls a failing migration back and leaves the version alone', async () => { + const broken: Migration = { + id: 2, + name: 'broken', + up: 'CREATE TABLE good (id TEXT); CREATE TABLE bad (this is not sql)', + }; + + await expect(migrate(db, [CREATE_CONTACTS, broken])).rejects.toThrow(MigrationError); + + await expect(currentVersion(db)).resolves.toBe(1); + const tables = await tableNames(); + expect(tables).toContain('contacts'); + // The half of the broken migration that was valid must not survive. + expect(tables).not.toContain('good'); + }); + + it('reports which migration failed', async () => { + const broken: Migration = { id: 1, name: 'broken one', up: 'NOT SQL AT ALL' }; + await expect(migrate(db, [broken])).rejects.toThrow(/Migration 1 \("broken one"\)/); + }); + + it('ships no migrations yet, so the first table belongs to the first feature', async () => { + // Guards the boundary described in migrations.ts: when this starts failing, + // it is because a feature added a table, which is the intended way in. + expect(MIGRATIONS).toHaveLength(0); + await expect(migrate(db)).resolves.toBe(0); + }); +}); diff --git a/mobile/src/lib/db/migrations.ts b/mobile/src/lib/db/migrations.ts new file mode 100644 index 0000000..c5448ba --- /dev/null +++ b/mobile/src/lib/db/migrations.ts @@ -0,0 +1,104 @@ +// Schema migrations, tracked in SQLite's own `user_version` pragma. +// +// The pattern in Expo's docs is a hand-written if-ladder over the version +// number. That reads fine at one migration and badly at fifteen, and Phase 1 +// alone adds a table per journal section, so this keeps the same mechanism - +// `user_version` as the source of truth, no metadata table of our own - behind +// a list you append to. + +import type { SQLiteDatabase } from 'expo-sqlite'; + +export type Migration = { + /** 1-based, contiguous, and never reordered or renumbered once merged. */ + readonly id: number; + /** Human label, only ever used in error messages. */ + readonly name: string; + /** SQL applied to move the schema from `id - 1` to `id`. */ + readonly up: string; +}; + +/** + * Every migration, in order. + * + * Deliberately empty: the storage foundation ships the mechanism, and the first + * table belongs to the first feature that needs one - Caregiver/Emergency + * Contacts, issue #118 - so that "foundation" and "the first screen" stay + * separable in review. Append here; never edit a merged entry, because devices + * in the field have already run it and only see what comes after. + */ +export const MIGRATIONS: readonly Migration[] = []; + +export class MigrationError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = 'MigrationError'; + } +} + +/** Rejects a list that would silently skip or re-run steps. */ +function assertWellFormed(migrations: readonly Migration[]): void { + migrations.forEach((migration, index) => { + const expected = index + 1; + if (migration.id !== expected) { + throw new MigrationError( + `Migrations must be numbered contiguously from 1: expected ${expected}, found ${migration.id} ("${migration.name}").` + ); + } + }); +} + +/** Current schema version of an open database. 0 for a fresh file. */ +export async function currentVersion(db: SQLiteDatabase): Promise { + const row = await db.getFirstAsync<{ user_version: number }>('PRAGMA user_version'); + return row?.user_version ?? 0; +} + +/** + * Applies whatever has not been applied yet, and returns the resulting version. + * + * Each migration runs in its own exclusive transaction together with the + * `user_version` bump, so an interrupted upgrade leaves the database on a + * version that matches its actual schema rather than half-way through one. + * withExclusiveTransactionAsync rather than withTransactionAsync because only + * the former confines the transaction to the statements in the callback - with + * the latter, anything else running concurrently gets swept into it. + */ +export async function migrate( + db: SQLiteDatabase, + migrations: readonly Migration[] = MIGRATIONS +): Promise { + assertWellFormed(migrations); + + let version = await currentVersion(db); + + if (version > migrations.length) { + // The file was written by a newer build of the app - a TestFlight user + // rolling back, say. Continuing would run the older code against a schema + // it does not understand, so stop while everything is still intact. + throw new MigrationError( + `Database is at version ${version} but this build only knows ${migrations.length}. It was probably written by a newer version of the app.` + ); + } + + for (const migration of migrations) { + if (migration.id <= version) continue; + + try { + await db.withExclusiveTransactionAsync(async (txn) => { + await txn.execAsync(migration.up); + // Not parameterisable - PRAGMA does not take bindings - but `id` is a + // number from our own list, never user input. + await txn.execAsync(`PRAGMA user_version = ${migration.id}`); + }); + } catch (cause) { + throw new MigrationError( + `Migration ${migration.id} ("${migration.name}") failed. The database is still at version ${version}.`, + { cause } + ); + } + + version = migration.id; + } + + return version; +} diff --git a/mobile/src/lib/db/repository.test.ts b/mobile/src/lib/db/repository.test.ts new file mode 100644 index 0000000..08ff407 --- /dev/null +++ b/mobile/src/lib/db/repository.test.ts @@ -0,0 +1,217 @@ +import { createInMemoryDatabase, HAS_NODE_SQLITE } from '../../../jest/in-memory-sqlite'; +import type { InMemoryDatabase } from '../../../jest/in-memory-sqlite'; +import { createRepository, ENTRY_COLUMNS_SQL, RepositoryError } from './repository'; + +// expo-crypto is native. Sequential ids keep assertions readable; uniqueness is +// all the repository asks of them. +// Jest hoists the factory above the file, so anything it closes over has to be +// `mock`-prefixed for the transform to allow it. +let mockUuidCounter = 0; +jest.mock('expo-crypto', () => ({ + randomUUID: () => `uuid-${String(++mockUuidCounter).padStart(3, '0')}`, + getRandomBytesAsync: async (n: number) => new Uint8Array(n), +})); + +// node:sqlite landed in Node 22.5. package.json allows 20.19.4, CI pins 22.x, +// so these run in CI always and locally on a modern Node. +const describeSql = HAS_NODE_SQLITE ? describe : describe.skip; + +type Contact = { name: string; relationship: string; phone: string | null }; + +describeSql('createRepository', () => { + let db: InMemoryDatabase; + let contacts: ReturnType; + + function makeRepository() { + return createRepository( + { table: 'contacts', fields: ['name', 'relationship', 'phone'] }, + async () => db + ); + } + + beforeEach(async () => { + mockUuidCounter = 0; + db = createInMemoryDatabase(); + await db.execAsync( + `CREATE TABLE contacts (${ENTRY_COLUMNS_SQL}, name TEXT NOT NULL, relationship TEXT, phone TEXT)` + ); + contacts = makeRepository(); + }); + + afterEach(async () => { + await db.closeAsync(); + jest.useRealTimers(); + }); + + describe('configuration', () => { + it('rejects a table name that is not a plain identifier', () => { + expect(() => + createRepository({ table: 'contacts; DROP TABLE users', fields: ['name'] }) + ).toThrow(RepositoryError); + }); + + it('rejects a column name that is not a plain identifier', () => { + expect(() => + createRepository({ table: 'contacts', fields: ['name = 1 OR 1'] }) + ).toThrow(RepositoryError); + }); + + it('rejects columns the repository manages itself', () => { + expect(() => createRepository({ table: 'contacts', fields: ['id'] })).toThrow( + /managed by the repository/ + ); + expect(() => createRepository({ table: 'contacts', fields: ['created_at'] })).toThrow( + RepositoryError + ); + }); + + it('rejects a repository with no fields', () => { + expect(() => createRepository({ table: 'contacts', fields: [] })).toThrow(/no fields/); + }); + }); + + describe('create', () => { + it('stores the entry and returns it with generated metadata', async () => { + const created = await contacts.create({ + name: 'Alex Reyes', + relationship: 'Sister', + phone: '555-0101', + }); + + expect(created).toEqual({ + id: 'uuid-001', + name: 'Alex Reyes', + relationship: 'Sister', + phone: '555-0101', + createdAt: expect.any(String), + updatedAt: expect.any(String), + }); + expect(created.createdAt).toBe(created.updatedAt); + + await expect(contacts.find('uuid-001')).resolves.toEqual(created); + }); + + it('writes a missing optional field as NULL rather than undefined', async () => { + const created = await contacts.create({ + name: 'Sam Okafor', + relationship: 'Neighbour', + } as Contact); + + const stored = await contacts.find(created.id); + expect(stored?.phone).toBeNull(); + }); + + it('treats SQL in a value as text, not as SQL', async () => { + const created = await contacts.create({ + name: "Robert'); DROP TABLE contacts;--", + relationship: 'Friend', + phone: null, + }); + + const stored = await contacts.find(created.id); + expect(stored?.name).toBe("Robert'); DROP TABLE contacts;--"); + // The table is still there, which is the actual assertion. + await expect(contacts.list()).resolves.toHaveLength(1); + }); + }); + + describe('list', () => { + it('returns an empty array when there is nothing stored', async () => { + await expect(contacts.list()).resolves.toEqual([]); + }); + + it('orders by creation time, oldest first', async () => { + // Inserted directly so the timestamps are controlled rather than racing + // inside the same millisecond. + for (const [id, createdAt, name] of [ + ['c', '2026-03-01T10:00:00.000Z', 'Third'], + ['a', '2026-01-01T10:00:00.000Z', 'First'], + ['b', '2026-02-01T10:00:00.000Z', 'Second'], + ]) { + await db.runAsync( + 'INSERT INTO contacts (id, created_at, updated_at, name) VALUES (?, ?, ?, ?)', + [id, createdAt, createdAt, name] + ); + } + + const listed = await contacts.list(); + expect(listed.map((entry) => entry.name)).toEqual(['First', 'Second', 'Third']); + }); + }); + + describe('find', () => { + it('returns null for an id that is not there', async () => { + await expect(contacts.find('nope')).resolves.toBeNull(); + }); + }); + + describe('update', () => { + it('changes only the fields it is given and bumps updatedAt', async () => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2026-05-01T09:00:00.000Z')); + const created = await contacts.create({ + name: 'Alex Reyes', + relationship: 'Sister', + phone: '555-0101', + }); + + jest.setSystemTime(new Date('2026-05-02T09:00:00.000Z')); + const updated = await contacts.update(created.id, { phone: '555-0199' }); + + expect(updated.phone).toBe('555-0199'); + expect(updated.name).toBe('Alex Reyes'); + expect(updated.relationship).toBe('Sister'); + expect(updated.createdAt).toBe(created.createdAt); + expect(updated.updatedAt).toBe('2026-05-02T09:00:00.000Z'); + }); + + it('leaves updatedAt alone when nothing actually changed', async () => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2026-05-01T09:00:00.000Z')); + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + + jest.setSystemTime(new Date('2026-05-02T09:00:00.000Z')); + const updated = await contacts.update(created.id, {}); + + expect(updated.updatedAt).toBe(created.updatedAt); + }); + + it('can clear a field by setting it to null', async () => { + const created = await contacts.create({ + name: 'Alex', + relationship: 'Sister', + phone: '555-0101', + }); + const updated = await contacts.update(created.id, { phone: null }); + expect(updated.phone).toBeNull(); + }); + + it('throws for an id that is not there', async () => { + await expect(contacts.update('nope', { name: 'X' })).rejects.toThrow(RepositoryError); + }); + }); + + describe('remove', () => { + it('deletes the entry', async () => { + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + await contacts.remove(created.id); + + await expect(contacts.find(created.id)).resolves.toBeNull(); + await expect(contacts.list()).resolves.toEqual([]); + }); + + it('throws for an id that is not there', async () => { + await expect(contacts.remove('nope')).rejects.toThrow(RepositoryError); + }); + + it('leaves other entries alone', async () => { + const first = await contacts.create({ name: 'One', relationship: 'A', phone: null }); + await contacts.create({ name: 'Two', relationship: 'B', phone: null }); + + await contacts.remove(first.id); + + const remaining = await contacts.list(); + expect(remaining.map((entry) => entry.name)).toEqual(['Two']); + }); + }); +}); diff --git a/mobile/src/lib/db/repository.ts b/mobile/src/lib/db/repository.ts new file mode 100644 index 0000000..abd1d4e --- /dev/null +++ b/mobile/src/lib/db/repository.ts @@ -0,0 +1,188 @@ +// The repeatable-entry data layer. +// +// Most of Phase 1 is the same shape: a list of small records you can add to, +// edit and delete - contacts, providers, allergies, chronic conditions, +// hospitalisations, family history. 0008 asks for that pattern to be built once +// and reused rather than rediscovered per screen, so the storage half lives +// here and the UI half (issues #49 and #118) is built on top of it. +// +// This is the data layer only. It knows nothing about React, and deliberately +// nothing about any particular journal section either - a section is a table +// name plus a list of columns, declared where the feature lives. + +import * as Crypto from 'expo-crypto'; +import type { SQLiteDatabase } from 'expo-sqlite'; + +import { getJournalDatabase } from './database'; + +/** What SQLite will accept as a bound value for these simple records. */ +export type FieldValue = string | number | null; + +export type FieldSet = Record; + +/** Columns every repeatable entry carries, managed here rather than by callers. */ +export type EntryMeta = { + readonly id: string; + readonly createdAt: string; + readonly updatedAt: string; +}; + +export type Entry = TFields & EntryMeta; + +export type RepositoryConfig = { + readonly table: string; + readonly fields: readonly (keyof TFields & string)[]; +}; + +export type Repository = { + list(): Promise[]>; + find(id: string): Promise | null>; + create(fields: TFields): Promise>; + update(id: string, changes: Partial): Promise>; + remove(id: string): Promise; +}; + +export class RepositoryError extends Error { + constructor(message: string) { + super(message); + this.name = 'RepositoryError'; + } +} + +/** + * Table and column names cannot be bound as parameters, so they are + * interpolated - which means they have to be checked rather than trusted. + * + * These names come from a config object written by a developer, not from + * anything a user types, so this is a guard against a typo becoming a very + * confusing bug, not a defence against hostile input. Values always go through + * bindings. + */ +const IDENTIFIER = /^[A-Za-z_][A-Za-z0-9_]*$/; + +function checkIdentifier(kind: string, name: string): string { + if (!IDENTIFIER.test(name)) { + throw new RepositoryError(`Invalid ${kind} name: ${JSON.stringify(name)}.`); + } + return name; +} + +/** Column names that this module owns; a feature cannot redeclare them. */ +const RESERVED = new Set(['id', 'created_at', 'updated_at']); + +/** + * Builds a repository for one repeatable-entry table. + * + * `getDatabase` is injectable so tests can drive a real in-memory SQLite + * database instead of the encrypted one, which needs a device to exist. + */ +export function createRepository( + config: RepositoryConfig, + getDatabase: () => Promise = getJournalDatabase +): Repository { + const table = checkIdentifier('table', config.table); + + if (config.fields.length === 0) { + throw new RepositoryError(`Repository for "${table}" declares no fields.`); + } + + const fields = config.fields.map((field) => { + checkIdentifier('column', field); + if (RESERVED.has(field)) { + throw new RepositoryError( + `Column "${field}" on "${table}" is managed by the repository and cannot be declared as a field.` + ); + } + return field; + }); + + const selection = ['id', 'created_at AS createdAt', 'updated_at AS updatedAt', ...fields].join( + ', ' + ); + + // Oldest first, with id as a tiebreak so that two entries added in the same + // millisecond keep a stable order between renders instead of swapping around. + const ordering = 'ORDER BY created_at ASC, id ASC'; + + return { + async list() { + const db = await getDatabase(); + return db.getAllAsync>(`SELECT ${selection} FROM ${table} ${ordering}`); + }, + + async find(id) { + const db = await getDatabase(); + const row = await db.getFirstAsync>( + `SELECT ${selection} FROM ${table} WHERE id = ?`, + id + ); + return row ?? null; + }, + + async create(values) { + const db = await getDatabase(); + const id = Crypto.randomUUID(); + const now = new Date().toISOString(); + + const columns = ['id', 'created_at', 'updated_at', ...fields]; + const placeholders = columns.map(() => '?').join(', '); + const bound = [id, now, now, ...fields.map((field) => values[field] ?? null)]; + + await db.runAsync( + `INSERT INTO ${table} (${columns.join(', ')}) VALUES (${placeholders})`, + bound + ); + + return { ...values, id, createdAt: now, updatedAt: now }; + }, + + async update(id, changes) { + const db = await getDatabase(); + + const changed = fields.filter((field) => field in changes); + if (changed.length === 0) { + // Nothing to write. Returning the row as-is beats touching updatedAt + // for an edit that changed nothing. + const current = await this.find(id); + if (!current) throw new RepositoryError(`No ${table} entry with id ${id}.`); + return current; + } + + const now = new Date().toISOString(); + const assignments = [...changed.map((field) => `${field} = ?`), 'updated_at = ?'].join(', '); + const bound = [...changed.map((field) => changes[field] ?? null), now, id]; + + const result = await db.runAsync( + `UPDATE ${table} SET ${assignments} WHERE id = ?`, + bound as FieldValue[] + ); + + if (result.changes === 0) { + throw new RepositoryError(`No ${table} entry with id ${id}.`); + } + + const updated = await this.find(id); + if (!updated) throw new RepositoryError(`No ${table} entry with id ${id}.`); + return updated; + }, + + async remove(id) { + const db = await getDatabase(); + const result = await db.runAsync(`DELETE FROM ${table} WHERE id = ?`, id); + if (result.changes === 0) { + throw new RepositoryError(`No ${table} entry with id ${id}.`); + } + }, + }; +} + +/** + * The columns every repeatable-entry table needs, for use in the CREATE TABLE + * of a migration. Kept next to the repository that reads them so the two cannot + * drift apart. + */ +export const ENTRY_COLUMNS_SQL = [ + 'id TEXT PRIMARY KEY NOT NULL', + 'created_at TEXT NOT NULL', + 'updated_at TEXT NOT NULL', +].join(', '); From 253c8ead98621d729e69a86e356491120973ea94 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Thu, 13 Aug 2026 02:33:11 +0000 Subject: [PATCH 03/13] Add decision records for storage and unlock (0015-0018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/decisions/0015-database-key-storage.md | 33 +++++++++++++++++ .../0016-development-builds-required.md | 35 +++++++++++++++++++ .../0017-journal-data-is-native-only.md | 30 ++++++++++++++++ .../0018-no-device-lock-behaviour.md | 27 ++++++++++++++ docs/decisions/README.md | 6 +++- 5 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 docs/decisions/0015-database-key-storage.md create mode 100644 docs/decisions/0016-development-builds-required.md create mode 100644 docs/decisions/0017-journal-data-is-native-only.md create mode 100644 docs/decisions/0018-no-device-lock-behaviour.md diff --git a/docs/decisions/0015-database-key-storage.md b/docs/decisions/0015-database-key-storage.md new file mode 100644 index 0000000..ebe3409 --- /dev/null +++ b/docs/decisions/0015-database-key-storage.md @@ -0,0 +1,33 @@ +# 0015 — Database key storage and the biometric boundary + +**Status:** Proposed — implemented on `feature/encrypted-storage-unlock-foundation`, needs technical-lead sign-off +**Resolves:** the key-storage half of the local-database-encryption decision that [0003](0003-device-encryption-exports-backups.md) and [0007](0007-authentication-recovery-deletion.md) both reference as living in `architecture-plan.md` + +## Context + +The journal database is encrypted with SQLCipher, which needs a key. Where that key lives and what protects it was never written down — `architecture-plan.md` settled "SQLCipher + biometric unlock" as a direction without specifying the mechanism. Building the storage foundation forced the question. + +`expo-secure-store` offers `requireAuthentication: true`, which binds a stored value to the device's biometric in the secure element. It is the obvious way to implement "biometric unlock", and reading its documentation is what raised the problem: a value stored that way "will become inaccessible if there are changes to the user's biometric settings, such as adding a new fingerprint." + +## Decision + +**Store the key without `requireAuthentication`, and apply the biometric gate separately at the app shell.** + +- A 256-bit CSPRNG key, hex-encoded, in Keychain/Keystore via `expo-secure-store` with `WHEN_UNLOCKED_THIS_DEVICE_ONLY`. +- The biometric prompt is a separate `expo-local-authentication` call gating the app, not a property of the key entry. +- The key is handed to SQLCipher as a raw key (`PRAGMA key = "x'...'"`) rather than a passphrase. It is already 256 bits of random, so the passphrase form's PBKDF2 pass would add startup latency and no security. This is only correct *because* of where the key comes from — a user-chosen passphrase would need the KDF. + +## Rationale + +Per [0001](0001-local-only-architecture.md) there is no server copy of anything a user writes. A key that becomes unreadable is therefore not an inconvenience; it is the permanent loss of someone's medical journal, with no support path that can recover it. + +`requireAuthentication` puts that outcome one routine action away: adding a fingerprint because your other thumb is bandaged, re-enrolling Face ID after new glasses. [0007](0007-authentication-recovery-deletion.md) accepts "lose the phone, lose the journal" as a risk *and requires telling people about it during onboarding*. Nobody has told anyone "add a fingerprint, lose the journal", and it is not a risk a user would accept if asked. + +Separating the two still delivers what 0007 actually asks for — a device-biometric lock with the OS passcode fallback — while leaving the key's survival independent of enrollment state. + +## Consequences + +- **The trade, stated plainly:** the key is protected by the device lock rather than bound to biometric enrollment in the secure element. Someone with an unlocked device, or with a rooted/jailbroken one and some patience, can reach it. Against that we are weighing a failure mode that is silent, permanent, and triggered by something people do routinely. For a journal whose worst case is disclosure and whose *other* worst case is total loss, this is the better side of the trade — but it is a security decision and deserves the second pair of eyes [0003](0003-device-encryption-exports-backups.md) also asks for. +- `WHEN_UNLOCKED_THIS_DEVICE_ONLY` 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 for exports; issue #115 still has to do the equivalent for the database file itself. +- A unit test asserts the option stays off, so switching it on means deleting a test that says why not to. +- **If this is rejected,** the alternative is `requireAuthentication` plus an onboarding line telling people that changing their biometric settings destroys the journal. That is honest, and a worse product. diff --git a/docs/decisions/0016-development-builds-required.md b/docs/decisions/0016-development-builds-required.md new file mode 100644 index 0000000..1219fe0 --- /dev/null +++ b/docs/decisions/0016-development-builds-required.md @@ -0,0 +1,35 @@ +# 0016 — Development builds required; Expo Go no longer runs this app + +**Status:** Proposed — follows from encrypting the database at all, but the workflow cost needs accepting explicitly +**Arises from:** building the storage foundation ([0015](0015-database-key-storage.md)) + +## Context + +SQLCipher is a native fork of SQLite. `expo-sqlite` compiles it in only when `useSQLCipher` is set in its config plugin, which means a native rebuild, and Expo's SDK 57 documentation says so plainly: *"SQLCipher is not supported on Expo Go."* + +Until now a contributor could clone the repo, run `npm start`, scan a QR code and be looking at the app in a minute. That stops being true the moment the database is encrypted. + +## Decision + +**Accept it: the project uses Continuous Native Generation (`npx expo prebuild`) and a development build. Expo Go is no longer a way to run this app.** + +`ios/` and `android/` stay generated rather than committed — CNG regenerates them from `app.json`, which keeps configuration in one reviewable place. + +## Rationale + +The alternative that preserves Expo Go is application-level encryption: plain SQLite, with `expo-crypto` encrypting individual field values before they are written. It was considered and rejected. + +- Every read and write would go through app code, and anything the app forgets to encrypt is stored in clear. +- Encrypted values cannot be indexed, sorted or compared by SQLite, so any list the user sorts or filters — medications by name, contacts alphabetically — has to be pulled into memory and decrypted wholesale first. Note this does **not** apply to drug search: per [0013](0013-medicine-diary-on-device-drug-search.md) that runs against `catalog.db`, which is public and deliberately unencrypted, so encryption never constrains it. +- It is a hand-rolled scheme in place of a reviewed one, protecting medical data, maintained by rotating volunteers. This is the argument that actually decides it. + +SQLCipher encrypts the whole file, including indexes and the write-ahead log, and is the mechanism `architecture-plan.md` already assumed. + +## Consequences + +- **`useSQLCipher` is a build-wide flag, not a per-database one.** [0013](0013-medicine-diary-on-device-drug-search.md) puts two databases on the device: `journal.db` encrypted, and `catalog.db` — the public drug catalog — deliberately not, because it is public data and treating it otherwise buys nothing. Turning SQLCipher on links the whole app against it, so `catalog.db` gets opened by a SQLCipher build with no `PRAGMA key` set. Reading a plaintext file that way is ordinary SQLCipher behaviour, but Expo's documentation does not cover it and nothing here has tested it. **Verify on device before the catalog work depends on it** — issue #101 is the natural place, and it is a cheap check that would be expensive to discover late. +- **`enableFTS` must stay on.** It defaults to `true`, and 0013's search is SQLite FTS5. Expo's own config example sets `enableFTS` and `useSQLCipher` together, so there is no conflict — but an edit that switched FTS off would break the Medicine Diary while looking like a storage-layer change, which is not where anyone would think to look. Worth setting explicitly in `app.json` rather than relying on the default. +- **Contributor onboarding changes and the docs have to change with it.** `mobile/README.md` needs the prebuild/dev-client path, and the first-run cost (a native build) needs saying up front rather than discovered. +- This lands on the same people [0008](0008-first-vertical-slice.md) designed a gentle first ticket for, so the setup instructions matter more than usual. +- CI currently runs lint, typecheck and jest, none of which need a native build, so it is unaffected for now. A build job would need macOS runners for iOS. +- Anything that only ever runs in Expo Go — quick demos to non-technical stakeholders — needs a different answer, most likely a shared development build or the web landing page ([0017](0017-journal-data-is-native-only.md)). diff --git a/docs/decisions/0017-journal-data-is-native-only.md b/docs/decisions/0017-journal-data-is-native-only.md new file mode 100644 index 0000000..94e6727 --- /dev/null +++ b/docs/decisions/0017-journal-data-is-native-only.md @@ -0,0 +1,30 @@ +# 0017 — Journal data is native-only; web is not a journal surface + +**Status:** Proposed — needs sign-off +**Arises from:** building the storage foundation ([0015](0015-database-key-storage.md)) + +## Context + +The app builds for web: `react-native-web`, a static export target, `.web.tsx` variants of several components, and a landing page that already exists. SQLCipher does not build for web — Expo's documentation lists it for Android, iOS and macOS only. + +So the encrypted database cannot exist in the browser, and something has to give. + +## Decision + +**The journal database refuses to open on web, and web stays the landing and marketing surface.** No unencrypted fallback. + +The unlock gate passes web traffic straight through, since gating a marketing page protects nothing. + +## Rationale + +The tempting move is to fall back to plain `expo-sqlite`, which does have a web build. That would mean the web build quietly writing an unencrypted medical journal into browser storage — on whatever computer someone happened to open it on, quite possibly a shared or library machine. + +The whole basis of [0001](0001-local-only-architecture.md) is that this data stays under the user's control and the org never holds it. A silent downgrade from "encrypted" to "not encrypted", on a platform nobody was told behaves differently, makes that promise untrue in exactly the way nobody would notice until it mattered. Failing loudly is better than a fallback that is wrong. + +## Consequences + +- The web build serves the landing page and nothing that touches journal data. Any screen that reads the journal will throw there rather than render, which is intentional — but it does mean `npm run web` is not a preview of the app, and contributors need to know that. +- **This is about the journal, not about databases in general.** [0013](0013-medicine-diary-on-device-drug-search.md) explicitly contemplates a web client downloading the public drug catalog and searching it client-side, and nothing here rules that out: `catalog.db` is public, unencrypted, and has no reason to be platform-gated. The line is drawn around patient data, not around `expo-sqlite`. +- This makes the web target genuinely useful for one thing: demoing the product to stakeholders without a development build ([0016](0016-development-builds-required.md)). +- If browser access to the journal is ever wanted, it needs its own decision and its own mechanism. It is not a matter of removing a platform check. +- The "grab and go" emergency access in [0005](0005-grab-and-go-emergency-access.md) is a phone-in-hand scenario, so nothing in it depends on web. diff --git a/docs/decisions/0018-no-device-lock-behaviour.md b/docs/decisions/0018-no-device-lock-behaviour.md new file mode 100644 index 0000000..45666ed --- /dev/null +++ b/docs/decisions/0018-no-device-lock-behaviour.md @@ -0,0 +1,27 @@ +# 0018 — Unlock behaviour on a device with no lock screen + +**Status:** **Open** — product call, needs a named owner +**Arises from:** building the unlock gate ([0015](0015-database-key-storage.md)) + +## Context + +[0007](0007-authentication-recovery-deletion.md) settled what happens when the biometric *fails*: fall back to the device passcode, which is a mechanism people already know. It was written about lockout, and it assumes there is a device lock to fall back to. + +Some phones have neither a biometric nor a passcode set. There is then nothing for the app to prompt with, and 0007 has no answer for it. This is not a rare edge case in this audience — a passcode is one more thing to remember, and people who find phones difficult are exactly who this app is for. + +## Decision + +**None yet.** This record exists so the gap is visible rather than settled by whoever writes the code. + +The current implementation explains the situation and lets the user continue. The journal is still encrypted at rest either way; the device lock is a second layer, not the only one. That is a placeholder, chosen because silently doing nothing and hard-refusing both seemed worse than saying something true, and it should not be mistaken for a decision. + +## Options + +- **Refuse to open until a device lock is set.** Strongest, and locks someone out of their own medical information over a phone setting they may not know how to change. Hard to square with "keep technology in a supporting role". +- **Continue, with a plain explanation.** What is built today. Honest, and leaves the app's own protection at whatever the encrypted-at-rest key gives ([0015](0015-database-key-storage.md)). +- **Prompt once, remember the answer.** Nudges without trapping. More UI, and needs a place to store the answer. +- **Continue, and offer a shortcut into the OS settings screen.** Same as above with a lower barrier to actually fixing it. + +## What is needed + +A named owner to pick one, in the same way [0009](0009-hosting-support-incident-ownership.md) needs one. Whoever decides should also settle whether onboarding says anything about it, since [0007](0007-authentication-recovery-deletion.md) already commits to one honest line about backup being the user's responsibility and this is adjacent to it. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index baa3520..5a5571f 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -20,10 +20,14 @@ Each record has a status. **Decided** means treat it as settled — don't re-lit | [0012](0012-medicine-diary-drug-data-source.md) | Medicine Diary drug data source (RxNorm primary, openFDA secondary) | Needs formal sign-off | | [0013](0013-medicine-diary-on-device-drug-search.md) | Medicine Diary drug search — on-device catalog, two-database split, no user data server-side | Decided by Paul; needs Jesse's sign-off | | [0014](0014-medicine-diary-shape.md) | Section 2 (Medicine Diary) shape — current list, dated log, or both | **Open** — product call, needs a named owner | +| [0015](0015-database-key-storage.md) | Database key storage and the biometric boundary | Proposed — needs technical-lead sign-off | +| [0016](0016-development-builds-required.md) | Development builds required; Expo Go no longer runs this app | Proposed — workflow cost needs accepting | +| [0017](0017-journal-data-is-native-only.md) | Journal data is native-only; web is not a journal surface | Proposed — needs sign-off | +| [0018](0018-no-device-lock-behaviour.md) | Unlock behaviour on a device with no lock screen | **Open** — product call, needs a named owner | ## Related, not duplicated here -- Local database encryption (SQLCipher, biometric unlock) was decided separately and lives in `architecture-plan.md` in the team's shared docs — not repeated here since it's a separate decision from what this folder covers (data *leaving* the device vs. data at rest). +- Local database encryption (SQLCipher, biometric unlock) was decided in `architecture-plan.md` in the team's shared docs — the *direction* is not re-litigated here. What that document left unspecified, and what building it turned up, is now recorded: [0015](0015-database-key-storage.md) for where the key lives, [0016](0016-development-builds-required.md) and [0017](0017-journal-data-is-native-only.md) for what encrypting the database costs elsewhere. - Full content/data model audit (all 11 paper-journal sections, phased ticket backlog) lives in the team's `project-plan-and-tickets.md` — this folder covers cross-cutting product/legal/architecture decisions, not the per-screen field list. ## Adding a new record From ae0baeaead7a818a00d7896a72ef15ec19753e11 Mon Sep 17 00:00:00 2001 From: Jesse Berman <51378950+Jberma23@users.noreply.github.com> Date: Sun, 16 Aug 2026 12:45:30 -0600 Subject: [PATCH 04/13] update brakeman version (#137) * update brakeman version * switch to double qoutes * bump sqlite --- Gemfile | 4 ++-- Gemfile.lock | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 317f2c8..d59d620 100644 --- a/Gemfile +++ b/Gemfile @@ -5,7 +5,7 @@ gem "rails", "~> 8.1.3" # The modern asset pipeline for Rails [https://github.com/rails/propshaft] gem "propshaft" # Use sqlite3 as the database for Active Record -gem "sqlite3", ">= 2.1" +gem "sqlite3", ">= 2.9.6" # Use the Puma web server [https://github.com/puma/puma] gem "puma", ">= 5.0" # Use JavaScript with ESM import maps [https://github.com/rails/importmap-rails] @@ -56,7 +56,7 @@ group :development, :test do gem "bundler-audit", require: false # Static analysis for security vulnerabilities [https://brakemanscanner.org/] - gem "brakeman", require: false + gem "brakeman", "~> 8", require: false # Omakase Ruby styling [https://github.com/rails/rubocop-rails-omakase/] gem "rubocop-rails-omakase", require: false diff --git a/Gemfile.lock b/Gemfile.lock index f7b14db..7ffa82e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -391,7 +391,7 @@ PLATFORMS DEPENDENCIES bootsnap - brakeman + brakeman (~> 8) bundler-audit capybara debug @@ -409,7 +409,7 @@ DEPENDENCIES solid_cable solid_cache solid_queue - sqlite3 (>= 2.1) + sqlite3 (>= 2.9.6) stimulus-rails thruster turbo-rails From d443b71e8addfde3331a1017ae92e6ad573125cd Mon Sep 17 00:00:00 2001 From: Jberma23 Date: Sun, 16 Aug 2026 19:44:28 +0000 Subject: [PATCH 05/13] Handle a journal no key can open, and close it when locking 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) --- mobile/src/components/unlock-gate.test.tsx | 26 +++++++++ mobile/src/components/unlock-gate.tsx | 13 +++++ mobile/src/lib/db/database.test.ts | 60 +++++++++++++++++++- mobile/src/lib/db/database.ts | 66 ++++++++++++++++++++-- mobile/src/lib/db/key.test.ts | 15 ++++- mobile/src/lib/db/key.ts | 46 +++++++++++++-- 6 files changed, 212 insertions(+), 14 deletions(-) diff --git a/mobile/src/components/unlock-gate.test.tsx b/mobile/src/components/unlock-gate.test.tsx index 62b5bb2..c536d57 100644 --- a/mobile/src/components/unlock-gate.test.tsx +++ b/mobile/src/components/unlock-gate.test.tsx @@ -3,14 +3,23 @@ import { AppState, Platform, Text as RNText } from 'react-native'; import { UnlockGate } from './unlock-gate'; import { checkUnlockAvailability, requestUnlock } from '@/lib/auth/unlock'; +import { closeJournalDatabase } from '@/lib/db/database'; jest.mock('@/lib/auth/unlock', () => ({ checkUnlockAvailability: jest.fn(), requestUnlock: jest.fn(), })); +// The gate closes the database when it re-locks. Mocked rather than exercised: +// the real one needs expo-sqlite and a device, and what matters here is only +// that the gate asks. +jest.mock('@/lib/db/database', () => ({ + closeJournalDatabase: jest.fn(async () => undefined), +})); + const availability = checkUnlockAvailability as jest.Mock; const unlock = requestUnlock as jest.Mock; +const closeDatabase = closeJournalDatabase as jest.Mock; const originalOS = Platform.OS; function setPlatform(os: typeof Platform.OS) { @@ -148,9 +157,26 @@ describe('UnlockGate', () => { await waitFor(() => expect(screen.queryByText('Journal contents')).toBeNull()); expect(screen.getByText('Your journal is locked')).toBeVisible(); + // Locking the view is only half of it. While the handle stays open the key + // is still in memory and any caller can still read, which makes 0015's + // WHEN_UNLOCKED_THIS_DEVICE_ONLY a first-launch property and nothing more. + expect(closeDatabase).toHaveBeenCalled(); + jest.restoreAllMocks(); }); + it('does not close a database the web build never opened', async () => { + setPlatform('web'); + + await render( + + + + ); + + expect(closeDatabase).not.toHaveBeenCalled(); + }); + it('does not gate the web build, which holds no journal data', async () => { setPlatform('web'); diff --git a/mobile/src/components/unlock-gate.tsx b/mobile/src/components/unlock-gate.tsx index ba53a1f..7ed26ae 100644 --- a/mobile/src/components/unlock-gate.tsx +++ b/mobile/src/components/unlock-gate.tsx @@ -16,6 +16,7 @@ import { ActivityIndicator, AppState, Platform, View } from 'react-native'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { checkUnlockAvailability, requestUnlock } from '@/lib/auth/unlock'; +import { closeJournalDatabase } from '@/lib/db/database'; type GateState = | { status: 'checking' } @@ -105,6 +106,18 @@ export function UnlockGate({ children, skip = false }: UnlockGateProps) { const subscription = AppState.addEventListener('change', (next) => { if (next === 'background') { setState({ status: 'locked', message: null }); + + // Close the database too, not just the view. The gate is the only thing + // that knows the app is meant to be locked, and locking only the UI + // leaves db/database.ts holding a decrypted handle and the key in memory + // for the life of the process - at which point 0015's + // WHEN_UNLOCKED_THIS_DEVICE_ONLY is a property of the first launch and + // nothing after it, because the keychain is never asked again. Dropping + // the handle here is what makes that option mean something. + // + // Failures are swallowed on purpose: this runs on the way out of the + // foreground, there is nobody to tell, and the next unlock re-opens. + void closeJournalDatabase().catch(() => undefined); } }); diff --git a/mobile/src/lib/db/database.test.ts b/mobile/src/lib/db/database.test.ts index 9807630..5f24c26 100644 --- a/mobile/src/lib/db/database.test.ts +++ b/mobile/src/lib/db/database.test.ts @@ -6,6 +6,7 @@ import { DatabaseUnavailableError, destroyJournalDatabase, getJournalDatabase, + UnrecoverableJournalError, } from './database'; import { deleteDatabaseKey, getOrCreateDatabaseKey } from './key'; import { migrate } from './migrations'; @@ -57,9 +58,13 @@ beforeEach(async () => { setPlatform('ios'); db = fakeDatabase(); openDatabaseAsync.mockResolvedValue(db); - getKey.mockResolvedValue(KEY); + getKey.mockResolvedValue({ key: KEY, created: false }); migrateMock.mockResolvedValue(0); deleteDatabaseAsync.mockResolvedValue(undefined); + // Has to resolve, not return undefined: the code under test chains .catch() + // onto it, and a bare jest.fn() would throw a TypeError that swallows the + // error the caller was actually meant to see. + deleteKey.mockResolvedValue(undefined); }); afterEach(async () => { @@ -124,6 +129,59 @@ describe('getJournalDatabase', () => { }); }); +describe('a journal that no key can open', () => { + /** A key we just minted cannot fail against a file we just created. */ + function restoredFromAnotherPhone() { + getKey.mockResolvedValue({ key: KEY, created: true }); + db.getFirstAsync.mockRejectedValue(new Error('file is not a database')); + } + + it('is reported as its own error rather than a generic open failure', async () => { + // Restoring a backup onto a new phone brings journal.db back but not the + // key, which 0015 keeps THIS_DEVICE_ONLY on purpose. Saying "could not open + // the database" there is true and useless; this is the case that has to be + // nameable so something can eventually offer to start over. + restoredFromAnotherPhone(); + + await expect(getJournalDatabase()).rejects.toThrow(UnrecoverableJournalError); + }); + + it('takes the useless key back out, so the next launch reaches the same branch', async () => { + // Leaving it stored would make created=false next time, and the diagnosis + // would degrade to a generic failure that explains nothing. + restoredFromAnotherPhone(); + + await expect(getJournalDatabase()).rejects.toThrow(UnrecoverableJournalError); + expect(deleteKey).toHaveBeenCalled(); + }); + + it('closes the handle it could not read', async () => { + restoredFromAnotherPhone(); + + await expect(getJournalDatabase()).rejects.toThrow(UnrecoverableJournalError); + expect(db.closeAsync).toHaveBeenCalled(); + }); + + it('does not blame a stored key for a decrypt failure, or delete it', async () => { + // Same symptom, different cause: the key was already there, so this is + // corruption or something else - not a journal from another phone. Deleting + // the key here would destroy a database that might still be readable. + getKey.mockResolvedValue({ key: KEY, created: false }); + db.getFirstAsync.mockRejectedValue(new Error('file is not a database')); + + await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + expect(deleteKey).not.toHaveBeenCalled(); + }); + + it('leaves a genuine first run alone', async () => { + // Fresh install: key minted, empty file, decrypt fine. Nothing to report. + getKey.mockResolvedValue({ key: KEY, created: true }); + + await expect(getJournalDatabase()).resolves.toBeDefined(); + expect(deleteKey).not.toHaveBeenCalled(); + }); +}); + describe('closeJournalDatabase', () => { it('closes an open handle', async () => { await getJournalDatabase(); diff --git a/mobile/src/lib/db/database.ts b/mobile/src/lib/db/database.ts index 402a9a5..f246310 100644 --- a/mobile/src/lib/db/database.ts +++ b/mobile/src/lib/db/database.ts @@ -25,6 +25,32 @@ export class DatabaseUnavailableError extends Error { } } +/** + * There is a journal on this device and no key that opens it. The data is gone. + * + * The path here is not exotic - it is the ordinary one. 0015 stores 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. So restoring a backup + * onto a new phone - which is what people do when they replace a handset - + * brings back the file without the key. + * + * Treating that as a first run and minting a fresh key is the worst available + * response: every read then fails, and the app is bricked on every launch after + * with a message that explains nothing. So it is called out as its own error. + * The journal genuinely cannot be recovered - 0007 commits to telling people + * that during onboarding - but "your journal was made on a different phone and + * cannot be opened here" is a true thing to say, and it leaves the user + * somewhere to go. `destroyJournalDatabase` is the way to start over. + * + * The UI for that is not built; this is the mechanism it needs. + */ +export class UnrecoverableJournalError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = 'UnrecoverableJournalError'; + } +} + /** * Cached as the in-flight promise, not the resolved handle, so that two screens * mounting at once share one open rather than racing to create two. Cleared on @@ -45,17 +71,14 @@ async function open(): Promise { ); } - const key = await getOrCreateDatabaseKey(); + const { key, created } = await getOrCreateDatabaseKey(); const db = await SQLite.openDatabaseAsync(DATABASE_NAME); try { // Has to be the first statement executed against the connection. await db.execAsync(rawKeyPragma(key)); - // 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. - await db.getFirstAsync('SELECT count(*) FROM sqlite_master'); + await assertReadable(db, created); await db.execAsync('PRAGMA journal_mode = WAL'); await db.execAsync('PRAGMA foreign_keys = ON'); @@ -64,12 +87,45 @@ async function open(): Promise { } catch (cause) { // Do not leave a half-configured handle behind for the next caller. await db.closeAsync().catch(() => undefined); + if (cause instanceof UnrecoverableJournalError) throw cause; throw new DatabaseUnavailableError('Could not open the journal database.', { cause }); } return db; } +/** + * Reads a page, so a key that does not fit this file 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 - SQLite ignores unknown pragmas - and this read succeeds + * against a plaintext file. Detecting that needs `PRAGMA cipher_version`; see + * issue #130. + * + * What it does catch, given `keyWasCreated`, is the restored-backup case. A key + * we just minted cannot fail to read a database we just created, so if it fails + * the file was already there and belonged to a key that is gone. + */ +async function assertReadable(db: SQLiteDatabase, keyWasCreated: boolean): Promise { + try { + await db.getFirstAsync('SELECT count(*) FROM sqlite_master'); + } catch (cause) { + if (!keyWasCreated) throw cause; + + // Take the useless key back out. Leaving it would turn a diagnosable state + // into an undiagnosable one: the next launch would read a stored key, and + // this branch - the only thing that knows what actually happened - would + // never run again. + await deleteDatabaseKey().catch(() => undefined); + + throw new UnrecoverableJournalError( + 'There is a journal on this device that no key can open. It was almost certainly restored from a backup of another phone, which does not carry the key. The journal cannot be recovered; starting a new one means erasing it.', + { cause } + ); + } +} + /** The shared database handle, opening and migrating it on first call. */ export function getJournalDatabase(): Promise { openPromise ??= open().catch((error: unknown) => { diff --git a/mobile/src/lib/db/key.test.ts b/mobile/src/lib/db/key.test.ts index 21351a6..476e5d7 100644 --- a/mobile/src/lib/db/key.test.ts +++ b/mobile/src/lib/db/key.test.ts @@ -37,7 +37,7 @@ describe('getOrCreateDatabaseKey', () => { it('generates, stores and returns a key the first time', async () => { getItemAsync.mockResolvedValue(null); - await expect(getOrCreateDatabaseKey()).resolves.toBe(HEX); + await expect(getOrCreateDatabaseKey()).resolves.toEqual({ key: HEX, created: true }); expect(getRandomBytesAsync).toHaveBeenCalledWith(32); expect(setItemAsync).toHaveBeenCalledWith('journal.database.key', HEX, expect.any(Object)); @@ -46,12 +46,23 @@ describe('getOrCreateDatabaseKey', () => { it('returns the stored key without generating a new one', async () => { getItemAsync.mockResolvedValue(HEX); - await expect(getOrCreateDatabaseKey()).resolves.toBe(HEX); + await expect(getOrCreateDatabaseKey()).resolves.toEqual({ key: HEX, created: false }); expect(getRandomBytesAsync).not.toHaveBeenCalled(); expect(setItemAsync).not.toHaveBeenCalled(); }); + it('reports whether it minted the key, because null is ambiguous', async () => { + // SecureStore returns null both for "nothing stored yet" and for "the entry + // was invalidated". Only database.ts can tell those apart - by finding out + // whether the key opens the file - so this flag has to reach it. + getItemAsync.mockResolvedValue(null); + await expect(getOrCreateDatabaseKey()).resolves.toMatchObject({ created: true }); + + getItemAsync.mockResolvedValue(HEX); + await expect(getOrCreateDatabaseKey()).resolves.toMatchObject({ created: false }); + }); + it('stores the key without requireAuthentication, and device-only', async () => { // This is the decision documented at length in key.ts. If someone turns // requireAuthentication on, adding a fingerprint silently destroys every diff --git a/mobile/src/lib/db/key.ts b/mobile/src/lib/db/key.ts index 7d01ca9..8ea9b5f 100644 --- a/mobile/src/lib/db/key.ts +++ b/mobile/src/lib/db/key.ts @@ -47,9 +47,26 @@ const KEYCHAIN_ENTRY = 'journal.database.key'; * loss, this is the better side of the trade - but it is a product decision as * much as a technical one and it deserves a signed-off ADR of its own. * - * 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. Note that database.ts has to handle + * the other side of that: the file does come back from a backup, so a restored + * phone finds a journal it cannot read and has to say so rather than mint a new + * key over it. + * + * This option is iOS-only - `keychainAccessible` is @platform ios in + * expo-secure-store, so it does nothing on Android. The Android equivalent + * comes from the expo-secure-store config plugin, whose backup rules exclude + * the SecureStore shared preferences from Auto Backup. That makes the bare + * "expo-secure-store" entry in app.json load-bearing rather than registration. + * + * 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 documents it as "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. */ const KEYCHAIN_OPTIONS: SecureStore.SecureStoreOptions = { keychainAccessible: SecureStore.WHEN_UNLOCKED_THIS_DEVICE_ONLY, @@ -67,13 +84,30 @@ function toHex(bytes: Uint8Array): string { return Array.from(bytes, (b) => b.toString(16).padStart(2, '0')).join(''); } +export type DatabaseKey = { + /** Raw key as hex, not a passphrase - see `rawKeyPragma`. */ + readonly key: string; + /** + * True when this call minted the key rather than reading a stored one. + * + * The caller needs this, and the reason is worth stating. `getItemAsync` + * resolves to null "if there is no entry for the given key **or if the key + * has been invalidated**" - expo's own wording. One value, two very different + * situations, and only one of them is a first run. Nothing at this level can + * tell them apart, because that takes knowing whether a database file already + * exists; database.ts can, by trying to decrypt it. So this flag hands the + * question up rather than guessing here. + */ + readonly created: boolean; +}; + /** * Returns the database key, generating and storing one the first time. * * The hex string this returns is a raw key, not a passphrase - see * `rawKeyPragma` for why that distinction matters at the PRAGMA. */ -export async function getOrCreateDatabaseKey(): Promise { +export async function getOrCreateDatabaseKey(): Promise { let existing: string | null; try { existing = await SecureStore.getItemAsync(KEYCHAIN_ENTRY, KEYCHAIN_OPTIONS); @@ -88,7 +122,7 @@ export async function getOrCreateDatabaseKey(): Promise { }); } - if (existing) return existing; + if (existing) return { key: existing, created: false }; const key = toHex(await Crypto.getRandomBytesAsync(KEY_BYTES)); @@ -100,7 +134,7 @@ export async function getOrCreateDatabaseKey(): Promise { }); } - return key; + return { key, created: true }; } /** From 504dd519aa70ceb709c4cbd87fbed1ece71a62c3 Mon Sep 17 00:00:00 2001 From: Jberma23 Date: Sun, 16 Aug 2026 19:44:37 +0000 Subject: [PATCH 06/13] Fix repository update() binding and create() return value 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) --- mobile/src/lib/db/repository.test.ts | 44 +++++++++++++++++++++++++ mobile/src/lib/db/repository.ts | 48 ++++++++++++++++++++-------- 2 files changed, 79 insertions(+), 13 deletions(-) diff --git a/mobile/src/lib/db/repository.test.ts b/mobile/src/lib/db/repository.test.ts index 08ff407..b9deec8 100644 --- a/mobile/src/lib/db/repository.test.ts +++ b/mobile/src/lib/db/repository.test.ts @@ -101,6 +101,31 @@ describeSql('createRepository', () => { expect(stored?.phone).toBeNull(); }); + it('returns exactly what find() reads back', async () => { + // The two used to disagree: an omitted field was stored as NULL and + // returned as absent, so a screen rendering `created` saw something the + // database did not contain. + const created = await contacts.create({ + name: 'Sam Okafor', + relationship: 'Neighbour', + } as Contact); + + await expect(contacts.find(created.id)).resolves.toEqual(created); + }); + + it('does not hand back keys that were never columns', async () => { + // Undeclared keys are dropped on insert, so echoing them makes the return + // value look like a saved record when it is not. + const created = await contacts.create({ + name: 'Sam', + relationship: 'Friend', + phone: null, + nickname: 'Sammy', + } as never); + + expect(created).not.toHaveProperty('nickname'); + }); + it('treats SQL in a value as text, not as SQL', async () => { const created = await contacts.create({ name: "Robert'); DROP TABLE contacts;--", @@ -189,6 +214,25 @@ describeSql('createRepository', () => { it('throws for an id that is not there', async () => { await expect(contacts.update('nope', { name: 'X' })).rejects.toThrow(RepositoryError); }); + + it('still works when pulled off the repository', async () => { + // `const { update } = repo` and `onPress={repo.update}` are both ordinary + // React. While update reached its sibling through `this`, both threw + // TypeError at runtime, and TypeScript had nothing to say about it. + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + const { update } = contacts; + + await expect(update(created.id, { name: 'Alexis' })).resolves.toMatchObject({ + name: 'Alexis', + }); + }); + + it('still works detached on the no-op path, which reads a row back too', async () => { + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + const { update } = contacts; + + await expect(update(created.id, {})).resolves.toMatchObject({ name: 'Alex' }); + }); }); describe('remove', () => { diff --git a/mobile/src/lib/db/repository.ts b/mobile/src/lib/db/repository.ts index abd1d4e..c0b5328 100644 --- a/mobile/src/lib/db/repository.ts +++ b/mobile/src/lib/db/repository.ts @@ -104,36 +104,58 @@ export function createRepository( // millisecond keep a stable order between renders instead of swapping around. const ordering = 'ORDER BY created_at ASC, id ASC'; + // A plain function rather than a method, deliberately. `update` needs to read + // a row back, and reaching a sibling through `this` breaks the moment anyone + // writes `const { update } = repo` or passes `repo.update` as a callback - + // both ordinary in React, neither caught by TypeScript, and the failure is a + // TypeError at runtime. + async function find(id: string): Promise | null> { + const db = await getDatabase(); + const row = await db.getFirstAsync>( + `SELECT ${selection} FROM ${table} WHERE id = ?`, + id + ); + return row ?? null; + } + + /** + * The declared fields, and only those, with anything the caller left out + * turned into the NULL that will actually be stored. + * + * This is what `create` returns rather than the caller's own object, so that + * the entry it hands back matches the row `find` reads. Spreading the input + * instead lets an omitted field come back as `undefined` while the database + * holds `null`, and lets keys that were never columns travel onwards as + * though they had been saved. + */ + function normalise(values: Partial): TFields { + return Object.fromEntries(fields.map((field) => [field, values[field] ?? null])) as TFields; + } + return { async list() { const db = await getDatabase(); return db.getAllAsync>(`SELECT ${selection} FROM ${table} ${ordering}`); }, - async find(id) { - const db = await getDatabase(); - const row = await db.getFirstAsync>( - `SELECT ${selection} FROM ${table} WHERE id = ?`, - id - ); - return row ?? null; - }, + find, async create(values) { const db = await getDatabase(); const id = Crypto.randomUUID(); const now = new Date().toISOString(); + const stored = normalise(values); const columns = ['id', 'created_at', 'updated_at', ...fields]; const placeholders = columns.map(() => '?').join(', '); - const bound = [id, now, now, ...fields.map((field) => values[field] ?? null)]; + const bound = [id, now, now, ...fields.map((field) => stored[field])]; await db.runAsync( `INSERT INTO ${table} (${columns.join(', ')}) VALUES (${placeholders})`, - bound + bound as FieldValue[] ); - return { ...values, id, createdAt: now, updatedAt: now }; + return { ...stored, id, createdAt: now, updatedAt: now }; }, async update(id, changes) { @@ -143,7 +165,7 @@ export function createRepository( if (changed.length === 0) { // Nothing to write. Returning the row as-is beats touching updatedAt // for an edit that changed nothing. - const current = await this.find(id); + const current = await find(id); if (!current) throw new RepositoryError(`No ${table} entry with id ${id}.`); return current; } @@ -161,7 +183,7 @@ export function createRepository( throw new RepositoryError(`No ${table} entry with id ${id}.`); } - const updated = await this.find(id); + const updated = await find(id); if (!updated) throw new RepositoryError(`No ${table} entry with id ${id}.`); return updated; }, From 2d42b97a61c71b59830044085813568f503fda05 Mon Sep 17 00:00:00 2001 From: Jberma23 Date: Sun, 16 Aug 2026 19:44:46 +0000 Subject: [PATCH 07/13] Make the node:sqlite guard actually run 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) --- mobile/jest/in-memory-sqlite.test.ts | 57 ++++++++++++++++++++++++++++ mobile/jest/in-memory-sqlite.ts | 29 +++++++++++--- 2 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 mobile/jest/in-memory-sqlite.test.ts diff --git a/mobile/jest/in-memory-sqlite.test.ts b/mobile/jest/in-memory-sqlite.test.ts new file mode 100644 index 0000000..3c6f149 --- /dev/null +++ b/mobile/jest/in-memory-sqlite.test.ts @@ -0,0 +1,57 @@ +// The guard in in-memory-sqlite.ts only earns its place if it actually runs on +// a Node without node:sqlite. It did not: a static `import ... from +// 'node:sqlite'` is resolved before any module code, so the two SQLite-backed +// suites errored out on Node below 22.5 instead of skipping - issue #133, on a +// version `engines` still permits. +// +// CI runs 22.x, so nothing here would notice a regression. Making the require +// throw is the only way to test the old-Node path from a new Node. + +// doMock registrations live for the whole file, so each test states which world +// it wants rather than inheriting the previous one's. +beforeEach(() => { + jest.resetModules(); + jest.dontMock('node:sqlite'); +}); + +function withoutNodeSqlite() { + jest.doMock('node:sqlite', () => { + throw new Error('No such built-in module: node:sqlite'); + }); +} + +describe('on a Node without node:sqlite', () => { + it('reports the feature as missing rather than throwing at import', () => { + withoutNodeSqlite(); + + // The import itself is the assertion: before the fix it threw here. + const { HAS_NODE_SQLITE } = require('./in-memory-sqlite'); + + expect(HAS_NODE_SQLITE).toBe(false); + }); + + it('refuses to build a database, and says which Node it needs', () => { + withoutNodeSqlite(); + + const { createInMemoryDatabase } = require('./in-memory-sqlite'); + + expect(() => createInMemoryDatabase()).toThrow(/Node 22.5 or newer/); + }); +}); + +describe('on a Node that has it', () => { + it('reports the feature as present and builds a working database', async () => { + const { HAS_NODE_SQLITE, createInMemoryDatabase } = require('./in-memory-sqlite'); + + expect(HAS_NODE_SQLITE).toBe(true); + + const db = createInMemoryDatabase(); + try { + await db.execAsync('CREATE TABLE t (id TEXT)'); + await db.runAsync('INSERT INTO t (id) VALUES (?)', 'a'); + await expect(db.getFirstAsync('SELECT id FROM t')).resolves.toEqual({ id: 'a' }); + } finally { + await db.closeAsync(); + } + }); +}); diff --git a/mobile/jest/in-memory-sqlite.ts b/mobile/jest/in-memory-sqlite.ts index 8498874..aa976e3 100644 --- a/mobile/jest/in-memory-sqlite.ts +++ b/mobile/jest/in-memory-sqlite.ts @@ -10,18 +10,29 @@ // cannot tell you is anything about SQLCipher, which has no Node build and // needs a device. Encryption is verified on hardware (issue #101), not here. -import { DatabaseSync } from 'node:sqlite'; +import type { DatabaseSync } from 'node:sqlite'; import type { SQLiteDatabase } from 'expo-sqlite'; -/** True when the running Node has node:sqlite (22.5+). */ -export const HAS_NODE_SQLITE = (() => { +/** + * node:sqlite's `DatabaseSync`, or null on a Node that does not have it. + * + * The require is lazy on purpose. A static `import { DatabaseSync } from + * 'node:sqlite'` is resolved before any code in this module runs, so on Node + * below 22.5 the module throws at import time and no guard written here can + * catch it - the dependent suites then error out instead of skipping. That is + * issue #133, and `engines` currently permits 20.19.4. + */ +const DatabaseSyncClass: typeof DatabaseSync | null = (() => { try { - return typeof DatabaseSync === 'function'; + return (require('node:sqlite') as typeof import('node:sqlite')).DatabaseSync; } catch { - return false; + return null; } })(); +/** True when the running Node has node:sqlite (22.5+). */ +export const HAS_NODE_SQLITE = DatabaseSyncClass !== null; + /** * expo-sqlite accepts either `run(sql, a, b)` or `run(sql, [a, b])`, and this * codebase uses both. Collapse them into one array. @@ -34,7 +45,13 @@ function normaliseParams(params: unknown[]): unknown[] { export type InMemoryDatabase = SQLiteDatabase & { readonly raw: DatabaseSync }; export function createInMemoryDatabase(): InMemoryDatabase { - const raw = new DatabaseSync(':memory:'); + if (!DatabaseSyncClass) { + throw new Error( + 'node:sqlite is unavailable - this helper needs Node 22.5 or newer. Guard the suite with HAS_NODE_SQLITE.' + ); + } + + const raw = new DatabaseSyncClass(':memory:'); const db = { raw, From 190d55344b67ac5500c630e15846d92bd61eba16 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 18 Aug 2026 01:36:55 +0000 Subject: [PATCH 08/13] Guard the node:sqlite regression test, and correct the destroy ordering note Two follow-ups to #138. The fix for #133 makes the SQLite-backed suites skip on a Node without node:sqlite, but the test added to prove it asserts HAS_NODE_SQLITE is true with no guard. On Node 20.19.4 - permitted by `engines`, and the exact range #133 is about - the fix therefore fails its own regression test while the suites it protects skip cleanly. Verified on Node 18: before, 1 failure; after, 109 tests with 2 suites skipped. The absent direction is not fakeable, so it takes the same `describeSql` guard migrations.test.ts and repository.test.ts already use. The file-before-key comment on destroyJournalDatabase described neither interrupted state: file first leaves a key with nothing to open, not "an unreadable database". #138 supplies the reason it was missing - key first would strand a file no key can open, which is UnrecoverableJournalError, reached by the very path destroy is meant to be the escape from. Reworded here rather than deferred to #135, which is about destroy reporting success without deleting and does not touch the ordering rationale. Co-Authored-By: Claude Opus 5 (1M context) --- mobile/jest/in-memory-sqlite.test.ts | 11 ++++++++++- mobile/src/lib/db/database.test.ts | 6 ++++-- mobile/src/lib/db/database.ts | 16 ++++++++++++---- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/mobile/jest/in-memory-sqlite.test.ts b/mobile/jest/in-memory-sqlite.test.ts index 3c6f149..491f209 100644 --- a/mobile/jest/in-memory-sqlite.test.ts +++ b/mobile/jest/in-memory-sqlite.test.ts @@ -7,6 +7,15 @@ // CI runs 22.x, so nothing here would notice a regression. Making the require // throw is the only way to test the old-Node path from a new Node. +import { HAS_NODE_SQLITE } from './in-memory-sqlite'; + +// The other direction is not fakeable: no amount of mocking gives Node 20 a +// working node:sqlite, so the suite that exercises the real one skips exactly +// where the suites it guards do - see migrations.test.ts and repository.test.ts. +// Without this the fix for #133 would fail its own regression test on the Node +// versions #133 is about. +const describeSql = HAS_NODE_SQLITE ? describe : describe.skip; + // doMock registrations live for the whole file, so each test states which world // it wants rather than inheriting the previous one's. beforeEach(() => { @@ -39,7 +48,7 @@ describe('on a Node without node:sqlite', () => { }); }); -describe('on a Node that has it', () => { +describeSql('on a Node that has it', () => { it('reports the feature as present and builds a working database', async () => { const { HAS_NODE_SQLITE, createInMemoryDatabase } = require('./in-memory-sqlite'); diff --git a/mobile/src/lib/db/database.test.ts b/mobile/src/lib/db/database.test.ts index 5f24c26..bd9abbf 100644 --- a/mobile/src/lib/db/database.test.ts +++ b/mobile/src/lib/db/database.test.ts @@ -208,8 +208,10 @@ describe('destroyJournalDatabase', () => { await getJournalDatabase(); await destroyJournalDatabase(); - // Interrupted between the two, this order leaves an unreadable file rather - // than a readable one whose key is gone. + // Interrupted between the two, this order leaves a key with nothing to + // open, and the next launch is an ordinary first run. The reverse would + // leave a file no key can open - UnrecoverableJournalError, which is the + // state this whole control exists to get out of. expect(order).toEqual(['file', 'key']); expect(db.closeAsync).toHaveBeenCalled(); }); diff --git a/mobile/src/lib/db/database.ts b/mobile/src/lib/db/database.ts index f246310..6fe3826 100644 --- a/mobile/src/lib/db/database.ts +++ b/mobile/src/lib/db/database.ts @@ -149,10 +149,18 @@ export async function closeJournalDatabase(): Promise { * Erases the journal: the database file first, then the key. * * This is the mechanism behind the "Delete all my data" control 0007 requires - * and issue #116 tracks. File before key, so that an interruption between the - * two leaves an unreadable database rather than a readable one with no key - - * either way there is nothing recoverable, but only one of those orders is - * still true to the promise if the process dies half-way. + * and issue #116 tracks. The order matters only if the process dies between the + * two steps, but it decides what the user finds when it does. + * + * File first leaves a key with nothing to open, and the next launch reads that + * key and creates an empty journal - a clean start, which is what was asked + * for. Key first would leave a file no key can open: `UnrecoverableJournalError` + * above, reached by exactly the path this is meant to be the escape from. A + * half-finished delete would be indistinguishable from a journal restored off + * someone else's phone, and the app would say so instead of starting over. + * + * Both orders destroy the data. Only one of them leaves the app somewhere the + * user can go. */ export async function destroyJournalDatabase(): Promise { await closeJournalDatabase(); From a8afb2ae4a0af28c0082c5b8ebbbd017a0160383 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Thu, 20 Aug 2026 02:06:01 +0000 Subject: [PATCH 09/13] Make the development-build workflow real in the docs and the config 0016 decided that SQLCipher ends Expo Go and listed the docs change as a required consequence. That change was never made, so both READMEs still told contributors to scan a QR code, and mobile/README.md still promised "Expo Go still runs the app on a physical iPhone without a Mac" - which this PR makes false. Following those instructions was worse than just failing. Expo Go has no SQLCipher, `PRAGMA key` is an unknown pragma there and SQLite ignores it, so the app would have come up writing an unencrypted journal: the silent downgrade 0017 refuses for web, on the path the docs actually recommended. Prerequisites are now Xcode or Android Studio, setup is prebuild plus a run, and the first-build cost is stated up front rather than discovered - 0016 asked for that specifically, because this lands on the people 0008 designed a gentle first ticket for. Also records that `npm run web` is the landing page and not a preview of the app, and that two suites skip below Node 22.5 so a green local run can mean thirty fewer tests than CI. app.json gains ios.bundleIdentifier and android.package, and the run scripts move to `expo run:*`. Without them `npx expo prebuild` - now the first command a contributor types - invented `com.anonymous.alongwithyou`, wrote it into app.json and rewrote package.json, so the mandated first step left two tracked files dirty. Prebuild is now a no-op on both. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 35 +++++++++++++++++-------- mobile/README.md | 62 ++++++++++++++++++++++++++++++++++++++------- mobile/app.json | 6 +++-- mobile/package.json | 4 +-- 4 files changed, 84 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index a46e0c2..9e1d2ef 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,15 @@ in development and shipped to the app stores by EAS in production. | | | |---|---| | Ruby | 4.0.5 (see `.ruby-version`) | -| Node | **20.19.4 or newer** — React Native 0.86 refuses to build on older versions | +| Node | **20.19.4 or newer** — React Native 0.86 refuses to build on older versions. Two mobile test suites need **22.5 or newer** and skip below it | +| iOS | Xcode, which needs macOS | +| Android | Android Studio with the Android SDK | + +Xcode or Android Studio are needed because the mobile app no longer runs in Expo +Go — the journal is encrypted with SQLCipher, which is native code. See +[`mobile/README.md`](mobile/README.md) for the build, and +[`docs/decisions/0016`](docs/decisions/0016-development-builds-required.md) for +why. If `node -v` reports something older, install a current LTS with [nvm](https://github.com/nvm-sh/nvm): @@ -57,15 +65,24 @@ You need both processes up. Use two terminals: # Terminal 1 — the API. Bind to 0.0.0.0 so a phone can reach it. bin/rails server -b 0.0.0.0 -# Terminal 2 — the phone app +# Terminal 2 — the phone app. First run only: +cd mobile && npx expo prebuild && npm run ios # or npm run android + +# After that, Metro on its own is enough: cd mobile && npx expo start ``` -Then press `i` for the iOS simulator, `a` for the Android emulator, `w` for the -browser, or scan the QR code with Expo Go on a real phone. Open the **Tasks** +The first build takes tens of minutes and downloads several GB of native +toolchain; later ones are quick. Once the development build is installed, press +`i` for the iOS simulator or `a` for the Android emulator. Open the **Tasks** tab: the list is served by Rails, and adding, ticking and deleting write back to it. +`w` still opens the browser, but only the landing page — per +[`docs/decisions/0017`](docs/decisions/0017-journal-data-is-native-only.md) the +encrypted journal refuses to open on web rather than falling back to +unencrypted browser storage, so the web target is not a preview of the app. + ## How the app finds Rails `localhost` means a different machine on every target, so hardcoding it breaks @@ -185,12 +202,10 @@ rotating-contributor model, read that folder before you read the code. - **Rate limiting.** Also absent. Rails 8 ships `rate_limit` at the controller level, which needs a cache store configured in whatever environment serves this API. -- **Native builds.** `npx expo start` runs inside Expo Go, which only includes - Expo's own native modules. The moment you add a library with custom native - code you need a development build (`npx expo run:ios` / `run:android`) and - [EAS Build](https://docs.expo.dev/build/introduction/) for the app stores. - `mobile/app.json` still needs `ios.bundleIdentifier`, `android.package` and an - EAS project id before any of that works. +- **Store builds.** Development builds work locally, and `app.json` now carries + `ios.bundleIdentifier` and `android.package`. Shipping to the app stores still + needs [EAS Build](https://docs.expo.dev/build/introduction/) and an EAS + project id, which nothing here sets up yet. - **The starter screens.** Home and Explore are still Expo's template: Expo branding, Expo logo, links to Expo's docs. Tasks is the only screen that belongs to this project. Replacing them also retires several dependencies diff --git a/mobile/README.md b/mobile/README.md index 404dc8f..139f393 100644 --- a/mobile/README.md +++ b/mobile/README.md @@ -10,26 +10,50 @@ screen, but **it is not the backend for the real patient journal** — see - Node.js 20.19.4 or newer (React Native 0.86 will not build on older versions) - npm -- [Expo Go](https://expo.dev/go) on an Android or iOS device, or a simulator +- **Xcode** for iOS, which needs macOS — or **Android Studio** with the Android + SDK for Android + +**This app no longer runs in [Expo Go](https://expo.dev/go).** The journal is +encrypted with SQLCipher, which is a native fork of SQLite that Expo Go does not +ship, so running the app means building it yourself. +[`docs/decisions/0016`](../docs/decisions/0016-development-builds-required.md) +argues the trade. Two consequences are worth knowing before you start rather +than discovering half way through: + +- **The first build takes tens of minutes** and pulls down several GB of native + toolchain. Builds after that are quick, and day-to-day work only needs Metro. +- **Testing on a physical iPhone now needs a Mac**, or EAS Build. Expo Go used + to be the way around that, and is not any more. ## Setup ```bash cd mobile npm install -npm start +npx expo prebuild # generates ios/ and android/ from app.json +npm run ios # or: npm run android ``` -Start the Rails API too, or the app will load with a connection error: +`ios/` and `android/` are generated rather than checked in — `npx expo prebuild` +recreates them from `app.json`, which keeps the native configuration in one +reviewable place. Re-run it after changing `app.json`, or after adding a +dependency that has native code. + +Once the development build is installed on the simulator or device, `npm start` +runs Metro on its own and the app picks it up, the same as it always did. You +only need to build again when native code changes. + +Start the Rails API too, or the **Tasks** demo screen will load with a +connection error. The journal itself needs no server — see +[Talking to Rails](#talking-to-rails). ```bash # from the repository root, in another terminal bin/rails server -b 0.0.0.0 ``` -Then press `i` for the iOS simulator, `a` for the Android emulator, `w` for the -browser, or scan the QR code with Expo Go. Your computer and phone need to be on -the same network; if local discovery fails, try a tunnel: +Your computer and phone need to be on the same network; if local discovery +fails, try a tunnel: ```bash npx expo start --tunnel @@ -50,8 +74,16 @@ npm test npm run test:watch ``` -The iOS simulator requires macOS and Xcode. Expo Go still runs the app on a -physical iPhone without a Mac. +`npm run ios` and `npm run android` build and install the development build, +so the first run of either is the slow one described above. + +The iOS simulator requires macOS and Xcode, and so now does any iOS device. + +`npm run web` serves the landing page only. Per +[`docs/decisions/0017`](../docs/decisions/0017-journal-data-is-native-only.md) +SQLCipher has no web build, so the journal deliberately refuses to open in a +browser rather than quietly falling back to unencrypted storage. `npm run web` +is not a preview of the app. ## Project structure @@ -60,12 +92,15 @@ physical iPhone without a Mac. - `src/components/ui/` — the React Native Reusables components (see below) - `src/hooks/`, `src/constants/` — theming and colour scheme - `src/lib/api.ts` — typed client for the Rails API +- `src/lib/db/` — the SQLCipher-encrypted journal: key storage, migrations, and + the repeatable-entry repository +- `src/lib/auth/` — biometric unlock, applied by `src/components/unlock-gate.tsx` - `src/global.css`, `src/lib/theme.ts` — the design tokens `src/components/ui/` reads - `src/lib/utils.ts` — the `cn` class-name helper - `src/__tests__/` — tests for screens (see below) - `assets/` — icons and splash screens - `types/`, `nativewind-env.d.ts` — ambient declarations TypeScript cannot infer on its own -- `jest/` — jest setup and the CSS stub +- `jest/` — jest setup, the CSS stub, and the in-memory SQLite test double - `app.json` — Expo configuration - `babel.config.js`, `metro.config.js`, `tailwind.config.js`, `components.json` — NativeWind and the component CLI @@ -200,6 +235,15 @@ Three things to know before writing more: inside Metro, and jest stubs the stylesheet out, so `className` arrives as a plain prop and `style` is never set. Assert on behaviour, roles and text; use `className` only where the class itself is the point. +- **Two suites need Node 22.5 or newer.** `src/lib/db/migrations.test.ts` and + `src/lib/db/repository.test.ts` run against real SQLite through `node:sqlite`, + which older Node does not have. They *skip* rather than fail below that, so a + green run on Node 20 has quietly exercised about thirty fewer tests than CI + does. Check the skip count, or use the version CI uses. + +Note that nothing in the suite proves the database is actually encrypted: +`node:sqlite` is stock SQLite with no SQLCipher, so encryption is verified on a +device instead (issue #101). `jest/in-memory-sqlite.ts` says so at the top. ## Documentation diff --git a/mobile/app.json b/mobile/app.json index 0be0e57..cb7972d 100644 --- a/mobile/app.json +++ b/mobile/app.json @@ -8,7 +8,8 @@ "scheme": "alongwithyou", "userInterfaceStyle": "automatic", "ios": { - "icon": "./assets/expo.icon" + "icon": "./assets/expo.icon", + "bundleIdentifier": "org.rubyforgood.alongwithyou" }, "android": { "adaptiveIcon": { @@ -17,7 +18,8 @@ "backgroundImage": "./assets/images/android-icon-background.png", "monochromeImage": "./assets/images/android-icon-monochrome.png" }, - "predictiveBackGestureEnabled": false + "predictiveBackGestureEnabled": false, + "package": "org.rubyforgood.alongwithyou" }, "web": { "output": "static", diff --git a/mobile/package.json b/mobile/package.json index 2d8ac44..1cec52d 100644 --- a/mobile/package.json +++ b/mobile/package.json @@ -63,8 +63,8 @@ }, "scripts": { "start": "expo start", - "android": "expo start --android", - "ios": "expo start --ios", + "android": "expo run:android", + "ios": "expo run:ios", "web": "expo start --web", "lint": "expo lint", "typecheck": "tsc --noEmit", From cef7eac3350ab7d59ba092282a7b4df7a765bb32 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Thu, 20 Aug 2026 02:06:23 +0000 Subject: [PATCH 10/13] Run migrations on the keyed connection, and stop the double lying withExclusiveTransactionAsync does not run on the database handed to it. It builds a Transaction with useNewConnection: true, and SQLiteOpenOptions has no key field - keying is done only by executing PRAGMA key, which is per-connection. So the transaction connection was unkeyed, and the first statement to touch a page would have returned SQLITE_NOTADB. Because migrate() runs inside open(), that is not a failed migration: it is the journal failing to open at all, on every launch, permanently, for every user who already has data. The first migration anyone wrote would have shipped that. It was invisible for two reasons. MIGRATIONS is empty, so the loop body is dead code today. And jest/in-memory-sqlite.ts implemented the method as BEGIN EXCLUSIVE on the *same* connection, so the double contradicted the library on exactly the point that mattered and the tests agreed with the bug. The double now refuses instead: node:sqlite has one connection and no second to hand out, so running the callback on it is the only alternative to refusing, and that is the lie. Replaced with BEGIN IMMEDIATE / COMMIT / ROLLBACK on the keyed handle - the same execAsync path every other query already takes. The isolation argument in the old comment is moot rather than outweighed: migrate() runs before openPromise resolves, so no caller can hold the handle yet and there is no concurrent statement to sweep in. withTransactionAsync was rejected too: deferred BEGIN, and its unguarded ROLLBACK masks the cause. Reverting migrations.ts with the new double in place fails four tests. Co-Authored-By: Claude Opus 5 (1M context) --- mobile/jest/in-memory-sqlite.ts | 28 ++++++++++++------ mobile/src/lib/db/migrations.ts | 51 +++++++++++++++++++++++++-------- 2 files changed, 58 insertions(+), 21 deletions(-) diff --git a/mobile/jest/in-memory-sqlite.ts b/mobile/jest/in-memory-sqlite.ts index aa976e3..7b1aedb 100644 --- a/mobile/jest/in-memory-sqlite.ts +++ b/mobile/jest/in-memory-sqlite.ts @@ -77,15 +77,25 @@ export function createInMemoryDatabase(): InMemoryDatabase { return raw.prepare(sql).get(...(normaliseParams(params) as never[])) ?? null; }, - async withExclusiveTransactionAsync(task: (txn: unknown) => Promise) { - raw.exec('BEGIN EXCLUSIVE'); - try { - await task(db); - raw.exec('COMMIT'); - } catch (error) { - raw.exec('ROLLBACK'); - throw error; - } + // Refuses on purpose, and this is the one method here that is worth + // explaining. The real withExclusiveTransactionAsync does not run the + // callback on the database you called it on: expo-sqlite's + // `Transaction.createAsync` reopens the file with `useNewConnection: true`, + // and `SQLiteOpenOptions` carries no key. `PRAGMA key` is per-connection, + // so under SQLCipher that second handle cannot read the file at all. + // + // node:sqlite has one connection and nothing to hand out as a second, so + // the only implementations available are "run it on this connection" and + // "refuse". The first is what used to be here, and it asserted the opposite + // of what the library does - which is precisely why migrations.ts went in + // using a call that would have failed on the first real device migration + // with every test green. Refusing keeps that failure in the suite. + withExclusiveTransactionAsync(): Promise { + return Promise.reject( + new Error( + 'withExclusiveTransactionAsync is deliberately not modelled: the real one runs on a new, unkeyed connection that cannot read a SQLCipher database. Use BEGIN IMMEDIATE / COMMIT on this connection instead.' + ) + ); }, async closeAsync() { diff --git a/mobile/src/lib/db/migrations.ts b/mobile/src/lib/db/migrations.ts index c5448ba..628ca8d 100644 --- a/mobile/src/lib/db/migrations.ts +++ b/mobile/src/lib/db/migrations.ts @@ -53,15 +53,47 @@ export async function currentVersion(db: SQLiteDatabase): Promise { return row?.user_version ?? 0; } +/** + * One migration and its `user_version` bump, all or nothing, on `db` itself. + * + * The transaction is hand-rolled rather than delegated to either of + * expo-sqlite's helpers. `withExclusiveTransactionAsync` is the tempting one - + * it is the only one that confines the transaction to the statements in its + * callback - but it runs them on a *different* native connection: + * `Transaction.createAsync` reopens the file with `useNewConnection: true`, and + * `SQLiteOpenOptions` has no field for a key. `PRAGMA key` is per-connection, + * so that second handle arrives unkeyed and a SQLCipher database answers + * SQLITE_NOTADB. The isolation it buys is worth nothing to us anyway: `migrate` + * runs inside `open()` in database.ts, before the handle is published, and the + * app opens exactly one connection - there is no concurrent statement to sweep + * in. `withTransactionAsync` does stay on this connection, but it opens with a + * deferred `BEGIN` and, if its own `ROLLBACK` fails, throws that instead of the + * error that explains what actually broke. IMMEDIATE takes the write lock up + * front and the cause survives. + */ +async function applyInTransaction(db: SQLiteDatabase, migration: Migration): Promise { + await db.execAsync('BEGIN IMMEDIATE'); + try { + await db.execAsync(migration.up); + // Not parameterisable - PRAGMA does not take bindings - but `id` is a + // number from our own list, never user input. + await db.execAsync(`PRAGMA user_version = ${migration.id}`); + await db.execAsync('COMMIT'); + } catch (error) { + // Swallowed on purpose: SQLite unwinds the transaction itself for some + // failures, and then ROLLBACK errors too. Reporting that would bury the + // error that says what went wrong. + await db.execAsync('ROLLBACK').catch(() => undefined); + throw error; + } +} + /** * Applies whatever has not been applied yet, and returns the resulting version. * - * Each migration runs in its own exclusive transaction together with the - * `user_version` bump, so an interrupted upgrade leaves the database on a - * version that matches its actual schema rather than half-way through one. - * withExclusiveTransactionAsync rather than withTransactionAsync because only - * the former confines the transaction to the statements in the callback - with - * the latter, anything else running concurrently gets swept into it. + * Each migration runs in its own transaction together with the `user_version` + * bump, so an interrupted upgrade leaves the database on a version that matches + * its actual schema rather than half-way through one. */ export async function migrate( db: SQLiteDatabase, @@ -84,12 +116,7 @@ export async function migrate( if (migration.id <= version) continue; try { - await db.withExclusiveTransactionAsync(async (txn) => { - await txn.execAsync(migration.up); - // Not parameterisable - PRAGMA does not take bindings - but `id` is a - // number from our own list, never user input. - await txn.execAsync(`PRAGMA user_version = ${migration.id}`); - }); + await applyInTransaction(db, migration); } catch (cause) { throw new MigrationError( `Migration ${migration.id} ("${migration.name}") failed. The database is still at version ${version}.`, From 37161a2471adce3e1002bff8683e2d24349bef7e Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Thu, 20 Aug 2026 02:06:23 +0000 Subject: [PATCH 11/13] Detect a build with no SQLCipher, and stop destroy racing close Three gaps in database.ts, all of them ways to lose or expose data quietly. A build without SQLCipher accepts PRAGMA key, ignores it, and reads a plaintext file happily - so the app would write an unencrypted medical journal and nothing would notice. assertReadable could not catch it, since it cannot tell a decrypted file from one that never needed decrypting. assertEncrypted now asks PRAGMA cipher_version first. Note the shape: on stock SQLite that returns no rows rather than erroring, so the check tests the value. cipher_provider_version would have looked equivalent and been wrong - it is guarded by if(ctx), so a wrong key would report as "no SQLCipher" and brick the restored-backup path. It throws rather than warns, like 0017's web check: there is no degraded mode to continue into. destroyJournalDatabase swallowed every delete failure and then deleted the key regardless, so a still-open file meant the key was destroyed over a journal that survived - the exact permanent-brick state the delete ordering exists to prevent, reported as success. iOS collapses all three native delete errors to one code, so the only cross-platform discriminator is the message; that is fragile, so it fails in the safe direction - a spurious error on an empty journal, never a destroyed key over live data. closeJournalDatabase nulled openPromise before awaiting closeAsync, so a concurrent open could start against a closing connection. That is not theoretical now: the unlock gate closes the database from an AppState handler on every backgrounding, and destroy racing that close is what made the bug above reachable. Co-Authored-By: Claude Opus 5 (1M context) --- mobile/src/lib/db/database.test.ts | 264 ++++++++++++++++++++++++++++- mobile/src/lib/db/database.ts | 172 ++++++++++++++++++- 2 files changed, 420 insertions(+), 16 deletions(-) diff --git a/mobile/src/lib/db/database.test.ts b/mobile/src/lib/db/database.test.ts index bd9abbf..8b49283 100644 --- a/mobile/src/lib/db/database.test.ts +++ b/mobile/src/lib/db/database.test.ts @@ -1,6 +1,7 @@ import * as SQLite from 'expo-sqlite'; import { Platform } from 'react-native'; +import { createInMemoryDatabase, HAS_NODE_SQLITE } from '../../../jest/in-memory-sqlite'; import { closeJournalDatabase, DatabaseUnavailableError, @@ -35,7 +36,13 @@ function setPlatform(os: typeof Platform.OS) { (Platform as { OS: typeof Platform.OS }).OS = os; } -/** Records the statements run against it, in order. */ +/** + * Records the statements run against it, in order. + * + * Answers `PRAGMA cipher_version` like a SQLCipher build, because that is the + * build the app is meant to run on and every test that is not about #130 should + * be describing that world. The plaintext build gets its own fake below. + */ function fakeDatabase() { const statements: string[] = []; return { @@ -43,11 +50,15 @@ function fakeDatabase() { execAsync: jest.fn(async (sql: string) => { statements.push(sql); }), - getFirstAsync: jest.fn(async (sql: string) => { + // Return types are stated rather than inferred: tests below replace these + // with a null (no such pragma) or a promise that resolves later, and an + // inferred type from the happy path alone rejects both. + getFirstAsync: jest.fn(async (sql: string): Promise | null> => { statements.push(sql); + if (sql.includes('cipher_version')) return { cipher_version: '4.5.7 community' }; return { 'count(*)': 0 }; }), - closeAsync: jest.fn(async () => undefined), + closeAsync: jest.fn((): Promise => Promise.resolve()), }; } @@ -83,7 +94,12 @@ describe('getJournalDatabase', () => { it('forces a decrypt before handing the connection out', async () => { await getJournalDatabase(); - expect(db.statements[1]).toMatch(/FROM sqlite_master/); + + const read = db.statements.findIndex((sql) => /FROM sqlite_master/.test(sql)); + const wal = db.statements.indexOf('PRAGMA journal_mode = WAL'); + + expect(read).toBeGreaterThan(-1); + expect(read).toBeLessThan(wal); }); it('sets WAL and foreign keys, then migrates', async () => { @@ -129,11 +145,120 @@ describe('getJournalDatabase', () => { }); }); +describe('a build without SQLCipher', () => { + // #130. `PRAGMA key` against plain SQLite is an unknown pragma, and SQLite + // ignores unknown pragmas: it does not throw, so nothing downstream of it can + // notice, and the journal is written in cleartext by an app that looks + // entirely healthy. `PRAGMA cipher_version` is the only thing that separates + // the two builds, and it separates them by returning nothing. + + it('refuses to open rather than writing the journal in cleartext', async () => { + db.getFirstAsync.mockImplementation(async (sql: string) => { + db.statements.push(sql); + // What stock SQLite does with a pragma it does not implement. + if (sql.includes('cipher_version')) return null; + return { 'count(*)': 0 }; + }); + + await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + }); + + it('says which build is wrong and how to fix it', async () => { + // The person who hits this is a developer who skipped `npx expo prebuild`, + // and the message is the whole diagnosis they get. + db.getFirstAsync.mockImplementation(async () => null); + + await expect(getJournalDatabase()).rejects.toThrow(/SQLCipher/); + await expect(getJournalDatabase()).rejects.toThrow(/prebuild/); + }); + + it('checks before reading, so a plaintext file cannot pass as a decrypted one', async () => { + db.getFirstAsync.mockImplementation(async (sql: string) => { + db.statements.push(sql); + if (sql.includes('cipher_version')) return null; + return { 'count(*)': 0 }; + }); + + await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + + // A plaintext database reads perfectly, so assertReadable would have said + // yes. Order is the only thing stopping it. + expect(db.statements.some((sql) => /FROM sqlite_master/.test(sql))).toBe(false); + }); + + it('does not mistake a stored key for the problem, or delete it', async () => { + // The key is fine. The binary is wrong. Deleting the key here would destroy + // a perfectly good journal over a build misconfiguration. + getKey.mockResolvedValue({ key: KEY, created: true }); + db.getFirstAsync.mockImplementation(async () => null); + + await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + expect(deleteKey).not.toHaveBeenCalled(); + }); + + it('closes the connection it refused to use', async () => { + db.getFirstAsync.mockImplementation(async () => null); + + await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + expect(db.closeAsync).toHaveBeenCalled(); + }); +}); + +// The fake above only proves the code does what the fake was told to say. This +// runs the real open path against real, genuinely non-SQLCipher SQLite - +// node:sqlite is stock - so `PRAGMA key` is accepted and ignored for real and +// `PRAGMA cipher_version` comes back empty for real. It is the exact build #130 +// is about. +// +// The other direction cannot be tested here at all: there is no SQLCipher for +// Node, so nothing on CI can show that a real SQLCipher build passes this check. +// That is device work - issue #101. +const describeSql = HAS_NODE_SQLITE ? describe : describe.skip; + +describeSql('against real stock SQLite', () => { + it('is caught by the cipher check and nothing else', async () => { + const real = createInMemoryDatabase(); + openDatabaseAsync.mockResolvedValue(real); + + // Not a mock: stock SQLite really does accept the keying statement without + // complaint, which is the whole reason the check below has to exist. + await expect(real.execAsync(`PRAGMA key = "x'${KEY}'"`)).resolves.toBeUndefined(); + await expect(real.getFirstAsync('PRAGMA cipher_version')).resolves.toBeNull(); + + await expect(getJournalDatabase()).rejects.toThrow(/SQLCipher/); + + // And it never got as far as creating anything. + expect(migrateMock).not.toHaveBeenCalled(); + }); +}); + +/** + * A SQLCipher build holding a key that does not fit the file. + * + * Only the read fails. `PRAGMA cipher_version` still answers, and that is a + * property of SQLCipher rather than a convenience here: it reports what the + * binary was compiled with, so it does not depend on the key being right or on + * the file having decrypted. (In the vendored amalgamation it returns a + * compile-time constant, unguarded - unlike `cipher_provider_version` next to + * it, which needs a codec context and would therefore report a wrong key as a + * missing SQLCipher. That is why the check uses this pragma and not that one.) + * + * Rejecting every pragma instead would describe a build with no SQLCipher at + * all, which is a different fault with a different message. + */ +function decryptFails() { + db.getFirstAsync.mockImplementation(async (sql: string) => { + db.statements.push(sql); + if (sql.includes('cipher_version')) return { cipher_version: '4.5.7 community' }; + throw new Error('file is not a database'); + }); +} + describe('a journal that no key can open', () => { /** A key we just minted cannot fail against a file we just created. */ function restoredFromAnotherPhone() { getKey.mockResolvedValue({ key: KEY, created: true }); - db.getFirstAsync.mockRejectedValue(new Error('file is not a database')); + decryptFails(); } it('is reported as its own error rather than a generic open failure', async () => { @@ -167,7 +292,7 @@ describe('a journal that no key can open', () => { // corruption or something else - not a journal from another phone. Deleting // the key here would destroy a database that might still be readable. getKey.mockResolvedValue({ key: KEY, created: false }); - db.getFirstAsync.mockRejectedValue(new Error('file is not a database')); + decryptFails(); await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); expect(deleteKey).not.toHaveBeenCalled(); @@ -193,6 +318,61 @@ describe('closeJournalDatabase', () => { await expect(closeJournalDatabase()).resolves.toBeUndefined(); expect(db.closeAsync).not.toHaveBeenCalled(); }); + + /** + * Holds closeAsync open until the test lets it finish, so "during the close" + * is an actual window rather than a hope about microtask ordering. + */ + function slowClose() { + let release!: () => void; + const closed = new Promise((resolve) => { + release = resolve; + }); + db.closeAsync.mockImplementation(() => closed); + return release; + } + + it('does not let a concurrent open start a second connection mid-close', async () => { + // The unlock gate closes from an AppState listener, so this interleaving + // happens every time the app is backgrounded with a screen mid-query. + await getJournalDatabase(); + const release = slowClose(); + + const closing = closeJournalDatabase(); + const reopening = getJournalDatabase(); + + // Still one open: the second must be waiting on the close, not racing it. + // Before the fix openPromise was already null here, so this was a fresh + // open against a file the first connection had not released. + await Promise.resolve(); + expect(openDatabaseAsync).toHaveBeenCalledTimes(1); + + release(); + await closing; + await reopening; + + expect(openDatabaseAsync).toHaveBeenCalledTimes(2); + }); + + it('waits for a close another caller started before reporting closed', async () => { + // destroyJournalDatabase depends on this: it deletes the file straight + // after, and expo-sqlite refuses to delete one that is still open. + await getJournalDatabase(); + const release = slowClose(); + + const first = closeJournalDatabase(); + let secondFinished = false; + const second = closeJournalDatabase().then(() => { + secondFinished = true; + }); + + await Promise.resolve(); + expect(secondFinished).toBe(false); + + release(); + await Promise.all([first, second]); + expect(secondFinished).toBe(true); + }); }); describe('destroyJournalDatabase', () => { @@ -217,9 +397,79 @@ describe('destroyJournalDatabase', () => { }); it('still removes the key when the file was already gone', async () => { - deleteDatabaseAsync.mockRejectedValue(new Error('no such file')); + // expo-sqlite throws for a missing file rather than resolving. Both + // platforms word it this way; see isDatabaseAlreadyGone. + deleteDatabaseAsync.mockRejectedValue(new Error("Database 'journal.db' not found")); await expect(destroyJournalDatabase()).resolves.toBeUndefined(); expect(deleteKey).toHaveBeenCalled(); }); + + it('keeps the key when the file is still open, rather than stranding it', async () => { + // #135. The native layer refuses to delete a database that is still open, + // and it throws to say so - the same way it throws when the file is + // missing. Swallowing both destroyed the key while leaving the file: a + // journal no key can open, produced by the control that exists to escape + // that state, and reported as success. + deleteDatabaseAsync.mockRejectedValue( + new Error("Unable to delete database 'journal.db' that is currently open. Close it prior to deletion.") + ); + + await expect(destroyJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + expect(deleteKey).not.toHaveBeenCalled(); + }); + + it('keeps the key when the file could not be unlinked', async () => { + deleteDatabaseAsync.mockRejectedValue( + new Error("Unable to delete the database file for 'journal.db' database") + ); + + await expect(destroyJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + expect(deleteKey).not.toHaveBeenCalled(); + }); + + it('tells the user nothing was erased, because nothing was', async () => { + // A "delete my data" control that fails silently is worse than one that + // fails: the user walks away believing the journal is gone. + deleteDatabaseAsync.mockRejectedValue(new Error('currently open')); + + await expect(destroyJournalDatabase()).rejects.toThrow(/still readable/); + }); + + it('does not delete the file while a close someone else started is running', async () => { + // The realistic race, not a contrived one: the unlock gate closes on + // background, and the user taps "Delete all my data" while that close is + // still in flight. destroyJournalDatabase's own close then finds nothing + // cached and, before the fix, returned straight away - so the delete landed + // on a file that was still open, and the key went with it. + await getJournalDatabase(); + + let closed = false; + let release!: () => void; + const closing = new Promise((resolve) => { + release = resolve; + }); + db.closeAsync.mockImplementation(async () => { + await closing; + closed = true; + }); + + let deletedWhileOpen = false; + deleteDatabaseAsync.mockImplementation(async () => { + if (!closed) deletedWhileOpen = true; + }); + + // The gate's close, left in flight. + const gateClose = closeJournalDatabase(); + + const destroying = destroyJournalDatabase(); + await Promise.resolve(); + expect(deleteDatabaseAsync).not.toHaveBeenCalled(); + + release(); + await Promise.all([gateClose, destroying]); + + expect(deletedWhileOpen).toBe(false); + expect(deleteKey).toHaveBeenCalled(); + }); }); diff --git a/mobile/src/lib/db/database.ts b/mobile/src/lib/db/database.ts index 6fe3826..dfc32ce 100644 --- a/mobile/src/lib/db/database.ts +++ b/mobile/src/lib/db/database.ts @@ -58,6 +58,17 @@ export class UnrecoverableJournalError extends Error { */ let openPromise: Promise | null = null; +/** + * The close in flight, if there is one. + * + * `closeJournalDatabase` has to drop the cached open *before* it awaits + * `closeAsync`, or a caller arriving mid-close would be handed a handle that is + * already shutting down. That leaves a window where nothing records that the + * file is still owned, and `getJournalDatabase` would open a second connection + * across it. This is what fills the window. + */ +let closePromise: Promise | null = null; + async function open(): Promise { // expo-sqlite runs on web via wasm, but SQLCipher does not: the docs list it // for Android, iOS and macOS only. Falling back to plain SQLite here would @@ -78,6 +89,10 @@ async function open(): Promise { // Has to be the first statement executed against the connection. await db.execAsync(rawKeyPragma(key)); + // Before assertReadable, which cannot tell an encrypted file it decrypted + // from a plaintext one it never had to. + await assertEncrypted(db); + await assertReadable(db, created); await db.execAsync('PRAGMA journal_mode = WAL'); @@ -88,20 +103,65 @@ async function open(): Promise { // Do not leave a half-configured handle behind for the next caller. await db.closeAsync().catch(() => undefined); if (cause instanceof UnrecoverableJournalError) throw cause; + // Already both specific and accurate - re-wrapping would bury the one + // sentence that says what to do about it. + if (cause instanceof DatabaseUnavailableError) throw cause; throw new DatabaseUnavailableError('Could not open the journal database.', { cause }); } return db; } +/** + * Refuses to carry on when the binary has no SQLCipher in it. + * + * `PRAGMA key` is not evidence of anything. SQLite's response to a pragma it + * does not recognise is to ignore it - no error, no warning, no rows - so on a + * plain SQLite build the keying statement above succeeds, every read and write + * after it succeeds, and the journal is written in cleartext while the app + * behaves in every observable way as though it were not. That is the failure + * this project can least afford to let pass quietly, and issue #130 is it. + * + * `PRAGMA cipher_version` is the discriminator, and it works *because* of the + * same rule that causes the bug. SQLCipher implements it and answers with a + * single row holding its version string; stock SQLite has never heard of it and + * so returns no rows at all. Absence is the signal, which is why this tests the + * value rather than waiting for a throw - neither build throws. + * + * The two sides of that are checkable in this repo rather than taken on trust: + * `vendor/sqlcipher/sqlite3.c` in expo-sqlite answers `cipher_version` from its + * pragma table, and `vendor/sqlite3/sqlite3.c` - the source compiled in when + * `useSQLCipher` is absent - does not contain the string. + * + * It throws rather than warning. 0017 already settled the principle for web: a + * downgrade from encrypted to plaintext is not a reduced service this app + * offers, so there is no degraded mode to continue into, and a warning is a + * line in a log nobody reads while the journal is written in the clear + * regardless. Nor can this strand a user on a correct build - whether SQLCipher + * is compiled in is fixed when the binary is built, so it fails identically on + * every launch of a misbuilt app and on none of a good one. It surfaces on the + * machine of whoever skipped `npx expo prebuild`, which is where it is fixable. + * + * What no test on a laptop can show is the *passing* side: node:sqlite is stock + * SQLite, so CI can only ever prove that a non-SQLCipher build is rejected. That + * a real SQLCipher build satisfies this needs a device - issue #101. + */ +async function assertEncrypted(db: SQLiteDatabase): Promise { + 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` set for expo-sqlite in app.json and run `npx expo prebuild`; Expo Go cannot run this app.' + ); + } +} + /** * Reads a page, so a key that does not fit this file 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 - SQLite ignores unknown pragmas - and this read succeeds - * against a plaintext file. Detecting that needs `PRAGMA cipher_version`; see - * issue #130. + * This says nothing about whether the file is encrypted - a plaintext database + * reads perfectly. `assertEncrypted` above runs first for that reason. * * What it does catch, given `keyWasCreated`, is the restored-backup case. A key * we just minted cannot fail to read a database we just created, so if it fails @@ -128,21 +188,70 @@ async function assertReadable(db: SQLiteDatabase, keyWasCreated: boolean): Promi /** The shared database handle, opening and migrating it on first call. */ export function getJournalDatabase(): Promise { - openPromise ??= open().catch((error: unknown) => { + openPromise ??= openOnceClosed().catch((error: unknown) => { openPromise = null; throw error; }); return openPromise; } +/** + * Waits out a close still in progress, then opens. + * + * Without the wait, a `getJournalDatabase()` arriving while `closeAsync` is + * still running opens a second connection to a file the first one has not let + * go of. Two things then go wrong, and neither is theoretical now that the + * unlock gate closes the database from an AppState listener - backgrounding the + * app races every screen that is mid-query. + * + * The first is that locking stops locking. The gate closes the handle so the + * decrypted database and its key do not outlive the foreground; an open that + * slips in behind it hands back a fresh decrypted handle moments later, and the + * cached promise now points at *that* one, so the close completes against a + * connection nobody is using any more. + * + * The second is `destroyJournalDatabase`, which cannot delete a file while a + * connection to it is cached - see the note there. + * + * A close that fails is still a close that finished, so the rejection is + * swallowed here rather than blocking every future open on one bad teardown. + */ +async function openOnceClosed(): Promise { + await closePromise?.catch(() => undefined); + return open(); +} + /** Closes the handle if one is open. Safe to call when none is. */ export async function closeJournalDatabase(): Promise { const pending = openPromise; - if (!pending) return; + if (!pending) { + // Nothing of ours to close, but a close another caller started may still be + // running. Returning now would report "closed" while the file is open, + // which is exactly the lie destroyJournalDatabase must not be told. + await closePromise; + return; + } + + // Dropped before the await so nobody is handed a handle that is on its way + // out; closePromise below is what keeps the file accounted for meanwhile. openPromise = null; - const db = await pending.catch(() => null); - await db?.closeAsync(); + const closing = (async () => { + const db = await pending.catch(() => null); + await db?.closeAsync(); + })(); + + // Published in the same synchronous turn as the line above - the body of + // `closing` suspends at its first await - so there is no tick in which both + // are null and an open could slip between them. + closePromise = closing; + + try { + await closing; + } finally { + // Only if it is still ours: a later close may already have replaced it. + if (closePromise === closing) closePromise = null; + } } /** @@ -161,9 +270,54 @@ export async function closeJournalDatabase(): Promise { * * Both orders destroy the data. Only one of them leaves the app somewhere the * user can go. + * + * Ordering the two steps is not on its own enough, which is what issue #135 was + * about. If the first step fails and the second runs anyway, the order bought + * nothing: the outcome is the same key-less file, arrived at without the process + * needing to die at all. So a failed delete aborts here rather than being + * swallowed, and only a file that was already absent counts as a delete that + * succeeded. */ export async function destroyJournalDatabase(): Promise { await closeJournalDatabase(); - await SQLite.deleteDatabaseAsync(DATABASE_NAME).catch(() => undefined); + + try { + await SQLite.deleteDatabaseAsync(DATABASE_NAME); + } catch (cause) { + if (!isDatabaseAlreadyGone(cause)) { + // Stop here, before the key. The file is still on disk, and deleting the + // key on top of it produces the one outcome the ordering above exists to + // prevent - reached, absurdly, by the control that is meant to be the way + // out of it. + throw new DatabaseUnavailableError( + 'The journal could not be erased, so nothing was erased and it is still readable. Please try again.', + { cause } + ); + } + } + await deleteDatabaseKey(); } + +/** + * True for the single delete failure that is not one: there was no file. + * + * This has to be a guess about a string, and it is worth being clear about why. + * expo-sqlite throws for a missing file (`DatabaseNotFoundException`) exactly as + * it throws when the database is still open (`DeleteDatabaseException`) or when + * the unlink fails outright (`DeleteDatabaseFileException`), and the SDK + * documents none of the three. There is no shared code to switch on either: iOS + * files all three under `E_SQLITE_DELETE_DATABASE`. The message is what is left, + * and it is at least consistent across platforms - "Database not found" + * on Android, "Database not found" on iOS. + * + * So this is deliberately narrow, and deliberately fragile in the safe + * direction. If a future SDK rewords the message, the match fails and erasing an + * already-empty journal reports an error instead of finishing quietly: wrong, + * visible, and harmless - the next launch starts clean anyway. The failure this + * replaces was the other kind. `.catch(() => undefined)` treated "still open" + * as success, destroyed the key, left the file, and said it had worked. + */ +function isDatabaseAlreadyGone(error: unknown): boolean { + return error instanceof Error && /not found/i.test(error.message); +} From e8b4dbf7f1a241a0bd05641d7aa64fa9e6b6065e Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Thu, 20 Aug 2026 02:06:41 +0000 Subject: [PATCH 12/13] Give the lock screen a way out of every state it can reach Five gaps, all of them dead ends for the user rather than wrong results. A rejecting native call left the gate on 'checking' for ever: an unlabelled spinner, no button, no way to the journal, reproducing on every launch. The realistic trigger is expo-local-authentication not being linked, which is what Expo Go looks like from here - so the person most likely to hit it was a new contributor following the old README. unlock.ts now maps a throw to an 'unavailable' outcome rather than letting it escape. Deliberately not folded into 'none' or 'unsupported': those two open the app, which is the right answer when we know there is no lock and the wrong one when we could not find out. Not knowing fails closed. Backgrounding set 'locked' unconditionally, so someone on a phone with no lock who pressed Continue came back to "Unlock with your phone" - about a phone with no lock - and looped through Continue again on every resume, two taps at a time, aimed exactly at the people 0018 says are most likely to have no passcode. The re-lock now only applies to users who unlocked, and returning re-runs the check, so setting a passcode while away works. It also stops backgrounding wiping the lockout message. passcode_not_set / not_enrolled / not_available were shown as "you can try again", which no amount of trying fixes; they route to the no-device-lock screen now. privacy-cover.tsx covers the app on 'inactive' so the journal is not in the task-switcher snapshot. It asks isUnlockPromptOnScreen() first, because our own prompt also makes the app inactive and there are reports of the paired 'active' never arriving - which would strand an opaque cover over the lock screen. This shortens the race rather than removing it; the version without one needs a native window from applicationWillResignActive. #136 stays open until someone watches a real device, and Android needs FLAG_SECURE. The 0018 placeholder is now marked as one, in the rendered branch and in the test, so whoever implements the real decision reads a failure as "the placeholder changed" rather than "a requirement broke". Both headings use variant="h1" so they carry a heading role, and the failure message is announced on iOS too, where accessibilityLiveRegion does nothing. Co-Authored-By: Claude Opus 5 (1M context) --- mobile/src/app/_layout.tsx | 4 + mobile/src/components/privacy-cover.test.tsx | 115 ++++++++++ mobile/src/components/privacy-cover.tsx | 81 +++++++ mobile/src/components/unlock-gate.test.tsx | 214 +++++++++++++++++-- mobile/src/components/unlock-gate.tsx | 138 ++++++++++-- mobile/src/lib/auth/unlock.test.ts | 64 +++++- mobile/src/lib/auth/unlock.ts | 100 +++++++-- 7 files changed, 667 insertions(+), 49 deletions(-) create mode 100644 mobile/src/components/privacy-cover.test.tsx create mode 100644 mobile/src/components/privacy-cover.tsx diff --git a/mobile/src/app/_layout.tsx b/mobile/src/app/_layout.tsx index 251baf4..6ee2219 100644 --- a/mobile/src/app/_layout.tsx +++ b/mobile/src/app/_layout.tsx @@ -10,6 +10,7 @@ import { useColorScheme } from 'react-native'; import { AnimatedSplashOverlay } from '@/components/animated-icon'; import AppTabs from '@/components/app-tabs'; +import { PrivacyCover } from '@/components/privacy-cover'; import { UnlockGate } from '@/components/unlock-gate'; import { NAV_THEME } from '@/lib/theme'; @@ -31,6 +32,9 @@ export default function TabLayout() { providers to sit on top of the rest of the tree. https://reactnativereusables.com/docs/installation/manual */} + {/* Last of all, so that what it hides from the task switcher includes + anything a dialog put on top through the portal above. */} + ); } diff --git a/mobile/src/components/privacy-cover.test.tsx b/mobile/src/components/privacy-cover.test.tsx new file mode 100644 index 0000000..218bfa8 --- /dev/null +++ b/mobile/src/components/privacy-cover.test.tsx @@ -0,0 +1,115 @@ +import { act, render, screen } from '@testing-library/react-native'; +import { AppState, Platform } from 'react-native'; + +import { PrivacyCover } from './privacy-cover'; +import { isUnlockPromptOnScreen } from '@/lib/auth/unlock'; + +jest.mock('@/lib/auth/unlock', () => ({ + isUnlockPromptOnScreen: jest.fn(() => false), +})); + +const promptOnScreen = isUnlockPromptOnScreen as jest.Mock; + +const originalOS = Platform.OS; +function setPlatform(os: typeof Platform.OS) { + (Platform as { OS: typeof Platform.OS }).OS = os; +} + +/** Lets a test drive AppState by hand; the real one needs a running app. */ +function captureAppState() { + const listeners: ((state: string) => void)[] = []; + + jest.spyOn(AppState, 'addEventListener').mockImplementation((( + _event: string, + handler: (state: string) => void + ) => { + listeners.push(handler); + return { remove: jest.fn() }; + }) as never); + + return { + async emit(next: string) { + await act(async () => listeners.forEach((handler) => handler(next))); + }, + get subscribed() { + return listeners.length > 0; + }, + }; +} + +beforeEach(() => { + jest.clearAllMocks(); + setPlatform('ios'); + promptOnScreen.mockReturnValue(false); +}); + +afterEach(() => { + jest.restoreAllMocks(); + setPlatform(originalOS); +}); + +describe('PrivacyCover', () => { + it('shows nothing while the app is in the foreground', async () => { + captureAppState(); + + await render(); + + expect(screen.queryByTestId('privacy-cover')).toBeNull(); + }); + + it('covers the app on inactive, before iOS takes its switcher photograph', async () => { + const appState = captureAppState(); + + await render(); + // 'inactive' rather than 'background' on purpose: iOS raises it as the + // multitasking view starts to appear, which is the only part of this the + // render can win. + await appState.emit('inactive'); + + expect(screen.getByTestId('privacy-cover')).toBeVisible(); + }); + + it('covers on background too, for platforms that never say inactive', async () => { + setPlatform('android'); + const appState = captureAppState(); + + await render(); + await appState.emit('background'); + + expect(screen.getByTestId('privacy-cover')).toBeVisible(); + }); + + it('gets out of the way as soon as the app is back', async () => { + const appState = captureAppState(); + + await render(); + await appState.emit('inactive'); + await appState.emit('active'); + + expect(screen.queryByTestId('privacy-cover')).toBeNull(); + }); + + it('leaves the OS unlock sheet alone, which iOS also reports as inactive', async () => { + // The failure this avoids is the bad one. Face ID makes the app inactive, + // and the matching 'active' is not always delivered afterwards - covering + // here could leave an opaque view over the lock screen with the Unlock + // button underneath it and no way back. Nothing leaks by skipping: that + // sheet only ever opens over the lock screen. + promptOnScreen.mockReturnValue(true); + const appState = captureAppState(); + + await render(); + await appState.emit('inactive'); + + expect(screen.queryByTestId('privacy-cover')).toBeNull(); + }); + + it('does not watch the web build, which has no switcher and no journal', async () => { + setPlatform('web'); + const appState = captureAppState(); + + await render(); + + expect(appState.subscribed).toBe(false); + }); +}); diff --git a/mobile/src/components/privacy-cover.tsx b/mobile/src/components/privacy-cover.tsx new file mode 100644 index 0000000..b44fdd5 --- /dev/null +++ b/mobile/src/components/privacy-cover.tsx @@ -0,0 +1,81 @@ +// The cover that goes over the app on its way out of the foreground. +// +// Issue #136: iOS photographs the app as it leaves the foreground, shows that +// picture as the card in the task switcher, and keeps it in the app's Snapshots +// directory on disk. unlock-gate.tsx re-locks on 'background', but that is the +// same moment the photograph is taken - a state change, a re-render and a +// native view update all have to land inside that window, and they do not +// reliably do it. What is left in the switcher, and on disk, is the journal. +// +// This is deliberately not part of the gate's state machine. It is mounted for +// the life of the app, it holds one boolean, and it flips on 'inactive', which +// iOS sends when the multitasking view begins to appear - earlier than +// 'background', so the re-render gets a longer run at the same race. Keeping it +// separate is what makes 'inactive' safe to use at all: the gate must not +// re-lock on 'inactive', because the OS's own unlock sheet raises that event +// too and locking on it would fight the prompt it just opened. +// +// Two things this is not: +// +// - It is not a guarantee. The race is shorter, not gone. The version with no +// race puts a native overlay on the UIWindow from +// applicationWillResignActive, which means a config plugin and a +// development build. #136 should stay open until someone has looked at the +// switcher on a real phone. +// - It is not Android's answer. Android does not raise 'inactive', and its +// recents thumbnail is controlled by FLAG_SECURE rather than by anything +// drawn in JavaScript. Covering on 'background' there is better than +// nothing and no more than that. + +import { useEffect, useState } from 'react'; +import { AppState, Platform, View } from 'react-native'; + +import { isUnlockPromptOnScreen } from '@/lib/auth/unlock'; + +export function PrivacyCover() { + const [covered, setCovered] = useState(false); + + useEffect(() => { + // Web has no task switcher to hide from, and per db/database.ts no journal + // data to hide. + if (Platform.OS === 'web') return; + + const subscription = AppState.addEventListener('change', (next) => { + if (next === 'active') { + setCovered(false); + return; + } + + // Nothing else is worth reacting to: 'unknown' and 'extension' are not + // the app being put away. + if (next !== 'inactive' && next !== 'background') return; + + // Stay out of the way while the OS's unlock sheet is up. iOS reports that + // sheet as the app going inactive as well, and covering for it risks the + // worst outcome available here - there are reports of the matching + // 'active' never arriving after an LAContext prompt, which would leave an + // opaque view sitting over the lock screen with no button underneath it + // and no way back. Skipping leaks nothing: the sheet only ever opens over + // the lock screen, which has no journal on it to photograph. + if (isUnlockPromptOnScreen()) return; + + setCovered(true); + }); + + return () => subscription.remove(); + }, []); + + if (!covered) return null; + + return ( + + ); +} diff --git a/mobile/src/components/unlock-gate.test.tsx b/mobile/src/components/unlock-gate.test.tsx index c536d57..47928d5 100644 --- a/mobile/src/components/unlock-gate.test.tsx +++ b/mobile/src/components/unlock-gate.test.tsx @@ -1,5 +1,5 @@ import { act, render, screen, userEvent, waitFor } from '@testing-library/react-native'; -import { AppState, Platform, Text as RNText } from 'react-native'; +import { AccessibilityInfo, AppState, Platform, Text as RNText } from 'react-native'; import { UnlockGate } from './unlock-gate'; import { checkUnlockAvailability, requestUnlock } from '@/lib/auth/unlock'; @@ -30,6 +30,25 @@ function Journal() { return Journal contents; } +/** Lets a test drive AppState by hand; the real one needs a running app. */ +function captureAppState() { + const listeners: ((state: string) => void)[] = []; + + jest.spyOn(AppState, 'addEventListener').mockImplementation((( + _event: string, + handler: (state: string) => void + ) => { + listeners.push(handler); + return { remove: jest.fn() }; + }) as never); + + return { + async emit(next: string) { + await act(async () => listeners.forEach((handler) => handler(next))); + }, + }; +} + beforeEach(() => { jest.clearAllMocks(); setPlatform('ios'); @@ -37,7 +56,10 @@ beforeEach(() => { unlock.mockResolvedValue({ status: 'unlocked' }); }); -afterEach(() => setPlatform(originalOS)); +afterEach(() => { + jest.restoreAllMocks(); + setPlatform(originalOS); +}); describe('UnlockGate', () => { it('shows nothing of what it wraps until the device says yes', async () => { @@ -89,6 +111,49 @@ describe('UnlockGate', () => { expect(await screen.findByText('Journal contents')).toBeVisible(); }); + it('offers a way forward when the check itself throws', async () => { + // Not a hypothetical: expo-local-authentication throws "Cannot find native + // module" in Expo Go, which is what a contributor who has not made a + // development build is running. unlock.ts turns that into 'unavailable'; + // what matters here is that the gate gives the user a way forward rather + // than sitting on its spinner for ever with no text and no button. + availability.mockResolvedValueOnce({ kind: 'unavailable' }); + + await render( + + + + ); + + expect( + await screen.findByText("We couldn't check your phone's lock. You can try again.") + ).toBeVisible(); + expect(screen.getByRole('button', { name: 'Unlock' })).toBeVisible(); + expect(screen.queryByText('Journal contents')).toBeNull(); + }); + + it('recovers when the prompt could not run and the next attempt works', async () => { + const user = userEvent.setup(); + unlock.mockResolvedValueOnce({ status: 'unavailable' }); + + await render( + + + + ); + + expect( + await screen.findByText("We couldn't check your phone's lock. You can try again.") + ).toBeVisible(); + + // The re-entrancy guard has to have been released, or the button below + // would do nothing. + unlock.mockResolvedValueOnce({ status: 'unlocked' }); + await user.press(screen.getByRole('button', { name: 'Unlock' })); + + expect(await screen.findByText('Journal contents')).toBeVisible(); + }); + it('does not blame the user when authentication fails', async () => { unlock.mockResolvedValue({ status: 'failed', reason: 'authentication_failed' }); @@ -116,7 +181,12 @@ describe('UnlockGate', () => { expect(await screen.findByText(/try again in a moment/)).toBeVisible(); }); - it('lets someone through on a phone with no lock, and says so', async () => { + // 0018 is open, and this test records what the placeholder does rather than + // what the app has to do. Refusing to open, nudging once and offering a + // shortcut into Settings are all still on the table; whoever settles 0018 + // should expect to rewrite this alongside the screen, and should read a + // failure here as "the placeholder changed", not "a requirement broke". + it('lets someone through on a phone with no lock, and says so (0018 placeholder)', async () => { const user = userEvent.setup(); availability.mockResolvedValue({ kind: 'none' }); @@ -133,15 +203,61 @@ describe('UnlockGate', () => { expect(await screen.findByText('Journal contents')).toBeVisible(); }); + it('does not offer a retry for something retrying cannot fix', async () => { + // The check said there was a lock and the prompt disagreed - a passcode + // removed while the app sat in the background, most likely. "You can try + // again" would be untrue: the next tap meets the same missing lock. + unlock.mockResolvedValue({ status: 'no-device-lock' }); + + await render( + + + + ); + + expect(await screen.findByText('Your phone has no lock set')).toBeVisible(); + expect(screen.queryByText("That didn't work. You can try again.")).toBeNull(); + }); + + it.each([ + ['locked', { kind: 'biometric' }, { status: 'cancelled' }, 'Your journal is locked'], + ['no-device-lock', { kind: 'none' }, undefined, 'Your phone has no lock set'], + ])('gives the %s screen a heading a screen reader can find', async (_name, kind, outcome, heading) => { + availability.mockResolvedValue(kind); + if (outcome) unlock.mockResolvedValue(outcome); + + await render( + + + + ); + + // Bare styled Text is a paragraph that happens to be large. On a screen + // with nothing else on it, the heading is the only landmark there is. + expect(await screen.findByRole('heading', { name: heading as string })).toBeVisible(); + }); + + it('says the failure out loud on iOS, where the live region is inert', async () => { + const announce = jest + .spyOn(AccessibilityInfo, 'announceForAccessibility') + .mockImplementation(() => {}); + unlock.mockResolvedValue({ status: 'failed', reason: 'authentication_failed' }); + + await render( + + + + ); + + await screen.findByText("That didn't work. You can try again."); + + // accessibilityLiveRegion is Android-only, so without this the one + // explanation the screen offers is silent on the platform Face ID is on. + expect(announce).toHaveBeenCalledWith("That didn't work. You can try again."); + }); + it('re-locks when the app goes to the background', async () => { - const listeners: ((state: string) => void)[] = []; - jest.spyOn(AppState, 'addEventListener').mockImplementation((( - _event: string, - handler: (state: string) => void - ) => { - listeners.push(handler); - return { remove: jest.fn() }; - }) as never); + const appState = captureAppState(); await render( @@ -152,7 +268,7 @@ describe('UnlockGate', () => { expect(await screen.findByText('Journal contents')).toBeVisible(); // Without this the gate is a splash screen, not a lock. - await act(async () => listeners.forEach((handler) => handler('background'))); + await appState.emit('background'); await waitFor(() => expect(screen.queryByText('Journal contents')).toBeNull()); expect(screen.getByText('Your journal is locked')).toBeVisible(); @@ -161,8 +277,80 @@ describe('UnlockGate', () => { // is still in memory and any caller can still read, which makes 0015's // WHEN_UNLOCKED_THIS_DEVICE_ONLY a first-launch property and nothing more. expect(closeDatabase).toHaveBeenCalled(); + }); + + it('keeps what it was explaining when the app comes back', async () => { + unlock.mockResolvedValue({ status: 'locked-out' }); + const appState = captureAppState(); + + await render( + + + + ); + + expect(await screen.findByText(/try again in a moment/)).toBeVisible(); + + await appState.emit('background'); + await appState.emit('active'); + + // Re-locking an already-locked screen used to overwrite the message, so + // someone who checked the time during a lockout came back to a bare "Your + // journal is locked" and no explanation of why their last try had failed. + expect(screen.getByText(/try again in a moment/)).toBeVisible(); + }); - jest.restoreAllMocks(); + it('does not tell someone with no phone lock to unlock with their phone', async () => { + const user = userEvent.setup(); + availability.mockResolvedValue({ kind: 'none' }); + const appState = captureAppState(); + + await render( + + + + ); + + await user.press(await screen.findByText('Continue')); + expect(await screen.findByText('Journal contents')).toBeVisible(); + + await appState.emit('background'); + expect(screen.queryByText('Journal contents')).toBeNull(); + + await appState.emit('active'); + + // The whole point. "Your journal is locked / Unlock with your phone to open + // it" is a false statement on a phone with no lock, and its button can only + // fail - two taps back to this same screen, on every resume, for the people + // 0018 is about. + expect(await screen.findByText('Your phone has no lock set')).toBeVisible(); + expect(screen.queryByText('Your journal is locked')).toBeNull(); + expect(unlock).not.toHaveBeenCalled(); + }); + + it('notices a lock set while the app was away', async () => { + const user = userEvent.setup(); + availability.mockResolvedValue({ kind: 'none' }); + const appState = captureAppState(); + + await render( + + + + ); + + await user.press(await screen.findByText('Continue')); + expect(await screen.findByText('Journal contents')).toBeVisible(); + + // The screen they just read recommends setting a passcode, and going to + // Settings to do it is what put the app in the background. Coming back to + // "Your phone has no lock set" would be wrong, and would undo the nudge. + await appState.emit('background'); + availability.mockResolvedValue({ kind: 'biometric' }); + await appState.emit('active'); + + expect(await screen.findByText('Journal contents')).toBeVisible(); + expect(unlock).toHaveBeenCalled(); }); it('does not close a database the web build never opened', async () => { diff --git a/mobile/src/components/unlock-gate.tsx b/mobile/src/components/unlock-gate.tsx index 7ed26ae..2f3e15d 100644 --- a/mobile/src/components/unlock-gate.tsx +++ b/mobile/src/components/unlock-gate.tsx @@ -11,7 +11,7 @@ import { useCallback, useEffect, useRef, useState } from 'react'; import type { ReactNode } from 'react'; -import { ActivityIndicator, AppState, Platform, View } from 'react-native'; +import { AccessibilityInfo, ActivityIndicator, AppState, Platform, View } from 'react-native'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; @@ -21,10 +21,27 @@ import { closeJournalDatabase } from '@/lib/db/database'; type GateState = | { status: 'checking' } | { status: 'locked'; message: string | null } - | { status: 'unlocked' } + | { + status: 'unlocked'; + /** + * True when the user got here by pressing Continue on the no-device-lock + * screen rather than by unlocking. Remembered because the re-lock below + * has to know: sending this user to "unlock with your phone" would be a + * lie about the phone in their hand. + */ + viaNoDeviceLock?: boolean; + } /** Native device with no lock configured - see the note in unlock.ts. */ | { status: 'no-device-lock' }; +/** + * Shown when we could not ask the device what lock it has, rather than when the + * user failed a prompt. It does not guess at the cause, because it cannot know + * it, and it offers another go because a transient failure is the one case the + * user can do anything about. + */ +const COULD_NOT_CHECK = "We couldn't check your phone's lock. You can try again."; + export type UnlockGateProps = { children: ReactNode; /** Escape hatch for tests and Storybook-style previews. */ @@ -57,10 +74,21 @@ export function UnlockGate({ children, skip = false }: UnlockGateProps) { if (passthrough || prompting.current) return; prompting.current = true; + // No try/catch here on purpose. lib/auth/unlock.ts turns a native module + // that is missing or throwing into an 'unavailable' outcome, so everything + // this function has to handle arrives as a value. Letting an exception + // reach here instead would leave the gate on 'checking' for ever: an + // unlabelled spinner, no button, and no way to the journal, on every + // launch - which is what running in Expo Go used to look like. try { const availability = await checkUnlockAvailability(); if (!mounted.current) return; + if (availability.kind === 'unavailable') { + setState({ status: 'locked', message: COULD_NOT_CHECK }); + return; + } + if (availability.kind === 'unsupported' || availability.kind === 'none') { setState({ status: 'no-device-lock' }); return; @@ -82,6 +110,14 @@ export function UnlockGate({ children, skip = false }: UnlockGateProps) { message: 'Too many attempts. Your phone will let you try again in a moment.', }); break; + case 'no-device-lock': + // The lock went away between the check above and the prompt, or the + // OS disagreed with the check. Either way there is nothing to retry. + setState({ status: 'no-device-lock' }); + break; + case 'unavailable': + setState({ status: 'locked', message: COULD_NOT_CHECK }); + break; default: setState({ status: 'locked', @@ -97,6 +133,14 @@ export function UnlockGate({ children, skip = false }: UnlockGateProps) { void attemptUnlock(); }, [attemptUnlock]); + // The AppState listener is subscribed once and would otherwise close over the + // state as it was at subscription time. A ref, rather than re-subscribing on + // every state change. + const latest = useRef(state); + useEffect(() => { + latest.current = state; + }, [state]); + // Re-lock when the app leaves the foreground. Without this the gate is a // one-time splash rather than a lock: hand someone an already-open phone and // the journal is simply there. @@ -104,25 +148,62 @@ export function UnlockGate({ children, skip = false }: UnlockGateProps) { if (passthrough) return; const subscription = AppState.addEventListener('change', (next) => { - if (next === 'background') { - setState({ status: 'locked', message: null }); - - // Close the database too, not just the view. The gate is the only thing - // that knows the app is meant to be locked, and locking only the UI - // leaves db/database.ts holding a decrypted handle and the key in memory - // for the life of the process - at which point 0015's - // WHEN_UNLOCKED_THIS_DEVICE_ONLY is a property of the first launch and - // nothing after it, because the keychain is never asked again. Dropping - // the handle here is what makes that option mean something. - // - // Failures are swallowed on purpose: this runs on the way out of the - // foreground, there is nobody to tell, and the next unlock re-opens. - void closeJournalDatabase().catch(() => undefined); + if (next === 'active') { + // Only ever the resume half of the no-device-lock case below; every + // other path leaves a screen with a button on it rather than a + // spinner, and attemptUnlock's own guard covers a check still in + // flight. + if (latest.current.status === 'checking') void attemptUnlock(); + return; } + + if (next !== 'background') return; + + const current = latest.current; + + // Only the unlocked state is re-locked. Anything already showing a lock + // screen is left exactly as it was, because overwriting it threw the + // message away: someone who backgrounded the app during a lockout came + // back to a bare "Your journal is locked" with nothing left to say why + // their last attempt had not worked. + if (current.status === 'unlocked') { + // Someone who came through the no-device-lock screen has no lock on + // their phone, so "Unlock with your phone to open it" would be false + // and its button could only fail - two taps and the same screen again, + // on every single resume. Re-check instead of assuming: that screen + // recommends setting a passcode, and going to Settings to do it is + // precisely what backgrounds the app. + setState( + current.viaNoDeviceLock ? { status: 'checking' } : { status: 'locked', message: null } + ); + } + + // Close the database too, not just the view. The gate is the only thing + // that knows the app is meant to be locked, and locking only the UI + // leaves db/database.ts holding a decrypted handle and the key in memory + // for the life of the process - at which point 0015's + // WHEN_UNLOCKED_THIS_DEVICE_ONLY is a property of the first launch and + // nothing after it, because the keychain is never asked again. Dropping + // the handle here is what makes that option mean something. + // + // Failures are swallowed on purpose: this runs on the way out of the + // foreground, there is nobody to tell, and the next unlock re-opens. + void closeJournalDatabase().catch(() => undefined); }); return () => subscription.remove(); - }, [passthrough]); + }, [passthrough, attemptUnlock]); + + const failureMessage = state.status === 'locked' ? state.message : null; + + // accessibilityLiveRegion below is Android-only, which leaves the message + // silent on iOS - where Face ID is, so where a refusal is most likely to need + // explaining. This is the iOS half of the same job. It is a no-op when no + // screen reader is running. + useEffect(() => { + if (Platform.OS !== 'ios' || !failureMessage) return; + AccessibilityInfo.announceForAccessibility(failureMessage); + }, [failureMessage]); if (state.status === 'unlocked') return <>{children}; @@ -137,8 +218,23 @@ export function UnlockGate({ children, skip = false }: UnlockGateProps) { return ( {state.status === 'no-device-lock' ? ( + // PLACEHOLDER, not a decision. 0018 is open: whether the app refuses to + // open, explains and continues, nudges once, or hands the user a + // shortcut into their phone's settings needs a named owner, and this + // screen is only the least-bad of those chosen so something true could + // be said in the meantime. Whoever settles 0018 should expect to + // replace all of it - the copy, the Continue button, and the tests + // covering them, which are written as "what the placeholder does" + // rather than as a requirement. <> - Your phone has no lock set + {/* variant h1 for the heading role and aria-level, as on the landing + screen; the classes walk its size and weight back down. Without a + variant this is a paragraph of text that happens to be large, and + a screen reader has no heading to jump to on the one screen where + there is nothing else to orient by. */} + + Your phone has no lock set + Your journal is still encrypted on this phone. Setting a passcode or fingerprint in your phone's settings adds another layer, and we'd recommend it. @@ -146,13 +242,15 @@ export function UnlockGate({ children, skip = false }: UnlockGateProps) { ) : ( <> - Your journal is locked + + Your journal is locked + Unlock with your phone to open it. diff --git a/mobile/src/lib/auth/unlock.test.ts b/mobile/src/lib/auth/unlock.test.ts index e294a3e..1d05922 100644 --- a/mobile/src/lib/auth/unlock.test.ts +++ b/mobile/src/lib/auth/unlock.test.ts @@ -1,7 +1,7 @@ import * as LocalAuthentication from 'expo-local-authentication'; import { Platform } from 'react-native'; -import { checkUnlockAvailability, requestUnlock } from './unlock'; +import { checkUnlockAvailability, isUnlockPromptOnScreen, requestUnlock } from './unlock'; jest.mock('expo-local-authentication', () => ({ hasHardwareAsync: jest.fn(), @@ -59,6 +59,23 @@ describe('checkUnlockAvailability', () => { await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'unsupported' }); expect(hasHardwareAsync).not.toHaveBeenCalled(); }); + + // The realistic cause is the native module not being in the build at all, + // which is what Expo Go looks like from here now that 0016 requires a + // development build. It must not read as 'none': that one opens the app. + it('reports unavailable, not none, when the hardware check throws', async () => { + hasHardwareAsync.mockRejectedValueOnce( + new Error("Cannot find native module 'ExpoLocalAuthentication'") + ); + + await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'unavailable' }); + }); + + it('reports unavailable when the enrolled-level check throws', async () => { + getEnrolledLevelAsync.mockRejectedValueOnce(new Error('boom')); + + await expect(checkUnlockAvailability()).resolves.toEqual({ kind: 'unavailable' }); + }); }); describe('requestUnlock', () => { @@ -95,10 +112,53 @@ describe('requestUnlock', () => { }); }); + // 'failed' is what the UI turns into "you can try again". None of these three + // can be fixed by trying again - the phone has no lock to authenticate + // against - so offering the retry was offering something that could not work. + it.each(['passcode_not_set', 'not_enrolled', 'not_available'])( + 'reports %s as a missing device lock rather than a retryable failure', + async (error) => { + authenticateAsync.mockResolvedValue({ success: false, error }); + await expect(requestUnlock()).resolves.toEqual({ status: 'no-device-lock' }); + } + ); + it('does not prompt when the device has nothing to prompt with', async () => { getEnrolledLevelAsync.mockResolvedValue(LocalAuthentication.SecurityLevel.NONE); - await expect(requestUnlock()).resolves.toEqual({ status: 'failed', reason: 'none' }); + await expect(requestUnlock()).resolves.toEqual({ status: 'no-device-lock' }); expect(authenticateAsync).not.toHaveBeenCalled(); }); }); + +// privacy-cover.tsx hides the app on 'inactive', and iOS raises 'inactive' for +// this prompt as well as for the task switcher. If the flag were ever left on +// after the sheet closed the cover would stop working; if it were left off +// while the sheet was up the cover could be stranded over the lock screen. +describe('isUnlockPromptOnScreen', () => { + it('is true only while the OS sheet is actually up', async () => { + expect(isUnlockPromptOnScreen()).toBe(false); + + let duringPrompt = false; + authenticateAsync.mockImplementation(async () => { + duringPrompt = isUnlockPromptOnScreen(); + return { success: true }; + }); + + await requestUnlock(); + + expect(duringPrompt).toBe(true); + expect(isUnlockPromptOnScreen()).toBe(false); + }); + + it('clears even when the prompt throws', async () => { + authenticateAsync.mockRejectedValue(new Error("Cannot find native module 'ExpoLocalAuthentication'")); + + // The throw is absorbed into an outcome rather than propagated, but the + // flag still has to come back down: privacy-cover.tsx reads it to decide + // whether it is looking at our own prompt, and a stuck true would leave the + // app uncovered in the task switcher from here on. + await expect(requestUnlock()).resolves.toEqual({ status: 'unavailable' }); + expect(isUnlockPromptOnScreen()).toBe(false); + }); +}); diff --git a/mobile/src/lib/auth/unlock.ts b/mobile/src/lib/auth/unlock.ts index 510ab7e..57f71a1 100644 --- a/mobile/src/lib/auth/unlock.ts +++ b/mobile/src/lib/auth/unlock.ts @@ -25,14 +25,27 @@ export type UnlockAvailability = * sitting in an unlocked keystore. * * 0007 does not say what to do here, because it was written about lockout - * rather than about a device with no lock screen. Someone needs to decide - * whether the app refuses to open, opens with a plain warning, or asks the - * user to set a device passcode - and that is a product call, so this type - * surfaces the case honestly rather than picking one silently. + * rather than about a device with no lock screen. 0018 is the open record + * for that gap: refuse to open, open with a plain warning, or send the user + * to Settings is a product call with no owner yet, so this type surfaces the + * case honestly rather than picking one silently. */ | { readonly kind: 'none' } /** No supported hardware or platform - the web build, mainly. */ - | { readonly kind: 'unsupported' }; + | { readonly kind: 'unsupported' } + /** + * The check itself failed, so we do not know what this device can do. + * + * Deliberately not folded into 'unsupported' or 'none'. Both of those send + * the user to the 0018 screen, which lets them through - the right answer + * when we know there is no lock to prompt with, and the wrong one when we + * simply could not find out. Not knowing has to fail closed. + * + * The realistic cause is expo-local-authentication not being linked, which + * is what running in Expo Go rather than the development build 0016 requires + * looks like from in here. + */ + | { readonly kind: 'unavailable' }; export type UnlockOutcome = | { readonly status: 'unlocked' } @@ -40,14 +53,51 @@ export type UnlockOutcome = | { readonly status: 'cancelled' } /** Too many failed attempts; the OS has temporarily disabled the prompt. */ | { readonly status: 'locked-out' } + /** + * There is no lock on this device to prompt with, so there is nothing to + * retry. Separate from 'failed' because the two need opposite screens: a + * failure invites another go, and this one cannot be fixed by having another + * go - only by changing a phone setting, or by 0018 deciding otherwise. + */ + | { readonly status: 'no-device-lock' } + /** + * We could not ask the device. Unlike 'failed' this is not about the user + * having got it wrong, and unlike 'no-device-lock' it must not open the app. + * @see UnlockAvailability - the 'unavailable' variant + */ + | { readonly status: 'unavailable' } | { readonly status: 'failed'; readonly reason: string }; +/** + * True while the OS's own unlock sheet is on screen. + * + * The sheet takes focus, which iOS reports as the app becoming inactive - the + * same signal privacy-cover.tsx uses to hide the app from the task switcher. + * That component needs to tell the two apart, and this module is the only + * place that knows which one is happening. + */ +let promptOnScreen = false; + +/** @see promptOnScreen */ +export function isUnlockPromptOnScreen(): boolean { + return promptOnScreen; +} + /** What kind of lock, if any, this device can offer us. */ export async function checkUnlockAvailability(): Promise { if (Platform.OS === 'web') return { kind: 'unsupported' }; - const hasHardware = await LocalAuthentication.hasHardwareAsync(); - const level = await LocalAuthentication.getEnrolledLevelAsync(); + // These reject, rather than reporting a level, when the native module is not + // in the build at all. Mapping that to a value here keeps every caller of + // this module dealing in outcomes rather than in exceptions. + let hasHardware: boolean; + let level: LocalAuthentication.SecurityLevel; + try { + hasHardware = await LocalAuthentication.hasHardwareAsync(); + level = await LocalAuthentication.getEnrolledLevelAsync(); + } catch { + return { kind: 'unavailable' }; + } if (level === LocalAuthentication.SecurityLevel.NONE) { // No biometric *and* no passcode. Distinguish "this phone could never do @@ -72,16 +122,28 @@ export async function checkUnlockAvailability(): Promise { export async function requestUnlock(): Promise { const availability = await checkUnlockAvailability(); + if (availability.kind === 'unavailable') return { status: 'unavailable' }; + if (availability.kind === 'unsupported' || availability.kind === 'none') { - return { status: 'failed', reason: availability.kind }; + return { status: 'no-device-lock' }; } - const result = await LocalAuthentication.authenticateAsync({ - promptMessage: 'Unlock your journal', - // Plain, and not a threat. Someone opening this app may be having a bad - // day already. - cancelLabel: 'Not now', - }); + let result: LocalAuthentication.LocalAuthenticationResult; + try { + promptOnScreen = true; + result = await LocalAuthentication.authenticateAsync({ + promptMessage: 'Unlock your journal', + // Plain, and not a threat. Someone opening this app may be having a bad + // day already. + cancelLabel: 'Not now', + }); + } catch { + // Same reasoning as the availability check: a throw here means we could not + // ask, which is not the same as the user failing and must not open the app. + return { status: 'unavailable' }; + } finally { + promptOnScreen = false; + } if (result.success) return { status: 'unlocked' }; @@ -92,6 +154,16 @@ export async function requestUnlock(): Promise { return { status: 'cancelled' }; case 'lockout': return { status: 'locked-out' }; + // The device lost, or never had, anything to authenticate against. The + // check above should have caught this, but it can also change underneath + // us - someone removes their passcode while the app sits in the + // background - and the OS reports it here. Sending these to 'failed' put + // the user in front of "you can try again", which is not true of any of + // them: another tap runs into the same missing lock. + case 'passcode_not_set': + case 'not_enrolled': + case 'not_available': + return { status: 'no-device-lock' }; default: return { status: 'failed', reason: result.error }; } From 8350d031cb0a2c7392be05e6e6c29895f3f9e744 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Thu, 20 Aug 2026 02:06:57 +0000 Subject: [PATCH 13/13] Make 0015 signable, and close two silent failures in the repository 0015 asked for technical-lead sign-off without giving anyone something to sign off against. It now carries a three-row threat model, and the row that matters says plainly that someone holding the unlocked phone gets the key and nothing here stops them. That is not an abstract attacker: 0004 has no caregiver account and no role distinction, and 0006 exists because the team already accepts some things are too sensitive for a device others handle. requireAuthentication would have covered part of it, and only part, since a caregiver with their own fingerprint enrolled passes that check too. A "what would change the answer" section records that availability only wins because 0001 leaves no server copy and nothing yet gives users one of their own. Once #115 puts a passphrase-protected backup in their hands, key loss stops being terminal and stronger binding becomes an ordinary trade. That makes 0015 "correct while the device holds the only copy" rather than a permanent position - much easier to sign, and it says when to revisit. The status line no longer points at a branch name that dies at merge. In the repository, RESERVED held the snake_case column names but not the camelCase aliases the selection emits, so a field named createdAt passed validation and produced two output columns of that name - the metadata silently shadowed by feature data. Confirmed against real SQLite before fixing: the true created_at is simply gone from the row. And update() filtered to declared fields, so update(id, {name, nickname}) wrote name, dropped nickname and reported success. It rejects unknown keys now, before opening the database, naming them, applying none of them. encryption-config.test.ts asserts app.json still declares useSQLCipher and still lists expo-secure-store, whose plugin carries the Android backup rules key.ts depends on. Deleting either broke encryption for everyone with a green suite. It proves the intent is declared, not that a build honoured it - that is #101, on hardware. Co-Authored-By: Claude Opus 5 (1M context) --- docs/decisions/0015-database-key-storage.md | 26 ++++- mobile/src/lib/db/encryption-config.test.ts | 54 +++++++++ mobile/src/lib/db/key.test.ts | 5 +- mobile/src/lib/db/repository-guards.test.ts | 121 ++++++++++++++++++++ mobile/src/lib/db/repository.ts | 35 +++++- 5 files changed, 235 insertions(+), 6 deletions(-) create mode 100644 mobile/src/lib/db/encryption-config.test.ts create mode 100644 mobile/src/lib/db/repository-guards.test.ts diff --git a/docs/decisions/0015-database-key-storage.md b/docs/decisions/0015-database-key-storage.md index ebe3409..5a2737d 100644 --- a/docs/decisions/0015-database-key-storage.md +++ b/docs/decisions/0015-database-key-storage.md @@ -1,6 +1,6 @@ # 0015 — Database key storage and the biometric boundary -**Status:** Proposed — implemented on `feature/encrypted-storage-unlock-foundation`, needs technical-lead sign-off +**Status:** Proposed — implemented in `mobile/src/lib/db/key.ts`, needs technical-lead sign-off **Resolves:** the key-storage half of the local-database-encryption decision that [0003](0003-device-encryption-exports-backups.md) and [0007](0007-authentication-recovery-deletion.md) both reference as living in `architecture-plan.md` ## Context @@ -25,9 +25,31 @@ Per [0001](0001-local-only-architecture.md) there is no server copy of anything Separating the two still delivers what 0007 actually asks for — a device-biometric lock with the OS passcode fallback — while leaving the key's survival independent of enrollment state. +## Threat model + +Three rows, so that signing this off means agreeing to something specific rather than endorsing a direction. + +| Who | What they get | What stops them | +|---|---|---| +| Someone who finds or steals the phone, locked | Nothing. An encrypted file they cannot open. | The device lock. `WHEN_UNLOCKED_THIS_DEVICE_ONLY` also keeps the key out of any backup they could restore elsewhere. | +| Someone reading the keystore off a rooted or jailbroken device | The key, and therefore the journal. | Nothing we control. `requireAuthentication` would have raised the cost here. | +| **Someone holding the phone while it is unlocked** | **The key, and the journal.** | **Nothing in this decision.** | + +The third row is the one that matters, and it is not an abstract attacker. [0004](0004-primary-user-caregiver-role.md) has no caregiver account and no role distinction — whoever is holding the phone sees the journal, deliberately. [0006](0006-excluded-field-safety-boundary.md) exists because the team already accepts that some things are too sensitive to sit on a device other people handle. This is that same person, and the honest statement is that the app does not defend against them: the biometric gate is a prompt at app open, not a property of the key, so once the app is past it the key is readable to anyone the phone is passed to. + +`requireAuthentication` would have covered part of that — a fresh OS check at the key itself rather than only at the door. Only part, because a caregiver whose own fingerprint is enrolled on the phone passes that check too. That is what is being given up, and it is the piece the sign-off is really about. + ## Consequences -- **The trade, stated plainly:** the key is protected by the device lock rather than bound to biometric enrollment in the secure element. Someone with an unlocked device, or with a rooted/jailbroken one and some patience, can reach it. Against that we are weighing a failure mode that is silent, permanent, and triggered by something people do routinely. For a journal whose worst case is disclosure and whose *other* worst case is total loss, this is the better side of the trade — but it is a security decision and deserves the second pair of eyes [0003](0003-device-encryption-exports-backups.md) also asks for. +- **The trade, stated plainly:** the key is protected by the device lock rather than bound to biometric enrollment in the secure element — the threat model above says who that leaves in. Against that we are weighing a failure mode that is silent, permanent, and triggered by something people do routinely. For a journal whose worst case is disclosure and whose *other* worst case is total loss, this is the better side of the trade — but it is a security decision and deserves the second pair of eyes [0003](0003-device-encryption-exports-backups.md) also asks for. - `WHEN_UNLOCKED_THIS_DEVICE_ONLY` 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 for exports; issue #115 still has to do the equivalent for the database file itself. - A unit test asserts the option stays off, so switching it on means deleting a test that says why not to. - **If this is rejected,** the alternative is `requireAuthentication` plus an onboarding line telling people that changing their biometric settings destroys the journal. That is honest, and a worse product. + +## What would change the answer + +Availability only has to win here because of something missing, not something permanent. [0001](0001-local-only-architecture.md) means there is no server copy, and nothing shipped yet gives users a copy of their own — so an unreadable key ends the journal, and no confidentiality gain is worth that. + +The encrypted device-transfer file [0003](0003-device-encryption-exports-backups.md) scopes, and export (issue #115), change that arithmetic. Once someone holds a passphrase-protected backup, losing the key costs them what they have written since they made it rather than everything, and `requireAuthentication: true` stops being a silent data-loss bomb and becomes an ordinary trade worth taking. Revisit this record when that ships rather than inheriting it. (`WHEN_PASSCODE_SET_THIS_DEVICE_ONLY` does not come back with it: it still cannot store a key at all on the devices [0018](0018-no-device-lock-behaviour.md) is about.) + +So this is correct while the device holds the only copy of the journal, and that is the basis to sign it off on — not as a permanent position on the biometric boundary. diff --git a/mobile/src/lib/db/encryption-config.test.ts b/mobile/src/lib/db/encryption-config.test.ts new file mode 100644 index 0000000..4349896 --- /dev/null +++ b/mobile/src/lib/db/encryption-config.test.ts @@ -0,0 +1,54 @@ +// Encryption is two lines of app.json, and nothing else in this suite reads +// them. +// +// Everything else about the journal database is testable here: key generation, +// the PRAGMA, the migrations. What is not is whether the binary those run +// against was built with SQLCipher at all - that comes from a config plugin, +// applied at prebuild, on a machine no test touches. Delete `useSQLCipher` and +// expo-sqlite links stock SQLite, `PRAGMA key` is silently ignored (see the +// probe in database.ts and issue #130), and every journal in the field is +// written in the clear. Nothing goes red. +// +// So this reads the config file itself. It is a weak test of a strong claim - +// it proves the intent is still declared, not that a build honoured it, which +// is issue #101 on hardware - but it is the difference between that regression +// being caught in CI and being caught by a user. + +import appJson from '../../../app.json'; + +/** Only the shape this file asserts on; app.json holds a great deal more. */ +type AppConfig = { + expo: { + plugins: (string | [string, Record?])[]; + }; +}; + +const config = appJson as unknown as AppConfig; + +function pluginEntry(name: string) { + return config.expo.plugins.find((entry) => + typeof entry === 'string' ? entry === name : entry[0] === name + ); +} + +describe('app.json', () => { + it('builds expo-sqlite with SQLCipher', () => { + // Without this the journal is a plaintext SQLite file and 0015, 0003 and + // the local-only promise in 0001 all quietly stop being true. + const entry = pluginEntry('expo-sqlite'); + + expect(Array.isArray(entry)).toBe(true); + expect((entry as [string, Record])[1]).toMatchObject({ + useSQLCipher: true, + }); + }); + + it('keeps the expo-secure-store plugin entry, which is not just registration', () => { + // key.ts protects the key with WHEN_UNLOCKED_THIS_DEVICE_ONLY, and that + // option is iOS-only. On Android the equivalent - keeping the key out of + // Auto Backup, so a restored backup on a new phone cannot decrypt a copied + // database file - comes entirely from this plugin's backup rules. It looks + // like a redundant line in a plugin list and it is load-bearing. + expect(config.expo.plugins).toContain('expo-secure-store'); + }); +}); diff --git a/mobile/src/lib/db/key.test.ts b/mobile/src/lib/db/key.test.ts index 476e5d7..9898847 100644 --- a/mobile/src/lib/db/key.test.ts +++ b/mobile/src/lib/db/key.test.ts @@ -71,7 +71,10 @@ describe('getOrCreateDatabaseKey', () => { await getOrCreateDatabaseKey(); const [, , options] = setItemAsync.mock.calls[0]; - expect(options).not.toHaveProperty('requireAuthentication', true); + // Absence, not `!== true`. Anything truthy in that slot binds the entry to + // biometric enrollment just the same, and this is the one assertion 0015 + // stakes its reputation on. + expect(options).not.toHaveProperty('requireAuthentication'); expect(options.keychainAccessible).toBe(SecureStore.WHEN_UNLOCKED_THIS_DEVICE_ONLY); }); diff --git a/mobile/src/lib/db/repository-guards.test.ts b/mobile/src/lib/db/repository-guards.test.ts new file mode 100644 index 0000000..bdfbba6 --- /dev/null +++ b/mobile/src/lib/db/repository-guards.test.ts @@ -0,0 +1,121 @@ +// The two ways this layer used to give a wrong answer without an error. +// +// Kept separate from repository.test.ts, which covers what the repository does +// when it is used correctly. Everything here is about what it does when it is +// not - and in both cases the old behaviour was to carry on and report success. + +import { createInMemoryDatabase, HAS_NODE_SQLITE } from '../../../jest/in-memory-sqlite'; +import type { InMemoryDatabase } from '../../../jest/in-memory-sqlite'; +import { createRepository, ENTRY_COLUMNS_SQL, RepositoryError } from './repository'; + +let mockUuidCounter = 0; +jest.mock('expo-crypto', () => ({ + randomUUID: () => `uuid-${String(++mockUuidCounter).padStart(3, '0')}`, + getRandomBytesAsync: async (n: number) => new Uint8Array(n), +})); + +const describeSql = HAS_NODE_SQLITE ? describe : describe.skip; + +type Contact = { name: string; relationship: string; phone: string | null }; + +describe('a field that collides with the metadata', () => { + // `selection` aliases created_at to createdAt, so a field of that name gives + // the SELECT two output columns called createdAt. SQLite returns the last + // one, which means the entry's createdAt would quietly be the feature's + // column and the real timestamp would never reach a caller. Only the + // snake_case spellings used to be reserved, so this passed configuration. + it('is rejected under the camelCase name the selection emits', () => { + expect(() => createRepository({ table: 'contacts', fields: ['createdAt'] })).toThrow( + /managed by the repository/ + ); + expect(() => createRepository({ table: 'contacts', fields: ['updatedAt'] })).toThrow( + RepositoryError + ); + }); + + it('is rejected whatever the case, because SQLite ignores case in a column name', () => { + expect(() => createRepository({ table: 'contacts', fields: ['ID'] })).toThrow(RepositoryError); + expect(() => createRepository({ table: 'contacts', fields: ['Created_At'] })).toThrow( + RepositoryError + ); + expect(() => createRepository({ table: 'contacts', fields: ['CreatedAt'] })).toThrow( + RepositoryError + ); + }); + + it('still allows a name that merely starts the same way', () => { + expect(() => + createRepository({ table: 'contacts', fields: ['created_at_source', 'identifier'] }) + ).not.toThrow(); + }); +}); + +describe('update with a key that is not a column', () => { + it('refuses before it even opens the database', async () => { + // The old path filtered the key out, found nothing left to write, read the + // row back and returned it - a successful-looking edit that saved nothing. + const contacts = createRepository( + { table: 'contacts', fields: ['name', 'relationship', 'phone'] }, + async () => { + throw new Error('the database should not have been opened'); + } + ); + + await expect(contacts.update('uuid-001', { nickname: 'Sammy' } as never)).rejects.toThrow( + RepositoryError + ); + }); +}); + +describeSql('update with a key that is not a column, against real SQLite', () => { + let db: InMemoryDatabase; + let contacts: ReturnType; + + function makeRepository() { + return createRepository( + { table: 'contacts', fields: ['name', 'relationship', 'phone'] }, + async () => db + ); + } + + beforeEach(async () => { + mockUuidCounter = 0; + db = createInMemoryDatabase(); + await db.execAsync( + `CREATE TABLE contacts (${ENTRY_COLUMNS_SQL}, name TEXT NOT NULL, relationship TEXT, phone TEXT)` + ); + contacts = makeRepository(); + }); + + afterEach(async () => { + await db.closeAsync(); + }); + + it('names the key it did not recognise', async () => { + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + + await expect(contacts.update(created.id, { nickname: 'Al' } as never)).rejects.toThrow( + /Unknown column on "contacts": "nickname"/ + ); + }); + + it('applies none of the call, not just the part it understood', async () => { + // A typo alongside a real field used to write the real one and drop the + // typo, leaving a row that is half of what the caller asked for. + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + + await expect( + contacts.update(created.id, { name: 'Alexis', nickname: 'Al' } as never) + ).rejects.toThrow(RepositoryError); + + await expect(contacts.find(created.id)).resolves.toMatchObject({ name: 'Alex' }); + }); + + it('still treats an empty change set as a legitimate no-op', async () => { + // The distinction that matters: asking for nothing is fine, asking for + // something that does not exist is not. + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + + await expect(contacts.update(created.id, {})).resolves.toEqual(created); + }); +}); diff --git a/mobile/src/lib/db/repository.ts b/mobile/src/lib/db/repository.ts index c0b5328..307d145 100644 --- a/mobile/src/lib/db/repository.ts +++ b/mobile/src/lib/db/repository.ts @@ -67,8 +67,21 @@ function checkIdentifier(kind: string, name: string): string { return name; } -/** Column names that this module owns; a feature cannot redeclare them. */ -const RESERVED = new Set(['id', 'created_at', 'updated_at']); +/** + * Column names that this module owns; a feature cannot redeclare them. + * + * Both spellings of each, because both reach a result row: the stored + * `created_at` and the `createdAt` that `selection` aliases it to. Without the + * camelCase half, a feature could declare a field called `createdAt`, and the + * SELECT would then ask for `created_at AS createdAt, ..., createdAt` - two + * output columns with one name. SQLite answers with the last, so the entry's + * `createdAt` would silently be the feature's column and the real timestamp + * would be gone, with no error anywhere. + * + * Compared lowercased, because SQLite's column names are case-insensitive and + * `ID` would collide with the managed `id` in the CREATE TABLE regardless. + */ +const RESERVED = new Set(['id', 'created_at', 'updated_at', 'createdat', 'updatedat']); /** * Builds a repository for one repeatable-entry table. @@ -88,7 +101,7 @@ export function createRepository( const fields = config.fields.map((field) => { checkIdentifier('column', field); - if (RESERVED.has(field)) { + if (RESERVED.has(field.toLowerCase())) { throw new RepositoryError( `Column "${field}" on "${table}" is managed by the repository and cannot be declared as a field.` ); @@ -96,6 +109,8 @@ export function createRepository( return field; }); + const declared = new Set(fields); + const selection = ['id', 'created_at AS createdAt', 'updated_at AS updatedAt', ...fields].join( ', ' ); @@ -159,6 +174,20 @@ export function createRepository( }, async update(id, changes) { + // A key that is not a column is a typo, not an instruction. Dropping it + // quietly lands on the no-op path below, which reads the row back and + // returns it - so the caller is told the edit succeeded and the value it + // asked to save is nowhere. Refuse the whole call instead: partially + // applying an update whose intent is already in doubt is worse. + const unknown = Object.keys(changes).filter((key) => !declared.has(key)); + if (unknown.length > 0) { + throw new RepositoryError( + `Unknown column${unknown.length === 1 ? '' : 's'} on "${table}": ${unknown + .map((key) => JSON.stringify(key)) + .join(', ')}.` + ); + } + const db = await getDatabase(); const changed = fields.filter((field) => field in changes);