From 2df4084c563bb8eee504797b79ae38b54e1c1071 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 16 May 2026 23:30:19 +0200 Subject: [PATCH] test: tighten audit-flagged tests and add missing coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tightens three tests whose names overstated what they checked: - `login_succeeds_and_rotates_session` now asserts the login cookie differs from the registration cookie, and that the registration cookie is still valid after login (the documented contract). - `storage::local::rejects_path_traversal` exercises three extra rejection paths the existing implementation already handled but the tests didn't probe: `a/./b`, the single-segment `.`, and the empty segment `a//b`. - `create_and_use_bot_token` asserts that `token_hash` is *absent* from the response (`get(...).is_none()`), not just `is_null()`, which would have accepted an explicit `"token_hash": null` payload too. Adds four coverage cases that the audit flagged as missing: - `me_rejects_expired_session` — hand-craft a session row with `expires_at = now() - 1h`, hit `/auth/me` with the matching cookie, expect 401 + `unauthenticated`. Proves the extractor's `expires_at > now()` filter is wired. - `concurrent_manga_bookmarks_serialised_by_unique_index` — spawn two POSTs in parallel for the same `(user, manga, chapter=null)`, assert one wins (201) and one collides (409) via the partial unique index from migration 0004. - `bookmark_create_accepts_bearer_token` — mint a bot token and POST /bookmarks with `Authorization: Bearer`, asserting `CurrentUser` resolves identically to the cookie path on a write endpoint (not just `/auth/me`). - Three new unit tests on `app::cors_layer` covering the allowlist (origin reflected, credentials true), a foreign origin (no allow-origin header emitted), and the same-origin default (empty allowlist emits no CORS headers at all). `cors_layer` is `pub(crate)` now so the tests in `app::tests` can reach it; the function itself is unchanged. No version bump. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/Cargo.lock | 2 +- backend/src/app.rs | 79 +++++++++++++++++++++++++++++++- backend/src/storage/local.rs | 9 ++++ backend/tests/api_auth.rs | 77 ++++++++++++++++++++++++++++--- backend/tests/api_bookmarks.rs | 84 ++++++++++++++++++++++++++++++++++ 5 files changed, 243 insertions(+), 8 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 10937ec..cb4ee23 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1033,7 +1033,7 @@ checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897" [[package]] name = "mangalord" -version = "0.9.3" +version = "0.9.4" dependencies = [ "anyhow", "argon2", diff --git a/backend/src/app.rs b/backend/src/app.rs index 93c3382..58d8aa6 100644 --- a/backend/src/app.rs +++ b/backend/src/app.rs @@ -48,7 +48,7 @@ pub fn router(state: AppState) -> Router { .layer(TraceLayer::new_for_http()) } -fn cors_layer(allowed_origins: &[String]) -> CorsLayer { +pub(crate) fn cors_layer(allowed_origins: &[String]) -> CorsLayer { if allowed_origins.is_empty() { // Same-origin only — no CORS headers emitted. return CorsLayer::new(); @@ -66,3 +66,80 @@ fn cors_layer(allowed_origins: &[String]) -> CorsLayer { HeaderName::from_static("authorization"), ]) } + +#[cfg(test)] +mod tests { + use super::*; + use axum::body::Body; + use axum::http::Request; + use axum::routing::get; + use tower::ServiceExt; + + fn test_router() -> Router { + Router::new().route("/", get(|| async { "ok" })) + } + + #[tokio::test] + async fn allowlist_preflight_emits_credentialed_headers() { + let app = test_router().layer(cors_layer(&["https://app.example.com".to_string()])); + let resp = app + .oneshot( + Request::builder() + .method(Method::OPTIONS) + .uri("/") + .header("origin", "https://app.example.com") + .header("access-control-request-method", "POST") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + resp.headers().get("access-control-allow-origin").unwrap(), + "https://app.example.com" + ); + assert_eq!( + resp.headers().get("access-control-allow-credentials").unwrap(), + "true" + ); + } + + #[tokio::test] + async fn allowlist_rejects_unlisted_origin() { + let app = test_router().layer(cors_layer(&["https://app.example.com".to_string()])); + let resp = app + .oneshot( + Request::builder() + .method(Method::OPTIONS) + .uri("/") + .header("origin", "https://evil.example.org") + .header("access-control-request-method", "POST") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + // Browsers will refuse the response when the allow-origin header + // is absent (or doesn't echo the requesting origin). + assert!(resp.headers().get("access-control-allow-origin").is_none()); + } + + #[tokio::test] + async fn empty_allowlist_is_same_origin_only() { + let app = test_router().layer(cors_layer(&[])); + let resp = app + .oneshot( + Request::builder() + .method(Method::OPTIONS) + .uri("/") + .header("origin", "https://app.example.com") + .header("access-control-request-method", "POST") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert!(resp.headers().get("access-control-allow-origin").is_none()); + assert!(resp.headers().get("access-control-allow-credentials").is_none()); + } +} diff --git a/backend/src/storage/local.rs b/backend/src/storage/local.rs index ddedfb1..7e6dd23 100644 --- a/backend/src/storage/local.rs +++ b/backend/src/storage/local.rs @@ -102,9 +102,18 @@ mod tests { async fn rejects_path_traversal() { let dir = tempdir().unwrap(); let s = LocalStorage::new(dir.path()); + // Parent-dir reference at the start. assert!(matches!(s.put("../escape", b"x").await, Err(StorageError::BadKey))); + // Parent-dir reference mid-path. assert!(matches!(s.get("a/../../b").await, Err(StorageError::BadKey))); + // Empty key. assert!(matches!(s.exists("").await, Err(StorageError::BadKey))); + // Current-dir reference (the implementation rejects `.` segments + // alongside `..`; this exercises that arm). + assert!(matches!(s.get("a/./b").await, Err(StorageError::BadKey))); + assert!(matches!(s.get(".").await, Err(StorageError::BadKey))); + // Empty segment via doubled slash. + assert!(matches!(s.get("a//b").await, Err(StorageError::BadKey))); } #[tokio::test] diff --git a/backend/tests/api_auth.rs b/backend/tests/api_auth.rs index 5934a89..689b067 100644 --- a/backend/tests/api_auth.rs +++ b/backend/tests/api_auth.rs @@ -110,21 +110,41 @@ async fn register_rejects_short_password(pool: PgPool) { #[sqlx::test(migrations = "./migrations")] async fn login_succeeds_and_rotates_session(pool: PgPool) { let h = common::harness(pool); - let _ = h + let register_resp = h .app .clone() .oneshot(common::post_json("/api/v1/auth/register", creds("alice"))) .await .unwrap(); + let register_cookie = common::extract_session_cookie(®ister_resp) + .expect("register sets a cookie"); - let resp = h + let login_resp = h .app + .clone() .oneshot(common::post_json("/api/v1/auth/login", creds("alice"))) .await .unwrap(); - assert_eq!(resp.status(), StatusCode::OK); - let cookie = common::extract_session_cookie(&resp).expect("login sets a cookie"); - assert!(cookie.starts_with("mangalord_session=")); + assert_eq!(login_resp.status(), StatusCode::OK); + let login_cookie = + common::extract_session_cookie(&login_resp).expect("login sets a cookie"); + assert!(login_cookie.starts_with("mangalord_session=")); + + // Login must mint a *new* session, not echo the registration one. + assert_ne!( + register_cookie, login_cookie, + "login should rotate the session token; got the register cookie back" + ); + + // The registration cookie is still valid until it expires naturally — + // that's the documented behaviour, asserted here so a regression that + // invalidates other devices' sessions on login would be noisy. + let me_resp = h + .app + .oneshot(common::get_with_cookie("/api/v1/auth/me", ®ister_cookie)) + .await + .unwrap(); + assert_eq!(me_resp.status(), StatusCode::OK); } #[sqlx::test(migrations = "./migrations")] @@ -213,6 +233,44 @@ async fn logout_clears_session(pool: PgPool) { assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); } +#[sqlx::test(migrations = "./migrations")] +async fn me_rejects_expired_session(pool: PgPool) { + use chrono::{Duration, Utc}; + use mangalord::auth::token::generate_token; + + let h = common::harness(pool.clone()); + common::register_user(&h.app).await; + + // Grab the user that was just registered so we can hand-craft an + // expired session for them. + let user_id: uuid::Uuid = sqlx::query_scalar("SELECT id FROM users LIMIT 1") + .fetch_one(&pool) + .await + .unwrap(); + + let (raw, hash) = generate_token(); + let expires_at = Utc::now() - Duration::hours(1); + sqlx::query( + "INSERT INTO sessions (user_id, token_hash, expires_at) VALUES ($1, $2, $3)", + ) + .bind(user_id) + .bind(&hash[..]) + .bind(expires_at) + .execute(&pool) + .await + .unwrap(); + + let cookie = format!("mangalord_session={raw}"); + let resp = h + .app + .oneshot(common::get_with_cookie("/api/v1/auth/me", &cookie)) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + let body = common::body_json(resp).await; + assert_eq!(body["error"]["code"], "unauthenticated"); +} + #[sqlx::test(migrations = "./migrations")] async fn create_and_use_bot_token(pool: PgPool) { let h = common::harness(pool); @@ -235,7 +293,14 @@ async fn create_and_use_bot_token(pool: PgPool) { .as_str() .expect("raw bearer in response") .to_string(); - assert!(body["token_hash"].is_null(), "token_hash must not leak"); + // `token_hash` is `#[serde(skip)]` on `ApiToken`, so it must be + // *absent* from the JSON. `is_null()` would also accept a + // `"token_hash": null` payload, which we don't want — use + // `get(...).is_none()` for the stronger assertion. + assert!( + body.get("token_hash").is_none(), + "token_hash must not appear in the response at all" + ); // Use the bearer to hit /me — should authenticate. let resp = h diff --git a/backend/tests/api_bookmarks.rs b/backend/tests/api_bookmarks.rs index 6b6d2c9..b57605b 100644 --- a/backend/tests/api_bookmarks.rs +++ b/backend/tests/api_bookmarks.rs @@ -187,6 +187,90 @@ async fn user_a_cannot_delete_user_b_bookmark(pool: PgPool) { assert_eq!(resp.status(), StatusCode::NO_CONTENT); } +#[sqlx::test(migrations = "./migrations")] +async fn concurrent_manga_bookmarks_serialised_by_unique_index(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 app_a = h.app.clone(); + let cookie_a = cookie.clone(); + let mid_a = manga_id.to_string(); + let app_b = h.app.clone(); + let cookie_b = cookie.clone(); + let mid_b = manga_id.to_string(); + + let f1 = tokio::spawn(async move { + app_a + .oneshot(common::post_json_with_cookie( + "/api/v1/bookmarks", + json!({ "manga_id": mid_a }), + &cookie_a, + )) + .await + .unwrap() + .status() + }); + let f2 = tokio::spawn(async move { + app_b + .oneshot(common::post_json_with_cookie( + "/api/v1/bookmarks", + json!({ "manga_id": mid_b }), + &cookie_b, + )) + .await + .unwrap() + .status() + }); + + let (s1, s2) = tokio::join!(f1, f2); + let statuses = [s1.unwrap(), s2.unwrap()]; + assert!( + statuses.contains(&StatusCode::CREATED), + "expected one winner with 201, got {statuses:?}" + ); + assert!( + statuses.contains(&StatusCode::CONFLICT), + "expected one loser with 409 (the partial unique index), got {statuses:?}" + ); +} + +#[sqlx::test(migrations = "./migrations")] +async fn bookmark_create_accepts_bearer_token(pool: PgPool) { + // Bot scripts use Authorization: Bearer; cover that path on a + // *write* endpoint to make sure CurrentUser resolves identically + // whether the credential is a cookie or a bearer. + 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 resp = h + .app + .clone() + .oneshot(common::post_json_with_cookie( + "/api/v1/auth/tokens", + json!({ "name": "ci-bot" }), + &cookie, + )) + .await + .unwrap(); + let bearer = common::body_json(resp).await["bearer"] + .as_str() + .unwrap() + .to_string(); + + let resp = h + .app + .oneshot(common::post_json_with_bearer( + "/api/v1/bookmarks", + json!({ "manga_id": manga_id.to_string() }), + &bearer, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::CREATED); +} + #[sqlx::test(migrations = "./migrations")] async fn delete_unknown_bookmark_is_404(pool: PgPool) { let h = common::harness(pool);