Harden reliability and security across all top-level commands

CHANGELOG.md

@@ -1,5 +1,30 @@
 # Changelog
 
+## Unreleased
+
+### Security
+
+- Rejected API keys containing control characters. Gemini stores keys as `GEMINI_API_KEY=<key>` in a `.env` file the CLI sources, so a key containing a newline injected additional environment variables (for example `GOOGLE_CLOUD_PROJECT`) into that file. The same rule now applies to Claude and Codex.
+- Hardened profile file handling against path traversal, and switched the symlink guard to `symlink_metadata` so a dangling symlink can no longer redirect a credential write to its target.
+- `uninstall --remove-data` now purges the system-keyring secrets it created instead of stranding them, and refuses to run when `AISW_HOME` is the user's home directory.
+- Unit tests no longer resolve tool binaries from the developer's real `PATH`. Detection spawns whatever it finds to read `--version`, and running the real `claude`/`codex`/`gemini` CLIs from the test suite could rotate and invalidate live OAuth tokens.
+
+### Fixed
+
+- **Behavior change:** `aisw use --all` now exits non-zero when a tool switch fails. It previously exited `0` and reported success, contradicting the documented contract that a zero exit means success. On partial failure the `--json` output is now the standard failure envelope (`{"ok": false, "error": {...}}`) instead of `{"ok": true, ..., "warnings": [...]}`.
+- `aisw use --all` now honors `--emit-env` and `--state-mode`. `--emit-env` previously fell through to the human-readable summary, which a shell hook would then `eval`.
+- `aisw remove` no longer deletes credentials and profile files before checking whether a context still references the profile — the removal was rejected afterwards, leaving the data already destroyed.
+- Removing the active profile now clears `active` in the same locked config mutation, so config can no longer name a profile that does not exist.
+- `aisw status` no longer panics (`no entry found for key`) when `active` names a profile missing from the config; it reports the inconsistency instead.
+- `aisw init --json` no longer aborts on a shell it has no hook for (for example `/bin/sh`, the default in many containers).
+- `aisw doctor` no longer reports a false `credentials file missing` failure for every Gemini profile. It now checks the files a profile actually stores rather than one hardcoded name per tool, which also stopped `aisw verify` from inheriting the failure.
+- Antigravity is now guarded by the generated shell hooks and included in `workspace status --json` and `status --context --json`.
+- OAuth capture no longer leaves an orphaned interactive login process when a step inside the polling loop fails.
+
+### Performance
+
+- `aisw workspace check`, which the shell hook runs on every directory change, no longer probes tool binaries, credential files, and the OS keyring. It reads the active profiles from config instead.
+
 ## 0.3.6 - 2026-06-11
 
 ### Fixed

src/auth/child_guard.rs

