From 07d2aef5eb5c4a82c7ab2d0ca1493b37c1e9c01a Mon Sep 17 00:00:00 2001 From: Ophir LOJKINE Date: Thu, 11 Jun 2026 11:17:58 +0200 Subject: [PATCH] fix: hide SQL details in production error responses for every output format In production, error responses must not leak the SQL statement, the source file path, the raw database error, environment values, or configuration, for ANY output format. In development the full detail is shown, and the full error is always logged server-side. This centralizes the dev-vs-production decision in a single place. An internal error stays a full `anyhow::Error` everywhere; the only place that turns it into a user-facing representation is `ClientError::new` in `src/webserver/error.rs`, which is also the only caller of `DevOrProd::is_prod`. Every renderer (HTML, JSON, NDJSON, SSE, CSV) and the header/pre-body path obtains a `ClientError` from that one function and only formats it; none of them inspect the environment. Leaking is therefore impossible by construction: a renderer cannot emit what it never receives. The error type, the production message, `get_backtrace_as_strings`, and the error-component data construction live in `error.rs`; `render.rs` only renders components and formats a `ClientError`. --- CHANGELOG.md | 1 + src/render.rs | 199 ++++++++++++--------- src/webserver/error.rs | 211 +++++++++++++++++++++-- src/webserver/mod.rs | 2 +- tests/data_formats/csv_error_leak.sql | 3 + tests/data_formats/csv_error_no_rows.sql | 4 + tests/data_formats/json_error_leak.sql | 3 + tests/data_formats/mod.rs | 120 +++++++++++++ tests/data_formats/text_error_leak.sql | 2 + 9 files changed, 450 insertions(+), 95 deletions(-) create mode 100644 tests/data_formats/csv_error_leak.sql create mode 100644 tests/data_formats/csv_error_no_rows.sql create mode 100644 tests/data_formats/json_error_leak.sql create mode 100644 tests/data_formats/text_error_leak.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 15e211c1..1d2f5994 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - **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. - **Security fix: OIDC protected paths could be bypassed with percent-encoded URLs.** When `oidc_protected_paths` used a non-default prefix such as `["/protected"]`, an unauthenticated visitor could percent-encode a byte of the prefix (for example requesting `/%70rotected/`, where `%70` is `p`) to make the OIDC middleware treat the page as public and serve it without logging in, even though SQLPage then decoded the URL and ran the protected SQL file. You are affected if you rely on `oidc_protected_paths` or `oidc_public_paths` with prefixes other than the default `/`. SQLPage now percent-decodes the request path and the configured prefixes before applying the rules, so encoded and plain URLs are classified identically (including when `site_prefix` contains characters such as a space). No configuration change is required; just upgrade. +- **Security: production JSON, NDJSON, SSE, and CSV responses no longer leak SQL on errors.** When `environment` is set to `production`, the HTML renderer already hid database errors behind a generic message, but the JSON, NDJSON, Server-Sent Events, and CSV renderers still wrote the full error into the response body once streaming had started. That error included the path of the `.sql` file, the exact SQL statement SQLPage sent to the database, and the raw database error text. Because the response format is chosen from the request's `Accept` header, an unauthenticated visitor could request an ordinary HTML page with `Accept: application/json` and read back the SQL of pages that were never meant to expose it. You are affected if you run with `environment = production` and serve any page that can hit a database error (most apps can). There is nothing to change in your SQL: after upgrading, every output format returns the same generic "Please contact the administrator for more information. The error has been logged." message that HTML already used, the full error is still written to the server logs, and in development the detailed error is still shown. ## v0.44.0 diff --git a/src/render.rs b/src/render.rs index 364e6b2f..17558ef7 100644 --- a/src/render.rs +++ b/src/render.rs @@ -44,6 +44,7 @@ use crate::AppState; use crate::templates::SplitTemplate; use crate::webserver::ErrorWithStatus; +use crate::webserver::error::ClientError; use crate::webserver::http::{RequestContext, ResponseFormat}; use crate::webserver::response_writer::{AsyncResponseWriter, ResponseWriter}; use actix_web::body::MessageBody; @@ -134,15 +135,20 @@ impl HeaderContext { } pub async fn handle_error(self, err: anyhow::Error) -> anyhow::Result { - if self.app_state.config.environment.is_prod() { + // Reduce the error to its client-safe form. The single environment + // check lives in `ClientError::new`; here we only branch on the result. + let client_error = ClientError::new(&err, self.app_state.config.environment, None); + if client_error.is_generic() { + // Production: no body byte has been sent yet, so bubble the error + // up. The top-level handler then produces a proper error *response* + // (correct status code and the production-safe HTML error page) + // instead of a 200 body. The detailed error never reaches the client. return Err(err); } log::debug!("Handling header error: {err}"); - let data = json!({ - "component": "error", - "description": err.to_string(), - "backtrace": get_backtrace_as_strings(&err), - }); + // Development: show the full detail inline as an error component. + let mut data = client_error.to_html_data(); + data["component"] = json!("error"); self.start_body(data).await } @@ -288,7 +294,10 @@ impl HeaderContext { "Invalid value for the 'type' property of the json component: {body_type:?}" ), }; - let renderer = AnyRenderBodyContext::Json(json_renderer); + let renderer = AnyRenderBodyContext::new( + BodyRenderer::Json(json_renderer), + self.app_state.config.environment, + ); let http_response = self.response; Ok(PageContext::Body { http_response, @@ -305,8 +314,9 @@ impl HeaderContext { let extension = if filename.contains('.') { "" } else { ".csv" }; self.insert_header(attachment_with_filename(&format!("{filename}{extension}")))?; } + let environment = self.app_state.config.environment; let csv_renderer = CsvBodyRenderer::new(self.writer, options).await?; - let renderer = AnyRenderBodyContext::Csv(csv_renderer); + let renderer = AnyRenderBodyContext::new(BodyRenderer::Csv(csv_renderer), environment); let http_response = self.response.take(); Ok(PageContext::Body { renderer, @@ -396,11 +406,13 @@ impl HeaderContext { async fn start_body(mut self, data: JsonValue) -> anyhow::Result { 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), - ), - ResponseFormat::JsonLines => AnyRenderBodyContext::Json( + let environment = self.app_state.config.environment; + let body_renderer = match self.request_context.response_format { + ResponseFormat::Json => BodyRenderer::Json(JsonBodyRenderer::new_array_with_first_row( + self.writer, + &data, + )), + ResponseFormat::JsonLines => BodyRenderer::Json( JsonBodyRenderer::new_jsonlines_with_first_row(self.writer, &data), ), ResponseFormat::Html => { @@ -410,9 +422,10 @@ impl HeaderContext { .with_context( || "Failed to create a render context from the header context.", )?; - AnyRenderBodyContext::Html(html_renderer) + BodyRenderer::Html(html_renderer) } }; + let renderer = AnyRenderBodyContext::new(body_renderer, environment); let http_response = self.response; Ok(PageContext::Body { renderer, @@ -463,64 +476,93 @@ fn take_object_str(json: &mut JsonValue, key: &str) -> Option { } } -/** - * Can receive rows, and write them in a given format to an `io::Write` - */ -pub enum AnyRenderBodyContext { +/// A body renderer for one of the supported output formats. It can receive +/// rows and write them, in its format, to an `io::Write`. +pub enum BodyRenderer { Html(HtmlRenderContext), Json(JsonBodyRenderer), Csv(CsvBodyRenderer), } -/** - * Dummy impl to dispatch method calls to the underlying renderer - */ +/// Wraps a [`BodyRenderer`] together with the environment, so errors can be +/// reduced to their client-safe form ([`ClientError`]) at this single boundary +/// before they ever reach a format-specific renderer. +/// +/// Renderers never see the raw [`anyhow::Error`]; they only receive a +/// [`ClientError`], which already hides every sensitive detail in production. +/// This makes leaking impossible by construction: a renderer cannot emit what +/// it never receives. +pub struct AnyRenderBodyContext { + renderer: BodyRenderer, + environment: crate::app_config::DevOrProd, +} + impl AnyRenderBodyContext { + #[must_use] + pub fn new(renderer: BodyRenderer, environment: crate::app_config::DevOrProd) -> Self { + Self { + renderer, + environment, + } + } + pub async fn handle_row(&mut self, data: &JsonValue) -> anyhow::Result<()> { log::debug!( "<- Rendering properties: {}", serde_json::to_string(&data).unwrap_or_else(|e| e.to_string()) ); - match self { - AnyRenderBodyContext::Html(render_context) => render_context.handle_row(data).await, - AnyRenderBodyContext::Json(json_body_renderer) => json_body_renderer.handle_row(data), - AnyRenderBodyContext::Csv(csv_renderer) => csv_renderer.handle_row(data).await, + match &mut self.renderer { + BodyRenderer::Html(render_context) => render_context.handle_row(data).await, + BodyRenderer::Json(json_body_renderer) => json_body_renderer.handle_row(data), + BodyRenderer::Csv(csv_renderer) => csv_renderer.handle_row(data).await, } } + pub async fn handle_error(&mut self, error: &anyhow::Error) -> anyhow::Result<()> { + // The full error is always logged server-side, regardless of the + // environment and of which format the client requested. log::error!("SQL error: {error:?}"); - match self { - AnyRenderBodyContext::Html(render_context) => render_context.handle_error(error).await, - AnyRenderBodyContext::Json(json_body_renderer) => { - json_body_renderer.handle_error(error) + // Reduce the raw error to its client-safe form ONCE, here, before it + // reaches any format renderer. In production this strips the source + // path, the SQL statement, the raw database error, and the backtrace. + let query_number = match &self.renderer { + BodyRenderer::Html(html) => Some(html.current_statement), + BodyRenderer::Json(_) | BodyRenderer::Csv(_) => None, + }; + let client_error = ClientError::new(error, self.environment, query_number); + match &mut self.renderer { + BodyRenderer::Html(render_context) => render_context.handle_error(&client_error).await, + BodyRenderer::Json(json_body_renderer) => { + json_body_renderer.handle_error(&client_error) } - AnyRenderBodyContext::Csv(csv_renderer) => csv_renderer.handle_error(error).await, + BodyRenderer::Csv(csv_renderer) => csv_renderer.handle_error(&client_error).await, } } + pub async fn finish_query(&mut self) -> anyhow::Result<()> { - match self { - AnyRenderBodyContext::Html(render_context) => render_context.finish_query().await, - AnyRenderBodyContext::Json(_json_body_renderer) => Ok(()), - AnyRenderBodyContext::Csv(_csv_renderer) => Ok(()), + match &mut self.renderer { + BodyRenderer::Html(render_context) => render_context.finish_query().await, + BodyRenderer::Json(_json_body_renderer) => Ok(()), + BodyRenderer::Csv(_csv_renderer) => Ok(()), } } pub async fn flush(&mut self) -> anyhow::Result<()> { - match self { - AnyRenderBodyContext::Html(HtmlRenderContext { writer, .. }) - | AnyRenderBodyContext::Json(JsonBodyRenderer { writer, .. }) => { + match &mut self.renderer { + BodyRenderer::Html(HtmlRenderContext { writer, .. }) + | BodyRenderer::Json(JsonBodyRenderer { writer, .. }) => { writer.async_flush().await?; } - AnyRenderBodyContext::Csv(csv_renderer) => csv_renderer.flush().await?, + BodyRenderer::Csv(csv_renderer) => csv_renderer.flush().await?, } Ok(()) } pub async fn close(self) -> ResponseWriter { - match self { - AnyRenderBodyContext::Html(render_context) => render_context.close().await, - AnyRenderBodyContext::Json(json_body_renderer) => json_body_renderer.close(), - AnyRenderBodyContext::Csv(csv_renderer) => csv_renderer.close().await, + match self.renderer { + BodyRenderer::Html(render_context) => render_context.close().await, + BodyRenderer::Json(json_body_renderer) => json_body_renderer.close(), + BodyRenderer::Csv(csv_renderer) => csv_renderer.close().await, } } } @@ -590,10 +632,8 @@ impl JsonBodyRenderer { serde_json::to_writer(&mut self.writer, data)?; Ok(()) } - pub fn handle_error(&mut self, error: &anyhow::Error) -> anyhow::Result<()> { - self.handle_row(&json!({ - "error": error.to_string() - })) + pub fn handle_error(&mut self, error: &ClientError) -> anyhow::Result<()> { + self.handle_row(&json!({ "error": error.message() })) } pub fn close(mut self) -> W { @@ -677,17 +717,24 @@ impl CsvBodyRenderer { Ok(()) } - pub async fn handle_error(&mut self, error: &anyhow::Error) -> anyhow::Result<()> { - let err_str = error.to_string(); - self.writer - .write_record( - self.columns - .iter() - .enumerate() - .map(|(i, _)| if i == 0 { &err_str } else { "" }) - .collect::>(), - ) - .await?; + pub async fn handle_error(&mut self, error: &ClientError) -> anyhow::Result<()> { + let err_str = error.message(); + if self.columns.is_empty() { + // The error happened before any header row was written, so there are + // no columns to align with. Emit a single-field record so the error + // is still reported instead of an empty record. + self.writer.write_record([err_str]).await?; + } else { + self.writer + .write_record( + self.columns + .iter() + .enumerate() + .map(|(i, _)| if i == 0 { err_str } else { "" }) + .collect::>(), + ) + .await?; + } Ok(()) } @@ -848,31 +895,27 @@ impl HtmlRenderContext { Ok(()) } - /// Handles the rendering of an error. - /// Returns whether the error is irrecoverable and the rendering must stop - pub async fn handle_error(&mut self, error: &anyhow::Error) -> anyhow::Result<()> { + /// Renders an already-reduced, client-safe error into the page. + pub async fn handle_error(&mut self, error: &ClientError) -> anyhow::Result<()> { self.close_component()?; - let data = if self.app_state.config.environment.is_prod() { - json!({ - "description": format!("Please contact the administrator for more information. The error has been logged."), - }) - } else { - json!({ - "query_number": self.current_statement, - "description": error.to_string(), - "backtrace": get_backtrace_as_strings(error), - "note": "You can hide error messages like this one from your users by setting the 'environment' configuration option to 'production'." - }) - }; + let data = error.to_html_data(); let saved_component = self.open_component_with_data("error", &data).await?; self.close_component()?; self.current_component = saved_component; Ok(()) } + /// Reduces a raw error to its client-safe form and renders it. Used for + /// errors that arise during HTML rendering itself (e.g. while closing a + /// component), where the page is already committed to HTML. pub async fn handle_result(&mut self, result: &anyhow::Result) -> anyhow::Result<()> { if let Err(error) = result { - self.handle_error(error).await + let client_error = ClientError::new( + error, + self.app_state.config.environment, + Some(self.current_statement), + ); + self.handle_error(&client_error).await } else { Ok(()) } @@ -992,16 +1035,6 @@ fn handle_log_component( Ok(()) } -pub(super) fn get_backtrace_as_strings(error: &anyhow::Error) -> Vec { - let mut backtrace = vec![]; - let mut source = error.source(); - while let Some(s) = source { - backtrace.push(format!("{s}")); - source = s.source(); - } - backtrace -} - struct HandlebarWriterOutput(W); impl handlebars::Output for HandlebarWriterOutput { diff --git a/src/webserver/error.rs b/src/webserver/error.rs index df1bf9c0..817d7083 100644 --- a/src/webserver/error.rs +++ b/src/webserver/error.rs @@ -1,16 +1,140 @@ //! HTTP error handling +//! +//! This module owns the single boundary where an internal [`anyhow::Error`] +//! becomes the user-facing error representation. The [`ClientError`] type, built +//! by [`ClientError::new`], is the *only* place that consults the environment +//! (development vs production) to decide how much detail an error may expose. +//! Every output format ([`crate::render`]) renders a `ClientError` and never +//! inspects the environment itself, so no renderer can leak the SQL statement, +//! the source path, the raw database error, environment values, or +//! configuration. The full error is always logged server-side, independently of +//! what the client receives. use std::path::PathBuf; use crate::AppState; -use crate::render::get_backtrace_as_strings; +use crate::app_config::DevOrProd; use crate::webserver::ErrorWithStatus; use actix_web::HttpResponseBuilder; use actix_web::error::UrlencodedError; use actix_web::http::{StatusCode, header}; use actix_web::{HttpRequest, HttpResponse}; use handlebars::{Renderable, StringOutput}; -use serde_json::json; +use serde_json::{Value, json}; + +/// Generic message shown to end users in production instead of the detailed +/// error, which would leak the source file path, the SQL statement, and the +/// raw database error text. +const PRODUCTION_ERROR_MESSAGE: &str = + "Please contact the administrator for more information. The error has been logged."; + +const DEV_ERROR_NOTE: &str = "You can hide error messages like this one from your users by setting the 'environment' configuration option to 'production'."; + +/// An internal error reduced to the form that is safe to send to a client. +/// +/// This is the single boundary where a raw [`anyhow::Error`] becomes a +/// user-facing error. It is built once, from the error and the environment, by +/// [`ClientError::new`] (the only place that consults the environment). +/// Renderers (JSON, NDJSON, SSE, CSV, HTML) only ever receive a `ClientError`, +/// never the raw error, so no output format can leak the source path, the SQL +/// statement, the raw database error, environment values, or configuration: in +/// production every `ClientError` holds nothing but the generic message. The +/// full error is always logged server-side, independently of what the client +/// receives. +/// +/// Adding a new output format is therefore safe by construction: it can only +/// render the fields of `ClientError`, all of which are already +/// production-safe. +#[derive(Debug, Clone)] +pub struct ClientError { + /// One-line, client-safe message. Generic in production, detailed in + /// development. Suitable for machine formats (JSON/NDJSON/SSE/CSV). + message: String, + /// Causes chain, for the human-facing HTML backtrace. Empty in production. + backtrace: Vec, + /// The query that failed, shown to humans in development. `None` in + /// production and when not applicable. + query_number: Option, + /// Hint shown to humans in development on how to hide errors. `None` in + /// production. + note: Option<&'static str>, + /// `true` when this error was reduced to the generic production message. + /// This is the only environment-derived flag callers may branch on, so the + /// environment itself is never consulted outside [`ClientError::new`]. + is_generic: bool, +} + +impl ClientError { + /// Reduces a raw error to the client-safe form for the given environment. + /// In production, only the generic message survives; in development, the + /// full detail (message, backtrace, hint) is kept. + /// + /// This is the single location in the whole codebase that consults the + /// environment (the only call to `DevOrProd::is_prod`) to decide how much + /// of an error reaches a client. Renderers never make this decision; they + /// only forward the configured environment here and then format the result. + #[must_use] + pub fn new(error: &anyhow::Error, environment: DevOrProd, query_number: Option) -> Self { + if environment.is_prod() { + Self { + message: PRODUCTION_ERROR_MESSAGE.to_owned(), + backtrace: Vec::new(), + query_number: None, + note: None, + is_generic: true, + } + } else { + Self { + message: error.to_string(), + backtrace: get_backtrace_as_strings(error), + query_number, + note: Some(DEV_ERROR_NOTE), + is_generic: false, + } + } + } + + /// The single-line, client-safe message used by machine-readable formats + /// (JSON, NDJSON, SSE) and CSV. + #[must_use] + pub fn message(&self) -> &str { + &self.message + } + + /// Whether this error carries only the generic production message (i.e. it + /// was built in production). Lets the header path decide between rendering + /// the error inline and bubbling up to a top-level error response, without + /// itself looking at the environment. + #[must_use] + pub fn is_generic(&self) -> bool { + self.is_generic + } + + /// The data passed to the `error` HTML component. Every field is already + /// client-safe (empty/`None` in production). + #[must_use] + pub fn to_html_data(&self) -> Value { + json!({ + "query_number": self.query_number, + "description": self.message, + "backtrace": self.backtrace, + "note": self.note, + }) + } +} + +/// Collects the chain of error causes as strings, for the human-facing HTML +/// backtrace shown in development. +#[must_use] +pub(crate) fn get_backtrace_as_strings(error: &anyhow::Error) -> Vec { + let mut backtrace = vec![]; + let mut source = error.source(); + while let Some(s) = source { + backtrace.push(format!("{s}")); + source = s.source(); + } + backtrace +} fn error_to_html_string(app_state: &AppState, err: &anyhow::Error) -> anyhow::Result { let mut out = StringOutput::new(); @@ -18,15 +142,10 @@ fn error_to_html_string(app_state: &AppState, err: &anyhow::Error) -> anyhow::Re let error_template = app_state.all_templates.get_static_template("error")?; let registry = &app_state.all_templates.handlebars; let shell_ctx = handlebars::Context::null(); - let data = if app_state.config.environment.is_prod() { - json!(null) - } else { - json!({ - "description": err.to_string(), - "backtrace": get_backtrace_as_strings(err), - "note": "You can hide error messages like this one from your users by setting the 'environment' configuration option to 'production'.", - }) - }; + // Reduce the error to its client-safe form before rendering. In production + // this hides the message, backtrace, and source detail behind the generic + // text shown by the `error` template. + let data = ClientError::new(err, app_state.config.environment, None).to_html_data(); let err_ctx = handlebars::Context::wraps(data)?; let rc = &mut handlebars::RenderContext::new(None); @@ -160,3 +279,73 @@ pub(super) fn bind_error(e: std::io::Error, listen_on: std::net::SocketAddr) -> }; anyhow::anyhow!(e).context(ctx) } + +#[cfg(test)] +mod tests { + use super::*; + + /// Structural guard for the production-leak invariant. + /// + /// Every output format renders an error solely from the fields of + /// [`ClientError`] (via [`ClientError::message`] for machine formats and + /// [`ClientError::to_html_data`] for HTML). If, in production, neither of + /// those exposes any sensitive substring of the original error, then no + /// renderer can leak, including formats added in the future. This test + /// asserts exactly that at the single boundary, so it does not need to be + /// repeated per format. + #[test] + fn test_production_client_error_hides_all_detail() { + let sensitive = anyhow::anyhow!("DB error near 'secret_table'") + .context("The SQL statement sent by SQLPage was: SELECT * FROM secret_table") + .context("Error in file /srv/www/private/admin.sql"); + + let client_error = ClientError::new(&sensitive, DevOrProd::Production, Some(3)); + + // Everything a renderer can read about the error, serialized together. + let exposed = format!( + "{}\n{}", + client_error.message(), + client_error.to_html_data() + ); + + for needle in [ + "secret_table", + "SELECT", + "/srv/www/private/admin.sql", + ".sql", + "DB error", + ] { + assert!( + !exposed.contains(needle), + "production ClientError leaked {needle:?}: {exposed}" + ); + } + assert!( + exposed.to_lowercase().contains("administrator"), + "production ClientError should carry the generic message: {exposed}" + ); + // The query number is detail too: it must not survive into production. + assert!( + !exposed.contains('3'), + "production ClientError leaked the query number: {exposed}" + ); + assert!( + client_error.is_generic(), + "a production ClientError must report itself as generic" + ); + } + + /// In development, the full detail must be preserved so authors can debug. + #[test] + fn test_development_client_error_keeps_detail() { + let error = anyhow::anyhow!("near 'secret_table': syntax error") + .context("The SQL statement sent by SQLPage was: SELECT 1"); + let client_error = ClientError::new(&error, DevOrProd::Development, Some(2)); + assert!(client_error.message().contains("SELECT 1")); + assert!(!client_error.is_generic()); + let html = client_error.to_html_data(); + assert_eq!(html["query_number"], json!(2)); + assert!(html["backtrace"].as_array().is_some_and(|b| !b.is_empty())); + assert!(html["note"].is_string()); + } +} diff --git a/src/webserver/mod.rs b/src/webserver/mod.rs index a692616d..97448514 100644 --- a/src/webserver/mod.rs +++ b/src/webserver/mod.rs @@ -31,7 +31,7 @@ pub mod content_security_policy; pub mod database; -mod error; +pub(crate) mod error; pub mod error_with_status; pub mod http; pub mod http_client; diff --git a/tests/data_formats/csv_error_leak.sql b/tests/data_formats/csv_error_leak.sql new file mode 100644 index 00000000..4f97e53c --- /dev/null +++ b/tests/data_formats/csv_error_leak.sql @@ -0,0 +1,3 @@ +select 'csv' as component; +select 0 as id, 'before the error' as msg; +select * from definitely_missing_table_xyz; diff --git a/tests/data_formats/csv_error_no_rows.sql b/tests/data_formats/csv_error_no_rows.sql new file mode 100644 index 00000000..7cb8ee6f --- /dev/null +++ b/tests/data_formats/csv_error_no_rows.sql @@ -0,0 +1,4 @@ +select 'csv' as component; +-- Error before any data row: the CSV header is never written, so columns is +-- empty when handle_error runs. +select * from definitely_missing_table_xyz; diff --git a/tests/data_formats/json_error_leak.sql b/tests/data_formats/json_error_leak.sql new file mode 100644 index 00000000..e6ab3e79 --- /dev/null +++ b/tests/data_formats/json_error_leak.sql @@ -0,0 +1,3 @@ +select 'json' as component; +select 'before the error' as message; +select * from definitely_missing_table_xyz; diff --git a/tests/data_formats/mod.rs b/tests/data_formats/mod.rs index e459eef6..44a55330 100644 --- a/tests/data_formats/mod.rs +++ b/tests/data_formats/mod.rs @@ -231,3 +231,123 @@ async fn test_accept_json_redirect_still_works() -> actix_web::Result<()> { ); Ok(()) } + +/// Builds an `AppState` running in production mode. +async fn make_prod_app_data() -> actix_web::web::Data { + crate::common::init_log(); + let mut config = crate::common::test_config(); + config.environment = sqlpage::app_config::DevOrProd::Production; + crate::common::make_app_data_from_config(config).await +} + +async fn req_prod_with_accept(path: &str, accept: &str) -> String { + let app_data = make_prod_app_data().await; + let req = TestRequest::get() + .uri(path) + .insert_header((header::ACCEPT, accept)) + .app_data(app_data) + .to_srv_request(); + let resp = main_handler(req) + .await + .expect("request should not fail at the handler level"); + let body = test::read_body(resp).await; + String::from_utf8(body.to_vec()).unwrap() +} + +/// In production, a SQL error that happens mid-stream must not leak the SQL +/// statement, the source file path, or the raw database error text. +fn assert_no_sql_leak(body: &str, context: &str) { + for needle in [ + "definitely_missing_table_xyz", + "The SQL statement sent by SQLPage", + "error_leak.sql", + ".sql\"", + ] { + assert!( + !body.contains(needle), + "production error response leaked {needle:?} in {context}: {body}" + ); + } + assert!( + body.to_lowercase().contains("administrator"), + "production error response should contain a generic message in {context}: {body}" + ); +} + +#[actix_web::test] +async fn test_prod_json_error_does_not_leak_sql() { + let body = req_prod_with_accept( + "/tests/data_formats/json_error_leak.sql", + "application/json", + ) + .await; + assert!( + body.contains("before the error"), + "the good row should still be streamed: {body}" + ); + assert_no_sql_leak(&body, "json error"); +} + +#[actix_web::test] +async fn test_prod_csv_error_does_not_leak_sql() { + let app_data = make_prod_app_data().await; + if matches!( + app_data.db.info.database_type, + sqlpage::webserver::database::SupportedDatabase::Oracle + ) { + return; + } + let req = TestRequest::get() + .uri("/tests/data_formats/csv_error_leak.sql") + .insert_header((header::ACCEPT, "text/csv")) + .app_data(app_data) + .to_srv_request(); + let resp = main_handler(req).await.expect("handler should not fail"); + let body = test::read_body(resp).await; + let body = String::from_utf8(body.to_vec()).unwrap(); + assert!( + body.contains("before the error"), + "the good row should still be streamed: {body}" + ); + assert_no_sql_leak(&body, "csv error"); +} + +/// A CSV page can hit an error before its first data row (so no header has been +/// written and `columns` is empty). The generic error message must still be +/// emitted instead of an empty record. +#[actix_web::test] +async fn test_prod_csv_error_before_any_row_still_reports() { + let app_data = make_prod_app_data().await; + if matches!( + app_data.db.info.database_type, + sqlpage::webserver::database::SupportedDatabase::Oracle + ) { + return; + } + let req = TestRequest::get() + .uri("/tests/data_formats/csv_error_no_rows.sql") + .insert_header((header::ACCEPT, "text/csv")) + .app_data(app_data) + .to_srv_request(); + let resp = main_handler(req).await.expect("handler should not fail"); + let body = test::read_body(resp).await; + let body = String::from_utf8(body.to_vec()).unwrap(); + assert!( + body.to_lowercase().contains("administrator"), + "csv error before the first row must still emit the generic error message: {body:?}" + ); + assert_no_sql_leak(&body, "csv error before any row"); +} + +/// An author may only intend a page to be served as HTML, but a client can +/// request it with `Accept: application/json` and pick the JSON renderer. +/// In production that path must not leak SQL text either. +#[actix_web::test] +async fn test_prod_html_page_requested_as_json_does_not_leak_sql() { + let body = req_prod_with_accept( + "/tests/data_formats/text_error_leak.sql", + "application/json", + ) + .await; + assert_no_sql_leak(&body, "text page requested as json"); +} diff --git a/tests/data_formats/text_error_leak.sql b/tests/data_formats/text_error_leak.sql new file mode 100644 index 00000000..69fe4e7d --- /dev/null +++ b/tests/data_formats/text_error_leak.sql @@ -0,0 +1,2 @@ +select 'text' as component, 'before the error' as contents; +select * from definitely_missing_table_xyz;