Skip to content

fix(mcp): restrict daemon state files to the owning user - #295

Merged
StefanSteiner merged 2 commits into
tableau:mainfrom
StefanSteiner:security/daemon-state-file-perms
Sep 7, 2026
Merged

fix(mcp): restrict daemon state files to the owning user#295
StefanSteiner merged 2 commits into
tableau:mainfrom
StefanSteiner:security/daemon-state-file-perms

Conversation

@StefanSteiner

@StefanSteiner StefanSteiner commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What

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 logs/ records it too, so both are now restricted to the owning user: 0700 for the directories, 0600 for the discovery file and for the files already inside.

How

A new daemon::state_perms module is the single place that decides how state paths are created, so the policy is not scattered across the three call sites that create them (discovery::write_discovery_record, daemon::run, and the main.rs daemon-mode log setup).

  • The mode is set before any content is written. write_owner_only_atomic opens the temp file with OpenOptions::mode(0o600) and restricts the open descriptor as well, so the endpoint is never on disk in a world-readable file — not even briefly.
  • The temp file is one this call made. It is unlinked and recreated with O_CREAT | O_EXCL and O_NOFOLLOW rather than opened with a plain create, which would open whatever already bore the name and leave the intended mode dependent on what that turned out to be. The record therefore always lands on a regular file inside the state directory, with the mode it is meant to have; a temp file left behind by an interrupted earlier write no longer carries its own mode into the record. Restricting the descriptor still matters, because OpenOptions::mode is a request the umask can narrow and a mode-less filesystem can ignore.
  • The existing atomic-rename contract is preserved. The temp file is still renamed onto daemon.json, so a concurrent discover() sees either the old record or the new one. Because rename replaces the target's inode rather than rewriting it, the published file carries 0600 even when the target already existed with looser permissions — that is what corrects a record left behind by an earlier release.
  • logs/ is restricted as a directory, and the files already in it are tightened too. hyperd is a separate process writing its own diagnostic logs there under its own umask, so the mode of a log it rotates next is not ours to choose; a 0700 directory settles the path for every file inside, whoever wrote it. But closing the directory says nothing about a log file already present, or about anything already holding one open, which is the state an upgrade actually meets — so regular files directly inside are corrected as well. The sweep is one level deep and skips anything that is not a regular file, so it cannot follow a link out of the directory or descend into whatever HYPERDB_STATE_DIR names.
  • Directories left loose by an earlier run are corrected, not accepted as-is, at both levels — the state directory and the logs/ directory inside it.

Failure behaviour

chmod is refused outright on 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. That is not a statement about this directory's contents, and refusing to start there would trade a pre-existing condition for a total loss of the MCP on those setups: worse than what it was trying to fix, for a local developer tool. So tightening an existing directory warns and carries on.

The same filesystem refuses the call on the file as well, so simply propagating that refusal would leave the daemon warning about the directory and then unable to publish daemon.json at all — the same total loss, reached one step later. Instead the refusal is reconciled against the mode actually on disk:

  • if the file already reads back with nothing granted to group or other, the record is protected — the filesystem is reporting a fixed mode — and it is published;
  • if it reads back wider, nothing has protected the record, the error stands, and it is not published.

So "never publish a record whose permissions could not be set" is kept as an invariant about the mode that is really in force, rather than about whether a syscall succeeded.

Platforms

Unix modes have no Windows equivalent, so the mode calls compile out there. Windows relies 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.

That inheritance is positional, so two things can move the state directory out of the profile, and they are not equivalent:

  • Setting HYPERDB_STATE_DIR is the user's own call — now documented as such beside the variable in README.md and DEVELOPMENT.md, rather than only in a source comment.
  • Inheriting a HOME from an MSYS2, Cygwin or Git Bash shell is not a choice about this directory at all. state_dir's documentation already described ~ as "HOME on Unix, USERPROFILE on Windows", but home_dir() tried HOME first on every platform — so under those shells the state directory landed outside the profile without the user having asked. Windows now prefers USERPROFILE and keeps HOME as a last resort, matching paths::persistent_home_dir. Unix resolution is unchanged.

