Lantern

Appearance

Branch review remediation plan ​

Branch: claude/session-2026-06-14 (vs dev) Date: 2026-06-16 Source: multi-agent /code-review ultra (14 finder angles, adversarial verify, gap sweep). 56 findings survived verification; consolidated and re-grounded against the live code here. A few reviewer claims were corrected on close reading (see "Corrections to the review").

How to use this doc ​

Work items are ordered by priority (P0 first) and grouped into cohesive PRs. Each item is independently shippable. Check the box when landed on the branch; fold the matching SECURITY_REMEDIATION.md tracker edit into the SAME PR (per repo rule). Open PRs as drafts to dev. A handful of items carry a DECISION marker: pick the recommended option or override it before implementing.

Severity legend: P0 = security correctness, ship first. P1 = wrong behavior on a common path. P2 = hardening + cleanup.

Decisions taken (2026-06-16) ​

  • PR structure: everything lands on the current branch claude/session-2026-06-14 as ONE PR (operator preference), not the 4 PRs the first draft suggested. The "PR A/B/C/D" labels below are now just batch ordering.
  • WI-3 (block behavior): blocking CLOSES the connection. The send path should refuse to encrypt to a blocked peer; no re-blacklist-on-send needed.
  • WI-5 (taxonomy): chose option (b). The client still emits lantern_scheduled on form submit, so reusing that name server-side would double-count. Registered the two new server names the routes already use instead.

Progress (2026-06-16) ​

Landed on-branch and test-green (not yet committed):

  • [x] WI-4 wave_sent entityType lantern_pin to wave (lanterns.js:128).
  • [x] WI-5 registered lantern_schedule_created + lantern_schedule_cancelled in the taxonomy (name list + EVENT_REGISTRY), option (b).
  • [x] WI-6 devEchoEnabled now reads GOOGLE_CLOUD_PROJECT / GCLOUD_PROJECT / GCP_PROJECT; trackEvent.test.js updated to mirror the real deploy var + parity and default-env cases. forge.getEnvironment broad fix left as a flagged follow-up.
  • [x] WI-6b new analyticsContracts.test.js asserts every lanterns-api server event passes forge validation (prevents the whole silent-drop class).
  • [x] WI-1 device-trust now fingerprints on the per-device ed25519 (baseline + verified records are device->key maps, v3/v2). A same-id key swap, ghost device, or removal flips trust to 'changed'; getConnectionSafetyInfo also cross-checks the live vs stored safety number. Files: deviceTrust.js, firestoreTransport.js, roomCrypto.js, Chat.jsx.
  • [x] WI-1b rules comment corrected to admit the delete+recreate gap; added a rules test documenting it is allowed by rules and caught client-side.
  • [x] WI-2 /to-device now returns { failures, retryable } (via pure planToDeviceWrites); the client throws on a retryable drop so the OlmMachine re-shares (fail-closed) instead of acking a lost room key.
  • [x] WI-3 sendMessage refuses to encrypt/send to a blocked peer (blocking closes the connection), so a blocked peer's devices never get a fresh room key.
  • [x] WI-7 plaintext-downgrade closed at the root: the userKeys PARENT doc is now server-only to delete (firestore.rules), so a participant can't delete their directory to flip connectionIsE2eeCapable and re-permit plaintext. Chose the delete-lockdown over a connection latch (simpler, monotonic, no cooperative write). Added a deny test; comment corrected.
  • [x] WI-8 venue refresh lock is token-fenced: acquireRefreshLock mints a lockToken; releaseRefreshLock + the primary-cell branch of updateRefreshMetadata compare-and-clear inside a transaction and only clear inProgress when the token is still ours, so a stolen-and-overran holder can't clobber the new one. Token threaded through import.js; catch-path lat/lng re-check simplified (review #44). Lock tests rewritten.
  • [x] WI-9 durable, cross-instance rate limiter: new checkKeyDurable (Firestore sliding window, one transaction per call) backs the M-E2EE claim-otk + to-device caps; IP/user soft limits stay in-memory. Needs a Firestore TTL policy on _rateLimits.expireAt (IaC follow-up) for storage hygiene.
  • [x] WI-10 (part 1) per-device flood accounting: the durable limiter takes a cost; to-device charges one slot per target device, so a 20-device payload spends 20 of the per-recipient budget (doc-granular bound, not request-granular).
  • [x] WI-11 removed the unreachable trackEvent ':dropped' branch (forge.track resolves tracked:true or throws); parallelized the to-device mailbox writes; fixed the deploy-dev scheduler to pass ?limit=100 (matches its "100/run" comment; route caps at 100); dropped the unused getPeerTrustState barrel export.
  • Tests green per workspace (full suites): apps/web 1137, packages/shared 156, lanterns 69, auth 83, venues 88; firestore.rules 118.

