Skip to content

Security fixes (backend campaign 4) - #93

Closed
theobong wants to merge 46 commits into
chore/backend-campaign-03-formatfrom
fix/backend-campaign-04-security
Closed

theobong wants to merge 46 commits into
chore/backend-campaign-03-formatfrom
fix/backend-campaign-04-security

Conversation

@theobong

@theobong theobong commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

What changed

Security fixes across the API, found by the campaign audit and hardened by an adversarial review pass. Sign-in never adopts an account whose email was never verified. Account deletion revokes every session before it runs. Uploads can only be attached by the person who uploaded them, and nobody but the uploader ever receives an unprocessed original. Private events answer like unknown ones to people without standing. Bans, org invites, host messaging, CSV exports, inbound city mail, websockets and logs each close their own gaps. Nothing is removed from any response a client relies on.

Before you start

  • Where: staging (civfix.dev, admin.civfix.dev, api.civfix.dev) after this merges to main
  • Sign in as: a citizen account of your own (emailed code; the reviewer code is mobile-only and not enabled on staging) for web and mobile; a second citizen account for the "other person" steps; operator (admin.civfix.dev through Cloudflare Access) for admin
  • Data: one event you host with a ticket type and an attendee; one photo report of your own
  • Stacked on Backend formatting pass with the repo's Prettier config #91 (formatting). Read only this PR's diff against that branch.
  • Size: 15226 counted lines. Campaign PR; the author approved PRs over the 400-line cap for this cleanup. About three quarters of it is tests.
  • Migrations: 0181 (nullable media_assets.uploader, catalog-only) and 0182 (partial index on pending org invites by inviter, small table).

Verify

[Web]

  1. Sign out, tap "Sign in", then "Continue with Google" with a Google account whose email is already used by an existing civfix account that never verified that email — Expect: sign-in is refused with "An account already uses this email address. Sign in the way you did before, or contact support." (If you have no such account, skip; the API check is under Staging checks.)
  2. Start a report, tap "Choose from library", pick a photo, tap "Continue" through the steps, then "Drop pin", then "View my report" — Expect: while the photo processes you see "Photos are still processing..."; once processed, the photo appears.
  3. As the second citizen, open the same report while its photo is still processing — Expect: no photo and no broken image; the photo appears once processing finishes.
  4. Open an event chat ("Message crew"), attach a photo and send it; as the second citizen in the same chat, watch the room — Expect: the sender sees the photo; the other member does not receive a link to the photo until it is processed (it appears after reopening the chat).
  5. As host, open your event's console, click "Attendees", then "Export", then "Attendees"; when it is ready click "Download" under "Recent exports" and open the file in a spreadsheet — Expect: a name typed as x;=1+1 shows as text with an apostrophe before the =, never as a formula; plain numbers stay numbers.
  6. Open Settings, "Account", "Delete account", "Email me a code", enter the code, "Permanently delete" — Expect: you are signed out; signing in again on another tab or device with the old session does not work.

[Mobile]

  1. Open the app, sign in, and repeat Web step 2 from the "Report" tab ("Drop pin", "View my report") — Expect: "Photos are still processing..." until processed, then the photo.
  2. Long-press your own message in an event chat, tap "Edit", change it, tap the check-mark button — Expect: the message shows "(edited)".
  3. Delete the account from Settings, "Account", "Delete account" — Expect: signed out; the app does not keep working on the old session.

[Admin]

  1. Open admin.civfix.dev, go to "Host messaging", pick a host, click "Suspend messaging", give a reason and confirm — Expect: "Messaging suspended" toast; a message that host scheduled earlier does not go out, and a message already sending stops.
  2. Go to "Mail", "Inbox", open an inbound message with an attachment — Expect: the message and attachment list show as before.

Regression

Signing in — [Web] [Mobile]

  1. Sign in with "Continue with Google" (and with an email code) on an account that already exists and verified its email — Expect: signed in as before.

