fabi/EventSnap
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: close four confirmed defects an adversarial review found

Browse Source 
Findings from a multi-angle review, most of them in code I wrote in the last
few commits. Each was verified against the code before being acted on.

## Backend

**The export daily limit was bypassable ~60x/minute.** `SseTicketStore` is
untyped, and the export download quietly started reusing it. `POST
/stream/ticket` is free and rate-limited at 60/min per user; `POST
/export/ticket` charges one of three PER-DAY downloads. So a guest could mint
at the cheap endpoint and redeem at the expensive one, each redemption
streaming the whole multi-GB keepsake, `no-store`, off the same filesystem
Postgres writes WAL to. Tickets now carry a `TicketKind` and `consume` requires
it to match, asserted in both directions. The comment claiming "one mint is at
most one download" was simply false.

**`export_ticket` answered 200 `{"ticket": null}` when the store was full** —
after charging a daily slot. `issue` returns `Option`; `sse.rs` handles the
None with a 503 and this call site unwrapped it into the JSON body. The page
toasted success, the iframe navigated to `?ticket=null`, and one of three
downloads was gone. That is the phantom-success failure the pre-validation in
5b70531 exists to prevent, arriving through the other door.

**`finalize_job` collapsed a DB error into "we lost the epoch race."** At that
point the archive is built, fsynced and renamed, so the caller deleted the
finished multi-GB file and returned the Superseded sentinel — which
`abandon_if_superseded` swallows into Ok, so `mark_failed` never ran either.
The row stayed `running` at 99% at the LIVE epoch: "Wird erstellt (99 %)",
download disabled, forever. No sweep re-examines `running` rows and
`recover_exports` runs only at boot. `claim_job`'s own doc comment says errors
are distinguished there precisely because of this failure shape. A pool timeout
is not exotic: max_connections 10, acquire_timeout 5s, firing at the end of a
full-gallery export while 100 guests upload.

**`PATCH {"hashtags": []}` was a free keepsake-retire loop.** The no-op guard
only compared captions, and my comment defended the gap by claiming an
identical hashtag list "is not a free loop". It is exactly one. Each request
bumped the epoch, retiring the HTML keepsake; REGEN_DEBOUNCE throttles when a
rebuild may start, not the bump, so at 30/min no rebuild ever gets a quiet
window and /export/html 404s all event. Now compares against the stored tags.

Also: four config keys migration 025 inserts (and the handlers read) were
missing from `patch_config`'s allowlist, so `GET /admin/config` listed them
while `PATCH` answered "Unbekannter Konfigurationsschlüssel" — the rate limits
an operator reaches for while abuse is happening.

## Client upload queue

**The ✕ was cosmetic.** A cancel deliberately charges no attempt and sets no
backoff — so `requeueRetriable` matched it on both counts and restarted the
upload from byte zero within ~120s (an `online` event, or the SSE backstop's
`feed-delta` poll). It then restarted forever, because a path that never
charges an attempt can never exhaust the budget that would stop it. The row
read "Abgebrochen. Tippe auf „Erneut“." throughout. Cancels are now explicitly
terminal until the guest taps Erneut.

**The retry budget was a lifetime quota, not a rate.** Five attempts on a
5/10/20/40s ladder is ~75 seconds, so any outage longer than that — a venue AP
brownout, a captive portal re-arming, an `app` restart, all with
`navigator.onLine` still true — permanently parked every in-flight photo
behind a per-row button three taps deep. It now refills after 10 quiet minutes,
which still forbids a hot loop re-sending a 200 MB video over a shared uplink.

**A test asserted a property the code does not have.** The suspension test
omitted the MAX_SUSPEND_CREDIT_MS clamp the production tick applies, so it
could not fail. Replaced with a helper that replays the real tick loop, and the
true bound is now asserted: a 60s lock survives, a 3-minute lock aborts.