DEFERRED (documented, not silently dropped):

  • WI-10 (part 2) mailbox relationship check: ✅ DONE (2026-07-23), see SECURITY_REMEDIATION.md M-E2EE-6. The feared wave-accept timing edge does not exist: acceptWave only writes the connection doc (no encryption at accept), and the sole key-share origin is message-send, which requires a settled connectionId. Gate is status-blind doc existence, so archived-chat re-keys still deliver.
  • WI-11 low-value items left for later: claim-otk / fulfillKeysQuery sequential-IO parallelization (touches crypto paths, low payoff), areaPrefix constant dedup into @lantern/shared/venues, setup-analytics-alerts.sh shared helper, and the grandfathered em-dash lines (the write-time hook blocks new ones; the ratchet burns down old ones).
  • forge.getEnvironment reads GCLOUD_PROJECT/GCP_PROJECT but Cloud Run sets GOOGLE_CLOUD_PROJECT, so forge may mis-tag environment on every service. Broad blast radius; flagged in WI-6, left as a standalone audit.

Validation (2026-06-16) ​

npm run validate -> 25 passed, 0 failed. npm run test:rules -> 118 passed.

Three PRE-EXISTING branch lint failures (not introduced by this work) were unblocked in passing:

  • CTA baseline: the branch's E2EE verify UI added one new low-emphasis rose-outline "Remove verification" button; cta-classes-baseline.json Chat.jsx bumped 3->4 (the canonical DANGER_TINT_CLASSES is a filled red tint, visually different, so a reskin was deliberately avoided; migrate during the CTA consolidation project).
  • OpenAPI: POST /auth/key-directory/to-device was implemented (M-E2EE-5) but undocumented; added a full spec entry to services/api/auth/openapi.json.
  • Em-dash baseline: comment rewrites in this work removed em dashes from 4 files; baseline re-snapshotted to lock in the reduction.

Round 2 (2026-06-16, same session) - cleared most deferrals ​

