fix(config): empty allowed-hosts guard, fail-fast port, 0600 secrets, no request-path panics

src/admin.rs

@@ -10,7 +10,7 @@ use crate::db::{Db, now_unix};
 /// Open the database the running server uses. Idempotent — applies the same schema
 /// and pragmas, safe to run alongside the daemon (SQLite WAL allows it).
 fn open_db() -> anyhow::Result<Db> {
-    Db::open(&crate::config::db_path())
+    Db::open(&crate::config::db_path()?)
 }
 
 // ---- jobs ----
@@ -125,7 +125,14 @@ async fn kill_job(db: &Db, id: &str) -> anyhow::Result<String> {
         ));
     };
 
-    let killed = crate::jobs::kill_group(pgid as u32).await;
+    // A pgid outside u32 is a corrupt row; a raw `as` cast would wrap it into a
+    // real (wrong) process group and signal that instead.
+    let Ok(pgid) = u32::try_from(pgid) else {
+        return Ok(format!(
+            "job {id} has a corrupt pgid ({pgid}) — refusing to signal"
+        ));
+    };
+    let killed = crate::jobs::kill_group(pgid).await;
     // Record the kill only if the row is still `running`: when the server owns the
     // job, its waiter may already have written the real exit as the group died.
     let lookup = id.to_string();
