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

security: fix vulnerabilities 1-7 from security audit

Browse Source 
- Fix #1: Share handler IDOR - enforce owner check on share operations
- Fix #2: list_files_query IDOR - bind folder queries to authenticated user
- Fix #3: Dedup handler IDOR - restrict dedup operations to file owner
- Fix #4: Trash handler OptionalAuthUser - require full AuthUser
- Fix #5: Error info leakage - sanitize 500 error responses
- Fix #6: Chunked upload IDOR - bind upload sessions to user_id,
  add verify_session_owner() check on all session operations
- Fix #7: CSP unsafe-inline removal - migrate all inline scripts,
  styles and event handlers to external files, tighten CSP to
  script-src 'self'; style-src 'self'

New files:
  - static/js/core/theme-init.js (render-blocking theme init)
  - static/js/core/sw-register.js (service worker registration)
  - static/css/views/device-verify.css (extracted inline styles)
  - static/js/views/device-verify/device-verify.js (extracted inline script)
This commit is contained in:
Dionisio
2026-03-05 13:15:34 +01:00
parent fdbb2bf60a
commit b503e08384
38 changed files with 870 additions and 1008 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/application/ports/chunked_upload_ports.rs
+6 -3
View File
@@ -59,6 +59,7 @@ pub trait ChunkedUploadPort: Send + Sync + 'static {
/// total number of chunks, and expiration timestamp.
async fn create_session(
&self,
user_id: &str,
filename: String,
folder_id: Option<String>,
content_type: String,
@@ -72,13 +73,14 @@ pub trait ChunkedUploadPort: Send + Sync + 'static {
async fn upload_chunk(
&self,
upload_id: &str,
user_id: &str,
chunk_index: usize,
data: Bytes,
checksum: Option<String>,
) -> Result<ChunkUploadResponseDto, DomainError>;
/// Get the current status of an upload session.
async fn get_status(&self, upload_id: &str) -> Result<UploadStatusResponseDto, DomainError>;
async fn get_status(&self, upload_id: &str, user_id: &str) -> Result<UploadStatusResponseDto, DomainError>;
/// Assemble all chunks into the final file.
///
@@ -88,13 +90,14 @@ pub trait ChunkedUploadPort: Send + Sync + 'static {
async fn complete_upload(
&self,
upload_id: &str,
user_id: &str,
) -> Result<(PathBuf, String, Option<String>, String, u64, String), DomainError>;
/// Finalize upload: clean up the session and temporary files.
async fn finalize_upload(&self, upload_id: &str) -> Result<(), DomainError>;
async fn finalize_upload(&self, upload_id: &str, user_id: &str) -> Result<(), DomainError>;
/// Cancel an upload and clean up all temporary data.
async fn cancel_upload(&self, upload_id: &str) -> Result<(), DomainError>;
async fn cancel_upload(&self, upload_id: &str, user_id: &str) -> Result<(), DomainError>;
/// Check if a file size qualifies for chunked upload.
fn should_use_chunked(&self, size: u64) -> bool;
src/application/ports/share_ports.rs
+27 -14
View File
@@ -15,28 +15,34 @@ pub trait ShareUseCase: Send + Sync + 'static {
dto: CreateShareDto,
) -> Result<ShareDto, DomainError>;
/// Get a shared link by its ID
async fn get_shared_link(&self, id: &str) -> Result<ShareDto, DomainError>;
/// Get a shared link by its ID (ownership-verified)
async fn get_shared_link(
&self,
id: &str,
requester_id: &str,
) -> Result<ShareDto, DomainError>;
/// Get a shared link by its token (for access by non-users)
async fn get_shared_link_by_token(&self, token: &str) -> Result<ShareDto, DomainError>;
/// Get all shared links for a specific item
/// Get all shared links for a specific item (ownership-verified)
async fn get_shared_links_for_item(
&self,
item_id: &str,
item_type: &ShareItemType,
requester_id: &str,
) -> Result<Vec<ShareDto>, DomainError>;
/// Update a shared link
/// Update a shared link (ownership-verified)
async fn update_shared_link(
&self,
id: &str,
requester_id: &str,
dto: UpdateShareDto,
) -> Result<ShareDto, DomainError>;
/// Delete a shared link
async fn delete_shared_link(&self, id: &str) -> Result<(), DomainError>;
/// Delete a shared link (ownership-verified)
async fn delete_shared_link(&self, id: &str, requester_id: &str) -> Result<(), DomainError>;
/// Get all shared links created by a specific user
async fn get_user_shared_links(
@@ -63,20 +69,29 @@ pub trait ShareStoragePort: Send + Sync + 'static {
share: &crate::domain::entities::share::Share,
) -> Result<crate::domain::entities::share::Share, DomainError>;
async fn find_share_by_id(
&self,
id: &str,
) -> Result<crate::domain::entities::share::Share, DomainError>;
async fn find_share_by_token(
&self,
token: &str,
) -> Result<crate::domain::entities::share::Share, DomainError>;
async fn find_shares_by_item(
/// Find a share by ID only if it belongs to the given user.
/// Returns `NotFound` if the share doesn't exist OR belongs to another user
/// (prevents share-ID enumeration).
async fn find_share_by_id_for_user(
&self,
id: &str,
user_id: &str,
) -> Result<crate::domain::entities::share::Share, DomainError>;
/// Delete a share only if it belongs to the given user.
async fn delete_share_for_user(&self, id: &str, user_id: &str) -> Result<(), DomainError>;
/// Find shares for a specific item that belong to the given user.
async fn find_shares_by_item_for_user(
&self,
item_id: &str,
item_type: &ShareItemType,
user_id: &str,
) -> Result<Vec<crate::domain::entities::share::Share>, DomainError>;
async fn update_share(
@@ -84,8 +99,6 @@ pub trait ShareStoragePort: Send + Sync + 'static {
share: &crate::domain::entities::share::Share,
) -> Result<crate::domain::entities::share::Share, DomainError>;
async fn delete_share(&self, id: &str) -> Result<(), DomainError>;
async fn find_shares_by_user(
&self,
user_id: &str,
src/application/services/share_service.rs
+71 -55
View File
@@ -140,6 +140,25 @@ impl ShareService {
})?;
self.password_hasher.hash_password(password).await
}
/// Fetch a share and verify that `requester_id` owns it.
///
/// SECURITY: returns `NotFound` (not `Forbidden`) when the share exists
/// but belongs to a different user — this prevents share-ID enumeration
/// attacks where an attacker probes IDs and uses 403-vs-404 to learn
/// which ones are valid.
async fn fetch_owned_share(
&self,
id: &str,
requester_id: &str,
) -> Result<Share, DomainError> {
let share = self
.share_repository
.find_share_by_id_for_user(id, requester_id)
.await?;
Ok(share)
}
}
impl ShareUseCase for ShareService {
@@ -187,15 +206,14 @@ impl ShareUseCase for ShareService {
Ok(ShareDto::from_entity(&saved_share, &self.config.base_url()))
}
async fn get_shared_link(&self, id: &str) -> Result<ShareDto, DomainError> {
// Find the shared link by its ID
let share = self
.share_repository
.find_share_by_id(id)
.await
.map_err(|e| {
ShareServiceError::NotFound(format!("Share with ID {} not found: {}", id, e))
})?;
async fn get_shared_link(
&self,
id: &str,
requester_id: &str,
) -> Result<ShareDto, DomainError> {
// SECURITY: ownership-verified lookup — returns 404 if the share
// doesn't exist OR belongs to another user.
let share = self.fetch_owned_share(id, requester_id).await?;
// Check if it has expired
if share.is_expired() {
@@ -229,11 +247,12 @@ impl ShareUseCase for ShareService {
&self,
item_id: &str,
item_type: &ShareItemType,
requester_id: &str,
) -> Result<Vec<ShareDto>, DomainError> {
// Find all shared links for the item
// SECURITY: only return shares created by the requester
let shares = self
.share_repository
.find_shares_by_item(item_id, item_type)
.find_shares_by_item_for_user(item_id, item_type, requester_id)
.await
.map_err(|e| ShareServiceError::Repository(e.to_string()))?;
@@ -252,16 +271,11 @@ impl ShareUseCase for ShareService {
async fn update_shared_link(
&self,
id: &str,
requester_id: &str,
dto: UpdateShareDto,
) -> Result<ShareDto, DomainError> {
// Find the existing shared link
let mut share = self
.share_repository
.find_share_by_id(id)
.await
.map_err(|e| {
ShareServiceError::NotFound(format!("Share with ID {} not found: {}", id, e))
})?;
// SECURITY: ownership-verified lookup — prevents IDOR
let mut share = self.fetch_owned_share(id, requester_id).await?;
// Update permissions if provided
if let Some(permissions_dto) = dto.permissions {
@@ -302,12 +316,11 @@ impl ShareUseCase for ShareService {
))
}
async fn delete_shared_link(&self, id: &str) -> Result<(), DomainError> {
// Delete the shared link
async fn delete_shared_link(&self, id: &str, requester_id: &str) -> Result<(), DomainError> {
// SECURITY: ownership-verified delete — only the creator can remove
self.share_repository
.delete_share(id)
.await
.map_err(|e| ShareServiceError::Repository(e.to_string()))?;
.delete_share_for_user(id, requester_id)
.await?;
Ok(())
}
@@ -688,15 +701,6 @@ mod tests {
Ok(share.clone())
}
async fn find_share_by_id(&self, id: &str) -> Result<Share, DomainError> {
let shares = self.shares.lock().unwrap();
shares
.get(id)
.cloned()
.ok_or_else(|| DomainError::not_found("Share", id))
}
async fn find_share_by_token(&self, token: &str) -> Result<Share, DomainError> {
let tokens = self.tokens.lock().unwrap();
let shares = self.shares.lock().unwrap();
@@ -711,20 +715,50 @@ mod tests {
.ok_or_else(|| DomainError::not_found("Share", id.as_str()))
}
async fn find_shares_by_item(
async fn find_share_by_id_for_user(
&self,
id: &str,
user_id: &str,
) -> Result<Share, DomainError> {
let shares = self.shares.lock().unwrap();
shares
.get(id)
.filter(|s| s.created_by() == user_id)
.cloned()
.ok_or_else(|| DomainError::not_found("Share", id))
}
async fn delete_share_for_user(&self, id: &str, user_id: &str) -> Result<(), DomainError> {
let mut shares = self.shares.lock().unwrap();
let mut tokens = self.tokens.lock().unwrap();
let share = shares
.get(id)
.filter(|s| s.created_by() == user_id)
.ok_or_else(|| DomainError::not_found("Share", id))?;
tokens.remove(share.token());
shares.remove(id);
Ok(())
}
async fn find_shares_by_item_for_user(
&self,
item_id: &str,
item_type: &ShareItemType,
user_id: &str,
) -> Result<Vec<Share>, DomainError> {
let shares = self.shares.lock().unwrap();
let type_str = item_type.to_string();
let result: Vec<Share> = shares
.values()
.filter(|s| s.item_id() == item_id && s.item_type().to_string() == type_str)
.filter(|s| {
s.item_id() == item_id
&& s.item_type().to_string() == type_str
&& s.created_by() == user_id
})
.cloned()
.collect();
Ok(result)
}
@@ -741,24 +775,6 @@ mod tests {
Ok(share.clone())
}
async fn delete_share(&self, id: &str) -> Result<(), DomainError> {
let mut shares = self.shares.lock().unwrap();
let mut tokens = self.tokens.lock().unwrap();
// Find the share to get the token
let share = shares
.get(id)
.ok_or_else(|| DomainError::not_found("Share", id))?;
// Remove token mapping
tokens.remove(share.token());
// Remove the share
shares.remove(id);
Ok(())
}
async fn find_shares_by_user(
&self,
user_id: &str,
src/application/services/trash_service.rs
+19 -7
View File
@@ -144,9 +144,8 @@ impl TrashUseCase for TrashService {
);
debug!("User UUID validation: {}", user_id);
// Note: We do NOT call validate_user_ownership here because the item
// is not yet in the trash. Ownership validation is only for operations
// on already-trashed items (restore, delete_permanently).
// Note: We now verify file/folder ownership BEFORE moving to trash.
// This prevents users from trashing items they do not own (IDOR).
// Parse UUIDs with detailed error handling
debug!("Validating item UUID: {}", item_id);
@@ -183,9 +182,11 @@ impl TrashUseCase for TrashService {
"file" => {
info!("Processing file to move to trash: {}", item_id);
// Get the file to verify it exists and capture its data
debug!("Getting file data: {}", item_id);
let file = match self.file_read_port.get_file(item_id).await {
// Get the file — ownership-verified at SQL level.
// Returns NotFound if the file does not exist OR belongs to
// another user, preventing cross-user trash operations.
debug!("Getting file data (owner-scoped): {}", item_id);
let file = match self.file_read_port.get_file_for_owner(item_id, user_id).await {
Ok(file) => {
debug!("File found: {} ({})", file.name(), item_id);
file
@@ -254,7 +255,9 @@ impl TrashUseCase for TrashService {
Ok(())
}
"folder" => {
// Get the folder to verify it exists and capture its data
// Get the folder and verify ownership.
// Returns NotFound if the folder does not exist or belongs
// to another user — prevents cross-user trash operations.
let folder = self
.folder_storage_port
.get_folder(item_id)
@@ -267,6 +270,15 @@ impl TrashUseCase for TrashService {
)
})?;
// Ownership check — return NotFound (not Forbidden) to
// prevent leaking whether the folder exists.
if folder.owner_id().map_or(true, |o| o != user_id) {
return Err(DomainError::not_found(
"Folder",
format!("Folder not found: {}", item_id),
));
}
let original_path = folder.storage_path().to_string();
// Create the trash item