diff --git a/CHANGELOG.md b/CHANGELOG.md index df2786e9b..97ad63575 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - Updated sqlx-oldapi to v0.6.57 to fix SQL Server fallback expressions such as `ISNULL($missing, 'default')` truncating defaults or failing for date values when the bound variable is `NULL`. - Fixed MSSQL `JSON_OBJECT('key': value)` expressions being rejected by SQLPage's parser, including when used in `SET` statements or nested in `sqlpage.*` function calls. - OIDC now checks both normalized request paths and their resolved SQL files against protected prefixes, closing authentication bypasses through path and clean-URL aliases. Nonce verification also rejects provider-returned Argon2 parameters outside SQLPage's fixed low-cost profile before hashing. +- SQL files with a mixed-case `.sql` extension are executed instead of served as static files. HTTP paths with trailing spaces, and Windows file aliases with trailing dots or NTFS stream names, are rejected to prevent source disclosure through storage aliases. - `cargo install sqlpage`, and any build from the crates.io tarball, no longer needs internet access. The browser libraries now come from npm and ship inside the published crate. Building from a git checkout needs `npm ci` first. Pre-built binaries and the Docker image are unaffected. - The browser libraries are now part of the browser scripts. SQLPage no longer defines the `window.tabler` and `window.bootstrap` globals; custom scripts that reached for them should load their own copy of Bootstrap. - The startup message now reports the address the server actually bound instead of the one it was configured with. diff --git a/examples/official-site/sqlpage/migrations/64_blog_routing.sql b/examples/official-site/sqlpage/migrations/64_blog_routing.sql index 44f920b7b..ebdcdcea6 100644 --- a/examples/official-site/sqlpage/migrations/64_blog_routing.sql +++ b/examples/official-site/sqlpage/migrations/64_blog_routing.sql @@ -23,12 +23,14 @@ SQLPage will redirect all requests that do not start with the prefix to `/ anyhow::Result<()> { for (i, component) in path.components().enumerate() { if let Component::Normal(c) = component { + let name = c.as_encoded_bytes(); if i == 0 && c.eq_ignore_ascii_case("sqlpage") { return Err(ErrorWithStatus { status: actix_web::http::StatusCode::FORBIDDEN, @@ -221,12 +222,24 @@ fn validate_unprivileged_path(path: &Path) -> anyhow::Result<()> { || "The /sqlpage/ path prefix is reserved for internal use. It is not public.", ); } - if c.as_encoded_bytes().starts_with(b".") { + if name.starts_with(b".") { return Err(ErrorWithStatus { status: actix_web::http::StatusCode::FORBIDDEN, }) .with_context(|| "Directory traversal is not allowed"); } + // SQL Server ignores trailing spaces when comparing file-store paths. + // Windows can also alias trailing dots and NTFS stream names to the + // same file. Reject these spellings before routing can serve a SQL + // file as an ordinary static asset. + if name.ends_with(b" ") + || cfg!(windows) && (name.ends_with(b".") || name.contains(&b':')) + { + return Err(ErrorWithStatus { + status: actix_web::http::StatusCode::FORBIDDEN, + }) + .with_context(|| "Noncanonical file paths are not allowed"); + } } else { return Err(ErrorWithStatus { status: actix_web::http::StatusCode::FORBIDDEN, @@ -242,6 +255,31 @@ fn validate_unprivileged_path(path: &Path) -> anyhow::Result<()> { Ok(()) } +#[cfg(test)] +mod unprivileged_path_tests { + use super::FileAccess; + use std::path::Path; + + #[test] + fn rejects_trailing_space_aliases() { + assert!(FileAccess::unprivileged(Path::new("index.sql ")).is_err()); + assert!(FileAccess::unprivileged(Path::new("folder /index.sql")).is_err()); + assert!(FileAccess::unprivileged(Path::new("index.sql")).is_ok()); + } + + #[cfg(windows)] + #[test] + fn rejects_windows_trailing_dot_aliases() { + assert!(FileAccess::unprivileged(Path::new("index.sql.")).is_err()); + } + + #[cfg(windows)] + #[test] + fn rejects_windows_ntfs_stream_aliases() { + assert!(FileAccess::unprivileged(Path::new("index.sql::$DATA")).is_err()); + } +} + fn is_path_missing_error(error: &std::io::Error) -> bool { matches!(error.kind(), ErrorKind::NotFound | ErrorKind::NotADirectory) } diff --git a/src/webserver/routing.rs b/src/webserver/routing.rs index 7f9ade70c..4d71492bf 100644 --- a/src/webserver/routing.rs +++ b/src/webserver/routing.rs @@ -16,7 +16,7 @@ //! - If found: **Execute** the SQL file //! - If not found: Look for custom 404 handlers (see Error Handling below) //! -//! #### Paths with `.sql` extension: +//! #### Paths with `.sql` extension (any ASCII letter case): //! - If the file exists: **Execute** the SQL file //! - If not found: Look for custom 404 handlers (see Error Handling below) //! @@ -292,10 +292,24 @@ where C: RoutingConfig, { let result = match &request_path.relative_file_path { - Some(path) => match path.extension().and_then(|e| e.to_str()) { - Some(SQL_EXTENSION) => find_file_or_not_found(path, SQL_EXTENSION, store).await?, - Some(extension) => match find_file(path, extension, store).await? { - Some(action) => action, + Some(path) => { + let _ = FileAccess::unprivileged(path)?; + match path.extension().and_then(|e| e.to_str()) { + Some(extension) if extension.eq_ignore_ascii_case(SQL_EXTENSION) => { + find_file_or_not_found(path, SQL_EXTENSION, store).await? + } + Some(extension) => match find_file(path, extension, store).await? { + Some(action) => action, + None => { + calculate_route_without_extension( + path_and_query, + path, + request_path.trailing_slash, + store, + ) + .await? + } + }, None => { calculate_route_without_extension( path_and_query, @@ -305,17 +319,8 @@ where ) .await? } - }, - None => { - calculate_route_without_extension( - path_and_query, - path, - request_path.trailing_slash, - store, - ) - .await? } - }, + } None => Redirect(config.prefix().to_string()), }; debug!("Route: [{path_and_query}] -> {result:?}"); @@ -420,6 +425,13 @@ mod tests { use std::path::PathBuf; use std::str::FromStr; + #[tokio::test] + async fn rejects_trailing_space_before_clean_url_resolution() { + let request = PathAndQuery::from_static("/index%20"); + let result = calculate_route(&request, &Store::new("index .sql"), &Config::default()).await; + assert!(result.is_err()); + } + #[test] fn canonical_request_uses_one_decode_for_policy_and_file_path() { let request = PathAndQuery::from_static("/my%20app/private/%2e/secret.sql"); @@ -487,7 +499,12 @@ mod tests { mod execute { use super::StoreConfig::{Default, File}; - use super::{do_route, execute}; + use super::{RoutingConfig, do_route, execute}; + use crate::filesystem::FileAccess; + use crate::webserver::routing::{ + CanonicalRequestPath, FileStore, canonical_resource_url_for_action, resolve_route, + }; + use awc::http::uri::PathAndQuery; #[tokio::test] async fn root_path_executes_index() { @@ -521,6 +538,39 @@ mod tests { assert_eq!(expected, actual); } + struct CaseInsensitiveSqlStore; + + impl FileStore for CaseInsensitiveSqlStore { + async fn contains(&self, access: FileAccess<'_>) -> anyhow::Result { + tokio::task::yield_now().await; + Ok(access + .path() + .to_string_lossy() + .eq_ignore_ascii_case("index.sql")) + } + } + + #[tokio::test] + async fn mixed_case_sql_alias_is_executed_on_case_insensitive_storage() { + let request = PathAndQuery::from_static("/index.%53QL"); + let config = super::Config::default(); + let path = CanonicalRequestPath::parse(&request, config.prefix()); + let route = resolve_route(&path, &request, &CaseInsensitiveSqlStore, &config) + .await + .unwrap(); + assert_eq!(&execute("index.SQL"), route.action()); + assert_eq!( + canonical_resource_url_for_action(config.prefix(), route.action()).as_deref(), + Some("/index.SQL") + ); + } + + #[tokio::test] + async fn mixed_case_sql_file_is_executed_with_site_prefix() { + let actual = do_route("/prefix/index.Sql", File("index.Sql"), Some("/prefix/")).await; + assert_eq!(execute("index.Sql"), actual); + } + #[tokio::test] async fn no_extension() { let actual = do_route("/path", File("path.sql"), None).await; diff --git a/tests/core/mixed_case.Sql b/tests/core/mixed_case.Sql new file mode 100644 index 000000000..586bc68ef --- /dev/null +++ b/tests/core/mixed_case.Sql @@ -0,0 +1,2 @@ +-- SQLPAGE_SOURCE_CANARY: this comment must never be returned over HTTP. +SELECT 'text' AS component, 'SQL executed' AS contents; diff --git a/tests/core/mod.rs b/tests/core/mod.rs index 1845400ef..6062d877c 100644 --- a/tests/core/mod.rs +++ b/tests/core/mod.rs @@ -7,6 +7,8 @@ use sqlx::executor::Executor as _; use crate::common::{make_app_data_from_config, req_path, req_path_with_app_data, test_config}; +mod path_aliases; + #[actix_web::test] async fn test_concurrent_requests() { let components = [ diff --git a/tests/core/path_aliases.rs b/tests/core/path_aliases.rs new file mode 100644 index 000000000..a83b3f68a --- /dev/null +++ b/tests/core/path_aliases.rs @@ -0,0 +1,56 @@ +use actix_web::{body::to_bytes, http::StatusCode}; +use sqlpage::webserver::http::main_handler; + +use crate::common::get_request_to; + +async fn assert_sql_response(path: &str, expected_status: StatusCode) { + let request = get_request_to(path).await.unwrap().to_srv_request(); + let response = match main_handler(request).await { + Ok(response) => response.into_parts().1, + Err(error) => error.error_response(), + }; + assert_eq!(response.status(), expected_status, "{path}"); + let body = to_bytes(response.into_body()).await.unwrap(); + let body = String::from_utf8(body.to_vec()).unwrap(); + assert!(!body.contains("SQLPAGE_SOURCE_CANARY"), "{path}: {body}"); + if expected_status == StatusCode::OK { + assert!(body.starts_with(""), "{path}: {body}"); + assert!(body.contains("SQL executed"), "{path}: {body}"); + } +} + +#[actix_web::test] +async fn mixed_case_sql_file_is_executed() { + assert_sql_response("/tests/core/mixed_case.%53ql", StatusCode::OK).await; +} + +#[actix_web::test] +async fn trailing_space_sql_path_is_forbidden() { + assert_sql_response("/tests/core/sql_source.sql%20", StatusCode::FORBIDDEN).await; +} + +#[cfg(windows)] +async fn assert_windows_alias_response(path: &str, expected_status: StatusCode) { + // Establish that this spelling really reaches the SQL source on this filesystem. + let source = std::fs::read(path.strip_prefix('/').unwrap()).unwrap(); + assert_eq!(source, include_bytes!("sql_source.sql")); + assert_sql_response(path, expected_status).await; +} + +#[cfg(windows)] +#[actix_web::test] +async fn windows_mixed_case_sql_alias_is_executed() { + assert_windows_alias_response("/tests/core/sql_source.SQL", StatusCode::OK).await; +} + +#[cfg(windows)] +#[actix_web::test] +async fn windows_trailing_dot_sql_alias_is_forbidden() { + assert_windows_alias_response("/tests/core/sql_source.sql.", StatusCode::FORBIDDEN).await; +} + +#[cfg(windows)] +#[actix_web::test] +async fn windows_ntfs_stream_sql_alias_is_forbidden() { + assert_windows_alias_response("/tests/core/sql_source.sql::$DATA", StatusCode::FORBIDDEN).await; +} diff --git a/tests/core/sql_source.sql b/tests/core/sql_source.sql new file mode 100644 index 000000000..586bc68ef --- /dev/null +++ b/tests/core/sql_source.sql @@ -0,0 +1,2 @@ +-- SQLPAGE_SOURCE_CANARY: this comment must never be returned over HTTP. +SELECT 'text' AS component, 'SQL executed' AS contents;