From 93bb156fbaea5dfb4aff4404f5c92c2d29c26ecd Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Mon, 22 Jun 2026 22:28:16 +0200 Subject: [PATCH] fix(admin-security): close the bearer-cookie ride; require session for admin gate (0.87.10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to 0.87.2 + 0.87.3 from the adversarial review: 1. **CSRF cookie-ride.** Bearer-bypass branch checked Authorization header presence only — an attacker page could mint `Authorization: Bearer junk` and ride the still-attached session cookie. Bypass now requires bearer AND no cookie. Drive the cookie-name comparison off `SESSION_COOKIE_NAME` and split on `=` (not prefix). 2. **`require_can_edit` admin path open to bearer.** Now reads admin authority off an `Option` extractor — None on bearer-only requests. Owner-match path still accepts bearer; admin path is cookie-gated. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/api/mangas.rs | 52 ++++++++++++------ backend/src/app.rs | 40 ++++++++++---- backend/tests/api_admin_crawler.rs | 75 ++++++++++++++++++++++++-- backend/tests/api_mangas_metadata.rs | 79 ++++++++++++++++++++++++++++ frontend/package.json | 2 +- 7 files changed, 220 insertions(+), 32 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 777d35d..8fe3158 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1517,7 +1517,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.87.9" +version = "0.87.10" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index c2af7b3..7552fdd 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.87.9" +version = "0.87.10" edition = "2021" default-run = "mangalord" diff --git a/backend/src/api/mangas.rs b/backend/src/api/mangas.rs index 10ea644..2127f16 100644 --- a/backend/src/api/mangas.rs +++ b/backend/src/api/mangas.rs @@ -8,7 +8,7 @@ use uuid::Uuid; use crate::api::pagination::PagedResponse; use crate::app::AppState; -use crate::auth::extractor::CurrentUser; +use crate::auth::extractor::{CurrentSessionUser, CurrentUser}; use crate::domain::manga::{MangaCard, MangaDetail, MangaPatch, NewManga}; use crate::domain::patch::Patch; use crate::domain::tag::TagRef; @@ -229,13 +229,14 @@ async fn create( async fn update( State(state): State, CurrentUser(user): CurrentUser, + session: Option, Path(id): Path, Json(patch): Json, ) -> AppResult> { if !repo::manga::exists(&state.db, id).await? { return Err(AppError::NotFound); } - require_can_edit(&state, id, user.id, user.is_admin).await?; + require_can_edit(&state, id, user.id, admin_via_session(&session)).await?; if let Some(ref status) = patch.status { let trimmed = status.trim(); @@ -300,13 +301,14 @@ async fn update( async fn put_cover( State(state): State, CurrentUser(user): CurrentUser, + session: Option, Path(id): Path, mut multipart: Multipart, ) -> AppResult> { if !repo::manga::exists(&state.db, id).await? { return Err(AppError::NotFound); } - require_can_edit(&state, id, user.id, user.is_admin).await?; + require_can_edit(&state, id, user.id, admin_via_session(&session)).await?; let mut cover: Option = None; while let Some(field) = next_field(&mut multipart).await? { @@ -349,12 +351,13 @@ async fn put_cover( async fn delete_cover( State(state): State, CurrentUser(user): CurrentUser, + session: Option, Path(id): Path, ) -> AppResult> { if !repo::manga::exists(&state.db, id).await? { return Err(AppError::NotFound); } - require_can_edit(&state, id, user.id, user.is_admin).await?; + require_can_edit(&state, id, user.id, admin_via_session(&session)).await?; if let Some(key) = repo::manga::get(&state.db, id).await?.cover_image_path { match state.storage.delete(&key).await { Ok(()) | Err(StorageError::NotFound) => {} @@ -468,15 +471,23 @@ fn validate_new_manga(input: &NewManga) -> AppResult<()> { /// surfaces as `NotFound`, not `Forbidden`). /// /// Rule: a non-NULL `uploaded_by` must match the current user, OR the -/// caller is an admin. Rows with `uploaded_by IS NULL` (crawler-imported -/// + legacy pre-0011) are admin-only. +/// caller is an admin **via a session cookie**. Rows with +/// `uploaded_by IS NULL` (crawler-imported + legacy pre-0011) are +/// admin-via-session-only. /// -/// Why: every crawler row has NULL `uploaded_by` (see -/// `repo::crawler::upsert_manga`). The earlier "any signed-in user can -/// edit a NULL row" rule meant `register → PATCH /api/v1/mangas/` -/// rewrote the catalog for free, and `delete_cover` removed blobs from -/// `storage`. Now that the admin role exists (migration 0018), gate -/// crawler/legacy rows on it. +/// Why "via session": [`crate::auth::extractor`] documents that admin +/// authority is unreachable via bearer tokens — a leaked bot token +/// must not yield admin powers. Keep that invariant by reading the +/// `is_admin_session` flag from the optional `CurrentSessionUser` +/// extractor (`None` ⇒ caller authenticated via bearer ⇒ admin path +/// is closed) rather than from `User.is_admin` (which is true whichever +/// way the user authenticated). +/// +/// Why NULL is admin-only: every crawler row has NULL `uploaded_by` +/// (see `repo::crawler::upsert_manga`). The earlier "any signed-in +/// user can edit a NULL row" rule meant `register → PATCH +/// /api/v1/mangas/` rewrote the catalog for free, and +/// `delete_cover` removed blobs from `storage`. /// /// Returns `Forbidden` (not `NotFound`) on owner mismatch — mangas /// are listable via `GET /mangas`, so existence isn't a secret and @@ -488,17 +499,28 @@ async fn require_can_edit( state: &AppState, manga_id: Uuid, user_id: Uuid, - is_admin: bool, + is_admin_session: bool, ) -> AppResult<()> { match repo::manga::uploaded_by(&state.db, manga_id).await? { Some(owner) if owner == user_id => Ok(()), - Some(_) if is_admin => Ok(()), + Some(_) if is_admin_session => Ok(()), Some(_) => Err(AppError::Forbidden), - None if is_admin => Ok(()), + None if is_admin_session => Ok(()), None => Err(AppError::Forbidden), } } +/// True iff the caller authenticated via session cookie AND is an admin. +/// Used to gate the admin-only branches of [`require_can_edit`]. The +/// `Option` extractor returns `None` on a bearer-only +/// request, so admin authority over bot tokens never composes here. +fn admin_via_session(session: &Option) -> bool { + session + .as_ref() + .map(|CurrentSessionUser(u)| u.is_admin) + .unwrap_or(false) +} + async fn validate_genre_ids(state: &AppState, ids: &[Uuid]) -> AppResult<()> { if ids.is_empty() { return Ok(()); diff --git a/backend/src/app.rs b/backend/src/app.rs index 559d4f5..579ac0e 100644 --- a/backend/src/app.rs +++ b/backend/src/app.rs @@ -1089,27 +1089,45 @@ async fn admin_csrf_guard( return Ok(next.run(req).await); } let headers = req.headers(); - // Bearer-token requests are non-browser bot callers and bypass the - // gate entirely (the Authorization header is never set by an - // attacker page). + + // Detect both auth modes BEFORE choosing a bypass. The previous + // version short-circuited on "is_bearer" regardless of cookie state, + // which let an attacker page do a credentialed cross-site POST with + // a forged `Authorization: Bearer junk` header: the header existed, + // CSRF bypassed, then the auth extractor authenticated the victim + // via the session cookie. Cookie precedence here re-anchors the + // gate to the actual authority being ridden. + // Drive the parse off the auth-module constant so a rename of + // `SESSION_COOKIE_NAME` propagates here instead of silently + // reopening the cookie-ride. Split on `=` (not a prefix match) so + // an attacker can't shadow with `mangalord_session_x=` etc. + let has_session_cookie = headers + .get(axum::http::header::COOKIE) + .and_then(|v| v.to_str().ok()) + .is_some_and(|raw| { + raw.split(';').any(|p| { + p.trim_start().split('=').next() + == Some(crate::auth::extractor::SESSION_COOKIE_NAME) + }) + }); + + // Bearer-token-only requests are bot callers and bypass the gate — + // a browser can't set Authorization on a cross-site POST. But ONLY + // when no session cookie is also attached; with both present we + // must treat the request as cookie-auth to defeat the cookie-ride. let is_bearer = headers .get(axum::http::header::AUTHORIZATION) .and_then(|v| v.to_str().ok()) .is_some_and(|v| v.trim_start().to_ascii_lowercase().starts_with("bearer ")); - if is_bearer { + if is_bearer && !has_session_cookie { return Ok(next.run(req).await); } - // No session cookie either → let the auth extractor return a clean 401 + + // No auth context at all → let the auth extractor return a clean 401 // ("log in first"). A no-auth request can't be a CSRF vector — there's // no authority to ride. Without this, an anonymous curl POST to an // admin endpoint surfaces 403 with our CSRF message instead of the // expected 401, which is confusing for both operators and tests. - let has_session_cookie = headers - .get(axum::http::header::COOKIE) - .and_then(|v| v.to_str().ok()) - .is_some_and(|raw| raw.split(';').any(|p| { - p.trim_start().starts_with("mangalord_session=") - })); if !has_session_cookie { return Ok(next.run(req).await); } diff --git a/backend/tests/api_admin_crawler.rs b/backend/tests/api_admin_crawler.rs index ef9f44a..4ff3972 100644 --- a/backend/tests/api_admin_crawler.rs +++ b/backend/tests/api_admin_crawler.rs @@ -362,14 +362,12 @@ async fn csrf_rejects_cookie_auth_without_origin_or_referer(pool: PgPool) { // ("curl bypass"). It's now refused: an extension or no-referrer-policy // page can produce exactly this shape and would otherwise be a CSRF // vector. Real curl/script callers should authenticate with a bearer - // token instead — see `csrf_allows_bearer_token_without_origin`. + // token instead — see `csrf_bearer_only_request_bypasses_gate`. let h = harness_with_admin_origins( pool.clone(), vec!["http://localhost:3000".to_string()], ); let cookie = seed_admin(&pool, &h.app).await; - // `post_json_with_cookie_origin(..., None, None)` builds the exact "no - // Origin, no Referer" cookie-auth shape this test pins. let resp = h .app .clone() @@ -385,6 +383,77 @@ async fn csrf_rejects_cookie_auth_without_origin_or_referer(pool: PgPool) { assert_eq!(resp.status(), StatusCode::FORBIDDEN); } +#[sqlx::test(migrations = "./migrations")] +async fn csrf_bearer_only_request_bypasses_gate(pool: PgPool) { + // Bearer-only (no session cookie, no Origin) bypasses the CSRF gate + // because a browser can't set `Authorization` on a cross-site POST. + // RequireAdmin then rejects bot tokens for admin endpoints — we + // expect 401, not the CSRF 403. The path proves the bypass branch + // is reachable for legitimate bot callers. + let h = harness_with_admin_origins( + pool.clone(), + vec!["http://localhost:3000".to_string()], + ); + let req = axum::http::Request::builder() + .method("POST") + .uri("/api/v1/admin/crawler/session/clear-expired") + .header(axum::http::header::CONTENT_TYPE, "application/json") + .header(axum::http::header::AUTHORIZATION, "Bearer fake-bot-token") + .body(axum::body::Body::from("{}")) + .unwrap(); + let resp = h.app.clone().oneshot(req).await.unwrap(); + // 401 from RequireAdmin (bad/non-admin token), NOT 403 from CSRF. + // If the CSRF gate had fired, we'd get FORBIDDEN. + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); +} + +#[sqlx::test(migrations = "./migrations")] +async fn csrf_bearer_plus_cookie_still_enforces_gate(pool: PgPool) { + // The cookie-ride vector: an attacker page mints `Authorization: + // Bearer junk` on a cross-site credentialed POST. The cookie is + // attached automatically by the browser; the bearer header alone + // used to bypass CSRF, then `CurrentUser` authenticated the victim + // via the cookie. Cookie precedence here means: as soon as a + // session cookie is present, the CSRF gate fires regardless of + // Authorization — the bearer can't whitelist the cookie ride. + let h = harness_with_admin_origins( + pool.clone(), + vec!["http://localhost:3000".to_string()], + ); + let cookie = seed_admin(&pool, &h.app).await; + let req = axum::http::Request::builder() + .method("POST") + .uri("/api/v1/admin/crawler/session/clear-expired") + .header(axum::http::header::CONTENT_TYPE, "application/json") + .header(axum::http::header::COOKIE, cookie) + .header(axum::http::header::AUTHORIZATION, "Bearer junk") + .header(axum::http::header::ORIGIN, "https://evil.example.com") + .body(axum::body::Body::from("{}")) + .unwrap(); + let resp = h.app.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); +} + +#[sqlx::test(migrations = "./migrations")] +async fn csrf_no_auth_falls_through_to_401(pool: PgPool) { + // No cookie, no bearer — there's no authority to ride, so the CSRF + // gate yields and lets `RequireAdmin` return the more meaningful 401. + // Pins the carve-out that keeps anonymous curl-against-admin from + // 403-via-CSRF (which would mislead operators about what's blocking). + let h = harness_with_admin_origins( + pool.clone(), + vec!["http://localhost:3000".to_string()], + ); + let req = axum::http::Request::builder() + .method("POST") + .uri("/api/v1/admin/crawler/session/clear-expired") + .header(axum::http::header::CONTENT_TYPE, "application/json") + .body(axum::body::Body::from("{}")) + .unwrap(); + let resp = h.app.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); +} + #[sqlx::test(migrations = "./migrations")] async fn csrf_falls_back_to_referer_when_origin_missing(pool: PgPool) { diff --git a/backend/tests/api_mangas_metadata.rs b/backend/tests/api_mangas_metadata.rs index 13db87c..8673b56 100644 --- a/backend/tests/api_mangas_metadata.rs +++ b/backend/tests/api_mangas_metadata.rs @@ -642,6 +642,85 @@ async fn patch_null_uploader_rejected_for_non_admin(pool: PgPool) { assert_eq!(resp.status(), StatusCode::FORBIDDEN); } +/// The original uploader-of-record (when one exists) must keep editing +/// their own row even after admin gating tightens, so a legitimate +/// uploader on the same auth surface isn't locked out alongside drive-by +/// attackers. Re-runs the patch as the original cookie. +#[sqlx::test(migrations = "./migrations")] +async fn patch_owner_can_still_edit_after_admin_gate(pool: PgPool) { + let h = common::harness(pool.clone()); + let (_, cookie) = common::register_user(&h.app).await; + let created = create_manga(&h.app, &cookie, json!({ "title": "Owner" })).await; + let id = id_of(&created); + // Row keeps uploaded_by set to the creating user (no NULL twiddle). + let resp = h + .app + .oneshot(common::patch_json_with_cookie( + &format!("/api/v1/mangas/{id}"), + json!({ "status": "completed" }), + &cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); +} + +/// Cover endpoints share `require_can_edit` with PATCH but write blob +/// state to `storage`, so a regression that lets non-admins through +/// `put_cover` would let any registered user replace a crawler-imported +/// cover. `delete_cover` would wipe blobs entirely. Tests both, mirroring +/// the PATCH guard tests above. +#[sqlx::test(migrations = "./migrations")] +async fn put_cover_null_uploader_rejected_for_non_admin(pool: PgPool) { + let h = common::harness(pool.clone()); + let (_, cookie) = common::register_user(&h.app).await; + let created = create_manga(&h.app, &cookie, json!({ "title": "Catalog" })).await; + let id = id_of(&created); + sqlx::query("UPDATE mangas SET uploaded_by = NULL WHERE id = $1") + .bind(id) + .execute(&pool) + .await + .unwrap(); + let (_, other_cookie) = common::register_user(&h.app).await; + // Tiny PNG `cover` part is what put_cover expects to look at. + let png = common::fake_png_bytes(); + let mp = common::MultipartBuilder::new() + .add_file("cover", "cover.png", "image/png", &png); + let resp = h + .app + .oneshot(common::put_multipart_with_cookie( + &format!("/api/v1/mangas/{id}/cover"), + mp, + &other_cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); +} + +#[sqlx::test(migrations = "./migrations")] +async fn delete_cover_null_uploader_rejected_for_non_admin(pool: PgPool) { + let h = common::harness(pool.clone()); + let (_, cookie) = common::register_user(&h.app).await; + let created = create_manga(&h.app, &cookie, json!({ "title": "Catalog" })).await; + let id = id_of(&created); + sqlx::query("UPDATE mangas SET uploaded_by = NULL WHERE id = $1") + .bind(id) + .execute(&pool) + .await + .unwrap(); + let (_, other_cookie) = common::register_user(&h.app).await; + let resp = h + .app + .oneshot(common::delete_with_cookie( + &format!("/api/v1/mangas/{id}/cover"), + &other_cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); +} + /// Admins CAN edit NULL-`uploaded_by` rows, since they're the operators /// curating the crawled catalog. Mirror of the test above with the /// caller promoted. diff --git a/frontend/package.json b/frontend/package.json index 3f9e0cc..b8be20f 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.87.9", + "version": "0.87.10", "private": true, "type": "module", "scripts": {