diff --git a/CHANGELOG.md b/CHANGELOG.md index ca6ac58b..6a390324 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/render.rs b/src/render.rs index d2b69857..fc80ffac 100644 --- a/src/render.rs +++ b/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 { 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 { - 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 { - 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 { 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(mut self, body: B) -> anyhow::Result { - 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 { - 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 { + self.into_response(()) } } diff --git a/src/webserver/http.rs b/src/webserver/http.rs index ae048c94..9fca9f69 100644 --- a/src/webserver/http.rs +++ b/src/webserver/http.rs @@ -190,7 +190,7 @@ async fn build_response_header_and_stream>( } } 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 }) } diff --git a/tests/errors/invalid_header.rs b/tests/errors/invalid_header.rs new file mode 100644 index 00000000..a636faf6 --- /dev/null +++ b/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; + } +} diff --git a/tests/errors/invalid_header.sql b/tests/errors/invalid_header.sql new file mode 100644 index 00000000..c420fb75 --- /dev/null +++ b/tests/errors/invalid_header.sql @@ -0,0 +1 @@ +select 'dynamic' as component, $properties as properties; diff --git a/tests/errors/mod.rs b/tests/errors/mod.rs index ba7b7aea..2938bc9c 100644 --- a/tests/errors/mod.rs +++ b/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() {