fix(webdav): wire orphaned PATCH tests + fix NC error-mapping and path bugs they caught

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.
This commit is contained in:
M.Schmidt
2026-07-15 09:04:47 +02:00
parent b79738a89d
commit d57f7bfe3a
6 changed files with 1003 additions and 58 deletions
+29 -3
View File
@@ -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<String, AppError> {
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");