From 1eb40433a906905c23475dcb60960518e48ddc4c Mon Sep 17 00:00:00 2001 From: funman300 Date: Sun, 17 May 2026 20:22:38 -0700 Subject: [PATCH] fix(server): auth-guard avatar serving, atomic write, user_id assertion in merge - Move /avatars ServeDir behind require_auth middleware so avatar files can only be fetched by authenticated users (H-11) - Make avatar upload atomic via .tmp write + rename, cleaning up stale extensions only after the rename succeeds (H-12) - Return 401 instead of silently returning an empty username string when the user row is unexpectedly missing a username (L-17) - Add user_id mismatch guard to merge(): returns local payload unchanged with a ConflictReport rather than silently cross-contaminating data (H-2) - Truncate opt-in display_name to 32 chars client-side before sending, matching the server's DISPLAY_NAME_MAX validation (L-5) Co-Authored-By: Claude Sonnet 4.6 --- solitaire_engine/src/leaderboard_plugin.rs | 2 +- solitaire_server/src/auth.rs | 20 +++++++++++++------- solitaire_server/src/lib.rs | 2 +- solitaire_sync/src/merge.rs | 9 +++++++++ 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/solitaire_engine/src/leaderboard_plugin.rs b/solitaire_engine/src/leaderboard_plugin.rs index a7a6f41..1cc50a5 100644 --- a/solitaire_engine/src/leaderboard_plugin.rs +++ b/solitaire_engine/src/leaderboard_plugin.rs @@ -350,7 +350,7 @@ fn handle_opt_in_button( None } }) - .map(str::to_string) + .map(|n| n.chars().take(32).collect::()) }) .unwrap_or_else(|| "Player".to_string()); diff --git a/solitaire_server/src/auth.rs b/solitaire_server/src/auth.rs index 4333695..b671afd 100644 --- a/solitaire_server/src/auth.rs +++ b/solitaire_server/src/auth.rs @@ -336,7 +336,7 @@ pub async fn get_me( Ok(Json(MeResponse { id: user.user_id, - username: row.username.unwrap_or_default(), + username: row.username.ok_or(AppError::Unauthorized)?, avatar_url: row.avatar_url, })) } @@ -386,13 +386,19 @@ pub async fn upload_avatar( std::fs::create_dir_all("avatars").map_err(|e| AppError::Internal(e.to_string()))?; let filename = format!("{}.{}", user.user_id, ext); let path = std::path::Path::new("avatars").join(&filename); - // Remove stale files with other extensions first. + let tmp_path = std::path::Path::new("avatars").join(format!("{}.{}.tmp", user.user_id, ext)); + // Write to a temp file then atomically rename so concurrent readers never + // see a partially-written avatar. + std::fs::write(&tmp_path, &body).map_err(|e| AppError::Internal(e.to_string()))?; + std::fs::rename(&tmp_path, &path).map_err(|e| AppError::Internal(e.to_string()))?; + // Remove stale files with other extensions after the atomic rename. for old_ext in &["jpg", "png", "webp", "gif"] { - let _ = std::fs::remove_file( - std::path::Path::new("avatars").join(format!("{}.{}", user.user_id, old_ext)), - ); + if *old_ext != ext { + let _ = std::fs::remove_file( + std::path::Path::new("avatars").join(format!("{}.{}", user.user_id, old_ext)), + ); + } } - std::fs::write(&path, &body).map_err(|e| AppError::Internal(e.to_string()))?; let avatar_url = format!("/avatars/{filename}"); sqlx::query!( @@ -412,7 +418,7 @@ pub async fn upload_avatar( Ok(Json(MeResponse { id: user.user_id, - username: username.unwrap_or_default(), + username: username.ok_or(AppError::Unauthorized)?, avatar_url: Some(avatar_url), })) } diff --git a/solitaire_server/src/lib.rs b/solitaire_server/src/lib.rs index 0079adb..5c4fdb7 100644 --- a/solitaire_server/src/lib.rs +++ b/solitaire_server/src/lib.rs @@ -146,6 +146,7 @@ fn build_router_inner(state: AppState, rate_limit: bool) -> Router { .route("/api/account", delete(auth::delete_account)) .route("/api/me", get(auth::get_me)) .route("/api/me/avatar", put(auth::upload_avatar)) + .nest_service("/avatars", ServeDir::new("avatars")) .layer(axum_middleware::from_fn_with_state( state.clone(), middleware::require_auth, @@ -231,7 +232,6 @@ fn build_router_inner(state: AppState, rate_limit: bool) -> Router { ) .nest_service("/web", ServeDir::new("solitaire_server/web")) .nest_service("/assets", ServeDir::new("assets")) - .nest_service("/avatars", ServeDir::new("avatars")) .layer(axum_middleware::from_fn(security_headers)); Router::new() diff --git a/solitaire_sync/src/merge.rs b/solitaire_sync/src/merge.rs index b1bd3a4..2198318 100644 --- a/solitaire_sync/src/merge.rs +++ b/solitaire_sync/src/merge.rs @@ -47,6 +47,15 @@ use crate::progress::{level_for_xp, DAILY_CHALLENGE_HISTORY_CAP}; pub fn merge(local: &SyncPayload, remote: &SyncPayload) -> (SyncPayload, Vec) { let mut conflicts = Vec::new(); + if local.user_id != remote.user_id { + conflicts.push(ConflictReport { + field: "user_id".to_string(), + local_value: local.user_id.to_string(), + remote_value: remote.user_id.to_string(), + }); + return (local.clone(), conflicts); + } + let stats = merge_stats(&local.stats, &remote.stats, &mut conflicts); let achievements = merge_achievements(&local.achievements, &remote.achievements); let progress = merge_progress(&local.progress, &remote.progress, &mut conflicts);