From e0156a43f525c2f1cce8f74e6177b3ff4f39f511 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Fri, 17 Jul 2026 20:37:54 +0200 Subject: [PATCH] security(wopi): resolve PutFile drive_id from file, not caller's default --- src/interfaces/api/handlers/wopi_handler.rs | 48 +++++- tests/api/run.sh | 3 +- tests/api/wopi_shared_drive.hurl | 162 ++++++++++++++++++++ 3 files changed, 205 insertions(+), 8 deletions(-) create mode 100644 tests/api/wopi_shared_drive.hurl diff --git a/src/interfaces/api/handlers/wopi_handler.rs b/src/interfaces/api/handlers/wopi_handler.rs index 1ce94960..9587136f 100644 --- a/src/interfaces/api/handlers/wopi_handler.rs +++ b/src/interfaces/api/handlers/wopi_handler.rs @@ -332,23 +332,57 @@ async fn put_file( }; // ── Atomic store: swap the file row onto the ingested blob ── - // `drive_id` scopes the path-based lookups in `update_file_streaming` - // post-D0. WOPI tokens carry the user UUID in `claims.sub`; we resolve - // that to the caller's default drive (WOPI today is a single-drive - // editing surface — no drive marker travels in the token). + // `drive_id` scopes the path-based lookups in + // `update_file_streaming_with_perms` post-D0. + // + // AuthZ audit #18 (2026-07-12): the pre-fix path resolved + // `drive_id` via `find_default_for_user(claims_sub_uuid)` — + // ALWAYS the caller's own default personal drive, regardless of + // where the file actually lived. Shared-drive edits either + // misrouted the write into the caller's personal drive (if the + // filename happened to collide with a personal-drive path) or + // 500'd on the parent-folder lookup. Resolve from the file's + // own parent folder instead — one PK probe, returns the drive + // the file genuinely belongs to. Also unlocks shared-drive WOPI + // editing. let claims_sub_uuid = match uuid::Uuid::parse_str(&claims.sub) { Ok(u) => u, Err(_) => return StatusCode::UNAUTHORIZED.into_response(), }; + let Some(folder_id_str) = file.folder_id.as_deref() else { + // Files always live under a folder (drive-root files use the + // drive-root folder id). A `None` here means the file entity + // is malformed — safest is a 500. + tracing::error!( + "WOPI PutFile: file {} has no parent folder id — cannot resolve drive", + file_id + ); + return StatusCode::INTERNAL_SERVER_ERROR.into_response(); + }; + let folder_uuid = match uuid::Uuid::parse_str(folder_id_str) { + Ok(u) => u, + Err(_) => { + tracing::error!( + "WOPI PutFile: file {} parent folder id '{}' is not a UUID", + file_id, + folder_id_str + ); + return StatusCode::INTERNAL_SERVER_ERROR.into_response(); + } + }; let drive_id = match state .app_state .drive_repo - .find_default_for_user(claims_sub_uuid) + .drive_id_for_folder(folder_uuid) .await { - Ok(d) => d.drive.id, + Ok(id) => id, Err(e) => { - tracing::error!("WOPI PutFile: default-drive lookup failed: {:?}", e); + tracing::error!( + "WOPI PutFile: drive-id lookup for folder {} failed: {:?}", + folder_uuid, + e + ); return StatusCode::INTERNAL_SERVER_ERROR.into_response(); } }; diff --git a/tests/api/run.sh b/tests/api/run.sh index d289c111..f2313c89 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -207,7 +207,8 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/webdav_drive_root.hurl" \ "$API_DIR/webdav_permissions.hurl" \ "$API_DIR/webdav_nested_move_cascade.hurl" \ - "$API_DIR/wopi_authz.hurl" + "$API_DIR/wopi_authz.hurl" \ + "$API_DIR/wopi_shared_drive.hurl" #bash "$API_DIR/dedup_bulk_upload.sh" diff --git a/tests/api/wopi_shared_drive.hurl b/tests/api/wopi_shared_drive.hurl new file mode 100644 index 00000000..11c7ab36 --- /dev/null +++ b/tests/api/wopi_shared_drive.hurl @@ -0,0 +1,162 @@ +# ============================================================= +# OxiCloud — WOPI PutFile against a shared drive +# ============================================================= +# Regression pin for AuthZ audit #18 (2026-07-12). +# +# `wopi_handler.rs::put_file` used to resolve the write's target +# drive via `drive_repo.find_default_for_user(claims_sub_uuid)` — +# ALWAYS the caller's own default personal drive, regardless of +# where the file being edited actually lived. Consequences for a +# shared-drive file: +# +# - If the file's path happened to collide with a personal-drive +# path, the write MISROUTED into the caller's personal drive +# (silent cross-drive data ejection). +# - Otherwise the parent-folder lookup inside +# `update_file_streaming_with_perms` missed and the request +# 500'd — a UX brick on shared-drive WOPI editing. +# +# Fix: resolve `drive_id` from the FILE's own parent folder via +# `drive_repo.drive_id_for_folder(file.folder_id)`. Same file → +# same drive → write lands in the shared drive it belongs to. +# +# This test: +# 1. Admin creates a shared drive (D3a shape). +# 2. Admin uploads `hello.txt` to the shared drive's root. +# 3. Admin mints a WOPI edit token. +# 4. Admin PutFile with fresh content → 200. +# Pre-fix this 500'd because the personal-drive-scoped +# parent-folder lookup couldn't find a folder named "" in +# admin's personal drive. +# 5. Admin GetFile → the shared drive holds the new content. +# Proves the write landed on the correct drive. +# +# Prereqs: `OXICLOUD_WOPI_ENABLED=true`, `OXICLOUD_WOPI_SECRET` +# pinned, mock discovery running (all wired in +# `tests/common/server.env` + run.sh — same as `wopi_authz.hurl`). +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Setup — admin login. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +admin_token: jsonpath "$.access_token" +admin_user_id: jsonpath "$.user.id" + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Admin creates a shared drive owned by themselves. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/drives +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "kind": "shared", + "name": "wopi-shared-drive-audit-18", + "owner": { "type": "user", "id": "{{admin_user_id}}" } +} + +HTTP 201 +[Captures] +wopi_drive_id: jsonpath "$.id" +wopi_drive_root_id: jsonpath "$.root_folder_id" + + +# ───────────────────────────────────────────────────────────── +# Step 2 — Upload `hello.txt` to the shared drive's root. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/files/upload +Authorization: Bearer {{admin_token}} +[MultipartFormData] +folder_id: {{wopi_drive_root_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +wopi_file_id: jsonpath "$.id" +[Asserts] +jsonpath "$.mime_type" == "text/plain" + + +# ───────────────────────────────────────────────────────────── +# Step 3 — Mint an editor URL. Admin has Update on their own +# shared drive → `can_write=true` in the token. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/wopi/editor-url?file_id={{wopi_file_id}}&action=edit +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Captures] +wopi_edit_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 4 — CheckFileInfo — sanity check the token is redeemable +# and reports `UserCanWrite=true`. Not the audit-#18 +# pin itself (this verb didn't touch the drive-lookup +# bug) but a quick "the setup is sound" gate before +# Step 5. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/wopi/files/{{wopi_file_id}}?access_token={{wopi_edit_token}} + +HTTP 200 +[Asserts] +jsonpath "$.UserCanWrite" == true + + +# ───────────────────────────────────────────────────────────── +# Step 5 — PutFile with fresh content → 200. +# +# PRE-FIX (before #18 close): this 500'd. The handler +# resolved drive_id via find_default_for_user(admin), +# got admin's personal drive, then +# `update_file_streaming_with_perms(path, personal_drive_id)` +# did a parent-folder-by-path lookup scoped to the +# personal drive — nothing at the shared-drive path +# existed there → error → 500 wrapper. +# +# POST-FIX: drive_id resolves from the file's own +# parent folder → shared drive → write lands in the +# correct drive. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/wopi/files/{{wopi_file_id}}/contents?access_token={{wopi_edit_token}} +Content-Type: application/octet-stream +``` +audit-#18 shared-drive WOPI PutFile canary +``` + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 6 — Round-trip proof: GetFile from the same token returns +# the NEW content, and it's coming from the shared +# drive (the only place `wopi_file_id` exists). +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/wopi/files/{{wopi_file_id}}/contents?access_token={{wopi_edit_token}} + +HTTP 200 +[Asserts] +body contains "audit-#18 shared-drive WOPI PutFile canary" + + +# ───────────────────────────────────────────────────────────── +# Cleanup — delete the file, then delete the shared drive +# (D3b: empty-drive precondition holds since the file is gone). +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/files/{{wopi_file_id}} +Authorization: Bearer {{admin_token}} + +HTTP 204 + + +DELETE {{base_url}}/api/drives/{{wopi_drive_id}} +Authorization: Bearer {{admin_token}} + +HTTP 204