Submitting reports and posts with photos — [Web] [Mobile]

  1. File a report with two photos, and a post with a photo — Expect: both publish with their photos after processing, as before.
  2. Change your profile photo — Expect: the new photo shows after processing.

Chat and DMs — [Web] [Mobile]

  1. Send a text message and a photo in a DM and in a group chat — Expect: both arrive; the photo appears for the other person once processed.
  2. Send a burst of messages quickly — Expect: the chat stays connected.

Events and RSVPs — [Web]

  1. Signed out, on a public event tap "No account? RSVP as a guest", "Continue as guest", fill in the form, "Send code", enter the code, "Confirm RSVP" — Expect: "You're on the list" as before.
  2. As host, remove an attendee who holds a ticket, choosing to ban them — Expect: they leave the roster, their ticket no longer checks in, and their seat is free again.
  3. As host, schedule a message ("Messages", "New message", "Schedule"), then cancel it ("Cancel", "Cancel the message") — Expect: "Message cancelled".

Organizations — [Web]

  1. In an organization's "Members", click "Invite", fill in an email, "Send invite" — Expect: "Invitation sent".
  2. Change an admin's role to member ("Change role?"), then accept an invite that admin had sent, from the invitee's account — Expect: the invite is no longer valid; nobody is seated.

Host email links — [Web]

  1. Open a link in an invite or broadcast email — Expect: it points at civfix.dev on staging (not civfix.org, not localhost).

Unsubscribe — [Web]

  1. Open the unsubscribe link at the bottom of a host message email — Expect: "You're unsubscribed from this event's messages."

Findings addressed

Grouped by theme; IDs are campaign ledger IDs (.campaign/backend/ledger.json, not in this repo).

  • Sign-in and sessions: BE-SEC-012 (high: unverified provider email took over an account), BE-SEC-014, BE-SEC-016, BE-SEC-097, plus review follow-ups (refuse to adopt an unverified-email account on OAuth and operator sign-in; erasure sets the ban marker before it runs).
  • Account erasure: BE-SEC-043, BE-SEC-059 (invites of a deleted account).
  • Media: BE-SEC-119 (high: validating uploads served publicly), BE-SEC-098, BE-SEC-020, BE-SEC-105, plus review follow-ups (uploader recorded and required on every claim; chat and DM readers other than the sender never get an unprocessed original; event pages serve processed media; finalize checks the owner).
  • Private events and existence oracles: BE-SEC-036 (high: guest RSVP to private events), BE-SEC-040, BE-SEC-046, BE-SEC-112.
  • Registration, bans, orgs: BE-SEC-037, BE-SEC-051, BE-SEC-057, BE-SEC-066, BE-SEC-067, BE-SEC-069, BE-SEC-078, plus review follow-ups (one ban path; invites re-check the inviter under the org lock).
  • Host messaging and exports: BE-SEC-071, BE-SEC-072, BE-SEC-073, BE-SEC-074, BE-SEC-075, BE-SEC-095, plus review follow-ups (CSV separators; mid-send suspension; atomic export audit).
  • Inbound city mail: BE-SEC-080 (high: attachment overwrite), BE-SEC-025, BE-SEC-081, BE-SEC-026.
  • Websocket and chat: BE-SEC-108, BE-SEC-109, BE-SEC-111, BE-SEC-124, BE-SEC-126.
  • Errors, logs, links: BE-SEC-001, BE-SEC-002, BE-SEC-003, BE-SEC-009, BE-SEC-023, BE-SEC-079, BE-SEC-106.
  • Anonymous reports: BE-SEC-013, BE-SEC-017, BE-SEC-039 (claim nudge refuses foreign origins).

