fix(security): harden permissions for local mnemonic database

src/cli.rs

@@ -440,25 +440,37 @@ fn resolve_mostro_pubkey(cli: &Cli) -> Result<String> {
         })
 }
 
+/// Ensure `RELAYS` is configured (and non-empty) before we create any local
+/// state. `connect_nostr` reads this variable; validating it here means a
+/// command run without relays fails before the mnemonic database is created.
+/// `get_env_var` has already mirrored `--relays` into the env var by this point.
+fn resolve_relays() -> Result<String> {
+    match std::env::var("RELAYS") {
+        Ok(relays) if !relays.trim().is_empty() => Ok(relays),
+        _ => Err(anyhow::anyhow!(
+            "RELAYS not set.\n\
+             Provide it using one of the following methods:\n\
+             1) --relays <relay[,relay...]>\n\
+             2) export RELAYS=<relay[,relay...]>"
+        )),
+    }
+}
+
 async fn init_context(cli: &Cli) -> Result<Context> {
     // Get environment variables
     get_env_var(cli);
 
-    // Initialize database pool
-    let pool = connect().await?;
-
-    // Get identity keys
-    let identity_keys = User::get_identity_keys(&pool)
-        .await
-        .map_err(|e| anyhow::anyhow!("Failed to get identity keys: {}", e))?;
-
-    // Get trade keys
-    let (trade_keys, trade_index) = User::get_next_trade_keys(&pool)
-        .await
-        .map_err(|e| anyhow::anyhow!("Failed to get trade keys: {}", e))?;
+    // Validate all required configuration *before* touching the local database.
+    // `connect()` creates `~/.mcli/mcli.db` and generates the mnemonic on first
+    // run, so a mistaken command with missing config must fail here, without
+    // ever writing that sensitive material to disk. See issue #179.
+    let mostro_pubkey = PublicKey::from_str(&resolve_mostro_pubkey(cli)?)?;
+    resolve_relays()?;
 
-    // Load private key of admin - only required for admin commands
-    // For regular user commands, this will be None
+    // Load private key of admin - only required for admin commands.
+    // For regular user commands, this will be None. Validated up front so an
+    // admin command with a missing/invalid ADMIN_NSEC also fails before the
+    // database is created.
     let context_keys = if is_admin_command(&cli.command) {
         Some(
             std::env::var("ADMIN_NSEC")
@@ -472,9 +484,18 @@ async fn init_context(cli: &Cli) -> Result<Context> {
         None
     };
 
-    // Resolve Mostro pubkey from env (required for all flows)
+    // Initialize database pool (creates the DB and mnemonic on first run)
+    let pool = connect().await?;
 
-    let mostro_pubkey = PublicKey::from_str(&resolve_mostro_pubkey(cli)?)?;
+    // Get identity keys
+    let identity_keys = User::get_identity_keys(&pool)
+        .await
+        .map_err(|e| anyhow::anyhow!("Failed to get identity keys: {}", e))?;
+
+    // Get trade keys
+    let (trade_keys, trade_index) = User::get_next_trade_keys(&pool)
+        .await
+        .map_err(|e| anyhow::anyhow!("Failed to get trade keys: {}", e))?;
 
     // Connect to Nostr relays
     let client = util::connect_nostr().await?;

src/db.rs

@@ -6,7 +6,7 @@ use nostr_sdk::prelude::*;
 use sqlx::pool::Pool;
 use sqlx::Sqlite;
 use sqlx::SqlitePool;
-use std::fs::File;
+use std::io;
 use std::path::Path;
 
 pub async fn connect() -> Result<Pool<Sqlite>> {
@@ -15,7 +15,11 @@ pub async fn connect() -> Result<Pool<Sqlite>> {
     let db_url = format!("sqlite://{}", mcli_db_path);
     let pool: Pool<Sqlite>;
     if !Path::exists(Path::new(&mcli_db_path)) {
-        if let Err(res) = File::create(&mcli_db_path) {
+        // Create the database file with owner-only permissions (0600 on Unix)
+        // *before* writing anything to it. This file stores the mnemonic that
+        // derives the user's identity and trade keys, so it must not be readable
+        // by other local users under a permissive umask. See issue #179.
+        if let Err(res) = create_private_db_file(&mcli_db_path) {
             println!("Error in creating db file: {}", res);
             return Err(res.into());
         }
@@ -63,6 +67,20 @@ pub async fn connect() -> Result<Pool<Sqlite>> {
         let user = User::new(mnemonic, &pool).await?;
         println!("User created with pubkey: {}", user.i0_pubkey);
     } else {
+        // Defensively re-tighten the permissions of a database that an older
+        // version (or a permissive umask) may have left world-readable, so
+        // existing installs get hardened on the next run too. Fail closed: if we
+        // can't make the mnemonic DB owner-only, refuse to open it rather than
+        // keep using a potentially group/world-readable file. See issue #179.
+        #[cfg(unix)]
+        crate::util::misc::set_mode(&mcli_db_path, 0o600).map_err(|e| {
+            anyhow::anyhow!(
+                "could not restrict permissions on {}; refusing to open mnemonic DB: {}",
+                mcli_db_path,
+                e
+            )
+        })?;
+
         pool = SqlitePool::connect(&db_url).await?;
 
         // Migration: Drop buyer_token and seller_token columns if they exist
@@ -72,6 +90,29 @@ pub async fn connect() -> Result<Pool<Sqlite>> {
     Ok(pool)
 }
 
+/// Create the SQLite database file restricted to the owner (mode `0600` on
+/// Unix). On non-Unix platforms this falls back to a plain create.
+///
+/// Uses `create_new` so the file is created atomically with the right
+/// permissions instead of being created world-readable and tightened
+/// afterwards, which would leave a brief window where another local user could
+/// open it.
+fn create_private_db_file(path: &str) -> io::Result<()> {
+    let mut options = std::fs::OpenOptions::new();
+    options.write(true).create_new(true);
+
+    // The owner-only mode bit is Unix-specific; `create_new` (fail on existing)
+    // applies on every platform so a second create never truncates an existing
+    // database.
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::OpenOptionsExt;
+        options.mode(0o600);
+    }
+
+    options.open(path).map(|_| ())
+}
+
 async fn migrate_remove_token_columns(pool: &SqlitePool) -> Result<()> {
     println!("Checking for legacy token columns...");
 
@@ -577,3 +618,58 @@ impl Order {
         Ok(rows_affected > 0)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// Unique, non-existent path inside the OS temp dir (no extra crates, no
+    /// forbidden `Date`/random APIs).
+    fn unique_temp_path(label: &str) -> std::path::PathBuf {
+        use std::sync::atomic::{AtomicU64, Ordering};
+        static COUNTER: AtomicU64 = AtomicU64::new(0);
+        let n = COUNTER.fetch_add(1, Ordering::Relaxed);
+        std::env::temp_dir().join(format!(
+            "mcli-db-test-{}-{}-{}",
+            label,
+            std::process::id(),
+            n
+        ))
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_create_private_db_file_is_0600() {
+        use std::os::unix::fs::PermissionsExt;
+
+        let path = unique_temp_path("perms");
+        let path_str = path.to_str().unwrap();
+
+        create_private_db_file(path_str).expect("should create db file");
+
+        assert!(path.is_file(), "db file should exist");
+        let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777;
+        assert_eq!(
+            mode, 0o600,
+            "mnemonic db must be readable only by the owner"
+        );
+
+        std::fs::remove_file(&path).ok();
+    }
+
+    #[test]
+    fn test_create_private_db_file_fails_if_exists() {
+        let path = unique_temp_path("exists");
+        let path_str = path.to_str().unwrap();
+
+        create_private_db_file(path_str).expect("first create should succeed");
+        // `create_new` semantics: a second create on the same path must error
+        // rather than truncate an existing (possibly populated) database.
+        assert!(
+            create_private_db_file(path_str).is_err(),
+            "creating over an existing db file must fail"
+        );
+
+        std::fs::remove_file(&path).ok();
+    }
+}

src/util/misc.rs

@@ -1,4 +1,4 @@
-use std::{fs, path::Path};
+use std::io;
 
 pub fn uppercase_first(s: &str) -> String {
     let mut c = s.chars();
@@ -11,14 +11,62 @@ pub fn uppercase_first(s: &str) -> String {
 pub fn get_mcli_path() -> String {
     let home_dir = dirs::home_dir().expect("Couldn't get home directory");
     let mcli_path = format!("{}/.mcli", home_dir.display());
-    if !Path::new(&mcli_path).exists() {
-        match fs::create_dir(&mcli_path) {
-            Ok(_) => println!("Directory {} created.", mcli_path),
-            Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {
-                // Directory was created by another thread/process, which is fine
-            }
-            Err(e) => panic!("Couldn't create mostro-cli directory in HOME: {}", e),
-        }
+    if let Err(e) = ensure_private_dir(&mcli_path) {
+        panic!("Couldn't create mostro-cli directory in HOME: {}", e);
     }
     mcli_path
 }
+
+/// Ensure `path` exists as a directory that is private to the current user.
+///
+/// The `.mcli` directory holds `mcli.db`, which in turn stores the mnemonic that
+/// derives the user's Mostro identity and trade keys. To avoid that secret being
+/// exposed to other local users under a permissive `umask`, the directory is
+/// created with mode `0700` on Unix. We also tighten the permissions of a
+/// pre-existing directory, so users who created `~/.mcli` with an older version
+/// (or a loose `umask`) get hardened on the next run. See issue #179.
+pub fn ensure_private_dir(path: &str) -> io::Result<()> {
+    match create_dir_private(path) {
+        Ok(()) => {}
+        // Another thread/process won the race, or the path already existed.
+        Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {}
+        Err(e) => return Err(e),
+    }
+
+    // `AlreadyExists` only tells us *something* is there. If it's a regular file
+    // (or anything other than a directory) we must not chmod it and claim
+    // success — callers were promised a private directory.
+    if !std::fs::metadata(path)?.is_dir() {
+        return Err(io::Error::new(
+            io::ErrorKind::AlreadyExists,
+            format!("{} exists but is not a directory", path),
+        ));
+    }
+
+    #[cfg(unix)]
+    set_mode(path, 0o700)?;
+
+    Ok(())
+}
+
+/// Create a directory restricted to the owner (mode `0700` on Unix).
+///
+/// Returns an `AlreadyExists` error when the directory is already present, so
+/// callers can treat that case as success.
+#[cfg(unix)]
+fn create_dir_private(path: &str) -> io::Result<()> {
+    use std::os::unix::fs::DirBuilderExt;
+    std::fs::DirBuilder::new().mode(0o700).create(path)
+}
+
+#[cfg(not(unix))]
+fn create_dir_private(path: &str) -> io::Result<()> {
+    std::fs::create_dir(path)
+}
+
+/// Set the Unix permission bits of `path` to `mode`.
+#[cfg(unix)]
+pub fn set_mode(path: &str, mode: u32) -> io::Result<()> {
+    use std::os::unix::fs::PermissionsExt;
+    std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode))
+}

src/util/mod.rs

@@ -15,7 +15,7 @@ pub use messaging::{
     send_admin_chat_message_via_shared_key, send_dm, send_plain_text_dm, wait_for_dm,
     PowRequirementUnmet, WaitForDmTimeout,
 };
-pub use misc::{get_mcli_path, uppercase_first};
+pub use misc::{ensure_private_dir, get_mcli_path, uppercase_first};
 pub use net::connect_nostr;
 pub use storage::{admin_send_dm, run_simple_order_msg, save_order};
 pub use types::{Event, ListKind};

src/util/net.rs

@@ -14,8 +14,17 @@ const CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
 pub async fn connect_nostr() -> Result<Client> {
     let my_keys = Keys::generate();
 
-    let relays = var("RELAYS").expect("RELAYS is not set");
-    let relays = relays.split(',').collect::<Vec<&str>>();
+    let relays = var("RELAYS").map_err(|_| anyhow::anyhow!("RELAYS is not set"))?;
+    // Trim each entry and drop empty ones so stray whitespace or trailing commas
+    // (e.g. `RELAYS="wss://a, ,wss://b,"`) don't reach `add_relay` and fail.
+    let relays = relays
+        .split(',')
+        .map(str::trim)
+        .filter(|r| !r.is_empty())
+        .collect::<Vec<&str>>();
+    if relays.is_empty() {
+        return Err(anyhow::anyhow!("RELAYS is not set"));
+    }
     let client = Client::new(my_keys);
     for r in relays.into_iter() {
         client.add_relay(r).await?;

tests/util_misc.rs

@@ -1,4 +1,4 @@
-use mostro_client::util::misc::{get_mcli_path, uppercase_first};
+use mostro_client::util::misc::{ensure_private_dir, get_mcli_path, uppercase_first};
 
 #[test]
 fn test_uppercase_first_empty_string() {
@@ -86,3 +86,79 @@ fn test_get_mcli_path_consistent() {
     // Should return the same path on multiple calls
     assert_eq!(path1, path2);
 }
+
+/// Build a unique, non-existent path inside the OS temp dir without pulling in
+/// extra crates or the forbidden `Date`/random APIs.
+fn unique_temp_path(label: &str) -> std::path::PathBuf {
+    use std::sync::atomic::{AtomicU64, Ordering};
+    static COUNTER: AtomicU64 = AtomicU64::new(0);
+    let n = COUNTER.fetch_add(1, Ordering::Relaxed);
+    std::env::temp_dir().join(format!("mcli-test-{}-{}-{}", label, std::process::id(), n))
+}
+
+#[cfg(unix)]
+#[test]
+fn test_ensure_private_dir_creates_with_0700() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let dir = unique_temp_path("dir");
+    let dir_str = dir.to_str().unwrap();
+
+    ensure_private_dir(dir_str).expect("should create directory");
+
+    assert!(dir.is_dir(), "directory should exist");
+    let mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777;
+    assert_eq!(mode, 0o700, "directory must be private to the owner");
+
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[cfg(unix)]
+#[test]
+fn test_ensure_private_dir_tightens_existing_dir() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let dir = unique_temp_path("loose");
+    let dir_str = dir.to_str().unwrap();
+
+    // Simulate a directory left world-readable by a permissive umask.
+    std::fs::create_dir(&dir).unwrap();
+    std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap();
+
+    ensure_private_dir(dir_str).expect("should tighten existing directory");
+
+    let mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777;
+    assert_eq!(mode, 0o700, "existing directory must be tightened to 0700");
+
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn test_ensure_private_dir_is_idempotent() {
+    let dir = unique_temp_path("idem");
+    let dir_str = dir.to_str().unwrap();
+
+    ensure_private_dir(dir_str).expect("first call should succeed");
+    ensure_private_dir(dir_str).expect("second call should succeed");
+
+    assert!(dir.is_dir());
+
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn test_ensure_private_dir_rejects_non_directory() {
+    let path = unique_temp_path("file");
+    let path_str = path.to_str().unwrap();
+
+    // A regular file already occupies the path: we must not chmod it and report
+    // success, since callers were promised a directory.
+    std::fs::write(&path, b"not a dir").unwrap();
+
+    assert!(
+        ensure_private_dir(path_str).is_err(),
+        "must fail when the path exists but is not a directory"
+    );
+
+    std::fs::remove_file(&path).ok();
+}