Fix http header only page crashes

Return errors early on invalid header values

Author: lovasoa

CHANGELOG.md

@@ -3,9 +3,9 @@
 ## unreleased
 
 - Made OIDC and `sqlpage.fetch` debug logs safer and simpler by removing raw token, cookie, claims, and response body dumps while keeping useful request and response metadata.
-- Fixed a bug where the single-sign-on oidc code would generate an unbounded amount of cookies when receiving many unauthenticated requests in sequence. 
+- Fixed a bug where the single-sign-on oidc code would generate an unbounded amount of cookies when receiving many unauthenticated requests in sequence.
 - Fixed multiple incorrect or imprecise HTTP statuses returned by sqlpage on error
-  - this makes it easier for an administrator to distinguish between user errors (4xx, non actionnable) and server errors (5xx, when you see them you should do something) 
+  - this makes it easier for an administrator to distinguish between user errors (4xx, non actionnable) and server errors (5xx, when you see them you should do something)
   - for instance: invalid UTF-8 in multipart text fields now returns `400 Bad Request` instead of `500 Internal Server Error`.
 - Logging: `LOG_LEVEL` is now the primary environment variable for configuring SQLPage's log filter. `RUST_LOG` remains supported as an alias.
 - You can now easily understand and debug slow page loads thanks to the added support for [OpenTelemetry](https://opentelemetry.io) tracing & metrics
@@ -15,6 +15,7 @@
 - Added an argument to `sqlpage.persist_uploaded_file(...)` to control the permissions of the newly created file.
   - Notably, this makes it easier to accelerate serving of uploaded files by letting a reverse proxy like nginx serve them directly.
 - Added an [`id` row-level parameter to the datagrid component](https://github.com/sqlpage/SQLPage/issues/1243)
+- Fixed crashes when a header-only page returned an http header containing an invalid character
 
 ## 0.43.0
 

src/render.rs

@@ -158,6 +158,18 @@ impl HeaderContext {
         Ok(self)
     }
 
+    fn insert_header(&mut self, header: impl TryIntoHeaderPair) -> anyhow::Result<()> {
+        let pair = header.try_into_pair().map_err(Into::into)?;
+        self.response.insert_header(pair);
+        Ok(())
+    }
+
+    fn append_header(&mut self, header: impl TryIntoHeaderPair) -> anyhow::Result<()> {
+        let pair = header.try_into_pair().map_err(Into::into)?;
+        self.response.append_header(pair);
+        Ok(())
+    }
+
     fn add_http_header(mut self, data: &JsonValue) -> anyhow::Result<Self> {
         let obj = data.as_object().with_context(|| "expected object")?;
         for (name, value) in obj {
@@ -173,7 +185,7 @@ impl HeaderContext {
             }
             let header = TryIntoHeaderPair::try_into_pair((name.as_str(), value_str))
                 .map_err(|e| anyhow::anyhow!("Invalid header: {name}:{value_str}: {e:#?}"))?;
-            self.response.insert_header(header);
+            self.insert_header(header)?;
         }
         Ok(self)
     }
@@ -238,8 +250,7 @@ impl HeaderContext {
             }));
         }
         log::trace!("Setting cookie {cookie}");
-        self.response
-            .append_header((header::SET_COOKIE, cookie.encoded().to_string()));
+        self.append_header((header::SET_COOKIE, cookie.encoded().to_string()))?;
         Ok(self)
     }
 
