diff --git a/crates/manager-core/src/files_api.rs b/crates/manager-core/src/files_api.rs index 8de6c73..dd4f90c 100644 --- a/crates/manager-core/src/files_api.rs +++ b/crates/manager-core/src/files_api.rs @@ -123,10 +123,20 @@ async fn list_files( } /// `GET /apps/{id}/files/{collection}/{file_id}` — stream the file's -/// bytes inline. The dashboard's Download button hits this; the audit -/// flagged the missing handler (callers were 405'ing). Metadata gives -/// the Content-Type + Content-Disposition; bytes come from -/// `FilesRepo::get` (which checksum-verifies on read). +/// bytes as a download. The dashboard's Download button hits this; +/// metadata is read via `FilesRepo::head`, bytes via `FilesRepo::get` +/// (which checksum-verifies on read). +/// +/// Audit 2026-06-11 C-2 — response hardening. The previous handler +/// served `Content-Disposition: inline` with the user-supplied +/// `Content-Type` and no `nosniff` / CSP, so an SVG or HTML upload +/// rendered same-origin and hijacked the admin session. Now: +/// * `Content-Disposition: attachment` always. +/// * `Content-Type` re-sanitized to the storage allowlist as +/// belt-and-suspenders (`files_service::create` / `update` already +/// coerce at write, but older rows may pre-date that change). +/// * `X-Content-Type-Options: nosniff` blocks MIME-sniffing fallbacks. +/// * Restrictive CSP scopes any residual rendering attempt. async fn get_file( State(s): State, Extension(principal): Extension, @@ -150,16 +160,23 @@ async fn get_file( .get(app_id, &collection, id) .await? .ok_or(FilesApiError::NotFound)?; + let safe_ct = picloud_shared::sanitize_stored_content_type(&meta.content_type); let disposition = format!( - "inline; filename=\"{}\"", + "attachment; filename=\"{}\"", meta.name.replace('"', "").replace(['\r', '\n'], "") ); let len = bytes.len(); Ok(Response::builder() .status(StatusCode::OK) - .header(CONTENT_TYPE, meta.content_type) + .header(CONTENT_TYPE, safe_ct) .header(CONTENT_DISPOSITION, disposition) .header(CONTENT_LENGTH, len) + .header("X-Content-Type-Options", "nosniff") + .header( + "Content-Security-Policy", + "default-src 'none'; sandbox; frame-ancestors 'none'", + ) + .header("Referrer-Policy", "no-referrer") .body(Body::from(bytes)) .expect("build files download response")) } diff --git a/crates/manager-core/src/files_service.rs b/crates/manager-core/src/files_service.rs index 8464db0..ddc8c42 100644 --- a/crates/manager-core/src/files_service.rs +++ b/crates/manager-core/src/files_service.rs @@ -17,8 +17,8 @@ use std::sync::Arc; use async_trait::async_trait; use picloud_shared::{ - validate_files_collection, FileMeta, FileUpdate, FilesError, FilesListPage, FilesService, - NewFile, SdkCallCx, ServiceEvent, ServiceEventEmitter, + sanitize_stored_content_type, validate_files_collection, FileMeta, FileUpdate, FilesError, + FilesListPage, FilesService, NewFile, SdkCallCx, ServiceEvent, ServiceEventEmitter, }; use uuid::Uuid; @@ -124,11 +124,14 @@ impl FilesService for FilesServiceImpl { &self, cx: &SdkCallCx, collection: &str, - new: NewFile, + mut new: NewFile, ) -> Result { validate_files_collection(collection)?; self.check_write(cx).await?; new.validate(self.max_file_size_bytes)?; + // Audit 2026-06-11 C-2 — coerce dangerous render types to + // application/octet-stream after the shape checks pass. + new.content_type = sanitize_stored_content_type(&new.content_type); let meta = self.repo.create(cx.app_id, collection, new).await?; self.emit(cx, "create", collection, &meta, None).await; Ok(meta.id) @@ -167,11 +170,15 @@ impl FilesService for FilesServiceImpl { cx: &SdkCallCx, collection: &str, id: &str, - upd: FileUpdate, + mut upd: FileUpdate, ) -> Result<(), FilesError> { validate_files_collection(collection)?; self.check_write(cx).await?; upd.validate(self.max_file_size_bytes)?; + // Audit 2026-06-11 C-2 — sanitize after the shape checks pass. + if let Some(ct) = upd.content_type.as_deref() { + upd.content_type = Some(sanitize_stored_content_type(ct)); + } let Some(uuid) = parse_id(id) else { return Err(FilesError::NotFound); }; @@ -479,6 +486,68 @@ mod tests { } } + #[tokio::test] + async fn dangerous_render_content_type_is_coerced_on_create() { + // Audit 2026-06-11 C-2 closure. + let files = svc(); + let cx = anon_cx(AppId::new()); + let id = files + .create( + &cx, + "c", + NewFile { + name: "evil.html".into(), + content_type: "text/html".into(), + data: b"".to_vec(), + }, + ) + .await + .unwrap(); + let meta = files + .head(&cx, "c", &id.to_string()) + .await + .unwrap() + .unwrap(); + assert_eq!(meta.content_type, "application/octet-stream"); + } + + #[tokio::test] + async fn dangerous_render_content_type_is_coerced_on_update() { + let files = svc(); + let cx = anon_cx(AppId::new()); + let id = files + .create( + &cx, + "c", + NewFile { + name: "a.bin".into(), + content_type: "application/octet-stream".into(), + data: b"x".to_vec(), + }, + ) + .await + .unwrap(); + files + .update( + &cx, + "c", + &id.to_string(), + FileUpdate { + data: b"y".to_vec(), + name: None, + content_type: Some("image/svg+xml".into()), + }, + ) + .await + .unwrap(); + let meta = files + .head(&cx, "c", &id.to_string()) + .await + .unwrap() + .unwrap(); + assert_eq!(meta.content_type, "application/octet-stream"); + } + #[tokio::test] async fn create_then_get_head_round_trips() { let files = svc(); diff --git a/crates/shared/src/files.rs b/crates/shared/src/files.rs index 8810a7d..4c562a8 100644 --- a/crates/shared/src/files.rs +++ b/crates/shared/src/files.rs @@ -247,6 +247,52 @@ impl FileUpdate { } } +/// Content type used in place of any upload whose declared type would +/// render dangerously in a browser. See [`sanitize_stored_content_type`]. +pub const SAFE_RENDER_FALLBACK: &str = "application/octet-stream"; + +/// Coerce a script-supplied `content_type` to a safe value for storage. +/// +/// Audit 2026-06-11 C-2 — storage-side defense. The download response +/// also forces `Content-Disposition: attachment`, `X-Content-Type-Options: +/// nosniff`, and a restrictive CSP (see `manager-core::files_api::get_file`), +/// so this is belt-and-suspenders: even if a future code path serves a +/// file with `inline` again, the stored type can't be `text/html` / +/// `image/svg+xml` / `application/javascript` etc. +/// +/// Allowlist (the audit's recommended set): +/// - `application/octet-stream`, `application/pdf`, `application/json` +/// - `text/plain`, `text/csv` +/// - `image/*` (except `image/svg+xml`) +/// - `audio/*`, `video/*` +/// +/// Anything else returns `application/octet-stream`. Parameters +/// (`; charset=…`) are preserved when the base type is on the allowlist. +#[must_use] +pub fn sanitize_stored_content_type(ct: &str) -> String { + let base = ct + .split(';') + .next() + .unwrap_or("") + .trim() + .to_ascii_lowercase(); + let in_allowlist = matches!( + base.as_str(), + "application/octet-stream" + | "application/pdf" + | "application/json" + | "text/plain" + | "text/csv" + ) || (base.starts_with("image/") && !base.starts_with("image/svg")) + || base.starts_with("audio/") + || base.starts_with("video/"); + if in_allowlist { + ct.to_string() + } else { + SAFE_RENDER_FALLBACK.to_string() + } +} + /// Reject a collection name that is empty or could escape the per-app /// files tree. UUID-shaped ids never produce traversal paths, but /// collection names come from scripts so they're validated defensively @@ -337,3 +383,67 @@ impl FilesService for NoopFilesService { Err(FilesError::Backend("files is not wired in".into())) } } + +#[cfg(test)] +mod content_type_sanitizer_tests { + use super::*; + + #[test] + fn allowlist_passes_safe_types() { + for ct in [ + "application/octet-stream", + "application/pdf", + "application/json", + "text/plain", + "text/csv", + "image/png", + "image/jpeg", + "image/webp", + "audio/mpeg", + "video/mp4", + ] { + assert_eq!(sanitize_stored_content_type(ct), ct, "{ct} should pass"); + } + } + + #[test] + fn dangerous_render_types_are_coerced() { + for ct in [ + "text/html", + "text/html; charset=utf-8", + "image/svg+xml", + "image/svg", + "application/xhtml+xml", + "application/javascript", + "text/javascript", + "application/ecmascript", + "application/x-shockwave-flash", + "text/xml", + "application/xml", + ] { + assert_eq!( + sanitize_stored_content_type(ct), + SAFE_RENDER_FALLBACK, + "{ct} should coerce" + ); + } + } + + #[test] + fn case_and_whitespace_insensitive() { + assert_eq!( + sanitize_stored_content_type(" TEXT/HTML "), + SAFE_RENDER_FALLBACK + ); + assert_eq!( + sanitize_stored_content_type("IMAGE/Svg+XML"), + SAFE_RENDER_FALLBACK + ); + } + + #[test] + fn parameters_are_preserved_for_safe_types() { + let ct = "text/plain; charset=utf-8"; + assert_eq!(sanitize_stored_content_type(ct), ct); + } +} diff --git a/crates/shared/src/lib.rs b/crates/shared/src/lib.rs index 11f399b..bb32d2e 100644 --- a/crates/shared/src/lib.rs +++ b/crates/shared/src/lib.rs @@ -50,8 +50,9 @@ pub use events::{EmitError, NoopEventEmitter, ServiceEvent, ServiceEventEmitter} pub use exec_summary::ExecResponseSummary; pub use execution_log::{ExecutionLog, ExecutionStatus}; pub use files::{ - validate_collection as validate_files_collection, FileMeta, FileUpdate, FilesError, - FilesListPage, FilesService, NewFile, NoopFilesService, + sanitize_stored_content_type, validate_collection as validate_files_collection, FileMeta, + FileUpdate, FilesError, FilesListPage, FilesService, NewFile, NoopFilesService, + SAFE_RENDER_FALLBACK, }; pub use http::{HttpError, HttpRequest, HttpResponse, HttpService, NoopHttpService}; pub use ids::{