05063694d2
Two adversarial reviews over61119be,1d9fb11andeb0e405. The merge itself came back clean — client_upload_id end to end, TempFileGuard's arm/retarget/disarm, the supervised sweep wiring and v_feed's column parity were all verified sound. What follows is what my own three commits broke. BLOCKER — a post-release rebuild was permanently impossible, and it 404'd the keepsake1d9fb11deferred prune_superseded_archives to run only on success, so a failed rebuild could no longer destroy the last good archive. It did not follow that through: ensure_export_space runs BEFORE the prune, so at rebuild time the previous generation is still on disk and counted against free. That halves the gallery a rebuild can survive (~4.6 GB) relative to what the upload gate accepts (~7.8 GB) — and it self-locks, because invalidate_and_arm bumps the epoch on COMMIT, which 404s both download routes immediately, while the only code that could free the space now runs only after a success that can never happen. A guest deleting their own photo is enough to trigger it. Recovery needed `docker exec rm`. Now two-phase: try to build while preserving the old generation; if that genuinely does not fit, reclaim it and try once more. Strictly better than both the original ordering and my change — the old archive is sacrificed only when it is the only way to get a new one. BLOCKER — the deferred prune could delete the last archive when a worker LOST the race run_*_export_inner returned Ok(()) on the superseded/discard path, so `res.is_ok()` fired the prune with the worker's own RETIRED epoch as keep_seq. At that moment the winning generation is still `pending` with no file, so protected_files is empty and the last good archive was deleted with no replacement. Exactly the invariant deferring the prune was meant to establish. Returns Err(Superseded) now, which abandon_if_superseded already swallows for the caller. BLOCKER — the low-disk banner could never fire before the wall eb0e405's gate refuses at `free < keepsake + DISK_RESERVE`, while disk_is_low warned at `free < keepsake`. The two differ by the whole reserve, so the wall always came first: every guest blocked from uploading while the host dashboard showed ~27 GB free and no banner, with nobody on site. disk_is_low now shares the gate's expression plus a 25% margin, and a test asserts the banner fires at the gate threshold across the whole gallery-size range. BLOCKER — I raised the unauthenticated bcrypt ceiling 24x on a 2 vCPU box1d9fb11moved admin_login's tight bucket after verify_password (correct — that is what stops a guest locking the operator out) but replaced the incidental 5/min bound on bcrypt with 120/min and nothing global. bcrypt is on spawn_blocking, but tokio's blocking pool is 512 threads, so "off the runtime" is not "bounded": enough concurrent verifies preempt both async workers and uploads, feed and SSE stall. Three unauthenticated endpoints reach bcrypt and every guest shares one NAT IP, so per-IP limits bound nothing globally. Adds a process-wide semaphore of `cores - 1` around both verify and hash, and drops the ceiling to 30. Also correcting my own claim: "a correct password is never throttled" was wrong. The failure bucket cannot block it, but the CPU ceiling still can. The code comment said so; the commit message did not. BLOCKER — migration 025 could crash-loop the app on boot Its UPDATE derives `Name (8hex)` with no guard against idx_user_event_name_ci. A guest who had already joined as exactly that string makes the migration fail, which propagates out of create_pool, exits main, and `restart: unless-stopped` turns it into a permanent loop — a worse version of the lockout the migration exists to clean up. Now skips colliding rows (create_admin_user already falls back to Admin-<8hex>, so the cleanup is convenience, not load-bearing). Also `role = 'guest'` rather than `<> 'admin'`, which was renaming legitimately promoted hosts named "Host". DEGRADATION — the watchdog's suspension credit was unbounded Background tabs are throttled to ~1 tick/min WITHOUT the network stack pausing, and the tick gap cannot tell that from a freeze. Crediting every late tick grew the observed silence by only one interval per real minute, so a dead socket took ~18 minutes to detect while holding the queue's processing latch. Credit is now capped at one stall window and REFILLS on real progress: an upload that is moving survives any number of screen locks, while one that is silent and suspended is detected within ~3 minutes. DEGRADATION — the 4xx log line was an unauthenticated log-injection vector validate_display_name allowed newlines, several 4xx messages interpolate the name, and %message wrote it unescaped. Two unauthenticated /join requests could forge arbitrary lines in the only forensic record an unattended event has. Fixed at both ends: control characters rejected at the door, and `detail = ?message` escapes on the way out (which also stops colliding with tracing's reserved `message` field). 401/404 drop to DEBUG — they carry no operator signal and were the cheapest lines for a scanner to use to roll the 30 MB log window in minutes. DEGRADATION — the quota floor was inverted exactly where it mattered `computed.max(MIN.min(budget))`: `budget` is the whole disk's share, so below 500 MiB the "floor" became the entire remaining budget and EVERY uploader was authorised all of it — 400 MB free, 3 uploaders, 300 MB each. A test pinned that as correct under the name `the_floor_never_exceeds_what_the_disk_can_back`. Both fixed. Also replaces the headline gate test, which asserted its own precondition inside an `if` on that precondition and could not fail. It now pins what actually binds the gate to the preflight — that required_free_bytes charges for both halves — plus the ceiling band. Verified: 151/151 backend tests against a live Postgres, clippy clean, 58/58 vitest, svelte-check 0 errors, eslint clean, both builds, caddy validate, and the migration collision reproduced against Postgres 16 before and after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
71 lines
4.2 KiB
SQL
71 lines
4.2 KiB
SQL
-- Two independent auth defects that share a migration because they share a table.
|
|
|
|
-- 1. RESERVED NAMES — free any guest squatting on a name the admin path used to depend on.
|
|
--
|
|
-- Migration 007 made display_name unique per event case-insensitively, and `join` had no
|
|
-- reserved-name guard. So any guest could join as "admin"/"Admin"/"ADMIN" before the operator's
|
|
-- first admin login; admin_login then looked its user up BY NAME, missed (wrong role), fell
|
|
-- through to creating "Admin", violated that unique index, and returned a 500 — permanently,
|
|
-- with no in-app recovery. Moderation, config and gallery release all gone, fixed only by SQL.
|
|
--
|
|
-- The real fix is in code (look the admin up by role, never by name — see auth/handlers.rs).
|
|
-- This clears the state an already-deployed database may be carrying.
|
|
--
|
|
-- RENAMED, NEVER DELETED: the guest keeps their uploads, their PIN and their session. Only
|
|
-- non-admin rows are touched — a real admin row named "Admin" is the expected state.
|
|
-- Two guards that are not optional, because this statement runs INSIDE the migration
|
|
-- transaction on boot and a failure here exits the process — `restart: unless-stopped` then
|
|
-- turns it into a crash loop with no in-app recovery. That is a strictly worse version of the
|
|
-- lockout this migration exists to clean up after.
|
|
--
|
|
-- * role = 'guest', not role <> 'admin'. The enum also has 'host' (001), and hosts are
|
|
-- promoted from guests at runtime — so <> 'admin' renamed a legitimately promoted staff
|
|
-- member whose name happens to be "Host".
|
|
-- * NOT EXISTS. The target name is derived, not unique: `idx_user_event_name_ci` (007) is a
|
|
-- UNIQUE index on (event_id, lower(display_name)), and nothing stopped a second guest from
|
|
-- having already joined as exactly "Admin (a1b2c3d4)" — the old code had no reserved-name
|
|
-- guard and the join response hands each guest their own id. Rare, but the cost of losing
|
|
-- that bet is the whole event.
|
|
--
|
|
-- A row that collides is simply left alone: `create_admin_user` already handles a name clash by
|
|
-- falling back to `Admin-<8hex>`, and `admin_login` no longer resolves by name at all, so this
|
|
-- cleanup is convenience rather than load-bearing.
|
|
UPDATE "user" u
|
|
SET display_name = u.display_name || ' (' || left(u.id::text, 8) || ')'
|
|
WHERE u.role = 'guest'
|
|
AND lower(u.display_name) IN ('admin', 'administrator', 'host', 'eventsnap')
|
|
AND NOT EXISTS (
|
|
SELECT 1 FROM "user" x
|
|
WHERE x.event_id = u.event_id
|
|
AND lower(x.display_name) = lower(u.display_name || ' (' || left(u.id::text, 8) || ')')
|
|
);
|
|
|
|
-- 2. PIN LOCKOUT DECAY.
|
|
--
|
|
-- failed_pin_attempts only ever cleared on a successful recovery or after a lockout expired, so
|
|
-- honest typos accumulated across days: a guest who fat-fingered their PIN twice last night
|
|
-- arrives today already two-thirds of the way to being locked out. With the threshold now
|
|
-- raised (see below) a decay window is what keeps that raise safe rather than merely lenient.
|
|
ALTER TABLE "user" ADD COLUMN IF NOT EXISTS last_failed_pin_at TIMESTAMPTZ;
|
|
|
|
-- Rate-limit knobs introduced with this release.
|
|
--
|
|
-- recover_name_rate_per_15min (4, was a hardcoded 5): the per-(IP, name) ceiling. It MUST stay
|
|
-- below the account-lock threshold, which is the whole defect — at 5-per-IP against a 3-strike
|
|
-- lock, three requests from one IP locked any guest whose name is visible on the feed, every 15
|
|
-- minutes, forever. The lock threshold moves to 12 in code, so locking a victim now needs at
|
|
-- least three distinct sources while an honest guest never comes close.
|
|
--
|
|
-- pin_reset_ip_rate_per_min (30): /recover/request was the one unauthenticated endpoint with no
|
|
-- per-IP ceiling at all — /join got one in 017 and /recover in 019, and this third one was
|
|
-- simply missed. Its per-name key is attacker-chosen, so cycling names minted a fresh bucket
|
|
-- every time and the per-IP cost was unbounded.
|
|
--
|
|
-- upload_edit_rate_per_min (30): PATCH /upload/{id} had no rate limit of any kind.
|
|
INSERT INTO config (key, value) VALUES
|
|
('recover_name_rate_per_15min', '4'),
|
|
('pin_reset_ip_rate_per_min', '30'),
|
|
('upload_edit_rate_per_min', '30'),
|
|
('upload_edit_rate_enabled', 'true')
|
|
ON CONFLICT (key) DO NOTHING;
|