@@ -0,0 +1,107 @@
+//! RAII wrapper that guarantees a spawned child process is reaped.
+//!
+//! The OAuth capture loops poll a live `claude auth login` / `codex login`
+//! child while watching for credentials to appear. Those loops contain
+//! fallible steps (reading a credential file, querying the keychain, polling
+//! the child), and an early `?` return there used to leave the interactive
+//! child running with the terminal still attached to it.
+
+use std::process::Child;
+
+pub(crate) struct ChildGuard {
+    child: Option<Child>,
+}
+
+impl ChildGuard {
+    pub(crate) fn new(child: Child) -> Self {
+        Self { child: Some(child) }
+    }
+
+    pub(crate) fn as_mut(&mut self) -> &mut Child {
+        self.child
+            .as_mut()
+            .expect("child is only taken in Drop, which consumes the guard")
+    }
+
+    /// Kill and reap the child now. Idempotent, and safe to call on a child
+    /// that already exited.
+    pub(crate) fn terminate(&mut self) {
+        if let Some(mut child) = self.child.take() {
+            let _ = child.kill();
+            let _ = child.wait();
+        }
+    }
+}
+
+impl Drop for ChildGuard {
+    fn drop(&mut self) {
+        self.terminate();
+    }
+}
+
+#[cfg(all(test, unix))]
+mod tests {
+    use super::*;
+    use std::process::{Command, Stdio};
+
+    fn spawn_sleeper() -> Child {
+        Command::new("/bin/sh")
+            .args(["-c", "sleep 30"])
+            .stdin(Stdio::null())
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .spawn()
+            .expect("sleeper should spawn")
+    }
+
+    fn is_running(pid: u32) -> bool {
+        // Signal 0 probes for existence without delivering a signal. A reaped
+        // child is gone entirely; a zombie would still report as present, so
+        // this also proves the guard waits rather than only killing.
+        unsafe { libc::kill(pid as i32, 0) == 0 }
+    }
+
+    #[test]
+    fn dropping_the_guard_kills_and_reaps_the_child() {
+        let _g = crate::SPAWN_LOCK.lock().unwrap_or_else(|p| p.into_inner());
+        let child = spawn_sleeper();
+        let pid = child.id();
+
+        drop(ChildGuard::new(child));
+
+        assert!(
+            !is_running(pid),
+            "guard must kill and reap the child on drop"
+        );
+    }
+
+    /// This is the case that regressed: an error inside the polling loop.
+    #[test]
+    fn an_early_error_return_does_not_orphan_the_child() {
+        let _g = crate::SPAWN_LOCK.lock().unwrap_or_else(|p| p.into_inner());
+        let child = spawn_sleeper();
+        let pid = child.id();
+
+        let result: anyhow::Result<()> = (|| {
+            let _guard = ChildGuard::new(child);
+            anyhow::bail!("simulated failure while polling for credentials")
+        })();
+
+        assert!(result.is_err());
+        assert!(!is_running(pid), "child must not outlive a failed capture");
+    }
+
+    #[test]
+    fn terminate_is_idempotent() {
+        let _g = crate::SPAWN_LOCK.lock().unwrap_or_else(|p| p.into_inner());
+        let child = spawn_sleeper();
+        let pid = child.id();
+
+        let mut guard = ChildGuard::new(child);
+        guard.terminate();
+        guard.terminate();
+        drop(guard);
+
+        assert!(!is_running(pid));
+    }
+}

src/auth/claude/api_key.rs

@@ -102,13 +102,11 @@ pub fn add_api_key_with_backend(
 
 /// Validates that the given API key is non-empty.
 pub fn validate_api_key(key: &str) -> Result<()> {
-    if key.trim().is_empty() {
-        bail!(
-            "Claude API key must not be empty.\n  \
-             Get your API key at console.anthropic.com → API Keys.",
-        );
-    }
-    Ok(())
+    crate::auth::validate_api_key_charset(
+        key,
+        "Claude",
+        "Get your API key at console.anthropic.com → API Keys.",
+    )
 }
 
 /// Reads the stored API key from a profile's credentials file.

src/auth/claude/oauth.rs

@@ -460,9 +460,13 @@ If you need a different Claude account, fully sign out of claude.com first, then
     if let Some(config_dir) = target_config_dir {
         cmd.env("CLAUDE_CONFIG_DIR", config_dir);
     }
-    let mut child = cmd
-        .spawn()
-        .with_context(|| format!("could not spawn {}", claude_bin.display()))?;
+    // Guarded so that a failure anywhere in the poll loop below (keychain read,
+    // credential read, child poll) still reaps the interactive login child
+    // instead of orphaning it with the terminal attached.
+    let mut child = crate::auth::child_guard::ChildGuard::new(
+        cmd.spawn()
+            .with_context(|| format!("could not spawn {}", claude_bin.display()))?,
+    );
 
     let deadline = Instant::now() + timeout;
 
@@ -475,8 +479,7 @@ If you need a different Claude account, fully sign out of claude.com first, then
             if let Some(current) = current {
                 let changed = keychain_before.as_deref() != Some(current.as_slice());
                 if changed {
-                    let _ = child.kill();
-                    let _ = child.wait();
+                    child.terminate();
                     return Ok(current);
                 }
             }
@@ -487,8 +490,7 @@ If you need a different Claude account, fully sign out of claude.com first, then
                 .with_context(|| format!("could not read {}", credential_path.display()))?;
             let changed = file_before.as_deref() != Some(current.as_slice());
             if changed {
-                let _ = child.kill();
-                let _ = child.wait();
+                child.terminate();
                 return Ok(current);
             }
         }
