Merge branch 'main' into ophir.lojkine/fix-oidc-logout-session-binding

Author: lovasoa

CHANGELOG.md

@@ -3,6 +3,8 @@
 ## Unreleased
 
 - **Security: OIDC logout links are now bound to the session that requested them.** Previously, a logout URL produced by `sqlpage.oidc_logout_url` only signed the redirect target and a timestamp, so any visitor who opened a still-valid logout link had their SQLPage auth cookies cleared. This allowed a forced logout via CSRF (a logout link made for one user could log out another, or be replayed). It did NOT allow account takeover or access to anyone's data. Logout links are now tied to the current `sqlpage_auth` cookie, so following one only logs out the browser it was generated for. You are affected only if you use built-in OIDC authentication together with `sqlpage.oidc_logout_url`. No action is needed: the normal "click your own logout link" flow keeps working, and old links simply stop being honored for other sessions.
+- **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.
 
 ## v0.44.0
 

SECURITY.md

@@ -129,12 +129,24 @@ SQLPage vulnerabilities:
 - An operator intentionally changes configuration to expose files, trust a
   different database, make an OIDC path public, weaken CSP, enable dangerous
   Markdown options, load SQLite extensions, or enable `allow_exec`.
+- A symlink placed under `web_root` exposes its target. SQLPage follows
+  symlinks during static file serving, so operators must not create symlinks
+  under `web_root` that point to reserved or private files, such as the
+  `sqlpage/` configuration directory or dotfiles, or to files outside
+  `web_root`, since those targets would then be publicly reachable over HTTP.
 - An attacker can modify SQL files, templates, configuration, environment
   variables, migrations, database code, or `sqlpage_files`.
 - The configured database role has broader permissions than the application
   needs.
 - A SQLPage application is publicly reachable because no authentication was
   configured.
+- An attacker can plant or overwrite cookies for the SQLPage origin (for
+  example through a compromised subdomain, a sibling application on a shared
+  parent domain, or a man-in-the-middle on plain HTTP). Attacks that depend on
+  injecting attacker-chosen cookies into the victim's browser, such as OIDC
+  login CSRF or session fixation via a forged login-flow-state cookie, are out
+  of scope. SQLPage assumes its origin's cookie jar is writable only by the
+  user agent, not by attackers.
 - Trusted SQL asks SQLPage or the database to perform expensive work.
 
 These may still be serious and should be fixed in the affected application,

configuration.md

@@ -20,7 +20,7 @@ Here are the available configuration options and their default values:
 | `database_connection_retries`                 | 6                                                           | Database connection attempts before giving up. Retries will happen every 5 seconds.                                                                                                                                                                    |
 | `database_connection_acquire_timeout_seconds` | 10                                                          | How long to wait when acquiring a database connection from the pool before giving up and returning an error.                                                                                                                                           |
 | `sqlite_extensions`                           |                                                             | An array of SQLite extensions to load, such as `mod_spatialite`                                                                                                                                                                                        |
