diff --git a/backend/migrations/026_idempotency_excludes_deleted.down.sql b/backend/migrations/026_idempotency_excludes_deleted.down.sql new file mode 100644 index 0000000..892322d --- /dev/null +++ b/backend/migrations/026_idempotency_excludes_deleted.down.sql @@ -0,0 +1,13 @@ +-- Restore migration 022's wider index (which also covered soft-deleted rows). +-- +-- Note this can FAIL where the up-migration succeeded: once retries-after-delete have been +-- allowed, two rows may legitimately share a `client_upload_id` (one deleted, one live), and +-- the wider unique index cannot be rebuilt over them. That is inherent to reverting this +-- direction, not a defect in the down-migration. If it fails, the live-only index is still +-- correct and should simply be kept. + +DROP INDEX IF EXISTS upload_client_upload_id_key; + +CREATE UNIQUE INDEX upload_client_upload_id_key + ON upload (client_upload_id) + WHERE client_upload_id IS NOT NULL; diff --git a/backend/migrations/026_idempotency_excludes_deleted.up.sql b/backend/migrations/026_idempotency_excludes_deleted.up.sql new file mode 100644 index 0000000..a114b2e --- /dev/null +++ b/backend/migrations/026_idempotency_excludes_deleted.up.sql @@ -0,0 +1,35 @@ +-- Narrow the client-upload idempotency index so it stops covering soft-deleted rows. +-- +-- The bug (H9). Migration 022 created the index partial on `client_upload_id IS NOT NULL` +-- only, so a soft-deleted row kept occupying its key. But `find_by_client_upload_id` filters +-- `deleted_at IS NULL` — deliberately, and its doc comment says so: "if the guest deleted the +-- photo and their queue later retries, they should get a fresh upload rather than a +-- resurrection of a deleted one." The index and the lookup therefore disagreed, and the +-- disagreement is reachable by an ordinary guest: +-- +-- 1. guest uploads a photo, then deletes it (soft delete — the row stays, `deleted_at` set) +-- 2. their queue retries the same item (reconnect requeue, or they tap "Erneut") +-- 3. the whole body is re-streamed and re-validated, then `ON CONFLICT DO NOTHING` matches +-- the DEAD row and inserts nothing +-- 4. the replay lookup filters that row out and finds nothing, so the handler returns 409 +-- 5. the client classifies 409 as terminal and DELETES the blob from IndexedDB +-- +-- The photo is now gone from the device with no row in the gallery, and there is no path back. +-- Re-selecting the same file from the camera roll mints a new `client_upload_id`, so that does +-- work — but the guest has no way to know that is what is required. +-- +-- Adding `deleted_at IS NULL` makes the index agree with the lookup: a key is claimed only +-- while a LIVE row holds it, so step 3 inserts a fresh row and the retry succeeds. +-- +-- Uniqueness among live rows is what the feature actually needs. The property migration 022 +-- was protecting — "the same photo must not land in the gallery twice" — is about rows the +-- guest can see, and a soft-deleted row is not one of those. + +DROP INDEX IF EXISTS upload_client_upload_id_key; + +-- CONCURRENTLY is deliberately NOT used: sqlx runs each migration inside a transaction, and +-- CREATE INDEX CONCURRENTLY cannot run in one. The table is small (one event's uploads) and +-- this runs at boot before the server accepts requests, so the brief lock costs nothing. +CREATE UNIQUE INDEX upload_client_upload_id_key + ON upload (client_upload_id) + WHERE client_upload_id IS NOT NULL AND deleted_at IS NULL; diff --git a/backend/migrations/027_join_idempotency.down.sql b/backend/migrations/027_join_idempotency.down.sql new file mode 100644 index 0000000..3502c99 --- /dev/null +++ b/backend/migrations/027_join_idempotency.down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS user_client_join_id_key; +ALTER TABLE "user" DROP COLUMN IF EXISTS client_join_id; diff --git a/backend/migrations/027_join_idempotency.up.sql b/backend/migrations/027_join_idempotency.up.sql new file mode 100644 index 0000000..bbbf8c2 --- /dev/null +++ b/backend/migrations/027_join_idempotency.up.sql @@ -0,0 +1,34 @@ +-- Idempotency key for /join, supplied by the client. +-- +-- The failure this closes (H16) is the single most likely failure of the evening, on step one +-- of the product. `/join` commits the user row AND the bcrypt hash of the PIN, but the PLAINTEXT +-- PIN exists nowhere except the HTTP response body. So: +-- +-- 1. guest scans the QR in the venue car park, taps "Beitreten" +-- 2. the server creates the account and hashes the PIN +-- 3. the response is lost on the way back — the 5G-to-nothing transition every wedding venue +-- has, or the AP handing off +-- 4. the client retries; the name is now taken, so it 409s +-- 5. the client shows a PIN entry form for a PIN THAT WAS NEVER DISPLAYED +-- +-- The guest is locked out of their own brand-new account, and the only recovery is finding a +-- host with a dashboard open. `/upload` already solved exactly this with `client_upload_id`; +-- join never got the same treatment. +-- +-- With a key, a retry is recognised as the same join and answered with a usable PIN. We do NOT +-- store the plaintext to replay it — see the handler: a retry ROTATES the PIN. That is sound +-- precisely because the original was never shown to anybody, so there is nothing to preserve, +-- and it keeps this table free of recoverable credentials. + +ALTER TABLE "user" ADD COLUMN client_join_id UUID; + +-- Partial, for the same reasons as `upload_client_upload_id_key`: index only the rows that +-- carry a key, and state the rule exactly. NULL is allowed and unconstrained, so any client +-- that does not send one (and every row that predates this column) behaves exactly as before. +-- +-- Scoped per event as well as per key. The key is a client-generated v4 UUID so a cross-event +-- collision is not realistic, but a reused install genuinely has two events in one table and +-- "this join belongs to that event" is the property we actually mean. +CREATE UNIQUE INDEX user_client_join_id_key + ON "user" (event_id, client_join_id) + WHERE client_join_id IS NOT NULL; diff --git a/backend/migrations/028_feed_counts_exclude_banned.down.sql b/backend/migrations/028_feed_counts_exclude_banned.down.sql new file mode 100644 index 0000000..dcc4d4f --- /dev/null +++ b/backend/migrations/028_feed_counts_exclude_banned.down.sql @@ -0,0 +1,22 @@ +-- Restore migration 024's counts (which included banned users' likes and comments). +CREATE OR REPLACE VIEW v_feed AS +SELECT + u.id, + u.event_id, + u.user_id, + usr.display_name AS uploader_name, + usr.is_banned, + usr.uploads_hidden, + u.preview_path, + u.thumbnail_path, + u.display_path, + u.mime_type, + u.caption, + u.created_at, + (SELECT count(*) FROM "like" l WHERE l.upload_id = u.id) AS like_count, + (SELECT count(*) FROM comment c WHERE c.upload_id = u.id AND c.deleted_at IS NULL) AS comment_count +FROM upload u +JOIN "user" usr ON u.user_id = usr.id +WHERE u.deleted_at IS NULL + AND usr.uploads_hidden = FALSE + AND usr.is_banned = FALSE; diff --git a/backend/migrations/028_feed_counts_exclude_banned.up.sql b/backend/migrations/028_feed_counts_exclude_banned.up.sql new file mode 100644 index 0000000..15d2963 --- /dev/null +++ b/backend/migrations/028_feed_counts_exclude_banned.up.sql @@ -0,0 +1,44 @@ +-- Exclude banned users' likes and comments from the feed's scalar counts (H11). +-- +-- `v_feed` already excludes banned UPLOADERS (`usr.is_banned = FALSE` on the join), but the two +-- correlated subqueries added by migration 024 counted every like and every non-deleted comment +-- regardless of who wrote it. So after a ban: +-- +-- * the banned guest's own photos disappear from the feed (correct), but +-- * their likes still inflate the counter on everyone else's photos, and +-- * their comments still contribute to `comment_count` — and, until the change to +-- `Comment::list_for_upload` that ships with this migration, were still RENDERED in the +-- lightbox on the most-viewed photo of the evening. +-- +-- The host's mental model of "ban" is "this person's contributions are gone". Photos honoured it; +-- likes and comments did not. Migration 021 already applied exactly this reasoning to hashtag +-- counts, and the export query filters `is_banned` too — this brings the last read path in line. +-- +-- Derived at read time, so `unban_user` restores the counts with no extra work, exactly as it +-- already restores the photos. + +CREATE OR REPLACE VIEW v_feed AS +SELECT + u.id, + u.event_id, + u.user_id, + usr.display_name AS uploader_name, + usr.is_banned, + usr.uploads_hidden, + u.preview_path, + u.thumbnail_path, + u.display_path, + u.mime_type, + u.caption, + u.created_at, + (SELECT count(*) FROM "like" l + JOIN "user" lu ON lu.id = l.user_id + WHERE l.upload_id = u.id AND NOT lu.is_banned) AS like_count, + (SELECT count(*) FROM comment c + JOIN "user" cu ON cu.id = c.user_id + WHERE c.upload_id = u.id AND c.deleted_at IS NULL AND NOT cu.is_banned) AS comment_count +FROM upload u +JOIN "user" usr ON u.user_id = usr.id +WHERE u.deleted_at IS NULL + AND usr.uploads_hidden = FALSE + AND usr.is_banned = FALSE; diff --git a/backend/migrations/029_host_action_audit.down.sql b/backend/migrations/029_host_action_audit.down.sql new file mode 100644 index 0000000..b8b379b --- /dev/null +++ b/backend/migrations/029_host_action_audit.down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS host_action_audit_event_created_idx; +DROP TABLE IF EXISTS host_action_audit; diff --git a/backend/migrations/029_host_action_audit.up.sql b/backend/migrations/029_host_action_audit.up.sql new file mode 100644 index 0000000..0e5b843 --- /dev/null +++ b/backend/migrations/029_host_action_audit.up.sql @@ -0,0 +1,41 @@ +-- An audit trail for privileged actions (H17). +-- +-- What existed before: nothing. `grep -i audit` across `handlers/host.rs` and `handlers/admin.rs` +-- returned no hits. Individual actions logged a `tracing::info!` line, but config changes, gallery +-- release and event lock/unlock logged nothing at all — and the "audit trail" as a whole was a +-- 30 MB rotating Docker log that the runbook's own retention settings will discard. +-- +-- Why it matters here specifically: a host is a promoted GUEST, and `reset_pin` overwrites another +-- guest's credential and returns the new PIN in the clear. So a host can take over any guest's +-- account and post as them, and nothing in the record showed it happened (only /recover FAILURES +-- were logged). At a wedding the people involved know each other; the point is not catching a +-- villain, it is being able to answer "what happened to my photo?" the next morning without +-- guessing. +-- +-- Deliberately append-only in practice: no UPDATE or DELETE path is written for it anywhere. Small +-- (a few hundred rows for a real event), so no partitioning or retention job. + +CREATE TABLE host_action_audit ( + id BIGSERIAL PRIMARY KEY, + event_id UUID NOT NULL REFERENCES event(id) ON DELETE CASCADE, + -- The privileged caller. NOT a FK with ON DELETE CASCADE: the record must survive the actor's + -- account being removed, which is exactly when it is most likely to be wanted. + actor_id UUID, + actor_name TEXT, + actor_role TEXT NOT NULL, + -- Short stable slug: 'ban_user', 'unban_user', 'reset_pin', 'delete_upload', + -- 'delete_comment', 'release_gallery', 'lock_uploads', 'unlock_uploads', 'patch_config', + -- 'promote_user', 'demote_user', 'delete_user'. + action TEXT NOT NULL, + -- The guest or object acted upon, when there is one. + target_id UUID, + target_name TEXT, + -- Free-form context: the config key and its old/new value, the caption that was removed, etc. + -- Never credentials — a reset PIN must not be recoverable from this table. + detail JSONB, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +-- The only query shape this needs: "what happened at this event, newest first". +CREATE INDEX host_action_audit_event_created_idx + ON host_action_audit (event_id, created_at DESC); diff --git a/backend/src/db.rs b/backend/src/db.rs index 3031c5b..dab6466 100644 --- a/backend/src/db.rs +++ b/backend/src/db.rs @@ -2,7 +2,11 @@ use anyhow::{Context, Result}; use sqlx::PgPool; use sqlx::postgres::PgPoolOptions; -const DEFAULT_MAX_CONNECTIONS: u32 = 10; +/// Keep in step with `.env.example` and the `db` sizing comment in `docker-compose.yml`. +/// These three drifted apart once (code 10 / `.env.example` 15 / runbook 30) and the runbook +/// presented its number as authoritative, so the contradiction was invisible at deploy time. +/// 15 is sized to 2 vCPU and the 1G `db` memory limit — raise it only alongside both. +const DEFAULT_MAX_CONNECTIONS: u32 = 15; /// SQLSTATE for `invalid_password`. const PG_INVALID_PASSWORD: &str = "28P01"; @@ -47,10 +51,23 @@ fn explain_auth_failure(err: &sqlx::Error) { } pub async fn create_pool(database_url: &str) -> Result { - let max_connections = std::env::var("DATABASE_MAX_CONNECTIONS") - .ok() - .and_then(|s| s.parse::().ok()) - .unwrap_or(DEFAULT_MAX_CONNECTIONS); + // A malformed value must not silently become the default: an operator who typed + // `DATABASE_MAX_CONNECTIONS=3O` (letter O) would otherwise get 15 with no indication, + // and would keep tuning a knob that never took effect. + let max_connections = match std::env::var("DATABASE_MAX_CONNECTIONS") { + Err(_) => DEFAULT_MAX_CONNECTIONS, + Ok(raw) => match raw.trim().parse::() { + Ok(0) => { + anyhow::bail!("DATABASE_MAX_CONNECTIONS must be at least 1 (got 0)"); + } + Ok(n) => n, + Err(e) => { + anyhow::bail!( + "DATABASE_MAX_CONNECTIONS must be a positive integer (got {raw:?}): {e}" + ); + } + }, + }; let pool = match PgPoolOptions::new() .max_connections(max_connections) diff --git a/backend/src/models/comment.rs b/backend/src/models/comment.rs index 7dd9282..3326878 100644 --- a/backend/src/models/comment.rs +++ b/backend/src/models/comment.rs @@ -62,13 +62,27 @@ impl Comment { ) -> Result, sqlx::Error> { // Two-step: pick the newest `limit` rows older than `before`, then flip // them back into ascending order so the caller can render top-to-bottom. + // `AND NOT u.is_banned` — the filter that was missing (H11). + // + // Only `deleted_at` was checked, so a banned guest's comments stayed on the live feed + // forever: the host bans somebody for an abusive comment, watches every photo of theirs + // vanish, and the comment is still sitting there on the most-viewed photo of the evening. + // Nothing on the client evicted them either. + // + // The tell that this was an oversight rather than a decision: the EXPORT query already + // filters `is_banned`, so the comment disappeared from the keepsake but not from the app — + // the two views of the same moderation action disagreed. Migration 021 did the same for + // hashtag counts. This brings the live read path in line with both. + // + // A ban is reversible and this is derived at read time, so `unban_user` restores the + // comments with no extra work. sqlx::query_as::<_, CommentDto>( "SELECT * FROM ( SELECT c.id, c.upload_id, c.user_id, u.display_name AS uploader_name, c.body, c.created_at FROM comment c JOIN \"user\" u ON u.id = c.user_id - WHERE c.upload_id = $1 AND c.deleted_at IS NULL + WHERE c.upload_id = $1 AND c.deleted_at IS NULL AND NOT u.is_banned AND ($2::timestamptz IS NULL OR c.created_at < $2) ORDER BY c.created_at DESC LIMIT $3 diff --git a/backend/src/models/upload.rs b/backend/src/models/upload.rs index a1fb058..bdd22ae 100644 --- a/backend/src/models/upload.rs +++ b/backend/src/models/upload.rs @@ -77,10 +77,19 @@ impl Upload { // // The conflict target repeats the index's `WHERE` clause because it is a partial index; // without it Postgres cannot prove which index to use and rejects the statement. + // + // KEEP THIS IN LOCKSTEP WITH `upload_client_upload_id_key` (migration 026). The predicate + // here must match the index's, or the arbiter cannot be inferred and every upload that + // carries a `client_upload_id` fails as a runtime 500 — queries in this codebase are not + // compile-time checked, so nothing catches a drift between the two at build time. + // + // `AND deleted_at IS NULL` is what makes a retry-after-delete work instead of 409ing + // forever: the key is claimed only while a LIVE row holds it, which is what + // `find_by_client_upload_id` below has always assumed. sqlx::query_as::<_, Self>( "INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes, caption, client_upload_id) VALUES ($1, $2, $3, $4, $5, $6, $7) - ON CONFLICT (client_upload_id) WHERE client_upload_id IS NOT NULL DO NOTHING + ON CONFLICT (client_upload_id) WHERE client_upload_id IS NOT NULL AND deleted_at IS NULL DO NOTHING RETURNING *", ) .bind(event_id) @@ -229,6 +238,18 @@ impl Upload { .await } + /// Read the lifetime derivative-attempt counter WITHOUT charging it. + /// + /// Used by the in-request retries after the first: those re-enter `do_process` but must not + /// spend the lifetime budget again (see `charge_lifetime_attempt`). `None` still means the + /// row vanished, so the caller's "nothing to do" branch keeps working unchanged. + pub async fn derivative_attempts(pool: &PgPool, id: Uuid) -> Result, sqlx::Error> { + sqlx::query_scalar("SELECT derivative_attempts FROM upload WHERE id = $1") + .bind(id) + .fetch_optional(pool) + .await + } + /// Store why the last derivative attempt failed. Diagnostics only — nothing branches on it. pub async fn record_derivative_failure( pool: &PgPool, diff --git a/backend/src/models/user.rs b/backend/src/models/user.rs index cad4fb5..d0da175 100644 --- a/backend/src/models/user.rs +++ b/backend/src/models/user.rs @@ -47,19 +47,41 @@ impl User { event_id: Uuid, display_name: &str, pin_hash: &str, + client_join_id: Option, ) -> Result { sqlx::query_as::<_, Self>( - "INSERT INTO \"user\" (event_id, display_name, recovery_pin_hash) - VALUES ($1, $2, $3) + "INSERT INTO \"user\" (event_id, display_name, recovery_pin_hash, client_join_id) + VALUES ($1, $2, $3, $4) RETURNING *", ) .bind(event_id) .bind(display_name) .bind(pin_hash) + .bind(client_join_id) .fetch_one(pool) .await } + /// Look up a join that already succeeded, by the idempotency key its client sent. + /// + /// The retry path for H16: the account was created but the response never arrived, so the + /// client re-sends the same `client_join_id`. Finding a row here means "this join already + /// happened" — the caller rotates the PIN and answers with a usable one rather than 409ing + /// on a name the caller itself owns. + pub async fn find_by_client_join_id( + pool: &PgPool, + event_id: Uuid, + client_join_id: Uuid, + ) -> Result, sqlx::Error> { + sqlx::query_as::<_, Self>( + "SELECT * FROM \"user\" WHERE event_id = $1 AND client_join_id = $2", + ) + .bind(event_id) + .bind(client_join_id) + .fetch_optional(pool) + .await + } + /// Create a user with an explicit role, in ONE statement. /// /// `create` + a separate `UPDATE ... SET role` is not equivalent: a crash or a pool error diff --git a/backend/src/services/audit.rs b/backend/src/services/audit.rs new file mode 100644 index 0000000..eff277c --- /dev/null +++ b/backend/src/services/audit.rs @@ -0,0 +1,62 @@ +//! Append-only record of privileged actions. See migration 029 for why it exists. +//! +//! Design constraints, both learned from the rest of this codebase: +//! +//! * **Never fail the action.** An audit write that can turn a successful ban into a 500 makes +//! moderation less reliable than no audit at all. Every failure here is logged and swallowed. +//! * **Never store a credential.** `reset_pin` is the action most worth recording and the one +//! whose payload must never be in `detail` — a table that could hand back a guest's PIN would +//! be a worse privacy problem than the gap it closes. + +use serde_json::Value; +use sqlx::PgPool; +use uuid::Uuid; + +use crate::models::user::UserRole; + +/// Record one privileged action. +/// +/// Takes `&PgPool` rather than a transaction on purpose: the audit row is not part of the action's +/// atomicity. If the action commits and the audit write fails we want the action to stand (and a +/// loud log line); if the action rolls back, an orphan audit row saying "someone tried" is more +/// useful than silence. +#[allow(clippy::too_many_arguments)] +pub async fn record( + pool: &PgPool, + event_id: Uuid, + actor_id: Uuid, + actor_name: Option<&str>, + actor_role: UserRole, + action: &str, + target_id: Option, + target_name: Option<&str>, + detail: Option, +) { + let result = sqlx::query( + "INSERT INTO host_action_audit + (event_id, actor_id, actor_name, actor_role, action, target_id, target_name, detail) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8)", + ) + .bind(event_id) + .bind(actor_id) + .bind(actor_name) + .bind(format!("{actor_role:?}").to_lowercase()) + .bind(action) + .bind(target_id) + .bind(target_name) + .bind(detail) + .execute(pool) + .await; + + match result { + Ok(_) => {} + Err(e) => { + // `error`, not `warn`: losing an audit row is the kind of thing that should show up in + // whatever is watching the logs, even though it must not fail the request. + tracing::error!( + error = ?e, action, %actor_id, ?target_id, + "failed to write host action audit row" + ); + } + } +} diff --git a/backend/src/services/mod.rs b/backend/src/services/mod.rs index 3a99259..0cea944 100644 --- a/backend/src/services/mod.rs +++ b/backend/src/services/mod.rs @@ -1,3 +1,4 @@ +pub mod audit; pub mod compression; pub mod config; pub mod disk; diff --git a/backend/tests/upload_idempotency.rs b/backend/tests/upload_idempotency.rs index 0c55d84..6f5ff52 100644 --- a/backend/tests/upload_idempotency.rs +++ b/backend/tests/upload_idempotency.rs @@ -26,7 +26,7 @@ async fn create_upload( let row: Option<(Uuid,)> = sqlx::query_as( "INSERT INTO upload (event_id, user_id, original_path, mime_type, original_size_bytes, caption, client_upload_id) VALUES ($1, $2, $3, $4, $5, $6, $7) - ON CONFLICT (client_upload_id) WHERE client_upload_id IS NOT NULL DO NOTHING + ON CONFLICT (client_upload_id) WHERE client_upload_id IS NOT NULL AND deleted_at IS NULL DO NOTHING RETURNING id", ) .bind(event_id) @@ -172,4 +172,29 @@ async fn a_deleted_upload_is_not_replayed(pool: PgPool) { None, "a soft-deleted upload must not be replayed" ); + + // The other half of that rule, and the half that was missing (H9). Asserting only that the + // lookup returns None left the index free to disagree with it: migration 022's predicate + // covered soft-deleted rows, so the retry's INSERT hit `ON CONFLICT DO NOTHING` against the + // dead row, the replay lookup above then found nothing, and the handler answered 409 — which + // the client classifies terminal and purges the blob for. The photo was gone from the phone + // AND absent from the gallery, with no way back. + // + // Migration 026 narrowed the index to live rows so a retry after a delete inserts a FRESH + // upload, which is what `find_by_client_upload_id`'s own doc comment always claimed happened. + let retried = create_upload(&pool, event_id, user_id, "originals/a.jpg", Some(key)).await; + assert!( + retried.is_some(), + "a retry after the guest deleted the photo must create a fresh upload, not 409 forever" + ); + assert_ne!( + retried, + Some(id), + "the retry must be a new row, not the dead one" + ); + assert_eq!( + find_by_key(&pool, user_id, key).await, + retried, + "the live row is the one the replay lookup must now find" + ); }