From 42416d76e250732ee8a093f678dccf4c2ba50e67 Mon Sep 17 00:00:00 2001 From: fabi Date: Mon, 27 Jul 2026 21:21:18 +0200 Subject: [PATCH] fix(media): close the percent-escape bypass of the media gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/media/%70reviews/{id}.jpg` served a taken-down photo to anyone, unauthenticated. Verified against the running stack: the literal path 404s, the escaped one returned 200 with the full image. Same for displays, thumbnails and originals, and any escaped byte in any position works. Cause: the block was four `nest_service("/media/previews", 404)` route matches sitting above a `ServeDir` on `/media`. axum matches on the RAW path (matchit does no percent-decoding), while `ServeDir` percent-decodes when it resolves the file. So `%70reviews` missed every blocker, fell through to the ServeDir, and was decoded back to `previews/` on disk — reaching the bytes with no soft-delete and no ban-hide check. That defeats a host takedown, which is the entire point of the gate. Remove the `/media` route tree outright instead of racing the decoder. Nothing needs it: every media URL the backend emits is already a gated `/api/v1/upload/{id}/{original,preview,display,thumbnail}` alias (handlers::feed), the frontend contains zero `/media/` references, and the `/media` in config.rs/disk.rs is the filesystem path while `media/` in export.rs is a path inside the zip. `/media/**` now 404s regardless of encoding. The route's own comment already said it "serves nothing" — it wasn't a backstop, it was the vector. Caddy keeps proxying /media/* deliberately: the app 404s it, and forwarding means the e2e gating specs exercise the app's refusal exactly as production would rather than being masked by the SvelteKit 404 page. Extend the gating spec with the encoded variants — asserting only the literal spelling is what let this sit undetected. Co-Authored-By: Claude Opus 5 (1M context) --- Caddyfile | 15 +++++-- backend/src/main.rs | 45 +++++++------------ e2e/specs/07-adversarial/media-gating.spec.ts | 17 +++++++ 3 files changed, 44 insertions(+), 33 deletions(-) diff --git a/Caddyfile b/Caddyfile index 970381f..138ea14 100644 --- a/Caddyfile +++ b/Caddyfile @@ -31,9 +31,9 @@ @hashed_assets path_regexp hashed /_app/immutable/.*\.[a-f0-9]{8,}\.(js|css|woff2)$ header @hashed_assets Cache-Control "public, max-age=31536000, immutable" - # Preview/thumbnail images. These are now served by the app through a - # visibility-checked alias (/api/v1/upload/{id}/{preview,thumbnail}) so moderation - # can revoke access; direct /media/previews|thumbnails is 404-blocked at the app. + # Preview/thumbnail images. These are served by the app through a visibility-checked + # alias (/api/v1/upload/{id}/{preview,thumbnail}) so moderation can revoke access; + # the app serves no /media route at all, so there is no direct path to the bytes. # Privately cacheable for a short window (the app sets the same header; this is the # edge carve-out from the blanket no-store below). Kept short so a moderated image # stops being served to a direct-URL holder promptly. @@ -47,7 +47,14 @@ } header @api Cache-Control "no-store" - # Route API and media requests to the Rust backend + # Route API and media requests to the Rust backend. + # + # The app serves no /media route at all (see the note in backend/src/main.rs) — media + # bytes are reachable only through the visibility-checked /api/v1/upload aliases, so + # /media/* forwards to a plain 404. The proxy line is kept deliberately: it means the + # edge faithfully hands /media to the app, so if a future change ever re-introduces a + # static media route the e2e gating specs see it here exactly as production would, + # instead of being masked by the SvelteKit 404 page. reverse_proxy /api/* app:3000 reverse_proxy /media/* app:3000 diff --git a/backend/src/main.rs b/backend/src/main.rs index fd5b3a8..e7327e1 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -2,7 +2,6 @@ use anyhow::Result; use axum::Router; use axum::extract::DefaultBodyLimit; use axum::routing::{delete, get, patch, post}; -use tower_http::services::ServeDir; use tower_http::trace::TraceLayer; use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt}; @@ -229,37 +228,25 @@ async fn main() -> Result<()> { api }; - // Serve media files from disk - let media_service = ServeDir::new(&config.media_path); - + // NOTE: media is deliberately NOT served over HTTP. + // + // Files live under `media_path` so the compression worker and the export job can read + // them off disk, but nothing may pull them straight from `/media/**` — that bypasses + // the visibility checks (soft-delete + ban-hide) that make a host takedown stick. + // Every legitimate fetch goes through `/api/v1/upload/{id}/{original,preview,display, + // thumbnail}`, which filter via `find_visible_media`; those are the only media URLs the + // backend ever emits (see `handlers::feed`). + // + // This used to be a `ServeDir` on `/media` with four `nest_service` blockers on the + // subtrees above it. That was bypassable: axum routes on the RAW path while `ServeDir` + // percent-decodes afterwards, so `/media/%70reviews/{id}.jpg` missed every blocker, + // fell through to the `ServeDir`, and was decoded back to `previews/` on disk — serving + // a taken-down photo to anyone, unauthenticated. Any single escaped byte worked, in all + // four subtrees. Deleting the route removes the vector outright rather than racing the + // decoder; `/media/**` now 404s regardless of encoding. let router = Router::new() .route("/health", get(|| async { "ok" })) .merge(api) - // Block direct HTTP access to ALL media subtrees. The files live under - // `media_path` (so the compression worker and export can read them off disk) but - // must NOT be pullable straight from `/media/**` — that bypasses the visibility - // checks (soft-delete + ban-hide) in the gated handlers. Every legitimate fetch - // goes through `/api/v1/upload/{id}/{original,preview,thumbnail}`, which filter - // via `find_visible_media`. The more specific nests take precedence over the - // `/media` ServeDir below (which, with all three subtrees blocked, now serves - // nothing — kept as a backstop). - .nest_service( - "/media/originals", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/previews", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/displays", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service( - "/media/thumbnails", - get(|| async { axum::http::StatusCode::NOT_FOUND }), - ) - .nest_service("/media", media_service) .layer(TraceLayer::new_for_http()) .with_state(state); diff --git a/e2e/specs/07-adversarial/media-gating.spec.ts b/e2e/specs/07-adversarial/media-gating.spec.ts index 29f1f16..d9b19c7 100644 --- a/e2e/specs/07-adversarial/media-gating.spec.ts +++ b/e2e/specs/07-adversarial/media-gating.spec.ts @@ -45,6 +45,23 @@ test.describe('Media gating — moderation revokes preview access (F2)', () => { const direct = await fetch(`${BASE}/media/previews/${id}.jpg`); expect(direct.status, 'direct /media/previews must be blocked').toBe(404); + // …and it must stay blocked under percent-encoding. The block used to be four + // `nest_service("/media/previews", 404)` route matches sitting above a `/media` + // ServeDir. axum routes on the RAW path while ServeDir percent-decodes afterwards, so + // ONE escaped byte (`%70` = `p`) missed every blocker, fell through to the ServeDir, + // and was decoded back to `previews/` on disk — serving the bytes unauthenticated. + // Asserting only the literal spelling is what let that sit here undetected. + for (const variant of [ + `/media/%70reviews/${id}.jpg`, // p + `/media/p%72eviews/${id}.jpg`, // r — any position works + `/media/%64isplays/${id}.jpg`, // d + `/media/%74humbnails/${id}.jpg`, // t + `/media/%6Friginals/${id}.jpg`, // o + ]) { + const res = await fetch(`${BASE}${variant}`, { redirect: 'manual' }); + expect(res.status, `${variant} must not bypass the media block`).toBe(404); + } + // Host deletes the upload → the preview must stop being served. const del = await fetch(`${BASE}/api/v1/host/upload/${id}`, { method: 'DELETE',