fix(auth,upload): close the admin lockout and four unbounded-input paths
ADMIN LOCKOUT. admin_login looked its user up BY NAME. Migration 007 makes
display_name unique per event case-insensitively and join had no reserved-name
guard, so any guest joining as "admin"/"Admin"/"ADMIN" before the operator's first
login made find(role == Admin) miss, the fallback create("Admin") violate that
index, and `?` return a 500 — permanently, with no in-app recovery. Moderation,
config and gallery release all gone; the fix was hand-editing the database.
The root cause is the lookup key, not the creation. The name was never the
identity. User::find_admin_for_event resolves by role, which makes the whole class
of name collisions irrelevant — including the homoglyph bypasses of the new
reserved-name list, which is now defence in depth rather than the control.
Promoting the squatting row would be the obvious fix and is a serious mistake: it
carries a recovery_pin_hash the guest knows, so it would hand them the admin
dashboard via /recover, permanently, through a path needing no password. A
separate row under a fallback name is worse UX and much better security. Verified
against the real schema — the guest keeps their uploads, PIN and session under a
freed name, and the role lookup then finds exactly one admin.
Second, independent bug in that block: create() followed by a SEPARATE UPDATE ...
SET role = 'admin' manufactures the same poisoned state if anything fails between
them. Collapsed into create_with_role.
UNBOUNDED INPUTS — one root cause, four places: validation ran after the
allocation.
- upload caption/hashtags used Field::text(), which buffers the whole field, on
the one route whose DefaultBodyLimit is 576 MiB — so 576 MiB of heap per
concurrent request in a 1 GiB container, with the length check running
afterwards on a string already built. Now refused mid-read.
- the hashtag CSV was never length-checked at all and was upserted tag by tag
INSIDE the commit transaction, which holds FOR SHARE on the event row — one
request could stall every other upload behind tens of thousands of round trips.
Capped at 30 tags of <=50 chars.
- /recover and /recover/request built rate-limiter keys by format!() from an
unvalidated, unbounded display name, retained up to 24h in a map pruned hourly:
the limiter itself became the memory-exhaustion primitive it exists to prevent.
join validated first; that check is now shared by all three. /recover/request
also had no per-IP ceiling at all — /join got one in 017, /recover in 019, and
019's own comment describes exactly this attack. It returns 204 rather than 400
on a bad name, because a 400 would be a new signal on an endpoint whose contract
is that it cannot enumerate guests.
- the SSE ticket store had no size cap, no per-session cap and no rate limit on
its endpoint, while prune ran hourly against a 30s TTL. Now pruned on issue,
capped, and rate-limited. At capacity it REFUSES rather than evicting a
stranger's ticket — evicting would let one client deny SSE to the venue. Not
one-ticket-per-session either: two tabs open their EventSources concurrently.
PATCH /upload/{id} had no rate limit, no validation, and called
invalidate_and_arm unconditionally — outside both `if let Some` guards. So
PATCH {} bumped export_epoch and armed a fresh pair of full-gallery export workers
every call; REGEN_DEBOUNCE bounds the rate of that, not the total work, so a guest
could keep the keepsake permanently un-downloadable. All three fixed. The
validation also resolves a divergence: upload normalised tags while edit stored
them raw, so #Party via edit and party via upload became two hashtag rows.
PIN LOCKOUT was an ordering bug before a policy one: the account-lock threshold
(3) sat BELOW the per-(IP, name) ceiling (5), so three requests from one IP locked
any guest whose name is on the feed, every 15 minutes, forever. The tier meant to
protect a guest was the cheapest way to attack them. Ceiling drops to 4, threshold
rises to 12, so locking a victim now needs at least three distinct sources.
Brute-force cost is unchanged — 48 attempts/hour means 10k PINs still take ~208h
regardless of IP count — and increment_failed_pin now decays the streak after 15
minutes, since the counter previously only cleared on success and honest typos
accumulated across days. Both invariants are pinned by tests rather than comments.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -38,7 +38,26 @@ pub async fn issue_ticket(
|
||||
State(state): State<AppState>,
|
||||
auth: AuthUser,
|
||||
) -> Result<Json<StreamTicketResponse>, AppError> {
|
||||
let ticket = state.sse_tickets.issue(auth.token_hash);
|
||||
// The endpoint had no rate limit at all. Authentication is not a bound here: one valid
|
||||
// session could loop it freely. 60/min is far above a real client (one ticket per SSE
|
||||
// (re)connect, and reconnects are backed off) while capping a loop.
|
||||
if let Err(retry_after_secs) = state.rate_limiter.check_with_retry(
|
||||
format!("sse_ticket:{}", auth.user_id),
|
||||
60,
|
||||
Duration::from_secs(60),
|
||||
) {
|
||||
return Err(AppError::TooManyRequests(
|
||||
"Zu viele Verbindungsversuche. Bitte warte kurz.".into(),
|
||||
Some(retry_after_secs),
|
||||
));
|
||||
}
|
||||
|
||||
let ticket = state.sse_tickets.issue(auth.token_hash).ok_or_else(|| {
|
||||
AppError::ServiceUnavailable(
|
||||
"Server ist gerade ausgelastet. Live-Updates folgen in Kürze.".into(),
|
||||
Some(30),
|
||||
)
|
||||
})?;
|
||||
let server_time = sqlx::query_scalar("SELECT NOW()")
|
||||
.fetch_one(&state.pool)
|
||||
.await?;
|
||||
|
||||
@@ -17,6 +17,79 @@ use crate::state::AppState;
|
||||
|
||||
const MAX_CAPTION_LENGTH: usize = 2000;
|
||||
|
||||
/// Byte ceiling for the caption field, enforced WHILE reading it.
|
||||
///
|
||||
/// `Field::text()` buffers the entire field before returning, and this is the one route whose
|
||||
/// `DefaultBodyLimit` is raised to 576 MiB (main.rs) — so `caption=<576 MiB of text>` allocated
|
||||
/// 576 MiB of heap per concurrent request inside a 1 GiB container, and the
|
||||
/// `MAX_CAPTION_LENGTH` check only ran afterwards, on a string that had already been built.
|
||||
/// 4 bytes per code point is the worst case for UTF-8, so this can never reject a caption the
|
||||
/// character limit would have accepted.
|
||||
const MAX_CAPTION_BYTES: usize = MAX_CAPTION_LENGTH * 4;
|
||||
|
||||
/// Byte ceiling for the raw hashtag CSV. Generous next to what the tag caps below allow.
|
||||
const MAX_HASHTAGS_BYTES: usize = 4 * 1024;
|
||||
|
||||
/// Hashtags stored per upload. The CSV was never length-checked at all and was split into an
|
||||
/// unbounded `Vec`, then upserted TAG BY TAG inside the commit transaction — which holds a
|
||||
/// `FOR SHARE` lock on the event row, so one request could stall every other upload behind
|
||||
/// tens of thousands of round trips.
|
||||
const MAX_HASHTAGS_PER_UPLOAD: usize = 30;
|
||||
/// Characters per stored tag. `extract_hashtags` already self-bounds at 40; this covers the CSV
|
||||
/// path, which had no bound of its own.
|
||||
const MAX_HASHTAG_LENGTH: usize = 50;
|
||||
|
||||
/// Read a multipart text field, refusing it the moment it exceeds `max_bytes`.
|
||||
///
|
||||
/// The point is to fail DURING the read rather than after it — `Field::text()` cannot, because
|
||||
/// it has already allocated the whole thing by the time it returns.
|
||||
async fn read_text_field_bounded(
|
||||
mut field: axum::extract::multipart::Field<'_>,
|
||||
max_bytes: usize,
|
||||
) -> Result<String, AppError> {
|
||||
let mut buf: Vec<u8> = Vec::new();
|
||||
while let Some(chunk) = field
|
||||
.chunk()
|
||||
.await
|
||||
.map_err(|e| AppError::BadRequest(e.to_string()))?
|
||||
{
|
||||
if buf.len() + chunk.len() > max_bytes {
|
||||
return Err(AppError::BadRequest(
|
||||
"Eingabe ist zu lang.".to_string(),
|
||||
));
|
||||
}
|
||||
buf.extend_from_slice(&chunk);
|
||||
}
|
||||
String::from_utf8(buf).map_err(|_| AppError::BadRequest("Ungültige Zeichenkodierung.".into()))
|
||||
}
|
||||
|
||||
/// Normalise, dedupe and CAP the tags for one upload.
|
||||
///
|
||||
/// Extracted as a pure function so the caps are testable without standing up multipart, and
|
||||
/// shared by the upload and edit paths — which previously disagreed: upload lowercased and
|
||||
/// stripped `#`, while edit upserted raw strings, so `#Party` via edit and `party` via upload
|
||||
/// became two different hashtag rows.
|
||||
///
|
||||
/// Truncates rather than rejecting. `extract_hashtags` legitimately derives tags from a
|
||||
/// 2000-character caption, and 400-ing a guest for writing an enthusiastic caption would be a
|
||||
/// worse outcome than silently keeping the first 30.
|
||||
fn normalize_tags(caption_tags: Vec<String>, csv: Option<&str>) -> Vec<String> {
|
||||
let mut tags = caption_tags;
|
||||
if let Some(csv) = csv {
|
||||
for tag in csv.split(',') {
|
||||
let t = tag.trim().trim_start_matches('#').to_lowercase();
|
||||
if !t.is_empty() {
|
||||
tags.push(t);
|
||||
}
|
||||
}
|
||||
}
|
||||
tags.sort();
|
||||
tags.dedup();
|
||||
tags.retain(|t| t.chars().count() <= MAX_HASHTAG_LENGTH);
|
||||
tags.truncate(MAX_HASHTAGS_PER_UPLOAD);
|
||||
tags
|
||||
}
|
||||
|
||||
/// Owns the bytes an in-flight upload has written to disk, and deletes them unless the
|
||||
/// request reaches the point where a database row takes ownership.
|
||||
///
|
||||
@@ -204,20 +277,10 @@ pub async fn upload(
|
||||
streamed = Some(stream_field_to_file(field, &temp_abs, cap_bytes).await?);
|
||||
}
|
||||
"caption" => {
|
||||
caption = Some(
|
||||
field
|
||||
.text()
|
||||
.await
|
||||
.map_err(|e| AppError::BadRequest(e.to_string()))?,
|
||||
);
|
||||
caption = Some(read_text_field_bounded(field, MAX_CAPTION_BYTES).await?);
|
||||
}
|
||||
"hashtags" => {
|
||||
hashtags_csv = Some(
|
||||
field
|
||||
.text()
|
||||
.await
|
||||
.map_err(|e| AppError::BadRequest(e.to_string()))?,
|
||||
);
|
||||
hashtags_csv = Some(read_text_field_bounded(field, MAX_HASHTAGS_BYTES).await?);
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
@@ -344,21 +407,14 @@ pub async fn upload(
|
||||
// owns `temp_abs`, which is why retargeting comes after it rather than before.
|
||||
file_guard.retarget(absolute_path.clone());
|
||||
|
||||
// Process hashtags from caption and explicit CSV
|
||||
let mut tags: Vec<String> = Vec::new();
|
||||
if let Some(ref cap) = caption {
|
||||
tags.extend(hashtag::extract_hashtags(cap));
|
||||
}
|
||||
if let Some(ref csv) = hashtags_csv {
|
||||
for tag in csv.split(',') {
|
||||
let t = tag.trim().trim_start_matches('#').to_lowercase();
|
||||
if !t.is_empty() {
|
||||
tags.push(t);
|
||||
}
|
||||
}
|
||||
}
|
||||
tags.sort();
|
||||
tags.dedup();
|
||||
// Process hashtags from caption and explicit CSV, capped — see `normalize_tags`.
|
||||
let tags = normalize_tags(
|
||||
caption
|
||||
.as_deref()
|
||||
.map(hashtag::extract_hashtags)
|
||||
.unwrap_or_default(),
|
||||
hashtags_csv.as_deref(),
|
||||
);
|
||||
|
||||
// Quota accounting, the upload row, and its hashtag links must be atomic: a
|
||||
// crash between the bytes increment and the insert would permanently charge
|
||||
@@ -501,6 +557,57 @@ pub async fn edit_upload(
|
||||
return Err(AppError::Forbidden("Nur eigene Uploads bearbeiten.".into()));
|
||||
}
|
||||
|
||||
// This endpoint had no rate limit of any kind, while every other mutating route has one.
|
||||
let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await;
|
||||
let edit_rate_on = config::get_bool(&state.config_cache, "upload_edit_rate_enabled", true).await;
|
||||
if rate_limits_on && edit_rate_on {
|
||||
let edit_rate =
|
||||
config::get_i64(&state.config_cache, "upload_edit_rate_per_min", 30).await as usize;
|
||||
if let Err(retry_after_secs) = state.rate_limiter.check_with_retry(
|
||||
format!("upload_edit:{}", auth.user_id),
|
||||
edit_rate,
|
||||
Duration::from_secs(60),
|
||||
) {
|
||||
return Err(AppError::TooManyRequests(
|
||||
"Zu viele Änderungen. Bitte warte kurz.".into(),
|
||||
Some(retry_after_secs),
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// Validate to the same limits as the upload path. This route had none at all, so a caption
|
||||
// rejected at upload could be set here instead, and the tags went in raw — meaning `#Party`
|
||||
// via edit and `party` via upload became two different hashtag rows.
|
||||
if let Some(ref caption) = body.caption
|
||||
&& caption.chars().count() > MAX_CAPTION_LENGTH
|
||||
{
|
||||
return Err(AppError::BadRequest(format!(
|
||||
"Beschreibung ist zu lang. Maximum: {MAX_CAPTION_LENGTH} Zeichen."
|
||||
)));
|
||||
}
|
||||
let normalized_tags = body
|
||||
.hashtags
|
||||
.as_ref()
|
||||
.map(|tags| normalize_tags(tags.clone(), None));
|
||||
|
||||
// A PATCH that changes nothing must not retire the keepsake generation.
|
||||
//
|
||||
// `invalidate_and_arm` below ran unconditionally, outside both `if let Some(...)` guards, so
|
||||
// `PATCH {}` — which any authenticated guest can send in a loop against their own upload —
|
||||
// bumped export_epoch and armed a fresh pair of full-gallery export workers every time.
|
||||
// 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.
|
||||
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() {
|
||||
return Ok(StatusCode::OK);
|
||||
}
|
||||
|
||||
// Caption update + hashtag wipe-then-relink in one transaction, so a crash
|
||||
// mid-relink can't leave the upload with its hashtags stripped.
|
||||
//
|
||||
@@ -516,7 +623,7 @@ pub async fn edit_upload(
|
||||
if let Some(ref caption) = body.caption {
|
||||
Upload::update_caption(&mut *tx, upload_id, Some(caption)).await?;
|
||||
}
|
||||
if let Some(ref hashtags) = body.hashtags {
|
||||
if let Some(ref hashtags) = normalized_tags {
|
||||
Hashtag::unlink_all_from_upload(&mut *tx, upload_id).await?;
|
||||
for tag in hashtags {
|
||||
let h = Hashtag::upsert(&mut *tx, auth.event_id, tag).await?;
|
||||
@@ -1180,4 +1287,53 @@ mod tests {
|
||||
drop(TempFileGuard::new(p)); // must not panic
|
||||
}
|
||||
}
|
||||
|
||||
/// The CSV hashtag path had no length check at all and was upserted tag-by-tag INSIDE the
|
||||
/// commit transaction — which holds a FOR SHARE lock on the event row, so one request could
|
||||
/// stall every other upload behind tens of thousands of round trips.
|
||||
mod hashtag_caps {
|
||||
use super::super::{MAX_HASHTAGS_PER_UPLOAD, MAX_HASHTAG_LENGTH, normalize_tags};
|
||||
|
||||
#[test]
|
||||
fn a_huge_csv_is_capped_not_upserted_in_full() {
|
||||
let csv = (0..10_000)
|
||||
.map(|i| format!("tag{i}"))
|
||||
.collect::<Vec<_>>()
|
||||
.join(",");
|
||||
let tags = normalize_tags(vec![], Some(&csv));
|
||||
assert_eq!(tags.len(), MAX_HASHTAGS_PER_UPLOAD);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_overlong_tag_is_dropped_rather_than_stored() {
|
||||
let long = "a".repeat(MAX_HASHTAG_LENGTH + 1);
|
||||
let tags = normalize_tags(vec![], Some(&format!("ok,{long}")));
|
||||
assert_eq!(tags, vec!["ok"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn tags_are_normalised_and_deduped_across_both_sources() {
|
||||
// The upload path lowercased and stripped `#` while the edit path did not, so
|
||||
// `#Party` and `party` became two different hashtag rows. One helper, one rule.
|
||||
let tags = normalize_tags(vec!["party".into()], Some("#Party, PARTY ,tanz"));
|
||||
assert_eq!(tags, vec!["party", "tanz"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn truncation_keeps_a_stable_prefix_not_an_arbitrary_one() {
|
||||
// Sorted before truncation, so the same input always yields the same tags —
|
||||
// otherwise an edit could silently shuffle which 30 survived.
|
||||
let csv = "zulu,alpha,mike,bravo";
|
||||
assert_eq!(
|
||||
normalize_tags(vec![], Some(csv)),
|
||||
normalize_tags(vec![], Some("bravo,mike,alpha,zulu"))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_empty_or_absent_csv_yields_nothing() {
|
||||
assert!(normalize_tags(vec![], None).is_empty());
|
||||
assert!(normalize_tags(vec![], Some(",, ,")).is_empty());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user