From 3495190854501e983e244ccf0ef15c77208b3f46 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Thu, 28 May 2026 07:43:55 +0200 Subject: [PATCH] bugfix: gate manga PATCH and cover endpoints on uploader (0.34.1) PATCH /mangas/:id, PUT /mangas/:id/cover and DELETE /mangas/:id/cover took the current user but never compared it against the row's uploaded_by. Any signed-in user could overwrite or clear any manga's metadata and cover. Add require_can_edit gate: non-NULL uploaded_by must match the caller; legacy NULL rows stay open until an admin role lands (per migration 0011 historical-data note). Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/api/mangas.rs | 40 +++++++++++---- backend/src/repo/manga.rs | 14 ++++++ backend/tests/api_mangas_cover.rs | 50 +++++++++++++++++++ backend/tests/api_mangas_metadata.rs | 75 ++++++++++++++++++++++++++++ frontend/package.json | 2 +- 7 files changed, 172 insertions(+), 13 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 5642fcc..f6fb229 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1470,7 +1470,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.34.0" +version = "0.34.1" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index c091570..a126262 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.34.0" +version = "0.34.1" edition = "2021" default-run = "mangalord" diff --git a/backend/src/api/mangas.rs b/backend/src/api/mangas.rs index f0e21af..d0d423c 100644 --- a/backend/src/api/mangas.rs +++ b/backend/src/api/mangas.rs @@ -196,16 +196,14 @@ async fn create( async fn update( State(state): State, - CurrentUser(_user): CurrentUser, + CurrentUser(user): CurrentUser, Path(id): Path, Json(patch): Json, ) -> AppResult> { - // TODO(auth): until uploaders are tracked (Phase 5), any signed-in - // user can edit any manga. Restrict to uploader + admin once that - // column lands. if !repo::manga::exists(&state.db, id).await? { return Err(AppError::NotFound); } + require_can_edit(&state, id, user.id).await?; if let Some(ref status) = patch.status { let trimmed = status.trim(); @@ -269,16 +267,14 @@ async fn update( /// `MangaDetail`. async fn put_cover( State(state): State, - CurrentUser(_user): CurrentUser, + CurrentUser(user): CurrentUser, Path(id): Path, mut multipart: Multipart, ) -> AppResult> { - // TODO(auth): until uploaders are tracked (Phase 5), any signed-in - // user can edit any manga's cover. Restrict to uploader + admin - // once that column lands. if !repo::manga::exists(&state.db, id).await? { return Err(AppError::NotFound); } + require_can_edit(&state, id, user.id).await?; let mut cover: Option = None; while let Some(field) = next_field(&mut multipart).await? { @@ -320,13 +316,13 @@ async fn put_cover( /// with the unchanged detail. async fn delete_cover( State(state): State, - CurrentUser(_user): CurrentUser, + CurrentUser(user): CurrentUser, Path(id): Path, ) -> AppResult> { - // TODO(auth): same caveat as put_cover. if !repo::manga::exists(&state.db, id).await? { return Err(AppError::NotFound); } + require_can_edit(&state, id, user.id).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) => {} @@ -413,6 +409,30 @@ fn validate_new_manga(input: &NewManga) -> AppResult<()> { Ok(()) } +/// Authorisation gate for manga mutations. The manga is assumed to +/// exist (the caller runs [`repo::manga::exists`] first so a missing id +/// surfaces as `NotFound`, not `Forbidden`). +/// +/// Rule: a non-NULL `uploaded_by` must match the current user. Legacy +/// rows with `uploaded_by IS NULL` (pre-migration-0011) are still +/// editable by any signed-in user — there's nobody to gate on yet, and +/// the historical-data note in 0011 acknowledges the gap. Once an +/// admin role lands the NULL case can flip to admin-only. +/// +/// Returns `Forbidden` (not `NotFound`) on owner mismatch — mangas +/// are listable via `GET /mangas`, so existence isn't a secret and +/// the more accurate 403 is fine. This deliberately differs from +/// `repo::collection::require_owner`, which collapses both states to +/// `NotFound` because collections are private to a user and existence +/// itself is information worth hiding from non-owners. +async fn require_can_edit(state: &AppState, manga_id: Uuid, user_id: Uuid) -> AppResult<()> { + match repo::manga::uploaded_by(&state.db, manga_id).await? { + Some(owner) if owner != user_id => Err(AppError::Forbidden), + // Some(owner) == user_id (good) or None (legacy row, no owner). + _ => Ok(()), + } +} + async fn validate_genre_ids(state: &AppState, ids: &[Uuid]) -> AppResult<()> { if ids.is_empty() { return Ok(()); diff --git a/backend/src/repo/manga.rs b/backend/src/repo/manga.rs index f285940..53d2ace 100644 --- a/backend/src/repo/manga.rs +++ b/backend/src/repo/manga.rs @@ -281,3 +281,17 @@ pub async fn exists(pool: &PgPool, id: Uuid) -> AppResult { .await?; Ok(exists) } + +/// Returns the uploader's user id for a manga. `None` either when the +/// manga doesn't exist or when the row predates the `uploaded_by` +/// column (historical NULL — see migration 0011). Callers must +/// distinguish "manga missing" via [`exists`] before relying on this +/// to make an authz decision. +pub async fn uploaded_by(pool: &PgPool, id: Uuid) -> AppResult> { + let row: Option<(Option,)> = + sqlx::query_as("SELECT uploaded_by FROM mangas WHERE id = $1") + .bind(id) + .fetch_optional(pool) + .await?; + Ok(row.and_then(|(u,)| u)) +} diff --git a/backend/tests/api_mangas_cover.rs b/backend/tests/api_mangas_cover.rs index 396eacd..398a3f3 100644 --- a/backend/tests/api_mangas_cover.rs +++ b/backend/tests/api_mangas_cover.rs @@ -410,3 +410,53 @@ async fn delete_cover_404_on_unknown_id(pool: PgPool) { .unwrap(); assert_eq!(resp.status(), StatusCode::NOT_FOUND); } + +/// Authz: PUT /mangas/:id/cover must be uploader-only. +#[sqlx::test(migrations = "./migrations")] +async fn put_cover_forbidden_for_non_uploader(pool: PgPool) { + let h = harness(pool); + let (_, owner_cookie) = register_user(&h.app).await; + let (_, intruder_cookie) = register_user(&h.app).await; + + let manga = + create_manga_with_cover(&h.app, &owner_cookie, "Mine", None).await; + let id = id_of(&manga); + + let resp = h + .app + .oneshot(put_multipart_with_cookie( + &format!("/api/v1/mangas/{id}/cover"), + cover_form(&fake_png_bytes()), + &intruder_cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); +} + +/// Authz: DELETE /mangas/:id/cover must be uploader-only. +#[sqlx::test(migrations = "./migrations")] +async fn delete_cover_forbidden_for_non_uploader(pool: PgPool) { + let h = harness(pool); + let (_, owner_cookie) = register_user(&h.app).await; + let (_, intruder_cookie) = register_user(&h.app).await; + + let manga = create_manga_with_cover( + &h.app, + &owner_cookie, + "Mine", + Some(("image/jpeg", &fake_jpeg_bytes())), + ) + .await; + let id = id_of(&manga); + + let resp = h + .app + .oneshot(delete_with_cookie( + &format!("/api/v1/mangas/{id}/cover"), + &intruder_cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); +} diff --git a/backend/tests/api_mangas_metadata.rs b/backend/tests/api_mangas_metadata.rs index 2797d34..107f9d2 100644 --- a/backend/tests/api_mangas_metadata.rs +++ b/backend/tests/api_mangas_metadata.rs @@ -566,3 +566,78 @@ async fn patch_requires_authentication(pool: PgPool) { .unwrap(); assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); } + +/// A signed-in user who didn't upload the manga must not be able to +/// PATCH it. Without the uploader-gate this returned 200 — see +/// REVIEW.md "manga PATCH / cover endpoints don't check ownership". +#[sqlx::test(migrations = "./migrations")] +async fn patch_forbidden_for_non_uploader(pool: PgPool) { + let h = common::harness(pool); + let (_, owner_cookie) = common::register_user(&h.app).await; + let (_, intruder_cookie) = common::register_user(&h.app).await; + + let created = create_manga(&h.app, &owner_cookie, json!({ "title": "Mine" })).await; + let id = id_of(&created); + + let resp = h + .app + .oneshot(common::patch_json_with_cookie( + &format!("/api/v1/mangas/{id}"), + json!({ "status": "completed" }), + &intruder_cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); +} + +/// Owner can still edit their own manga (regression guard for the +/// authz fix). +#[sqlx::test(migrations = "./migrations")] +async fn patch_allowed_for_uploader(pool: PgPool) { + let h = common::harness(pool); + let (_, cookie) = common::register_user(&h.app).await; + let created = create_manga(&h.app, &cookie, json!({ "title": "Owned" })).await; + let id = id_of(&created); + 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); +} + +/// Legacy rows with `uploaded_by IS NULL` (created before migration +/// 0011) remain editable by any signed-in user. Without this carve-out +/// the historical-data note in 0011 would be broken. +#[sqlx::test(migrations = "./migrations")] +async fn patch_allowed_on_legacy_null_uploader(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": "Legacy" })).await; + let id = id_of(&created); + + // Simulate a row uploaded before the column existed: clear + // uploaded_by directly via SQL. + 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::patch_json_with_cookie( + &format!("/api/v1/mangas/{id}"), + json!({ "status": "completed" }), + &other_cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); +} diff --git a/frontend/package.json b/frontend/package.json index 159dad9..72a32cd 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.34.0", + "version": "0.34.1", "private": true, "type": "module", "scripts": {