diff --git a/backend/src/handlers/admin.rs b/backend/src/handlers/admin.rs index 5faa338..026de3c 100644 --- a/backend/src/handlers/admin.rs +++ b/backend/src/handlers/admin.rs @@ -115,9 +115,15 @@ pub async fn patch_config( // accept but that silently revert to the hardcoded default at read time // (get_usize/get_i64 can't parse negatives/NaN/fractionals). `compression_concurrency` // is intentionally absent — it's read once at boot, so a live edit was a no-op. + // The two size limits are bounded by what the router's `DefaultBodyLimit` can carry, NOT by + // a round number. They used to allow 1024 and 10240 MB against a 576 MiB body cap, so the + // admin dashboard's plain "Max. Videogröße (MB)" field could be set to a value that destroys + // every video above the cap — the limit trips mid-body, surfaces as a 400, and the client + // purges the blob as terminal. See `crate::MAX_CONFIGURABLE_UPLOAD_MB` for the full chain. + const MAX_SIZE_MB: f64 = crate::MAX_CONFIGURABLE_UPLOAD_MB as f64; const NUMERIC_SPECS: &[(&str, bool, f64, f64)] = &[ - ("max_image_size_mb", true, 1.0, 1024.0), - ("max_video_size_mb", true, 1.0, 10240.0), + ("max_image_size_mb", true, 1.0, MAX_SIZE_MB), + ("max_video_size_mb", true, 1.0, MAX_SIZE_MB), ("upload_rate_per_hour", true, 1.0, 100_000.0), ("feed_rate_per_min", true, 1.0, 100_000.0), ("export_rate_per_day", true, 1.0, 100_000.0), diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index a9798b5..14aea2a 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -25,10 +25,15 @@ const MAX_CAPTION_LENGTH: usize = 2000; /// `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; +pub(crate) 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; +pub(crate) const MAX_HASHTAGS_BYTES: usize = 4 * 1024; + +/// Byte ceiling for the `client_upload_id` field. 64 bytes fits a hyphenated UUID (36) with +/// room to spare; named rather than inline so the multipart-envelope test in `main.rs` can +/// account for every text field this handler will read. +pub(crate) const MAX_CLIENT_UPLOAD_ID_BYTES: usize = 64; /// 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 @@ -280,9 +285,24 @@ pub async fn upload( return Err(AppError::UploadsLocked("Uploads sind gesperrt.".into())); } - // Read config limits from DB - let max_image_mb: i64 = config::get_i64(&state.config_cache, "max_image_size_mb", 20).await; - let max_video_mb: i64 = config::get_i64(&state.config_cache, "max_video_size_mb", 500).await; + // Read config limits from DB. + // + // CLAMPED to what the router's `DefaultBodyLimit` can actually carry. `patch_config` now + // refuses a larger value, but that guard only covers values written THROUGH it: a config row + // stored before the bound existed, or edited straight into the table, would otherwise sail + // past it and hand the guest the worst failure in the app — the body limit tripping mid-upload, + // surfacing as a 400, and the client purging the blob as terminal. See + // `crate::MAX_CONFIGURABLE_UPLOAD_MB`. + // + // Clamping (rather than refusing the upload) is right here: the operator's intent was "allow + // bigger files", and the honest answer to an unsatisfiable limit is the largest one that + // works, applied consistently by both the streaming cap and the per-class check below. + let max_image_mb: i64 = config::get_i64(&state.config_cache, "max_image_size_mb", 20) + .await + .min(crate::MAX_CONFIGURABLE_UPLOAD_MB); + let max_video_mb: i64 = config::get_i64(&state.config_cache, "max_video_size_mb", 500) + .await + .min(crate::MAX_CONFIGURABLE_UPLOAD_MB); // The uploaded file is streamed straight to a temp file on disk (never buffered // whole in memory — a 500 MB video used to cost 500 MB of RAM per concurrent @@ -379,8 +399,8 @@ pub async fn upload( // `hashtags` were bounded by the helper for exactly this reason; this field // arrived later (migration 022) and missed it. // - // 64 bytes fits a hyphenated UUID (36) with room to spare. - let raw = read_text_field_bounded(field, 64).await?; + // See `MAX_CLIENT_UPLOAD_ID_BYTES`. + let raw = read_text_field_bounded(field, MAX_CLIENT_UPLOAD_ID_BYTES).await?; // A malformed key is not worth rejecting an upload over — the photo is the // thing the guest cares about. Drop the key and lose only the retry // protection, which is exactly where we were before it existed. diff --git a/backend/src/main.rs b/backend/src/main.rs index 1d4cd74..4f28926 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -19,7 +19,36 @@ use state::AppState; /// Hard HTTP body cap for the upload endpoint (576 MiB). Backstop against /// memory-exhaustion; precise per-class size limits are enforced in the handler. -const MAX_UPLOAD_BYTES: usize = 576 * 1024 * 1024; +pub(crate) const MAX_UPLOAD_BYTES: usize = 576 * 1024 * 1024; + +/// Largest per-file size limit (MB) an operator may configure for `max_image_size_mb` / +/// `max_video_size_mb`, derived from [`MAX_UPLOAD_BYTES`] rather than written down twice. +/// +/// This used to be enforced by a COMMENT — "if an admin raises max_video_size_mb above this, +/// bump MAX_UPLOAD_BYTES" — while `patch_config` happily accepted 10240 and the admin dashboard +/// rendered "Max. Videogröße (MB)" as a bare number field. Set it to 1000 and every video +/// between 576 MB and the new limit is destroyed, in a shape that is much worse than a refusal: +/// +/// * The body limit trips mid-upload, inside `field.chunk()`, so `stream_field_to_file` maps +/// it to `AppError::BadRequest` — a 400, not a 413 with a `quota_exceeded` code. +/// * `classifyUploadStatus` (frontend/src/lib/upload-queue.ts) puts every non-401/408/429 4xx +/// in the `terminal` bucket, and `isReversibleLock(400, 'bad_request')` is false — so the +/// queue DELETES the blob from IndexedDB and moves the row to `blocked`, which by design +/// offers no retry button. +/// * All of that after the guest has already pushed 600 MB over cellular, and the message they +/// get names a read failure rather than a limit. +/// +/// So the ceiling is enforced where the value is SET (`patch_config`) and again where it is READ +/// (`handlers::upload`), because a value stored before this bound existed — or written by hand +/// into the `config` table — would otherwise walk straight past the first check. +/// +/// The subtraction is the multipart envelope: the body carries the file PLUS the boundary +/// framing and the `caption` / `hashtags` / `client_upload_id` fields. 1 MiB is enormously more +/// than those can occupy (see the test below, which pins it against their actual caps) and costs +/// nothing — the alternative is a limit that is satisfiable in theory and off-by-a-header in +/// practice. +pub(crate) const MAX_CONFIGURABLE_UPLOAD_MB: i64 = + (MAX_UPLOAD_BYTES as i64 - 1024 * 1024) / (1024 * 1024); #[tokio::main] async fn main() -> Result<()> { @@ -118,7 +147,9 @@ async fn main() -> Result<()> { // the precise per-class limits from DB config (max_image/video_size_mb); this // layer just stops a multi-GB body from being buffered into memory before that // check runs. Sized generously above the default 500 MB video limit + multipart - // overhead — if an admin raises max_video_size_mb above this, bump MAX_UPLOAD_BYTES. + // overhead. The DB-configured limits can no longer exceed it: both are clamped to + // MAX_CONFIGURABLE_UPLOAD_MB at write time and at read time — see that constant + // for why a comment was not enough. .route( "/api/v1/upload", post(handlers::upload::upload).route_layer(DefaultBodyLimit::max(MAX_UPLOAD_BYTES)), @@ -467,3 +498,56 @@ async fn shutdown_signal() { std::process::exit(0); }); } + +#[cfg(test)] +mod tests { + use super::{MAX_CONFIGURABLE_UPLOAD_MB, MAX_UPLOAD_BYTES}; + use crate::handlers::upload::{ + MAX_CAPTION_BYTES, MAX_CLIENT_UPLOAD_ID_BYTES, MAX_HASHTAGS_BYTES, + }; + + /// Boundary lines, `Content-Disposition` / `Content-Type` headers and CRLFs for the four + /// fields the handler reads. A few hundred bytes in reality; 4 KiB is a deliberately fat + /// allowance so this test asserts the invariant rather than a precise byte count. + const MULTIPART_FRAMING_BYTES: usize = 4 * 1024; + + /// Both bounds on `MAX_CONFIGURABLE_UPLOAD_MB`, asserted at COMPILE time. + /// + /// `const _: () = assert!(...)` rather than a runtime `assert!`, matching the precedent in + /// `handlers::upload` and `services::compression`: every operand is a constant, so a + /// violation is a build failure rather than something that has to be run to be noticed. The + /// cost is that a const panic takes a static message — the reasoning lives here instead. + /// + /// UPPER: a file at the largest configurable limit must still fit inside the body limit the + /// router enforces, envelope included. If it does not, an operator can set a limit the + /// handler accepts and the router then refuses MID-BODY — a 400 that the upload queue + /// classifies as terminal and answers by deleting the guest's only copy of the photo. See + /// `MAX_CONFIGURABLE_UPLOAD_MB`. Written against the field caps rather than a hardcoded + /// number, so raising `MAX_CAPTION_LENGTH` (or adding another text field to the envelope) + /// fails HERE instead of silently eating the margin. + /// + /// LOWER: the ceiling must not be so conservative that it forbids the shipped default. + /// `max_video_size_mb` is seeded at 500 (migration 005), so a bound below that would clamp + /// every video upload on a stock install and reject the stock config through `patch_config`. + #[test] + fn the_configurable_ceiling_is_bounded_at_both_ends() { + const FILE: usize = MAX_CONFIGURABLE_UPLOAD_MB as usize * 1024 * 1024; + const ENVELOPE: usize = MAX_CAPTION_BYTES + + MAX_HASHTAGS_BYTES + + MAX_CLIENT_UPLOAD_ID_BYTES + + MULTIPART_FRAMING_BYTES; + const _: () = { + assert!( + FILE + ENVELOPE <= MAX_UPLOAD_BYTES, + "a file at MAX_CONFIGURABLE_UPLOAD_MB plus its multipart envelope exceeds \ + MAX_UPLOAD_BYTES — an operator could configure a limit that DESTROYS uploads \ + (400 mid-body, blob purged as terminal) instead of refusing them" + ); + assert!( + MAX_CONFIGURABLE_UPLOAD_MB >= 500, + "MAX_CONFIGURABLE_UPLOAD_MB must clear the 500 MB max_video_size_mb default \ + seeded by migration 005, or a stock install clamps every video upload" + ); + }; + } +} diff --git a/frontend/src/routes/admin/+page.svelte b/frontend/src/routes/admin/+page.svelte index 9dc93a8..3c38b82 100644 --- a/frontend/src/routes/admin/+page.svelte +++ b/frontend/src/routes/admin/+page.svelte @@ -80,8 +80,25 @@ { title: 'Limits & Größen', fields: [ - { key: 'max_image_size_mb', label: 'Max. Bildgröße (MB)', kind: 'number' }, - { key: 'max_video_size_mb', label: 'Max. Videogröße (MB)', kind: 'number' } + // The 575 MB ceiling is not arbitrary and it is not a policy choice: it is what the + // upload route's HTTP body limit can carry (backend MAX_CONFIGURABLE_UPLOAD_MB, + // derived from MAX_UPLOAD_BYTES minus the multipart envelope). Above it the body + // limit trips MID-UPLOAD, which the upload queue reads as a terminal 4xx and + // answers by deleting the guest's photo — so the backend refuses the value, and + // these hints exist so the operator learns the bound from the form rather than + // from an error after typing 1000. + { + key: 'max_image_size_mb', + label: 'Max. Bildgröße (MB)', + kind: 'number', + hint: 'Maximal 575 — darüber kann der Server den Upload nicht mehr entgegennehmen.' + }, + { + key: 'max_video_size_mb', + label: 'Max. Videogröße (MB)', + kind: 'number', + hint: 'Maximal 575 — darüber kann der Server den Upload nicht mehr entgegennehmen.' + } // compression_concurrency is set via COMPRESSION_WORKER_CONCURRENCY at // boot, not live — omitted so it isn't a dead no-op control. ] diff --git a/frontend/src/routes/upload/+page.svelte b/frontend/src/routes/upload/+page.svelte index 2240d26..3734326 100644 --- a/frontend/src/routes/upload/+page.svelte +++ b/frontend/src/routes/upload/+page.svelte @@ -26,11 +26,19 @@ const MAX_CAPTION_LENGTH = 2000; - // Mirrors MAX_UPLOAD_BYTES in backend/src/main.rs — the axum body limit, which is a - // BOOT CONSTANT rather than an admin-tunable value, so checking it here cannot drift - // out of sync with the dashboard the way max_image_size_mb / max_video_size_mb would. - // Anything above this is refused by the server no matter how the event is configured. - const HARD_MAX_UPLOAD_BYTES = 576 * 1024 * 1024; + // Mirrors MAX_CONFIGURABLE_UPLOAD_MB in backend/src/main.rs — the largest per-file limit an + // operator can configure, itself derived from the axum body limit. Both are BOOT CONSTANTS + // rather than admin-tunable values, so checking it here cannot drift out of sync with the + // dashboard the way max_image_size_mb / max_video_size_mb would. Anything above this is + // refused by the server no matter how the event is configured. + // + // 575 MB, not the raw 576 MiB body limit this used to mirror. The gap is the multipart + // envelope, and it is the difference between two failure shapes: at or below the file + // ceiling the UPLOAD HANDLER rejects with "Datei ist zu groß. Maximum: 575 MB." — a clean, + // self-explaining 400 — whereas past the body limit axum aborts the request MID-STREAM and + // the handler answers with a read error instead. Rejecting here at the lower of the two + // means a guest never reaches the confusing one, and never pushes 575 MB to find out. + const HARD_MAX_UPLOAD_BYTES = 575 * 1024 * 1024; /** * Reject files the server is certain to refuse, BEFORE any bytes leave the phone.