From 33cfb0faefe994a255cc3154e2069fd8e277d3c5 Mon Sep 17 00:00:00 2001 From: Dionisio Date: Thu, 5 Mar 2026 14:52:11 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20security=20audit=20=E2=80=94=20patch=20v?= =?UTF-8?q?ulnerabilities=20V-02=20through=20V-16?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - V-02: XSS via innerHTML in profile.js — wrap err.message in escapeHtml() - V-03: IDOR upload to other users' folders — add folder ownership check - V-04: IDOR create folders in other users' trees — add parent ownership check - V-06: Content-Disposition header injection — RFC 5987 percent-encoding - V-08: WebDAV MOVE/COPY destination without ownership — add assert_owner checks - V-09: .gitignore missing cert/key patterns — add *.pem, *.key, *.p12, etc. - V-11: Username accepts XSS payloads — restrict to [a-zA-Z0-9._-] - V-12: Minimal email validation — reject forbidden chars, require domain dot - V-13: admin_reset_password doesn't invalidate sessions — revoke all sessions - V-14: Rate limiting bypassable via X-Forwarded-For — gate behind OXICLOUD_TRUST_PROXY_HEADERS - V-15: Cookie Secure flag off by default — default to true (safe-by-default) - V-16: LIKE wildcard injection in searches — add like_escape() helper across 9 sites --- .gitignore | 8 ++ .../services/auth_application_service.rs | 11 ++- src/domain/entities/user.rs | 88 +++++++++++++++---- .../pg/calendar_event_pg_repository.rs | 2 +- .../repositories/pg/contact_pg_repository.rs | 4 +- .../pg/file_blob_read_repository.rs | 6 +- .../repositories/pg/folder_db_repository.rs | 6 +- src/infrastructure/repositories/pg/mod.rs | 13 +++ src/interfaces/api/cookie_auth.rs | 32 +++++-- src/interfaces/api/handlers/file_handler.rs | 63 ++++++++++++- src/interfaces/api/handlers/folder_handler.rs | 13 +++ src/interfaces/api/handlers/webdav_handler.rs | 36 ++++++-- src/interfaces/middleware/rate_limit.rs | 37 +++++--- static/js/views/profile/profile.js | 4 +- 14 files changed, 265 insertions(+), 58 deletions(-) diff --git a/.gitignore b/.gitignore index d5d47b6c..c8d20b01 100644 --- a/.gitignore +++ b/.gitignore @@ -59,6 +59,14 @@ logs/ # Storage data (user files, blobs — never commit) storage/ +# TLS certificates and private keys — NEVER commit +*.pem +*.key +*.p12 +*.pfx +*.crt +*.csr + # Temporary files *.tmp *.bak diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 5163eaed..1c595fc7 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -731,7 +731,16 @@ impl AuthApplicationService { )); } let hash = self.password_hasher.hash_password(new_password).await?; - self.user_storage.change_password(user_id, &hash).await + self.user_storage.change_password(user_id, &hash).await?; + + // Invalidate all existing sessions so the user must re-login + // with the new password. Mirrors the behaviour of change_password(). + self.session_storage + .revoke_all_user_sessions(user_id) + .await?; + + tracing::info!(user_id = %user_id, "Admin reset password — all sessions revoked"); + Ok(()) } /// Get a single user by ID (for admin panel) diff --git a/src/domain/entities/user.rs b/src/domain/entities/user.rs index 1cb1c88a..baaf86a6 100644 --- a/src/domain/entities/user.rs +++ b/src/domain/entities/user.rs @@ -58,15 +58,8 @@ impl User { storage_quota_bytes: i64, ) -> UserResult { // Validations - if username.is_empty() || username.len() < 3 || username.len() > 32 { - return Err(UserError::InvalidUsername( - "Username must be between 3 and 32 characters".to_string(), - )); - } - - if !email.contains('@') || email.len() < 5 { - return Err(UserError::ValidationError("Invalid email".to_string())); - } + Self::validate_username(&username)?; + Self::validate_email(&email)?; if password_hash.is_empty() { return Err(UserError::InvalidPassword( @@ -102,14 +95,8 @@ impl User { oidc_provider: String, oidc_subject: String, ) -> UserResult { - if username.is_empty() || username.len() < 3 || username.len() > 32 { - return Err(UserError::InvalidUsername( - "Username must be between 3 and 32 characters".to_string(), - )); - } - if !email.contains('@') || email.len() < 5 { - return Err(UserError::ValidationError("Invalid email".to_string())); - } + Self::validate_username(&username)?; + Self::validate_email(&email)?; let now = Utc::now(); Ok(Self { id: Uuid::new_v4().to_string(), @@ -283,4 +270,71 @@ impl User { self.active = true; self.updated_at = Utc::now(); } + + // ── Shared validation helpers ────────────────────────────────────── + + /// Usernames must be 3-32 chars and contain only ASCII alphanumerics, + /// hyphens, underscores, and dots. This prevents XSS payloads like + /// `` from being stored as usernames. + fn validate_username(username: &str) -> UserResult<()> { + if username.len() < 3 || username.len() > 32 { + return Err(UserError::InvalidUsername( + "Username must be between 3 and 32 characters".to_string(), + )); + } + if !username + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') + { + return Err(UserError::InvalidUsername( + "Username may only contain letters, digits, hyphens, underscores, and dots" + .to_string(), + )); + } + // Disallow leading/trailing dots or hyphens + if username.starts_with('.') || username.starts_with('-') + || username.ends_with('.') || username.ends_with('-') + { + return Err(UserError::InvalidUsername( + "Username must not start or end with a dot or hyphen".to_string(), + )); + } + Ok(()) + } + + /// Basic but meaningful email validation: + /// - Must contain exactly one `@` + /// - Local part and domain must be non-empty + /// - Domain must contain at least one dot + /// - No angle brackets, spaces, or other characters used in XSS payloads + fn validate_email(email: &str) -> UserResult<()> { + let parts: Vec<&str> = email.splitn(2, '@').collect(); + if parts.len() != 2 { + return Err(UserError::ValidationError("Invalid email: missing @".to_string())); + } + let (local, domain) = (parts[0], parts[1]); + if local.is_empty() || domain.is_empty() { + return Err(UserError::ValidationError( + "Invalid email: empty local part or domain".to_string(), + )); + } + if !domain.contains('.') { + return Err(UserError::ValidationError( + "Invalid email: domain must contain a dot".to_string(), + )); + } + // Reject characters commonly used in XSS / header injection + let forbidden = ['<', '>', '"', '\'', '\\', ' ', '\t', '\n', '\r', '(', ')', ',', ';']; + if email.chars().any(|c| forbidden.contains(&c)) { + return Err(UserError::ValidationError( + "Invalid email: contains forbidden characters".to_string(), + )); + } + if email.len() > 254 { + return Err(UserError::ValidationError( + "Invalid email: too long (max 254 characters)".to_string(), + )); + } + Ok(()) + } } diff --git a/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs b/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs index 6436b7db..8af1a991 100644 --- a/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs +++ b/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs @@ -274,7 +274,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { calendar_id: &Uuid, summary: &str, ) -> CalendarEventRepositoryResult> { - let search_pattern = format!("%{}%", summary); + let search_pattern = super::like_escape(summary); let rows = sqlx::query( r#" diff --git a/src/infrastructure/repositories/pg/contact_pg_repository.rs b/src/infrastructure/repositories/pg/contact_pg_repository.rs index 5aa9e454..da145b87 100644 --- a/src/infrastructure/repositories/pg/contact_pg_repository.rs +++ b/src/infrastructure/repositories/pg/contact_pg_repository.rs @@ -277,7 +277,7 @@ impl ContactRepository for ContactPgRepository { } async fn get_contacts_by_email(&self, email: &str) -> ContactRepositoryResult> { - let search_pattern = format!("%{}%", email); + let search_pattern = super::like_escape(email); let rows = sqlx::query( r#" @@ -339,7 +339,7 @@ impl ContactRepository for ContactPgRepository { address_book_id: &Uuid, query: &str, ) -> ContactRepositoryResult> { - let search_pattern = format!("%{}%", query); + let search_pattern = super::like_escape(query); let rows = sqlx::query( r#" diff --git a/src/infrastructure/repositories/pg/file_blob_read_repository.rs b/src/infrastructure/repositories/pg/file_blob_read_repository.rs index f88432e1..8cafd120 100644 --- a/src/infrastructure/repositories/pg/file_blob_read_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_read_repository.rs @@ -736,7 +736,7 @@ impl FileReadPort for FileBlobReadRepository { if let Some(name) = &criteria.name_contains && name.len() >= 3 { - query = query.bind(format!("%{}%", name)); + query = query.bind(super::like_escape(name)); } query = query.bind(limit).bind(offset); @@ -895,7 +895,7 @@ impl FileReadPort for FileBlobReadRepository { if let Some(name) = &criteria.name_contains && name.len() >= 3 { - query = query.bind(format!("%{}%", name)); + query = query.bind(super::like_escape(name)); } if let Some(types) = &criteria.file_types && !types.is_empty() @@ -963,7 +963,7 @@ impl FileReadPort for FileBlobReadRepository { query: &str, limit: usize, ) -> Result, DomainError> { - let pattern = format!("%{}%", query); + let pattern = super::like_escape(query); let limit_i64 = limit as i64; let rows: Vec = if let Some(fid) = folder_id { diff --git a/src/infrastructure/repositories/pg/folder_db_repository.rs b/src/infrastructure/repositories/pg/folder_db_repository.rs index 77072e2b..73e43843 100644 --- a/src/infrastructure/repositories/pg/folder_db_repository.rs +++ b/src/infrastructure/repositories/pg/folder_db_repository.rs @@ -739,7 +739,7 @@ impl FolderRepository for FolderDbRepository { } else { " AND fo.name ILIKE $3" }, - Some(format!("%{}%", name)), + Some(super::like_escape(name)), ), _ => ("", None), }; @@ -861,7 +861,7 @@ impl FolderRepository for FolderDbRepository { user_id: &str, ) -> Result, DomainError> { let (where_extra, name_pattern) = match name_contains { - Some(name) if name.len() >= 3 => (" AND fo.name ILIKE $3", Some(format!("%{}%", name))), + Some(name) if name.len() >= 3 => (" AND fo.name ILIKE $3", Some(super::like_escape(name))), _ => ("", None), }; @@ -908,7 +908,7 @@ impl FolderRepository for FolderDbRepository { query: &str, limit: usize, ) -> Result, DomainError> { - let pattern = format!("%{}%", query); + let pattern = super::like_escape(query); let limit_i64 = limit as i64; let rows: Vec = if let Some(pid) = parent_id { diff --git a/src/infrastructure/repositories/pg/mod.rs b/src/infrastructure/repositories/pg/mod.rs index 88e0a4b7..b456d537 100644 --- a/src/infrastructure/repositories/pg/mod.rs +++ b/src/infrastructure/repositories/pg/mod.rs @@ -38,3 +38,16 @@ pub use settings_pg_repository::SettingsPgRepository; pub use share_pg_repository::SharePgRepository; pub use trash_db_repository::TrashDbRepository; pub use user_pg_repository::UserPgRepository; + +// ── SQL helpers ───────────────────────────────────────────────────────────── + +/// Escape SQL `LIKE` / `ILIKE` wildcard characters (`%` and `_`) in user +/// input and wrap the result in `%…%` for a contains-match. +/// +/// Without this, a user searching for `100%` would match *every* row because +/// `%` is a wildcard in LIKE patterns. +#[inline] +pub fn like_escape(raw: &str) -> String { + let escaped = raw.replace('\\', "\\\\").replace('%', "\\%").replace('_', "\\_"); + format!("%{escaped}%") +} diff --git a/src/interfaces/api/cookie_auth.rs b/src/interfaces/api/cookie_auth.rs index d144c4f1..c52cfaca 100644 --- a/src/interfaces/api/cookie_auth.rs +++ b/src/interfaces/api/cookie_auth.rs @@ -26,16 +26,36 @@ pub const CSRF_COOKIE: &str = "oxicloud_csrf"; pub const CSRF_HEADER: &str = "x-csrf-token"; /// Whether the `Secure` flag should be set on cookies. -/// Auto-detected from `OXICLOUD_BASE_URL` (if it starts with `https`) -/// or overridden with `OXICLOUD_COOKIE_SECURE=true|false`. +/// +/// Resolution order: +/// 1. `OXICLOUD_COOKIE_SECURE=true|false` — explicit override. +/// 2. `OXICLOUD_BASE_URL` starts with `https` → `true`. +/// 3. **Default: `true`** (safe-by-default). Set `OXICLOUD_COOKIE_SECURE=false` +/// explicitly for plain-HTTP development environments. fn cookie_secure() -> bool { if let Ok(v) = std::env::var("OXICLOUD_COOKIE_SECURE") { - return v == "true" || v == "1"; + let secure = v == "true" || v == "1"; + if !secure { + tracing::warn!( + "OXICLOUD_COOKIE_SECURE is explicitly disabled — \ + cookies will be sent over plain HTTP. \ + Do NOT use this in production." + ); + } + return secure; } - // Auto-detect from base URL - std::env::var("OXICLOUD_BASE_URL") + // Auto-detect from base URL, defaulting to secure when unset + let secure = std::env::var("OXICLOUD_BASE_URL") .map(|u| u.starts_with("https")) - .unwrap_or(false) + .unwrap_or(true); + if !secure { + tracing::warn!( + "OXICLOUD_BASE_URL does not start with https — \ + cookie Secure flag is OFF. Set OXICLOUD_COOKIE_SECURE=true \ + to override if your proxy terminates TLS." + ); + } + secure } /// Build a `Set-Cookie` header value. diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index 83913852..66784b1b 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -102,6 +102,22 @@ impl FileHandler { .unwrap_or("application/octet-stream") .to_string(); + // ── SECURITY: Verify folder ownership before upload (IDOR V-03 fix) ── + if let Some(ref fid) = folder_id { + use crate::application::ports::inbound::FolderUseCase; + let folder_service = &state.applications.folder_service; + if folder_service.get_folder_owned(fid, &auth_user.id).await.is_err() { + tracing::warn!( + "⛔ UPLOAD REJECTED (IDOR): user='{}' attempted upload to folder '{}' owned by another user", + auth_user.username, + fid, + ); + return Err(Self::domain_error_response( + crate::common::errors::DomainError::not_found("Folder", fid), + )); + } + } + // ── Early quota check (before spooling to disk) ────── if let Some(storage_svc) = state.storage_usage_service.as_ref() { let estimated_size = field @@ -707,20 +723,59 @@ impl FileHandler { // ═══════════════════════════════════════════════════════════════════════ /// Build a Content-Disposition header value. + /// + /// Uses RFC 5987 `filename*=UTF-8''` to safely handle + /// filenames with quotes, non-ASCII characters, or other special chars. + /// A sanitised ASCII `filename=` fallback is included for legacy clients. fn content_disposition(name: &str, mime: &str, params: &HashMap) -> String { let force_inline = params .get("inline") .is_some_and(|v| v == "true" || v == "1"); - if force_inline + let disposition = if force_inline || mime.starts_with("image/") || mime == "application/pdf" || mime.starts_with("video/") || mime.starts_with("audio/") { - format!("inline; filename=\"{}\"", name) + "inline" } else { - format!("attachment; filename=\"{}\"", name) - } + "attachment" + }; + + // RFC 5987 percent-encode for filename* (attr-char safe set) + use percent_encoding::{AsciiSet, NON_ALPHANUMERIC, utf8_percent_encode}; + // Characters that DON'T need encoding per RFC 5987 attr-char: + // ALPHA / DIGIT / "!" / "#" / "$" / "&" / "+" / "-" / "." / + // "^" / "_" / "`" / "|" / "~" + const RFC5987_SET: &AsciiSet = &NON_ALPHANUMERIC + .remove(b'!') + .remove(b'#') + .remove(b'$') + .remove(b'&') + .remove(b'+') + .remove(b'-') + .remove(b'.') + .remove(b'^') + .remove(b'_') + .remove(b'`') + .remove(b'|') + .remove(b'~'); + let encoded = utf8_percent_encode(name, RFC5987_SET).to_string(); + + // ASCII fallback: strip anything outside printable ASCII and + // replace '"' and '\\' to prevent header injection. + let ascii_safe: String = name + .chars() + .filter(|c| c.is_ascii_graphic() || *c == ' ') + .map(|c| match c { + '"' | '\\' => '_', + _ => c, + }) + .collect(); + + format!( + "{disposition}; filename=\"{ascii_safe}\"; filename*=UTF-8''{encoded}" + ) } /// Build a 201 Created JSON response. diff --git a/src/interfaces/api/handlers/folder_handler.rs b/src/interfaces/api/handlers/folder_handler.rs index 7abed479..9edb4c60 100644 --- a/src/interfaces/api/handlers/folder_handler.rs +++ b/src/interfaces/api/handlers/folder_handler.rs @@ -67,6 +67,19 @@ impl FolderHandler { } } + // ── SECURITY: Verify parent folder ownership (IDOR V-04 fix) ── + if let Some(ref parent_id) = dto.parent_id { + use crate::application::ports::inbound::FolderUseCase; + if service.get_folder_owned(parent_id, &auth_user.id).await.is_err() { + tracing::warn!( + "create_folder: user '{}' attempted to create folder in parent '{}' owned by another user", + auth_user.username, + parent_id, + ); + return AppError::not_found(format!("Parent folder not found: {}", parent_id)).into_response(); + } + } + match service.create_folder(dto).await { Ok(folder) => (StatusCode::CREATED, Json(folder)).into_response(), Err(err) => AppError::from(err).into_response() diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 279c7c17..871333d8 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -1160,7 +1160,11 @@ async fn handle_move( None } else { match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => Some(parent.id), + Ok(parent) => { + // SECURITY: verify destination parent belongs to caller (V-08) + assert_owner(parent.owner_id.as_deref(), &user.id, dest_parent_path)?; + Some(parent.id) + } Err(_) => None, } }, @@ -1246,7 +1250,11 @@ async fn handle_move( None } else { match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => Some(parent.id), + Ok(parent) => { + // SECURITY: verify destination parent belongs to caller (V-08) + assert_owner(parent.owner_id.as_deref(), &user.id, dest_parent_path)?; + Some(parent.id) + } Err(_) => None, } }, @@ -1417,7 +1425,11 @@ async fn handle_copy( None } else { match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => Some(parent.id), + Ok(parent) => { + // SECURITY: verify destination parent belongs to caller (V-08) + assert_owner(parent.owner_id.as_deref(), &user.id, dest_parent_path)?; + Some(parent.id) + } Err(_) => None, } }; @@ -1461,7 +1473,11 @@ async fn handle_copy( None } else { match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => Some(parent.id), + Ok(parent) => { + // SECURITY: verify destination parent belongs to caller (V-08) + assert_owner(parent.owner_id.as_deref(), &user.id, dest_parent_path)?; + Some(parent.id) + } Err(_) => None, } }; @@ -1501,7 +1517,11 @@ async fn handle_copy( None } else { match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => Some(parent.id), + Ok(parent) => { + // SECURITY: verify destination parent belongs to caller (V-08) + assert_owner(parent.owner_id.as_deref(), &user.id, dest_parent_path)?; + Some(parent.id) + } Err(_) => None, } }; @@ -1552,7 +1572,11 @@ async fn handle_copy( None } else { match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => Some(parent.id), + Ok(parent) => { + // SECURITY: verify destination parent belongs to caller (V-08) + assert_owner(parent.owner_id.as_deref(), &user.id, dest_parent_path)?; + Some(parent.id) + } Err(_) => None, } }; diff --git a/src/interfaces/middleware/rate_limit.rs b/src/interfaces/middleware/rate_limit.rs index d9d0789c..37443c1d 100644 --- a/src/interfaces/middleware/rate_limit.rs +++ b/src/interfaces/middleware/rate_limit.rs @@ -89,24 +89,35 @@ impl RateLimiter { // ─── Axum middleware factories ────────────────────────────────────────────── /// Extract the most-likely real client IP from headers / connection info. +/// +/// Proxy headers (`X-Forwarded-For`, `X-Real-Ip`) are only trusted when +/// `OXICLOUD_TRUST_PROXY_HEADERS=true` is set. Without a trusted reverse +/// proxy in front of the app, an attacker can spoof these headers to bypass +/// rate limiting. pub fn extract_client_ip(req: &Request) -> String { + let trust_proxy = std::env::var("OXICLOUD_TRUST_PROXY_HEADERS") + .map(|v| v == "true" || v == "1") + .unwrap_or(false); + let headers = req.headers(); - // 1. X-Forwarded-For (first entry — closest to the client) - if let Some(xff) = headers.get("x-forwarded-for").and_then(|v| v.to_str().ok()) - && let Some(first) = xff.split(',').next() - { - let ip = first.trim(); - if !ip.is_empty() { - return ip.to_string(); + if trust_proxy { + // 1. X-Forwarded-For (first entry — closest to the client) + if let Some(xff) = headers.get("x-forwarded-for").and_then(|v| v.to_str().ok()) + && let Some(first) = xff.split(',').next() + { + let ip = first.trim(); + if !ip.is_empty() { + return ip.to_string(); + } } - } - // 2. X-Real-Ip - if let Some(xri) = headers.get("x-real-ip").and_then(|v| v.to_str().ok()) { - let ip = xri.trim(); - if !ip.is_empty() { - return ip.to_string(); + // 2. X-Real-Ip + if let Some(xri) = headers.get("x-real-ip").and_then(|v| v.to_str().ok()) { + let ip = xri.trim(); + if !ip.is_empty() { + return ip.to_string(); + } } } diff --git a/static/js/views/profile/profile.js b/static/js/views/profile/profile.js index 662a1d10..3cfd6547 100644 --- a/static/js/views/profile/profile.js +++ b/static/js/views/profile/profile.js @@ -122,10 +122,10 @@ async function changePassword(e) { document.getElementById('password-form').reset(); } else { const err = await resp.json().catch(() => ({})); - statusEl.innerHTML = '
' + (err.message || 'Failed to change password') + '
'; + statusEl.innerHTML = '
' + escapeHtml(err.message || 'Failed to change password') + '
'; } } catch (err) { - statusEl.innerHTML = '
Network error: ' + err.message + '
'; + statusEl.innerHTML = '
Network error: ' + escapeHtml(err.message) + '
'; } btn.disabled = false;