Security fixes (backend campaign 4)

docs/erasure-behavior.md

@@ -20,7 +20,15 @@ response can be answered truthfully. It is the source of record for the
 `[auth][csrf]`) performs a **soft delete**:
 
 0. An **email-OTP gate**: the caller must re-prove control of the account email
-   before any destructive work runs.
+   before any destructive work runs. Then **the session ban marker is set**
+   (`SessionService.markBanned`), before anything is erased. A cached session
+   projection is checked against that marker and the session epoch, never against the `sessions` table, so deleting the rows alone would
+   leave a cached bearer (and a websocket re-auth) working, and sliding its
+   expiry forward, up to the 90-day absolute session cap. If the marker cannot be written the request fails with 503 and
+   nothing is erased; if the erasure transaction below then fails, the marker is
+   cleared again so the still-live account keeps working (a failure to clear it
+   is logged: the account stays locked out until the marker expires or an
+   operator restores its status).
 1. `UserStore.softDeleteAndAnonymize(userId)` — see
    `services/api/src/auth/pg-stores.ts`. **One transaction**, so a partial erasure
    is not a reachable state:
@@ -34,8 +42,9 @@ response can be answered truthfully. It is the source of record for the
    - **Unlists** the user's `public` reports (→ `hidden`).
    - **Transfers, then cancels** the events they organize — the host-transfer
      ladder below. Only what nobody could take over is cancelled.
-   - **Releases the organizations they owned** and scrubs their pending team
-     invitations — see below.
+   - **Releases the organizations they owned**, scrubs their pending team
+     invitations and revokes the pending organization invitations they sent or
+     received (see below).
    - **Scrubs their own attendee free text** (event answers, attendee names,
      host notes) and cancels their live waitlist entries, releasing any seats
      those entries held.
@@ -44,33 +53,40 @@ response can be answered truthfully. It is the source of record for the
      the rows written while it did are retained. See the ⚖️ DECISION below.
    - **Revokes and scrubs their issued service-hours certificates** — see the
      dedicated section below.
+   - **Deletes** every durable session row, every device push token and every
+     notification row of the user, so no session outlives the tombstone and no
+     device can be reached after the erasure commits.
    - Keeps every content foreign key intact (the rows survive; the author
      de-links).
    - AFTER the commit, best-effort deletes each revoked certificate's R2 object.
      Failures are logged, never thrown: a completed erasure must not surface to
      the client as "deletion failed".
-2. `SessionStore.banUser(userId)` — deletes every durable session row, drops the
-   write-through cache entries, and sets the ban/veto marker so any warm session
-   that slipped a revoke is rejected on its next request.
-3. Clears the session + CSRF cookies on the response.
-4. Three independent best-effort cleanups (`allSettled`, each logged on failure):
-   unlink the OAuth identities, hard-delete the device push tokens, and write the
-   audit-log row (`account.deleted`, actor = the user).
+2. Clears the session + CSRF cookies on the response.
+3. Three independent best-effort steps after the commit (`allSettled`, each logged
+   on failure, none of them able to fail a deletion that already happened):
+   `SessionService.banUser(userId)` refreshes the ban marker, bumps the session
+   epoch and drops any write-through cache entries it can still find; unlink the
+   OAuth identities; and write the audit-log row (`account.deleted`, actor = the
+   user). Revocation does not depend on this step: by the time it runs every
+   session row is gone and the marker set in step 0 already rejects every
+   cached projection.
 
 ## What is scrubbed vs. kept
 
 | Data | After `DELETE /me` |
 |---|---|
-| Live sessions / login | **Revoked** — all sessions deleted, ban marker set, cookies cleared. |
+| Live sessions / login | **Revoked**: the ban marker is set before the erasure (the deletion is refused if it cannot be), every session row is deleted inside the erasure transaction; afterwards (best-effort) the epoch is bumped and the cached sessions evicted; cookies cleared. |
 | DM reachability | **Off** — `allow_direct_messages = false`. |
 | `display_name`, `handle`, `email`, `bio`, `avatar_url`, `avatar_media_id`, `social_links`, `donation_url`, `primary_organization_id` | **Scrubbed** on the `users` row — nulled, or replaced with the `Deleted User` label / a generated placeholder handle. |
