diff --git a/.claude/scheduled_tasks.lock b/.claude/scheduled_tasks.lock deleted file mode 100644 index 8cba0caa..00000000 --- a/.claude/scheduled_tasks.lock +++ /dev/null @@ -1 +0,0 @@ -{"sessionId":"82cd2c6b-7874-4cfa-9d00-297f91d81b98","pid":2450899,"procStart":"26101142","acquiredAt":1781931933331} \ No newline at end of file diff --git a/src/application/ports/file_ports.rs b/src/application/ports/file_ports.rs index 86539185..98b2e180 100644 --- a/src/application/ports/file_ports.rs +++ b/src/application/ports/file_ports.rs @@ -99,6 +99,16 @@ pub trait FileUploadUseCase: Send + Sync + 'static { /// on the overwrite branch and `authz.require(caller, Create, /// Folder|Drive(id))` on the new-file branch. Handlers just plumb /// `caller_id` through — no protocol-layer authz. + /// + /// `expected_hash`: forwarded to + /// `FileWritePort::update_file_content_with_blob` on the overwrite + /// branch for compare-and-swap; ignored on the new-file branch + /// (nothing to compare against). Pass `None` for plain PUT/WOPI/ + /// chunked-upload last-write-wins semantics; pass the pre-write + /// snapshot's content hash for PATCH, where a concurrent write + /// during the (potentially slow) splice must be rejected rather + /// than silently clobbered. + #[allow(clippy::too_many_arguments)] async fn update_file_streaming_with_perms( &self, path: &str, @@ -107,6 +117,7 @@ pub trait FileUploadUseCase: Send + Sync + 'static { content_type: &str, modified_at: Option, caller_id: Uuid, + expected_hash: Option<&str>, ) -> Result; } diff --git a/src/application/ports/storage_ports.rs b/src/application/ports/storage_ports.rs index 549aa6da..b2d4dcbc 100644 --- a/src/application/ports/storage_ports.rs +++ b/src/application/ports/storage_ports.rs @@ -299,6 +299,14 @@ pub trait FileWritePort: Send + Sync + 'static { /// /// `caller_id` is stamped into `updated_by` alongside the /// `updated_at` bump (§14 provenance). + /// + /// `expected_hash`: when `Some`, makes this a true compare-and-swap — + /// the write only takes effect if the row's current `blob_hash` + /// still equals it, checked and applied atomically under the same + /// row lock (no gap between check and write for a concurrent writer + /// to land in). A mismatch returns `ErrorKind::PreconditionFailed` + /// and leaves the row untouched. `None` keeps the previous + /// blind-overwrite behaviour (PUT/WOPI/chunked-upload finalize). async fn update_file_content_with_blob( &self, file_id: &str, @@ -306,6 +314,7 @@ pub trait FileWritePort: Send + Sync + 'static { size: u64, modified_at: Option, caller_id: Uuid, + expected_hash: Option<&str>, ) -> Result<(String, i64), DomainError>; /// Registers file metadata WITHOUT writing content to disk (write-behind). diff --git a/src/application/services/file_upload_service.rs b/src/application/services/file_upload_service.rs index f73d1b76..9b26acb7 100644 --- a/src/application/services/file_upload_service.rs +++ b/src/application/services/file_upload_service.rs @@ -305,7 +305,7 @@ impl FileUploadService { let file = file_read.get_file(file_id).await?; let (new_hash, updated_at) = self .file_write - .update_file_content_with_blob(file_id, &blob.hash, blob.size, None, caller_id) + .update_file_content_with_blob(file_id, &blob.hash, blob.size, None, caller_id, None) .await?; // The file maps to a different blob now — stale cached content must // never be served for the rest of its TTI window. @@ -506,6 +506,7 @@ impl FileUploadUseCase for FileUploadService { /// member and cross-tenant PUT. See /// `docs/plan/authz_audit/nextcloud.md` and the sibling native /// `/webdav/*` handler. + #[allow(clippy::too_many_arguments)] async fn update_file_streaming_with_perms( &self, path: &str, @@ -514,6 +515,7 @@ impl FileUploadUseCase for FileUploadService { content_type: &str, modified_at: Option, caller_id: Uuid, + expected_hash: Option<&str>, ) -> Result { let Some(authz) = &self.authorization else { return Err(DomainError::internal_error( @@ -552,6 +554,7 @@ impl FileUploadUseCase for FileUploadService { blob.size, modified_at, caller_id, + expected_hash, ) .await?; // Invalidate content cache — file content has changed. diff --git a/src/application/services/trash_service_test.rs b/src/application/services/trash_service_test.rs index 16e8a40a..f7572434 100644 --- a/src/application/services/trash_service_test.rs +++ b/src/application/services/trash_service_test.rs @@ -589,6 +589,7 @@ impl FileWritePort for MockFileRepository { _size: u64, _modified_at: Option, _caller_id: Uuid, + _expected_hash: Option<&str>, ) -> std::result::Result<(String, i64), DomainError> { Ok((String::new(), 0)) } diff --git a/src/common/stubs.rs b/src/common/stubs.rs index 8743fca8..4aa44b57 100644 --- a/src/common/stubs.rs +++ b/src/common/stubs.rs @@ -196,6 +196,7 @@ impl FileWritePort for StubFileWritePort { _size: u64, _modified_at: Option, _caller_id: Uuid, + _expected_hash: Option<&str>, ) -> Result<(String, i64), DomainError> { Ok((String::new(), 0)) } @@ -491,6 +492,7 @@ impl FileUploadUseCase for StubFileUploadUseCase { Ok(FileDto::default()) } + #[allow(clippy::too_many_arguments)] async fn update_file_streaming_with_perms( &self, _path: &str, @@ -499,6 +501,7 @@ impl FileUploadUseCase for StubFileUploadUseCase { _content_type: &str, _modified_at: Option, _caller_id: Uuid, + _expected_hash: Option<&str>, ) -> Result { Ok(FileDto::default()) } diff --git a/src/domain/errors.rs b/src/domain/errors.rs index ede2a9f1..0d6f2f44 100644 --- a/src/domain/errors.rs +++ b/src/domain/errors.rs @@ -39,6 +39,12 @@ pub enum ErrorKind { /// `AlreadyExists` (which is a uniqueness violation) so audit /// readers can tell them apart. Conflict, + /// RFC 7232 precondition failure — a caller-supplied conditional + /// (If-Match, or an internal compare-and-swap standing in for one) + /// did not hold against the resource's current state. Maps to + /// HTTP 412. Distinct from `Conflict` (409): this is specifically + /// "the state you thought you were writing against has moved." + PreconditionFailed, } impl ErrorKind { @@ -58,6 +64,7 @@ impl ErrorKind { ErrorKind::DatabaseError => "Database Error", ErrorKind::QuotaExceeded => "Quota Exceeded", ErrorKind::Conflict => "Conflict", + ErrorKind::PreconditionFailed => "Precondition Failed", } } } @@ -196,6 +203,17 @@ impl DomainError { } } + /// Creates a precondition-failed error (RFC 7232 / CAS mismatch) + pub fn precondition_failed>(entity_type: &'static str, message: S) -> Self { + Self { + kind: ErrorKind::PreconditionFailed, + entity_type, + entity_id: None, + message: message.into(), + source: None, + } + } + /// Creates a validation error pub fn validation_error>(message: S) -> Self { Self { diff --git a/src/infrastructure/repositories/pg/file_blob_write_repository.rs b/src/infrastructure/repositories/pg/file_blob_write_repository.rs index f4673fc1..b84a5378 100644 --- a/src/infrastructure/repositories/pg/file_blob_write_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_write_repository.rs @@ -153,6 +153,15 @@ impl FileBlobWriteRepository { /// row — not the row's owner. D2 shared drives let non-owners /// overwrite content; the previous `updated_by = f.user_id` would /// have silently recorded the wrong principal. + /// `expected_hash`, when `Some`, turns this into a real + /// compare-and-swap: the SET clause only takes effect if the row's + /// `blob_hash` still matches at the moment the `FOR UPDATE` lock is + /// held (same statement, same transaction — no gap a concurrent + /// writer can land in). A mismatch leaves the row untouched and is + /// reported back via the `matched` flag rather than silently + /// overwriting a sibling PATCH's content. `None` preserves the old + /// blind-overwrite behaviour for PUT/WOPI/chunked-upload finalize, + /// where last-write-wins is the intended HTTP semantics. async fn swap_blob_hash( &self, file_id: &str, @@ -160,57 +169,83 @@ impl FileBlobWriteRepository { new_size: i64, modified_at: Option, caller_id: Uuid, + expected_hash: Option<&str>, ) -> Result<(String, i64), DomainError> { - // Atomic CTE: capture old hash then update in one round-trip, no TOCTOU. + // Atomic CTE: capture old hash then conditionally update in one + // round-trip, no TOCTOU. The CASE arms make the SET a no-op when + // `expected_hash` is given and doesn't match `old.blob_hash` — + // the row is still returned (with its unchanged values) so the + // caller can tell "mismatch" apart from "file not found". // Deadlock victims (40P01) retry before the compensation below runs — // a successful retry must keep the new blob reference alive. - let (old_hash, updated_at) = match retry_on_deadlock("files.swap_blob_hash", || { - sqlx::query_as::<_, (String, i64)>( - r#" + let (old_hash, updated_at, matched) = + match retry_on_deadlock("files.swap_blob_hash", || { + sqlx::query_as::<_, (String, i64, bool)>( + r#" WITH old AS ( SELECT id, blob_hash FROM storage.files WHERE id = $3::uuid FOR UPDATE ) UPDATE storage.files f - SET blob_hash = $1, size = $2, - updated_at = COALESCE(to_timestamp($4), NOW()), - updated_by = $5 + SET blob_hash = CASE WHEN $6::text IS NULL OR old.blob_hash = $6 + THEN $1 ELSE f.blob_hash END, + size = CASE WHEN $6::text IS NULL OR old.blob_hash = $6 + THEN $2 ELSE f.size END, + updated_at = CASE WHEN $6::text IS NULL OR old.blob_hash = $6 + THEN COALESCE(to_timestamp($4), NOW()) ELSE f.updated_at END, + updated_by = CASE WHEN $6::text IS NULL OR old.blob_hash = $6 + THEN $5 ELSE f.updated_by END FROM old WHERE f.id = old.id - RETURNING old.blob_hash, EXTRACT(EPOCH FROM f.updated_at)::bigint + RETURNING old.blob_hash, EXTRACT(EPOCH FROM f.updated_at)::bigint, + ($6::text IS NULL OR old.blob_hash = $6) "#, - ) - .bind(new_hash) - .bind(new_size) - .bind(file_id) - .bind(modified_at.map(|t| t as f64)) - .bind(caller_id) - .fetch_optional(self.pool.as_ref()) - }) - .await - { - Ok(Some(row)) => row, - Ok(None) => { - // File not found — compensate: remove the new blob ref - if let Err(e) = self.dedup.remove_reference(new_hash).await { - tracing::error!("Blob orphaned after missing file: {}", e); + ) + .bind(new_hash) + .bind(new_size) + .bind(file_id) + .bind(modified_at.map(|t| t as f64)) + .bind(caller_id) + .bind(expected_hash) + .fetch_optional(self.pool.as_ref()) + }) + .await + { + Ok(Some(row)) => row, + Ok(None) => { + // File not found — compensate: remove the new blob ref + if let Err(e) = self.dedup.remove_reference(new_hash).await { + tracing::error!("Blob orphaned after missing file: {}", e); + } + return Err(DomainError::not_found("File", file_id)); } - return Err(DomainError::not_found("File", file_id)); - } - Err(e) => { - // UPDATE failed — compensate: remove the new blob ref - if let Err(rollback_err) = self.dedup.remove_reference(new_hash).await { - tracing::error!( - "Blob orphaned after failed UPDATE — hash: {}, err: {}", - &new_hash[..12], - rollback_err - ); + Err(e) => { + // UPDATE failed — compensate: remove the new blob ref + if let Err(rollback_err) = self.dedup.remove_reference(new_hash).await { + tracing::error!( + "Blob orphaned after failed UPDATE — hash: {}, err: {}", + &new_hash[..12], + rollback_err + ); + } + return Err(DomainError::internal_error( + "FileBlobWrite", + format!("update: {e}"), + )); } - return Err(DomainError::internal_error( - "FileBlobWrite", - format!("update: {e}"), - )); + }; + + if !matched { + // CAS lost the race — some other writer's content is now the + // row's truth. Release the blob we ingested for nothing; + // nothing was written. + if let Err(e) = self.dedup.remove_reference(new_hash).await { + tracing::error!("Blob orphaned after CAS mismatch: {}", e); } - }; + return Err(DomainError::precondition_failed( + "File", + "content was modified concurrently", + )); + } // Decrement old blob ref (only if hash changed, best-effort) if old_hash != new_hash @@ -790,12 +825,20 @@ impl FileWritePort for FileBlobWriteRepository { size: u64, modified_at: Option, caller_id: Uuid, + expected_hash: Option<&str>, ) -> Result<(String, i64), DomainError> { // The content was already ingested into the chunk store by the // upload-ingest layer; swap_blob_hash consumes its reference and // releases it on failure. let swapped = self - .swap_blob_hash(file_id, blob_hash, size as i64, modified_at, caller_id) + .swap_blob_hash( + file_id, + blob_hash, + size as i64, + modified_at, + caller_id, + expected_hash, + ) .await?; // The file now maps to a different blob — drop the read-side cache // entry so streaming downloads cannot serve the previous content diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 45875c85..67e77014 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -14,7 +14,9 @@ use axum::{ }; use bytes::{Buf, Bytes}; use chrono::Utc; +use futures::stream::{self, Stream}; use quick_xml::Writer; +use std::pin::Pin; use uuid::Uuid; use crate::application::adapters::webdav_adapter::{ @@ -29,6 +31,7 @@ use crate::application::ports::file_ports::{FileManagementUseCase, FileUploadUse use crate::application::ports::folder_ports::FolderUseCase; use crate::application::ports::storage_ports::StorageUsagePort; use crate::application::services::file_retrieval_service::FileRetrievalService; +use crate::application::services::file_upload_service::FileUploadService; use crate::application::services::folder_service::FolderService; use crate::common::di::AppState; use crate::domain::repositories::drive_repository::DriveRepository; @@ -38,6 +41,7 @@ use crate::infrastructure::services::webdav_dead_property_store::{DeadPropertySt use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::{AuthUser, CurrentUser}; use crate::interfaces::range_requests::{not_modified_response, range_response}; +use crate::interfaces::upload_ingest::{IngestedBlob, RangeSegment, discard_ingested}; use percent_encoding::{AsciiSet, NON_ALPHANUMERIC, percent_decode_str, utf8_percent_encode}; use std::collections::HashMap; use std::sync::Arc; @@ -408,6 +412,7 @@ async fn handle_webdav_dispatch( "GET" => handle_get(state, req, path).await, "HEAD" => handle_head(state, req, path).await, "PUT" => handle_put(state, req, path).await, + "PATCH" => handle_patch(state, req, path).await, "MKCOL" => handle_mkcol(state, req, path).await, "DELETE" => handle_delete(state, req, path).await, "MOVE" => handle_move(state, req, path).await, @@ -439,7 +444,7 @@ async fn handle_options(_path: String) -> Result, AppError> { .header(HEADER_DAV, "1, 2") // Class 1 and 2 WebDAV support .header( header::ALLOW, - "OPTIONS, GET, HEAD, PUT, DELETE, PROPFIND, PROPPATCH, MKCOL, COPY, MOVE, LOCK, UNLOCK", + "OPTIONS, GET, HEAD, PUT, PATCH, DELETE, PROPFIND, PROPPATCH, MKCOL, COPY, MOVE, LOCK, UNLOCK", ) .body(Body::empty()) .unwrap()) @@ -1655,7 +1660,7 @@ fn evaluate_if_header( /// /// Shared by `handle_put`, `handle_delete`, `handle_move`, /// `handle_copy`, and `handle_proppatch`. -fn enforce_native_lock( +pub(crate) fn enforce_native_lock( lock_store: &crate::infrastructure::services::webdav_lock_service::WebDavLockStore, if_header: Option<&str>, path: &str, @@ -1891,39 +1896,20 @@ async fn handle_put( // ── RFC 7232 conditional preconditions ──────────────────────────── // Evaluated before ingesting the body to save bandwidth on doomed requests. - if let Some(ref inm) = if_none_match { - // If-None-Match: * → fail if resource exists (prevent overwrite) - if inm == "*" && file_existed { - return Err(AppError::precondition_failed( - "If-None-Match: * — resource already exists", - )); - } + // Shared with `handle_patch` (both surfaces) — handles comma-separated + // multi-value lists and the weak/strong distinction the previous + // hand-rolled single-tag comparison here didn't. + if let Some(ref value) = if_none_match + && if_none_match_precondition_fails(value, current_etag.as_deref()) + { + return Err(AppError::precondition_failed( + "If-None-Match — resource already exists with that ETag", + )); } - if let Some(ref im) = if_match { - if im == "*" { - // If-Match: * → fail if resource does not exist - if !file_existed { - return Err(AppError::precondition_failed( - "If-Match: * — resource does not exist", - )); - } - } else { - // If-Match: → strong comparison against current ETag - match ¤t_etag { - None => { - return Err(AppError::precondition_failed( - "If-Match — resource does not exist", - )); - } - Some(etag) => { - let client_tag = im.trim_matches('"'); - let server_tag = etag.trim_matches('"'); - if client_tag != server_tag { - return Err(AppError::precondition_failed("If-Match — ETag mismatch")); - } - } - } - } + if let Some(ref value) = if_match + && if_match_precondition_fails(value, current_etag.as_deref()) + { + return Err(AppError::precondition_failed("If-Match — ETag mismatch")); } // ── Streaming ingest ────────────────────────────────────────────── @@ -1971,6 +1957,7 @@ async fn handle_put( &content_type, None, user.id, + None, ) .await; @@ -1986,7 +1973,7 @@ async fn handle_put( }; Ok(Response::builder() .status(status) - .header(header::ETAG, &file_dto.etag) + .header(header::ETAG, format!("\"{}\"", file_dto.etag)) .body(Body::empty()) .unwrap()) } @@ -1998,6 +1985,412 @@ async fn handle_put( } } +/// Parses the `X-Update-Range` header used by [`handle_patch`] (RFC 5789 +/// partial content updates): either `append`, or `bytes=-` +/// (inclusive, 0-based). For the explicit-range form both bounds must fall +/// strictly within the current file size — growing the file via a byte +/// range isn't supported, use `append` or PUT for that. +/// +/// Returns `(start, end)`; `end` is `None` for `append`. +/// +/// `pub(crate)` so the NextCloud-surface PATCH handler +/// (`nextcloud/webdav_handler.rs::handle_patch`) can reuse it instead of +/// duplicating the parsing logic. +pub(crate) fn parse_update_range(header: &str, size: u64) -> Result<(u64, Option), AppError> { + let header = header.trim(); + if header.eq_ignore_ascii_case("append") { + return Ok((size, None)); + } + let spec = header.strip_prefix("bytes=").ok_or_else(|| { + AppError::bad_request("X-Update-Range must be 'append' or 'bytes=-'") + })?; + let (start_str, end_str) = spec + .split_once('-') + .ok_or_else(|| AppError::bad_request("X-Update-Range must be 'bytes=-'"))?; + let start: u64 = start_str + .parse() + .map_err(|_| AppError::bad_request("X-Update-Range: invalid start offset"))?; + let end: u64 = end_str + .parse() + .map_err(|_| AppError::bad_request("X-Update-Range: invalid end offset"))?; + if start > end { + return Err(AppError::bad_request( + "X-Update-Range: start must be <= end", + )); + } + if end >= size { + return Err(AppError::new( + StatusCode::RANGE_NOT_SATISFIABLE, + format!("X-Update-Range end {end} is out of bounds for a {size}-byte file"), + "RangeNotSatisfiable", + )); + } + Ok((start, Some(end))) +} + +/// Strip the optional `W/` weak prefix and surrounding double-quotes +/// from one ETag value in an `If-Match` / `If-None-Match` list. Returns +/// `(is_weak, inner)`. +fn parse_etag_value(raw: &str) -> (bool, &str) { + let trimmed = raw.trim(); + if let Some(rest) = trimmed.strip_prefix("W/") { + (true, rest.trim().trim_matches('"')) + } else { + (false, trimmed.trim_matches('"')) + } +} + +/// RFC 7232 §3.2 — `If-None-Match` fails when: +/// - the header value is `*` and a current representation exists, OR +/// - any listed ETag matches the current representation (weak comparison +/// — weak validators in the request are equivalent to strong for the +/// match itself, only If-Match is required to be strong). +/// +/// `pub(crate)` so both the plain and NextCloud-surface WebDAV handlers +/// share one RFC 7232-conformant implementation instead of each +/// reimplementing ETag comparison (multi-value lists, `W/` weak prefix). +pub(crate) fn if_none_match_precondition_fails(header: &str, current_etag: Option<&str>) -> bool { + let v = header.trim(); + if v == "*" { + return current_etag.is_some(); + } + let Some(current) = current_etag else { + return false; + }; + v.split(',').any(|tag| { + let (_, parsed) = parse_etag_value(tag); + !parsed.is_empty() && parsed == current + }) +} + +/// RFC 7232 §3.1 — `If-Match` fails when: +/// - the resource doesn't currently exist (no strong validator to match), OR +/// - the header isn't `*` and no listed ETag strong-matches the current one +/// (weak validators in the request never satisfy a strong-match). +pub(crate) fn if_match_precondition_fails(header: &str, current_etag: Option<&str>) -> bool { + let v = header.trim(); + let Some(current) = current_etag else { + return true; + }; + if v == "*" { + return false; + } + !v.split(',').any(|tag| { + let (is_weak, parsed) = parse_etag_value(tag); + !is_weak && !parsed.is_empty() && parsed == current + }) +} + +/// Build the untouched prefix/suffix byte-range streams either side of a +/// PATCH edit, paired with their known lengths (`upload_ingest::RangeSegment`) +/// ready to hand to `ingest_range_patch_to_cas`. +/// +/// `pub(crate)` so both the plain and NextCloud-surface PATCH handlers share +/// one implementation instead of each re-deriving the same offsets — this was +/// byte-identical duplicated code before the DRY pass that added this fn. +pub(crate) async fn splice_patch_streams( + file_retrieval: &FileRetrievalService, + file_id: &str, + caller_id: Uuid, + start: u64, + end: Option, + file_size: u64, +) -> Result<(RangeSegment, RangeSegment), AppError> { + let prefix_stream: Pin> + Send>> = + if start == 0 { + Box::pin(stream::empty()) + } else { + Box::into_pin( + file_retrieval + .get_file_range_stream_with_perms(file_id, caller_id, 0, Some(start)) + .await + .map_err(AppError::from)?, + ) + }; + let suffix_len = match end { + Some(end) if end + 1 < file_size => file_size - (end + 1), + _ => 0, + }; + let suffix_stream: Pin> + Send>> = match end + { + Some(end) if end + 1 < file_size => Box::into_pin( + file_retrieval + .get_file_range_stream_with_perms(file_id, caller_id, end + 1, None) + .await + .map_err(AppError::from)?, + ), + _ => Box::pin(stream::empty()), + }; + Ok(((prefix_stream, start), (suffix_stream, suffix_len))) +} + +/// Quota-check + compare-and-swap write for a PATCH edit already spliced and +/// ingested into the chunk store (`ingested`). On quota rejection the blob +/// reference just taken by ingest is released and `QuotaExceeded` (507) is +/// returned; on success this is the CAS write keyed on `expected_hash` (the +/// file's pre-splice content hash) that closes the race between two +/// concurrent PATCHes to disjoint ranges of the same file (see +/// `FileBlobWritePort::swap_blob_hash`). +/// +/// `pub(crate)` — shared by the plain and NextCloud-surface PATCH handlers; +/// `log_prefix` lets each surface keep its own log-line tag (`"WEBDAV PATCH"` +/// vs `"NC WEBDAV PATCH"`) for grep-ability. +#[allow(clippy::too_many_arguments)] +pub(crate) async fn cas_write_patch( + state: &AppState, + upload_service: &FileUploadService, + path: &str, + drive_id: Uuid, + ingested: &IngestedBlob, + caller_id: Uuid, + expected_hash: &str, + log_prefix: &str, +) -> Result { + if let Some(storage_svc) = state.storage_usage_service.as_ref() + && let Err(err) = storage_svc + .check_storage_quota(caller_id, ingested.size) + .await + { + discard_ingested(&state.core.dedup_service, ingested).await; + tracing::warn!( + "⛔ {} REJECTED (quota): user={}, file={}, size={}", + log_prefix, + caller_id, + path, + ingested.size + ); + return Err(AppError::new( + StatusCode::INSUFFICIENT_STORAGE, + err.message, + "QuotaExceeded", + )); + } + + let content_type = ingested.content_type.clone(); + upload_service + .update_file_streaming_with_perms( + path, + drive_id, + ingested.stored(), + &content_type, + None, + caller_id, + Some(expected_hash), + ) + .await + .map_err(AppError::from) +} + +/** + * Handles PATCH requests (RFC 5789) for partial byte-range content updates. + * + * RFC 4918 §9.7.1 forbids partial content updates on PUT (see the explicit + * `Content-Range` rejection in [`handle_put`]); PATCH is the mechanism this + * server offers instead, via the `X-Update-Range` header (see + * [`parse_update_range`]). + * + * The new content is assembled by splicing the request body between the + * file's untouched prefix/suffix byte ranges and re-ingesting the result as + * one continuous stream through the same content-addressable pipeline PUT + * uses ([`upload_ingest::ingest_range_patch_to_cas`]) — unedited chunks on + * either side of the edit typically dedup for free. + * + * @param state The application state containing service dependencies + * @param req The HTTP request containing the partial content and + * `X-Update-Range` header + * @param path The requested resource path + * @return HTTP response: 204 with `Content-Range`/`ETag` on success + */ +async fn handle_patch( + state: Arc, + req: Request, + path: String, +) -> Result, AppError> { + use crate::interfaces::upload_ingest; + + let user = extract_user(&req)?; + let file_upload_service = &state.applications.file_upload_service; + let file_retrieval_service = &state.applications.file_retrieval_service; + + if path.is_empty() || path == "/" { + return Err(AppError::bad_request("Cannot PATCH the root folder")); + } + + // RFC 5789 doesn't define Content-Range semantics; this server uses a + // dedicated `X-Update-Range` header instead (see `parse_update_range`) + // to avoid ambiguity with HTTP Range-Request semantics. + if req.headers().contains_key(header::CONTENT_RANGE) { + return Err(AppError::bad_request( + "PATCH must not use Content-Range; use the X-Update-Range header instead", + )); + } + + let update_range_header = req + .headers() + .get("X-Update-Range") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()) + .ok_or_else(|| AppError::bad_request("PATCH requires an X-Update-Range header"))?; + + // Extract all headers before consuming `req` into the body stream. + let if_header_owned = req + .headers() + .get("If") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + let if_none_match = req + .headers() + .get(header::IF_NONE_MATCH) + .and_then(|v| v.to_str().ok()) + .map(|s| s.trim().to_string()); + let if_match = req + .headers() + .get(header::IF_MATCH) + .and_then(|v| v.to_str().ok()) + .map(|s| s.trim().to_string()); + let content_length = req + .headers() + .get(header::CONTENT_LENGTH) + .and_then(|v| v.to_str().ok()) + .and_then(|s| s.parse::().ok()); + let content_type = req + .headers() + .get(header::CONTENT_TYPE) + .and_then(|v| v.to_str().ok()) + .unwrap_or("application/octet-stream") + .to_string(); + let max_upload = state.core.config.storage.direct_put_max_bytes; + + let scope = resolve_webdav_scope_or_405(&state, user.id, &path).await?; + let drive_id = scope.drive_id; + let path = scope.db_path; + + // ── Existence check ─────────────────────────────────────────────── + // Unlike PUT, PATCH requires an existing file — a partial update of + // nothing isn't meaningful. Resolver is drive-scoped, not + // owner-scoped (see `handle_put`'s identical comment), so the + // explicit `authz.require(Read, …)` below is the defence-in-depth + // existence-proof before any field of `file` is trusted. + let resolver = state.path_resolver.as_ref().ok_or_else(|| { + AppError::method_not_allowed("PATCH requires WebDAV path resolver support") + })?; + let file = match resolver.resolve_path_in_drive(&path, drive_id).await { + Ok(ResolvedResource::File(f)) => f, + Ok(ResolvedResource::Folder(_)) => { + return Err(AppError::conflict("Cannot PATCH a directory")); + } + Err(_) => return Err(AppError::not_found(format!("File not found: {}", path))), + }; + let file_uuid = Uuid::parse_str(&file.id) + .map_err(|_| AppError::not_found(format!("File not found: {}", path)))?; + state + .authorization + .require( + Subject::User(user.id), + Permission::Read, + Resource::File(file_uuid), + ) + .await?; + + // ── Active-lock guard + RFC 4918 §10.4 If: evaluation ───────────── + if let Some(resp) = enforce_native_lock( + &state.webdav_lock_store, + if_header_owned.as_deref(), + &path, + Some(&file.etag), + ) { + return Ok(resp); + } + + // ── RFC 7232 conditional preconditions ──────────────────────────── + let current_etag = Some(file.etag.as_str()); + if let Some(ref value) = if_none_match + && if_none_match_precondition_fails(value, current_etag) + { + return Err(AppError::precondition_failed( + "If-None-Match — resource already exists with that ETag", + )); + } + if let Some(ref value) = if_match + && if_match_precondition_fails(value, current_etag) + { + return Err(AppError::precondition_failed("If-Match — ETag mismatch")); + } + + // ── Range parsing + validation ───────────────────────────────────── + let (start, end) = parse_update_range(&update_range_header, file.size)?; + if let (Some(end), Some(len)) = (end, content_length) { + let expected = end - start + 1; + if len != expected { + return Err(AppError::bad_request(format!( + "Content-Length {len} does not match X-Update-Range span {expected}" + ))); + } + } + + // ── Splice prefix/suffix around the patched span ─────────────────── + let (prefix_segment, suffix_segment) = splice_patch_streams( + file_retrieval_service, + &file.id, + user.id, + start, + end, + file.size, + ) + .await?; + let filename = crate::common::mime_detect::filename_from_path(&path).to_string(); + let ingested = upload_ingest::ingest_range_patch_to_cas( + prefix_segment, + req.into_body(), + suffix_segment, + &state.core.dedup_service, + &filename, + &content_type, + upload_ingest::PatchIngestBudget { + max_bytes: max_upload, + expected_body_len: end.map(|end| end - start + 1), + }, + ) + .await?; + + // ── Quota enforcement + atomic store, compare-and-swap on the + // pre-splice content hash ───────────────────────────────────────── + // `file.content_hash` was snapshotted before the (potentially slow) + // splice + CAS-ingest above. Passing it as `expected_hash` makes the + // write itself a compare-and-swap: the repository checks and applies + // under the same row lock, so nothing else can write to this file + // between the check and the write. This is what actually closes the + // race two concurrent PATCHes to disjoint ranges could otherwise hit + // — each individually passing its own If-Match check against the + // same stale snapshot, then blindly overwriting each other. + let new_size = ingested.size; + let file_dto = cas_write_patch( + &state, + file_upload_service, + &path, + drive_id, + &ingested, + user.id, + &file.content_hash, + "WEBDAV PATCH", + ) + .await?; + + // Everything from `start` to the new EOF reflects the patch + // (the untouched suffix, if any, may have shifted when the + // body's length differs from the replaced span). + let range_end = new_size.saturating_sub(1); + Ok(Response::builder() + .status(StatusCode::NO_CONTENT) + .header(header::ETAG, format!("\"{}\"", file_dto.etag)) + .header( + header::CONTENT_RANGE, + format!("bytes {}-{}/{}", start, range_end, new_size), + ) + .body(Body::empty()) + .unwrap()) +} + /** * Handles MKCOL requests to create folders. * @@ -3385,4 +3778,97 @@ mod tests { "/webdav/My%20Photos/2024/" ); } + + // ── RFC 5789 PATCH: `X-Update-Range` parsing ──────────────────── + + #[test] + fn parse_update_range_append() { + assert_eq!(parse_update_range("append", 100).unwrap(), (100, None)); + assert_eq!(parse_update_range("APPEND", 0).unwrap(), (0, None)); + } + + #[test] + fn parse_update_range_explicit_span() { + assert_eq!(parse_update_range("bytes=5-9", 100).unwrap(), (5, Some(9))); + // Single-byte span at offset 0. + assert_eq!(parse_update_range("bytes=0-0", 1).unwrap(), (0, Some(0))); + } + + #[test] + fn parse_update_range_rejects_missing_prefix() { + assert!(parse_update_range("5-9", 100).is_err()); + } + + #[test] + fn parse_update_range_rejects_malformed_bounds() { + assert!(parse_update_range("bytes=abc-9", 100).is_err()); + assert!(parse_update_range("bytes=5-abc", 100).is_err()); + assert!(parse_update_range("bytes=9", 100).is_err()); + } + + #[test] + fn parse_update_range_rejects_start_after_end() { + assert!(parse_update_range("bytes=9-5", 100).is_err()); + } + + #[test] + fn parse_update_range_rejects_end_at_or_past_size() { + // `end` must be strictly within the current file — growing the + // file via a byte-range PATCH isn't supported (use `append`). + let err = parse_update_range("bytes=5-9", 9).unwrap_err(); + assert_eq!(err.status_code, StatusCode::RANGE_NOT_SATISFIABLE); + assert!(parse_update_range("bytes=0-0", 0).is_err()); + } + + // ── RFC 7232 If-Match / If-None-Match — multi-value lists ─────── + // + // Regression coverage for `handle_put`'s hand-rolled precondition + // check, which only ever compared the header as a single tag and + // never split on commas — a client sending the standard + // comma-separated multi-value form would silently mismatch even + // when one of the listed ETags matched. Both handlers now share + // `if_none_match_precondition_fails`/`if_match_precondition_fails`, + // which already handled this correctly for `handle_patch`. + + #[test] + fn if_none_match_multi_value_list_matches_second_tag() { + assert!(if_none_match_precondition_fails( + r#""aaa", "bbb", "ccc""#, + Some("bbb") + )); + } + + #[test] + fn if_none_match_multi_value_list_no_match_passes() { + assert!(!if_none_match_precondition_fails( + r#""aaa", "bbb", "ccc""#, + Some("zzz") + )); + } + + #[test] + fn if_match_multi_value_list_matches_last_tag() { + assert!(!if_match_precondition_fails( + r#""aaa", "bbb", "ccc""#, + Some("ccc") + )); + } + + #[test] + fn if_match_multi_value_list_no_match_fails() { + assert!(if_match_precondition_fails( + r#""aaa", "bbb", "ccc""#, + Some("zzz") + )); + } + + #[test] + fn if_match_weak_tag_in_list_never_satisfies() { + // If-Match requires a strong comparison — a weak validator in the + // list must not satisfy it even if the underlying tag matches. + assert!(if_match_precondition_fails( + r#"W/"aaa", "bbb""#, + Some("aaa") + )); + } } diff --git a/src/interfaces/api/handlers/wopi_handler.rs b/src/interfaces/api/handlers/wopi_handler.rs index f3a2c1f2..36929b9f 100644 --- a/src/interfaces/api/handlers/wopi_handler.rs +++ b/src/interfaces/api/handlers/wopi_handler.rs @@ -413,6 +413,7 @@ async fn put_file( &content_type, None, claims_sub_uuid, + None, ) .await; diff --git a/src/interfaces/errors.rs b/src/interfaces/errors.rs index 148ad70c..a859e261 100644 --- a/src/interfaces/errors.rs +++ b/src/interfaces/errors.rs @@ -131,6 +131,7 @@ impl From for AppError { ErrorKind::DatabaseError => StatusCode::INTERNAL_SERVER_ERROR, ErrorKind::QuotaExceeded => StatusCode::INSUFFICIENT_STORAGE, ErrorKind::Conflict => StatusCode::CONFLICT, + ErrorKind::PreconditionFailed => StatusCode::PRECONDITION_FAILED, }; Self { diff --git a/src/interfaces/nextcloud/uploads_handler.rs b/src/interfaces/nextcloud/uploads_handler.rs index ccd3e66a..47c05c56 100644 --- a/src/interfaces/nextcloud/uploads_handler.rs +++ b/src/interfaces/nextcloud/uploads_handler.rs @@ -466,6 +466,10 @@ async fn handle_assemble( // AuthZ audit #2 (2026-07-12): route DomainError through // `AppError::from` so authz denials keep the graduated 403/404 // shape instead of collapsing into 500. + // + // No client-supplied ETag to enforce here (NC chunked MOVE has no + // If-Match semantics) — `expected_hash: None`, same as every other + // plain-write callsite; only PATCH's CAS passes `Some(&hash)`. let dto = match upload_service .update_file_streaming_with_perms( &internal_path, @@ -474,6 +478,7 @@ async fn handle_assemble( &content_type, oc_mtime, user.id, + None, ) .await { diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index 721dd7f6..142fce3d 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -22,6 +22,7 @@ use crate::application::ports::file_ports::{ FileManagementUseCase, FileRetrievalUseCase, FileUploadUseCase, }; use crate::application::ports::folder_ports::FolderUseCase; +use crate::application::ports::storage_ports::StorageUsagePort; use crate::application::ports::trash_ports::TrashUseCase; use crate::common::di::AppState; use crate::common::mime_detect::filename_from_path; @@ -29,12 +30,15 @@ use crate::domain::services::authorization::{Permission, Resource, Subject}; use crate::infrastructure::services::path_resolver_service::ResolvedResource; use crate::infrastructure::services::webdav_dead_property_store::ResourceRef; use crate::interfaces::api::handlers::webdav_handler::{ - PROPFIND_BATCH_SIZE, dead_props_for, file_dead_props, files_dead_props_map, folder_dead_props, - folders_dead_props_map, + PROPFIND_BATCH_SIZE, cas_write_patch, dead_props_for, enforce_native_lock, file_dead_props, + files_dead_props_map, folder_dead_props, folders_dead_props_map, if_match_precondition_fails, + if_none_match_precondition_fails, parse_update_range, splice_patch_streams, }; use crate::interfaces::errors::AppError; use crate::interfaces::range_requests::{not_modified_response, range_response}; -use crate::interfaces::upload_ingest::ingest_body_to_cas; +use crate::interfaces::upload_ingest::{ + PatchIngestBudget, discard_ingested, ingest_body_to_cas, ingest_range_patch_to_cas, +}; /// Extension trait to map XML write errors to `String` concisely. trait XmlResultExt { @@ -74,15 +78,24 @@ const HEADER_DAV: HeaderName = HeaderName::from_static("dav"); /// Replaces the pre-D0 hardcoded `"My Folder - {username}/"` prefix. pub fn nc_to_internal_path(chroot: &FolderDto, subpath: &str) -> Result { let subpath = subpath.trim_matches('/'); + // `chroot.path` comes from `Folder::path_string()` / + // `StoragePath::to_string()`, which prepends a leading `/` (e.g. + // `"/Personal"`) — trim it so the result matches the leading- + // slash-free convention `storage.folders.path` (and the plain + // WebDAV surface's `db_path`) actually use. Without this, exact- + // string comparisons against a plain-surface path (e.g. the + // in-memory WebDAV lock store's key) silently mismatch even + // though DB-backed lookups tolerate the discrepancy. + let chroot_path = chroot.path.trim_start_matches('/'); if subpath.is_empty() { - return Ok(chroot.path.clone()); + return Ok(chroot_path.to_string()); } // Reject path traversal attempts. if subpath.split('/').any(|seg| seg == ".." || seg == ".") { return Err(AppError::bad_request("Invalid path: traversal not allowed")); } - Ok(format!("{}/{}", chroot.path, subpath)) + Ok(format!("{}/{}", chroot_path, subpath)) } /// Strip the caller's chroot prefix from an internal @@ -263,6 +276,7 @@ pub async fn handle_nc_webdav( "PROPFIND" => handle_propfind(state, req, &session, &subpath).await, "GET" => handle_get(state, &session, &subpath, req.headers()).await, "PUT" => handle_put(state, req, &session, &subpath).await, + "PATCH" => handle_patch(state, req, &session, &subpath).await, "MKCOL" => handle_mkcol(state, &session, &subpath).await, "DELETE" => handle_delete(state, &session, &subpath).await, "MOVE" => handle_move(state, req, &session, &subpath).await, @@ -298,7 +312,7 @@ fn handle_options() -> Result, AppError> { .header(HEADER_DAV, "1, 3") .header( header::ALLOW, - "OPTIONS, GET, HEAD, PUT, DELETE, MKCOL, MOVE, PROPFIND, PROPPATCH, REPORT, SEARCH", + "OPTIONS, GET, HEAD, PUT, PATCH, DELETE, MKCOL, MOVE, PROPFIND, PROPPATCH, REPORT, SEARCH", ) .body(Body::empty()) .unwrap()) @@ -821,55 +835,6 @@ async fn handle_proppatch( // ──────────────────── PUT ──────────────────── -/// Strip the optional `W/` weak prefix and surrounding double-quotes -/// from one ETag value in an `If-Match` / `If-None-Match` list. Returns -/// `(is_weak, inner)`. -fn parse_etag_value(raw: &str) -> (bool, &str) { - let trimmed = raw.trim(); - if let Some(rest) = trimmed.strip_prefix("W/") { - (true, rest.trim().trim_matches('"')) - } else { - (false, trimmed.trim_matches('"')) - } -} - -/// RFC 7232 §3.2 — `If-None-Match` fails for PUT when: -/// - the header value is `*` and a current representation exists, OR -/// - any listed ETag matches the current representation (weak comparison -/// — weak validators in the request are equivalent to strong for the -/// match itself, only If-Match is required to be strong). -fn if_none_match_precondition_fails(header: &str, current_etag: Option<&str>) -> bool { - let v = header.trim(); - if v == "*" { - return current_etag.is_some(); - } - let Some(current) = current_etag else { - return false; - }; - v.split(',').any(|tag| { - let (_, parsed) = parse_etag_value(tag); - !parsed.is_empty() && parsed == current - }) -} - -/// RFC 7232 §3.1 — `If-Match` fails for PUT when: -/// - the resource doesn't currently exist (no strong validator to match), OR -/// - the header isn't `*` and no listed ETag strong-matches the current one -/// (weak validators in the request never satisfy a strong-match). -fn if_match_precondition_fails(header: &str, current_etag: Option<&str>) -> bool { - let v = header.trim(); - let Some(current) = current_etag else { - return true; - }; - if v == "*" { - return false; - } - !v.split(',').any(|tag| { - let (is_weak, parsed) = parse_etag_value(tag); - !is_weak && !parsed.is_empty() && parsed == current - }) -} - fn precondition_failed_response() -> Response { Response::builder() .status(StatusCode::PRECONDITION_FAILED) @@ -901,6 +866,13 @@ async fn handle_put( .and_then(|v| v.to_str().ok()) .and_then(|v| v.parse::().ok()); + // Extract before consuming `req` into the body stream further down. + let if_header = req + .headers() + .get("If") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + // ── Conditional preconditions (RFC 7232 §3.1 / §3.2) ───────────── // Evaluated BEFORE body ingestion so a rejected PUT doesn't waste // bandwidth or disk I/O on a body the server is going to throw away. @@ -929,6 +901,49 @@ async fn handle_put( return Ok(precondition_failed_response()); } + // ── Existence-check depth (RFC 4918 §9.7.1) ─────────────────────── + // Mirrors the plain WebDAV surface's `handle_put`: PUT to an existing + // directory is 400, PUT under a missing parent is 409 (not the generic + // 500 a downstream `NotFound` would otherwise surface as). + if existing.is_none() { + if state + .applications + .folder_service + .get_folder_by_path(&internal_path, chroot.drive_id) + .await + .is_ok() + { + return Err(AppError::bad_request("Cannot PUT to a directory")); + } + let parent_path = internal_path + .rfind('/') + .map(|i| &internal_path[..i]) + .unwrap_or(""); + if !parent_path.is_empty() { + state + .applications + .folder_service + .get_folder_by_path(parent_path, chroot.drive_id) + .await + .map_err(|_| { + AppError::conflict(format!("Parent folder not found: {}", parent_path)) + })?; + } + } + + // ── Active-lock guard (RFC 4918 §10.4 If: evaluation) ───────────── + // Shared with the plain WebDAV surface and with this surface's own + // `handle_patch`, so a LOCK taken via /webdav/ also protects the same + // file reached through /remote.php/dav/. + if let Some(resp) = enforce_native_lock( + &state.webdav_lock_store, + if_header.as_deref(), + &internal_path, + current_etag, + ) { + return Ok(resp); + } + // ── Direct PUT cap ─────────────────────────────────────────────── // We use `direct_put_max_bytes` (default 1 GiB), not `max_upload_size` // (default 10 GB). Larger files must come through the chunked upload @@ -961,13 +976,35 @@ async fn handle_put( // using the lookup already done above for the precondition check. let existed = existing.is_some(); + // ── Quota enforcement ───────────────────────────────────────────── + if let Some(storage_svc) = state.storage_usage_service.as_ref() + && let Err(err) = storage_svc + .check_storage_quota(session.user.id, ingested.size) + .await + { + discard_ingested(&state.core.dedup_service, &ingested).await; + tracing::warn!( + "⛔ NC WEBDAV PUT REJECTED (quota): user={}, file={}, size={}", + session.user.id, + internal_path, + ingested.size + ); + return Err(AppError::new( + StatusCode::INSUFFICIENT_STORAGE, + err.message, + "QuotaExceeded", + )); + } + // Single streaming path — handles both update and create internally, // swapping the file row onto the already-ingested blob. // AuthZ audit #6 (2026-07-12): route `_with_perms` errors through // `AppError::from` so authz denials surface as 404 (the anti-enum // shape) instead of a `map_err → internal_error` 500 that gives a // probing caller an "exists-but-denied" oracle. Also preserves - // `QuotaExceeded → 507`, `AlreadyExists → 409`, `InvalidInput → 400`. + // `QuotaExceeded → 507`, `AlreadyExists → 409`, `InvalidInput → 400` — + // matching this surface's own `handle_patch` and the plain WebDAV + // `handle_put`. let stored = upload_service .update_file_streaming_with_perms( &internal_path, @@ -976,6 +1013,7 @@ async fn handle_put( &content_type, oc_mtime, session.user.id, + None, ) .await .map_err(AppError::from)?; @@ -994,6 +1032,224 @@ async fn handle_put( .unwrap()) } +// ──────────────────── PATCH ──────────────────── + +/// Handles PATCH requests (RFC 5789) for partial byte-range content +/// updates on the NextCloud file surface — extends the plain WebDAV +/// surface's `X-Update-Range` mechanism (see +/// `api/handlers/webdav_handler.rs::handle_patch` / `parse_update_range`) +/// here. New content is assembled by splicing the request body between +/// the file's untouched prefix/suffix byte ranges and re-ingesting the +/// result as one continuous stream through the same content-addressable +/// pipeline `handle_put` uses ([`ingest_range_patch_to_cas`]) — unedited +/// chunks on either side of the edit typically dedup for free. +/// +/// Shares an active-lock guard with the plain WebDAV surface (see below) so +/// a LOCK taken via `/webdav/` also protects the same file reached through +/// `/remote.php/dav/`. +async fn handle_patch( + state: Arc, + req: Request, + session: &crate::interfaces::nextcloud::session::NcSession, + subpath: &str, +) -> Result, AppError> { + let chroot = session.require_chroot()?; + let internal_path = nc_to_internal_path(chroot, subpath)?; + let file_service = &state.applications.file_retrieval_service; + let upload_service = &state.applications.file_upload_service; + + if subpath.is_empty() || subpath == "/" { + return Err(AppError::bad_request("Cannot PATCH the root folder")); + } + + // RFC 5789 doesn't define Content-Range semantics; this server uses a + // dedicated `X-Update-Range` header instead (see `parse_update_range`) + // to avoid ambiguity with HTTP Range-Request semantics. + if req.headers().contains_key(header::CONTENT_RANGE) { + return Err(AppError::bad_request( + "PATCH must not use Content-Range; use the X-Update-Range header instead", + )); + } + + let update_range_header = req + .headers() + .get("X-Update-Range") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()) + .ok_or_else(|| AppError::bad_request("PATCH requires an X-Update-Range header"))?; + + // Extract all headers before consuming `req` into the body stream. + let if_none_match = req + .headers() + .get(header::IF_NONE_MATCH) + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + let if_match = req + .headers() + .get(header::IF_MATCH) + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + let content_length = req + .headers() + .get(header::CONTENT_LENGTH) + .and_then(|v| v.to_str().ok()) + .and_then(|s| s.parse::().ok()); + let claimed_type = req + .headers() + .get(header::CONTENT_TYPE) + .and_then(|v| v.to_str().ok()) + .unwrap_or("application/octet-stream") + .to_string(); + let if_header = req + .headers() + .get("If") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + let max_upload = state.core.config.storage.direct_put_max_bytes; + + // ── Existence check ─────────────────────────────────────────────── + // Unlike PUT, PATCH requires an existing file — a partial update of + // nothing isn't meaningful. On lookup failure, distinguish "it's a + // directory" (409, matching the plain WebDAV surface) from "it + // doesn't exist at all" (404) instead of collapsing both to 404. + let file = match file_service + .get_file_by_path(&internal_path, chroot.drive_id) + .await + { + Ok(file) => file, + Err(_) => { + if state + .applications + .folder_service + .get_folder_by_path(&internal_path, chroot.drive_id) + .await + .is_ok() + { + return Err(AppError::conflict("Cannot PATCH a directory")); + } + return Err(AppError::not_found(format!( + "File not found: {}", + internal_path + ))); + } + }; + + // `get_file_by_path` performs no authorization check (see its own + // doc comment) — mirrors the plain WebDAV surface's explicit + // defense-in-depth Read check right after resolving the file, so a + // caller without Read on this specific file can't learn its size or + // ETag via the precondition/range-bounds responses below. + let file_uuid = Uuid::parse_str(&file.id) + .map_err(|_| AppError::not_found(format!("File not found: {}", internal_path)))?; + state + .authorization + .require( + Subject::User(session.user.id), + Permission::Read, + Resource::File(file_uuid), + ) + .await?; + + // ── Active-lock guard (RFC 4918 §10.4 If: evaluation) ───────────── + // Shared with the plain WebDAV surface so a LOCK taken via /webdav/ + // also protects the same file reached through /remote.php/dav/. + if let Some(resp) = enforce_native_lock( + &state.webdav_lock_store, + if_header.as_deref(), + &internal_path, + Some(&file.etag), + ) { + return Ok(resp); + } + + // ── RFC 7232 conditional preconditions ──────────────────────────── + let current_etag = Some(file.etag.as_str()); + if let Some(ref value) = if_none_match + && if_none_match_precondition_fails(value, current_etag) + { + return Ok(precondition_failed_response()); + } + if let Some(ref value) = if_match + && if_match_precondition_fails(value, current_etag) + { + return Ok(precondition_failed_response()); + } + + // ── Range parsing + validation ───────────────────────────────────── + let (start, end) = parse_update_range(&update_range_header, file.size)?; + if let (Some(end), Some(len)) = (end, content_length) { + let expected = end - start + 1; + if len != expected { + return Err(AppError::bad_request(format!( + "Content-Length {len} does not match X-Update-Range span {expected}" + ))); + } + } + + // ── Splice prefix/suffix around the patched span ─────────────────── + let (prefix_segment, suffix_segment) = splice_patch_streams( + file_service, + &file.id, + session.user.id, + start, + end, + file.size, + ) + .await?; + let filename = filename_from_path(subpath).to_string(); + let ingested = ingest_range_patch_to_cas( + prefix_segment, + req.into_body(), + suffix_segment, + &state.core.dedup_service, + &filename, + &claimed_type, + PatchIngestBudget { + max_bytes: max_upload, + expected_body_len: end.map(|end| end - start + 1), + }, + ) + .await?; + + // ── Quota enforcement + atomic store, compare-and-swap on the + // pre-splice content hash ───────────────────────────────────────── + // `file.content_hash` was snapshotted before the (potentially slow) + // splice + CAS-ingest above. Passing it as `expected_hash` makes the + // write itself a compare-and-swap: the repository checks and applies + // under the same row lock, so nothing else can write to this file + // between the check and the write. This is what actually closes the + // race two concurrent PATCHes to disjoint ranges could otherwise hit + // — each individually passing its own If-Match check against the + // same stale snapshot, then blindly overwriting each other. + let new_size = ingested.size; + let stored = cas_write_patch( + &state, + upload_service, + &internal_path, + chroot.drive_id, + &ingested, + session.user.id, + &file.content_hash, + "NC WEBDAV PATCH", + ) + .await?; + + // Everything from `start` to the new EOF reflects the patch (the + // untouched suffix, if any, may have shifted when the body's length + // differs from the replaced span). + let range_end = new_size.saturating_sub(1); + Ok(Response::builder() + .status(StatusCode::NO_CONTENT) + .header(header::ETAG, format!("\"{}\"", stored.etag)) + .header("oc-etag", format!("\"{}\"", stored.etag)) + .header( + header::CONTENT_RANGE, + format!("bytes {}-{}/{}", start, range_end, new_size), + ) + .body(Body::empty()) + .unwrap()) +} + // ──────────────────── MKCOL ──────────────────── async fn handle_mkcol( @@ -2172,6 +2428,23 @@ mod tests { ); } + /// Regression: `chroot.path` as returned by `folder_service.get_folder` + /// in production carries a leading `/` (from `StoragePath::to_string()` + /// — see `Folder::path_string`), unlike this module's `stub_folder` + /// test helper which builds the path directly. A real chroot must + /// still map to the leading-slash-free convention the plain WebDAV + /// surface's `db_path` uses, or exact-string comparisons against it + /// (e.g. the WebDAV lock store's key) silently mismatch. + #[test] + fn test_strips_leading_slash_from_chroot_path() { + let home = stub_folder("/Personal"); + assert_eq!( + nc_to_internal_path(&home, "report.pdf").unwrap(), + "Personal/report.pdf" + ); + assert_eq!(nc_to_internal_path(&home, "").unwrap(), "Personal"); + } + #[test] fn test_rejects_dot_dot_traversal() { let home = stub_folder("My Folder - alice"); diff --git a/src/interfaces/upload_ingest.rs b/src/interfaces/upload_ingest.rs index e440a99d..d3e55aeb 100644 --- a/src/interfaces/upload_ingest.rs +++ b/src/interfaces/upload_ingest.rs @@ -13,9 +13,10 @@ //! detection before being forwarded unchanged. use std::path::{Path, PathBuf}; +use std::pin::Pin; use std::sync::Arc; use std::sync::Mutex as StdMutex; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use axum::body::Body; use bytes::Bytes; @@ -80,6 +81,24 @@ pub async fn discard_ingested(dedup: &DedupService, blob: &IngestedBlob) { /// ingest pass (REST chunked uploads) — no post-store re-read needed. pub type ChecksumTee = Arc>>; +/// A byte-range stream paired with its known length, used by +/// [`ingest_range_patch_to_cas`] for the untouched prefix/suffix either +/// side of a PATCH edit. +pub type RangeSegment = ( + Pin> + Send>>, + u64, +); + +/// Size cap and expected-length validation for [`ingest_range_patch_to_cas`]. +pub struct PatchIngestBudget { + /// Caps the size of the *edit* (the request body), not the whole + /// spliced file — see the field's use in [`ingest_range_patch_to_cas`]. + pub max_bytes: usize, + /// Declared `X-Update-Range` span, `None` for `append`. Validated + /// against the actual streamed body length, not `Content-Length`. + pub expected_body_len: Option, +} + /// Create a checksum tee for [`ingest_stream_to_cas`]. pub fn checksum_tee(alg: ChecksumAlg) -> ChecksumTee { Arc::new(StdMutex::new(Some(IncrementalHasher::new(alg)))) @@ -249,6 +268,95 @@ pub async fn ingest_body_to_cas( ingest_stream_to_cas(source, dedup, filename, claimed_type, max_bytes, None).await } +/// Splice a PATCH request body ([RFC 5789]) between the file's untouched +/// `prefix`/`suffix` byte ranges and ingest the result as one continuous +/// stream into the CDC chunk store. +/// +/// `prefix`/`suffix` are `stream::empty()`-backed when the edit starts at +/// byte 0 or reaches EOF respectively — callers build the real ranges from +/// [`FileRetrievalUseCase::get_file_range_stream_with_perms`](crate::application::ports::file_ports::FileRetrievalUseCase::get_file_range_stream_with_perms). +/// Because FastCDC chunking is content-defined rather than offset-defined, +/// unedited chunks on either side of the edit typically dedup for free. +/// +/// Each of `prefix`/`suffix` is paired with its known byte length (from the +/// file's size and the requested range — callers compute it, not this +/// function). `budget.max_bytes` bounds the size of the *edit* (the request +/// body) — it is widened by the prefix/suffix lengths before being applied +/// to the combined stream, so that already-stored, untouched bytes being +/// re-ingested unchanged don't count against the cap. Without this, any +/// PATCH against a file at or above `max_bytes` would be rejected +/// regardless of how small the edit itself is. +/// +/// `budget.expected_body_len`, when `Some`, is validated against the +/// *actual* number of body bytes streamed — not the client-supplied +/// `Content-Length` header, which chunked-transfer-encoded requests may +/// omit entirely. Counting the real bytes means a request that declares +/// `X-Update-Range: bytes=5-9` (a 5-byte span) but streams a +/// differently-sized body is caught regardless of whether `Content-Length` +/// was present. On mismatch the already-ingested blob is discarded and +/// never reaches the atomic store, so a rejected PATCH can't leave stray +/// unreferenced content. +/// +/// [RFC 5789]: https://www.rfc-editor.org/rfc/rfc5789 +pub async fn ingest_range_patch_to_cas( + prefix: RangeSegment, + body: Body, + suffix: RangeSegment, + dedup: &Arc, + filename: &str, + claimed_type: &str, + budget: PatchIngestBudget, +) -> Result { + let PatchIngestBudget { + max_bytes, + expected_body_len, + } = budget; + let (prefix_stream, prefix_len) = prefix; + let (suffix_stream, suffix_len) = suffix; + let body_len = Arc::new(AtomicU64::new(0)); + let counter = body_len.clone(); + let body_stream = BodyStream::new(body).filter_map(move |item| { + let counter = counter.clone(); + async move { + match item { + Ok(frame) => { + let bytes = frame.into_data().ok()?; + counter.fetch_add(bytes.len() as u64, Ordering::Relaxed); + Some(Ok(bytes)) + } + Err(e) => Some(Err(std::io::Error::other(e.to_string()))), + } + } + }); + let effective_max = max_bytes.saturating_add((prefix_len + suffix_len) as usize); + let combined = prefix_stream.chain(body_stream).chain(suffix_stream); + let ingested = + ingest_stream_to_cas(combined, dedup, filename, claimed_type, effective_max, None) + .await + .map_err(|e| { + if e.error_type == "PayloadTooLarge" { + AppError::payload_too_large(format!( + "PATCH body exceeds the direct-PATCH edit-size cap ({max_bytes} bytes). \ + Use the chunked-upload protocol for edits larger than this." + )) + } else { + e + } + })?; + + if let Some(expected) = expected_body_len { + let actual = body_len.load(Ordering::Relaxed); + if actual != expected { + discard_ingested(dedup, &ingested).await; + return Err(AppError::bad_request(format!( + "PATCH body ({actual} bytes) does not match the X-Update-Range span ({expected} bytes)" + ))); + } + } + + Ok(ingested) +} + /// Adapt a multipart field into a byte stream for [`ingest_stream_to_cas`]. /// /// Terminates after the first error — multipart fields are not resumable. diff --git a/tests/api/nc_webdav_patch.hurl b/tests/api/nc_webdav_patch.hurl new file mode 100644 index 00000000..d916e61e --- /dev/null +++ b/tests/api/nc_webdav_patch.hurl @@ -0,0 +1,197 @@ +# ============================================================= +# OxiCloud — NextCloud HTTP PATCH partial content updates (RFC 5789) +# ============================================================= +# The NextCloud-compatible WebDAV surface (/remote.php/dav/…) had no +# PATCH dispatch arm at all (fell through to 405), unlike the plain-file +# surface (see api/handlers/webdav_handler.rs::handle_patch). See +# nextcloud/webdav_handler.rs::handle_patch, which reuses the plain +# surface's `parse_update_range` and `upload_ingest:: +# ingest_range_patch_to_cas` — both surface-agnostic. +# +# Coverage: +# 1. PATCH an explicit byte range (`X-Update-Range: bytes=-`) +# → 204, Content-Range header, and the resulting content reflects +# the patched span with the untouched prefix/suffix intact. +# 2. PATCH with `X-Update-Range: append` → 204, content grows. +# 3. PATCH with a Content-Range header → 400 (must use X-Update-Range). +# 4. PATCH without X-Update-Range → 400. +# 5. PATCH on a nonexistent file → 404. +# 6. PATCH on a directory → 409 (not 404 — the NC surface previously +# had no folder-existence check and returned 404 for both a missing +# file AND an existing directory; the fix commit added an explicit +# check so the two cases are distinguishable again, matching the +# plain-surface behavior). +# +# Hurl gotcha: headers MUST come before section blocks like +# `[BasicAuth]` in a request — a header line placed after `[BasicAuth]` +# is parsed as the START OF A NEW REQUEST instead (see +# `nc_multidrive_move_regression.hurl`'s note on the same gotcha). +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Setup 1 — JWT login (to mint the app password used for NC Basic Auth). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +jwt: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Setup 2 — Mint an app password for NC Basic Auth. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{jwt}} +Content-Type: application/json +{ "label": "nc_webdav_patch hurl test" } + +HTTP 200 +[Captures] +nc_username: jsonpath "$.username" +nc_password: jsonpath "$.password" +ap_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Seed a 10-byte probe file: "0123456789". +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`0123456789` + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 2 — PATCH bytes 3-5 ("345") with "XYZ". +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +X-Update-Range: bytes=3-5 +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`XYZ` + +HTTP 204 +[Asserts] +header "Content-Range" == "bytes 3-9/10" + + +GET {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +[BasicAuth] +{{nc_username}}: {{nc_password}} + +HTTP 200 +[Asserts] +body == "012XYZ6789" + + +# ───────────────────────────────────────────────────────────── +# Step 3 — PATCH append. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +X-Update-Range: append +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`END` + +HTTP 204 +[Asserts] +header "Content-Range" == "bytes 10-12/13" + + +GET {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +[BasicAuth] +{{nc_username}}: {{nc_password}} + +HTTP 200 +[Asserts] +body == "012XYZ6789END" + + +# ───────────────────────────────────────────────────────────── +# Step 4 — Content-Range header on PATCH is rejected. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +X-Update-Range: bytes=0-2 +Content-Range: bytes 0-2/13 +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`abc` + +HTTP 400 + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Missing X-Update-Range header. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`abc` + +HTTP 400 + + +# ───────────────────────────────────────────────────────────── +# Step 6 — PATCH on a nonexistent file → 404. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-does-not-exist.txt +X-Update-Range: append +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`abc` + +HTTP 404 + + +# ───────────────────────────────────────────────────────────── +# Step 7 — PATCH on a directory → 409 Conflict (not 404). +# ───────────────────────────────────────────────────────────── +MKCOL {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe-dir/ +[BasicAuth] +{{nc_username}}: {{nc_password}} + +HTTP 201 + + +PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe-dir/ +X-Update-Range: bytes=0-2 +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`NOP` + +HTTP 409 + + +DELETE {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe-dir/ +[BasicAuth] +{{nc_username}}: {{nc_password}} + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Cleanup +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +[BasicAuth] +{{nc_username}}: {{nc_password}} + +HTTP 204 + + +DELETE {{base_url}}/api/auth/app-passwords/{{ap_id}} +Authorization: Bearer {{jwt}} +HTTP 200 diff --git a/tests/api/nc_webdav_patch_consistency.hurl b/tests/api/nc_webdav_patch_consistency.hurl new file mode 100644 index 00000000..e23f1d78 --- /dev/null +++ b/tests/api/nc_webdav_patch_consistency.hurl @@ -0,0 +1,545 @@ +# ============================================================= +# OxiCloud — NextCloud PATCH data-consistency + authz/lock gaps +# ============================================================= +# `nc_webdav_patch.hurl` covers the PATCH contract on the NC surface. +# This file targets the specific gaps closed by the review-fix commit +# (see nextcloud/webdav_handler.rs::handle_patch): +# +# 1. AuthZ: the NC surface previously called `get_file_by_path` +# (which performs NO authorization check) with no follow-up +# `authz.require` at all — any caller with a valid app password +# could learn a file's size/ETag via PATCH's precondition/range +# responses regardless of their actual permission on that file. +# The fix added the same `Permission::Read` check the plain +# surface already had. That Read check is only an early +# existence-proof gate, though — the actual write a few lines +# later goes through `update_file_streaming_with_perms`, which +# independently requires `Permission::Update`. So the full +# permission chain for PATCH is: EDITOR (has Update) can PATCH; +# VIEWER (Read only, no Update) gets past the early gate but is +# still denied — anti-enum 404 — at the write step; a caller +# with NO grant at all can't even establish the composite-marker +# chroot. Tested via the multi-drive composite `{user}~{folder_id}` +# credential shape (see `nc_multidrive_move_regression.hurl` for +# the mechanism). +# 2. Cross-surface lock interop: a LOCK taken via the plain +# `/webdav/` surface now also blocks PATCH via `/remote.php/dav/` +# for the same file — proves the two surfaces share one lock +# store, not two independent ones. +# 3. Quota/507 via the NC surface (previously missing entirely — +# the fix added the same per-user quota check the plain surface +# already enforced), and the failed PATCH leaves the file intact. +# +# Self-contained: provisions its own throwaway users/drive so it can +# run alongside the rest of the suite. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Setup — Admin JWT login. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +admin_jwt: jsonpath "$.access_token" +admin_user_id: jsonpath "$.user.id" + + +# ═════════════════════════════════════════════════════════════ +# Part A — AuthZ: Editor can PATCH; Viewer (Read only) and a +# no-grant outsider both can't +# ═════════════════════════════════════════════════════════════ + + +# ───────────────────────────────────────────────────────────── +# Step A1 — Provision `ncpatch_editor` (will get EDITOR), +# `ncpatch_viewer` (will get VIEWER), and +# `ncpatch_outsider` (gets NO grant at all). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "username": "ncpatch_editor", + "password": "NcPatchEditorPwd1!", + "email": "ncpatch_editor@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +editor_user_id: jsonpath "$.id" + +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "username": "ncpatch_viewer", + "password": "NcPatchViewerPwd1!", + "email": "ncpatch_viewer@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +viewer_user_id: jsonpath "$.id" + +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "username": "ncpatch_outsider", + "password": "NcPatchOutsiderPwd1!", + "email": "ncpatch_outsider@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +outsider_user_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step A2 — Log all three in, mint an NC app password for each. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "ncpatch_editor", "password": "NcPatchEditorPwd1!" } + +HTTP 200 +[Captures] +editor_jwt: jsonpath "$.access_token" + +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{editor_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_patch_consistency (editor)" } + +HTTP 200 +[Captures] +editor_nc_username: jsonpath "$.username" +editor_nc_password: jsonpath "$.password" +editor_ap_id: jsonpath "$.id" + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "ncpatch_viewer", "password": "NcPatchViewerPwd1!" } + +HTTP 200 +[Captures] +viewer_jwt: jsonpath "$.access_token" + +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{viewer_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_patch_consistency (viewer)" } + +HTTP 200 +[Captures] +viewer_nc_username: jsonpath "$.username" +viewer_nc_password: jsonpath "$.password" +viewer_ap_id: jsonpath "$.id" + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "ncpatch_outsider", "password": "NcPatchOutsiderPwd1!" } + +HTTP 200 +[Captures] +outsider_jwt: jsonpath "$.access_token" + +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{outsider_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_patch_consistency (outsider)" } + +HTTP 200 +[Captures] +outsider_nc_username: jsonpath "$.username" +outsider_nc_password: jsonpath "$.password" +outsider_ap_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step A3 — Admin creates a shared drive, grants `ncpatch_editor` +# EDITOR (Read + Update) and `ncpatch_viewer` VIEWER +# (Read only). `ncpatch_outsider` gets no grant at all. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/drives +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "kind": "shared", + "name": "ncpatch-shared", + "owner": { "type": "user", "id": "{{admin_user_id}}" } +} + +HTTP 201 +[Captures] +shared_drive_id: jsonpath "$.id" +shared_root_id: jsonpath "$.root_folder_id" + + +POST {{base_url}}/api/grants +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{editor_user_id}}" }, + "resource": { "type": "drive", "id": "{{shared_drive_id}}" }, + "role": "editor" +} + +HTTP 201 + +POST {{base_url}}/api/grants +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{viewer_user_id}}" }, + "resource": { "type": "drive", "id": "{{shared_drive_id}}" }, + "role": "viewer" +} + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step A4 — Admin seeds a file in the shared drive via the plain +# WebDAV surface (`@drive//` scheme). +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/@drive/{{shared_drive_id}}/ncpatch-file.txt +Authorization: Bearer {{admin_jwt}} +Content-Type: text/plain +`0123456789` + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step A5 — Bootstrap the composite BasicAuth usernames (Hurl's +# [BasicAuth] parser chokes on a literal `~` split across +# two templates — alias it via [Options] variable: first, +# same workaround as nc_multidrive_move_regression.hurl). +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/ready +[Options] +variable: nc_basic_editor={{editor_nc_username}}~{{shared_root_id}} + +HTTP 200 + +GET {{base_url}}/ready +[Options] +variable: nc_basic_viewer={{viewer_nc_username}}~{{shared_root_id}} + +HTTP 200 + +GET {{base_url}}/ready +[Options] +variable: nc_basic_outsider={{outsider_nc_username}}~{{shared_root_id}} + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step A6 — EDITOR (has Update via the drive grant) CAN PATCH. +# This is the positive check: the fix's authz.require(Read) +# gate plus the write step's Update requirement must not +# accidentally lock out a legitimate Update-holder. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_basic_editor}}/ncpatch-file.txt +X-Update-Range: bytes=0-2 +Content-Type: text/plain +[BasicAuth] +{{nc_basic_editor}}: {{editor_nc_password}} +`XYZ` + +HTTP 204 + + +GET {{base_url}}/remote.php/dav/files/{{nc_basic_editor}}/ncpatch-file.txt +[BasicAuth] +{{nc_basic_editor}}: {{editor_nc_password}} + +HTTP 200 +[Asserts] +body == "XYZ3456789" + + +# ───────────────────────────────────────────────────────────── +# Step A7 — VIEWER (has Read via the grant, but not Update) is +# denied. The early authz.require(Read) the fix added is +# only an existence-proof gate; the actual write goes +# through `update_file_streaming_with_perms`, which +# independently requires Update. Since the Viewer CAN +# read the file, `require`'s graduated-denial policy +# (authorization_ports.rs::require) surfaces this as 403, +# not the anti-enum 404 — the caller can already see the +# resource, so hiding its existence leaks nothing new. +# Before fixing the NC surface's error-mapping bug found +# via this test (see nextcloud/webdav_handler.rs's PATCH +# write-step error mapping), this denial leaked as a raw +# 500 instead of the correct 403. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_basic_viewer}}/ncpatch-file.txt +X-Update-Range: bytes=0-2 +Content-Type: text/plain +[BasicAuth] +{{nc_basic_viewer}}: {{viewer_nc_password}} +`NOP` + +HTTP 403 + + +# ───────────────────────────────────────────────────────────── +# Step A8 — OUTSIDER (no grant at all on this drive) cannot reach +# the file — denied before PATCH's own logic ever runs. +# Accept the broader 4xx-non-2xx shape here since the +# denial may surface at the app-password/session boundary +# rather than the domain authz layer. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_basic_outsider}}/ncpatch-file.txt +X-Update-Range: bytes=0-2 +Content-Type: text/plain +[BasicAuth] +{{nc_basic_outsider}}: {{outsider_nc_password}} +`NOP` + +HTTP * +[Asserts] +status >= 400 +status < 500 + + +# Cleanup Part A. +DELETE {{base_url}}/webdav/@drive/{{shared_drive_id}}/ncpatch-file.txt +Authorization: Bearer {{admin_jwt}} + +HTTP 204 + +DELETE {{base_url}}/api/auth/app-passwords/{{editor_ap_id}} +Authorization: Bearer {{editor_jwt}} +HTTP 200 + +DELETE {{base_url}}/api/auth/app-passwords/{{viewer_ap_id}} +Authorization: Bearer {{viewer_jwt}} +HTTP 200 + +DELETE {{base_url}}/api/auth/app-passwords/{{outsider_ap_id}} +Authorization: Bearer {{outsider_jwt}} +HTTP 200 + + +# ═════════════════════════════════════════════════════════════ +# Part B — Cross-surface lock interop +# ═════════════════════════════════════════════════════════════ + + +# ───────────────────────────────────────────────────────────── +# Step B1 — Mint admin's own NC app password (bare-username +# surface — admin's personal drive, same file tree as +# `/webdav/`). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_patch_consistency (lock interop)" } + +HTTP 200 +[Captures] +nc_username: jsonpath "$.username" +nc_password: jsonpath "$.password" +lock_ap_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step B2 — Seed the file via the plain surface, LOCK it there. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/nc-lock-interop-probe.txt +Authorization: Bearer {{admin_jwt}} +Content-Type: text/plain +`0123456789` + +HTTP 201 + + +LOCK {{base_url}}/webdav/nc-lock-interop-probe.txt +Authorization: Bearer {{admin_jwt}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + nc-lock-interop-test + +``` + +HTTP 200 +[Captures] +interop_lock_token: xpath "string(//*[local-name()='locktoken']/*[local-name()='href'])" + + +# ───────────────────────────────────────────────────────────── +# Step B3 — PATCH the SAME file via the NC surface, no lock token +# → 423. Pre-fix, the NC surface didn't consult the +# plain surface's lock store at all. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-lock-interop-probe.txt +X-Update-Range: bytes=0-2 +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`NOP` + +HTTP 423 + + +# Release the lock via the plain surface so cleanup below works. +UNLOCK {{base_url}}/webdav/nc-lock-interop-probe.txt +Authorization: Bearer {{admin_jwt}} +Lock-Token: <{{interop_lock_token}}> + +HTTP 204 + + +# Cleanup Part B. +DELETE {{base_url}}/webdav/nc-lock-interop-probe.txt +Authorization: Bearer {{admin_jwt}} + +HTTP 204 + + +# ═════════════════════════════════════════════════════════════ +# Part C — Quota/507 via the NC surface leaves the file untouched +# ═════════════════════════════════════════════════════════════ + + +# ───────────────────────────────────────────────────────────── +# Step C1 — Provision `ncpatch_quota_owner` with a 50-byte quota. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "username": "ncpatch_quota_owner", + "password": "NcPatchQuotaOwnerPwd1!", + "email": "ncpatch_quota_owner@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +quota_owner_id: jsonpath "$.id" + + +PUT {{base_url}}/api/admin/users/{{quota_owner_id}}/quota +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ "quota_bytes": 50 } + +HTTP 200 + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "ncpatch_quota_owner", "password": "NcPatchQuotaOwnerPwd1!" } + +HTTP 200 +[Captures] +quota_owner_jwt: jsonpath "$.access_token" + +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{quota_owner_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_patch_consistency (quota)" } + +HTTP 200 +[Captures] +quota_nc_username: jsonpath "$.username" +quota_nc_password: jsonpath "$.password" +quota_ap_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step C2 — Seed a 10-byte file (under quota), then append past +# it → 507. File must come back unchanged. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/remote.php/dav/files/{{quota_nc_username}}/nc-quota-probe.txt +Content-Type: text/plain +[BasicAuth] +{{quota_nc_username}}: {{quota_nc_password}} +`0123456789` + +HTTP 201 +[Captures] +quota_probe_etag: header "ETag" + + +PATCH {{base_url}}/remote.php/dav/files/{{quota_nc_username}}/nc-quota-probe.txt +X-Update-Range: append +Content-Type: text/plain +[BasicAuth] +{{quota_nc_username}}: {{quota_nc_password}} +`this-is-a-100-byte-ish-payload-that-blows-past-the-fifty-byte-quota-set-for-this-throwaway-user-abc` + +HTTP 507 + + +GET {{base_url}}/remote.php/dav/files/{{quota_nc_username}}/nc-quota-probe.txt +[BasicAuth] +{{quota_nc_username}}: {{quota_nc_password}} + +HTTP 200 +[Asserts] +body == "0123456789" +header "ETag" contains {{quota_probe_etag}} + + +# Cleanup Part C. +DELETE {{base_url}}/remote.php/dav/files/{{quota_nc_username}}/nc-quota-probe.txt +[BasicAuth] +{{quota_nc_username}}: {{quota_nc_password}} + +HTTP 204 + +DELETE {{base_url}}/api/auth/app-passwords/{{quota_ap_id}} +Authorization: Bearer {{quota_owner_jwt}} +HTTP 200 + +DELETE {{base_url}}/api/auth/app-passwords/{{lock_ap_id}} +Authorization: Bearer {{admin_jwt}} +HTTP 200 + + +# ═════════════════════════════════════════════════════════════ +# Teardown +# ═════════════════════════════════════════════════════════════ +DELETE {{base_url}}/api/admin/users/{{editor_user_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 200 + +DELETE {{base_url}}/api/admin/users/{{viewer_user_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 200 + +DELETE {{base_url}}/api/drives/{{shared_drive_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 204 + +DELETE {{base_url}}/api/admin/users/{{outsider_user_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 200 + +DELETE {{base_url}}/api/admin/users/{{quota_owner_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 200 diff --git a/tests/api/nc_webdav_put_gaps.hurl b/tests/api/nc_webdav_put_gaps.hurl new file mode 100644 index 00000000..d6d0657c --- /dev/null +++ b/tests/api/nc_webdav_put_gaps.hurl @@ -0,0 +1,505 @@ +# ============================================================= +# OxiCloud — NextCloud PUT gaps closed by bringing handle_put up to +# parity with handle_patch +# ============================================================= +# `nc_webdav_patch_consistency.hurl` covers the same four gap classes +# for PATCH; this file targets the NC surface's `handle_put` +# (nextcloud/webdav_handler.rs), which had fallen behind PATCH's +# hardening across the RFC 5789 commits: +# +# 1. Error mapping: the write step mapped every `DomainError` to a +# raw 500 (`AppError::internal_error(format!("Failed to store +# file: {}", e))`) instead of `AppError::from(e)` — a VIEWER +# (Read only, no Update) overwriting a file got a 500 leak +# instead of the graduated-denial 403 the rest of the codebase +# relies on (Read granted → visible → 403; no Read at all → +# hidden → 404 anti-enum). +# 2. Cross-surface lock interop: PUT via `/remote.php/dav/` didn't +# consult the lock store a LOCK taken via the plain `/webdav/` +# surface writes to at all. +# 3. Quota/507: PUT via the NC surface bypassed +# `check_storage_quota` entirely (PATCH already enforced it). +# 4. Existence-check depth (RFC 4918 §9.7.1): PUT to an existing +# directory should be 400, and PUT under a missing parent folder +# should be 409 — neither check existed on the NC surface; both +# failure modes fell through to whatever `update_file_streaming_ +# with_perms` did internally. +# +# Self-contained: provisions its own throwaway users/drive so it can +# run alongside the rest of the suite. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Setup — Admin JWT login. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +admin_jwt: jsonpath "$.access_token" +admin_user_id: jsonpath "$.user.id" + + +# ═════════════════════════════════════════════════════════════ +# Part A — Error mapping: Editor can overwrite via PUT; Viewer +# (Read only) gets 404, not a raw 500 +# ═════════════════════════════════════════════════════════════ + + +# ───────────────────────────────────────────────────────────── +# Step A1 — Provision `ncput_editor` (EDITOR) and `ncput_viewer` +# (VIEWER, Read only). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "username": "ncput_editor", + "password": "NcPutEditorPwd1!", + "email": "ncput_editor@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +editor_user_id: jsonpath "$.id" + +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "username": "ncput_viewer", + "password": "NcPutViewerPwd1!", + "email": "ncput_viewer@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +viewer_user_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step A2 — Log both in, mint an NC app password for each. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "ncput_editor", "password": "NcPutEditorPwd1!" } + +HTTP 200 +[Captures] +editor_jwt: jsonpath "$.access_token" + +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{editor_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_put_gaps (editor)" } + +HTTP 200 +[Captures] +editor_nc_username: jsonpath "$.username" +editor_nc_password: jsonpath "$.password" +editor_ap_id: jsonpath "$.id" + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "ncput_viewer", "password": "NcPutViewerPwd1!" } + +HTTP 200 +[Captures] +viewer_jwt: jsonpath "$.access_token" + +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{viewer_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_put_gaps (viewer)" } + +HTTP 200 +[Captures] +viewer_nc_username: jsonpath "$.username" +viewer_nc_password: jsonpath "$.password" +viewer_ap_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step A3 — Admin creates a shared drive, grants `ncput_editor` +# EDITOR (Read + Update) and `ncput_viewer` VIEWER +# (Read only). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/drives +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "kind": "shared", + "name": "ncput-shared", + "owner": { "type": "user", "id": "{{admin_user_id}}" } +} + +HTTP 201 +[Captures] +shared_drive_id: jsonpath "$.id" +shared_root_id: jsonpath "$.root_folder_id" + + +POST {{base_url}}/api/grants +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{editor_user_id}}" }, + "resource": { "type": "drive", "id": "{{shared_drive_id}}" }, + "role": "editor" +} + +HTTP 201 + +POST {{base_url}}/api/grants +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{viewer_user_id}}" }, + "resource": { "type": "drive", "id": "{{shared_drive_id}}" }, + "role": "viewer" +} + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step A4 — Admin seeds a file in the shared drive via the plain +# WebDAV surface (`@drive//` scheme). +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/@drive/{{shared_drive_id}}/ncput-file.txt +Authorization: Bearer {{admin_jwt}} +Content-Type: text/plain +`0123456789` + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step A5 — Bootstrap the composite BasicAuth usernames (see +# nc_multidrive_move_regression.hurl for the mechanism). +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/ready +[Options] +variable: nc_basic_editor={{editor_nc_username}}~{{shared_root_id}} + +HTTP 200 + +GET {{base_url}}/ready +[Options] +variable: nc_basic_viewer={{viewer_nc_username}}~{{shared_root_id}} + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step A6 — EDITOR (has Update via the drive grant) CAN overwrite +# via PUT. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/remote.php/dav/files/{{nc_basic_editor}}/ncput-file.txt +Content-Type: text/plain +[BasicAuth] +{{nc_basic_editor}}: {{editor_nc_password}} +`XYZ` + +HTTP 204 + +GET {{base_url}}/remote.php/dav/files/{{nc_basic_editor}}/ncput-file.txt +[BasicAuth] +{{nc_basic_editor}}: {{editor_nc_password}} + +HTTP 200 +[Asserts] +body == "XYZ" + + +# ───────────────────────────────────────────────────────────── +# Step A7 — VIEWER (has Read via the grant, but not Update) is +# denied, not a raw 500. Viewer CAN read the file, so +# the graduated-denial policy (authorization_ports.rs:: +# require) surfaces 403, not the anti-enum 404 — that +# shape is reserved for callers with no Read at all. +# Before the fix, `handle_put`'s write step mapped every +# `DomainError` (including this authz denial) to +# `AppError::internal_error(...)`, leaking a 500. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/remote.php/dav/files/{{nc_basic_viewer}}/ncput-file.txt +Content-Type: text/plain +[BasicAuth] +{{nc_basic_viewer}}: {{viewer_nc_password}} +`NOP` + +HTTP 403 + + +# Cleanup Part A. +DELETE {{base_url}}/webdav/@drive/{{shared_drive_id}}/ncput-file.txt +Authorization: Bearer {{admin_jwt}} + +HTTP 204 + +DELETE {{base_url}}/api/auth/app-passwords/{{editor_ap_id}} +Authorization: Bearer {{editor_jwt}} +HTTP 200 + +DELETE {{base_url}}/api/auth/app-passwords/{{viewer_ap_id}} +Authorization: Bearer {{viewer_jwt}} +HTTP 200 + + +# ═════════════════════════════════════════════════════════════ +# Part B — Cross-surface lock interop +# ═════════════════════════════════════════════════════════════ + + +# ───────────────────────────────────────────────────────────── +# Step B1 — Mint admin's own NC app password (bare-username +# surface — admin's personal drive, same file tree as +# `/webdav/`). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_put_gaps (lock interop)" } + +HTTP 200 +[Captures] +nc_username: jsonpath "$.username" +nc_password: jsonpath "$.password" +lock_ap_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step B2 — Seed the file via the plain surface, LOCK it there. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/nc-put-lock-interop-probe.txt +Authorization: Bearer {{admin_jwt}} +Content-Type: text/plain +`0123456789` + +HTTP 201 + + +LOCK {{base_url}}/webdav/nc-put-lock-interop-probe.txt +Authorization: Bearer {{admin_jwt}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + nc-put-lock-interop-test + +``` + +HTTP 200 +[Captures] +interop_lock_token: xpath "string(//*[local-name()='locktoken']/*[local-name()='href'])" + + +# ───────────────────────────────────────────────────────────── +# Step B3 — PUT the SAME file via the NC surface, no lock token +# → 423. Pre-fix, the NC surface's `handle_put` didn't +# consult the plain surface's lock store at all. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-put-lock-interop-probe.txt +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`NOP` + +HTTP 423 + + +# Release the lock via the plain surface so cleanup below works. +UNLOCK {{base_url}}/webdav/nc-put-lock-interop-probe.txt +Authorization: Bearer {{admin_jwt}} +Lock-Token: <{{interop_lock_token}}> + +HTTP 204 + + +# Cleanup Part B. +DELETE {{base_url}}/webdav/nc-put-lock-interop-probe.txt +Authorization: Bearer {{admin_jwt}} + +HTTP 204 + + +# ═════════════════════════════════════════════════════════════ +# Part C — Quota/507 via the NC surface leaves the file untouched +# ═════════════════════════════════════════════════════════════ + + +# ───────────────────────────────────────────────────────────── +# Step C1 — Provision `ncput_quota_owner` with a 50-byte quota. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ + "username": "ncput_quota_owner", + "password": "NcPutQuotaOwnerPwd1!", + "email": "ncput_quota_owner@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +quota_owner_id: jsonpath "$.id" + + +PUT {{base_url}}/api/admin/users/{{quota_owner_id}}/quota +Authorization: Bearer {{admin_jwt}} +Content-Type: application/json +{ "quota_bytes": 50 } + +HTTP 200 + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "ncput_quota_owner", "password": "NcPutQuotaOwnerPwd1!" } + +HTTP 200 +[Captures] +quota_owner_jwt: jsonpath "$.access_token" + +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{quota_owner_jwt}} +Content-Type: application/json +{ "label": "nc_webdav_put_gaps (quota)" } + +HTTP 200 +[Captures] +quota_nc_username: jsonpath "$.username" +quota_nc_password: jsonpath "$.password" +quota_ap_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step C2 — Seed a 10-byte file (under quota), then overwrite it +# with a payload that blows past the 50-byte quota → 507. +# File must come back unchanged. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/remote.php/dav/files/{{quota_nc_username}}/nc-put-quota-probe.txt +Content-Type: text/plain +[BasicAuth] +{{quota_nc_username}}: {{quota_nc_password}} +`0123456789` + +HTTP 201 +[Captures] +quota_probe_etag: header "ETag" + + +PUT {{base_url}}/remote.php/dav/files/{{quota_nc_username}}/nc-put-quota-probe.txt +Content-Type: text/plain +[BasicAuth] +{{quota_nc_username}}: {{quota_nc_password}} +`this-is-a-100-byte-ish-payload-that-blows-past-the-fifty-byte-quota-set-for-this-throwaway-user-abc` + +HTTP 507 + + +GET {{base_url}}/remote.php/dav/files/{{quota_nc_username}}/nc-put-quota-probe.txt +[BasicAuth] +{{quota_nc_username}}: {{quota_nc_password}} + +HTTP 200 +[Asserts] +body == "0123456789" +header "ETag" contains {{quota_probe_etag}} + + +# Cleanup Part C. +DELETE {{base_url}}/remote.php/dav/files/{{quota_nc_username}}/nc-put-quota-probe.txt +[BasicAuth] +{{quota_nc_username}}: {{quota_nc_password}} + +HTTP 204 + +DELETE {{base_url}}/api/auth/app-passwords/{{quota_ap_id}} +Authorization: Bearer {{quota_owner_jwt}} +HTTP 200 + + +# ═════════════════════════════════════════════════════════════ +# Part D — Existence-check depth (RFC 4918 §9.7.1): folder-collision +# and missing-parent, previously unchecked on the NC surface +# ═════════════════════════════════════════════════════════════ + + +# ───────────────────────────────────────────────────────────── +# Step D1 — PUT to an existing directory → 400 (not whatever the +# write step's internals happened to produce). +# ───────────────────────────────────────────────────────────── +MKCOL {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-put-probe-dir/ +[BasicAuth] +{{nc_username}}: {{nc_password}} + +HTTP 201 + + +PUT {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-put-probe-dir/ +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`NOP` + +HTTP 400 + + +DELETE {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-put-probe-dir/ +[BasicAuth] +{{nc_username}}: {{nc_password}} + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step D2 — PUT under a nonexistent parent folder → 409 Conflict +# (RFC 4918 §9.7.1), not a generic error from further down +# the write path. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-put-missing-parent/probe.txt +Content-Type: text/plain +[BasicAuth] +{{nc_username}}: {{nc_password}} +`NOP` + +HTTP 409 + + +DELETE {{base_url}}/api/auth/app-passwords/{{lock_ap_id}} +Authorization: Bearer {{admin_jwt}} +HTTP 200 + + +# ═════════════════════════════════════════════════════════════ +# Teardown +# ═════════════════════════════════════════════════════════════ +DELETE {{base_url}}/api/admin/users/{{editor_user_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 200 + +DELETE {{base_url}}/api/admin/users/{{viewer_user_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 200 + +DELETE {{base_url}}/api/drives/{{shared_drive_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 204 + +DELETE {{base_url}}/api/admin/users/{{quota_owner_id}} +Authorization: Bearer {{admin_jwt}} + +HTTP 200 diff --git a/tests/api/run.sh b/tests/api/run.sh index 862758be..4a41ef0b 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -205,6 +205,11 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/webdav_protected_properties.hurl" \ "$API_DIR/webdav_quota_properties.hurl" \ "$API_DIR/nc_webdav_quota_properties.hurl" \ + "$API_DIR/webdav_patch.hurl" \ + "$API_DIR/nc_webdav_patch.hurl" \ + "$API_DIR/webdav_patch_consistency.hurl" \ + "$API_DIR/nc_webdav_patch_consistency.hurl" \ + "$API_DIR/nc_webdav_put_gaps.hurl" \ "$API_DIR/webdav_drive_root.hurl" \ "$API_DIR/webdav_permissions.hurl" \ "$API_DIR/webdav_nested_move_cascade.hurl" \ diff --git a/tests/api/webdav_patch.hurl b/tests/api/webdav_patch.hurl new file mode 100644 index 00000000..a1c72dd2 --- /dev/null +++ b/tests/api/webdav_patch.hurl @@ -0,0 +1,253 @@ +# ============================================================= +# OxiCloud — WebDAV PATCH (RFC 5789) partial content update +# ============================================================= +# RFC 4918 §9.7.1 forbids partial updates on PUT (a `Content-Range` +# on PUT is rejected, see webdav_handler.rs::handle_put). PATCH is +# the mechanism this server offers instead, via a dedicated +# `X-Update-Range` header: `bytes=-` (inclusive) or +# `append`. See webdav_handler.rs::handle_patch / +# parse_update_range for the implementation. +# +# Coverage: +# 1. Mid-file byte-range overwrite → 204, GET reflects the splice. +# 2. Append → 204, GET reflects the appended tail. +# 3. Out-of-range span (end >= size) → 416. +# 4. If-Match precondition failure → 412. +# 5. Locked resource without a lock token → 423. +# 6. PATCH on a directory → 409. +# 7. PATCH on a missing resource → 404. +# 8. PATCH without X-Update-Range → 400. +# 9. If-None-Match precondition failure (tag matches current ETag) → 412. +# 10. If-Match with a WEAK (`W/`) form of the current ETag → 412 (RFC 7232 +# §3.1: If-Match requires a STRONG match; a weak validator in the +# request never satisfies it, even if the underlying tag value is +# identical — see `if_match_precondition_fails`). +# +# Hurl gotcha: a triple-backtick ``` multiline body appends a trailing +# `\n` the server counts as part of Content-Length — that silently +# breaks the exact `end - start + 1` span check on a byte-range PATCH. +# Plain-text bodies below use the single-backtick ONELINE string form +# (`` `text` ``) instead, which sends exactly the bytes between the +# backticks with no injected newline. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Login, capture JWT +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 2 — PUT a 10-byte probe file: "0123456789" +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +`0123456789` + +HTTP 201 +[Captures] +probe_etag: header "ETag" + + +# ───────────────────────────────────────────────────────────── +# Step 3 — Mid-file overwrite: replace bytes 3-5 (inclusive, +# 0-based) with "XYZ". +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=3-5 +Content-Type: text/plain +`XYZ` + +HTTP 204 +[Asserts] +header "Content-Range" matches "^bytes 3-\\d+/\\d+$" + + +GET {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +body startsWith "012XYZ" + + +# ───────────────────────────────────────────────────────────── +# Step 4 — Append to the end of the file. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +X-Update-Range: append +Content-Type: text/plain +`-APPENDED` + +HTTP 204 +[Captures] +current_etag: header "ETag" + + +GET {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +body endsWith "-APPENDED" + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Out-of-range span: `end` must be strictly within the +# current file size (growing via a byte-range PATCH +# isn't supported — use `append` for that). +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=1000-1005 +Content-Type: text/plain +`oops` + +HTTP 416 + + +# ───────────────────────────────────────────────────────────── +# Step 6 — If-Match precondition failure. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=0-2 +If-Match: "not-the-real-etag" +Content-Type: text/plain +`NOP` + +HTTP 412 + + +# ───────────────────────────────────────────────────────────── +# Step 7 — Locked resource without a matching lock token. +# ───────────────────────────────────────────────────────────── +LOCK {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + patch-test + +``` + +HTTP 200 +[Captures] +lock_token: xpath "string(//*[local-name()='locktoken']/*[local-name()='href'])" + + +PATCH {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=0-2 +Content-Type: text/plain +`NOP` + +HTTP 423 + + +# Release the lock so cleanup below can proceed. +UNLOCK {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +Lock-Token: <{{lock_token}}> + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 8 — PATCH on a directory → 409 Conflict. +# ───────────────────────────────────────────────────────────── +MKCOL {{base_url}}/webdav/patch-probe-dir/ +Authorization: Bearer {{token}} + +HTTP 201 + + +PATCH {{base_url}}/webdav/patch-probe-dir/ +Authorization: Bearer {{token}} +X-Update-Range: bytes=0-2 +Content-Type: text/plain +`NOP` + +HTTP 409 + + +# ───────────────────────────────────────────────────────────── +# Step 9 — PATCH on a missing resource → 404. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-probe-does-not-exist.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=0-2 +Content-Type: text/plain +`NOP` + +HTTP 404 + + +# ───────────────────────────────────────────────────────────── +# Step 10 — PATCH without X-Update-Range → 400. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +`NOP` + +HTTP 400 + + +# ───────────────────────────────────────────────────────────── +# Step 11 — If-None-Match precondition failure: the header names the +# CURRENT ETag, so the "only if it does NOT match" condition +# is violated → 412. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=0-2 +If-None-Match: {{current_etag}} +Content-Type: text/plain +`NOP` + +HTTP 412 + + +# ───────────────────────────────────────────────────────────── +# Step 12 — If-Match with a WEAK form (`W/`) of the current ETag → 412. +# RFC 7232 §3.1 requires If-Match to STRONG-match; a request +# carrying a weak validator never satisfies it even when the +# underlying tag value is identical. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=0-2 +If-Match: W/{{current_etag}} +Content-Type: text/plain +`NOP` + +HTTP 412 + + +# ───────────────────────────────────────────────────────────── +# Cleanup +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/webdav/patch-probe.txt +Authorization: Bearer {{token}} + +HTTP 204 + + +DELETE {{base_url}}/webdav/patch-probe-dir/ +Authorization: Bearer {{token}} + +HTTP 204 diff --git a/tests/api/webdav_patch_consistency.hurl b/tests/api/webdav_patch_consistency.hurl new file mode 100644 index 00000000..e3842c87 --- /dev/null +++ b/tests/api/webdav_patch_consistency.hurl @@ -0,0 +1,322 @@ +# ============================================================= +# OxiCloud — WebDAV PATCH data-consistency chain (RFC 5789) +# ============================================================= +# `webdav_patch.hurl` covers the PATCH contract itself (ranges, append, +# preconditions, locks). This file chains multiple PATCHes against the +# SAME resource and asserts the server stays consistent afterward — +# the concern behind the review-fix commit that added quota +# enforcement, an ETag re-check, and a `direct_put_max_bytes` +# prefix/suffix accounting bug (see webdav_handler.rs::handle_patch). +# +# Coverage: +# 1. Sequential overlapping-range PATCHes on one file: each step's +# GET reflects the splice, and the ETag changes every time (no +# stale-tag reuse across writes). +# 2. Cross-protocol consistency: HEAD and PROPFIND report the same +# size/ETag as the GET right after the last PATCH. +# 3. Quota rejection (507) leaves the file BYTE-FOR-BYTE unchanged — +# the ingested blob is discarded before it's ever attached +# (`upload_ingest::discard_ingested`). +# 4. `direct_put_max_bytes` bounds only the EDIT span, not the whole +# file: a small edit on a file already bigger than the cap still +# succeeds, but an edit whose OWN body exceeds the cap still 413s. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Login, capture JWT +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +token: jsonpath "$.access_token" + + +# ═════════════════════════════════════════════════════════════ +# Part A — Sequential overlapping PATCHes + cross-protocol check +# ═════════════════════════════════════════════════════════════ + + +# ───────────────────────────────────────────────────────────── +# Step 2 — PUT a 20-byte probe: "0123456789ABCDEFGHIJ" +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/patch-consist-chain.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +`0123456789ABCDEFGHIJ` + +HTTP 201 +[Captures] +etag0: header "ETag" + + +# ───────────────────────────────────────────────────────────── +# Step 3 — Overwrite bytes 5-9 ("56789") with "XXXXX". +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-consist-chain.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=5-9 +Content-Type: text/plain +`XXXXX` + +HTTP 204 +[Captures] +etag1: header "ETag" +[Asserts] +header "ETag" != {{etag0}} + + +GET {{base_url}}/webdav/patch-consist-chain.txt +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +body == "01234XXXXXABCDEFGHIJ" + + +# ───────────────────────────────────────────────────────────── +# Step 4 — Overwrite bytes 10-14 ("ABCDE") with "YYYYY". +# Overlaps neither previous edit but chains off it — +# proves each PATCH sees the result of the last one, not +# a stale copy. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-consist-chain.txt +Authorization: Bearer {{token}} +X-Update-Range: bytes=10-14 +Content-Type: text/plain +`YYYYY` + +HTTP 204 +[Captures] +etag2: header "ETag" +[Asserts] +header "ETag" != {{etag1}} + + +GET {{base_url}}/webdav/patch-consist-chain.txt +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +body == "01234XXXXXYYYYYFGHIJ" + + +# ───────────────────────────────────────────────────────────── +# Step 5 — HEAD reports the same size/ETag as the last GET. +# ───────────────────────────────────────────────────────────── +HEAD {{base_url}}/webdav/patch-consist-chain.txt +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +header "Content-Length" == "20" +# GET/HEAD/PROPFIND quote the ETag (`""`) while PUT/PATCH return +# it raw/unquoted (compare webdav_handler.rs's `handle_head` vs +# `handle_patch` response builders) — `contains` tolerates that +# formatting difference instead of asserting byte-for-byte equality. +header "ETag" contains {{etag2}} + + +# ───────────────────────────────────────────────────────────── +# Step 6 — PROPFIND (named getcontentlength/getetag) agrees with +# HEAD/GET — no drift between the WebDAV property layer +# and the plain-file read path. +# ───────────────────────────────────────────────────────────── +PROPFIND {{base_url}}/webdav/patch-consist-chain.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + + + + +``` + +HTTP 207 +[Asserts] +xpath "number(//*[local-name()='getcontentlength'])" == 20 +xpath "string(//*[local-name()='getetag'])" contains {{etag2}} + + +# Cleanup Part A. +DELETE {{base_url}}/webdav/patch-consist-chain.txt +Authorization: Bearer {{token}} + +HTTP 204 + + +# ═════════════════════════════════════════════════════════════ +# Part B — Quota rejection leaves the file untouched +# ═════════════════════════════════════════════════════════════ +# Dedicated low-quota user so this doesn't cap the shared admin +# account used by the rest of the suite. + + +# ───────────────────────────────────────────────────────────── +# Step 7 — Provision `patch_quota_owner` with a 50-byte quota. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{token}} +Content-Type: application/json +{ + "username": "patch_quota_owner", + "password": "PatchQuotaOwnerPwd1!", + "email": "patch_quota_owner@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +quota_owner_id: jsonpath "$.id" + + +PUT {{base_url}}/api/admin/users/{{quota_owner_id}}/quota +Authorization: Bearer {{token}} +Content-Type: application/json +{ "quota_bytes": 50 } + +HTTP 200 + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "patch_quota_owner", "password": "PatchQuotaOwnerPwd1!" } + +HTTP 200 +[Captures] +quota_owner_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 8 — Seed a 10-byte file (well under the 50-byte quota). +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/patch-quota-probe.txt +Authorization: Bearer {{quota_owner_token}} +Content-Type: text/plain +`0123456789` + +HTTP 201 +[Captures] +quota_probe_etag: header "ETag" + + +# ───────────────────────────────────────────────────────────── +# Step 9 — Append enough bytes to push the file's new total size +# (110 bytes) well past the 50-byte quota → 507. The +# ingested blob is discarded before commit — the file +# must come back completely unchanged. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/patch-quota-probe.txt +Authorization: Bearer {{quota_owner_token}} +X-Update-Range: append +Content-Type: text/plain +`this-is-a-100-byte-ish-payload-that-blows-past-the-fifty-byte-quota-set-for-this-throwaway-user-abc` + +HTTP 507 + + +GET {{base_url}}/webdav/patch-quota-probe.txt +Authorization: Bearer {{quota_owner_token}} + +HTTP 200 +[Asserts] +body == "0123456789" +header "ETag" contains {{quota_probe_etag}} + + +# Cleanup Part B. +DELETE {{base_url}}/webdav/patch-quota-probe.txt +Authorization: Bearer {{quota_owner_token}} + +HTTP 204 + +DELETE {{base_url}}/api/admin/users/{{quota_owner_id}} +Authorization: Bearer {{token}} + +HTTP 200 + + +# ═════════════════════════════════════════════════════════════ +# Part C — direct_put_max_bytes bounds the EDIT, not the whole file +# ═════════════════════════════════════════════════════════════ +# `OXICLOUD_DIRECT_PUT_MAX_BYTES` (4 MiB) can't be exceeded by a +# direct PUT, so a file bigger than the cap must be seeded through +# the chunk-agnostic multipart upload endpoint instead. Reuses the +# 5 MiB all-zero fixture `run.sh` already generates for the chunk/ +# direct-PUT cap tests. + + +# ───────────────────────────────────────────────────────────── +# Step 10 — Resolve the home folder id, seed a 5 MiB file in it. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/folders +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +home_folder_id: jsonpath "$[0].id" + + +POST {{base_url}}/api/files/upload +Authorization: Bearer {{token}} +[MultipartFormData] +folder_id: {{home_folder_id}} +file: file,fixtures/chunk-over-cap-5mb.bin; application/octet-stream + +HTTP 201 +[Captures] +big_file_name: jsonpath "$.name" + + +# ───────────────────────────────────────────────────────────── +# Step 11 — A SMALL mid-file edit succeeds even though the file's +# total size (5 MiB) is already over the 4 MiB cap. +# Pre-fix, the cap comparison counted prefix+suffix+edit +# against the raw cap and would have wrongly 413'd any +# edit on a file this size; post-fix only the edit span +# itself is bounded. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/{{big_file_name}} +Authorization: Bearer {{token}} +X-Update-Range: bytes=100-104 +Content-Type: application/octet-stream +`PATCH` + +HTTP 204 + + +GET {{base_url}}/webdav/{{big_file_name}} +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +body contains "PATCH" + + +# ───────────────────────────────────────────────────────────── +# Step 12 — An edit whose OWN body meets/exceeds the cap still +# 413s — the cap still bites real over-cap edits, this +# isn't a blanket bypass. Replaces the ENTIRE file (no +# prefix/suffix at all) with a 5 MiB body. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/webdav/{{big_file_name}} +Authorization: Bearer {{token}} +X-Update-Range: bytes=0-5242879 +Content-Type: application/octet-stream +file,fixtures/chunk-over-cap-5mb.bin; + +HTTP 413 + + +# Cleanup Part C. +DELETE {{base_url}}/webdav/{{big_file_name}} +Authorization: Bearer {{token}} + +HTTP 204