bugfix: case-insensitive usernames, reject non-positive bookmark page
Two related correctness fixes from the audit: - Username uniqueness was case-sensitive (`username text UNIQUE`), so "Alice" and "alice" could both register and then race on login. Migration 0006 adds a unique index on `lower(username)`; the existing constraint is kept (overlapping but cheap) to avoid a destructive migration on any deployments that may already exist. `repo::user::find_by_username` now matches on `lower(username) = lower($1)` so login is case-insensitive against the same index. Test: registering "alice" then "Alice" returns 409 conflict; login with "ALICE" succeeds against the existing user. - `POST /api/v1/bookmarks` silently accepted `page: 0` and `page: -1` even though both are nonsense for a 1-indexed page number. Reject with 422 `validation_failed` and `details.page` populated, matching the pattern used for missing-metadata / empty-title elsewhere. Test covers both 0 and -1. Lockstep version bump to 0.9.4. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
2
backend/Cargo.lock
generated
2
backend/Cargo.lock
generated
@@ -1033,7 +1033,7 @@ checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897"
|
|||||||
|
|
||||||
[[package]]
|
[[package]]
|
||||||
name = "mangalord"
|
name = "mangalord"
|
||||||
version = "0.9.2"
|
version = "0.9.3"
|
||||||
dependencies = [
|
dependencies = [
|
||||||
"anyhow",
|
"anyhow",
|
||||||
"argon2",
|
"argon2",
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
[package]
|
[package]
|
||||||
name = "mangalord"
|
name = "mangalord"
|
||||||
version = "0.9.3"
|
version = "0.9.4"
|
||||||
edition = "2021"
|
edition = "2021"
|
||||||
|
|
||||||
[lib]
|
[lib]
|
||||||
|
|||||||
15
backend/migrations/0006_username_case_insensitive.sql
Normal file
15
backend/migrations/0006_username_case_insensitive.sql
Normal file
@@ -0,0 +1,15 @@
|
|||||||
|
-- Make username uniqueness case-insensitive.
|
||||||
|
--
|
||||||
|
-- The 0001 schema declared `username text NOT NULL UNIQUE`, which let
|
||||||
|
-- "Alice" and "alice" both register and then race for who gets which
|
||||||
|
-- session cookie on subsequent logins. Adding a partial / functional
|
||||||
|
-- unique index over `lower(username)` blocks the conflict at the DB
|
||||||
|
-- layer regardless of how the application normalises the input.
|
||||||
|
--
|
||||||
|
-- The original `username UNIQUE` constraint is kept — it now overlaps
|
||||||
|
-- with the new index but the duplication is cheap and removing the
|
||||||
|
-- inline constraint would require a multi-step destructive migration
|
||||||
|
-- on existing data.
|
||||||
|
|
||||||
|
CREATE UNIQUE INDEX users_username_lower_uniq
|
||||||
|
ON users (lower(username));
|
||||||
@@ -7,6 +7,7 @@ use axum::http::StatusCode;
|
|||||||
use axum::routing::{delete, get, post};
|
use axum::routing::{delete, get, post};
|
||||||
use axum::{Json, Router};
|
use axum::{Json, Router};
|
||||||
use serde::Deserialize;
|
use serde::Deserialize;
|
||||||
|
use serde_json::json;
|
||||||
use uuid::Uuid;
|
use uuid::Uuid;
|
||||||
|
|
||||||
use crate::api::pagination::PagedResponse;
|
use crate::api::pagination::PagedResponse;
|
||||||
@@ -49,6 +50,18 @@ async fn create(
|
|||||||
CurrentUser(user): CurrentUser,
|
CurrentUser(user): CurrentUser,
|
||||||
Json(input): Json<NewBookmark>,
|
Json(input): Json<NewBookmark>,
|
||||||
) -> AppResult<(StatusCode, Json<Bookmark>)> {
|
) -> AppResult<(StatusCode, Json<Bookmark>)> {
|
||||||
|
// Reject obviously-bad page numbers up front (0-based or negative
|
||||||
|
// page indexes were silently accepted before; not exploitable but
|
||||||
|
// not what callers mean).
|
||||||
|
if let Some(p) = input.page {
|
||||||
|
if p < 1 {
|
||||||
|
return Err(AppError::ValidationFailed {
|
||||||
|
message: "page must be 1 or greater".into(),
|
||||||
|
details: json!({ "page": "must be >= 1" }),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Surface 404 on a non-existent manga / chapter rather than letting
|
// Surface 404 on a non-existent manga / chapter rather than letting
|
||||||
// the foreign-key violation collapse into a generic 500.
|
// the foreign-key violation collapse into a generic 500.
|
||||||
repo::manga::get(&state.db, input.manga_id).await?;
|
repo::manga::get(&state.db, input.manga_id).await?;
|
||||||
|
|||||||
@@ -28,9 +28,17 @@ pub async fn create(pool: &PgPool, username: &str, password_hash: &str) -> AppRe
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Case-insensitive lookup so login with "Alice" matches a user
|
||||||
|
/// registered as "alice" (the unique index on `lower(username)` keeps
|
||||||
|
/// the comparison cheap). Equivalent in spirit to ILIKE but uses the
|
||||||
|
/// functional index directly.
|
||||||
pub async fn find_by_username(pool: &PgPool, username: &str) -> AppResult<Option<User>> {
|
pub async fn find_by_username(pool: &PgPool, username: &str) -> AppResult<Option<User>> {
|
||||||
let row = sqlx::query_as::<_, User>(
|
let row = sqlx::query_as::<_, User>(
|
||||||
r#"SELECT id, username, password_hash, created_at FROM users WHERE username = $1"#,
|
r#"
|
||||||
|
SELECT id, username, password_hash, created_at
|
||||||
|
FROM users
|
||||||
|
WHERE lower(username) = lower($1)
|
||||||
|
"#,
|
||||||
)
|
)
|
||||||
.bind(username)
|
.bind(username)
|
||||||
.fetch_optional(pool)
|
.fetch_optional(pool)
|
||||||
|
|||||||
@@ -64,6 +64,33 @@ async fn register_rejects_duplicate_username_with_conflict(pool: PgPool) {
|
|||||||
assert_eq!(body["error"]["code"], "conflict");
|
assert_eq!(body["error"]["code"], "conflict");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[sqlx::test(migrations = "./migrations")]
|
||||||
|
async fn register_rejects_case_only_username_collisions(pool: PgPool) {
|
||||||
|
let h = common::harness(pool);
|
||||||
|
let _ = h
|
||||||
|
.app
|
||||||
|
.clone()
|
||||||
|
.oneshot(common::post_json("/api/v1/auth/register", creds("alice")))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
// Mixed-case variant collides via the lower(username) index.
|
||||||
|
let resp = h
|
||||||
|
.app
|
||||||
|
.clone()
|
||||||
|
.oneshot(common::post_json("/api/v1/auth/register", creds("Alice")))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(resp.status(), StatusCode::CONFLICT);
|
||||||
|
|
||||||
|
// Login with either casing finds the same user.
|
||||||
|
let resp = h
|
||||||
|
.app
|
||||||
|
.oneshot(common::post_json("/api/v1/auth/login", creds("ALICE")))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(resp.status(), StatusCode::OK);
|
||||||
|
}
|
||||||
|
|
||||||
#[sqlx::test(migrations = "./migrations")]
|
#[sqlx::test(migrations = "./migrations")]
|
||||||
async fn register_rejects_short_password(pool: PgPool) {
|
async fn register_rejects_short_password(pool: PgPool) {
|
||||||
let h = common::harness(pool);
|
let h = common::harness(pool);
|
||||||
|
|||||||
@@ -89,6 +89,39 @@ async fn create_404_on_unknown_manga(pool: PgPool) {
|
|||||||
assert_eq!(resp.status(), StatusCode::NOT_FOUND);
|
assert_eq!(resp.status(), StatusCode::NOT_FOUND);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[sqlx::test(migrations = "./migrations")]
|
||||||
|
async fn create_rejects_non_positive_page_with_422(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 resp = h
|
||||||
|
.app
|
||||||
|
.clone()
|
||||||
|
.oneshot(common::post_json_with_cookie(
|
||||||
|
"/api/v1/bookmarks",
|
||||||
|
json!({ "manga_id": manga_id.to_string(), "page": 0 }),
|
||||||
|
&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"]["page"].is_string());
|
||||||
|
|
||||||
|
let resp = h
|
||||||
|
.app
|
||||||
|
.oneshot(common::post_json_with_cookie(
|
||||||
|
"/api/v1/bookmarks",
|
||||||
|
json!({ "manga_id": manga_id.to_string(), "page": -1 }),
|
||||||
|
&cookie,
|
||||||
|
))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(resp.status(), StatusCode::UNPROCESSABLE_ENTITY);
|
||||||
|
}
|
||||||
|
|
||||||
#[sqlx::test(migrations = "./migrations")]
|
#[sqlx::test(migrations = "./migrations")]
|
||||||
async fn create_requires_authentication(pool: PgPool) {
|
async fn create_requires_authentication(pool: PgPool) {
|
||||||
let h = common::harness(pool);
|
let h = common::harness(pool);
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"name": "mangalord-frontend",
|
"name": "mangalord-frontend",
|
||||||
"version": "0.9.3",
|
"version": "0.9.4",
|
||||||
"private": true,
|
"private": true,
|
||||||
"type": "module",
|
"type": "module",
|
||||||
"scripts": {
|
"scripts": {
|
||||||
|
|||||||
Reference in New Issue
Block a user