-| OAuth identity links | **Deleted** (best-effort, step 4) — otherwise a provider sign-in walks back into the tombstone once the ban marker's TTL lapses. |
-| Device push tokens | **Deleted** (best-effort, step 4). |
+| OAuth identity links | **Deleted** (best-effort, step 3); otherwise a provider sign-in walks back into the tombstone once the ban marker's TTL lapses. |
+| Device push tokens | **Deleted** inside the erasure transaction. |
+| Notifications | **Deleted** inside the erasure transaction (they can hold verbatim chat/DM previews and are not civic record). |
 | Reports the user filed | **Kept** as rows; the user's `public` ones are flipped to `hidden` (see public rendering below). |
 | Discussion comments, chat, DMs the user wrote | **Kept** (soft-deleted only where the user deleted them individually). |
 | Cleanups organized / joined | **Kept**; an `upcoming`/`active` event they organize is TRANSFERRED where anyone can take it over, and cancelled only when nobody can (ladder below). |
 | Organizations they belonged to | Membership rows **deleted**, and `users.primary_organization_id` (the affiliation badge pin, 0.43.0) is nulled in the same transaction. An organization they OWNED promotes its earliest live admin; one left with nobody is **soft-deleted** and its events lose their `organization_id`. The events keep their own `donation_url` — since the platform stopped processing donations that link belongs to the host, not to the organization's verification. |
 | Event team invitations they sent or received | Pending ones **revoked**, the invitee address **scrubbed**. |
+| Organization invitations they sent or received | Pending ones **revoked** (`org.invite_revoked` audit row per invite, `meta.reason = 'account_deleted'`), so an admin's invite cannot seat anyone after the admin is gone. An invite addressed only by email to the departing user's former address is not matched and expires on its own TTL. |
 | Posts published under an organization (`posts.organization_id`, 0.43.0) | **Kept**, exactly like every other post: the FK names the organization, not the person, and the author de-links the same way. Nothing about the org link identifies the departing account. |
 | `event_consents` | **Kept, untouched by every lane.** It carries no contact detail of its own (the subject is a foreign key) and it is the artifact THAT consent existed; the account row it points at is tombstoned rather than deleted. |
 | `cleanup_registrations`, `cleanup_registration_seats`, check-ins | **Kept** — they are the roster record of someone else's event. Only the departing person's own free text (`cleanup_answers` values, `attendee_name`, `host_note`) is scrubbed in the same transaction. |
@@ -79,6 +95,7 @@ response can be answered truthfully. It is the source of record for the
 | `donations` | **Kept.** `user_id` NULLed and `profile_unlinked_at` stamped immediately. On a CHARGED donation `donor_email` / `donor_name` survive until `charged_at + 7 years`; on one that never charged they are NULLed at once. ⚖️ DECISION below. |
 | `org_payouts` | **Kept, with the FK intact.** The row records that an organization moved its own money, not anything about the person who pressed the button; `requested_by` declares `ON DELETE SET NULL`, but civfix erasure is a SOFT delete, so that action never fires and the actor stays the tombstoned account. There is no contact detail in the table to scrub. |
 | `cleanup_slot_claims` (which signup slot they took, P9) | **Kept**. The row is `(cleanup_id, user_id, slot_id, claimed_at)` — roster data with no free-text PII, held exactly like the `cleanup_members` row it accompanies, and with no `ON DELETE CASCADE` to `users` by design (`drizzle/0063_cleanup_slots.sql`). The attendee/roster read joins `users` with `deleted_at IS NULL`, so a tombstoned claimant disappears from the visible roster; the row still counts toward the slot's `claimed` total. |
+| `media_assets.uploader` (who created an upload, 0181) | **Kept**, like `reports.reporter_user_id`: it names the tombstoned account, never an email or IP. Nulling it would make the departed user's unbound uploads claimable by anyone inside the claim window. |
 | `service_hours_certificates` (issued PDF transcripts, P5) | **Revoked + scrubbed**, rows kept, **R2 objects deleted**. See the next section. |
 
 ### The host-transfer ladder
@@ -295,11 +312,12 @@ instead of telling them to use an add-email flow that does not exist.
 
 ### F088 (delete half) — notifications purged on account deletion
 
