get_state_file: refuse symlinks below root + require world-readable mode

TODO.md

@@ -24,7 +24,7 @@
 ## Security
 
 - **Privsep the dashboard from the privileged daemon**: hive-c0re runs as root (it has to — `nixos-container` create / start / destroy, the meta git repo, every per-agent bind mount). The HTTP server lives in the same process, so every read-endpoint (`/api/state-file`, `/api/journal/{name}`, `/api/agent-config/{name}`) is one allow-list bug away from serving arbitrary host files. Split the architecture: keep the privileged daemon doing lifecycle + git + ipc, run the web UI as an unprivileged user that talks to the daemon over a unix socket with a narrow request surface (`ReadAgentStateFile { agent, rel_path }` etc.). The unprivileged process can't read `/etc/shadow` even if every check in `get_state_file` is bypassed — it doesn't have the bits. Container-lifecycle POSTs (`/restart`, `/destroy`, etc.) become forwarded RPCs the privileged side authorises on its terms.
-- **Defense in depth on `get_state_file`**: until privsep lands, the allow-list is load-bearing. Worth adding: refuse files whose mode is not world-readable (so an agent writing a 0600 file inside `state/` can't have its contents proxied through the endpoint to a different operator), and refuse symlinks at any path component (`O_NOFOLLOW`-style — `canonicalize` resolves them, but we currently don't reject if the original path had symlinks).
+- ~~**Defense in depth on `get_state_file`**~~ ✓ landed — `resolve_state_path` (shared by `get_state_file` + `scan_validated_paths`) now: (a) walks each path component below the matched root via `symlink_metadata` and refuses outright if any is a symlink (so an agent planting `ln -s /var/lib/hyperhive/agents/other/state/secret /agents/me/state/peek` can't have its target proxied — `canonicalize` would happily resolve past the allow-list check otherwise); (b) refuses any `..` traversal below the root with a friendlier error than "escapes allow-list"; (c) refuses files whose mode isn't world-readable (`mode & 0o004 == 0`) so a 0600 file inside `state/` doesn't leak via the endpoint; (d) bundles the metadata fetch into the resolve helper so callers don't restat. New tests in `hive-c0re/src/dashboard.rs::tests` cover leaf-symlink, mid-path-symlink, `..` traversal, and plain-dir-passthrough cases.
 
 ## Harness Ergonomics (agent-side wishlist)
 

hive-c0re/src/dashboard.rs

@@ -958,23 +958,66 @@ struct StateFileQuery {
 /// that has been verified against the allow-list. Returns `Err`
 /// with a human-readable reason for every failure mode (path
 /// outside roots, canonicalize failure, escape via symlink,
-/// per-agent subdir not `state`). Shared by `get_state_file` (read)
+/// per-agent subdir not `state`, symlink anywhere below the root,
-/// and `post_state_file_check` (existence probe) so both endpoints
+/// file not world-readable). Shared by `get_state_file` (read) and
-/// apply identical security rules.
+/// `scan_validated_paths` (linkify candidates in message bodies)
-fn resolve_state_path(raw: &str) -> std::result::Result<std::path::PathBuf, String> {
+/// so both apply identical security rules and the linkifier
+/// doesn't render a path the reader will refuse to serve.
+///
+/// Defense-in-depth layers (in order):
+/// 1. Caller-supplied prefix has to match the allow-list (agents/
+///    or shared/), else reject without touching the fs.
+/// 2. No symlinks below the matched root. Walked pre-canonicalize
+///    via `symlink_metadata` on each component so a sub-agent that
+///    plants `ln -s /var/lib/hyperhive/agents/other/state/secret
+///    /agents/me/state/peek` can't proxy a different agent's file
+///    through this endpoint (canonicalize would happily resolve
+///    the symlink to a path inside the allow-list).
+/// 3. Canonicalize is run anyway as a belt-and-braces check —
+///    resolves `..`/`.` traversal and rejects if the result
+///    escapes the roots.
+/// 4. Under `AGENTS_ROOT`, the second path component must be
+///    `state/` — agents' applied/proposed git repos and config dirs
+///    are off-limits.
+/// 5. The target's metadata is fetched once and returned to the
+///    caller so they don't restat. If the target is a regular
+///    file it must be world-readable (mode & 0o004); a 0600 file
+///    inside `state/` could leak through this endpoint to anyone
+///    holding the dashboard URL otherwise.
+fn resolve_state_path(
+    raw: &str,
+) -> std::result::Result<(std::path::PathBuf, std::fs::Metadata), String> {
+    use std::os::unix::fs::PermissionsExt as _;
     const AGENTS_ROOT: &str = "/var/lib/hyperhive/agents";
     const SHARED_ROOT: &str = "/var/lib/hyperhive/shared";
     let raw = raw.trim();
-    let mapped: std::path::PathBuf = if let Some(rest) = raw.strip_prefix("/agents/") {
+    let (mapped, root): (std::path::PathBuf, &str) =
-        std::path::PathBuf::from(format!("{AGENTS_ROOT}/{rest}"))
+        if let Some(rest) = raw.strip_prefix("/agents/") {
+            (
+                std::path::PathBuf::from(format!("{AGENTS_ROOT}/{rest}")),
+                AGENTS_ROOT,
+            )
         } else if let Some(rest) = raw.strip_prefix("/shared/") {
-        std::path::PathBuf::from(format!("{SHARED_ROOT}/{rest}"))
+            (
-    } else if raw.starts_with(AGENTS_ROOT) || raw.starts_with(SHARED_ROOT) {
+                std::path::PathBuf::from(format!("{SHARED_ROOT}/{rest}")),
-        std::path::PathBuf::from(raw)
+                SHARED_ROOT,
+            )
+        } else if let Some(rest) = raw.strip_prefix(&format!("{AGENTS_ROOT}/")) {
+            (
+                std::path::PathBuf::from(format!("{AGENTS_ROOT}/{rest}")),
+                AGENTS_ROOT,
+            )
+        } else if let Some(rest) = raw.strip_prefix(&format!("{SHARED_ROOT}/")) {
+            (
+                std::path::PathBuf::from(format!("{SHARED_ROOT}/{rest}")),
+                SHARED_ROOT,
+            )
         } else {
             return Err(format!("path not in allow-list: {raw}"));
         };
-    let canonical = std::fs::canonicalize(&mapped).map_err(|e| format!("{}: {e}", mapped.display()))?;
+    reject_symlinks_below(std::path::Path::new(root), &mapped)?;
+    let canonical = std::fs::canonicalize(&mapped)
+        .map_err(|e| format!("{}: {e}", mapped.display()))?;
     if !(canonical.starts_with(AGENTS_ROOT) || canonical.starts_with(SHARED_ROOT)) {
         return Err(format!(
             "resolved path escapes allow-list: {}",
@@ -992,7 +1035,134 @@ fn resolve_state_path(raw: &str) -> std::result::Result<std::path::PathBuf, Stri
             ));
         }
     }
-    Ok(canonical)
+    let meta = std::fs::metadata(&canonical)
+        .map_err(|e| format!("stat {}: {e}", canonical.display()))?;
+    if meta.is_file() {
+        let mode = meta.permissions().mode();
+        if mode & 0o004 == 0 {
+            return Err(format!(
+                "{} not world-readable (mode 0{:o}); refusing to proxy non-public file",
+                canonical.display(),
+                mode & 0o777,
+            ));
+        }
+    }
+    Ok((canonical, meta))
+}
+
+/// Walk every path component under `root` and refuse if any of
+/// them is a symlink. The roots themselves (`AGENTS_ROOT`,
+/// `SHARED_ROOT`) are hive-c0re-owned and assumed trusted; only
+/// the parts the agent / operator can plant matter. Components
+/// that don't exist yet are skipped — `canonicalize` reports
+/// non-existence separately, and missing-component checks would
+/// just race the filesystem.
+fn reject_symlinks_below(
+    root: &std::path::Path,
+    mapped: &std::path::Path,
+) -> std::result::Result<(), String> {
+    let Ok(rel) = mapped.strip_prefix(root) else {
+        return Ok(());
+    };
+    let mut cumulative = root.to_path_buf();
+    for component in rel.components() {
+        match component {
+            std::path::Component::Normal(name) => {
+                cumulative.push(name);
+                match std::fs::symlink_metadata(&cumulative) {
+                    Ok(m) if m.file_type().is_symlink() => {
+                        return Err(format!(
+                            "symlink at {} not allowed (canonicalize would resolve it past the \
+                             allow-list check; refuse outright)",
+                            cumulative.display()
+                        ));
+                    }
+                    Ok(_) | Err(_) => {}
+                }
+            }
+            std::path::Component::ParentDir => {
+                return Err(format!(
+                    "path contains `..` traversal below {}; refuse outright",
+                    root.display()
+                ));
+            }
+            _ => {}
+        }
+    }
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::os::unix::fs::symlink;
+
+    /// Make a unique tmp subdir for the calling test. Caller is responsible
+    /// for cleanup (we leak on panic, fine for ephemeral CI runs).
+    fn tmproot(tag: &str) -> std::path::PathBuf {
+        let ts = std::time::SystemTime::now()
+            .duration_since(std::time::UNIX_EPOCH)
+            .map(|d| d.as_nanos())
+            .unwrap_or(0);
+        let p = std::env::temp_dir().join(format!("hyperhive-test-{tag}-{ts}"));
+        std::fs::create_dir_all(&p).unwrap();
+        p
+    }
+
+    #[test]
+    fn reject_symlinks_below_accepts_plain_dirs_and_files() {
+        let root = tmproot("symlink-ok");
+        std::fs::create_dir_all(root.join("alice/state")).unwrap();
+        std::fs::write(root.join("alice/state/notes.md"), b"hi").unwrap();
+        assert!(reject_symlinks_below(&root, &root.join("alice/state/notes.md")).is_ok());
+    }
+
+    #[test]
+    fn reject_symlinks_below_rejects_leaf_symlink() {
+        let root = tmproot("symlink-leaf");
+        std::fs::create_dir_all(root.join("alice/state")).unwrap();
+        // Plant a symlink that points anywhere; resolve_state_path's
+        // canonicalize would happily resolve it past the allow-list
+        // check, so we have to refuse at the un-canonical layer.
+        symlink("/etc/shadow", root.join("alice/state/peek")).unwrap();
+        let err = reject_symlinks_below(&root, &root.join("alice/state/peek")).unwrap_err();
+        assert!(err.contains("symlink at"), "msg = {err}");
+        assert!(err.contains("peek"), "msg = {err}");
+    }
+
+    #[test]
+    fn reject_symlinks_below_rejects_directory_symlink_in_middle() {
+        let root = tmproot("symlink-mid");
+        std::fs::create_dir_all(root.join("real/state")).unwrap();
+        std::fs::write(root.join("real/state/secret.md"), b"hi").unwrap();
+        // alice's "state" dir is actually a symlink to real/state — a
+        // sub-agent shouldn't be able to plant this and proxy real's
+        // private files via the dashboard.
+        std::fs::create_dir_all(root.join("alice")).unwrap();
+        symlink(root.join("real/state"), root.join("alice/state")).unwrap();
+        let err = reject_symlinks_below(&root, &root.join("alice/state/secret.md")).unwrap_err();
+        assert!(err.contains("symlink at"), "msg = {err}");
+    }
+
+    #[test]
+    fn reject_symlinks_below_rejects_parent_dir_traversal() {
+        let root = tmproot("symlink-dotdot");
+        // `..` doesn't survive canonicalize anyway, but we want a
+        // friendlier error than "path escapes allow-list" — refusing
+        // upfront also avoids walking ancestors with `symlink_metadata`.
+        let p = root.join("alice/state/../escape");
+        let err = reject_symlinks_below(&root, &p).unwrap_err();
+        assert!(err.contains("`..`"), "msg = {err}");
+    }
+
+    #[test]
+    fn reject_symlinks_below_passes_through_when_path_not_under_root() {
+        // resolve_state_path's earlier allow-list check would reject
+        // this; reject_symlinks_below stays a no-op so the caller
+        // surfaces the better-fit error.
+        let root = std::path::Path::new("/var/lib/hyperhive/agents");
+        assert!(reject_symlinks_below(root, std::path::Path::new("/etc/shadow")).is_ok());
+    }
 }
 
 /// Snapshot the current tombstone list and emit a
@@ -1063,12 +1233,12 @@ pub(crate) fn scan_validated_paths(body: &str) -> Vec<String> {
         if out.iter().any(|s| s == token) {
             continue;
         }
-        if let Ok(canonical) = resolve_state_path(token) {
+        if let Ok((_canonical, meta)) = resolve_state_path(token)
-            if std::fs::metadata(&canonical).is_ok_and(|m| m.is_file()) {
+            && meta.is_file()
+        {
             out.push(token.to_owned());
         }
     }
-    }
     out
 }
 
@@ -1076,14 +1246,10 @@ async fn get_state_file(
     axum::extract::Query(q): axum::extract::Query<StateFileQuery>,
 ) -> Response {
     const MAX_BYTES: usize = 1 << 20; // 1 MiB
-    let canonical = match resolve_state_path(&q.path) {
+    let (canonical, meta) = match resolve_state_path(&q.path) {
-        Ok(p) => p,
+        Ok(pair) => pair,
         Err(e) => return error_response(&format!("state-file: {e}")),
     };
-    let meta = match std::fs::metadata(&canonical) {
-        Ok(m) => m,
-        Err(e) => return error_response(&format!("state-file: stat {}: {e}", canonical.display())),
-    };
     if !meta.is_file() {
         return error_response(&format!(
             "state-file: {} is not a regular file",