@@ -165,7 +172,7 @@ pub async fn sessions() -> anyhow::Result<()> {
     print!(
         "{}",
         render_sessions(
-            &crate::config::db_path().display().to_string(),
+            &crate::config::db_path()?.display().to_string(),
             &access,
             &refresh,
             now

src/config.rs

@@ -112,23 +112,28 @@ impl Config {
             "/var/lib/mcp-ssh/mcp-ssh.db",
         ));
 
-        let allowed_hosts = match env.get("MCP_SSH_ALLOWED_HOSTS") {
-            Some(v) => split_hosts(&v),
-            None => match file.allowed_hosts {
-                Some(hosts) => hosts,
-                // Only the real fallback — neither env nor file set hosts — risks
-                // DNS rebinding, so warn here rather than for any explicit config.
-                None => {
-                    if !bind.ip().is_loopback() {
-                        warn!(
-                            "MCP_SSH_ALLOWED_HOSTS unset and bind is non-loopback ({}) — \
-                             DNS-rebinding attacks possible. Set MCP_SSH_ALLOWED_HOSTS explicitly.",
-                            bind.ip()
-                        );
-                    }
-                    vec!["localhost".into(), "127.0.0.1".into()]
+        let explicit_hosts = match env.get("MCP_SSH_ALLOWED_HOSTS") {
+            Some(v) => Some(split_hosts(&v)),
+            None => file
+                .allowed_hosts
+                .map(|hosts| normalize_hosts(hosts.into_iter())),
+        };
+        let allowed_hosts = match explicit_hosts {
+            Some(hosts) if !hosts.is_empty() => hosts,
+            // Unset, OR set but empty after trimming (`MCP_SSH_ALLOWED_HOSTS=`,
+            // `allowed_hosts = []`). Empty is treated exactly like unset because
+            // rmcp reads an empty allowlist as allow-ALL hosts — the guard would
+            // be silently OFF, worse than the default.
+            _ => {
+                if !bind.ip().is_loopback() {
+                    warn!(
+                        "MCP_SSH_ALLOWED_HOSTS unset or empty and bind is non-loopback ({}) — \
+                         DNS-rebinding attacks possible. Set MCP_SSH_ALLOWED_HOSTS explicitly.",
+                        bind.ip()
+                    );
                 }
-            },
+                vec!["localhost".into(), "127.0.0.1".into()]
+            }
         };
 
         let public_url = opt(env, "MCP_SSH_PUBLIC_URL", file.public_url);
@@ -149,19 +154,21 @@ impl Config {
 /// Resolve just the SQLite path: env `MCP_SSH_DB`, else the file's `db_path`, else
 /// the default. The admin subcommands (`jobs`/`job kill`/`sessions`) touch only the
 /// database, so they resolve it this way instead of `Config::load`, which requires
-/// auth credentials they don't need.
-pub fn db_path() -> PathBuf {
+/// auth credentials they don't need. A config file that exists but can't be read
+/// or parsed is an error, not a silent fall-through to the default path — that
+/// made the admin CLI inspect (or kill in!) a different database than the server.
+pub fn db_path() -> Result<PathBuf, ConfigError> {
     db_path_in(&ProcessEnv)
 }
 
-fn db_path_in(env: &dyn EnvSource) -> PathBuf {
-    let file = load_file(&config_path(env)).unwrap_or_default();
-    PathBuf::from(pick(
+fn db_path_in(env: &dyn EnvSource) -> Result<PathBuf, ConfigError> {
+    let file = load_file(&config_path(env))?;
+    Ok(PathBuf::from(pick(
         env,
         "MCP_SSH_DB",
         file.db_path,
         "/var/lib/mcp-ssh/mcp-ssh.db",
-    ))
+    )))
 }
 
 /// Write `user`/`pass` into the config file, preserving other fields. Chmod 600.
@@ -179,11 +186,30 @@ fn set_auth_in(env: &dyn EnvSource, user: &str, pass: &str) -> Result<PathBuf, C
         std::fs::create_dir_all(parent).map_err(|e| ConfigError::Io(parent.to_path_buf(), e))?;
     }
     let toml = toml::to_string_pretty(&file).map_err(|e| ConfigError::Parse(e.to_string()))?;
-    std::fs::write(&path, toml).map_err(|e| ConfigError::Io(path.clone(), e))?;
-    chmod_600(&path);
+    write_secret_file(&path, &toml).map_err(|e| ConfigError::Io(path.clone(), e))?;
+    // The 0600 in `write_secret_file` applies only when the file is created; a
+    // pre-existing file keeps its old mode, so tighten it — and a failure here
+    // must be loud, not a plaintext password left world-readable forever.
+    chmod_600(&path).map_err(|e| ConfigError::Io(path.clone(), e))?;
     Ok(path)
 }
 
+/// Write `contents` with the file created 0600 from the first byte. Plain
+/// `fs::write` created it umask-default (typically 0644) and only chmodded
+/// afterwards — a window where any local user could read the plaintext password.
+fn write_secret_file(path: &std::path::Path, contents: &str) -> std::io::Result<()> {
+    use std::io::Write;
+    let mut opts = std::fs::OpenOptions::new();
+    opts.write(true).create(true).truncate(true);
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::OpenOptionsExt;
+        opts.mode(0o600);
+    }
+    let mut f = opts.open(path)?;
+    f.write_all(contents.as_bytes())
+}
+
 fn load_file(path: &std::path::Path) -> Result<FileConfig, ConfigError> {
     match std::fs::read_to_string(path) {
         Ok(s) => toml::from_str(&s).map_err(|e| ConfigError::Parse(e.to_string())),
@@ -193,15 +219,23 @@ fn load_file(path: &std::path::Path) -> Result<FileConfig, ConfigError> {
 }
 
 #[cfg(unix)]
-fn chmod_600(path: &std::path::Path) {
+fn chmod_600(path: &std::path::Path) -> std::io::Result<()> {
     use std::os::unix::fs::PermissionsExt;
-    let _ = std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600));
+    std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))
 }
 #[cfg(not(unix))]
-fn chmod_600(_path: &std::path::Path) {}
+fn chmod_600(_path: &std::path::Path) -> std::io::Result<()> {
+    Ok(())
+}
 
 fn split_hosts(v: &str) -> Vec<String> {
-    v.split(',')
+    normalize_hosts(v.split(',').map(str::to_string))
+}
+
+/// Trim and drop empty entries so `""` or `" , "` can't smuggle an empty list
+/// (or empty strings) into the allowlist.
+fn normalize_hosts(hosts: impl Iterator<Item = String>) -> Vec<String> {
+    hosts
         .map(|s| s.trim().to_string())
         .filter(|s| !s.is_empty())
         .collect()
@@ -326,6 +360,86 @@ mod tests {
         }
     }
 
+    #[test]
+    fn empty_allowed_hosts_falls_back_to_loopback_default() {
+        // rmcp treats an empty allowlist as allow-ALL hosts, so set-but-empty
+        // (`MCP_SSH_ALLOWED_HOSTS=`, whitespace/commas, or `allowed_hosts = []`
+        // in the file) must behave exactly like unset — loopback default, never
+        // an empty vec.
+        let dir = tempdir().unwrap();
+        let cfg_path = dir.path().join("config.toml");
+        for empty in ["", " , "] {
+            let env = MapEnv::new(&[
+                ("MCP_SSH_CONFIG", cfg_path.to_str().unwrap()),
+                ("MCP_SSH_ALLOWED_HOSTS", empty),
+                ("MCP_SSH_USER", "test"),
+                ("MCP_SSH_PASS", "test"),
+            ]);
+            let cfg = Config::from_env(&env).expect("load");
+            assert_eq!(
+                cfg.allowed_hosts,
+                vec!["localhost", "127.0.0.1"],
+                "env {empty:?} must not disable the host guard"
+            );
+        }
+
+        // Same for an explicit empty list in the file.
+        std::fs::write(&cfg_path, "allowed_hosts = []\n").unwrap();
+        let env = MapEnv::new(&[
+            ("MCP_SSH_CONFIG", cfg_path.to_str().unwrap()),
+            ("MCP_SSH_USER", "test"),
+            ("MCP_SSH_PASS", "test"),
+        ]);
+        let cfg = Config::from_env(&env).expect("load");
+        assert_eq!(cfg.allowed_hosts, vec!["localhost", "127.0.0.1"]);
+    }
+
+    #[test]
+    fn db_path_propagates_a_broken_config_file() {
+        // A config file that exists but doesn't parse must be an error — the old
+        // unwrap_or_default() sent admin commands to the DEFAULT db path while
+        // the server (whose Config::load errors loudly) used the configured one.
+        let dir = tempdir().unwrap();
+        let cfg_path = dir.path().join("config.toml");
+        std::fs::write(&cfg_path, "this is { not toml").unwrap();
+        let env = MapEnv::new(&[("MCP_SSH_CONFIG", cfg_path.to_str().unwrap())]);
+        assert!(
+            matches!(db_path_in(&env), Err(ConfigError::Parse(_))),
+            "broken config must not silently resolve to the default db path"
+        );
+
+        // A missing file is still fine (defaults apply).
+        let env = MapEnv::new(&[(
+            "MCP_SSH_CONFIG",
+            dir.path().join("absent.toml").to_str().unwrap(),
+        )]);
+        assert_eq!(
+            db_path_in(&env).unwrap(),
+            PathBuf::from("/var/lib/mcp-ssh/mcp-ssh.db")
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn set_auth_tightens_a_preexisting_loose_config_file() {
+        use std::os::unix::fs::PermissionsExt;
+        let dir = tempdir().unwrap();
+        let cfg_path = dir.path().join("config.toml");
+        std::fs::write(&cfg_path, "").unwrap();
+        std::fs::set_permissions(&cfg_path, std::fs::Permissions::from_mode(0o644)).unwrap();
+
+        let env = MapEnv::new(&[("MCP_SSH_CONFIG", cfg_path.to_str().unwrap())]);
+        set_auth_in(&env, "bob", "s3cr3t").expect("set_auth");
+
+        use std::os::unix::fs::MetadataExt;
+        let mode = std::fs::metadata(&cfg_path).unwrap().mode();
+        assert_eq!(
+            mode & 0o777,
+            0o600,
+            "a pre-existing world-readable file must be tightened"
+        );
+    }
+
     #[test]
     fn non_loopback_bind_without_allowed_hosts_env_loads() {
         let dir = tempdir().unwrap();

src/db.rs

@@ -70,18 +70,31 @@ impl Db {
 
     /// Run `f` against the connection on the blocking pool. The guard lives only
     /// inside the closure (a separate thread), so it never crosses an `.await`.
+    ///
+    /// Never panics: this sits on the request path (every bearer validation and
+    /// job write), where a panic would poison the mutex and turn ALL later calls
+    /// into panics until restart. A poisoned lock is recovered (the connection
+    /// itself is fine — only the closure panicked mid-hold), and a panicking
+    /// closure comes back as an error to its caller alone.
     pub async fn call<F, T>(&self, f: F) -> rusqlite::Result<T>
     where
         F: FnOnce(&Connection) -> rusqlite::Result<T> + Send + 'static,
         T: Send + 'static,
     {
         let conn = self.conn.clone();
         tokio::task::spawn_blocking(move || {
-            let guard = conn.lock().expect("db connection mutex poisoned");
+            let guard = conn
+                .lock()
+                .unwrap_or_else(std::sync::PoisonError::into_inner);
             f(&guard)
         })
         .await
-        .expect("db blocking task panicked")
+        .unwrap_or_else(|join_error| {
+            Err(rusqlite::Error::SqliteFailure(
+                rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_ERROR),
+                Some(format!("db task panicked: {join_error}")),
+            ))
+        })
     }
 }
 
@@ -141,6 +154,24 @@ mod tests {
         assert_eq!(exp, 123);
     }
 
+    #[tokio::test]
+    async fn panicking_closure_is_an_error_and_the_connection_survives() {
+        // A panic inside a db closure must come back as Err to its caller only —
+        // not panic the request path, and not poison the connection for every
+        // later call (which previously turned one bug into a full outage).
+        let db = Db::memory();
+        let r = db
+            .call(|_conn| -> rusqlite::Result<()> { panic!("boom") })
+            .await;
+        assert!(r.is_err(), "panicking closure must surface as an error");
+
+        let one: i64 = db
+            .call(|conn| conn.query_row("SELECT 1", [], |row| row.get(0)))
+            .await
+            .expect("connection must remain usable after a closure panic");
+        assert_eq!(one, 1);
+    }
+
     #[test]
     fn ensure_column_adds_missing_and_is_idempotent() {
         let conn = Connection::open_in_memory().unwrap();

src/main.rs

@@ -74,11 +74,10 @@ fn set_auth(user: String) -> anyhow::Result<()> {
 async fn serve(port: Option<u16>) -> anyhow::Result<()> {
     let mut cfg = config::Config::load()?;
     // `--port` (or MCP_SSH_PORT) overrides just the port of the bind address.
-    if let Some(p) = port.or_else(|| {
-        std::env::var("MCP_SSH_PORT")
-            .ok()
-            .and_then(|v| v.parse().ok())
-    }) {
+    if let Some(p) = match port {
+        Some(p) => Some(p),
+        None => env_port(std::env::var("MCP_SSH_PORT").ok())?,
+    } {
         cfg.bind.set_port(p);
     }
     // The single SQLite database (OAuth tokens + job metadata) under the systemd
@@ -114,6 +113,20 @@ async fn serve(port: Option<u16>) -> anyhow::Result<()> {
     Ok(())
 }
 
+/// Parse the `MCP_SSH_PORT` value. Unset/empty → no override; anything else must
+/// be a valid port. Silently ignoring `8080x` or `70000` (the old `.ok()` chain)
+/// left the server listening on the config/default port while the operator
+/// believed they'd moved it — fail fast instead, like an invalid `MCP_SSH_BIND`.
+fn env_port(raw: Option<String>) -> anyhow::Result<Option<u16>> {
+    match raw.as_deref() {
+        None | Some("") => Ok(None),
+        Some(v) => v
+            .parse::<u16>()
+            .map(Some)
+            .map_err(|e| anyhow::anyhow!("invalid MCP_SSH_PORT {v:?}: {e}")),
+    }
+}
+
 /// Resolves on Ctrl-C or (on Unix) SIGTERM, letting axum drain in-flight
 /// requests before exit. systemd sends SIGTERM on `stop`/`restart`.
 async fn shutdown_signal() {
@@ -159,6 +172,17 @@ mod tests {
 
     use super::*;
 
+    #[test]
+    fn env_port_rejects_garbage_and_passes_valid_values() {
+        assert_eq!(env_port(None).unwrap(), None, "unset → no override");
+        assert_eq!(env_port(Some("".into())).unwrap(), None, "empty → unset");
+        assert_eq!(env_port(Some("8080".into())).unwrap(), Some(8080));
+        // A typo'd or out-of-range port must fail fast, not silently bind the
+        // default port.
+        assert!(env_port(Some("8080x".into())).is_err());
+        assert!(env_port(Some("70000".into())).is_err());
+    }
+
     // SIGTERM (what systemd sends on stop) must resolve `shutdown_signal` so
     // axum begins draining instead of the process being killed outright.
     #[tokio::test]