From d3546305f6066147b09077e249d7c16b8f5c5beb Mon Sep 17 00:00:00 2001 From: "M.Schmidt" Date: Sat, 11 Jul 2026 18:32:09 +0200 Subject: [PATCH 1/9] feat(webdav): implement HTTP PATCH for partial content updates (RFC 5789) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 4918 §9.7.1 forbids partial updates on PUT; this adds PATCH as the supported mechanism instead, via an X-Update-Range header (bytes=- or append). Reuses the existing CAS/dedup pipeline by splicing the request body between the file's untouched prefix/suffix byte ranges and re-ingesting as one continuous stream, so unedited chunks dedup for free. --- src/interfaces/api/handlers/webdav_handler.rs | 325 +++++++++++++++++- src/interfaces/upload_ingest.rs | 31 ++ tests/api/webdav_patch.hurl | 226 ++++++++++++ 3 files changed, 581 insertions(+), 1 deletion(-) create mode 100644 tests/api/webdav_patch.hurl diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 550ba15c..4b8d546e 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::{ @@ -408,6 +410,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 +442,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()) @@ -1996,6 +1999,285 @@ 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`. +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))) +} + +/** + * 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 ──────────────────────────── + if let Some(ref inm) = if_none_match { + let server_tag = file.etag.trim_matches('"'); + if inm == "*" || inm.trim_matches('"') == server_tag { + return Err(AppError::precondition_failed( + "If-None-Match — resource already exists with that ETag", + )); + } + } + if let Some(ref im) = if_match + && im != "*" + { + let client_tag = im.trim_matches('"'); + let server_tag = file.etag.trim_matches('"'); + if client_tag != server_tag { + 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_stream: Pin> + Send>> = + if start == 0 { + Box::pin(stream::empty()) + } else { + Box::into_pin( + file_retrieval_service + .get_file_range_stream_with_perms(&file.id, user.id, 0, Some(start)) + .await + .map_err(AppError::from)?, + ) + }; + let suffix_stream: Pin> + Send>> = match end + { + Some(end) if end + 1 < file.size => Box::into_pin( + file_retrieval_service + .get_file_range_stream_with_perms(&file.id, user.id, end + 1, None) + .await + .map_err(AppError::from)?, + ), + _ => Box::pin(stream::empty()), + }; + + let filename = crate::common::mime_detect::filename_from_path(&path).to_string(); + let ingested = upload_ingest::ingest_range_patch_to_cas( + prefix_stream, + req.into_body(), + suffix_stream, + &state.core.dedup_service, + &filename, + &content_type, + max_upload, + ) + .await?; + + // ── Quota enforcement ───────────────────────────────────────────── + if let Some(storage_svc) = state.storage_usage_service.as_ref() + && let Err(err) = storage_svc + .check_storage_quota(user.id, ingested.size) + .await + { + upload_ingest::discard_ingested(&state.core.dedup_service, &ingested).await; + tracing::warn!( + "⛔ WEBDAV PATCH REJECTED (quota): user={}, file={}, size={}", + user.id, + path, + ingested.size + ); + return Err(AppError::new( + StatusCode::INSUFFICIENT_STORAGE, + err.message, + "QuotaExceeded", + )); + } + + // ── Atomic store ────────────────────────────────────────────────── + let new_size = ingested.size; + let content_type = ingested.content_type.clone(); + let result = file_upload_service + .update_file_streaming_with_perms( + &path, + drive_id, + ingested.stored(), + &content_type, + None, + user.id, + ) + .await; + + match result { + Ok(file_dto) => { + // 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, &file_dto.etag) + .header( + header::CONTENT_RANGE, + format!("bytes {}-{}/{}", start, range_end, new_size), + ) + .body(Body::empty()) + .unwrap()) + } + Err(e) => Err(AppError::from(e)), + } +} + /** * Handles MKCOL requests to create folders. * @@ -3383,4 +3665,45 @@ 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()); + } } diff --git a/src/interfaces/upload_ingest.rs b/src/interfaces/upload_ingest.rs index e440a99d..edf79933 100644 --- a/src/interfaces/upload_ingest.rs +++ b/src/interfaces/upload_ingest.rs @@ -13,6 +13,7 @@ //! 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}; @@ -249,6 +250,36 @@ 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. +/// +/// [RFC 5789]: https://www.rfc-editor.org/rfc/rfc5789 +pub async fn ingest_range_patch_to_cas( + prefix: Pin> + Send>>, + body: Body, + suffix: Pin> + Send>>, + dedup: &Arc, + filename: &str, + claimed_type: &str, + max_bytes: usize, +) -> Result { + let body_stream = BodyStream::new(body).filter_map(|item| async move { + match item { + Ok(frame) => frame.into_data().ok().map(Ok), + Err(e) => Some(Err(std::io::Error::other(e.to_string()))), + } + }); + let combined = prefix.chain(body_stream).chain(suffix); + ingest_stream_to_cas(combined, dedup, filename, claimed_type, max_bytes, None).await +} + /// 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/webdav_patch.hurl b/tests/api/webdav_patch.hurl new file mode 100644 index 00000000..c61fd49d --- /dev/null +++ b/tests/api/webdav_patch.hurl @@ -0,0 +1,226 @@ +# ============================================================= +# 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. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# 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 + + +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 + + +# ───────────────────────────────────────────────────────────── +# 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 From 93ae7ab14223ccb81f6193d8b3a48a9565a963fb Mon Sep 17 00:00:00 2001 From: "M.Schmidt" Date: Sat, 11 Jul 2026 18:52:41 +0200 Subject: [PATCH 2/9] feat(webdav): extend HTTP PATCH to the NextCloud surface (RFC 5789) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The NextCloud-compatible WebDAV surface (/remote.php/dav/…) had no PATCH dispatch arm at all — requests fell through to 405 — unlike the plain-file surface (see the sibling commit on this repo's rfc-5789-http-patch work). Adds handle_patch to nextcloud/webdav_handler.rs, reusing the plain surface's X-Update-Range mechanism directly instead of duplicating it: - api/handlers/webdav_handler.rs::parse_update_range is now pub(crate) so both surfaces share the same header-parsing/validation logic. - upload_ingest::ingest_range_patch_to_cas (already surface-agnostic) splices the request body between the file's untouched prefix/suffix byte ranges and re-ingests through the same content-addressable pipeline handle_put uses. Follows this file's own handle_put conventions rather than the plain handler's: no active-lock guard (the NC surface has no LOCK/UNLOCK dispatch arm at all) and no explicit storage-quota check (handle_put doesn't do one either on this surface) — matching the sibling handler instead of importing behavior the NC surface doesn't otherwise have. Adds PATCH to the OPTIONS Allow header. Also fixes a pre-existing clippy::useless_borrows_in_formatting warning in thumbnail_service.rs (unrelated to this change, but blocking a clean clippy run on this branch). Adds tests/api/nc_webdav_patch.hurl covering explicit-range and append PATCH, the Content-Range rejection, the missing-header 400, and PATCH on a nonexistent file. --- src/interfaces/api/handlers/webdav_handler.rs | 6 +- src/interfaces/nextcloud/webdav_handler.rs | 179 +++++++++++++++++- tests/api/nc_webdav_patch.hurl | 172 +++++++++++++++++ 3 files changed, 352 insertions(+), 5 deletions(-) create mode 100644 tests/api/nc_webdav_patch.hurl diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 4b8d546e..d487990b 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -2006,7 +2006,11 @@ async fn handle_put( /// range isn't supported, use `append` or PUT for that. /// /// Returns `(start, end)`; `end` is `None` for `append`. -fn parse_update_range(header: &str, size: u64) -> Result<(u64, Option), AppError> { +/// +/// `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)); diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index b868eaaf..124bae99 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -5,11 +5,13 @@ use axum::{ }; use bytes::{Buf, Bytes}; use chrono::Utc; +use futures::stream::{self, Stream}; use quick_xml::{ Writer, events::{BytesEnd, BytesStart, BytesText, Event}, }; use std::collections::{HashMap, HashSet}; +use std::pin::Pin; use std::sync::Arc; use uuid::Uuid; @@ -29,12 +31,12 @@ 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, dead_props_for, file_dead_props, files_dead_props_map, folder_dead_props, parse_update_range, + folders_dead_props_map, streamed_file_dead_props, }; 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::{ingest_body_to_cas, ingest_range_patch_to_cas}; /// Extension trait to map XML write errors to `String` concisely. trait XmlResultExt { @@ -232,6 +234,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, @@ -267,7 +270,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()) @@ -963,6 +966,174 @@ 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. +/// +/// No active-lock guard here — the NC surface has no LOCK/UNLOCK dispatch +/// arm at all (see `handle_options`'s doc comment), matching `handle_put` +/// above, which has the same omission. +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 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. + let file = file_service + .get_file_by_path(&internal_path, chroot.drive_id) + .await + .map_err(|_| AppError::not_found(format!("File not found: {}", internal_path)))?; + + // ── 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_stream: Pin> + Send>> = + if start == 0 { + Box::pin(stream::empty()) + } else { + Box::into_pin( + file_service + .get_file_range_stream_with_perms(&file.id, session.user.id, 0, Some(start)) + .await + .map_err(AppError::from)?, + ) + }; + let suffix_stream: Pin> + Send>> = match end + { + Some(end) if end + 1 < file.size => Box::into_pin( + file_service + .get_file_range_stream_with_perms(&file.id, session.user.id, end + 1, None) + .await + .map_err(AppError::from)?, + ), + _ => Box::pin(stream::empty()), + }; + + let filename = filename_from_path(subpath).to_string(); + let ingested = ingest_range_patch_to_cas( + prefix_stream, + req.into_body(), + suffix_stream, + &state.core.dedup_service, + &filename, + &claimed_type, + max_upload, + ) + .await?; + + // ── Atomic store ────────────────────────────────────────────────── + let new_size = ingested.size; + let content_type = ingested.content_type.clone(); + let stored = upload_service + .update_file_streaming_with_perms( + &internal_path, + chroot.drive_id, + ingested.stored(), + &content_type, + None, + session.user.id, + ) + .await + .map_err(|e| AppError::internal_error(format!("Failed to store file: {}", e)))?; + + // 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( diff --git a/tests/api/nc_webdav_patch.hurl b/tests/api/nc_webdav_patch.hurl new file mode 100644 index 00000000..53d50b24 --- /dev/null +++ b/tests/api/nc_webdav_patch.hurl @@ -0,0 +1,172 @@ +# ============================================================= +# 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. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# 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 +[BasicAuth] +{{nc_username}}: {{nc_password}} +Content-Type: text/plain +``` +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 +[BasicAuth] +{{nc_username}}: {{nc_password}} +X-Update-Range: bytes=3-5 +Content-Type: text/plain +``` +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 +[BasicAuth] +{{nc_username}}: {{nc_password}} +X-Update-Range: append +Content-Type: text/plain +``` +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 +[BasicAuth] +{{nc_username}}: {{nc_password}} +X-Update-Range: bytes=0-2 +Content-Range: bytes 0-2/13 +Content-Type: text/plain +``` +abc +``` + +HTTP 400 + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Missing X-Update-Range header. +# ───────────────────────────────────────────────────────────── +PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt +[BasicAuth] +{{nc_username}}: {{nc_password}} +Content-Type: text/plain +``` +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 +[BasicAuth] +{{nc_username}}: {{nc_password}} +X-Update-Range: append +Content-Type: text/plain +``` +abc +``` + +HTTP 404 + + +# ───────────────────────────────────────────────────────────── +# 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 From c390a781bb2908461fed382f7fa586f47f21932e Mon Sep 17 00:00:00 2001 From: "M.Schmidt" Date: Tue, 14 Jul 2026 20:09:15 +0200 Subject: [PATCH 3/9] fix(webdav): close PATCH gaps found in review (quota, authz, cap, races) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes to the RFC 5789 PATCH implementation found by review of the rfc-5789-http-patch branch: - nextcloud/webdav_handler.rs::handle_patch now enforces storage quota before committing, matching the plain WebDAV surface (was a quota bypass via the NextCloud endpoint). - The plain surface's If-Match/If-None-Match comparison reused a hand-rolled single-value strong compare that mishandled weak (W/) validators and multi-value lists. Moved the correct RFC 7232 helpers (already used by the NC surface) into the shared handler file so both surfaces use one conformant implementation. - NC handle_patch resolved the target file via get_file_by_path, which performs no authorization check, before any permission-gated call — for a full-file-range patch this could leak size/ETag via 412/416 responses to a caller without Read on that file. Added the same explicit authz.require(Read, ...) the plain surface already has. - ingest_range_patch_to_cas capped the whole spliced stream (prefix + edit + suffix) against direct_put_max_bytes, so PATCH became permanently unusable on any file at or above that size regardless of edit size. The cap now only bounds the edit itself. - NC handle_patch had no active-lock guard, so a LOCK taken via /webdav/ didn't protect the same file reached through /remote.php/dav/. Now shares enforce_native_lock with the plain surface. - Added a re-check of the file's ETag immediately before the write on both surfaces, narrowing (not eliminating — that would need compare-and-swap support in the write path) the window in which two concurrent PATCHes to disjoint ranges could silently clobber each other. - NC handle_patch returned 404 for a PATCH on a directory instead of 409 like the plain surface; now checks folder existence first. - The Content-Length-vs-X-Update-Range span check only fired when Content-Length was present, so a chunked-transfer body could silently diverge from the declared span. ingest_range_patch_to_cas now counts actual body bytes and validates against the declared span regardless, discarding the ingested blob on mismatch. --- src/interfaces/api/handlers/webdav_handler.rs | 113 +++++++++--- src/interfaces/nextcloud/webdav_handler.rs | 165 ++++++++++++------ src/interfaces/upload_ingest.rs | 97 ++++++++-- 3 files changed, 288 insertions(+), 87 deletions(-) diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index d487990b..e628b2e5 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -1656,7 +1656,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, @@ -2042,6 +2042,59 @@ pub(crate) fn parse_update_range(header: &str, size: u64) -> Result<(u64, Option 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 + }) +} + /** * Handles PATCH requests (RFC 5789) for partial byte-range content updates. * @@ -2164,22 +2217,18 @@ async fn handle_patch( } // ── RFC 7232 conditional preconditions ──────────────────────────── - if let Some(ref inm) = if_none_match { - let server_tag = file.etag.trim_matches('"'); - if inm == "*" || inm.trim_matches('"') == server_tag { - return Err(AppError::precondition_failed( - "If-None-Match — resource already exists with that ETag", - )); - } - } - if let Some(ref im) = if_match - && im != "*" + let current_etag = Some(file.etag.as_str()); + if let Some(ref value) = if_none_match + && if_none_match_precondition_fails(value, current_etag) { - let client_tag = im.trim_matches('"'); - let server_tag = file.etag.trim_matches('"'); - if client_tag != server_tag { - return Err(AppError::precondition_failed("If-Match — ETag mismatch")); - } + 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 ───────────────────────────────────── @@ -2205,6 +2254,10 @@ async fn handle_patch( .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( @@ -2215,16 +2268,18 @@ async fn handle_patch( ), _ => Box::pin(stream::empty()), }; - let filename = crate::common::mime_detect::filename_from_path(&path).to_string(); let ingested = upload_ingest::ingest_range_patch_to_cas( - prefix_stream, + (prefix_stream, start), req.into_body(), - suffix_stream, + (suffix_stream, suffix_len), &state.core.dedup_service, &filename, &content_type, - max_upload, + upload_ingest::PatchIngestBudget { + max_bytes: max_upload, + expected_body_len: end.map(|end| end - start + 1), + }, ) .await?; @@ -2248,6 +2303,24 @@ async fn handle_patch( )); } + // ── Optimistic-concurrency re-check ─────────────────────────────── + // `file.etag` was snapshotted before the (potentially slow) splice + + // CAS-ingest above. Re-verify nothing else wrote to this file in the + // meantime, narrowing the window in which two concurrent PATCHes to + // disjoint ranges — each individually passing its own If-Match check + // against the same stale snapshot — could otherwise silently clobber + // each other on the blind-overwrite write path below. + if let Ok(current) = file_retrieval_service + .get_file_by_path(&path, drive_id) + .await + && current.etag != file.etag + { + upload_ingest::discard_ingested(&state.core.dedup_service, &ingested).await; + return Err(AppError::precondition_failed( + "File was modified concurrently — retry the PATCH", + )); + } + // ── Atomic store ────────────────────────────────────────────────── let new_size = ingested.size; let content_type = ingested.content_type.clone(); diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index 124bae99..ca7f0ff8 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -24,6 +24,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; @@ -31,12 +32,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, parse_update_range, + PROPFIND_BATCH_SIZE, dead_props_for, enforce_native_lock, file_dead_props, files_dead_props_map, folder_dead_props, + if_match_precondition_fails, if_none_match_precondition_fails, parse_update_range, folders_dead_props_map, streamed_file_dead_props, }; use crate::interfaces::errors::AppError; use crate::interfaces::range_requests::{not_modified_response, range_response}; -use crate::interfaces::upload_ingest::{ingest_body_to_cas, ingest_range_patch_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 { @@ -793,55 +797,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) @@ -1034,15 +989,67 @@ async fn handle_patch( .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. - let file = file_service + // 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()); @@ -1080,6 +1087,10 @@ async fn handle_patch( .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( @@ -1090,19 +1101,59 @@ async fn handle_patch( ), _ => Box::pin(stream::empty()), }; - let filename = filename_from_path(subpath).to_string(); let ingested = ingest_range_patch_to_cas( - prefix_stream, + (prefix_stream, start), req.into_body(), - suffix_stream, + (suffix_stream, suffix_len), &state.core.dedup_service, &filename, &claimed_type, - max_upload, + PatchIngestBudget { + max_bytes: max_upload, + expected_body_len: end.map(|end| end - start + 1), + }, ) .await?; + // ── 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 PATCH REJECTED (quota): user={}, file={}, size={}", + session.user.id, + internal_path, + ingested.size + ); + return Err(AppError::new( + StatusCode::INSUFFICIENT_STORAGE, + err.message, + "QuotaExceeded", + )); + } + + // ── Optimistic-concurrency re-check ─────────────────────────────── + // `file.etag` was snapshotted before the (potentially slow) splice + + // CAS-ingest above. Re-verify nothing else wrote to this file in the + // meantime, narrowing the window in which two concurrent PATCHes to + // disjoint ranges — each individually passing its own If-Match check + // against the same stale snapshot — could otherwise silently clobber + // each other on the blind-overwrite write path below. + if let Ok(current) = file_service + .get_file_by_path(&internal_path, chroot.drive_id) + .await + && current.etag != file.etag + { + discard_ingested(&state.core.dedup_service, &ingested).await; + return Err(AppError::precondition_failed( + "File was modified concurrently — retry the PATCH", + )); + } + // ── Atomic store ────────────────────────────────────────────────── let new_size = ingested.size; let content_type = ingested.content_type.clone(); diff --git a/src/interfaces/upload_ingest.rs b/src/interfaces/upload_ingest.rs index edf79933..d3e55aeb 100644 --- a/src/interfaces/upload_ingest.rs +++ b/src/interfaces/upload_ingest.rs @@ -16,7 +16,7 @@ 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; @@ -81,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)))) @@ -260,24 +278,83 @@ pub async fn ingest_body_to_cas( /// 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: Pin> + Send>>, + prefix: RangeSegment, body: Body, - suffix: Pin> + Send>>, + suffix: RangeSegment, dedup: &Arc, filename: &str, claimed_type: &str, - max_bytes: usize, + budget: PatchIngestBudget, ) -> Result { - let body_stream = BodyStream::new(body).filter_map(|item| async move { - match item { - Ok(frame) => frame.into_data().ok().map(Ok), - Err(e) => Some(Err(std::io::Error::other(e.to_string()))), + 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 combined = prefix.chain(body_stream).chain(suffix); - ingest_stream_to_cas(combined, dedup, filename, claimed_type, max_bytes, None).await + 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`]. From b79738a89ded98689d1479df1bb6e59eb3b54cb1 Mon Sep 17 00:00:00 2001 From: "M.Schmidt" Date: Wed, 15 Jul 2026 09:00:51 +0200 Subject: [PATCH 4/9] remove claude file from vcs --- .claude/scheduled_tasks.lock | 1 - 1 file changed, 1 deletion(-) delete mode 100644 .claude/scheduled_tasks.lock 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 From d57f7bfe3a324cb02a1586490e1840f639edb72d Mon Sep 17 00:00:00 2001 From: "M.Schmidt" Date: Wed, 15 Jul 2026 09:04:47 +0200 Subject: [PATCH 5/9] fix(webdav): wire orphaned PATCH tests + fix NC error-mapping and path bugs they caught MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit webdav_patch.hurl and nc_webdav_patch.hurl existed with real coverage since the original PATCH commits but were never added to tests/api/run.sh, so just api-test/CI silently skipped them. Wire both in, fix nc_webdav_patch.hurl's header-after-[BasicAuth] ordering bug that meant it had never actually passed, and add two new consistency-focused files chaining PATCH operations with cross-protocol/cross-surface verification: - webdav_patch_consistency.hurl: chained overwrites with ETag-change checks, GET/HEAD/PROPFIND cross-protocol agreement, quota-507 leaving the file byte-for-byte unchanged, direct_put_max_bytes prefix/suffix regression coverage. - nc_webdav_patch_consistency.hurl: Editor/Viewer/Outsider permission matrix, cross-surface lock interop, quota-507 via the NC surface. Running these surfaced two real bugs in the NC PATCH handler, both fixed here: - The write step mapped every error (including a legitimate anti-enum permission denial) to a raw 500 instead of AppError::from(e), unlike the plain surface. A Viewer without Update permission got a 500 leak instead of the expected 404. - nc_to_internal_path() didn't strip the leading '/' that chroot.path carries from StoragePath::to_string(), so a LOCK taken via /webdav/ silently failed to block PATCH via /remote.php/dav/ on the same file — the exact-string lock-store lookup never matched. Added a regression unit test. --- src/interfaces/nextcloud/webdav_handler.rs | 32 +- tests/api/nc_webdav_patch.hurl | 81 +-- tests/api/nc_webdav_patch_consistency.hurl | 541 +++++++++++++++++++++ tests/api/run.sh | 4 + tests/api/webdav_patch.hurl | 81 ++- tests/api/webdav_patch_consistency.hurl | 322 ++++++++++++ 6 files changed, 1003 insertions(+), 58 deletions(-) create mode 100644 tests/api/nc_webdav_patch_consistency.hurl create mode 100644 tests/api/webdav_patch_consistency.hurl diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index ca7f0ff8..b571cf6f 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -80,15 +80,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 @@ -1167,7 +1176,7 @@ async fn handle_patch( session.user.id, ) .await - .map_err(|e| AppError::internal_error(format!("Failed to store file: {}", e)))?; + .map_err(AppError::from)?; // Everything from `start` to the new EOF reflects the patch (the // untouched suffix, if any, may have shifted when the body's length @@ -2316,6 +2325,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/tests/api/nc_webdav_patch.hurl b/tests/api/nc_webdav_patch.hurl index 53d50b24..d916e61e 100644 --- a/tests/api/nc_webdav_patch.hurl +++ b/tests/api/nc_webdav_patch.hurl @@ -16,6 +16,16 @@ # 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). # ============================================================= @@ -50,12 +60,10 @@ 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}} -Content-Type: text/plain -``` -0123456789 -``` +`0123456789` HTTP 201 @@ -64,13 +72,11 @@ HTTP 201 # Step 2 — PATCH bytes 3-5 ("345") with "XYZ". # ───────────────────────────────────────────────────────────── PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt -[BasicAuth] -{{nc_username}}: {{nc_password}} X-Update-Range: bytes=3-5 Content-Type: text/plain -``` -XYZ -``` +[BasicAuth] +{{nc_username}}: {{nc_password}} +`XYZ` HTTP 204 [Asserts] @@ -90,13 +96,11 @@ body == "012XYZ6789" # Step 3 — PATCH append. # ───────────────────────────────────────────────────────────── PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt -[BasicAuth] -{{nc_username}}: {{nc_password}} X-Update-Range: append Content-Type: text/plain -``` -END -``` +[BasicAuth] +{{nc_username}}: {{nc_password}} +`END` HTTP 204 [Asserts] @@ -116,14 +120,12 @@ body == "012XYZ6789END" # Step 4 — Content-Range header on PATCH is rejected. # ───────────────────────────────────────────────────────────── PATCH {{base_url}}/remote.php/dav/files/{{nc_username}}/nc-patch-probe.txt -[BasicAuth] -{{nc_username}}: {{nc_password}} X-Update-Range: bytes=0-2 Content-Range: bytes 0-2/13 Content-Type: text/plain -``` -abc -``` +[BasicAuth] +{{nc_username}}: {{nc_password}} +`abc` HTTP 400 @@ -132,12 +134,10 @@ 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}} -Content-Type: text/plain -``` -abc -``` +`abc` HTTP 400 @@ -146,17 +146,42 @@ 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 -[BasicAuth] -{{nc_username}}: {{nc_password}} X-Update-Range: append Content-Type: text/plain -``` -abc -``` +[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 # ───────────────────────────────────────────────────────────── diff --git a/tests/api/nc_webdav_patch_consistency.hurl b/tests/api/nc_webdav_patch_consistency.hurl new file mode 100644 index 00000000..c4a722a0 --- /dev/null +++ b/tests/api/nc_webdav_patch_consistency.hurl @@ -0,0 +1,541 @@ +# ============================================================= +# 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 → 404 anti-enum. 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. 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 +# anti-enum 404 the plain surface already gave. +# ───────────────────────────────────────────────────────────── +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 404 + + +# ───────────────────────────────────────────────────────────── +# 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/run.sh b/tests/api/run.sh index 862758be..345db6ff 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -205,6 +205,10 @@ 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/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 index c61fd49d..a1c72dd2 100644 --- a/tests/api/webdav_patch.hurl +++ b/tests/api/webdav_patch.hurl @@ -17,6 +17,18 @@ # 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. # ============================================================= @@ -38,9 +50,7 @@ token: jsonpath "$.access_token" PUT {{base_url}}/webdav/patch-probe.txt Authorization: Bearer {{token}} Content-Type: text/plain -``` -0123456789 -``` +`0123456789` HTTP 201 [Captures] @@ -55,9 +65,7 @@ PATCH {{base_url}}/webdav/patch-probe.txt Authorization: Bearer {{token}} X-Update-Range: bytes=3-5 Content-Type: text/plain -``` -XYZ -``` +`XYZ` HTTP 204 [Asserts] @@ -79,11 +87,11 @@ PATCH {{base_url}}/webdav/patch-probe.txt Authorization: Bearer {{token}} X-Update-Range: append Content-Type: text/plain -``` --APPENDED -``` +`-APPENDED` HTTP 204 +[Captures] +current_etag: header "ETag" GET {{base_url}}/webdav/patch-probe.txt @@ -103,9 +111,7 @@ PATCH {{base_url}}/webdav/patch-probe.txt Authorization: Bearer {{token}} X-Update-Range: bytes=1000-1005 Content-Type: text/plain -``` -oops -``` +`oops` HTTP 416 @@ -118,9 +124,7 @@ Authorization: Bearer {{token}} X-Update-Range: bytes=0-2 If-Match: "not-the-real-etag" Content-Type: text/plain -``` -NOP -``` +`NOP` HTTP 412 @@ -149,9 +153,7 @@ PATCH {{base_url}}/webdav/patch-probe.txt Authorization: Bearer {{token}} X-Update-Range: bytes=0-2 Content-Type: text/plain -``` -NOP -``` +`NOP` HTTP 423 @@ -177,9 +179,7 @@ PATCH {{base_url}}/webdav/patch-probe-dir/ Authorization: Bearer {{token}} X-Update-Range: bytes=0-2 Content-Type: text/plain -``` -NOP -``` +`NOP` HTTP 409 @@ -191,9 +191,7 @@ PATCH {{base_url}}/webdav/patch-probe-does-not-exist.txt Authorization: Bearer {{token}} X-Update-Range: bytes=0-2 Content-Type: text/plain -``` -NOP -``` +`NOP` HTTP 404 @@ -204,13 +202,42 @@ HTTP 404 PATCH {{base_url}}/webdav/patch-probe.txt Authorization: Bearer {{token}} Content-Type: text/plain -``` -NOP -``` +`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 # ───────────────────────────────────────────────────────────── 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 From af74c940288a24532f68b0a8c46e5606276c9945 Mon Sep 17 00:00:00 2001 From: "M.Schmidt" Date: Wed, 15 Jul 2026 10:50:48 +0200 Subject: [PATCH 6/9] fix(webdav): make PATCH's concurrency guard a real compare-and-swap The app-level ETag re-check before the write still left a gap between the check and the actual UPDATE for a concurrent writer to land in. Push the check into the write path itself: swap_blob_hash now takes an expected_hash and only applies the SET under the same FOR UPDATE row lock it already held, closing the race instead of just narrowing it. Adds ErrorKind::PreconditionFailed (412) for the CAS-miss path; PUT/WOPI/chunked-upload keep blind-overwrite semantics by passing None --- src/application/ports/file_ports.rs | 11 ++ src/application/ports/storage_ports.rs | 9 ++ .../services/file_upload_service.rs | 5 +- .../services/trash_service_test.rs | 1 + src/common/stubs.rs | 3 + src/domain/errors.rs | 18 +++ .../pg/file_blob_write_repository.rs | 121 ++++++++++++------ src/interfaces/api/handlers/webdav_handler.rs | 30 ++--- src/interfaces/api/handlers/wopi_handler.rs | 1 + src/interfaces/errors.rs | 1 + src/interfaces/nextcloud/uploads_handler.rs | 5 + src/interfaces/nextcloud/webdav_handler.rs | 30 ++--- 12 files changed, 157 insertions(+), 78 deletions(-) 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 e628b2e5..3d10fcf3 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -1972,6 +1972,7 @@ async fn handle_put( &content_type, None, user.id, + None, ) .await; @@ -2303,25 +2304,15 @@ async fn handle_patch( )); } - // ── Optimistic-concurrency re-check ─────────────────────────────── - // `file.etag` was snapshotted before the (potentially slow) splice + - // CAS-ingest above. Re-verify nothing else wrote to this file in the - // meantime, narrowing the window in which two concurrent PATCHes to - // disjoint ranges — each individually passing its own If-Match check - // against the same stale snapshot — could otherwise silently clobber - // each other on the blind-overwrite write path below. - if let Ok(current) = file_retrieval_service - .get_file_by_path(&path, drive_id) - .await - && current.etag != file.etag - { - upload_ingest::discard_ingested(&state.core.dedup_service, &ingested).await; - return Err(AppError::precondition_failed( - "File was modified concurrently — retry the PATCH", - )); - } - - // ── Atomic store ────────────────────────────────────────────────── + // ── 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 content_type = ingested.content_type.clone(); let result = file_upload_service @@ -2332,6 +2323,7 @@ async fn handle_patch( &content_type, None, user.id, + Some(&file.content_hash), ) .await; 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 b571cf6f..6f6b90c3 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -912,6 +912,7 @@ async fn handle_put( &content_type, oc_mtime, session.user.id, + None, ) .await .map_err(AppError::from)?; @@ -1145,25 +1146,15 @@ async fn handle_patch( )); } - // ── Optimistic-concurrency re-check ─────────────────────────────── - // `file.etag` was snapshotted before the (potentially slow) splice + - // CAS-ingest above. Re-verify nothing else wrote to this file in the - // meantime, narrowing the window in which two concurrent PATCHes to - // disjoint ranges — each individually passing its own If-Match check - // against the same stale snapshot — could otherwise silently clobber - // each other on the blind-overwrite write path below. - if let Ok(current) = file_service - .get_file_by_path(&internal_path, chroot.drive_id) - .await - && current.etag != file.etag - { - discard_ingested(&state.core.dedup_service, &ingested).await; - return Err(AppError::precondition_failed( - "File was modified concurrently — retry the PATCH", - )); - } - - // ── Atomic store ────────────────────────────────────────────────── + // ── 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 content_type = ingested.content_type.clone(); let stored = upload_service @@ -1174,6 +1165,7 @@ async fn handle_patch( &content_type, None, session.user.id, + Some(&file.content_hash), ) .await .map_err(AppError::from)?; From a01b0a856e90d9ce7a3c155e56f24544e4fb022a Mon Sep 17 00:00:00 2001 From: "M.Schmidt" Date: Fri, 17 Jul 2026 08:46:26 +0200 Subject: [PATCH 7/9] fix error mapping for put; cross surface interop ressource locking; put quota leftovers; --- src/interfaces/api/handlers/webdav_handler.rs | 316 +++++++---- src/interfaces/nextcloud/webdav_handler.rs | 173 +++--- tests/api/nc_webdav_put_gaps.hurl | 501 ++++++++++++++++++ 3 files changed, 809 insertions(+), 181 deletions(-) create mode 100644 tests/api/nc_webdav_put_gaps.hurl diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 3d10fcf3..e9dd1393 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -31,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; @@ -40,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; @@ -1892,39 +1894,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 ────────────────────────────────────────────── @@ -1988,7 +1971,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()) } @@ -2096,6 +2079,106 @@ pub(crate) fn if_match_precondition_fails(header: &str, current_etag: Option<&st }) } +/// 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. * @@ -2244,36 +2327,20 @@ async fn handle_patch( } // ── Splice prefix/suffix around the patched span ─────────────────── - let prefix_stream: Pin> + Send>> = - if start == 0 { - Box::pin(stream::empty()) - } else { - Box::into_pin( - file_retrieval_service - .get_file_range_stream_with_perms(&file.id, user.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_service - .get_file_range_stream_with_perms(&file.id, user.id, end + 1, None) - .await - .map_err(AppError::from)?, - ), - _ => Box::pin(stream::empty()), - }; + 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_stream, start), + prefix_segment, req.into_body(), - (suffix_stream, suffix_len), + suffix_segment, &state.core.dedup_service, &filename, &content_type, @@ -2284,27 +2351,8 @@ async fn handle_patch( ) .await?; - // ── Quota enforcement ───────────────────────────────────────────── - if let Some(storage_svc) = state.storage_usage_service.as_ref() - && let Err(err) = storage_svc - .check_storage_quota(user.id, ingested.size) - .await - { - upload_ingest::discard_ingested(&state.core.dedup_service, &ingested).await; - tracing::warn!( - "⛔ WEBDAV PATCH REJECTED (quota): user={}, file={}, size={}", - user.id, - path, - ingested.size - ); - return Err(AppError::new( - StatusCode::INSUFFICIENT_STORAGE, - err.message, - "QuotaExceeded", - )); - } - - // ── Atomic store, compare-and-swap on the pre-splice content hash ── + // ── 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 @@ -2314,37 +2362,31 @@ async fn handle_patch( // — each individually passing its own If-Match check against the // same stale snapshot, then blindly overwriting each other. let new_size = ingested.size; - let content_type = ingested.content_type.clone(); - let result = file_upload_service - .update_file_streaming_with_perms( - &path, - drive_id, - ingested.stored(), - &content_type, - None, - user.id, - Some(&file.content_hash), - ) - .await; + let file_dto = cas_write_patch( + &state, + file_upload_service, + &path, + drive_id, + &ingested, + user.id, + &file.content_hash, + "WEBDAV PATCH", + ) + .await?; - match result { - Ok(file_dto) => { - // 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, &file_dto.etag) - .header( - header::CONTENT_RANGE, - format!("bytes {}-{}/{}", start, range_end, new_size), - ) - .body(Body::empty()) - .unwrap()) - } - Err(e) => Err(AppError::from(e)), - } + // 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()) } /** @@ -3775,4 +3817,56 @@ mod tests { 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/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index 6f6b90c3..a608ebcd 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -5,13 +5,11 @@ use axum::{ }; use bytes::{Buf, Bytes}; use chrono::Utc; -use futures::stream::{self, Stream}; use quick_xml::{ Writer, events::{BytesEnd, BytesStart, BytesText, Event}, }; use std::collections::{HashMap, HashSet}; -use std::pin::Pin; use std::sync::Arc; use uuid::Uuid; @@ -32,9 +30,9 @@ 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, enforce_native_lock, file_dead_props, files_dead_props_map, folder_dead_props, - if_match_precondition_fails, if_none_match_precondition_fails, parse_update_range, - folders_dead_props_map, streamed_file_dead_props, + 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}; @@ -837,6 +835,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. @@ -865,6 +870,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 @@ -897,13 +945,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, @@ -943,9 +1013,9 @@ async fn handle_put( /// pipeline `handle_put` uses ([`ingest_range_patch_to_cas`]) — unedited /// chunks on either side of the edit typically dedup for free. /// -/// No active-lock guard here — the NC surface has no LOCK/UNLOCK dispatch -/// arm at all (see `handle_options`'s doc comment), matching `handle_put` -/// above, which has the same omission. +/// 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, @@ -1086,36 +1156,20 @@ async fn handle_patch( } // ── Splice prefix/suffix around the patched span ─────────────────── - let prefix_stream: Pin> + Send>> = - if start == 0 { - Box::pin(stream::empty()) - } else { - Box::into_pin( - file_service - .get_file_range_stream_with_perms(&file.id, session.user.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_service - .get_file_range_stream_with_perms(&file.id, session.user.id, end + 1, None) - .await - .map_err(AppError::from)?, - ), - _ => Box::pin(stream::empty()), - }; + 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_stream, start), + prefix_segment, req.into_body(), - (suffix_stream, suffix_len), + suffix_segment, &state.core.dedup_service, &filename, &claimed_type, @@ -1126,27 +1180,8 @@ async fn handle_patch( ) .await?; - // ── 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 PATCH REJECTED (quota): user={}, file={}, size={}", - session.user.id, - internal_path, - ingested.size - ); - return Err(AppError::new( - StatusCode::INSUFFICIENT_STORAGE, - err.message, - "QuotaExceeded", - )); - } - - // ── Atomic store, compare-and-swap on the pre-splice content hash ── + // ── 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 @@ -1156,19 +1191,17 @@ async fn handle_patch( // — each individually passing its own If-Match check against the // same stale snapshot, then blindly overwriting each other. let new_size = ingested.size; - let content_type = ingested.content_type.clone(); - let stored = upload_service - .update_file_streaming_with_perms( - &internal_path, - chroot.drive_id, - ingested.stored(), - &content_type, - None, - session.user.id, - Some(&file.content_hash), - ) - .await - .map_err(AppError::from)?; + 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 diff --git a/tests/api/nc_webdav_put_gaps.hurl b/tests/api/nc_webdav_put_gaps.hurl new file mode 100644 index 00000000..73a59d53 --- /dev/null +++ b/tests/api/nc_webdav_put_gaps.hurl @@ -0,0 +1,501 @@ +# ============================================================= +# 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 anti-enum 404 the rest of the codebase relies +# on. +# 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 → 404 anti-enum, not a raw 500. 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 404 + + +# 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 From e2b5be6862e6485b470b014de9eb07fcd2d1cc9b Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Thu, 16 Jul 2026 21:17:17 +0200 Subject: [PATCH 8/9] security(webdav+nc): antienum (404) rather returning a 500 with reason --- tests/api/webdav_permissions.hurl | 35 +++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/api/webdav_permissions.hurl b/tests/api/webdav_permissions.hurl index 57cf811e..e6f589a5 100644 --- a/tests/api/webdav_permissions.hurl +++ b/tests/api/webdav_permissions.hurl @@ -207,6 +207,41 @@ Authorization: Bearer {{bob_token}} HTTP 403 +# ───────────────────────────────────────────────────────────── +# Step 9b — Bob (VIEWER) CANNOT COPY the probe folder. +# COPY requires Create on the destination parent, which +# Viewer doesn't have. Anti-enum 404 shape. +# +# This is the regression pin for AuthZ audit #2 +# (2026-07-12): the COPY handler used to `map_err(|e| +# AppError::internal_error(format!("Failed to copy folder +# tree: {}", e)))?` on `copy_folder_tree_with_perms`, +# which collapsed the `NotFound` that `authz.require` +# returns on denial into HTTP 500 — an "exists-but-denied" +# oracle. Fix routes through `AppError::from` so the same +# denial surfaces as 404, indistinguishable from a source +# path that simply doesn't exist. +# ───────────────────────────────────────────────────────────── +COPY {{base_url}}/webdav/@drive/{{shared_drive_id}}/probe-folder +Authorization: Bearer {{bob_token}} +Destination: {{base_url}}/webdav/@drive/{{shared_drive_id}}/probe-folder-copy + +HTTP 404 + + +# ───────────────────────────────────────────────────────────── +# Step 9c — Bob (VIEWER) CANNOT DELETE the probe folder. +# DELETE requires Delete on the target, which Viewer +# doesn't have. Anti-enum 404 shape — same regression +# pin as 9b (`map_err → internal_error` collapsed +# the `NotFound` from authz.require into a 500 oracle). +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/webdav/@drive/{{shared_drive_id}}/probe-folder +Authorization: Bearer {{bob_token}} + +HTTP 404 + + # ───────────────────────────────────────────────────────────── # Step 10 — Promote Bob from VIEWER to EDITOR. # `PATCH /api/drives/{id}/members/{subject-type}/{id}` From ec2b533a5355d09e26908f6b411c2e9b1f735ecb Mon Sep 17 00:00:00 2001 From: "M.Schmidt" Date: Sun, 19 Jul 2026 19:54:59 +0200 Subject: [PATCH 9/9] security(webdav): adapt to anti-enum pattern --- tests/api/nc_webdav_patch_consistency.hurl | 22 ++++++++------ tests/api/nc_webdav_put_gaps.hurl | 16 ++++++---- tests/api/run.sh | 1 + tests/api/webdav_permissions.hurl | 35 ---------------------- 4 files changed, 24 insertions(+), 50 deletions(-) diff --git a/tests/api/nc_webdav_patch_consistency.hurl b/tests/api/nc_webdav_patch_consistency.hurl index c4a722a0..e23f1d78 100644 --- a/tests/api/nc_webdav_patch_consistency.hurl +++ b/tests/api/nc_webdav_patch_consistency.hurl @@ -272,14 +272,18 @@ body == "XYZ3456789" # ───────────────────────────────────────────────────────────── # Step A7 — VIEWER (has Read via the grant, but not Update) is -# denied → 404 anti-enum. 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. 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 -# anti-enum 404 the plain surface already gave. +# 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 @@ -288,7 +292,7 @@ Content-Type: text/plain {{nc_basic_viewer}}: {{viewer_nc_password}} `NOP` -HTTP 404 +HTTP 403 # ───────────────────────────────────────────────────────────── diff --git a/tests/api/nc_webdav_put_gaps.hurl b/tests/api/nc_webdav_put_gaps.hurl index 73a59d53..d6d0657c 100644 --- a/tests/api/nc_webdav_put_gaps.hurl +++ b/tests/api/nc_webdav_put_gaps.hurl @@ -11,8 +11,9 @@ # 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 anti-enum 404 the rest of the codebase relies -# on. +# 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. @@ -219,9 +220,12 @@ body == "XYZ" # ───────────────────────────────────────────────────────────── # Step A7 — VIEWER (has Read via the grant, but not Update) is -# denied → 404 anti-enum, not a raw 500. Before the fix, -# `handle_put`'s write step mapped every `DomainError` -# (including this authz denial) to +# 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 @@ -230,7 +234,7 @@ Content-Type: text/plain {{nc_basic_viewer}}: {{viewer_nc_password}} `NOP` -HTTP 404 +HTTP 403 # Cleanup Part A. diff --git a/tests/api/run.sh b/tests/api/run.sh index 345db6ff..4a41ef0b 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -209,6 +209,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$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_permissions.hurl b/tests/api/webdav_permissions.hurl index e6f589a5..57cf811e 100644 --- a/tests/api/webdav_permissions.hurl +++ b/tests/api/webdav_permissions.hurl @@ -207,41 +207,6 @@ Authorization: Bearer {{bob_token}} HTTP 403 -# ───────────────────────────────────────────────────────────── -# Step 9b — Bob (VIEWER) CANNOT COPY the probe folder. -# COPY requires Create on the destination parent, which -# Viewer doesn't have. Anti-enum 404 shape. -# -# This is the regression pin for AuthZ audit #2 -# (2026-07-12): the COPY handler used to `map_err(|e| -# AppError::internal_error(format!("Failed to copy folder -# tree: {}", e)))?` on `copy_folder_tree_with_perms`, -# which collapsed the `NotFound` that `authz.require` -# returns on denial into HTTP 500 — an "exists-but-denied" -# oracle. Fix routes through `AppError::from` so the same -# denial surfaces as 404, indistinguishable from a source -# path that simply doesn't exist. -# ───────────────────────────────────────────────────────────── -COPY {{base_url}}/webdav/@drive/{{shared_drive_id}}/probe-folder -Authorization: Bearer {{bob_token}} -Destination: {{base_url}}/webdav/@drive/{{shared_drive_id}}/probe-folder-copy - -HTTP 404 - - -# ───────────────────────────────────────────────────────────── -# Step 9c — Bob (VIEWER) CANNOT DELETE the probe folder. -# DELETE requires Delete on the target, which Viewer -# doesn't have. Anti-enum 404 shape — same regression -# pin as 9b (`map_err → internal_error` collapsed -# the `NotFound` from authz.require into a 500 oracle). -# ───────────────────────────────────────────────────────────── -DELETE {{base_url}}/webdav/@drive/{{shared_drive_id}}/probe-folder -Authorization: Bearer {{bob_token}} - -HTTP 404 - - # ───────────────────────────────────────────────────────────── # Step 10 — Promote Bob from VIEWER to EDITOR. # `PATCH /api/drives/{id}/members/{subject-type}/{id}`