From 4410ddc7a40c3b344c8262b32f64e52776cfd667 Mon Sep 17 00:00:00 2001 From: Ophir LOJKINE Date: Wed, 10 Jun 2026 16:03:38 +0200 Subject: [PATCH] Fix Content-Disposition parameter injection via download filenames The csv and download components built the Content-Disposition header by string-interpolating the user-supplied filename. A filename containing characters such as ';', '"' or '=' could inject an additional header parameter (e.g. a second, agent-preferred filename*=...), letting an app that interpolates untrusted data into the filename smuggle a different download name past the intended one. Build the header with actix-web's structured ContentDisposition type so the filename is always a single, properly quoted/escaped value and cannot create new parameters. --- CHANGELOG.md | 4 ++ src/render.rs | 26 ++++++++----- tests/data_formats/csv_filename_injection.sql | 5 +++ tests/data_formats/mod.rs | 39 +++++++++++++++++++ 4 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 tests/data_formats/csv_filename_injection.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ec88752..b7e86fb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # CHANGELOG.md +## Unreleased + +- **Download filenames can no longer inject extra `Content-Disposition` parameters.** The `csv` and `download` components now build the `Content-Disposition` header with a properly quoted and escaped filename instead of plain string concatenation. Before this fix, a filename containing characters such as `;`, `"`, or `=` could add a second header parameter (for example a `filename*=...` value), and some browsers prefer that injected value over the intended one. You are affected if your app puts untrusted data (such as a user-provided name or a value from the database) into the `filename` of a `csv` or `download` component. No SQL change is required: the supplied filename now always appears as a single, safely quoted `filename` value. + ## v0.44.0 This release focuses on making production SQLPage apps easier to understand, debug, and operate. Most apps should keep working without SQL changes, but maintainers should review the notes about logging and uploaded-file permissions. diff --git a/src/render.rs b/src/render.rs index 8fdd6cc2..364e6b2f 100644 --- a/src/render.rs +++ b/src/render.rs @@ -49,7 +49,9 @@ use crate::webserver::response_writer::{AsyncResponseWriter, ResponseWriter}; use actix_web::body::MessageBody; use actix_web::cookie::time::OffsetDateTime; use actix_web::cookie::time::format_description::well_known::Rfc3339; -use actix_web::http::header::TryIntoHeaderPair; +use actix_web::http::header::{ + ContentDisposition, DispositionParam, DispositionType, TryIntoHeaderPair, +}; use actix_web::http::{StatusCode, header}; use actix_web::{HttpResponse, HttpResponseBuilder}; use anyhow::{Context as AnyhowContext, bail, format_err}; @@ -301,10 +303,7 @@ impl HeaderContext { get_object_str(options, "filename").or_else(|| get_object_str(options, "title")) { let extension = if filename.contains('.') { "" } else { ".csv" }; - self.insert_header(( - header::CONTENT_DISPOSITION, - format!("attachment; filename={filename}{extension}"), - ))?; + self.insert_header(attachment_with_filename(&format!("{filename}{extension}")))?; } let csv_renderer = CsvBodyRenderer::new(self.writer, options).await?; let renderer = AnyRenderBodyContext::Csv(csv_renderer); @@ -345,10 +344,7 @@ impl HeaderContext { fn download(mut self, options: &JsonValue) -> anyhow::Result { if let Some(filename) = get_object_str(options, "filename") { - self.insert_header(( - header::CONTENT_DISPOSITION, - format!("attachment; filename=\"{filename}\""), - ))?; + self.insert_header(attachment_with_filename(filename))?; } let data_url = get_object_str(options, "data_url") .with_context(|| "The download component requires a 'data_url' property")?; @@ -442,6 +438,18 @@ async fn verify_password_async( .await? } +/// Builds an `attachment` `Content-Disposition` header with the given filename, +/// using actix-web's structured [`ContentDisposition`] type so the filename is +/// properly quoted and escaped. This prevents a user-supplied filename +/// containing `;`, `"`, or `=` from injecting additional header parameters +/// (e.g. a second, agent-preferred `filename*`). +fn attachment_with_filename(filename: &str) -> ContentDisposition { + ContentDisposition { + disposition: DispositionType::Attachment, + parameters: vec![DispositionParam::Filename(filename.to_owned())], + } +} + fn get_object_str<'a>(json: &'a JsonValue, key: &str) -> Option<&'a str> { json.as_object() .and_then(|obj| obj.get(key)) diff --git a/tests/data_formats/csv_filename_injection.sql b/tests/data_formats/csv_filename_injection.sql new file mode 100644 index 00000000..69eb1fec --- /dev/null +++ b/tests/data_formats/csv_filename_injection.sql @@ -0,0 +1,5 @@ +select + 'csv' as component, + 'report.csv; filename*=UTF-8''''evil.html' as filename; + +select 1 as a; diff --git a/tests/data_formats/mod.rs b/tests/data_formats/mod.rs index c7b3f178..e459eef6 100644 --- a/tests/data_formats/mod.rs +++ b/tests/data_formats/mod.rs @@ -68,6 +68,45 @@ async fn test_csv_body() -> actix_web::Result<()> { Ok(()) } +#[actix_web::test] +async fn test_csv_filename_header_injection() -> actix_web::Result<()> { + use actix_web::http::header::ContentDisposition; + + // The csv `filename` is `report.csv; filename*=UTF-8''evil.html`, which + // tries to smuggle an extra `filename*` parameter into the + // Content-Disposition header. The attacker-supplied value must NOT create a + // second, agent-preferred parameter; it has to stay inside a single, + // properly quoted `filename` value. + let resp = crate::common::req_path("/tests/data_formats/csv_filename_injection.sql") + .await + .expect("request failed"); + assert_eq!(resp.status(), StatusCode::OK); + let raw = resp + .headers() + .get(header::CONTENT_DISPOSITION) + .expect("missing Content-Disposition header") + .clone(); + + // Parse the header the same way a compliant user agent would, so that + // `;` and `=` inside a quoted value are treated as literal data, not as + // parameter separators. + let disposition = ContentDisposition::from_raw(&raw) + .unwrap_or_else(|e| panic!("invalid Content-Disposition {raw:?}: {e}")); + + // No extended `filename*` parameter must have been injected. + assert!( + disposition.get_filename_ext().is_none(), + "attacker injected a separate filename* parameter: {raw:?}" + ); + // The whole attacker payload must remain a single, inert `filename` value. + assert_eq!( + disposition.get_filename(), + Some("report.csv; filename*=UTF-8''evil.html"), + "the attacker payload should stay inside a single filename value: {raw:?}" + ); + Ok(()) +} + #[actix_web::test] async fn test_json_columns() { let app_data = crate::common::make_app_data().await;