Fix file streaming and streaming upload support - #19
Conversation
WalkthroughUploads now use streaming multipart handling (axum::extract::Multipart) with async chunked writes and optional SHA-512/SHA-1 computation; downloads support HTTP Range requests (206 Partial Content). Typed multipart deserialization and UploadForm were removed; hashing helpers became async streaming functions. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server as Axum Handler
participant FS as Filesystem
note right of Server: Streaming upload flow
Client->>Server: POST /upload (multipart stream, optional sha512/sha1)
Server->>Server: parse multipart fields (location, user, filename)
Server->>Server: canonicalize and validate path containment
alt client provided hash && file exists
Server->>FS: open & stream existing file to compute hash
FS-->>Server: computed hash
Server-->>Client: 200 OK (skipped=true, metadata)
else write upload
Server->>FS: create temp file
Client-->>Server: stream file chunks
Server->>FS: write chunks incrementally
Server->>Server: compute sha512/sha1 (if requested)
Server->>FS: move temp -> final path (atomic rename)
Server-->>Client: 201 Created (upload metadata)
end
sequenceDiagram
actor Client
participant Server as Axum Handler
participant FS as Filesystem
note right of Server: Range download flow
Client->>Server: GET /files/:path (with Range header)
Server->>Server: parse_range_header(range, file_size)
Server->>FS: open file and seek to start
FS-->>Server: stream requested bytes
Server-->>Client: 206 Partial Content (Content-Range, Accept-Ranges, Content-Length)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
filepi-server/src/handlers/files.rs (5)
410-456: Range header support looks correct, but consider cache policy.The Range request implementation correctly:
- Parses the Range header and validates it
- Seeks to the requested position
- Limits the stream using
take()- Returns 206 Partial Content with proper Content-Range header
However, note that
Cache-Control: no-cache(line 451) may not be optimal for video streaming. Consider allowing browser caching for better performance, especially for static video content.If caching is desired, consider:
- (header::CACHE_CONTROL, "no-cache".to_string()), + (header::CACHE_CONTROL, "public, max-age=3600".to_string()),
427-427: Move inline import to top of file.The
use tokio::io::AsyncSeekExt;import is declared inline. For better code organization and consistency, move this import to the top of the file alongside other imports.Apply this diff to move the import:
use tokio::fs::File; use tokio::io::AsyncReadExt; +use tokio::io::AsyncSeekExt; use tokio_util::io::ReaderStream;And remove the inline declaration:
// Seek to the start position - use tokio::io::AsyncSeekExt; file.seek(std::io::SeekFrom::Start(start))
677-682: Document the field ordering requirement.The implementation requires
locationanduserfields to be sent before thefilefield in the multipart request. This constraint is not obvious to API consumers and could lead to confusing errors. Consider documenting this requirement in the API documentation or function documentation.Add a doc comment to the function:
+/// Handles streaming file uploads. +/// +/// # Multipart Field Order +/// The `location` and `user` fields must be provided before the `file` field. +/// This is required for streaming validation. pub async fn upload_file( State(config): State<Arc<Config>>, mut multipart: axum::extract::Multipart,
783-807: Consider cleanup of partial files on streaming failure.If streaming fails midway through writing chunks (lines 785-794), the partially written file remains on disk. While not a critical issue since the error is returned to the client, consider implementing cleanup to avoid leaving corrupted partial files.
One approach is to write to a temporary file first, then rename on success:
// Create temp file with unique name let temp_path = upload_dir.join(format!(".{}.tmp", filename)); let mut output_file = tokio::fs::File::create(&temp_path).await?; // ... stream chunks ... output_file.flush().await?; // On success, rename to final location tokio::fs::rename(&temp_path, &target_path).await?;This ensures atomic file creation and automatic cleanup on error (temp file can be cleaned up by a separate process).
830-836: Cache canonical root to avoid redundant filesystem operations.The root directory is canonicalized here (lines 830-836) and also earlier in the file field processing (lines 711-719). Canonicalization involves filesystem calls and can be expensive. Consider caching
canonical_rootto avoid redundant operations.Move the canonicalization to the beginning of the function:
info!("Starting streaming file upload process"); + // Canonicalize root directory once at the start + let canonical_root = PathBuf::from(&config.root_dir) + .canonicalize() + .map_err(|e| { + error!("Failed to canonicalize root directory: {}", e); + AppError::InternalError("Invalid root directory configuration".to_string()) + })?; + let mut location = String::new(); let mut user = String::new();Then remove the redundant canonicalizations at lines 711-719 and 830-836, and use the cached
canonical_rootinstead.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
filepi-server/src/handlers/files.rs(9 hunks)filepi-server/src/models/mod.rs(0 hunks)
💤 Files with no reviewable changes (1)
- filepi-server/src/models/mod.rs
🧰 Additional context used
🧬 Code graph analysis (1)
filepi-server/src/handlers/files.rs (2)
filepi-server/src/models/file_info.rs (1)
from_path(22-107)filepi-server/src/handlers/hash_utilities.rs (1)
compute_file_sha512(5-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Rust (arm64)
- GitHub Check: Build Blazor Frontend
🔇 Additional comments (3)
filepi-server/src/handlers/files.rs (3)
14-14: LGTM: Import changes align with streaming implementation.The addition of
AsyncReadExtand removal ofUploadFormare consistent with the migration from buffered TypedMultipart to streaming Multipart.Also applies to: 24-24
625-628: LGTM: Signature change enables true streaming uploads.The migration from
TypedMultipart<UploadForm>toaxum::extract::Multipartenables processing file chunks without buffering the entire file in memory, which is essential for large file uploads.
815-850: LGTM: Upload finalization is correct.The finalization logic properly:
- Validates that a file was processed
- Computes SHA-512 hash of the uploaded file
- Constructs a complete response with all required fields
7738b40 to
064e3c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
filepi-server/src/handlers/files.rs (3)
300-320: Directory traversal still possible inserve_filedue to lack of canonicalization.
serve_filejoinsconfig.root_dirwith the user-suppliedfile_pathand only checksabs_path.starts_with(&config.root_dir)without canonicalizing either path. A path like"../etc/passwd"will still pass this check becausePathBuf::starts_withoperates on raw components and doesn't interpret.., allowing reads outsideroot_dir.Recommend canonicalizing both the root and requested path and checking confinement on the canonical paths:
- let file_path = file_path.trim_start_matches('/'); - let abs_path = PathBuf::from(&config.root_dir).join(file_path); - - // Security: prevent directory traversal - if !abs_path.starts_with(&config.root_dir) { - return Err(AppError::BadRequest("Invalid path".to_string())); - } + let file_path = file_path.trim_start_matches('/'); + let abs_path = PathBuf::from(&config.root_dir).join(file_path); + + // Security: canonicalize and ensure path stays within root_dir + let canonical_root = PathBuf::from(&config.root_dir) + .canonicalize() + .map_err(|e| { + error!("Failed to canonicalize root directory: {}", e); + AppError::InternalError("Invalid root directory configuration".to_string()) + })?; + + let abs_path = abs_path.canonicalize().map_err(|e| { + error!("Failed to canonicalize requested path {:?}: {}", abs_path, e); + AppError::NotFound("File not found".to_string()) + })?; + + if !abs_path.starts_with(&canonical_root) { + error!( + "Security violation: requested path {:?} is outside root {:?}", + abs_path, canonical_root + ); + return Err(AppError::BadRequest("Invalid path".to_string())); + }Same pattern should be applied to
stream_filebelow for consistent protection.
368-406: Apply the same canonicalization-based directory traversal protection instream_file.
stream_fileuses the samejoin+starts_with(&config.root_dir)pattern asserve_file, so a../path can escape the root directory and expose arbitrary files for streaming.Mirror the canonicalization-and-check approach used above for
serve_file:- let file_path = file_path.trim_start_matches('/'); - let abs_path = PathBuf::from(&config.root_dir).join(file_path); - - // Security: prevent directory traversal - if !abs_path.starts_with(&config.root_dir) { - return Err(AppError::BadRequest("Invalid path".to_string())); - } + let file_path = file_path.trim_start_matches('/'); + let abs_path = PathBuf::from(&config.root_dir).join(file_path); + + // Security: canonicalize and ensure path stays within root_dir + let canonical_root = PathBuf::from(&config.root_dir) + .canonicalize() + .map_err(|e| { + error!("Failed to canonicalize root directory: {}", e); + AppError::InternalError("Invalid root directory configuration".to_string()) + })?; + + let abs_path = abs_path.canonicalize().map_err(|e| { + error!("Failed to canonicalize requested path {:?}: {}", abs_path, e); + AppError::NotFound("File not found".to_string()) + })?; + + if !abs_path.starts_with(&canonical_root) { + error!( + "Security violation: requested path {:?} is outside root {:?}", + abs_path, canonical_root + ); + return Err(AppError::BadRequest("Invalid path".to_string())); + }This closes the same traversal hole for the streaming endpoint.
567-642:create_folderallows path traversal viafolder_name.
folder_namecomes directly from the query, anddir_pathis built as:let dir_path = PathBuf::from(&full_path).join(&folder_name);Without sanitization, a
folder_namelike"../outside"(or"..") will successfully pass all checks and create directories outsideroot_dir(e.g.,/srv/root/../outside).Add basic validation to restrict
folder_nameto a single, safe path segment:let folder_name = params.foldername.as_deref().unwrap_or_default(); @@ - if folder_name.is_empty() { + if folder_name.is_empty() { error!("Folder name is empty"); return Err(AppError::NotFound(format!( "Folder name should not be empty" ))); } + // Disallow path separators and parent/self components + if folder_name == "." || folder_name == ".." + || folder_name.contains(['/', '\\', '\0']) + { + error!("Invalid folder name: '{}'", folder_name); + return Err(AppError::BadRequest("Invalid folder name".to_string())); + } debug!("✓ Folder name validated");This prevents directory creation outside the configured root.
🧹 Nitpick comments (1)
filepi-server/src/main.rs (1)
6-13: DefaultBodyLimit is disabled for the entireapi_routesrouter, not just/uploadfile.
Router::layer(DefaultBodyLimit::disable())wraps the wholeapi_routesrouter (including routes added before and after), so all these endpoints now accept unbounded bodies. The comment suggests the intent was mainly for the streaming/uploadfileroute.If you only want to relax the limit for
/uploadfile(and any other explicit upload endpoints), consider applying the layer at the route level instead of the whole router, for example:- .route("/uploadfile", post(files::upload_file)) - // Disable body limit for streaming upload - we handle chunks directly - .layer(DefaultBodyLimit::disable()) + // Disable body limit for streaming upload - we handle chunks directly + .route( + "/uploadfile", + post(files::upload_file).layer(DefaultBodyLimit::disable()), + )and then decide explicitly which other routes (e.g.,
/syncfusion/upload) should also have the limit disabled.Also applies to: 63-82
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
filepi-server/src/handlers/files.rs(7 hunks)filepi-server/src/main.rs(2 hunks)filepi-server/src/models/mod.rs(0 hunks)
💤 Files with no reviewable changes (1)
- filepi-server/src/models/mod.rs
🧰 Additional context used
🧬 Code graph analysis (2)
filepi-server/src/main.rs (1)
filepi-server/src/handlers/files.rs (1)
upload_file(648-940)
filepi-server/src/handlers/files.rs (2)
filepi-server/src/handlers/hash_utilities.rs (1)
compute_file_sha512(5-11)filepi-server/src/models/file_info.rs (1)
from_path(22-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Rust (arm64)
- GitHub Check: Build Blazor Frontend
🔇 Additional comments (2)
filepi-server/src/handlers/files.rs (2)
1-25: Imports and logging setup align with new streaming handlers.The added imports for async I/O and structured logging are consistent with the new streaming and multipart logic in this file.
368-476: Range-aware streaming behavior is well-structured overall.Aside from the edge cases noted for
parse_range_header, thestream_fileimplementation cleanly:
- Parses valid
Rangeheaders into(start, end)pairs.- Seeks to the correct offset and limits the stream to the requested length.
- Returns
206 Partial Contentwith appropriateContent-Range,Content-Length,Accept-Ranges, and cache headers.- Falls back to full-file
200 OKstreaming when the Range header is absent or invalid.This is a good fit for video streaming clients.
- fix create folder to accept query param
064e3c4 to
13e3462
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
filepi-server/src/main.rs (1)
64-82: CRITICAL: Body limit disabled for all API routes, not just upload.The
DefaultBodyLimit::disable()layer at line 74 applies to the entireapi_routesRouter, including the syncfusion endpoints (lines 75-80). This exposes all routes to unbounded request bodies, creating a DoS vector.To apply the layer only to
/uploadfile, wrap that single route in its own Router:// Build API routes let api_routes = Router::new() .route("/files", get(files::get_files)) .route("/videos", get(files::get_videos)) .route("/search", get(files::search)) .route("/file/{*wildcard}", get(files::serve_file)) .route("/stream/{*wildcard}", get(files::stream_file)) .route("/thumbnail/{*wildcard}", get(files::get_thumbnail)) .route("/createfolder", post(files::create_folder)) - // Disable body limit for streaming upload - we handle chunks directly - .route("/uploadfile", post(files::upload_file)) - .layer(DefaultBodyLimit::disable()) + .merge( + // Disable body limit for streaming upload - we handle chunks directly + Router::new() + .route("/uploadfile", post(files::upload_file)) + .layer(DefaultBodyLimit::disable()) + ) .route( "/syncfusion/fileoperations", post(handlers::syncfusion::file_operations),
♻️ Duplicate comments (2)
filepi-server/src/handlers/files.rs (2)
478-516: Handle edge cases: empty files and zero-byte suffix ranges.The
parse_range_headerfunction doesn't guard against:
- Empty files (
file_size == 0): line 495 computesfile_size - 1which underflows tou64::MAX.- Zero-byte suffix (
bytes=-0): line 494 producesstart = file_size, while line 495 setsend = file_size - 1, sostart > end.Both cases cause
content_length = end - start + 1instream_file(line 419) to overflow or produce invalid ranges.Apply this diff:
fn parse_range_header(range_str: &str, file_size: u64) -> Option<(u64, u64)> { // Expected format: "bytes=start-end" or "bytes=start-" or "bytes=-suffix" + if file_size == 0 { + return None; + } let range_str = range_str.trim(); if !range_str.starts_with("bytes=") { @@ -489,8 +492,12 @@ if let Some((start_str, end_str)) = range_part.split_once('-') { if start_str.is_empty() { // Suffix range: "-500" means last 500 bytes if let Ok(suffix) = end_str.parse::<u64>() { + if suffix == 0 { + return None; + } let start = file_size.saturating_sub(suffix); - return Some((start, file_size - 1)); + if start < file_size { + return Some((start, file_size - 1)); + } }
780-825: Use streaming SHA-512 for deduplication to avoid blocking I/O.The deduplication check (line 786) calls
compute_file_sha512, which synchronously reads the entire existing file into memory withfs::read. For large files, this blocks the async runtime and can cause memory pressure.The PR added
compute_file_sha512_streaminginhash_utilities.rsspecifically to address this, but it's not used here.Apply this diff to use the streaming version:
+use crate::handlers::hash_utilities::compute_file_sha512_streaming;Then at the dedup check:
info!("Checking SHA-512 hash for deduplication..."); // Compute SHA-512 hash of existing file - let existing_hash = compute_file_sha512(&target_path).map_err(|e| { + let existing_hash = compute_file_sha512_streaming(&target_path).await.map_err(|e| { error!("Failed to compute SHA-512 hash of existing file: {}", e); AppError::InternalError(format!("Failed to compute file hash: {}", e)) })?;
🧹 Nitpick comments (2)
filepi-server/src/handlers/hash_utilities.rs (1)
14-33: Remove redundant import inside function.The
use sha2::{Digest, Sha512};statement at line 16 is redundant since these types are already imported at the module level (line 1).Apply this diff:
pub async fn compute_file_sha512_streaming(path: &PathBuf) -> Result<String, std::io::Error> { use tokio::{fs::File, io::{AsyncReadExt, BufReader}}; - use sha2::{Digest, Sha512}; let file = File::open(path).await?;filepi-server/src/handlers/files.rs (1)
905-911: Remove or uncomment the SHA-512 computation code.Lines 905-911 contain commented-out code for computing the SHA-512 hash of newly uploaded files. If this functionality is needed, uncomment and use the streaming version (
compute_file_sha512_streaming). If not, delete the commented block to keep the codebase clean.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
filepi-server/src/handlers/files.rs(7 hunks)filepi-server/src/handlers/hash_utilities.rs(1 hunks)filepi-server/src/main.rs(2 hunks)filepi-server/src/models/mod.rs(0 hunks)
💤 Files with no reviewable changes (1)
- filepi-server/src/models/mod.rs
🧰 Additional context used
🧬 Code graph analysis (2)
filepi-server/src/main.rs (1)
filepi-server/src/handlers/files.rs (1)
upload_file(648-940)
filepi-server/src/handlers/files.rs (2)
filepi-server/src/handlers/hash_utilities.rs (1)
compute_file_sha512(5-11)filepi-server/src/models/file_info.rs (1)
from_path(22-107)
🔇 Additional comments (2)
filepi-server/src/handlers/files.rs (2)
567-645: LGTM with improved validation and logging.The refactored
create_folderfunction correctly:
- Validates inputs before processing
- Canonicalizes paths and enforces root containment
- Adds comprehensive debug/info logging for troubleshooting
647-940: Streaming upload implementation is well-structured.The refactored upload handler correctly:
- Validates required fields (
location,user) before processing the file- Sanitizes filenames to prevent path traversal (line 725)
- Enforces root directory containment via canonicalization
- Streams chunks directly to disk without buffering
- Supports deduplication when client provides SHA-512 hash
The comprehensive logging will aid debugging and monitoring.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
filepi-server/src/handlers/files.rs (1)
738-742: Filename sanitization incomplete - path traversal still possible.While line 741 replaces path separators with underscores, it doesn't extract just the basename component. A malicious client could still send
"../../etc/passwd"which becomes"__..__..etc_passwd"- still containing directory traversal attempts.The past review comment suggested extracting the basename using
Path::new().file_name(), but the current implementation only replaces characters. Although subsequent canonicalization provides defense-in-depth, the filename should be sanitized to contain only the basename component.Apply this diff to properly extract the basename:
// Get filename from field metadata let raw_filename = field.file_name().unwrap_or("unnamed"); debug!("Raw filename from client: '{}'", raw_filename); - filename = raw_filename.replace(['/', '\\', '\0'], "_"); + // Extract just the filename component, removing any path elements + filename = std::path::Path::new(raw_filename) + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or("unnamed") + .replace(['\0'], "_"); // Still sanitize null bytes debug!("✓ Sanitized filename: '{}'", filename);
🧹 Nitpick comments (3)
filepi-server/src/handlers/hash_utilities.rs (1)
27-49: Consider extracting common streaming logic.Both
compute_file_sha512_streamingandcompute_file_sha1_streamingshare identical streaming logic with only the hasher type differing. Consider extracting the common pattern to reduce duplication.Example approach:
use sha2::Digest; async fn compute_hash_streaming<D: Digest + Default>(path: &PathBuf) -> Result<String, std::io::Error> { use tokio::{ fs::File, io::{AsyncReadExt, BufReader}, }; let file = File::open(path).await?; let mut reader = BufReader::new(file); let mut hasher = D::default(); let mut buf = [0u8; 8192]; loop { let n = reader.read(&mut buf).await?; if n == 0 { break; } hasher.update(&buf[..n]); } let result = hasher.finalize(); Ok(format!("{:x}", result)) } pub async fn compute_file_sha512_streaming(path: &PathBuf) -> Result<String, std::io::Error> { compute_hash_streaming::<sha2::Sha512>(path).await } pub async fn compute_file_sha1_streaming(path: &PathBuf) -> Result<String, std::io::Error> { compute_hash_streaming::<sha1::Sha1>(path).await }filepi-server/src/handlers/files.rs (2)
725-734: Field ordering requirement may surprise API clients.The upload handler requires
locationanduserfields to be sent before thefilefield in the multipart request. This strict ordering requirement could break clients that send fields in a different order.Consider either:
- Documenting this requirement clearly in API documentation
- Making the handler more flexible by buffering field values and processing the file field whenever it arrives
950-952: Consider logging or processing remaining fields instead of breaking.The handler breaks immediately after processing the
filefield, which means any subsequent fields in the multipart request will be silently ignored. Consider either:
- Logging a warning if there are additional fields
- Continuing to consume the remaining fields for completeness
// Break after processing file - it's the last field and stream is consumed debug!("Breaking from field loop after file processing"); - break; + // Note: Continue loop to log any additional fields, but file stream is consumed
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
filepi-server/Cargo.toml(1 hunks)filepi-server/src/handlers/files.rs(7 hunks)filepi-server/src/handlers/hash_utilities.rs(1 hunks)filepi-server/src/models/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
filepi-server/src/handlers/files.rs (2)
filepi-server/src/handlers/hash_utilities.rs (2)
compute_file_sha1_streaming(27-49)compute_file_sha512_streaming(3-25)filepi-server/src/models/file_info.rs (1)
from_path(22-107)
🔇 Additional comments (7)
filepi-server/src/models/mod.rs (1)
49-50: LGTM!The addition of the optional
sha1field follows the same pattern as the existingsha512field and correctly usesskip_serializing_ifto omit the field whenNone.filepi-server/src/handlers/hash_utilities.rs (1)
3-25: LGTM! Streaming implementation prevents memory issues.The streaming implementation correctly addresses the previous concern about loading entire files into memory. The async I/O with an 8192-byte buffer is appropriate for balancing throughput and memory usage.
filepi-server/src/handlers/files.rs (4)
368-476: LGTM! Range support implementation is solid.The streaming file handler correctly:
- Parses Range headers via the helper function
- Returns 206 Partial Content with proper Content-Range headers for valid ranges
- Seeks to the correct position and limits the stream appropriately
- Falls back to full file streaming when no range or invalid range is provided
- Includes proper Accept-Ranges header in all responses
567-645: LGTM! Enhanced logging improves debuggability.The create_folder function maintains all necessary security checks (path canonicalization, root containment validation) while adding comprehensive debug and info logging that will help troubleshoot upload issues.
796-893: LGTM! Deduplication logic with streaming hash computation is well-implemented.The deduplication flow correctly:
- Prioritizes SHA-512 over SHA-1 for better security
- Uses streaming hash computation to avoid memory issues
- Returns early with appropriate response when file is deduplicated
- Includes informative logging for debugging
897-942: LGTM! Streaming upload implementation is memory-efficient.The chunk-by-chunk streaming correctly:
- Processes chunks without buffering the entire file
- Includes comprehensive error handling for each chunk
- Logs progress for large files (every 100 chunks)
- Properly flushes data to disk after streaming completes
filepi-server/Cargo.toml (1)
20-20: SHA-1 is cryptographically deprecated; clarify its intended use.SHA-1 should be considered cryptographically broken and unsuitable for security-critical use, provided only for legacy interoperability purposes. Version 0.10.6 is the current stable release and has no known implementation vulnerabilities. However, NIST formally deprecated SHA-1 in 2011 and disallowed use for digital signatures in 2013; as of 2020, chosen-prefix attacks are practical.
Evaluate whether this dependency requires collision detection. If so, consider using the sha1-checked crate which provides collision detection and safe hash alternatives. If SHA-1 is for non-cryptographic purposes (e.g., legacy Git object compatibility), the current version is acceptable.
| // Compute SHA-512 hash of newly uploaded file | ||
| // info!("Computing SHA-512 hash of uploaded file..."); | ||
| // let new_file_hash = compute_file_sha512(&file_path).map_err(|e| { | ||
| // error!("Failed to compute SHA-512 hash of uploaded file: {}", e); | ||
| // AppError::InternalError(format!("Failed to compute file hash: {}", e)) | ||
| // })?; | ||
| // info!("✓ New file SHA-512: {}...", &new_file_hash[..16]); | ||
|
|
||
| // Security: ensure the upload path is within root_dir | ||
| // Get the relative path from root_dir | ||
| let canonical_root = PathBuf::from(&config.root_dir) | ||
| .canonicalize() | ||
| .map_err(|e| { | ||
| error!("Failed to canonicalize root directory: {}", e); | ||
| AppError::InternalError("Invalid root directory configuration".to_string()) | ||
| })?; | ||
|
|
||
| if !upload_dir.starts_with(&canonical_root) { | ||
| return Err(AppError::BadRequest( | ||
| "Invalid upload path: outside root directory".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| // Full path for the file | ||
| let file_path = upload_dir.join(&filename); | ||
|
|
||
| // Check if file already exists and SHA-512 hash is provided | ||
| if file_path.exists() { | ||
| if let Some(client_hash) = client_sha512 { | ||
| info!("File already exists, checking SHA-512 hash for deduplication"); | ||
|
|
||
| // Compute SHA-512 hash of existing file | ||
| let existing_hash = compute_file_sha512(&file_path).map_err(|e| { | ||
| error!("Failed to compute SHA-512 hash of existing file: {}", e); | ||
| AppError::InternalError(format!("Failed to compute file hash: {}", e)) | ||
| })?; | ||
|
|
||
| info!( | ||
| "Client SHA-512: {}..., Existing file SHA-512: {}...", | ||
| &client_hash[..16], | ||
| &existing_hash[..16] | ||
| ); | ||
|
|
||
| // If hashes match, skip upload | ||
| if client_hash == existing_hash { | ||
| info!("SHA-512 match - skipping upload for file: {}", filename); | ||
|
|
||
| let relative_path = file_path | ||
| .strip_prefix(&canonical_root) | ||
| .unwrap_or(&file_path) | ||
| .to_string_lossy() | ||
| .to_string(); | ||
|
|
||
| return Ok(Json(crate::models::UploadResponse { | ||
| message: "File already exists with identical content, upload skipped" | ||
| .to_string(), | ||
| filename, | ||
| location: relative_path, | ||
| uploaded_by: user.to_string(), | ||
| skipped: true, | ||
| sha512: Some(existing_hash), | ||
| })); | ||
| } else { | ||
| info!("SHA-512 mismatch - file will be replaced"); | ||
| } | ||
| } else { | ||
| info!("No SHA-512 provided - file will be replaced"); | ||
| } | ||
| } | ||
|
|
||
| info!("Saving file to location: {:?}", file_path); | ||
|
|
||
| // Write the file (will overwrite if exists) | ||
| let mut file = std::fs::File::create(&file_path).map_err(|e| { | ||
| error!("Failed to create file: {}", e); | ||
| AppError::InternalError(format!("Failed to create file: {}", e)) | ||
| })?; | ||
|
|
||
| file.write_all(&form.file.contents).map_err(|e| { | ||
| error!("Failed to write file: {}", e); | ||
| AppError::InternalError(format!("Failed to write file: {}", e)) | ||
| })?; | ||
|
|
||
| info!( | ||
| "File uploaded successfully: {} to path: {:?}", | ||
| filename, file_path | ||
| ); | ||
|
|
||
| // Compute SHA-512 hash of newly uploaded file | ||
| let new_file_hash = compute_file_sha512(&file_path).map_err(|e| { | ||
| error!("Failed to compute SHA-512 hash of uploaded file: {}", e); | ||
| AppError::InternalError(format!("Failed to compute file hash: {}", e)) | ||
| })?; | ||
|
|
||
| info!("New file SHA-512: {}...", &new_file_hash[..16]); | ||
|
|
||
| // Get the relative path from root_dir | ||
| let relative_path = file_path | ||
| .strip_prefix(&canonical_root) | ||
| .unwrap_or(&file_path) | ||
| .to_string_lossy() | ||
| .to_string(); | ||
|
|
||
| info!( | ||
| "=== Upload completed successfully: '{}' at '{}' ===", | ||
| filename, relative_path | ||
| ); | ||
|
|
||
| Ok(Json(crate::models::UploadResponse { | ||
| message: "File uploaded successfully".to_string(), | ||
| filename, | ||
| location: relative_path, | ||
| uploaded_by: user.to_string(), | ||
| uploaded_by: user, | ||
| skipped: false, | ||
| sha512: Some(new_file_hash), | ||
| sha512: None, | ||
| sha1: None, | ||
| })) |
There was a problem hiding this comment.
Inconsistent hash computation behavior.
Lines 973-979 show commented-out SHA-512 hash computation for newly uploaded files, and the response at lines 1006-1007 returns sha512: None and sha1: None. This creates an inconsistency:
- Deduplication path (lines 830-840, 874-884): Returns the computed hash
- Successful upload path: Returns no hash
This makes it impossible for clients to verify uploaded files or cache hashes for future deduplication.
Consider either:
- Uncomment and fix the hash computation to provide integrity verification for uploads
- Document why hashes are not computed for new uploads (e.g., performance reasons)
- Make it optional via a query parameter if hash computation is expensive
If hash computation is intentionally disabled for performance, update the comments to explain this decision.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
filepi-server/src/handlers/files.rs (1)
983-1019: Inconsistent hash behavior between deduplication and new uploads.The deduplication path (lines 840-850, 884-894) returns computed hashes in the response, but newly uploaded files return
sha512: Noneandsha1: None(lines 1016-1017). This prevents clients from:
- Verifying upload integrity
- Caching hashes for future deduplication requests
- Detecting silent corruption
The commented-out code at lines 983-989 suggests this was intentional. Consider either:
- Computing hashes for new uploads to enable client-side verification
- Documenting why hash computation is disabled (e.g., performance cost)
- Making hash computation optional via a query parameter
Note: This issue was previously flagged in past review comments.
🧹 Nitpick comments (3)
filepi-server/src/handlers/files.rs (3)
472-472: Consider including filename in Content-Disposition header.Line 472 sets
Content-Disposition: inlinewithout including the filename. This is inconsistent withserve_file(lines 356-361), which includes the filename. While not strictly required for inline disposition, including the filename can be helpful for clients that save the file.Apply this diff to include the filename:
+ // Get filename for Content-Disposition header + let file_name = abs_path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or("download"); + // Build response with streaming headers (inline, not attachment) Ok(( StatusCode::OK, [ (header::CONTENT_TYPE, mime_type), (header::CONTENT_LENGTH, file_size.to_string()), (header::ACCEPT_RANGES, "bytes".to_string()), (header::CACHE_CONTROL, "no-cache".to_string()), - (header::CONTENT_DISPOSITION, "inline".to_string()), + (header::CONTENT_DISPOSITION, format!("inline; filename=\"{}\"", file_name)), ], body, ))
748-752: Consider extracting basename to fully prevent path injection.The current sanitization replaces path separators with underscores, which is safe but preserves directory components. For example,
"subdir/file.txt"becomes"subdir_file.txt".Extracting just the filename component (as suggested in past reviews) would be more robust:
Apply this diff to extract the basename:
// Get filename from field metadata let raw_filename = field.file_name().unwrap_or("unnamed"); debug!("Raw filename from client: '{}'", raw_filename); - filename = raw_filename.replace(['/', '\\', '\0'], "_"); + filename = std::path::Path::new(raw_filename) + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or("unnamed") + .to_string(); debug!("✓ Sanitized filename: '{}'", filename);This extracts only the final path component, so
"subdir/file.txt"→"file.txt"and"../../etc/passwd"→"passwd".
735-744: Field ordering dependency is validated but not documented.The code requires
locationanduserfields to be sent before thefilefield in the multipart request. This constraint is validated but not documented in the function signature or comments.Consider adding a doc comment to document this requirement:
+/// Streaming upload handler - processes file chunks without loading entire file into memory. +/// +/// # Multipart field order +/// The multipart request must include fields in this order: +/// - `location`: Upload directory path (required) +/// - `user`: Username (required) +/// - `sha512`: Optional SHA-512 hash for deduplication +/// - `sha1`: Optional SHA-1 hash for deduplication +/// - `file`: File data (required, must be last) pub async fn upload_file(
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
filepi-server/src/handlers/files.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
filepi-server/src/handlers/files.rs (2)
filepi-server/src/handlers/hash_utilities.rs (2)
compute_file_sha1_streaming(27-49)compute_file_sha512_streaming(3-25)filepi-server/src/models/file_info.rs (1)
from_path(22-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Blazor Frontend
🔇 Additional comments (4)
filepi-server/src/handlers/files.rs (4)
478-526: LGTM! Edge cases properly handled.The
parse_range_headerfunction now correctly handles empty files and zero-length suffixes, preventing underflow and invalid ranges. The three range format cases (suffix, open-ended, full range) are all implemented correctly with proper bounds checking.
577-655: API change from JSON body to query parameters.The
create_folderfunction now accepts parameters via query string (Query<CreateFolderRequest>) instead of JSON body (Json<CreateFolderRequest>). This is a breaking change for existing clients.The implementation itself is solid with proper validation, canonicalization, and security checks. The added debug logging is helpful for troubleshooting.
814-822: LGTM! Async streaming hash computation implemented.The deduplication logic now uses
compute_file_sha512_streamingandcompute_file_sha1_streaming(async streaming functions), which read files in chunks without blocking the async runtime or allocating large buffers. This addresses the concern from previous reviews about blocking I/O and memory efficiency for large files.
913-952: LGTM! Efficient streaming upload implementation.The file upload correctly streams chunks directly to disk without buffering the entire file in memory. The implementation includes:
- Proper async I/O with
AsyncWriteExt- Chunk-by-chunk processing with progress logging
- Error handling for each chunk operation
- Explicit flush to ensure data is written to disk
This design efficiently handles large file uploads without memory pressure.
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.