diff --git a/src/app/bgbash/job.rs b/src/app/bgbash/job.rs index 12bb9b7..dfbb090 100644 --- a/src/app/bgbash/job.rs +++ b/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) { diff --git a/src/app/runtime/actions/mod.rs b/src/app/runtime/actions/mod.rs index 5cc2477..b812ff0 100644 --- a/src/app/runtime/actions/mod.rs +++ b/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 { 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 { 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 { + 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() } diff --git a/src/model/memory.rs b/src/model/memory.rs index 5253f13..68b5ff1 100644 --- a/src/model/memory.rs +++ b/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(()) diff --git a/src/model/msglog/mod.rs b/src/model/msglog/mod.rs index a3d9f35..d8cce85 100644 --- a/src/model/msglog/mod.rs +++ b/src/model/msglog/mod.rs @@ -22,6 +22,8 @@ pub fn open_or_create(session_dir: &std::path::Path) -> anyhow::Result Result<()> { + conn.execute_batch("PRAGMA foreign_keys = ON;")?; conn.execute_batch( " CREATE TABLE IF NOT EXISTS messages ( diff --git a/src/service/oauth/loopback.rs b/src/service/oauth/loopback.rs index 83fc152..4f601b2 100644 --- a/src/service/oauth/loopback.rs +++ b/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 { + /// 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 { 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 { + fn read_callback(stream: &mut TcpStream, expected_state: &str) -> std::io::Result { 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 { + 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). diff --git a/src/service/oauth/pkce.rs b/src/service/oauth/pkce.rs index 2cec0b3..b950c75 100644 --- a/src/service/oauth/pkce.rs +++ b/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. diff --git a/src/tool/git_operator.rs b/src/tool/git_operator.rs index df093a8..8b73eac 100644 --- a/src/tool/git_operator.rs +++ b/src/tool/git_operator.rs @@ -38,11 +38,14 @@ impl Tool for GitOperator { /// Run `git [args...]` and return its combined output. /// - /// Flow: extract `operation` + `args` → spawn `git ` → trim and - /// join stdout/stderr. + /// Flow: extract `operation` + `args` → gate through `shell_filter::git` + /// to block destructive operations → spawn `git ` → + /// 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) diff --git a/src/tool/mod.rs b/src/tool/mod.rs index 342d074..3b0b953 100644 --- a/src/tool/mod.rs +++ b/src/tool/mod.rs @@ -203,7 +203,34 @@ pub fn resolve_path(workspaces: &[PathBuf], rel: &str) -> Result { } 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 { diff --git a/src/tool/shell_filter/credentials.rs b/src/tool/shell_filter/credentials.rs index 8c1c313..ddbf710 100644 --- a/src/tool/shell_filter/credentials.rs +++ b/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()); + } +} diff --git a/src/tool/shell_filter/git.rs b/src/tool/shell_filter/git.rs index f469216..f7f9b43 100644 --- a/src/tool/shell_filter/git.rs +++ b/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()); + } +}