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

fix(admin): security-audit findings — paginate chapters, lock down unchecked helper (0.41.2)

Browse Source 
Addresses the security-audit findings on top of the admin feature stack:

M1: /admin/mangas/:id/chapters now paginates (default limit 200, max 500).
A long-runner with thousands of chapters would otherwise produce a multi-MB
response with that many scalar subqueries per row — admin-only but a real
stall risk on one expand-click. Adds explicit pagination tests for the cap
and offset; frontend renders a "Showing first N of M" hint when the cap
clips the result.

L1: repo::user::set_is_admin renamed to set_is_admin_unchecked with a
doc-comment pointing at admin_safe_set_is_admin for production use. The
short name was a footgun — a future contributor reaching for it would
silently bypass self-protection, the last-admin invariant, and the audit
log. Used only by integration-test setup; production code goes through
the admin_safe_* paths.

CSRF posture: build_session_cookie carries a comment that the
SameSite=Lax default is the project's CSRF defense for state-changing
mutations and breaks the instant anyone adds a side-effecting GET under
/admin/*. Spells out what to do then (Strict + explicit token check).

Test counts: 43 backend admin tests + 12 vitest admin tests all green;
svelte-check 0/0 across 446 files.
This commit is contained in:
MechaCat02
2026-05-30 22:23:55 +02:00
parent aa2159ca06
commit f6728dc71a
13 changed files with 214 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
backend/Cargo.toml
View File

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

29
backend/src/api/admin/mangas.rs
View File

@@ -25,6 +25,18 @@ pub fn routes() -> Router<AppState> {
.route("/admin/mangas/:id/chapters", get(list_chapters))
}
#[derive(Debug, Deserialize, Default)]
pub struct ListChaptersParams {
#[serde(default = "default_chapter_limit")]
pub limit: i64,
#[serde(default)]
pub offset: i64,
}
fn default_chapter_limit() -> i64 {
200
}
#[derive(Debug, Deserialize, Default)]
pub struct ListMangasParams {
#[serde(default)]
@@ -76,12 +88,23 @@ async fn list_chapters(
State(state): State<AppState>,
_admin: RequireAdmin,
Path(manga_id): Path<Uuid>,
) -> AppResult<Json<Vec<AdminChapterRow>>> {
Query(params): Query<ListChaptersParams>,
) -> AppResult<Json<PagedResponse<AdminChapterRow>>> {
// Explicit existence check so a typo / deleted manga returns 404
// rather than a misleading "no chapters" 200.
if !repo::manga::exists(&state.db, manga_id).await? {
return Err(AppError::NotFound);
}
let rows = repo::admin_view::list_chapters_with_sync_state(&state.db, manga_id).await?;
Ok(Json(rows))
// Cap at 500 to bound the per-row scalar-subquery cost on
// long-runners with thousands of chapters; default 200 covers
// typical browsing without paging round-trips.
let limit = params.limit.clamp(1, 500);
let offset = params.offset.max(0);
let q = repo::admin_view::ListAdminChaptersQuery {
manga_id,
limit,
offset,
};
let (items, total) = repo::admin_view::list_chapters_with_sync_state(&state.db, &q).await?;
Ok(Json(PagedResponse::with_total(items, limit, offset, total)))
}

12
backend/src/api/auth.rs
View File

@@ -309,6 +309,18 @@ async fn start_session(
Ok(jar.add(build_session_cookie(raw, &state.auth)))
}
// CSRF posture: `SameSite=Lax` is the project's primary CSRF defense.
// Browsers refuse to attach this cookie to cross-site POST / PATCH /
// DELETE requests, which covers every state-changing endpoint (auth
// mutations, uploads, bookmarks, collections, admin user management,
// etc. — all JSON over POST/PATCH/DELETE). Lax DOES still attach the
// cookie on top-level cross-site GETs, so this defense breaks the
// instant anyone adds a state-changing GET. If you reach for one,
// switch to `SameSite=Strict` here AND add an explicit CSRF-token
// check on the new endpoint. The Bearer-token branch in the
// extractor is unaffected (bots authenticate with the token header,
// not the cookie) and admin routes reject Bearer entirely — see
// `auth::extractor::RequireAdmin`.
fn build_session_cookie(raw: String, cfg: &AuthConfig) -> Cookie<'static> {
let mut builder = Cookie::build((SESSION_COOKIE_NAME, raw))
.http_only(true)

31
backend/src/repo/admin_view.rs
View File

@@ -166,11 +166,23 @@ pub struct AdminChapterRow {
pub latest_seen_at: Option<DateTime<Utc>>,
}
#[derive(Debug, Default)]
pub struct ListAdminChaptersQuery {
pub manga_id: Uuid,
pub limit: i64,
pub offset: i64,
}
/// Paginated chapter list with derived sync state. Pagination is non-
/// optional — long-runners can have thousands of chapters and the
/// per-row scalar subqueries make the unbounded variant a real
/// stall risk even behind an admin guard. Returns the page slice plus
/// the unfiltered total so the UI can render "showing N of M".
pub async fn list_chapters_with_sync_state(
pool: &PgPool,
manga_id: Uuid,
) -> AppResult<Vec<AdminChapterRow>> {
let rows: Vec<AdminChapterRow> = sqlx::query_as(
q: &ListAdminChaptersQuery,
) -> AppResult<(Vec<AdminChapterRow>, i64)> {
let items: Vec<AdminChapterRow> = sqlx::query_as(
r#"
SELECT
c.id, c.manga_id, c.number, c.title, c.page_count, c.created_at,
@@ -202,10 +214,19 @@ pub async fn list_chapters_with_sync_state(
FROM chapters c
WHERE c.manga_id = $1
ORDER BY c.number ASC
LIMIT $2 OFFSET $3
"#,
)
.bind(manga_id)
.bind(q.manga_id)
.bind(q.limit)
.bind(q.offset)
.fetch_all(pool)
.await?;
Ok(rows)
let total: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM chapters WHERE manga_id = $1")
.bind(q.manga_id)
.fetch_one(pool)
.await?;
Ok((items, total))
}

7
backend/src/repo/user.rs
View File

@@ -114,7 +114,12 @@ pub async fn list_with_total(
Ok((items, total))
}
pub async fn set_is_admin(pool: &PgPool, id: Uuid, value: bool) -> AppResult<()> {
/// Raw `is_admin` update with no safety checks, no audit log, and no
/// advisory lock. Exists only as a test setup helper for the admin-
/// feature integration suite — production code MUST go through
/// [`admin_safe_set_is_admin`], which enforces self-protection, the
/// last-admin invariant, and the audit log atomically.
pub async fn set_is_admin_unchecked(pool: &PgPool, id: Uuid, value: bool) -> AppResult<()> {
sqlx::query("UPDATE users SET is_admin = $1 WHERE id = $2")
.bind(value)
.bind(id)

102
backend/tests/api_admin_mangas.rs
View File

@@ -24,7 +24,7 @@ async fn seed_admin(pool: &PgPool, app: &Router) -> (String, String) {
.await
.unwrap()
.unwrap();
repo::user::set_is_admin(pool, u.id, true).await.unwrap();
repo::user::set_is_admin_unchecked(pool, u.id, true).await.unwrap();
(username, cookie)
}
@@ -107,6 +107,25 @@ async fn insert_job(pool: &PgPool, payload: serde_json::Value, state: &str) {
.unwrap();
}
/// Per-variant tests don't care about pagination — fetch the whole
/// chapter set (up to the hard cap) and discard the total.
async fn fetch_chapter_rows(
pool: &PgPool,
manga_id: Uuid,
) -> Vec<mangalord::repo::admin_view::AdminChapterRow> {
let (rows, _) = repo::admin_view::list_chapters_with_sync_state(
pool,
&repo::admin_view::ListAdminChaptersQuery {
manga_id,
limit: 500,
offset: 0,
},
)
.await
.unwrap();
rows
}
// ---- manga sync state ------------------------------------------------------
#[sqlx::test(migrations = "./migrations")]
@@ -257,9 +276,7 @@ async fn chapter_state_synced_when_pages_present(pool: PgPool) {
let c = insert_chapter(&pool, m, 1, 12).await;
insert_chapter_source(&pool, c, "ckey-1", false).await;
let rows = repo::admin_view::list_chapters_with_sync_state(&pool, m)
.await
.unwrap();
let rows = fetch_chapter_rows(&pool, m).await;
assert_eq!(rows.len(), 1);
assert_eq!(rows[0].id, c);
assert_eq!(rows[0].sync_state, mangalord::domain::ChapterSyncState::Synced);
@@ -272,9 +289,7 @@ async fn chapter_state_not_downloaded_when_page_count_zero(pool: PgPool) {
let c = insert_chapter(&pool, m, 1, 0).await;
insert_chapter_source(&pool, c, "ckey-1", false).await;
let rows = repo::admin_view::list_chapters_with_sync_state(&pool, m)
.await
.unwrap();
let rows = fetch_chapter_rows(&pool, m).await;
assert_eq!(
rows[0].sync_state,
mangalord::domain::ChapterSyncState::NotDownloaded
@@ -299,9 +314,7 @@ async fn chapter_state_downloading_when_job_in_flight(pool: PgPool) {
)
.await;
let rows = repo::admin_view::list_chapters_with_sync_state(&pool, m)
.await
.unwrap();
let rows = fetch_chapter_rows(&pool, m).await;
assert_eq!(
rows[0].sync_state,
mangalord::domain::ChapterSyncState::Downloading
@@ -315,9 +328,7 @@ async fn chapter_state_dropped_when_all_sources_dropped(pool: PgPool) {
let c = insert_chapter(&pool, m, 1, 0).await;
insert_chapter_source(&pool, c, "ckey-1", true).await;
let rows = repo::admin_view::list_chapters_with_sync_state(&pool, m)
.await
.unwrap();
let rows = fetch_chapter_rows(&pool, m).await;
assert_eq!(
rows[0].sync_state,
mangalord::domain::ChapterSyncState::Dropped
@@ -342,9 +353,7 @@ async fn chapter_state_failed_when_most_recent_job_dead(pool: PgPool) {
)
.await;
let rows = repo::admin_view::list_chapters_with_sync_state(&pool, m)
.await
.unwrap();
let rows = fetch_chapter_rows(&pool, m).await;
assert_eq!(
rows[0].sync_state,
mangalord::domain::ChapterSyncState::Failed
@@ -415,12 +424,67 @@ async fn http_list_chapters_returns_per_chapter_state(pool: PgPool) {
.unwrap();
assert_eq!(resp.status(), StatusCode::OK);
let body = common::body_json(resp).await;
let items = body.as_array().unwrap();
let items = body["items"].as_array().unwrap();
assert_eq!(items.len(), 2);
assert_eq!(items[0]["id"], c1.to_string());
assert_eq!(items[0]["sync_state"], "synced");
assert_eq!(items[1]["id"], c2.to_string());
assert_eq!(items[1]["sync_state"], "not_downloaded");
assert_eq!(body["page"]["total"], 2);
}
#[sqlx::test(migrations = "./migrations")]
async fn http_list_chapters_caps_limit_at_500(pool: PgPool) {
// The handler clamps limit to [1, 500] so a long-runner with
// thousands of chapters can't be turned into a request-stall by an
// admin (or by a curious admin tab) just clicking expand.
let h = common::harness(pool.clone());
let (_admin, cookie) = seed_admin(&pool, &h.app).await;
seed_source(&pool).await;
let m = insert_manga(&pool, "M").await;
for n in 1..=3 {
let _c = insert_chapter(&pool, m, n, 0).await;
}
let resp = h
.app
.oneshot(common::get_with_cookie(
&format!("/api/v1/admin/mangas/{m}/chapters?limit=999"),
&cookie,
))
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::OK);
let body = common::body_json(resp).await;
assert_eq!(body["page"]["limit"], 500, "limit must clamp to 500");
assert_eq!(body["items"].as_array().unwrap().len(), 3);
}
#[sqlx::test(migrations = "./migrations")]
async fn http_list_chapters_paginates(pool: PgPool) {
let h = common::harness(pool.clone());
let (_admin, cookie) = seed_admin(&pool, &h.app).await;
seed_source(&pool).await;
let m = insert_manga(&pool, "M").await;
for n in 1..=5 {
let _c = insert_chapter(&pool, m, n, 0).await;
}
let resp = h
.app
.clone()
.oneshot(common::get_with_cookie(
&format!("/api/v1/admin/mangas/{m}/chapters?limit=2&offset=2"),
&cookie,
))
.await
.unwrap();
let body = common::body_json(resp).await;
let items = body["items"].as_array().unwrap();
assert_eq!(items.len(), 2);
// Ordered by chapter number ascending; offset=2 skips chapters 1 & 2.
assert_eq!(items[0]["number"], 3);
assert_eq!(items[1]["number"], 4);
assert_eq!(body["page"]["total"], 5);
}
#[sqlx::test(migrations = "./migrations")]
@@ -463,9 +527,7 @@ async fn chapter_state_synced_when_pages_present_even_with_dead_job(pool: PgPool
)
.await;
let rows = repo::admin_view::list_chapters_with_sync_state(&pool, m)
.await
.unwrap();
let rows = fetch_chapter_rows(&pool, m).await;
assert_eq!(
rows[0].sync_state,
mangalord::domain::ChapterSyncState::Synced,

4
backend/tests/api_admin_role.rs
View File

@@ -189,7 +189,7 @@ async fn require_admin_accepts_admin_cookie(pool: PgPool) {
.await
.unwrap()
.unwrap();
repo::user::set_is_admin(&pool, u.id, true).await.unwrap();
repo::user::set_is_admin_unchecked(&pool, u.id, true).await.unwrap();
let resp = app
.oneshot(common::get_with_cookie("/_test/admin_only", &cookie))
@@ -216,7 +216,7 @@ async fn require_admin_rejects_bearer_token_even_for_admin_user(pool: PgPool) {
.await
.unwrap()
.unwrap();
repo::user::set_is_admin(&pool, u.id, true).await.unwrap();
repo::user::set_is_admin_unchecked(&pool, u.id, true).await.unwrap();
let resp = app
.clone()

2
backend/tests/api_admin_system.rs
View File

@@ -20,7 +20,7 @@ async fn seed_admin(pool: &PgPool, app: &Router) -> String {
.await
.unwrap()
.unwrap();
repo::user::set_is_admin(pool, u.id, true).await.unwrap();
repo::user::set_is_admin_unchecked(pool, u.id, true).await.unwrap();
cookie
}

2
backend/tests/api_admin_users.rs
View File

@@ -35,7 +35,7 @@ async fn seed_admin(pool: &PgPool, app: &Router) -> (String, String, Uuid) {
.await
.unwrap()
.unwrap();
repo::user::set_is_admin(pool, u.id, true).await.unwrap();
repo::user::set_is_admin_unchecked(pool, u.id, true).await.unwrap();
(username, cookie, u.id)
}

2
frontend/package.json
View File

@@ -1,6 +1,6 @@
{
"name": "mangalord-frontend",
"version": "0.41.1",
"version": "0.41.2",
"private": true,
"type": "module",
"scripts": {

21
frontend/src/lib/api/admin.test.ts
View File

@@ -149,7 +149,7 @@ describe('admin api client', () => {
expect(url).toContain('limit=100');
});
it('listAdminChapters GETs the nested chapter route', async () => {
it('listAdminChapters GETs the nested chapter route and parses the paged envelope', async () => {
const chapter = {
id: 'c-1',
manga_id: 'm-1',
@@ -160,13 +160,26 @@ describe('admin api client', () => {
sync_state: 'synced' as const,
latest_seen_at: null
};
fetchSpy.mockResolvedValueOnce(ok([chapter]));
const rows = await listAdminChapters('m-1');
expect(rows).toEqual([chapter]);
fetchSpy.mockResolvedValueOnce(
ok({ items: [chapter], page: { limit: 200, offset: 0, total: 1 } })
);
const resp = await listAdminChapters('m-1');
expect(resp.items).toEqual([chapter]);
expect(resp.page.total).toBe(1);
const url = fetchSpy.mock.calls[0][0] as string;
expect(url).toMatch(/\/v1\/admin\/mangas\/m-1\/chapters$/);
});
it('listAdminChapters forwards limit + offset query params', async () => {
fetchSpy.mockResolvedValueOnce(
ok({ items: [], page: { limit: 50, offset: 100, total: 0 } })
);
await listAdminChapters('m-1', { limit: 50, offset: 100 });
const url = fetchSpy.mock.calls[0][0] as string;
expect(url).toContain('limit=50');
expect(url).toContain('offset=100');
});
// ---- system ----
it('getSystemStats GETs /v1/admin/system and parses the four-key envelope', async () => {

23
frontend/src/lib/api/admin.ts
View File

@@ -102,9 +102,26 @@ export type AdminChapterRow = {
latest_seen_at: string | null;
};
export async function listAdminChapters(mangaId: string): Promise<AdminChapterRow[]> {
return request<AdminChapterRow[]>(
`/v1/admin/mangas/${encodeURIComponent(mangaId)}/chapters`
export type AdminChaptersPage = {
items: AdminChapterRow[];
page: Page;
};
export type ListAdminChaptersOptions = {
limit?: number;
offset?: number;
};
export async function listAdminChapters(
mangaId: string,
opts: ListAdminChaptersOptions = {}
): Promise<AdminChaptersPage> {
const params = new URLSearchParams();
if (opts.limit != null) params.set('limit', String(opts.limit));
if (opts.offset != null) params.set('offset', String(opts.offset));
const qs = params.toString();
return request<AdminChaptersPage>(
`/v1/admin/mangas/${encodeURIComponent(mangaId)}/chapters${qs ? `?${qs}` : ''}`
);
}

24
frontend/src/routes/admin/mangas/+page.svelte
View File

@@ -14,7 +14,11 @@
let syncFilter: MangaSyncState | '' = $state('');
let error: string | null = $state(null);
let expandedId: string | null = $state(null);
let chaptersByManga: Record<string, AdminChapterRow[] | 'loading'> = $state({});
type ChaptersView = {
items: AdminChapterRow[];
total: number;
};
let chaptersByManga: Record<string, ChaptersView | 'loading'> = $state({});
async function load() {
error = null;
@@ -40,7 +44,11 @@
if (!chaptersByManga[id]) {
chaptersByManga[id] = 'loading';
try {
chaptersByManga[id] = await listAdminChapters(id);
const resp = await listAdminChapters(id, { limit: 500 });
chaptersByManga[id] = {
items: resp.items,
total: resp.page.total ?? resp.items.length
};
} catch {
delete chaptersByManga[id];
error = 'failed to load chapters';
@@ -113,10 +121,16 @@
{#if chaptersByManga[m.id] === 'loading'}
<p>Loading chapters…</p>
{:else if chaptersByManga[m.id]}
{@const list = chaptersByManga[m.id] as AdminChapterRow[]}
{#if list.length === 0}
{@const view = chaptersByManga[m.id] as ChaptersView}
{#if view.items.length === 0}
<p class="muted">No chapters.</p>
{:else}
{#if view.total > view.items.length}
<p class="muted">
Showing first {view.items.length} of {view.total}
chapters (cap reached).
</p>
{/if}
<table class="inner">
<thead>
<tr>
@@ -127,7 +141,7 @@
</tr>
</thead>
<tbody>
{#each list as c (c.id)}
{#each view.items as c (c.id)}
<tr>
<td>{c.number}</td>
<td>{c.title ?? '—'}</td>