@@ -248,14 +259,13 @@ impl HeaderContext {
         self.has_status = true;
         let link = get_object_str(data, "link")
             .with_context(|| "The redirect component requires a 'link' property")?;
-        self.response.insert_header((header::LOCATION, link));
+        self.insert_header((header::LOCATION, link))?;
         self.close_with_body(())
     }
 
     /// Answers to the HTTP request with a single json object
     fn json(mut self, data: &JsonValue) -> anyhow::Result<PageContext> {
-        self.response
-            .insert_header((header::CONTENT_TYPE, "application/json"));
+        self.insert_header((header::CONTENT_TYPE, "application/json"))?;
         if let Some(contents) = data.get("contents") {
             let json_response = if let Some(s) = contents.as_str() {
                 s.as_bytes().to_owned()
@@ -269,8 +279,7 @@ impl HeaderContext {
                 None | Some("array") => JsonBodyRenderer::new_array(self.writer),
                 Some("jsonlines") => JsonBodyRenderer::new_jsonlines(self.writer),
                 Some("sse") => {
-                    self.response
-                        .insert_header((header::CONTENT_TYPE, "text/event-stream"));
+                    self.insert_header((header::CONTENT_TYPE, "text/event-stream"))?;
                     JsonBodyRenderer::new_server_sent_events(self.writer)
                 }
                 _ => bail!(
@@ -287,16 +296,15 @@ impl HeaderContext {
     }
 
     async fn csv(mut self, options: &JsonValue) -> anyhow::Result<PageContext> {
-        self.response
-            .insert_header((header::CONTENT_TYPE, "text/csv; charset=utf-8"));
+        self.insert_header((header::CONTENT_TYPE, "text/csv; charset=utf-8"))?;
         if let Some(filename) =
             get_object_str(options, "filename").or_else(|| get_object_str(options, "title"))
         {
             let extension = if filename.contains('.') { "" } else { ".csv" };
-            self.response.insert_header((
+            self.insert_header((
                 header::CONTENT_DISPOSITION,
                 format!("attachment; filename={filename}{extension}"),
-            ));
+            ))?;
         }
         let csv_renderer = CsvBodyRenderer::new(self.writer, options).await?;
         let renderer = AnyRenderBodyContext::Csv(csv_renderer);
@@ -320,10 +328,9 @@ impl HeaderContext {
         log::debug!("Authentication failed");
         // The authentication failed
         if let Some(link) = get_object_str(&data, "link") {
-            self.response
-                .status(StatusCode::FOUND)
-                .insert_header((header::LOCATION, link));
+            self.response.status(StatusCode::FOUND);
             self.has_status = true;
+            self.insert_header((header::LOCATION, link))?;
             let response = self.into_response(
                 "Sorry, but you are not authorized to access this page. \
                 Redirecting to the login page...",
@@ -338,10 +345,10 @@ impl HeaderContext {
 
     fn download(mut self, options: &JsonValue) -> anyhow::Result<PageContext> {
         if let Some(filename) = get_object_str(options, "filename") {
-            self.response.insert_header((
+            self.insert_header((
                 header::CONTENT_DISPOSITION,
                 format!("attachment; filename=\"{filename}\""),
-            ));
+            ))?;
         }
         let data_url = get_object_str(options, "data_url")
             .with_context(|| "The download component requires a 'data_url' property")?;
@@ -360,8 +367,7 @@ impl HeaderContext {
                     .into();
         }
         if !content_type.is_empty() {
-            self.response
-                .insert_header((header::CONTENT_TYPE, content_type));
+            self.insert_header((header::CONTENT_TYPE, content_type))?;
         }
         self.close_with_body(body_bytes.into_owned())
     }
@@ -371,14 +377,15 @@ impl HeaderContext {
         Ok(PageContext::Header(self))
     }
 
-    fn add_server_timing_header(&mut self) {
+    fn add_server_timing_header(&mut self) -> anyhow::Result<()> {
         if let Some(header_value) = self.request_context.server_timing.header_value() {
-            self.response.insert_header(("Server-Timing", header_value));
+            self.insert_header(("Server-Timing", header_value))?;
         }
+        Ok(())
     }
 
     fn into_response<B: MessageBody + 'static>(mut self, body: B) -> anyhow::Result<HttpResponse> {
-        self.add_server_timing_header();
+        self.add_server_timing_header()?;
         match self.response.message_body(body) {
             Ok(response) => Ok(response.map_into_boxed_body()),
             Err(e) => Err(anyhow::anyhow!(
@@ -392,7 +399,7 @@ impl HeaderContext {
     }
 
     async fn start_body(mut self, data: JsonValue) -> anyhow::Result<PageContext> {
-        self.add_server_timing_header();
+        self.add_server_timing_header()?;
         let renderer = match self.request_context.response_format {
             ResponseFormat::Json => AnyRenderBodyContext::Json(
                 JsonBodyRenderer::new_array_with_first_row(self.writer, &data),
@@ -417,8 +424,8 @@ impl HeaderContext {
         })
     }
 
-    pub fn close(self) -> HttpResponse {
-        self.into_response(()).unwrap()
+    pub fn close(self) -> anyhow::Result<HttpResponse> {
+        self.into_response(())
     }
 }
 

src/webserver/http.rs

@@ -190,7 +190,7 @@ async fn build_response_header_and_stream<S: Stream<Item = DbItem>>(
         }
     }
     log::debug!("No SQL statements left to execute for the body of the response");
-    let http_response = head_context.close();
+    let http_response = head_context.close()?;
     Ok(ResponseWithWriter::FinishedResponse { http_response })
 }
 

tests/errors/invalid_header.rs

@@ -0,0 +1,108 @@
+use crate::common::req_path;
+use actix_web::{http::StatusCode, test};
+use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
+use serde_json::{json, Value};
+
+struct InvalidHeaderCase {
+    name: &'static str,
+    properties: Value,
+}
+
+async fn assert_invalid_header_response(case: &InvalidHeaderCase) {
+    let properties = serde_json::to_string(&case.properties).unwrap();
+    let properties = utf8_percent_encode(&properties, NON_ALPHANUMERIC).to_string();
+    let path = format!("/tests/errors/invalid_header.sql?properties={properties}");
+
+    let resp = req_path(&path).await.unwrap_or_else(|err| {
+        panic!(
+            "{} should return an error response instead of failing the request: {err:#}",
+            case.name
+        )
+    });
+
+    assert_eq!(
+        resp.status(),
+        StatusCode::INTERNAL_SERVER_ERROR,
+        "{} should return a 500 response",
+        case.name
+    );
+
+    let body = test::read_body(resp).await;
+    let body_str = String::from_utf8(body.to_vec()).unwrap();
+    assert!(
+        body_str.to_lowercase().contains("error"),
+        "{} should render an error response body, got:\n{body_str}",
+        case.name
+    );
+}
+
+#[actix_web::test]
+async fn test_invalid_header_components_return_an_error_response() {
+    let cases = vec![
+        InvalidHeaderCase {
+            name: "cookie domain with newline",
+            properties: json!({
+                "component": "cookie",
+                "name": "boom",
+                "value": "x",
+                "domain": "\n",
+            }),
+        },
+        InvalidHeaderCase {
+            name: "cookie path with DEL",
+            properties: json!({
+                "component": "cookie",
+                "name": "boom",
+                "value": "x",
+                "path": "\u{007f}",
+            }),
+        },
+        InvalidHeaderCase {
+            name: "http_header value with carriage return",
+            properties: json!({
+                "component": "http_header",
+                "X-Test": "\r",
+            }),
+        },
+        InvalidHeaderCase {
+            name: "redirect link with NUL",
+            properties: json!({
+                "component": "redirect",
+                "link": "\u{0000}",
+            }),
+        },
+        InvalidHeaderCase {
+            name: "authentication link with unit separator",
+            properties: json!({
+                "component": "authentication",
+                "link": "\u{001f}",
+            }),
+        },
+        InvalidHeaderCase {
+            name: "download filename with newline",
+            properties: json!({
+                "component": "download",
+                "data_url": "data:text/plain,ok",
+                "filename": "\n",
+            }),
+        },
+        InvalidHeaderCase {
+            name: "csv filename with carriage return",
+            properties: json!({
+                "component": "csv",
+                "filename": "\r",
+            }),
+        },
+        InvalidHeaderCase {
+            name: "csv title with NUL",
+            properties: json!({
+                "component": "csv",
+                "title": "\u{0000}",
+            }),
+        },
+    ];
+
+    for case in &cases {
+        assert_invalid_header_response(case).await;
+    }
+}

tests/errors/invalid_header.sql

@@ -0,0 +1 @@
+select 'dynamic' as component, $properties as properties;

tests/errors/mod.rs

@@ -5,6 +5,7 @@ use actix_web::{
 
 use crate::common::req_path;
 mod basic_auth;
+mod invalid_header;
 
 #[actix_web::test]
 async fn test_privileged_paths_are_not_accessible() {