-| `web_root`                                    | `.`                                                         | The root directory of the web server, where the `index.sql` file is located.                                                                                                                                                                           |
+| `web_root`                                    | `.`                                                         | The root directory of the web server, where the `index.sql` file is located. Static file serving follows symlinks, so do not place symlinks under `web_root` that point to private paths (such as the `sqlpage/` config directory) or to files outside `web_root`, as their targets would become publicly reachable (see [`SECURITY.md`](./SECURITY.md)).                                                                                                                                                                          |
 | `site_prefix`                                 | `/`                                                         | Base path of the site. If you want to host SQLPage at `https://example.com/sqlpage/`, set this to `/sqlpage/`. When using a reverse proxy, this allows hosting SQLPage together with other applications on the same subdomain. |
 | `configuration_directory`                     | `./sqlpage/`                                                | The directory where the `sqlpage.json` file is located. This is used to find the path to [`templates/`](https://sql-page.com/custom_components.sql), [`migrations/`](https://sql-page.com/your-first-sql-website/migrations.sql), and `on_connect.sql`. Obviously, this configuration parameter can be set only through environment variables, not through the `sqlpage.json` file itself in order to find the `sqlpage.json` file. Be careful not to use a path that is accessible from the public WEB_ROOT |
 | `allow_exec`                                  | false                                                       | Allow usage of the `sqlpage.exec` function. Do this only if all users with write access to sqlpage query files and to the optional `sqlpage_files` table on the database are trusted.                                                                  |

examples/official-site/sqlpage/migrations/39_persist_uploaded_file.sql

@@ -52,7 +52,9 @@ VALUES (
         'persist_uploaded_file',
         2,
         'destination_folder',
-        'Optional. Path to the folder where the file will be saved, relative to the web root (the root folder of your website files). By default, the file will be saved in the `uploads` folder.',
+        'Optional. Path to the folder where the file will be saved, relative to the web root (the root folder of your website files). By default, the file will be saved in the `uploads` folder.
+
+**Security note**: this value must be a folder name you choose yourself in your SQL code (a trusted constant). Never build it from untrusted input such as a form field, a query parameter, a request header, or anything else the visitor controls. The folder is joined directly to the web root, so a value containing `..` or an absolute path would cause the uploaded file to be written *outside* the web root, anywhere the SQLPage process can write. Keeping `destination_folder` a fixed value chosen by the application author avoids this.',
         'TEXT'
     ),
     (

sqlpage/private_cache_bypass_test.sql

@@ -0,0 +1 @@
+select 'text' as component, 'private cache bypass secret' as contents;

src/file_cache.rs

@@ -120,6 +120,12 @@ impl<T: AsyncFromStrWithState> FileCache<T> {
         privileged: bool,
     ) -> anyhow::Result<Arc<T>> {
         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!(

src/filesystem.rs

@@ -153,34 +153,7 @@ impl FileSystem {
                 return Ok(normalized);
             }
         } else {
-            for (i, component) in path.components().enumerate() {
-                if let Component::Normal(c) = component {
-                    if i == 0 && c.eq_ignore_ascii_case("sqlpage") {
-                        return Err(ErrorWithStatus {
-                            status: actix_web::http::StatusCode::FORBIDDEN,
-                        })
-                        .with_context(|| {
-                            "The /sqlpage/ path prefix is reserved for internal use. It is not public."
-                        });
-                    }
-                    if c.as_encoded_bytes().starts_with(b".") {
-                        return Err(ErrorWithStatus {
-                            status: actix_web::http::StatusCode::FORBIDDEN,
-                        })
-                        .with_context(|| "Directory traversal is not allowed");
-                    }
-                } else {
-                    return Err(ErrorWithStatus {
-                        status: actix_web::http::StatusCode::FORBIDDEN,
-                    })
-                    .with_context(|| {
-                        format!(
-                            "Unsupported path: {}. Path component '{component:?}' is not allowed.",
-                            path.display()
-                        )
-                    });
-                }
-            }
+            validate_unprivileged_path(path)?;
         }
         Ok(self.local_root.join(path))
     }
@@ -219,6 +192,47 @@ 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<()> {
+    for (i, component) in path.components().enumerate() {
+        if let Component::Normal(c) = component {
+            if i == 0 && c.eq_ignore_ascii_case("sqlpage") {
+                return Err(ErrorWithStatus {
+                    status: actix_web::http::StatusCode::FORBIDDEN,
+                })
+                .with_context(
+                    || "The /sqlpage/ path prefix is reserved for internal use. It is not public.",
+                );
+            }
+            if c.as_encoded_bytes().starts_with(b".") {
+                return Err(ErrorWithStatus {
+                    status: actix_web::http::StatusCode::FORBIDDEN,
+                })
+                .with_context(|| "Directory traversal is not allowed");
+            }
+        } else {
+            return Err(ErrorWithStatus {
+                status: actix_web::http::StatusCode::FORBIDDEN,
+            })
+            .with_context(|| {
+                format!(
+                    "Unsupported path: {}. Path component '{component:?}' is not allowed.",
+                    path.display()
+                )
+            });
+        }
+    }
+    Ok(())
+}
+
 fn is_path_missing_error(error: &std::io::Error) -> bool {
     matches!(error.kind(), ErrorKind::NotFound | ErrorKind::NotADirectory)
 }

src/render.rs

@@ -49,7 +49,9 @@ use crate::webserver::response_writer::{AsyncResponseWriter, ResponseWriter};
 use actix_web::body::MessageBody;
 use actix_web::cookie::time::OffsetDateTime;
 use actix_web::cookie::time::format_description::well_known::Rfc3339;
-use actix_web::http::header::TryIntoHeaderPair;
+use actix_web::http::header::{
+    ContentDisposition, DispositionParam, DispositionType, TryIntoHeaderPair,
+};
 use actix_web::http::{StatusCode, header};
 use actix_web::{HttpResponse, HttpResponseBuilder};
 use anyhow::{Context as AnyhowContext, bail, format_err};
@@ -301,10 +303,7 @@ impl HeaderContext {
             get_object_str(options, "filename").or_else(|| get_object_str(options, "title"))
         {
             let extension = if filename.contains('.') { "" } else { ".csv" };
-            self.insert_header((
-                header::CONTENT_DISPOSITION,
-                format!("attachment; filename={filename}{extension}"),
-            ))?;
+            self.insert_header(attachment_with_filename(&format!("{filename}{extension}")))?;
         }
         let csv_renderer = CsvBodyRenderer::new(self.writer, options).await?;
         let renderer = AnyRenderBodyContext::Csv(csv_renderer);
@@ -345,10 +344,7 @@ impl HeaderContext {
 
     fn download(mut self, options: &JsonValue) -> anyhow::Result<PageContext> {
         if let Some(filename) = get_object_str(options, "filename") {
-            self.insert_header((
-                header::CONTENT_DISPOSITION,
-                format!("attachment; filename=\"{filename}\""),
-            ))?;
+            self.insert_header(attachment_with_filename(filename))?;
         }
         let data_url = get_object_str(options, "data_url")
             .with_context(|| "The download component requires a 'data_url' property")?;
@@ -442,6 +438,18 @@ async fn verify_password_async(
     .await?
 }
 
+/// Builds an `attachment` `Content-Disposition` header with the given filename,
+/// using actix-web's structured [`ContentDisposition`] type so the filename is
+/// properly quoted and escaped. This prevents a user-supplied filename
+/// containing `;`, `"`, or `=` from injecting additional header parameters
+/// (e.g. a second, agent-preferred `filename*`).
+fn attachment_with_filename(filename: &str) -> ContentDisposition {
+    ContentDisposition {
+        disposition: DispositionType::Attachment,
+        parameters: vec![DispositionParam::Filename(filename.to_owned())],
+    }
+}
+
 fn get_object_str<'a>(json: &'a JsonValue, key: &str) -> Option<&'a str> {
     json.as_object()
         .and_then(|obj| obj.get(key))

src/webserver/database/sqlpage_functions/functions.rs

@@ -512,7 +512,12 @@ async fn persist_uploaded_file<'a>(
         let exts = allowed_extensions.collect::<Vec<_>>().join(", ");
         anyhow::bail!("file extension {extension} is not allowed. Allowed extensions: {exts}");
     }
-    // resolve the folder path relative to the web root
+    // Resolve the folder path relative to the web root.
+    // `folder` is trusted application input: it is expected to be a constant chosen by the
+    // app author in their SQL code, never attacker-controlled request data. It is joined
+    // directly to the web root, so a `folder` containing `..` or an absolute path would let
+    // the caller write the uploaded file outside the web root. Callers must not pass
+    // untrusted input (form fields, query parameters, headers, ...) as the folder.
     let web_root = &request.app_state.config.web_root;
     let target_folder = web_root.join(&*folder);
     // create the folder if it doesn't exist

src/webserver/routing.rs

@@ -130,6 +130,10 @@ impl<'a> AppFileStore<'a> {
 
 impl FileStore for AppFileStore<'_> {
     async fn contains(&self, path: &Path) -> anyhow::Result<bool> {
+        // 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? {
             Ok(true)
         } else {