diff --git a/lib/mail-index/__tests__/reindex.test.ts b/lib/mail-index/__tests__/reindex.test.ts new file mode 100644 index 00000000..86aa94d7 --- /dev/null +++ b/lib/mail-index/__tests__/reindex.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; +import { strayIdsAfterCatchUp } from '../reindex'; + +describe('strayIdsAfterCatchUp', () => { + it('removes locally-indexed ids absent from a complete (uncapped) fetch', () => { + // Contacts/files have no date filter, so a catch-up query that comes back + // under the cap IS the whole account - anything indexed but missing from + // it was deleted. This is the only place a JMAP `destroyed` ever reaches + // these two content types, since nothing in the renderer populates + // reindex's `removed` field. + const existing = new Set(['a', 'b', 'c']); + expect(strayIdsAfterCatchUp(existing, ['a', 'c'], 2, 2_000)).toEqual(['b']); + }); + + it('does nothing when the fetch is empty but so is the local index', () => { + expect(strayIdsAfterCatchUp(new Set(), [], 0, 2_000)).toEqual([]); + }); + + it('never removes anything when the query hit its cap - a truncated page is not the whole world', () => { + // Exactly the case that would otherwise delete objects that are still + // live: an account with >= cap contacts/files, where "missing from this + // page" only means "not on this page", not "gone". + const existing = new Set(['a', 'b', 'c']); + expect(strayIdsAfterCatchUp(existing, ['a'], 2_000, 2_000)).toEqual([]); + }); + + it('is a no-op when nothing is stray', () => { + const existing = new Set(['a', 'b']); + expect(strayIdsAfterCatchUp(existing, ['a', 'b', 'c'], 3, 2_000)).toEqual([]); + }); +}); diff --git a/lib/mail-index/__tests__/store.test.ts b/lib/mail-index/__tests__/store.test.ts index 610e03b6..3c935c32 100644 --- a/lib/mail-index/__tests__/store.test.ts +++ b/lib/mail-index/__tests__/store.test.ts @@ -4,8 +4,9 @@ import path from 'node:path'; import { randomBytes } from 'node:crypto'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { isSqlcipherAvailable } from '../binding'; +import type { SqlcipherConstructor, SqlcipherDatabase, SqlcipherStatement } from '../binding'; import { accountFileToken, getStoreDir, indexDbPath, STORE_DIR_ENV } from '../paths'; -import { MailIndex, toFtsMatchQuery, type IndexDoc } from '../store'; +import { MailIndex, openKeyed, toFtsMatchQuery, type IndexDoc } from '../store'; describe('toFtsMatchQuery', () => { it('quotes every token so FTS5 operators in user input cannot break the query', () => { @@ -78,6 +79,51 @@ describe('paths', () => { }); }); +/** A fake `SqlcipherDatabase` whose `pragma()` is driven by the given handler. */ +function fakeSqlcipherCtor( + pragmaHandler: (source: string) => unknown, +): { ctor: SqlcipherConstructor; instances: Array<{ closeCalls: number }> } { + const instances: Array<{ closeCalls: number }> = []; + function FakeDatabase(this: unknown, _path?: string): SqlcipherDatabase { + const state = { closeCalls: 0 }; + instances.push(state); + const stmt: SqlcipherStatement = { run: () => ({ changes: 0, lastInsertRowid: 0 }), get: () => undefined, all: () => [] }; + const db: SqlcipherDatabase = { + exec: () => {}, + prepare: () => stmt, + pragma: pragmaHandler, + close: () => { state.closeCalls += 1; }, + }; + return db; + } + return { ctor: FakeDatabase as unknown as SqlcipherConstructor, instances }; +} + +describe('openKeyed', () => { + it('closes the connection before rethrowing when a pragma AFTER assertEncrypted fails', () => { + // The bug this guards: `journal_mode = WAL` (or any pragma after the key + // check) throwing must not leak the native handle - a `try` that only + // wrapped SOME of these calls previously let exactly this escape. + const { ctor, instances } = fakeSqlcipherCtor((source) => { + if (source === 'cipher_version') return [{ cipher_version: 'fake-4.5.0' }]; + if (source === 'journal_mode = WAL') throw new Error('simulated pragma failure'); + return undefined; + }); + expect(() => openKeyed(ctor, '/fake/path.db', randomBytes(32))).toThrow(/simulated pragma failure/); + expect(instances).toHaveLength(1); + expect(instances[0].closeCalls).toBe(1); + }); + + it('tolerates the extra close when assertEncrypted itself is the failure (its own close is idempotent)', () => { + // assertEncrypted closes on its own failure before throwing; openKeyed's + // catch then calls close again. That second call must be harmless, not a + // new crash - hence >= 1 rather than a fixed count. + const { ctor, instances } = fakeSqlcipherCtor((source) => (source === 'cipher_version' ? [] : undefined)); + expect(() => openKeyed(ctor, '/fake/path.db', randomBytes(32))).toThrow(/no SQLCipher support/); + expect(instances[0].closeCalls).toBeGreaterThanOrEqual(1); + }); +}); + function doc(overrides: Partial = {}): IndexDoc { return { jmapAccountId: 'acc1', diff --git a/lib/mail-index/reindex.ts b/lib/mail-index/reindex.ts index f5a5c4b4..ac30ab4c 100644 --- a/lib/mail-index/reindex.ts +++ b/lib/mail-index/reindex.ts @@ -106,6 +106,26 @@ function isoDaysFromNow(days: number): string { return new Date(Date.now() + days * 24 * 60 * 60 * 1000).toISOString(); } +/** + * Which locally-indexed ids are stray after an uncapped (contact/file) + * catch-up fetch, and therefore safe to remove as deleted. + * + * Only safe when `queriedCount < cap`: a query that hit the cap was + * truncated - "the rest weren't asked for", not "the rest are gone" - and + * treating a truncated page as the whole world would delete objects that are + * still live. Exported for unit testing; the database-touching caller is not. + */ +export function strayIdsAfterCatchUp( + existingIds: ReadonlySet, + fetchedIds: readonly string[], + queriedCount: number, + cap: number, +): string[] { + if (queriedCount >= cap) return []; + const fetched = new Set(fetchedIds); + return [...existingIds].filter((id) => !fetched.has(id)); +} + /** * Which types this session can actually index. Calendar/contacts are session * capabilities; files is a PER-ACCOUNT capability (a server can advertise @@ -151,8 +171,19 @@ interface FetchArgs { ids: readonly string[] | null; } +interface FetchResult { + docs: IndexDoc[]; + /** + * How many ids the type's OWN query returned, before any `Foo/get` + * chunking. Only set when `ids === null` (a catch-up fetch); used to tell a + * complete uncapped fetch apart from one truncated at its cap - see + * `strayIdsAfterCatchUp`, the only consumer. + */ + queriedCount?: number; +} + /** Fetches and flattens one content type. `ids === null` means "the recent window". */ -async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise { +async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise { const { session, authHeader, jmapAccountId, ids } = args; switch (contentType) { @@ -170,7 +201,7 @@ async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise extractFile(jmapAccountId, node, { path: paths.get(node.id) })); + return { docs, queriedCount: ids ? undefined : targetIds.length }; } } } @@ -292,7 +324,7 @@ export async function runIndex( ? requestedIds.slice(0, MAX_IDS_PER_CALL) : null; - const docs = await fetchDocs(contentType, { + const { docs, queriedCount } = await fetchDocs(contentType, { session, authHeader: indexSession.authHeader, jmapAccountId, ids, }); written[contentType] = index.upsert(docs); @@ -302,6 +334,27 @@ export async function runIndex( // contacts have no date, and file rows are metadata-sized. index.pruneOlderThan(jmapAccountId, 'mail', isoDaysFromNow(-INDEX_WINDOW_DAYS)); } + + // Contact/file DELETES: a JMAP `destroyed` only ever reaches this + // route via `req.removed`, which nothing in the renderer populates + // today - so without this, a deleted contact or file stays + // searchable (and retrievable by the AI feature) forever. Mail and + // calendar can't use the same trick: their queries are windowed by + // date, so an id missing from one fetch may simply be outside the + // window, not gone. Contacts/files have no date filter at all - the + // query is "the first N, capped" - so when a catch-up fetch (ids + // === null) comes back under the cap, it IS the complete set, and + // anything indexed but absent from it is safely known to be deleted. + if (queriedCount !== undefined && (contentType === 'contact' || contentType === 'file')) { + const cap = contentType === 'contact' ? CONTACTS_MAX : FILES_MAX; + const stale = strayIdsAfterCatchUp( + index.existingIds(jmapAccountId, contentType), + docs.map((d) => d.id), + queriedCount, + cap, + ); + if (stale.length > 0) index.remove(jmapAccountId, contentType, stale); + } } catch (error) { // One unsupported or misbehaving type must not fail the others. const message = error instanceof Error ? error.message : String(error); diff --git a/lib/mail-index/store.ts b/lib/mail-index/store.ts index cae2a6f8..ddcdd0da 100644 --- a/lib/mail-index/store.ts +++ b/lib/mail-index/store.ts @@ -15,7 +15,7 @@ import fs from 'node:fs'; import path from 'node:path'; -import { loadSqlcipher, type SqlcipherDatabase } from './binding'; +import { loadSqlcipher, type SqlcipherConstructor, type SqlcipherDatabase } from './binding'; import { dbSiblings, indexDbPath } from './paths'; export const SCHEMA_VERSION = 1; @@ -137,6 +137,51 @@ export interface OpenOptions { key: Buffer; } +/** + * Opens the connection, sets the SQLCipher key, verifies real encryption, and + * applies the fixed pragmas - the sequence both the first attempt and the + * wrong-key retry in `open()` need identically. + * + * On ANY failure the just-opened connection is closed before the error + * propagates. This matters beyond `assertEncrypted`'s own failure (which + * already closes): a bare `db.pragma(...)` throwing - SQLITE_BUSY, a full disk + * on the first WAL write, anything - must not leak the native handle either, + * which a `try` wrapped around only some of these calls previously missed. + * + * Exported so the cleanup guarantee can be unit-tested against a fake + * `SqlcipherDatabase` - a real double failure (wrong key, THEN a pragma + * failure on the freshly rebuilt file) is not practically reproducible + * against the real binding. + */ +export function openKeyed( + Database: SqlcipherConstructor, + dbPath: string, + key: Buffer, +): { db: SqlcipherDatabase; version: number | null } { + const db = new Database(dbPath); + try { + // The key pragma must be the FIRST statement on the connection. Hex form + // means SQLCipher uses these 32 bytes as the raw key with no KDF, which is + // right for a random key (a passphrase would want the KDF). + db.pragma(`key = "x'${key.toString('hex')}'"`); + assertEncrypted(db, dbPath); + db.pragma('journal_mode = WAL'); + db.pragma('synchronous = NORMAL'); + // The offline replica (lib/offline-replica/**) is a SECOND connection to + // this same file, writing disjoint tables. WAL lets a writer and readers + // coexist, but two WRITERS get SQLITE_BUSY immediately without this - and + // both subsystems are driven by the same renderer push handler, so they + // genuinely do overlap. + db.pragma('busy_timeout = 8000'); + return { db, version: readSchemaVersion(db) }; + } catch (error) { + // Idempotent: assertEncrypted already closed on its own failure, so this + // is a harmless no-op in that case. + try { db.close(); } catch { /* already closed */ } + throw error; + } +} + export class MailIndex { private constructor( private readonly db: SqlcipherDatabase, @@ -163,46 +208,24 @@ export class MailIndex { const dbPath = indexDbPath(storeDir, accountId); fs.mkdirSync(path.dirname(dbPath), { recursive: true, mode: 0o700 }); - let db = new Database(dbPath); - // The key pragma must be the FIRST statement on the connection. Hex form - // means SQLCipher uses these 32 bytes as the raw key with no KDF, which is - // right for a random key (a passphrase would want the KDF). - db.pragma(`key = "x'${key.toString('hex')}'"`); - assertEncrypted(db, dbPath); - // A wrong key surfaces here rather than at open: SQLCipher only reads the // header lazily. Treat it as "unreadable" and rebuild from scratch - the // index is derived data, so there is nothing to recover and never anything // to prompt the user for (the key was never a user secret). - let version: number | null; + let opened: { db: SqlcipherDatabase; version: number | null }; try { - db.pragma('journal_mode = WAL'); - db.pragma('synchronous = NORMAL'); - // The offline replica (lib/offline-replica/**) is a SECOND connection to - // this same file, writing disjoint tables. WAL lets a writer and readers - // coexist, but two WRITERS get SQLITE_BUSY immediately without this - and - // both subsystems are driven by the same renderer push handler, so they - // genuinely do overlap. - db.pragma('busy_timeout = 8000'); - version = readSchemaVersion(db); + opened = openKeyed(Database, dbPath, key); } catch { - db.close(); + // `openKeyed` guarantees the failed connection above is already closed, + // so there is nothing to clean up here before retrying on a fresh file. for (const f of dbSiblings(dbPath)) { try { fs.rmSync(f, { force: true }); } catch { /* best effort */ } } - db = new Database(dbPath); - db.pragma(`key = "x'${key.toString('hex')}'"`); - assertEncrypted(db, dbPath); - db.pragma('journal_mode = WAL'); - db.pragma('synchronous = NORMAL'); - // The offline replica (lib/offline-replica/**) is a SECOND connection to - // this same file, writing disjoint tables. WAL lets a writer and readers - // coexist, but two WRITERS get SQLITE_BUSY immediately without this - and - // both subsystems are driven by the same renderer push handler, so they - // genuinely do overlap. - db.pragma('busy_timeout = 8000'); - version = null; + opened = openKeyed(Database, dbPath, key); + opened.version = null; // fresh file - nothing to read } + const db = opened.db; + let version = opened.version; if (version !== null && version !== SCHEMA_VERSION) { // Rebuildable derived data: drop, don't migrate.