-The `DELETE /me` post-revocation cleanup fan-out gained a
-`DELETE FROM notifications WHERE user_id = $1` step (alongside oauth-unlink,
-push-token purge, and the audit row). Notification rows are private to the deleted
-user (they can hold verbatim chat/DM previews) and are not civic record, so they
-are erased. Add this table to the "scrubbed on deletion" list. (The time-based
+Account deletion erases the user's notification rows
+(`DELETE FROM notifications WHERE user_id = $1`), now inside the erasure
+transaction together with the session and push-token deletes, so a failure rolls
+the whole erasure back instead of leaving them behind. Notification rows are
+private to the deleted user (they can hold verbatim chat/DM previews) and are not
+civic record, so they are erased. (The time-based
 retention sweep for notifications is the media-worker half of F088.)
 
 ### F139 — DSAR export completeness + truncation remedy

docs/security/2026-07-24-full-backend-security-review.md

@@ -322,7 +322,7 @@ Everything an operator has to do by hand, in order, plus the two infra facts and
 node dist/db/migrate.js        # == pnpm --filter @civfix/api db:migrate
 ```
 
-`drizzle/` holds **163 files**, `0000_extensions.sql` … `0180_civfix_official_account.sql`. The nine rows
+`drizzle/` holds **165 files**, `0000_extensions.sql` … `0182_organization_invites_invited_by_idx.sql`. The nine rows
 below are exactly what this change set adds — `0052`–`0060`, contiguous, no gaps — and everything from
 `0000` through `0051_social_posts.sql` predates it. (`0060` arrived later than the rest, with the feed
 redesign; it is listed here because this table is the single operator runbook. `0061`–`0064` arrived
@@ -570,6 +570,8 @@ foreign key into `organizations` from `0105`.
 | `0178_event_announcements.sql` | widens `broadcasts_kind_check` to accept the new `announcement` kind (event announcements ride the existing host-broadcast pipeline rather than a parallel table), adds the partial `broadcasts_announcement_public_idx (cleanup_id, created_at DESC, id DESC) WHERE kind = 'announcement'` that backs the public per-event announcements list, and re-creates `broadcasts_scrub_idx` with `kind <> 'announcement'` so the retention scrub can never NULL an announcement body - an announcement is permanent public event content rendered on the event page, not a one-shot email. `broadcasts` is NOT on the hot-table list in `docs/out-of-band-indexes.md`, so both indexes build inline; `IF NOT EXISTS` makes an out-of-band CONCURRENTLY build a no-op | Every `createEventAnnouncement` fails on the CHECK constraint. Applying the CHECK but not the scrub-index swap still works (the repository query carries the same predicate), it just scans more rows |
 | `0179_address_resolution.sql` | the address-resolution overhaul: creates `geocode_cache` (a read-through cache of reverse geocodes keyed by the shared 5-decimal point key, with its `geocode_cache_resolved_at_idx`; derived public data only, no user/report/event reference, TTL applied on read rather than by a cron), adds `cleanups.address_source` (resolved | edited | manual) and backfills it to `manual` for every event that already has an address, and adds `reports.addr_source` + `reports.addr_precision`. All three CHECK constraints are `NOT VALID` (new columns, so they hold by construction) and are listed in the outstanding-VALIDATE set below. No new index on a hot table: both `reports` columns are plain row columns | `POST /map/resolve-address` and every create path still answer, but uncached (the cache swallows its own errors) and with no provenance persisted; a new client's event create fails on the missing `cleanups.address_source` column |
 | `0180_civfix_official_account.sql` | DATA only, no DDL (the Drizzle mirror is unchanged): creates the official `@civfix` account (`users.id` `00000000-0000-4000-8000-00000000c1f1`, display name `CivFix`, role `citizen`, no email, DMs closed) that admin-panel chat posts are authored by. First it frees the handle: any OTHER row holding `civfix` (citext, so any case) gets its auto-placeholder handle `user` + the first 12 hex digits of its id, and nothing else about that row changes - not its display name, and not `handle_changed_at`, so its owner may pick a new handle at once. The INSERT conflicts on the id only, so a placeholder or handle clash fails the file instead of skipping it. At most two `users` rows are touched: milliseconds, row locks only. `SessionService` refuses to mint or resolve a session for the id, and with no email and no OAuth identity no sign-in path can reach it | Admin-panel report and event chat posts fail on the `chat_messages.sender_id` foreign key once the code that authors them as the official account is deployed; nothing else reads the row |
+| `0181_media_uploader.sql` | adds the nullable `media_assets.uploader` column (`u:<user id>`, `a:<anon token id>` or `anon`), written by upload create, so a report, post, chat or avatar claim by uploadId binds only the caller's own upload. Nullable with no default and no backfill: a catalog-only change on a hot table, lock held for milliseconds, no index (claims find the row through the unique `upload_id`). Rows from before the deploy stay NULL and are claimable only inside the claim window | Upload create fails writing a column that does not exist; claims fail on the missing column |
+| `0182_organization_invites_invited_by_idx.sql` | adds the partial index `organization_invites_inviter_pending_idx (invited_by) WHERE status = 'pending'`, so the account-erasure statement that revokes the pending organization invites a user sent or received (`invited_by = $1 OR user_id = $1`) can BitmapOr this index with `organization_invites_invitee_pending_idx` instead of scanning the table. `organization_invites` is not on the hot-table list, so it builds inline under the migration transaction (milliseconds at current size); if the table has grown, build it `CONCURRENTLY` by hand first and the `IF NOT EXISTS` guard makes the file a no-op | Account deletion still works, with a sequential scan of `organization_invites` inside the erasure transaction |
 
 
 **Deferred to the NEXT release** (expand/contract, `docs/migrations-expand-contract.md`): 0.43.0

infra/email-worker/README.md

@@ -1,11 +1,18 @@
 # civfix Email Worker
 
 Cloudflare Email Worker that ingests catch-all `*@civfix.org` mail. It writes the raw `.eml` to
-**R2** (`inbound/pending/<messageId>.eml` — the source of truth) and best-effort POSTs an HMAC-signed
+**R2** (`inbound/pending/<slug>.<digest>.eml`, the source of truth) and best-effort POSTs an HMAC-signed
 `{ key }` nudge to the backend webhook. The backend re-fetches from R2, parses, routes (reply → mail
 thread; else → inbox), and reconciles `inbound/pending/` on boot + a cron sweep, so a missed nudge is
 never a lost message.
 
+The pending key is `<slug>.<digest>`: `<slug>` is the Message-ID with `<>` stripped and every character
+outside `A-Za-z0-9._@-` replaced by `_`, cut to 120 characters, and `<digest>` is the first 32 hex
+characters of the SHA-256 of the raw message. The sender chooses the Message-ID, so the digest keeps a
+second mail with the same (or a same-slugging) Message-ID from overwriting a pending one, while a
+byte-identical redelivery lands on the same key. A message with no Message-ID is stored under the full
+64-character digest alone.
+
 The Worker does **not** judge sender authentication. `message.headers` does not expose the
 `Authentication-Results` header Cloudflare stamps (workerd#6740), so a header check here never fired.
 The backend is the only gate: it reads the top-most `Authentication-Results` in the raw message and
@@ -73,7 +80,8 @@ Content-Type: text/plain
 We received your report.'
 ```
 
-Expect an `inbound/pending/test-001@example.gov.eml` object and a signed POST to your local backend.
+Expect an `inbound/pending/test-001@example.gov.<digest>.eml` object and a signed POST to your local
+backend.
 
 ## Deploy + enable Email Routing
 

infra/email-worker/src/index.ts

@@ -11,8 +11,7 @@ export default {
   async email(message: ForwardableEmailMessage, env: Env, ctx: ExecutionContext): Promise<void> {
     const rawBytes = await new Response(message.raw).arrayBuffer()
 
-    const messageId = await deriveMessageId(message.headers, rawBytes)
-    const key = `${PENDING_PREFIX}${messageId}.eml`
+    const { messageId, key } = await derivePendingIdentity(message.headers, rawBytes)
 
     try {
       await env.R2_BUCKET.put(key, rawBytes, {
@@ -35,9 +34,41 @@ export default {
   },
 }
 
-export async function deriveMessageId(headers: Headers, raw: ArrayBuffer): Promise<string> {
+const PENDING_SLUG_MAX_CHARS = 120
+const PENDING_DIGEST_CHARS = 32
+
+export interface PendingIdentity {
+  messageId: string
+  key: string
+}
+
+// The sender picks the Message-ID, so a key made from it alone lets a later mail (or a Message-ID
+// that slugs alike) overwrite a pending .eml before the backend drains it; the content digest makes
+// the key follow the bytes, while an identical redelivery still lands on the same key. One digest
+// serves both the key and the fallback id, so a message is hashed once however it is addressed.
+export async function derivePendingIdentity(
+  headers: Headers,
+  raw: ArrayBuffer,
+): Promise<PendingIdentity> {
+  const digest = await contentDigest(raw)
   const slug = slugify(headers.get("message-id") ?? "")
-  if (slug.length > 0) return slug
+  const keySlug = slug.slice(0, PENDING_SLUG_MAX_CHARS)
+  const name = keySlug.length > 0 ? `${keySlug}.${digest.slice(0, PENDING_DIGEST_CHARS)}` : digest
+  return {
+    messageId: slug.length > 0 ? slug : digest,
+    key: `${PENDING_PREFIX}${name}.eml`,
+  }
+}
+
+export async function derivePendingKey(headers: Headers, raw: ArrayBuffer): Promise<string> {
+  return (await derivePendingIdentity(headers, raw)).key
+}
+
+export async function deriveMessageId(headers: Headers, raw: ArrayBuffer): Promise<string> {
+  return (await derivePendingIdentity(headers, raw)).messageId
+}
+
+async function contentDigest(raw: ArrayBuffer): Promise<string> {
   try {
     return await sha256Hex(raw)
   } catch {