From a8d6da167c7d55393e6cafb407246ad10916de13 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 16 May 2026 23:55:53 +0200 Subject: [PATCH] bugfix: second-pass audit follow-ups (N1-N4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small follow-ups from the second-pass audit: - N1: `manga_upload_rolls_back_when_cover_storage_fails` covers the manga-side of the transactional rollback path. The chapter case had a `FailingStorage` regression test already; this completes the symmetric pair. With fail-on-put-index=0, the cover put fails on the first call, the transaction aborts, and `SELECT count(*) FROM mangas WHERE title = 'Berserk'` is 0. - N2: The SvelteKit proxy now catches network-layer failures from the upstream `fetch` (DNS / connection refused / TLS handshake) and returns a 502 with the standard error envelope (`code: 'upstream_unavailable'`) instead of letting SvelteKit's generic 500 HTML page through. `client.ts` can `.json()` the result cleanly so callers see a real ApiError with a meaningful code. The underlying cause is logged via `console.error` for the operator. Test in hooks.server.test.ts asserts the 502, the JSON envelope, and that `resolve` is not called (the proxy short-circuits). - N3: `GET /api/v1/files/*key` now sets `X-Content-Type-Options: nosniff`. The upload-time magic-byte sniff is authoritative for what we declare as Content-Type; `nosniff` makes the contract explicit so older user-agents can't try to re-detect HTML/JS in a polyglot file that survived the sniff. Test in api_uploads.rs asserts the header. - N4: The /bookmarks page used `{#if b.page}` to gate the "— page N" display, which falsy-elided a legitimate `page == 0`. Backend now rejects `page < 1` for new bookmarks (already shipped in 0.9.4), but any pre-0.9.4 row with page=0 still rendered without its number. Strengthened to `{#if b.page != null && b.page > 0}`. Lockstep version bump to 0.10.1. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/api/files.rs | 10 +++++- backend/tests/api_uploads.rs | 41 ++++++++++++++++++++++ frontend/package.json | 2 +- frontend/src/hooks.server.test.ts | 17 +++++++++ frontend/src/hooks.server.ts | 25 ++++++++++++- frontend/src/routes/bookmarks/+page.svelte | 2 +- 8 files changed, 95 insertions(+), 6 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index cb4ee23..25f82f0 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1033,7 +1033,7 @@ checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897" [[package]] name = "mangalord" -version = "0.9.4" +version = "0.10.0" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 6cd513f..2da469c 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.10.0" +version = "0.10.1" edition = "2021" [lib] diff --git a/backend/src/api/files.rs b/backend/src/api/files.rs index 019b3ce..6cff2b8 100644 --- a/backend/src/api/files.rs +++ b/backend/src/api/files.rs @@ -7,7 +7,7 @@ use axum::body::Body; use axum::extract::{Path, State}; -use axum::http::header; +use axum::http::{header, HeaderName}; use axum::response::{IntoResponse, Response}; use axum::routing::get; use axum::Router; @@ -27,9 +27,17 @@ async fn serve(State(state): State, Path(key): Path) -> AppRes Err(e) => return Err(e.into()), }; let ct = content_type_for(&key); + // `nosniff` makes the contract explicit: the browser must trust the + // Content-Type we declared (and that the magic-byte sniff at upload + // time produced) instead of trying to detect HTML/JS in the body. + // Belt-and-braces vs. polyglot files that survive the upload sniff. let headers = [ (header::CONTENT_TYPE, ct.to_string()), (header::CONTENT_LENGTH, file.size_bytes.to_string()), + ( + HeaderName::from_static("x-content-type-options"), + "nosniff".to_string(), + ), ]; Ok((headers, Body::from_stream(file.stream)).into_response()) } diff --git a/backend/tests/api_uploads.rs b/backend/tests/api_uploads.rs index 68a004e..385bc62 100644 --- a/backend/tests/api_uploads.rs +++ b/backend/tests/api_uploads.rs @@ -162,6 +162,12 @@ async fn files_endpoint_streams_in_multiple_frames(pool: PgPool) { resp.headers().get(header::CONTENT_LENGTH).unwrap(), big.len().to_string().as_str() ); + // Browsers must trust the declared Content-Type rather than sniff + // the body — the upload-time magic-byte check is authoritative. + assert_eq!( + resp.headers().get("x-content-type-options").unwrap(), + "nosniff" + ); let mut body = resp.into_body(); let mut frames = 0usize; @@ -323,6 +329,41 @@ async fn create_chapter_requires_authentication(pool: PgPool) { assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); } +#[sqlx::test(migrations = "./migrations")] +async fn manga_upload_rolls_back_when_cover_storage_fails(pool: PgPool) { + // First `put` call errors. The manga create handler is the only + // thing that hits storage here, so the cover put on the first + // request triggers the injected failure and the transaction must + // roll back. + let h = common::harness_with_failing_storage(pool.clone(), 0); + let (_, cookie) = common::register_user(&h.app).await; + + let resp = h + .app + .oneshot(common::post_multipart_with_cookie( + "/api/v1/mangas", + MultipartBuilder::new() + .add_json("metadata", json!({ "title": "Berserk" })) + .add_file("cover", "cover.png", "image/png", &common::fake_png_bytes()), + &cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::INTERNAL_SERVER_ERROR); + let body = common::body_json(resp).await; + assert_eq!(body["error"]["code"], "internal_error"); + + // No manga row with that title — the INSERT inside the tx was + // rolled back when the cover put failed. + let (count,): (i64,) = + sqlx::query_as("SELECT count(*) FROM mangas WHERE title = $1") + .bind("Berserk") + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!(count, 0, "rolled-back manga must not persist"); +} + #[sqlx::test(migrations = "./migrations")] async fn chapter_upload_rolls_back_when_storage_fails_mid_loop(pool: PgPool) { // Configure storage so the second `put` call (0-indexed: index 1) diff --git a/frontend/package.json b/frontend/package.json index 9702efb..e8c2653 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.10.0", + "version": "0.10.1", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/hooks.server.test.ts b/frontend/src/hooks.server.test.ts index 59f620d..f6b278c 100644 --- a/frontend/src/hooks.server.test.ts +++ b/frontend/src/hooks.server.test.ts @@ -101,4 +101,21 @@ describe('hooks.server proxy', () => { expect(fetchSpy).not.toHaveBeenCalled(); expect(resolve).toHaveBeenCalledTimes(1); }); + + it('returns 502 with the standard error envelope when the upstream is unreachable', async () => { + // Silence the console.error the handler emits on failure so + // the test output stays clean. + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + fetchSpy.mockRejectedValueOnce(new TypeError('fetch failed')); + + const resolve = vi.fn(); + const resp = await handle({ event: makeEvent('/api/v1/health'), resolve }); + + expect(resolve).not.toHaveBeenCalled(); + expect(resp.status).toBe(502); + expect(resp.headers.get('content-type')).toContain('application/json'); + const body = await resp.json(); + expect(body.error.code).toBe('upstream_unavailable'); + expect(errSpy).toHaveBeenCalled(); + }); }); diff --git a/frontend/src/hooks.server.ts b/frontend/src/hooks.server.ts index 770d9af..2fa9912 100644 --- a/frontend/src/hooks.server.ts +++ b/frontend/src/hooks.server.ts @@ -34,7 +34,30 @@ export const handle: Handle = async ({ event, resolve }) => { init.duplex = 'half'; } - const upstream = await fetch(target, init); + let upstream: Response; + try { + upstream = await fetch(target, init); + } catch (e) { + // Network-layer failure (DNS / connection refused / TLS + // handshake) — most commonly "backend container restarting". + // SvelteKit's default 500 would be an HTML page that + // client.ts can't .json(), which masks the real cause. Emit + // the standard envelope with a dedicated code instead. + console.error('Proxy to backend failed:', e); + return new Response( + JSON.stringify({ + error: { + code: 'upstream_unavailable', + message: 'backend unreachable' + } + }), + { + status: 502, + headers: { 'content-type': 'application/json' } + } + ); + } + return new Response(upstream.body, { status: upstream.status, statusText: upstream.statusText, diff --git a/frontend/src/routes/bookmarks/+page.svelte b/frontend/src/routes/bookmarks/+page.svelte index ea3a410..150638f 100644 --- a/frontend/src/routes/bookmarks/+page.svelte +++ b/frontend/src/routes/bookmarks/+page.svelte @@ -23,7 +23,7 @@ {#if b.chapter_id && b.chapter_number != null} Chapter {b.chapter_number} - {#if b.page}— page {b.page}{/if} + {#if b.page != null && b.page > 0}— page {b.page}{/if} {:else if b.chapter_id}