docs: adversarial review of the Electron offline engine design (4 critical, 4 high)
This commit is contained in:
Bernd Rodler
@@ -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.
|
||||
Reference in New Issue
Block a user