No new dependency was added; the Unix path needs only std and the libc the crate already takes on Unix targets.

Tests

Unix-gated and assert the mode actually on disk (metadata().permissions().mode() & 0o777), not that a chmod was attempted — only the former is what another local account sees. Coverage: fresh directory and file; a pre-existing world-readable directory; a pre-existing world-readable directory tree (the shape an upgrade actually meets); pre-existing group- and world-readable files inside a loose directory; a rewritten world-readable file; a world-readable leftover temp file; a temp path that already resolves elsewhere; and both sides of the refused-chmod branch. One test drives the real write_discovery_file production path end to end through the existing isolated-child-process harness, and also asserts the record still round-trips — tightening permissions is worthless if it breaks discovery.

restrict_open_state_file takes the mode-setting call as a parameter so the refused-chmod branch is covered from both directions — published when the mode is already owner-only, not published when it is wider — without needing one of the exotic filesystems it exists for. Two guards pin the sweep's scope: it must not follow a link out of the directory, and it must not descend.

Before the change, the production-path test reported the real modes:

a newly created state directory was left at 0755 instead of 0700, so another local account can traverse it
a newly written discovery file was left at 0644 instead of 0600, so it is readable by other local accounts
a pre-existing world-readable state directory stayed at 0755 instead of being tightened to 0700
a pre-existing world-readable discovery file stayed at 0644 instead of being tightened to 0600

Gate

cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, and RUSTDOCFLAGS="-D warnings" cargo doc --no-deps all clean; cargo test -p hyperdb-mcp 658 passed / 0 failed; cargo test -p hyperdb-api 632 passed / 0 failed; npx markdownlint-cli2 0 issues in 68 files.

Windows ACL behaviour and the USERPROFILE precedence were not verified locally (no Windows runner available) — the test (windows-latest) CI leg covers that the change compiles and the cross-platform tests pass there.

Note on overlap

Rebased onto upstream/main at 6cfe312, cleanly and with no conflicts, so this now sits on top of #290 and #292.

@StefanSteiner
StefanSteiner force-pushed the security/daemon-state-file-perms branch from e2db50a to 6bce49f Compare September 7, 2026 02:04
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.
…tems

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.
@StefanSteiner
StefanSteiner force-pushed the security/daemon-state-file-perms branch from b955e29 to 375dbde Compare September 7, 2026 02:22
@StefanSteiner
StefanSteiner merged commit 9856f60 into tableau:main Sep 7, 2026
29 of 30 checks passed
StefanSteiner added a commit that referenced this pull request Sep 7, 2026
…modules (#301)

`hyperdb-mcp/src/lib.rs` carried a lint `reason` saying the library target "is
not a documented API surface", while `hyperdb-mcp/README.md` promised, without
scope, that "the public API is stable and follows semantic versioning". Against
a crate where all 21 modules are `pub`, that sentence promised semver stability
on every module and every item inside them, which is why two incidental
internals had to be written up as API events: `DaemonState` gaining a private
field (#289) and `state_perms` becoming new public surface (#295).

Scope the promise to the MCP tool surface — tool names, parameters, and
behavior as reached over the MCP protocol — and state that the Rust library
target is excluded. The root README says the same in its crate table, next to
the equivalent note that already existed for `hyperdb-api-core`.

Mark all 21 modules `#[doc(hidden)]`. None can become `pub(crate)`: Cargo
compiles the `[[bin]]`, each file under `tests/`, and `examples/demo.rs` as
separate crates that reach library items only through the external
`hyperdb_mcp::` path, and every module is used by at least one of them
(`paths` by the binary alone; `stats`, `subscriptions` and `watcher` by one
test file each). So `pub` is load-bearing for compilation, not an API
commitment. Nothing is privatised and no code that compiled before stops
compiling, so this is not source-breaking.

The crate-level architecture bullets now name modules in plain code spans
instead of intra-doc links, because rustdoc declines to resolve a link to a
hidden item and renders it as literal `[engine]` brackets. `readme_tests.rs`
locates the `engine` bullet by that syntax, so its delimiters move with it;
the terminology it guards is unchanged and still fails when removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant