From ee1dc2e20d2b16879cc40d9c79c73a0f5569133f Mon Sep 17 00:00:00 2001 From: Ophir LOJKINE Date: Fri, 12 Jun 2026 15:01:24 +0200 Subject: [PATCH] Centralize file access validation Closes #1325 --- src/file_cache.rs | 31 ++--- src/filesystem.rs | 122 ++++++++++-------- src/templates.rs | 3 +- .../database/sqlpage_functions/functions.rs | 8 +- src/webserver/http.rs | 11 +- src/webserver/routing.rs | 32 ++--- 6 files changed, 111 insertions(+), 96 deletions(-) diff --git a/src/file_cache.rs b/src/file_cache.rs index d07b98c6..7287e413 100644 --- a/src/file_cache.rs +++ b/src/file_cache.rs @@ -1,4 +1,5 @@ use crate::AppState; +use crate::filesystem::FileAccess; use crate::webserver::ErrorWithStatus; use crate::webserver::routing::FileStore; use actix_web::http::StatusCode; @@ -71,7 +72,8 @@ pub struct FileCache { } impl FileStore for FileCache { - async fn contains(&self, path: &Path) -> anyhow::Result { + async fn contains(&self, access: FileAccess<'_>) -> anyhow::Result { + let path = access.path(); Ok(self.cache.read().await.contains_key(path) || self.static_files.contains_key(path)) } } @@ -97,12 +99,6 @@ impl FileCache { self.static_files.insert(path, Cached::new(contents)); } - /// Gets a file from the cache, or loads it from the file system if it's not there - /// This is a privileged operation; it should not be used for user-provided paths - pub async fn get(&self, app_state: &AppState, path: &Path) -> anyhow::Result> { - self.get_with_privilege(app_state, path, true).await - } - pub fn get_static(&self, path: &Path) -> anyhow::Result> { self.static_files .get(path) @@ -110,22 +106,15 @@ impl FileCache { .ok_or_else(|| anyhow::anyhow!("File {} not found in static files", path.display())) } - /// Gets a file from the cache, or loads it from the file system if it's not there - /// The privileged parameter is used to determine whether the access should be denied - /// if the file is in the sqlpage/ config directory - pub async fn get_with_privilege( + /// Gets a file from the cache, or loads it from the file system if it's not there. + pub async fn get( &self, app_state: &AppState, - path: &Path, - privileged: bool, + access: FileAccess<'_>, ) -> anyhow::Result> { + let path = access.path(); + log::trace!("Attempting to get from cache {}", path.display()); - // Enforce the untrusted path guard before consulting the cache. A fresh cache - // entry (possibly populated by a privileged `run_sql` include) must never let an - // unprivileged request reach a reserved/private/traversal/absolute path. - if !privileged { - crate::filesystem::validate_unprivileged_path(path)?; - } if let Some(cached) = self.cache.read().await.get(path) { if !cached.needs_check(app_state.config.cache_stale_duration_ms()) { log::trace!( @@ -136,7 +125,7 @@ impl FileCache { } match app_state .file_system - .modified_since(app_state, path, cached.last_check_time(), privileged) + .modified_since(app_state, access, cached.last_check_time()) .await { Ok(false) => { @@ -159,7 +148,7 @@ impl FileCache { log::trace!("Loading and parsing {}", path.display()); let file_contents = app_state .file_system - .read_to_string(app_state, path, privileged) + .read_to_string(app_state, access) .await; let parsed = match file_contents { diff --git a/src/filesystem.rs b/src/filesystem.rs index e081d2fc..ce508b09 100644 --- a/src/filesystem.rs +++ b/src/filesystem.rs @@ -11,6 +11,37 @@ use std::fmt::Write; use std::io::ErrorKind; use std::path::{Component, Path, PathBuf}; +#[derive(Clone, Copy)] +#[must_use] +pub struct FileAccess<'a> { + path: &'a Path, + privileged: bool, +} + +impl<'a> FileAccess<'a> { + /// Creates access for an untrusted, HTTP-facing path after validating it. + pub fn unprivileged(path: &'a Path) -> anyhow::Result { + validate_unprivileged_path(path)?; + Ok(Self { + path, + privileged: false, + }) + } + + /// Creates access for a trusted internal path. + pub const fn privileged(path: &'a Path) -> Self { + Self { + path, + privileged: true, + } + } + + #[must_use] + pub const fn path(&self) -> &'a Path { + self.path + } +} + pub(crate) struct FileSystem { local_root: PathBuf, db_fs_queries: Option, @@ -39,11 +70,11 @@ impl FileSystem { pub async fn modified_since( &self, app_state: &AppState, - path: &Path, + access: FileAccess<'_>, since: DateTime, - priviledged: bool, ) -> anyhow::Result { - let local_path = self.safe_local_path(app_state, path, priviledged)?; + let path = access.path(); + let local_path = self.safe_local_path(app_state, access); let local_result = file_modified_since_local(&local_path, since).await; log::trace!( "Local file {} modified since {since:?} ? {local_result:?}", @@ -70,10 +101,10 @@ impl FileSystem { pub async fn read_to_string( &self, app_state: &AppState, - path: &Path, - priviledged: bool, + access: FileAccess<'_>, ) -> anyhow::Result { - let bytes = self.read_file(app_state, path, priviledged).await?; + let path = access.path(); + let bytes = self.read_file(app_state, access).await?; String::from_utf8(bytes).map_err(|utf8_err| { let invalid_idx = utf8_err.utf8_error().valid_up_to(); let bytes = utf8_err.into_bytes(); @@ -95,16 +126,13 @@ impl FileSystem { }) } - /** - * Priviledged files are the ones that are in sqlpage's config directory. - */ pub async fn read_file( &self, app_state: &AppState, - path: &Path, - priviledged: bool, + access: FileAccess<'_>, ) -> anyhow::Result> { - let local_path = self.safe_local_path(app_state, path, priviledged)?; + let path = access.path(); + let local_path = self.safe_local_path(app_state, access); log::debug!( "Reading file {} from {}", path.display(), @@ -130,13 +158,9 @@ impl FileSystem { } } - fn safe_local_path( - &self, - app_state: &AppState, - path: &Path, - priviledged: bool, - ) -> anyhow::Result { - if priviledged { + fn safe_local_path(&self, app_state: &AppState, access: FileAccess<'_>) -> PathBuf { + let path = access.path(); + if access.privileged { // Templates requests are always made to the static TEMPLATES_DIR, because this is where they are stored in the database // but when serving them from the filesystem, we need to serve them from the `SQLPAGE_CONFIGURATION_DIRECTORY/templates` directory if let Ok(template_path) = path.strip_prefix(TEMPLATES_DIR) { @@ -150,32 +174,29 @@ impl FileSystem { path.display(), normalized.display() ); - return Ok(normalized); + return normalized; } - } else { - validate_unprivileged_path(path)?; } - Ok(self.local_root.join(path)) + self.local_root.join(path) } pub(crate) async fn file_exists( &self, app_state: &AppState, - path: &Path, + access: FileAccess<'_>, ) -> anyhow::Result { - let local_exists = match self.safe_local_path(app_state, path, false) { - Ok(safe_path) => match tokio::fs::try_exists(safe_path).await { - Ok(exists) => exists, - Err(e) if is_path_missing_error(&e) => false, - Err(e) => { - let status = io_error_status(&e) - .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR); - return Err(e).with_status(status).with_context(|| { - format!("Unable to check if {} exists locally", path.display()) - }); - } - }, - Err(e) => return Err(e), + let path = access.path(); + let safe_path = self.safe_local_path(app_state, access); + let local_exists = match tokio::fs::try_exists(safe_path).await { + Ok(exists) => exists, + Err(e) if is_path_missing_error(&e) => false, + Err(e) => { + let status = io_error_status(&e) + .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR); + return Err(e).with_status(status).with_context(|| { + format!("Unable to check if {} exists locally", path.display()) + }); + } }; // If not in local fs and we have db_fs, check database @@ -192,16 +213,9 @@ impl FileSystem { } } -/// Rejects paths that an untrusted (unprivileged) HTTP request must never reach: -/// the reserved `sqlpage/` prefix, dotfiles, parent-directory traversal and -/// absolute/root paths. -/// -/// This guard must be enforced on every unprivileged resolution path, including -/// before trusting a fresh cache hit. A trusted page may legitimately load such a -/// file with privilege via `sqlpage.run_sql(...)`, which populates the shared file -/// cache; without this check a later direct HTTP request could be served the cached -/// private file instead of being forbidden. -pub fn validate_unprivileged_path(path: &Path) -> anyhow::Result<()> { +/// Rejects paths that an untrusted HTTP request must never reach: the reserved +/// `sqlpage/` prefix, dotfiles, parent-directory traversal and absolute/root paths. +fn validate_unprivileged_path(path: &Path) -> anyhow::Result<()> { for (i, component) in path.components().enumerate() { if let Component::Normal(c) = component { if i == 0 && c.eq_ignore_ascii_case("sqlpage") { @@ -449,7 +463,10 @@ async fn test_sql_file_read_utf8() -> anyhow::Result<()> { let fs = FileSystem::init("/", db).await; let actual = fs - .read_to_string(&state, "unit test file.txt".as_ref(), false) + .read_to_string( + &state, + FileAccess::unprivileged("unit test file.txt".as_ref())?, + ) .await?; assert_eq!(actual, "Héllö world! 😀"); @@ -457,7 +474,11 @@ async fn test_sql_file_read_utf8() -> anyhow::Result<()> { let one_hour_future = Utc::now() + chrono::Duration::hours(1); let was_modified = fs - .modified_since(&state, "unit test file.txt".as_ref(), one_hour_ago, false) + .modified_since( + &state, + FileAccess::unprivileged("unit test file.txt".as_ref())?, + one_hour_ago, + ) .await?; assert!(was_modified, "File should be modified since one hour ago"); @@ -465,9 +486,8 @@ async fn test_sql_file_read_utf8() -> anyhow::Result<()> { let was_modified = fs .modified_since( &state, - "unit test file.txt".as_ref(), + FileAccess::unprivileged("unit test file.txt".as_ref())?, one_hour_future, - false, ) .await?; assert!( diff --git a/src/templates.rs b/src/templates.rs index c27e70e4..af001f75 100644 --- a/src/templates.rs +++ b/src/templates.rs @@ -1,5 +1,6 @@ use crate::app_config::AppConfig; use crate::file_cache::AsyncFromStrWithState; +use crate::filesystem::FileAccess; use crate::template_helpers::register_all_helpers; use crate::{AppState, FileCache, TEMPLATES_DIR}; use async_trait::async_trait; @@ -127,7 +128,7 @@ impl AllTemplates { use anyhow::Context; let path = Self::template_path(name); self.split_templates - .get(app_state, &path) + .get(app_state, FileAccess::privileged(&path)) .await .with_context(|| format!("Unable to get the component '{name}'")) } diff --git a/src/webserver/database/sqlpage_functions/functions.rs b/src/webserver/database/sqlpage_functions/functions.rs index 92bfdfe7..d931bdb7 100644 --- a/src/webserver/database/sqlpage_functions/functions.rs +++ b/src/webserver/database/sqlpage_functions/functions.rs @@ -1,4 +1,5 @@ use super::{ExecutionContext, RequestInfo}; +use crate::filesystem::FileAccess; use crate::webserver::{ ErrorWithStatus, database::{ @@ -606,7 +607,7 @@ async fn read_file_bytes(request: &RequestInfo, path_str: &str) -> Result( let app_state = &request.app_state; let sql_file = app_state .sql_file_cache - .get_with_privilege( + .get( app_state, - std::path::Path::new(sql_file_path.as_ref()), - true, + FileAccess::privileged(std::path::Path::new(sql_file_path.as_ref())), ) .instrument(run_sql_span.clone()) .await diff --git a/src/webserver/http.rs b/src/webserver/http.rs index 242d9613..978b9011 100644 --- a/src/webserver/http.rs +++ b/src/webserver/http.rs @@ -27,6 +27,7 @@ use super::https::make_auto_rustls_config; use super::oidc::OidcMiddleware; use super::response_writer::ResponseWriter; use super::static_content; +use crate::filesystem::FileAccess; use crate::webserver::routing::RoutingAction::{ CustomNotFound, Execute, NotFound, Redirect, Serve, }; @@ -417,6 +418,8 @@ async fn process_sql_request( let app_state: &web::Data = req.app_data().expect("app_state"); let server_timing = ServerTiming::for_env(app_state.config.environment); + let access = + FileAccess::unprivileged(&sql_path).map_err(|e| anyhow_err_to_actix(e, app_state))?; let sql_file = { let span = tracing::info_span!( "sqlpage.file.load", @@ -424,7 +427,7 @@ async fn process_sql_request( ); app_state .sql_file_cache - .get_with_privilege(app_state, &sql_path, false) + .get(app_state, access) .instrument(span) .await .with_context(|| format!("Unable to read SQL file \"{}\"", sql_path.display())) @@ -441,11 +444,13 @@ async fn serve_file( if_modified_since: Option, ) -> actix_web::Result { let path = strip_site_prefix(path, state); + let access = + FileAccess::unprivileged(path.as_ref()).map_err(|e| anyhow_err_to_actix(e, state))?; if let Some(IfModifiedSince(date)) = if_modified_since { let since = DateTime::::from(SystemTime::from(date)); let modified = state .file_system - .modified_since(state, path.as_ref(), since, false) + .modified_since(state, access, since) .await .with_context(|| format!("Unable to get modification time of file {path:?}")) .map_err(|e| anyhow_err_to_actix(e, state))?; @@ -455,7 +460,7 @@ async fn serve_file( } state .file_system - .read_file(state, path.as_ref(), false) + .read_file(state, access) .await .with_context(|| format!("Unable to read file {path:?}")) .map_err(|e| anyhow_err_to_actix(e, state)) diff --git a/src/webserver/routing.rs b/src/webserver/routing.rs index 82efdc05..0bc92bf6 100644 --- a/src/webserver/routing.rs +++ b/src/webserver/routing.rs @@ -76,7 +76,7 @@ //! - Else: Default 404 //! ``` -use crate::filesystem::FileSystem; +use crate::filesystem::{FileAccess, FileSystem}; use crate::webserver::database::ParsedSqlFile; use crate::{AppState, file_cache::FileCache}; use RoutingAction::{CustomNotFound, Execute, NotFound, Redirect, Serve}; @@ -101,7 +101,7 @@ pub enum RoutingAction { #[expect(async_fn_in_trait)] pub trait FileStore { - async fn contains(&self, path: &Path) -> anyhow::Result; + async fn contains(&self, access: FileAccess<'_>) -> anyhow::Result; } pub trait RoutingConfig { @@ -129,15 +129,11 @@ impl<'a> AppFileStore<'a> { } impl FileStore for AppFileStore<'_> { - async fn contains(&self, path: &Path) -> anyhow::Result { - // HTTP routing is always an untrusted, unprivileged operation. Enforce the path - // guard before consulting the cache: a fresh cache entry populated by a - // privileged `run_sql` include must not make a reserved/private path routable. - crate::filesystem::validate_unprivileged_path(path)?; - if self.cache.contains(path).await? { + async fn contains(&self, access: FileAccess<'_>) -> anyhow::Result { + if self.cache.contains(access).await? { Ok(true) } else { - self.filesystem.file_exists(self.app_state, path).await + self.filesystem.file_exists(self.app_state, access).await } } } @@ -206,7 +202,11 @@ where match find_file_or_not_found(&path_with_ext, SQL_EXTENSION, store).await? { Execute(x) => Ok(Execute(x)), other_action => { - if store.contains(&path.join(INDEX)).await? { + let index_path = path.join(INDEX); + if store + .contains(FileAccess::unprivileged(&index_path)?) + .await? + { Ok(Redirect(append_to_path(path_and_query, FORWARD_SLASH))) } else { Ok(other_action) @@ -238,7 +238,7 @@ async fn find_file( where T: FileStore, { - if store.contains(path).await? { + if store.contains(FileAccess::unprivileged(path)?).await? { Ok(Some(if extension == SQL_EXTENSION { Execute(path.to_path_buf()) } else { @@ -256,7 +256,7 @@ where let mut parent = path.parent(); while let Some(p) = parent { let target = p.join(NOT_FOUND); - if store.contains(&target).await? { + if store.contains(FileAccess::unprivileged(&target)?).await? { return Ok(CustomNotFound(target)); } parent = p.parent(); @@ -274,11 +274,11 @@ 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::{FileStore, RoutingAction, RoutingConfig, calculate_route}; + use super::{FileAccess, FileStore, RoutingAction, RoutingConfig, calculate_route}; use StoreConfig::{Custom, Default, Empty, File}; use awc::http::uri::PathAndQuery; use std::default::Default as StdDefault; - use std::path::{Path, PathBuf}; + use std::path::PathBuf; use std::str::FromStr; mod execute { @@ -649,8 +649,8 @@ mod tests { } impl FileStore for Store { - async fn contains(&self, path: &Path) -> anyhow::Result { - Ok(self.contains(path.to_string_lossy().to_string().as_str())) + async fn contains(&self, access: FileAccess<'_>) -> anyhow::Result { + Ok(self.contains(access.path().to_string_lossy().to_string().as_str())) } }