Decisions for the reviewer

  • Existing data is untouched. No backfill, no data correction. Accounts that already hold an unverified email keep it; the read-only audit query is under Staging checks. Waiting-list rows of users banned before this deploy are excluded from queue positions and host counts instead of being cancelled.
  • Sign-in: Google/Apple sign-in never stores or adopts an unverified provider email. An existing account whose email was never verified is refused (409), not adopted and not modified; the operator console refuses it too (403, audited). Email-code sign-in onto such an account is refused too when a provider identity is linked to it (409, audited, points to support), because that identity never proved the address and letting the inbox owner in would share the account with it. An unverified account with no provider identity, and every verified account, sign in as before. No existing row is modified: cleaning up planted accounts (unlink the unproven identities, mark the email verified) rewrites data and is left for the maintainers to approve; the audit query sizes it.
  • Account deletion sets the session ban marker before the erasure and fails closed (503, nothing changed) if it cannot. Sessions, push tokens, notifications and pending org invites go inside the erasure transaction. The trade-off: deletion is unavailable during a Redis outage; if the erasure and the marker clear both fail, the account stays locked until an operator restores it or the marker expires (30 days).
  • Media ownership: new nullable media_assets.uploader (0181). Every claim by uploadId (report, anonymous report, post, chat, DM, avatar, group avatar, event cover and gallery, page media, org logo, verification document) and finalize require the caller to be the uploader. Rows from before the deploy (NULL) are claimable only inside the existing 6-hour claim window, so they age out. Operator org logos must be uploaded through the operator's own console session, which is how the admin app uploads today. A browser that uploaded as a guest and then signed in can still use those uploads for its report or post (the verified guest cookie is carried alongside the session for this check only; quota, cookies and CSRF are unchanged). Callers with neither a session nor a guest cookie share one upload subject; closing that needs the guest cookie issued at upload time (cookie change, deferred).
  • Unprocessed uploads are never served to anyone but the uploader. Public reads use the processed copy only; hosts see no cover, logo or avatar until processing finishes. Chat and DM attachments still processing are omitted for everyone except the sender (the contract requires a URL on every attachment); they appear on the next history read, and a DM preview shows no "Photo" text meanwhile. Handoff to civfix-app for a pending state.
  • Private events answer like unknown events (404) to people without standing: guest RSVP, event hours, content reports, post deletion. Guests cannot self-register on a private event; a host-seated walk-up and an already-waiting guest keep working.
  • Bans go through one path under the event lock: ban row, member role, every active registration including ticketed ones, seats, waitlist places, slot claims. A banned ticket no longer checks in.
  • Org invites re-check the inviter under the organization lock when created and when accepted; an invite from someone who has since lost the power to invite is refused ("That invitation is no longer valid.") and revoked with a reason.
  • Websocket: ?token= works only when WS_ALLOW_QUERY_TOKEN is set (validated env, default off). Cookie upgrades check Origin against the allowlist; a missing Origin is allowed (native clients). Before authentication: 32 frames and 64 KiB are buffered, extra frames are dropped. After: 64 queued frames or 256 KiB, then one RATE_LIMITED frame and close 1008.
  • Production 5xx bodies read "Internal error" unless the error is explicitly marked user-safe (form unavailable, unsubscribe unavailable, account deletion unavailable, identity provider unreachable). 4xx messages are unchanged. Logs are redacted at any depth, drop query strings, and stop after about 1,000 values per line.
  • Host messaging: critical automated notices cannot be edited, deleted or cancelled (409); removing an unknown message is a 404; a suspended organization's scheduled and in-flight broadcasts stop (delivery suppression reason kill_switch, because the DB constraint has no org value; the audit row carries org_suspended). Unsubscribe write failures answer 503 so one-click senders retry.
  • CSV exports quote every cell, prefix a leading formula trigger, and put an apostrophe before a trigger that follows an in-value separator (;, ,, tab, line break) even when spaces or double quotes sit between them, so a value like x;=1+1 shows an apostrophe. Numbers are never prefixed. The roster export's audit row is written in the export's transaction; an audit failure now fails the request.
  • Inbound city mail: attachments are keyed by message content, so a reused Message-ID cannot overwrite stored evidence; a Message-ID lookup failure leaves the mail pending for the sweep; the consumer-domain list gained US ISP and regional webmail domains (inverting to exact-mailbox matching by default stays a product decision, because many cities mail from non-.gov domains).
  • Anonymous reports: only the signed anon token is accepted; claim codes are never stored in plain text; a replay issues a fresh claim code; a lost duplicate-submit race answers the existing retryable 409 instead of rotating the winner's code.
  • Links in mail use the configured web origin; production refuses to start without it (env validated at boot), and nothing falls back to civfix.org or localhost.
  • Boundary prune script requires --yes and re-resolves only the rows it touches.
  • Suggest-contact input is bounded here (email 254, form URL 2048 http(s), geoid 64) until the shared contract carries the bounds (handoff).

