diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 949e167..6bf8ee8 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1517,7 +1517,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.87.18" +version = "0.87.19" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index f99d493..06ee234 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.87.18" +version = "0.87.19" edition = "2021" default-run = "mangalord" diff --git a/backend/src/app.rs b/backend/src/app.rs index 2eb63ad..54ba854 100644 --- a/backend/src/app.rs +++ b/backend/src/app.rs @@ -1058,9 +1058,17 @@ const ADMIN_PATH_PREFIX: &str = "/api/v1/admin/"; /// the configured allowlist. Safe methods (`GET`/`HEAD`/`OPTIONS`) are /// always allowed. /// -/// **Bearer-token requests** (`Authorization: Bearer …`) are bot API -/// callers — they can't be a CSRF vector because the browser never -/// attaches that header automatically. Skip the check for them. +/// **Bearer-token-only requests** (`Authorization: Bearer …` with NO +/// session cookie) are bot API callers — they can't be a CSRF vector +/// because the browser never attaches the Authorization header +/// automatically. Skip the check for them. +/// +/// **Bearer + cookie (the "cookie-ride")** is treated as cookie-auth. +/// An attacker page can mint `Authorization: Bearer junk` on a +/// credentialed cross-site POST; the cookie carries the actual +/// authority. Closing this hole means cookie precedence: as soon as a +/// session cookie is present, the CSRF gate fires regardless of the +/// Authorization header. (0.87.10 closed a 0.87.2 regression here.) /// /// **Cookie-auth requests** must: /// * be from an allowed origin (`Origin` then `Referer`), AND @@ -1073,7 +1081,11 @@ const ADMIN_PATH_PREFIX: &str = "/api/v1/admin/"; /// — this used to silently let everything through, so an operator who /// forgot to set `ADMIN_ALLOWED_ORIGINS` shipped an unguarded admin /// surface. Operators on a pure-bot-token deploy aren't impacted (their -/// requests carry `Authorization: Bearer …` and skip the gate above). +/// requests carry only `Authorization: Bearer …` with no session cookie +/// and skip the gate via the bearer-only branch above). +/// +/// **No auth at all** (no cookie, no bearer): bypass the CSRF gate so +/// the auth extractor returns a clean 401 instead of a confusing 403. async fn admin_csrf_guard( State(state): State, req: Request, diff --git a/backend/tests/api_mangas_metadata.rs b/backend/tests/api_mangas_metadata.rs index 8673b56..4592277 100644 --- a/backend/tests/api_mangas_metadata.rs +++ b/backend/tests/api_mangas_metadata.rs @@ -757,3 +757,125 @@ async fn patch_null_uploader_allowed_for_admin(pool: PgPool) { .unwrap(); assert_eq!(resp.status(), StatusCode::OK); } + +/// 0.87.10 split `require_can_edit` so admin authority must come via a +/// session cookie — bearer tokens (bot callers) DO NOT carry admin +/// powers per the auth-module contract. A bearer-authed admin trying +/// to PATCH/PUT-cover/DELETE-cover a NULL-`uploaded_by` row must be +/// rejected. Mechanical revert of `admin_via_session(&session)` to +/// `user.is_admin` would let bearer-authed admins through and pass +/// the 0.87.10 cookie tests; this is the missing axis the rereview +/// asked for. +#[sqlx::test(migrations = "./migrations")] +async fn bearer_authed_admin_cannot_edit_null_uploader(pool: PgPool) { + let h = common::harness(pool.clone()); + + // Catalog row with NULL uploaded_by (the crawler shape). + let (_, owner_cookie) = common::register_user(&h.app).await; + let created = create_manga(&h.app, &owner_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(); + + // Register a user, promote to admin, mint a bearer token from + // their session. + let (admin_name, admin_cookie) = common::register_user(&h.app).await; + let admin = mangalord::repo::user::find_by_username(&pool, &admin_name) + .await + .unwrap() + .unwrap(); + mangalord::repo::user::set_is_admin_unchecked(&pool, admin.id, true) + .await + .unwrap(); + let mint_resp = h + .app + .clone() + .oneshot(common::post_json_with_cookie( + "/api/v1/auth/tokens", + json!({ "name": "ci-bot" }), + &admin_cookie, + )) + .await + .unwrap(); + assert_eq!(mint_resp.status(), StatusCode::CREATED); + let bearer = common::body_json(mint_resp).await["bearer"] + .as_str() + .unwrap() + .to_string(); + + // PATCH via bearer — must 403 even though the user behind the + // token IS an admin. CurrentUser admits the call, but + // `admin_via_session` reads off `Option` + // which is None on a bearer request. + let patch_resp = h + .app + .clone() + .oneshot({ + let body = json!({ "status": "completed" }); + axum::http::Request::builder() + .method("PATCH") + .uri(format!("/api/v1/mangas/{id}")) + .header(axum::http::header::CONTENT_TYPE, "application/json") + .header(axum::http::header::AUTHORIZATION, format!("Bearer {bearer}")) + .body(axum::body::Body::from(body.to_string())) + .unwrap() + }) + .await + .unwrap(); + assert_eq!( + patch_resp.status(), + StatusCode::FORBIDDEN, + "bearer-authed admin must NOT be able to PATCH a NULL-uploader manga" + ); + + // PUT cover via bearer — same gate, same answer. + let png = common::fake_png_bytes(); + let (boundary, body) = common::MultipartBuilder::new() + .add_file("cover", "cover.png", "image/png", &png) + .finalize(); + let put_resp = h + .app + .clone() + .oneshot({ + axum::http::Request::builder() + .method("PUT") + .uri(format!("/api/v1/mangas/{id}/cover")) + .header( + axum::http::header::CONTENT_TYPE, + format!("multipart/form-data; boundary={boundary}"), + ) + .header(axum::http::header::AUTHORIZATION, format!("Bearer {bearer}")) + .body(axum::body::Body::from(body)) + .unwrap() + }) + .await + .unwrap(); + assert_eq!( + put_resp.status(), + StatusCode::FORBIDDEN, + "bearer-authed admin must NOT be able to put_cover on a NULL-uploader manga" + ); + + // DELETE cover via bearer. + let delete_resp = h + .app + .oneshot( + axum::http::Request::builder() + .method("DELETE") + .uri(format!("/api/v1/mangas/{id}/cover")) + .header(axum::http::header::AUTHORIZATION, format!("Bearer {bearer}")) + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + delete_resp.status(), + StatusCode::FORBIDDEN, + "bearer-authed admin must NOT be able to delete_cover on a NULL-uploader manga" + ); +} + diff --git a/backend/tests/common/mod.rs b/backend/tests/common/mod.rs index a8475da..0eb8642 100644 --- a/backend/tests/common/mod.rs +++ b/backend/tests/common/mod.rs @@ -615,7 +615,11 @@ impl MultipartBuilder { self.body.extend(b"\r\n"); } - fn finalize(self) -> (String, Vec) { + /// Pub so a test can mint a multipart body for a bearer-auth Request + /// without going through the cookie helpers. Returns + /// `(boundary, body)` ready to attach as `Content-Type: + /// multipart/form-data; boundary={boundary}` and the request body. + pub fn finalize(self) -> (String, Vec) { let mut body = self.body; body.extend(format!("--{}--\r\n", self.boundary).as_bytes()); (self.boundary, body) diff --git a/frontend/package.json b/frontend/package.json index 748ca0c..d449c40 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.87.18", + "version": "0.87.19", "private": true, "type": "module", "scripts": {