Skip to content

Correctness fixes (backend campaign 5) - #94

Closed
theobong wants to merge 51 commits into
fix/backend-campaign-04-securityfrom
fix/backend-campaign-05-correctness
Closed

theobong wants to merge 51 commits into
fix/backend-campaign-04-securityfrom
fix/backend-campaign-05-correctness

Conversation

@theobong

@theobong theobong commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

What changed

Correctness fixes across the API and the media worker, found by the campaign audit and hardened by an adversarial review pass. Silent failures now log or retry instead of disappearing; lists page without skipping or repeating rows; host registration, waitlists, exports and broadcasts keep their counts straight under concurrency; admin mail stops routing to bounced city addresses; the media worker retries its own infrastructure faults instead of rejecting people's uploads. 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) with a report you filed, some volunteer hours and a post you can repost; a host of a capped event with ticket types, registrations and more than one page of attendees; operator (admin.civfix.dev through Cloudflare Access)
  • Stacked on Security fixes (backend campaign 4) #93 (security fixes), which is stacked on Backend formatting pass with the repo's Prettier config #91. Read only this PR's diff against Security fixes (backend campaign 4) #93's branch.
  • Size: 20711 counted lines. Campaign PR; the author approved PRs over the 400-line cap for this cleanup. About two thirds of it is tests.
  • Migrations: 0184 (nullable media_assets.upload_etag, catalog-only) and 0185 (new small table inbound_bounce_attempts).
  • Boot check before a production release: APNS_PRODUCTION is now parsed strictly and required in production once all APNs credentials are set; accepted values are true/t/yes/y/on/1 and false/f/no/n/off/0. Malformed integer settings (for example 15s) now fail boot instead of silently using a default. Staging proves only staging's values.

Verify

[Web]

  1. Open "Your reports", open a report you filed, click "Mark as resolved", then "Reopen report" — Expect: the badge reads "Resolved", then returns to its earlier state; the timeline gains one entry per action; clicking "Mark as resolved" twice in a row does not add a second entry.
  2. As host, open your event, "Host dashboard", "Edit event", change the title and the linked reports, click "Save changes" — Expect: both changes are saved together; if the save is refused, the title is not changed either.
  3. In the host console for an event with more than one page of attendees, click "Attendees", sort by the "Checked in" column, click "Load more" until the list ends — Expect: every attendee appears exactly once across the pages.
  4. Open an attendee, choose another ticket type under "Move to another ticket type", click "Move" — Expect: "Registration moved"; if the old ticket type had a waitlist, the next person is offered the freed place.
  5. Open "Messages", "New message", save it, set "Send at", click "Schedule" — Expect: "Message scheduled". Then "Attendees", "Export", "Attendees", and "Download" when ready — Expect: the file downloads with the usual name.
  6. On your profile open the "Hours" tab, click "Load more" under the service record until it ends — Expect: no entry repeats or goes missing. Then "Prepare transcript", "Open PDF" — Expect: the PDF's verify line and QR code point to civfix.dev (not civfix.org) on staging.
  7. Repost a post from the feed, then choose "Undo repost" — Expect: the repost disappears and the original's repost count updates.
  8. Open Settings, "Account", "Request my data" — Expect: the usual confirmation; the email arrives with your data.

[Mobile]

  1. From your profile, "Reports", open a report you filed, tap "Mark as resolved", then "Reopen report" — Expect: same as Web step 1.
  2. Open an event you host, "Host dashboard", "Edit event", change the title, tap "Save changes" — Expect: saved.
  3. Repost a post and tap "Undo repost" — Expect: the repost disappears.

[Admin]

  1. Open "Users", click "Load more" several times; do the same in "Reports" and in "Moderation" — Expect: no row repeats and none is skipped between pages.
  2. In "Moderation", open a "Reported user" item and click "Remove", confirm — Expect: the item resolves; if the user was already banned, the account stays banned (not downgraded to a suspension).
  3. Open a report whose city contact has bounced and use "Send to city" — Expect: the packet is not sent to the bounced address (the report shows as waiting for routing until a usable contact exists).
  4. In "Mail", open an outbound thread whose last message is still sending and click "Reply" — Expect: the reply is refused until the send finishes or times out; "Resend" on a bounced thread still works.

Regression

Filing reports with photos — [Web] [Mobile]

  1. File a report with two photos — Expect: it publishes and the photos appear after processing, as before.

