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

fix(security): patch critical IDOR & auth vulnerabilities

Browse Source 
- Fix logout no-op: extract refresh token from cookie/body (auth_handler)
- Secure all 12 WebDAV handlers with AuthUser + resolve_path_for_user
- Secure all 7 batch handlers with caller_id ownership checks
- Add _owned variants: copy_file_owned, delete_file_owned, get_file_stream_owned, get_folder_owned
- Secure list_files_query: add AuthUser, SQL-level user_id filter, tenant-isolated ETag
- Remove deprecated unscoped resolve_path() and exists() from PathResolverService
- Remove dead list_files handler (unmounted, no auth)
- Add list_files_for_owner (SQL) and list_files_owned across trait chain
This commit is contained in:
Dionisio
2026-03-05 10:30:39 +01:00
parent ee86c3a128
commit fdbb2bf60a
14 changed files with 585 additions and 174 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/application/ports/file_ports.rs
+46
View File
@@ -119,12 +119,29 @@ pub trait FileRetrievalUseCase: Send + Sync + 'static {
/// Lists files in a folder
async fn list_files(&self, folder_id: Option<&str>) -> Result<Vec<FileDto>, DomainError>;
/// Lists files in a folder, scoped to the authenticated user.
///
/// Uses SQL-level `AND user_id` filtering — no in-memory post-filter.
/// All user-facing list handlers should use this method.
async fn list_files_owned(
&self,
folder_id: Option<&str>,
owner_id: &str,
) -> Result<Vec<FileDto>, DomainError>;
/// Gets file content as a stream (for large files)
async fn get_file_stream(
&self,
id: &str,
) -> Result<Box<dyn Stream<Item = Result<Bytes, std::io::Error>> + Send>, DomainError>;
/// Gets file content as a stream, enforcing that `caller_id` is the owner.
async fn get_file_stream_owned(
&self,
id: &str,
caller_id: &str,
) -> Result<Box<dyn Stream<Item = Result<Bytes, std::io::Error>> + Send>, DomainError>;
/// Optimized multi-tier download.
///
/// Internalises: write-behind lookup → content-cache → WebP transcode →
@@ -208,6 +225,24 @@ pub trait FileRetrievalUseCase: Send + Sync + 'static {
.take(limit as usize)
.collect())
}
/// Like [`list_files_batch`], but scoped to a specific owner.
///
/// Used by streaming WebDAV PROPFIND so that each user only sees their
/// own files, even in shared folder_id namespaces.
async fn list_files_batch_for_owner(
&self,
folder_id: Option<&str>,
owner_id: &str,
offset: i64,
limit: i64,
) -> Result<Vec<FileDto>, DomainError> {
let all = self.list_files_batch(folder_id, offset, limit).await?;
Ok(all
.into_iter()
.filter(|f| f.owner_id.as_deref().map_or(false, |o| o == owner_id))
.collect())
}
}
// ─────────────────────────────────────────────────────
@@ -238,6 +273,14 @@ pub trait FileManagementUseCase: Send + Sync + 'static {
target_folder_id: Option<String>,
) -> Result<FileDto, DomainError>;
/// Copies a file, enforcing that `caller_id` is the owner.
async fn copy_file_owned(
&self,
file_id: &str,
caller_id: &str,
target_folder_id: Option<String>,
) -> Result<FileDto, DomainError>;
/// Renames a file (system/internal — no ownership check).
async fn rename_file(&self, file_id: &str, new_name: &str) -> Result<FileDto, DomainError>;
@@ -252,6 +295,9 @@ pub trait FileManagementUseCase: Send + Sync + 'static {
/// Deletes a file (system/internal — no ownership check).
async fn delete_file(&self, id: &str) -> Result<(), DomainError>;
/// Deletes a file, enforcing that `caller_id` is the owner.
async fn delete_file_owned(&self, id: &str, caller_id: &str) -> Result<(), DomainError>;
/// Smart delete: trash-first with dedup reference cleanup.
///
/// 1. Tries to move to trash (soft delete).
src/application/ports/inbound.rs
+6
View File
@@ -16,6 +16,12 @@ pub trait FolderUseCase: Send + Sync + 'static {
/// Gets a folder by its ID
async fn get_folder(&self, id: &str) -> Result<FolderDto, DomainError>;
/// Gets a folder by its ID, enforcing that `caller_id` is the owner.
///
/// Returns `NotFound` if the folder does not exist **or** belongs to
/// another user. All user-facing handlers should use this method.
async fn get_folder_owned(&self, id: &str, caller_id: &str) -> Result<FolderDto, DomainError>;
/// Gets a folder by its path
async fn get_folder_by_path(&self, path: &str) -> Result<FolderDto, DomainError>;
src/application/ports/storage_ports.rs
+35
View File
@@ -46,6 +46,22 @@ pub trait FileReadPort: Send + Sync + 'static {
/// Lists files in a folder.
async fn list_files(&self, folder_id: Option<&str>) -> Result<Vec<File>, DomainError>;
/// Lists files in a folder scoped to a specific owner (SQL-level).
///
/// Default falls back to `list_files` + in-memory filter.
/// Repositories should override with a direct `AND user_id = $N` query.
async fn list_files_for_owner(
&self,
folder_id: Option<&str>,
owner_id: &str,
) -> Result<Vec<File>, DomainError> {
let all = self.list_files(folder_id).await?;
Ok(all
.into_iter()
.filter(|f| f.owner_id().map_or(false, |o| o == owner_id))
.collect())
}
/// Gets content as a stream (ideal for large files).
async fn get_file_stream(
&self,
@@ -108,6 +124,25 @@ pub trait FileReadPort: Send + Sync + 'static {
Ok(all.into_iter().skip(start).take(end - start).collect())
}
/// Like [`list_files_batch`], but only returns files owned by `owner_id`.
///
/// Used by streaming WebDAV PROPFIND to list files scoped to the
/// authenticated user, preventing cross-user data leakage.
async fn list_files_batch_for_owner(
&self,
folder_id: Option<&str>,
owner_id: &str,
offset: i64,
limit: i64,
) -> Result<Vec<File>, DomainError> {
// Default: filter in-memory (repos should override with SQL)
let all = self.list_files_batch(folder_id, offset, limit).await?;
Ok(all
.into_iter()
.filter(|f| f.owner_id().map_or(false, |o| o == owner_id))
.collect())
}
/// Streams every file in the subtree rooted at `folder_id`.
///
/// Uses an ltree `<@` join against `storage.folders` so the entire
src/application/services/batch_operations.rs
+43 -11
View File
@@ -117,6 +117,7 @@ impl BatchOperationService {
&self,
file_ids: Vec<String>,
target_folder_id: Option<String>,
caller_id: &str,
) -> Result<BatchResult<FileDto>, BatchOperationError> {
info!("Starting batch copy of {} files", file_ids.len());
let start_time = std::time::Instant::now();
@@ -134,15 +135,17 @@ impl BatchOperationService {
// Arc<str> avoids N heap-clones of the same string
let target_folder: Option<Arc<str>> = target_folder_id.map(|s| Arc::from(s.as_str()));
let caller: Arc<str> = Arc::from(caller_id);
// buffer_unordered materialises only max_concurrent futures at a time
let mut operation_stream = stream::iter(file_ids.into_iter().map(|file_id| {
let mgmt = self.file_management.clone();
let target_folder = target_folder.clone();
let caller = caller.clone();
async move {
let copy_result = mgmt
.copy_file(&file_id, target_folder.map(|s| s.to_string()))
.copy_file_owned(&file_id, &caller, target_folder.map(|s| s.to_string()))
.await;
(file_id, copy_result)
}
@@ -184,6 +187,7 @@ impl BatchOperationService {
&self,
file_ids: Vec<String>,
target_folder_id: Option<String>,
caller_id: &str,
) -> Result<BatchResult<FileDto>, BatchOperationError> {
info!("Starting batch move of {} files", file_ids.len());
let start_time = std::time::Instant::now();
@@ -200,14 +204,16 @@ impl BatchOperationService {
};
let target_folder: Option<Arc<str>> = target_folder_id.map(|s| Arc::from(s.as_str()));
let caller: Arc<str> = Arc::from(caller_id);
let mut operation_stream = stream::iter(file_ids.into_iter().map(|file_id| {
let mgmt = self.file_management.clone();
let target_folder = target_folder.clone();
let caller = caller.clone();
async move {
let move_result = mgmt
.move_file(&file_id, target_folder.map(|s| s.to_string()))
.move_file_owned(&file_id, &caller, target_folder.map(|s| s.to_string()))
.await;
(file_id, move_result)
}
@@ -247,6 +253,7 @@ impl BatchOperationService {
pub async fn delete_files(
&self,
file_ids: Vec<String>,
caller_id: &str,
) -> Result<BatchResult<String>, BatchOperationError> {
info!("Starting batch deletion of {} files", file_ids.len());
let start_time = std::time::Instant::now();
@@ -262,11 +269,14 @@ impl BatchOperationService {
};
// Define the operation to perform for each file
let caller: Arc<str> = Arc::from(caller_id);
let mut operation_stream = stream::iter(file_ids.into_iter().map(|file_id| {
let mgmt = self.file_management.clone();
let caller = caller.clone();
async move {
let delete_result = mgmt.delete_file(&file_id).await;
let delete_result = mgmt.delete_file_owned(&file_id, &caller).await;
let id_for_result = file_id.clone();
(file_id, delete_result.map(|_| id_for_result))
}
@@ -307,6 +317,7 @@ impl BatchOperationService {
pub async fn get_multiple_files(
&self,
file_ids: Vec<String>,
caller_id: &str,
) -> Result<BatchResult<FileDto>, BatchOperationError> {
info!("Starting batch load of {} files", file_ids.len());
let start_time = std::time::Instant::now();
@@ -322,11 +333,14 @@ impl BatchOperationService {
};
// Define the operation to perform for each file
let caller: Arc<str> = Arc::from(caller_id);
let mut operation_stream = stream::iter(file_ids.into_iter().map(|file_id| {
let retrieval = self.file_retrieval.clone();
let caller = caller.clone();
async move {
let get_result = retrieval.get_file(&file_id).await;
let get_result = retrieval.get_file_owned(&file_id, &caller).await;
(file_id, get_result)
}
}))
@@ -631,6 +645,7 @@ impl BatchOperationService {
&self,
file_ids: Vec<String>,
folder_ids: Vec<String>,
caller_id: &str,
) -> Result<NamedTempFile, BatchOperationError> {
info!(
"Starting batch download: {} files, {} folders",
@@ -650,10 +665,10 @@ impl BatchOperationService {
// ── Add individual files at the root of the ZIP ──────────────────
for file_id in &file_ids {
match self.file_retrieval.get_file(file_id).await {
match self.file_retrieval.get_file_owned(file_id, caller_id).await {
Ok(file_dto) => {
if let Err(e) = self
.add_file_entry_streamed(&mut zip, file_id, &file_dto.name)
.add_file_entry_streamed(&mut zip, file_id, &file_dto.name, caller_id)
.await
{
info!("Could not add file {} to ZIP: {}", file_dto.name, e);
@@ -667,10 +682,10 @@ impl BatchOperationService {
// ── Add folders as sub-trees (bulk subtree queries, not N+1) ─────
for folder_id in &folder_ids {
match self.folder_service.get_folder(folder_id).await {
match self.folder_service.get_folder_owned(folder_id, caller_id).await {
Ok(root_folder) => {
if let Err(e) = self
.add_folder_subtree_to_zip(&mut zip, folder_id, &root_folder)
.add_folder_subtree_to_zip(&mut zip, folder_id, &root_folder, caller_id)
.await
{
info!("Could not add folder {} to ZIP: {}", root_folder.name, e);
@@ -709,6 +724,7 @@ impl BatchOperationService {
zip: &mut ZipFileWriter<tokio_util::compat::Compat<BufWriter<tokio::fs::File>>>,
file_id: &str,
entry_name: &str,
caller_id: &str,
) -> Result<(), BatchOperationError> {
let entry = ZipEntryBuilder::new(entry_name.to_string().into(), Compression::Deflate);
let mut writer = zip
@@ -718,7 +734,7 @@ impl BatchOperationService {
let stream = self
.file_retrieval
.get_file_stream(file_id)
.get_file_stream_owned(file_id, caller_id)
.await
.map_err(BatchOperationError::Domain)?;
let mut stream = std::pin::Pin::from(stream);
@@ -749,6 +765,7 @@ impl BatchOperationService {
zip: &mut ZipFileWriter<tokio_util::compat::Compat<BufWriter<tokio::fs::File>>>,
folder_id: &str,
root_folder: &FolderDto,
caller_id: &str,
) -> Result<(), BatchOperationError> {
// Bulk-fetch folder tree (small — one entry per folder)
let all_folders = self
@@ -803,7 +820,7 @@ impl BatchOperationService {
for file in files {
let file_path = format!("{}{}", zip_dir, file.name);
if let Err(e) = self
.add_file_entry_streamed(zip, &file.id, &file_path)
.add_file_entry_streamed(zip, &file.id, &file_path, caller_id)
.await
{
info!("Could not add file {} to ZIP: {}", file.name, e);
@@ -887,6 +904,7 @@ impl BatchOperationService {
pub async fn create_folders(
&self,
folders: Vec<(String, Option<String>)>, // (name, parent_id)
caller_id: &str,
) -> Result<BatchResult<FolderDto>, BatchOperationError> {
info!("Starting batch creation of {} folders", folders.len());
let start_time = std::time::Instant::now();
@@ -902,10 +920,20 @@ impl BatchOperationService {
};
// Define the operation for each folder
let caller: Arc<str> = Arc::from(caller_id);
let mut operation_stream = stream::iter(folders.into_iter().map(|(name, parent_id)| {
let folder_service = self.folder_service.clone();
let caller = caller.clone();
async move {
// If a parent is specified, verify the caller owns it
if let Some(ref pid) = parent_id {
if let Err(e) = folder_service.get_folder_owned(pid, &caller).await {
let id = format!("{}:{}", name, pid);
return (id, Err(e.into()));
}
}
let dto = crate::application::dtos::folder_dto::CreateFolderDto {
name: name.clone(),
parent_id: parent_id.clone(),
@@ -951,6 +979,7 @@ impl BatchOperationService {
pub async fn get_multiple_folders(
&self,
folder_ids: Vec<String>,
caller_id: &str,
) -> Result<BatchResult<FolderDto>, BatchOperationError> {
info!("Starting batch load of {} folders", folder_ids.len());
let start_time = std::time::Instant::now();
@@ -966,11 +995,14 @@ impl BatchOperationService {
};
// Define the operation for each folder
let caller: Arc<str> = Arc::from(caller_id);
let mut operation_stream = stream::iter(folder_ids.into_iter().map(|folder_id| {
let folder_service = self.folder_service.clone();
let caller = caller.clone();
async move {
let get_result = folder_service.get_folder(&folder_id).await;
let get_result = folder_service.get_folder_owned(&folder_id, &caller).await;
(folder_id, get_result)
}
}))
src/application/services/file_management_service.rs
+15
View File
@@ -128,6 +128,16 @@ impl FileManagementUseCase for FileManagementService {
Ok(FileDto::from(copied_file))
}
async fn copy_file_owned(
&self,
file_id: &str,
caller_id: &str,
target_folder_id: Option<String>,
) -> Result<FileDto, DomainError> {
self.verify_owner(file_id, caller_id).await?;
self.copy_file(file_id, target_folder_id).await
}
async fn rename_file(&self, file_id: &str, new_name: &str) -> Result<FileDto, DomainError> {
info!("Renaming file with ID: {} to \"{}\"", file_id, new_name);
@@ -163,6 +173,11 @@ impl FileManagementUseCase for FileManagementService {
self.file_repository.delete_file(id).await
}
async fn delete_file_owned(&self, id: &str, caller_id: &str) -> Result<(), DomainError> {
self.verify_owner(id, caller_id).await?;
self.delete_file(id).await
}
/// Smart delete: trash-first with dedup reference cleanup.
///
/// Blob ref_count bookkeeping is handled entirely by the PG trigger
src/application/services/file_retrieval_service.rs
+32
View File
@@ -223,6 +223,15 @@ impl FileRetrievalUseCase for FileRetrievalService {
Ok(files.into_iter().map(FileDto::from).collect())
}
async fn list_files_owned(
&self,
folder_id: Option<&str>,
owner_id: &str,
) -> Result<Vec<FileDto>, DomainError> {
let files = self.file_read.list_files_for_owner(folder_id, owner_id).await?;
Ok(files.into_iter().map(FileDto::from).collect())
}
async fn get_file_stream(
&self,
id: &str,
@@ -230,6 +239,15 @@ impl FileRetrievalUseCase for FileRetrievalService {
self.file_read.get_file_stream(id).await
}
async fn get_file_stream_owned(
&self,
id: &str,
caller_id: &str,
) -> Result<Box<dyn Stream<Item = Result<Bytes, std::io::Error>> + Send>, DomainError> {
self.file_read.verify_file_owner(id, caller_id).await?;
self.file_read.get_file_stream(id).await
}
/// Multi-tier optimized download.
async fn get_file_optimized(
&self,
@@ -311,4 +329,18 @@ impl FileRetrievalUseCase for FileRetrievalService {
.await?;
Ok(files.into_iter().map(FileDto::from).collect())
}
async fn list_files_batch_for_owner(
&self,
folder_id: Option<&str>,
owner_id: &str,
offset: i64,
limit: i64,
) -> Result<Vec<FileDto>, DomainError> {
let files = self
.file_read
.list_files_batch_for_owner(folder_id, owner_id, offset, limit)
.await?;
Ok(files.into_iter().map(FileDto::from).collect())
}
}
src/application/services/folder_service.rs
+19
View File
@@ -32,6 +32,10 @@ impl FolderService {
Ok(FolderDto::empty())
}
async fn get_folder_owned(&self, _id: &str, _caller_id: &str) -> Result<FolderDto, DomainError> {
Ok(FolderDto::empty())
}
async fn get_folder_by_path(&self, _path: &str) -> Result<FolderDto, DomainError> {
Ok(FolderDto::empty())
}
@@ -196,6 +200,21 @@ impl FolderUseCase for FolderService {
Ok(FolderDto::from(folder))
}
/// Gets a folder by its ID, enforcing that `caller_id` is the owner.
async fn get_folder_owned(&self, id: &str, caller_id: &str) -> Result<FolderDto, DomainError> {
let folder_dto = self.get_folder(id).await?;
if folder_dto.owner_id.as_deref() != Some(caller_id) {
tracing::warn!(
"get_folder_owned: user '{}' attempted to access folder '{}' owned by '{:?}'",
caller_id,
id,
folder_dto.owner_id
);
return Err(DomainError::not_found("Folder", id));
}
Ok(folder_dto)
}
/// Gets a folder by its path
async fn get_folder_by_path(&self, path: &str) -> Result<FolderDto, DomainError> {
// Convert the string path to StoragePath