User-visible copy changes

  • Google/Apple sign-in onto an account whose email was never verified (409): "An account already uses this email address. Sign in the way you did before, or contact support."
  • Email-code sign-in onto an unverified account that a Google/Apple identity holds (409, new): "This email address is linked to an account that needs a quick check before you can sign in. Contact support at support@ and we'll sort it out."
  • Account deletion when the session ban cannot be set (503): "We couldn't delete your account right now. Nothing was changed. Please try again in a few minutes."
  • Production 5xx bodies: "Internal error" instead of internal detail (exposed ones keep their copy: "This form is temporarily unavailable. Please try again later.", "Could not reach the identity provider.", "Cloudflare Access is not configured.").
  • Unsubscribe write failure (was a silent 200): 503 "We couldn't save your unsubscribe. Please try again."
  • Critical automated notices (were allowed): 409 "Automatic messages can't be edited." / "... can't be deleted." / "... can't be cancelled."
  • Host messaging suspension of an unknown user (was 500): 404 "User not found."
  • Organization suspension refusals no longer echo the operator's private reason: "This organization has been suspended, so it can't be changed right now."
  • A chat photo that cannot be attached (was silently dropped): "One or more media uploads are unavailable."
  • An anonymous replay after the report was claimed: "This report was already submitted and can no longer be claimed."
  • Suggest-contact validation: "That email address is too long.", "That link is too long.", "Use a link that starts with http:// or https://."
  • Private events, hidden posts and unknown messages now answer "Event not found" / "Post not found" / "Message not found" where some paths said 403.
  • Email footers: user-authored text is no longer turned into links.

Tests changed

Existing tests whose expectation changed because the fix changes the behavior they pinned (no test was skipped, deleted or loosened):

  • test/unit/delete-account-route.test.ts: push tokens and notifications are purged inside the erasure transaction, so the route no longer issues those deletes itself.
  • test/unit/email-format.test.ts: a URL inside host-authored footer text is no longer auto-linked; the footer is built from typed segments.
  • test/unit/media-routes.test.ts: an unsigned anon cookie is no longer a quota subject (0 charges instead of 2).
  • test/integration/anon-pg.test.ts: a replayed anonymous submit returns a fresh claim code (plaintext codes are never stored).
  • test/unit/export-csv-security.test.ts (rewritten) and test/unit/host-export-csv.test.ts: separator-borne formulas are neutralized; every cell is quoted; numbers are not prefixed.
  • test/unit/host-route-audit-security.test.ts: the roster export audit failure now fails the request (was 200 plus a warning).
  • test/unit/socket-lifecycle-security.test.ts: with a 64-frame backlog the token bucket (60) answers the last frames with RATE_LIMITED while the socket stays open.
  • test/unit/anon-repository-security.test.ts: the lost duplicate-submit race answers 409 instead of rotating the winner's code.
  • test/integration/media-attach-claim-security-pg.test.ts: a chat send with an unclaimable upload is refused (422) instead of silently dropping it; fixtures now name their uploader.
  • test/integration/registration-repository-security-pg.test.ts: a ban also reports the ticket type freed by the cancelled registration.
  • test/unit/affiliation.test.ts, test/unit/chat-routes.test.ts, test/unit/chat-attachments-servable.test.ts, test/integration/avatar-media-pg.test.ts, test/integration/host-page-media-pg.test.ts, the chat integration tests: fixtures and call signatures only (new required arguments, the served key instead of the raw key, the production operator gate); no expectation loosened.
    Three source-text tests were made whitespace-insensitive in Backend formatting pass with the repo's Prettier config #91, not here.

