From 8667f8b9578f06f011bde27a082c57ed30fb44ff Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Thu, 28 May 2026 19:17:01 +0200 Subject: [PATCH] bugfix: tighten validation, drop dead sendBeacon, NUL byte (0.34.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five small fixes from REVIEW.md §2/§4/§8: - attach_tag: 64-char cap at the handler so the validation error envelope matches username/collection-name. - create_token: same 64-char cap on bot token names. - LocalStorage::resolve rejects NUL bytes explicitly so callers see BadKey instead of an opaque IO error. - sendBeacon dropped from the reader's pagehide flush — it's POST-only and the server's read-progress route is PUT, so every page-close was logging a 405 then falling through to the same keepalive fetch anyway. Keepalive fetch is now the only path. - Frontend logout sets content-type: application/json for symmetry with the other mutation helpers. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/api/auth.rs | 18 ++++++- backend/src/api/mangas.rs | 22 ++++++++ backend/src/storage/local.rs | 10 ++++ backend/tests/api_auth.rs | 24 +++++++++ backend/tests/api_tags.rs | 25 +++++++++ frontend/package.json | 2 +- frontend/src/lib/api/auth.test.ts | 5 ++ frontend/src/lib/api/auth.ts | 9 +++- .../[id]/chapter/[chapter_id]/+page.svelte | 52 ++++++++----------- 11 files changed, 137 insertions(+), 34 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/auth.rs b/backend/src/api/auth.rs index 3dd5cc2..5e3a0ae 100644 --- a/backend/src/api/auth.rs +++ b/backend/src/api/auth.rs @@ -230,8 +230,24 @@ async fn create_token( Json(input): Json, ) -> AppResult { let name = input.name.trim(); + // Both arms use `ValidationFailed` (422 with field details) to + // match the structured-error shape `attach_tag` returns for the + // same kind of free-form-identifier validation. The other + // /auth/* handlers in this file use `InvalidInput` (400); the + // divergence is pre-existing and would warrant a project-wide + // pass to flip them all if the client side wants uniform per- + // field error rendering. if name.is_empty() { - return Err(AppError::InvalidInput("token name is required".into())); + return Err(AppError::ValidationFailed { + message: "token name is required".into(), + details: serde_json::json!({ "name": "required" }), + }); + } + if name.chars().count() > 64 { + return Err(AppError::ValidationFailed { + message: "token name too long".into(), + details: serde_json::json!({ "name": "max 64 characters" }), + }); } let (raw, hash) = generate_token(); let token = repo::api_token::create(&state.db, user.id, name, &hash).await?; diff --git a/backend/src/api/mangas.rs b/backend/src/api/mangas.rs index f0e21af..592ec01 100644 --- a/backend/src/api/mangas.rs +++ b/backend/src/api/mangas.rs @@ -348,6 +348,7 @@ async fn attach_tag( Path(id): Path, Json(body): Json, ) -> AppResult<(StatusCode, Json)> { + validate_tag_name(&body.name)?; if !repo::manga::exists(&state.db, id).await? { return Err(AppError::NotFound); } @@ -394,6 +395,27 @@ async fn detach_tag( } } +/// Request-side validation for `POST /mangas/:id/tags` body. Mirrors +/// the repo-level cap in `repo::tag::upsert_by_name` (max 64 chars +/// after trim) but surfaces the failure at the handler boundary with +/// the same envelope shape other validations use. +fn validate_tag_name(name: &str) -> AppResult<()> { + let trimmed = name.trim(); + if trimmed.is_empty() { + return Err(AppError::ValidationFailed { + message: "tag name cannot be empty".into(), + details: json!({ "name": "required" }), + }); + } + if trimmed.chars().count() > 64 { + return Err(AppError::ValidationFailed { + message: "tag name too long".into(), + details: json!({ "name": "max 64 characters" }), + }); + } + Ok(()) +} + fn validate_new_manga(input: &NewManga) -> AppResult<()> { if input.title.trim().is_empty() { return Err(AppError::ValidationFailed { diff --git a/backend/src/storage/local.rs b/backend/src/storage/local.rs index 7e6dd23..77a4fa2 100644 --- a/backend/src/storage/local.rs +++ b/backend/src/storage/local.rs @@ -16,6 +16,13 @@ impl LocalStorage { } fn resolve(&self, key: &str) -> Result { + // NUL bytes are rejected by the Linux syscall layer, but the + // error surfaces as an opaque IO failure rather than the + // explicit `BadKey` the rest of the contract uses. Catch it + // here so the error path is consistent. + if key.contains('\0') { + return Err(StorageError::BadKey); + } let key = key.trim_start_matches('/'); if key.is_empty() { return Err(StorageError::BadKey); @@ -114,6 +121,9 @@ mod tests { assert!(matches!(s.get(".").await, Err(StorageError::BadKey))); // Empty segment via doubled slash. assert!(matches!(s.get("a//b").await, Err(StorageError::BadKey))); + // NUL byte (rejected explicitly so callers see BadKey rather + // than an opaque IO error from the kernel). + assert!(matches!(s.put("a\0b", b"x").await, Err(StorageError::BadKey))); } #[tokio::test] diff --git a/backend/tests/api_auth.rs b/backend/tests/api_auth.rs index 8ded505..d80674c 100644 --- a/backend/tests/api_auth.rs +++ b/backend/tests/api_auth.rs @@ -581,3 +581,27 @@ async fn delete_unknown_token_is_404(pool: PgPool) { .unwrap(); assert_eq!(resp.status(), StatusCode::NOT_FOUND); } + +/// Bot token names are user-supplied free-form strings; a 10 MB name +/// was accepted before. Cap at 64 chars to match the other free-form +/// identifier caps (tags, collection names). The response uses +/// `ValidationFailed` (422 with per-field details) so clients can +/// render the same shape they already handle for `attach_tag`. +#[sqlx::test(migrations = "./migrations")] +async fn create_token_rejects_name_over_64_chars(pool: PgPool) { + let h = common::harness(pool); + let (_, cookie) = common::register_user(&h.app).await; + let resp = h + .app + .oneshot(common::post_json_with_cookie( + "/api/v1/auth/tokens", + json!({ "name": "x".repeat(65) }), + &cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNPROCESSABLE_ENTITY); + let body = common::body_json(resp).await; + assert_eq!(body["error"]["code"], "validation_failed"); + assert!(body["error"]["details"]["name"].is_string()); +} diff --git a/backend/tests/api_tags.rs b/backend/tests/api_tags.rs index 1ae77b3..c959151 100644 --- a/backend/tests/api_tags.rs +++ b/backend/tests/api_tags.rs @@ -59,6 +59,31 @@ async fn reattach_same_tag_is_idempotent_and_returns_200(pool: PgPool) { assert_eq!(second.status(), StatusCode::OK); } +/// Tag names over 64 chars are rejected at the handler boundary. The +/// repo enforces the same cap, but doing it at the handler keeps the +/// envelope consistent with the other validation paths +/// (username, collection name, etc.). +#[sqlx::test(migrations = "./migrations")] +async fn attach_rejects_tag_name_over_64_chars(pool: PgPool) { + let h = common::harness(pool); + let (_, cookie) = common::register_user(&h.app).await; + let manga_id = common::seed_manga_via_api(&h.app, &cookie, "Berserk").await; + + let long_name: String = "x".repeat(65); + let resp = h + .app + .oneshot(common::post_json_with_cookie( + &format!("/api/v1/mangas/{manga_id}/tags"), + json!({ "name": long_name }), + &cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNPROCESSABLE_ENTITY); + let body = common::body_json(resp).await; + assert_eq!(body["error"]["code"], "validation_failed"); +} + #[sqlx::test(migrations = "./migrations")] async fn tag_names_dedup_case_insensitively(pool: PgPool) { let h = common::harness(pool); 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": { diff --git a/frontend/src/lib/api/auth.test.ts b/frontend/src/lib/api/auth.test.ts index 7826151..5281f93 100644 --- a/frontend/src/lib/api/auth.test.ts +++ b/frontend/src/lib/api/auth.test.ts @@ -94,6 +94,11 @@ describe('auth api client', () => { expect(url).toMatch(/\/v1\/auth\/logout$/); const init = fetchSpy.mock.calls[0][1] as RequestInit; expect(init.method).toBe('POST'); + // Consistent content-type for all mutation requests, matching + // the rest of the module — axum doesn't require it but the + // header keeps the request style uniform. + const headers = new Headers(init.headers); + expect(headers.get('content-type')).toBe('application/json'); }); it('me returns the user on 200', async () => { diff --git a/frontend/src/lib/api/auth.ts b/frontend/src/lib/api/auth.ts index bc9f905..4102325 100644 --- a/frontend/src/lib/api/auth.ts +++ b/frontend/src/lib/api/auth.ts @@ -32,7 +32,14 @@ export async function login(creds: Credentials): Promise { } export async function logout(): Promise { - await request('/v1/auth/logout', { method: 'POST' }); + await request('/v1/auth/logout', { + method: 'POST', + // Consistent with the other POST/PATCH helpers in this module. + // axum doesn't require it (no body), but keeping the header + // on every mutation request avoids the false-flag in logs and + // matches the project's style. + headers: { 'content-type': 'application/json' } + }); } export type ChangePassword = { diff --git a/frontend/src/routes/manga/[id]/chapter/[chapter_id]/+page.svelte b/frontend/src/routes/manga/[id]/chapter/[chapter_id]/+page.svelte index e0334b2..ed81173 100644 --- a/frontend/src/routes/manga/[id]/chapter/[chapter_id]/+page.svelte +++ b/frontend/src/routes/manga/[id]/chapter/[chapter_id]/+page.svelte @@ -350,54 +350,48 @@ }); /** - * `fetch()` initiated during `pagehide` / `beforeunload` is - * cancelled by every browser by default. `sendBeacon` is the - * supported way to ship a small payload during unload — it's - * guaranteed to survive even if the tab is closing. Failure here - * is silent because the API is fire-and-forget. + * Flush read-progress as the tab is closing. A plain `fetch()` + * during `pagehide` / `beforeunload` is cancelled by every + * browser; `fetch(..., { keepalive: true })` is the supported + * escape hatch and survives the close. + * + * `sendBeacon` would be the textbook alternative, but it's + * POST-only and `/me/read-progress` takes PUT — so a beacon + * always 405s, adds server-log noise, then falls through to this + * same keepalive path anyway. The beacon was dropped; the + * keepalive fetch is the only path. */ - function beaconFinalProgress() { + function flushFinalProgress() { if (!session.user) return; const body = JSON.stringify({ manga_id: manga.id, chapter_id: chapter.id, page: progressPage }); - const blob = new Blob([body], { type: 'application/json' }); - // sendBeacon only supports POST — the server's PUT route is - // strict on method. The dedicated POST alias is omitted; in - // practice the in-app navigation path (back-link, chapter - // links) already covers the common-case unmount via the - // onDestroy fetch. Fall through to fetch+keepalive for browser - // implementations that don't honor sendBeacon for this endpoint. try { - const ok = navigator.sendBeacon('/api/v1/me/read-progress', blob); - if (!ok) throw new Error('sendBeacon rejected'); + void fetch('/api/v1/me/read-progress', { + method: 'PUT', + headers: { 'content-type': 'application/json' }, + body, + keepalive: true, + credentials: 'include' + }); } catch { - try { - void fetch('/api/v1/me/read-progress', { - method: 'PUT', - headers: { 'content-type': 'application/json' }, - body, - keepalive: true, - credentials: 'include' - }); - } catch { - // Final fallback failed; the in-app onDestroy flush - // below catches the SPA-navigation case. - } + // keepalive fetch was rejected (very old Firefox etc.); + // the in-app onDestroy flush below catches the SPA- + // navigation case, which is the common one anyway. } } onMount(() => { - window.addEventListener('pagehide', beaconFinalProgress); + window.addEventListener('pagehide', flushFinalProgress); }); onDestroy(() => { observer?.disconnect(); if (progressTimer) clearTimeout(progressTimer); if (typeof window !== 'undefined') { - window.removeEventListener('pagehide', beaconFinalProgress); + window.removeEventListener('pagehide', flushFinalProgress); } // Don't let the fullscreen flag leak to non-reader pages — // otherwise the layout header would stay slid-off on /upload