Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ SQLPage will redirect all requests that do not start with the prefix to `/<site_

**Directory requests (paths ending with `/`)**: SQLPage looks for an `index.sql` file in that directory and executes it if found.

**Direct SQL file requests (`.sql` extension)**: SQLPage executes the requested SQL file if it exists.
**Direct SQL file requests (`.sql` extension)**: SQLPage executes the requested SQL file if it exists. The extension is matched without regard to ASCII letter case, so `/report.SQL` is also executed. It is never served as source text.

**Static asset requests (other extensions)**: SQLPage serves files like CSS, JavaScript, images, or any other static content directly.

**Clean URL requests (no extension)**: SQLPage first tries to find a matching `.sql` file. If that doesn''t exist but there''s an `index.sql` file in a directory with the same name, it redirects to the directory path with a trailing slash.

Paths with trailing spaces are rejected. On Windows, paths with trailing dots or NTFS stream names are rejected too. These spellings can refer to a different file than their URL suggests on some storage systems.

### Error Handling

When, after applying each of the rules above in order, SQLPage can''t find a requested file,
Expand Down
40 changes: 39 additions & 1 deletion src/filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ impl FileSystem {
fn validate_unprivileged_path(path: &Path) -> 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,
Expand All @@ -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.
Comment on lines +231 to +234

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] this shouldn't be a comment, it should be 3 different tests we can run in the CI. I see you have some of them already

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,
Expand All @@ -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)
}
Expand Down
82 changes: 66 additions & 16 deletions src/webserver/routing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
//!
Expand Down Expand Up @@ -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,
Expand All @@ -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:?}");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -521,6 +538,39 @@ mod tests {
assert_eq!(expected, actual);
}

struct CaseInsensitiveSqlStore;

impl FileStore for CaseInsensitiveSqlStore {
async fn contains(&self, access: FileAccess<'_>) -> anyhow::Result<bool> {
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;
Expand Down
2 changes: 2 additions & 0 deletions tests/core/mixed_case.Sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- SQLPAGE_SOURCE_CANARY: this comment must never be returned over HTTP.
SELECT 'text' AS component, 'SQL executed' AS contents;
2 changes: 2 additions & 0 deletions tests/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
56 changes: 56 additions & 0 deletions tests/core/path_aliases.rs
Original file line number Diff line number Diff line change
@@ -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("<!DOCTYPE html>"), "{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;
}
2 changes: 2 additions & 0 deletions tests/core/sql_source.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- SQLPAGE_SOURCE_CANARY: this comment must never be returned over HTTP.
SELECT 'text' AS component, 'SQL executed' AS contents;
Loading