Verification

  • Node 22, pnpm 9.12.0, on the full stacked tree.
  • Root pnpm typecheck, pnpm lint, pnpm build, pnpm check:sql, prettier check: pass.
  • api unit suite by path: 340 files, 5836 passed, 3 skipped (the three pre-existing forward-template skips).
  • media-worker unit suite by path (arm64 host, pinned FFmpeg via FFPROBE_PATH/FFMPEG_PATH): 281 passed, 2 failed. Both failures are the known ffprobe-helper environment issue fixed in Backend test safety net: SQL transcripts, CSRF pairing, authz and characterization tests #92, which this stack does not contain.
  • email-worker: 16 passed, tsc clean.
  • New integration tests (about 20 *-pg files) are written but not run locally (no Docker by rule). CI runs only on PRs based on main, so it starts on this PR when Backend formatting pass with the repo's Prettier config #91 merges and GitHub retargets it to main; merge this one only after that run is green.
  • Four adversarial review passes (auth/sessions, media, events/orgs, comms/mail/logs) found 5 blockers and 8 majors; all are fixed here. A second review of the fix commits found 2 blockers and 1 major (a quoted CSV separator, email-code sign-in onto a planted account, a guest's uploads refused after sign-in); all three are fixed here too.

Staging checks

  • https://api.civfix.dev/readyz answers ready after the deploy (migrations 0181 and 0182 applied).
  • Provider sign-in onto an unverified-email account answers 409 (API level: Google/Apple sign-in with such an account).
  • A production 5xx body reads "Internal error" without internal detail; 4xx messages are unchanged.
  • Operators, before a release, may run this read-only query on staging and prod to see accounts the old hole could have planted: SELECT DISTINCT u.id, u.created_at FROM users u JOIN oauth_identities o ON o.user_id = u.id WHERE u.email IS NOT NULL AND u.email_verified = false AND u.deleted_at IS NULL.

Not covered

  • The concurrency fixes (claim locks, org invite lock, ban lock, erasure ordering) are proven by SQL-shape unit tests and integration tests in CI, not by a live race on staging.
  • Unverified-email accounts created before this change are not cleaned up; the audit query lists them, and the cleanup is a maintainer decision.
  • Guest uploads reused after sign-in are covered for reports, posts and finalize; chat and DM sends need a contract field to carry the guest id (handoff). The mobile app's guest uploads are only covered if it sends the guest cookie.
  • Other chat members see a photo-only message as an empty bubble until they reopen the chat (handoff for a pending attachment state).
  • Accounts with no email still skip the email-code confirmation before deletion (pre-existing design; deferred).
  • Media worker log lines still go to the console without pino redaction (the worker has no pino logger; deferred).
  • Client follow-ups are handoffs, not in this PR: a pending state for chat photos, a retry state on the web unsubscribe page, mobile's localized fallback for "Internal error", admin inbox attachment links, suggest-contact bounds in the contract.

🤖 Generated with Claude Code

…, off-boarded operators lose report-chat powers
… unknown ids; claim nudge refuses foreign origins; resource budget checks the host first
…waitlist, guests cannot self-register on private events, roster search escapes wildcards
…-only invite email, generic suspension copy, escaped page search
…itical notices cannot be cancelled, unsubscribe failures answer 503, audits are logged or atomic
…an the sender never get an unprocessed upload; event pages serve processed media
@byteful
byteful added this pull request to stack #122 September 24, 2026 17:53
@byteful byteful closed this Sep 24, 2026
@byteful
byteful deleted the fix/backend-campaign-04-security branch September 24, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants