fix(security): apply 4 vulnerability fixes from security audit
1. Share password bypass (HIGH): enforce password check in get_shared_link_by_token, verify_shared_link_password now returns ShareDto only on correct password. 2. WebDAV MOVE ownership (MEDIUM): add assert_owner on destination parent folder for file moves in both PathResolver and legacy branches. 3. Path traversal defense-in-depth (LOW): add reject_path_traversal() to WebDAV, CalDAV, and CardDAV handlers rejecting '..' segments at HTTP boundary. 4. Setup race condition (LOW): atomic INSERT ... ON CONFLICT DO NOTHING in try_claim_initialization prevents duplicate admin creation.
This commit is contained in:
@@ -52,12 +52,14 @@ pub trait ShareUseCase: Send + Sync + 'static {
|
||||
per_page: usize,
|
||||
) -> Result<PaginatedResponseDto<ShareDto>, DomainError>;
|
||||
|
||||
/// Verify a password for a password-protected shared link
|
||||
/// Verify a password for a password-protected shared link.
|
||||
/// On success, returns the full share metadata (`ShareDto`).
|
||||
/// On failure (wrong password), returns `AccessDenied`.
|
||||
async fn verify_shared_link_password(
|
||||
&self,
|
||||
token: &str,
|
||||
password: &str,
|
||||
) -> Result<bool, DomainError>;
|
||||
) -> Result<ShareDto, DomainError>;
|
||||
|
||||
/// Register an access to a shared link
|
||||
async fn register_shared_link_access(&self, token: &str) -> Result<(), DomainError>;
|
||||
|
||||
@@ -378,6 +378,21 @@ impl AdminSettingsService {
|
||||
.await
|
||||
}
|
||||
|
||||
/// Atomically try to claim system initialization.
|
||||
///
|
||||
/// Returns `Ok(true)` if this call was the one that marked the system as
|
||||
/// initialized (the caller "won" the race), or `Ok(false)` if another
|
||||
/// request already did it. This eliminates the race-condition window
|
||||
/// between `is_system_initialized()` and `mark_system_initialized()`.
|
||||
pub async fn try_claim_initialization(
|
||||
&self,
|
||||
admin_user_id: &str,
|
||||
) -> Result<bool, DomainError> {
|
||||
self.settings_repo
|
||||
.try_claim_initialization(admin_user_id)
|
||||
.await
|
||||
}
|
||||
|
||||
// ========================================================================
|
||||
// Registration Control
|
||||
// ========================================================================
|
||||
|
||||
@@ -20,7 +20,7 @@ use crate::{
|
||||
storage_ports::FileReadPort,
|
||||
},
|
||||
},
|
||||
common::{config::AppConfig, errors::DomainError},
|
||||
common::{config::AppConfig, errors::{DomainError, ErrorKind}},
|
||||
domain::entities::share::{Share, ShareItemType, SharePermissions},
|
||||
};
|
||||
|
||||
@@ -239,6 +239,17 @@ impl ShareUseCase for ShareService {
|
||||
return Err(ShareServiceError::Expired.into());
|
||||
}
|
||||
|
||||
// SECURITY: If the share is password-protected, do NOT return
|
||||
// the full metadata. Force the caller to verify the password
|
||||
// first via `verify_shared_link_password`.
|
||||
if share.has_password() {
|
||||
return Err(DomainError::new(
|
||||
ErrorKind::AccessDenied,
|
||||
"Share",
|
||||
"This share is password protected",
|
||||
));
|
||||
}
|
||||
|
||||
// Convert the entity to DTO for the response
|
||||
Ok(ShareDto::from_entity(&share, &self.config.base_url()))
|
||||
}
|
||||
@@ -357,7 +368,7 @@ impl ShareUseCase for ShareService {
|
||||
&self,
|
||||
token: &str,
|
||||
password: &str,
|
||||
) -> Result<bool, DomainError> {
|
||||
) -> Result<ShareDto, DomainError> {
|
||||
// Find the shared link by its token
|
||||
let share = self
|
||||
.share_repository
|
||||
@@ -374,9 +385,21 @@ impl ShareUseCase for ShareService {
|
||||
|
||||
// Verify the password using the infrastructure port
|
||||
match share.password_hash() {
|
||||
Some(hash) => self.password_hasher.verify_password(password, hash).await,
|
||||
None => Ok(true), // No password required
|
||||
Some(hash) => {
|
||||
let is_valid = self.password_hasher.verify_password(password, hash).await?;
|
||||
if !is_valid {
|
||||
return Err(DomainError::new(
|
||||
ErrorKind::AccessDenied,
|
||||
"Share",
|
||||
"Invalid share password",
|
||||
));
|
||||
}
|
||||
}
|
||||
None => { /* No password required — allow access */ }
|
||||
}
|
||||
|
||||
// Password verified (or not required) — return full share metadata
|
||||
Ok(ShareDto::from_entity(&share, &self.config.base_url()))
|
||||
}
|
||||
|
||||
async fn register_shared_link_access(&self, token: &str) -> Result<(), DomainError> {
|
||||
|
||||
@@ -23,4 +23,35 @@ pub trait SettingsRepository: Send + Sync + 'static {
|
||||
|
||||
/// Delete a setting by key
|
||||
async fn delete(&self, key: &str) -> Result<(), DomainError>;
|
||||
|
||||
/// Atomically claim system initialization.
|
||||
///
|
||||
/// Inserts `system_initialized = "true"` **only if the key does not
|
||||
/// already exist**. Returns `true` when this call was the one that
|
||||
/// performed the insert (i.e. the caller "won" the race), `false` if
|
||||
/// the system was already initialized.
|
||||
///
|
||||
/// The default implementation falls back to the non-atomic
|
||||
/// get-then-set pattern for repositories that don't support a native
|
||||
/// atomic upsert.
|
||||
async fn try_claim_initialization(
|
||||
&self,
|
||||
admin_user_id: &str,
|
||||
) -> Result<bool, DomainError> {
|
||||
// Default: non-atomic fallback (overridden by PG implementation)
|
||||
match self.get("system_initialized").await? {
|
||||
Some(v) if v == "true" => Ok(false),
|
||||
_ => {
|
||||
self.set(
|
||||
"system_initialized",
|
||||
"true",
|
||||
"system",
|
||||
false,
|
||||
Some(admin_user_id),
|
||||
)
|
||||
.await?;
|
||||
Ok(true)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -97,4 +97,28 @@ impl SettingsRepository for SettingsPgRepository {
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Atomically claim system initialization using INSERT … ON CONFLICT DO NOTHING.
|
||||
///
|
||||
/// Only the first caller that inserts the row gets `rows_affected == 1`;
|
||||
/// concurrent callers see 0 rows affected and receive `false`.
|
||||
async fn try_claim_initialization(
|
||||
&self,
|
||||
admin_user_id: &str,
|
||||
) -> Result<bool, DomainError> {
|
||||
let result = sqlx::query(
|
||||
"INSERT INTO auth.admin_settings (key, value, category, is_secret, updated_by, updated_at)
|
||||
VALUES ('system_initialized', 'true', 'system', false, $1, NOW())
|
||||
ON CONFLICT (key) DO NOTHING"
|
||||
)
|
||||
.bind(admin_user_id)
|
||||
.execute(self.pool.as_ref())
|
||||
.await
|
||||
.map_err(|e| DomainError::new(
|
||||
ErrorKind::InternalError, "Settings", format!("DB error: {}", e),
|
||||
))?;
|
||||
|
||||
// rows_affected == 1 means we inserted; 0 means another caller already did
|
||||
Ok(result.rows_affected() == 1)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -354,6 +354,10 @@ async fn logout(
|
||||
/// Requires the setup token that was printed to the server log on first boot.
|
||||
/// Once the admin is created, the system is marked as initialized and this
|
||||
/// endpoint returns 403 for all subsequent requests.
|
||||
///
|
||||
/// Uses an atomic "claim" operation to prevent race conditions: even if two
|
||||
/// requests arrive simultaneously with the correct token, only one will
|
||||
/// succeed in marking the system as initialized and creating the admin.
|
||||
async fn setup_admin(
|
||||
State(state): State<Arc<AppState>>,
|
||||
Json(dto): Json<SetupAdminDto>,
|
||||
@@ -366,12 +370,14 @@ async fn setup_admin(
|
||||
.as_ref()
|
||||
.ok_or_else(|| AppError::internal_error("Authentication service not configured"))?;
|
||||
|
||||
// 2. Check if system is already initialized (fail-closed: DB error → deny)
|
||||
// 2. Verify admin settings service exists
|
||||
let admin_svc = state
|
||||
.admin_settings_service
|
||||
.as_ref()
|
||||
.ok_or_else(|| AppError::internal_error("Admin settings service not configured"))?;
|
||||
|
||||
// 3. Quick pre-check: if the system is already initialized, reject early
|
||||
// (avoids token validation and Argon2 work on obviously-late requests)
|
||||
if admin_svc.is_system_initialized().await {
|
||||
tracing::warn!(
|
||||
"Setup admin rejected: system already initialized (user: {})",
|
||||
@@ -384,7 +390,7 @@ async fn setup_admin(
|
||||
));
|
||||
}
|
||||
|
||||
// 3. Verify the one-time setup token
|
||||
// 4. Verify the one-time setup token
|
||||
let expected_token = state.setup_token.as_deref().ok_or_else(|| {
|
||||
AppError::new(
|
||||
StatusCode::FORBIDDEN,
|
||||
@@ -405,7 +411,30 @@ async fn setup_admin(
|
||||
));
|
||||
}
|
||||
|
||||
// 4. Create the first admin user
|
||||
// 5. ATOMIC: claim initialization — only one concurrent request can win.
|
||||
// We use a placeholder user_id ("pending") because the admin user
|
||||
// doesn't exist yet. It will be updated to the real id below.
|
||||
let claimed = admin_svc
|
||||
.try_claim_initialization("pending")
|
||||
.await
|
||||
.map_err(|e| {
|
||||
tracing::error!("Failed to claim system initialization: {}", e);
|
||||
AppError::internal_error("Failed to claim system initialization")
|
||||
})?;
|
||||
|
||||
if !claimed {
|
||||
tracing::warn!(
|
||||
"Setup admin rejected: another request already claimed initialization (user: {})",
|
||||
dto.username
|
||||
);
|
||||
return Err(AppError::new(
|
||||
StatusCode::FORBIDDEN,
|
||||
"System is already initialized. Use the admin panel to manage users.",
|
||||
"SystemAlreadyInitialized",
|
||||
));
|
||||
}
|
||||
|
||||
// 6. Create the first admin user (we hold the exclusive claim)
|
||||
let user = auth_service
|
||||
.auth_application_service
|
||||
.setup_create_admin(dto.username.clone(), dto.email, dto.password)
|
||||
@@ -415,13 +444,12 @@ async fn setup_admin(
|
||||
AppError::from(e)
|
||||
})?;
|
||||
|
||||
// 5. Mark system as initialized
|
||||
// 7. Update the initialization record with the real admin user_id
|
||||
if let Err(e) = admin_svc.mark_system_initialized(&user.id).await {
|
||||
// Admin was created but we couldn't mark as initialized.
|
||||
// This is not fatal — the setup token check prevents re-use, and
|
||||
// on next restart the system will detect the admin in DB.
|
||||
// Not fatal — the claim already prevents concurrent re-initialization,
|
||||
// and the "pending" marker is still "true" so the system stays locked.
|
||||
tracing::error!(
|
||||
"Created admin but failed to mark system as initialized: {}",
|
||||
"Created admin but failed to update initialized_by with real user id: {}",
|
||||
e
|
||||
);
|
||||
}
|
||||
|
||||
@@ -80,6 +80,7 @@ async fn handle_caldav_methods(
|
||||
) -> Result<Response<Body>, AppError> {
|
||||
let uri = req.uri().clone();
|
||||
let path = extract_caldav_path(uri.path());
|
||||
reject_path_traversal(&path)?;
|
||||
handle_caldav_methods_inner(state, req, path).await
|
||||
}
|
||||
|
||||
@@ -119,6 +120,18 @@ fn extract_caldav_path(uri_path: &str) -> String {
|
||||
percent_decode_str(encoded).decode_utf8_lossy().into_owned()
|
||||
}
|
||||
|
||||
/// Reject paths that contain path-traversal segments (`.` or `..`).
|
||||
fn reject_path_traversal(path: &str) -> Result<(), AppError> {
|
||||
for segment in path.split('/') {
|
||||
if segment == ".." || segment == "." {
|
||||
return Err(AppError::bad_request(
|
||||
"Path must not contain '.' or '..' segments",
|
||||
));
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// ─── Helper: extract user from request ───────────────────────────────
|
||||
|
||||
fn extract_user(req: &Request<Body>) -> Result<CurrentUser, AppError> {
|
||||
|
||||
@@ -66,6 +66,7 @@ async fn handle_carddav_methods(
|
||||
) -> Result<Response<Body>, AppError> {
|
||||
let uri = req.uri().clone();
|
||||
let path = extract_carddav_path(uri.path());
|
||||
reject_path_traversal(&path)?;
|
||||
handle_carddav_methods_inner(state, req, path).await
|
||||
}
|
||||
|
||||
@@ -107,6 +108,18 @@ fn extract_carddav_path(uri_path: &str) -> String {
|
||||
.into_owned()
|
||||
}
|
||||
|
||||
/// Reject paths that contain path-traversal segments (`.` or `..`).
|
||||
fn reject_path_traversal(path: &str) -> Result<(), AppError> {
|
||||
for segment in path.split('/') {
|
||||
if segment == ".." || segment == "." {
|
||||
return Err(AppError::bad_request(
|
||||
"Path must not contain '.' or '..' segments",
|
||||
));
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// ─── Helper: extract user from request ───────────────────────────────
|
||||
|
||||
fn extract_user(req: &Request<Body>) -> Result<CurrentUser, AppError> {
|
||||
|
||||
@@ -130,6 +130,22 @@ pub fn webdav_routes() -> Router<Arc<AppState>> {
|
||||
.route("/webdav", axum::routing::any(handle_webdav_methods_root))
|
||||
}
|
||||
|
||||
/// Reject paths that contain path-traversal segments (`.` or `..`).
|
||||
///
|
||||
/// Although deeper layers (PathResolver, StoragePath) also filter these out,
|
||||
/// blocking them at the HTTP boundary provides defense-in-depth and ensures
|
||||
/// no handler ever receives a traversal attempt.
|
||||
fn reject_path_traversal(path: &str) -> Result<(), AppError> {
|
||||
for segment in path.split('/') {
|
||||
if segment == ".." || segment == "." {
|
||||
return Err(AppError::bad_request(
|
||||
"Path must not contain '.' or '..' segments",
|
||||
));
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Extract the resource path from the request URI, stripping the `/webdav/` prefix
|
||||
/// and percent-decoding the result so that folder/file names with spaces and
|
||||
/// special characters match the values stored in the database.
|
||||
@@ -160,6 +176,7 @@ async fn handle_webdav_methods(
|
||||
req: Request<Body>,
|
||||
) -> Result<Response<Body>, AppError> {
|
||||
let path = extract_webdav_path(req.uri());
|
||||
reject_path_traversal(&path)?;
|
||||
handle_webdav_dispatch(state, req, path).await
|
||||
}
|
||||
|
||||
@@ -1109,6 +1126,9 @@ async fn handle_move(
|
||||
return Err(AppError::bad_request("Invalid destination URL"));
|
||||
};
|
||||
|
||||
// SECURITY: reject path-traversal in destination
|
||||
reject_path_traversal(&destination_path)?;
|
||||
|
||||
// Get services from state
|
||||
let file_retrieval_service = &state.applications.file_retrieval_service;
|
||||
let file_management_service = &state.applications.file_management_service;
|
||||
@@ -1206,6 +1226,12 @@ async fn handle_move(
|
||||
};
|
||||
|
||||
if source_parent_path != dest_parent_path {
|
||||
// SECURITY: verify destination parent belongs to caller (V-08)
|
||||
if !dest_parent_path.is_empty() {
|
||||
if let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await {
|
||||
assert_owner(parent.owner_id.as_deref(), &user.id, dest_parent_path)?;
|
||||
}
|
||||
}
|
||||
file_management_service
|
||||
.move_file(&file.id, Some(dest_parent_path.to_string()))
|
||||
.await
|
||||
@@ -1301,6 +1327,12 @@ async fn handle_move(
|
||||
};
|
||||
|
||||
if source_parent_path != dest_parent_path {
|
||||
// SECURITY: verify destination parent belongs to caller (V-08)
|
||||
if !dest_parent_path.is_empty() {
|
||||
if let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await {
|
||||
assert_owner(parent.owner_id.as_deref(), &user.id, dest_parent_path)?;
|
||||
}
|
||||
}
|
||||
file_management_service
|
||||
.move_file(&file.id, Some(dest_parent_path.to_string()))
|
||||
.await
|
||||
@@ -1367,6 +1399,9 @@ async fn handle_copy(
|
||||
return Err(AppError::bad_request("Invalid destination URL"));
|
||||
};
|
||||
|
||||
// SECURITY: reject path-traversal in destination
|
||||
reject_path_traversal(&destination_path)?;
|
||||
|
||||
// Get depth from Depth header
|
||||
let depth = req
|
||||
.headers()
|
||||
|
||||
Reference in New Issue
Block a user