@@ -499,14 +501,14 @@ If you need a different Claude account, fully sign out of claude.com first, then
                     .with_context(|| format!("could not read {}", fallback_path.display()))?;
                 let changed = fallback_before.as_deref() != Some(current.as_slice());
                 if changed {
-                    let _ = child.kill();
-                    let _ = child.wait();
+                    child.terminate();
                     return Ok(current);
                 }
             }
         }
 
         if let Some(status) = child
+            .as_mut()
             .try_wait()
             .with_context(|| format!("could not poll {}", claude_bin.display()))?
         {
@@ -547,8 +549,7 @@ If you need a different Claude account, fully sign out of claude.com first, then
         }
 
         if Instant::now() >= deadline {
-            let _ = child.kill();
-            let _ = child.wait();
+            child.terminate();
             bail!(
                 "Claude login timed out after {}s. \
                  The browser window may still be open.",

src/auth/codex.rs

@@ -276,13 +276,11 @@ pub fn add_api_key_with_backend(
 }
 
 pub fn validate_api_key(key: &str) -> Result<()> {
-    if key.trim().is_empty() {
-        bail!(
-            "Codex API key must not be empty.\n  \
-             Get your API key at platform.openai.com → API Keys."
-        );
-    }
-    Ok(())
+    crate::auth::validate_api_key_charset(
+        key,
+        "Codex",
+        "Get your API key at platform.openai.com → API Keys.",
+    )
 }
 
 /// Start the Codex OAuth flow using the installed `codex` binary.
@@ -551,23 +549,26 @@ fn run_oauth_flow(
 ) -> Result<PathBuf> {
     let _spinner = crate::output::start_spinner("Waiting for Codex login to complete...");
 
-    let mut child = Command::new(codex_bin)
+    let child = Command::new(codex_bin)
         .arg("login")
         .env("CODEX_HOME", capture_dir)
         .spawn()
         .with_context(|| format!("could not spawn {}", codex_bin.display()))?;
+    // Guarded so a failure in the poll loop below cannot orphan the
+    // interactive login child.
+    let mut child = crate::auth::child_guard::ChildGuard::new(child);
 
     let auth_path = capture_dir.join(AUTH_FILE);
     let deadline = Instant::now() + timeout;
 
     loop {
         if auth_path.exists() {
-            let _ = child.kill();
-            let _ = child.wait();
+            child.terminate();
             return Ok(auth_path);
         }
 
         if let Some(status) = child
+            .as_mut()
             .try_wait()
             .with_context(|| format!("could not poll {}", codex_bin.display()))?
         {
@@ -585,8 +586,7 @@ fn run_oauth_flow(
         }
 
         if Instant::now() >= deadline {
-            let _ = child.kill();
-            let _ = child.wait();
+            child.terminate();
             bail!(
                 "Codex login timed out after {}s. \
                  If auth.json was not written, verify that config.toml has \

src/auth/gemini.rs

@@ -211,13 +211,11 @@ pub fn add_api_key_with_backend(
 }
 
 pub fn validate_api_key(key: &str) -> Result<()> {
-    if key.trim().is_empty() {
-        bail!(
-            "Gemini API key must not be empty.\n  \
-             Get your API key at aistudio.google.com → Get API Key."
-        );
-    }
-    Ok(())
+    crate::auth::validate_api_key_charset(
+        key,
+        "Gemini",
+        "Get your API key at aistudio.google.com → Get API Key.",
+    )
 }
 
 /// Read the stored API key from a profile's .env file.

src/auth/mod.rs

@@ -1,4 +1,5 @@
 pub mod antigravity;
+pub(crate) mod child_guard;
 pub mod claude;
 pub mod codex;
 pub(crate) mod files;
@@ -10,3 +11,74 @@ pub(crate) mod secure_store;
 pub(crate) mod system_keyring;
 pub(crate) mod test_overrides;
 pub mod token_expiry;
+
+use anyhow::{bail, Result};
+
+/// Reject an API key that is empty or contains control characters.
+///
+/// Control characters are refused because stored credentials are later
+/// materialized into formats where they change meaning rather than being
+/// escaped. Gemini writes `GEMINI_API_KEY=<key>` into a `.env` file the CLI
+/// sources, so a key containing a newline injects arbitrary additional
+/// environment variables (for example `GOOGLE_CLOUD_PROJECT`). A real key from
+/// any of these providers is a single line of printable ASCII, so nothing
+/// legitimate is rejected here.
+pub(crate) fn validate_api_key_charset(key: &str, tool_label: &str, help: &str) -> Result<()> {
+    if key.trim().is_empty() {
+        bail!("{tool_label} API key must not be empty.\n  {help}");
+    }
+    if let Some(bad) = key.chars().find(|ch| ch.is_control()) {
+        bail!(
+            "{tool_label} API key contains an invalid control character ({}).\n  \
+             Keys must be a single line — check for a stray newline from copy/paste \
+             or from piping a file into --api-key.",
+            match bad {
+                '\n' => "newline".to_owned(),
+                '\r' => "carriage return".to_owned(),
+                '\t' => "tab".to_owned(),
+                other => format!("U+{:04X}", other as u32),
+            }
+        );
+    }
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::validate_api_key_charset;
+
+    #[test]
+    fn accepts_a_normal_key() {
+        assert!(validate_api_key_charset("sk-ant-api03-AAAA", "Claude", "help").is_ok());
+    }
+
+    #[test]
+    fn rejects_empty_and_whitespace_keys() {
+        assert!(validate_api_key_charset("", "Claude", "help").is_err());
+        assert!(validate_api_key_charset("   ", "Claude", "help").is_err());
+    }
+
+    /// A newline in the key would inject extra `KEY=value` lines into Gemini's
+    /// generated `.env` file.
+    #[test]
+    fn rejects_control_characters() {
+        for key in [
+            "AIzaValid\nGOOGLE_CLOUD_PROJECT=attacker",
+            "AIzaValid\rmore",
+            "AIzaValid\tmore",
+            "AIzaValid\u{0}more",
+        ] {
+            let err = validate_api_key_charset(key, "Gemini", "help").unwrap_err();
+            assert!(
+                err.to_string().contains("control character"),
+                "expected rejection for {key:?}, got: {err}"
+            );
+        }
+    }
+
+    #[test]
+    fn error_names_the_offending_character() {
+        let err = validate_api_key_charset("abc\ndef", "Gemini", "help").unwrap_err();
+        assert!(err.to_string().contains("newline"), "got: {err}");
+    }
+}

src/backup.rs

@@ -157,6 +157,12 @@ impl BackupManager {
                     Some(n) => n.to_owned(),
                     None => continue,
                 };
+                // Directory names under backups/ are only as trustworthy as the
+                // filesystem. Never let one steer a restore outside the
+                // profiles tree.
+                if crate::profile::validate_profile_name(&profile_name).is_err() {
+                    continue;
+                }
 
                 let dest_dir = profile_store.profile_dir(tool, &profile_name);
                 fs::create_dir_all(&dest_dir).with_context(|| {