After "fix everything", these landed (validate still 25/25):

  • Firestore TTL policy on _rateLimits.expireAt CREATED + ACTIVE on lantern-app-dev (gcloud firestore fields ttls update ... --enable-ttl). Apply the same on prod when that rollout happens.
  • Lock heartbeat (#25): heartbeatRefreshLock (token-fenced) + an unref'd interval in import.js bumps updatedAt at half the TTL, so a long import is not stolen mid-flight. Closes the concurrent-execution-on-overrun gap WI-8 left.
  • In-memory limiter eviction (#48): unref'd periodic sweep drops idle keys.
  • forge.getEnvironment (#6): now reads GOOGLE_CLOUD_PROJECT, so deployed Cloud Run services are tagged with their real environment instead of 'development'. NOTE: broad data-semantics change - prod analytics events now stamp environment='production'; expected/correct, but flag for anyone querying that col.
  • areaPrefix dedup (#42): GEOHASH_AREA_PRECISION is now a single source in @lantern/shared/venues, imported by both the venue-api and the web client.

Still deferred (deliberate, with reasons) ​

  • To-device mailbox relationship check (#12): a connections-by-pair gate needs a composite index (dev-deploy friction) and risks dropping a legit room-key share if any to-device timing edge was missed; LOW value (the durable per-device rate limit already bounds flooding). Worth its own scoped change.
  • Multi-device limitations (#13 safety-number asymmetry, #15/#36 to-device drain): require building real multi-device support, not a bug fix.
  • Micro-opts skipped (crypto-path risk / low payoff): claim-otk and fulfillKeysQuery sequential-IO parallelization (#46/#47); setup-analytics-alerts.sh shared-helper extraction (#41); grandfathered em-dash lines (ratchet burns down).
  • CTA primitive migration (baseline bumped, deferred to the CTA consolidation project).

FIXED (pre-existing, folded in on request): 5 forge realtime-destination tests were failing because forge.test.js mocked the legacy default firebase-admin export, but firestore.js migrated to the modular firebase-admin/firestore (getFirestore + FieldValue). Re-pointed the mock at the modular subpath; forge now 61/61. Test-only change; forge still isn't in the validate orchestrator.

Corrections to the review (read first) ​

  • The "dev analytics echo runs in prod" alarm (review findings #6/#21) does not hold. devEchoEnabled() reads GCLOUD_PROJECT || GOOGLE_CLOUD_PROJECT (trackEvent.js:21), and both deploy-dev.yml and deploy-prod.yml set GOOGLE_CLOUD_PROJECT=$PROJECT_ID. So on prod it reads lantern-app-prod, .includes('prod') is true, and the echo is correctly OFF. The real, smaller issues are captured in WI-6.
  • The multi-device safety-number mismatch (review #13) is real but only bites accounts with more than one device, which Lantern does not ship yet. It is folded into WI-1 (same function) and otherwise low priority.
  • The em-dash convention hits (review #52 to #55) are mostly pre-existing grandfathered lines (the ratchet baseline). Treat as lowest priority (WI-11) and only fix lines this branch actually added.

P0 - E2EE correctness (PR A: "E2EE hardening round 3") ​

WI-1: Make peer change-detection key-material-aware (the MITM hole) ​

  • [ ] Problem. A verified peer can be silently re-keyed and the green "Verified" badge stays lit. The change detector only compares the peer's device-ID set, never the identity-key material.
  • Evidence.
    • deviceTrust.js:233: const state = baseline && !sameIdSet(verified.deviceIds, baseline) ? 'changed' : 'verified'
    • firestoreTransport.js:200: recordPeerDevices(localUid, uid, Object.keys(devices)) passes device IDs only, discarding devices[id] = data.deviceKeys.
    • Rules permit the swap: firestore.rules:914-918 write-once guard has resource == null as its first disjunct, and allow delete is owner-allowed, so delete-then-recreate of the same device id installs any new key.
  • Root cause. Baseline/verified records store deviceIds: string[]. A delete+recreate (or in-place swap that slips the rule) keeps the id set constant, so sameIdSet returns true and trust stays verified. computeSafetyNumber would differ, but getPeerTrustState never compares the live number to the stored one.
  • Fix.
    1. Change the baseline + verified record to store a per-device key fingerprint map ({ [deviceId]: sha256(ed25519) } or the ed25519 itself), not a bare id list. Update recordPeerDevices, getPeerTrustState, markPeerVerified, and sameIdSet to a sameKeyMap comparison.
    2. fulfillKeysQuery passes the full { deviceId: deviceKeys } (or the extracted ed25519 per device) into recordPeerDevices.
    3. Belt-and-suspenders: in getConnectionSafetyInfo (roomCrypto.js:134) compare the freshly computed safetyNumber against the stored verifiedSafetyNumber and force state: 'changed' on mismatch even if the map check missed.
  • Tests. deviceTrust unit cases: same-id key swap to changed; ghost device added to changed; identical re-publish (reinstall) stays verified; first-sight to verified/baseline set without alarm.
  • Risk. Storage-format change for the localStorage records; bump the VERIFIED_PREFIX/BASELINE_PREFIX version so old records migrate cleanly (a missing fingerprint reads as unverified, which is safe).

WI-1b: Rules + test for the device-key delete-recreate gap (DECISION) ​

  • [ ] Add a deny/allow test in firestore.rules.test.js for delete-then-recreate with a different deviceKeys (today only the in-place merge swap is tested; the actual hole is untested, review #33).
  • DECISION, how hard to close it in rules:
    • (a) Recommended: keep warn-but-allow. Rely on WI-1 client detection (which now catches the swap) and document that the rule blocks only in-place swaps. This matches the stated Signal-style posture and is cheap. Update the rule comment at firestore.rules:911-913 to stop claiming the swap is fully blocked.
    • (b) Route device deletion through a server endpoint that writes an identity-key tombstone and enforces key continuity on re-create. Stronger, but adds a write path and a tombstone collection. Track for later.

WI-2: To-device delivery must not ACK a dropped key share ​

  • [ ] Problem. When the server drops a room-key payload (rate limit, content cap, or unrecognized recipient) it still returns 200 {}; the client treats that as success and acks the OlmMachine, which then never re-shares. The recipient is left permanently unable to decrypt that Megolm session.
  • Evidence.
    • Server drops but returns 200: keyDirectory.js:253-259 (continue on rate limit), :243 (unrecognized recipient), :264-267 (content/id caps), then :278return res.json({}).
    • Client acks on 200: firestoreTransport.js:252-253if (!res.ok) throw ...; return {} then markRequestAsSent in fulfillRequest.
  • Fix.
    1. Server /to-device: build a failures map (mirror /claim-otk's shape) keyed by recipient/device with an errcode that distinguishes retryable (rate-limited) from permanent (unrecognized / malformed). Return it in the body (and consider HTTP 429 when everything was rate-limited).
    2. Client fulfillToDevice: if the response carries any retryable failure, throw so the request is NOT marked-as-sent and the machine resurfaces it on the next drain. Permanent failures may be acked (and logged) since retry will not help.
  • DECISION, should key shares be rate-limited at all? The per-(sender, recipient) cap (30 / 10 min, keyDirectory.js:58) can be hit by legitimate churn (rapid block/unblock, rotations). Recommended: keep the cap as anti-flood but return it as a retryable failure (never a silent drop), and raise/scope it so normal use does not hit it. Pair with WI-9 so the cap is actually enforced cross-instance.
  • Risk. Infinite retry if a permanent drop is misclassified as retryable; the errcode split is load-bearing, so test both paths.

WI-3: Re-blacklist a blocked peer's NEW devices on send (DECISION) ​

  • [ ] Problem. Blocking blacklists only the devices the peer had at block time; a device added afterward (or a server-injected ghost) is not blacklisted, so the next shareRoomKey (allDevices strategy) hands it the room key.
  • Evidence.
  • DECISION, product behavior. Can a user still message a peer they blocked (one-sided block) or is the connection closed?
    • If closed: the send path should refuse to encrypt to a blocked peer (cleanest).
    • If still messageable: in encryptMessage, re-apply the blacklist for the peer (a fresh KeysQuery + blacklist) before shareRoomKey, so any new device is caught each send.
  • Tests. Needs the live two-device verification the code comment calls out.

P1 - analytics silent drops (PR B: "analytics taxonomy fixes") ​

WI-4: wave_sent uses an invalid entityType ​

  • [ ] Evidence. lanterns.js:125entityType: 'lantern_pin'. ENTITY_TYPES (analytics/index.js:148) has no lantern_pin, and wave_sent's registered entityTypes is ['wave']. So forge.validateEvent rejects, forge.track throws, and every wave is dropped.
  • Fix. Set entityType: 'wave'. Confirm entityId matches the registered schema (it documents entityId = wave document ID; the route currently passes recipientPinId, so set to the wave doc id or update the schema). Add a test that wave_sent validates.

WI-5: scheduled-lantern event names are unregistered (DECISION) ​

  • [ ] Evidence. schedule.js:67 (lantern_schedule_created) and schedule.js:120 (lantern_schedule_cancelled). The taxonomy registers only lantern_scheduled (analytics/index.js:53). Both throw, so both are dropped.
  • DECISION, taxonomy.
    • (a) Reuse lantern_scheduled for the create route and register one new lantern_schedule_cancelled event. Less taxonomy churn.
    • (b) Register both lantern_schedule_created + lantern_schedule_cancelled as new events and retire lantern_scheduled. Clearer semantics.
    • Recommended: (a). Add tests that the chosen names validate.

WI-6: forge / dev-echo / test env-var alignment ​

  • [ ] Evidence. forge.getEnvironment reads GCLOUD_PROJECT || GCP_PROJECT (forge/index.js:150); the deploys set GOOGLE_CLOUD_PROJECT only; trackEvent.test.js simulates prod via GCLOUD_PROJECT (test:40).
  • Fix.
    1. Fix the test to set GOOGLE_CLOUD_PROJECT (mirror the real deploy) and add cases for GCLOUD-only and neither.
    2. Extract one shared env-detection helper so devEchoEnabled and forge.getEnvironment agree on the var list (reuse, review #8/#28).
    3. Investigate separately (broad blast radius): forge.getEnvironment not reading GOOGLE_CLOUD_PROJECT means forge may mis-detect environment on every deployed service. Confirm what each service actually sets before changing forge; do not bundle that change blind.

WI-6b: Taxonomy validation test (altitude, prevents the whole #4/#5 class) ​

  • [ ] Add a CI test that enumerates every trackEvent / forge.track call site and asserts the (eventName, entityType) validates against the taxonomy. This turns silent runtime drops into a build failure and stops the next one shipping.

P1 - E2EE downgrade + venue lock (PR C) ​

WI-7: Plaintext downgrade latch (DECISION) ​

  • [ ] Problem. A client can re-permit plaintext on an E2EE connection by deleting its own userKeys/{uid} doc.
  • Evidence. connectionIsE2eeCapable uses a live exists(userKeys/...) check (firestore.rules:507-510); the message-create rule allows the plaintext text shape when not capable (firestore.rules:585); userKeys delete is owner-allowed (firestore.rules:883).
  • DECISION, enforce "once E2EE, always E2EE":
    • (a) Recommended: a monotonic latch on the connection doc (e.g. e2eeLocked) that can transition false to true but never back; gate the plaintext shape on !e2eeLocked instead of the live exists() check.
    • (b) Make userKeys delete server-only (drop the client allow delete) and handle device/account deletion via a server endpoint.
  • Tests. Deny test: once a connection is e2ee-capable, deleting userKeys must NOT re-permit a plaintext message.

WI-8: Fence the venue refresh lock against clobber ​

  • [ ] Problem. A slow import that overran the TTL and had its lock break-glass stolen still clears inProgress on completion/failure, clobbering the new holder and breaking serialization. No heartbeat means any import slower than 5 min is treated as dead while alive.
  • Evidence.
  • Fix. Token-fence the lock: acquireRefreshLock mints a unique lockToken per acquisition and writes it; releaseRefreshLock and the primary-cell branch of updateRefreshMetadata do a transactional compare-and-clear that only clears inProgress when the stored token matches the token this import acquired. Thread the token from import.js through to both release paths. Add a heartbeat (periodic updatedAt bump) OR raise the TTL above the realistic max import time; with fencing, a late clobber becomes a no-op even if the TTL is exceeded.
  • Tests. Extend refreshLock.test.js: overran-import release must not clear a stolen lock; two concurrent imports never both hold.
  • Fold in cleanup: the catch-block lat/lng re-check (import.js, review #44) and the GEOHASH_AREA_PRECISION duplication (move to @lantern/shared/venues next to REFRESH_LOCK_TTL_MINUTES, review #42).

P2 - rate-limiter durability + cleanup (PR D) ​

WI-9: Back the security rate limits with a durable store (DECISION) ​

  • [ ] Problem. The M-E2EE claim + to-device caps live in a per-process in-memory Map (rateLimiter.js:14), so on multi-instance Cloud Run the real cap is instances x limit, it resets on cold start/deploy, and the keyspace this PR introduces grows without eviction (cleanWindow filters timestamps but leaves empty entries).
  • DECISION:
    • (a) Recommended: move the security-critical limits (claim-otk per-target, to-device per-recipient) to a Firestore-backed window/counter (durable, cross-instance). These ops are rare, so the extra read/write cost is negligible. Keep IP/user limits in-memory.
    • (b) Accept best-effort: evict empty Map entries, set min-instances >= 1, document the limit as soft. Cheaper, weaker.
  • At minimum (either option): evict empty entries to stop unbounded growth (review #48).

WI-10: Per-device flood count + mailbox relationship check ​

  • [ ] After WI-2/WI-9: count the to-device limit per written doc (per device), not per request (review #40, today one request writes up to 20 docs under one counter tick). Add a connection/relationship check before depositing into a recipient's mailbox so only an established chat peer can write to-device (keyDirectory.js:241, review #12). Lower priority.

WI-11: Cleanup batch (low) ​

  • [ ] Dead code: trackEvent.js :dropped status is unreachable (forge.track returns {tracked:true} or throws). Drop the branch.
  • [ ] Efficiency: parallelize independent Firestore ops: /to-device col.add loop and /claim-otk per-device transactions (keyDirectory.js), per-peer getDocs in fulfillKeysQuery (firestoreTransport.js:184-201). Give getConnectionSafetyInfo a read-only path that does updateTrackedUsers + KeysQuery without the OTK-claim round-trip (roomCrypto.js:119).
  • [ ] Reuse: setup-analytics-alerts.sh is a near-copy of setup-billing-alerts.sh (extract a shared helper).
  • [ ] Simplification: unused getPeerTrustState barrel re-export (signal/index.js); over-spread return in getConnectionSafetyInfo.
  • [ ] Doc drift: deploy-dev.yml comment says venue refresh is "limit-capped to 100/run" but no ?limit is passed (default 50). Fix the comment or pass the limit.
  • [ ] Em dashes: only fix lines THIS branch added (offerService.js:121, firestore.rules:604, venueService.js:476, offerService.test.js:137); the rest are grandfathered baseline. The write-time hook blocks new ones.
  • [ ] Revisit markPeerVerified re-baseline (review #22) after WI-1 makes the records key-aware.

Suggested sequencing ​

  1. PR A (P0 E2EE): WI-1, WI-1b, WI-2, WI-3. Update SECURITY_REMEDIATION.md (new H-E2EE / M-E2EE follow-up rows) in the same PR.
  2. PR B (analytics): WI-4, WI-5, WI-6, WI-6b.
  3. PR C (downgrade + lock): WI-7, WI-8.
  4. PR D (rate-limiter + cleanup): WI-9, WI-10, WI-11.

Bundle related work; open as drafts to dev; run npm run validate once before each PR. Decisions to confirm before implementing: WI-1b, WI-2 (cap policy), WI-3 (block messageability), WI-5 (taxonomy), WI-7 (latch vs server-only delete), WI-9 (durable vs best-effort).

Built with VitePress

Copyright © 2026 Lantern