fix(server): login timing pad, 409 on register race, avatar magic-byte check

Closes the three actionable server Low findings from the 2026-07-06
review:

- #139: unknown-username logins now verify against a static bcrypt
  dummy hash so both failure paths pay the same cost — response timing
  no longer reveals which usernames exist.
- #140: register maps a unique-constraint violation to UsernameTaken
  (409) — the SELECT pre-check stays as the friendly fast path, the
  constraint is the arbiter for the concurrent case.
- #141: avatar uploads must start with the magic bytes of the declared
  image type; the stored extension (which decides how the file is
  re-served) is now backed by content, not the Content-Type header.

Unit tests: real/spoofed/truncated signatures for all four formats,
and the dummy hash's validity at BCRYPT_COST.

Closes #139
Closes #140
Closes #141

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

solitaire_server/src/auth.rs

@@ -74,6 +74,15 @@ struct UserRow {
 /// bcrypt work factor. Cost 12 ≈ 300 ms on modern hardware — balances security against registration latency.
 pub const BCRYPT_COST: u32 = 12;
 
+/// Static bcrypt hash used to equalise login timing when the username does
+/// not exist (issue #139: user-enumeration timing oracle). Both login paths
+/// must pay the same bcrypt cost; this hash is verified against when there
+/// is no real one. Computed once at first use with the same [`BCRYPT_COST`]
+/// as real hashes. `None` only if bcrypt itself fails on a constant input —
+/// in that case the dummy verify is skipped rather than panicking.
+static DUMMY_PASSWORD_HASH: std::sync::LazyLock<Option<String>> =
+    std::sync::LazyLock::new(|| hash("ferrous-dummy-timing-pad", BCRYPT_COST).ok());
+
 async fn hash_password(password: String) -> Result<String, AppError> {
     tokio::task::spawn_blocking(move || hash(password, BCRYPT_COST))
         .await
@@ -208,6 +217,10 @@ pub async fn register(
     let password_hash = hash_password(body.password).await?;
     let now = Utc::now().to_rfc3339();
 
+    // The SELECT above is a friendly fast path; the UNIQUE constraint is the
+    // real arbiter. A concurrent registration that slips between the two
+    // surfaces as a unique violation here — map it to the same 409 the
+    // fast path produces instead of a raw 500 (issue #140).
     sqlx::query!(
         "INSERT INTO users (id, username, password_hash, created_at) VALUES (?, ?, ?, ?)",
         user_id,
@@ -216,7 +229,11 @@ pub async fn register(
         now
     )
     .execute(&state.pool)
-    .await?;
+    .await
+    .map_err(|e| match &e {
+        sqlx::Error::Database(db) if db.is_unique_violation() => AppError::UsernameTaken,
+        _ => AppError::from(e),
+    })?;
 
     let access_token = make_access_token(&user_id, &state.jwt_secret)?;
     let (refresh_token, refresh_jti) = make_refresh_token(&user_id, &state.jwt_secret)?;
@@ -242,7 +259,16 @@ pub async fn login(
     .fetch_optional(&state.pool)
     .await?;
 
-    let row = row.ok_or(AppError::InvalidCredentials)?;
+    let Some(row) = row else {
+        // Unknown username: burn a bcrypt verify against a static dummy hash
+        // so this path costs the same as a wrong-password attempt. Returning
+        // immediately here would let response timing reveal which usernames
+        // exist (issue #139).
+        if let Some(dummy) = DUMMY_PASSWORD_HASH.as_ref() {
+            let _ = verify_password(body.password, dummy.clone()).await;
+        }
+        return Err(AppError::InvalidCredentials);
+    };
     let row_id = row
         .id
         .ok_or_else(|| AppError::Internal("user id missing".into()))?;
@@ -362,6 +388,22 @@ const AVATAR_MAX_BYTES: usize = 1024 * 1024;
 ///
 /// The `Content-Type` header must be one of `image/jpeg`, `image/png`,
 /// `image/webp`, or `image/gif`. The previous avatar file is replaced in-place.
+/// Returns `true` when `body` begins with the magic bytes of the image type
+/// implied by `ext` (the extension derived from the declared Content-Type).
+///
+/// Avatars are re-served under the stored extension, so the extension must be
+/// derived from the content itself — a client header is not evidence of what
+/// the bytes are (issue #141).
+fn magic_bytes_match(ext: &str, body: &[u8]) -> bool {
+    match ext {
+        "png" => body.starts_with(&[0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A]),
+        "jpg" => body.starts_with(&[0xFF, 0xD8, 0xFF]),
+        "gif" => body.starts_with(b"GIF87a") || body.starts_with(b"GIF89a"),
+        "webp" => body.len() >= 12 && body.starts_with(b"RIFF") && &body[8..12] == b"WEBP",
+        _ => false,
+    }
+}
+
 pub async fn upload_avatar(
     State(state): State<AppState>,
     user: AuthenticatedUser,
@@ -387,6 +429,14 @@ pub async fn upload_avatar(
     if body.len() > AVATAR_MAX_BYTES {
         return Err(AppError::BadRequest("avatar must be ≤ 1 MB".into()));
     }
+    // The stored extension decides how the file is re-served, so it must be
+    // backed by the bytes, not just the client's Content-Type header
+    // (issue #141).
+    if !magic_bytes_match(ext, &body) {
+        return Err(AppError::BadRequest(
+            "avatar bytes do not match the declared image type".into(),
+        ));
+    }
 
     // Write to avatars/ directory, replacing any previous file for this user.
     tokio::fs::create_dir_all("avatars")
@@ -587,6 +637,46 @@ mod tests {
         assert!(username_chars_ok(""));
     }
 
+    #[test]
+    fn magic_bytes_match_accepts_real_signatures() {
+        assert!(magic_bytes_match(
+            "png",
+            &[0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A, 0x00]
+        ));
+        assert!(magic_bytes_match("jpg", &[0xFF, 0xD8, 0xFF, 0xE0, 0x00]));
+        assert!(magic_bytes_match("gif", b"GIF89a\x00\x00"));
+        assert!(magic_bytes_match("gif", b"GIF87a\x00\x00"));
+        assert!(magic_bytes_match("webp", b"RIFF\x00\x00\x00\x00WEBPVP8 "));
+    }
+
+    #[test]
+    fn magic_bytes_match_rejects_mismatched_or_bogus_content() {
+        // HTML declared as PNG — the exact spoof the check exists to stop.
+        assert!(!magic_bytes_match("png", b"<html><script>"));
+        // Real PNG bytes declared as JPEG: extension must match the bytes.
+        assert!(!magic_bytes_match(
+            "jpg",
+            &[0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A]
+        ));
+        // Truncated / empty bodies never match.
+        assert!(!magic_bytes_match("png", &[0x89, b'P']));
+        assert!(!magic_bytes_match("webp", b"RIFF1234WEB"));
+        assert!(!magic_bytes_match("gif", b""));
+        // Unknown extensions are never accepted.
+        assert!(!magic_bytes_match("svg", b"<svg xmlns="));
+    }
+
+    #[test]
+    fn dummy_password_hash_is_available_and_verifiable() {
+        // The login timing pad (issue #139) must be a real bcrypt hash so the
+        // unknown-user path pays a genuine verify at BCRYPT_COST.
+        let dummy = DUMMY_PASSWORD_HASH
+            .as_ref()
+            .expect("bcrypt of a constant input must succeed");
+        assert!(verify("ferrous-dummy-timing-pad", dummy).unwrap_or(false));
+        assert!(!verify("wrong-password", dummy).unwrap_or(true));
+    }
+
     #[test]
     fn username_chars_ok_rejects_unicode_letters() {
         // Non-ASCII characters must be rejected even if they look like letters.