fix(audit-2026-06-11/C-2): file download attachment + nosniff + CSP + MIME allowlist
Stored XSS: the previous get_file handler streamed user-supplied bytes
with Content-Disposition: inline, the user-supplied Content-Type, and
no X-Content-Type-Options / CSP. A Rhai script could store an SVG or
HTML payload whose download URL rendered same-origin under the admin
session cookie.
Closes the response side and the storage side:
* shared::sanitize_stored_content_type: allowlist
(octet-stream/pdf/json/text-plain/text-csv/image-non-svg/audio/video)
with anything else coerced to application/octet-stream. New unit tests
cover the safe/unsafe/case-insensitive/parameter-preserving paths.
* files_service::create/update: sanitize the stored content_type after
the shape checks pass (sanitize-after-validate keeps the existing
MissingField / TooLong errors intact). Two new tests confirm text/html
and image/svg+xml are coerced to application/octet-stream on
create/update respectively.
* files_api::get_file (admin download):
- Content-Disposition: attachment (was inline)
- Content-Type re-sanitized via the shared helper as belt-and-
suspenders for any pre-existing row that pre-dates this change.
- X-Content-Type-Options: nosniff
- Content-Security-Policy: default-src 'none'; sandbox;
frame-ancestors 'none'
- Referrer-Policy: no-referrer
Audit ref: security_audit/07_http_cors_csrf_xss.md#c07-02 (response side)
+ security_audit/06_files_pathtraversal.md#f-fs-001 (storage side).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -123,10 +123,20 @@ async fn list_files(
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// `GET /apps/{id}/files/{collection}/{file_id}` — stream the file's
|
/// `GET /apps/{id}/files/{collection}/{file_id}` — stream the file's
|
||||||
/// bytes inline. The dashboard's Download button hits this; the audit
|
/// bytes as a download. The dashboard's Download button hits this;
|
||||||
/// flagged the missing handler (callers were 405'ing). Metadata gives
|
/// metadata is read via `FilesRepo::head`, bytes via `FilesRepo::get`
|
||||||
/// the Content-Type + Content-Disposition; bytes come from
|
/// (which checksum-verifies on read).
|
||||||
/// `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(
|
async fn get_file(
|
||||||
State(s): State<FilesAdminState>,
|
State(s): State<FilesAdminState>,
|
||||||
Extension(principal): Extension<Principal>,
|
Extension(principal): Extension<Principal>,
|
||||||
@@ -150,16 +160,23 @@ async fn get_file(
|
|||||||
.get(app_id, &collection, id)
|
.get(app_id, &collection, id)
|
||||||
.await?
|
.await?
|
||||||
.ok_or(FilesApiError::NotFound)?;
|
.ok_or(FilesApiError::NotFound)?;
|
||||||
|
let safe_ct = picloud_shared::sanitize_stored_content_type(&meta.content_type);
|
||||||
let disposition = format!(
|
let disposition = format!(
|
||||||
"inline; filename=\"{}\"",
|
"attachment; filename=\"{}\"",
|
||||||
meta.name.replace('"', "").replace(['\r', '\n'], "")
|
meta.name.replace('"', "").replace(['\r', '\n'], "")
|
||||||
);
|
);
|
||||||
let len = bytes.len();
|
let len = bytes.len();
|
||||||
Ok(Response::builder()
|
Ok(Response::builder()
|
||||||
.status(StatusCode::OK)
|
.status(StatusCode::OK)
|
||||||
.header(CONTENT_TYPE, meta.content_type)
|
.header(CONTENT_TYPE, safe_ct)
|
||||||
.header(CONTENT_DISPOSITION, disposition)
|
.header(CONTENT_DISPOSITION, disposition)
|
||||||
.header(CONTENT_LENGTH, len)
|
.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))
|
.body(Body::from(bytes))
|
||||||
.expect("build files download response"))
|
.expect("build files download response"))
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -17,8 +17,8 @@ use std::sync::Arc;
|
|||||||
|
|
||||||
use async_trait::async_trait;
|
use async_trait::async_trait;
|
||||||
use picloud_shared::{
|
use picloud_shared::{
|
||||||
validate_files_collection, FileMeta, FileUpdate, FilesError, FilesListPage, FilesService,
|
sanitize_stored_content_type, validate_files_collection, FileMeta, FileUpdate, FilesError,
|
||||||
NewFile, SdkCallCx, ServiceEvent, ServiceEventEmitter,
|
FilesListPage, FilesService, NewFile, SdkCallCx, ServiceEvent, ServiceEventEmitter,
|
||||||
};
|
};
|
||||||
use uuid::Uuid;
|
use uuid::Uuid;
|
||||||
|
|
||||||
@@ -124,11 +124,14 @@ impl FilesService for FilesServiceImpl {
|
|||||||
&self,
|
&self,
|
||||||
cx: &SdkCallCx,
|
cx: &SdkCallCx,
|
||||||
collection: &str,
|
collection: &str,
|
||||||
new: NewFile,
|
mut new: NewFile,
|
||||||
) -> Result<Uuid, FilesError> {
|
) -> Result<Uuid, FilesError> {
|
||||||
validate_files_collection(collection)?;
|
validate_files_collection(collection)?;
|
||||||
self.check_write(cx).await?;
|
self.check_write(cx).await?;
|
||||||
new.validate(self.max_file_size_bytes)?;
|
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?;
|
let meta = self.repo.create(cx.app_id, collection, new).await?;
|
||||||
self.emit(cx, "create", collection, &meta, None).await;
|
self.emit(cx, "create", collection, &meta, None).await;
|
||||||
Ok(meta.id)
|
Ok(meta.id)
|
||||||
@@ -167,11 +170,15 @@ impl FilesService for FilesServiceImpl {
|
|||||||
cx: &SdkCallCx,
|
cx: &SdkCallCx,
|
||||||
collection: &str,
|
collection: &str,
|
||||||
id: &str,
|
id: &str,
|
||||||
upd: FileUpdate,
|
mut upd: FileUpdate,
|
||||||
) -> Result<(), FilesError> {
|
) -> Result<(), FilesError> {
|
||||||
validate_files_collection(collection)?;
|
validate_files_collection(collection)?;
|
||||||
self.check_write(cx).await?;
|
self.check_write(cx).await?;
|
||||||
upd.validate(self.max_file_size_bytes)?;
|
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 {
|
let Some(uuid) = parse_id(id) else {
|
||||||
return Err(FilesError::NotFound);
|
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"<script>alert(1)</script>".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]
|
#[tokio::test]
|
||||||
async fn create_then_get_head_round_trips() {
|
async fn create_then_get_head_round_trips() {
|
||||||
let files = svc();
|
let files = svc();
|
||||||
|
|||||||
@@ -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
|
/// Reject a collection name that is empty or could escape the per-app
|
||||||
/// files tree. UUID-shaped ids never produce traversal paths, but
|
/// files tree. UUID-shaped ids never produce traversal paths, but
|
||||||
/// collection names come from scripts so they're validated defensively
|
/// 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()))
|
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);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -50,8 +50,9 @@ pub use events::{EmitError, NoopEventEmitter, ServiceEvent, ServiceEventEmitter}
|
|||||||
pub use exec_summary::ExecResponseSummary;
|
pub use exec_summary::ExecResponseSummary;
|
||||||
pub use execution_log::{ExecutionLog, ExecutionStatus};
|
pub use execution_log::{ExecutionLog, ExecutionStatus};
|
||||||
pub use files::{
|
pub use files::{
|
||||||
validate_collection as validate_files_collection, FileMeta, FileUpdate, FilesError,
|
sanitize_stored_content_type, validate_collection as validate_files_collection, FileMeta,
|
||||||
FilesListPage, FilesService, NewFile, NoopFilesService,
|
FileUpdate, FilesError, FilesListPage, FilesService, NewFile, NoopFilesService,
|
||||||
|
SAFE_RENDER_FALLBACK,
|
||||||
};
|
};
|
||||||
pub use http::{HttpError, HttpRequest, HttpResponse, HttpService, NoopHttpService};
|
pub use http::{HttpError, HttpRequest, HttpResponse, HttpService, NoopHttpService};
|
||||||
pub use ids::{
|
pub use ids::{
|
||||||
|
|||||||
Reference in New Issue
Block a user