diff --git a/crates/rungu-api/src/attachment_routes.rs b/crates/rungu-api/src/attachment_routes.rs index 75eb259..7a4a4eb 100644 --- a/crates/rungu-api/src/attachment_routes.rs +++ b/crates/rungu-api/src/attachment_routes.rs @@ -106,12 +106,23 @@ pub async fn upload_attachment( // Save to storage state.storage.save(&key, data.to_vec()).await.map_err(|_| ApiError::internal_default())?; - // Save metadata to DB - let attachment = state + // Save metadata to DB. If this fails, remove the just-written file so + // it doesn't leak as an orphan on disk (defense against partial-failure + // disk growth). + let attachment = match state .store .create_attachment(&post_id, &filename, &verified_mime, data.len() as i64, &key, &user.id) .await - .map_err(|_| ApiError::internal_default())?; + { + Ok(a) => a, + Err(e) => { + tracing::warn!(error = %e, key = %key, "DB insert failed; cleaning up orphaned attachment file"); + if let Err(cleanup_err) = state.storage.delete(&key).await { + tracing::warn!(error = %cleanup_err, key = %key, "Failed to clean up orphaned attachment file"); + } + return Err(ApiError::internal_default()); + } + }; let attachment_id = attachment.id.clone(); let response = AttachmentResponse { @@ -196,11 +207,18 @@ pub async fn get_attachment_file( let data = state.storage.load(&storage_path).await.map_err(|_| ApiError::not_found("File not found in storage"))?; + // Sanitize the filename for the Content-Disposition header. The stored + // filename is user-supplied (from the upload), so it must never carry CR/LF + // (header injection) or unescaped quotes (header breaking). We also strip + // control chars. On anything unsafe we fall back to a neutral name so the + // download still works. + let safe_filename = sanitize_filename(&attachment.filename); + Ok(( StatusCode::OK, [ (header::CONTENT_TYPE, attachment.mime), - (header::CONTENT_DISPOSITION, format!("inline; filename=\"{}\"", attachment.filename)), + (header::CONTENT_DISPOSITION, format!("inline; filename=\"{}\"", safe_filename)), (header::CACHE_CONTROL, "public, max-age=86400".to_string()), ("x-content-type-options".parse::().unwrap(), "nosniff".to_string()), ], @@ -249,3 +267,45 @@ pub async fn delete_attachment( Ok(StatusCode::NO_CONTENT) } + +/// Make a stored filename safe to embed in a `Content-Disposition` header value. +/// +/// The filename originates from a user upload, so it must not carry: +/// - CR/LF (would split/inject HTTP headers), +/// - `"` or `\` (would break out of the quoted filename token), +/// - other control characters. +/// +/// Anything unsafe is replaced with `_`; a fully-unsafe/empty result falls +/// back to a neutral `download` so the response still works. +fn sanitize_filename(name: &str) -> String { + let cleaned: String = name + .chars() + .map(|c| match c { + '\r' | '\n' | '"' | '\\' => '_', + c if c.is_control() => '_', + c => c, + }) + .collect(); + let cleaned = cleaned.trim(); + if cleaned.is_empty() { "download".to_string() } else { cleaned.to_string() } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn sanitize_filename_strips_header_injection() { + // CR/LF would split headers → neutralized. + assert_eq!(sanitize_filename("a.txt\r\nX-Evil: 1"), "a.txt__X-Evil: 1"); + // Quotes/backslash break the quoted token. + assert_eq!(sanitize_filename("a\"b\\c.png"), "a_b_c.png"); + // Control chars removed. + assert_eq!(sanitize_filename("a\u{0000}b"), "a_b"); + // Empty / whitespace-only → neutral fallback. + assert_eq!(sanitize_filename(" "), "download"); + assert_eq!(sanitize_filename(""), "download"); + // Clean name passes through. + assert_eq!(sanitize_filename("screenshot.png"), "screenshot.png"); + } +} diff --git a/crates/rungu-api/src/oauth.rs b/crates/rungu-api/src/oauth.rs index 84e5858..404a410 100644 --- a/crates/rungu-api/src/oauth.rs +++ b/crates/rungu-api/src/oauth.rs @@ -58,8 +58,11 @@ pub async fn exchange_code(client: &reqwest::Client, cfg: &ProviderConfig, code: let status = resp.status(); if !status.is_success() { let body = resp.text().await.unwrap_or_default(); - // Redact potential secrets — only log first 200 chars, never full error body - let redacted = if body.len() > 200 { &body[..200] } else { &body }; + // Redact potential secrets — only log a short preview, never the full + // error body. Bound by *characters* not bytes: a byte slice + // (`&body[..200]`) would panic if byte 200 falls inside a multibyte + // UTF-8 sequence, which is common in provider error responses. + let redacted = preview(&body, 200); tracing::error!(status = %status, url = %cfg.token_url, body_preview = %redacted, "Token exchange failed"); bail!("Token exchange failed: HTTP {status}"); } @@ -91,7 +94,7 @@ pub async fn fetch_identity( let status = resp.status(); if !status.is_success() { let body = resp.text().await.unwrap_or_default(); - let redacted = if body.len() > 200 { &body[..200] } else { &body }; + let redacted = preview(&body, 200); tracing::error!(status = %status, url = %userinfo_url, body_preview = %redacted, "Userinfo fetch failed"); bail!("Userinfo fetch failed: HTTP {status}"); } @@ -235,6 +238,13 @@ fn parse_keycloak(v: &serde_json::Value) -> Result { }) } +/// Take up to `max_chars` characters from the start of `s`. Used to preview +/// provider error bodies for logging without risking a panic on multibyte +/// UTF-8 (which a byte-length slice like `&s[..n]` would cause). +fn preview(s: &str, max_chars: usize) -> String { + s.chars().take(max_chars).collect() +} + #[cfg(test)] mod tests { use super::*; @@ -322,4 +332,15 @@ mod tests { }; assert_eq!(derived, "https://api.github.com/user/emails"); } + + #[test] + fn preview_never_panics_on_multibyte_and_truncates_by_char() { + // "é" is 2 bytes; a byte slice at an odd length would panic. + let s = "éééabc"; + assert_eq!(preview(s, 3), "ééé"); + // Shorter than the cap → returned whole. + assert_eq!(preview(s, 100), s); + // Empty input is fine. + assert_eq!(preview("", 5), ""); + } } diff --git a/crates/rungu-core/src/storage.rs b/crates/rungu-core/src/storage.rs index 49d2e97..d5b7982 100644 --- a/crates/rungu-core/src/storage.rs +++ b/crates/rungu-core/src/storage.rs @@ -88,11 +88,26 @@ impl FsStorage { } fn resolve_path(&self, key: &str) -> Result { - // Prevent path traversal — reject keys with .. or absolute paths - if key.contains("..") || key.starts_with('/') { + // Path-traversal defense. The authoritative gate is the *component* + // check below: reject absolute keys and any key containing a + // `ParentDir` ("..") component. Storage keys are uuid-derived + // (see `storage_key`), so after this check the joined path can only + // descend under `base_dir` — it cannot escape. + // + // We intentionally check *components*, not a naive `contains("..")` + // substring: the substring form both misses some traversals and + // falsely rejects legitimate filenames containing consecutive dots + // (e.g. "my..file.png"). + // + // `canonicalize()` is intentionally NOT used here: it requires the + // target to already exist, which is false on the write path (the file + // is about to be created), and the keyspace is internal/uuid so there + // is no user-controlled filename reaching this function. + let key_path = std::path::Path::new(key); + if key_path.is_absolute() || key_path.components().any(|c| matches!(c, std::path::Component::ParentDir)) { bail!("Invalid storage key"); } - Ok(self.base_dir.join(key)) + Ok(self.base_dir.join(key_path)) } }