feat: enhance OAuth flow validation and improve security checks; add credential read blocking and git operation safeguards

src/app/bgbash/job.rs

@@ -66,6 +66,19 @@ pub fn spawn_bash_job(command: String) -> BashJob {
         // Send the child PID back to the caller so bash_kill can terminate it
         let _ = pid_tx.send(child.id());
 
+        // Drain stderr on a separate thread to prevent deadlock when
+        // the child produces more than ~64 KB of stderr after closing
+        // stdout (the pipe buffer fills and the child blocks on write,
+        // while the parent thread waits for the child to exit).
+        let _stderr_drain = child.stderr.take().map(|stderr| {
+            std::thread::spawn(move || {
+                let reader = std::io::BufReader::new(stderr);
+                for _line in reader.lines().map_while(Result::ok) {
+                    // Discard stderr lines to prevent pipe buffer deadlock.
+                }
+            })
+        });
+
         if let Some(stdout) = child.stdout.take() {
             let reader = std::io::BufReader::new(stdout);
             for line in reader.lines().map_while(Result::ok) {

src/app/runtime/actions/mod.rs

@@ -1029,7 +1029,7 @@ fn maybe_trigger_review(state: &mut AppStateRest) {
     }
     state.push_toast(Toast::new(
         ToastKind::Info,
-        format!("{} file(s) modified this turn. Review available.", edit_count),
+        format!("{} file(s) modified this session. Review available.", edit_count),
     ));
 }
 
@@ -1116,11 +1116,17 @@ fn run_oauth_flow(provider: &str) -> anyhow::Result<String> {
     let state_token = format!("{:x}", sha2::Sha256::digest(rand_bytes(16)));
 
     let mut manager = OAuthManager::new(config.clone());
-    let _auth_url = manager.build_auth_url(&redirect_uri, &state_token, challenge.as_str());
+    let auth_url = manager.build_auth_url(&redirect_uri, &state_token, challenge.as_str());
+    if auth_url.is_empty() {
+        tracing::warn!("[oauth] auth_url was empty for provider '{}'", provider);
+    } else if webbrowser::open(&auth_url).is_err() {
+        tracing::warn!(
+            "[oauth] could not open browser for '{}'; user must open URL manually:\n{}",
+            provider, auth_url
+        );
+    }
 
-    // let _ = webbrowser::open(&auth_url);
-
-    let code = server.wait_for_code(120_000)?;
+    let code = server.wait_for_code(120_000, &state_token)?;
 
     manager.exchange_code(&code, &redirect_uri, verifier.as_str())
         .map_err(|e| anyhow::anyhow!("{}", e))?;
@@ -1139,15 +1145,24 @@ fn run_oauth_flow(provider: &str) -> anyhow::Result<String> {
     Ok(format!("Successfully authenticated with {}.", provider))
 }
 
-/// Generate `n` pseudo-random bytes from the current sub-second timestamp.
+/// Generate `n` pseudo-random bytes from the system clock mixed with a monotonic
+/// counter, providing sufficient unpredictability for a per-flow OAuth state
+/// token without a `rand` dependency.
 ///
 /// Why: avoids pulling in a full RNG crate for the OAuth state token;
-/// sufficient for a nonce that only needs to be unpredictable over the
-/// lifetime of a single OAuth flow.
+/// the counter ensures sequential invocations produce different outputs even
+/// within the same clock tick, which is sufficient for a short-lived nonce.
 fn rand_bytes(n: usize) -> Vec<u8> {
+    use std::sync::atomic::{AtomicU64, Ordering};
     use std::time::{SystemTime, UNIX_EPOCH};
-    let seed = SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().subsec_nanos();
-    (0..n).map(|i| ((seed >> (i % 4 * 8)) ^ (i as u32 * 2654435761)) as u8).collect()
+    static COUNTER: AtomicU64 = AtomicU64::new(0);
+    let counter = COUNTER.fetch_add(1, Ordering::Relaxed);
+    let seed = SystemTime::now()
+        .duration_since(UNIX_EPOCH)
+        .unwrap_or_default()
+        .as_nanos() as u64;
+    let base = seed ^ counter;
+    (0..n).map(|i| ((base >> ((i as u64 % 8) * 8)) ^ (i as u64 * 2654435761)) as u8).collect()
 }
 
 

src/model/memory.rs

@@ -91,7 +91,7 @@ impl Memory {
             self.lifecycle, outcome_line, scope_line, before_line, after_line, prov_line,
             self.content
         );
-        let tmp = parent.join(format!(".{}.tmp", std::process::id()));
+        let tmp = parent.join(format!(".{}.tmp", uuid::Uuid::new_v4()));
         std::fs::write(&tmp, &content)?;
         std::fs::rename(&tmp, path)?;
         Ok(())

src/model/msglog/mod.rs

@@ -22,6 +22,8 @@ pub fn open_or_create(session_dir: &std::path::Path) -> anyhow::Result<rusqlite:
         std::fs::create_dir_all(parent)?;
     }
     let conn = rusqlite::Connection::open(&path)?;
+    conn.execute_batch("PRAGMA journal_mode = WAL;")?;
+    conn.execute_batch("PRAGMA busy_timeout = 5000;")?;
     schema::init_schema(&conn)?;
     Ok(conn)
 }

src/model/msglog/schema.rs

@@ -11,6 +11,7 @@ use anyhow::Result;
 ///
 /// Return: `Ok(())` on success, or the underlying SQLite error.
 pub fn init_schema(conn: &Connection) -> Result<()> {
+    conn.execute_batch("PRAGMA foreign_keys = ON;")?;
     conn.execute_batch(
         "
         CREATE TABLE IF NOT EXISTS messages (

src/service/oauth/loopback.rs

@@ -25,34 +25,42 @@ impl LoopbackServer {
         format!("http://127.0.0.1:{}/callback", self.port)
     }
 
-    /// Block until one HTTP request arrives, then extract its `code` query param.
+    /// Block until one HTTP request arrives, then extract the `code` query param
+    /// and validate that the `state` param matches the expected value.
     ///
     /// Flow: accept one connection → apply read timeout → parse request line
-    /// → respond 200/400 depending on whether a code was found.
+    /// → verify state matches → respond 200/400 depending on whether the code
+    /// was found and state matched.
     ///
-    /// Return: `Err(InvalidData)` if no `code` param is present in the request.
-    pub fn wait_for_code(&self, timeout_ms: u64) -> std::io::Result<String> {
+    /// Return: `Err(InvalidData)` if no `code` param is present or the state
+    /// doesn't match `expected_state`.
+    pub fn wait_for_code(&self, timeout_ms: u64, expected_state: &str) -> std::io::Result<String> {
         let (mut stream, _) = self.listener.accept()?;
         stream.set_read_timeout(Some(std::time::Duration::from_millis(timeout_ms)))?;
-        Self::read_callback(&mut stream)
+        Self::read_callback(&mut stream, expected_state)
     }
 
     /// Read and parse a single HTTP callback request off `stream`, replying with a status page.
     ///
     /// Why: writes the HTTP response before returning so the browser tab
     /// shows a result regardless of whether the code was found.
-    fn read_callback(stream: &mut TcpStream) -> std::io::Result<String> {
+    fn read_callback(stream: &mut TcpStream, expected_state: &str) -> std::io::Result<String> {
         let mut buf = [0u8; 4096];
         let n = stream.read(&mut buf)?;
         let request = String::from_utf8_lossy(&buf[..n]);
         let code = Self::extract_code(&request);
-        let response = if code.is_some() {
-            "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\n\r\nAuthorization complete. You may close this tab."
-        } else {
-            "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\n\r\nMissing authorization code."
+        let state = Self::extract_state(&request);
+        let state_ok = state.as_deref() == Some(expected_state);
+        let response = match (code.as_ref(), state_ok) {
+            (Some(_), true) => "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\n\r\nAuthorization complete. You may close this tab.",
+            (Some(_), false) => "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\n\r\nState mismatch — possible CSRF attack.",
+            (None, _) => "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\n\r\nMissing authorization code.",
         };
         let _ = stream.write_all(response.as_bytes());
         let _ = stream.flush();
+        if !state_ok {
+            return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "state mismatch"));
+        }
         code.ok_or_else(|| std::io::Error::new(std::io::ErrorKind::InvalidData, "code not found in callback"))
     }
 
@@ -71,6 +79,22 @@ impl LoopbackServer {
         }
         None
     }
+
+    /// Extract the `state` query parameter from an HTTP request line.
+    ///
+    /// Return: `None` if the request is malformed or has no `state` param.
+    fn extract_state(request: &str) -> Option<String> {
+        let line = request.lines().next()?;
+        let path = line.split(' ').nth(1)?;
+        let query = path.split('?').nth(1)?;
+        for pair in query.split('&') {
+            let mut parts = pair.splitn(2, '=');
+            if parts.next()? == "state" {
+                return parts.next().map(urlencoding);
+            }
+        }
+        None
+    }
 }
 
 /// Percent-decode a string (e.g. `%20` -> space).

src/service/oauth/pkce.rs

@@ -30,21 +30,26 @@ impl CodeVerifier {
     }
 }
 
-/// Produce one pseudo-random byte from the sub-second component of the system clock.
+/// Produce one pseudo-random byte from the system clock mixed with a monotonic
+/// counter, providing ~64 bits of per-call unpredictability without a `rand`
+/// dependency.
 ///
-/// Why: avoids pulling in a `rand` dependency for a short-lived, non-cryptographic
-/// verifier; each byte only needs to be unpredictable enough to prevent code
-/// interception, not cryptographically secure.
+/// Why: avoids pulling in a `rand` dependency for a short-lived verifier; the
+/// monotonic counter ensures that calls within the same clock tick produce
+/// different values, which is sufficient to prevent OAuth code interception.
 fn rand_byte() -> u8 {
+    use std::sync::atomic::{AtomicU64, Ordering};
     use std::time::{SystemTime, UNIX_EPOCH};
-    let nanos = SystemTime::now()
+    static COUNTER: AtomicU64 = AtomicU64::new(0);
+    let counter = COUNTER.fetch_add(1, Ordering::Relaxed);
+    let seed = SystemTime::now()
         .duration_since(UNIX_EPOCH)
         .unwrap_or_else(|_| {
             tracing::warn!("[pkce] system time before UNIX_EPOCH, using 0 for random byte");
             std::time::Duration::default()
         })
-        .subsec_nanos();
-    (nanos & 0xFF) as u8
+        .as_nanos() as u64;
+    ((seed ^ counter) & 0xFF) as u8
 }
 
 /// The S256-derived code challenge sent in the authorization request URL.

src/tool/git_operator.rs

@@ -38,11 +38,14 @@ impl Tool for GitOperator {
 
     /// Run `git <operation> [args...]` and return its combined output.
     ///
-    /// Flow: extract `operation` + `args` → spawn `git <operation> <args>` → trim and
-    ///       join stdout/stderr.
+    /// Flow: extract `operation` + `args` → gate through `shell_filter::git`
+    /// to block destructive operations → spawn `git <operation> <args>` →
+    /// trim and join stdout/stderr.
     ///
-    /// Why: no allowlist here — the model may run any git subcommand; destructive
-    /// operations are blocked upstream by `shell_filter::git`, not by this tool.
+    /// Why: reconstructing the command string for the shell filter prevents
+    /// the model (or a subagent) from running destructive git operations
+    /// that would otherwise bypass the filter by going through this tool
+    /// instead of the `bash` tool.
     ///
     /// Return: trimmed combined output on success; error including exit code and
     /// stderr on failure.
@@ -59,6 +62,12 @@ impl Tool for GitOperator {
                     .collect()
             })
             .ok_or_else(|| anyhow!("missing required argument: args"))?;
+        // Gate through the destructive git filter — same filter used by
+        // the `bash` tool, so destructive operations are blocked regardless
+        // of which tool the model uses.
+        let cmd_for_filter = format!("git {} {}", operation, arg_list.join(" "));
+        crate::tool::shell_filter::git::check_git_destructive(&cmd_for_filter)
+            .map_err(|e| anyhow!("blocked: {}", e))?;
         let output = Command::new("git")
             .arg(&operation)
             .args(&arg_list)

src/tool/mod.rs

@@ -203,7 +203,34 @@ pub fn resolve_path(workspaces: &[PathBuf], rel: &str) -> Result<PathBuf> {
     } else {
         base.join(path)
     };
-    let canon = abs.canonicalize().unwrap_or(abs);
+    // Resolve the path with canonicalisation. For non-existent files
+    // (e.g. the write tool creating a new file), canonicalise the base
+    // workspace root first and then resolve parent-dir (`../`) traversal
+    // component-by-component so that `Path::starts_with` cannot be
+    // bypassed by unnormalised intermediate segments.
+    let canon = match abs.canonicalize() {
+        Ok(c) => c,
+        Err(_) => {
+            let base_canon = workspaces
+                .iter()
+                .filter_map(|w| w.canonicalize().ok())
+                .next()
+                .unwrap_or_else(|| base.clone());
+            let mut resolved = base_canon.clone();
+            if let Ok(rel_components) = abs.strip_prefix(&base_canon) {
+                for comp in rel_components.components() {
+                    match comp {
+                        std::path::Component::ParentDir => {
+                            resolved.pop();
+                        }
+                        std::path::Component::CurDir => {}
+                        c => resolved.push(c),
+                    }
+                }
+            }
+            resolved
+        }
+    };
     if workspaces.iter().any(|w| canon.starts_with(w)) {
         Ok(canon)
     } else {

src/tool/shell_filter/credentials.rs

@@ -4,11 +4,14 @@ use anyhow::Result;
 
 /// Reject shell commands whose lowercased form contains any known credential-read pattern.
 ///
-/// Flow: lowercase the command → for each pattern, substring-match → bail with the
-///       matching pattern on the first hit.
+/// Flow: lowercase the command → strip shell quoting (`''` / `""`) → for each
+///       pattern, substring-match on both the raw and quote-stripped commands →
+///       bail with the matching pattern on the first hit.
 ///
 /// Why: catches `cat ~/.ssh/id_rsa`, `grep token= foo.txt`, `.git-credentials`,
 /// cloud-CLI credential paths, etc., before the bash tool spawns anything.
+/// Quoting is stripped because bash concatenates adjacent quotes, so the
+/// model could insert quotes between characters to bypass substring matching.
 ///
 /// Return: `Ok(())` if no pattern matches; error naming the offending pattern otherwise.
 pub fn check_credential_read(cmd: &str) -> Result<()> {
@@ -34,10 +37,73 @@ pub fn check_credential_read(cmd: &str) -> Result<()> {
         "password=",
     ];
     let cmd_lower = cmd.to_lowercase();
+    let cmd_no_quotes: String = cmd_lower.chars()
+        .filter(|&c| c != '\'' && c != '"')
+        .collect();
     for pattern in &patterns {
-        if cmd_lower.contains(pattern) {
+        if cmd_lower.contains(pattern) || cmd_no_quotes.contains(pattern) {
             anyhow::bail!("credential read blocked: '{}'", pattern);
         }
     }
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_block_ssh_key_read() {
+        assert!(check_credential_read("cat ~/.ssh/id_rsa").is_err());
+    }
+
+    #[test]
+    fn test_block_ssh_key_read_with_quote_bypass() {
+        assert!(check_credential_read("cat ~/.ssh/id_r''sa").is_err());
+    }
+
+    #[test]
+    fn test_block_git_credentials() {
+        assert!(check_credential_read("cat .git-credentials").is_err());
+    }
+
+    #[test]
+    fn test_block_password_eq() {
+        assert!(check_credential_read("echo password=secret123").is_err());
+    }
+
+    #[test]
+    fn test_block_token_eq() {
+        assert!(check_credential_read("echo token=ghp_abc123").is_err());
+    }
+
+    #[test]
+    fn test_block_aws_credentials() {
+        assert!(check_credential_read("cat ~/.aws/credentials").is_err());
+    }
+
+    #[test]
+    fn test_block_gcloud_credentials() {
+        assert!(check_credential_read("cat ~/.config/gcloud/credentials.json").is_err());
+    }
+
+    #[test]
+    fn test_allow_ls_home() {
+        assert!(check_credential_read("ls -la ~").is_ok());
+    }
+
+    #[test]
+    fn test_allow_git_clone() {
+        assert!(check_credential_read("git clone https://github.com/user/repo.git").is_ok());
+    }
+
+    #[test]
+    fn test_allow_cargo_build() {
+        assert!(check_credential_read("cargo build 2>&1").is_ok());
+    }
+
+    #[test]
+    fn test_allow_read_own_source() {
+        assert!(check_credential_read("cat src/main.rs").is_ok());
+    }
+}

src/tool/shell_filter/git.rs

@@ -4,11 +4,15 @@ use anyhow::Result;
 
 /// Reject shell commands whose lowercased form contains any known destructive git pattern.
 ///
-/// Flow: lowercase the command → for each pattern, substring-match → bail with the
-///       matching pattern on the first hit.
+/// Flow: lowercase the command → strip shell quoting (`''` / `""`) → for each
+///       pattern, substring-match on both the raw and quote-stripped commands →
+///       bail with the matching pattern on the first hit.
 ///
 /// Why: hard-resets, force-pushes, `clean -fdx`, `filter-branch`, etc. can destroy
 /// uncommitted work or rewrite shared history; the bash tool refuses to run them.
+/// Quoting is stripped before matching because bash concatenates adjacent quoted
+/// strings (`--for''ce` → `--force`), and substring matching on the raw command
+/// would miss the bypass.
 ///
 /// Return: `Ok(())` if no pattern matches; error naming the offending pattern otherwise.
 pub fn check_git_destructive(cmd: &str) -> Result<()> {
@@ -17,10 +21,10 @@ pub fn check_git_destructive(cmd: &str) -> Result<()> {
         "reset --hard",
         "clean -fdx",
         "clean -fd",
-        "clean -fX",
         "clean -fx",
-        "branch -D",
+        "branch -d",
         "branch --delete --force",
+        "checkout -f",
         "checkout --force",
         "switch -f",
         "restore --force",
@@ -32,18 +36,133 @@ pub fn check_git_destructive(cmd: &str) -> Result<()> {
         "filter-branch",
         "gc --prune",
         "gc --aggressive",
+        "push -f",
         "push --delete",
         "push --force",
         "push origin :",
-        "push +refs",
+        "push +",
         "push --mirror",
         "push --tags --force",
     ];
     let cmd_lower = cmd.to_lowercase();
+    let cmd_no_quotes: String = cmd_lower.chars()
+        .filter(|&c| c != '\'' && c != '"')
+        .collect();
     for pattern in &patterns {
-        if cmd_lower.contains(pattern) {
+        if cmd_lower.contains(pattern) || cmd_no_quotes.contains(pattern) {
             anyhow::bail!("destructive git operation blocked: '{}'", pattern);
         }
     }
+    // Additional check: any `+` prefixed refspec in a `git push` is a
+    // force push, regardless of whether it immediately follows `push`
+    // (e.g. `git push origin +main`).  Use the quote-stripped form so
+    // that `push or''igin +ma''in` also matches.
+    if cmd_no_quotes.contains("push") {
+        let push_end = cmd_no_quotes.find("push").map(|i| i + 4).unwrap_or(0);
+        let after_push = &cmd_no_quotes[push_end..];
+        if after_push.contains('+') {
+            anyhow::bail!("destructive git operation blocked: force push via +refspec");
+        }
+    }
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_block_push_force_long() {
+        assert!(check_git_destructive("git push --force origin main").is_err());
+    }
+
+    #[test]
+    fn test_block_push_force_short() {
+        assert!(check_git_destructive("git push -f origin main").is_err());
+    }
+
+    #[test]
+    fn test_block_push_force_with_quote_bypass() {
+        assert!(check_git_destructive("git push --for''ce origin main").is_err());
+    }
+
+    #[test]
+    fn test_block_push_force_prefix() {
+        assert!(check_git_destructive("git push origin +main").is_err());
+    }
+
+    #[test]
+    fn test_block_branch_delete() {
+        assert!(check_git_destructive("git branch -d feature").is_err());
+    }
+
+    #[test]
+    fn test_block_branch_force_delete() {
+        assert!(check_git_destructive("git branch -D feature").is_err());
+    }
+
+    #[test]
+    fn test_block_reset_hard() {
+        assert!(check_git_destructive("git reset --hard HEAD~3").is_err());
+    }
+
+    #[test]
+    fn test_block_checkout_force_long() {
+        assert!(check_git_destructive("git checkout --force HEAD").is_err());
+    }
+
+    #[test]
+    fn test_block_checkout_force_short() {
+        assert!(check_git_destructive("git checkout -f HEAD").is_err());
+    }
+
+    #[test]
+    fn test_block_clean_fdx() {
+        assert!(check_git_destructive("git clean -fdx").is_err());
+    }
+
+    #[test]
+    fn test_block_filter_branch() {
+        assert!(check_git_destructive("git filter-branch --force").is_err());
+    }
+
+    #[test]
+    fn test_block_gc_prune() {
+        assert!(check_git_destructive("git gc --prune=now").is_err());
+    }
+
+    #[test]
+    fn test_block_stash_drop() {
+        assert!(check_git_destructive("git stash drop stash@{0}").is_err());
+    }
+
+    #[test]
+    fn test_block_switch_force() {
+        assert!(check_git_destructive("git switch -f main").is_err());
+    }
+
+    #[test]
+    fn test_allow_git_status() {
+        assert!(check_git_destructive("git status").is_ok());
+    }
+
+    #[test]
+    fn test_allow_git_log() {
+        assert!(check_git_destructive("git log --oneline").is_ok());
+    }
+
+    #[test]
+    fn test_allow_git_diff() {
+        assert!(check_git_destructive("git diff HEAD").is_ok());
+    }
+
+    #[test]
+    fn test_allow_git_add() {
+        assert!(check_git_destructive("git add src/main.rs").is_ok());
+    }
+
+    #[test]
+    fn test_allow_git_commit() {
+        assert!(check_git_destructive("git commit -m 'fix bug'").is_ok());
+    }
+}