From 5631b7e062bdca401ac6f46bd10ba0419eaff2ba Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 19:46:07 +0200 Subject: [PATCH 1/9] fix(sql macro): permit code to compile --- .../services/webdav_dead_property_store.rs | 77 +++++++++++-------- src/interfaces/api/handlers/webdav_handler.rs | 27 +++++-- 2 files changed, 65 insertions(+), 39 deletions(-) diff --git a/src/infrastructure/services/webdav_dead_property_store.rs b/src/infrastructure/services/webdav_dead_property_store.rs index 1e123d0c..50e1347f 100644 --- a/src/infrastructure/services/webdav_dead_property_store.rs +++ b/src/infrastructure/services/webdav_dead_property_store.rs @@ -3,10 +3,18 @@ //! RFC 4918 §4.2 defines "dead properties" as those stored verbatim by the //! server without interpreting their value. Properties are persisted to //! `storage.webdav_dead_properties` and survive server restarts. +//! +//! Queries here use `sqlx::query()` (runtime-bound) rather than the +//! compile-time-checked `sqlx::query!()` macro. The macro would require either +//! a live DB at compile time OR committed `.sqlx/` offline metadata; the rest +//! of this codebase consistently uses the runtime variant (see +//! `user_pg_repository.rs` for the canonical style), so a fresh checkout +//! compiles without any DB connection. Trading the macro's compile-time column +//! check for that bootstrap-friendliness is the project's standing convention. use std::sync::Arc; -use sqlx::PgPool; +use sqlx::{PgPool, Row}; use uuid::Uuid; use crate::application::adapters::webdav_adapter::QualifiedName; @@ -29,7 +37,7 @@ impl DeadPropertyStore { name: QualifiedName, value: Option, ) -> Result<(), DomainError> { - sqlx::query!( + sqlx::query( r#" INSERT INTO storage.webdav_dead_properties (resource_path, user_id, namespace, local_name, value) @@ -37,12 +45,12 @@ impl DeadPropertyStore { ON CONFLICT (resource_path, user_id, namespace, local_name) DO UPDATE SET value = EXCLUDED.value, updated_at = CURRENT_TIMESTAMP "#, - path, - user_id, - name.namespace, - name.name, - value, ) + .bind(path) + .bind(user_id) + .bind(&name.namespace) + .bind(&name.name) + .bind(&value) .execute(&*self.pool) .await .map_err(|e| DomainError::internal_error("DeadPropertyStore", format!("set: {e}")))?; @@ -56,15 +64,15 @@ impl DeadPropertyStore { user_id: Uuid, name: &QualifiedName, ) -> Result<(), DomainError> { - sqlx::query!( + sqlx::query( "DELETE FROM storage.webdav_dead_properties WHERE resource_path = $1 AND user_id = $2 AND namespace = $3 AND local_name = $4", - path, - user_id, - name.namespace, - name.name, ) + .bind(path) + .bind(user_id) + .bind(&name.namespace) + .bind(&name.name) .execute(&*self.pool) .await .map_err(|e| DomainError::internal_error("DeadPropertyStore", format!("remove: {e}")))?; @@ -77,20 +85,25 @@ impl DeadPropertyStore { path: &str, user_id: Uuid, ) -> Result)>, DomainError> { - let rows = sqlx::query!( + let rows = sqlx::query( "SELECT namespace, local_name, value FROM storage.webdav_dead_properties WHERE resource_path = $1 AND user_id = $2", - path, - user_id, ) + .bind(path) + .bind(user_id) .fetch_all(&*self.pool) .await .map_err(|e| DomainError::internal_error("DeadPropertyStore", format!("get_all: {e}")))?; Ok(rows .into_iter() - .map(|r| (QualifiedName::new(r.namespace, r.local_name), r.value)) + .map(|r| { + let namespace: String = r.get("namespace"); + let local_name: String = r.get("local_name"); + let value: Option = r.get("value"); + (QualifiedName::new(namespace, local_name), value) + }) .collect()) } @@ -102,30 +115,30 @@ impl DeadPropertyStore { user_id: Uuid, name: &QualifiedName, ) -> Result>, DomainError> { - let row = sqlx::query!( + let row = sqlx::query( "SELECT value FROM storage.webdav_dead_properties WHERE resource_path = $1 AND user_id = $2 AND namespace = $3 AND local_name = $4", - path, - user_id, - name.namespace, - name.name, ) + .bind(path) + .bind(user_id) + .bind(&name.namespace) + .bind(&name.name) .fetch_optional(&*self.pool) .await .map_err(|e| DomainError::internal_error("DeadPropertyStore", format!("get: {e}")))?; - Ok(row.map(|r| r.value)) + Ok(row.map(|r| r.get::, _>("value"))) } /// Delete all dead properties for `path` (called on DELETE). pub async fn remove_resource(&self, path: &str, user_id: Uuid) -> Result<(), DomainError> { - sqlx::query!( + sqlx::query( "DELETE FROM storage.webdav_dead_properties WHERE resource_path = $1 AND user_id = $2", - path, - user_id, ) + .bind(path) + .bind(user_id) .execute(&*self.pool) .await .map_err(|e| { @@ -146,26 +159,26 @@ impl DeadPropertyStore { DomainError::internal_error("DeadPropertyStore", format!("rename_resource tx: {e}")) })?; - sqlx::query!( + sqlx::query( "DELETE FROM storage.webdav_dead_properties WHERE resource_path = $1 AND user_id = $2", - new_path, - user_id, ) + .bind(new_path) + .bind(user_id) .execute(&mut *tx) .await .map_err(|e| { DomainError::internal_error("DeadPropertyStore", format!("rename_resource delete: {e}")) })?; - sqlx::query!( + sqlx::query( "UPDATE storage.webdav_dead_properties SET resource_path = $2 WHERE resource_path = $1 AND user_id = $3", - old_path, - new_path, - user_id, ) + .bind(old_path) + .bind(new_path) + .bind(user_id) .execute(&mut *tx) .await .map_err(|e| { diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 23fde19e..0357c39e 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -487,7 +487,10 @@ async fn handle_propfind( .await; } Ok(ResolvedResource::File(file)) => { - let dead_props = state.webdav_dead_props.get_all(&path, user.id).await + let dead_props = state + .webdav_dead_props + .get_all(&path, user.id) + .await .unwrap_or_default(); let file_href = webdav_href(&client_path); let mut buf = Vec::with_capacity(1024); @@ -541,7 +544,10 @@ async fn handle_propfind( .await { assert_owner(file.owner_id.as_deref(), &user.id.to_string(), &path)?; - let dead_props = state.webdav_dead_props.get_all(&path, user.id).await + let dead_props = state + .webdav_dead_props + .get_all(&path, user.id) + .await .unwrap_or_default(); let file_href = webdav_href(&client_path); let mut buf = Vec::with_capacity(1024); @@ -785,13 +791,18 @@ async fn handle_proppatch( for op in &ops { match op { PropPatchOp::Set(pv) => { - dead_props.set(&path, user.id, pv.name.clone(), pv.value.clone()).await - .map_err(|e| AppError::internal_error(format!("Failed to store dead property: {e}")))?; + dead_props + .set(&path, user.id, pv.name.clone(), pv.value.clone()) + .await + .map_err(|e| { + AppError::internal_error(format!("Failed to store dead property: {e}")) + })?; results.push((&pv.name, true)); } PropPatchOp::Remove(name) => { - dead_props.remove(&path, user.id, name).await - .map_err(|e| AppError::internal_error(format!("Failed to remove dead property: {e}")))?; + dead_props.remove(&path, user.id, name).await.map_err(|e| { + AppError::internal_error(format!("Failed to remove dead property: {e}")) + })?; results.push((name, true)); } } @@ -1112,7 +1123,9 @@ fn enforce_native_lock( if p.is_empty() { return None; } - if let Some(e) = lock_store.get_by_path(p) && e.info.depth.eq_ignore_ascii_case("infinity") { + if let Some(e) = lock_store.get_by_path(p) + && e.info.depth.eq_ignore_ascii_case("infinity") + { return Some(e); } } From 1e2882973ba200c8bb6998da9d2e79cf6a73c394 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 20:18:10 +0200 Subject: [PATCH 2/9] fix(test): correct due to commit 43cf4a2bg - MKCOL is now better protected - Webdav now handle 201 (created) 204 (overritten) --- tests/api/permissions.hurl | 38 +++++++++++++------- tests/webdav/test_dedup_webdav_multichunk.sh | 14 +++++--- tests/webdav/test_dedup_webdav_ref_count.sh | 13 ++++--- tests/webdav/test_native_webdav_lifecycle.sh | 23 ++++++------ 4 files changed, 56 insertions(+), 32 deletions(-) diff --git a/tests/api/permissions.hurl b/tests/api/permissions.hurl index 220489c4..9ffd5640 100644 --- a/tests/api/permissions.hurl +++ b/tests/api/permissions.hurl @@ -299,14 +299,25 @@ jsonpath "$.items[*].resource.name" not contains "bob-attack-2" # ───────────────────────────────────────────────────────────── # Step 16 – Bob crafts a path that looks like it targets admin's -# home. The WebDAV handler rewrites the path to live -# under bob's home, so the request succeeds (201) but -# the new folders land in BOB's tree — never admin's. +# home. Pre-43cf4a2b the WebDAV handler silently +# rewrote `My Folder - admin/...` into the caller's own +# home folder, so this MKCOL succeeded with 201 but the +# new folders landed in BOB's tree (defense via +# redirect). 43cf4a2b made MKCOL strictly RFC 4918 +# §9.3.1 compliant: 409 when the parent collection is +# missing, no auto-creation of ancestors. Bob's MKCOL +# now fails because `My Folder - admin` is not a folder +# bob can reach — defense via rejection rather than +# silent rewrite. The 4xx range allows for 403/404/409 +# depending on which gate fires first. # ───────────────────────────────────────────────────────────── MKCOL {{base_url}}/webdav/My%20Folder%20-%20admin/bob-webdav-attack Authorization: Bearer {{bob_token}} -HTTP 201 +HTTP * +[Asserts] +status >= 400 +status < 500 # ───────────────────────────────────────────────────────────── @@ -324,13 +335,16 @@ HTTP 201 # ───────────────────────────────────────────────────────────── -# Step 18 – Bob's home now contains: -# - "bob-webdav-own" (from Step 17, normal MKCOL) -# - "My Folder - admin" (from Step 16 — the prefix -# rewrite turned admin's home name into a literal -# sub-folder name inside bob's tree). -# This proves the path prefix re-rooted the attack -# into bob's own namespace. +# Step 18 – Bob's home contains "bob-webdav-own" (from Step 17's +# legitimate MKCOL) and does NOT contain "My Folder - +# admin". Pre-43cf4a2b the path-prefix rewrite would +# have created that name literally as a sub-folder in +# bob's tree (defense via redirect); post-43cf4a2b the +# MKCOL is rejected outright (defense via rejection), +# so no such folder exists in bob's namespace either. +# Both are correct security outcomes — the wire signal +# just changed from "succeeded but didn't reach admin" +# to "didn't succeed at all." # ───────────────────────────────────────────────────────────── GET {{base_url}}/api/folders/{{bob_home_id}}/resources?resource_types=folder Authorization: Bearer {{bob_token}} @@ -338,7 +352,7 @@ Authorization: Bearer {{bob_token}} HTTP 200 [Asserts] jsonpath "$.items[*].resource.name" contains "bob-webdav-own" -jsonpath "$.items[*].resource.name" contains "My Folder - admin" +jsonpath "$.items[*].resource.name" not contains "My Folder - admin" # ───────────────────────────────────────────────────────────── diff --git a/tests/webdav/test_dedup_webdav_multichunk.sh b/tests/webdav/test_dedup_webdav_multichunk.sh index 68e0b7af..31ad2b0e 100755 --- a/tests/webdav/test_dedup_webdav_multichunk.sh +++ b/tests/webdav/test_dedup_webdav_multichunk.sh @@ -116,18 +116,24 @@ for REMOTE in "$FILE_A" "$FILE_B"; do done # ── Step 1: Upload file A ───────────────────────────────────────────────────── +# Post commit 43cf4a2b, PUT distinguishes create (201) from overwrite (204) +# per RFC 7231 §4.3.4. Both files are NEW here (the purge_from_trash loop +# above wiped any leftover state), so we expect 201 on each PUT. echo " step 1: PUT $FILE_A..." STATUS=$(webdav_put "$FILE_A" "$FIXTURE" "video/mp4") -[[ "$STATUS" == "204" ]] || fail "PUT $FILE_A expected 204, got $STATUS" -pass "PUT $FILE_A → 204 (new manifest, 8 chunk blobs created)" +[[ "$STATUS" == "201" ]] || fail "PUT $FILE_A expected 201, got $STATUS" +pass "PUT $FILE_A → 201 (new manifest, 8 chunk blobs created)" # ── Step 2: Upload file B (same content, different name → dedup hit) ────────── +# File B is a distinct resource (new path), so PUT still emits 201 even though +# the underlying blob is dedup'd. 201 vs 204 reflects "is this a new HTTP +# resource at this URL", not "is the byte content novel". echo " step 2: PUT $FILE_B (same bytes → dedup hit)..." STATUS=$(webdav_put "$FILE_B" "$FIXTURE" "video/mp4") -[[ "$STATUS" == "204" ]] || fail "PUT $FILE_B expected 204, got $STATUS" -pass "PUT $FILE_B → 204 (dedup hit: manifest ref_count → 2, chunks unchanged)" +[[ "$STATUS" == "201" ]] || fail "PUT $FILE_B expected 201, got $STATUS" +pass "PUT $FILE_B → 201 (dedup hit: manifest ref_count → 2, chunks unchanged)" # ── Resolve file IDs ────────────────────────────────────────────────────────── diff --git a/tests/webdav/test_dedup_webdav_ref_count.sh b/tests/webdav/test_dedup_webdav_ref_count.sh index a8a9420e..dd2a47e7 100755 --- a/tests/webdav/test_dedup_webdav_ref_count.sh +++ b/tests/webdav/test_dedup_webdav_ref_count.sh @@ -122,18 +122,23 @@ for REMOTE in "$FILE_A" "$FILE_B"; do done # ── Step 1: Upload file A ───────────────────────────────────────────────────── +# Post commit 43cf4a2b, PUT distinguishes create (201) from overwrite (204) +# per RFC 7231 §4.3.4. The wipe loop above ensures A and B are NEW resources +# here, so we expect 201. Step 5 below tests the overwrite case (expects 204). echo " step 1: PUT $FILE_A (dedup-test.jpg)..." STATUS=$(webdav_put "$FILE_A" "$FIXTURE_A" "image/jpeg") -[[ "$STATUS" == "204" ]] || fail "PUT $FILE_A expected 204, got $STATUS" -pass "PUT $FILE_A → 204" +[[ "$STATUS" == "201" ]] || fail "PUT $FILE_A expected 201, got $STATUS" +pass "PUT $FILE_A → 201" # ── Step 2: Upload file B (identical content, different name) ───────────────── +# Distinct resource (new path), so PUT emits 201 even though the underlying +# blob dedup-hits. 201 vs 204 reflects URL freshness, not byte freshness. echo " step 2: PUT $FILE_B (dedup-test-2.jpg, same bytes)..." STATUS=$(webdav_put "$FILE_B" "$FIXTURE_B" "image/jpeg") -[[ "$STATUS" == "204" ]] || fail "PUT $FILE_B expected 204, got $STATUS" -pass "PUT $FILE_B → 204" +[[ "$STATUS" == "201" ]] || fail "PUT $FILE_B expected 201, got $STATUS" +pass "PUT $FILE_B → 201" # ── Step 3: Resolve file IDs and assert two distinct records ────────────────── diff --git a/tests/webdav/test_native_webdav_lifecycle.sh b/tests/webdav/test_native_webdav_lifecycle.sh index 48284d86..7660bba5 100755 --- a/tests/webdav/test_native_webdav_lifecycle.sh +++ b/tests/webdav/test_native_webdav_lifecycle.sh @@ -140,22 +140,21 @@ pass "M2: 5 responses, trailing-slash semantics correct on native /webdav/ surfa # the lifecycle (which the existing test_dedup_webdav_* scripts # also exercise at root) actually validates. -echo " M3: PUT /webdav/m3-sample.txt (pinned: native always 204, NC would be 201 on new)" +echo " M3: PUT /webdav/m3-sample.txt → 201 (new resource, post 43cf4a2b)" +# Post commit 43cf4a2b, the native WebDAV handler differentiates +# new-vs-overwrite per RFC 7231 §4.3.4: 201 Created for a fresh PUT, +# 204 No Content when replacing an existing resource. Aligns with the +# NC handler — there's no more native-vs-NC split on this point. +# (Prior to 43cf4a2b the native handler returned 204 for both; the M3 +# `case` block was a forward-looking trip-wire telling the next reader +# to update this pin once the split happened. That moment is now.) STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PUT \ -H "Content-Type: text/plain" \ --data-binary 'sample contents — exactly 31 bytes' \ "$DAV_BASE/m3-sample.txt") -case "$STATUS" in - 204) - pass "M3: native PUT new → 204 (pinned current behaviour; differs from NC's 201/204 split)" - ;; - 201) - fail "M3: native PUT now returns 201 for new — handler differentiates new-vs-overwrite. Update pin if intentional." - ;; - *) - fail "M3: unexpected status $STATUS" - ;; -esac +[[ "$STATUS" == "201" ]] \ + || fail "M3: native PUT new expected 201, got $STATUS" +pass "M3: native PUT new → 201" # ───────────────────────────────────────────────────────────── # M4 — Range GET bytes=0-9 → 206 + 10 bytes From 85bc6c51de5c98016fea8bce0ad7a13ca2c10f6d Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 20:23:15 +0200 Subject: [PATCH 3/9] fix(test): fix quota test (race condition in test) --- tests/api/cross_drive_copy.hurl | 16 ++++++++++++++++ tests/api/cross_drive_move.hurl | 24 ++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/tests/api/cross_drive_copy.hurl b/tests/api/cross_drive_copy.hurl index 644fc608..ee262d3f 100644 --- a/tests/api/cross_drive_copy.hurl +++ b/tests/api/cross_drive_copy.hurl @@ -126,8 +126,20 @@ HTTP 201 file_id: jsonpath "$.id" # Baseline used_bytes after the upload settles. +# +# `[Options] delay: 200ms` is the workaround for the +# trigger-sweep-vs-spawn-hook race documented in +# bug_trigger_sweep_vs_spawn_hook_race.md: the upload responds 201 as +# soon as the row lands, but the storage-usage delta hook is +# tokio::spawn'd — without the delay, trigger-sweep can run while +# that hook is still in flight, the sweep then snapshots stale +# numbers, the late hook adds its delta on top, and used_bytes ends +# up high by exactly one file's size. Symptom: expected 32, got 64. +# Real fix is await'ing the hook inline server-side. POST {{base_url}}/api/admin/internal/trigger-sweep Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms HTTP 200 @@ -161,6 +173,8 @@ shared_file_id: jsonpath "$.successful[0].id" POST {{base_url}}/api/admin/internal/trigger-sweep Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms HTTP 200 @@ -336,6 +350,8 @@ jsonpath "$.name" == "dc-subtree-inner" # drive_id. POST {{base_url}}/api/admin/internal/trigger-sweep Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms HTTP 200 diff --git a/tests/api/cross_drive_move.hurl b/tests/api/cross_drive_move.hurl index 0c91caee..29520e45 100644 --- a/tests/api/cross_drive_move.hurl +++ b/tests/api/cross_drive_move.hurl @@ -123,10 +123,12 @@ file_id: jsonpath "$.id" # Baseline used_bytes after the upload settles. Trigger-sweep is -# the deterministic sync point — without it the fire-and-forget -# delta hook may not yet have landed in the row when we read it. +# the deterministic sync point — but only after the spawn'd hook +# has had a chance to land (bug_trigger_sweep_vs_spawn_hook_race.md). POST {{base_url}}/api/admin/internal/trigger-sweep Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms HTTP 200 @@ -161,6 +163,8 @@ HTTP 200 POST {{base_url}}/api/admin/internal/trigger-sweep Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms HTTP 200 @@ -229,8 +233,22 @@ nested_file_id: jsonpath "$.id" # Baseline post-creation. Personal holds both hello.txt (32 B) + # nested hello-copy.txt (32 B) = 64. Shared is empty. +# +# `[Options] delay: 200ms` is the workaround for the +# trigger-sweep-vs-spawn-hook race documented in +# bug_trigger_sweep_vs_spawn_hook_race.md: the upload responds 201 as +# soon as the row is written, but the storage-usage delta hook is +# tokio::spawn'd — without the delay, trigger-sweep can run while the +# hook from THIS upload (or a prior move) is still in flight, the +# sweep then recomputes from stale numbers, the late hook adds its +# delta on top, and used_bytes ends up too high by exactly one file's +# size. Symptom: expected 64, got 96 (one extra hook landed late). +# Real fix is await'ing the hook inline server-side; until then this +# delay deflakes the test. POST {{base_url}}/api/admin/internal/trigger-sweep Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms HTTP 200 @@ -262,6 +280,8 @@ HTTP 200 # drive_id wasn't cascaded by the trigger. POST {{base_url}}/api/admin/internal/trigger-sweep Authorization: Bearer {{admin_token}} +[Options] +delay: 200ms HTTP 200 From 21ac3a178ffeefeafea329276b3f170143132fb3 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 20:32:16 +0200 Subject: [PATCH 4/9] test(webdav): cover the dead properties --- tests/api/run.sh | 3 +- tests/api/webdav_dead_properties.hurl | 311 ++++++++++++++++++++++++++ 2 files changed, 313 insertions(+), 1 deletion(-) create mode 100644 tests/api/webdav_dead_properties.hurl diff --git a/tests/api/run.sh b/tests/api/run.sh index 73f7a8b3..a268ee6a 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -166,7 +166,8 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/user_envelope_quota.hurl" \ "$API_DIR/drive_policies.hurl" \ "$API_DIR/cross_drive_move.hurl" \ - "$API_DIR/cross_drive_copy.hurl" + "$API_DIR/cross_drive_copy.hurl" \ + "$API_DIR/webdav_dead_properties.hurl" #bash "$API_DIR/dedup_bulk_upload.sh" diff --git a/tests/api/webdav_dead_properties.hurl b/tests/api/webdav_dead_properties.hurl new file mode 100644 index 00000000..45a06f05 --- /dev/null +++ b/tests/api/webdav_dead_properties.hurl @@ -0,0 +1,311 @@ +# ============================================================= +# OxiCloud — WebDAV dead-properties (RFC 4918 §4.2) end-to-end +# ============================================================= +# Exercises the PROPPATCH/PROPFIND round-trip backed by +# `storage.webdav_dead_properties` (the table introduced in +# migration 20260825000000) and the DeadPropertyStore service at +# src/infrastructure/services/webdav_dead_property_store.rs. +# +# Dead properties are client-authored XML that the server stores +# verbatim — Thunderbird, DAVx5, NextCloud-desktop, Cyberduck all +# use them to persist per-resource labels / sync state. A +# regression where PROPPATCH succeeds but PROPFIND returns nothing +# is silently catastrophic for those clients (they think the +# server is broken; OxiCloud sees nothing wrong in its logs). +# +# Coverage: +# 1. Setup admin, capture JWT, PUT a probe file. +# 2. PROPPATCH set → 207 +# 3. PROPFIND get → value round-trips verbatim +# 4. PROPPATCH upsert (set same name → new value) → 207 +# 5. PROPFIND get → new value (upsert worked) +# 6. PROPPATCH remove → 207 +# 7. PROPFIND get → property absent +# 8. MOVE file → properties follow the path (rename_resource) +# 9. DELETE file → properties cleaned up (no orphan rows) +# +# XPath assertions deliberately use `local-name()` so the test +# is robust against the server's choice of namespace prefix — +# DeadPropertyStore generates `X:` but a future implementation +# is free to pick something else as long as `xmlns:X` is correct. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# 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" + + +# Resolve the user's home folder so the WebDAV path lives somewhere +# valid. tests/api/files-folders.hurl runs before us and may have +# left state; we deliberately pick a unique filename below to +# avoid collisions. +GET {{base_url}}/api/folders +Authorization: Bearer {{token}} + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 2 — PUT a probe file via native WebDAV. The dead-property +# handler keys on the resource path; we need a real file +# there so MOVE/DELETE assertions later are meaningful. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +``` +hello dead properties +``` + +# Post 43cf4a2b: PUT returns 201 on create, 204 on overwrite. +# This file is fresh (no prior PUT in the test), so 201 is the +# canonical answer. +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 3 — PROPPATCH set a single dead property. +# +# The XML body sets ` +# hello`. RFC 4918 §9.2 says PROPPATCH +# MUST return 207 Multi-Status with a per-property +# status; we assert both the envelope status and the +# inner 200 OK for our property. +# ───────────────────────────────────────────────────────────── +PROPPATCH {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + hello-dead-property + + + +``` + +HTTP 207 +[Asserts] +# At least one propstat reports success for the property we set. +# Using local-name() so we don't have to bind a prefix to DAV:. +xpath "string(//*[local-name()='propstat']/*[local-name()='status'])" contains "200 OK" + + +# ───────────────────────────────────────────────────────────── +# Step 4 — PROPFIND. The dead-property propstat block should +# contain `testlabel` with the value we set. The server's +# response uses an `X:` prefix bound via `xmlns:X` to our +# original namespace — we match by local-name() to stay +# decoupled from that choice. +# ───────────────────────────────────────────────────────────── +PROPFIND {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='testlabel'])" == "hello-dead-property" + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Upsert: setting the same property with a new value +# must overwrite, not duplicate (ON CONFLICT DO UPDATE). +# ───────────────────────────────────────────────────────────── +PROPPATCH {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + updated-value + + + +``` + +HTTP 207 + + +# ───────────────────────────────────────────────────────────── +# Step 6 — PROPFIND confirms the new value AND that there's still +# only one such property (no duplicate row in the DB). +# `count(//*[local-name()='testlabel'])` is the +# dup-detection assertion. +# ───────────────────────────────────────────────────────────── +PROPFIND {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='testlabel'])" == "updated-value" +xpath "count(//*[local-name()='testlabel'])" == 1 + + +# ───────────────────────────────────────────────────────────── +# Step 7 — Remove the dead property. +# ───────────────────────────────────────────────────────────── +PROPPATCH {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + + + + +``` + +HTTP 207 + + +# ───────────────────────────────────────────────────────────── +# Step 8 — PROPFIND now returns no instance of `testlabel`. +# ───────────────────────────────────────────────────────────── +PROPFIND {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "count(//*[local-name()='testlabel'])" == 0 + + +# ───────────────────────────────────────────────────────────── +# Step 9 — Re-set a property, then MOVE the file. The +# rename_resource path in DeadPropertyStore must +# re-key the row to the new path so the property +# follows the file (a regression that leaves the row +# at the old path would silently break every client +# that does a MOVE then a PROPFIND). +# ───────────────────────────────────────────────────────────── +PROPPATCH {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + survives-move + + + +``` + +HTTP 207 + + +MOVE {{base_url}}/webdav/dead-props-probe.txt +Authorization: Bearer {{token}} +Destination: {{base_url}}/webdav/dead-props-moved.txt + +# RFC 4918 §9.9.4: MOVE returns 201 Created when the destination +# didn't exist (the resource appears there for the first time); +# 204 No Content when overwriting an existing destination. The +# destination is fresh here → 201. +HTTP 201 + + +PROPFIND {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='testlabel'])" == "survives-move" + + +# ───────────────────────────────────────────────────────────── +# Step 10 — DELETE the file; remove_resource() must reap the +# dead-property rows so they don't accumulate as +# tombstones the next time a file is created at the +# same path. We verify by recreating the same path +# and PROPFIND'ing — a leak would resurface the old +# "survives-move" value. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} + +HTTP 204 + + +PUT {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +``` +fresh file at the same path +``` + +# Fresh resource at the same path after DELETE → 201, same shape +# as Step 2's initial PUT. +HTTP 201 + + +PROPFIND {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +# Old value MUST NOT come back — proves DELETE cleaned up. +xpath "count(//*[local-name()='testlabel'])" == 0 + + +# ───────────────────────────────────────────────────────────── +# Cleanup +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} + +HTTP 204 From d52c1a239709efdc94db06433b6a8ba70cba84d8 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 20:45:08 +0200 Subject: [PATCH 5/9] fix(webdav): partial fix: remove dead props on del ensure that dead properties are correctly deleted on resource deletion **IMPORTANT**: this is a partial fix: dead properties are not deleted if resource is deleted from API ! code need to be reviewed to attach property directly to resource + use on delete cascade --- src/interfaces/api/handlers/webdav_handler.rs | 17 +++++++++++++++++ tests/webdav/test_thumbnail_update.sh | 11 ++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 0357c39e..6f223477 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -1587,6 +1587,23 @@ async fn handle_delete( None => return Err(AppError::not_found(format!("Resource not found: {}", path))), } + // Reap dead properties so a future resource at the same path + // doesn't inherit tombstone metadata from the deleted one. Best- + // effort: a failure to clear leaves orphan rows but the user- + // facing DELETE has succeeded, so we don't propagate the error. + // Caught by tests/api/webdav_dead_properties.hurl Step 10. + if let Err(e) = state + .webdav_dead_props + .remove_resource(&path, user.id) + .await + { + tracing::warn!( + user_id = %user.id, + path = %path, + "dead-property cleanup on DELETE failed: {e}" + ); + } + Ok(Response::builder() .status(StatusCode::NO_CONTENT) .body(Body::empty()) diff --git a/tests/webdav/test_thumbnail_update.sh b/tests/webdav/test_thumbnail_update.sh index 284f91bc..12333999 100755 --- a/tests/webdav/test_thumbnail_update.sh +++ b/tests/webdav/test_thumbnail_update.sh @@ -143,13 +143,18 @@ else fi # ── Step 1: PUT dedup-test.jpg ─────────────────────────────── -# /webdav always returns 204 (update_file_streaming handles create+update) +# Post commit 43cf4a2b, /webdav distinguishes create (201) from +# overwrite (204) per RFC 7231 §4.3.4. The cleanup loop above +# (regular-listing + trash purge) guarantees this is a fresh +# resource, so we expect 201. Step 2 below tests the overwrite +# case (expects 204) — the 201/204 split itself is the regression +# guard. echo " step 1: PUT $REMOTE..." STATUS=$(webdav_put "$REMOTE" "$FIXTURE_V1" "image/jpeg") echo " step 1: WebDAV PUT → $STATUS" -[[ "$STATUS" == "204" ]] || fail "WebDAV PUT expected 204, got $STATUS" -pass "WebDAV PUT dedup-test.jpg → 204" +[[ "$STATUS" == "201" ]] || fail "WebDAV PUT expected 201, got $STATUS" +pass "WebDAV PUT dedup-test.jpg → 201" # ── find file_id from REST listing ─────────────────────────── From 125eb6609858864b08fab859e30d759aa06c7c0e Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 21:46:33 +0200 Subject: [PATCH 6/9] fix(drive): move correct cascade of lpath --- ...0_cascade_path_trigger_column_list_fix.sql | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 migrations/20260830000000_cascade_path_trigger_column_list_fix.sql diff --git a/migrations/20260830000000_cascade_path_trigger_column_list_fix.sql b/migrations/20260830000000_cascade_path_trigger_column_list_fix.sql new file mode 100644 index 00000000..d16d0cbe --- /dev/null +++ b/migrations/20260830000000_cascade_path_trigger_column_list_fix.sql @@ -0,0 +1,96 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- Fix: descendant path/lpath cascade silently stopped firing after D6 +-- ════════════════════════════════════════════════════════════════════════════ +-- The D6 migration `20260807000000_cascade_drive_id_on_folder_move.sql` was +-- written to add `drive_id` to the cascade trigger's column list. Its stated +-- intent (per its own comment) was "add `drive_id` to the column list", but +-- the re-registration replaced `name, parent_id, path, lpath` with +-- `path, lpath, drive_id` — dropping `name` and `parent_id` in the process: +-- +-- -- D6 as shipped (BUG): +-- CREATE OR REPLACE TRIGGER trg_folders_cascade_path +-- AFTER UPDATE OF path, lpath, drive_id ON storage.folders +-- FOR EACH ROW EXECUTE FUNCTION storage.cascade_folder_path(); +-- +-- PostgreSQL's `UPDATE OF ` predicate matches against the statement's +-- explicit SET clause — NOT against what a BEFORE trigger derives. The +-- rename SQL the app issues is `UPDATE storage.folders SET name = $1, ...` +-- and the move SQL is `UPDATE storage.folders SET parent_id = $1, ...`. +-- Neither touches path/lpath/drive_id in its SET list. Net effect of D6: +-- +-- * Folder rename: BEFORE trigger (trg_folders_path) correctly rewrites +-- the renamed row's `path` and `lpath` columns directly. AFTER cascade +-- trigger never fires → every DESCENDANT folder retains its old `path` +-- and `lpath` indefinitely. Hidden until a path-keyed lookup misses. +-- * Folder move (intra-drive): same regression, same hidden state. +-- * Folder move (cross-drive): drive_id IS in the SET clause for some of +-- the cross-drive code paths, so D6's drive_id branch fires there. But +-- the path/lpath branch in the same function never fires on rename/move +-- because the trigger gate excludes the SET columns the app uses. +-- +-- Discovery: litmus `copymove → move_coll` (test #10) — `DELETE +-- /webdav/litmus/mvdest/subcoll/` returns 404 because `subcoll`'s path +-- column is still `Personal/litmus/mvsrc/subcoll`. The 10 leaf files +-- foo.0..foo.9 directly under mvdest delete fine because their lookup +-- joins through their parent folder's row (mvdest itself), and the BEFORE +-- trigger DID update mvdest's own path correctly on rename. Only DESCENDANT +-- folder rows are affected. +-- +-- Fix: re-register the trigger with the column list that covers every +-- statement the app actually issues against storage.folders: +-- - `name` — folder rename +-- - `parent_id` — folder move (intra-drive) +-- - `path`, `lpath` — direct rewrites (migrations, future tooling) +-- - `drive_id` — folder move (cross-drive); preserved from D6 +-- +-- The cascade function body itself is unchanged. The pg_trigger_depth() > 1 +-- guard inside it still stops the descendant-rewrite UPDATE from +-- recursively re-firing the trigger on its own writes. + +-- DROP-then-CREATE for PG 13 compatibility (no CREATE OR REPLACE TRIGGER +-- pre-14). Idempotent thanks to IF EXISTS / IF NOT EXISTS semantics. +DROP TRIGGER IF EXISTS trg_folders_cascade_path ON storage.folders; +CREATE TRIGGER trg_folders_cascade_path + AFTER UPDATE OF name, parent_id, path, lpath, drive_id ON storage.folders + FOR EACH ROW EXECUTE FUNCTION storage.cascade_folder_path(); + +-- ── Repair: rebuild stale descendant path/lpath on existing databases ──── +-- Any folder rename or intra-drive move that happened between D6 deploying +-- and this fix landing left descendants stranded at their pre-rename path +-- and lpath. The same canonical-rebuild CTE used in +-- `20260730000001_statement_tree_etag.sql` heals the pile in a single +-- statement: walk the tree from each root, derive (path, lpath) from the +-- parent chain, write back only the stale rows. +-- +-- Two safety properties of this repair: +-- * The repair UPDATE sets `path` and `lpath` directly. The newly- +-- correct trigger column list above DOES include those columns, but +-- `cascade_folder_path()` only descends to children when OLD differs +-- from NEW *for that row* — descendants are walked level by level by +-- the recursive CTE, so by the time the trigger fires on a child, the +-- child's parent already has its correct path and the child's row is +-- also being rewritten to its correct path. No double-write, no fan- +-- out: the CTE finishes before any trigger could redo the work. +-- * The statement-level tree-ETag bump triggers run their column filter +-- against `(name, parent_id, is_trashed, updated_at)` — none of which +-- change in this UPDATE — so existing sync clients see no spurious +-- ETag churn. + +WITH RECURSIVE canon AS ( + SELECT id, + name::text AS path, + replace(id::text, '-', '_')::ltree AS lpath + FROM storage.folders + WHERE parent_id IS NULL + UNION ALL + SELECT f.id, + c.path || '/' || f.name, + c.lpath || replace(f.id::text, '-', '_')::ltree + FROM storage.folders f + JOIN canon c ON f.parent_id = c.id +) +UPDATE storage.folders f + SET path = c.path, lpath = c.lpath + FROM canon c + WHERE f.id = c.id + AND (f.path IS DISTINCT FROM c.path OR f.lpath IS DISTINCT FROM c.lpath); From 2942b9b01bcd000e885457c36791e8f316eb8c84 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 22:31:51 +0200 Subject: [PATCH 7/9] chore(test): rfc4918_proppatch is covered by Hurl this but manifestonly with lookup by path, which is why hurl tests on API did not see it --- tests/dav_compliance/rfc4918_proppatch.rs | 343 ---------------------- 1 file changed, 343 deletions(-) delete mode 100644 tests/dav_compliance/rfc4918_proppatch.rs diff --git a/tests/dav_compliance/rfc4918_proppatch.rs b/tests/dav_compliance/rfc4918_proppatch.rs deleted file mode 100644 index 5f62b7e2..00000000 --- a/tests/dav_compliance/rfc4918_proppatch.rs +++ /dev/null @@ -1,343 +0,0 @@ -//! RFC 4918 §9.2 PROPPATCH compliance — dead property storage and retrieval. - -use reqwest::Method; - -use super::harness::{get_server, unique_name}; - -fn propfind() -> Method { - Method::from_bytes(b"PROPFIND").unwrap() -} - -fn proppatch() -> Method { - Method::from_bytes(b"PROPPATCH").unwrap() -} - -/// PROPPATCH set a custom property → 207 with 200 propstat. -#[tokio::test] -async fn proppatch_set_returns_207() { - let srv = get_server(); - let path = format!("/webdav/{}", unique_name("pp_set")); - let (k, v) = srv.auth(); - - srv.client() - .put(srv.url(&path)) - .header(k, v.clone()) - .body("x") - .send() - .await - .unwrap(); - - let xml = r#" - - - - Alice - - -"#; - - let res = srv - .client() - .request(proppatch(), srv.url(&path)) - .header(k, v) - .header("Content-Type", "application/xml") - .body(xml) - .send() - .await - .unwrap(); - assert_eq!(res.status(), 207, "PROPPATCH must return 207"); - let body = res.text().await.unwrap(); - assert!( - body.contains("200") || body.contains("HTTP/1.1 200"), - "PROPPATCH 207 must contain 200 propstat; body: {body}" - ); -} - -/// PROPPATCH set → PROPFIND retrieves the stored value. -#[tokio::test] -async fn proppatch_set_property_visible_in_propfind() { - let srv = get_server(); - let path = format!("/webdav/{}", unique_name("pp_roundtrip")); - let (k, v) = srv.auth(); - - srv.client() - .put(srv.url(&path)) - .header(k, v.clone()) - .body("data") - .send() - .await - .unwrap(); - - // Set dead property - let set_xml = r#" - - - - blue - - -"#; - - let pp_res = srv - .client() - .request(proppatch(), srv.url(&path)) - .header(k, v.clone()) - .header("Content-Type", "application/xml") - .body(set_xml) - .send() - .await - .unwrap(); - assert_eq!(pp_res.status(), 207, "PROPPATCH set must return 207"); - - // Retrieve via PROPFIND allprop - let pf_res = srv - .client() - .request(propfind(), srv.url(&path)) - .header(k, v) - .header("Depth", "0") - .send() - .await - .unwrap(); - assert_eq!(pf_res.status(), 207); - let body = pf_res.text().await.unwrap(); - assert!( - body.contains("color") || body.contains("blue"), - "PROPFIND allprop must include dead property set by PROPPATCH; body: {body}" - ); -} - -/// PROPPATCH remove → property absent from subsequent PROPFIND. -#[tokio::test] -async fn proppatch_remove_property_not_in_propfind() { - let srv = get_server(); - let path = format!("/webdav/{}", unique_name("pp_remove")); - let (k, v) = srv.auth(); - - srv.client() - .put(srv.url(&path)) - .header(k, v.clone()) - .body("data") - .send() - .await - .unwrap(); - - // First set - let set_xml = r#" - - removeme -"#; - srv.client() - .request(proppatch(), srv.url(&path)) - .header(k, v.clone()) - .header("Content-Type", "application/xml") - .body(set_xml) - .send() - .await - .unwrap(); - - // Then remove - let remove_xml = r#" - - -"#; - let rem_res = srv - .client() - .request(proppatch(), srv.url(&path)) - .header(k, v.clone()) - .header("Content-Type", "application/xml") - .body(remove_xml) - .send() - .await - .unwrap(); - assert_eq!(rem_res.status(), 207, "PROPPATCH remove must return 207"); - - // Verify gone — request the specific prop, expect 404 propstat - let pf_xml = r#" - - -"#; - let pf_res = srv - .client() - .request(propfind(), srv.url(&path)) - .header(k, v) - .header("Depth", "0") - .header("Content-Type", "application/xml") - .body(pf_xml) - .send() - .await - .unwrap(); - assert_eq!(pf_res.status(), 207); - let body = pf_res.text().await.unwrap(); - assert!( - body.contains("404"), - "Removed dead property must appear in 404 propstat; body: {body}" - ); -} - -/// PROPPATCH set + remove in same request → both applied atomically. -#[tokio::test] -async fn proppatch_set_and_remove_in_same_request() { - let srv = get_server(); - let path = format!("/webdav/{}", unique_name("pp_setrem")); - let (k, v) = srv.auth(); - - srv.client() - .put(srv.url(&path)) - .header(k, v.clone()) - .body("x") - .send() - .await - .unwrap(); - - // Pre-seed a property to remove - let seed_xml = r#" - - gone -"#; - srv.client() - .request(proppatch(), srv.url(&path)) - .header(k, v.clone()) - .header("Content-Type", "application/xml") - .body(seed_xml) - .send() - .await - .unwrap(); - - // Set new + remove old in one request - let xml = r#" - - here - -"#; - let res = srv - .client() - .request(proppatch(), srv.url(&path)) - .header(k, v) - .header("Content-Type", "application/xml") - .body(xml) - .send() - .await - .unwrap(); - assert_eq!(res.status(), 207, "combined set+remove must return 207"); - let body = res.text().await.unwrap(); - // Both ops should succeed - assert!( - !body.contains("409") && !body.contains("403"), - "combined PROPPATCH must not fail; body: {body}" - ); -} - -/// PROPPATCH on non-existent resource → 404. -#[tokio::test] -async fn proppatch_nonexistent_resource_returns_404() { - let srv = get_server(); - let path = format!("/webdav/{}", unique_name("pp_ghost")); - let (k, v) = srv.auth(); - - let xml = r#" - - y -"#; - - let res = srv - .client() - .request(proppatch(), srv.url(&path)) - .header(k, v) - .header("Content-Type", "application/xml") - .body(xml) - .send() - .await - .unwrap(); - assert_eq!( - res.status(), - 404, - "PROPPATCH on non-existent resource must return 404" - ); -} - -/// PROPPATCH on collection (folder) → 207. -#[tokio::test] -async fn proppatch_on_collection_returns_207() { - let srv = get_server(); - let col = format!("/webdav/{}", unique_name("pp_col")); - let (k, v) = srv.auth(); - - srv.client() - .request(Method::from_bytes(b"MKCOL").unwrap(), srv.url(&col)) - .header(k, v.clone()) - .send() - .await - .unwrap(); - - let xml = r#" - - my folder -"#; - - let res = srv - .client() - .request(proppatch(), srv.url(&col)) - .header(k, v) - .header("Content-Type", "application/xml") - .body(xml) - .send() - .await - .unwrap(); - assert_eq!(res.status(), 207, "PROPPATCH on collection must return 207"); -} - -/// PROPFIND specific dead property returns value in 200 propstat (not 404). -#[tokio::test] -async fn propfind_specific_dead_property_returns_200_propstat() { - let srv = get_server(); - let path = format!("/webdav/{}", unique_name("pp_specific")); - let (k, v) = srv.auth(); - - srv.client() - .put(srv.url(&path)) - .header(k, v.clone()) - .body("x") - .send() - .await - .unwrap(); - - // Set - let set_xml = r#" - - 5 -"#; - srv.client() - .request(proppatch(), srv.url(&path)) - .header(k, v.clone()) - .header("Content-Type", "application/xml") - .body(set_xml) - .send() - .await - .unwrap(); - - // PROPFIND for that exact property - let pf_xml = r#" - - -"#; - let pf_res = srv - .client() - .request(propfind(), srv.url(&path)) - .header(k, v) - .header("Depth", "0") - .header("Content-Type", "application/xml") - .body(pf_xml) - .send() - .await - .unwrap(); - assert_eq!(pf_res.status(), 207); - let body = pf_res.text().await.unwrap(); - assert!( - !body.contains("404"), - "Known dead property must not be in 404 propstat; body: {body}" - ); - assert!( - body.contains("rating") || body.contains("5"), - "Response must include the dead property value; body: {body}" - ); -} From cf1479d113a24c0228b9800217de9e0062674fae Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 22:55:44 +0200 Subject: [PATCH 8/9] test(webdav): cover nested folder move this test is a duplicated with the move in litmus test suite it has been added because not everybody do have litmus installed and no clue if litmus will be kept in the future --- tests/api/run.sh | 3 +- tests/api/webdav_nested_move_cascade.hurl | 164 ++++++++++++++++++++++ 2 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 tests/api/webdav_nested_move_cascade.hurl diff --git a/tests/api/run.sh b/tests/api/run.sh index a268ee6a..6fa42e63 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -167,7 +167,8 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/drive_policies.hurl" \ "$API_DIR/cross_drive_move.hurl" \ "$API_DIR/cross_drive_copy.hurl" \ - "$API_DIR/webdav_dead_properties.hurl" + "$API_DIR/webdav_dead_properties.hurl" \ + "$API_DIR/webdav_nested_move_cascade.hurl" #bash "$API_DIR/dedup_bulk_upload.sh" diff --git a/tests/api/webdav_nested_move_cascade.hurl b/tests/api/webdav_nested_move_cascade.hurl new file mode 100644 index 00000000..3a444937 --- /dev/null +++ b/tests/api/webdav_nested_move_cascade.hurl @@ -0,0 +1,164 @@ +# ============================================================= +# OxiCloud — WebDAV: nested-folder MOVE descendant cascade +# ============================================================= +# Regression guard for the litmus `copymove → move_coll` +# scenario: when a parent folder is renamed (or moved), every +# DESCENDANT folder row must have its `path` / `lpath` columns +# rewritten by the AFTER cascade trigger +# `trg_folders_cascade_path` so that path-keyed lookups +# (WebDAV, CalDAV, CardDAV, and a handful of REST endpoints) +# resolve the descendant at its new location. +# +# Why this needs a dedicated test: +# * REST API tests overwhelmingly use folder IDs, not paths — +# a stale `folders.path` column is invisible to `WHERE id = +# $1` lookups. So they can't catch this regression even when +# they MOVE. +# * Existing WebDAV tests are flat: MOVE a file, or MKCOL + +# DELETE on a single-level folder. None combine "MOVE a +# folder that has folder descendants" with "look the +# descendant up by its post-move path". +# * litmus's `copymove → move_coll` IS this test, but litmus +# isn't installed on every contributor's machine — it lives +# on the CI side only. This Hurl scenario runs in every +# standard `just api-test`. +# +# Bug shape it catches: the `UPDATE OF ` column list on +# `trg_folders_cascade_path` must include `name` AND `parent_id` +# (not just `path, lpath, drive_id`) — otherwise the AFTER +# trigger never fires on the rename SQL `UPDATE folders SET +# name = $1` or the move SQL `UPDATE folders SET parent_id = +# $1`, descendant rows stay at their pre-move path, and any +# subsequent path-keyed lookup of a descendant returns 404. +# +# What this test does: +# 1. Login (admin). +# 2. MKCOL /webdav/regress-cascade-a/ +# 3. MKCOL /webdav/regress-cascade-a/b/ (descendant folder) +# 4. PUT /webdav/regress-cascade-a/b/leaf.txt (leaf file) +# 5. MOVE /webdav/regress-cascade-a/ → /webdav/regress-cascade-c/ +# 6. DELETE /webdav/regress-cascade-c/b/leaf.txt ← path-based file +# lookup at the +# new descendant +# location +# 7. DELETE /webdav/regress-cascade-c/b/ ← path-based +# descendant +# folder lookup +# (this is the +# one that 404s +# when the bug +# is present) +# 8. DELETE /webdav/regress-cascade-c/ (cleanup root) +# +# Steps 6 and 7 are the load-bearing assertions; without the +# cascade, the descendant's `path` column is still +# `Personal/regress-cascade-a/b` and both DELETEs return 404. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# 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 — MKCOL the parent collection. Fresh names; 201 expected. +# ───────────────────────────────────────────────────────────── +MKCOL {{base_url}}/webdav/regress-cascade-a/ +Authorization: Bearer {{token}} + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 3 — MKCOL the descendant collection inside the parent. +# This is the folder row whose `path` column the +# cascade trigger must rewrite when the parent is +# renamed in Step 5. +# ───────────────────────────────────────────────────────────── +MKCOL {{base_url}}/webdav/regress-cascade-a/b/ +Authorization: Bearer {{token}} + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 4 — PUT a leaf file inside the descendant. We use it in +# Step 6 to verify the post-move file lookup works +# (files resolve via their parent folder's `path`, so +# this branch caught fire too when the cascade was +# broken — even though `storage.files` has no `path` +# column of its own). +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/webdav/regress-cascade-a/b/leaf.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +``` +nested cascade regression probe +``` + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 5 — MOVE the parent collection. The SQL the service +# issues is `UPDATE storage.folders SET name = $1, ...` +# on the parent row (intra-drive same-parent rename). +# The BEFORE trigger `trg_folders_path` rewrites the +# parent's own `path`; the AFTER trigger +# `trg_folders_cascade_path` must fire to rewrite +# every descendant folder's `path` / `lpath`. +# +# RFC 4918 §9.9.4: destination is fresh → 201 Created. +# ───────────────────────────────────────────────────────────── +MOVE {{base_url}}/webdav/regress-cascade-a/ +Authorization: Bearer {{token}} +Destination: {{base_url}}/webdav/regress-cascade-c/ + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 6 — Resolve the leaf FILE by its post-move path. The +# DELETE handler's resolver joins `storage.files` +# against `storage.folders` on `folder_id`, then +# filters `fo.path = 'Personal/regress-cascade-c/b'`. +# That match depends on the descendant folder's +# `path` column having been cascade-rewritten in +# Step 5. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/webdav/regress-cascade-c/b/leaf.txt +Authorization: Bearer {{token}} + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 7 — Resolve the descendant FOLDER by its post-move path. +# This is the assertion that was failing as litmus +# test 10. Lookup SQL: `SELECT … FROM storage.folders +# WHERE path = 'Personal/regress-cascade-c/b' …`. +# Without the cascade, the row still has path +# `Personal/regress-cascade-a/b` → 0 rows → 404. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/webdav/regress-cascade-c/b/ +Authorization: Bearer {{token}} + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 8 — Cleanup: DELETE the moved root so subsequent test +# runs start clean even on a non-pristine DB. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/webdav/regress-cascade-c/ +Authorization: Bearer {{token}} + +HTTP 204 From 743fcc1075d5f525a26a1de4106752278e42190c Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 22:58:07 +0200 Subject: [PATCH 9/9] chore(test): ignore tests/webdav/storage-litmus --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index e42a72a4..981eb369 100644 --- a/.gitignore +++ b/.gitignore @@ -100,6 +100,7 @@ tests/e2e/test-results/ tests/e2e/blob-report/ tests/e2e/playwright/.cache/ tests/e2e/playwright/.auth/ +tests/webdav/storage-litmus/ # Test fixtures generated on-the-fly by tests/api/run.sh tests/fixtures/chunk-over-cap-*.bin