152/152 backend, 59/59 vitest, clippy clean, svelte-check 0 errors, eslint
clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-08-09 14:39:27 +02:00
parent 253878e027
commit 214f9e3062
8 changed files with 289 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
backend/src/handlers/admin.rs
View File

@@ -10,6 +10,7 @@ use uuid::Uuid;
use crate::auth::middleware::RequireAdmin;
use crate::error::AppError;
use crate::services::config;
use crate::services::sse_tickets::TicketKind;
use crate::state::AppState;
// ── DTOs ─────────────────────────────────────────────────────────────────────
@@ -132,6 +133,16 @@ pub async fn patch_config(
("social_rate_per_min", true, 1.0, 100_000.0),
("quota_tolerance", false, 0.0, 1.0),
("estimated_guest_count", true, 1.0, 1_000_000.0),
// The three limiters migration 025 introduced. All are READ at runtime
// (`upload.rs` for the edit limiter, `auth/handlers.rs` for the other two) and 025
// INSERTs all of them into `config`, so `GET /admin/config` listed them while
// `PATCH /admin/config` answered "Unbekannter Konfigurationsschlüssel" — the same
// dead-key defect the comment under BOOL_KEYS says was fixed for the two login
// toggles. These are precisely the knobs an operator reaches for while abuse is
// happening, which is the one moment a restart to change them is unaffordable.
("upload_edit_rate_per_min", true, 1.0, 100_000.0),
("recover_name_rate_per_15min", true, 1.0, 100_000.0),
("pin_reset_ip_rate_per_min", true, 1.0, 100_000.0),
];
const BOOL_KEYS: &[&str] = &[
"rate_limits_enabled",
@@ -145,6 +156,9 @@ pub async fn patch_config(
"admin_login_rate_enabled",
"recover_rate_enabled",
"social_rate_enabled",
// Read by `upload::edit_upload`, inserted by migration 025, and until now unreachable
// from this endpoint — see the note in NUMERIC_SPECS.
"upload_edit_rate_enabled",
"quota_enabled",
"storage_quota_enabled",
"upload_count_quota_enabled",
@@ -397,7 +411,21 @@ pub async fn export_ticket(
enforce_export_rate(&state, auth.user_id).await?;
let ticket = state.sse_tickets.issue(auth.token_hash);
// `issue` returns None when the ticket store is at capacity. Unwrapping it into the JSON body
// serialized `{"ticket": null}` with a 200 — so `api.post` resolved happily, the page toasted
// success, the iframe navigated to `?ticket=null`, and one of the guest's three DAILY
// downloads had already been charged above. That is precisely the phantom-success failure
// this endpoint's pre-validation was added to eliminate, arriving through the other door.
// 503 + Retry-After, matching how `sse::issue_ticket` answers the identical condition.
let ticket = state
.sse_tickets
.issue(auth.token_hash, TicketKind::Download)
.ok_or_else(|| {
AppError::ServiceUnavailable(
"Server ist gerade ausgelastet. Bitte versuch es in einem Moment erneut.".into(),
Some(30),
)
})?;
Ok(Json(serde_json::json!({ "ticket": ticket })))
}
@@ -407,7 +435,7 @@ pub async fn export_ticket(
async fn authenticate_download_ticket(state: &AppState, ticket: &str) -> Result<Uuid, AppError> {
let token_hash = state
.sse_tickets
.consume(ticket)
.consume(ticket, TicketKind::Download)
.ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?;
let session = crate::models::session::Session::find_by_token_hash(&state.pool, &token_hash)
.await

5
backend/src/handlers/sse.rs
View File

@@ -13,6 +13,7 @@ use tokio_stream::wrappers::errors::BroadcastStreamRecvError;
use crate::auth::middleware::AuthUser;
use crate::error::AppError;
use crate::models::session::Session;
use crate::services::sse_tickets::TicketKind;
use crate::state::AppState;
#[derive(Deserialize)]
@@ -52,7 +53,7 @@ pub async fn issue_ticket(
));
}
let ticket = state.sse_tickets.issue(auth.token_hash).ok_or_else(|| {
let ticket = state.sse_tickets.issue(auth.token_hash, TicketKind::Sse).ok_or_else(|| {
AppError::ServiceUnavailable(
"Server ist gerade ausgelastet. Live-Updates folgen in Kürze.".into(),
Some(30),
@@ -75,7 +76,7 @@ pub async fn stream(
) -> Result<Sse<impl Stream<Item = Result<Event, Infallible>>>, AppError> {
let token_hash = state
.sse_tickets
.consume(&q.ticket)
.consume(&q.ticket, TicketKind::Sse)
.ok_or_else(|| AppError::Unauthorized("Ticket ungültig oder abgelaufen.".into()))?;
// NOTE: this authenticates via ticket→session, not the `AuthUser` extractor. The

30
backend/src/handlers/upload.rs
View File

@@ -774,13 +774,37 @@ pub async fn edit_upload(
// REGEN_DEBOUNCE bounds the rate of that, not the total work, so the keepsake could be kept
// permanently un-downloadable.
//
// Residual, deliberately not fixed: re-sending an IDENTICAL hashtag list still counts as a
// change. Comparing would need another query, and unlike `PATCH {}` it is not a free loop.
// The hashtag half of that guard was wrong, and the comment here used to defend it: it said
// re-sending an identical list "is not a free loop". It is exactly a free loop. `PATCH
// {"hashtags": []}` carries no photo, no bytes and no client-side cost, yet it made
// `normalized_tags` `Some`, sailed past the caption-only check, and bumped `export_epoch` on
// every request — retiring the HTML keepsake instantly. REGEN_DEBOUNCE (20s) throttles when a
// rebuild may START, not the epoch bump, so at the 30/min this endpoint allows no rebuild ever
// gets a quiet window to finish in and `GET /export/html` 404s for the whole event. The ZIP is
// carried forward, so this denied exactly half the product.
//
// So compare properly. One indexed lookup against `upload_hashtag` is cheap next to the
// full-gallery rebuild a false positive arms.
let caption_changed = match (&body.caption, &upload.caption) {
(Some(new), existing) => Some(new.as_str()) != existing.as_deref(),
(None, _) => false,
};
if !caption_changed && normalized_tags.is_none() {
let tags_changed = match &normalized_tags {
None => false,
Some(incoming) => {
// Compare on the same normalised form `upsert` keys on, so "#Party", "party" and
// " #PARTY " are all the same tag and none of them counts as an edit.
let mut want: Vec<String> = incoming
.iter()
.map(|t| t.trim().trim_start_matches('#').to_lowercase())
.collect();
want.sort();
want.dedup();
let have = Hashtag::normalized_for_upload(&state.pool, upload_id).await?;
want != have
}
};
if !caption_changed && !tags_changed {
return Ok(StatusCode::OK);
}

27
backend/src/models/hashtag.rs
View File

@@ -63,6 +63,33 @@ impl Hashtag {
.await?;
Ok(())
}
/// The upload's current tags, lowercased and sorted — the comparable form.
///
/// Exists so `edit_upload` can tell a real hashtag change from a re-send of the same list.
/// Without it, `PATCH {"hashtags": []}` in a loop retired the HTML keepsake on every request
/// (readiness is derived from `event.export_epoch`), and every armed rebuild was superseded
/// before the debounce let it start — so the viewer 404'd for the rest of the event at zero
/// cost to the client. One indexed lookup on `upload_hashtag(upload_id)` is a fair price for
/// closing that.
pub async fn normalized_for_upload<'e, E>(
executor: E,
upload_id: Uuid,
) -> Result<Vec<String>, sqlx::Error>
where
E: sqlx::PgExecutor<'e>,
{
let rows: Vec<(String,)> = sqlx::query_as(
"SELECT lower(h.tag) FROM hashtag h
JOIN upload_hashtag uh ON uh.hashtag_id = h.id
WHERE uh.upload_id = $1
ORDER BY lower(h.tag)",
)
.bind(upload_id)
.fetch_all(executor)
.await?;
Ok(rows.into_iter().map(|(t,)| t).collect())
}
}
/// Extract `#hashtags` from text (caption or body). Tags are restricted to

25
backend/src/services/export.rs
View File

@@ -691,7 +691,7 @@ async fn run_zip_export_inner(
// Note what ISN'T here any more: the ready-flag flip. Readiness is derived from
// (released AND job.epoch = event.epoch AND status = 'done'), so writing `done` at a live epoch
// IS the publish, atomically. A worker at a dead epoch simply writes a row nobody can see.
if !finalize_job(pool, event_id, "zip", epoch, &format!("exports/{out_name}")).await {
if !finalize_job(pool, event_id, "zip", epoch, &format!("exports/{out_name}")).await? {
let _ = tokio::fs::remove_file(&out_path).await;
tracing::info!(
"ZIP export for event {event_id} superseded (epoch {epoch} retired); discarded"
@@ -1129,7 +1129,7 @@ async fn run_html_export_inner(
epoch,
&format!("exports/{out_name}"),
)
.await
.await?
{
let _ = tokio::fs::remove_file(&out_path).await;
tracing::info!(
@@ -1250,14 +1250,27 @@ async fn claim_job(pool: &PgPool, event_id: Uuid, export_type: &str, epoch: i64)
/// AND status = 'done')`, so a single row write makes the keepsake downloadable — there is no
/// second flag to flip and therefore no window between "done" and "visible". A worker whose epoch
/// was retired matches nothing here and must discard its output.
///
/// ERRORS ARE DISTINGUISHED FROM A LOST RACE, for the same reason `claim_job` distinguishes them —
/// and the consequence here is strictly worse. This used to end `.unwrap_or(false)`, collapsing a
/// pool timeout into "we were superseded". At that point the archive is already built, fsynced and
/// renamed into place, so the caller went on to DELETE the finished multi-GB file and return the
/// `Superseded` sentinel — which `abandon_if_superseded` swallows into `Ok(())`, so
/// `spawn_export_jobs` never called `mark_failed` either. The row stayed `running` at 99% at the
/// LIVE epoch, which the host dashboard renders as "Wird erstellt (99 %)" with the download
/// disabled, forever: no sweep re-examines `running` rows, and `recover_exports` runs only at boot.
///
/// A pool timeout is not exotic here. `max_connections` is 10, `acquire_timeout` 5s, and this fires
/// at the end of a full-gallery export while ~100 guests are uploading. Returning `Err` instead
/// leaves the finished archive on disk and lets the caller's `mark_failed` record a real reason.
async fn finalize_job(
pool: &PgPool,
event_id: Uuid,
export_type: &str,
epoch: i64,
file_path: &str,
) -> bool {
sqlx::query(
) -> Result<bool> {
let r = sqlx::query(
"UPDATE export_job
SET status = 'done', progress_pct = 100, file_path = $3, completed_at = NOW()
WHERE event_id = $1 AND type = $2::export_type
@@ -1269,8 +1282,8 @@ async fn finalize_job(
.bind(epoch)
.execute(pool)
.await
.map(|r| r.rows_affected() > 0)
.unwrap_or(false)
.context("finalizing export job")?;
Ok(r.rows_affected() > 0)
}
/// Parse the trailing generation number out of `<prefix><n><suffix>` (e.g.

100
backend/src/services/sse_tickets.rs
View File

@@ -23,6 +23,26 @@ const MAX_TICKETS: usize = 4096;
/// EventSources concurrently; 4 absorbs that without letting a reconnect loop accumulate.
const MAX_TICKETS_PER_SESSION: usize = 4;
/// What a ticket may be redeemed for.
///
/// The store began life serving only SSE and stayed untyped when the export download started
/// reusing it, which silently made the two interchangeable. That is not a theoretical mixing
/// concern: `POST /stream/ticket` is rate-limited at 60/min per user and charges nothing, while
/// `POST /export/ticket` charges one of three PER-DAY downloads. An untyped ticket let any guest
/// mint at the cheap endpoint and redeem at the expensive one, so the daily export limit was
/// bypassable ~60×/minute — each redemption streaming the whole multi-GB keepsake, `no-store`,
/// off the same filesystem Postgres writes WAL to.
///
/// `consume` therefore requires the kind to MATCH. A ticket is only ever valid for the thing it
/// was minted for.
#[derive(Clone, Copy, PartialEq, Eq, Debug)]
pub enum TicketKind {
/// Opens the SSE stream (`GET /stream`). Cheap, high volume.
Sse,
/// Downloads an export archive (`GET /export/{zip,html}`). Expensive, rate-limited per day.
Download,
}
#[derive(Clone)]
pub struct SseTicketStore {
inner: Arc<Mutex<HashMap<String, Entry>>>,
@@ -32,6 +52,7 @@ pub struct SseTicketStore {
struct Entry {
token_hash: String,
issued_at: Instant,
kind: TicketKind,
}
impl SseTicketStore {
@@ -55,7 +76,7 @@ impl SseTicketStore {
/// Three bounds, because `issue` had none: no size cap, no per-caller cap, and no rate
/// limit on the endpoint, while `prune` ran only hourly against a 30-second TTL. So any
/// authenticated session could loop the endpoint and grow the map for an hour.
pub fn issue(&self, token_hash: String) -> Option<String> {
pub fn issue(&self, token_hash: String, kind: TicketKind) -> Option<String> {
let ticket = random_ticket();
let mut map = self.inner.lock().unwrap();
@@ -95,20 +116,34 @@ impl SseTicketStore {
Entry {
token_hash,
issued_at: Instant::now(),
kind,
},
);
Some(ticket)
}
/// Consume a ticket. Returns `Some(token_hash)` if the ticket exists and is
/// not expired. Single-use: the ticket is removed regardless of whether it
/// was still fresh, so a replay can't slip through after expiry.
pub fn consume(&self, ticket: &str) -> Option<String> {
/// Consume a ticket minted for `kind`. Returns `Some(token_hash)` if the ticket exists, is
/// not expired, and was minted for this purpose. Single-use: the ticket is removed regardless
/// of whether it was still fresh, so a replay can't slip through after expiry.
///
/// A ticket of the WRONG kind is also removed. It was a valid ticket the caller legitimately
/// held, so this is not punitive — but leaving it would let a redemption loop probe the store
/// without ever spending anything, and the client has no legitimate reason to present a
/// ticket at the wrong endpoint.
pub fn consume(&self, ticket: &str, kind: TicketKind) -> Option<String> {
let mut map = self.inner.lock().unwrap();
let entry = map.remove(ticket)?;
if entry.issued_at.elapsed() > TTL {
return None;
}
if entry.kind != kind {
tracing::warn!(
expected = ?kind,
found = ?entry.kind,
"ticket presented at the wrong endpoint; rejected"
);
return None;
}
Some(entry.token_hash)
}
@@ -135,17 +170,54 @@ mod tests {
/// `issue` now returns `Option`; in every test below the store is far from capacity, so an
/// `expect` here documents that refusing is exceptional rather than routine.
fn issue(store: &SseTicketStore, hash: &str) -> String {
store.issue(hash.into()).expect("store has capacity")
store
.issue(hash.into(), TicketKind::Sse)
.expect("store has capacity")
}
/// The store is shared by two endpoints with wildly different costs: `/stream/ticket` is
/// 60/min per user and free, `/export/ticket` charges one of three PER-DAY downloads. While
/// entries were untyped, a ticket minted at the cheap endpoint opened the expensive one — so
/// the daily export limit could be bypassed ~60×/minute, each redemption streaming the whole
/// multi-GB keepsake off the disk Postgres writes WAL to.
///
/// Asserted in BOTH directions so this cannot be "fixed" by a check that only guards one.
#[test]
fn a_ticket_is_only_valid_for_the_purpose_it_was_minted_for() {
let store = SseTicketStore::new();
let sse = store.issue("h".into(), TicketKind::Sse).unwrap();
assert_eq!(
store.consume(&sse, TicketKind::Download),
None,
"an SSE ticket must not open the export download"
);
let dl = store.issue("h".into(), TicketKind::Download).unwrap();
assert_eq!(
store.consume(&dl, TicketKind::Sse),
None,
"a download ticket must not open the SSE stream"
);
// And the matching cases still work, so the guard is not simply rejecting everything.
let sse = store.issue("h".into(), TicketKind::Sse).unwrap();
assert_eq!(store.consume(&sse, TicketKind::Sse).as_deref(), Some("h"));
let dl = store.issue("h".into(), TicketKind::Download).unwrap();
assert_eq!(
store.consume(&dl, TicketKind::Download).as_deref(),
Some("h")
);
}
#[test]
fn issue_then_consume_returns_the_hash_exactly_once() {
let store = SseTicketStore::new();
let ticket = issue(&store, "hash-1");
assert_eq!(store.consume(&ticket).as_deref(), Some("hash-1"));
assert_eq!(store.consume(&ticket, TicketKind::Sse).as_deref(), Some("hash-1"));
// Single-use: a replay of the same ticket is rejected.
assert_eq!(
store.consume(&ticket),
store.consume(&ticket, TicketKind::Sse),
None,
"a consumed ticket must not be reusable"
);
@@ -154,7 +226,7 @@ mod tests {
#[test]
fn unknown_ticket_consumes_to_none() {
let store = SseTicketStore::new();
assert_eq!(store.consume("never-issued"), None);
assert_eq!(store.consume("never-issued", TicketKind::Sse), None);
}
#[test]
@@ -172,7 +244,7 @@ mod tests {
let store = SseTicketStore::new();
let ticket = issue(&store, "h");
store.prune(); // not expired → kept
assert_eq!(store.consume(&ticket).as_deref(), Some("h"));
assert_eq!(store.consume(&ticket, TicketKind::Sse).as_deref(), Some("h"));
}
/// Build an entry that is already past the TTL.
@@ -180,6 +252,7 @@ mod tests {
store.inner.lock().unwrap().insert(
key.to_string(),
Entry {
kind: TicketKind::Sse,
token_hash: token_hash.into(),
issued_at: Instant::now()
.checked_sub(TTL + Duration::from_secs(1))
@@ -193,7 +266,7 @@ mod tests {
let store = SseTicketStore::new();
insert_stale(&store, "stale-ticket", "h");
assert_eq!(
store.consume("stale-ticket"),
store.consume("stale-ticket", TicketKind::Sse),
None,
"an expired ticket must not authenticate"
);
@@ -236,7 +309,7 @@ mod tests {
"the newest ticket is the one the caller is about to use"
);
assert_eq!(
store.consume(&stranger).as_deref(),
store.consume(&stranger, TicketKind::Sse).as_deref(),
Some("other-session"),
"another session's ticket must survive — evicting it would let one client deny \
SSE to the venue"
@@ -254,6 +327,7 @@ mod tests {
map.insert(
format!("filler-{i}"),
Entry {
kind: TicketKind::Sse,
token_hash: format!("session-{i}"),
issued_at: Instant::now(),
},
@@ -261,7 +335,7 @@ mod tests {
}
}
assert_eq!(
store.issue("newcomer".into()),
store.issue("newcomer".into(), TicketKind::Sse),
None,
"a full store must refuse, so the caller can answer 503"
);