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 <noreply@anthropic.com>
This commit is contained in:
@@ -350,7 +350,7 @@ fn handle_opt_in_button(
|
|||||||
None
|
None
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
.map(str::to_string)
|
.map(|n| n.chars().take(32).collect::<String>())
|
||||||
})
|
})
|
||||||
.unwrap_or_else(|| "Player".to_string());
|
.unwrap_or_else(|| "Player".to_string());
|
||||||
|
|
||||||
|
|||||||
@@ -336,7 +336,7 @@ pub async fn get_me(
|
|||||||
|
|
||||||
Ok(Json(MeResponse {
|
Ok(Json(MeResponse {
|
||||||
id: user.user_id,
|
id: user.user_id,
|
||||||
username: row.username.unwrap_or_default(),
|
username: row.username.ok_or(AppError::Unauthorized)?,
|
||||||
avatar_url: row.avatar_url,
|
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()))?;
|
std::fs::create_dir_all("avatars").map_err(|e| AppError::Internal(e.to_string()))?;
|
||||||
let filename = format!("{}.{}", user.user_id, ext);
|
let filename = format!("{}.{}", user.user_id, ext);
|
||||||
let path = std::path::Path::new("avatars").join(&filename);
|
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"] {
|
for old_ext in &["jpg", "png", "webp", "gif"] {
|
||||||
let _ = std::fs::remove_file(
|
if *old_ext != ext {
|
||||||
std::path::Path::new("avatars").join(format!("{}.{}", user.user_id, old_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}");
|
let avatar_url = format!("/avatars/{filename}");
|
||||||
sqlx::query!(
|
sqlx::query!(
|
||||||
@@ -412,7 +418,7 @@ pub async fn upload_avatar(
|
|||||||
|
|
||||||
Ok(Json(MeResponse {
|
Ok(Json(MeResponse {
|
||||||
id: user.user_id,
|
id: user.user_id,
|
||||||
username: username.unwrap_or_default(),
|
username: username.ok_or(AppError::Unauthorized)?,
|
||||||
avatar_url: Some(avatar_url),
|
avatar_url: Some(avatar_url),
|
||||||
}))
|
}))
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -146,6 +146,7 @@ fn build_router_inner(state: AppState, rate_limit: bool) -> Router {
|
|||||||
.route("/api/account", delete(auth::delete_account))
|
.route("/api/account", delete(auth::delete_account))
|
||||||
.route("/api/me", get(auth::get_me))
|
.route("/api/me", get(auth::get_me))
|
||||||
.route("/api/me/avatar", put(auth::upload_avatar))
|
.route("/api/me/avatar", put(auth::upload_avatar))
|
||||||
|
.nest_service("/avatars", ServeDir::new("avatars"))
|
||||||
.layer(axum_middleware::from_fn_with_state(
|
.layer(axum_middleware::from_fn_with_state(
|
||||||
state.clone(),
|
state.clone(),
|
||||||
middleware::require_auth,
|
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("/web", ServeDir::new("solitaire_server/web"))
|
||||||
.nest_service("/assets", ServeDir::new("assets"))
|
.nest_service("/assets", ServeDir::new("assets"))
|
||||||
.nest_service("/avatars", ServeDir::new("avatars"))
|
|
||||||
.layer(axum_middleware::from_fn(security_headers));
|
.layer(axum_middleware::from_fn(security_headers));
|
||||||
|
|
||||||
Router::new()
|
Router::new()
|
||||||
|
|||||||
@@ -47,6 +47,15 @@ use crate::progress::{level_for_xp, DAILY_CHALLENGE_HISTORY_CAP};
|
|||||||
pub fn merge(local: &SyncPayload, remote: &SyncPayload) -> (SyncPayload, Vec<ConflictReport>) {
|
pub fn merge(local: &SyncPayload, remote: &SyncPayload) -> (SyncPayload, Vec<ConflictReport>) {
|
||||||
let mut conflicts = Vec::new();
|
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 stats = merge_stats(&local.stats, &remote.stats, &mut conflicts);
|
||||||
let achievements = merge_achievements(&local.achievements, &remote.achievements);
|
let achievements = merge_achievements(&local.achievements, &remote.achievements);
|
||||||
let progress = merge_progress(&local.progress, &remote.progress, &mut conflicts);
|
let progress = merge_progress(&local.progress, &remote.progress, &mut conflicts);
|
||||||
|
|||||||
Reference in New Issue
Block a user