diff --git a/src/agent/recovery.rs b/src/agent/recovery.rs index 61b54cdc..184a8d52 100644 --- a/src/agent/recovery.rs +++ b/src/agent/recovery.rs @@ -75,13 +75,56 @@ impl RecoveryPolicy { /// `:` being absent (some providers emit `retry-after 30`). pub(crate) fn retry_after_from_error_msg(msg: &str) -> Option { fn parse_after_label(msg: &str, label: &str) -> Option { - let lower = msg.to_lowercase(); - let idx = lower.find(label)?; - // Skip past label + optional `:` / whitespace / quote. - let tail = &msg[idx + label.len()..]; + // Case-insensitive search WITHOUT lowercasing the whole + // message: previously we lowercased `msg` and then indexed + // into the ORIGINAL `msg` at the lowered string's byte + // offset. For ASCII that's identical, but `to_lowercase` + // can change byte length for some unicode (e.g. Turkish + // `İ` → `i̇` is 2 → 3 bytes). The mismatched offset could + // land mid-UTF-8 and panic on `&msg[...]`. Now we scan the + // original bytes window-by-window with case-insensitive + // ASCII comparison. The label itself is fixed-ASCII so this + // is sound — we just need to be case-insensitive against + // the message's casing. + let label_bytes = label.as_bytes(); + let msg_bytes = msg.as_bytes(); + if msg_bytes.len() < label_bytes.len() { + return None; + } + let mut idx = None; + for i in 0..=msg_bytes.len() - label_bytes.len() { + let window = &msg_bytes[i..i + label_bytes.len()]; + if window + .iter() + .zip(label_bytes.iter()) + .all(|(a, b)| a.eq_ignore_ascii_case(b)) + { + idx = Some(i); + break; + } + } + let idx = idx?; + // `idx` is now a byte offset into the original `msg`. + // Land at a char boundary (the ASCII label match guarantees + // we're on a boundary, but `idx + label.len()` could still + // hit one — for ASCII labels it can't, but defend anyway). + let after = idx + label.len(); + if !msg.is_char_boundary(after) { + return None; + } + let tail = &msg[after..]; let tail = tail.trim_start_matches([':', ' ', '\t', '"']).trim_start(); - // Consume contiguous digits. - let n: String = tail.chars().take_while(|c| c.is_ascii_digit()).collect(); + // Consume contiguous digits, with a hard cap so a malformed + // header (`Retry-After: 999999999999999999999`) doesn't + // produce a parsed integer that overflows or is absurdly + // large before the 5-min cap applies in the caller. Cap at + // 10^10 — any value larger is clearly bogus, and the cap + // saturates rather than overflowing u64. + let n: String = tail + .chars() + .take_while(|c| c.is_ascii_digit()) + .take(11) + .collect(); if n.is_empty() { return None; } @@ -479,6 +522,61 @@ mod tests { assert_eq!(retry_after_from_error_msg(msg), None); } + /// Regression: messages with multi-byte UTF-8 BEFORE the label + /// previously could panic — the original parser found the + /// label in a lowercased copy and indexed into the original + /// at that byte offset. `to_lowercase` can change byte length + /// (Turkish `İ` is 2 bytes lowercase as `i̇` = 3 bytes), so + /// the offsets disagreed and `&msg[idx + label.len()..]` could + /// land mid-UTF-8 → panic. Now the search is on byte windows + /// of the original string with case-insensitive ASCII compare. + #[test] + fn retry_after_handles_unicode_before_label() { + // Provider error message with a Turkish capital I before + // the label. Lowercasing produces a different byte length. + let msg = "İoError: Retry-After: 8"; + assert_eq!( + retry_after_from_error_msg(msg), + Some(Duration::from_secs(8)), + ); + } + + /// Case-insensitive matching against the label name itself. + /// `RETRY-AFTER-MS` and `retry-after-ms` should both parse. + #[test] + fn retry_after_label_match_is_case_insensitive() { + assert_eq!( + retry_after_from_error_msg("rate limited: RETRY-AFTER-MS: 750"), + Some(Duration::from_millis(750)), + ); + assert_eq!( + retry_after_from_error_msg("Retry-After-Ms: 750"), + Some(Duration::from_millis(750)), + ); + } + + /// Pathological huge digit run: cap at 11 digits before parse, + /// so `Retry-After: 999999999999999999999...` doesn't overflow + /// or produce a 100-year wait before the upper cap clamps. + #[test] + fn retry_after_caps_pathological_digit_run() { + let msg = "Retry-After: 99999999999999999999999"; + let parsed = retry_after_from_error_msg(msg); + // 11 digits = max ~10^11 seconds — `backoff_duration_for_msg` + // will cap at 5 minutes, but the unsanitized parse must + // produce SOMETHING (not None, not a panic). We don't pin + // the exact value; just verify it's bounded by the cap + // behavior in `backoff_duration_for_msg`. + assert!(parsed.is_some(), "must parse, not return None"); + let policy = RecoveryPolicy::default(); + let d = policy.backoff_duration_for_msg(0, msg); + assert!( + d <= Duration::from_secs(300), + "backoff must cap at 5min; got {:?}", + d, + ); + } + /// `backoff_duration_for_msg` picks the longer of the /// computed exponential backoff and the server's retry-after, /// capped at 5 minutes. diff --git a/src/agent/tools/bash.rs b/src/agent/tools/bash.rs index 8de53e5d..4d227266 100644 --- a/src/agent/tools/bash.rs +++ b/src/agent/tools/bash.rs @@ -263,13 +263,24 @@ async fn check_bash_segments( // a backslash escape. let segments = quote_aware_split(command); - // Flag command substitution / subshell constructs that need a - // full parser. Surface as one whole-command check so the user - // sees the unfamiliar form before any segment runs. + // Flag command substitution / subshell constructs / ANSI-C + // quoting that need a full parser. Surface as one + // whole-command check so the user sees the unfamiliar form + // before any segment runs. + // + // `$'...'` ANSI-C quoting was missing from the original + // check, leaving a small bypass: `echo $'hi\nrm -rf /; ls'` + // (with embedded literal newlines via `\n`) treated the body + // as one quoted token, so `quote_aware_split` didn't see the + // `;` as a separator. Adding `$'` to the substitution list + // makes the whole command get checked as a single string — + // the rules can still match the safe form, but the LLM + // doesn't get a free pass on obscure quoting. let has_substitution = command.contains("$(") || command.contains('`') || command.contains("<(") - || command.contains(">("); + || command.contains(">(") + || command.contains("$'"); if has_substitution { return check_perm(permission, ask_tx, "bash", command).await; } diff --git a/src/permission/checker.rs b/src/permission/checker.rs index b6704867..6cd8e40f 100644 --- a/src/permission/checker.rs +++ b/src/permission/checker.rs @@ -20,6 +20,14 @@ pub struct PermissionChecker { ext_dir_rules: Vec<(Pattern, Action)>, doom_loop_action: Action, working_dir: String, + /// Cached canonical form of `working_dir`, computed once at + /// construction (and refreshed by `set_working_dir`). Used by + /// `is_external_path` to compare canonical paths without + /// hitting the filesystem on every permission check — the + /// canonicalize syscall is otherwise called once per + /// read/write/edit/grep call, accumulating to hundreds of + /// stat()s per session. + working_dir_canonical: String, session_allowlist: Vec<(String, Pattern)>, recent_calls: VecDeque<(String, String)>, mode: SecurityMode, @@ -105,6 +113,7 @@ impl PermissionChecker { .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()) .to_string_lossy() .to_string(); + let working_dir_canonical = canonicalize_for_cache(&working_dir); PermissionChecker { rules, @@ -112,6 +121,7 @@ impl PermissionChecker { ext_dir_rules, doom_loop_action, working_dir, + working_dir_canonical, session_allowlist: Vec::new(), recent_calls: VecDeque::with_capacity(16), mode, @@ -320,6 +330,7 @@ impl PermissionChecker { pub fn set_working_dir(&mut self, dir: &str) { self.working_dir = dir.to_string(); + self.working_dir_canonical = canonicalize_for_cache(dir); } fn is_path_tool(&self, tool: &str) -> bool { @@ -349,12 +360,15 @@ impl PermissionChecker { return false; } let cwd = Path::new(&self.working_dir); - // Canonicalize cwd for the comparison too — if a symlinked - // working_dir or one with `..` segments was stored, a - // canonicalized `resolved` could no longer share the - // prefix even when it's the same directory. - let canonical_cwd = std::fs::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf()); - !p.starts_with(&canonical_cwd) && !p.starts_with(cwd) + // Canonical cwd is precomputed (see `working_dir_canonical`). + // Comparing against BOTH the canonical and literal forms + // handles symlinked roots like macOS's `/tmp → /private/tmp`: + // `resolved` is canonical (`/private/tmp/...`) but `cwd` + // may still be the literal `/tmp` form. Without both checks + // every in-tree access in such a setup would classify as + // external. + let canonical_cwd = Path::new(&self.working_dir_canonical); + !p.starts_with(canonical_cwd) && !p.starts_with(cwd) } fn match_ext_dir(&self, path_str: &str) -> Option { @@ -384,6 +398,18 @@ impl PermissionChecker { } } +/// One-shot canonicalize for the working-directory cache. Best +/// effort: if canonicalize fails (cwd doesn't exist on disk, e.g. +/// in tests that pass a fixture path), fall back to the literal +/// string so the `starts_with` comparisons in `is_external_path` +/// still work for the literal form. +fn canonicalize_for_cache(working_dir: &str) -> String { + std::fs::canonicalize(working_dir) + .ok() + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_else(|| working_dir.to_string()) +} + fn resolve_absolute(path: &str, working_dir: &str) -> String { let p = Path::new(path); let joined = if p.is_absolute() {