test: tighten audit-flagged tests and add missing coverage

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) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-05-16 23:30:19 +02:00
parent 785b9755cf
commit 2df4084c56
5 changed files with 243 additions and 8 deletions

2
backend/Cargo.lock generated
View File

@@ -1033,7 +1033,7 @@ checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897"
[[package]] [[package]]
name = "mangalord" name = "mangalord"
version = "0.9.3" version = "0.9.4"
dependencies = [ dependencies = [
"anyhow", "anyhow",
"argon2", "argon2",

View File

@@ -48,7 +48,7 @@ pub fn router(state: AppState) -> Router {
.layer(TraceLayer::new_for_http()) .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() { if allowed_origins.is_empty() {
// Same-origin only — no CORS headers emitted. // Same-origin only — no CORS headers emitted.
return CorsLayer::new(); return CorsLayer::new();
@@ -66,3 +66,80 @@ fn cors_layer(allowed_origins: &[String]) -> CorsLayer {
HeaderName::from_static("authorization"), 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());
}
}

View File

@@ -102,9 +102,18 @@ mod tests {
async fn rejects_path_traversal() { async fn rejects_path_traversal() {
let dir = tempdir().unwrap(); let dir = tempdir().unwrap();
let s = LocalStorage::new(dir.path()); let s = LocalStorage::new(dir.path());
// Parent-dir reference at the start.
assert!(matches!(s.put("../escape", b"x").await, Err(StorageError::BadKey))); 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))); assert!(matches!(s.get("a/../../b").await, Err(StorageError::BadKey)));
// Empty key.
assert!(matches!(s.exists("").await, Err(StorageError::BadKey))); 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] #[tokio::test]

View File

@@ -110,21 +110,41 @@ async fn register_rejects_short_password(pool: PgPool) {
#[sqlx::test(migrations = "./migrations")] #[sqlx::test(migrations = "./migrations")]
async fn login_succeeds_and_rotates_session(pool: PgPool) { async fn login_succeeds_and_rotates_session(pool: PgPool) {
let h = common::harness(pool); let h = common::harness(pool);
let _ = h let register_resp = h
.app .app
.clone() .clone()
.oneshot(common::post_json("/api/v1/auth/register", creds("alice"))) .oneshot(common::post_json("/api/v1/auth/register", creds("alice")))
.await .await
.unwrap(); .unwrap();
let register_cookie = common::extract_session_cookie(&register_resp)
.expect("register sets a cookie");
let resp = h let login_resp = h
.app .app
.clone()
.oneshot(common::post_json("/api/v1/auth/login", creds("alice"))) .oneshot(common::post_json("/api/v1/auth/login", creds("alice")))
.await .await
.unwrap(); .unwrap();
assert_eq!(resp.status(), StatusCode::OK); assert_eq!(login_resp.status(), StatusCode::OK);
let cookie = common::extract_session_cookie(&resp).expect("login sets a cookie"); let login_cookie =
assert!(cookie.starts_with("mangalord_session=")); 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", &register_cookie))
.await
.unwrap();
assert_eq!(me_resp.status(), StatusCode::OK);
} }
#[sqlx::test(migrations = "./migrations")] #[sqlx::test(migrations = "./migrations")]
@@ -213,6 +233,44 @@ async fn logout_clears_session(pool: PgPool) {
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); 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")] #[sqlx::test(migrations = "./migrations")]
async fn create_and_use_bot_token(pool: PgPool) { async fn create_and_use_bot_token(pool: PgPool) {
let h = common::harness(pool); let h = common::harness(pool);
@@ -235,7 +293,14 @@ async fn create_and_use_bot_token(pool: PgPool) {
.as_str() .as_str()
.expect("raw bearer in response") .expect("raw bearer in response")
.to_string(); .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. // Use the bearer to hit /me — should authenticate.
let resp = h let resp = h

View File

@@ -187,6 +187,90 @@ async fn user_a_cannot_delete_user_b_bookmark(pool: PgPool) {
assert_eq!(resp.status(), StatusCode::NO_CONTENT); 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")] #[sqlx::test(migrations = "./migrations")]
async fn delete_unknown_bookmark_is_404(pool: PgPool) { async fn delete_unknown_bookmark_is_404(pool: PgPool) {
let h = common::harness(pool); let h = common::harness(pool);