diff --git a/docs/ELECTRON-OFFLINE-ENGINE-REVIEW.md b/docs/ELECTRON-OFFLINE-ENGINE-REVIEW.md new file mode 100644 index 00000000..4f5bf7a6 --- /dev/null +++ b/docs/ELECTRON-OFFLINE-ENGINE-REVIEW.md @@ -0,0 +1,232 @@ +# Adversarial review: `docs/ELECTRON-OFFLINE-ENGINE-DESIGN.md` + +Reviewer: independent agent, fresh context, no relation to the design's author. 2026-08-04. + +## Verdict + +**Needs substantial rework before implementation — but narrowly scoped rework.** + +The delta-sync core (everything tagged `[reused]` from M, the mobile design) is genuinely sound +and transfers; the reviewer attacked it directly and could not break it. The problem is that **all +four genuinely-new sections have an unclosed load-bearing mechanism**, and one of them breaks a +build that ships today: + +- §3 (binding choice) contains a packaging decision that breaks the hosted Docker image and the + integration fixture. +- §2 (process choice) rests on a credential claim that is only true inside an HTTP request. +- §6 (key handoff) is under-specified in a way that doesn't work as sequenced, and its "unresolved + implementation choice" is not security-neutral. +- §5.3/§8.3 (multi-account) breaks the specific premise M's D6 fix relies on. + +Nothing here requires re-architecting the sync engine. Stages B-G can proceed against M as +written. Stage A as currently specified would not surface most of this. + +All file:line citations in the design doc that were checked resolve correctly (one trivial +miscount, noted at the end) — citation quality is high; the problems are in the reasoning built +on top. + +--- + +## CRITICAL + +### C1 — Adding `@signalapp/sqlcipher` to `dependencies` breaks the hosted Docker build *and* the integration fixture + +**Where:** §3.3.1 ("entering `dependencies`"), §3.3.5, §2.4, §10.5, E2, §13 item 6. + +`Dockerfile:1-4` — `FROM node:24-alpine`, `RUN npm ci`. `integration/webmail.Dockerfile:12-15` — +same, `FROM node:24-alpine` + `npm ci`. + +Verified from the published tarball that `@signalapp/sqlcipher@4.0.3`: +- ships **6** prebuilds — `darwin-{arm64,x64}`, `linux-{arm64,x64}`, `win32-{arm64,x64}`. No + `linuxmusl-*`. (The design doc's list is exactly right.) +- ships **no build sources at all** — published `files` is `dist/*`, `prebuilds`, `README.md`. No + `binding.gyp`, no `src/`, no `deps/`. +- has `install: node-gyp-build`. `node-gyp-build`'s `bin.js` runs `node-gyp-build-test`; on + failure it calls `build()` → spawns `node-gyp rebuild` → `process.exit(code)`. + +No prebuild + no `binding.gyp` ⇒ `node-gyp rebuild` fails ⇒ **`npm ci` exits nonzero**. The Linux +prebuild also has a glibc ≥ 2.34 floor, so it could not load on musl even if copied. + +**Concrete failure:** the next `docker build` of the production image fails at line 4. +`npm run test:integration` fails to build the webmail container. Neither is gated by +`VNCMAIL_DESKTOP_STORE_DIR` — that env var only governs *activation*, not *installation*. + +§3.3.5 dismisses musl as "relevant if an Alpine-based container ever wants the engine — which, +per §2.4, it must not" — that reasoning is inverted: the Alpine container doesn't want the engine, +it just needs `npm install` to succeed regardless. + +**Fix direction:** `optionalDependencies` + a guarded runtime `require` (which also delivers E2's +graceful-load-failure behavior for free), or a separate optional package, or `--omit=optional` in +both Dockerfiles. Pick one and say so explicitly; add "`docker build` of both Dockerfiles still +succeeds" to the Stage A verification list. + +### C2 — Option A's central justification is only true inside an HTTP request; the Worker credential path does not exist + +**Where:** §2.1-for-1, §1.2, §2.4 (Worker), §5.3, §8.1 triggers T1/T4/T5/T11, §13 item 1. + +Verified in `app/api/auth/session/route.ts` and `app/api/auth/token/route.ts`: every credential +read goes through `cookies()` from `next/headers` — request-scoped. `lib/oauth/cookie-config.ts:11` +sets `httpOnly: true`. The cookies live in the renderer's cookie jar, not in the server. The +standalone server holds no session state whatsoever; it decrypts a cookie per request and +discards it. + +So "the credentials are already there... No new credential path, no IPC carrying secrets, no +second copy" (§2.1-for-1) is materially overstated. What is actually there is *the ability to +decrypt a credential presented on an inbound request* — not a resident credential. + +Consequences the design never addresses: + +1. **T1 ("server process ready + an account's credentials resolvable") cannot fire.** At + server-ready there are no cookies anywhere. Nor can T4 (network regained), T5 (`StateChange` + on the engine's own socket), or T11 (resume from sleep) — none is an inbound renderer request. +2. **A `worker_threads` Worker is a separate execution context with no cookie access at all.** + §2.4 mandates the Worker and routes talk to it via `postMessage`, but there's no specified + point at which the Worker actually receives credentials. +3. The only workable shape is: on the first renderer request, decrypt and hand the **plaintext** + credentials to the Worker, which retains them for the process lifetime. That is a new + long-lived plaintext secret in a new location — the exact thing §2.1-for-1 claims doesn't + happen, and the same category of thing `client.ts:6055-6059` already declined once (a + resident credential copy in a process that didn't previously hold one). It also creates an + invalidation problem never addressed: password change, logout elsewhere, or a cleared cookie + leaves the Worker retrying stale credentials indefinitely (since `AuthenticationError` is + correctly never treated as a purge signal) — against a server with failed-auth lockout, this + locks the user's account. + +This doesn't kill Option A, but it kills the argument that Option A is free of new secret +handling — which was the design's #1 stated reason for choosing it over the alternative. That +comparison needs to be redone with the resident-copy cost included, not dropped. + +### C3 — The proposed OAuth-refresh mitigation (E11) is not just unimplementable; it *is* the bug it's meant to prevent + +**Where:** E11 (failure-mode table), §1.2's note about `app/api/auth/token/route.ts:104-106`. + +E11's rule: *"the engine never refreshes independently. It obtains tokens only through the +existing `PUT /api/auth/token` route, in-process, so there is exactly one refresher and one +rotation writer — the route."* + +But that route: reads the refresh token from the **request's** cookie; writes the rotated token +as a `Set-Cookie` on the **response**; and on a 400/401/403 from the identity provider, **deletes** +the refresh-token cookie and returns 401. + +An in-process server-side call to that route has no cookie to send (401s immediately), and even +if the engine forged one from a resident copy, the rotated token would land in a response the +engine discards. Net effect: engine refreshes → identity provider rotates the token → the new +token lands in a discarded response → the browser still holds the now-superseded token → the +next real refresh from the browser gets rejected → the route deletes the cookie → **the user is +silently logged out of that account, and the offline store's credentials are dead.** + +The per-slot lock the design suggests as a fallback does not help — the problem is that cookie +state lives in the browser, not that the writes race each other. + +Separately, `PUT /api/auth/session` requires three `sec-fetch-*` headers with a comment claiming +"non-browser clients cannot forge these" — a Node-side `fetch` call *can* set all three, silently +turning a security control into decoration if any engine path goes through this route. Not +discussed in the design at all. + +### C4 — The shared registry file breaks the exact premise the multi-account safety fix relies on + +**Where:** §5.1 (`registry.json`, epoch ownership), §2.4 ("one worker per account"), §4.3 (mutex +described as "belt-and-braces"), §7.1 (`completePendingPurges()`), §8.3 cross-account, §5.5. + +The mobile design's cross-account safety guarantee depends explicitly on there being **exactly +one writer process-wide** — its own JMAP client is a renderer singleton, so multi-account +simultaneous sync was out of scope for it, and its own adversarial review never examined +concurrent multi-account execution. + +This design introduces multi-account-simultaneous as "a genuine capability gain" and disposes of +the concurrency consequences with a one-line "the jitter matters more here" — but the epoch +value (the fencing token the whole safety guarantee rests on) lives in `registry.json`, a single +JSON file shared across every account. No SQLite transaction covers a plain JSON file. The +argument that a real database transaction demotes the old per-account mutex to +"belt-and-braces" is correct for state stored *inside* the SQLite file, and does not apply to +`registry.json` at all — which names no owner thread, no lock, and no atomic-write discipline. + +Two concrete failures: +1. **Lost epoch bump.** Worker A read-modify-writes the registry to bump account A's epoch + (purge, clear, logout). Worker B, holding a stale parse, writes its own update and clobbers + A's bump. A's in-flight cycle's next commit now passes the epoch check and lands on top of a + wipe — an empty record store with a live, advanced cursor and `resyncRequired: false`, exactly + the unreachable-by-design state the mobile design's whole S1 fix exists to prevent. +2. **Torn read on a shared file.** Worker B is mid-write; the server's launch-time + `completePendingPurges()` reads and the parse throws or yields a partial object. The + documented rule ("unreadable → treated as a purge") means a transient concurrency artifact + triggers a full purge-and-rebootstrap for accounts that were perfectly fine — and because the + file is shared, one torn read can hit every account at once, not just one. + +### Other critical-adjacent findings, condensed + +- **H1** — the "sync enabled" toggle lives in the renderer's local storage; the server-side engine + (and its background triggers) has no way to read it, so it will materialize an encrypted store + and a keychain entry for accounts that never opted in — precisely the failure the design's own + lazy-materialization rule was meant to prevent. +- **H2** — the key-handoff sequencing assumes accounts exist at server-spawn time; they don't + (accounts are added later, by logging in). The two proposed handoff mechanisms are not + equivalent: one of them passes a nonce via the spawned process's environment variables, which + are readable by any other process running as the same OS user — defeating the entire point of + using the OS keychain in the first place. Needs re-sequencing plus picking the other mechanism + on security grounds, not "whichever is cleaner to implement." +- **H3** — the "no optimistic-mutation layer exists, so nothing to keep coherent" claim is false; + the webmail already does local-delta arithmetic on mailbox unread counts and totals for + mark-read/move/delete actions, with a comment referencing a prior production bug from getting + this exact kind of cutoff wrong. A read-only offline cache sitting underneath that arithmetic + needs an explicit coherence story, which the design currently declares unnecessary. +- **H4** — no cap specified on how many accounts sync simultaneously; since this is the same + process serving the live webmail UI, an unbounded background sync could contend for the same + rate-limited server connection as the user's foreground activity, throttling their visible mail + during their own multi-account first sync. +- Several medium/low findings: one proposed API call is Linux-only and would crash the app on + macOS/Windows if implemented as literally described; the Linux keychain fallback behavior is + described slightly wrong (Electron already fails safely there; the real hazard is a *different* + API a future maintainer might reach for); the claim that two SQLite bindings are "the same code + either way" doesn't hold — verified real API differences exist between them; the "single-user" + safety check for the hosted-deployment gate doesn't actually verify what it claims to. + +--- + +## What the reviewer independently re-verified (not just re-read) + +Re-ran two of the design's three "verified by execution" claims independently, in Electron 43.2.0 +itself under the same execution mode the standalone server actually uses: + +1. **`@signalapp/sqlcipher@4.0.3` in Electron 43 — fully re-confirmed by actual re-execution.** + Loads with no rebuild, real SQLCipher encryption confirmed (encrypted header, no plaintext + canary recoverable from raw bytes, wrong key correctly rejected). The strongest part of the + original design. +2. **`node:sqlite`'s `PRAGMA key` silent no-op — fully re-confirmed by actual re-execution.** No + throrw, mailbox left in cleartext, canary recoverable from raw bytes. The design is right to + call this the sharpest landmine found and to mandate a positive verification check after every + store open (though the exact check needs a small correction — checking for a non-empty + *string* rather than a non-empty *result set*, since the no-cipher case returns zero rows, not + an empty string, and a naive string comparison would pass vacuously). +3. **The Linux keychain-fallback claim — not independently confirmed, and partially contradicted** + by reading Electron's own source and current documentation (no Linux desktop was available to + actually execute this one). The decision made (refuse outright rather than risk a false sense + of security) stays correct regardless and costs nothing, but the specific mechanism described + needs correcting. + +## Recommended gate + +Do not start implementation as currently written. Resolve in this order: +1. **C1** — decide the dependency-installation shape so the existing Docker builds keep working; + add a Docker-build check to the first implementation step's own verification list. +2. **C2 + C3** — specify the credential lifecycle end to end: how a background worker actually + gets credentials, where they live, how long, how invalidation reaches them, and how token + refresh can work given rotation needs to land in the browser's cookie jar, not a discarded + response. This may change the process-architecture verdict; re-run that comparison honestly + rather than inheriting the original conclusion. +3. **C4** — name a single owner (or a real lock plus atomic write) for the shared registry file, + and re-derive the multi-account safety guarantee under concurrent writers rather than citing + the mobile design's single-writer proof as if it still applied. +4. **H1** — decide where the "sync enabled" setting needs to live (or how the engine learns it) + so lazy materialization is actually enforceable from where the engine's triggers fire. +5. **H2** — pick the handoff mechanism that doesn't leak via process environment variables, and + re-sequence it for accounts that don't exist yet at process-spawn time. +6. **H3** — add real coherence rules for the counters/totals the webmail already computes locally, + or narrow the offline read path to skip anything those computations touch. +7. **H4** — state a concurrency bound and a rule that foreground user activity isn't starved by + background multi-account sync. +8. The smaller medium/low findings should land in the same pass since they're cheap to fix once + noticed. + +Everything reused from the mobile design's core sync-engine logic is safe to build against as +written — the problems are entirely in the four sections that are genuinely new to this platform.