From 5b996bb218d19dcca8d75ca2c6fc63b5b4472903 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Thu, 16 Jul 2026 21:17:17 +0200 Subject: [PATCH] security(webdav+nc): antienum (404) rather returning a 500 with reason --- src/interfaces/api/handlers/webdav_handler.rs | 70 +++++++++--------- src/interfaces/nextcloud/webdav_handler.rs | 74 +++++++++++-------- tests/api/webdav_permissions.hurl | 35 +++++++++ 3 files changed, 111 insertions(+), 68 deletions(-) diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 67c2af38..838b551a 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -2220,18 +2220,27 @@ async fn handle_delete( // optimized resolver and the read repositories disagree on path // shape for some files; see `resolve_or_legacy` docs. let _ = file_retrieval_service; // present for legacy fallback if needed elsewhere + // AuthZ audit #2 (2026-07-12): route service errors through + // `AppError::from` so authz denials from `_with_perms` surface as + // 404 (the anti-enum shape). The prior `map_err(|e| internal_error…)` + // collapsed every error — including the `NotFound` that + // `authz.require` returns on denial — into HTTP 500, giving a + // reliable "exists-but-denied" vs "missing" oracle to a probing + // caller. Also preserves `QuotaExceeded → 507`, + // `AlreadyExists → 409`, `InvalidInput → 400` shapes surfacing + // through the standard error mapping. match resolve_or_legacy(&state, &path, drive_id).await { Some(ResolvedResource::Folder(folder)) => { folder_service .delete_folder_with_perms(&folder.id, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to delete folder: {}", e)))?; + .map_err(AppError::from)?; } Some(ResolvedResource::File(file)) => { file_management_service .delete_file_with_perms(&file.id, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to delete file: {}", e)))?; + .map_err(AppError::from)?; } None => return Err(AppError::not_found(format!("Resource not found: {}", path))), } @@ -2380,28 +2389,23 @@ async fn handle_move( // RFC 4918 §9.9.3: when Overwrite: T, perform a DELETE on the // destination before moving. Without this the rename/move fails // on a unique-index conflict (same name in same parent). + // AuthZ audit #2 (2026-07-12): `_with_perms` returns `DomainError`; + // route 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`. match resolve_or_legacy(&state, &destination_path, dst_drive_id).await { Some(ResolvedResource::Folder(f)) => { folder_service .delete_folder_with_perms(&f.id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to delete existing destination: {}", - e - )) - })?; + .map_err(AppError::from)?; } Some(ResolvedResource::File(f)) => { file_management_service .delete_file_with_perms(&f.id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to delete existing destination: {}", - e - )) - })?; + .map_err(AppError::from)?; } None => {} } @@ -2679,28 +2683,23 @@ async fn handle_copy( // RFC 4918 §9.8.4: when Overwrite: T, the server MUST perform a // DELETE on the destination before the copy. Without this the copy // service returns a unique-index conflict (500). + // AuthZ audit #2 (2026-07-12): `_with_perms` returns `DomainError`; + // route 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`. match resolve_or_legacy(&state, &destination_path, dst_drive_id).await { Some(ResolvedResource::Folder(f)) => { folder_service .delete_folder_with_perms(&f.id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to delete existing destination: {}", - e - )) - })?; + .map_err(AppError::from)?; } Some(ResolvedResource::File(f)) => { file_management_service .delete_file_with_perms(&f.id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to delete existing destination: {}", - e - )) - })?; + .map_err(AppError::from)?; } None => {} } @@ -2754,6 +2753,12 @@ async fn handle_copy( } }; + // AuthZ audit #2 (2026-07-12): route service errors through + // `AppError::from` so authz denials from `_with_perms` 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` shapes. match resolved { ResolvedResource::Folder(folder) => { let recursive = depth != "0"; @@ -2766,9 +2771,7 @@ async fn handle_copy( Some(dest_name.to_string()), ) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to copy folder tree: {}", e)) - })?; + .map_err(AppError::from)?; } else { let create_dto = crate::application::dtos::folder_dto::CreateFolderDto { name: dest_name.to_string(), @@ -2777,12 +2780,7 @@ async fn handle_copy( folder_service .create_folder_with_perms(create_dto, user.id) .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to create destination folder: {}", - e - )) - })?; + .map_err(AppError::from)?; } } ResolvedResource::File(file) => { @@ -2790,7 +2788,7 @@ async fn handle_copy( file_management_service .copy_file_with_perms(&file.id, user.id, target_parent_id, copy_name) .await - .map_err(|e| AppError::internal_error(format!("Failed to copy file: {}", e)))?; + .map_err(AppError::from)?; } } diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index 0bd771f4..3b97d96d 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -933,6 +933,11 @@ async fn handle_put( // 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`. let stored = upload_service .update_file_streaming_with_perms( &internal_path, @@ -943,7 +948,7 @@ async fn handle_put( session.user.id, ) .await - .map_err(|e| AppError::internal_error(format!("Failed to store file: {}", e)))?; + .map_err(AppError::from)?; let status = if existed { StatusCode::NO_CONTENT @@ -1032,10 +1037,14 @@ async fn handle_mkcol( name: target_name.to_string(), parent_id: Some(parent_folder.id.clone()), }; + // AuthZ audit #7 (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. Also preserves + // `AlreadyExists → 409`, `QuotaExceeded → 507`, `InvalidInput → 400`. folder_service .create_folder_with_perms(dto, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to create folder: {}", e)))?; + .map_err(AppError::from)?; Ok(Response::builder() .status(StatusCode::CREATED) @@ -1077,20 +1086,22 @@ async fn handle_delete( Resource::Folder(folder_uuid), ) .await?; + // AuthZ audit #8 (2026-07-12): route service 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. `move_to_trash` and `delete_folder_with_perms` + // both return `DomainError` and both call `authz.require`. if let Some(trash_svc) = state.trash_service.as_ref() { trash_svc .move_to_trash(&folder.id, "folder", user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to trash folder: {}", e)) - })?; + .map_err(AppError::from)?; } else { folder_service .delete_folder_with_perms(&folder.id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to delete folder: {}", e)) - })?; + .map_err(AppError::from)?; } } ResolvedResource::File(file) => { @@ -1104,21 +1115,18 @@ async fn handle_delete( Resource::File(file_uuid), ) .await?; + // AuthZ audit #8 (2026-07-12): same anti-enum fix as folder branch above. if let Some(trash_svc) = state.trash_service.as_ref() { trash_svc .move_to_trash(&file.id, "file", user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to trash file: {}", e)) - })?; + .map_err(AppError::from)?; } else { let file_mgmt = &state.applications.file_management_service; file_mgmt .delete_file_with_perms(&file.id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to delete file: {}", e)) - })?; + .map_err(AppError::from)?; } } } @@ -1196,6 +1204,12 @@ async fn handle_move( // then proceed with the move. Trashing is fine: per RFC the source // resource appears at the destination URI; what happens to the // overwritten one is up to the server. + // + // AuthZ audit #9 (2026-07-12): route the `_with_perms` delete + // errors through `AppError::from` so authz denials surface as 404 + // (anti-enum) instead of `map_err → internal_error` 500. Also + // preserves `QuotaExceeded → 507`, `AlreadyExists → 409`, + // `InvalidInput → 400`. match existing { ResolvedResource::File(existing_file) => { let file_uuid = Uuid::parse_str(&existing_file.id).map_err(|_| { @@ -1212,12 +1226,7 @@ async fn handle_move( file_mgmt .delete_and_cleanup_with_perms(&existing_file.id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to overwrite destination file: {}", - e - )) - })?; + .map_err(AppError::from)?; } ResolvedResource::Folder(existing_folder) => { let folder_uuid = Uuid::parse_str(&existing_folder.id).map_err(|_| { @@ -1234,12 +1243,7 @@ async fn handle_move( folder_service .delete_folder_with_perms(&existing_folder.id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to overwrite destination folder: {}", - e - )) - })?; + .map_err(AppError::from)?; } } } @@ -1267,12 +1271,15 @@ async fn handle_move( None => "", }; + // AuthZ audit #9 (2026-07-12): route `_with_perms` errors + // through `AppError::from` so authz denials surface as 404 + // (anti-enum) instead of `map_err → internal_error` 500. if src_parent_sub == dest_parent_sub { // Same parent → rename. file_mgmt .rename_file_with_perms(&file.id, user.id, dest_name) .await - .map_err(|e| AppError::internal_error(format!("Rename failed: {}", e)))?; + .map_err(AppError::from)?; } else { // Different parent → move. let dest_parent = folder_service @@ -1283,14 +1290,14 @@ async fn handle_move( file_mgmt .move_file_with_perms(&file.id, user.id, Some(dest_parent.id.clone())) .await - .map_err(|e| AppError::internal_error(format!("Move failed: {}", e)))?; + .map_err(AppError::from)?; // If the filename changed too, rename after move. if file.name != dest_name { file_mgmt .rename_file_with_perms(&file.id, user.id, dest_name) .await - .map_err(|e| AppError::internal_error(format!("Rename failed: {}", e)))?; + .map_err(AppError::from)?; } } @@ -1332,6 +1339,9 @@ async fn handle_move( None => "", }; + // AuthZ audit #9 (2026-07-12): route `_with_perms` errors + // through `AppError::from` so authz denials surface as 404 + // (anti-enum) instead of `map_err → internal_error` 500. if src_parent_sub == dest_parent_sub { // Same parent → rename. use crate::application::dtos::folder_dto::RenameFolderDto; @@ -1344,7 +1354,7 @@ async fn handle_move( user.id, ) .await - .map_err(|e| AppError::internal_error(format!("Rename failed: {}", e)))?; + .map_err(AppError::from)?; } else { // Different parent → move. let dest_parent = folder_service @@ -1362,7 +1372,7 @@ async fn handle_move( user.id, ) .await - .map_err(|e| AppError::internal_error(format!("Move failed: {}", e)))?; + .map_err(AppError::from)?; // If the name changed too, rename. if folder.name != dest_name { @@ -1376,7 +1386,7 @@ async fn handle_move( user.id, ) .await - .map_err(|e| AppError::internal_error(format!("Rename failed: {}", e)))?; + .map_err(AppError::from)?; } } diff --git a/tests/api/webdav_permissions.hurl b/tests/api/webdav_permissions.hurl index 48707af3..a8e3be21 100644 --- a/tests/api/webdav_permissions.hurl +++ b/tests/api/webdav_permissions.hurl @@ -167,6 +167,41 @@ Destination: {{base_url}}/webdav/@drive/{{shared_drive_id}}/probe-folder-renamed HTTP 404 +# ───────────────────────────────────────────────────────────── +# 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}`