Fix Content-Disposition parameter injection via download filenames

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## 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.
 - **Security fix: reserved and private files could be served directly over HTTP after a trusted page loaded them.** Files that SQLPage normally refuses to serve to direct HTTP requests (anything under the reserved `sqlpage/` prefix such as migrations, configuration, and database connection details, as well as dotfiles, parent-directory traversal paths like `../secret.sql`, and absolute paths) could briefly become reachable. This happened only after a trusted page loaded that same file with `sqlpage.run_sql(...)`, which loads files with elevated privileges and caches the parsed result. While that cache entry was fresh, a direct request such as `GET /sqlpage/secret.sql` (or the extensionless alias `GET /sqlpage/secret`) returned `200 OK` and executed the private SQL instead of returning `403 Forbidden`. Worst case, an attacker who can reach your site could read or execute internal SQL that was meant to stay private, including migration and configuration logic. You are affected if your app calls `sqlpage.run_sql()` on files inside `sqlpage/` (or on dotfiles or paths outside the web root) and is reachable by untrusted users. The fix enforces the unprivileged path guard on every direct HTTP request regardless of the cache, so these paths now always return `403 Forbidden`. Upgrade to get the fix; no configuration change is required. Legitimate `sqlpage.run_sql()` includes of such files from your own pages keep working as before.
 
 ## v0.44.0

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<PageContext> {
         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))

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;

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;