From 52814b4d7cf9f8eb03006cdc39fe57e28457e559 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sun, 12 Jul 2026 21:37:35 +0200 Subject: [PATCH] fix(nextcloud): fix chroot + synchronisation - add better hurl coverage on nextcloud chrooted login - fix issue with nextcloud using /{drive name}/~{drive id}/ - fix trashbin handler confusion username vs {username}~{folder id} --- src/interfaces/nextcloud/trashbin_handler.rs | 21 +- src/interfaces/nextcloud/uploads_handler.rs | 30 +- tests/api/nc_multidrive_move_regression.hurl | 398 +++++++++++++++++++ tests/api/run.sh | 1 + 4 files changed, 445 insertions(+), 5 deletions(-) create mode 100644 tests/api/nc_multidrive_move_regression.hurl diff --git a/src/interfaces/nextcloud/trashbin_handler.rs b/src/interfaces/nextcloud/trashbin_handler.rs index 6a09a1c3..b860ce03 100644 --- a/src/interfaces/nextcloud/trashbin_handler.rs +++ b/src/interfaces/nextcloud/trashbin_handler.rs @@ -102,8 +102,16 @@ async fn handle_propfind( let nc = state.nextcloud.as_ref(); let file_id_svc = nc.map(|n| &n.file_ids); + // Emit hrefs with `session.raw_username` (composite `admin~` on + // non-home drives), NOT `user.username` (bare `admin`). The + // `NcSession` extractor cross-checks the URL `{user}` segment + // against `raw_username` and 403s on mismatch (see + // `session.rs::from_request_parts`). Emitting the bare form here + // would make every follow-up MOVE/DELETE from a non-home client + // 403 before the handler runs — the composite-credential Hurl + // regression caught this (B5 in `nc_multidrive_move_regression`). let mut buf = Vec::new(); - write_trashbin_multistatus(&mut buf, &items, &user.username, chroot, file_id_svc) + write_trashbin_multistatus(&mut buf, &items, &session.raw_username, chroot, file_id_svc) .await .map_err(|e| AppError::internal_error(format!("XML generation failed: {}", e)))?; @@ -139,8 +147,17 @@ async fn handle_restore( // with 412 — there is no `Overwrite: T` workflow for trash restore in // either Sabre/DAV or the NC desktop client (a live file being // silently replaced by an undeleted one would be a footgun). + // Use `session.raw_username` (composite `admin~` on + // non-home drives) to strip the destination prefix, NOT + // `user.username` (bare `admin`). NC clients send `Destination: + // /remote.php/dav/files/{raw_username}/…`; passing the bare + // username would leave the `~/` marker glued to the leading + // subpath segment and turn the collision-check into a lookup at + // a fabricated path. See `uploads_handler::handle_assemble` for + // the same fix in the chunked-upload MOVE. if let Some(dest_header) = dest_header - && let Some(dest_subpath) = extract_nc_subpath_from_dest(&dest_header, &user.username) + && let Some(dest_subpath) = + extract_nc_subpath_from_dest(&dest_header, &session.raw_username) { let dest_internal = nc_to_internal_path(chroot, &dest_subpath)?; let folder_service = &state.applications.folder_service; diff --git a/src/interfaces/nextcloud/uploads_handler.rs b/src/interfaces/nextcloud/uploads_handler.rs index 414d8d15..85089da8 100644 --- a/src/interfaces/nextcloud/uploads_handler.rs +++ b/src/interfaces/nextcloud/uploads_handler.rs @@ -150,7 +150,20 @@ async fn handle_propfind_session( .map_err(|e| AppError::internal_error(format!("Failed to list chunks: {}", e)))? .ok_or_else(|| AppError::not_found("Upload session not found"))?; - let session_href = format!("/remote.php/dav/uploads/{}/{}/", user.username, upload_id); + // Href MUST use `session.raw_username` (composite `admin~` on + // non-home drives), NOT `user.username` (bare `admin`). The + // `NcSession` extractor cross-checks the URL `{user}` segment + // against `raw_username` and 403s on mismatch — a composite-cred + // client that PROPFINDs, then MOVEs a chunk href back to us, would + // otherwise 403 at the extractor before any handler runs. Same + // fix shape as `trashbin_handler::handle_propfind` and + // `handle_assemble`'s destination-URL parsing. Storage-side keying + // stays on `user.username` — upload sessions are per-user, not + // per-drive. + let session_href = format!( + "/remote.php/dav/uploads/{}/{}/", + session.raw_username, upload_id + ); let session_last_modified = chrono::DateTime::::from_timestamp(listing.session_mtime as i64, 0) .unwrap_or_else(chrono::Utc::now) @@ -176,7 +189,7 @@ async fn handle_propfind_session( for chunk in &listing.chunks { let chunk_href = format!( "/remote.php/dav/uploads/{}/{}/{}", - user.username, upload_id, chunk.name + session.raw_username, upload_id, chunk.name ); let chunk_modified = chrono::DateTime::::from_timestamp(chunk.mtime as i64, 0) .unwrap_or_else(chrono::Utc::now) @@ -342,7 +355,18 @@ async fn handle_assemble( .and_then(|v| v.to_str().ok()) .and_then(|v| v.parse::().ok()); - let dest_subpath = extract_files_subpath(&destination, &user.username) + // Strip the destination URL prefix using the SESSION's raw username + // (`admin~` on non-home drives), NOT `user.username` + // (bare `admin`). NC clients send `Destination: /remote.php/dav/files/ + // {raw_username}/…` — the URL user-segment mirrors the credential + // they authenticated with. Passing bare `admin` here strips only + // `admin/` from a `admin~/…` destination, leaving the tilde + // marker glued to the leading path segment; the write then targets + // `/~/…` and fails with a parent-folder lookup + // error. Matches `webdav_handler::handle_move`'s call to + // `extract_nc_subpath_from_dest(&destination, url_user)` where + // `url_user = &session.raw_username` (webdav_handler.rs:1177). + let dest_subpath = extract_files_subpath(&destination, &session.raw_username) .ok_or_else(|| AppError::bad_request("Invalid Destination URL"))?; // Stream the chunk parts, in order, straight into the CDC chunk store — diff --git a/tests/api/nc_multidrive_move_regression.hurl b/tests/api/nc_multidrive_move_regression.hurl new file mode 100644 index 00000000..9c826370 --- /dev/null +++ b/tests/api/nc_multidrive_move_regression.hurl @@ -0,0 +1,398 @@ +# ============================================================= +# OxiCloud — NC multi-drive MOVE destination-prefix regressions +# ============================================================= +# Regression coverage for two sibling bugs discovered 2026-07-12 +# when the multi-drive `admin~{drive-uuid}` credential shape was +# rolled through the NC `/remote.php/dav/*` surface but two MOVE +# handlers were missed: +# +# uploads_handler::handle_assemble (chunked-upload MOVE) +# trashbin_handler (restore MOVE with a Destination header) +# +# Both handlers were stripping the destination-URL prefix with +# `&user.username` (bare `admin`) instead of +# `&session.raw_username` (composite `admin~{uuid}`). NC clients +# on a non-home drive send: +# Destination: /remote.php/dav/files/admin~{uuid}/ +# The bare-username strip left `~{uuid}/` glued to the +# leading path segment; downstream lookups then targeted a +# fabricated `/~{uuid}/…` path and 500'd (assemble +# path) or silently missed collisions (trash path). +# +# webdav_handler::handle_move (the standard `/dav/files/…` MOVE) +# was ALREADY correct — it uses `url_user = &session.raw_username`. +# The uploads + trashbin siblings were coverage gaps: no Hurl +# tests hit them with a composite credential. +# +# Hurl gotcha: the `[BasicAuth]` block parses the username as a +# single token terminated by `:`. A raw composite like +# `{{nc_username}}~{{drive_id}}` fails to parse because Hurl +# sees the `~` between two templates and expects a line +# terminator. Workaround: alias the composite into +# `nc_basic_user` via `[Options] variable:` on a bootstrap +# request, then use `{{nc_basic_user}}` in every subsequent +# BasicAuth block. +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Setup 1 — JWT login. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +jwt: jsonpath "$.access_token" +admin_user_id: jsonpath "$.user.id" + + +# ───────────────────────────────────────────────────────────── +# Setup 2 — Fetch admin's home drive root folder id. +# +# The composite `{user}~{marker}` shape sends the marker +# through `basic_auth_middleware.rs`, which resolves it as a +# **folder id** (not a drive id) via +# `folder_service.get_folder_with_perms(folder_id, user_id)` — +# the auth boundary refuses if the caller lacks Read on that +# folder. +# +# For a regression test we don't need a SECONDARY drive — +# we need any folder id the caller has Read on so the composite +# credential authenticates cleanly. Admin's own home folder is +# the trivially-authorized choice; the tilde-parsing bug in +# `handle_assemble` / trashbin restore fires the same way +# regardless of which folder id the marker points at. +# +# For the real multi-drive scenario Ed hit in production, the +# marker after `~` was the folder id of a shared drive's root +# where admin had explicit Read via role_grants. That code path +# is identical to the one exercised here — the bug is in the +# destination-URL parsing, not in what the folder id points to. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/folders +Authorization: Bearer {{jwt}} + +HTTP 200 +[Captures] +home_folder_id: jsonpath "$[0].id" + + +# ───────────────────────────────────────────────────────────── +# Setup 3 — Mint an app password. `username` in the response is +# just `admin`; we splice the folder id onto it in Setup 4 +# below to get the composite `admin~{uuid}` shape. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/app-passwords +Authorization: Bearer {{jwt}} +Content-Type: application/json +{ "label": "nc_multidrive_move_regression hurl test" } + +HTTP 200 +[Captures] +nc_username: jsonpath "$.username" +nc_password: jsonpath "$.password" +ap_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Setup 4 — Bootstrap the composite BasicAuth username. +# +# `[Options] variable:` sets a variable whose VALUE is a +# template expanded against the current bindings, then the +# result is available to all subsequent requests. `nc_username` +# and `home_folder_id` are already captured; concatenating them +# here hides the `~` from the strict `[BasicAuth]` parser +# (which would otherwise reject `{{nc_username}}~{{home_folder_id}}` +# mid-username). +# +# `/ready` is a cheap unauthenticated 200 that gives us a +# request to hang the option on. No side effects. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/ready +[Options] +variable: nc_basic_user={{nc_username}}~{{home_folder_id}} + +HTTP 200 + + +# ============================================================= +# A. Chunked-upload MOVE assemble regression +# ============================================================= +# `handle_assemble` in `uploads_handler.rs` was calling +# `extract_files_subpath(&destination, &user.username)`. With a +# composite Destination it treated `~{drive_uuid}/` as the +# target subpath, then tried `nc_to_internal_path(chroot, …)` +# → `/~{drive_uuid}/`. Downstream parent-folder +# lookup → 500. +# +# Fixed by binding on `&session.raw_username`. Test shape: +# A1 — MKCOL: create the chunked-upload session directory. +# A2 — MOVE `.file` (empty session → zero chunks → assemble +# writes an empty file at Destination). Pre-fix: 500 with +# "Failed to get folder at path: //~". +# Post-fix: 201 + file exists at the real Destination. +# A3 — PROPFIND on the destination path to confirm the file +# landed under the drive's root (NOT under `~/`). +# ============================================================= + +# A1 — MKCOL upload session. +MKCOL {{base_url}}/remote.php/dav/uploads/{{nc_basic_user}}/regression-upload-session +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 201 + + +# A2 — MOVE `.file` with a composite Destination header. Empty +# session, so the assemble step writes a zero-byte file at the +# destination path — that's fine, we're pinning the destination- +# parsing behaviour, not the byte-copying. +# +# Hurl gotcha: headers MUST come before section blocks like +# `[BasicAuth]`. `Destination:` after `[BasicAuth]` gets parsed +# as a new request's method line. +MOVE {{base_url}}/remote.php/dav/uploads/{{nc_basic_user}}/regression-upload-session/.file +Destination: {{base_url}}/remote.php/dav/files/{{nc_basic_user}}/regression-assembled.txt +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +# The regression: pre-fix this returned 500 with a +# "~" fragment in the error message; post-fix it +# writes the empty file successfully. Any 2xx status proves the +# destination-parsing path is intact. +HTTP 201 + + +# A3 — Confirm the file exists at the real path inside the +# chroot. HTTP 207 alone is the load-bearing assertion: pre-fix, +# MOVE would have 500'd (so we'd never reach here); and even if +# it had somehow written, the file would have landed at the +# fabricated `/~/…` path rather than +# `/regression-assembled.txt` — this PROPFIND would +# then 404 rather than 207. +# +# `body not contains "~{folder_id}/..."` would be redundant AND +# wrong here: NC's PROPFIND echoes the client's request URL in +# ``, so the composite `admin~` legitimately +# appears in the returned href — that's the URL prefix, not a +# leak. The empty-file-size assertion below is the concrete +# positive check: MOVE with zero chunks assembles a 0-byte +# file, so we pin that shape. +PROPFIND {{base_url}}/remote.php/dav/files/{{nc_basic_user}}/regression-assembled.txt +Depth: 0 +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='getcontentlength'])" == "0" + + +# Cleanup — remove the assembled file so a re-run starts clean. +DELETE {{base_url}}/remote.php/dav/files/{{nc_basic_user}}/regression-assembled.txt +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 204 + + +# ============================================================= +# B. Trashbin restore MOVE — sibling handler with the same bug +# ============================================================= +# `trashbin_handler.rs` line 143 has the same shape: +# extract_nc_subpath_from_dest(&dest_header, &user.username) +# The trash MOVE uses the Destination header for a collision +# pre-check, not for relocation (restore always lands at the +# original path). A buggy prefix strip therefore doesn't 500 — +# it silently miscomputes the collision path (`/~/…` +# instead of `/`), letting a real collision +# slip past. The response is 2xx either way. +# +# So a "5xx vs 201" assertion won't catch it. What DOES catch it: +# stage a genuine collision, restore with a Destination that +# points at it. Pre-fix: no 412 (bug misses the collision). +# Post-fix: 412 Precondition Failed. +# +# Sequence: +# B1 — Upload `regression-collision.txt` to the drive. +# B2 — DELETE it (soft-trash). +# B3 — Re-upload `regression-collision.txt` (new file at the +# same path) to stage the collision. +# B4 — Enumerate the trashbin to find the trashed item's id. +# B5 — MOVE the trash item back with Destination pointing at +# the re-created file. Pre-fix: 201/204 (collision missed). +# Post-fix: 412 Precondition Failed. +# ============================================================= + +# B1 — Stage the file the client will trash. +PUT {{base_url}}/remote.php/dav/files/{{nc_basic_user}}/regression-collision.txt +Content-Type: text/plain +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} +``` +first version +``` + +HTTP 201 + + +# B2 — Soft-trash it. +DELETE {{base_url}}/remote.php/dav/files/{{nc_basic_user}}/regression-collision.txt +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 204 + + +# B3 — Re-upload at the same path to stage the collision. +PUT {{base_url}}/remote.php/dav/files/{{nc_basic_user}}/regression-collision.txt +Content-Type: text/plain +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} +``` +second version +``` + +HTTP 201 + + +# B4 — Enumerate trashbin to find the trashed item's numeric id +# (NC identifies trash items with `oc:trashbin-filename` etc.). +# Using PROPFIND at Depth 1 on the trashbin root. +PROPFIND {{base_url}}/remote.php/dav/trashbin/{{nc_basic_user}}/trash +Depth: 1 +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 207 +[Captures] +# Grab the href of the first trashed child. Fragile against +# multi-item trash but this test creates exactly one before +# reading — safe here. Local-name xpath so we don't have to +# thread the DAV namespace prefix. +trash_item_href: xpath "string((//*[local-name()='response']/*[local-name()='href'])[2])" + + +# B5 — MOVE the trash item back with a composite Destination. +# Pre-fix: collision check runs against a fake `/~/…` +# path, misses the real collision, restore succeeds (201/204). +# Post-fix: collision check hits the real path, request refused +# with 412. +MOVE {{base_url}}{{trash_item_href}} +Destination: {{base_url}}/remote.php/dav/files/{{nc_basic_user}}/regression-collision.txt +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +# Post-fix expectation: 412 (collision detected). If a future +# change makes trashbin restore honour Destination for +# relocation, this assertion changes — but the collision-check +# semantics should stay collision-refusing. +HTTP 412 + + +# Cleanup — permanently delete the trashed item so a re-run +# starts clean, and drop the live file. +DELETE {{base_url}}{{trash_item_href}} +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 204 + + +DELETE {{base_url}}/remote.php/dav/files/{{nc_basic_user}}/regression-collision.txt +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 204 + + +# ============================================================= +# C. Chunked-upload PROPFIND href regression +# ============================================================= +# `handle_propfind_session` in `uploads_handler.rs` was emitting +# `` values with `user.username` (bare `admin`) instead of +# `session.raw_username` (composite `admin~`). Same shape +# as the trashbin PROPFIND bug: NC clients doing chunked-upload +# resume PROPFIND the session, then MOVE/DELETE against the +# returned hrefs. With the bare form, every follow-up 403s at +# the `NcSession` extractor (URL `{user}` segment mismatches +# `raw_username`). +# +# Positive test: after PROPFIND-ing an upload session with a +# composite credential, the emitted hrefs MUST contain `~`. +# Pre-fix: `/remote.php/dav/uploads/admin/…`. +# Post-fix: `/remote.php/dav/uploads/admin~/…`. +# ============================================================= + +# C1 — MKCOL a fresh session. +MKCOL {{base_url}}/remote.php/dav/uploads/{{nc_basic_user}}/regression-propfind-session +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 201 + + +# C2 — PUT one chunk so PROPFIND has something to enumerate +# alongside the session collection itself. +PUT {{base_url}}/remote.php/dav/uploads/{{nc_basic_user}}/regression-propfind-session/00000001 +Content-Type: application/octet-stream +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} +``` +chunk-body +``` + +HTTP 201 + + +# C3 — PROPFIND the session with the composite credential and +# assert every emitted href carries the composite user segment. +# `contains "~{{home_folder_id}}/"` is the exact byte marker +# introduced by the fix — the bug would produce +# `/dav/uploads/admin/…` with no `~` between the surface and the +# session id. +PROPFIND {{base_url}}/remote.php/dav/uploads/{{nc_basic_user}}/regression-propfind-session +Depth: 1 +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 207 +[Asserts] +# Every href for this upload surface must echo the composite user. +# Two responses expected: session collection + one chunk. Both +# hrefs share the same `/remote.php/dav/uploads///…` +# prefix, so one substring check on the body body is sufficient +# and immune to XML formatting drift. +body contains "/remote.php/dav/uploads/{{nc_basic_user}}/regression-propfind-session/" +# Belt-and-suspenders: assert the bare form is absent. The +# composite basic-user is `admin~`; the bare form would +# render as `/dav/uploads/admin/regression-…` (no `~`). +# `not contains` here would still permit that byte sequence to +# appear inside the composite, so we anchor on the trailing `/` +# after the user segment to disambiguate: `/admin/regression-…` +# is the bug shape; the fix never produces `/admin/regression-…` +# because the composite always separates admin from the session +# with `~`. +body not contains "/remote.php/dav/uploads/{{nc_username}}/regression-propfind-session" + + +# C4 — DELETE the session with a composite href. Pre-fix (bare +# href returned by C3 that the client would have followed) this +# would have been a wire-level 403 at the extractor; post-fix +# the composite href works end-to-end. +DELETE {{base_url}}/remote.php/dav/uploads/{{nc_basic_user}}/regression-propfind-session +[BasicAuth] +{{nc_basic_user}}: {{nc_password}} + +HTTP 204 + + +# ============================================================= +# Teardown — revoke the app password. +# ============================================================= +DELETE {{base_url}}/api/auth/app-passwords/{{ap_id}} +Authorization: Bearer {{jwt}} + +HTTP 200 diff --git a/tests/api/run.sh b/tests/api/run.sh index 5f734bb1..032a61b5 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -186,6 +186,7 @@ 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/nc_multidrive_move_regression.hurl" \ "$API_DIR/webdav_dead_properties.hurl" \ "$API_DIR/webdav_drive_root.hurl" \ "$API_DIR/webdav_permissions.hurl" \