From 5e638691ad71fa36e62bc8fb3ed96351d41d72e2 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Mon, 8 Jun 2026 21:14:04 +0200 Subject: [PATCH] security(upload): cap upload size to prevent memody/disk consumption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit add OXICLOUD_CHUNK_MAX_BYTES which correspond to the max upload chunk allowed (differs from OXICLOUD_MAX_UPLOAD_SIZE which is the max total size of a file) hurl test validate the change Streams the request body straight to the chunk file with peak heap of ~one HTTP frame, regardless of chunk size or the configured cap. The `storage.chunk_max_bytes` config (env `OXICLOUD_CHUNK_MAX_BYTES`, default 100 MB) bounds a single PUT — separate from `max_upload_size` which governs whole-file uploads. Without this separation, a client could submit a chunk up to the whole-file cap (10 GB default) and monopolise server memory. --- .gitignore | 3 + src/common/config.rs | 13 ++ .../nextcloud_chunked_upload_service.rs | 28 ++- .../api/handlers/chunked_upload_handler.rs | 51 +++++- src/interfaces/nextcloud/uploads_handler.rs | 25 ++- src/interfaces/upload_spool.rs | 66 +++++++- tests/api/chunked_upload_cap.hurl | 159 ++++++++++++++++++ tests/api/run.sh | 15 +- tests/common/server.env | 5 + 9 files changed, 345 insertions(+), 20 deletions(-) create mode 100644 tests/api/chunked_upload_cap.hurl diff --git a/.gitignore b/.gitignore index 28ed45c8..0d7b9636 100644 --- a/.gitignore +++ b/.gitignore @@ -97,3 +97,6 @@ tests/e2e/test-results/ tests/e2e/blob-report/ tests/e2e/playwright/.cache/ tests/e2e/playwright/.auth/ + +# Test fixtures generated on-the-fly by tests/api/run.sh +tests/fixtures/chunk-over-cap-*.bin diff --git a/src/common/config.rs b/src/common/config.rs index 36e3d78b..79cb9226 100644 --- a/src/common/config.rs +++ b/src/common/config.rs @@ -211,6 +211,13 @@ pub struct StorageConfig { /// Maximum upload file size in bytes (default: 10 GB). /// Applied as a hard limit to WebDAV PUT and streaming uploads. pub max_upload_size: usize, + /// Maximum size of a single chunk in a chunked-upload session, in bytes + /// (default: 100 MB). Distinct from [`max_upload_size`] (which bounds the + /// total file size): NC desktop and other clients split large files into + /// many smaller PUTs against `/dav/uploads/…`, so the per-chunk cap can + /// be far tighter than the whole-file cap and prevents one HTTP request + /// from monopolising server memory or disk. Env: `OXICLOUD_CHUNK_MAX_BYTES`. + pub chunk_max_bytes: usize, /// Directory for upload spool temp files. When `Some`, large uploads are /// spooled here instead of the OS default temp dir (often tmpfs/RAM in /// containers, where the spool's page-cache counts against the cgroup @@ -359,6 +366,7 @@ impl Default for StorageConfig { parallel_threshold: 100 * 1024 * 1024, // 100 MB trash_retention_days: 30, // 30 days max_upload_size: MAX_UPLOAD_SIZE, + chunk_max_bytes: 100 * 1024 * 1024, // 100 MB — sane upper bound for a single chunked-upload PUT upload_temp_dir: None, usage_reconcile_secs: 600, // 10 minutes backend: StorageBackendType::Local, @@ -1224,6 +1232,11 @@ impl AppConfig { { config.storage.max_upload_size = val; } + if let Ok(chunk_max) = env::var("OXICLOUD_CHUNK_MAX_BYTES").map(|v| v.parse::()) + && let Ok(val) = chunk_max + { + config.storage.chunk_max_bytes = val; + } // Upload spool directory — keep large upload temp files off tmpfs/RAM // (otherwise their page-cache counts against the cgroup memory limit). diff --git a/src/infrastructure/services/nextcloud_chunked_upload_service.rs b/src/infrastructure/services/nextcloud_chunked_upload_service.rs index 16d80e63..80218181 100644 --- a/src/infrastructure/services/nextcloud_chunked_upload_service.rs +++ b/src/infrastructure/services/nextcloud_chunked_upload_service.rs @@ -52,7 +52,30 @@ impl NextcloudChunkedUploadService { Ok(()) } - /// Store a chunk in the session directory. + /// Resolve and validate the filesystem path for a chunk file. + /// + /// Public so the interface layer can stream an HTTP body straight into + /// the chunk file without copying through the service. The service + /// retains responsibility for path-component validation; the caller + /// owns the I/O (open, write, fsync, size enforcement, cleanup on + /// failure). All three `validate_path_component` calls run before the + /// path is constructed, so a returned `PathBuf` is always inside + /// `base_dir/{user}/{upload_id}`. + pub fn safe_chunk_path( + &self, + user: &str, + upload_id: &str, + chunk_name: &str, + ) -> Result { + Self::validate_path_component(chunk_name, "chunk_name")?; + Ok(self.safe_session_dir(user, upload_id)?.join(chunk_name)) + } + + /// Store a chunk in the session directory. Buffers `data` in memory — + /// use [`safe_chunk_path`](Self::safe_chunk_path) + the + /// `interfaces/upload_spool::stream_body_to_path` helper to stream the + /// HTTP body directly to disk and avoid materialising the whole chunk + /// in RAM. pub async fn store_chunk( &self, user: &str, @@ -60,8 +83,7 @@ impl NextcloudChunkedUploadService { chunk_name: &str, data: &[u8], ) -> Result<()> { - Self::validate_path_component(chunk_name, "chunk_name")?; - let chunk_path = self.safe_session_dir(user, upload_id)?.join(chunk_name); + let chunk_path = self.safe_chunk_path(user, upload_id, chunk_name)?; let mut file = fs::File::create(&chunk_path) .await .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?; diff --git a/src/interfaces/api/handlers/chunked_upload_handler.rs b/src/interfaces/api/handlers/chunked_upload_handler.rs index 893060bc..4c520123 100644 --- a/src/interfaces/api/handlers/chunked_upload_handler.rs +++ b/src/interfaces/api/handlers/chunked_upload_handler.rs @@ -420,29 +420,68 @@ pub async fn create_upload( params( ("upload_id" = String, Path, description = "Upload session ID"), ("chunk_index" = usize, Query, description = "Zero-based chunk index"), - ("checksum" = Option, Query, description = "Optional MD5 checksum for integrity verification"), + ( + "checksum" = Option, + Query, + description = "Optional hex-encoded checksum for integrity verification. \ + Computed incrementally during the streaming write. \ + Algorithm is selected by `checksumalg` (default `md5`). \ + Also accepted via the legacy `Content-MD5` request header." + ), + ( + "checksumalg" = Option, + Query, + description = "Algorithm used by `checksum`. One of: `md5` (default, legacy), `sha256` / `sha-256`, `blake3`. \ + Unknown values return 400." + ), ), request_body(content_type = "application/octet-stream", description = "Raw chunk bytes"), responses( (status = 200, description = "Chunk received", body = crate::application::ports::chunked_upload_ports::ChunkUploadResponseDto), - (status = 400, description = "Invalid chunk or checksum mismatch"), + (status = 400, description = "Invalid chunk, size mismatch, checksum mismatch, or unknown `checksumalg`"), (status = 404, description = "Upload session not found"), + (status = 413, description = "Chunk exceeds `storage.chunk_max_bytes` cap"), ), tag = "uploads", security(("bearerAuth" = [])) )] pub async fn upload_chunk( - state: State>, + State(state): State>, auth_user: AuthUser, path: Path, query: Query, headers: HeaderMap, request: Request, ) -> impl IntoResponse { - let body = axum::body::to_bytes(request.into_body(), usize::MAX) + // Cap the chunk body at `storage.chunk_max_bytes` (env + // `OXICLOUD_CHUNK_MAX_BYTES`, default 100 MB). Previous code used + // `usize::MAX` and `unwrap_or_default()` — two compounding bugs: + // - No upper bound → an oversized chunk OOMs the server. + // - Silent fallback to an empty body on transport error → the + // inner size check would either reject (good case) or — if the + // declared chunk_size was 0 (illegal but conceivable) — accept + // an empty upload as success. Either way the client got no + // actionable error. + let max_chunk = state.core.config.storage.chunk_max_bytes; + let body = match axum::body::to_bytes(request.into_body(), max_chunk).await { + Ok(b) => b, + Err(e) => { + tracing::warn!( + error = %e, + upload_id = %path.0, + max_chunk, + "Chunked upload PATCH rejected — body read failed (size cap or transport error)" + ); + return AppError::payload_too_large(format!( + "Chunk read failed (cap {} bytes): {}", + max_chunk, e + )) + .into_response(); + } + }; + ChunkedUploadHandler::upload_chunk_impl(State(state), auth_user, path, query, headers, body) .await - .unwrap_or_default(); - ChunkedUploadHandler::upload_chunk_impl(state, auth_user, path, query, headers, body).await + .into_response() } #[utoipa::path( diff --git a/src/interfaces/nextcloud/uploads_handler.rs b/src/interfaces/nextcloud/uploads_handler.rs index a5da00c3..192688ee 100644 --- a/src/interfaces/nextcloud/uploads_handler.rs +++ b/src/interfaces/nextcloud/uploads_handler.rs @@ -1,5 +1,5 @@ use axum::{ - body::{self, Body}, + body::Body, http::{Request, StatusCode, header}, response::Response, }; @@ -10,6 +10,7 @@ use crate::common::di::AppState; use crate::common::mime_detect::{filename_from_path, refine_content_type_from_file}; use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::{AuthUser, CurrentUser}; +use crate::interfaces::upload_spool::stream_body_to_path; /// Dispatch Nextcloud chunked upload WebDAV requests. /// @@ -164,6 +165,14 @@ async fn handle_mkcol( } /// PUT — store a chunk. +/// +/// Streams the request body straight to the chunk file with peak heap of +/// ~one HTTP frame, regardless of chunk size or the configured cap. The +/// `storage.chunk_max_bytes` config (env `OXICLOUD_CHUNK_MAX_BYTES`, +/// default 100 MB) bounds a single PUT — separate from `max_upload_size` +/// which governs whole-file uploads. Without this separation, a client +/// could submit a chunk up to the whole-file cap (10 GB default) and +/// monopolise server memory. async fn handle_put_chunk( state: Arc, req: Request, @@ -181,15 +190,13 @@ async fn handle_put_chunk( return Err(AppError::bad_request("Missing chunk name")); } - let max_upload = state.core.config.storage.max_upload_size; - let body_bytes = body::to_bytes(req.into_body(), max_upload) - .await - .map_err(|e| AppError::bad_request(format!("Failed to read chunk body: {}", e)))?; + let chunk_path = nc + .chunked_uploads + .safe_chunk_path(&user.username, upload_id, chunk_name) + .map_err(|e| AppError::bad_request(format!("Invalid chunk path: {}", e)))?; - nc.chunked_uploads - .store_chunk(&user.username, upload_id, chunk_name, &body_bytes) - .await - .map_err(|e| AppError::internal_error(format!("Failed to store chunk: {}", e)))?; + let max_chunk = state.core.config.storage.chunk_max_bytes; + stream_body_to_path(req.into_body(), &chunk_path, max_chunk).await?; Ok(Response::builder() .status(StatusCode::CREATED) diff --git a/src/interfaces/upload_spool.rs b/src/interfaces/upload_spool.rs index 694128e2..de6b6279 100644 --- a/src/interfaces/upload_spool.rs +++ b/src/interfaces/upload_spool.rs @@ -6,7 +6,7 @@ //! file (off tmpfs when [`StorageConfig::upload_temp_dir`] is configured) and //! BLAKE3-hashed on the fly so the dedup layer can short-circuit on a hit. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use axum::body::Body; use http_body_util::BodyStream; @@ -84,3 +84,67 @@ pub async fn spool_body_to_temp( size: total_bytes as u64, }) } + +/// Stream an HTTP request body directly to a known destination file, +/// enforcing `max_bytes` as a hard size limit. +/// +/// Used by the chunked-upload PUT handlers — each chunk has a deterministic +/// on-disk path (computed by `NextcloudChunkedUploadService::safe_chunk_path` +/// or the equivalent REST helper), so there's no need for a spool/move +/// dance. Peak heap is ~one HTTP frame regardless of chunk size or `max_bytes`. +/// +/// **No hashing** — chunked uploads dedup at the assembled-file level, not +/// the chunk level, so computing BLAKE3 here would be wasted work. +/// +/// On size overflow the partial file is removed before the function returns, +/// so a client retry against the same chunk name starts from a clean slate. +/// On any other I/O error the partial file is also removed and the error +/// surfaces — callers can assume the path is either fully written or absent. +pub async fn stream_body_to_path( + body: Body, + path: &Path, + max_bytes: usize, +) -> Result { + let mut file = tokio::fs::File::create(path) + .await + .map_err(|e| AppError::internal_error(format!("Failed to open chunk file: {e}")))?; + + let mut total_bytes: usize = 0; + let mut stream = BodyStream::new(body); + + while let Some(frame_result) = stream.next().await { + let frame = match frame_result { + Ok(f) => f, + Err(e) => { + drop(file); + let _ = tokio::fs::remove_file(path).await; + return Err(AppError::bad_request(format!( + "Failed to read request body: {e}" + ))); + } + }; + if let Some(chunk) = frame.data_ref() { + total_bytes += chunk.len(); + if total_bytes > max_bytes { + drop(file); + let _ = tokio::fs::remove_file(path).await; + return Err(AppError::payload_too_large(format!( + "Chunk exceeds maximum size of {max_bytes} bytes" + ))); + } + if let Err(e) = file.write_all(chunk).await { + drop(file); + let _ = tokio::fs::remove_file(path).await; + return Err(AppError::internal_error(format!( + "Failed to write chunk: {e}" + ))); + } + } + } + file.flush() + .await + .map_err(|e| AppError::internal_error(format!("Failed to flush chunk file: {e}")))?; + drop(file); + + Ok(total_bytes as u64) +} diff --git a/tests/api/chunked_upload_cap.hurl b/tests/api/chunked_upload_cap.hurl new file mode 100644 index 00000000..e4ced792 --- /dev/null +++ b/tests/api/chunked_upload_cap.hurl @@ -0,0 +1,159 @@ +# ============================================================= +# OxiCloud — Chunked upload size cap + streaming verification +# ============================================================= +# Validates `storage.chunk_max_bytes` (env `OXICLOUD_CHUNK_MAX_BYTES`, +# set to 4 MiB by `tests/common/server.env`) on the REST chunked upload +# surface `/api/uploads/{id}`. +# +# Two scenarios: +# 1. SUCCESS path — uploads `hello.txt` (32 bytes) in one chunk and +# asserts the stored file's BLAKE3 matches the known hash of the +# fixture. Confirms streaming + assembly + dedup integrity. +# 2. CAP REJECTION — uploads `chunk-over-cap-5mb.bin` (5 MiB) which +# exceeds the 4 MiB cap, asserting the server returns 413 Payload +# Too Large instead of OOMing or silently truncating. +# +# Why REST chunked, not NC chunked: both surfaces share the same +# `chunk_max_bytes` config; the REST path is JWT-authed (already set +# up in this Hurl run), the NC path would require minting an app +# password mid-test. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Login and capture the JWT token +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ + "username": "{{username}}", + "password": "{{password}}" +} + +HTTP 200 +[Captures] +token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 2 — Resolve home folder id (single root folder for admin) +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/folders +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +home_folder_id: jsonpath "$[0].id" + + +# ───────────────────────────────────────────────────────────── +# Step 3 — Create chunked upload session for hello.txt (32 bytes) +# chunk_size = 1 MiB (server minimum is 1 MiB). With +# total_size < chunk_size the server computes a single +# chunk of `total_size` bytes for index 0. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/uploads +Authorization: Bearer {{token}} +Content-Type: application/json +{ + "filename": "chunked-cap-hello.txt", + "folder_id": "{{home_folder_id}}", + "content_type": "text/plain", + "total_size": 32, + "chunk_size": 1048576 +} + +HTTP 201 +[Captures] +upload_id_ok: jsonpath "$.upload_id" + + +# ───────────────────────────────────────────────────────────── +# Step 4 — Upload the single 32-byte chunk → 200 +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/api/uploads/{{upload_id_ok}}?chunk_index=0 +Authorization: Bearer {{token}} +Content-Type: application/octet-stream +file,fixtures/hello.txt; + +HTTP 200 +[Asserts] +header "Upload-Complete" == "true" + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Complete the upload → 201, capture file id +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/uploads/{{upload_id_ok}}/complete +Authorization: Bearer {{token}} + +HTTP 201 +[Captures] +file_id: jsonpath "$.file_id" +[Asserts] +jsonpath "$.filename" == "chunked-cap-hello.txt" +jsonpath "$.size" == 32 + + +# ───────────────────────────────────────────────────────────── +# Step 6 — Verify the stored file's BLAKE3 matches the known +# hash of `tests/fixtures/hello.txt`. This proves the +# streaming path wrote the exact bytes — no truncation, +# no buffering corruption, no off-by-one. +# +# Expected hash: +# b2208c5dc33ff951227bd0c139f5eccb04105d6da6a7519ee23f7bc00a17bb5a +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/files?folder_id={{home_folder_id}} +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +jsonpath "$[?(@.id == '{{file_id}}')].content_hash" includes "b2208c5dc33ff951227bd0c139f5eccb04105d6da6a7519ee23f7bc00a17bb5a" + + +# ───────────────────────────────────────────────────────────── +# Step 7 — CAP REJECTION: create a session declaring a 5 MiB +# chunk and attempt to upload exactly 5 MiB → 413. +# +# The cap fires in `axum::body::to_bytes(req, max_chunk)` +# BEFORE the inner chunk-size check, so the server never +# materialises the full 5 MiB body in memory. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/uploads +Authorization: Bearer {{token}} +Content-Type: application/json +{ + "filename": "chunked-cap-over.bin", + "folder_id": "{{home_folder_id}}", + "content_type": "application/octet-stream", + "total_size": 5242880, + "chunk_size": 5242880 +} + +HTTP 201 +[Captures] +upload_id_big: jsonpath "$.upload_id" + + +# ───────────────────────────────────────────────────────────── +# Step 8 — Send a 5 MiB chunk → 413 (cap is 4 MiB). +# Pre-fix this PATCH would either OOM the server or be +# silently treated as an empty body (via the old +# `to_bytes(.., usize::MAX).unwrap_or_default()` path). +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/api/uploads/{{upload_id_big}}?chunk_index=0 +Authorization: Bearer {{token}} +Content-Type: application/octet-stream +file,fixtures/chunk-over-cap-5mb.bin; + +HTTP 413 + + +# ───────────────────────────────────────────────────────────── +# Step 9 — Clean up the abandoned over-cap session. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/uploads/{{upload_id_big}} +Authorization: Bearer {{token}} + +HTTP 204 diff --git a/tests/api/run.sh b/tests/api/run.sh index 5bc8adff..80bac0d6 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -85,6 +85,18 @@ log "Waiting for server at $base_url..." wait_for_http "$base_url/ready" 120 log "Server is ready." +# ── 3.5. Generate ephemeral test fixtures ───────────────────────────────────── +# chunked_upload_cap.hurl needs a body > OXICLOUD_CHUNK_MAX_BYTES (4 MiB) to +# trigger the 413. Committing a 5 MiB binary to the repo would bloat git for +# every clone; generating it at run time is reproducible and the file is in +# `.gitignore`. + +OVER_CAP_FIXTURE="$REPO_ROOT/tests/fixtures/chunk-over-cap-5mb.bin" +if [[ ! -s "$OVER_CAP_FIXTURE" ]]; then + log "Generating 5 MiB fixture for chunk-cap test → $OVER_CAP_FIXTURE" + dd if=/dev/zero of="$OVER_CAP_FIXTURE" bs=1024 count=5120 status=none +fi + # ── 4. Run Hurl tests ───────────────────────────────────────────────────────── log "Running Hurl tests..." @@ -104,7 +116,8 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/grants.hurl" \ "$API_DIR/subject_groups.hurl" \ "$API_DIR/grants_nested_groups.hurl" \ - "$API_DIR/external_users.hurl" + "$API_DIR/external_users.hurl" \ + "$API_DIR/chunked_upload_cap.hurl" #bash "$API_DIR/dedup_bulk_upload.sh" diff --git a/tests/common/server.env b/tests/common/server.env index 88d87cc7..d36eed35 100644 --- a/tests/common/server.env +++ b/tests/common/server.env @@ -22,6 +22,11 @@ RUST_LOG="warn,audit=info" #RUST_LOG=debug #RUST_LOG=info +# Per-chunk upload cap, exercised by chunked_upload_cap.hurl. +# 4 MiB: lets the existing grants.hurl single-chunk test (2.76 MB) pass +# under the cap, while the cap test sends a 5 MiB fixture to trigger 413. +OXICLOUD_CHUNK_MAX_BYTES=4194304 + # grow up limits for tests OXICLOUD_RATE_LIMIT_REFRESH_MAX=360 OXICLOUD_RATE_LIMIT_LOGIN_MAX=360