fix: security audit — patch vulnerabilities V-02 through V-16

- 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
This commit is contained in:
Dionisio
2026-03-05 14:52:11 +01:00
parent b503e08384
commit 33cfb0faef
14 changed files with 265 additions and 58 deletions
+8
View File
@@ -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
@@ -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)
+71 -17
View File
@@ -58,15 +58,8 @@ impl User {
storage_quota_bytes: i64,
) -> UserResult<Self> {
// 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<Self> {
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
/// `<img/src=x>` 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(())
}
}
@@ -274,7 +274,7 @@ impl CalendarEventRepository for CalendarEventPgRepository {
calendar_id: &Uuid,
summary: &str,
) -> CalendarEventRepositoryResult<Vec<CalendarEvent>> {
let search_pattern = format!("%{}%", summary);
let search_pattern = super::like_escape(summary);
let rows = sqlx::query(
r#"
@@ -277,7 +277,7 @@ impl ContactRepository for ContactPgRepository {
}
async fn get_contacts_by_email(&self, email: &str) -> ContactRepositoryResult<Vec<Contact>> {
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<Vec<Contact>> {
let search_pattern = format!("%{}%", query);
let search_pattern = super::like_escape(query);
let rows = sqlx::query(
r#"
@@ -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<Vec<File>, DomainError> {
let pattern = format!("%{}%", query);
let pattern = super::like_escape(query);
let limit_i64 = limit as i64;
let rows: Vec<FileRow> = if let Some(fid) = folder_id {
@@ -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<Vec<Folder>, 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<Vec<Folder>, DomainError> {
let pattern = format!("%{}%", query);
let pattern = super::like_escape(query);
let limit_i64 = limit as i64;
let rows: Vec<FolderRow> = if let Some(pid) = parent_id {
+13
View File
@@ -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}%")
}
+26 -6
View File
@@ -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.
+59 -4
View File
@@ -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''<percent-encoded>` 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, String>) -> 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.
@@ -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()
+30 -6
View File
@@ -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,
}
};
+24 -13
View File
@@ -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<B>(req: &Request<B>) -> 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();
}
}
}
+2 -2
View File
@@ -122,10 +122,10 @@ async function changePassword(e) {
document.getElementById('password-form').reset();
} else {
const err = await resp.json().catch(() => ({}));
statusEl.innerHTML = '<div class="alert alert-error"><i class="fas fa-exclamation-circle"></i> ' + (err.message || 'Failed to change password') + '</div>';
statusEl.innerHTML = '<div class="alert alert-error"><i class="fas fa-exclamation-circle"></i> ' + escapeHtml(err.message || 'Failed to change password') + '</div>';
}
} catch (err) {
statusEl.innerHTML = '<div class="alert alert-error"><i class="fas fa-exclamation-circle"></i> Network error: ' + err.message + '</div>';
statusEl.innerHTML = '<div class="alert alert-error"><i class="fas fa-exclamation-circle"></i> Network error: ' + escapeHtml(err.message) + '</div>';
}
btn.disabled = false;