fix(authz): require admin to edit/cover crawler-imported mangas (0.87.3)
`require_can_edit` matched `Some(owner) != caller → Forbidden` but let `None` through. Every crawler row (`repo::crawler::upsert_manga`) has NULL `uploaded_by`, so any signed-in user could PATCH /api/v1/mangas/<id> and rewrite the catalog — `update`, `put_cover`, and `delete_cover` were all in scope. With self-registration on by default, the path was: register → mass-edit catalog + delete cover blobs from storage. The original carve-out at the comment said "Once an admin role lands the NULL case can flip to admin-only." The admin role landed in migration 0018; flipping it now. New rule: * `Some(owner)` and owner == caller → ok (unchanged) * `Some(_)` and caller is admin → ok (admin moderation) * `Some(_)` otherwise → 403 (unchanged) * `None` and caller is admin → ok (NEW — operator can curate) * `None` otherwise → 403 (was: ok — the bug) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
2
backend/Cargo.lock
generated
2
backend/Cargo.lock
generated
@@ -1517,7 +1517,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4"
|
|||||||
|
|
||||||
[[package]]
|
[[package]]
|
||||||
name = "mangalord"
|
name = "mangalord"
|
||||||
version = "0.87.2"
|
version = "0.87.3"
|
||||||
dependencies = [
|
dependencies = [
|
||||||
"anyhow",
|
"anyhow",
|
||||||
"argon2",
|
"argon2",
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
[package]
|
[package]
|
||||||
name = "mangalord"
|
name = "mangalord"
|
||||||
version = "0.87.2"
|
version = "0.87.3"
|
||||||
edition = "2021"
|
edition = "2021"
|
||||||
default-run = "mangalord"
|
default-run = "mangalord"
|
||||||
|
|
||||||
|
|||||||
@@ -235,7 +235,7 @@ async fn update(
|
|||||||
if !repo::manga::exists(&state.db, id).await? {
|
if !repo::manga::exists(&state.db, id).await? {
|
||||||
return Err(AppError::NotFound);
|
return Err(AppError::NotFound);
|
||||||
}
|
}
|
||||||
require_can_edit(&state, id, user.id).await?;
|
require_can_edit(&state, id, user.id, user.is_admin).await?;
|
||||||
|
|
||||||
if let Some(ref status) = patch.status {
|
if let Some(ref status) = patch.status {
|
||||||
let trimmed = status.trim();
|
let trimmed = status.trim();
|
||||||
@@ -306,7 +306,7 @@ async fn put_cover(
|
|||||||
if !repo::manga::exists(&state.db, id).await? {
|
if !repo::manga::exists(&state.db, id).await? {
|
||||||
return Err(AppError::NotFound);
|
return Err(AppError::NotFound);
|
||||||
}
|
}
|
||||||
require_can_edit(&state, id, user.id).await?;
|
require_can_edit(&state, id, user.id, user.is_admin).await?;
|
||||||
|
|
||||||
let mut cover: Option<UploadedImage> = None;
|
let mut cover: Option<UploadedImage> = None;
|
||||||
while let Some(field) = next_field(&mut multipart).await? {
|
while let Some(field) = next_field(&mut multipart).await? {
|
||||||
@@ -354,7 +354,7 @@ async fn delete_cover(
|
|||||||
if !repo::manga::exists(&state.db, id).await? {
|
if !repo::manga::exists(&state.db, id).await? {
|
||||||
return Err(AppError::NotFound);
|
return Err(AppError::NotFound);
|
||||||
}
|
}
|
||||||
require_can_edit(&state, id, user.id).await?;
|
require_can_edit(&state, id, user.id, user.is_admin).await?;
|
||||||
if let Some(key) = repo::manga::get(&state.db, id).await?.cover_image_path {
|
if let Some(key) = repo::manga::get(&state.db, id).await?.cover_image_path {
|
||||||
match state.storage.delete(&key).await {
|
match state.storage.delete(&key).await {
|
||||||
Ok(()) | Err(StorageError::NotFound) => {}
|
Ok(()) | Err(StorageError::NotFound) => {}
|
||||||
@@ -467,11 +467,16 @@ fn validate_new_manga(input: &NewManga) -> AppResult<()> {
|
|||||||
/// exist (the caller runs [`repo::manga::exists`] first so a missing id
|
/// exist (the caller runs [`repo::manga::exists`] first so a missing id
|
||||||
/// surfaces as `NotFound`, not `Forbidden`).
|
/// surfaces as `NotFound`, not `Forbidden`).
|
||||||
///
|
///
|
||||||
/// Rule: a non-NULL `uploaded_by` must match the current user. Legacy
|
/// Rule: a non-NULL `uploaded_by` must match the current user, OR the
|
||||||
/// rows with `uploaded_by IS NULL` (pre-migration-0011) are still
|
/// caller is an admin. Rows with `uploaded_by IS NULL` (crawler-imported
|
||||||
/// editable by any signed-in user — there's nobody to gate on yet, and
|
/// + legacy pre-0011) are admin-only.
|
||||||
/// the historical-data note in 0011 acknowledges the gap. Once an
|
///
|
||||||
/// admin role lands the NULL case can flip to admin-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.
|
||||||
///
|
///
|
||||||
/// Returns `Forbidden` (not `NotFound`) on owner mismatch — mangas
|
/// Returns `Forbidden` (not `NotFound`) on owner mismatch — mangas
|
||||||
/// are listable via `GET /mangas`, so existence isn't a secret and
|
/// are listable via `GET /mangas`, so existence isn't a secret and
|
||||||
@@ -479,11 +484,18 @@ fn validate_new_manga(input: &NewManga) -> AppResult<()> {
|
|||||||
/// `repo::collection::require_owner`, which collapses both states to
|
/// `repo::collection::require_owner`, which collapses both states to
|
||||||
/// `NotFound` because collections are private to a user and existence
|
/// `NotFound` because collections are private to a user and existence
|
||||||
/// itself is information worth hiding from non-owners.
|
/// itself is information worth hiding from non-owners.
|
||||||
async fn require_can_edit(state: &AppState, manga_id: Uuid, user_id: Uuid) -> AppResult<()> {
|
async fn require_can_edit(
|
||||||
|
state: &AppState,
|
||||||
|
manga_id: Uuid,
|
||||||
|
user_id: Uuid,
|
||||||
|
is_admin: bool,
|
||||||
|
) -> AppResult<()> {
|
||||||
match repo::manga::uploaded_by(&state.db, manga_id).await? {
|
match repo::manga::uploaded_by(&state.db, manga_id).await? {
|
||||||
Some(owner) if owner != user_id => Err(AppError::Forbidden),
|
Some(owner) if owner == user_id => Ok(()),
|
||||||
// Some(owner) == user_id (good) or None (legacy row, no owner).
|
Some(_) if is_admin => Ok(()),
|
||||||
_ => Ok(()),
|
Some(_) => Err(AppError::Forbidden),
|
||||||
|
None if is_admin => Ok(()),
|
||||||
|
None => Err(AppError::Forbidden),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -611,18 +611,18 @@ async fn patch_allowed_for_uploader(pool: PgPool) {
|
|||||||
assert_eq!(resp.status(), StatusCode::OK);
|
assert_eq!(resp.status(), StatusCode::OK);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Legacy rows with `uploaded_by IS NULL` (created before migration
|
/// Rows with `uploaded_by IS NULL` — crawler-imported plus any pre-0011
|
||||||
/// 0011) remain editable by any signed-in user. Without this carve-out
|
/// legacy rows — are admin-only. A regular user editing them used to be
|
||||||
/// the historical-data note in 0011 would be broken.
|
/// allowed; that meant `register → PATCH /mangas/<id>` rewrote the
|
||||||
|
/// catalog for free.
|
||||||
#[sqlx::test(migrations = "./migrations")]
|
#[sqlx::test(migrations = "./migrations")]
|
||||||
async fn patch_allowed_on_legacy_null_uploader(pool: PgPool) {
|
async fn patch_null_uploader_rejected_for_non_admin(pool: PgPool) {
|
||||||
let h = common::harness(pool.clone());
|
let h = common::harness(pool.clone());
|
||||||
let (_, cookie) = common::register_user(&h.app).await;
|
let (_, cookie) = common::register_user(&h.app).await;
|
||||||
let created = create_manga(&h.app, &cookie, json!({ "title": "Legacy" })).await;
|
let created = create_manga(&h.app, &cookie, json!({ "title": "Catalog" })).await;
|
||||||
let id = id_of(&created);
|
let id = id_of(&created);
|
||||||
|
|
||||||
// Simulate a row uploaded before the column existed: clear
|
// Simulate a crawler-imported / pre-0011 row: clear uploaded_by.
|
||||||
// uploaded_by directly via SQL.
|
|
||||||
sqlx::query("UPDATE mangas SET uploaded_by = NULL WHERE id = $1")
|
sqlx::query("UPDATE mangas SET uploaded_by = NULL WHERE id = $1")
|
||||||
.bind(id)
|
.bind(id)
|
||||||
.execute(&pool)
|
.execute(&pool)
|
||||||
@@ -639,5 +639,42 @@ async fn patch_allowed_on_legacy_null_uploader(pool: PgPool) {
|
|||||||
))
|
))
|
||||||
.await
|
.await
|
||||||
.unwrap();
|
.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.
|
||||||
|
#[sqlx::test(migrations = "./migrations")]
|
||||||
|
async fn patch_null_uploader_allowed_for_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();
|
||||||
|
|
||||||
|
// Mint an admin and let them edit the row.
|
||||||
|
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 resp = h
|
||||||
|
.app
|
||||||
|
.oneshot(common::patch_json_with_cookie(
|
||||||
|
&format!("/api/v1/mangas/{id}"),
|
||||||
|
json!({ "status": "completed" }),
|
||||||
|
&admin_cookie,
|
||||||
|
))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
assert_eq!(resp.status(), StatusCode::OK);
|
assert_eq!(resp.status(), StatusCode::OK);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"name": "mangalord-frontend",
|
"name": "mangalord-frontend",
|
||||||
"version": "0.87.2",
|
"version": "0.87.3",
|
||||||
"private": true,
|
"private": true,
|
||||||
"type": "module",
|
"type": "module",
|
||||||
"scripts": {
|
"scripts": {
|
||||||
|
|||||||
Reference in New Issue
Block a user