From 35e7ca55b29501b54822ac330df797cb14718ec9 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:25:05 +0000 Subject: [PATCH 01/13] fix(storage-sqlite): create the database file with mode 0600 before SQLite opens it `extenddb init --backend sqlite` left the database file and its -wal and -shm files at mode 0644. SQLite's unix VFS creates a database file with mode 0644 less the umask and gives each new sidecar the mode of the database file, and the bootstrapper pool that init and migrate use never changed either mode. The engine set the modes after connecting, on the configured string with a suffix appended, and ignored failures. Every pool opener on a file database (the bootstrapper, the engine and the catalog pool) now creates a missing database file with mode 0600 before it connects, when the connection would create the file, and sets the file and any -wal, -shm and -journal file present to 0600 before and after it connects. The sidecar names come from the resolved path, because SQLite follows symlinks before naming them. A mode that cannot be set is an error, as it is for the lock file. Other platforms are unchanged. --- Cargo.lock | 1 + crates/storage-sqlite/Cargo.toml | 1 + crates/storage-sqlite/src/bootstrapper.rs | 12 +- crates/storage-sqlite/src/file_mode.rs | 160 ++++++++++++++++++++++ crates/storage-sqlite/src/lib.rs | 11 +- crates/storage-sqlite/src/serve_lock.rs | 2 +- crates/storage-sqlite/src/store.rs | 21 +-- 7 files changed, 185 insertions(+), 23 deletions(-) create mode 100644 crates/storage-sqlite/src/file_mode.rs diff --git a/Cargo.lock b/Cargo.lock index c59cdea4e..b8e985f28 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1997,6 +1997,7 @@ dependencies = [ "tokio", "toml", "tracing", + "url", "uuid", "zeroize", ] diff --git a/crates/storage-sqlite/Cargo.toml b/crates/storage-sqlite/Cargo.toml index 09447140d..01bb11de7 100644 --- a/crates/storage-sqlite/Cargo.toml +++ b/crates/storage-sqlite/Cargo.toml @@ -39,6 +39,7 @@ bigdecimal = { workspace = true } crc32fast = { workspace = true } zeroize = { workspace = true } libc = { workspace = true } +url = { workspace = true } [dev-dependencies] # The crate's own tests need `test-hooks`; a dev-dependency on itself turns diff --git a/crates/storage-sqlite/src/bootstrapper.rs b/crates/storage-sqlite/src/bootstrapper.rs index 293a39c8d..a65d0caf5 100644 --- a/crates/storage-sqlite/src/bootstrapper.rs +++ b/crates/storage-sqlite/src/bootstrapper.rs @@ -105,7 +105,8 @@ impl SqliteBootstrapper { /// Open a short-lived single-connection pool with the standard PRAGMAs. /// For a real filesystem path, the parent directory is created first: /// `init --sqlite-path` may point into a directory that does not exist - /// yet, and bootstrapping it is exactly init's job. + /// yet, and bootstrapping it is exactly init's job. The database file is + /// created, and kept, owner read/write only; see `file_mode`. async fn pool(&self) -> OpResult { if self.path != ":memory:" && let Some(parent) = std::path::Path::new(&self.path).parent() @@ -118,7 +119,10 @@ impl SqliteBootstrapper { )) })?; } - SqlitePoolOptions::new() + let open_err = + |e: String| OpError::Internal(format!("open SQLite database '{}': {e}", self.path)); + crate::file_mode::prepare(&self.path).map_err(open_err)?; + let pool = SqlitePoolOptions::new() .max_connections(1) .after_connect(|conn, _| { Box::pin(async move { @@ -132,7 +136,9 @@ impl SqliteBootstrapper { }) .connect(&sqlite_url(&self.path)) .await - .map_err(|e| OpError::Internal(format!("open SQLite database '{}': {e}", self.path))) + .map_err(|e| open_err(e.to_string()))?; + crate::file_mode::restrict(&self.path).map_err(open_err)?; + Ok(pool) } } diff --git a/crates/storage-sqlite/src/file_mode.rs b/crates/storage-sqlite/src/file_mode.rs new file mode 100644 index 000000000..63a4600b5 --- /dev/null +++ b/crates/storage-sqlite/src/file_mode.rs @@ -0,0 +1,160 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Owner-only modes for a SQLite database file and the files SQLite keeps +//! beside it. +//! +//! The database stores the AES key next to the secrets it encrypts +//! (access-key secrets, password hashes), so neither the file nor its `-wal`, +//! `-shm` and `-journal` files may be readable by other local users. SQLite's +//! unix VFS creates a database file with mode 0644 less the umask, and creates +//! each sidecar with the mode of the database file. So every pool opener on a +//! file database calls [`prepare`] before it connects, which creates the +//! database file with mode 0600 (or sets an existing one to 0600) before the +//! connection can create a sidecar from it, and [`restrict`] after it +//! connects, which sets the file and every sidecar present to 0600 whoever +//! created them. +//! +//! As with the lock file in `serve_lock`, a mode that cannot be set is an +//! error. The directory's mode is left alone. On platforms other than Unix +//! both functions do nothing. + +#[cfg(unix)] +use std::path::{Path, PathBuf}; +#[cfg(unix)] +use std::str::FromStr; + +#[cfg(unix)] +use crate::serve_lock::{database_file, resolve_dangling_symlinks}; +#[cfg(unix)] +use crate::sqlite_util::sqlite_url; + +/// Before a pool connects to `location`: create the database file with mode +/// 0600 if the connection would create it, then set the file and any sidecar +/// left by an earlier run to 0600, so the sidecars this connection creates +/// take mode 0600 from the database file. +/// +/// A location that names no file on this machine (in-memory, unparseable, a +/// remote `file:` authority) is left to the connection, which reports it in +/// its own terms. +/// +/// # Errors +/// +/// A file's mode cannot be set. +#[cfg(unix)] +pub(crate) fn prepare(location: &str) -> Result<(), String> { + let Ok(Some(path)) = database_file(location) else { + return Ok(()); + }; + if open_creates_file(location) { + create_owner_only(&resolve_dangling_symlinks(&path))?; + } + restrict_files(&path) +} + +#[cfg(not(unix))] +pub(crate) fn prepare(_location: &str) -> Result<(), String> { + Ok(()) +} + +/// After a pool connects to `location`: set the database file and every +/// `-wal`, `-shm` and `-journal` file present to mode 0600. +/// +/// # Errors +/// +/// A file's mode cannot be set. +#[cfg(unix)] +pub(crate) fn restrict(location: &str) -> Result<(), String> { + match database_file(location) { + Ok(Some(path)) => restrict_files(&path), + _ => Ok(()), + } +} + +#[cfg(not(unix))] +pub(crate) fn restrict(_location: &str) -> Result<(), String> { + Ok(()) +} + +/// Whether connecting to `location` creates the database file when it is +/// missing. sqlx reads the pairs after the first `?` as form-urlencoded and +/// passes `SQLITE_OPEN_CREATE` for `mode=rwc` unless `mode=ro` is also given; +/// a plain path always gets `mode=rwc` from `sqlite_url`. Two forms are left +/// to SQLite because they can keep it from creating a file on disk: a `file:` +/// URI with its own query (its `mode` can narrow the open) and a VFS outside +/// the unix family. +#[cfg(unix)] +fn open_creates_file(location: &str) -> bool { + let url = sqlite_url(location); + let Some((_, query)) = url.split_once('?') else { + return false; + }; + let (mut rwc, mut ro, mut unix_vfs) = (false, false, true); + for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { + match &*key { + "mode" => { + rwc |= value == "rwc"; + ro |= value == "ro"; + } + "vfs" => unix_vfs &= value.starts_with("unix"), + _ => {} + } + } + if !rwc || ro || !unix_vfs { + return false; + } + sqlx::sqlite::SqliteConnectOptions::from_str(&url).is_ok_and(|options| { + let name = options.get_filename().to_string_lossy(); + !(name.starts_with("file:") && name.contains('?')) + }) +} + +/// Create `path` with mode 0600 if nothing is there yet. +#[cfg(unix)] +fn create_owner_only(path: &Path) -> Result<(), String> { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + match std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) + { + // The umask applies to `mode` and can remove owner bits as well. + Ok(file) => file + .set_permissions(std::fs::Permissions::from_mode(0o600)) + .map_err(|e| { + format!( + "cannot set the mode of SQLite database {}: {e}", + path.display() + ) + }), + // The file exists, or cannot be created here. SQLite's own open + // decides what that means and reports it in its usual terms. + Err(_) => Ok(()), + } +} + +/// Set the database file and each sidecar that exists to mode 0600. +#[cfg(unix)] +fn restrict_files(path: &Path) -> Result<(), String> { + use std::os::unix::fs::PermissionsExt; + // SQLite follows symlinks before it names the sidecars, so they sit next + // to the target, not next to the link. + let base = std::fs::canonicalize(path).unwrap_or_else(|_| resolve_dangling_symlinks(path)); + for suffix in ["", "-wal", "-shm", "-journal"] { + let mut name = base.clone().into_os_string(); + name.push(suffix); + let file = PathBuf::from(name); + match std::fs::set_permissions(&file, std::fs::Permissions::from_mode(0o600)) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + return Err(format!( + "cannot set the mode of SQLite file {}: {e}", + file.display() + )); + } + } + } + Ok(()) +} diff --git a/crates/storage-sqlite/src/lib.rs b/crates/storage-sqlite/src/lib.rs index 7e8085ff9..c100e04d6 100644 --- a/crates/storage-sqlite/src/lib.rs +++ b/crates/storage-sqlite/src/lib.rs @@ -30,6 +30,7 @@ mod create_table; mod credential_store; mod data; mod delete_table; +mod file_mode; mod hooks; mod management_store; mod metadata; @@ -106,9 +107,10 @@ use extenddb_storage::settings_store::SettingsStoreError; use crate::sqlite_util::sqlite_url; /// Open a small catalog-only pool for the settings/diagnostics factories, -/// applying the same PRAGMAs as the engine. -async fn catalog_pool(connection_string: &str) -> Result { - sqlx::sqlite::SqlitePoolOptions::new() +/// applying the same PRAGMAs and file modes as the engine. +async fn catalog_pool(connection_string: &str) -> Result { + file_mode::prepare(connection_string)?; + let pool = sqlx::sqlite::SqlitePoolOptions::new() .max_connections(4) .after_connect(|conn, _| { Box::pin(async move { @@ -121,6 +123,9 @@ async fn catalog_pool(connection_string: &str) -> Result PathBuf { +pub(crate) fn resolve_dangling_symlinks(path: &Path) -> PathBuf { let mut current = path.to_path_buf(); // Bounded like the kernel's own symlink-loop limit. for _ in 0..40 { diff --git a/crates/storage-sqlite/src/store.rs b/crates/storage-sqlite/src/store.rs index 075d25b28..d89e51add 100644 --- a/crates/storage-sqlite/src/store.rs +++ b/crates/storage-sqlite/src/store.rs @@ -127,6 +127,10 @@ impl SqliteEngine { if in_memory { opts = opts.idle_timeout(None).max_lifetime(None); } + // The database file stores the AES encryption key alongside the data it + // protects (encrypted access-key secrets, password hashes), so the file + // and its sidecars are kept to owner read/write; see `file_mode`. + crate::file_mode::prepare(path_or_url).map_err(StorageError::Connection)?; let pool = opts .after_connect(|conn, _| { Box::pin(async move { @@ -142,22 +146,7 @@ impl SqliteEngine { .connect(&url) .await .map_err(|e| StorageError::Connection(e.to_string()))?; - - // The database file stores the AES encryption key alongside the data it - // protects (encrypted access-key secrets, password hashes). Restrict the - // file and its WAL/SHM sidecars to owner read/write so other local users - // or processes cannot read it and decrypt secrets. In-memory databases - // have no file to protect. - #[cfg(unix)] - if !in_memory { - use std::os::unix::fs::PermissionsExt; - for suffix in ["", "-wal", "-shm"] { - let f = format!("{path_or_url}{suffix}"); - if std::path::Path::new(&f).exists() { - let _ = std::fs::set_permissions(&f, std::fs::Permissions::from_mode(0o600)); - } - } - } + crate::file_mode::restrict(path_or_url).map_err(StorageError::Connection)?; let initial_index_delay: u64 = sqlx::query_as::<_, (String,)>(INDEX_PROPAGATION_DELAY_QUERY) From 39f70de31031469be0cee38aca8d184e2b7351cd Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:48:17 +0000 Subject: [PATCH 02/13] test(storage-sqlite): check the modes of the database and WAL files after init, serve and a write Unit tests for file_mode: prepare creates a missing database at 0600 and sets an existing database and its sidecars to 0600; a sqlite: URL without mode=rwc, or with mode=ro, creates nothing; a WAL pool opened after prepare gets 0600 sidecars from SQLite; a dangling symlink has its target created. An integration test binary sets a umask of 022 in its own process and runs the bootstrapper sequence init uses, the server components factory, a management write, CreateTable and TagResource, a clean stop and a second start, checking that every file in the deployment directory is 0600 with the WAL sidecars present while a server is up. It also covers a dev-mode serve on a missing path, and serve and migrate over files left at 0644. A control test shows a connection opened outside the crate's openers leaves 0644 files under that umask. --- crates/storage-sqlite/src/file_mode.rs | 157 +++++++++++ crates/storage-sqlite/tests/file_modes.rs | 313 ++++++++++++++++++++++ 2 files changed, 470 insertions(+) create mode 100644 crates/storage-sqlite/tests/file_modes.rs diff --git a/crates/storage-sqlite/src/file_mode.rs b/crates/storage-sqlite/src/file_mode.rs index 63a4600b5..5673eee38 100644 --- a/crates/storage-sqlite/src/file_mode.rs +++ b/crates/storage-sqlite/src/file_mode.rs @@ -158,3 +158,160 @@ fn restrict_files(path: &Path) -> Result<(), String> { } Ok(()) } + +#[cfg(all(test, unix))] +mod tests { + use std::os::unix::fs::PermissionsExt; + use std::path::{Path, PathBuf}; + + use super::{prepare, restrict}; + + fn temp_dir() -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "extenddb-file-mode-{}", + uuid::Uuid::new_v4().simple() + )); + std::fs::create_dir_all(&dir).expect("dir"); + dir + } + + fn mode(path: &Path) -> u32 { + std::fs::metadata(path) + .unwrap_or_else(|e| panic!("{}: {e}", path.display())) + .permissions() + .mode() + & 0o777 + } + + fn set_mode(path: &Path, mode: u32) { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)).expect("chmod"); + } + + fn sidecar(db: &Path, suffix: &str) -> PathBuf { + let mut name = db.as_os_str().to_owned(); + name.push(suffix); + PathBuf::from(name) + } + + /// A pool opened the way the crate's openers open theirs (WAL journal), + /// without `prepare` or `restrict`, holding its connection so the + /// sidecars stay on disk. + async fn raw_wal_pool(path: &Path) -> sqlx::SqlitePool { + use sqlx::Executor; + let pool = sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .after_connect(|conn, _| { + Box::pin(async move { + conn.execute("PRAGMA journal_mode = WAL").await?; + Ok(()) + }) + }) + .connect(&crate::sqlite_util::sqlite_url(&path.to_string_lossy())) + .await + .expect("connect"); + sqlx::query("CREATE TABLE IF NOT EXISTS t (k INTEGER PRIMARY KEY)") + .execute(&pool) + .await + .expect("write"); + pool + } + + #[test] + fn prepare_creates_a_missing_database_with_mode_0600() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + prepare(&db.to_string_lossy()).expect("prepare"); + assert_eq!(mode(&db), 0o600); + assert_eq!(std::fs::metadata(&db).expect("metadata").len(), 0); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn prepare_and_restrict_set_an_existing_database_and_its_sidecars_to_0600() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let files: Vec = ["", "-wal", "-shm", "-journal"] + .iter() + .map(|suffix| sidecar(&db, suffix)) + .collect(); + for file in &files { + std::fs::write(file, b"").expect("create"); + set_mode(file, 0o644); + } + prepare(&db.to_string_lossy()).expect("prepare"); + for file in &files { + assert_eq!(mode(file), 0o600, "{}", file.display()); + } + for file in &files { + set_mode(file, 0o644); + } + restrict(&db.to_string_lossy()).expect("restrict"); + for file in &files { + assert_eq!(mode(file), 0o600, "{}", file.display()); + } + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn prepare_creates_nothing_when_the_connection_would_not() { + let dir = temp_dir(); + let plain = dir.join("plain.sqlite"); + let read_only = dir.join("ro.sqlite"); + // A sqlite: URL is passed to sqlx as given: without mode=rwc sqlx does + // not pass SQLITE_OPEN_CREATE, and mode=ro wins over mode=rwc. + prepare(&format!("sqlite://{}", plain.display())).expect("prepare"); + prepare(&format!("sqlite://{}?mode=ro", read_only.display())).expect("prepare"); + prepare(&format!( + "sqlite://{}?mode=rwc&mode=ro", + read_only.display() + )) + .expect("prepare"); + prepare(":memory:").expect("prepare"); + prepare("sqlite::memory:").expect("prepare"); + restrict(&plain.to_string_lossy()).expect("restrict"); + assert_eq!(std::fs::read_dir(&dir).expect("read_dir").count(), 0); + let _ = std::fs::remove_dir_all(&dir); + } + + /// The rule the fix relies on: SQLite gives a new `-wal` or `-shm` file + /// the mode of the database file, so a database created at 0600 gets + /// 0600 sidecars without any chmod after the fact. + #[tokio::test] + async fn sidecars_created_after_prepare_take_mode_0600() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + prepare(&db.to_string_lossy()).expect("prepare"); + let pool = raw_wal_pool(&db).await; + for suffix in ["", "-wal", "-shm"] { + let file = sidecar(&db, suffix); + assert!(file.exists(), "{} missing", file.display()); + assert_eq!(mode(&file), 0o600, "{}", file.display()); + } + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); + } + + #[tokio::test] + async fn prepare_creates_the_target_of_a_dangling_symlink() { + let dir = temp_dir(); + let target = dir.join("real.sqlite"); + let link = dir.join("link.sqlite"); + std::os::unix::fs::symlink(&target, &link).expect("symlink"); + let location = link.to_string_lossy().into_owned(); + prepare(&location).expect("prepare"); + assert!(target.exists(), "target not created"); + assert_eq!(mode(&target), 0o600); + // SQLite names the sidecars after the target, and they take its mode. + let pool = raw_wal_pool(&link).await; + for suffix in ["-wal", "-shm"] { + let file = sidecar(&target, suffix); + assert!(file.exists(), "{} missing", file.display()); + assert_eq!(mode(&file), 0o600, "{}", file.display()); + } + set_mode(&sidecar(&target, "-wal"), 0o644); + restrict(&location).expect("restrict"); + assert_eq!(mode(&sidecar(&target, "-wal")), 0o600); + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/storage-sqlite/tests/file_modes.rs b/crates/storage-sqlite/tests/file_modes.rs new file mode 100644 index 000000000..41be979ae --- /dev/null +++ b/crates/storage-sqlite/tests/file_modes.rs @@ -0,0 +1,313 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Modes of the SQLite database file and its `-wal` and `-shm` files after +//! `init`, `serve`, a first write, a clean stop and a second `serve`. The +//! database holds the encryption key beside the secrets it protects, so the +//! files must be readable by their owner only, whatever the umask. +//! +//! SQLite creates a database file at 0644 less the umask, and each sidecar +//! with the mode of the database file. These tests run in their own process +//! and set a umask of 022, under which that default comes out 0644. The first +//! test shows that a connection opened outside the crate's openers does leave +//! 0644 files under this umask, so the other tests measure the openers. + +#![cfg(unix)] + +use std::collections::BTreeMap; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; + +use extenddb_storage::bootstrapper::Bootstrapper; +use extenddb_storage::server_components::{ServerComponents, ServerComponentsOptions}; +use extenddb_storage::{MetadataEngine, TableEngine}; +use extenddb_storage_sqlite::{SqliteBootstrapper, SqliteConfig}; + +const ACCOUNT: &str = "000000000000"; + +/// A fresh directory for one test, with the process umask set to 022. +fn deployment_dir() -> PathBuf { + // SAFETY: umask reads and replaces the process file-mode mask and has no + // other effect; every test in this binary sets the same value. + unsafe { libc::umask(0o022) }; + let dir = std::env::temp_dir().join(format!( + "extenddb-file-modes-{}", + uuid::Uuid::new_v4().simple() + )); + std::fs::create_dir_all(&dir).expect("dir"); + dir +} + +fn database_path(dir: &Path) -> String { + dir.join("x.sqlite").to_string_lossy().into_owned() +} + +/// Name and mode of every file directly in `dir`. A file that a closing +/// connection removes between the listing and the stat is skipped. +fn modes(dir: &Path) -> BTreeMap { + std::fs::read_dir(dir) + .expect("read_dir") + .filter_map(|entry| { + let entry = entry.expect("entry"); + let meta = match std::fs::metadata(entry.path()) { + Ok(meta) => meta, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, + Err(e) => panic!("{}: {e}", entry.path().display()), + }; + Some(( + entry.file_name().to_string_lossy().into_owned(), + meta.permissions().mode() & 0o777, + )) + }) + .collect() +} + +/// Every file in `dir` is 0600; `expected` names files that must be there. +fn assert_owner_only(dir: &Path, step: &str, expected: &[&str]) { + let modes = modes(dir); + assert!( + modes.contains_key("x.sqlite"), + "after {step}: no database file in {modes:?}" + ); + for name in expected { + assert!( + modes.contains_key(*name), + "after {step}: {name} missing from {modes:?}" + ); + } + for (name, mode) in &modes { + assert_eq!( + *mode, 0o600, + "after {step}: {name} is {mode:o} in {modes:?}" + ); + } +} + +fn set_every_mode(dir: &Path, mode: u32) { + for name in modes(dir).keys() { + std::fs::set_permissions(dir.join(name), std::fs::Permissions::from_mode(mode)) + .expect("chmod"); + } +} + +/// What `extenddb init --backend sqlite` does through the bootstrapper. +async fn init(path: &str) { + let bootstrapper = SqliteBootstrapper::new(path); + bootstrapper + .ensure_app_user() + .await + .expect("ensure_app_user"); + bootstrapper + .grant_app_role_to_admin() + .await + .expect("grant_app_role_to_admin"); + bootstrapper + .create_catalog_db() + .await + .expect("create_catalog_db"); + bootstrapper.create_data_db().await.expect("create_data_db"); + bootstrapper + .acquire_migration_lock() + .await + .expect("acquire_migration_lock"); + assert!( + !bootstrapper + .is_catalog_initialized() + .await + .expect("is_catalog_initialized") + ); + bootstrapper + .run_catalog_migrations() + .await + .expect("run_catalog_migrations"); + bootstrapper + .record_data_connection() + .await + .expect("record_data_connection"); + bootstrapper + .run_data_migrations() + .await + .expect("run_data_migrations"); + bootstrapper + .bootstrap_encryption_key() + .await + .expect("bootstrap_encryption_key"); + bootstrapper + .bootstrap_default_account() + .await + .expect("bootstrap_default_account"); + bootstrapper + .bootstrap_admin_user(None, None) + .await + .expect("bootstrap_admin_user"); + bootstrapper + .release_migration_lock() + .await + .expect("release_migration_lock"); +} + +/// What `extenddb migrate` opens on a database with nothing pending: the lock, +/// then the bootstrapper's pool for the catalog state and the pending data +/// migrations. +async fn migrate_reads(path: &str) { + let bootstrapper = SqliteBootstrapper::new(path); + bootstrapper + .acquire_migration_lock() + .await + .expect("acquire_migration_lock"); + assert!( + bootstrapper + .is_catalog_initialized() + .await + .expect("is_catalog_initialized") + ); + bootstrapper + .pending_data_migrations() + .await + .expect("pending_data_migrations"); + bootstrapper + .release_migration_lock() + .await + .expect("release_migration_lock"); +} + +/// What `extenddb serve` does to open the database. +async fn serve(path: &str, options: ServerComponentsOptions) -> ServerComponents { + let config = SqliteConfig { + path: path.to_owned(), + pool_size: 2, + }; + (extenddb_storage_sqlite::backend().server_components)(&config, "us-east-1", options) + .await + .expect("server components") +} + +/// A first write: an account through the management store (the `manage` +/// command's opener), then CreateTable and TagResource through the engine. +async fn first_write(path: &str, components: &ServerComponents) { + let management = (extenddb_storage_sqlite::backend().management_store)(path) + .await + .expect("management store"); + management + .create_account(ACCOUNT, "modes") + .await + .expect("create account"); + let input: extenddb_core::types::CreateTableInput = serde_json::from_value(serde_json::json!({ + "TableName": "modes", + "KeySchema": [{"AttributeName": "pk", "KeyType": "HASH"}], + "AttributeDefinitions": [{"AttributeName": "pk", "AttributeType": "S"}], + "BillingMode": "PAY_PER_REQUEST" + })) + .expect("input"); + let engine = &*components.engine; + TableEngine::create_table(engine, ACCOUNT, input) + .await + .expect("create table"); + MetadataEngine::tag_resource( + engine, + &format!("arn:aws:dynamodb:us-east-1:{ACCOUNT}:table/modes"), + &[extenddb_core::types::Tag { + key: "k".to_owned(), + value: "v".to_owned(), + }], + ) + .await + .expect("tag resource"); +} + +/// The control for the other tests: a connection that none of the crate's +/// openers made leaves the database and its sidecars at 0644 under this +/// umask, so a 0600 result below comes from the openers, not the umask. +#[tokio::test] +async fn a_connection_outside_the_openers_leaves_0644_files() { + use sqlx::Executor; + let dir = deployment_dir(); + let pool = sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .after_connect(|conn, _| { + Box::pin(async move { + conn.execute("PRAGMA journal_mode = WAL").await?; + Ok(()) + }) + }) + .connect(&format!("sqlite://{}?mode=rwc", database_path(&dir))) + .await + .expect("connect"); + sqlx::query("CREATE TABLE t (k INTEGER PRIMARY KEY)") + .execute(&pool) + .await + .expect("write"); + let modes = modes(&dir); + for name in ["x.sqlite", "x.sqlite-wal", "x.sqlite-shm"] { + assert_eq!(modes.get(name), Some(&0o644), "{name} in {modes:?}"); + } + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); +} + +/// `init`, `serve`, a first write, a clean stop and a second `serve` leave +/// every file owner-only, with the WAL sidecars present while a server is up. +#[tokio::test] +async fn init_serve_write_stop_and_restart_leave_every_file_owner_only() { + let dir = deployment_dir(); + let path = database_path(&dir); + let sidecars = ["x.sqlite-wal", "x.sqlite-shm"]; + + init(&path).await; + assert_owner_only(&dir, "init", &["x.sqlite.lock"]); + + let components = serve(&path, ServerComponentsOptions::default()).await; + assert_owner_only(&dir, "serve", &sidecars); + + first_write(&path, &components).await; + assert_owner_only(&dir, "the first write", &sidecars); + + drop(components); + assert_owner_only(&dir, "a clean stop", &[]); + + let again = serve(&path, ServerComponentsOptions::default()).await; + assert_owner_only(&dir, "a second serve", &sidecars); + drop(again); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A dev-mode `serve` that bootstraps a database no `init` created makes it +/// owner-only too. +#[tokio::test] +async fn a_bootstrapping_serve_creates_owner_only_files() { + let dir = deployment_dir(); + let path = database_path(&dir); + let mut options = ServerComponentsOptions::default(); + options.bootstrap_if_uninitialized = true; + let components = serve(&path, options).await; + assert_owner_only( + &dir, + "a bootstrapping serve", + &["x.sqlite-wal", "x.sqlite-shm"], + ); + drop(components); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Files left at 0644 by an earlier version are set to 0600 by the next +/// `serve`, and by the next `migrate`, before anything reads them. +#[tokio::test] +async fn serve_and_migrate_restrict_files_left_at_0644() { + let dir = deployment_dir(); + let path = database_path(&dir); + init(&path).await; + + set_every_mode(&dir, 0o644); + let components = serve(&path, ServerComponentsOptions::default()).await; + assert_owner_only( + &dir, + "serve over 0644 files", + &["x.sqlite-wal", "x.sqlite-shm"], + ); + drop(components); + + set_every_mode(&dir, 0o644); + migrate_reads(&path).await; + assert_owner_only(&dir, "migrate over 0644 files", &[]); + let _ = std::fs::remove_dir_all(&dir); +} From ab45c21cdc5f79f15910e821aa9d2c5a7a043128 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:11:20 +0000 Subject: [PATCH 03/13] fix(storage-sqlite): open every SQLite file pool through one owner-only connect The bootstrapper pool, the serve engine and the catalog pool now open their pools through file_mode::connect. It runs prepare before the pool's first connection and restrict after it, so an opener cannot run one step without the other. The new unit tests read the modes of the database file and its -wal and -shm files inside the connection setup, before restrict runs. A database left at 0644 with no sidecars gets 0600 sidecars only because prepare set it to 0600 before SQLite opened it. --- crates/storage-sqlite/src/bootstrapper.rs | 16 +-- crates/storage-sqlite/src/file_mode.rs | 118 +++++++++++++++++----- crates/storage-sqlite/src/lib.rs | 13 +-- crates/storage-sqlite/src/store.rs | 25 ++--- 4 files changed, 109 insertions(+), 63 deletions(-) diff --git a/crates/storage-sqlite/src/bootstrapper.rs b/crates/storage-sqlite/src/bootstrapper.rs index a65d0caf5..06d1be114 100644 --- a/crates/storage-sqlite/src/bootstrapper.rs +++ b/crates/storage-sqlite/src/bootstrapper.rs @@ -105,8 +105,7 @@ impl SqliteBootstrapper { /// Open a short-lived single-connection pool with the standard PRAGMAs. /// For a real filesystem path, the parent directory is created first: /// `init --sqlite-path` may point into a directory that does not exist - /// yet, and bootstrapping it is exactly init's job. The database file is - /// created, and kept, owner read/write only; see `file_mode`. + /// yet, and bootstrapping it is exactly init's job. async fn pool(&self) -> OpResult { if self.path != ":memory:" && let Some(parent) = std::path::Path::new(&self.path).parent() @@ -119,10 +118,7 @@ impl SqliteBootstrapper { )) })?; } - let open_err = - |e: String| OpError::Internal(format!("open SQLite database '{}': {e}", self.path)); - crate::file_mode::prepare(&self.path).map_err(open_err)?; - let pool = SqlitePoolOptions::new() + let options = SqlitePoolOptions::new() .max_connections(1) .after_connect(|conn, _| { Box::pin(async move { @@ -133,12 +129,10 @@ impl SqliteBootstrapper { conn.execute("PRAGMA busy_timeout = 5000").await?; Ok(()) }) - }) - .connect(&sqlite_url(&self.path)) + }); + crate::file_mode::connect(&self.path, options) .await - .map_err(|e| open_err(e.to_string()))?; - crate::file_mode::restrict(&self.path).map_err(open_err)?; - Ok(pool) + .map_err(|e| OpError::Internal(format!("open SQLite database '{}': {e}", self.path))) } } diff --git a/crates/storage-sqlite/src/file_mode.rs b/crates/storage-sqlite/src/file_mode.rs index 5673eee38..b8e7782f7 100644 --- a/crates/storage-sqlite/src/file_mode.rs +++ b/crates/storage-sqlite/src/file_mode.rs @@ -8,16 +8,16 @@ //! (access-key secrets, password hashes), so neither the file nor its `-wal`, //! `-shm` and `-journal` files may be readable by other local users. SQLite's //! unix VFS creates a database file with mode 0644 less the umask, and creates -//! each sidecar with the mode of the database file. So every pool opener on a -//! file database calls [`prepare`] before it connects, which creates the +//! each sidecar with the mode of the database file. So every pool on a file +//! database is opened through [`connect`], which calls [`prepare`] before the +//! pool's first connection and [`restrict`] after it. `prepare` creates the //! database file with mode 0600 (or sets an existing one to 0600) before the -//! connection can create a sidecar from it, and [`restrict`] after it -//! connects, which sets the file and every sidecar present to 0600 whoever -//! created them. +//! connection can create a sidecar from it; `restrict` sets the file and every +//! sidecar present to 0600 whoever created them. //! //! As with the lock file in `serve_lock`, a mode that cannot be set is an //! error. The directory's mode is left alone. On platforms other than Unix -//! both functions do nothing. +//! `prepare` and `restrict` do nothing. #[cfg(unix)] use std::path::{Path, PathBuf}; @@ -26,13 +26,32 @@ use std::str::FromStr; #[cfg(unix)] use crate::serve_lock::{database_file, resolve_dangling_symlinks}; -#[cfg(unix)] use crate::sqlite_util::sqlite_url; -/// Before a pool connects to `location`: create the database file with mode -/// 0600 if the connection would create it, then set the file and any sidecar -/// left by an earlier run to 0600, so the sidecars this connection creates -/// take mode 0600 from the database file. +/// Open a pool on `location` (a path, `:memory:` or a `sqlite:` URL) with +/// `options`, keeping the database file and its sidecars owner read/write +/// only. +/// +/// # Errors +/// +/// The pool cannot connect, or a file's mode cannot be set. +pub(crate) async fn connect( + location: &str, + options: sqlx::sqlite::SqlitePoolOptions, +) -> Result { + prepare(location)?; + let pool = options + .connect(&sqlite_url(location)) + .await + .map_err(|e| e.to_string())?; + restrict(location)?; + Ok(pool) +} + +/// Before the pool connects to `location`: create the database file with +/// mode 0600 if the connection would create it, then set the file and any +/// sidecar left by an earlier run to 0600, so the sidecars this connection +/// creates take mode 0600 from the database file. /// /// A location that names no file on this machine (in-memory, unparseable, a /// remote `file:` authority) is left to the connection, which reports it in @@ -42,7 +61,7 @@ use crate::sqlite_util::sqlite_url; /// /// A file's mode cannot be set. #[cfg(unix)] -pub(crate) fn prepare(location: &str) -> Result<(), String> { +fn prepare(location: &str) -> Result<(), String> { let Ok(Some(path)) = database_file(location) else { return Ok(()); }; @@ -53,18 +72,18 @@ pub(crate) fn prepare(location: &str) -> Result<(), String> { } #[cfg(not(unix))] -pub(crate) fn prepare(_location: &str) -> Result<(), String> { +fn prepare(_location: &str) -> Result<(), String> { Ok(()) } -/// After a pool connects to `location`: set the database file and every +/// After the pool connects to `location`: set the database file and every /// `-wal`, `-shm` and `-journal` file present to mode 0600. /// /// # Errors /// /// A file's mode cannot be set. #[cfg(unix)] -pub(crate) fn restrict(location: &str) -> Result<(), String> { +fn restrict(location: &str) -> Result<(), String> { match database_file(location) { Ok(Some(path)) => restrict_files(&path), _ => Ok(()), @@ -72,7 +91,7 @@ pub(crate) fn restrict(location: &str) -> Result<(), String> { } #[cfg(not(unix))] -pub(crate) fn restrict(_location: &str) -> Result<(), String> { +fn restrict(_location: &str) -> Result<(), String> { Ok(()) } @@ -163,8 +182,9 @@ fn restrict_files(path: &Path) -> Result<(), String> { mod tests { use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; + use std::sync::{Arc, Mutex}; - use super::{prepare, restrict}; + use super::{connect, prepare, restrict}; fn temp_dir() -> PathBuf { let dir = std::env::temp_dir().join(format!( @@ -273,20 +293,64 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } - /// The rule the fix relies on: SQLite gives a new `-wal` or `-shm` file - /// the mode of the database file, so a database created at 0600 gets - /// 0600 sidecars without any chmod after the fact. + /// Open a pool through `connect` that switches to WAL and writes in its + /// connection setup, and return it with the modes of the database file + /// and its `-wal` and `-shm` files as that setup saw them, which is before + /// `connect` calls `restrict`. + async fn connect_and_record(db: &Path) -> (sqlx::SqlitePool, Vec<(&'static str, u32)>) { + let seen = Arc::new(Mutex::new(Vec::new())); + let options = { + let (seen, db) = (Arc::clone(&seen), db.to_path_buf()); + sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .after_connect(move |conn, _| { + let (seen, db) = (Arc::clone(&seen), db.clone()); + Box::pin(async move { + use sqlx::Executor; + conn.execute("PRAGMA journal_mode = WAL").await?; + conn.execute("CREATE TABLE IF NOT EXISTS t (k INTEGER PRIMARY KEY)") + .await?; + let modes = ["", "-wal", "-shm"] + .map(|suffix| (suffix, mode(&sidecar(&db, suffix)))); + seen.lock().expect("lock").extend(modes); + Ok(()) + }) + }) + }; + let pool = connect(&db.to_string_lossy(), options) + .await + .expect("connect"); + let seen = seen.lock().expect("lock").clone(); + (pool, seen) + } + + /// The database file `connect` creates, and the sidecars its connection + /// creates, are 0600 from the moment they exist. + #[tokio::test] + async fn connect_creates_a_missing_database_and_its_sidecars_at_0600() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let (pool, seen) = connect_and_record(&db).await; + assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); + } + + /// The rule the fix relies on, whatever the umask: SQLite gives a new + /// `-wal` or `-shm` file the mode of the database file. A database left + /// at 0644 with no sidecars is set to 0600 by `connect` before the + /// connection opens it, so the sidecars the connection creates are 0600. #[tokio::test] - async fn sidecars_created_after_prepare_take_mode_0600() { + async fn connect_restricts_an_existing_database_before_sqlite_creates_its_sidecars() { let dir = temp_dir(); let db = dir.join("db.sqlite"); - prepare(&db.to_string_lossy()).expect("prepare"); - let pool = raw_wal_pool(&db).await; - for suffix in ["", "-wal", "-shm"] { - let file = sidecar(&db, suffix); - assert!(file.exists(), "{} missing", file.display()); - assert_eq!(mode(&file), 0o600, "{}", file.display()); + raw_wal_pool(&db).await.close().await; + for suffix in ["-wal", "-shm"] { + let _ = std::fs::remove_file(sidecar(&db, suffix)); } + set_mode(&db, 0o644); + let (pool, seen) = connect_and_record(&db).await; + assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); pool.close().await; let _ = std::fs::remove_dir_all(&dir); } diff --git a/crates/storage-sqlite/src/lib.rs b/crates/storage-sqlite/src/lib.rs index c100e04d6..2fc888409 100644 --- a/crates/storage-sqlite/src/lib.rs +++ b/crates/storage-sqlite/src/lib.rs @@ -104,13 +104,10 @@ use extenddb_storage::management_store::{ManagementStore, SettingsStore}; use extenddb_storage::management_store_factory::ManagementStoreError; use extenddb_storage::settings_store::SettingsStoreError; -use crate::sqlite_util::sqlite_url; - /// Open a small catalog-only pool for the settings/diagnostics factories, /// applying the same PRAGMAs and file modes as the engine. async fn catalog_pool(connection_string: &str) -> Result { - file_mode::prepare(connection_string)?; - let pool = sqlx::sqlite::SqlitePoolOptions::new() + let options = sqlx::sqlite::SqlitePoolOptions::new() .max_connections(4) .after_connect(|conn, _| { Box::pin(async move { @@ -120,12 +117,8 @@ async fn catalog_pool(connection_string: &str) -> Result Result { - let url = sqlite_url(path_or_url); // Use the same parsed-location classifier as the serve lock. String // containment misclassifies file names such as `mode=memory.db`. let in_memory = crate::serve_lock::database_file(path_or_url) @@ -123,15 +121,7 @@ impl SqliteEngine { } let mut opts = SqlitePoolOptions::new() .max_connections(max_connections) - .min_connections(1); - if in_memory { - opts = opts.idle_timeout(None).max_lifetime(None); - } - // The database file stores the AES encryption key alongside the data it - // protects (encrypted access-key secrets, password hashes), so the file - // and its sidecars are kept to owner read/write; see `file_mode`. - crate::file_mode::prepare(path_or_url).map_err(StorageError::Connection)?; - let pool = opts + .min_connections(1) .after_connect(|conn, _| { Box::pin(async move { use sqlx::Executor; @@ -142,11 +132,16 @@ impl SqliteEngine { conn.execute("PRAGMA cache_size = -32000").await?; Ok(()) }) - }) - .connect(&url) + }); + if in_memory { + opts = opts.idle_timeout(None).max_lifetime(None); + } + // The database file stores the AES encryption key alongside the data it + // protects (encrypted access-key secrets, password hashes), so the file + // and its sidecars are kept to owner read/write; see `file_mode`. + let pool = crate::file_mode::connect(path_or_url, opts) .await - .map_err(|e| StorageError::Connection(e.to_string()))?; - crate::file_mode::restrict(path_or_url).map_err(StorageError::Connection)?; + .map_err(StorageError::Connection)?; let initial_index_delay: u64 = sqlx::query_as::<_, (String,)>(INDEX_PROPAGATION_DELAY_QUERY) From c23b8bf16421cd47969b2ed2ee56565038719684 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:16:31 +0000 Subject: [PATCH 04/13] test(storage-sqlite): check the restrict after connect and the locations prepare leaves to SQLite A pool whose connection setup sets the database file and its sidecars to 0644 finds them at 0600 once connect returns. prepare creates nothing for a file: URI with its own query or for a VFS outside the unix family. --- crates/storage-sqlite/src/file_mode.rs | 49 ++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/crates/storage-sqlite/src/file_mode.rs b/crates/storage-sqlite/src/file_mode.rs index b8e7782f7..4041bb9fd 100644 --- a/crates/storage-sqlite/src/file_mode.rs +++ b/crates/storage-sqlite/src/file_mode.rs @@ -286,6 +286,18 @@ mod tests { read_only.display() )) .expect("prepare"); + // A `file:` URI with its own query (here `?mode=ro`, percent-encoded + // for sqlx), and a VFS outside the unix family, are left to SQLite. + prepare(&format!( + "sqlite:file:{}%3Fmode=ro?mode=rwc", + dir.join("uri.sqlite").display() + )) + .expect("prepare"); + prepare(&format!( + "sqlite://{}?mode=rwc&vfs=memdb", + dir.join("vfs.sqlite").display() + )) + .expect("prepare"); prepare(":memory:").expect("prepare"); prepare("sqlite::memory:").expect("prepare"); restrict(&plain.to_string_lossy()).expect("restrict"); @@ -296,8 +308,12 @@ mod tests { /// Open a pool through `connect` that switches to WAL and writes in its /// connection setup, and return it with the modes of the database file /// and its `-wal` and `-shm` files as that setup saw them, which is before - /// `connect` calls `restrict`. - async fn connect_and_record(db: &Path) -> (sqlx::SqlitePool, Vec<(&'static str, u32)>) { + /// `connect` calls `restrict`. With `set_in_setup`, the setup then sets + /// those files to that mode. + async fn connect_and_record( + db: &Path, + set_in_setup: Option, + ) -> (sqlx::SqlitePool, Vec<(&'static str, u32)>) { let seen = Arc::new(Mutex::new(Vec::new())); let options = { let (seen, db) = (Arc::clone(&seen), db.to_path_buf()); @@ -310,9 +326,15 @@ mod tests { conn.execute("PRAGMA journal_mode = WAL").await?; conn.execute("CREATE TABLE IF NOT EXISTS t (k INTEGER PRIMARY KEY)") .await?; - let modes = ["", "-wal", "-shm"] - .map(|suffix| (suffix, mode(&sidecar(&db, suffix)))); + let files = + ["", "-wal", "-shm"].map(|suffix| (suffix, sidecar(&db, suffix))); + let modes = files.clone().map(|(suffix, file)| (suffix, mode(&file))); seen.lock().expect("lock").extend(modes); + if let Some(setup_mode) = set_in_setup { + for (_, file) in &files { + set_mode(file, setup_mode); + } + } Ok(()) }) }) @@ -330,7 +352,7 @@ mod tests { async fn connect_creates_a_missing_database_and_its_sidecars_at_0600() { let dir = temp_dir(); let db = dir.join("db.sqlite"); - let (pool, seen) = connect_and_record(&db).await; + let (pool, seen) = connect_and_record(&db, None).await; assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); pool.close().await; let _ = std::fs::remove_dir_all(&dir); @@ -349,12 +371,27 @@ mod tests { let _ = std::fs::remove_file(sidecar(&db, suffix)); } set_mode(&db, 0o644); - let (pool, seen) = connect_and_record(&db).await; + let (pool, seen) = connect_and_record(&db, None).await; assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); pool.close().await; let _ = std::fs::remove_dir_all(&dir); } + /// Whatever the connection leaves behind is set to 0600 once the pool + /// has connected. Here the connection setup itself sets the database file + /// and its sidecars to 0644. + #[tokio::test] + async fn connect_restricts_the_files_after_the_pool_connects() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let (pool, _) = connect_and_record(&db, Some(0o644)).await; + for suffix in ["", "-wal", "-shm"] { + assert_eq!(mode(&sidecar(&db, suffix)), 0o600, "{suffix}"); + } + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); + } + #[tokio::test] async fn prepare_creates_the_target_of_a_dangling_symlink() { let dir = temp_dir(); From 5b075d4140008bd01b26076c396ba2ec02ca23a3 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:37:08 +0000 Subject: [PATCH 05/13] test(storage-sqlite): check prepare and restrict on a sqlite: URL location A configured sqlite: URL names the same database file and sidecars as the path inside it. The engine's old post-connect chmod appended the suffixes to the configured string, which for a URL named files that do not exist. --- crates/storage-sqlite/src/file_mode.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crates/storage-sqlite/src/file_mode.rs b/crates/storage-sqlite/src/file_mode.rs index 4041bb9fd..dcb917083 100644 --- a/crates/storage-sqlite/src/file_mode.rs +++ b/crates/storage-sqlite/src/file_mode.rs @@ -246,6 +246,24 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// A configured `sqlite:` URL names the same files as the path in it. + #[test] + fn prepare_and_restrict_accept_a_sqlite_url() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let location = format!("sqlite://{}?mode=rwc", db.display()); + prepare(&location).expect("prepare"); + assert_eq!(mode(&db), 0o600); + let wal = sidecar(&db, "-wal"); + std::fs::write(&wal, b"").expect("create"); + set_mode(&db, 0o644); + set_mode(&wal, 0o644); + restrict(&location).expect("restrict"); + assert_eq!(mode(&db), 0o600); + assert_eq!(mode(&wal), 0o600); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn prepare_and_restrict_set_an_existing_database_and_its_sidecars_to_0600() { let dir = temp_dir(); From bc497d1c8d694622131e84e319432340285ad700 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 21:37:00 +0000 Subject: [PATCH 06/13] test(storage-sqlite): move the file_mode unit tests into their own file The tests move unchanged from file_mode.rs to file_mode/tests.rs, so the module stays under 600 lines as the tests grow. --- crates/storage-sqlite/src/file_mode.rs | 256 +----------------- crates/storage-sqlite/src/file_mode/tests.rs | 258 +++++++++++++++++++ 2 files changed, 259 insertions(+), 255 deletions(-) create mode 100644 crates/storage-sqlite/src/file_mode/tests.rs diff --git a/crates/storage-sqlite/src/file_mode.rs b/crates/storage-sqlite/src/file_mode.rs index dcb917083..81c3f81eb 100644 --- a/crates/storage-sqlite/src/file_mode.rs +++ b/crates/storage-sqlite/src/file_mode.rs @@ -179,258 +179,4 @@ fn restrict_files(path: &Path) -> Result<(), String> { } #[cfg(all(test, unix))] -mod tests { - use std::os::unix::fs::PermissionsExt; - use std::path::{Path, PathBuf}; - use std::sync::{Arc, Mutex}; - - use super::{connect, prepare, restrict}; - - fn temp_dir() -> PathBuf { - let dir = std::env::temp_dir().join(format!( - "extenddb-file-mode-{}", - uuid::Uuid::new_v4().simple() - )); - std::fs::create_dir_all(&dir).expect("dir"); - dir - } - - fn mode(path: &Path) -> u32 { - std::fs::metadata(path) - .unwrap_or_else(|e| panic!("{}: {e}", path.display())) - .permissions() - .mode() - & 0o777 - } - - fn set_mode(path: &Path, mode: u32) { - std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)).expect("chmod"); - } - - fn sidecar(db: &Path, suffix: &str) -> PathBuf { - let mut name = db.as_os_str().to_owned(); - name.push(suffix); - PathBuf::from(name) - } - - /// A pool opened the way the crate's openers open theirs (WAL journal), - /// without `prepare` or `restrict`, holding its connection so the - /// sidecars stay on disk. - async fn raw_wal_pool(path: &Path) -> sqlx::SqlitePool { - use sqlx::Executor; - let pool = sqlx::sqlite::SqlitePoolOptions::new() - .max_connections(1) - .after_connect(|conn, _| { - Box::pin(async move { - conn.execute("PRAGMA journal_mode = WAL").await?; - Ok(()) - }) - }) - .connect(&crate::sqlite_util::sqlite_url(&path.to_string_lossy())) - .await - .expect("connect"); - sqlx::query("CREATE TABLE IF NOT EXISTS t (k INTEGER PRIMARY KEY)") - .execute(&pool) - .await - .expect("write"); - pool - } - - #[test] - fn prepare_creates_a_missing_database_with_mode_0600() { - let dir = temp_dir(); - let db = dir.join("db.sqlite"); - prepare(&db.to_string_lossy()).expect("prepare"); - assert_eq!(mode(&db), 0o600); - assert_eq!(std::fs::metadata(&db).expect("metadata").len(), 0); - let _ = std::fs::remove_dir_all(&dir); - } - - /// A configured `sqlite:` URL names the same files as the path in it. - #[test] - fn prepare_and_restrict_accept_a_sqlite_url() { - let dir = temp_dir(); - let db = dir.join("db.sqlite"); - let location = format!("sqlite://{}?mode=rwc", db.display()); - prepare(&location).expect("prepare"); - assert_eq!(mode(&db), 0o600); - let wal = sidecar(&db, "-wal"); - std::fs::write(&wal, b"").expect("create"); - set_mode(&db, 0o644); - set_mode(&wal, 0o644); - restrict(&location).expect("restrict"); - assert_eq!(mode(&db), 0o600); - assert_eq!(mode(&wal), 0o600); - let _ = std::fs::remove_dir_all(&dir); - } - - #[test] - fn prepare_and_restrict_set_an_existing_database_and_its_sidecars_to_0600() { - let dir = temp_dir(); - let db = dir.join("db.sqlite"); - let files: Vec = ["", "-wal", "-shm", "-journal"] - .iter() - .map(|suffix| sidecar(&db, suffix)) - .collect(); - for file in &files { - std::fs::write(file, b"").expect("create"); - set_mode(file, 0o644); - } - prepare(&db.to_string_lossy()).expect("prepare"); - for file in &files { - assert_eq!(mode(file), 0o600, "{}", file.display()); - } - for file in &files { - set_mode(file, 0o644); - } - restrict(&db.to_string_lossy()).expect("restrict"); - for file in &files { - assert_eq!(mode(file), 0o600, "{}", file.display()); - } - let _ = std::fs::remove_dir_all(&dir); - } - - #[test] - fn prepare_creates_nothing_when_the_connection_would_not() { - let dir = temp_dir(); - let plain = dir.join("plain.sqlite"); - let read_only = dir.join("ro.sqlite"); - // A sqlite: URL is passed to sqlx as given: without mode=rwc sqlx does - // not pass SQLITE_OPEN_CREATE, and mode=ro wins over mode=rwc. - prepare(&format!("sqlite://{}", plain.display())).expect("prepare"); - prepare(&format!("sqlite://{}?mode=ro", read_only.display())).expect("prepare"); - prepare(&format!( - "sqlite://{}?mode=rwc&mode=ro", - read_only.display() - )) - .expect("prepare"); - // A `file:` URI with its own query (here `?mode=ro`, percent-encoded - // for sqlx), and a VFS outside the unix family, are left to SQLite. - prepare(&format!( - "sqlite:file:{}%3Fmode=ro?mode=rwc", - dir.join("uri.sqlite").display() - )) - .expect("prepare"); - prepare(&format!( - "sqlite://{}?mode=rwc&vfs=memdb", - dir.join("vfs.sqlite").display() - )) - .expect("prepare"); - prepare(":memory:").expect("prepare"); - prepare("sqlite::memory:").expect("prepare"); - restrict(&plain.to_string_lossy()).expect("restrict"); - assert_eq!(std::fs::read_dir(&dir).expect("read_dir").count(), 0); - let _ = std::fs::remove_dir_all(&dir); - } - - /// Open a pool through `connect` that switches to WAL and writes in its - /// connection setup, and return it with the modes of the database file - /// and its `-wal` and `-shm` files as that setup saw them, which is before - /// `connect` calls `restrict`. With `set_in_setup`, the setup then sets - /// those files to that mode. - async fn connect_and_record( - db: &Path, - set_in_setup: Option, - ) -> (sqlx::SqlitePool, Vec<(&'static str, u32)>) { - let seen = Arc::new(Mutex::new(Vec::new())); - let options = { - let (seen, db) = (Arc::clone(&seen), db.to_path_buf()); - sqlx::sqlite::SqlitePoolOptions::new() - .max_connections(1) - .after_connect(move |conn, _| { - let (seen, db) = (Arc::clone(&seen), db.clone()); - Box::pin(async move { - use sqlx::Executor; - conn.execute("PRAGMA journal_mode = WAL").await?; - conn.execute("CREATE TABLE IF NOT EXISTS t (k INTEGER PRIMARY KEY)") - .await?; - let files = - ["", "-wal", "-shm"].map(|suffix| (suffix, sidecar(&db, suffix))); - let modes = files.clone().map(|(suffix, file)| (suffix, mode(&file))); - seen.lock().expect("lock").extend(modes); - if let Some(setup_mode) = set_in_setup { - for (_, file) in &files { - set_mode(file, setup_mode); - } - } - Ok(()) - }) - }) - }; - let pool = connect(&db.to_string_lossy(), options) - .await - .expect("connect"); - let seen = seen.lock().expect("lock").clone(); - (pool, seen) - } - - /// The database file `connect` creates, and the sidecars its connection - /// creates, are 0600 from the moment they exist. - #[tokio::test] - async fn connect_creates_a_missing_database_and_its_sidecars_at_0600() { - let dir = temp_dir(); - let db = dir.join("db.sqlite"); - let (pool, seen) = connect_and_record(&db, None).await; - assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); - pool.close().await; - let _ = std::fs::remove_dir_all(&dir); - } - - /// The rule the fix relies on, whatever the umask: SQLite gives a new - /// `-wal` or `-shm` file the mode of the database file. A database left - /// at 0644 with no sidecars is set to 0600 by `connect` before the - /// connection opens it, so the sidecars the connection creates are 0600. - #[tokio::test] - async fn connect_restricts_an_existing_database_before_sqlite_creates_its_sidecars() { - let dir = temp_dir(); - let db = dir.join("db.sqlite"); - raw_wal_pool(&db).await.close().await; - for suffix in ["-wal", "-shm"] { - let _ = std::fs::remove_file(sidecar(&db, suffix)); - } - set_mode(&db, 0o644); - let (pool, seen) = connect_and_record(&db, None).await; - assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); - pool.close().await; - let _ = std::fs::remove_dir_all(&dir); - } - - /// Whatever the connection leaves behind is set to 0600 once the pool - /// has connected. Here the connection setup itself sets the database file - /// and its sidecars to 0644. - #[tokio::test] - async fn connect_restricts_the_files_after_the_pool_connects() { - let dir = temp_dir(); - let db = dir.join("db.sqlite"); - let (pool, _) = connect_and_record(&db, Some(0o644)).await; - for suffix in ["", "-wal", "-shm"] { - assert_eq!(mode(&sidecar(&db, suffix)), 0o600, "{suffix}"); - } - pool.close().await; - let _ = std::fs::remove_dir_all(&dir); - } - - #[tokio::test] - async fn prepare_creates_the_target_of_a_dangling_symlink() { - let dir = temp_dir(); - let target = dir.join("real.sqlite"); - let link = dir.join("link.sqlite"); - std::os::unix::fs::symlink(&target, &link).expect("symlink"); - let location = link.to_string_lossy().into_owned(); - prepare(&location).expect("prepare"); - assert!(target.exists(), "target not created"); - assert_eq!(mode(&target), 0o600); - // SQLite names the sidecars after the target, and they take its mode. - let pool = raw_wal_pool(&link).await; - for suffix in ["-wal", "-shm"] { - let file = sidecar(&target, suffix); - assert!(file.exists(), "{} missing", file.display()); - assert_eq!(mode(&file), 0o600, "{}", file.display()); - } - set_mode(&sidecar(&target, "-wal"), 0o644); - restrict(&location).expect("restrict"); - assert_eq!(mode(&sidecar(&target, "-wal")), 0o600); - pool.close().await; - let _ = std::fs::remove_dir_all(&dir); - } -} +mod tests; diff --git a/crates/storage-sqlite/src/file_mode/tests.rs b/crates/storage-sqlite/src/file_mode/tests.rs new file mode 100644 index 000000000..432030cc5 --- /dev/null +++ b/crates/storage-sqlite/src/file_mode/tests.rs @@ -0,0 +1,258 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Unit tests for `file_mode`. + +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex}; + +use super::{connect, prepare, restrict}; + +fn temp_dir() -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "extenddb-file-mode-{}", + uuid::Uuid::new_v4().simple() + )); + std::fs::create_dir_all(&dir).expect("dir"); + dir +} + +fn mode(path: &Path) -> u32 { + std::fs::metadata(path) + .unwrap_or_else(|e| panic!("{}: {e}", path.display())) + .permissions() + .mode() + & 0o777 +} + +fn set_mode(path: &Path, mode: u32) { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)).expect("chmod"); +} + +fn sidecar(db: &Path, suffix: &str) -> PathBuf { + let mut name = db.as_os_str().to_owned(); + name.push(suffix); + PathBuf::from(name) +} + +/// A pool opened the way the crate's openers open theirs (WAL journal), +/// without `prepare` or `restrict`, holding its connection so the +/// sidecars stay on disk. +async fn raw_wal_pool(path: &Path) -> sqlx::SqlitePool { + use sqlx::Executor; + let pool = sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .after_connect(|conn, _| { + Box::pin(async move { + conn.execute("PRAGMA journal_mode = WAL").await?; + Ok(()) + }) + }) + .connect(&crate::sqlite_util::sqlite_url(&path.to_string_lossy())) + .await + .expect("connect"); + sqlx::query("CREATE TABLE IF NOT EXISTS t (k INTEGER PRIMARY KEY)") + .execute(&pool) + .await + .expect("write"); + pool +} + +#[test] +fn prepare_creates_a_missing_database_with_mode_0600() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + prepare(&db.to_string_lossy()).expect("prepare"); + assert_eq!(mode(&db), 0o600); + assert_eq!(std::fs::metadata(&db).expect("metadata").len(), 0); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A configured `sqlite:` URL names the same files as the path in it. +#[test] +fn prepare_and_restrict_accept_a_sqlite_url() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let location = format!("sqlite://{}?mode=rwc", db.display()); + prepare(&location).expect("prepare"); + assert_eq!(mode(&db), 0o600); + let wal = sidecar(&db, "-wal"); + std::fs::write(&wal, b"").expect("create"); + set_mode(&db, 0o644); + set_mode(&wal, 0o644); + restrict(&location).expect("restrict"); + assert_eq!(mode(&db), 0o600); + assert_eq!(mode(&wal), 0o600); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn prepare_and_restrict_set_an_existing_database_and_its_sidecars_to_0600() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let files: Vec = ["", "-wal", "-shm", "-journal"] + .iter() + .map(|suffix| sidecar(&db, suffix)) + .collect(); + for file in &files { + std::fs::write(file, b"").expect("create"); + set_mode(file, 0o644); + } + prepare(&db.to_string_lossy()).expect("prepare"); + for file in &files { + assert_eq!(mode(file), 0o600, "{}", file.display()); + } + for file in &files { + set_mode(file, 0o644); + } + restrict(&db.to_string_lossy()).expect("restrict"); + for file in &files { + assert_eq!(mode(file), 0o600, "{}", file.display()); + } + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn prepare_creates_nothing_when_the_connection_would_not() { + let dir = temp_dir(); + let plain = dir.join("plain.sqlite"); + let read_only = dir.join("ro.sqlite"); + // A sqlite: URL is passed to sqlx as given: without mode=rwc sqlx does + // not pass SQLITE_OPEN_CREATE, and mode=ro wins over mode=rwc. + prepare(&format!("sqlite://{}", plain.display())).expect("prepare"); + prepare(&format!("sqlite://{}?mode=ro", read_only.display())).expect("prepare"); + prepare(&format!( + "sqlite://{}?mode=rwc&mode=ro", + read_only.display() + )) + .expect("prepare"); + // A `file:` URI with its own query (here `?mode=ro`, percent-encoded + // for sqlx), and a VFS outside the unix family, are left to SQLite. + prepare(&format!( + "sqlite:file:{}%3Fmode=ro?mode=rwc", + dir.join("uri.sqlite").display() + )) + .expect("prepare"); + prepare(&format!( + "sqlite://{}?mode=rwc&vfs=memdb", + dir.join("vfs.sqlite").display() + )) + .expect("prepare"); + prepare(":memory:").expect("prepare"); + prepare("sqlite::memory:").expect("prepare"); + restrict(&plain.to_string_lossy()).expect("restrict"); + assert_eq!(std::fs::read_dir(&dir).expect("read_dir").count(), 0); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Open a pool through `connect` that switches to WAL and writes in its +/// connection setup, and return it with the modes of the database file +/// and its `-wal` and `-shm` files as that setup saw them, which is before +/// `connect` calls `restrict`. With `set_in_setup`, the setup then sets +/// those files to that mode. +async fn connect_and_record( + db: &Path, + set_in_setup: Option, +) -> (sqlx::SqlitePool, Vec<(&'static str, u32)>) { + let seen = Arc::new(Mutex::new(Vec::new())); + let options = { + let (seen, db) = (Arc::clone(&seen), db.to_path_buf()); + sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .after_connect(move |conn, _| { + let (seen, db) = (Arc::clone(&seen), db.clone()); + Box::pin(async move { + use sqlx::Executor; + conn.execute("PRAGMA journal_mode = WAL").await?; + conn.execute("CREATE TABLE IF NOT EXISTS t (k INTEGER PRIMARY KEY)") + .await?; + let files = + ["", "-wal", "-shm"].map(|suffix| (suffix, sidecar(&db, suffix))); + let modes = files.clone().map(|(suffix, file)| (suffix, mode(&file))); + seen.lock().expect("lock").extend(modes); + if let Some(setup_mode) = set_in_setup { + for (_, file) in &files { + set_mode(file, setup_mode); + } + } + Ok(()) + }) + }) + }; + let pool = connect(&db.to_string_lossy(), options) + .await + .expect("connect"); + let seen = seen.lock().expect("lock").clone(); + (pool, seen) +} + +/// The database file `connect` creates, and the sidecars its connection +/// creates, are 0600 from the moment they exist. +#[tokio::test] +async fn connect_creates_a_missing_database_and_its_sidecars_at_0600() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let (pool, seen) = connect_and_record(&db, None).await; + assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); +} + +/// The rule the fix relies on, whatever the umask: SQLite gives a new +/// `-wal` or `-shm` file the mode of the database file. A database left +/// at 0644 with no sidecars is set to 0600 by `connect` before the +/// connection opens it, so the sidecars the connection creates are 0600. +#[tokio::test] +async fn connect_restricts_an_existing_database_before_sqlite_creates_its_sidecars() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + raw_wal_pool(&db).await.close().await; + for suffix in ["-wal", "-shm"] { + let _ = std::fs::remove_file(sidecar(&db, suffix)); + } + set_mode(&db, 0o644); + let (pool, seen) = connect_and_record(&db, None).await; + assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); +} + +/// Whatever the connection leaves behind is set to 0600 once the pool +/// has connected. Here the connection setup itself sets the database file +/// and its sidecars to 0644. +#[tokio::test] +async fn connect_restricts_the_files_after_the_pool_connects() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let (pool, _) = connect_and_record(&db, Some(0o644)).await; + for suffix in ["", "-wal", "-shm"] { + assert_eq!(mode(&sidecar(&db, suffix)), 0o600, "{suffix}"); + } + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); +} + +#[tokio::test] +async fn prepare_creates_the_target_of_a_dangling_symlink() { + let dir = temp_dir(); + let target = dir.join("real.sqlite"); + let link = dir.join("link.sqlite"); + std::os::unix::fs::symlink(&target, &link).expect("symlink"); + let location = link.to_string_lossy().into_owned(); + prepare(&location).expect("prepare"); + assert!(target.exists(), "target not created"); + assert_eq!(mode(&target), 0o600); + // SQLite names the sidecars after the target, and they take its mode. + let pool = raw_wal_pool(&link).await; + for suffix in ["-wal", "-shm"] { + let file = sidecar(&target, suffix); + assert!(file.exists(), "{} missing", file.display()); + assert_eq!(mode(&file), 0o600, "{}", file.display()); + } + set_mode(&sidecar(&target, "-wal"), 0o644); + restrict(&location).expect("restrict"); + assert_eq!(mode(&sidecar(&target, "-wal")), 0o600); + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); +} From 4dca12f3fb1ed4081704a83883171ab807fb70cf Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 21:50:24 +0000 Subject: [PATCH 07/13] fix(storage-sqlite): narrow only regular files wider than 0600, through a handle that follows no link The mode was set by path on whatever the database name and each sidecar name resolved to. A database path naming a directory set the directory to 0600, a symlink planted at a sidecar name set its target to 0600, and a read-only mount failed the read-only commands although every file was already 0600. Each name is now opened with O_PATH | O_NOFOLLOW on Linux, which opens no file for reading or writing: a symlink yields a handle on the link itself and a FIFO or a device is not opened, so nothing waits. The type and mode are read from that handle, and only a regular file with a permission bit outside 0600 is changed, by chmod of /proc/self/fd/, so the check and the change act on the same file. Closing an O_PATH handle keeps the POSIX locks the process's SQLite connections hold on the file, which closing a descriptor opened for reading would release. On other Unix systems the type and mode come from lstat and the change is fchmodat with AT_SYMLINK_NOFOLLOW, which opens nothing. A regular file wider than 0600 that cannot be narrowed is still an error. The calls that change a mode can be replaced in tests, which use that to check the error without changing any file the test did not create. --- crates/storage-sqlite/src/file_mode.rs | 202 +++++++++++++++---- crates/storage-sqlite/src/file_mode/tests.rs | 147 +++++++++++++- 2 files changed, 310 insertions(+), 39 deletions(-) diff --git a/crates/storage-sqlite/src/file_mode.rs b/crates/storage-sqlite/src/file_mode.rs index 81c3f81eb..1c5edd525 100644 --- a/crates/storage-sqlite/src/file_mode.rs +++ b/crates/storage-sqlite/src/file_mode.rs @@ -11,13 +11,16 @@ //! each sidecar with the mode of the database file. So every pool on a file //! database is opened through [`connect`], which calls [`prepare`] before the //! pool's first connection and [`restrict`] after it. `prepare` creates the -//! database file with mode 0600 (or sets an existing one to 0600) before the -//! connection can create a sidecar from it; `restrict` sets the file and every -//! sidecar present to 0600 whoever created them. +//! database file with mode 0600 (or narrows an existing one to 0600) before the +//! connection can create a sidecar from it; `restrict` narrows the file and +//! every sidecar present to 0600 whoever created them. //! -//! As with the lock file in `serve_lock`, a mode that cannot be set is an -//! error. The directory's mode is left alone. On platforms other than Unix -//! `prepare` and `restrict` do nothing. +//! Only a regular file with a permission bit outside 0600 is changed, and it is +//! set to 0600. Anything else at one of those names is left alone: a symlink +//! (never followed), a directory, a FIFO, a socket, a device, or a regular file +//! already 0600 or narrower. As with the lock file in `serve_lock`, a regular +//! file that cannot be narrowed is an error. The directory's mode is left +//! alone. On platforms other than Unix `prepare` and `restrict` do nothing. #[cfg(unix)] use std::path::{Path, PathBuf}; @@ -48,8 +51,25 @@ pub(crate) async fn connect( Ok(pool) } +/// The calls that change a mode. [`SYSTEM`] makes them; tests substitute them +/// to read a file's mode before it changes, or to make the change fail. +#[cfg(unix)] +struct ModeCalls<'a> { + /// Sets a file `prepare` has just created to 0600, through its descriptor. + created: &'a dyn Fn(&std::fs::File) -> std::io::Result<()>, + /// Sets an existing regular file to 0600, given the path to change (on + /// Linux, the file's handle under `/proc/self/fd`). + existing: &'a dyn Fn(&Path) -> std::io::Result<()>, +} + +#[cfg(unix)] +const SYSTEM: ModeCalls<'static> = ModeCalls { + created: &set_created_owner_only, + existing: &set_existing_owner_only, +}; + /// Before the pool connects to `location`: create the database file with -/// mode 0600 if the connection would create it, then set the file and any +/// mode 0600 if the connection would create it, then narrow the file and any /// sidecar left by an earlier run to 0600, so the sidecars this connection /// creates take mode 0600 from the database file. /// @@ -62,13 +82,18 @@ pub(crate) async fn connect( /// A file's mode cannot be set. #[cfg(unix)] fn prepare(location: &str) -> Result<(), String> { + prepare_with(location, &SYSTEM) +} + +#[cfg(unix)] +fn prepare_with(location: &str, calls: &ModeCalls<'_>) -> Result<(), String> { let Ok(Some(path)) = database_file(location) else { return Ok(()); }; if open_creates_file(location) { - create_owner_only(&resolve_dangling_symlinks(&path))?; + create_owner_only(&resolve_dangling_symlinks(&path), calls)?; } - restrict_files(&path) + restrict_files(&path, calls) } #[cfg(not(unix))] @@ -76,7 +101,7 @@ fn prepare(_location: &str) -> Result<(), String> { Ok(()) } -/// After the pool connects to `location`: set the database file and every +/// After the pool connects to `location`: narrow the database file and every /// `-wal`, `-shm` and `-journal` file present to mode 0600. /// /// # Errors @@ -84,8 +109,13 @@ fn prepare(_location: &str) -> Result<(), String> { /// A file's mode cannot be set. #[cfg(unix)] fn restrict(location: &str) -> Result<(), String> { + restrict_with(location, &SYSTEM) +} + +#[cfg(unix)] +fn restrict_with(location: &str, calls: &ModeCalls<'_>) -> Result<(), String> { match database_file(location) { - Ok(Some(path)) => restrict_files(&path), + Ok(Some(path)) => restrict_files(&path, calls), _ => Ok(()), } } @@ -128,10 +158,11 @@ fn open_creates_file(location: &str) -> bool { }) } -/// Create `path` with mode 0600 if nothing is there yet. +/// Create `path` with mode 0600 if nothing is there yet. `create_new` refuses +/// any existing entry, a symlink included, so nothing is followed or replaced. #[cfg(unix)] -fn create_owner_only(path: &Path) -> Result<(), String> { - use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; +fn create_owner_only(path: &Path, calls: &ModeCalls<'_>) -> Result<(), String> { + use std::os::unix::fs::OpenOptionsExt; match std::fs::OpenOptions::new() .write(true) .create_new(true) @@ -139,44 +170,141 @@ fn create_owner_only(path: &Path) -> Result<(), String> { .open(path) { // The umask applies to `mode` and can remove owner bits as well. - Ok(file) => file - .set_permissions(std::fs::Permissions::from_mode(0o600)) - .map_err(|e| { - format!( - "cannot set the mode of SQLite database {}: {e}", - path.display() - ) - }), + Ok(file) => (calls.created)(&file).map_err(|e| { + format!( + "cannot set the mode of SQLite database {}: {e}", + path.display() + ) + }), // The file exists, or cannot be created here. SQLite's own open // decides what that means and reports it in its usual terms. Err(_) => Ok(()), } } -/// Set the database file and each sidecar that exists to mode 0600. +/// Set a file this module has just created to 0600. Closing its descriptor +/// afterwards releases no lock: the file is new, so no SQLite connection of +/// this process has it open. #[cfg(unix)] -fn restrict_files(path: &Path) -> Result<(), String> { +fn set_created_owner_only(file: &std::fs::File) -> std::io::Result<()> { use std::os::unix::fs::PermissionsExt; - // SQLite follows symlinks before it names the sidecars, so they sit next - // to the target, not next to the link. + file.set_permissions(std::fs::Permissions::from_mode(0o600)) +} + +/// Narrow the database file and each sidecar of it to mode 0600 where needed. +/// The sidecars are named after the database file's resolved path, because +/// SQLite resolves symlinks before it names them. +#[cfg(unix)] +fn restrict_files(path: &Path, calls: &ModeCalls<'_>) -> Result<(), String> { let base = std::fs::canonicalize(path).unwrap_or_else(|_| resolve_dangling_symlinks(path)); for suffix in ["", "-wal", "-shm", "-journal"] { let mut name = base.clone().into_os_string(); name.push(suffix); - let file = PathBuf::from(name); - match std::fs::set_permissions(&file, std::fs::Permissions::from_mode(0o600)) { - Ok(()) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => { - return Err(format!( - "cannot set the mode of SQLite file {}: {e}", - file.display() - )); - } - } + restrict_file(&PathBuf::from(name), calls)?; } Ok(()) } +/// Whether `metadata` is a regular file with a permission bit outside 0600. +#[cfg(unix)] +fn wider_than_owner_only(metadata: &std::fs::Metadata) -> bool { + use std::os::unix::fs::PermissionsExt; + metadata.file_type().is_file() && metadata.permissions().mode() & 0o7777 & !0o600 != 0 +} + +#[cfg(unix)] +fn mode_error(file: &Path, e: &std::io::Error) -> String { + format!("cannot set the mode of SQLite file {}: {e}", file.display()) +} + +/// Set `file` to 0600 if it is a regular file with a permission bit outside +/// 0600, and leave anything else at the name alone (see the module docs). +/// +/// The name is opened with `O_PATH | O_NOFOLLOW`, which opens no file for +/// reading or writing: a symlink yields a handle on the link itself, a FIFO +/// or a device is not opened, so nothing waits, and closing the handle keeps +/// the POSIX locks this process's SQLite connections hold on the file, which +/// closing a descriptor opened for reading would release. The type and mode +/// are read from the handle and the mode is changed through +/// `/proc/self/fd/`, so the check and the change act on the same file. +#[cfg(any(target_os = "linux", target_os = "android"))] +fn restrict_file(file: &Path, calls: &ModeCalls<'_>) -> Result<(), String> { + use std::os::fd::AsRawFd; + use std::os::unix::fs::OpenOptionsExt; + let handle = match std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_PATH | libc::O_NOFOLLOW) + .open(file) + { + Ok(handle) => handle, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(mode_error(file, &e)), + }; + let metadata = handle.metadata().map_err(|e| mode_error(file, &e))?; + if !wider_than_owner_only(&metadata) { + return Ok(()); + } + let by_handle = Path::new("/proc/self/fd").join(handle.as_raw_fd().to_string()); + (calls.existing)(&by_handle).map_err(|e| mode_error(file, &e)) +} + +/// `chmod` of a `/proc/self/fd/` name changes the file the handle refers +/// to, whatever is at the original name by then. +#[cfg(any(target_os = "linux", target_os = "android"))] +fn set_existing_owner_only(by_handle: &Path) -> std::io::Result<()> { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(by_handle, std::fs::Permissions::from_mode(0o600)).map_err(|e| { + if e.kind() == std::io::ErrorKind::NotFound { + std::io::Error::new( + e.kind(), + format!("{} is not available ({e})", by_handle.display()), + ) + } else { + e + } + }) +} + +/// As the Linux version, on a Unix without `O_PATH` (macOS, the BSDs): the +/// type and mode are read with `lstat` and the mode is changed with +/// `fchmodat(AT_SYMLINK_NOFOLLOW)`, which follows no link and opens nothing, +/// so no descriptor is closed on a file SQLite holds locks on. The two calls +/// name the same directory entry; replacing that entry between them takes +/// write access to the database's directory. +#[cfg(all(unix, not(any(target_os = "linux", target_os = "android"))))] +fn restrict_file(file: &Path, calls: &ModeCalls<'_>) -> Result<(), String> { + let metadata = match std::fs::symlink_metadata(file) { + Ok(metadata) => metadata, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(mode_error(file, &e)), + }; + if !wider_than_owner_only(&metadata) { + return Ok(()); + } + (calls.existing)(file).map_err(|e| mode_error(file, &e)) +} + +#[cfg(all(unix, not(any(target_os = "linux", target_os = "android"))))] +fn set_existing_owner_only(file: &Path) -> std::io::Result<()> { + use std::os::unix::ffi::OsStrExt; + let name = std::ffi::CString::new(file.as_os_str().as_bytes()) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, e))?; + // SAFETY: `fchmodat` reads the NUL-terminated name, which `name` owns for + // the duration of the call, and touches no other memory. + let rc = unsafe { + libc::fchmodat( + libc::AT_FDCWD, + name.as_ptr(), + 0o600, + libc::AT_SYMLINK_NOFOLLOW, + ) + }; + if rc == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + #[cfg(all(test, unix))] mod tests; diff --git a/crates/storage-sqlite/src/file_mode/tests.rs b/crates/storage-sqlite/src/file_mode/tests.rs index 432030cc5..0ea6a2415 100644 --- a/crates/storage-sqlite/src/file_mode/tests.rs +++ b/crates/storage-sqlite/src/file_mode/tests.rs @@ -167,8 +167,7 @@ async fn connect_and_record( conn.execute("PRAGMA journal_mode = WAL").await?; conn.execute("CREATE TABLE IF NOT EXISTS t (k INTEGER PRIMARY KEY)") .await?; - let files = - ["", "-wal", "-shm"].map(|suffix| (suffix, sidecar(&db, suffix))); + let files = ["", "-wal", "-shm"].map(|suffix| (suffix, sidecar(&db, suffix))); let modes = files.clone().map(|(suffix, file)| (suffix, mode(&file))); seen.lock().expect("lock").extend(modes); if let Some(setup_mode) = set_in_setup { @@ -256,3 +255,147 @@ async fn prepare_creates_the_target_of_a_dangling_symlink() { pool.close().await; let _ = std::fs::remove_dir_all(&dir); } + +/// Mode calls that record each existing file they are asked to change and +/// fail the change, as `chmod` does on a read-only mount. Nothing outside the +/// test's directory is touched, whoever runs the test. +fn read_only_calls<'a>(existing: &'a dyn Fn(&Path) -> std::io::Result<()>) -> super::ModeCalls<'a> { + super::ModeCalls { + created: &super::set_created_owner_only, + existing, + } +} + +/// Run `f` on another thread and fail the test if it does not return within +/// ten seconds, so a call that blocks shows up as a failure, not a hang. +fn within_ten_seconds( + f: impl FnOnce() -> Result<(), String> + Send + 'static, +) -> Result<(), String> { + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let _ = tx.send(f()); + }); + rx.recv_timeout(std::time::Duration::from_secs(10)) + .expect("blocked for ten seconds") +} + +/// A database path that names a directory (a config with the file name left +/// off) is left to SQLite, which refuses it; the directory keeps its mode. +#[test] +fn a_directory_at_the_database_name_keeps_its_mode() { + let dir = temp_dir(); + let db_dir = dir.join("dbdir"); + std::fs::create_dir(&db_dir).expect("dbdir"); + set_mode(&db_dir, 0o755); + for location in [ + db_dir.to_string_lossy().into_owned(), + format!("{}/", db_dir.display()), + ] { + prepare(&location).expect("prepare"); + restrict(&location).expect("restrict"); + assert_eq!(mode(&db_dir), 0o755, "{location}"); + } + let _ = std::fs::remove_dir_all(&dir); +} + +/// Only a regular file is changed. A symlink at a sidecar name is not +/// followed (its target keeps its mode), and a FIFO or a socket there keeps +/// its mode; the FIFO, which no writer ever opens, does not make the call +/// wait. The database file itself is narrowed. +#[test] +fn only_regular_files_are_changed_and_nothing_waits() { + use std::os::unix::ffi::OsStrExt; + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + std::fs::write(&db, b"").expect("db"); + set_mode(&db, 0o644); + let decoy = dir.join("decoy.txt"); + std::fs::write(&decoy, b"").expect("decoy"); + set_mode(&decoy, 0o644); + let journal = sidecar(&db, "-journal"); + std::os::unix::fs::symlink(&decoy, &journal).expect("symlink"); + let fifo = sidecar(&db, "-wal"); + let name = std::ffi::CString::new(fifo.as_os_str().as_bytes()).expect("name"); + // SAFETY: `mkfifo` reads the NUL-terminated name, which `name` owns for + // the duration of the call. + assert_eq!(unsafe { libc::mkfifo(name.as_ptr(), 0o644) }, 0, "mkfifo"); + set_mode(&fifo, 0o644); + // A socket node, made with mknod because the test directory's path is + // longer than a socket address allows; Linux lets any user make one. + let socket = sidecar(&db, "-shm"); + #[cfg(target_os = "linux")] + { + let name = std::ffi::CString::new(socket.as_os_str().as_bytes()).expect("name"); + // SAFETY: `mknod` reads the NUL-terminated name, which `name` owns for + // the duration of the call. + let rc = unsafe { libc::mknod(name.as_ptr(), libc::S_IFSOCK | 0o644, 0) }; + assert_eq!(rc, 0, "mknod"); + set_mode(&socket, 0o644); + } + + let location = db.to_string_lossy().into_owned(); + let (for_prepare, for_restrict) = (location.clone(), location); + within_ten_seconds(move || prepare(&for_prepare)).expect("prepare"); + within_ten_seconds(move || restrict(&for_restrict)).expect("restrict"); + + assert_eq!(mode(&db), 0o600); + assert_eq!(mode(&decoy), 0o644, "the symlink at -journal was followed"); + let link = std::fs::symlink_metadata(&journal).expect("lstat -journal"); + assert!(link.file_type().is_symlink(), "-journal replaced"); + assert_eq!(mode(&fifo), 0o644, "the FIFO at -wal was changed"); + #[cfg(target_os = "linux")] + assert_eq!(mode(&socket), 0o644, "the socket at -shm was changed"); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A regular file wider than 0600 whose mode cannot be changed (here the +/// change fails with EROFS) is an error naming the file, and stays as it was. +#[test] +fn a_file_that_cannot_be_narrowed_is_an_error() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + std::fs::write(&db, b"").expect("db"); + set_mode(&db, 0o644); + let fail = |_: &Path| Err(std::io::Error::from_raw_os_error(libc::EROFS)); + let calls = read_only_calls(&fail); + let location = db.to_string_lossy().into_owned(); + for result in [ + super::prepare_with(&location, &calls), + super::restrict_with(&location, &calls), + ] { + let err = result.expect_err("a mode that cannot be set is accepted"); + assert!( + err.starts_with("cannot set the mode of SQLite file ") + && err.contains(&*db.to_string_lossy()) + && err.contains("Read-only file system"), + "{err}" + ); + } + assert_eq!(mode(&db), 0o644); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Files already 0600, or narrower, are not changed at all, so a read-only +/// mount whose files are already owner-only still opens. +#[test] +fn owner_only_files_are_left_alone() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + for (suffix, file_mode) in [("", 0o600), ("-wal", 0o600), ("-shm", 0o400)] { + std::fs::write(sidecar(&db, suffix), b"").expect("create"); + set_mode(&sidecar(&db, suffix), file_mode); + } + let asked = Mutex::new(Vec::new()); + let fail = |file: &Path| { + asked.lock().expect("lock").push(file.to_path_buf()); + Err(std::io::Error::from_raw_os_error(libc::EROFS)) + }; + let calls = read_only_calls(&fail); + let location = db.to_string_lossy().into_owned(); + super::prepare_with(&location, &calls).expect("prepare"); + super::restrict_with(&location, &calls).expect("restrict"); + assert_eq!(*asked.lock().expect("lock"), Vec::::new()); + assert_eq!(mode(&db), 0o600); + assert_eq!(mode(&sidecar(&db, "-shm")), 0o400); + let _ = std::fs::remove_dir_all(&dir); +} From 561f6f591439e00aa7934ede4a9f68f29ab1ea95 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 21:52:38 +0000 Subject: [PATCH 08/13] test(storage-sqlite): check that narrowing a database's files keeps the locks of its open connection A WAL connection holds a read lock on the database file's shared range and on the DMS byte of the -shm file. The test narrows the files of a database whose connection is open and asks, with an open file description lock, whether a write lock on either range would still conflict. --- crates/storage-sqlite/src/file_mode/tests.rs | 57 ++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/crates/storage-sqlite/src/file_mode/tests.rs b/crates/storage-sqlite/src/file_mode/tests.rs index 0ea6a2415..c64dad904 100644 --- a/crates/storage-sqlite/src/file_mode/tests.rs +++ b/crates/storage-sqlite/src/file_mode/tests.rs @@ -399,3 +399,60 @@ fn owner_only_files_are_left_alone() { assert_eq!(mode(&sidecar(&db, "-shm")), 0o400); let _ = std::fs::remove_dir_all(&dir); } + +/// Whether a write lock on `len` bytes at `start` of `file` would conflict +/// with a lock already held there. The question is asked with an open file +/// description lock, which conflicts with this process's own POSIX locks as +/// well as other processes'. Closing the descriptor afterwards releases this +/// process's POSIX locks on the file, so ask once per file, last. +#[cfg(target_os = "linux")] +fn write_lock_conflicts(file: &Path, start: i64, len: i64) -> bool { + use std::os::fd::AsRawFd; + let probe = std::fs::OpenOptions::new() + .read(true) + .write(true) + .open(file) + .expect("open"); + // SAFETY: `flock` is plain data, for which all zero bytes is a value. + let mut lock: libc::flock = unsafe { std::mem::zeroed() }; + lock.l_type = libc::F_WRLCK as libc::c_short; + lock.l_whence = libc::SEEK_SET as libc::c_short; + lock.l_start = start; + lock.l_len = len; + // SAFETY: F_OFD_GETLK reads and writes the `flock` that `lock` owns; the + // descriptor belongs to `probe`, which lives until the call returns. + let rc = unsafe { libc::fcntl(probe.as_raw_fd(), libc::F_OFD_GETLK, &mut lock) }; + assert_eq!(rc, 0, "F_OFD_GETLK: {}", std::io::Error::last_os_error()); + lock.l_type != libc::F_UNLCK as libc::c_short +} + +/// Narrowing the files of a database this process has open keeps the +/// connection's POSIX locks: the read lock a WAL connection holds on the +/// database file's shared range and the one on the `-shm` file's DMS byte. +/// A closed read descriptor on either file would release them, and another +/// process could then reset the `-shm` file or delete the WAL under the open +/// connection. +#[cfg(target_os = "linux")] +#[tokio::test] +async fn narrowing_keeps_the_locks_of_an_open_connection() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let pool = raw_wal_pool(&db).await; + for suffix in ["", "-wal", "-shm"] { + set_mode(&sidecar(&db, suffix), 0o644); + } + restrict(&db.to_string_lossy()).expect("restrict"); + for suffix in ["", "-wal", "-shm"] { + assert_eq!(mode(&sidecar(&db, suffix)), 0o600, "{suffix}"); + } + assert!( + write_lock_conflicts(&db, 0x4000_0002, 510), + "the connection's lock on the database file was released" + ); + assert!( + write_lock_conflicts(&sidecar(&db, "-shm"), 128, 1), + "the connection's lock on the -shm file was released" + ); + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); +} From 967693e9285d5923fed131e39fce1ee3a1d24b33 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 22:07:59 +0000 Subject: [PATCH 09/13] test(storage-sqlite): check each opener alone and the mode the database file is created with The integration tests opened every pool in sequence and checked the end state, so a later opener's narrowing hid an opener that skipped its own. Three tests now open one pool each on a missing path and read the modes before any other pool opens the file: the bootstrapper's pool, the engine's pool and the catalog pool through the settings store factory. A unit test reads the mode of the database file prepare creates as the file is created, before prepare narrows it, and compares it with a file the test creates with mode 0600 under the same umask. --- crates/storage-sqlite/src/file_mode/tests.rs | 32 +++++++++++++ crates/storage-sqlite/tests/file_modes.rs | 49 +++++++++++++++++++- 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/crates/storage-sqlite/src/file_mode/tests.rs b/crates/storage-sqlite/src/file_mode/tests.rs index c64dad904..4b45ce756 100644 --- a/crates/storage-sqlite/src/file_mode/tests.rs +++ b/crates/storage-sqlite/src/file_mode/tests.rs @@ -456,3 +456,35 @@ async fn narrowing_keeps_the_locks_of_an_open_connection() { pool.close().await; let _ = std::fs::remove_dir_all(&dir); } + +/// `prepare` creates the database file with mode 0600, rather than a wider +/// mode it narrows afterwards: the mode is read as the file is created, before +/// `prepare` changes it, and compared with a file the test creates with mode +/// 0600 under the same umask. +#[test] +fn prepare_creates_the_database_file_with_mode_0600() { + use std::os::unix::fs::OpenOptionsExt; + let dir = temp_dir(); + let reference = dir.join("reference"); + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&reference) + .expect("reference"); + let created_with = Mutex::new(None); + let record = |file: &std::fs::File| { + let created = file.metadata()?.permissions().mode() & 0o7777; + *created_with.lock().expect("lock") = Some(created); + super::set_created_owner_only(file) + }; + let calls = super::ModeCalls { + created: &record, + existing: &super::set_existing_owner_only, + }; + let db = dir.join("db.sqlite"); + super::prepare_with(&db.to_string_lossy(), &calls).expect("prepare"); + assert_eq!(*created_with.lock().expect("lock"), Some(mode(&reference))); + assert_eq!(mode(&db), 0o600); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/storage-sqlite/tests/file_modes.rs b/crates/storage-sqlite/tests/file_modes.rs index 41be979ae..9289f883a 100644 --- a/crates/storage-sqlite/tests/file_modes.rs +++ b/crates/storage-sqlite/tests/file_modes.rs @@ -21,7 +21,7 @@ use std::path::{Path, PathBuf}; use extenddb_storage::bootstrapper::Bootstrapper; use extenddb_storage::server_components::{ServerComponents, ServerComponentsOptions}; use extenddb_storage::{MetadataEngine, TableEngine}; -use extenddb_storage_sqlite::{SqliteBootstrapper, SqliteConfig}; +use extenddb_storage_sqlite::{SqliteBootstrapper, SqliteConfig, SqliteEngine}; const ACCOUNT: &str = "000000000000"; @@ -311,3 +311,50 @@ async fn serve_and_migrate_restrict_files_left_at_0644() { assert_owner_only(&dir, "migrate over 0644 files", &[]); let _ = std::fs::remove_dir_all(&dir); } + +// Each opener alone, on a database path that does not exist yet, read before +// any other pool opens the file: a later opener's narrowing cannot hide one +// that skipped its own. + +/// The bootstrapper's pool, which `init` and `migrate` open for each step. +#[tokio::test] +async fn the_bootstrapper_pool_alone_creates_owner_only_files() { + let dir = deployment_dir(); + let path = database_path(&dir); + SqliteBootstrapper::new(&path) + .run_catalog_migrations() + .await + .expect("run_catalog_migrations"); + assert_owner_only(&dir, "the bootstrapper's pool", &[]); + let _ = std::fs::remove_dir_all(&dir); +} + +/// The engine's pool, which `serve` opens first. +#[tokio::test] +async fn the_engine_pool_alone_creates_owner_only_files() { + let dir = deployment_dir(); + let path = database_path(&dir); + let engine = SqliteEngine::new(&path, 2, "us-east-1", 409_600) + .await + .expect("engine"); + assert_owner_only(&dir, "the engine's pool", &["x.sqlite-wal", "x.sqlite-shm"]); + drop(engine); + let _ = std::fs::remove_dir_all(&dir); +} + +/// The catalog pool, which `settings`, `verify` and the management and +/// diagnostics stores open, here through the settings store factory. Only the +/// database file exists when the factory returns: the pool's connection has +/// run its PRAGMAs but no transaction, and SQLite opens the `-wal` and `-shm` +/// files at the first transaction. +#[tokio::test] +async fn the_catalog_pool_alone_creates_owner_only_files() { + let dir = deployment_dir(); + let path = database_path(&dir); + let store = (extenddb_storage_sqlite::backend().settings_store)(&path) + .await + .expect("settings store"); + assert_owner_only(&dir, "the catalog pool", &[]); + drop(store); + let _ = std::fs::remove_dir_all(&dir); +} From 8c88ed67de69b1ddcba02544be6de8ae99cff6a6 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 22:22:18 +0000 Subject: [PATCH 10/13] fix(storage-sqlite): read SQLite locations the way sqlx and SQLite read them before creating the database prepare decided whether to create the database file, and which file, from a reading of the location that differed from sqlx's and SQLite's in four ways. With several vfs values it required all of them to be in the unix family, where sqlx uses the last. It left every file: URI with its own query to SQLite, which created the file at 0644 when that query did not narrow the open. database_file did not decode %HH in a file: URI path, so prepare created a stray file and the database SQLite opened stayed 0644; it matched mode=memory with a raw split, so an encoded key such as mod%65 made prepare create a file SQLite never used; and a file: name combined with vfs or immutable, which sqlx wraps in a second file: URI so that SQLite opens a relative path spelled file:..., got a stray file at the path the URI names. A new module, sqlite_uri, reads a file: URI as the bundled SQLite does: the authority, the path and query split at raw separators, %HH decoded, %00 ending a part, the last vfs, and each mode applied in order with the rule that a mode may not ask for more access than the one before it. prepare creates the file only when SQLite would create it on disk. database_file reads the path through sqlite_uri and mode through form_urlencoded, and file_mode leaves a file: name with vfs or immutable alone. The lock file moves only where the decoding changes the path: for a file: URI with %HH in its path it is now next to the file SQLite opens, and an encoded mode=memory takes no lock, as every other in-memory form. --- crates/storage-sqlite/src/file_mode.rs | 84 ++++--- crates/storage-sqlite/src/file_mode/tests.rs | 112 ++++++++- crates/storage-sqlite/src/lib.rs | 1 + crates/storage-sqlite/src/serve_lock.rs | 79 +++--- crates/storage-sqlite/src/sqlite_uri.rs | 247 +++++++++++++++++++ 5 files changed, 456 insertions(+), 67 deletions(-) create mode 100644 crates/storage-sqlite/src/sqlite_uri.rs diff --git a/crates/storage-sqlite/src/file_mode.rs b/crates/storage-sqlite/src/file_mode.rs index 1c5edd525..6308bbe48 100644 --- a/crates/storage-sqlite/src/file_mode.rs +++ b/crates/storage-sqlite/src/file_mode.rs @@ -87,13 +87,13 @@ fn prepare(location: &str) -> Result<(), String> { #[cfg(unix)] fn prepare_with(location: &str, calls: &ModeCalls<'_>) -> Result<(), String> { - let Ok(Some(path)) = database_file(location) else { + let Some(opened) = opened_file(location) else { return Ok(()); }; - if open_creates_file(location) { - create_owner_only(&resolve_dangling_symlinks(&path), calls)?; + if opened.created { + create_owner_only(&resolve_dangling_symlinks(&opened.path), calls)?; } - restrict_files(&path, calls) + restrict_files(&opened.path, calls) } #[cfg(not(unix))] @@ -114,9 +114,9 @@ fn restrict(location: &str) -> Result<(), String> { #[cfg(unix)] fn restrict_with(location: &str, calls: &ModeCalls<'_>) -> Result<(), String> { - match database_file(location) { - Ok(Some(path)) => restrict_files(&path, calls), - _ => Ok(()), + match opened_file(location) { + Some(opened) => restrict_files(&opened.path, calls), + None => Ok(()), } } @@ -125,37 +125,57 @@ fn restrict(_location: &str) -> Result<(), String> { Ok(()) } -/// Whether connecting to `location` creates the database file when it is -/// missing. sqlx reads the pairs after the first `?` as form-urlencoded and -/// passes `SQLITE_OPEN_CREATE` for `mode=rwc` unless `mode=ro` is also given; -/// a plain path always gets `mode=rwc` from `sqlite_url`. Two forms are left -/// to SQLite because they can keep it from creating a file on disk: a `file:` -/// URI with its own query (its `mode` can narrow the open) and a VFS outside -/// the unix family. +/// The file a connection to a location opens on disk. #[cfg(unix)] -fn open_creates_file(location: &str) -> bool { +struct OpenedFile { + path: PathBuf, + /// Whether the connection creates the file when it is missing. + created: bool, +} + +/// The file a connection to `location` opens on disk, or `None` for a +/// location left to the connection: in memory, unparseable, a remote `file:` +/// authority, or a `file:` name with `vfs` or `immutable` in the query. sqlx +/// wraps such a name in a second `file:` URI, so SQLite opens a relative path +/// spelled `file:...`, not the file the URI names. +/// +/// sqlx reads the pairs after the first `?` as form-urlencoded: any +/// `mode=rwc` passes `SQLITE_OPEN_CREATE` unless a `mode=ro` is also given, +/// and the last `vfs` and the last `immutable` win. A plain path always gets +/// `mode=rwc` from `sqlite_url`. A `file:` URI's own query is then read as +/// SQLite reads it (`sqlite_uri`), and the file is created only if SQLite +/// would create it on disk, with a VFS in the unix family. +#[cfg(unix)] +fn opened_file(location: &str) -> Option { + let path = database_file(location).ok()??; let url = sqlite_url(location); - let Some((_, query)) = url.split_once('?') else { - return false; - }; - let (mut rwc, mut ro, mut unix_vfs) = (false, false, true); - for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { - match &*key { - "mode" => { - rwc |= value == "rwc"; - ro |= value == "ro"; + let (mut rwc, mut ro, mut vfs, mut immutable) = (false, false, None, false); + if let Some((_, query)) = url.split_once('?') { + for (key, value) in url::form_urlencoded::parse(query.as_bytes()) { + match &*key { + "mode" => { + rwc |= value == "rwc"; + ro |= value == "ro"; + } + "vfs" => vfs = Some(value), + "immutable" => immutable = matches!(&*value, "true" | "1"), + _ => {} } - "vfs" => unix_vfs &= value.starts_with("unix"), - _ => {} } } - if !rwc || ro || !unix_vfs { - return false; + let options = sqlx::sqlite::SqliteConnectOptions::from_str(&url).ok()?; + let name = options.get_filename().to_string_lossy(); + let uri = name.strip_prefix("file:"); + if uri.is_some() && (vfs.is_some() || immutable) { + return None; } - sqlx::sqlite::SqliteConnectOptions::from_str(&url).is_ok_and(|options| { - let name = options.get_filename().to_string_lossy(); - !(name.starts_with("file:") && name.contains('?')) - }) + let created = rwc + && !ro + && vfs.is_none_or(|vfs| vfs.starts_with("unix")) + && uri.is_none_or(|rest| { + crate::sqlite_uri::FileUri::parse(rest).is_ok_and(|uri| uri.creates_file()) + }); + Some(OpenedFile { path, created }) } /// Create `path` with mode 0600 if nothing is there yet. `create_new` refuses diff --git a/crates/storage-sqlite/src/file_mode/tests.rs b/crates/storage-sqlite/src/file_mode/tests.rs index 4b45ce756..961060470 100644 --- a/crates/storage-sqlite/src/file_mode/tests.rs +++ b/crates/storage-sqlite/src/file_mode/tests.rs @@ -141,6 +141,21 @@ fn prepare_creates_nothing_when_the_connection_would_not() { .expect("prepare"); prepare(":memory:").expect("prepare"); prepare("sqlite::memory:").expect("prepare"); + // A `file:` URI whose own query SQLite reads as no creation: `mode=rw`, + // `mode=memory`, a VFS outside the unix family, and `mode=ro` followed by + // `mode=rwc`, which asks for more access than `ro` allows and fails. + for (name, query) in [ + ("rw.sqlite", "mode=rw"), + ("memory.sqlite", "mode=memory"), + ("memdb.sqlite", "vfs=memdb"), + ("escalate.sqlite", "mode=ro%26mode=rwc"), + ] { + let uri = format!( + "sqlite:file:{}%3F{query}?mode=rwc", + dir.join(name).display() + ); + prepare(&uri).expect("prepare"); + } restrict(&plain.to_string_lossy()).expect("restrict"); assert_eq!(std::fs::read_dir(&dir).expect("read_dir").count(), 0); let _ = std::fs::remove_dir_all(&dir); @@ -154,6 +169,15 @@ fn prepare_creates_nothing_when_the_connection_would_not() { async fn connect_and_record( db: &Path, set_in_setup: Option, +) -> (sqlx::SqlitePool, Vec<(&'static str, u32)>) { + connect_and_record_at(&db.to_string_lossy(), db, set_in_setup).await +} + +/// As `connect_and_record`, connecting to `location`, which opens `db`. +async fn connect_and_record_at( + location: &str, + db: &Path, + set_in_setup: Option, ) -> (sqlx::SqlitePool, Vec<(&'static str, u32)>) { let seen = Arc::new(Mutex::new(Vec::new())); let options = { @@ -179,9 +203,7 @@ async fn connect_and_record( }) }) }; - let pool = connect(&db.to_string_lossy(), options) - .await - .expect("connect"); + let pool = connect(location, options).await.expect("connect"); let seen = seen.lock().expect("lock").clone(); (pool, seen) } @@ -488,3 +510,87 @@ fn prepare_creates_the_database_file_with_mode_0600() { assert_eq!(mode(&db), 0o600); let _ = std::fs::remove_dir_all(&dir); } + +/// Location forms on which SQLite creates the database on disk: the last of +/// several `vfs` values is `unix`, and a `file:` URI's own query does not +/// narrow the open. The database and its sidecars are 0600 from the moment +/// they exist, as the connection setup sees them. +#[tokio::test] +async fn forms_sqlite_creates_on_disk_start_at_0600() { + for form in [ + "sqlite://{db}?mode=rwc&vfs=memdb&vfs=unix", + "sqlite:file:{db}%3Fcache=shared?mode=rwc", + "sqlite:file:{db}%3Fmode=rwc?mode=rwc", + ] { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let location = form.replace("{db}", &db.to_string_lossy()); + let (pool, seen) = connect_and_record_at(&location, &db, None).await; + assert_eq!( + seen, + [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)], + "{location}" + ); + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); + } +} + +/// A `file:` URI whose path SQLite decodes (`%2520` reaches SQLite as `%20`, +/// a space): `prepare` creates the file SQLite opens and no other, and +/// `restrict` reaches it and its sidecars, which the setup leaves at 0644. +#[tokio::test] +async fn a_decoded_file_uri_path_names_the_file_sqlite_opens() { + let dir = temp_dir(); + let db = dir.join("a b.sqlite"); + let location = format!("sqlite:file:{}/a%2520b.sqlite?mode=rwc", dir.display()); + let (pool, seen) = connect_and_record_at(&location, &db, Some(0o644)).await; + assert_eq!(seen, [("", 0o600), ("-wal", 0o600), ("-shm", 0o600)]); + for suffix in ["", "-wal", "-shm"] { + assert_eq!(mode(&sidecar(&db, suffix)), 0o600, "{suffix}"); + } + let mut names: Vec = std::fs::read_dir(&dir) + .expect("read_dir") + .map(|entry| { + entry + .expect("entry") + .file_name() + .to_string_lossy() + .into_owned() + }) + .collect(); + names.sort(); + assert_eq!(names, ["a b.sqlite", "a b.sqlite-shm", "a b.sqlite-wal"]); + pool.close().await; + let _ = std::fs::remove_dir_all(&dir); +} + +/// Locations that name a path SQLite does not open: `mod%65=memory`, which +/// sqlx reads as `mode=memory`, and a `file:` URI with `vfs` or `immutable` +/// in the query, which sqlx wraps so that SQLite opens a relative path spelled +/// `file:...`. `prepare` and `restrict` create nothing in the directory the +/// path names, and the in-memory connection leaves nothing there either. +#[tokio::test] +async fn locations_sqlite_does_not_open_as_named_are_left_alone() { + let dir = temp_dir(); + let db = dir.join("db.sqlite"); + let in_memory = format!("sqlite://{}?mode=rwc&mod%65=memory", db.display()); + for location in [ + in_memory.clone(), + format!("sqlite:file:{}?mode=rwc&vfs=unix", db.display()), + format!("sqlite:file:{}?mode=rwc&immutable=1", db.display()), + ] { + prepare(&location).expect("prepare"); + restrict(&location).expect("restrict"); + } + let pool = connect(&in_memory, sqlx::sqlite::SqlitePoolOptions::new()) + .await + .expect("connect"); + sqlx::query("CREATE TABLE t (k INTEGER PRIMARY KEY)") + .execute(&pool) + .await + .expect("write"); + pool.close().await; + assert_eq!(std::fs::read_dir(&dir).expect("read_dir").count(), 0); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/storage-sqlite/src/lib.rs b/crates/storage-sqlite/src/lib.rs index 2fc888409..4937249bd 100644 --- a/crates/storage-sqlite/src/lib.rs +++ b/crates/storage-sqlite/src/lib.rs @@ -39,6 +39,7 @@ mod operations; mod schema; mod serve_lock; mod snapshot_engine; +mod sqlite_uri; mod sqlite_util; mod store; mod stream; diff --git a/crates/storage-sqlite/src/serve_lock.rs b/crates/storage-sqlite/src/serve_lock.rs index 77fbb777c..7207cd716 100644 --- a/crates/storage-sqlite/src/serve_lock.rs +++ b/crates/storage-sqlite/src/serve_lock.rs @@ -49,15 +49,12 @@ pub(crate) fn database_file(location: &str) -> Result, String> { let options = SqliteConnectOptions::from_str(&url) .map_err(|e| format!("cannot parse SQLite location {location:?}: {e}"))?; // `mode=memory` is recorded in a private field; read it off the query the - // same way sqlx does (form-urlencoded pairs after the first `?`). - let mode_memory = url - .split_once('?') - .map(|(_, q)| { - q.split('&') - .filter_map(|kv| kv.split_once('=')) - .any(|(k, v)| k == "mode" && v == "memory") - }) - .unwrap_or(false); + // same way sqlx does (form-urlencoded pairs after the first `?`, so an + // encoded key such as `mod%65` is `mode`). + let mode_memory = url.split_once('?').is_some_and(|(_, query)| { + url::form_urlencoded::parse(query.as_bytes()) + .any(|(key, value)| key == "mode" && value == "memory") + }); let filename = options.get_filename().to_string_lossy().into_owned(); if mode_memory || filename == ":memory:" @@ -68,35 +65,25 @@ pub(crate) fn database_file(location: &str) -> Result, String> { } // sqlx opens every name with SQLITE_OPEN_URI, so a `file:` name is a // SQLite URI: `file:/abs`, `file:rel`, `file:///abs`, `file://localhost/abs`, - // each with an optional `?query`. Anything else must be a plain path. + // each with an optional `?query`, and SQLite decodes `%HH` in its path. + // Anything else must be a plain path. let path = match filename.strip_prefix("file:") { - Some(rest) => { - let rest = rest.split('?').next().unwrap_or(rest); - let rest = rest.split('#').next().unwrap_or(rest); - match rest.strip_prefix("//") { - Some(after_slashes) => { - let (authority, path) = after_slashes - .find('/') - .map_or((after_slashes, ""), |i| after_slashes.split_at(i)); - if !(authority.is_empty() || authority == "localhost") { - return Err(format!( - "cannot lock SQLite location {location:?}: `file:` URI authority \ - {authority:?} is not a local path" - )); - } - path.to_owned() - } - None => rest.to_owned(), - } - } - None => filename, + Some(rest) => crate::sqlite_uri::FileUri::parse(rest) + .map_err(|authority| { + format!( + "cannot lock SQLite location {location:?}: `file:` URI authority \ + {authority:?} is not a local path" + ) + })? + .path(), + None => PathBuf::from(filename), }; - if path.is_empty() { + if path.as_os_str().is_empty() { return Err(format!( "cannot lock SQLite location {location:?}: it names no database file" )); } - Ok(Some(PathBuf::from(path))) + Ok(Some(path)) } /// Holds the lock until dropped. @@ -325,6 +312,34 @@ mod database_file_tests { assert!(database_file("file://otherhost/tmp/db.sqlite").is_err()); } + #[test] + fn a_file_uri_path_is_decoded_as_sqlite_decodes_it() { + // sqlx decodes `%25` to `%`; SQLite then decodes `%20` in the path. + assert_eq!( + file("sqlite:file:/tmp/a%2520b.sqlite?mode=rwc"), + Some(PathBuf::from("/tmp/a b.sqlite")) + ); + assert_eq!( + file("file:/tmp/a%2520b.sqlite"), + Some(PathBuf::from("/tmp/a b.sqlite")) + ); + } + + #[test] + fn an_encoded_mode_key_is_read_as_sqlx_reads_it() { + assert_eq!(file("sqlite:///tmp/db.sqlite?mode=rwc&mod%65=memory"), None); + } + + /// sqlx wraps a `file:` name in a second `file:` URI when the query sets + /// `vfs`, so SQLite opens something else; the lock stays where it was. + #[test] + fn a_file_uri_with_a_vfs_keeps_its_lock_path() { + assert_eq!( + file("sqlite:file:/tmp/db.sqlite?mode=rwc&vfs=unix"), + Some(PathBuf::from("/tmp/db.sqlite")) + ); + } + #[test] fn a_disk_file_named_like_a_memory_parameter_is_still_a_file() { assert_eq!( diff --git a/crates/storage-sqlite/src/sqlite_uri.rs b/crates/storage-sqlite/src/sqlite_uri.rs new file mode 100644 index 000000000..628f4a37e --- /dev/null +++ b/crates/storage-sqlite/src/sqlite_uri.rs @@ -0,0 +1,247 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! SQLite's reading of a `file:` URI, ported from `sqlite3ParseUri` in the +//! bundled SQLite (3.46.0), for code that must name the file SQLite opens. +//! +//! After `file:`, a `//` starts an authority, which SQLite accepts only empty +//! or `localhost`. The path runs to the first `?` and the query to the first +//! `#`. The query splits into options at `&`, and each option into a name and +//! a value at its first `=`. `%HH` escapes are decoded in the path, the names +//! and the values, and a decoded character is never a separator. `%00` ends +//! the path, name or value it appears in. An option with an empty name is +//! skipped. + +use std::path::PathBuf; + +/// A `file:` URI as SQLite reads it. +pub(crate) struct FileUri { + /// The path, decoded. + path: Vec, + /// The query options in order, decoded. + options: Vec<(Vec, Vec)>, +} + +/// SQLite's `SQLITE_OPEN_*` access flags. +const READONLY: u32 = 0x1; +const READWRITE: u32 = 0x2; +const CREATE: u32 = 0x4; +const MEMORY: u32 = 0x80; +const ACCESS: u32 = READONLY | READWRITE | CREATE | MEMORY; + +impl FileUri { + /// Read `rest`, the text after `file:`. An authority other than empty or + /// `localhost`, which SQLite refuses, is returned as the error. + pub(crate) fn parse(rest: &str) -> Result { + let uri = rest.as_bytes(); + // A NUL ends the text, as it ends the C string SQLite reads. + let at = |i: usize| uri.get(i).copied().unwrap_or(0); + let mut i = 0; + if uri.starts_with(b"//") { + i = 2; + while at(i) != 0 && at(i) != b'/' { + i += 1; + } + let authority = &rest[2..i]; + if !(authority.is_empty() || authority == "localhost") { + return Err(authority.to_owned()); + } + } + // The path, then each name and value, each followed by a NUL, as + // SQLite lays them out in one buffer. State 0 is the path, 1 a name, + // 2 a value. + let mut out: Vec = Vec::new(); + let mut state = 0; + while at(i) != 0 && at(i) != b'#' { + let mut c = at(i); + i += 1; + if c == b'%' && at(i).is_ascii_hexdigit() && at(i + 1).is_ascii_hexdigit() { + let octet = (hex(at(i)) << 4) | hex(at(i + 1)); + i += 2; + if octet == 0 { + while at(i) != 0 + && at(i) != b'#' + && (state != 0 || at(i) != b'?') + && (state != 1 || (at(i) != b'=' && at(i) != b'&')) + && (state != 2 || at(i) != b'&') + { + i += 1; + } + continue; + } + c = octet; + } else if state == 1 && (c == b'&' || c == b'=') { + if out.last() == Some(&0) { + // An empty name: skip the option. + while at(i) != 0 && at(i) != b'#' && at(i - 1) != b'&' { + i += 1; + } + continue; + } + if c == b'&' { + out.push(0); + } else { + state = 2; + } + c = 0; + } else if (state == 0 && c == b'?') || (state == 2 && c == b'&') { + c = 0; + state = 1; + } + out.push(c); + } + if state == 1 { + out.push(0); + } + out.extend_from_slice(&[0, 0, 0, 0]); + Ok(Self::from_buffer(&out)) + } + + /// Read the buffer the way SQLite does: the path, then name and value + /// pairs until an empty name. + fn from_buffer(out: &[u8]) -> Self { + let mut parts = out.split(|&b| b == 0); + let path = parts.next().unwrap_or_default().to_vec(); + let mut options = Vec::new(); + while let Some(name) = parts.next().filter(|name| !name.is_empty()) { + let value = parts.next().unwrap_or_default(); + options.push((name.to_vec(), value.to_vec())); + } + Self { path, options } + } + + /// The path SQLite opens. + pub(crate) fn path(&self) -> PathBuf { + #[cfg(unix)] + { + use std::os::unix::ffi::OsStringExt; + PathBuf::from(std::ffi::OsString::from_vec(self.path.clone())) + } + #[cfg(not(unix))] + { + PathBuf::from(String::from_utf8_lossy(&self.path).into_owned()) + } + } + + /// Whether SQLite creates the file on disk when it is missing, opened with + /// the read-write-create access sqlx passes for `mode=rwc`, after these + /// options. Every `cache` and `mode` value must be one SQLite knows; a + /// `mode` may not ask for more access than the flags before it allow; the + /// last `mode` decides; and the last `vfs` must be in the unix family. + pub(crate) fn creates_file(&self) -> bool { + let mut flags = READWRITE | CREATE; + let mut vfs: Option<&[u8]> = None; + for (name, value) in &self.options { + match name.as_slice() { + b"vfs" => vfs = Some(value), + b"cache" if !matches!(value.as_slice(), b"shared" | b"private") => return false, + b"mode" => { + let mode = match value.as_slice() { + b"ro" => READONLY, + b"rw" => READWRITE, + b"rwc" => READWRITE | CREATE, + b"memory" => MEMORY, + _ => return false, + }; + if (mode & !MEMORY) > (ACCESS & flags) { + return false; + } + flags = (flags & !ACCESS) | mode; + } + _ => {} + } + } + flags & CREATE != 0 && flags & MEMORY == 0 && vfs.is_none_or(|vfs| vfs.starts_with(b"unix")) + } +} + +fn hex(c: u8) -> u8 { + match c { + b'0'..=b'9' => c - b'0', + b'a'..=b'f' => c - b'a' + 10, + _ => c - b'A' + 10, + } +} + +#[cfg(test)] +mod tests { + use super::FileUri; + use std::path::PathBuf; + + fn parse(rest: &str) -> FileUri { + FileUri::parse(rest).expect(rest) + } + + fn options(rest: &str) -> Vec<(String, String)> { + let text = |b: &[u8]| String::from_utf8(b.to_vec()).expect("UTF-8"); + parse(rest) + .options + .iter() + .map(|(name, value)| (text(name), text(value))) + .collect() + } + + #[test] + fn the_path_is_decoded_and_ends_at_the_query_or_the_fragment() { + assert_eq!( + parse("/d/a%20b.sqlite").path(), + PathBuf::from("/d/a b.sqlite") + ); + assert_eq!(parse("/d/a%3fb?x=1").path(), PathBuf::from("/d/a?b")); + assert_eq!( + parse("/d/x.sqlite#frag?mode=ro").path(), + PathBuf::from("/d/x.sqlite") + ); + assert_eq!(parse("/d/a%00b?mode=ro").path(), PathBuf::from("/d/a")); + assert_eq!( + parse("/d/100%.sqlite").path(), + PathBuf::from("/d/100%.sqlite") + ); + assert_eq!(parse("///d/x").path(), PathBuf::from("/d/x")); + assert_eq!(parse("//localhost/d/x").path(), PathBuf::from("/d/x")); + assert_eq!(parse("rel.sqlite").path(), PathBuf::from("rel.sqlite")); + assert_eq!( + FileUri::parse("//otherhost/d/x").err().as_deref(), + Some("otherhost") + ); + } + + #[test] + fn options_are_split_on_raw_separators_and_decoded() { + assert_eq!( + options("/d?mode=ro&cache=shared&psow"), + [("mode", "ro"), ("cache", "shared"), ("psow", "")].map(|(n, v)| (n.into(), v.into())) + ); + // A decoded `&` or `=` is part of the name or value. + assert_eq!(options("/d?mo%64e=r%26o"), [("mode".into(), "r&o".into())]); + // An empty name is skipped, with its value. + assert_eq!(options("/d?=x&&mode=rw"), [("mode".into(), "rw".into())]); + // `%00` ends the name or value it is in. + assert_eq!( + options("/d?mode=rw%00c&vfs=unix"), + [("mode", "rw"), ("vfs", "unix")].map(|(n, v)| (n.into(), v.into())) + ); + } + + #[test] + fn creates_file_follows_the_last_mode_and_vfs() { + for (rest, creates) in [ + ("/d", true), + ("/d?cache=shared", true), + ("/d?mode=rwc", true), + ("/d?psow=0", true), + ("/d?mode=ro", false), + ("/d?mode=rw", false), + ("/d?mode=memory", false), + ("/d?mode=memory&mode=rwc", true), + // Asking for more access than an earlier mode allows is refused. + ("/d?mode=ro&mode=rwc", false), + ("/d?mode=rwx", false), + ("/d?cache=none", false), + ("/d?vfs=memdb", false), + ("/d?vfs=memdb&vfs=unix-excl", true), + ] { + assert_eq!(parse(rest).creates_file(), creates, "{rest}"); + } + } +} From d65be1f4d9c0af74ee6a02fccb00cf70345509d3 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Sat, 31 Oct 2026 00:07:09 +0000 Subject: [PATCH 11/13] docs: describe the modes set on SQLite database files and the error when one cannot be set The security model left file permissions to the operator and did not mention the SQLite database files, and the troubleshooting guide described settings and verify as commands that only read. With the SQLite backend, init, migrate, settings, verify and serve now create the database file with mode 0600 and set it and its -wal, -shm and -journal files to 0600 when they are wider, and they fail when a mode cannot be set. The security model now names those files. The lock entry in the troubleshooting guide says that settings and verify set these modes although they take no lock. A new entry covers the errors "cannot set the mode of SQLite file" and "cannot set the mode of SQLite database" and their causes: a read-only mount holding files wider than 0600, and database files that belong to another user. --- docs/manuals/10-security-model.md | 2 +- docs/troubleshooting.md | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/docs/manuals/10-security-model.md b/docs/manuals/10-security-model.md index bb7a1c75d..3b6eaaaf1 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -23,7 +23,7 @@ This document describes the security architecture of extenddb, including the thr ### Out of Scope - **PostgreSQL security**: extenddb relies on PostgreSQL access controls and network security. Securing the PostgreSQL instance (firewall rules, TLS, authentication) is the operator's responsibility. -- **Operating system security**: File permissions on `extenddb.toml`, TLS keys, and the PID file are the operator's responsibility. +- **Operating system security**: File permissions on `extenddb.toml`, TLS keys, and the PID file are the operator's responsibility. With the SQLite backend, extenddb creates the database file with mode 0600 and, whenever a command opens the database, sets the database file and its `-wal`, `-shm` and `-journal` files to 0600 if they are wider. `init`, `migrate`, `settings`, `verify` and `serve` fail if a mode cannot be set (see `cannot set the mode of SQLite file` in `docs/troubleshooting.md`). - **Key management**: Access key secrets are encrypted with a locally generated AES key stored in the catalog database. For HSM-grade key management, use a KMS-backed encryption layer at the PostgreSQL level. ## Authentication diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 11a35c1b5..639d4a138 100755 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -95,12 +95,24 @@ extenddb serve --port 19443 --config extenddb.toml # use a different port ### `another extenddb process is already using (lock held on .lock)` -**Cause:** A SQLite database file has one owner at a time. `extenddb serve` takes an exclusive lock on `.lock` (next to the file SQLite opens, after resolving any `sqlite:` or `file:` form, relative path, or symlink) before it opens the database and holds it until it exits. `init` (through to the admin user being written), `migrate`, and `destroy` take the same lock for as long as they run, because they rewrite or remove the file. The backend serializes writers inside one process and recovers interrupted work at startup, so a second server, or a migration or destroy under a running server, would corrupt the first one's work. Read-only commands (`settings`, `manage`, `verify`, `status`) take no lock and run alongside a server. +**Cause:** A SQLite database file has one owner at a time. `extenddb serve` takes an exclusive lock on `.lock` (next to the file SQLite opens, after resolving any `sqlite:` or `file:` form, relative path, or symlink) before it opens the database and holds it until it exits. `init` (through to the admin user being written), `migrate`, and `destroy` take the same lock for as long as they run, because they rewrite or remove the file. The backend serializes writers inside one process and recovers interrupted work at startup, so a second server, or a migration or destroy under a running server, would corrupt the first one's work. Read-only commands (`settings`, `manage`, `verify`, `status`) take no lock and run alongside a server. `settings` and `verify` open the database, and they set the database file and its `-wal`, `-shm` and `-journal` files to mode 0600 when those are wider; they fail if they cannot (see the next entry). **Fix:** Stop the other process (`extenddb stop --config ` for a server), or point this command at a different database (`--sqlite-path` at `init` time). The lock is released by the operating system when the holder exits, including after a crash, so a leftover `.lock` file is harmless and needs no cleanup. **Scope:** The lock is `flock(2)`, taken on Linux and macOS. It is advisory, and on network filesystems (NFS, SMB) its behavior depends on the server and mount options; keep SQLite databases on local disk. On other platforms no lock is taken and the server logs a warning at startup. The directory holding the database must be writable so the lock file can be created; a database in a read-only directory now fails to start with `cannot open lock file`. The lock file is created mode 0600 like the database itself; a world-readable lock file would let any local user hold the lock and keep the server from starting. +### `cannot set the mode of SQLite file : ` or `cannot set the mode of SQLite database : ` + +**Cause:** The SQLite database holds the encryption key next to the secrets it protects, so it must be readable by its owner only. `init`, `migrate`, `settings`, `verify` and `serve` create a missing database file with mode 0600, and set the database file and its `-wal`, `-shm` and `-journal` files to 0600 when they are wider. Only regular files are changed, and a file already at 0600 or narrower is left as it is. A symlink at the database name is followed to the file SQLite opens. The `SQLite file` message means a file wider than 0600 could not be set to 0600; the `SQLite database` message means the command created the database file and could not set its mode. Possible causes: +- The database is on a read-only mount (`Read-only file system (os error 30)`) and its files are wider than 0600, for example files left at 0644 by an earlier version. +- The database files belong to another user (`Operation not permitted (os error 1)`), for example a database shared through group permissions, or a host directory mounted into a container that runs as a different user. + +**Fix:** For a read-only mount, run one command against the same files on a writable mount first, as the user who owns them, for example `extenddb verify --config `. It sets the files to 0600, and later commands on the read-only mount find nothing to change. Or set the modes yourself as the owner: +```bash +chmod 600 extenddb.sqlite extenddb.sqlite-wal extenddb.sqlite-shm +``` +For files that belong to another user, run extenddb as that user. The lock file next to the database has the same requirement for `serve`, `migrate`, `init` and `destroy`. + ### `Failed to load TLS certificates: ` **Cause:** TLS is enabled (the default) but the server could not load the certificate or private key files. Possible causes: From 8bc4fea6530f595c319dafa0ac33f73c8e07cc57 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:01:17 +0000 Subject: [PATCH 12/13] fix(storage-sqlite): compile the URI access flags only on Unix creates_file and the SQLITE_OPEN_* flags it reads are used only by the Unix file mode code, so a Windows build reported seven dead-code warnings for them. They are now compiled on Unix only, and the options field they read allows dead code on other platforms. --- crates/storage-sqlite/src/sqlite_uri.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/storage-sqlite/src/sqlite_uri.rs b/crates/storage-sqlite/src/sqlite_uri.rs index 628f4a37e..01ed3b7fa 100644 --- a/crates/storage-sqlite/src/sqlite_uri.rs +++ b/crates/storage-sqlite/src/sqlite_uri.rs @@ -18,15 +18,22 @@ use std::path::PathBuf; pub(crate) struct FileUri { /// The path, decoded. path: Vec, - /// The query options in order, decoded. + /// The query options in order, decoded. Read only by `creates_file`, + /// which the Unix file mode code uses. + #[cfg_attr(not(unix), allow(dead_code))] options: Vec<(Vec, Vec)>, } /// SQLite's `SQLITE_OPEN_*` access flags. +#[cfg(unix)] const READONLY: u32 = 0x1; +#[cfg(unix)] const READWRITE: u32 = 0x2; +#[cfg(unix)] const CREATE: u32 = 0x4; +#[cfg(unix)] const MEMORY: u32 = 0x80; +#[cfg(unix)] const ACCESS: u32 = READONLY | READWRITE | CREATE | MEMORY; impl FileUri { @@ -128,6 +135,7 @@ impl FileUri { /// options. Every `cache` and `mode` value must be one SQLite knows; a /// `mode` may not ask for more access than the flags before it allow; the /// last `mode` decides; and the last `vfs` must be in the unix family. + #[cfg(unix)] pub(crate) fn creates_file(&self) -> bool { let mut flags = READWRITE | CREATE; let mut vfs: Option<&[u8]> = None; @@ -224,6 +232,7 @@ mod tests { } #[test] + #[cfg(unix)] fn creates_file_follows_the_last_mode_and_vfs() { for (rest, creates) in [ ("/d", true), From 51c2f1f9922fda99cb047270a066c75189b495ba Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:01:17 +0000 Subject: [PATCH 13/13] docs: name every cause of the SQLite file mode errors The SQLite file message also appears when the database file or one of its sidecars cannot be looked up, for example when the user cannot search the directory that holds the database. The SQLite database message comes from a filesystem that refuses to set the mode of the file the command created. The troubleshooting entry now says so, and lists the directory case with its fix. --- docs/troubleshooting.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 639d4a138..7e3b36c85 100755 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -103,15 +103,16 @@ extenddb serve --port 19443 --config extenddb.toml # use a different port ### `cannot set the mode of SQLite file : ` or `cannot set the mode of SQLite database : ` -**Cause:** The SQLite database holds the encryption key next to the secrets it protects, so it must be readable by its owner only. `init`, `migrate`, `settings`, `verify` and `serve` create a missing database file with mode 0600, and set the database file and its `-wal`, `-shm` and `-journal` files to 0600 when they are wider. Only regular files are changed, and a file already at 0600 or narrower is left as it is. A symlink at the database name is followed to the file SQLite opens. The `SQLite file` message means a file wider than 0600 could not be set to 0600; the `SQLite database` message means the command created the database file and could not set its mode. Possible causes: +**Cause:** The SQLite database holds the encryption key next to the secrets it protects, so it must be readable by its owner only. `init`, `migrate`, `settings`, `verify` and `serve` create a missing database file with mode 0600, and set the database file and its `-wal`, `-shm` and `-journal` files to 0600 when they are wider. Only regular files are changed, and a file already at 0600 or narrower is left as it is. A symlink at the database name is followed to the file SQLite opens. The `SQLite file` message means a file wider than 0600 could not be set to 0600, or that the command could not look up the database file or one of its `-wal`, `-shm` or `-journal` files. The `SQLite database` message means the filesystem refused to set the mode of the database file the command created. Causes of the `SQLite file` message: - The database is on a read-only mount (`Read-only file system (os error 30)`) and its files are wider than 0600, for example files left at 0644 by an earlier version. - The database files belong to another user (`Operation not permitted (os error 1)`), for example a database shared through group permissions, or a host directory mounted into a container that runs as a different user. +- The user running the command cannot search the directory that holds the database (`Permission denied (os error 13)`). **Fix:** For a read-only mount, run one command against the same files on a writable mount first, as the user who owns them, for example `extenddb verify --config `. It sets the files to 0600, and later commands on the read-only mount find nothing to change. Or set the modes yourself as the owner: ```bash chmod 600 extenddb.sqlite extenddb.sqlite-wal extenddb.sqlite-shm ``` -For files that belong to another user, run extenddb as that user. The lock file next to the database has the same requirement for `serve`, `migrate`, `init` and `destroy`. +For files that belong to another user, run extenddb as that user. The lock file next to the database has the same requirement for `serve`, `migrate`, `init` and `destroy`. For a directory the user cannot search, run extenddb as a user who can, or give that user search permission on the directory. ### `Failed to load TLS certificates: `