fabi/Mangalord
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(admin-security): close the bearer-cookie ride; require session for admin gate (0.87.10)

Browse Source 
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<CurrentSessionUser>` 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) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-06-22 22:28:16 +02:00
parent 747bdeda46
commit 93bb156fba
7 changed files with 220 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
backend/Cargo.lock generated
View File

@@ -1517,7 +1517,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4"
[[package]]
name = "mangalord"
version = "0.87.9"
version = "0.87.10"
dependencies = [
"anyhow",
"argon2",

2
backend/Cargo.toml
View File

@@ -1,6 +1,6 @@
[package]
name = "mangalord"
version = "0.87.9"
version = "0.87.10"
edition = "2021"
default-run = "mangalord"

52
backend/src/api/mangas.rs
View File

@@ -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<AppState>,
CurrentUser(user): CurrentUser,
session: Option<CurrentSessionUser>,
Path(id): Path<Uuid>,
Json(patch): Json<MangaPatch>,
) -> AppResult<Json<MangaDetail>> {
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<AppState>,
CurrentUser(user): CurrentUser,
session: Option<CurrentSessionUser>,
Path(id): Path<Uuid>,
mut multipart: Multipart,
) -> AppResult<Json<MangaDetail>> {
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<UploadedImage> = 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<AppState>,
CurrentUser(user): CurrentUser,
session: Option<CurrentSessionUser>,
Path(id): Path<Uuid>,
) -> AppResult<Json<MangaDetail>> {
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/<id>`
/// 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/<id>` 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<CurrentSessionUser>` extractor returns `None` on a bearer-only
/// request, so admin authority over bot tokens never composes here.
fn admin_via_session(session: &Option<CurrentSessionUser>) -> 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(());

40
backend/src/app.rs
View File

@@ -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);
}

75
backend/tests/api_admin_crawler.rs
View File

@@ -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) {

79
backend/tests/api_mangas_metadata.rs
View File

@@ -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.