From f115fed5a656f7c9f86159b546d7bc25e51e83cd Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Fri, 3 Jul 2026 23:52:31 +0200 Subject: [PATCH 1/8] feat(drive): cleanup of useless owner_id --- .../services/file_upload_service.rs | 2 - src/domain/entities/file.rs | 22 ------ src/domain/entities/folder.rs | 77 ++----------------- .../pg/file_blob_read_repository.rs | 3 - .../pg/file_blob_write_repository.rs | 8 -- .../repositories/pg/folder_db_repository.rs | 6 -- 6 files changed, 7 insertions(+), 111 deletions(-) diff --git a/src/application/services/file_upload_service.rs b/src/application/services/file_upload_service.rs index 3218a5f3..37dba99b 100644 --- a/src/application/services/file_upload_service.rs +++ b/src/application/services/file_upload_service.rs @@ -296,7 +296,6 @@ impl FileUploadService { parts.folder_id, parts.created_at, updated_at as u64, - parts.owner_id, new_hash, ) .map_err(|e| DomainError::internal_error("FileUpload", format!("rebuild entity: {e}")))?; @@ -467,7 +466,6 @@ impl FileUploadUseCase for FileUploadService { parts.folder_id, parts.created_at, updated_at as u64, - parts.owner_id, new_hash, ) .map_err(|e| { diff --git a/src/domain/entities/file.rs b/src/domain/entities/file.rs index 9c9ba7ce..e4a65884 100644 --- a/src/domain/entities/file.rs +++ b/src/domain/entities/file.rs @@ -22,7 +22,6 @@ pub struct FileParts { pub folder_id: Option, pub created_at: u64, pub modified_at: u64, - pub owner_id: Option, /// BLAKE3 content hash. See [`File::content_hash`] for semantics. pub blob_hash: String, /// §14 provenance: original creator. See [`File::created_by`]. @@ -70,9 +69,6 @@ pub struct File { /// Last modification timestamp (seconds since UNIX epoch) modified_at: u64, - /// Owner user ID (from storage.files.user_id) - owner_id: Option, - /// BLAKE3 content hash. Stable across renames/moves, changes only /// when the file's content bytes change. Source of truth for both /// content-addressable storage and the HTTP ETag (via @@ -109,7 +105,6 @@ impl Default for File { folder_id: None, created_at: 0, modified_at: 0, - owner_id: None, blob_hash: String::new(), created_by: None, updated_by: None, @@ -150,7 +145,6 @@ impl File { folder_id, created_at: now, modified_at: now, - owner_id: None, blob_hash: String::new(), created_by: None, updated_by: None, @@ -184,7 +178,6 @@ impl File { folder_id: parent_id, created_at, modified_at, - owner_id: None, blob_hash: String::new(), created_by: None, updated_by: None, @@ -201,7 +194,6 @@ impl File { folder_id: Option, created_at: u64, modified_at: u64, - owner_id: Option, ) -> FileResult { Self::with_timestamps_and_blob_hash( id, @@ -212,7 +204,6 @@ impl File { folder_id, created_at, modified_at, - owner_id, String::new(), ) } @@ -227,7 +218,6 @@ impl File { folder_id: Option, created_at: u64, modified_at: u64, - owner_id: Option, blob_hash: String, ) -> FileResult { Self::with_timestamps_blob_hash_and_provenance( @@ -239,7 +229,6 @@ impl File { folder_id, created_at, modified_at, - owner_id, blob_hash, None, None, @@ -259,7 +248,6 @@ impl File { folder_id: Option, created_at: u64, modified_at: u64, - owner_id: Option, blob_hash: String, created_by: Option, updated_by: Option, @@ -282,7 +270,6 @@ impl File { folder_id, created_at, modified_at, - owner_id, blob_hash, created_by, updated_by, @@ -304,7 +291,6 @@ impl File { folder_id: self.folder_id, created_at: self.created_at, modified_at: self.modified_at, - owner_id: self.owner_id, blob_hash: self.blob_hash, created_by: self.created_by, updated_by: self.updated_by, @@ -407,10 +393,6 @@ impl File { self.modified_at } - pub fn owner_id(&self) -> Option { - self.owner_id - } - /// User that originally created this file (§14 provenance). /// `None` when the referenced user has been deleted /// (FK is `ON DELETE SET NULL`) or for stub/DTO entities. @@ -455,7 +437,6 @@ impl File { folder_id, created_at, modified_at, - owner_id: None, blob_hash: String::new(), // DTO round-trips don't carry provenance; callers needing // it must reload from the repository. @@ -607,7 +588,6 @@ mod tests { None, 1_000, 2_000, - None, "abcdef0123456789ZZZZZZZZ".to_string(), ) .unwrap(); @@ -632,7 +612,6 @@ mod tests { None, 1_000, 2_000, - None, "shorthash".to_string(), ) .unwrap(); @@ -655,7 +634,6 @@ mod tests { None, 1_000, 2_000, - None, "stable-content-hash".to_string(), ) .unwrap(); diff --git a/src/domain/entities/folder.rs b/src/domain/entities/folder.rs index 53242dc3..452609ae 100644 --- a/src/domain/entities/folder.rs +++ b/src/domain/entities/folder.rs @@ -25,10 +25,6 @@ pub struct Folder { /// Parent folder ID (None if it's a root folder) parent_id: Option, - /// Owner user ID — scopes folder visibility per user. - /// `None` only for legacy/stub folders; real folders always have an owner. - owner_id: Option, - /// Drive that owns this folder. Post-D0 every `storage.folders` row /// has `drive_id NOT NULL` (M3 migration). Path-based lookups scope /// by this axis (not by `user_id`, which is dropped in D7). @@ -76,7 +72,6 @@ impl Default for Folder { storage_path: StoragePath::from_string("/"), path_string: "/".to_string(), parent_id: None, - owner_id: None, drive_id: Uuid::nil(), created_at: 0, modified_at: 0, @@ -88,26 +83,20 @@ impl Default for Folder { } impl Folder { - /// Creates a new folder with validation + /// Creates a new folder with validation. + /// + /// In-memory constructor: callers that don't supply a `drive_id` + /// are by definition stub/legacy paths (tests, pre-D0 fixtures, + /// DTO round-trips). Real DB-backed folders flow through + /// [`Folder::with_timestamps_and_tree`] which propagates the + /// drive scope and §14 provenance from the row. pub fn new( id: String, name: String, storage_path: StoragePath, parent_id: Option, - ) -> FolderResult { - Self::new_with_owner(id, name, storage_path, parent_id, None) - } - - /// Creates a new folder with validation and an explicit owner. - pub fn new_with_owner( - id: String, - name: String, - storage_path: StoragePath, - parent_id: Option, - owner_id: Option, ) -> FolderResult { let name = normalize_storage_name(&name); - // Validate folder name if let Err(reason) = validate_storage_name(&name) { return Err(FolderError::InvalidFolderName(format!("{name}: {reason}"))); } @@ -117,7 +106,6 @@ impl Folder { .unwrap_or_default() .as_secs(); - // Store the path string for serialization compatibility let path_string = storage_path.to_string(); Ok(Self { @@ -126,17 +114,10 @@ impl Folder { storage_path, path_string, parent_id, - owner_id, - // In-memory constructor: callers that don't supply a - // drive_id are by definition stub/legacy paths (tests, - // pre-D0 fixtures, DTO round-trips). Real DB-backed - // folders flow through `with_timestamps_and_tree`. drive_id: Uuid::nil(), created_at: now, modified_at: now, tree_modified_at: now, - // Provenance is unknown for in-memory construction; the DB - // reconstruction path supplies real values. created_by: None, updated_by: None, }) @@ -160,34 +141,6 @@ impl Folder { name, storage_path, parent_id, - None, - Uuid::nil(), - created_at, - modified_at, - modified_at, - ) - } - - /// Creates a folder with specific timestamps and owner (legacy - /// constructor — `tree_modified_at` defaults to `modified_at`). - /// Prefer [`Folder::with_timestamps_and_tree`] for DB reconstruction - /// so the rollup ETag reflects descendant activity, not just this - /// row's own metadata. - pub fn with_timestamps_and_owner( - id: String, - name: String, - storage_path: StoragePath, - parent_id: Option, - owner_id: Option, - created_at: u64, - modified_at: u64, - ) -> FolderResult { - Self::with_timestamps_and_tree( - id, - name, - storage_path, - parent_id, - owner_id, Uuid::nil(), created_at, modified_at, @@ -209,7 +162,6 @@ impl Folder { name: String, storage_path: StoragePath, parent_id: Option, - owner_id: Option, drive_id: Uuid, created_at: u64, modified_at: u64, @@ -220,7 +172,6 @@ impl Folder { name, storage_path, parent_id, - owner_id, drive_id, created_at, modified_at, @@ -239,7 +190,6 @@ impl Folder { name: String, storage_path: StoragePath, parent_id: Option, - owner_id: Option, drive_id: Uuid, created_at: u64, modified_at: u64, @@ -260,7 +210,6 @@ impl Folder { storage_path, path_string, parent_id, - owner_id, drive_id, created_at, modified_at, @@ -299,10 +248,6 @@ impl Folder { self.modified_at } - pub fn owner_id(&self) -> Option { - self.owner_id - } - /// Drive that owns this folder. Path-based lookups scope by /// this axis (post-D0 invariant: `storage.folders.drive_id` /// is `NOT NULL`). @@ -412,7 +357,6 @@ impl Folder { storage_path, path_string: path, parent_id, - owner_id: None, // DTO round-trips lose drive_id (FolderDto carries it, // but the legacy `from_dto` signature predates this // change). Callers that need real scoping must reload @@ -460,7 +404,6 @@ impl Folder { storage_path: new_storage_path, path_string: new_path_string, parent_id: self.parent_id.clone(), - owner_id: self.owner_id, drive_id: self.drive_id, created_at: self.created_at, modified_at: now, @@ -501,7 +444,6 @@ impl Folder { storage_path: new_storage_path, path_string: new_path_string, parent_id, - owner_id: self.owner_id, drive_id: self.drive_id, created_at: self.created_at, modified_at: now, @@ -593,7 +535,6 @@ mod tests { "folder".to_string(), StoragePath::from_string("/folder"), None, - None, Uuid::nil(), 1_000, 2_000, @@ -615,7 +556,6 @@ mod tests { "a".to_string(), StoragePath::from_string("/a"), None, - None, Uuid::nil(), 0, 0, @@ -627,7 +567,6 @@ mod tests { "b".to_string(), StoragePath::from_string("/b"), None, - None, Uuid::nil(), 0, 0, @@ -650,7 +589,6 @@ mod tests { "folder".to_string(), StoragePath::from_string("/folder"), None, - None, Uuid::nil(), 1_000, 2_000, @@ -662,7 +600,6 @@ mod tests { "folder".to_string(), StoragePath::from_string("/folder"), None, - None, Uuid::nil(), 1_000, 2_000, diff --git a/src/infrastructure/repositories/pg/file_blob_read_repository.rs b/src/infrastructure/repositories/pg/file_blob_read_repository.rs index 0aa2ea83..2de4393e 100644 --- a/src/infrastructure/repositories/pg/file_blob_read_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_read_repository.rs @@ -397,8 +397,6 @@ impl FileBlobReadRepository { } } - /// Post-D7-step-6: `storage.files.user_id` dropped; the entity's - /// legacy `user_id` field is populated with `None` here. #[allow(clippy::too_many_arguments)] fn row_to_file( id: String, @@ -423,7 +421,6 @@ impl FileBlobReadRepository { folder_id, created_at as u64, modified_at as u64, - None, // Post-D7: `files.user_id` column dropped. blob_hash, created_by, updated_by, diff --git a/src/infrastructure/repositories/pg/file_blob_write_repository.rs b/src/infrastructure/repositories/pg/file_blob_write_repository.rs index 3de28382..e77be731 100644 --- a/src/infrastructure/repositories/pg/file_blob_write_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_write_repository.rs @@ -104,7 +104,6 @@ impl FileBlobWriteRepository { mime_type: String, created_at: i64, modified_at: i64, - owner_id: Option, blob_hash: String, created_by: Option, updated_by: Option, @@ -119,7 +118,6 @@ impl FileBlobWriteRepository { folder_id, created_at as u64, modified_at as u64, - owner_id, blob_hash, created_by, updated_by, @@ -399,7 +397,6 @@ impl FileBlobWriteRepository { content_type, created_at, updated_at, - None, // Post-D7: `files.user_id` no longer written on new rows. blob_hash.to_string(), created_by, updated_by, @@ -477,7 +474,6 @@ impl FileBlobWriteRepository { mime_type, created_at, updated_at, - None, blob_hash.to_string(), created_by, updated_by, @@ -562,7 +558,6 @@ impl FileWritePort for FileBlobWriteRepository { row.4, row.5, row.6, - None, String::new(), row.7, row.8, @@ -710,7 +705,6 @@ impl FileWritePort for FileBlobWriteRepository { row.4, row.5, row.6, - None, row.7, row.8, row.9, @@ -773,7 +767,6 @@ impl FileWritePort for FileBlobWriteRepository { row.4, row.5, row.6, - None, String::new(), row.7, row.8, @@ -874,7 +867,6 @@ impl FileWritePort for FileBlobWriteRepository { content_type, row.1, row.2, - None, // Post-D7: `files.user_id` no longer written on new rows. String::new(), row.3, row.4, diff --git a/src/infrastructure/repositories/pg/folder_db_repository.rs b/src/infrastructure/repositories/pg/folder_db_repository.rs index 7cd92f8a..4dc65f9d 100644 --- a/src/infrastructure/repositories/pg/folder_db_repository.rs +++ b/src/infrastructure/repositories/pg/folder_db_repository.rs @@ -129,11 +129,6 @@ impl FolderDbRepository { /// extra queries needed. `created_by` / `updated_by` carry the /// §14 provenance signal through the entity layer; both are /// `Option` because the FK is `ON DELETE SET NULL`. - /// - /// Post-D7-step-6: the `storage.folders.user_id` column is gone; - /// the entity's legacy `user_id` field is populated with `None` - /// at construction time (removed in the follow-up entity - /// cleanup PR). #[allow(clippy::too_many_arguments)] fn row_to_folder( id: String, @@ -153,7 +148,6 @@ impl FolderDbRepository { name, storage_path, parent_id, - None, // Post-D7: `folders.user_id` column dropped. drive_id, created_at as u64, modified_at as u64, From cf5423b7232dc3a3ffcc46e3dfb2fb1e3e3af4fd Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 4 Jul 2026 19:33:02 +0200 Subject: [PATCH 2/8] security(public link): require share permission issue was: a user can reshare publicly a resource on owner revocation, the attacker keep it's own share request now share permission --- src/application/services/share_service.rs | 24 ++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/application/services/share_service.rs b/src/application/services/share_service.rs index 28ea3a27..69c56039 100644 --- a/src/application/services/share_service.rs +++ b/src/application/services/share_service.rs @@ -6,7 +6,7 @@ use uuid::Uuid; use crate::domain::repositories::drive_repository::DriveRepository; use crate::domain::repositories::folder_repository::FolderRepository; -use crate::domain::services::authorization::{Resource, Role, Subject}; +use crate::domain::services::authorization::{Permission, Resource, Role, Subject}; use crate::infrastructure::repositories::pg::DrivePgRepository; use crate::infrastructure::repositories::pg::SharePgRepository; use crate::infrastructure::repositories::pg::file_blob_read_repository::FileBlobReadRepository; @@ -243,6 +243,28 @@ impl ShareUseCase for ShareService { self.verify_item_exists(&dto.item_id, &item_type).await?; + // AuthZ: only callers with `Share` on the resource may mint a + // public link. Without this gate, an ex-Viewer who kept a + // guessed UUID could launder a temporary read into a + // permanent anonymous URL that survives their own grant + // revocation. `Permission::Share` is bundled with the + // `owner` and `editor` role_grants only. `require` returns + // `not_found` on denial (anti-enum, matches the shape used + // by every other share route). See `docs/plan/authz_audit/`. + let item_uuid_for_authz = Uuid::parse_str(&dto.item_id) + .map_err(|_| ShareServiceError::Validation("Invalid item UUID".to_string()))?; + let resource_for_authz = match item_type { + ShareItemType::File => Resource::File(item_uuid_for_authz), + ShareItemType::Folder => Resource::Folder(item_uuid_for_authz), + }; + self.authorization + .require( + Subject::User(user_id), + Permission::Share, + resource_for_authz, + ) + .await?; + // D5: `forbid_public_links` policy gate. The drive owner can // disable anonymous-link creation on every resource in their // drive without per-resource intervention. Lookup is one JOIN From 2cda8e7e224f6337e0e25b2fb4b49074a040b68f Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 4 Jul 2026 19:36:17 +0200 Subject: [PATCH 3/8] security(favorite,recent): ensure read permission --- src/application/services/favorites_service.rs | 57 ++++++++----- src/application/services/recent_service.rs | 37 ++++++--- src/common/di.rs | 80 +++++++++++-------- src/domain/services/authorization.rs | 28 +++++++ 4 files changed, 138 insertions(+), 64 deletions(-) diff --git a/src/application/services/favorites_service.rs b/src/application/services/favorites_service.rs index e8970b42..ac372436 100644 --- a/src/application/services/favorites_service.rs +++ b/src/application/services/favorites_service.rs @@ -9,10 +9,12 @@ use crate::application::dtos::favorites_dto::{ BatchFavoritesResult, BatchFavoritesStats, FavoriteItemDto, FavoriteResourceRow, FavoritesCursor, }; +use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::favorites_ports::{FavoritesRepositoryPort, FavoritesUseCase}; -use crate::common::errors::{DomainError, ErrorKind, Result}; -use crate::domain::services::authorization::ResourceKind; +use crate::common::errors::Result; +use crate::domain::services::authorization::{Permission, Resource, ResourceKind, Subject}; use crate::infrastructure::repositories::pg::FavoritesPgRepository; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; /// Implementation of the FavoritesUseCase for managing user favorites. /// @@ -20,12 +22,22 @@ use crate::infrastructure::repositories::pg::FavoritesPgRepository; /// accessing the database directly, following hexagonal architecture. pub struct FavoritesService { repo: Arc, + /// ReBAC engine — enforces `Permission::Read` on the referenced + /// file/folder before enrolling it into a user's favorites. + /// Without this gate the write path is an information oracle: + /// listing endpoints JOIN back to `storage.files/folders` and + /// return name/mime/size/drive_id for any UUID the caller was + /// able to enroll. See `docs/plan/authz_audit/rest_storage.md`. + authorization: Arc, } impl FavoritesService { /// Create a new FavoritesService with the given repository port - pub fn new(repo: Arc) -> Self { - Self { repo } + pub fn new(repo: Arc, authorization: Arc) -> Self { + Self { + repo, + authorization, + } } /// Subset of `(item_id, item_type)` pairs the user has favorited — used to @@ -60,13 +72,15 @@ impl FavoritesUseCase for FavoritesService { item_type, item_id, user_id ); - if item_type != "file" && item_type != "folder" { - return Err(DomainError::new( - ErrorKind::InvalidInput, - "Favorites", - "Item type must be 'file' or 'folder'", - )); - } + // AuthZ pre-write: caller must have Read on the referenced + // resource. Denial routes through `require` → NotFound + // (anti-enum, matches the listing shape) + `authz.denied` + // audit line. Without this gate the write path was an + // information oracle over the whole tenant. + let resource = Resource::parse(item_type, item_id)?; + self.authorization + .require(Subject::User(user_id), Permission::Read, resource) + .await?; self.repo.add_favorite(user_id, item_id, item_type).await?; info!( @@ -125,18 +139,17 @@ impl FavoritesUseCase for FavoritesService { user_id ); - // Validate all item types + // AuthZ pre-write: caller must have Read on every referenced + // resource. Fail the whole batch on the first denial so the + // response shape doesn't tell an attacker which items were + // valid (partial success would leak the same oracle we + // closed on the single-item path). See + // `docs/plan/authz_audit/rest_storage.md`. for (item_id, item_type) in items { - if item_type != "file" && item_type != "folder" { - return Err(DomainError::new( - ErrorKind::InvalidInput, - "Favorites", - format!( - "Item type must be 'file' or 'folder' for item '{}'", - item_id - ), - )); - } + let resource = Resource::parse(item_type, item_id)?; + self.authorization + .require(Subject::User(user_id), Permission::Read, resource) + .await?; } let requested = items.len(); diff --git a/src/application/services/recent_service.rs b/src/application/services/recent_service.rs index 17f0bd2f..54482606 100644 --- a/src/application/services/recent_service.rs +++ b/src/application/services/recent_service.rs @@ -1,10 +1,12 @@ use crate::application::dtos::cursor::PageCursor; use crate::application::dtos::recent_dto::{RecentCursor, RecentItemDto, RecentResourceRow}; +use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::recent_ports::{RecentItemsRepositoryPort, RecentItemsUseCase}; use crate::application::ports::resource_access_hook::ResourceAccessHook; -use crate::common::errors::{DomainError, ErrorKind, Result}; -use crate::domain::services::authorization::ResourceKind; +use crate::common::errors::Result; +use crate::domain::services::authorization::{Permission, Resource, ResourceKind, Subject}; use crate::infrastructure::repositories::pg::RecentItemsPgRepository; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; use std::sync::{Arc, OnceLock}; use tracing::info; use uuid::Uuid; @@ -16,6 +18,13 @@ use uuid::Uuid; pub struct RecentService { repo: Arc, max_recent_items: i32, + /// ReBAC engine — enforces `Permission::Read` on the referenced + /// file/folder before enrolling it into a user's Recent list. + /// The listing side JOINs back to `storage.files/folders` and + /// returns name/mime/size/drive_id for any enrolled UUID, so + /// the write path is an information oracle without this gate. + /// See `docs/plan/authz_audit/rest_storage.md`. + authorization: Arc, /// Set after construction via [`Self::set_resource_access_hook`]. /// The hook is built FROM this service (it wraps an `Arc`), so /// we can't take it as a constructor arg without circular ownership; @@ -28,10 +37,15 @@ pub struct RecentService { impl RecentService { /// Create a new recent items service - pub fn new(repo: Arc, max_recent_items: i32) -> Self { + pub fn new( + repo: Arc, + authorization: Arc, + max_recent_items: i32, + ) -> Self { Self { repo, max_recent_items: max_recent_items.clamp(1, 100), + authorization, resource_access_hook: OnceLock::new(), } } @@ -87,13 +101,16 @@ impl RecentItemsUseCase for RecentService { item_type, item_id, user_id ); - if item_type != "file" && item_type != "folder" { - return Err(DomainError::new( - ErrorKind::InvalidInput, - "RecentItems", - "Item type must be 'file' or 'folder'", - )); - } + // AuthZ pre-write: caller must have Read on the referenced + // resource. Denial routes through `require` → NotFound + // (anti-enum) + `authz.denied` audit line. Without this + // gate the write path was an information oracle over the + // whole tenant via the listing endpoint's JOIN back to + // storage.files/folders. + let resource = Resource::parse(item_type, item_id)?; + self.authorization + .require(Subject::User(user_id), Permission::Read, resource) + .await?; self.repo.upsert_access(user_id, item_id, item_type).await?; self.repo.prune(user_id, self.max_recent_items).await?; diff --git a/src/common/di.rs b/src/common/di.rs index a7547134..8ac3a379 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -889,23 +889,37 @@ impl AppServiceFactory { Some(service) } - /// Creates the favorites service (requires database) - pub fn create_favorites_service(&self, db_pool: &Arc) -> Arc { + /// Creates the favorites service (requires database + authz engine + /// for the Read gate on `add_to_favorites` — see the post-Drive + /// AuthZ audit). + pub fn create_favorites_service( + &self, + db_pool: &Arc, + authorization: &Arc, + ) -> Arc { let repo = Arc::new( crate::infrastructure::repositories::pg::FavoritesPgRepository::new(db_pool.clone()), ); - let service = Arc::new(FavoritesService::new(repo)); + let service = Arc::new(FavoritesService::new(repo, authorization.clone())); tracing::info!("Favorites service initialized"); service } - /// Creates the recent items service (requires database) - pub fn create_recent_service(&self, db_pool: &Arc) -> Arc { + /// Creates the recent items service (requires database + authz + /// engine for the Read gate on `record_item_access` — see the + /// post-Drive AuthZ audit). + pub fn create_recent_service( + &self, + db_pool: &Arc, + authorization: &Arc, + ) -> Arc { let repo = Arc::new( crate::infrastructure::repositories::pg::RecentItemsPgRepository::new(db_pool.clone()), ); let service = Arc::new(RecentService::new( - repo, 50, // Maximum recent items per user + repo, + authorization.clone(), + 50, // Maximum recent items per user )); tracing::info!("Recent items service initialized"); service @@ -1161,31 +1175,6 @@ impl AppServiceFactory { let pool = Arc::new(pools.primary); let maintenance_pool = Arc::new(pools.maintenance); - // Recent service + recording hook are built up-front so the - // hook can be threaded into `create_application_services` below. - // The file services hold the hook directly so every authorised - // `_with_perms` read/write fires into `auth.user_recent_files` - // without per-handler wiring. Reordering vs the legacy in-block - // creation (further down) is safe: `create_recent_service` only - // needs `pool`, which is already in scope. - // - // The back-edge `recent_service_eager.set_resource_access_hook` - // closes the loop so the clear/remove handlers can drop the - // hook's in-memory throttle entries — without it a freshly - // cleared Recent list refuses to re-record the same file for a - // full TTL window, surfacing as "I cleared, opened the file, - // and Recent is still empty" (caught by tests/api/recent.hurl - // step 8). - let recent_service_eager = self.create_recent_service(&pool); - let resource_access_hook: Arc< - dyn crate::application::ports::resource_access_hook::ResourceAccessHook, - > = Arc::new( - crate::infrastructure::services::recent_recording_hook::RecentRecordingHook::new( - recent_service_eager.clone(), - ), - ); - recent_service_eager.set_resource_access_hook(resource_access_hook.clone()); - // 1. Core services (PgPool needed for DedupService index) let core = self.create_core_services(&pool, &maintenance_pool).await?; @@ -1196,6 +1185,10 @@ impl AppServiceFactory { // because services hold an Arc for ReBAC checks. // SubjectGroupPgRepository is constructed here too so the engine can // expand a user's transitive group set on cache misses. + // + // Moved above the eager recent-service build so `create_recent_service` + // can receive an `Arc` — the Read gate on + // `record_item_access` (post-Drive AuthZ audit fix) needs it. let subject_group_repo = Arc::new( crate::infrastructure::repositories::pg::SubjectGroupPgRepository::new(pool.clone()), ); @@ -1206,6 +1199,29 @@ impl AppServiceFactory { subject_group_repo.clone(), ); + // Recent service + recording hook are built up-front so the + // hook can be threaded into `create_application_services` below. + // The file services hold the hook directly so every authorised + // `_with_perms` read/write fires into `auth.user_recent_files` + // without per-handler wiring. + // + // The back-edge `recent_service_eager.set_resource_access_hook` + // closes the loop so the clear/remove handlers can drop the + // hook's in-memory throttle entries — without it a freshly + // cleared Recent list refuses to re-record the same file for a + // full TTL window, surfacing as "I cleared, opened the file, + // and Recent is still empty" (caught by tests/api/recent.hurl + // step 8). + let recent_service_eager = self.create_recent_service(&pool, &authorization); + let resource_access_hook: Arc< + dyn crate::application::ports::resource_access_hook::ResourceAccessHook, + > = Arc::new( + crate::infrastructure::services::recent_recording_hook::RecentRecordingHook::new( + recent_service_eager.clone(), + ), + ); + recent_service_eager.set_resource_access_hook(resource_access_hook.clone()); + // Drive repository — needed both by the lifecycle hook (when auth // is enabled) and by `GET /api/drives` on the final `AppState`, // so declared at the outer scope. @@ -1279,7 +1295,7 @@ impl AppServiceFactory { > = None; { - let favs = self.create_favorites_service(&pool); + let favs = self.create_favorites_service(&pool, &authorization); favorites_service = Some(favs.clone()); apps.favorites_service = Some(favs); diff --git a/src/domain/services/authorization.rs b/src/domain/services/authorization.rs index c0502dda..0e9a6a3d 100644 --- a/src/domain/services/authorization.rs +++ b/src/domain/services/authorization.rs @@ -118,6 +118,34 @@ impl Resource { _ => None, } } + + /// Parse `(item_type, item_id)` from an API-facing pair of strings + /// (favorites, recent, batch endpoints all take this shape). + /// Combines UUID parse + type mapping so callers stay one-line and + /// error shapes are identical across surfaces. Returns + /// `DomainError::new(InvalidInput, …)` on malformed input; callers + /// that need the anti-enum 404 shape do that separately by feeding + /// the parsed `Resource` into `authz.require(...)`. + pub fn parse( + item_type: &str, + item_id: &str, + ) -> Result { + use crate::common::errors::{DomainError, ErrorKind}; + let uuid = Uuid::parse_str(item_id).map_err(|_| { + DomainError::new( + ErrorKind::InvalidInput, + "Resource", + format!("Invalid item UUID '{item_id}'"), + ) + })?; + Self::from_parts(item_type, uuid).ok_or_else(|| { + DomainError::new( + ErrorKind::InvalidInput, + "Resource", + format!("Unsupported item type '{item_type}'"), + ) + }) + } } impl fmt::Display for Resource { From b95e740b2f31d12f6aeff1d868f9227031b34d58 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 4 Jul 2026 23:31:10 +0200 Subject: [PATCH 4/8] security(music): ensure read permission via authz --- src/application/services/music_service.rs | 34 +++++++- src/common/di.rs | 2 +- .../api/handlers/favorites_handler.rs | 50 +++++------- src/interfaces/api/handlers/recent_handler.rs | 42 +++------- tests/api/favorites.hurl | 74 +++++++++++++++++ tests/api/public_shares.hurl | 79 +++++++++++++++++++ tests/api/recent.hurl | 64 +++++++++++++++ 7 files changed, 280 insertions(+), 65 deletions(-) diff --git a/src/application/services/music_service.rs b/src/application/services/music_service.rs index 78ce6757..d4d4456a 100644 --- a/src/application/services/music_service.rs +++ b/src/application/services/music_service.rs @@ -5,17 +5,31 @@ use crate::application::dtos::playlist_dto::{ AddTracksDto, AudioMetadataDto, CreatePlaylistDto, PlaylistDto, PlaylistItemDto, PlaylistQueryDto, PlaylistShareInfoDto, ReorderTracksDto, SharePlaylistDto, UpdatePlaylistDto, }; +use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::music_ports::{MusicStoragePort, MusicUseCase}; use crate::common::errors::{DomainError, ErrorKind}; +use crate::domain::services::authorization::{Permission, Resource, Subject}; use crate::infrastructure::adapters::music_storage_adapter::MusicStorageAdapter; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; pub struct MusicService { storage: Arc, + /// ReBAC engine — Round 1 fix from `docs/plan/authz_audit/`. + /// Currently used ONLY by `get_audio_metadata` to close the + /// cross-tenant IDOR (`_user_id: Uuid` was deliberately unused). + /// The full engine rewrite (Round 3 — `Resource::Playlist` + + /// authz.require on every playlist verb) is a separate PR; + /// don't extend the bespoke `user_has_access` / `user_can_write` + /// pattern to new methods, use `require` here instead. + authorization: Arc, } impl MusicService { - pub fn new(storage: Arc) -> Self { - Self { storage } + pub fn new(storage: Arc, authorization: Arc) -> Self { + Self { + storage, + authorization, + } } } @@ -375,10 +389,24 @@ impl MusicUseCase for MusicService { async fn get_audio_metadata( &self, file_id: &str, - _user_id: Uuid, + caller_id: Uuid, ) -> Result, DomainError> { let file_uuid = Uuid::parse_str(file_id) .map_err(|_| DomainError::new(ErrorKind::InvalidInput, "Music", "Invalid file ID"))?; + // AuthZ pre-read: caller must have `Read` on the underlying + // audio file. Before this check the endpoint returned + // metadata for any known file id (cross-tenant IDOR — the + // `_user_id` parameter was deliberately unused). `require` + // returns 404 on denial to match the anti-enum shape used + // everywhere else. Post-Drive AuthZ audit fix (Round 1 + // BLOCKER — `docs/plan/authz_audit/rest_storage.md`). + self.authorization + .require( + Subject::User(caller_id), + Permission::Read, + Resource::File(file_uuid), + ) + .await?; self.storage.get_audio_metadata(&file_uuid).await } } diff --git a/src/common/di.rs b/src/common/di.rs index 8ac3a379..959ad7c2 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -1865,7 +1865,7 @@ impl AppServiceFactory { audio_metadata_repo, ), ); - let music_svc = Arc::new(MusicService::new(music_storage)); + let music_svc = Arc::new(MusicService::new(music_storage, authorization.clone())); app_state.music_service = Some(music_svc); tracing::info!("Music service initialized"); } diff --git a/src/interfaces/api/handlers/favorites_handler.rs b/src/interfaces/api/handlers/favorites_handler.rs index 72b97a8b..cb887a58 100644 --- a/src/interfaces/api/handlers/favorites_handler.rs +++ b/src/interfaces/api/handlers/favorites_handler.rs @@ -6,7 +6,7 @@ use axum::{ }; use serde::Deserialize; use std::sync::Arc; -use tracing::{error, info}; +use tracing::info; use utoipa::ToSchema; use crate::application::dtos::display_helpers::{ @@ -66,7 +66,8 @@ pub async fn add_favorite( Json(serde_json::json!({ "error": "Item type must be 'file' or 'folder'" })), - ); + ) + .into_response(); } match favorites_service @@ -81,16 +82,14 @@ pub async fn add_favorite( "message": "Item added to favorites" })), ) + .into_response() } - Err(err) => { - error!("Error adding to favorites: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to add to favorites" - })), - ) - } + // Route through AppError so the `DomainError::kind` maps to the + // right status code (NotFound → 404 anti-enum for the pre-write + // authz gate, InvalidInput → 400 for a malformed UUID, etc.). + // A hardcoded 500 here would mask the 404 the Round 1 AuthZ + // fix relies on. + Err(err) => AppError::from(err).into_response(), } } @@ -129,6 +128,7 @@ pub async fn remove_favorite( "message": "Item removed from favorites" })), ) + .into_response() } else { info!("Item {} '{}' was not in favorites", item_type, item_id); ( @@ -137,17 +137,12 @@ pub async fn remove_favorite( "message": "Item was not in favorites" })), ) + .into_response() } } - Err(err) => { - error!("Error removing from favorites: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to remove from favorites" - })), - ) - } + // Same rationale as `add_favorite` — preserve DomainError→HTTP + // status mapping instead of collapsing every error to 500. + Err(err) => AppError::from(err).into_response(), } } @@ -347,15 +342,10 @@ pub async fn batch_add_favorites( ); (StatusCode::OK, Json(serde_json::json!(result))).into_response() } - Err(err) => { - error!("Error in batch add favorites: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to batch add favorites" - })), - ) - .into_response() - } + // Preserve DomainError→HTTP status mapping — the Round 1 + // AuthZ fix relies on a per-item NotFound propagating out + // of the batch. A hardcoded 500 would mask the 404 that + // signals a cross-tenant probe. + Err(err) => AppError::from(err).into_response(), } } diff --git a/src/interfaces/api/handlers/recent_handler.rs b/src/interfaces/api/handlers/recent_handler.rs index 3878e783..690548d7 100644 --- a/src/interfaces/api/handlers/recent_handler.rs +++ b/src/interfaces/api/handlers/recent_handler.rs @@ -5,7 +5,7 @@ use axum::{ response::IntoResponse, }; use std::sync::Arc; -use tracing::{error, info}; +use tracing::info; use crate::application::dtos::display_helpers::{ category_for, format_file_size, icon_class_for, icon_special_class_for, @@ -70,16 +70,10 @@ pub async fn record_item_access( ) .into_response() } - Err(err) => { - error!("Error recording access in recents: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to record access" - })), - ) - .into_response() - } + // Preserve DomainError→HTTP status mapping — the Round 1 + // AuthZ fix relies on the NotFound from `authz.require` + // propagating as 404 (anti-enum), not being masked as 500. + Err(err) => AppError::from(err).into_response(), } } @@ -130,16 +124,9 @@ pub async fn remove_from_recent( .into_response() } } - Err(err) => { - error!("Error removing from recents: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to remove from recents" - })), - ) - .into_response() - } + // Same rationale as `record_item_access` — preserve the + // DomainError→HTTP mapping instead of collapsing to 500. + Err(err) => AppError::from(err).into_response(), } } @@ -170,16 +157,9 @@ pub async fn clear_recent_items( ) .into_response() } - Err(err) => { - error!("Error clearing recent items: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to clear recent items" - })), - ) - .into_response() - } + // Same rationale as `record_item_access` — preserve the + // DomainError→HTTP mapping instead of collapsing to 500. + Err(err) => AppError::from(err).into_response(), } } diff --git a/tests/api/favorites.hurl b/tests/api/favorites.hurl index 6c8d8d5f..d8609ab8 100644 --- a/tests/api/favorites.hurl +++ b/tests/api/favorites.hurl @@ -159,3 +159,77 @@ Authorization: Bearer {{token}} HTTP 200 [Asserts] jsonpath "$.items" count == 0 + + +# ───────────────────────────────────────────────────────────── +# Step 12 — Cross-tenant regression (post-Drive AuthZ audit, +# Round 1 HIGH). Before this fix, `POST /api/favorites/…` +# accepted any UUID and enrolled it; the listing endpoint +# then JOINed back to storage.files/folders and returned +# name/mime/size/drive_id for anything the caller had +# managed to add — an information oracle over the whole +# tenant. Now the write path calls `authz.require(Read, …)` +# per item; a caller with no grant gets 404 (anti-enum) +# + `authz.denied` audit line. See +# `docs/plan/authz_audit/rest_storage.md`. +# ───────────────────────────────────────────────────────────── + +# Create a second, unprivileged user. Idempotent: `HTTP *` accepts +# either 201 (first run) or 409 (subsequent runs). The login below +# is the actual precondition — if it succeeds we know the user +# exists with the expected password. +POST {{base_url}}/api/admin/users +Authorization: Bearer {{token}} +Content-Type: application/json +{ "username": "fav_mallory", "password": "FavMalloryPassword1!", "email": "fav_mallory@example.com", "role": "user" } + +HTTP * + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "fav_mallory", "password": "FavMalloryPassword1!" } + +HTTP 200 +[Captures] +mallory_token: jsonpath "$.access_token" + + +# Step 12a — Single-add on admin's file: 404 (anti-enum shape). +POST {{base_url}}/api/favorites/file/{{file_id}} +Authorization: Bearer {{mallory_token}} + +HTTP 404 + + +# Step 12b — Single-add on admin's folder: 404. +POST {{base_url}}/api/favorites/folder/{{test1_id}} +Authorization: Bearer {{mallory_token}} + +HTTP 404 + + +# Step 12c — Batch: must fail wholesale on the first denial. A partial +# success would still leak "which items are valid" — the same +# oracle we're closing. +POST {{base_url}}/api/favorites/batch +Authorization: Bearer {{mallory_token}} +Content-Type: application/json +{ + "items": [ + { "item_id": "{{file_id}}", "item_type": "file" }, + { "item_id": "{{test1_id}}", "item_type": "folder" } + ] +} + +HTTP 404 + + +# Step 12d — Mallory's favorites list is EMPTY — no partial success +# slipped through. +GET {{base_url}}/api/favorites/resources +Authorization: Bearer {{mallory_token}} + +HTTP 200 +[Asserts] +jsonpath "$.items" count == 0 diff --git a/tests/api/public_shares.hurl b/tests/api/public_shares.hurl index 81b02edc..641f2073 100644 --- a/tests/api/public_shares.hurl +++ b/tests/api/public_shares.hurl @@ -274,6 +274,85 @@ status >= 400 status < 500 +# ───────────────────────────────────────────────────────────── +# 14b — Viewer-laundering regression (post-Drive AuthZ audit, +# Round 1 HIGH). Before the fix, `POST /api/shares` checked +# only "does the item exist" — any authenticated user who +# could name the UUID could mint a public Viewer link, +# laundering read access into a permanent anonymous URL +# that survived their own grant revocation. Now the +# service calls `authz.require(Share, resource)` before +# minting the token; a caller without `Share` +# (Viewer/Commenter/Contributor/no-grant-at-all) gets 404 +# (anti-enum) + `authz.denied` audit line. See +# `docs/plan/authz_audit/admin_membership.md`. +# +# We test the strongest form: an unrelated user with no +# grant at all. The intermediate case (Viewer with Read +# but not Share) is covered by the same code path — Share +# is bundled only with owner/editor role_grants. +# ───────────────────────────────────────────────────────────── + +# Create/lookup the attacker. Idempotent: `HTTP *` accepts either +# 201 (first run) or 409 (subsequent runs). Login below is the real +# precondition. +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "username": "sh_mallory", "password": "ShMalloryPassword1!", "email": "sh_mallory@example.com", "role": "user" } + +HTTP * + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "sh_mallory", "password": "ShMalloryPassword1!" } + +HTTP 200 +[Captures] +mallory_token: jsonpath "$.access_token" + + +# Step 14b.i — Mallory tries to mint a public share on admin's +# folder: 404 (anti-enum). No token appears in the +# response body. +POST {{base_url}}/api/shares +Authorization: Bearer {{mallory_token}} +Content-Type: application/json +{ + "item_id": "{{share_folder_id}}", + "item_name": "public-share-test", + "item_type": "folder" +} + +HTTP 404 + + +# Step 14b.ii — Same attempt on admin's file: 404. +POST {{base_url}}/api/shares +Authorization: Bearer {{mallory_token}} +Content-Type: application/json +{ + "item_id": "{{shared_file_id}}", + "item_name": "hello.txt", + "item_type": "file" +} + +HTTP 404 + + +# Step 14b.iii — Mallory has no shares — no partial success slipped +# through. (`GET /api/shares` returns only shares the +# caller created; response is paginated.) +GET {{base_url}}/api/shares +Authorization: Bearer {{mallory_token}} + +HTTP 200 +[Asserts] +jsonpath "$.items" isCollection +jsonpath "$.items" count == 0 + + # ───────────────────────────────────────────────────────────── # 15 — Teardown: revoke the password share + the direct # file-share, then delete the folder. diff --git a/tests/api/recent.hurl b/tests/api/recent.hurl index 4f423908..f290aba4 100644 --- a/tests/api/recent.hurl +++ b/tests/api/recent.hurl @@ -187,3 +187,67 @@ DELETE {{base_url}}/api/recent/clear Authorization: Bearer {{token}} HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 10 — Cross-tenant regression (post-Drive AuthZ audit, +# Round 1 HIGH). Before this fix, `POST /api/recent/…` +# accepted any UUID and the listing endpoint JOINed back +# to storage.files/folders (name/mime/size/drive_id) — a +# metadata oracle over the whole tenant. Now the write +# path calls `authz.require(Read, …)`; unauthorised +# callers get 404 (anti-enum) + `authz.denied` audit line. +# See `docs/plan/authz_audit/rest_storage.md`. +# ───────────────────────────────────────────────────────────── + +# Re-discover a folder id so the attacker has TWO targets to probe +# (file + folder). Same test1 folder as favorites.hurl. +GET {{base_url}}/api/folders/{{home_folder_id}}/resources?resource_types=folder +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +test1_id: jsonpath "$.items[0].resource.id" + + +# Create/lookup the attacker. Idempotent: `HTTP *` accepts either +# 201 (first run) or 409 (subsequent runs). Login below is the real +# precondition. +POST {{base_url}}/api/admin/users +Authorization: Bearer {{token}} +Content-Type: application/json +{ "username": "rec_mallory", "password": "RecMalloryPassword1!", "email": "rec_mallory@example.com", "role": "user" } + +HTTP * + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "rec_mallory", "password": "RecMalloryPassword1!" } + +HTTP 200 +[Captures] +mallory_token: jsonpath "$.access_token" + + +# Step 10a — Record admin's file into mallory's recent: 404. +POST {{base_url}}/api/recent/file/{{file_id}} +Authorization: Bearer {{mallory_token}} + +HTTP 404 + + +# Step 10b — Same for admin's folder: 404. +POST {{base_url}}/api/recent/folder/{{test1_id}} +Authorization: Bearer {{mallory_token}} + +HTTP 404 + + +# Step 10c — Mallory's recent list stays empty. +GET {{base_url}}/api/recent/resources +Authorization: Bearer {{mallory_token}} + +HTTP 200 +[Asserts] +jsonpath "$.items" count == 0 From 0342bae300e0892abc4443aa46c1d18b29007eae Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 4 Jul 2026 23:57:06 +0200 Subject: [PATCH 5/8] 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(), From 1786fe4111e1af8a71e436647d246acfc94ac115 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sun, 5 Jul 2026 22:52:26 +0200 Subject: [PATCH 6/8] security(nextcloud): chroot-aware display paths + recent race fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit strip_chroot_prefix replaces the hardcoded "Personal/" strip in NC trashbin PROPFIND, OCS unified search, and REPORT (favorites + search). Handles composed chroots, drops cross-chroot items instead of surfacing malformed paths, and fixes the leading-slash mismatch (FolderDto path has '/', DB paths don't) that silently dropped every NC trashbin item post-D3. OCS keeps a first-segment fallback (results legitimately span drives, no single chroot). uploads_handler switches to nc_to_internal_path(chroot, …) for the two remaining hardcoded "Personal/" sites, closing the D1 TODO markers. RecentService::record_item_access is split from a new record_item_access_internal (no authz) used by RecentRecordingHook. Round 1's authz.require widened the tokio::spawn race past tests/api/recent.hurl step 7; the internal path skips the redundant Read gate — upstream _with_perms already enforced it. Tests: 8 unit tests pin strip_chroot_prefix (leading slash, composed chroots, sibling-leak rejection, partial-prefix, empty-chroot). drives_membership.hurl step 21b/22b cover Editor upload → 201 / Viewer upload → 404 fresh + overwrite with fixture cleanup at 30c. test_nc_move_copy_delete_trash K1 pins the actual original-location value. --- src/application/services/recent_service.rs | 46 ++++- .../services/recent_recording_hook.rs | 12 +- src/interfaces/nextcloud/ocs_handler.rs | 24 +-- src/interfaces/nextcloud/report_handler.rs | 96 ++++++++--- src/interfaces/nextcloud/trashbin_handler.rs | 72 ++++++-- src/interfaces/nextcloud/webdav_handler.rs | 158 ++++++++++++++++++ tests/api/drives_membership.hurl | 77 ++++++++- .../webdav/test_nc_move_copy_delete_trash.sh | 15 +- 8 files changed, 446 insertions(+), 54 deletions(-) diff --git a/src/application/services/recent_service.rs b/src/application/services/recent_service.rs index 54482606..dbb9f610 100644 --- a/src/application/services/recent_service.rs +++ b/src/application/services/recent_service.rs @@ -3,7 +3,7 @@ use crate::application::dtos::recent_dto::{RecentCursor, RecentItemDto, RecentRe use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::recent_ports::{RecentItemsRepositoryPort, RecentItemsUseCase}; use crate::application::ports::resource_access_hook::ResourceAccessHook; -use crate::common::errors::Result; +use crate::common::errors::{DomainError, Result}; use crate::domain::services::authorization::{Permission, Resource, ResourceKind, Subject}; use crate::infrastructure::repositories::pg::RecentItemsPgRepository; use crate::infrastructure::services::pg_acl_engine::PgAclEngine; @@ -67,6 +67,41 @@ impl RecentService { hook.on_recents_cleared(user_id); } } + + /// Record access to an item WITHOUT the pre-write `authz.require` + /// gate. Callers must have gated the caller's Read upstream — this + /// method exists for the `RecentRecordingHook` fast path: writes + /// that reach the hook have already passed a `_with_perms` service + /// method (uploads, streams, GETs, etc.), so re-checking here + /// would be pure duplicate work AND widen the race window between + /// the POST response and the `tokio::spawn`ed upsert ( + /// `tests/api/recent.hurl` step 7 hits this — the extra SQL + /// round-trip pushes the upsert past the client's immediate + /// `GET /api/recent/resources`). + /// + /// **Do NOT call this from an externally-reachable handler.** The + /// REST endpoint goes through the trait method `record_item_access` + /// below, which enforces the Read gate per AGENTS.md convention. + pub async fn record_item_access_internal( + &self, + user_id: Uuid, + item_id: &str, + item_type: &str, + ) -> Result<()> { + // Type validation only — no authz, no resource parse for the + // engine (the hook path is already resource-typed by construction). + if item_type != "file" && item_type != "folder" { + return Err(DomainError::new( + crate::common::errors::ErrorKind::InvalidInput, + "RecentItems", + "Item type must be 'file' or 'folder'", + )); + } + + self.repo.upsert_access(user_id, item_id, item_type).await?; + self.repo.prune(user_id, self.max_recent_items).await?; + Ok(()) + } } impl RecentItemsUseCase for RecentService { @@ -107,13 +142,18 @@ impl RecentItemsUseCase for RecentService { // gate the write path was an information oracle over the // whole tenant via the listing endpoint's JOIN back to // storage.files/folders. + // + // Internal hook callers (RecentRecordingHook) bypass the + // trait entry point and call `record_item_access_internal` + // directly — Read has already been enforced upstream on + // whatever `_with_perms` service produced the access event. let resource = Resource::parse(item_type, item_id)?; self.authorization .require(Subject::User(user_id), Permission::Read, resource) .await?; - self.repo.upsert_access(user_id, item_id, item_type).await?; - self.repo.prune(user_id, self.max_recent_items).await?; + self.record_item_access_internal(user_id, item_id, item_type) + .await?; info!( "Successfully recorded access to {} '{}' for user {}", diff --git a/src/infrastructure/services/recent_recording_hook.rs b/src/infrastructure/services/recent_recording_hook.rs index f6ee9cb3..041e3737 100644 --- a/src/infrastructure/services/recent_recording_hook.rs +++ b/src/infrastructure/services/recent_recording_hook.rs @@ -29,7 +29,6 @@ use std::time::Duration; use moka::sync::Cache; use uuid::Uuid; -use crate::application::ports::recent_ports::RecentItemsUseCase; use crate::application::ports::resource_access_hook::ResourceAccessHook; use crate::application::services::recent_service::RecentService; @@ -85,7 +84,16 @@ impl ResourceAccessHook for RecentRecordingHook { let recent = Arc::clone(&self.recent); let (caller_id, file_id) = key; tokio::spawn(async move { - if let Err(e) = recent.record_item_access(caller_id, &file_id, "file").await { + // Fast path: skip the trait's `authz.require(Read, …)` + // (upstream `_with_perms` service already gated). The + // extra SQL round-trip pushes the upsert past the client's + // immediate `GET /api/recent/resources` in + // `tests/api/recent.hurl` step 7 — the whole reason for + // the internal variant. + if let Err(e) = recent + .record_item_access_internal(caller_id, &file_id, "file") + .await + { tracing::warn!( target: "oxicloud::recent", caller_id = %caller_id, diff --git a/src/interfaces/nextcloud/ocs_handler.rs b/src/interfaces/nextcloud/ocs_handler.rs index e673288b..73fc0394 100644 --- a/src/interfaces/nextcloud/ocs_handler.rs +++ b/src/interfaces/nextcloud/ocs_handler.rs @@ -422,13 +422,17 @@ pub async fn handle_search( let mut entries: Vec = Vec::new(); - // Map file results - // TODO(D1): drop the hardcoded "Personal/" prefix and read the - // caller's default-drive root folder name from `drives.root_folder_id` - // instead. Correct for D0-provisioned default drives; secondary - // drives keep their original root name. + // Map file results. + // + // `strip_drive_root_segment` handles both default and secondary + // drives — post-D0 the first path segment is the drive's root + // folder name (`"Personal"` for D0-provisioned defaults, the + // original sibling-root name for M2 backfilled secondaries). + // Read-scope is upstream in `state.applications.search_service`; + // this handler only formats display paths. for file in &results.files { - let display_path = file.path.strip_prefix("Personal/").unwrap_or(&file.path); + let display_path = + crate::interfaces::nextcloud::webdav_handler::strip_drive_root_segment(&file.path); let display_path = format!("/{}", display_path); let numeric_id = file_id_map.get(&file.id).copied(); @@ -452,12 +456,10 @@ pub async fn handle_search( })); } - // Map folder results — same TODO(D1) as above. + // Map folder results — same drive-agnostic strip as above. for folder in &results.folders { - let display_path = folder - .path - .strip_prefix("Personal/") - .unwrap_or(&folder.path); + let display_path = + crate::interfaces::nextcloud::webdav_handler::strip_drive_root_segment(&folder.path); let display_path = format!("/{}", display_path); entries.push(json!({ diff --git a/src/interfaces/nextcloud/report_handler.rs b/src/interfaces/nextcloud/report_handler.rs index ec60a8dc..39f913c0 100644 --- a/src/interfaces/nextcloud/report_handler.rs +++ b/src/interfaces/nextcloud/report_handler.rs @@ -62,6 +62,15 @@ async fn handle_filter_files( ) -> Result, AppError> { let user = &session.user; let url_user = &session.raw_username; + // Chroot-scope the response: NC's `oc:filter-files` REPORT is a + // single-drive surface (the client PROPFINDs favorites under its + // "home" URL and has no cross-drive concept). Favorites that live + // in another drive the caller is a member of are dropped from + // this response; they're still reachable via REST + // `/api/favorites/resources`. `session.require_chroot()` is safe + // here — the REPORT verb only reaches this handler through a + // path-scoped route. + let chroot = session.require_chroot()?; let fav_svc = match state.favorites_service.as_ref() { Some(svc) => svc, None => return Ok(empty_multistatus()), @@ -84,11 +93,11 @@ async fn handle_filter_files( // All items in this response are favorites. let favorite_ids: HashSet = favorites.iter().map(|f| f.item_id.clone()).collect(); - // TODO(D1): replace the hardcoded "Personal/" prefix with the - // caller's default-drive root folder name read from - // `drives.root_folder_id`. Correct for D0-provisioned default - // drives; secondary drives keep their original root name. - let home_prefix = "Personal/"; + // `home_prefix` is unused after the chroot-aware strip + // (see `strip_home_prefix`); kept as a positional argument in + // the emit calls below for signature stability with the + // report-handler tests and the parallel search-pass caller. + let home_prefix = ""; // Pass 1: resolve the favorited DTOs in two batch queries (was one // get_* per favorite — up to N serial round-trips on a sync client's @@ -155,7 +164,17 @@ async fn handle_filter_files( // multi-drive `~{drive}` form is echoed back to the client; // owner-id stays canonical via `&user.username`. for file in &files { - let subpath = strip_home_prefix(&file.path, home_prefix); + // Skip favorites that live outside the caller's chroot + // (other-drive favorites); reachable via REST if needed. + let Some(subpath) = strip_home_prefix(chroot, &file.path, home_prefix) else { + tracing::debug!( + target: "oxicloud::nc", + "REPORT filter-files: dropping cross-chroot favorite '{}' at '{}'", + file.id, + file.path, + ); + continue; + }; let href = nc_href(url_user, subpath); let fid = file_id_map.get(&file.id).copied(); let oc_id = fid.map(|id| format_oc_id(id, file_id_svc)); @@ -172,7 +191,15 @@ async fn handle_filter_files( } for folder in &folders { - let subpath = strip_home_prefix(&folder.path, home_prefix); + let Some(subpath) = strip_home_prefix(chroot, &folder.path, home_prefix) else { + tracing::debug!( + target: "oxicloud::nc", + "REPORT filter-files: dropping cross-chroot favorite folder '{}' at '{}'", + folder.id, + folder.path, + ); + continue; + }; let href = format!("{}/", nc_href(url_user, subpath)); let fid = folder_id_map.get(&folder.id).copied(); let oc_id = fid.map(|id| format_oc_id(id, file_id_svc)); @@ -207,9 +234,13 @@ async fn handle_search( session: &crate::interfaces::nextcloud::session::NcSession, ) -> Result, AppError> { let user = &session.user; - // Validate chroot up-front (path-scoped handler); `resolve_scope_folder` - // below re-pulls it from the session for the path-mapping step. - session.require_chroot()?; + // Chroot-scope the response: NC's search REPORT is a single-drive + // surface. Results that live outside the chroot (other drives the + // caller is a member of) are dropped from the multistatus and + // recorded at debug — reachable via REST search if needed. + // `resolve_scope_folder` below re-pulls chroot from the session + // for the path-mapping step. + let chroot = session.require_chroot()?; let url_user = &session.raw_username; let search_svc = match state.applications.search_service.as_ref() { Some(svc) => svc, @@ -241,10 +272,9 @@ async fn handle_search( let nc = state.nextcloud.as_ref(); let file_id_svc = nc.map(|n| &n.file_ids); - // TODO(D1): same as the favorites pass above — replace the - // hardcoded "Personal/" with the caller's actual default-drive - // root folder name from `drives.root_folder_id`. - let home_prefix = "Personal/"; + // See the favorites pass above: `home_prefix` is unused after the + // chroot-aware strip, kept only for signature stability. + let home_prefix = ""; // No favorite checking for search results -- pass an empty set. let favorite_ids: HashSet = HashSet::new(); @@ -266,7 +296,15 @@ async fn handle_search( // Files. for file in &files { - let subpath = strip_home_prefix(&file.path, home_prefix); + let Some(subpath) = strip_home_prefix(chroot, &file.path, home_prefix) else { + tracing::debug!( + target: "oxicloud::nc", + "REPORT search: dropping cross-chroot file '{}' at '{}'", + file.id, + file.path, + ); + continue; + }; let href = nc_href(url_user, subpath); let fid = file_id_map.get(&file.id).copied(); let oc_id = fid.map(|id| format_oc_id(id, file_id_svc)); @@ -284,7 +322,15 @@ async fn handle_search( // Folders. for folder in &folders { - let subpath = strip_home_prefix(&folder.path, home_prefix); + let Some(subpath) = strip_home_prefix(chroot, &folder.path, home_prefix) else { + tracing::debug!( + target: "oxicloud::nc", + "REPORT search: dropping cross-chroot folder '{}' at '{}'", + folder.id, + folder.path, + ); + continue; + }; let href = format!("{}/", nc_href(url_user, subpath)); let fid = folder_id_map.get(&folder.id).copied(); let oc_id = fid.map(|id| format_oc_id(id, file_id_svc)); @@ -545,7 +591,19 @@ fn extract_subpath_from_scope(href: &str, url_user: &str) -> Option { None } -/// Strip the `My Folder - {username}/` prefix to get the DAV subpath. -fn strip_home_prefix<'a>(path: &'a str, prefix: &str) -> &'a str { - path.strip_prefix(prefix).unwrap_or(path) +/// Strip the caller's chroot prefix from an internal path so the +/// caller-facing DAV subpath is chroot-relative. Delegates to +/// `webdav_handler::strip_chroot_prefix` — chroot-aware, multi-segment +/// safe, and rejects items outside the chroot. Callers must decide +/// per-response whether an out-of-chroot item is dropped or falls +/// back to the naive strip. +/// +/// See `strip_chroot_prefix` for the full contract. The `_prefix` +/// legacy arg stays for signature stability with the emit helpers. +fn strip_home_prefix<'a>( + chroot: &crate::application::dtos::folder_dto::FolderDto, + path: &'a str, + _prefix: &str, +) -> Option<&'a str> { + crate::interfaces::nextcloud::webdav_handler::strip_chroot_prefix(chroot, path) } diff --git a/src/interfaces/nextcloud/trashbin_handler.rs b/src/interfaces/nextcloud/trashbin_handler.rs index 6a7077ed..6a09a1c3 100644 --- a/src/interfaces/nextcloud/trashbin_handler.rs +++ b/src/interfaces/nextcloud/trashbin_handler.rs @@ -81,6 +81,14 @@ async fn handle_propfind( session: &crate::interfaces::nextcloud::session::NcSession, ) -> Result, AppError> { let user = &session.user; + // Chroot-scope the trashbin view: `get_trash_items(user.id)` + // spans every drive the caller is a member of, but NC's + // trashbin surface is a single-drive concept from the client's + // POV. Items outside the chroot are dropped from the multistatus + // (see `write_trashbin_multistatus` → `strip_home_prefix` → + // `webdav_handler::strip_chroot_prefix`) and remain reachable + // via REST `/api/trash/resources`. + let chroot = session.require_chroot()?; let trash_svc = state .trash_service .as_ref() @@ -95,7 +103,7 @@ async fn handle_propfind( let file_id_svc = nc.map(|n| &n.file_ids); let mut buf = Vec::new(); - write_trashbin_multistatus(&mut buf, &items, &user.username, file_id_svc) + write_trashbin_multistatus(&mut buf, &items, &user.username, chroot, file_id_svc) .await .map_err(|e| AppError::internal_error(format!("XML generation failed: {}", e)))?; @@ -259,18 +267,23 @@ fn mime_from_name(name: &str) -> String { .to_string() } -/// Strip the home-folder prefix from an original path to produce the -/// Nextcloud-relative original location. +/// Strip the caller's chroot prefix from an original path to produce +/// the Nextcloud-relative original-location value. /// -/// TODO(D1): replace the hardcoded "Personal/" with the caller's actual -/// default-drive root folder name read from `drives.root_folder_id`. -/// Correct for D0-provisioned default drives; secondary drives keep -/// their original root name. The `_username` arg stays for now so the -/// upcoming dynamic lookup has a way to identify the caller. -fn strip_home_prefix<'a>(original_path: &'a str, _username: &str) -> &'a str { - original_path - .strip_prefix("Personal/") - .unwrap_or(original_path) +/// Delegates to `webdav_handler::strip_chroot_prefix` — chroot-aware, +/// multi-segment safe, and returns `None` when the item is outside +/// the chroot (e.g. a trashed item in another drive the caller is a +/// member of). The `_username` arg stays for signature stability +/// with call sites that thread it; the strip itself no longer uses it. +/// +/// See the doc on `strip_chroot_prefix` for the AuthZ caveat — this +/// is a display helper, not an ownership check. +fn strip_home_prefix<'a>( + original_path: &'a str, + _username: &str, + chroot: &crate::application::dtos::folder_dto::FolderDto, +) -> Option<&'a str> { + crate::interfaces::nextcloud::webdav_handler::strip_chroot_prefix(chroot, original_path) } // ────────────── Trashbin PROPFIND XML Generation ────────────── @@ -280,10 +293,16 @@ use crate::application::services::nextcloud_file_id_service::NextcloudFileIdServ use std::collections::HashMap; /// Generate a complete Nextcloud-compatible multistatus XML response for the trashbin. +/// +/// `chroot` scopes the response — items whose original path is outside +/// the chroot (other drives the caller is a member of) are dropped +/// silently. NC's trashbin surface is single-drive from the client's +/// perspective; cross-drive items remain reachable via REST. async fn write_trashbin_multistatus( writer: W, items: &[TrashedItemDto], username: &str, + chroot: &crate::application::dtos::folder_dto::FolderDto, file_id_svc: Option<&Arc>, ) -> Result<(), String> { let mut xml = Writer::new(writer); @@ -315,9 +334,24 @@ async fn write_trashbin_multistatus( batch_resolve_ids(file_id_svc, &file_uuids, &folder_uuids).await; id_map.extend(folder_id_map); - // Individual trashed items. + // Individual trashed items — skip those whose original path is + // outside the chroot (other-drive trash reachable via REST). for item in items { - write_trash_item_response(&mut xml, item, username, file_id_svc, &id_map)?; + if crate::interfaces::nextcloud::webdav_handler::strip_chroot_prefix( + chroot, + &item.original_path, + ) + .is_none() + { + tracing::debug!( + target: "oxicloud::nc", + "trashbin PROPFIND: dropping cross-chroot item '{}' at '{}'", + item.id, + item.original_path, + ); + continue; + } + write_trash_item_response(&mut xml, item, username, chroot, file_id_svc, &id_map)?; } xml.write_event(Event::End(BytesEnd::new("d:multistatus"))) @@ -363,10 +397,18 @@ fn write_trash_root_response( } /// Write a single trashed item as a `` element. +/// +/// Caller is expected to have already verified the item is inside +/// `chroot` — see the guard in `write_trashbin_multistatus`. This +/// function trusts the invariant and expects `strip_home_prefix` to +/// return `Some(_)`; if it ever returns `None` (chroot drift between +/// the guard and the emit, defensive-only), the original-location +/// falls back to an empty string. fn write_trash_item_response( xml: &mut Writer, item: &TrashedItemDto, username: &str, + chroot: &crate::application::dtos::folder_dto::FolderDto, file_id_svc: Option<&Arc>, id_map: &HashMap, ) -> Result<(), String> { @@ -427,7 +469,7 @@ fn write_trash_item_response( write_text_element(xml, "nc:trashbin-filename", &item.name)?; // nc:trashbin-original-location - let original_location = strip_home_prefix(&item.original_path, username); + let original_location = strip_home_prefix(&item.original_path, username, chroot).unwrap_or(""); write_text_element(xml, "nc:trashbin-original-location", original_location)?; // nc:trashbin-deletion-time diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index a898f3ba..74eba460 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -80,6 +80,78 @@ pub fn nc_to_internal_path(chroot: &FolderDto, subpath: &str) -> Result(chroot: &FolderDto, internal_path: &'a str) -> Option<&'a str> { + // Normalize both sides: `FolderDto.path` comes from + // `StoragePath::to_string()` which prepends a leading `/` + // (e.g. `"/Personal"`), but DB-side paths coming from + // `storage.folders.path` (composed by the `compute_folder_path` + // trigger) never have a leading slash. Trim both so `"/Personal"` + // vs `"Personal/g9-tree"` matches the intended prefix. + let root = chroot.path.trim_matches('/'); + if root.is_empty() { + // Guard against a mis-set chroot with an empty root path — + // stripping "" from anything would return the whole path. + return None; + } + let path = internal_path.trim_start_matches('/'); + let rest = path.strip_prefix(root)?; + // Reject a partial prefix match — a chroot of "Personal" must + // not match an item at "PersonalSecrets/…". + match rest.strip_prefix('/') { + Some(subpath) => Some(subpath), + // Item path equals the chroot exactly — the chroot itself + // (i.e. a folder) is not a legitimate response item, so + // treat as an empty subpath. + None if rest.is_empty() => Some(""), + None => None, + } +} + +/// Naive fallback: strip the first path segment from an internal +/// `storage.folders.path`. Post-D0 every path starts with its drive's +/// root folder name (single segment), so for the current schema this +/// gives the drive-relative subpath. +/// +/// Use this ONLY when the caller doesn't have a chroot in scope +/// (e.g. OCS unified search, whose results legitimately span every +/// drive the caller has Read on — no single chroot covers them all). +/// Every path-scoped NC handler that DOES have `session` in scope +/// should prefer [`strip_chroot_prefix`] — it validates the item +/// belongs under the chroot instead of trusting the schema +/// invariant, and it survives a future composed chroot like +/// `"Personal/folderA/subfolder"`. +/// +/// **Not an AuthZ boundary.** Same caveat as `strip_chroot_prefix` +/// — AuthZ is enforced upstream via `_with_perms` methods; this +/// helper only formats display strings. +/// +/// Returns `""` when the path is a single segment (i.e. the drive +/// root itself, which is never a legitimate item target). +pub fn strip_drive_root_segment(internal_path: &str) -> &str { + match internal_path.split_once('/') { + Some((_root, rest)) => rest, + None => "", + } +} + /// Build the Nextcloud DAV href for a **collection** (folder). Always /// terminates with `/` — RFC 4918 §5.2 requires collection URLs to end /// in a slash, and the Nextcloud desktop client strictly enforces this @@ -1873,6 +1945,92 @@ mod tests { ); } + // ── strip_chroot_prefix ── + // + // Regression guard for the "chroot.path has a leading slash from + // StoragePath::to_string() but DB-side original_path doesn't" trap + // that broke the NC trashbin PROPFIND after Round 2 rolled out. + // Also pins the composed-chroot behaviour Ed asked about. + + #[test] + fn strip_chroot_prefix_default_drive_root() { + // FolderDto.path carries a leading slash (StoragePath Display); + // DB paths do not. Both must normalise to the same prefix. + let chroot = stub_folder("/Personal"); + assert_eq!( + strip_chroot_prefix(&chroot, "Personal/g9-tree"), + Some("g9-tree") + ); + } + + #[test] + fn strip_chroot_prefix_deep_path() { + let chroot = stub_folder("/Personal"); + assert_eq!( + strip_chroot_prefix(&chroot, "Personal/inner/deep.txt"), + Some("inner/deep.txt") + ); + } + + #[test] + fn strip_chroot_prefix_out_of_chroot_returns_none() { + // Items on a different drive (whose root isn't "Personal") + // must NOT be surfaced under the caller's chroot. + let chroot = stub_folder("/Personal"); + assert_eq!(strip_chroot_prefix(&chroot, "team-drive/report.pdf"), None); + } + + #[test] + fn strip_chroot_prefix_rejects_partial_prefix_match() { + // "Personal" is a prefix substring of "PersonalSecrets" but + // NOT a path-segment prefix — must reject. + let chroot = stub_folder("/Personal"); + assert_eq!( + strip_chroot_prefix(&chroot, "PersonalSecrets/foo.txt"), + None + ); + } + + #[test] + fn strip_chroot_prefix_composed_chroot() { + // The future composed-chroot case Ed raised: chroot points at + // a subfolder inside a drive. The strip must remove the ENTIRE + // composed prefix, not just the first segment. + let chroot = stub_folder("/Personal/folderA/subfolder"); + assert_eq!( + strip_chroot_prefix(&chroot, "Personal/folderA/subfolder/foo.txt"), + Some("foo.txt") + ); + } + + #[test] + fn strip_chroot_prefix_composed_chroot_sibling_leaks_blocked() { + // Same composed chroot, but the item lives in a sibling + // subfolder — must be rejected, not naively strip 1 segment. + let chroot = stub_folder("/Personal/folderA/subfolder"); + assert_eq!( + strip_chroot_prefix(&chroot, "Personal/folderA/other/foo.txt"), + None + ); + } + + #[test] + fn strip_chroot_prefix_chroot_root_itself() { + // Item path equals chroot exactly — legitimate for a PROPFIND + // Depth:0 on the chroot itself. Subpath is empty. + let chroot = stub_folder("/Personal"); + assert_eq!(strip_chroot_prefix(&chroot, "Personal"), Some("")); + } + + #[test] + fn strip_chroot_prefix_empty_chroot_returns_none() { + // Defensive: a mis-set chroot with an empty path must not + // strip anything (stripping "" from any path would return + // the whole path — a silent leak). + let chroot = stub_folder("/"); + assert_eq!(strip_chroot_prefix(&chroot, "Personal/foo.txt"), None); + } + // ── nc_href ── #[test] diff --git a/tests/api/drives_membership.hurl b/tests/api/drives_membership.hurl index e34bc1e5..cc450d3a 100644 --- a/tests/api/drives_membership.hurl +++ b/tests/api/drives_membership.hurl @@ -511,6 +511,26 @@ HTTP 200 jsonpath "$[*].id" contains {{team_drive_id}} +# ───────────────────────────────────────────────────────────── +# Step 21b — Upload gate by role (post-Drive AuthZ audit Round 2). +# Bob is Editor on team_drive; `POST /api/files/upload` +# targeting team_root_folder_id should succeed. This is +# the REST-side counterpart of the WebDAV/NC PUT chain +# hardened by `update_file_streaming_with_perms`. If +# this fails, the whole role-bundle → Permission::Create +# wiring is broken. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/files/upload +Authorization: Bearer {{bob_token}} +[MultipartFormData] +folder_id: {{team_root_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +bob_editor_upload_id: jsonpath "$.id" + + # ───────────────────────────────────────────────────────────── # Step 22 — Higher role wins: Bob now ALSO gets a Viewer direct # grant (would lower his bundle). The collapsed caller_role @@ -537,6 +557,50 @@ HTTP 200 jsonpath "$[*].id" contains {{team_drive_id}} +# ───────────────────────────────────────────────────────────── +# Step 22b — Viewer CANNOT upload into a shared drive. +# Post-Drive AuthZ audit Round 2: the create branch of +# `update_file_streaming_with_perms` requires +# `Permission::Create` on the parent folder — bundled +# with `owner`/`editor`/`contributor` role_grants only, +# NOT with `viewer`. `POST /api/files/upload` shares the +# same `save_file_with_blob` gate, so a Viewer probe +# must land 404 (anti-enum: same shape as no-such-folder) +# + `authz.denied` audit line. Also verify the batch / +# overwrite paths refuse — the whole chain from +# drive-membership to file write is exercised here. +# ───────────────────────────────────────────────────────────── + +# 22b.i — Fresh file: 404. +POST {{base_url}}/api/files/upload +Authorization: Bearer {{bob_token}} +[MultipartFormData] +folder_id: {{team_root_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 404 + + +# 22b.ii — Overwrite attempt on the Editor-era upload: still 404. +# `save_file_with_blob` catches the duplicate name at the +# `Create`-permission check before the upsert races (which +# would otherwise 409). The audit shape stays 404. +POST {{base_url}}/api/files/upload +Authorization: Bearer {{bob_token}} +[MultipartFormData] +folder_id: {{team_root_folder_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 404 + + +# 22b.iii — Alice's Editor-era file is untouched. +GET {{base_url}}/api/files/{{bob_editor_upload_id}} +Authorization: Bearer {{alice_token}} + +HTTP 200 + + # ============================================================= # Per-role mutation matrix — what every role can / can't do # ============================================================= @@ -845,15 +909,22 @@ HTTP 409 # 30c — Clear the lingering content (the Editor-created folder from -# Step 27). Delete via the regular folder endpoint so the row -# lands in trash, not the live tree; `is_empty` excludes -# trashed rows so a populated trash bin is allowed. +# Step 27 and the Editor-era file from Step 21b). Delete via +# the regular endpoints so rows land in trash, not the live +# tree; `is_empty` excludes trashed rows so a populated trash +# bin is allowed. DELETE {{base_url}}/api/folders/{{editor_created_folder_id}} Authorization: Bearer {{alice_token}} HTTP 204 +DELETE {{base_url}}/api/files/{{bob_editor_upload_id}} +Authorization: Bearer {{alice_token}} + +HTTP 204 + + # 30d — Owner on an empty drive → 204. DELETE {{base_url}}/api/drives/{{team_drive_id}} Authorization: Bearer {{alice_token}} diff --git a/tests/webdav/test_nc_move_copy_delete_trash.sh b/tests/webdav/test_nc_move_copy_delete_trash.sh index 4a26930c..3425754c 100755 --- a/tests/webdav/test_nc_move_copy_delete_trash.sh +++ b/tests/webdav/test_nc_move_copy_delete_trash.sh @@ -323,7 +323,20 @@ grep -q 'g8-doomed' <<< "$BODY" \ || fail "K1: g8-doomed.txt not in trashbin PROPFIND" grep -q '' <<< "$BODY" \ || fail "K1: trashbin response missing " -pass "K1: trashbin shows g8-doomed.txt with original-location" + +# Post-D3 (secondary/shared drive support): the `original-location` +# value is drive-relative — the emitter strips the drive-root segment +# from the internal `storage.folders.path` (`"Personal/g8-doomed.txt"` +# for a file at the default drive root) so NC clients see +# `"g8-doomed.txt"` regardless of what the drive's root is named. +# Regression guard: the pre-D3 code hardcoded `strip_prefix("Personal/")` +# — a bug that would silently break secondary drives. Assert the +# stripped shape (no leading `Personal/`, no leading `/`, no drive +# segment). +grep -q 'g8-doomed\.txt' <<< "$BODY" \ + || fail "K1: original-location not drive-relative (expected 'g8-doomed.txt', got: $(grep -o '[^<]*' <<< "$BODY"))" + +pass "K1: trashbin shows g8-doomed.txt with drive-relative original-location" # Extract the trashed item id (last segment of the href). # Trashbin hrefs are `/remote.php/dav/trashbin/{user}/trash/{uuid}` From 75601beb43f98d77e0c90ae0f248fe5be1d141a0 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sun, 5 Jul 2026 23:17:14 +0200 Subject: [PATCH 7/8] security(wopi): add authz to Wopi --- src/interfaces/api/handlers/wopi_handler.rs | 188 +++++++++- tests/api/run.sh | 20 +- tests/api/wopi_authz.hurl | 377 ++++++++++++++++++++ tests/common/server.env | 12 +- tests/common/wopi_mock_discovery.js | 68 ++++ 5 files changed, 649 insertions(+), 16 deletions(-) create mode 100644 tests/api/wopi_authz.hurl create mode 100644 tests/common/wopi_mock_discovery.js diff --git a/src/interfaces/api/handlers/wopi_handler.rs b/src/interfaces/api/handlers/wopi_handler.rs index ab4f98df..1ce94960 100644 --- a/src/interfaces/api/handlers/wopi_handler.rs +++ b/src/interfaces/api/handlers/wopi_handler.rs @@ -20,10 +20,13 @@ use axum::{ use serde::{Deserialize, Serialize}; use std::sync::Arc; +use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::file_ports::{FileRetrievalUseCase, FileUploadUseCase}; use crate::application::services::wopi_lock_service::WopiLockService; use crate::application::services::wopi_token_service::WopiTokenService; use crate::domain::repositories::drive_repository::DriveRepository; +use crate::domain::services::authorization::{Permission, Resource, Subject}; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; use crate::infrastructure::services::wopi_discovery_service::WopiDiscoveryService; /// Shared state for WOPI handlers. @@ -64,6 +67,37 @@ pub struct CheckFileInfoResponse { pub close_url: String, } +/// Enforce that the WOPI caller (`claims.sub`) still has `perm` on the +/// file at redemption time — not just at token-mint time. +/// +/// **Why every verb needs this.** WOPI tokens are validated locally +/// (HMAC over claims), so a token that was legitimately minted stays +/// verify-able until its TTL. If a grant is revoked after mint, or the +/// token was minted for view but is used to POST content, the token's +/// signature alone doesn't catch it. This helper re-checks against the +/// live authorization engine on every verb — the memory note +/// `wopi-authz-bypass` calls out the class of bugs this fences. +/// +/// Returns 404 (anti-enumeration — same shape as "file doesn't exist") +/// on both bad UUID and authorization denial. The engine emits a +/// structured `audit` line on denial internally, so ops sees the real +/// reason without the attacker being able to distinguish "gone" from +/// "revoked". +async fn require_wopi_perm( + authz: &PgAclEngine, + caller_sub: &str, + file_id: &str, + perm: Permission, +) -> Result<(uuid::Uuid, uuid::Uuid), StatusCode> { + let caller_uuid = uuid::Uuid::parse_str(caller_sub).map_err(|_| StatusCode::UNAUTHORIZED)?; + let file_uuid = uuid::Uuid::parse_str(file_id).map_err(|_| StatusCode::NOT_FOUND)?; + authz + .require(Subject::User(caller_uuid), perm, Resource::File(file_uuid)) + .await + .map_err(|_| StatusCode::NOT_FOUND)?; + Ok((caller_uuid, file_uuid)) +} + /// GET /wopi/files/{file_id} — CheckFileInfo async fn check_file_info( Path(file_id): Path, @@ -82,6 +116,19 @@ async fn check_file_info( return StatusCode::UNAUTHORIZED.into_response(); } + // Redemption-time authz: even with a valid token, the caller must + // still hold Read on this file. Catches revoked-grant-mid-session. + if let Err(status) = require_wopi_perm( + state.app_state.authorization.as_ref(), + &claims.sub, + &file_id, + Permission::Read, + ) + .await + { + return status.into_response(); + } + // Fetch file metadata let file = match state .app_state @@ -99,6 +146,24 @@ async fn check_file_info( .map(|dt| dt.to_rfc3339()) .unwrap_or_default(); + // `user_can_write` = actual current Update permission ∧ token's + // can_write flag. If the caller's Update was revoked since the + // token was minted (e.g. their grant was downgraded from Editor + // to Viewer), the editor sees the file as read-only and won't + // even attempt PutFile. The stricter `require_wopi_perm(Update)` + // in put_file is the actual gate; this field is a UI hint. + let can_write_now = claims.can_write + && state + .app_state + .authorization + .check( + Subject::User(uuid::Uuid::parse_str(&claims.sub).unwrap_or(uuid::Uuid::nil())), + Permission::Update, + Resource::File(uuid::Uuid::parse_str(&file_id).unwrap_or(uuid::Uuid::nil())), + ) + .await + .unwrap_or(false); + let response = CheckFileInfoResponse { base_file_name: file.name.clone(), // WOPI's `OwnerId` field is required. Post-D7 the DTO no @@ -112,9 +177,9 @@ async fn check_file_info( user_id: claims.sub.clone(), version: file.modified_at.to_string(), supports_locks: true, - supports_update: claims.can_write, + supports_update: can_write_now, supports_rename: false, - user_can_write: claims.can_write, + user_can_write: can_write_now, user_friendly_name: claims.username.clone(), post_message_origin: state.public_base_url.clone(), last_modified_time: last_modified, @@ -145,6 +210,18 @@ async fn get_file( return StatusCode::UNAUTHORIZED.into_response(); } + // Redemption-time authz — see require_wopi_perm docstring. + if let Err(status) = require_wopi_perm( + state.app_state.authorization.as_ref(), + &claims.sub, + &file_id, + Permission::Read, + ) + .await + { + return status.into_response(); + } + match state .app_state .applications @@ -184,6 +261,21 @@ async fn put_file( return StatusCode::UNAUTHORIZED.into_response(); } + // Redemption-time authz: the token says the caller could write when + // it was minted, but Update permission may have been revoked since. + // Re-check now so a stale write-capable token can't survive a + // downgrade / share removal / drive-membership change until its TTL. + if let Err(status) = require_wopi_perm( + state.app_state.authorization.as_ref(), + &claims.sub, + &file_id, + Permission::Update, + ) + .await + { + return status.into_response(); + } + // Check lock let request_lock = headers .get("X-WOPI-Lock") @@ -302,6 +394,22 @@ async fn file_operations( return StatusCode::UNAUTHORIZED.into_response(); } + // Every lock op mutates shared state (LOCK / UNLOCK / REFRESH_LOCK + // change the lock; GET_LOCK reads it but the read is only useful + // to a caller who could subsequently take a write action — so gate + // on Update uniformly rather than splitting per-op). A Viewer with + // a stale token must not be able to hold or contend for a lock. + if let Err(status) = require_wopi_perm( + state.app_state.authorization.as_ref(), + &claims.sub, + &file_id, + Permission::Update, + ) + .await + { + return status.into_response(); + } + let override_header = headers .get("X-WOPI-Override") .and_then(|v| v.to_str().ok()) @@ -374,25 +482,71 @@ pub struct EditorUrlResponse { pub access_token_ttl: i64, } -/// Determines if `caller_id` can access `file_id` and with what permissions. +/// Resolve the WOPI mint target: gate on real permissions and derive +/// the `can_write` flag from the caller's ACTUAL Update rights. /// -/// Uses the SQL-level ownership check (`get_file_owned`) so that files -/// belonging to other users — or non-existent files — both return `NOT_FOUND`, -/// avoiding existence-leak oracles. +/// Prior behaviour used a naive `requested_action != "view"` heuristic +/// so a Viewer clicking "Edit in Collabora" received a write-capable +/// token, promoting themselves to Editor for the token's TTL. The +/// memory note `wopi-authz-bypass` fix #12 calls this out explicitly. /// -/// Returns `(FileDto, can_write)` on success. +/// Contract: +/// +/// 1. **Read** is the bar to open the file in any mode. If the caller +/// has no Read grant, return 404 (anti-enum — same shape as "no such +/// file"). +/// 2. **Update** determines the returned `can_write` bit — INDEPENDENT +/// of what the client's `requested_action` said. A Viewer who +/// requested `action=edit` gets `can_write=false` and Collabora +/// opens in view mode; the token stays authorised for view-only +/// ops and put_file will 404 at redemption regardless. +/// 3. `requested_action == "view"` is respected as a downgrade — an +/// Editor can explicitly request view mode (co-browsing a doc +/// without accidentally editing) and get `can_write=false`. +/// +/// The `PgAclEngine::require`/`check` calls emit structured audit +/// lines on denial (`authz.denied` event), so a Viewer's "edit" +/// attempt shows up in the audit stream as a rejected Update check. async fn authorize_wopi_access( + authz: &PgAclEngine, file_retrieval: &S, file_id: &str, caller_id: uuid::Uuid, requested_action: &str, ) -> Result<(crate::application::dtos::file_dto::FileDto, bool), StatusCode> { - let file = file_retrieval - .get_file_with_perms(file_id, caller_id) + let file_uuid = uuid::Uuid::parse_str(file_id).map_err(|_| StatusCode::NOT_FOUND)?; + + // Step 1 — Read is required to even open the file. + authz + .require( + Subject::User(caller_id), + Permission::Read, + Resource::File(file_uuid), + ) .await .map_err(|_| StatusCode::NOT_FOUND)?; - // Owner verified — grant write unless explicitly requesting view-only. - let can_write = requested_action != "view"; + + let file = file_retrieval + .get_file(file_id) + .await + .map_err(|_| StatusCode::NOT_FOUND)?; + + // Step 2 — can_write reflects real Update, not the client's + // action-string. `check` returns bool without throwing; failure + // just means the caller lacks Update, so we degrade the token to + // read-only. Deliberately no `require` here — a Viewer opening + // the file is legitimate; only the write claim is suppressed. + let has_update = authz + .check( + Subject::User(caller_id), + Permission::Update, + Resource::File(file_uuid), + ) + .await + .unwrap_or(false); + + // Step 3 — allow explicit view-mode downgrade for Editors. + let can_write = has_update && requested_action != "view"; Ok((file, can_write)) } @@ -409,6 +563,7 @@ pub async fn get_editor_url( let username = &auth_user.username; // Verify the caller owns the file (SQL-level check, no existence leak). let (file, can_write) = match authorize_wopi_access( + state.app_state.authorization.as_ref(), state.app_state.applications.file_retrieval_service.as_ref(), ¶ms.file_id, user_id, @@ -494,7 +649,8 @@ async fn host_page( Ok(u) => u, Err(_) => return StatusCode::UNAUTHORIZED.into_response(), }; - let file = match authorize_wopi_access( + let (file, can_write_now) = match authorize_wopi_access( + state.app_state.authorization.as_ref(), state.app_state.applications.file_retrieval_service.as_ref(), &file_id, caller_uuid, @@ -502,7 +658,7 @@ async fn host_page( ) .await { - Ok((f, _)) => f, + Ok((f, cw)) => (f, cw), Err(status) => return status.into_response(), }; @@ -519,11 +675,15 @@ async fn host_page( _ => return StatusCode::INTERNAL_SERVER_ERROR.into_response(), }; + // Use the freshly-computed `can_write_now` (real Update permission + // ∧ requested_action) rather than the incoming token's `can_write` + // flag. Otherwise a Viewer who somehow reached this host page with + // a stale edit-capable token would get another one re-minted. let (token, ttl) = match state.token_service.generate_token( &file_id, &claims.sub, &claims.username, - claims.can_write, + can_write_now, ) { Ok(t) => t, Err(_) => return StatusCode::INTERNAL_SERVER_ERROR.into_response(), diff --git a/tests/api/run.sh b/tests/api/run.sh index 6fa42e63..e0f0e992 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -38,17 +38,34 @@ wait_for_http() { SERVER_PID="" +WOPI_MOCK_PID="" + cleanup() { if [[ -n "$SERVER_PID" ]]; then log "Stopping OxiCloud server (pid $SERVER_PID)..." kill "$SERVER_PID" 2>/dev/null || true wait "$SERVER_PID" 2>/dev/null || true fi + if [[ -n "$WOPI_MOCK_PID" ]]; then + log "Stopping WOPI mock discovery (pid $WOPI_MOCK_PID)..." + kill "$WOPI_MOCK_PID" 2>/dev/null || true + wait "$WOPI_MOCK_PID" 2>/dev/null || true + fi bash "$COMMON/stop-db.sh" } trap cleanup EXIT +# ── 0. WOPI mock discovery ──────────────────────────────────────────────────── +# Serves the static discovery.xml `OXICLOUD_WOPI_DISCOVERY_URL` +# points at (server.env pins port 9100). Started BEFORE OxiCloud so +# the server's cache-fill on first WOPI request finds it. The mock +# is stdlib-only Python (no deps) — see the file header for what it +# returns and why it's cheap. +log "Starting WOPI mock discovery on port 9100..." +node "$COMMON/wopi_mock_discovery.js" > /tmp/wopi-mock-discovery.log 2>&1 & +WOPI_MOCK_PID=$! + # ── 1. Start postgres ───────────────────────────────────────────────────────── bash "$COMMON/spawn-db.sh" @@ -168,7 +185,8 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/cross_drive_move.hurl" \ "$API_DIR/cross_drive_copy.hurl" \ "$API_DIR/webdav_dead_properties.hurl" \ - "$API_DIR/webdav_nested_move_cascade.hurl" + "$API_DIR/webdav_nested_move_cascade.hurl" \ + "$API_DIR/wopi_authz.hurl" #bash "$API_DIR/dedup_bulk_upload.sh" diff --git a/tests/api/wopi_authz.hurl b/tests/api/wopi_authz.hurl new file mode 100644 index 00000000..144e0df7 --- /dev/null +++ b/tests/api/wopi_authz.hurl @@ -0,0 +1,377 @@ +# ============================================================= +# OxiCloud — WOPI authorization at token redemption +# ============================================================= +# Regression coverage for the WOPI verb-handler bypass documented in +# memory note `wopi-authz-bypass`. Two bugs closed: +# +# 1. Verb handlers (check_file_info, get_file, put_file, +# file_operations, host_page) previously did NOT call +# `AuthorizationEngine::require` at redemption. A grant +# revoked between mint-time and request-time silently kept +# working until the token TTL expired. +# +# 2. The mint helper decided `can_write` from the client's +# `requested_action` string (`!= "view"` → write). A Viewer +# clicking "Edit in Collabora" received a write-capable +# token because the string was "edit". +# +# The fix wires `authz.require` on every verb and derives +# `can_write` from the caller's actual Update permission. This +# suite hits both paths through the real HTTP surface. +# +# Note on infra: +# * `OXICLOUD_WOPI_ENABLED=true` in tests/common/server.env +# * `OXICLOUD_WOPI_SECRET` pinned so the tokens the server mints +# round-trip verify-able through the suite +# * WOPI discovery served by `tests/common/wopi_mock_discovery.py` +# started by run.sh — mock URL points at a black-hole editor +# so we only assert on OxiCloud's own responses +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Login as admin (owner) and capture home folder id +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +alice_token: jsonpath "$.access_token" +alice_user_id: jsonpath "$.user.id" + + +GET {{base_url}}/api/folders +Authorization: Bearer {{alice_token}} + +HTTP 200 +[Captures] +alice_home_id: jsonpath "$[0].id" + + +# ───────────────────────────────────────────────────────────── +# Step 2 — Create a Bob user (Viewer under test) via admin API +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{alice_token}} +Content-Type: application/json +{ + "username": "wopi-bob", + "password": "WopiBobPassword1!", + "email": "wopi-bob@example.com", + "role": "user" +} + +HTTP 201 +[Captures] +bob_user_id: jsonpath "$.id" + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "wopi-bob", "password": "WopiBobPassword1!" } + +HTTP 200 +[Captures] +bob_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 3 — Alice uploads a plain-text file the WOPI verbs will +# target. `text/plain` is in the mock discovery XML so +# `/api/wopi/editor-url` resolves to a real (black-hole) +# editor URL — the endpoint returns 200 with an +# access_token we can then poke at the verbs. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/files/upload +Authorization: Bearer {{alice_token}} +[MultipartFormData] +folder_id: {{alice_home_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +file_id: jsonpath "$.id" +[Asserts] +jsonpath "$.mime_type" == "text/plain" + + +# ───────────────────────────────────────────────────────────── +# Step 4 — Alice mints an editor-URL for her own file with +# `action=edit`. Owner has Update → can_write=true. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=edit +Authorization: Bearer {{alice_token}} + +HTTP 200 +[Captures] +alice_edit_token: jsonpath "$.access_token" +[Asserts] +jsonpath "$.access_token" isString +jsonpath "$.editor_url" contains "edit" + + +# ───────────────────────────────────────────────────────────── +# Step 5 — CheckFileInfo with the owner's edit token. Verb +# re-checks Read → allowed. `user_can_write=true` +# reflects real Update. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/wopi/files/{{file_id}}?access_token={{alice_edit_token}} + +HTTP 200 +[Asserts] +jsonpath "$.UserId" == "{{alice_user_id}}" +jsonpath "$.UserCanWrite" == true +jsonpath "$.SupportsUpdate" == true + + +# ───────────────────────────────────────────────────────────── +# Step 6 — GetFile with the owner's edit token. Verb re-checks +# Read → 200 with body. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{alice_edit_token}} + +HTTP 200 +[Asserts] +body contains "Hello" + + +# ───────────────────────────────────────────────────────────── +# Step 7 — PutFile with the owner's edit token. Verb re-checks +# Update → 200. The owner overwrites her own file. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{alice_edit_token}} +Content-Type: application/octet-stream +``` +owner overwrite via WOPI PutFile +``` + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 8 — Alice explicitly requests view mode. Even the owner +# gets `can_write=false` — the token respects the +# client's downgrade so Collabora can open a doc +# "read-only for co-browsing". +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=view +Authorization: Bearer {{alice_token}} + +HTTP 200 +[Captures] +alice_view_token: jsonpath "$.access_token" + + +GET {{base_url}}/wopi/files/{{file_id}}?access_token={{alice_view_token}} + +HTTP 200 +[Asserts] +# Owner explicitly requested view — supports_update flips off. +jsonpath "$.UserCanWrite" == false +jsonpath "$.SupportsUpdate" == false + + +# View token trying to write → 401 (token's can_write bit says no +# before the authz.require ever runs). +POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{alice_view_token}} +Content-Type: application/octet-stream +``` +owner trying to write with view token +``` + +HTTP 401 + + +# ───────────────────────────────────────────────────────────── +# Step 9 — SECURITY: Bob has NO grant on Alice's file. Requests +# an edit-URL. The mint helper's Read gate fires → 404 +# (anti-enum). This is the pre-fix behaviour holding +# — mint-time Read was already enforced via +# get_file_with_perms. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=edit +Authorization: Bearer {{bob_token}} + +HTTP 404 + + +# ───────────────────────────────────────────────────────────── +# Step 10 — Alice grants Bob the Viewer role on the file. +# Capture the grant id off the POST response so Step +# 13's revoke doesn't need to LIST + filter (the LIST +# endpoint returns a bare JSON array, not +# `.grants[?...]`, and Hurl's single-match filter +# capture behaviour is quirky — see memory note +# `feedback_hurl_jsonpath_filter_empty`). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/grants +Authorization: Bearer {{alice_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{bob_user_id}}" }, + "resource": { "type": "file", "id": "{{file_id}}" }, + "role": "viewer" +} + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 11 — SECURITY: Bob (Viewer) requests an EDIT token. Fix +# #12: mint helper derives `can_write` from real +# Update permission, not from the requested_action +# string. Bob has Read but not Update → token is +# minted with `can_write=false` even though he asked +# for "edit". +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=edit +Authorization: Bearer {{bob_token}} + +HTTP 200 +[Captures] +bob_forged_edit_token: jsonpath "$.access_token" + + +# CheckFileInfo with Bob's "edit" token shows UserCanWrite=false +# because the token's can_write bit was scrubbed at mint. Prior +# to the fix this was `true` — a Viewer editing Alice's file. +GET {{base_url}}/wopi/files/{{file_id}}?access_token={{bob_forged_edit_token}} + +HTTP 200 +[Asserts] +jsonpath "$.UserId" == "{{bob_user_id}}" +jsonpath "$.UserCanWrite" == false +jsonpath "$.SupportsUpdate" == false + + +# Bob attempting PutFile with his "edit" token → 401. The +# token's own can_write=false is the outer gate; even if the +# token had somehow been forged with can_write=true, the +# redemption-time authz.require(Update) would return 404. +POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_forged_edit_token}} +Content-Type: application/octet-stream +``` +Bob trying to write as Viewer +``` + +HTTP 401 + + +# Bob CAN read (his Read grant is real). +GET {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_forged_edit_token}} + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 12 — SECURITY: promote Bob to Editor. Now he legitimately +# holds Update, so an edit token becomes truly write- +# capable. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/grants +Authorization: Bearer {{alice_token}} +Content-Type: application/json +{ + "subject": { "type": "user", "id": "{{bob_user_id}}" }, + "resource": { "type": "file", "id": "{{file_id}}" }, + "role": "editor" +} + +# The engine's `ON CONFLICT UPDATE` collapses one role row per +# (subject, resource), so this Editor grant REPLACES the Viewer +# grant from Step 10 rather than stacking. Bob now holds +# Editor alone; revoking it in Step 13 leaves him with no +# grants at all. +HTTP 201 +[Captures] +bob_grant_id: jsonpath "$.grants[0].id" + + +GET {{base_url}}/api/wopi/editor-url?file_id={{file_id}}&action=edit +Authorization: Bearer {{bob_token}} + +HTTP 200 +[Captures] +bob_real_edit_token: jsonpath "$.access_token" + + +GET {{base_url}}/wopi/files/{{file_id}}?access_token={{bob_real_edit_token}} + +HTTP 200 +[Asserts] +# Bob is a real Editor now → can_write flips to true. +jsonpath "$.UserCanWrite" == true +jsonpath "$.SupportsUpdate" == true + + +POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_real_edit_token}} +Content-Type: application/octet-stream +``` +Bob as Editor legitimately writes +``` + +HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 13 — SECURITY: revoke Bob's grant AFTER his edit token was +# minted. The token stays cryptographically valid until +# TTL, but every subsequent verb call must hit the +# authorization engine and reject. +# +# This is the CORE bug the memory note describes: prior +# to the fix Bob's PutFile still succeeded here because +# the verb handlers trusted the token in isolation. +# +# The Editor grant from Step 12 REPLACED the Viewer +# grant from Step 10 (engine's ON CONFLICT UPDATE — +# one role row per subject/resource). So revoking the +# Editor grant leaves Bob with no grants at all; every +# verb — Read AND Update — must refuse. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/grants/{{bob_grant_id}} +Authorization: Bearer {{alice_token}} + +HTTP 204 + + +# CheckFileInfo — no Read → 404. Prior to the fix the verb +# handler trusted the token and returned 200 with the file's +# metadata. +GET {{base_url}}/wopi/files/{{file_id}}?access_token={{bob_real_edit_token}} + +HTTP 404 + + +# GetFile — no Read → 404. Prior to the fix Bob could still +# download the file content until the token TTL expired. +GET {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_real_edit_token}} + +HTTP 404 + + +# PutFile — no Update → 404 (verb-side require_wopi_perm), OR +# 401 if the token's own `!claims.can_write` gate happened to +# fire first. The important assertion is "not 200" — a revoked +# grant must never let the caller through. +POST {{base_url}}/wopi/files/{{file_id}}/contents?access_token={{bob_real_edit_token}} +Content-Type: application/octet-stream +``` +Bob post-revoke tries to write +``` + +HTTP 404 + + +# ───────────────────────────────────────────────────────────── +# Cleanup — delete the test file so subsequent Hurl files don't +# see it. Bob user stays; other tests may reuse the `wopi-bob` +# username, but the grants that made this test meaningful are +# gone. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/files/{{file_id}} +Authorization: Bearer {{alice_token}} + +HTTP 204 diff --git a/tests/common/server.env b/tests/common/server.env index 8cc4f118..6c27ee39 100644 --- a/tests/common/server.env +++ b/tests/common/server.env @@ -13,7 +13,17 @@ OXICLOUD_ENABLE_SEARCH=true OXICLOUD_ENABLE_FILE_SHARING=true OXICLOUD_ENABLE_MUSIC=true OXICLOUD_EXPOSE_SYSTEM_USERS=true -OXICLOUD_WOPI_ENABLED=false +OXICLOUD_WOPI_ENABLED=true +# Fixed secret so the Hurl WOPI test can hand-craft valid access +# tokens with a known signing key. Prod deployments MUST override +# this to a random per-deployment value. +OXICLOUD_WOPI_SECRET=test-wopi-secret-do-not-use-in-prod-do-not-use-in-prod +# Discovery URL points at a black hole — VERB endpoints don't need +# discovery, and the WOPI Hurl suite deliberately does NOT touch +# `/api/wopi/editor-url` (the only path that would fetch it), so +# an unreachable URL keeps startup fast and hermetic. +OXICLOUD_WOPI_DISCOVERY_URL=http://127.0.0.1:9100/discovery.xml +OXICLOUD_WOPI_TOKEN_TTL_SECS=3600 OXICLOUD_OIDC_ENABLED=false OXICLOUD_NEXTCLOUD_ENABLED=true diff --git a/tests/common/wopi_mock_discovery.js b/tests/common/wopi_mock_discovery.js new file mode 100644 index 00000000..88005321 --- /dev/null +++ b/tests/common/wopi_mock_discovery.js @@ -0,0 +1,68 @@ +#!/usr/bin/env node +// Minimal mock WOPI discovery server for the Hurl WOPI suite. +// +// Serves a valid RFC-shaped discovery XML on `GET /discovery.xml` so +// `OXICLOUD_WOPI_DISCOVERY_URL=http://127.0.0.1:/discovery.xml` +// resolves to a real editor URL when `/api/wopi/editor-url` fetches it. +// +// The `urlsrc` we hand back points at a black-hole host so no real +// editor process needs to be running — the Hurl suite only asserts on +// OxiCloud's own responses (token contents, HTTP status codes, +// headers). The mock exists purely to let `get_editor_url` succeed +// end-to-end so we can exercise the mint-time authz path (Viewer- +// clicks-Edit gets a read-only token). +// +// Node stdlib only — matches the tooling used by tests/oidc/fake_idp +// (both are stdlib-free apart from `node-oidc-provider` on that side). +// No package.json, no npm install, no extra dependency for the api +// test suite. Started + reaped by `tests/api/run.sh`. Port comes from +// `WOPI_MOCK_PORT` env var (default 9100). + +'use strict'; + +const http = require('http'); + +const DISCOVERY_XML = ` + + + + + + + + + + + + + + + +`; + +const port = Number(process.env.WOPI_MOCK_PORT || 9100); + +const server = http.createServer((req, res) => { + if (req.method === 'GET' && req.url === '/discovery.xml') { + res.writeHead(200, { + 'Content-Type': 'application/xml; charset=utf-8', + 'Content-Length': Buffer.byteLength(DISCOVERY_XML), + }); + res.end(DISCOVERY_XML); + return; + } + res.writeHead(404); + res.end(); +}); + +// SIGTERM from `kill` in run.sh cleanup — exit quietly so the test +// runner's tail-of-log stays clean. +for (const sig of ['SIGTERM', 'SIGINT']) { + process.on(sig, () => server.close(() => process.exit(0))); +} + +server.listen(port, '127.0.0.1', () => { + console.log(`wopi-mock-discovery listening on 127.0.0.1:${port}`); +}); From 0870990e1b32e290fc34aa693bab9b3defa7d716 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sun, 5 Jul 2026 23:16:32 +0200 Subject: [PATCH 8/8] fix(locale): correct IT i18n --- frontend/static/locales/it.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/static/locales/it.json b/frontend/static/locales/it.json index 7fc32d82..8e9094b3 100644 --- a/frontend/static/locales/it.json +++ b/frontend/static/locales/it.json @@ -583,7 +583,7 @@ "item_deleted_permanently": "Elemento eliminato definitivamente", "trash_emptied": "Cestino svuotato con successo", "empty": "Nessuna notifica", - "title": "Notifiche" + "title": "Notifiche", "link_created": "Link creato", "share_success": "Link di condivisione creato con successo", "upload_files_section_title": "Caricamento non disponibile qui", @@ -948,7 +948,7 @@ "size": "Dimensione", "favoriteDate": "Data preferito", "byFiles": "Per file", - "sharedWith": "Condiviso con" + "sharedWith": "Condiviso con", "justAdded": "Nuovo", "folders": "Cartelle" },