security(nc-uploads+trash): close #12 chunked-upload create bypass; graduated denial on empty-trash-for-drive

- nc chunked-upload MOVE assembly (#12): both branches now funnel through
  update_file_streaming_with_perms, whose internal fork enforces Update on
  the existing file OR Create on the parent folder / drive root. Pre-fix,
  the create branch went through plain upload_file_streaming with no
  authz.require — a Viewer on a shared drive could MKCOL → PUT chunks →
  MOVE and land a brand-new file. Error mapping switched to AppError::from
  so denials keep the graduated 403/404 shape.

- trash empty-for-drive: route through authz.require(Delete, Drive) instead
  of the bespoke drives_with_delete_for check + hardcoded not_found. Viewer
  now gets 403 (has Read), outsider stays 404 (no Read, anti-enum). Emits
  the standard authz.denied event with visibility field instead of the
  ad-hoc trash.empty_drive_rejected.

- tests/api/trash_per_drive.hurl: flip Viewer/Editor asserts 404 → 403;
  new Step 11b regression pin for finding #10 (Editor restore + delete
  attempts must 403 AND body must not contain "success":true — trips if
  the historical substring-match-on-"not found" hack ever comes back).
This commit is contained in:
Edouard Vanbelle
2026-07-17 00:29:10 +02:00
parent 8aa013d3ee
commit 20b1ea1a6a
3 changed files with 134 additions and 82 deletions
+19 -16
View File
@@ -662,22 +662,25 @@ impl TrashUseCase for TrashService {
async fn empty_trash_for_drive(&self, user_id: Uuid, drive_id: Uuid) -> Result<()> {
// Per-drive trash empty — the Drive group-by on `/trash` exposes
// this as a per-row affordance so multi-drive owners can clear
// one drive without touching the others. Refuses with
// `NotFound` (anti-enum) when the caller lacks Delete on the
// named drive — same shape as the user-facing drive listing
// would emit for an unknown id.
let allowed = self.drives_with_delete_for(user_id).await?;
if !allowed.contains(&drive_id) {
tracing::info!(
target: "audit",
event = "trash.empty_drive_rejected",
reason = "no_delete_on_drive",
user_id = %user_id,
drive_id = %drive_id,
"👮🏻‍♂️ refused per-drive empty — caller lacks Delete on this drive",
);
return Err(DomainError::not_found("Drive", drive_id.to_string()));
}
// one drive without touching the others.
//
// Route through `authz.require(Delete, Drive)` so the denial
// shape stays consistent with every other write verb: 403 when
// the caller has Read on the drive (viewer/editor holding no
// Delete), 404 when they don't (anti-enum). Before 2026-07-16
// this method rolled its own `drives_with_delete_for` check +
// hardcoded `NotFound` — that predated the graduated-denial
// engine change and returned 404 unconditionally even for a
// Viewer who could see the drive in `/api/drives`. The engine
// now emits `authz.denied` with `visibility="visible"|"hidden"`
// and the standard mapping renders it as 403 or 404.
self.authz
.require(
Subject::User(user_id),
Permission::Delete,
Resource::Drive(drive_id),
)
.await?;
info!("Emptying trash for drive {} (user {})", drive_id, user_id);
self.clear_trash_in(&[drive_id], user_id).await
}
+35 -60
View File
@@ -6,7 +6,7 @@ use axum::{
use std::sync::Arc;
use uuid::Uuid;
use crate::application::ports::file_ports::{FileRetrievalUseCase, FileUploadUseCase};
use crate::application::ports::file_ports::FileUploadUseCase;
use crate::application::ports::storage_ports::StorageUsagePort;
use crate::common::di::AppState;
use crate::common::mime_detect::filename_from_path;
@@ -402,8 +402,6 @@ async fn handle_assemble(
.map_err(|e| AppError::internal_error(format!("Failed to list chunks: {}", e)))?;
let upload_service = &state.applications.file_upload_service;
let file_service = &state.applications.file_retrieval_service;
let folder_service = &state.applications.folder_service;
// Path-based lookups below scope by `drive_id`. The NC session's
// chroot is always populated for path-scoped handlers (see
@@ -433,64 +431,41 @@ async fn handle_assemble(
.await?;
let content_type = ingested.content_type.clone();
// Check if file exists (update vs create).
let existing = file_service
.get_file_by_path(&internal_path, drive_id)
.await;
let etag: Option<String> = if existing.is_ok() {
let dto = upload_service
.update_file_streaming_with_perms(
&internal_path,
drive_id,
ingested.stored(),
&content_type,
oc_mtime,
user.id,
)
.await
.map_err(|e| AppError::internal_error(format!("Failed to update file: {}", e)))?;
Some(dto.etag)
} else {
// New-file branch: resolve the parent folder by path and register
// the file row against the already-ingested blob.
let (parent_sub, filename) = match dest_subpath.rsplit_once('/') {
Some((p, n)) => (p, n),
None => ("", dest_subpath.as_str()),
};
let parent_internal =
crate::interfaces::nextcloud::webdav_handler::nc_to_internal_path(chroot, parent_sub)?;
let parent_internal = parent_internal.trim_end_matches('/');
use crate::application::ports::folder_ports::FolderUseCase;
let parent_folder = match folder_service
.get_folder_by_path(parent_internal, drive_id)
.await
{
Ok(folder) => folder,
Err(e) => {
discard_ingested(&state.core.dedup_service, &ingested).await;
return Err(AppError::internal_error(format!(
"Parent folder lookup failed: {}",
e
)));
}
};
let dto = upload_service
.upload_file_streaming(
filename.to_string(),
Some(parent_folder.id),
content_type.to_string(),
ingested.stored(),
user.id,
)
.await
.map_err(|e| AppError::internal_error(format!("Failed to create file: {}", e)))?;
Some(dto.etag)
// AuthZ audit #12 (2026-07-12): the previous shape branched on
// file existence — `update_file_streaming_with_perms` on the
// overwrite path (correct), plain `upload_file_streaming` on
// the create path (NO `authz.require`). Viewer/Commenter on a
// shared drive could MKCOL → PUT chunks → MOVE and land a
// brand-new file, skipping the `Create`-on-parent-folder gate.
//
// `update_file_streaming_with_perms` handles both branches
// atomically: `Update` on the existing file OR `Create` on the
// parent folder / drive root (per the service's own internal
// fork). Funneling everything through the one method also
// deletes the duplicated parent-folder lookup that used to
// live here.
//
// AuthZ audit #2 (2026-07-12): route DomainError through
// `AppError::from` so authz denials keep the graduated 403/404
// shape instead of collapsing into 500.
let dto = match upload_service
.update_file_streaming_with_perms(
&internal_path,
drive_id,
ingested.stored(),
&content_type,
oc_mtime,
user.id,
)
.await
{
Ok(dto) => dto,
Err(e) => {
discard_ingested(&state.core.dedup_service, &ingested).await;
return Err(AppError::from(e));
}
};
let etag: Option<String> = Some(dto.etag);
// Cleanup session.
let _ = nc.chunked_uploads.cleanup(&user.username, upload_id).await;