From 35fe27e675e6dd6e62b9eb08688774bd7f045db2 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Thu, 24 Sep 2026 08:15:22 +0200 Subject: [PATCH 1/2] refactor(oidc) :: centralize route authorization --- src/webserver/http.rs | 31 +++-- src/webserver/oidc.rs | 181 +++++++++++++------------- src/webserver/routing.rs | 265 +++++++++++++++++++++++++++++++++------ tests/oidc/mod.rs | 37 +++++- 4 files changed, 372 insertions(+), 142 deletions(-) diff --git a/src/webserver/http.rs b/src/webserver/http.rs index 892c7aa46..76e48af27 100644 --- a/src/webserver/http.rs +++ b/src/webserver/http.rs @@ -33,7 +33,7 @@ use crate::filesystem::FileAccess; use crate::webserver::routing::RoutingAction::{ CustomNotFound, Execute, NotFound, Redirect, Serve, }; -use crate::webserver::routing::{AppFileStore, calculate_route}; +use crate::webserver::routing::{AppFileStore, CanonicalRequestPath, ResolvedRoute, resolve_route}; use actix_web::body::MessageBody; use anyhow::{Context, bail}; use chrono::{DateTime, Utc}; @@ -489,21 +489,20 @@ pub async fn main_handler( .uri() .path_and_query() .ok_or_else(|| ErrorBadRequest("expected valid path with query from request"))?; - let authorized_route = service_request - .extensions_mut() - .remove::(); - let routing_action = match authorized_route { - Some(action) => Ok(action), - None => calculate_route(path_and_query, &store, &app_state.config).await, - }; - let routing_action = match routing_action { - Ok(action) => action, - Err(e) => { - let e = e.context(format!( - "The server was unable to fulfill your request. \n\ - The following page is not accessible: {path_and_query:?}" - )); - return Err(anyhow_err_to_actix(e, app_state)); + let authorized_route = { service_request.extensions_mut().remove::() }; + let routing_action = if let Some(route) = authorized_route { + route.into_action() + } else { + let path = CanonicalRequestPath::parse(path_and_query, &app_state.config.site_prefix); + match resolve_route(&path, path_and_query, &store, &app_state.config).await { + Ok(route) => route.into_action(), + Err(e) => { + let e = e.context(format!( + "The server was unable to fulfill your request. \n\ + The following page is not accessible: {path_and_query:?}" + )); + return Err(anyhow_err_to_actix(e, app_state)); + } } }; match routing_action { diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index ce9f73178..eeacae3cf 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -6,7 +6,9 @@ use std::{future::Future, pin::Pin, str::FromStr, sync::Arc}; use tokio::time::Instant; use crate::webserver::http_client::get_http_client_from_appdata; -use crate::webserver::routing::{AppFileStore, RoutingAction, calculate_route}; +use crate::webserver::routing::{ + AppFileStore, CanonicalRequestPath, RoutingAction, canonical_url_path, resolve_route, +}; use crate::{AppState, app_config::AppConfig}; use actix_web::http::header; use actix_web::{ @@ -140,47 +142,8 @@ impl TryFrom<&AppConfig> for OidcConfig { impl OidcConfig { #[must_use] pub fn is_public_path(&self, path: &str) -> bool { - let Some(path) = normalize_oidc_path(path) else { - return false; - }; - self.is_public_normalized_path(&path) - } - - fn is_public_normalized_path(&self, path: &str) -> bool { - // Filesystems can resolve differently cased paths to the same file. - // Protect all ASCII case aliases; a public override must still match - // the configured spelling exactly. - !self.protected_paths.iter().any(|p| { - normalize_oidc_path(p).is_none_or(|prefix| { - path.to_ascii_lowercase() - .starts_with(&prefix.to_ascii_lowercase()) - }) - }) || self - .public_paths - .iter() - .any(|p| normalize_oidc_path(p).is_some_and(|prefix| path.starts_with(&prefix))) - } - - fn is_public_route(&self, action: &RoutingAction) -> bool { - let (RoutingAction::Execute(path) | RoutingAction::CustomNotFound(path)) = action else { - return true; - }; - let Some(site_prefix) = normalize_oidc_path(&self.site_prefix) else { - return false; - }; - let mut route = site_prefix.trim_end_matches('/').to_string(); - for component in path.components() { - if let std::path::Component::Normal(name) = component { - let Some(name) = name.to_str() else { - return false; - }; - route.push('/'); - route.push_str(name); - } else { - return false; - } - } - self.is_public_normalized_path(&route) + let path = canonical_url_path(path); + OidcPathPolicy::new(self).is_public(path.as_deref()) } /// Creates a custom ID token verifier that supports multiple issuers @@ -223,35 +186,53 @@ impl OidcConfig { } } -/// Match the router's decoded filesystem path components before applying URL -/// prefix rules. Preserve a final slash: `/public` and `/public/` are different -/// routes. Invalid or ambiguous paths must never be classified as public. -fn normalize_oidc_path(path: &str) -> Option { - use std::path::{Component, Path}; +/// Prefixes are decoded once when the OIDC state starts. The protected +/// spelling is case-folded conservatively for case-insensitive file stores; +/// public exceptions still require their configured spelling. +struct OidcPathPolicy { + protected: Option>, + public: Vec, +} - let decoded = percent_encoding::percent_decode_str(path) - .decode_utf8() - .ok()?; - if !decoded.starts_with('/') || decoded.contains('\\') { - return None; +impl OidcPathPolicy { + fn new(config: &OidcConfig) -> Self { + Self { + protected: config + .protected_paths + .iter() + .map(|path| canonical_url_path(path).map(|path| path.to_ascii_lowercase())) + .collect(), + public: config + .public_paths + .iter() + .filter_map(|path| canonical_url_path(path)) + .collect(), + } } - let mut normalized = String::from("/"); - for component in Path::new(decoded.as_ref()).components() { - match component { - Component::RootDir | Component::CurDir => {} - Component::Normal(name) => { - if normalized.len() > 1 { - normalized.push('/'); - } - normalized.push_str(name.to_str()?); + + fn is_public(&self, path: Option<&str>) -> bool { + let (Some(path), Some(protected)) = (path, self.protected.as_ref()) else { + return false; + }; + let folded = path.to_ascii_lowercase(); + !protected.iter().any(|prefix| folded.starts_with(prefix)) + || self.public.iter().any(|prefix| path.starts_with(prefix)) + } + + fn is_public_route(&self, action: &RoutingAction, site_prefix: &str) -> bool { + match action { + RoutingAction::Execute(_) + | RoutingAction::CustomNotFound(_) + | RoutingAction::Serve(_) => { + let resource = crate::webserver::routing::canonical_resource_url_for_action( + site_prefix, + action, + ); + self.is_public(resource.as_deref()) } - Component::ParentDir | Component::Prefix(_) => return None, + RoutingAction::Redirect(_) | RoutingAction::NotFound => true, } } - if decoded.ends_with('/') && normalized != "/" { - normalized.push('/'); - } - Some(normalized) } fn get_app_host(config: &AppConfig) -> String { @@ -288,6 +269,7 @@ struct OidcSnapshot { pub struct OidcState { pub config: OidcConfig, + path_policy: OidcPathPolicy, /// Current snapshot. The lock is only held for the instant /// needed to clone/swap the Arc — never across await points. snapshot: std::sync::RwLock>, @@ -301,6 +283,7 @@ impl OidcState { let (client, end_session_endpoint) = build_oidc_client(&oidc_cfg, &http_client).await?; Ok(Self { + path_policy: OidcPathPolicy::new(&oidc_cfg), config: oidc_cfg, snapshot: std::sync::RwLock::new(Arc::new(OidcSnapshot { client, @@ -409,13 +392,6 @@ impl OidcState { } } -fn request_is_builtin_static_path(request: &ServiceRequest, config: &OidcConfig) -> bool { - request - .path() - .strip_prefix(&config.site_prefix) - .is_some_and(|path| path.starts_with("sqlpage/")) -} - pub async fn initialize_oidc_state( app_config: &AppConfig, ) -> anyhow::Result>> { @@ -566,26 +542,38 @@ async fn handle_unauthenticated_request( ) -> MiddlewareResponse { log::debug!("Handling unauthenticated request to {}", request.path()); - if oidc_state.config.is_public_path(request.path()) { + let path_and_query = request.uri().path_and_query(); + let canonical = path_and_query + .map(|path| CanonicalRequestPath::parse(path, &oidc_state.config.site_prefix)); + if canonical + .as_ref() + .is_some_and(|path| oidc_state.path_policy.is_public(path.normalized_url())) + { + let canonical = canonical.expect("checked above"); + // Built-in assets are handled by their own Actix services, not the + // SQL-file router. Classify them explicitly before route resolution. + if canonical.is_builtin_static() { + return MiddlewareResponse::Forward(request); + } let route = { let app_state = request.app_data::>(); - let path_and_query = request.uri().path_and_query(); match (app_state, path_and_query) { (Some(state), Some(path_and_query)) => { let store = AppFileStore::new(&state.sql_file_cache, &state.file_system, state); - Some(calculate_route(path_and_query, &store, &state.config).await) + Some(resolve_route(&canonical, path_and_query, &store, &state.config).await) } _ => None, } }; match route { - Some(Ok(action)) if oidc_state.config.is_public_route(&action) => { + Some(Ok(route)) + if oidc_state + .path_policy + .is_public_route(route.action(), &oidc_state.config.site_prefix) => + { // Reuse the authorized route in the main handler so a file-store // change between this check and dispatch cannot change its identity. - request.extensions_mut().insert(action); - return MiddlewareResponse::Forward(request); - } - Some(Err(_)) if request_is_builtin_static_path(&request, &oidc_state.config) => { + request.extensions_mut().insert(route); return MiddlewareResponse::Forward(request); } _ => {} @@ -1691,10 +1679,22 @@ mod tests { assert!(config.is_public_path("/users/profile.sql")); } + #[test] + fn compiled_policy_checks_the_resolved_static_target_and_fails_closed() { + let config = test_oidc_config_with_paths(vec!["/private/secret.txt".to_string()], vec![]); + let policy = OidcPathPolicy::new(&config); + assert!(policy.is_public(Some("/private/alias.txt"))); + let action = RoutingAction::Serve("private/secret.txt".into()); + assert!(!policy.is_public_route(&action, &config.site_prefix)); + + let invalid = test_oidc_config_with_paths(vec!["/%ff".to_string()], vec![]); + assert!(!OidcPathPolicy::new(&invalid).is_public(Some("/public/page.sql"))); + } + #[tokio::test] async fn resolved_sql_file_aliases_do_not_bypass_oidc_protection() { use crate::filesystem::FileAccess; - use crate::webserver::routing::FileStore; + use crate::webserver::routing::{FileStore, calculate_route}; use awc::http::uri::PathAndQuery; struct OneFile(&'static str); @@ -1713,6 +1713,7 @@ mod tests { ("/private/missing", "private/404.sql"), ] { let oidc = test_oidc_config_with_paths(vec![format!("/{file}")], vec![]); + let policy = OidcPathPolicy::new(&oidc); assert!(oidc.is_public_path(url), "{url} is a distinct URL spelling"); let route = calculate_route( &PathAndQuery::try_from(url).unwrap(), @@ -1728,7 +1729,7 @@ mod tests { ); } assert!( - !oidc.is_public_route(&route), + !policy.is_public_route(&route, &oidc.site_prefix), "{url} resolves to a protected SQL file" ); } @@ -1736,17 +1737,19 @@ mod tests { let mut prefixed_oidc = test_oidc_config_with_paths(vec!["/my%20app/private/secret.sql".to_string()], vec![]); prefixed_oidc.site_prefix = "/my%20app/".to_string(); - assert!( - !prefixed_oidc.is_public_route(&RoutingAction::Execute("private/secret.sql".into())) - ); + assert!(!OidcPathPolicy::new(&prefixed_oidc).is_public_route( + &RoutingAction::Execute("private/secret.sql".into()), + &prefixed_oidc.site_prefix + )); let public_oidc = test_oidc_config_with_paths( vec!["/private".to_string()], vec!["/private/public/".to_string()], ); - assert!( - public_oidc.is_public_route(&RoutingAction::Execute("private/public/index.sql".into())) - ); + assert!(OidcPathPolicy::new(&public_oidc).is_public_route( + &RoutingAction::Execute("private/public/index.sql".into()), + &public_oidc.site_prefix + )); } #[test] diff --git a/src/webserver/routing.rs b/src/webserver/routing.rs index 62a7e21b0..93dff562d 100644 --- a/src/webserver/routing.rs +++ b/src/webserver/routing.rs @@ -99,6 +99,132 @@ pub enum RoutingAction { Serve(PathBuf), } +/// The HTTP path as interpreted by routing. The request is percent-decoded +/// once; the URL spelling used by OIDC and the filesystem candidate are +/// derived from those same bytes. Invalid UTF-8 has no public URL identity, +/// but can still be routed for authenticated users on Unix. +#[derive(Debug)] +pub(crate) struct CanonicalRequestPath { + normalized_url: Option, + relative_file_path: Option, + trailing_slash: bool, + builtin_static: bool, +} + +impl CanonicalRequestPath { + pub(crate) fn parse(path_and_query: &PathAndQuery, prefix: &str) -> Self { + let raw_path = path_and_query.path(); + let decoded = percent_encoding::percent_decode_str(raw_path).collect::>(); + let normalized_url = normalized_url_path(&decoded); + let relative = raw_path.strip_prefix(prefix); + let relative_file_path = relative.and_then(|_| { + let decoded_prefix = percent_encoding::percent_decode_str(prefix).collect::>(); + decoded + .strip_prefix(decoded_prefix.as_slice()) + .map(decoded_file_path) + }); + Self { + normalized_url, + relative_file_path, + trailing_slash: raw_path.ends_with(FORWARD_SLASH), + builtin_static: relative.is_some_and(|path| path.starts_with("sqlpage/")), + } + } + + pub(crate) fn normalized_url(&self) -> Option<&str> { + self.normalized_url.as_deref() + } + + pub(crate) const fn is_builtin_static(&self) -> bool { + self.builtin_static + } +} + +#[cfg(unix)] +fn decoded_file_path(decoded: &[u8]) -> PathBuf { + use std::{ffi::OsString, os::unix::ffi::OsStringExt}; + PathBuf::from(OsString::from_vec(decoded.to_vec())) +} + +#[cfg(not(unix))] +fn decoded_file_path(decoded: &[u8]) -> PathBuf { + PathBuf::from(String::from_utf8_lossy(decoded).as_ref()) +} + +/// Canonical URL spelling for policy prefixes and decoded request paths. +/// Trailing slash is retained because `/public` and `/public/` differ. +pub(crate) fn canonical_url_path(encoded: &str) -> Option { + let decoded = percent_encoding::percent_decode_str(encoded).collect::>(); + normalized_url_path(&decoded) +} + +fn normalized_url_path(decoded: &[u8]) -> Option { + use std::path::Component; + + let decoded = std::str::from_utf8(decoded).ok()?; + if !decoded.starts_with('/') || decoded.contains('\\') { + return None; + } + let mut normalized = String::from("/"); + for component in Path::new(decoded).components() { + match component { + Component::RootDir | Component::CurDir => {} + Component::Normal(name) => { + if normalized.len() > 1 { + normalized.push('/'); + } + normalized.push_str(name.to_str()?); + } + Component::ParentDir | Component::Prefix(_) => return None, + } + } + if decoded.ends_with('/') && normalized != "/" { + normalized.push('/'); + } + Some(normalized) +} + +/// Routing's resolved action is kept intact from authorization to dispatch. +#[derive(Debug)] +pub(crate) struct ResolvedRoute(RoutingAction); + +impl ResolvedRoute { + pub(crate) fn action(&self) -> &RoutingAction { + &self.0 + } + + pub(crate) fn into_action(self) -> RoutingAction { + self.0 + } +} + +pub(crate) fn canonical_resource_url_for_action( + site_prefix: &str, + action: &RoutingAction, +) -> Option { + match action { + Execute(path) | CustomNotFound(path) | Serve(path) => { + canonical_resource_url(site_prefix, path) + } + Redirect(_) | NotFound => None, + } +} + +fn canonical_resource_url(site_prefix: &str, path: &Path) -> Option { + use std::path::Component; + + let prefix = canonical_url_path(site_prefix)?; + let mut url = prefix.trim_end_matches('/').to_string(); + for component in path.components() { + let Component::Normal(name) = component else { + return None; + }; + url.push('/'); + url.push_str(name.to_str()?); + } + Some(url) +} + #[expect(async_fn_in_trait)] pub trait FileStore { async fn contains(&self, access: FileAccess<'_>) -> anyhow::Result; @@ -147,56 +273,64 @@ where T: FileStore, C: RoutingConfig, { - let result = match check_path(path_and_query, config) { - Ok(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, - None => calculate_route_without_extension(path_and_query, path, store).await?, - }, - None => calculate_route_without_extension(path_and_query, path, store).await?, - }, - Err(action) => action, - }; - debug!("Route: [{path_and_query}] -> {result:?}"); - Ok(result) + let request_path = CanonicalRequestPath::parse(path_and_query, config.prefix()); + Ok(resolve_route(&request_path, path_and_query, store, config) + .await? + .into_action()) } -fn check_path(path_and_query: &PathAndQuery, config: &C) -> Result +pub(crate) async fn resolve_route( + request_path: &CanonicalRequestPath, + path_and_query: &PathAndQuery, + store: &T, + config: &C, +) -> anyhow::Result where + T: FileStore, C: RoutingConfig, { - match path_and_query.path().strip_prefix(config.prefix()) { - None => Err(Redirect(config.prefix().to_string())), - Some(path) => { - let decoded = percent_encoding::percent_decode_str(path); - #[cfg(unix)] - { - use std::ffi::OsString; - use std::os::unix::ffi::OsStringExt; - - let decoded = decoded.collect::>(); - Ok(PathBuf::from(OsString::from_vec(decoded))) - } - #[cfg(not(unix))] - { - Ok(PathBuf::from(decoded.decode_utf8_lossy().as_ref())) + 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, + None => { + calculate_route_without_extension( + path_and_query, + path, + request_path.trailing_slash, + store, + ) + .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:?}"); + Ok(ResolvedRoute(result)) } async fn calculate_route_without_extension( path_and_query: &PathAndQuery, - mut path: PathBuf, + path: &Path, + trailing_slash: bool, store: &T, ) -> anyhow::Result where T: FileStore, { - if path_and_query.path().ends_with(FORWARD_SLASH) { - path.push(INDEX); - find_file_or_not_found(&path, SQL_EXTENSION, store).await + if trailing_slash { + find_file_or_not_found(&path.join(INDEX), SQL_EXTENSION, store).await } else { let path_with_ext = PathBuf::from(format!("{}.{SQL_EXTENSION}", path.display())); match find_file_or_not_found(&path_with_ext, SQL_EXTENSION, store).await? { @@ -274,13 +408,72 @@ fn append_to_path(path_and_query: &PathAndQuery, append: &str) -> String { #[cfg(test)] mod tests { use super::RoutingAction::{CustomNotFound, Execute, NotFound, Redirect, Serve}; - use super::{FileAccess, FileStore, RoutingAction, RoutingConfig, calculate_route}; + use super::{ + CanonicalRequestPath, FileAccess, FileStore, RoutingAction, RoutingConfig, calculate_route, + canonical_resource_url_for_action, resolve_route, + }; use StoreConfig::{Custom, Default, Empty, File}; use awc::http::uri::PathAndQuery; use std::default::Default as StdDefault; use std::path::PathBuf; use std::str::FromStr; + #[test] + fn canonical_request_uses_one_decode_for_policy_and_file_path() { + let request = PathAndQuery::from_static("/my%20app/private/%2e/secret.sql"); + let path = CanonicalRequestPath::parse(&request, "/my%20app/"); + assert_eq!(path.normalized_url(), Some("/my app/private/secret.sql")); + assert_eq!( + path.relative_file_path.as_deref(), + Some(std::path::Path::new("private/secret.sql")) + ); + + let encoded_twice = PathAndQuery::from_static("/my%20app/private/%252e/secret.sql"); + let path = CanonicalRequestPath::parse(&encoded_twice, "/my%20app/"); + assert_eq!( + path.normalized_url(), + Some("/my app/private/%2e/secret.sql") + ); + assert_eq!( + path.relative_file_path.as_deref(), + Some(std::path::Path::new("private/%2e/secret.sql")) + ); + + let invalid = PathAndQuery::from_static("/my%20app/private/%ff.sql"); + let path = CanonicalRequestPath::parse(&invalid, "/my%20app/"); + assert_eq!(path.normalized_url(), None); + assert!(path.relative_file_path.is_some()); + + let malformed_prefix = CanonicalRequestPath::parse(&request, "/my%2"); + assert!(malformed_prefix.relative_file_path.is_none()); + } + + #[test] + fn builtin_assets_are_identified_from_the_unmodified_prefixed_path() { + let request = PathAndQuery::from_static("/my%20app/sqlpage/sqlpage.js"); + let path = CanonicalRequestPath::parse(&request, "/my%20app/"); + assert!(path.is_builtin_static()); + + let encoded = PathAndQuery::from_static("/my%20app/%73qlpage/sqlpage.js"); + let path = CanonicalRequestPath::parse(&encoded, "/my%20app/"); + assert!(!path.is_builtin_static()); + } + + #[tokio::test] + async fn resolved_route_carries_the_dispatched_file_identity() { + let request = PathAndQuery::from_static("/my%20app/private/secret"); + let config = Config::new("/my%20app/"); + let path = CanonicalRequestPath::parse(&request, config.prefix()); + let route = resolve_route(&path, &request, &Store::new("private/secret.sql"), &config) + .await + .unwrap(); + assert_eq!( + canonical_resource_url_for_action(config.prefix(), route.action()).as_deref(), + Some("/my app/private/secret.sql") + ); + assert_eq!(route.into_action(), execute("private/secret.sql")); + } + mod execute { use super::StoreConfig::{Default, File}; use super::{do_route, execute}; diff --git a/tests/oidc/mod.rs b/tests/oidc/mod.rs index 56f8c34ce..f1bac1be2 100644 --- a/tests/oidc/mod.rs +++ b/tests/oidc/mod.rs @@ -320,6 +320,21 @@ async fn setup_oidc_test( Error = actix_web::Error, >, FakeOidcProvider, +) { + setup_oidc_test_with_paths(provider_mutator, &["/"], &[]).await +} + +async fn setup_oidc_test_with_paths( + provider_mutator: impl FnOnce(&mut ProviderState<'_>), + protected_paths: &[&str], + public_paths: &[&str], +) -> ( + impl actix_web::dev::Service< + actix_http::Request, + Response = actix_web::dev::ServiceResponse, + Error = actix_web::Error, + >, + FakeOidcProvider, ) { use sqlpage::{ AppState, @@ -330,6 +345,8 @@ async fn setup_oidc_test( provider.with_state_mut(provider_mutator); let db_url = test_database_url(); + let protected_paths = serde_json::to_string(protected_paths).unwrap(); + let public_paths = serde_json::to_string(public_paths).unwrap(); let config_json = format!( r#"{{ "database_url": "{db_url}", @@ -343,7 +360,8 @@ async fn setup_oidc_test( "oidc_issuer_url": "{}", "oidc_client_id": "{}", "oidc_client_secret": "{}", - "oidc_protected_paths": ["/"], + "oidc_protected_paths": {protected_paths}, + "oidc_public_paths": {public_paths}, "host": "localhost:1" }}"#, provider.issuer_url, provider.client_id, provider.client_secret @@ -355,6 +373,23 @@ async fn setup_oidc_test( (app, provider) } +#[actix_web::test] +async fn test_public_clean_url_cannot_execute_a_protected_sql_file() { + let file = "/tests/sql_test_files/data/regex_match_routing.sql"; + let protected_paths = [file]; + let (app, _provider) = setup_oidc_test_with_paths(|_| {}, &protected_paths, &[]).await; + let mut cookies: Vec> = Vec::new(); + + let clean_url = file.strip_suffix(".sql").unwrap(); + let resp = request_with_cookies!(app, test::TestRequest::get().uri(clean_url), cookies); + assert_eq!(resp.status(), StatusCode::SEE_OTHER); + let resp = request_with_cookies!(app, test::TestRequest::get().uri(file), cookies); + assert_eq!(resp.status(), StatusCode::SEE_OTHER); + + let public = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies); + assert_ne!(public.status(), StatusCode::SEE_OTHER); +} + #[actix_web::test] async fn test_oidc_cached_authorization_redirect_cannot_replay_consumed_state() { let (app, provider) = setup_oidc_test(|_| {}).await; From 68a1edd227c0c159aa0405da8cf9d8bee9b61b16 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Thu, 24 Sep 2026 10:28:22 +0200 Subject: [PATCH 2/2] fix(oidc) :: keep top-level builtin assets public --- src/webserver/oidc.rs | 14 +++++++++----- src/webserver/routing.rs | 13 ++++++++++++- src/webserver/static_content.rs | 11 +++++++++++ tests/oidc/mod.rs | 23 +++++++++++++++++++++++ 4 files changed, 55 insertions(+), 6 deletions(-) diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index eeacae3cf..153284894 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -545,16 +545,20 @@ async fn handle_unauthenticated_request( let path_and_query = request.uri().path_and_query(); let canonical = path_and_query .map(|path| CanonicalRequestPath::parse(path, &oidc_state.config.site_prefix)); + // Built-in assets have fixed content and are served by their own Actix + // services, so they stay public even when their top-level URLs match a + // configured protected-path prefix. + if canonical + .as_ref() + .is_some_and(CanonicalRequestPath::is_builtin_static) + { + return MiddlewareResponse::Forward(request); + } if canonical .as_ref() .is_some_and(|path| oidc_state.path_policy.is_public(path.normalized_url())) { let canonical = canonical.expect("checked above"); - // Built-in assets are handled by their own Actix services, not the - // SQL-file router. Classify them explicitly before route resolution. - if canonical.is_builtin_static() { - return MiddlewareResponse::Forward(request); - } let route = { let app_state = request.app_data::>(); match (app_state, path_and_query) { diff --git a/src/webserver/routing.rs b/src/webserver/routing.rs index 93dff562d..7f9ade70c 100644 --- a/src/webserver/routing.rs +++ b/src/webserver/routing.rs @@ -127,7 +127,9 @@ impl CanonicalRequestPath { normalized_url, relative_file_path, trailing_slash: raw_path.ends_with(FORWARD_SLASH), - builtin_static: relative.is_some_and(|path| path.starts_with("sqlpage/")), + builtin_static: relative.is_some_and(|path| { + path.starts_with("sqlpage/") || super::static_content::is_builtin_asset_path(path) + }), } } @@ -457,6 +459,15 @@ mod tests { let encoded = PathAndQuery::from_static("/my%20app/%73qlpage/sqlpage.js"); let path = CanonicalRequestPath::parse(&encoded, "/my%20app/"); assert!(!path.is_builtin_static()); + + let filename = crate::utils::static_filename!("sqlpage.js"); + let request = PathAndQuery::from_str(&format!("/my%20app/{filename}")).unwrap(); + let path = CanonicalRequestPath::parse(&request, "/my%20app/"); + assert!(path.is_builtin_static()); + + let encoded = PathAndQuery::from_str(&format!("/my%20app/%73{}", &filename[1..])).unwrap(); + let path = CanonicalRequestPath::parse(&encoded, "/my%20app/"); + assert!(!path.is_builtin_static()); } #[tokio::test] diff --git a/src/webserver/static_content.rs b/src/webserver/static_content.rs index 8adff33c6..2e6f6660b 100644 --- a/src/webserver/static_content.rs +++ b/src/webserver/static_content.rs @@ -7,6 +7,17 @@ use actix_web::{ web, }; +pub(super) fn is_builtin_asset_path(path: &str) -> bool { + [ + static_filename!("sqlpage.js"), + static_filename!("apexcharts.js"), + static_filename!("tomselect.js"), + static_filename!("sqlpage.css"), + static_filename!("favicon.svg"), + ] + .contains(&path) +} + macro_rules! static_file_endpoint { ($filestem:literal, $extension:literal, $mime:literal) => {{ const FILENAME_WITH_TAG: &str = static_filename!(concat!($filestem, ".", $extension)); diff --git a/tests/oidc/mod.rs b/tests/oidc/mod.rs index f1bac1be2..4eba0421c 100644 --- a/tests/oidc/mod.rs +++ b/tests/oidc/mod.rs @@ -390,6 +390,29 @@ async fn test_public_clean_url_cannot_execute_a_protected_sql_file() { assert_ne!(public.status(), StatusCode::SEE_OTHER); } +#[actix_web::test] +async fn test_top_level_builtin_assets_are_accessible_without_login_when_protected() { + let protected_paths = ["/sqlpage."]; + let (app, _provider) = setup_oidc_test_with_paths(|_| {}, &protected_paths, &[]).await; + let mut cookies: Vec> = Vec::new(); + + let homepage = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies); + assert_ne!(homepage.status(), StatusCode::SEE_OTHER); + let homepage = String::from_utf8(test::read_body(homepage).await.to_vec()).unwrap(); + let script_src = homepage + .split_once("src=\"/sqlpage.") + .and_then(|(_, rest)| rest.split_once('"')) + .map(|(src, _)| format!("/sqlpage.{src}")) + .expect("homepage should reference the top-level SQLPage JavaScript bundle"); + + let asset = request_with_cookies!(app, test::TestRequest::get().uri(&script_src), cookies); + assert_eq!(asset.status(), StatusCode::OK); + assert_eq!( + asset.headers().get(header::CONTENT_TYPE).unwrap(), + "application/javascript;charset=UTF-8" + ); +} + #[actix_web::test] async fn test_oidc_cached_authorization_redirect_cannot_replay_consumed_state() { let (app, provider) = setup_oidc_test(|_| {}).await;