fix: cora scan security findings (traversal, byte-slice panic, header inject, orphan file)

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::<axum::http::HeaderName>().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");
+    }
+}

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<OAuthIdentity> {
     })
 }
 
+/// 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), "");
+    }
 }

crates/rungu-core/src/storage.rs

@@ -88,11 +88,26 @@ impl FsStorage {
     }
 
     fn resolve_path(&self, key: &str) -> Result<PathBuf> {
-        // 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))
     }
 }