Merge pull request 'fix(colibri-vault): harden bw interaction — server-match fail-closed + serialize + note-key validation' (#100) from fix/colibri-vault-bw-hardening into main

crates/colibri-vault/Cargo.toml

@@ -9,4 +9,4 @@ serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 thiserror = "2"
 tracing = "0.1"
-tokio = { version = "1", features = ["process", "rt"] }
+tokio = { version = "1", features = ["process", "rt", "sync", "macros"] }

crates/colibri-vault/src/lib.rs

@@ -22,8 +22,10 @@
 
 use std::path::{Path, PathBuf};
 use std::process::Stdio;
+use std::sync::OnceLock;
 use thiserror::Error;
 use tokio::process::Command;
+use tokio::sync::Mutex;
 
 #[derive(Error, Debug)]
 pub enum VaultError {
@@ -39,6 +41,9 @@ pub enum VaultError {
     #[error("bw unlock returned an empty session")]
     EmptySession,
 
+    #[error("bw is logged in to server '{current}' but expected '{expected}' — logout and rerun")]
+    ServerMismatch { expected: String, current: String },
+
     #[error("collection not found: {0}")]
     CollectionNotFound(String),
 
@@ -78,6 +83,12 @@ pub async fn provision(
     std::fs::write(&test, b"")?;
     std::fs::remove_file(&test)?;
 
+    // Serialize the whole login→unlock→fetch→lock lifecycle: `bw` keeps
+    // process-global state (one configured server + session per process), so
+    // concurrent provisions could otherwise tear down each other's session
+    // mid-fetch or race on `bw config server`.
+    let _guard = vault_lock().lock().await;
+
     let session = VaultSession::login_unlock().await?;
     let result = provision_with_session(collection_name, target_dir, &session).await;
     finish_with_lock(result, session).await
@@ -104,8 +115,26 @@ impl VaultSession {
         require_bootstrap_env("BW_PASSWORD")?;
 
         if let Ok(server) = std::env::var("BW_SERVER") {
-            if !server.trim().is_empty() {
-                bw(&["config", "server", server.trim()]).await?;
+            let server = server.trim();
+            if !server.is_empty() {
+                match bw(&["config", "server", server]).await {
+                    Ok(_) => {}
+                    // bw refuses `config server` while already logged in. Tolerate
+                    // that ONLY if the already-configured server matches what we
+                    // expect; otherwise fail closed, so a stale login can't fetch
+                    // from the wrong Bitwarden host.
+                    Err(VaultError::BwFailed(msg)) if is_server_config_locked(&msg) => {
+                        let current = bw(&["config", "server"]).await.unwrap_or_default();
+                        let current = current.trim();
+                        if current != server {
+                            return Err(VaultError::ServerMismatch {
+                                expected: server.to_string(),
+                                current: current.to_string(),
+                            });
+                        }
+                    }
+                    Err(e) => return Err(e),
+                }
             }
         }
 
@@ -185,14 +214,27 @@ async fn provision_with_session(
                 env_content.push_str(&format!("{key}={val}\n"));
             }
         }
-        // Also handle secure notes (KEY=VALUE pairs separated by newlines)
+        // Also handle secure notes (KEY=VALUE pairs separated by newlines).
+        // Validate the key the same way as login items, so a note can't inject
+        // an unchecked key into the .env.
         if item.kind == 2 {
             // Secure note
             if let Some(notes) = &item.notes {
                 for line in notes.lines() {
                     let line = line.trim();
-                    if line.contains('=') && !line.starts_with('#') {
-                        env_content.push_str(&format!("{line}\n"));
+                    if line.is_empty() || line.starts_with('#') {
+                        continue;
+                    }
+                    if let Some((raw_key, val)) = line.split_once('=') {
+                        let key = validate_key(raw_key.trim());
+                        if key.is_empty() {
+                            tracing::warn!(
+                                item = item.name,
+                                "skipping note line with invalid env key"
+                            );
+                            continue;
+                        }
+                        env_content.push_str(&format!("{key}={}\n", val.trim()));
                     }
                 }
             }
@@ -287,6 +329,21 @@ fn is_already_logged_in(message: &str) -> bool {
     msg.contains("already logged in") || msg.contains("you are logged in")
 }
 
+/// True when `bw config server` is refused because the CLI is already
+/// authenticated (matches the clawdie-vault-fetch helper's tolerated cases).
+fn is_server_config_locked(message: &str) -> bool {
+    let msg = message.to_ascii_lowercase();
+    msg.contains("logout required")
+        || msg.contains("already configured")
+        || msg.contains("already set")
+}
+
+/// Process-wide lock serializing all `bw` interaction (login/unlock/fetch/lock).
+fn vault_lock() -> &'static Mutex<()> {
+    static VAULT_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
+    VAULT_LOCK.get_or_init(|| Mutex::new(()))
+}
+
 fn which(cmd: &str) -> Option<PathBuf> {
     std::env::var_os("PATH").and_then(|path| {
         std::env::split_paths(&path).find_map(|dir| {
@@ -379,6 +436,24 @@ mod tests {
         assert!(!is_already_logged_in("Invalid client secret."));
     }
 
+    #[test]
+    fn server_config_locked_detection_matches_bw_cli_text() {
+        assert!(is_server_config_locked(
+            "Logout required before server config update."
+        ));
+        assert!(is_server_config_locked("Server is already configured."));
+        assert!(!is_server_config_locked("Invalid client secret."));
+    }
+
+    #[tokio::test]
+    async fn vault_lock_serializes() {
+        // Two holders cannot both hold the lock at once.
+        let g1 = vault_lock().lock().await;
+        assert!(vault_lock().try_lock().is_err());
+        drop(g1);
+        assert!(vault_lock().try_lock().is_ok());
+    }
+
     #[test]
     fn validate_key_rejects_dangerous_chars() {
         assert_eq!(validate_key("MY_KEY"), "MY_KEY");