cjw/Oxicloud
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

security(webdav+nc): antienum (404) rather returning a 500 with reason

Browse Source
This commit is contained in:
Edouard Vanbelle
2026-07-16 21:17:17 +02:00
parent c1924c825b
commit 5b996bb218
3 changed files with 111 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/interfaces/api/handlers/webdav_handler.rs
+34 -36
View File
@@ -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)?;
}
}
src/interfaces/nextcloud/webdav_handler.rs
+42 -32
View File
@@ -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)?;
}
}
tests/api/webdav_permissions.hurl
+35
View File
@@ -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}`