Chat — [Web] [Mobile]

  1. Send messages in an event chat and a DM; mention someone who joined recently; edit a message — Expect: delivered, the mention notifies, "(edited)" shows.
  2. In a group chat, remove a member — Expect: they are removed and cannot rejoin while banned.

Feed and notifications — [Web] [Mobile]

  1. Scroll the feed and a post's replies to the end — Expect: no repeated or missing posts.
  2. Open "Notifications" and "Mark all read" — Expect: as before.

Events and RSVPs — [Web]

  1. Signed out, on a public event with free seats, "No account? RSVP as a guest", "Continue as guest", "Send code", enter the code, "Confirm RSVP" — Expect: "You're on the list".
  2. On a sold-out event, try the same — Expect: the RSVP is refused (the sheet's wording is updated separately in the app; the server now names the reason).

Host messaging — [Web]

  1. "Messages", "New message", "Send now" and confirm — Expect: "Sending…", then delivered as before.

Admin mail — [Admin]

  1. "Mail", open a thread, "Reply" — Expect: "Reply sent to …" as before.

Findings addressed

Grouped by package; IDs are campaign ledger IDs (.campaign/backend/ledger.json, not in this repo). About 150 items were fixed, some partly; the rest are refuted, already fixed, or listed under decisions.

  • Adapters, env, auth, platform: BE-BUG-001 to BE-BUG-004, BE-BUG-007 to BE-BUG-011, BE-BUG-015, BE-BUG-016, BE-BUG-018 to BE-BUG-021, BE-SEC-011 (adapters log through the redacting logger).
  • Database scripts and cursors: BE-BUG-023, BE-BUG-024, BE-BUG-026 to BE-BUG-032, BE-BUG-117, BE-BUG-118 (one microsecond keyset cursor for every list; forged cursors start from the first page).
  • Reports and media: BE-BUG-144, BE-BUG-145, BE-BUG-147 to BE-BUG-154, BE-BUG-171 (docs).
  • Events, hours, posts, notifications: BE-BUG-033 to BE-BUG-038, BE-BUG-040, BE-BUG-041, BE-BUG-043 to BE-BUG-050, BE-BUG-053.
  • Chat: BE-BUG-156, BE-BUG-157, BE-BUG-159 to BE-BUG-161, BE-BUG-163, BE-BUG-165 to BE-BUG-168.
  • Host registration and pages: BE-BUG-057 to BE-BUG-060, BE-BUG-062 to BE-BUG-064, BE-BUG-067, BE-BUG-071 to BE-BUG-074, BE-BUG-077 to BE-BUG-083.
  • Host messaging, metrics, exports: BE-BUG-084 to BE-BUG-092, BE-BUG-095 to BE-BUG-101.
  • Admin mail and discovery: BE-BUG-102 to BE-BUG-105, BE-BUG-109 to BE-BUG-116.
  • Admin plane: BE-BUG-117 to BE-BUG-128, BE-BUG-131 to BE-BUG-134, BE-BUG-136 to BE-BUG-143, BE-BUG-170.
  • Found later (performance planning, 2026-09-23): the stricter link cleanup in the inbound HTML sanitizer (BE-BUG-102 area) used a regex that was quadratic on a long run of spaces inside an href: one crafted email could block the API for seconds (6 s at 64 KiB). The last commit makes it linear, with the same output on 1,000,000 fuzzed inputs, and adds a timing test.
  • Swallowed-error sweep and test hygiene: failures that were silently caught now log or propagate; the flaky ranked-feed and event-duplicate tests are fixed at the root.

Decisions for the reviewer

  • Boot strictness. Malformed integer settings fail boot; APNS_PRODUCTION is strict (list above) and defaults to the production gateway; SHUTDOWN_DRAIN_MS keeps its lenient reading on purpose (a boot failure over shutdown timing would be worse).
  • Container shutdown. Jobs still draining at shutdown keep database access; after they stop, no getter can open a new pool.
  • Cursors. Every list pages on the column's exact microsecond instant; old cursors still work (an old millisecond cursor may repeat or skip one row once). A cursor carrying an impossible date or offset starts from the first page instead of failing. Message threads keep their deliberate millisecond cursor (four sources merge on it).
  • Media worker. Failures of its own sandbox setup or tools now retry as infrastructure faults instead of rejecting the upload; content failures still reject. It refuses to start if the image timeout would outrun the job budget. Stuck uploads requeue with their upload etag (0184).
  • Reports. An owner's repeat "resolve" or a "reopen" of a report that is not resolved changes nothing (was a silent regression to published). Replays of a report submission return the live report with fresh photo links. The home-turf form no longer answers "too many submissions" for a second sign-up with the same email; it simply sends no second confirmation.
  • Events. An event edit (fields, linked reports, slots) saves all or nothing under the event lock. joined now means the caller RSVP'd: an org owner or admin who never joined sees Join, and keeps host powers. A guest RSVP the registration refuses now answers an error with a reason code, and a refusal that is certain from public event state (access code needed, registration or sales closed) is answered before a code is sent.
  • Host plane. Registrations with the same idempotency key serialize, so a double-tap on the last seat replays instead of answering "full". A full or foreign signup slot refuses the whole registration. Waitlists wake on transfers and raised caps and skip ended or cancelled events. Portfolio totals cover every hosted event. Refused sends give back their rate-limit charges. Each export run writes its own object, the reaper fences a stale run before deleting, and a superseded run can no longer fail the live one. A suspension refuses deleted users. Audit action types are closed.
  • Admin mail. A send with no outcome younger than 15 minutes blocks reply, resend and re-route. Bounced contacts (per-category and legacy) stop receiving report packets, city forwards and event mail, and the routable count follows one shared rule. A bounce is recorded before discovery is queued; a bounce whose bookkeeping keeps failing is parked after 6 tries (0185). City packet dates use Los Angeles local time.
  • Admin plane. Keyset cursors keep microseconds; operator message removals update open chats; flag, role and status writes lock their rows without blocking foreign-key checks; an owner takedown skips the author's strike only when the owner alone opened the item; a removal never downgrades a ban; data exports back off across mail outages and are always on record as undeliverable when they cannot be sent.
  • Chat. The report bell cap is 500 on every path (was 2000 on three of four); the fan-out job fails and retries only when every bell failed (bells coalesce, so no double bells); lookups and failures log one summary per fan-out.
  • Unrepost of a post that became unreadable removes the repost and answers a withdrawn card, never the post's content.
  • Open product questions are listed in the campaign's decision log (for example: the order of the 500-member report bell cap, whether operators may cancel an ended event, whether a checked-in-then-cancelled seat counts as attended).

User-visible copy changes

  • Guest RSVP refusals (were a silent success): "This event has no seats left.", "Registration for this event is closed.", "Ticket sales for this event are closed.", and at the code request "Check your details, then try again." (verify step: "Check your details, then request a new code to try again."); each carries a reason code for the app.
  • Registering with a signup slot: 404 "That slot no longer exists." / 409 "That slot is already full." (existing strings; was a silent success without the slot).
  • Saving event questions with another event's ticket type: 422 "not a ticket type on this event" (was a 500).
  • Test sends over their limit: "You have sent several test messages. Try again in an hour." (was "This event has reached its daily message limit.").
  • Downloading an expired export: "That export has expired." (new).
  • Certificate PDF: "Verify this record at <this deployment's host>/service-record" (was always civfix.org; production text unchanged).
  • City reply notes on the event timeline: the quote-stripped, clipped reply instead of the whole raw email.
  • Data export too large for email: a short notice email "Your civfix data export" (new).
  • Admin activity feed: labels for operator sign-in denied, account deleted, data export undeliverable, takedown requested, report chat message removed, announcement sent (were raw action names).
  • Page-view beacon: 200 { "ok": true } (was 204, matching the registry).
  • Unrepost of an unreadable post: 200 with a withdrawn card (was 404 after removing it).
  • Operator boot errors: "APNS_PRODUCTION: must be one of true/t/yes/y/on/1 or false/f/no/n/off/0", ": must be an integer", ": must be a positive integer", and the media worker's image-timeout error.

Tests changed

Existing tests whose expectation changed because the fix changes the behavior they pinned (no test was skipped, deleted or loosened; each is listed in the campaign reports with its reason):

  • test/unit/feed-ranked-service.test.ts, test/unit/host/cleanup-duplicate.test.ts: flaky tests fixed at the root (unseeded random order; two clock reads).
  • test/unit/home-turf-routes.test.ts: a same-address resubmit answers 200 without a second confirmation (was 429 after the staff email went out).
  • test/unit/guest-rsvp-service.test.ts: a sold-out verify is refused (it pinned the silent success).
  • test/unit/storage-r2-proxy.test.ts, test/unit/adapters-push-classification.test.ts: every storage client now carries a timeout handler; invalid-argument pruning is decided per batch.
  • test/unit/host-export-csv.test.ts, test/unit/host-export-lifecycle.test.ts, test/unit/host-reminders.test.ts, test/unit/host/announcement-pipeline.test.ts, test/unit/host-broadcast-send-correctness.test.ts: export keys carry the run token; a cancellation retry re-plans; cursors carry the instant as text; the budget refund is a decrement.
  • test/unit/admin-moderation-correctness.test.ts, test/unit/admin-user-correctness.test.ts, test/unit/admin-event-correctness.test.ts, test/unit/admin-report-correctness.test.ts: owner takedown folding; FOR NO KEY UPDATE locks.
  • test/unit/chat-room-fanout-failures.test.ts (rewritten against the real notification service), test/unit/chat-room-notifier-wiring.test.ts, test/unit/post-unrepost-unreadable.test.ts, test/unit/ws-socket-lifecycle.test.ts, test/unit/admin-mail.test.ts, services/media-worker/test/unit/maintenance.test.ts, services/media-worker/test/unit/pg-boss-jobs.test.ts.
  • Several test harnesses gained the new methods their fakes must provide; no expectation changed there.
  • New test/unit/inbound-html-sanitizer-linear-time.test.ts: a long space run inside an href is cleaned in under 500 ms (6 s before the last commit), and edge trimming still applies.

Verification

  • Node 22, pnpm 9.12.0, on the full stacked tree (this PR over Security fixes (backend campaign 4) #93 over Backend formatting pass with the repo's Prettier config #91).
  • Root pnpm typecheck, pnpm lint, pnpm build, pnpm check:sql, prettier check: pass.
  • api unit suite by path: 427 files, 6351 passed, 3 skipped (the pre-existing forward-template skips).
  • media-worker unit suite by path (arm64 host, pinned FFmpeg via FFPROBE_PATH/FFMPEG_PATH): 300 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.
  • 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 here when the PRs below this one merge and GitHub retargets this one to main; merge it only after that run is green.
  • Five adversarial review passes (platform, media and cursors, events and chat, host plane, admin plane) found 0 blockers and 10 majors; all majors and most minors are fixed here, the rest are listed in the decision log.

Staging checks

  • https://api.civfix.dev/readyz answers ready after the deploy (migrations 0184 and 0185 applied).
  • GET https://api.civfix.dev/v1/reports/search?cursor=2026-02-30T00:00:00Z|00000000-0000-4000-8000-000000000001 answers 200 with the first page (not 500).
  • The API and the media worker start with staging's configuration (this proves the strict env parsing against staging's values; production's must be confirmed separately before a release).
  • A host broadcast is sent and delivered; a data export email arrives.

Not covered

  • Concurrency fixes (registration twins, slot and ban locks, export fencing, notification locks) are proven by SQL-shape unit tests and CI integration tests, not by a live race on staging.
  • Production's APNS_PRODUCTION and integer settings are not readable here; confirm them before a release.
  • The guest RSVP sheet still shows its older wording for the new refusal reasons until the app update lands (handoff).
  • Hosts cannot offer a waitlist place by hand in the app today; automatic promotion is what this PR changes.
  • The admin app has no audit log or activity screen, so the new activity labels are visible only through the API.
  • The Home Turf form on civfix.dev posts to the production API (an app issue, handed off), so don't use it to test staging.

🤖 Generated with Claude Code

… rollups page every event, exports never orphan objects
…s, owner toggles are idempotent, replays re-sign media, silent failures are logged
…v integers, adapter logs through pino, closed container fails loudly
… csrf cookie lifetime, oauth cancel redirects
…nding, certificate year and verify link follow the deployment, leaderboard stops at its ceiling
…ws serialize, replies to deleted posts 404, reposts refresh counts, feed test no longer flakes
… mentioned, kicks and joins are atomic, edits re-record mentions, failures are logged
… and raised caps and skip ended events, portfolio totals span every event, invite caps hold under concurrency
…room, flag and role writes lock their rows, owner takedowns keep their meaning, exports report what they could not deliver
…n session, strict layer and batch args, demo seeder matches the live write paths
…h window, bounced contacts stop routing, bounces redrive, sanitizer keeps spaces and decodes entities once
…ension refuses deleted users; audit actions are typed
@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-05-correctness 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