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

fix(security): patch 3 vulnerabilities — IDOR, ownership bypass, XSS

Browse Source 
V1: Add owner-scoped folder pagination (list_folders_by_owner_paginated)
  - New method in FolderRepository trait, PG implementation, service & handler
  - Prevents IDOR by filtering folder listings to authenticated user

V2: Enforce ownership checks on folder mutations
  - rename_folder, move_folder, delete_folder now require caller_id
  - Service verifies folder.owner_id == caller_id (returns 404 on mismatch)
  - Propagated to folder_handler, batch_handler, batch_operations, webdav_handler
  - delete_folder_with_trash upgraded from OptionalAuthUser to AuthUser
  - download_folder_zip now checks ownership before streaming

V3: Fix XSS in frontend via DOM APIs
  - sharedView.js: innerHTML → createElement + textContent
  - contextMenus.js: innerHTML → DOM construction for share dialog

Cleanup: removed unused OptionalAuthUser import, updated all stubs/mocks
This commit is contained in:
Dionisio
2026-02-16 00:22:42 +01:00
parent 66b4acd9d6
commit 5a679dfc90
25 changed files with 684 additions and 269 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/application/services/batch_operations.rs
+6 -4
View File
@@ -408,6 +408,7 @@ impl BatchOperationService {
&self,
folder_ids: Vec<String>,
_recursive: bool,
caller_id: &str,
) -> Result<BatchResult<String>, BatchOperationError> {
info!("Starting batch deletion of {} folders", folder_ids.len());
let start_time = std::time::Instant::now();
@@ -427,14 +428,13 @@ impl BatchOperationService {
let folder_service = self.folder_service.clone();
let semaphore = self.semaphore.clone();
let id_clone = folder_id.clone();
let caller = caller_id.to_string();
async move {
// Acquire semaphore permit
let permit = semaphore.acquire().await.unwrap();
// For both recursive and non-recursive, use the standard delete_folder method
// since FolderUseCase only has a single delete_folder method
let delete_result = folder_service.delete_folder(&folder_id).await;
let delete_result = folder_service.delete_folder(&folder_id, &caller).await;
// Release the permit explicitly
drop(permit);
@@ -616,6 +616,7 @@ impl BatchOperationService {
&self,
folder_ids: Vec<String>,
target_folder_id: Option<String>,
caller_id: &str,
) -> Result<BatchResult<FolderDto>, BatchOperationError> {
info!("Starting batch move of {} folders", folder_ids.len());
let start_time = std::time::Instant::now();
@@ -633,11 +634,12 @@ impl BatchOperationService {
let folder_service = self.folder_service.clone();
let target = target_folder_id.clone();
let semaphore = self.semaphore.clone();
let caller = caller_id.to_string();
async move {
let permit = semaphore.acquire().await.unwrap();
let dto = MoveFolderDto { parent_id: target };
let move_result = folder_service.move_folder(&folder_id, dto).await;
let move_result = folder_service.move_folder(&folder_id, dto, &caller).await;
drop(permit);
(folder_id, move_result)
}
src/application/services/folder_service.rs
+98 -16
View File
@@ -71,10 +71,30 @@ impl FolderService {
)
}
async fn list_folders_for_owner_paginated(
&self,
_parent_id: Option<&str>,
_owner_id: &str,
_pagination: &crate::application::dtos::pagination::PaginationRequestDto,
) -> Result<
crate::application::dtos::pagination::PaginatedResponseDto<FolderDto>,
DomainError,
> {
Ok(
crate::application::dtos::pagination::PaginatedResponseDto::new(
vec![],
0,
10,
0,
),
)
}
async fn rename_folder(
&self,
_id: &str,
_dto: RenameFolderDto,
_caller_id: &str,
) -> Result<FolderDto, DomainError> {
Ok(FolderDto::empty())
}
@@ -83,11 +103,12 @@ impl FolderService {
&self,
_id: &str,
_dto: MoveFolderDto,
_caller_id: &str,
) -> Result<FolderDto, DomainError> {
Ok(FolderDto::empty())
}
async fn delete_folder(&self, _id: &str) -> Result<(), DomainError> {
async fn delete_folder(&self, _id: &str, _caller_id: &str) -> Result<(), DomainError> {
Ok(())
}
}
@@ -211,17 +232,15 @@ impl FolderUseCase for FolderService {
pagination: &crate::application::dtos::pagination::PaginationRequestDto,
) -> Result<crate::application::dtos::pagination::PaginatedResponseDto<FolderDto>, DomainError>
{
// Validate and adjust pagination
let pagination = pagination.validate_and_adjust();
// Get paginated folders and total count
let (folders, total_items) = self
.folder_storage
.list_folders_paginated(
parent_id,
pagination.offset(),
pagination.limit(),
true, // Always include total for better UX
true,
)
.await
.map_err(|e| {
@@ -234,10 +253,8 @@ impl FolderUseCase for FolderService {
)
})?;
// The total is needed to calculate pagination
let total = total_items.unwrap_or(folders.len());
// Convert to PaginatedResponseDto
let response = crate::application::dtos::pagination::PaginatedResponseDto::new(
folders.into_iter().map(FolderDto::from).collect(),
pagination.page,
@@ -248,11 +265,54 @@ impl FolderUseCase for FolderService {
Ok(response)
}
/// Renames a folder
/// Lists folders with pagination, scoped to a specific owner.
async fn list_folders_for_owner_paginated(
&self,
parent_id: Option<&str>,
owner_id: &str,
pagination: &crate::application::dtos::pagination::PaginationRequestDto,
) -> Result<crate::application::dtos::pagination::PaginatedResponseDto<FolderDto>, DomainError>
{
let pagination = pagination.validate_and_adjust();
let (folders, total_items) = self
.folder_storage
.list_folders_by_owner_paginated(
parent_id,
owner_id,
pagination.offset(),
pagination.limit(),
true,
)
.await
.map_err(|e| {
DomainError::internal_error(
"FolderStorage",
format!(
"Failed to list folders for owner '{}' with pagination in parent {:?}: {}",
owner_id, parent_id, e
),
)
})?;
let total = total_items.unwrap_or(folders.len());
let response = crate::application::dtos::pagination::PaginatedResponseDto::new(
folders.into_iter().map(FolderDto::from).collect(),
pagination.page,
pagination.page_size,
total,
);
Ok(response)
}
/// Renames a folder after verifying ownership.
async fn rename_folder(
&self,
id: &str,
dto: RenameFolderDto,
caller_id: &str,
) -> Result<FolderDto, DomainError> {
// Input validation
if dto.name.is_empty() {
@@ -263,7 +323,7 @@ impl FolderUseCase for FolderService {
));
}
// Verify the folder exists
// Verify the folder exists and belongs to the caller
let existing_folder = self.folder_storage.get_folder(id).await.map_err(|e| {
DomainError::internal_error(
"FolderStorage",
@@ -271,6 +331,14 @@ impl FolderUseCase for FolderService {
)
})?;
if existing_folder.owner_id() != Some(caller_id) {
tracing::warn!(
"rename_folder: user '{}' attempted to rename folder '{}' owned by '{:?}'",
caller_id, id, existing_folder.owner_id()
);
return Err(DomainError::not_found("Folder", id));
}
// Create transaction for renaming
let mut transaction = StorageTransaction::new("rename_folder");
@@ -323,9 +391,9 @@ impl FolderUseCase for FolderService {
Ok(FolderDto::from(folder))
}
/// Moves a folder to a new parent
async fn move_folder(&self, id: &str, dto: MoveFolderDto) -> Result<FolderDto, DomainError> {
// Verify the source folder exists
/// Moves a folder to a new parent after verifying ownership.
async fn move_folder(&self, id: &str, dto: MoveFolderDto, caller_id: &str) -> Result<FolderDto, DomainError> {
// Verify the source folder exists and belongs to the caller
let source_folder = self.folder_storage.get_folder(id).await.map_err(|e| {
DomainError::internal_error(
"FolderStorage",
@@ -333,6 +401,14 @@ impl FolderUseCase for FolderService {
)
})?;
if source_folder.owner_id() != Some(caller_id) {
tracing::warn!(
"move_folder: user '{}' attempted to move folder '{}' owned by '{:?}'",
caller_id, id, source_folder.owner_id()
);
return Err(DomainError::not_found("Folder", id));
}
// If a parent_id is specified, verify it exists
if let Some(parent_id) = &dto.parent_id {
// Verify we are not trying to move the folder into itself or one of its descendants
@@ -408,17 +484,23 @@ impl FolderUseCase for FolderService {
Ok(FolderDto::from(folder))
}
/// Deletes a folder
async fn delete_folder(&self, id: &str) -> Result<(), DomainError> {
// Verify the folder exists
let _folder = self.folder_storage.get_folder(id).await.map_err(|e| {
/// Deletes a folder after verifying ownership.
async fn delete_folder(&self, id: &str, caller_id: &str) -> Result<(), DomainError> {
// Verify the folder exists and belongs to the caller
let folder = self.folder_storage.get_folder(id).await.map_err(|e| {
DomainError::internal_error(
"FolderStorage",
format!("Failed to get folder with ID: {} for deletion: {}", id, e),
)
})?;
// In a real implementation, we could verify permissions, dependencies, etc.
if folder.owner_id() != Some(caller_id) {
tracing::warn!(
"delete_folder: user '{}' attempted to delete folder '{}' owned by '{:?}'",
caller_id, id, folder.owner_id()
);
return Err(DomainError::not_found("Folder", id));
}
// Delete the folder
self.folder_storage.delete_folder(id).await.map_err(|e| {
src/application/services/share_service.rs
+12
View File
@@ -544,6 +544,18 @@ mod tests {
unimplemented!()
}
async fn list_folders_by_owner_paginated(
&self,
_parent_id: Option<&str>,
_owner_id: &str,
_offset: usize,
_limit: usize,
_include_total: bool,
) -> Result<(Vec<crate::domain::entities::folder::Folder>, Option<usize>), DomainError>
{
unimplemented!()
}
async fn rename_folder(
&self,
_id: &str,
src/application/services/trash_service_test.rs
+11
View File
@@ -373,6 +373,17 @@ impl FolderRepository for MockFolderRepository {
Ok((vec![], Some(0)))
}
async fn list_folders_by_owner_paginated(
&self,
_parent_id: Option<&str>,
_owner_id: &str,
_offset: usize,
_limit: usize,
_include_total: bool,
) -> std::result::Result<(Vec<Folder>, Option<usize>), DomainError> {
Ok((vec![], Some(0)))
}
async fn rename_folder(
&self,
_id: &str,