fix: close eight regressions the audit pass found, five of them mine
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>
This commit is contained in:
@@ -48,9 +48,22 @@ fn validate_display_name(raw: &str) -> Result<&str, AppError> {
|
||||
"Name muss zwischen 1 und 50 Zeichen lang sein.".into(),
|
||||
));
|
||||
}
|
||||
// Postgres rejects 0x00 in TEXT columns with a 500. Catch it here so callers see a clean
|
||||
// 400 instead of an internal error.
|
||||
if name.contains('\0') {
|
||||
// No control characters. NUL is the hard requirement — Postgres rejects 0x00 in TEXT with a
|
||||
// 500, so catching it here turns an internal error into a clean 400 — but the rest matter
|
||||
// too, and for reasons beyond tidiness:
|
||||
//
|
||||
// * Newlines make the name a LOG INJECTION vector. Several 4xx messages interpolate it
|
||||
// ("Der Name \"X\" ist bereits vergeben.") and those are logged; a name carrying a
|
||||
// newline plus a plausible timestamp prefix lets two unauthenticated requests forge
|
||||
// entries in the only forensic record an unattended event has. `error.rs` escapes on the
|
||||
// way out as well — this is the other half, and the half that keeps the forged text out
|
||||
// of the database and out of the feed byline in the first place.
|
||||
// * A bare CR or a bidi override renders as a name that is not what was typed, in the feed,
|
||||
// the host dashboard's moderation list and the keepsake.
|
||||
//
|
||||
// Deliberately NOT a whitelist: guests have accents, emoji and non-Latin scripts in their
|
||||
// names, and rejecting those would be worse than the problem.
|
||||
if name.chars().any(|c| c.is_control()) {
|
||||
return Err(AppError::BadRequest(
|
||||
"Name enthält ungültige Zeichen.".into(),
|
||||
));
|
||||
@@ -237,7 +250,32 @@ fn dummy_pin_hash() -> &'static str {
|
||||
/// flood of `/recover` or `/admin/login` attempts stalls every other request on the box,
|
||||
/// including the feed. Offloading moves that cost to the blocking pool, which is sized for
|
||||
/// exactly this and whose saturation degrades logins rather than the whole app.
|
||||
/// Process-wide ceiling on CONCURRENT bcrypt work.
|
||||
///
|
||||
/// bcrypt is deliberately expensive — ~250 ms of a core at cost 12, and this deployment's own
|
||||
/// runbook generates the admin hash at a higher cost than that. Every call is correctly on
|
||||
/// `spawn_blocking`, but tokio's blocking pool defaults to 512 threads, so "off the async
|
||||
/// runtime" is not the same as "bounded": enough concurrent hashes will preempt both async
|
||||
/// worker threads a 2 vCPU box gets, and uploads, feed and SSE stall behind them.
|
||||
///
|
||||
/// Three unauthenticated endpoints reach bcrypt — `/join` (hash), `/recover` (verify, including
|
||||
/// a deliberate throwaway verify for unknown names) and `/admin/login` (verify) — each with only
|
||||
/// a per-IP bucket in front, and at a venue every guest shares one public IP. A per-IP limit
|
||||
/// therefore bounds nothing globally.
|
||||
///
|
||||
/// `cores - 1` leaves a core for actually serving requests. Excess callers WAIT on the permit
|
||||
/// rather than burning CPU, so a flood degrades to latency instead of an outage.
|
||||
static BCRYPT_PERMITS: std::sync::LazyLock<tokio::sync::Semaphore> =
|
||||
std::sync::LazyLock::new(|| {
|
||||
let cores = std::thread::available_parallelism()
|
||||
.map(|n| n.get())
|
||||
.unwrap_or(2);
|
||||
tokio::sync::Semaphore::new(cores.saturating_sub(1).max(1))
|
||||
});
|
||||
|
||||
async fn verify_password(candidate: String, hash: String) -> bool {
|
||||
// `acquire()` only fails if the semaphore is closed, which never happens here.
|
||||
let _permit = BCRYPT_PERMITS.acquire().await;
|
||||
tokio::task::spawn_blocking(move || bcrypt::verify(&candidate, &hash).unwrap_or(false))
|
||||
.await
|
||||
.unwrap_or(false)
|
||||
@@ -246,6 +284,9 @@ async fn verify_password(candidate: String, hash: String) -> bool {
|
||||
/// Hash a secret on the blocking pool. Same reasoning as [`verify_password`] — and this one
|
||||
/// runs on the busiest auth path there is, since every guest who joins gets a PIN hashed.
|
||||
pub async fn hash_password(secret: String, cost: u32) -> Result<String, AppError> {
|
||||
// Same global ceiling as `verify_password` — `/join` hashes a PIN for every guest, and 100
|
||||
// guests scanning the QR at once is the arrival burst this box has to survive.
|
||||
let _permit = BCRYPT_PERMITS.acquire().await;
|
||||
tokio::task::spawn_blocking(move || bcrypt::hash(&secret, cost))
|
||||
.await
|
||||
.map_err(|e| AppError::Internal(anyhow::anyhow!(e)))?
|
||||
@@ -411,11 +452,15 @@ pub struct AdminLoginResponse {
|
||||
|
||||
/// Requests per minute per IP that may reach `verify_password` at all.
|
||||
///
|
||||
/// Not a security control — the failure bucket below is. This exists solely so an unauthenticated
|
||||
/// endpoint cannot burn the box's CPU on cost-12 bcrypt (~250 ms each) at line rate. Set far above
|
||||
/// anything a person typing a password can produce, because on venue NAT every guest shares the
|
||||
/// operator's IP and this ceiling, unlike the failure bucket, can still refuse a correct password.
|
||||
const ADMIN_LOGIN_CPU_CEILING: usize = 120;
|
||||
/// Not a security control — the failure bucket below is. It bounds how deep a queue can form on
|
||||
/// `BCRYPT_PERMITS`, which is what actually caps the CPU cost.
|
||||
///
|
||||
/// Still far above anything a person typing a password produces, but note the honest limitation:
|
||||
/// unlike the failure bucket, this ceiling CAN refuse a correct password, and on venue NAT every
|
||||
/// guest shares the operator's IP. It is a smaller number than it first was for exactly that
|
||||
/// reason — the earlier 120 was chosen when this was the only bound on bcrypt, which made it both
|
||||
/// too weak to cap CPU and too coarse to be safe for the operator.
|
||||
const ADMIN_LOGIN_CPU_CEILING: usize = 30;
|
||||
|
||||
pub async fn admin_login(
|
||||
State(state): State<AppState>,
|
||||
@@ -686,6 +731,22 @@ pub async fn request_pin_reset(
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Control characters are rejected at the door. Newlines in particular: several 4xx messages
|
||||
/// interpolate the display name and those are logged, so a name carrying a newline plus a
|
||||
/// plausible prefix would let two unauthenticated requests forge lines in the event's only
|
||||
/// forensic record. `error.rs` escapes on output too; this keeps it out of the database and
|
||||
/// the feed byline in the first place.
|
||||
#[test]
|
||||
fn a_display_name_may_not_carry_control_characters() {
|
||||
for bad in ["Anna\nERROR forged", "Anna\rX", "Anna\u{0}X", "A\u{7}B"] {
|
||||
assert!(validate_display_name(bad).is_err(), "{bad:?} must be rejected");
|
||||
}
|
||||
// Real guests have accents, emoji and non-Latin names — never reject those.
|
||||
for good in ["Anna", "Zo\u{eb}", "Jos\u{e9}", "\u{5c71}\u{7530}", "Anna \u{1f389}"] {
|
||||
assert!(validate_display_name(good).is_ok(), "{good:?} must be allowed");
|
||||
}
|
||||
}
|
||||
|
||||
/// THE defect, stated as arithmetic: the account-lock threshold sat BELOW the per-(IP, name)
|
||||
/// attempt ceiling, so a single IP could exhaust it and lock any guest whose display name is
|
||||
/// visible on the feed — every 15 minutes, indefinitely. The tier meant to protect a guest
|
||||
|
||||
Reference in New Issue
Block a user