From d955e7fd3b0e03479c37e5d787273bb501e78136 Mon Sep 17 00:00:00 2001 From: Stefan Steiner Date: Sun, 6 Sep 2026 18:16:31 -0700 Subject: [PATCH 1/2] fix(mcp): restrict daemon state files to the owning user The daemon state directory (`~/.hyperdb`, or `HYPERDB_STATE_DIR`) and the files in it took their permissions from the process umask, commonly leaving the directory at 0755 and `daemon.json` at 0644. `daemon.json` records the `hyperd` endpoint, and the `logs/` directory records it too, so both are now restricted to the owning user: 0700 for the directories, 0600 for the discovery file. The mode is set on the atomic write's temp file before any content is written, so the endpoint is never on disk in a world-readable file, and the subsequent rename replaces the target's inode, which also tightens a record an earlier release left readable. Restricting `logs/` as a directory is what covers `hyperd`'s own diagnostic logs: it is a separate process writing under its own umask, so the mode of the files it rotates is not ours to set. A directory left loose by an earlier run is corrected rather than accepted. Failing to tighten one is a warning, not a startup failure, because `chmod` can be refused for reasons unrelated to the contents -- a state directory on a filesystem with no Unix modes, for instance -- and losing the MCP entirely on those setups would be worse than the permissions it was trying to fix. A `daemon.json` whose permissions could not be set is not published at all, since that is the file the endpoint is about to go into. Unix modes have no Windows equivalent, so the mode calls compile out there; Windows relies on the ACL that `%USERPROFILE%` subdirectories inherit, which already excludes other interactive users. Tests assert the mode actually on disk rather than that a `chmod` was attempted, since only the former is what another local account sees. --- hyperdb-mcp/CHANGELOG.md | 21 ++ hyperdb-mcp/src/daemon/discovery.rs | 119 ++++++++- hyperdb-mcp/src/daemon/mod.rs | 1 + hyperdb-mcp/src/daemon/run.rs | 12 +- hyperdb-mcp/src/daemon/state_perms.rs | 343 ++++++++++++++++++++++++++ hyperdb-mcp/src/main.rs | 10 +- 6 files changed, 494 insertions(+), 12 deletions(-) create mode 100644 hyperdb-mcp/src/daemon/state_perms.rs diff --git a/hyperdb-mcp/CHANGELOG.md b/hyperdb-mcp/CHANGELOG.md index 509dfef1..74d980cd 100644 --- a/hyperdb-mcp/CHANGELOG.md +++ b/hyperdb-mcp/CHANGELOG.md @@ -632,6 +632,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/). the file has not committed. Fixes [issue #284](https://github.com/tableau/hyper-api-rust/issues/284). +### Security + +- **Daemon state files are now restricted to the owning user.** The state + directory (`~/.hyperdb`, or `HYPERDB_STATE_DIR`) is created `0700` and + `daemon.json` `0600` on Unix, where previously both took their mode from the + process umask — commonly `0755` and `0644`. `daemon.json` names the `hyperd` + endpoint, so it is owner-only from the moment it exists: the mode is set on + the atomic write's temp file *before* any content is written, and the + subsequent `rename` replaces the target's inode, which also tightens a record + an earlier release left readable. `logs/` gets the same `0700` treatment, + since `hyperd` writes its own diagnostic logs there under its own umask and + those records name the endpoint too — restricting the directory covers files + this process does not own. A directory left loose by an earlier run is + corrected rather than accepted; when the `chmod` itself fails (a state + directory on a filesystem without Unix modes, say) the daemon warns and + carries on, but a `daemon.json` whose permissions could not be set is never + published. Windows relies on the ACL that `%USERPROFILE%` subdirectories + inherit, which already excludes other interactive users. Exposes + `daemon::state_perms::ensure_owner_only_dir` so the binary target can share + the helper with the library. + ## [0.5.0] - 2026-06-07 ### Added diff --git a/hyperdb-mcp/src/daemon/discovery.rs b/hyperdb-mcp/src/daemon/discovery.rs index 259bac04..d3138a07 100644 --- a/hyperdb-mcp/src/daemon/discovery.rs +++ b/hyperdb-mcp/src/daemon/discovery.rs @@ -296,22 +296,25 @@ pub(super) fn write_enriched_discovery_file(info: &DaemonInfo) -> io::Result<()> fn write_discovery_record(record: &(impl Serialize + ?Sized)) -> io::Result<()> { let dir = state_dir()?; - std::fs::create_dir_all(&dir)?; + super::state_perms::ensure_owner_only_dir(&dir)?; let path = dir.join("daemon.json"); let tmp_path = dir.join("daemon.json.tmp"); let json = serde_json::to_string_pretty(record).map_err(|e| io::Error::other(e.to_string()))?; - std::fs::write(&tmp_path, json.as_bytes())?; - // `std::fs::rename` already replaces an existing target atomically on - // both Unix (`rename(2)`) and Windows (`MoveFileExW` with + // Writes into `tmp_path` and renames it onto `path`. `std::fs::rename` + // already replaces an existing target atomically on both Unix + // (`rename(2)`) and Windows (`MoveFileExW` with // `MOVEFILE_REPLACE_EXISTING`, falling back to `SetFileInformationByHandle` - // with `FILE_RENAME_FLAG_REPLACE_IF_EXISTS`). Pre-deleting the target here + // with `FILE_RENAME_FLAG_REPLACE_IF_EXISTS`). Pre-deleting the target // would reintroduce exactly the window this function's doc comment // promises not to have: a concurrent `discover()` could observe the file // as `Missing` mid-restart (see `try_restart_hyperd`, which rewrites this // file on every `hyperd` restart). - std::fs::rename(&tmp_path, &path)?; - Ok(()) + // + // The rename is also what tightens a record an earlier release left + // world-readable, because it replaces the target's inode rather than + // rewriting it in place. + super::state_perms::write_owner_only_atomic(&path, &tmp_path, json.as_bytes()) } /// Read the discovery file and validate that the daemon is still alive. @@ -1561,4 +1564,106 @@ mod tests { failures.join("\n") ); } + + /// The state directory and the discovery file in it both name the `hyperd` + /// endpoint, so both must be restricted to the owning user — including when + /// an earlier release already created them under the process umask. + /// + /// Asserts the mode actually on disk rather than that a `chmod` was + /// attempted, since only the former is what another local account sees. + #[cfg(unix)] + fn run_state_permissions_scenario() { + use std::os::unix::fs::PermissionsExt as _; + + assert!( + std::env::var_os("HYPERDB_STATE_DIR").is_some(), + "child scenario requires an isolated state directory" + ); + + fn mode_of(path: &Path) -> u32 { + std::fs::metadata(path) + .unwrap_or_else(|error| panic!("{} should exist: {error}", path.display())) + .permissions() + .mode() + & 0o777 + } + + let dir = state_dir().unwrap(); + let path = discovery_file_path().unwrap(); + let mut failures = Vec::new(); + + // A state directory and discovery file created from nothing. + write_discovery_file(&legacy_info()).unwrap(); + let fresh_dir_mode = mode_of(&dir); + if fresh_dir_mode != 0o700 { + failures.push(format!( + "a newly created state directory was left at {fresh_dir_mode:04o} instead of \ + 0700, so another local account can traverse it" + )); + } + let fresh_file_mode = mode_of(&path); + if fresh_file_mode != 0o600 { + failures.push(format!( + "a newly written discovery file was left at {fresh_file_mode:04o} instead of \ + 0600, so it hands the hyperd endpoint to any local reader" + )); + } + + // Permissions left loose by an earlier release must be corrected on the + // next write rather than accepted as they are. + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert_eq!( + (mode_of(&dir), mode_of(&path)), + (0o755, 0o644), + "fixture must start world-readable or it does not exercise the correction" + ); + + write_discovery_file(&legacy_info()).unwrap(); + let corrected_dir_mode = mode_of(&dir); + if corrected_dir_mode != 0o700 { + failures.push(format!( + "a pre-existing world-readable state directory stayed at {corrected_dir_mode:04o} \ + instead of being tightened to 0700" + )); + } + let corrected_file_mode = mode_of(&path); + if corrected_file_mode != 0o600 { + failures.push(format!( + "a pre-existing world-readable discovery file stayed at {corrected_file_mode:04o} \ + instead of being tightened to 0600" + )); + } + + // The record must still be readable and intact afterwards: tightening + // permissions is worthless if it breaks the daemon's own discovery. + match serde_json::from_slice::(&std::fs::read(&path).unwrap()) { + Ok(parsed) if parsed == legacy_info() => {} + Ok(_) => failures.push("the restricted record did not round-trip".to_string()), + Err(error) => { + failures.push(format!("the restricted record was unreadable: {error}")); + } + } + + assert!( + failures.is_empty(), + "daemon state permission failures:\n{}", + failures.join("\n") + ); + } + + #[cfg(unix)] + #[test] + fn state_directory_and_discovery_file_are_owner_only() { + const CHILD_SENTINEL_ENV: &str = "HYPERDB_MCP_STATE_PERMISSIONS_CHILD"; + const TEST_NAME: &str = + "daemon::discovery::tests::state_directory_and_discovery_file_are_owner_only"; + + if let Some(marker) = std::env::var_os(CHILD_SENTINEL_ENV) { + std::fs::write(std::path::PathBuf::from(marker), b"started").unwrap(); + run_state_permissions_scenario(); + return; + } + run_discovery_compatibility_child(TEST_NAME, CHILD_SENTINEL_ENV); + } } diff --git a/hyperdb-mcp/src/daemon/mod.rs b/hyperdb-mcp/src/daemon/mod.rs index bdc2a31a..187e65c3 100644 --- a/hyperdb-mcp/src/daemon/mod.rs +++ b/hyperdb-mcp/src/daemon/mod.rs @@ -7,6 +7,7 @@ pub mod discovery; pub mod health; pub mod run; pub mod spawn; +pub mod state_perms; /// Default base TCP port for the daemon health listener. When no env var is set, /// the daemon scans `[base, base + DAEMON_PORT_SCAN_SPAN)` to find a free port. diff --git a/hyperdb-mcp/src/daemon/run.rs b/hyperdb-mcp/src/daemon/run.rs index 4dc91555..6d3481fd 100644 --- a/hyperdb-mcp/src/daemon/run.rs +++ b/hyperdb-mcp/src/daemon/run.rs @@ -196,8 +196,16 @@ pub fn try_record_restart_attempt(history: &mut Vec, now: Instant) -> R /// Build the Parameters used for every hyperd spawn (initial start and restarts). fn build_params() -> std::io::Result { - let log_dir = discovery::state_dir()?.join("logs"); - std::fs::create_dir_all(&log_dir)?; + // The state directory holds `daemon.json`; `logs/` holds `hyperd`'s own + // diagnostic logs, which name the endpoint just as `daemon.json` does. + // `hyperd` is a separate process writing under its own umask, so + // restricting the directory is what covers those files. Both levels are + // restricted here so the daemon's own startup establishes the invariant + // instead of it depending on the later discovery-file write. + let state_dir = discovery::state_dir()?; + super::state_perms::ensure_owner_only_dir(&state_dir)?; + let log_dir = state_dir.join("logs"); + super::state_perms::ensure_owner_only_dir(&log_dir)?; let mut params = Parameters::new(); params.set("log_file_max_count", "2"); diff --git a/hyperdb-mcp/src/daemon/state_perms.rs b/hyperdb-mcp/src/daemon/state_perms.rs new file mode 100644 index 00000000..91e3a77a --- /dev/null +++ b/hyperdb-mcp/src/daemon/state_perms.rs @@ -0,0 +1,343 @@ +// Copyright (c) 2026, Salesforce, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 OR MIT + +//! Owner-only permissions for the daemon state directory and the files in it. +//! +//! The state directory (`~/.hyperdb`, or `HYPERDB_STATE_DIR`) holds the +//! daemon's connection details: `daemon.json` names the `hyperd` endpoint, and +//! `logs/` records it too. Those are the owning user's business, so this module +//! is the single place that decides how they are created. +//! +//! Created with [`std::fs::create_dir_all`] and [`std::fs::write`], both would +//! take their mode from the process umask instead — commonly `0755` and `0644`. +//! The helpers here pin the directory to `0700` and files to `0600`. +//! +//! Restricting the *directory* is what makes `logs/` safe: `hyperd` is a +//! separate process writing its own log files under its own umask, so their +//! individual modes are not ours to set. A `0700` directory settles it for +//! every file inside, whoever wrote it. +//! +//! # Platform behaviour +//! +//! Unix modes have no Windows equivalent, so the mode calls compile out there. +//! Windows leans on ACL inheritance instead: the default state directory sits +//! under `%USERPROFILE%`, whose ACL grants the owning user, `SYSTEM`, and +//! administrators — and *not* other interactive users — and a new subdirectory +//! inherits it. Pointing `HYPERDB_STATE_DIR` outside the profile forfeits that +//! inheritance, which is why the env var is documented as the user's own call. + +use std::io; +use std::path::Path; + +/// Directory mode for state directories: owner-only, including traversal. +#[cfg(unix)] +const STATE_DIR_MODE: u32 = 0o700; + +/// File mode for state files: readable and writable only by the owner. +#[cfg(unix)] +const STATE_FILE_MODE: u32 = 0o600; + +/// Create `dir` and any missing parents, restricted to the owning user. +/// +/// A directory this call creates is restricted from the start. One that already +/// exists — from a release that created it under the process umask — is +/// tightened in place. +/// +/// # Errors +/// Returns an error if the directory cannot be created. Failing to *tighten* an +/// existing directory is logged as a warning and tolerated, because `chmod` can +/// be refused for reasons unrelated to this directory's contents — a state +/// directory on a filesystem with no Unix modes, say — and refusing to start +/// there would be a worse outcome than the permissions it was trying to fix. +pub fn ensure_owner_only_dir(dir: &Path) -> io::Result<()> { + create_owner_only_dir(dir)?; + restrict_existing_dir(dir); + Ok(()) +} + +#[cfg(unix)] +fn create_owner_only_dir(dir: &Path) -> io::Result<()> { + use std::os::unix::fs::DirBuilderExt as _; + + std::fs::DirBuilder::new() + .recursive(true) + .mode(STATE_DIR_MODE) + .create(dir) +} + +#[cfg(not(unix))] +fn create_owner_only_dir(dir: &Path) -> io::Result<()> { + std::fs::create_dir_all(dir) +} + +/// Tighten an existing directory to owner-only access, best effort. +/// +/// Tolerating failure here is deliberate. `chmod` can fail for reasons that +/// have nothing to do with this directory's contents: `HYPERDB_STATE_DIR` may +/// name a path on a filesystem with no Unix modes at all (an SMB or NFS mount, +/// exFAT, some container bind-mounts), where the call is refused however the +/// directory is actually protected. Refusing to start would trade a +/// confidentiality gap the user already had for a total loss of the MCP on +/// those setups — a strictly worse outcome for a local developer tool. +/// +/// The file we are about to *publish* an endpoint into is the opposite case, so +/// [`write_owner_only_atomic`] fails loudly instead. +fn restrict_existing_dir(dir: &Path) { + #[cfg(unix)] + { + if let Err(error) = restrict_existing_unix(dir, STATE_DIR_MODE) { + tracing::warn!( + path = %dir.display(), + %error, + "could not restrict the daemon state directory to the current user; \ + its existing permissions are left in place" + ); + } + } + #[cfg(not(unix))] + { + let _ = dir; + } +} + +/// Reset an existing path's permission bits to `mode`. +/// +/// Skips the `chmod` when the mode already matches, so the steady state costs +/// one `stat`. `permissions().mode()` carries the file-type bits too, hence the +/// mask before comparing. +#[cfg(unix)] +fn restrict_existing_unix(path: &Path, mode: u32) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt as _; + + let metadata = std::fs::metadata(path)?; + if metadata.permissions().mode() & 0o777 == mode { + return Ok(()); + } + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)) +} + +/// Write `contents` to `path` atomically, restricted to the owning user. +/// +/// `tmp_path` receives the content and is then renamed onto `path`, preserving +/// the existing guarantee that a concurrent reader sees either the old record +/// or the new one, never a partial write. The mode is set on `tmp_path` *before +/// any content is written*, so the endpoint is never on disk in a +/// world-readable file — not even briefly. +/// +/// Because `rename` replaces the target's inode rather than its contents, the +/// published file carries this mode even if `path` already existed with looser +/// permissions. That is what corrects a `daemon.json` left behind by an earlier +/// release. +/// +/// # Errors +/// Returns an error if the temp file cannot be created with the intended mode, +/// cannot be written, or cannot be renamed onto `path`. Unlike a directory +/// that merely already exists, this file is one this process is about to +/// publish an endpoint into, so a record whose permissions could not be set is +/// not published at all. +pub(crate) fn write_owner_only_atomic( + path: &Path, + tmp_path: &Path, + contents: &[u8], +) -> io::Result<()> { + use std::io::Write as _; + + let mut options = std::fs::OpenOptions::new(); + options.write(true).create(true).truncate(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(STATE_FILE_MODE); + } + + let mut file = options.open(tmp_path)?; + + // `OpenOptions::mode` applies only to a file this call creates. An + // interrupted earlier write can leave `tmp_path` behind, and reopening + // that file keeps whatever mode it already had, so restrict the open + // handle too. Operating on the descriptor rather than the path also means + // no window in which the name could be swapped for another file. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + file.set_permissions(std::fs::Permissions::from_mode(STATE_FILE_MODE))?; + } + + file.write_all(contents)?; + // Close before renaming: Windows refuses to rename a file that is still + // open for writing. + drop(file); + + std::fs::rename(tmp_path, path) +} + +#[cfg(all(test, unix))] +mod tests { + use std::os::unix::fs::PermissionsExt as _; + + use tempfile::TempDir; + + use super::*; + + fn mode_of(path: &Path) -> u32 { + std::fs::metadata(path) + .expect("fixture path should exist") + .permissions() + .mode() + & 0o777 + } + + #[test] + fn state_directory_is_created_owner_only() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("state"); + + ensure_owner_only_dir(&dir).unwrap(); + + assert_eq!( + mode_of(&dir), + 0o700, + "a freshly created state directory must not be reachable by other users" + ); + } + + #[test] + fn nested_state_directory_is_created_owner_only_at_every_level() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().join("state"); + let nested = root.join("logs"); + + ensure_owner_only_dir(&nested).unwrap(); + + assert_eq!( + mode_of(&nested), + 0o700, + "the leaf state directory must be owner-only" + ); + assert_eq!( + mode_of(&root), + 0o700, + "an intermediate state directory must be owner-only too, or the leaf is still \ + reachable through it" + ); + } + + #[test] + fn a_pre_existing_world_readable_state_directory_is_tightened() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("state"); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert_eq!( + mode_of(&dir), + 0o755, + "fixture must start world-readable or it does not exercise the correction" + ); + + ensure_owner_only_dir(&dir).unwrap(); + + assert_eq!( + mode_of(&dir), + 0o700, + "a state directory left loose by an earlier run must be corrected, not accepted" + ); + } + + /// The shape an upgrade actually meets: both the state directory and the + /// `logs/` directory inside it already exist, world-readable, from a + /// release that created them under the process umask. + #[test] + fn a_pre_existing_world_readable_directory_tree_is_tightened_at_every_level() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().join("state"); + let nested = root.join("logs"); + std::fs::create_dir_all(&nested).unwrap(); + for dir in [&root, &nested] { + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert_eq!( + mode_of(dir), + 0o755, + "fixture must start world-readable or it does not exercise the correction" + ); + } + + // Mirrors the daemon startup order: the state directory, then the log + // directory inside it. + ensure_owner_only_dir(&root).unwrap(); + ensure_owner_only_dir(&nested).unwrap(); + + assert_eq!( + mode_of(&root), + 0o700, + "an existing state directory must be tightened on the next daemon start" + ); + assert_eq!( + mode_of(&nested), + 0o700, + "an existing log directory must be tightened too — it holds hyperd's own logs, \ + which name the endpoint" + ); + } + + #[test] + fn state_file_is_written_owner_only() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + + write_owner_only_atomic(&path, &tmp_path, b"{}").unwrap(); + + assert_eq!( + mode_of(&path), + 0o600, + "a state file naming the endpoint must be readable only by its owner" + ); + assert_eq!(std::fs::read(&path).unwrap(), b"{}"); + assert!( + !tmp_path.exists(), + "the temp file should have been renamed onto the target" + ); + } + + #[test] + fn replacing_a_world_readable_state_file_tightens_it() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + std::fs::write(&path, b"stale").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert_eq!( + mode_of(&path), + 0o644, + "fixture must start world-readable or it does not exercise the correction" + ); + + write_owner_only_atomic(&path, &tmp_path, b"{}").unwrap(); + + assert_eq!( + mode_of(&path), + 0o600, + "rewriting a state file left loose by an earlier release must tighten it" + ); + } + + #[test] + fn a_world_readable_leftover_temp_file_does_not_leak_the_new_record() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + // An interrupted earlier write leaves the temp file behind. Reopening + // it does not re-apply `OpenOptions::mode`, so without an explicit + // restriction the new record would land in a world-readable file. + std::fs::write(&tmp_path, b"interrupted").unwrap(); + std::fs::set_permissions(&tmp_path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + write_owner_only_atomic(&path, &tmp_path, b"{}").unwrap(); + + assert_eq!( + mode_of(&path), + 0o600, + "a leftover temp file must not carry its loose mode into the published record" + ); + } +} diff --git a/hyperdb-mcp/src/main.rs b/hyperdb-mcp/src/main.rs index 9d7a4ec5..3e594cb5 100644 --- a/hyperdb-mcp/src/main.rs +++ b/hyperdb-mcp/src/main.rs @@ -217,9 +217,13 @@ async fn run_daemon_mode( port: u16, idle_timeout: Option, ) -> Result<(), Box> { - // Daemon logs go to ~/.hyperdb/logs/ - let log_dir = discovery::state_dir()?.join("logs"); - std::fs::create_dir_all(&log_dir)?; + // Daemon logs go to ~/.hyperdb/logs/. They record the hyperd endpoint, so + // both the state directory and the log directory inside it are restricted + // to the owning user, as `daemon.json` is. + let state_dir = discovery::state_dir()?; + daemon::state_perms::ensure_owner_only_dir(&state_dir)?; + let log_dir = state_dir.join("logs"); + daemon::state_perms::ensure_owner_only_dir(&log_dir)?; let file_appender = tracing_appender::rolling::never(&log_dir, "hyperdb-daemon.log"); let (file_writer, _file_guard) = tracing_appender::non_blocking(file_appender); From 375dbdec1537d799dedc84b95288059efab61fe8 Mon Sep 17 00:00:00 2001 From: Stefan Steiner Date: Sun, 6 Sep 2026 19:04:04 -0700 Subject: [PATCH 2/2] fix(mcp): keep state-file permissions from breaking mode-less filesystems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The split failure policy defeated itself. Tightening the state *directory* warned and carried on when `chmod` was refused, on the grounds that a filesystem may have no Unix modes to set at all — an SMB or NFS mount, exFAT or vfat, some container bind-mounts, where the mode comes from `fmask`/`dmask` and `chmod` returns `EPERM`. But setting the mode on `daemon.json` propagated that same refusal, and it is the same filesystem failing both calls. So the daemon warned about the directory and then could not publish a discovery file at all, turning a working-if-loose configuration into a hard startup failure — the total loss of the MCP that the directory-side rationale calls strictly worse. A refusal is now reconciled against the mode actually on disk instead of being taken at face value: if the descriptor already reads back with nothing granted to group or other, the record is protected and is published; if it reads back wider, the error stands and nothing is published. The invariant the loud failure existed for is intact, and the two halves of the policy now tell one story. `restrict_open_state_file` takes the mode-setting call as a parameter so both sides of that branch are tested without one of the exotic filesystems they exist for. Also in this change: - The temp file is unlinked and recreated with `O_CREAT | O_EXCL` and `O_NOFOLLOW` rather than opened with a plain create, so the mode always applies to a regular file this call made inside the state directory. A comment claiming the descriptor left "no window in which the name could be swapped" was describing a property the open did not have; closing it makes the claim true rather than softening it. - Regular files already inside a state directory are tightened alongside the directory itself, one level deep, skipping anything that is not a regular file. Restricting the directory closes the path to a new reader but says nothing about a log file already in it, and `logs/` holds the daemon's own log and `hyperd`'s rotated logs, which name the endpoint as `daemon.json` does. Failure warns, as the directory path does. - On Windows, `home_dir()` now prefers `%USERPROFILE%` over `HOME`, keeping `HOME` as a last resort. `state_dir`'s documentation already described that order; the code tried `HOME` first on every platform, so MSYS2, Cygwin and Git Bash — which routinely set `HOME` outside the profile — put the state directory where it does not inherit the profile ACL, without the user having asked for it. - `HYPERDB_STATE_DIR` now carries its permission caveat where a user reads about it, in `README.md` and `DEVELOPMENT.md`, rather than only in a source comment: keep it under the user profile on Windows and on a mode-supporting filesystem on Unix. - `ensure_owner_only_dir`'s doc no longer overstates its reach — it restricts parents it creates, not pre-existing ones — and the new public `daemon::state_perms` surface is listed under Added, not only Security. --- hyperdb-mcp/CHANGELOG.md | 63 +++- hyperdb-mcp/DEVELOPMENT.md | 6 +- hyperdb-mcp/README.md | 13 +- hyperdb-mcp/src/daemon/discovery.rs | 18 +- hyperdb-mcp/src/daemon/state_perms.rs | 417 ++++++++++++++++++++++++-- 5 files changed, 472 insertions(+), 45 deletions(-) diff --git a/hyperdb-mcp/CHANGELOG.md b/hyperdb-mcp/CHANGELOG.md index 74d980cd..747f17cb 100644 --- a/hyperdb-mcp/CHANGELOG.md +++ b/hyperdb-mcp/CHANGELOG.md @@ -100,6 +100,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/). `bar_orientation`, `label_values`, `show_legend`, and positive-only `y_scale`, while preserving the public Rust `ChartOptions` surface and existing rendering defaults. +- **`daemon::state_perms` module, with `state_perms::ensure_owner_only_dir`.** + New public surface on the library target: the single place that decides how + the daemon's state directory and the files in it are created and tightened + (see Security, below). It is public because `hyperdb-mcp`'s `[[bin]]` is a + separate crate to Cargo and sets up the daemon's log directory itself; + `pub(crate)` would not reach it. Like the rest of `daemon::*` it is plumbing + for the binary rather than an API to build on — the library target "is not a + documented API surface" — so it may be narrowed without a breaking change. ### Changed @@ -280,6 +288,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Fixed +- **On Windows, the daemon state directory now resolves from `%USERPROFILE%` + before `HOME`.** `discovery::state_dir` documented `~` as "`HOME` on Unix, + `USERPROFILE` on Windows" but `home_dir()` tried `HOME` first on every + platform. MSYS2, Cygwin and Git Bash routinely set `HOME` to a path of their + own outside the user profile, so under those shells the state directory + landed outside `%USERPROFILE%` and did not inherit its ACL — the only thing + restricting these files on Windows — without the user having set + `HYPERDB_STATE_DIR` or otherwise asked for it. Windows now prefers + `USERPROFILE`, keeps `HOME` as a last resort so a machine that resolved + before still resolves, and matches `paths::persistent_home_dir`. Unix + resolution is unchanged. **Behaviour change on Windows:** a shell that sets + both to different paths now gets the profile-relative state directory, so a + daemon started before this change may not be discovered by a client started + after it until the old one is stopped. - **Daemon port `0` is now rejected at both entry points, instead of quietly multiplying daemons.** `--port` promises an exact bind and `"0".parse::()` succeeds, so `0` passed validation at both the flag and @@ -643,15 +665,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/). subsequent `rename` replaces the target's inode, which also tightens a record an earlier release left readable. `logs/` gets the same `0700` treatment, since `hyperd` writes its own diagnostic logs there under its own umask and - those records name the endpoint too — restricting the directory covers files - this process does not own. A directory left loose by an earlier run is - corrected rather than accepted; when the `chmod` itself fails (a state - directory on a filesystem without Unix modes, say) the daemon warns and - carries on, but a `daemon.json` whose permissions could not be set is never - published. Windows relies on the ACL that `%USERPROFILE%` subdirectories - inherit, which already excludes other interactive users. Exposes - `daemon::state_perms::ensure_owner_only_dir` so the binary target can share - the helper with the library. + those records name the endpoint too — restricting the directory covers a + file this process does not own and has not seen yet. Directories and the + regular files directly inside them are both corrected when an earlier run + left them loose, rather than accepted as-is: closing the directory does + nothing about a log file already in it, and `logs/` is where the daemon's own + log and `hyperd`'s rotated logs sit. The sweep is one level deep, skips + anything that is not a regular file, and warns rather than failing, so it + cannot follow a link out of the directory or walk into whatever + `HYPERDB_STATE_DIR` names. +- **The discovery file's temp file is created exclusively, so the mode always + applies to a file of our own.** `write_discovery_record` wrote + `daemon.json.tmp` with a plain create, which opens whatever already bears + that name and leaves the intended mode dependent on what that turns out to + be — a leftover temp file kept its own mode, and a name that resolved + somewhere else was written through. It is now unlinked and recreated with + `O_CREAT | O_EXCL` and `O_NOFOLLOW`, so the record always lands on a regular + file inside the state directory, created with the mode it is meant to have; + if the name cannot be created cleanly the write fails rather than publishing + anyway. +- **A filesystem that cannot represent Unix modes no longer costs the daemon + its discovery file.** `chmod` is refused outright on a mount whose + permissions come from `fmask`/`dmask` rather than from each file — SMB or + NFS, exFAT or vfat, some container bind-mounts. Tightening the *directory* + already warned and carried on there, but setting the mode on `daemon.json` + propagated the refusal, so on exactly those filesystems the daemon warned + about the directory and then could not publish a record at all — turning a + working-if-loose setup into a startup failure. A refusal is now reconciled + against the mode actually on disk: if the file already reads back with + nothing granted to group or other, the record is protected and is published; + if it reads back wider, the error stands and nothing is published. Windows + relies on the ACL that `%USERPROFILE%` subdirectories inherit, which already + excludes other interactive users. ## [0.5.0] - 2026-06-07 diff --git a/hyperdb-mcp/DEVELOPMENT.md b/hyperdb-mcp/DEVELOPMENT.md index 3c91b0cb..6fe598b8 100644 --- a/hyperdb-mcp/DEVELOPMENT.md +++ b/hyperdb-mcp/DEVELOPMENT.md @@ -234,7 +234,11 @@ Logs land next to the persistent file when one is supplied (so users find them i `Engine::new` defaults to *daemon mode* — it tries `daemon::spawn::ensure_daemon(resolve_port_scan())` first, which discovers an existing daemon via `~/.hyperdb/daemon.json` (overridable via -`HYPERDB_STATE_DIR`), else scans the port range for a running daemon, else +`HYPERDB_STATE_DIR` — see `daemon::state_perms`, which restricts that directory +and the files in it to the owning user, and which needs the override to name a +path that can carry those permissions: inside `%USERPROFILE%` on Windows, on a +mode-supporting filesystem on Unix), else scans the port range for a running +daemon, else auto-spawns one on the first free port as a detached background process. The Engine then connects via TCP (`Connection::connect(endpoint, …)`) without owning any `HyperProcess`, and records the daemon's `health_port` so the diff --git a/hyperdb-mcp/README.md b/hyperdb-mcp/README.md index fb394dee..ace99ebe 100644 --- a/hyperdb-mcp/README.md +++ b/hyperdb-mcp/README.md @@ -286,6 +286,15 @@ hyperdb-mcp daemon # Run as a daemon explicitly (rarely needed) `status` and `stop` locate the running daemon automatically (reading `daemon.json`, then scanning the port range), so they work even if the daemon scanned onto a non-default port. Pass `--port ` to target a specific port explicitly. State files live at `~/.hyperdb/` by default (override with `HYPERDB_STATE_DIR`). +They record the `hyperd` endpoint, so the daemon restricts them to your own +account: `0700` on the directories and `0600` on `daemon.json` on Unix, and on +Windows the ACL a `%USERPROFILE%` subdirectory inherits. If you override the +location, keep it somewhere that can carry those permissions — under your user +profile on Windows, and on a filesystem that supports Unix modes on Unix (a +network share or a FAT/exFAT volume takes its modes from mount options +instead). The daemon warns rather than refusing to start when it cannot tighten +the directory, but it will not publish `daemon.json` into a file it cannot keep +readable by you alone. For installation and configuration diagnostics that also work before MCP can start, use the native doctor command: @@ -970,7 +979,9 @@ Environment: HYPERD_PATH Hyperd executable or containing directory; when absent or non-UTF-8, walk upward for .hyperd/current/hyperd (no PATH lookup) HYPERDB_PERSISTENT_DB Override the default persistent-db path - HYPERDB_STATE_DIR Override daemon state directory (default ~/.hyperdb/) + HYPERDB_STATE_DIR Override daemon state directory (default ~/.hyperdb/); keep it + under your user profile on Windows and on a filesystem with Unix + modes on Unix, or it cannot be restricted to your account HYPERDB_DAEMON_PORT Pin auto-spawn discovery to one health/lock candidate; foreground startup binds this configured/base port exactly HYPERDB_DAEMON_IDLE_TIMEOUT Opt into idle shutdown (seconds); default: stay resident diff --git a/hyperdb-mcp/src/daemon/discovery.rs b/hyperdb-mcp/src/daemon/discovery.rs index d3138a07..1eea80c3 100644 --- a/hyperdb-mcp/src/daemon/discovery.rs +++ b/hyperdb-mcp/src/daemon/discovery.rs @@ -454,8 +454,22 @@ pub fn resolve_port() -> u16 { } /// Cross-platform home directory resolution. +/// +/// On Windows the user profile is consulted first. MSYS2, Cygwin and Git Bash +/// commonly set `HOME` to a path of their own outside `%USERPROFILE%`, and +/// preferring it would put the state directory outside the profile — losing the +/// inherited ACL that is the whole of the Windows protection for these files +/// (see [`super::state_perms`]) without the user having asked for it. `HOME` +/// stays as a last resort there, so a machine that resolved before still +/// resolves. This is also the order the documentation above already described, +/// and it matches `crate::paths::persistent_home_dir`. fn home_dir() -> Option { - // Try HOME (Unix) then USERPROFILE (Windows) + if cfg!(windows) { + return std::env::var_os("USERPROFILE") + .filter(|profile| !profile.is_empty()) + .or_else(|| std::env::var_os("HOME")) + .map(PathBuf::from); + } std::env::var_os("HOME") .or_else(|| std::env::var_os("USERPROFILE")) .map(PathBuf::from) @@ -1605,7 +1619,7 @@ mod tests { if fresh_file_mode != 0o600 { failures.push(format!( "a newly written discovery file was left at {fresh_file_mode:04o} instead of \ - 0600, so it hands the hyperd endpoint to any local reader" + 0600, so it is readable by other local accounts" )); } diff --git a/hyperdb-mcp/src/daemon/state_perms.rs b/hyperdb-mcp/src/daemon/state_perms.rs index 91e3a77a..9089f8ce 100644 --- a/hyperdb-mcp/src/daemon/state_perms.rs +++ b/hyperdb-mcp/src/daemon/state_perms.rs @@ -12,10 +12,13 @@ //! take their mode from the process umask instead — commonly `0755` and `0644`. //! The helpers here pin the directory to `0700` and files to `0600`. //! -//! Restricting the *directory* is what makes `logs/` safe: `hyperd` is a -//! separate process writing its own log files under its own umask, so their -//! individual modes are not ours to set. A `0700` directory settles it for -//! every file inside, whoever wrote it. +//! Restricting the *directory* is the main lever on `logs/`: `hyperd` is a +//! separate process writing its own log files under its own umask, so the mode +//! it gives a freshly rotated log is not ours to choose, and a `0700` directory +//! settles the path to every file inside, whoever wrote it. Files already +//! present are tightened too, best effort — closing the directory does nothing +//! about a descriptor or hard link taken while it was still open, which is the +//! state an upgrade actually meets. //! //! # Platform behaviour //! @@ -23,8 +26,17 @@ //! Windows leans on ACL inheritance instead: the default state directory sits //! under `%USERPROFILE%`, whose ACL grants the owning user, `SYSTEM`, and //! administrators — and *not* other interactive users — and a new subdirectory -//! inherits it. Pointing `HYPERDB_STATE_DIR` outside the profile forfeits that -//! inheritance, which is why the env var is documented as the user's own call. +//! inherits it. +//! +//! ACL inheritance is therefore the whole of the Windows protection, and it is +//! positional: a state directory outside the profile does not get it. Two +//! things can move it there, and they are not equivalent. Setting +//! `HYPERDB_STATE_DIR` is the user's own call, so it is documented as such +//! beside the variable in `README.md` rather than second-guessed here. +//! Inheriting a `HOME` from an MSYS2, Cygwin or Git Bash shell is *not* a +//! choice about this directory at all, so +//! [`super::discovery::state_dir`]'s resolution prefers `%USERPROFILE%` on +//! Windows and keeps `HOME` only as a last resort. use std::io; use std::path::Path; @@ -34,24 +46,28 @@ use std::path::Path; const STATE_DIR_MODE: u32 = 0o700; /// File mode for state files: readable and writable only by the owner. -#[cfg(unix)] const STATE_FILE_MODE: u32 = 0o600; -/// Create `dir` and any missing parents, restricted to the owning user. +/// Create `dir`, and any parents this call creates, restricted to the owning +/// user. /// -/// A directory this call creates is restricted from the start. One that already -/// exists — from a release that created it under the process umask — is -/// tightened in place. +/// A directory this call creates is restricted from the start, at every level. +/// A *pre-existing* `dir` — from a release that created it under the process +/// umask — is tightened in place, as are the regular files directly inside it; +/// a pre-existing **parent** is left alone, so a caller that cares about an +/// ancestor must pass it too. Both call sites do, naming the state directory +/// and the `logs/` directory in it explicitly. /// /// # Errors /// Returns an error if the directory cannot be created. Failing to *tighten* an -/// existing directory is logged as a warning and tolerated, because `chmod` can -/// be refused for reasons unrelated to this directory's contents — a state +/// existing directory or file is logged as a warning and tolerated, because +/// `chmod` can be refused for reasons unrelated to the contents — a state /// directory on a filesystem with no Unix modes, say — and refusing to start /// there would be a worse outcome than the permissions it was trying to fix. pub fn ensure_owner_only_dir(dir: &Path) -> io::Result<()> { create_owner_only_dir(dir)?; restrict_existing_dir(dir); + restrict_existing_files(dir); Ok(()) } @@ -80,8 +96,13 @@ fn create_owner_only_dir(dir: &Path) -> io::Result<()> { /// confidentiality gap the user already had for a total loss of the MCP on /// those setups — a strictly worse outcome for a local developer tool. /// -/// The file we are about to *publish* an endpoint into is the opposite case, so -/// [`write_owner_only_atomic`] fails loudly instead. +/// The file we are about to *publish* an endpoint into is held to more than +/// this: [`write_owner_only_atomic`] tolerates the same refusal only when the +/// mode already on disk is owner-only anyway, and fails loudly when it is not. +/// The two halves have to agree, because the filesystems that refuse `chmod` +/// on a directory refuse it on the file inside as well — tolerating one and +/// hard-failing the other would leave the daemon warning about the directory +/// and then unable to publish a record at all. fn restrict_existing_dir(dir: &Path) { #[cfg(unix)] { @@ -100,6 +121,62 @@ fn restrict_existing_dir(dir: &Path) { } } +/// Tighten the regular files directly inside `dir` to owner-only, best effort. +/// +/// Restricting the directory closes the path to a *new* reader, but a file +/// already inside keeps the mode it was created with, and anything that +/// reached it before the directory was tightened — an open descriptor, a hard +/// link — still reaches it afterwards. An upgrade over a state directory an +/// earlier release created under the umask therefore has files to correct and +/// not just directories: the daemon's own log and `hyperd`'s rotated logs name +/// the endpoint, as `daemon.json` does. +/// +/// One level, and regular files only. Both call sites pass `logs/` as well as +/// the state directory, so there is nothing deeper that this module puts the +/// endpoint into, and descending further would take a permission sweep into +/// whatever a user pointed `HYPERDB_STATE_DIR` at. Symlinks are skipped rather +/// than followed, since the target is not this directory's business. +/// +/// Failure is tolerated exactly as in [`restrict_existing_dir`], and for the +/// same reason. +fn restrict_existing_files(dir: &Path) { + #[cfg(unix)] + { + let entries = match std::fs::read_dir(dir) { + Ok(entries) => entries, + Err(error) => { + tracing::warn!( + path = %dir.display(), + %error, + "could not list the daemon state directory to restrict the files in it; \ + their existing permissions are left in place" + ); + return; + } + }; + for entry in entries.flatten() { + // From the directory read, so this reports a symlink as a symlink + // rather than as whatever it points at. + if !entry.file_type().is_ok_and(|kind| kind.is_file()) { + continue; + } + let path = entry.path(); + if let Err(error) = restrict_existing_unix(&path, STATE_FILE_MODE) { + tracing::warn!( + path = %path.display(), + %error, + "could not restrict a file in the daemon state directory to the current \ + user; its existing permissions are left in place" + ); + } + } + } + #[cfg(not(unix))] + { + let _ = dir; + } +} + /// Reset an existing path's permission bits to `mode`. /// /// Skips the `chmod` when the mode already matches, so the steady state costs @@ -129,39 +206,92 @@ fn restrict_existing_unix(path: &Path, mode: u32) -> io::Result<()> { /// permissions. That is what corrects a `daemon.json` left behind by an earlier /// release. /// +/// `tmp_path` is unlinked and created exclusively rather than reopened, so the +/// file the mode lands on is always one this call made — not a temp file an +/// interrupted earlier write left behind, and not another entry that happens +/// to bear the name. +/// /// # Errors /// Returns an error if the temp file cannot be created with the intended mode, /// cannot be written, or cannot be renamed onto `path`. Unlike a directory /// that merely already exists, this file is one this process is about to -/// publish an endpoint into, so a record whose permissions could not be set is -/// not published at all. +/// publish an endpoint into, so a record left readable beyond its owner is not +/// published at all — see [`restrict_open_state_file`] for the one refusal +/// that is tolerated, and why it does not weaken that. pub(crate) fn write_owner_only_atomic( path: &Path, tmp_path: &Path, contents: &[u8], +) -> io::Result<()> { + write_owner_only_atomic_with(path, tmp_path, contents, chmod_descriptor) +} + +/// How an open state file's mode is set. +/// +/// A parameter only so the mode-less-filesystem branch of +/// [`restrict_open_state_file`] can be exercised without one of the exotic +/// filesystems it exists for. Production always passes [`chmod_descriptor`]. +type ChmodFn = fn(&std::fs::File, u32) -> io::Result<()>; + +#[cfg(unix)] +fn chmod_descriptor(file: &std::fs::File, mode: u32) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt as _; + + file.set_permissions(std::fs::Permissions::from_mode(mode)) +} + +// `allow` rather than `expect`: this is the only configuration in which the +// lint fires, and it is not one the Unix development host can compile, so an +// unfulfilled expectation could not be caught here. +#[cfg(not(unix))] +#[allow( + clippy::unnecessary_wraps, + reason = "the signature is fixed by `ChmodFn`, whose Unix implementation can fail; there \ + is no mode to set here, so this reports success" +)] +fn chmod_descriptor(_file: &std::fs::File, _mode: u32) -> io::Result<()> { + Ok(()) +} + +fn write_owner_only_atomic_with( + path: &Path, + tmp_path: &Path, + contents: &[u8], + chmod: ChmodFn, ) -> io::Result<()> { use std::io::Write as _; + // An interrupted earlier write can leave `tmp_path` behind. Unlink it + // instead of reopening it, so the exclusive create below always gets a + // file this call made and `OpenOptions::mode` always applies. `remove_file` + // unlinks the name, so a leftover symlink goes rather than its target. + match std::fs::remove_file(tmp_path) { + Ok(()) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + let mut options = std::fs::OpenOptions::new(); - options.write(true).create(true).truncate(true); + // `create_new` is `O_CREAT | O_EXCL`, which fails on an existing entry of + // this name rather than opening it — a symlink included, since `O_EXCL` + // refuses one even when it dangles. `O_NOFOLLOW` states the same intent + // outright. Should something reappear at the name between the unlink and + // the open, the open is what fails, so the record is not published rather + // than published somewhere else. + options.write(true).create_new(true); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt as _; - options.mode(STATE_FILE_MODE); + options.mode(STATE_FILE_MODE).custom_flags(libc::O_NOFOLLOW); } let mut file = options.open(tmp_path)?; - // `OpenOptions::mode` applies only to a file this call creates. An - // interrupted earlier write can leave `tmp_path` behind, and reopening - // that file keeps whatever mode it already had, so restrict the open - // handle too. Operating on the descriptor rather than the path also means - // no window in which the name could be swapped for another file. - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt as _; - file.set_permissions(std::fs::Permissions::from_mode(STATE_FILE_MODE))?; - } + // `OpenOptions::mode` is a request, not a guarantee: the process umask + // clears bits from it, and a filesystem that stores no modes ignores it. + // Restricting the descriptor pins the result, and does so before any + // content is written. + restrict_open_state_file(&file, chmod)?; file.write_all(contents)?; // Close before renaming: Windows refuses to rename a file that is still @@ -171,6 +301,68 @@ pub(crate) fn write_owner_only_atomic( std::fs::rename(tmp_path, path) } +/// Restrict an open state file to [`STATE_FILE_MODE`], distinguishing "the +/// mode could not be set" from "this filesystem does not have modes to set". +/// +/// A filesystem whose permissions come from mount options rather than from +/// each file — an SMB or NFS mount, exFAT or vfat, some container bind-mounts +/// — refuses `chmod` outright while still reporting a mode, taken from +/// `fmask`. Propagating that refusal would leave the daemon unable to publish +/// `daemon.json` at all on those setups, which is the total loss of the MCP +/// that [`restrict_existing_dir`] declines to cause one directory earlier: the +/// same filesystem fails both calls, so warning about the directory and then +/// failing on the file would be the worst of both policies. +/// +/// So a refusal is reconciled against what is actually on disk. If the +/// descriptor already reads back owner-only, the record is protected — the +/// filesystem simply reports a fixed mode — and the write proceeds. If it +/// reads back wider, nothing has protected the record, the error stands, and +/// it is not published. That keeps the invariant the loud failure existed for +/// without breaking a configuration that was already safe. +fn restrict_open_state_file(file: &std::fs::File, chmod: ChmodFn) -> io::Result<()> { + match chmod(file, STATE_FILE_MODE) { + Ok(()) => Ok(()), + Err(error) => reconcile_refused_chmod(file, error), + } +} + +#[cfg(unix)] +fn reconcile_refused_chmod(file: &std::fs::File, error: io::Error) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt as _; + + let observed = file.metadata()?.permissions().mode() & 0o777; + if !is_owner_only(observed) { + return Err(error); + } + tracing::warn!( + %error, + mode = format!("{observed:04o}"), + "could not set the mode on the daemon state file; the filesystem reports it as \ + owner-only already, so the record is published with the mode it has" + ); + Ok(()) +} + +#[cfg(not(unix))] +fn reconcile_refused_chmod(_file: &std::fs::File, error: io::Error) -> io::Result<()> { + Err(error) +} + +/// Whether `mode` grants nothing to group or to other. +/// +/// Only the group and other bits are consulted: owner-execute makes no +/// difference to who besides the owner can read the record, and a filesystem +/// reporting a fixed mode may well set it. +#[cfg(unix)] +#[expect( + clippy::verbose_bit_mask, + reason = "`trailing_zeros() >= 6` is the same test but says nothing about permissions; \ + the octal mask names the group and other bits it clears" +)] +fn is_owner_only(mode: u32) -> bool { + mode & 0o077 == 0 +} + #[cfg(all(test, unix))] mod tests { use std::os::unix::fs::PermissionsExt as _; @@ -187,6 +379,21 @@ mod tests { & 0o777 } + /// Stands in for a filesystem that refuses `chmod` outright but already + /// reports the file as owner-only, the way a mount whose `fmask` denies + /// group and other does. + fn refuse_chmod(_file: &std::fs::File, _mode: u32) -> io::Result<()> { + Err(io::Error::from_raw_os_error(libc::EPERM)) + } + + /// Stands in for a filesystem that refuses `chmod` and reports a mode + /// wider than the one asked for. + fn refuse_chmod_over_a_wider_mode(file: &std::fs::File, _mode: u32) -> io::Result<()> { + file.set_permissions(std::fs::Permissions::from_mode(0o644)) + .expect("the fixture filesystem must support widening the temp file"); + Err(io::Error::from_raw_os_error(libc::EPERM)) + } + #[test] fn state_directory_is_created_owner_only() { let tmp = TempDir::new().unwrap(); @@ -279,6 +486,78 @@ mod tests { ); } + /// Tightening the directory closes the path to a new reader, but says + /// nothing about a file already inside it or about anything already + /// holding that file open. The log files an earlier release left in + /// `logs/` name the endpoint, so they are corrected too. + #[test] + fn pre_existing_files_in_a_loose_state_directory_are_tightened() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("logs"); + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + let group_readable = dir.join("hyperd-0.log"); + let world_readable = dir.join("hyperdb-daemon.log"); + for (file, mode) in [(&group_readable, 0o660), (&world_readable, 0o644)] { + std::fs::write(file, b"endpoint").unwrap(); + std::fs::set_permissions(file, std::fs::Permissions::from_mode(mode)).unwrap(); + assert_eq!( + mode_of(file), + mode, + "fixture must start readable beyond its owner or it does not exercise the \ + correction" + ); + } + + ensure_owner_only_dir(&dir).unwrap(); + + assert_eq!(mode_of(&dir), 0o700); + for file in [&group_readable, &world_readable] { + assert_eq!( + mode_of(file), + 0o600, + "a log file left readable beyond its owner must be corrected, not just \ + covered by the directory" + ); + assert_eq!( + std::fs::read(file).unwrap(), + b"endpoint", + "tightening a log file must not disturb its contents" + ); + } + } + + /// The sweep stays one level deep and leaves anything that is not a + /// regular file alone, so it cannot walk out of the state directory + /// through a link or descend into whatever `HYPERDB_STATE_DIR` names. + #[test] + fn the_file_sweep_does_not_follow_links_or_descend() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("state"); + let nested = dir.join("nested"); + std::fs::create_dir_all(&nested).unwrap(); + let outside = tmp.path().join("outside.txt"); + std::fs::write(&outside, b"unrelated").unwrap(); + std::fs::set_permissions(&outside, std::fs::Permissions::from_mode(0o644)).unwrap(); + std::os::unix::fs::symlink(&outside, dir.join("link.json")).unwrap(); + let deeper = nested.join("deeper.log"); + std::fs::write(&deeper, b"deeper").unwrap(); + std::fs::set_permissions(&deeper, std::fs::Permissions::from_mode(0o644)).unwrap(); + + ensure_owner_only_dir(&dir).unwrap(); + + assert_eq!( + mode_of(&outside), + 0o644, + "the sweep must not follow a link out of the state directory" + ); + assert_eq!( + mode_of(&deeper), + 0o644, + "the sweep must not descend; callers name each directory they care about" + ); + } + #[test] fn state_file_is_written_owner_only() { let tmp = TempDir::new().unwrap(); @@ -326,9 +605,9 @@ mod tests { let tmp = TempDir::new().unwrap(); let path = tmp.path().join("daemon.json"); let tmp_path = tmp.path().join("daemon.json.tmp"); - // An interrupted earlier write leaves the temp file behind. Reopening - // it does not re-apply `OpenOptions::mode`, so without an explicit - // restriction the new record would land in a world-readable file. + // An interrupted earlier write leaves the temp file behind, carrying + // whatever mode it was created with. Writing into that file as-is + // would put the new record in a world-readable file. std::fs::write(&tmp_path, b"interrupted").unwrap(); std::fs::set_permissions(&tmp_path, std::fs::Permissions::from_mode(0o644)).unwrap(); @@ -340,4 +619,78 @@ mod tests { "a leftover temp file must not carry its loose mode into the published record" ); } + + /// The temp file must be one this call created, not whatever its name + /// already resolves to. A stale link of that name is replaced rather than + /// followed: otherwise the record would be written wherever the link + /// points — outside the state directory, where none of this module's + /// permissions apply — and `daemon.json` would end up as a link instead of + /// the record. + #[test] + fn a_symlinked_temp_path_is_replaced_rather_than_followed() { + let tmp = TempDir::new().unwrap(); + let state = tmp.path().join("state"); + std::fs::create_dir(&state).unwrap(); + let unrelated = tmp.path().join("unrelated.txt"); + std::fs::write(&unrelated, b"unrelated contents").unwrap(); + + let path = state.join("daemon.json"); + let tmp_path = state.join("daemon.json.tmp"); + std::os::unix::fs::symlink(&unrelated, &tmp_path).unwrap(); + + write_owner_only_atomic(&path, &tmp_path, b"{}").unwrap(); + + assert_eq!( + std::fs::read(&unrelated).unwrap(), + b"unrelated contents", + "a stale link at the temp path must not redirect the record out of the state directory" + ); + assert!( + std::fs::symlink_metadata(&path) + .unwrap() + .file_type() + .is_file(), + "the published record must be a regular file, not a link left in place" + ); + assert_eq!(std::fs::read(&path).unwrap(), b"{}"); + assert_eq!(mode_of(&path), 0o600); + } + + /// A filesystem whose modes come from mount options rather than from each + /// file — an SMB or NFS mount, exFAT, some container bind-mounts — refuses + /// `chmod` while still reporting a mode. When the mode it reports is + /// already owner-only, the record is protected, and refusing to publish it + /// would cost the user the MCP over a permission that is in fact correct. + #[test] + fn a_refused_chmod_still_publishes_a_record_that_is_already_owner_only() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + + write_owner_only_atomic_with(&path, &tmp_path, b"{}", refuse_chmod).expect( + "a record the filesystem already reports as owner-only must still be published", + ); + + assert_eq!(mode_of(&path), 0o600); + assert_eq!(std::fs::read(&path).unwrap(), b"{}"); + } + + /// The converse, which is the invariant the tolerance must not cost us: a + /// refused `chmod` over a mode that really is wider must not publish. + #[test] + fn a_refused_chmod_over_a_wider_mode_publishes_nothing() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("daemon.json"); + let tmp_path = tmp.path().join("daemon.json.tmp"); + + let error = + write_owner_only_atomic_with(&path, &tmp_path, b"{}", refuse_chmod_over_a_wider_mode) + .expect_err("a record whose mode could not be restricted must not be published"); + + assert_eq!(error.raw_os_error(), Some(libc::EPERM)); + assert!( + !path.exists(), + "no record must be published when its mode could not be restricted" + ); + } }