codec review

crates/lanspread-peer/src/lib.rs

@@ -4,7 +4,7 @@ mod path_validation;
 mod peer;
 
 use std::{
-    collections::HashMap,
+    collections::{HashMap, VecDeque},
     net::SocketAddr,
     path::{Path, PathBuf},
     sync::Arc,
@@ -332,6 +332,7 @@ struct DownloadChunk {
     offset: u64,
     length: u64,
     retry_count: usize,
+    last_peer: Option<SocketAddr>,
 }
 
 #[derive(Debug, Default)]
@@ -361,14 +362,12 @@ async fn prepare_game_storage(
             if let Some(parent) = validated_path.parent() {
                 tokio::fs::create_dir_all(parent).await?;
             }
-            let file_size = desc.file_size().unwrap_or(0);
+            OpenOptions::new()
-            let file = OpenOptions::new()
                 .create(true)
                 .truncate(true)
                 .write(true)
                 .open(&validated_path)
                 .await?;
-            file.set_len(file_size).await?;
         }
     }
     Ok(())
@@ -395,6 +394,7 @@ fn build_peer_plans(
                     offset: 0,
                     length: 0,
                     retry_count: 0,
+                    last_peer: Some(peer),
                 });
                 continue;
             }
@@ -409,6 +409,7 @@ fn build_peer_plans(
                     offset,
                     length,
                     retry_count: 0,
+                    last_peer: Some(peer),
                 });
                 offset += length;
             }
@@ -601,6 +602,7 @@ async fn download_from_peer(
             offset: 0,
             length: 0, // Indicates whole file
             retry_count: 0,
+            last_peer: Some(peer_addr),
         };
 
         let result = download_whole_file(&mut conn, &base_dir, desc).await;
