From 0342bae300e0892abc4443aa46c1d18b29007eae Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 4 Jul 2026 23:57:06 +0200 Subject: [PATCH] security(nextcloud): add authz to PUT verb --- src/application/ports/file_ports.rs | 7 +- .../services/file_upload_service.rs | 87 ++++++++++++++++++- src/common/stubs.rs | 2 +- src/interfaces/api/handlers/webdav_handler.rs | 2 +- src/interfaces/api/handlers/wopi_handler.rs | 2 +- src/interfaces/nextcloud/uploads_handler.rs | 19 ++-- src/interfaces/nextcloud/webdav_handler.rs | 2 +- 7 files changed, 107 insertions(+), 14 deletions(-) diff --git a/src/application/ports/file_ports.rs b/src/application/ports/file_ports.rs index 454baebc..f4920106 100644 --- a/src/application/ports/file_ports.rs +++ b/src/application/ports/file_ports.rs @@ -75,7 +75,12 @@ pub trait FileUploadUseCase: Send + Sync + 'static { /// `updated_by` column reflects the principal that performed the /// PUT — not the file's existing owner (D2 shared drives let /// non-owners overwrite content). - async fn update_file_streaming( + /// `_with_perms` suffix (AGENTS.md AuthZ convention): the + /// implementation calls `authz.require(caller, Update, File(id))` + /// on the overwrite branch and `authz.require(caller, Create, + /// Folder|Drive(id))` on the new-file branch. Handlers just plumb + /// `caller_id` through — no protocol-layer authz. + async fn update_file_streaming_with_perms( &self, path: &str, drive_id: Uuid, diff --git a/src/application/services/file_upload_service.rs b/src/application/services/file_upload_service.rs index 37dba99b..a6ac8209 100644 --- a/src/application/services/file_upload_service.rs +++ b/src/application/services/file_upload_service.rs @@ -42,6 +42,16 @@ pub struct FileUploadService { /// `(file_id, blob_hash, content_type)`; the recording side needs the /// `caller_id` the service already has in hand. resource_access_hook: Option>, + /// ReBAC engine — enforces `Permission::Update` on + /// overwrite-existing and `Permission::Create` on new-file paths + /// inside `update_file_streaming_with_perms`. Optional at the + /// struct level for the minimal test constructors (`new`, + /// `new_with_read`) but the WebDAV/NC/WOPI put paths refuse + /// (fail-closed internal error) if this isn't wired. Set by + /// either `with_instant_upload` or `with_authorization` — both + /// stash the same Arc so DI callers wiring instant upload get + /// the streaming gate for free. + authorization: Option>, /// Dependencies of the instant-upload path /// (`create_file_from_owned_blob_with_perms`); `None` in minimal test /// wiring. @@ -66,6 +76,7 @@ impl FileUploadService { content_cache: None, file_lifecycle_hook: None, resource_access_hook: None, + authorization: None, instant_upload: None, } } @@ -82,18 +93,34 @@ impl FileUploadService { content_cache: None, file_lifecycle_hook: None, resource_access_hook: None, + authorization: None, instant_upload: None, } } + /// Wires the authorization engine used by + /// `update_file_streaming_with_perms` on the WebDAV / NC / WOPI + /// PUT path. Independent of `with_instant_upload` so callers can + /// enable the streaming gate without also opting into the + /// dedup-instant-upload check (test wiring, minimal deployments). + pub fn with_authorization(mut self, authz: Arc) -> Self { + self.authorization = Some(authz); + self + } + /// Wires the authorization engine, dedup index and quota service that /// power the instant-upload path. + /// + /// Also stashes the `authz` handle in `self.authorization` so + /// DI callers wiring instant upload get the streaming-put gate + /// for free — a single `Arc` clone, no behavioural coupling. pub fn with_instant_upload( mut self, authz: Arc, dedup: Arc, quota: Arc, ) -> Self { + self.authorization = Some(authz.clone()); self.instant_upload = Some(InstantUploadDeps { authz, dedup, @@ -424,7 +451,16 @@ impl FileUploadUseCase for FileUploadService { /// Swap the content of the file at `path` to an already-ingested blob, /// creating the file when it doesn't exist (WebDAV/NextCloud/WOPI PUT). - async fn update_file_streaming( + /// + /// AuthZ (post-Drive audit Round 2 fix): overwrite path requires + /// `Update` on the target file; new-file path requires `Create` + /// on the parent folder (or on the drive when writing at drive + /// root). Fail-closed if the engine wasn't wired — this method + /// is the last line of defence between a Viewer/Commenter drive + /// member and cross-tenant PUT. See + /// `docs/plan/authz_audit/nextcloud.md` and the sibling native + /// `/webdav/*` handler. + async fn update_file_streaming_with_perms( &self, path: &str, drive_id: Uuid, @@ -433,10 +469,33 @@ impl FileUploadUseCase for FileUploadService { modified_at: Option, caller_id: Uuid, ) -> Result { + let Some(authz) = &self.authorization else { + return Err(DomainError::internal_error( + "FileUpload", + "update_file_streaming_with_perms called without authorization engine wired", + )); + }; + // Try to find the existing file first if let Some(file_read) = &self.file_read && let Some(file) = file_read.find_file_by_path(path, drive_id).await? { + // Overwrite branch — caller must have `Update` on the + // target file. Denial routes through `require` → 404 + // (anti-enum, matches read-side shape). Before the D7 + // audit this whole branch ran unchecked; Viewer members + // of shared drives could PUT freely. + let file_uuid = Uuid::parse_str(file.id()).map_err(|_| { + DomainError::internal_error("FileUpload", "invalid file id from repository") + })?; + authz + .require( + Subject::User(caller_id), + Permission::Update, + Resource::File(file_uuid), + ) + .await?; + let file_id = file.id().to_string(); let (new_hash, updated_at) = self .file_write @@ -505,6 +564,32 @@ impl FileUploadUseCase for FileUploadService { None }; + // Create branch — caller must have `Create` on the parent + // scope. Two cases: + // * `parent_id.is_some()` → caller needs Create on the + // parent Folder resource. + // * `parent_id.is_none()` → the write lands at the drive + // root (either the path was single-segment, or the + // parent-folder lookup failed). We require Create on + // the Drive itself — bundled with owner/editor/contributor + // role_grants, refused for viewer/commenter. + let create_resource = match &parent_id { + Some(pid) => { + let uuid = Uuid::parse_str(pid).map_err(|_| { + DomainError::internal_error("FileUpload", "invalid parent folder id") + })?; + Resource::Folder(uuid) + } + None => Resource::Drive(drive_id), + }; + authz + .require( + Subject::User(caller_id), + Permission::Create, + create_resource, + ) + .await?; + let is_new_blob = blob.is_new_blob; let created = self .file_write diff --git a/src/common/stubs.rs b/src/common/stubs.rs index 2cde43f9..bdd15bf5 100644 --- a/src/common/stubs.rs +++ b/src/common/stubs.rs @@ -500,7 +500,7 @@ impl FileUploadUseCase for StubFileUploadUseCase { Ok(FileDto::default()) } - async fn update_file_streaming( + async fn update_file_streaming_with_perms( &self, _path: &str, _drive_id: Uuid, diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 1aac0907..27286d73 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -1756,7 +1756,7 @@ async fn handle_put( // internally via its `_with_perms` shape. let content_type = ingested.content_type.clone(); let result = file_upload_service - .update_file_streaming( + .update_file_streaming_with_perms( &path, drive_id, ingested.stored(), diff --git a/src/interfaces/api/handlers/wopi_handler.rs b/src/interfaces/api/handlers/wopi_handler.rs index 92147a51..ab4f98df 100644 --- a/src/interfaces/api/handlers/wopi_handler.rs +++ b/src/interfaces/api/handlers/wopi_handler.rs @@ -264,7 +264,7 @@ async fn put_file( .app_state .applications .file_upload_service - .update_file_streaming( + .update_file_streaming_with_perms( &file.path, drive_id, ingested.stored(), diff --git a/src/interfaces/nextcloud/uploads_handler.rs b/src/interfaces/nextcloud/uploads_handler.rs index f21a613a..414d8d15 100644 --- a/src/interfaces/nextcloud/uploads_handler.rs +++ b/src/interfaces/nextcloud/uploads_handler.rs @@ -367,12 +367,14 @@ async fn handle_assemble( let chroot = session.require_chroot()?; let drive_id = chroot.drive_id; - // TODO(D1): read the caller's default-drive root folder name from - // `drives.root_folder_id` instead of hardcoding "Personal". The - // constant is correct for every default personal drive provisioned - // by the D0 lifecycle hook, but secondary drives (M2 backfill from - // SQL-created sibling root folders) keep their original name. - let internal_path = format!("Personal/{}", dest_subpath.trim_matches('/')); + // Route through `nc_to_internal_path(chroot, …)` so the write + // lands under the caller's actual default-drive root (not the + // literal "Personal" folder). Post-D3 chroot resolution puts the + // correct FolderDto — including the drive's real root name — on + // the NcSession; secondary drives with SQL-provisioned sibling + // root names now work. + let internal_path = + crate::interfaces::nextcloud::webdav_handler::nc_to_internal_path(chroot, &dest_subpath)?; let filename = filename_from_path(&dest_subpath).to_string(); let ingested = ingest_stream_to_cas( @@ -393,7 +395,7 @@ async fn handle_assemble( let etag: Option = if existing.is_ok() { let dto = upload_service - .update_file_streaming( + .update_file_streaming_with_perms( &internal_path, drive_id, ingested.stored(), @@ -412,7 +414,8 @@ async fn handle_assemble( Some((p, n)) => (p, n), None => ("", dest_subpath.as_str()), }; - let parent_internal = format!("Personal/{}", parent_sub.trim_matches('/')); + 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; diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index 9e2382c9..a898f3ba 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -871,7 +871,7 @@ async fn handle_put( // Single streaming path — handles both update and create internally, // swapping the file row onto the already-ingested blob. let stored = upload_service - .update_file_streaming( + .update_file_streaming_with_perms( &internal_path, chroot.drive_id, ingested.stored(),