@@ -658,6 +660,7 @@ async fn download_game_files(
                         if chunk_result.chunk.retry_count < MAX_RETRY_COUNT {
                             let mut retry_chunk = chunk_result.chunk;
                             retry_chunk.retry_count += 1;
+                            retry_chunk.last_peer = Some(chunk_result.peer_addr);
                             failed_chunks.push(retry_chunk);
                         } else {
                             last_err = Some(eyre::eyre!(
@@ -701,43 +704,123 @@ async fn download_game_files(
     Ok(())
 }
 
+fn select_retry_peer(
+    peers: &[SocketAddr],
+    last_peer: Option<SocketAddr>,
+    attempt_offset: usize,
+) -> Option<SocketAddr> {
+    if peers.is_empty() {
+        return None;
+    }
+
+    if peers.len() > 1
+        && let Some(last) = last_peer
+        && let Some(pos) = peers.iter().position(|addr| *addr == last)
+    {
+        let next_index = (pos + 1 + attempt_offset) % peers.len();
+        return Some(peers[next_index]);
+    }
+
+    Some(peers[attempt_offset % peers.len()])
+}
+
+fn fallback_peer_addr(peers: &[SocketAddr], last_peer: Option<SocketAddr>) -> SocketAddr {
+    last_peer
+        .or_else(|| peers.first().copied())
+        .unwrap_or_else(|| SocketAddr::from(([0, 0, 0, 0], 0)))
+}
+
 async fn retry_failed_chunks(
     failed_chunks: Vec<DownloadChunk>,
     peers: &[SocketAddr],
     base_dir: &Path,
     game_id: &str,
 ) -> Vec<ChunkDownloadResult> {
-    let mut results = Vec::new();
+    let mut exhausted = Vec::new();
+    let mut queue: VecDeque<DownloadChunk> = failed_chunks.into_iter().collect();
+
+    while let Some(mut chunk) = queue.pop_front() {
+        if chunk.retry_count >= MAX_RETRY_COUNT {
+            exhausted.push(ChunkDownloadResult {
+                chunk: chunk.clone(),
+                result: Err(eyre::eyre!(
+                    "Retry budget exhausted for chunk: {}",
+                    chunk.relative_path
+                )),
+                peer_addr: fallback_peer_addr(peers, chunk.last_peer),
+            });
+            continue;
+        }
+
+        let retry_offset = chunk.retry_count.saturating_sub(1);
+        let Some(peer_addr) = select_retry_peer(peers, chunk.last_peer, retry_offset) else {
+            exhausted.push(ChunkDownloadResult {
+                chunk: chunk.clone(),
+                result: Err(eyre::eyre!(
+                    "No peers available to retry chunk: {}",
+                    chunk.relative_path
+                )),
+                peer_addr: fallback_peer_addr(peers, chunk.last_peer),
+            });
+            continue;
+        };
+
+        let mut attempt_chunk = chunk.clone();
+        attempt_chunk.last_peer = Some(peer_addr);
 
-    // Redistribute failed chunks among available peers
-    let _retry_plans = build_peer_plans(peers, &[]);
-    for (i, chunk) in failed_chunks.into_iter().enumerate() {
-        let peer_addr = peers[i % peers.len()];
         let plan = PeerDownloadPlan {
-            chunks: vec![chunk],
+            chunks: vec![attempt_chunk.clone()],
             whole_files: Vec::new(),
         };
 
         match download_from_peer(peer_addr, game_id, plan, base_dir.to_path_buf()).await {
-            Ok(chunk_results) => results.extend(chunk_results),
+            Ok(results) => {
+                for result in results {
+                    match result.result {
+                        Ok(()) => {}
                         Err(e) => {
-                log::error!("Failed to retry chunk: {e}");
+                            let mut retry_chunk = result.chunk.clone();
-                // Add empty failure result
+                            retry_chunk.retry_count = chunk.retry_count + 1;
-                results.push(ChunkDownloadResult {
+                            retry_chunk.last_peer = Some(result.peer_addr);
-                    chunk: DownloadChunk {
+
-                        relative_path: "unknown".to_string(),
+                            if retry_chunk.retry_count >= MAX_RETRY_COUNT {
-                        offset: 0,
+                                let context = format!(
-                        length: 0,
+                                    "Retry budget exhausted for chunk: {}",
-                        retry_count: MAX_RETRY_COUNT,
+                                    result.chunk.relative_path
-                    },
+                                );
-                    result: Err(e),
+                                exhausted.push(ChunkDownloadResult {
-                    peer_addr,
+                                    chunk: retry_chunk,
+                                    result: Err(e.wrap_err(context)),
+                                    peer_addr: result.peer_addr,
                                 });
+                            } else {
+                                queue.push_back(retry_chunk);
+                            }
+                        }
+                    }
+                }
+            }
+            Err(e) => {
+                chunk.retry_count += 1;
+                chunk.last_peer = Some(peer_addr);
+
+                if chunk.retry_count >= MAX_RETRY_COUNT {
+                    exhausted.push(ChunkDownloadResult {
+                        chunk: chunk.clone(),
+                        result: Err(e.wrap_err(format!(
+                            "Retry budget exhausted for chunk after connection failure: {}",
+                            chunk.relative_path
+                        ))),
+                        peer_addr: fallback_peer_addr(peers, chunk.last_peer),
+                    });
+                } else {
+                    queue.push_back(chunk);
+                }
             }
         }
     }
 
-    results
+    exhausted
 }
 
 async fn load_local_game_db(game_dir: &str) -> eyre::Result<GameDB> {

crates/lanspread-peer/src/path_validation.rs

@@ -1,79 +1,90 @@
-use std::path::{Path, PathBuf};
+use std::path::{Component, Path, PathBuf};
+
+use eyre::WrapErr;
+
+fn canonicalize_base_dir(base_dir: &Path) -> eyre::Result<PathBuf> {
+    if !base_dir.is_absolute() {
+        eyre::bail!("Base directory must be absolute: {}", base_dir.display());
+    }
+
+    let canonical = std::fs::canonicalize(base_dir).unwrap_or_else(|_| base_dir.to_path_buf());
+    Ok(canonical)
+}
+
+fn sanitize_relative_path(relative_path: &str) -> eyre::Result<PathBuf> {
+    if relative_path.is_empty() {
+        eyre::bail!("Relative path cannot be empty");
+    }
+
+    if relative_path.contains('\0') {
+        eyre::bail!("Path contains null byte");
+    }
+
+    // Normalise Windows separators so that component checks work uniformly.
+    let normalised = relative_path.replace('\\', "/");
+
+    if normalised.starts_with("//") {
+        eyre::bail!("UNC paths are not allowed: {relative_path}");
+    }
+
+    if normalised.contains(":/") {
+        eyre::bail!("Path contains drive letter: {relative_path}");
+    }
+
+    let path = PathBuf::from(&normalised);
+    if path.is_absolute() {
+        eyre::bail!("Path must be relative, not absolute: {relative_path}");
+    }
+
+    Ok(path)
+}
 
 /// Validates and sanitizes a relative path to prevent directory traversal attacks.
 ///
 /// # Errors
 /// Returns an error if the path attempts to escape the base directory or contains invalid components.
 pub fn validate_relative_path(base_dir: &Path, relative_path: &str) -> eyre::Result<PathBuf> {
-    // Create the full path by joining base directory with relative path
+    let canonical_base = canonicalize_base_dir(base_dir)?;
-    let full_path = base_dir.join(relative_path);
+    let relative = sanitize_relative_path(relative_path)?;
 
-    // Normalize the path to resolve any ".." or "." components
+    let mut resolved = canonical_base.clone();
-    let canonical_path = match std::fs::canonicalize(&full_path) {
-        Ok(path) => path,
-        Err(_) => {
-            // If the file doesn't exist, we can't canonicalize it.
-            // Instead, we'll manually validate the path components.
-            validate_path_components(&full_path, base_dir)?
-        }
-    };
 
-    // Get the canonical base directory
+    for component in relative.components() {
-    let canonical_base = match std::fs::canonicalize(base_dir) {
-        Ok(path) => path,
-        Err(_) => {
-            // If base directory doesn't exist, use its absolute form
-            match std::fs::canonicalize(base_dir) {
-                Ok(path) => path,
-                Err(_) => base_dir.to_path_buf(),
-            }
-        }
-    };
-
-    // Ensure the canonical path starts with the canonical base directory
-    if !canonical_path.starts_with(&canonical_base) {
-        eyre::bail!(
-            "Path validation failed: {} attempts to escape base directory",
-            relative_path
-        );
-    }
-
-    Ok(canonical_path)
-}
-
-/// Validates path components without requiring the file to exist
-fn validate_path_components(full_path: &Path, base_dir: &Path) -> eyre::Result<PathBuf> {
-    let mut result = base_dir.to_path_buf();
-
-    for component in full_path.components() {
         match component {
-            std::path::Component::Prefix(_) => {
+            Component::Prefix(_) | Component::RootDir => {
-                eyre::bail!("Path contains Windows prefix: {:?}", full_path);
+                eyre::bail!("Path is not relative: {relative_path}");
             }
-            std::path::Component::RootDir | std::path::Component::CurDir => {
+            Component::ParentDir => {
-                // Skip root directory and current directory components
+                eyre::bail!("Path attempts to escape base directory: {relative_path}");
             }
-            std::path::Component::ParentDir => {
+            Component::CurDir => {}
-                // Check if we can go up one level without escaping base_dir
+            Component::Normal(part) => {
-                if !result.pop() {
+                resolved.push(part);
+
+                if let Ok(metadata) = std::fs::symlink_metadata(&resolved)
+                    && metadata.file_type().is_symlink()
+                {
+                    let target = std::fs::canonicalize(&resolved).wrap_err_with(|| {
+                        format!("Failed to canonicalize symlink {}", resolved.display())
+                    })?;
+
+                    if !target.starts_with(&canonical_base) {
                         eyre::bail!(
-                        "Path attempts to escape base directory with '..': {:?}",
+                            "Path validation failed: {relative_path} escapes the base directory"
-                        full_path
                         );
                     }
+
+                    resolved = target;
                 }
-            std::path::Component::Normal(name) => {
-                // Validate the component name
-                let name_str = name.to_string_lossy();
-                if name_str.contains('\0') || name_str.contains('/') || name_str.contains('\\') {
-                    eyre::bail!("Path component contains invalid characters: {}", name_str);
-                }
-                result.push(name);
             }
         }
     }
 
-    Ok(result)
+    if !resolved.starts_with(&canonical_base) {
+        eyre::bail!("Path validation failed: {relative_path} escapes the base directory");
+    }
+
+    Ok(resolved)
 }
 
 /// Validates a relative path that will be used for accessing files within a game directory.
@@ -82,24 +93,6 @@ fn validate_path_components(full_path: &Path, base_dir: &Path) -> eyre::Result<P
 /// # Errors
 /// Returns an error if the path is absolute or attempts directory traversal.
 pub fn validate_game_file_path(game_dir: &Path, relative_path: &str) -> eyre::Result<PathBuf> {
-    // First check if the relative path is actually relative
-    if Path::new(relative_path).is_absolute() {
-        eyre::bail!("Path must be relative, not absolute: {}", relative_path);
-    }
-
-    // Check for obvious traversal attempts
-    if relative_path.contains("..") {
-        eyre::bail!("Path contains directory traversal: {}", relative_path);
-    }
-
-    // For Windows paths
-    if relative_path.contains(':')
-        && (relative_path.contains(":\\") || relative_path.contains(":/"))
-    {
-        eyre::bail!("Path contains drive letter: {}", relative_path);
-    }
-
-    // Use the general validation function
     validate_relative_path(game_dir, relative_path)
 }
 
@@ -109,7 +102,15 @@ mod tests {
 
     fn create_temp_dir() -> std::io::Result<std::path::PathBuf> {
         let mut dir = std::env::temp_dir();
-        dir.push(format!("lanspread_test_{}", std::process::id()));
+        let unique = format!(
+            "lanspread_test_{}_{}",
+            std::process::id(),
+            std::time::SystemTime::now()
+                .duration_since(std::time::UNIX_EPOCH)
+                .unwrap_or_default()
+                .as_nanos()
+        );
+        dir.push(unique);
         std::fs::create_dir_all(&dir)?;
         Ok(dir)
     }
@@ -143,6 +144,13 @@ mod tests {
         assert!(validate_game_file_path(base, "subdir/file.txt").is_ok());
         assert!(validate_game_file_path(base, "subdir/deep/nested/file.txt").is_ok());
         assert!(validate_game_file_path(base, "file with spaces.txt").is_ok());
+        assert!(validate_game_file_path(base, "./dot/./path.txt").is_ok());
+
+        let windows_style = validate_game_file_path(base, "mix\\windows\\path.txt").unwrap();
+        assert_eq!(
+            windows_style,
+            base.join("mix").join("windows").join("path.txt")
+        );
     }
 
     #[test]
@@ -156,6 +164,29 @@ mod tests {
         assert!(validate_game_file_path(base, "../../etc/passwd").is_err());
     }
 
+    #[test]
+    fn test_double_dot_in_filename_allowed() {
+        let temp_dir = TempDir::new().expect("Failed to create temp dir for test");
+        let base = temp_dir.path();
+
+        assert!(validate_game_file_path(base, "data/file..txt").is_ok());
+    }
+
+    #[test]
+    fn test_missing_file_stays_within_base() {
+        let temp_dir = TempDir::new().expect("Failed to create temp dir for test");
+        let base = temp_dir.path();
+
+        let resolved = validate_game_file_path(base, "new_dir/new_file.bin").unwrap();
+        assert_eq!(resolved, base.join("new_dir").join("new_file.bin"));
+    }
+
+    #[test]
+    fn test_relative_base_dir_rejected() {
+        let relative_base = std::path::Path::new("relative/base");
+        assert!(validate_game_file_path(relative_base, "file.bin").is_err());
+    }
+
     #[test]
     fn test_absolute_paths() {
         let temp_dir = TempDir::new().expect("Failed to create temp dir for test");
@@ -178,4 +209,20 @@ mod tests {
         assert!(validate_game_file_path(base, "D:/evil.txt").is_err());
         assert!(validate_game_file_path(base, "\\\\server\\share\\file.txt").is_err());
     }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_symlink_escape_rejected() {
+        use std::os::unix::fs::symlink;
+
+        let base_dir = TempDir::new().expect("Failed to create base temp dir");
+        let outside_dir = TempDir::new().expect("Failed to create outside temp dir");
+
+        let base = base_dir.path();
+        let outside = outside_dir.path();
+
+        symlink(outside, base.join("link")).expect("Failed to create symlink");
+
+        assert!(validate_game_file_path(base, "link/escape.txt").is_err());
+    }
 }