diff --git a/docs/BUILDING.md b/docs/BUILDING.md index abed8e384c0..04c4fd4f39e 100644 --- a/docs/BUILDING.md +++ b/docs/BUILDING.md @@ -150,3 +150,27 @@ cd apps/desktop-tauri && pnpm exec tsc --noEmit cargo clippy --all-targets -- -D warnings cargo fmt --all --check ``` + +## Stored process environments + +A process environment carries API keys, tokens and cookies. Every struct, union or enum-variant +field that stores one uses `codexbar::process_environment::ProcessEnvironment`, including +optional maps and `(OsString, OsString)` pair lists. Its `Debug` output is +`ProcessEnvironment(N entries; redacted)`, so `{:?}`, `dbg!`, `tracing` field captures and +`assert_eq!` failures never print variable names or values. Wrapping an `Option` keeps `None` +distinct from an empty map, and equality still compares the original contents. + +Deref, iteration, `get` and `insert` still return the original values for the child process, +so never log what they return. Function parameters and locals are transient and stay unwrapped. + +The `process_environment::storage_guard` test scans `rust/src` and +`apps/desktop-tauri/src-tauri/src` for environment-named fields whose type is a string map or a +string pair list, through `Option`, `Box`, `Arc`, references, slices and local `type` aliases, +and fails on any that is not wrapped. Each entry in its `REVIEWED_EXCEPTIONS` list must match +exactly one declaration, so stale or duplicate exceptions fail too. The guard is a lexical +tripwire, not a type checker: generic parameters, differently named fields and explicit logging +of an unwrapped map still need code review. + +```bash +cargo test -p codexbar process_environment +``` diff --git a/rust/src/agent_sessions.rs b/rust/src/agent_sessions.rs index 1780c4893e4..bd44cbcce03 100644 --- a/rust/src/agent_sessions.rs +++ b/rust/src/agent_sessions.rs @@ -543,7 +543,7 @@ impl LocalAgentSessionScanner { let pi_input = pi_family::PiFamilyScanInput { processes: &pi_processes, cwd_by_pid: std::collections::HashMap::new(), - environment: pi_family::PiFamilySessionScanner::scan_environment(), + environment: pi_family::PiFamilySessionScanner::scan_environment().into(), now, host: host.to_string(), config: self.config, diff --git a/rust/src/agent_sessions/pi_family/mod.rs b/rust/src/agent_sessions/pi_family/mod.rs index 49092987ea7..954007a585f 100644 --- a/rust/src/agent_sessions/pi_family/mod.rs +++ b/rust/src/agent_sessions/pi_family/mod.rs @@ -35,6 +35,7 @@ use super::{ AgentSessionProvider, AgentSessionSource, AgentSessionState, AgentSessionWorkspace, PiSessionDialect, SessionScanConfig, }; +use crate::process_environment::ProcessEnvironment; /// Bounded transcript prefix used for header/first-title parsing (upstream /// `maximumReadSize`). @@ -132,7 +133,7 @@ pub struct PiFamilyScanInput<'a> { /// rows result until cwd capture lands). pub cwd_by_pid: CwdByPid, /// Environment slice (HOME/USERPROFILE/PI_*/OMP_*). - pub environment: EnvMap, + pub environment: ProcessEnvironment, pub now: DateTime, pub host: String, pub config: SessionScanConfig, diff --git a/rust/src/agent_sessions/pi_family_tests.rs b/rust/src/agent_sessions/pi_family_tests.rs index 8acdc84bc09..513a10926e8 100644 --- a/rust/src/agent_sessions/pi_family_tests.rs +++ b/rust/src/agent_sessions/pi_family_tests.rs @@ -173,7 +173,7 @@ mod pi_family_tests { let input = PiFamilyScanInput { processes, cwd_by_pid, - environment, + environment: environment.into(), now, host: "fixture-host".to_string(), config: SessionScanConfig::default(), diff --git a/rust/src/cli/tty_runner.rs b/rust/src/cli/tty_runner.rs index 789bfca1318..78904de6f30 100755 --- a/rust/src/cli/tty_runner.rs +++ b/rust/src/cli/tty_runner.rs @@ -8,6 +8,7 @@ reason = "TTY runner types reserved for future interactive session management" )] +use crate::process_environment::ProcessEnvironment; use regex_lite::Regex; use std::collections::HashMap; use std::io::{Read, Write}; @@ -69,8 +70,8 @@ pub struct TtyCommandOptions { pub stop_on_substrings: Vec, /// Settle time after stopping (default: 0.25s) pub settle_after_stop_secs: f64, - /// Environment variables to set - pub env: HashMap, + /// Environment variables to set (`Debug` renders only the entry count) + pub env: ProcessEnvironment>, } impl Default for TtyCommandOptions { @@ -90,7 +91,7 @@ impl Default for TtyCommandOptions { stop_on_url: false, stop_on_substrings: Vec::new(), settle_after_stop_secs: 0.25, - env: HashMap::new(), + env: ProcessEnvironment::default(), } } } diff --git a/rust/src/core/token_accounts.rs b/rust/src/core/token_accounts.rs index 6dedc0fbde0..92b417d5d56 100755 --- a/rust/src/core/token_accounts.rs +++ b/rust/src/core/token_accounts.rs @@ -4,6 +4,7 @@ //! Supports parallel fetching and account switching. use crate::core::ProviderId; +use crate::process_environment::ProcessEnvironment; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -752,8 +753,8 @@ pub struct TokenAccountOverride { pub provider: ProviderId, /// The account being used pub account: TokenAccount, - /// Environment variables to set - pub env_override: Option>, + /// Environment variables to set (`Debug` renders only the entry count) + pub env_override: ProcessEnvironment>>, /// Cookie header to use pub cookie_header: Option, } @@ -774,7 +775,7 @@ impl TokenAccountOverride { Self { provider, account, - env_override, + env_override: env_override.into(), cookie_header, } } diff --git a/rust/src/host/command_runner.rs b/rust/src/host/command_runner.rs index 12a8a066a90..26a8089e203 100755 --- a/rust/src/host/command_runner.rs +++ b/rust/src/host/command_runner.rs @@ -18,6 +18,8 @@ use std::process::{Child, ChildStderr, ChildStdout, Command, Stdio}; use std::sync::mpsc::{self, RecvTimeoutError}; use std::time::{Duration, Instant}; +use crate::process_environment::ProcessEnvironment; + /// Command runner configuration #[derive(Debug, Clone)] pub struct CommandOptions { @@ -110,7 +112,7 @@ impl std::error::Error for CommandError {} /// Command runner for executing CLI tools pub struct CommandRunner { /// Environment variables to add. - env_additions: HashMap, + env_additions: ProcessEnvironment>, /// Whether the child inherits the ambient process environment. inherit_environment: bool, } @@ -122,7 +124,7 @@ impl CommandRunner { pub fn new() -> Self { Self { - env_additions: HashMap::new(), + env_additions: ProcessEnvironment::default(), inherit_environment: true, } } diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 1dfa17cff5d..0d1168106f0 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -19,6 +19,7 @@ pub mod login; #[cfg(windows)] pub mod managed_process; pub mod notifications; +pub mod process_environment; pub mod providers; pub mod secure_file; pub mod settings; diff --git a/rust/src/managed_process.rs b/rust/src/managed_process.rs index d6a96279142..f534df58313 100644 --- a/rust/src/managed_process.rs +++ b/rust/src/managed_process.rs @@ -50,6 +50,8 @@ use windows::Win32::System::Threading::{ }; use windows::core::{PCWSTR, PWSTR}; +use crate::process_environment::ProcessEnvironment; + /// Maximum number of terminal cursor-position replies sent to one child. const MAX_CURSOR_REPLIES: usize = 32; @@ -69,8 +71,8 @@ pub struct ManagedProcessConfig { pub program: PathBuf, /// Arguments passed to the executable. pub args: Vec, - /// Additional environment variables for the child. - pub env: Vec<(OsString, OsString)>, + /// Additional environment variables for the child (`Debug` renders only the entry count). + pub env: ProcessEnvironment>, /// Working directory, when the provider wants to pin one. pub cwd: Option, /// PTY geometry. @@ -841,7 +843,7 @@ mod tests { OsString::from("-Command"), OsString::from("Start-Sleep -Seconds 30"), ], - env: Vec::new(), + env: ProcessEnvironment::default(), cwd: None, pty_rows: 30, pty_cols: 120, @@ -1082,7 +1084,7 @@ mod tests { OsString::from("-Command"), OsString::from(script), ], - env: Vec::new(), + env: ProcessEnvironment::default(), cwd: None, pty_rows: 30, pty_cols: 120, diff --git a/rust/src/process_environment.rs b/rust/src/process_environment.rs new file mode 100644 index 00000000000..bd3f36092e9 --- /dev/null +++ b/rust/src/process_environment.rs @@ -0,0 +1,121 @@ +//! Count-only diagnostics for stored process environments (upstream 0.70.0, #4106). +//! +//! A process environment carries API keys, tokens and cookies. When a struct that stores one +//! derives `Debug`, `{:?}`, `dbg!`, a `tracing` field capture or an `assert_eq!` failure prints +//! every value. [`ProcessEnvironment`] keeps the original values for the child process and +//! renders only how many entries it holds. +//! +//! Every stored environment uses the wrapper. The `storage_guard` test scans the shipped Rust +//! sources and fails when an environment-named field holds a plain string map or string pair +//! list. + +use std::collections::{BTreeMap, HashMap}; +use std::fmt; +use std::ops::{Deref, DerefMut}; + +/// A stored process environment whose `Debug` output is `ProcessEnvironment(N entries; redacted)`. +/// +/// Explicit access (deref, iteration, `get`, `insert`) still returns the original values for +/// subprocess use, so never log what it returns. Wrapping an `Option` keeps `None` distinct from +/// an empty map, and equality compares the original contents. +#[derive(Clone, Default, PartialEq, Eq)] +pub struct ProcessEnvironment(T); + +impl ProcessEnvironment { + /// Wraps an environment for storage. + pub const fn new(value: T) -> Self { + Self(value) + } + + /// Returns the original environment, for handing to a child process. + pub fn into_inner(self) -> T { + self.0 + } +} + +impl From for ProcessEnvironment { + fn from(value: T) -> Self { + Self(value) + } +} + +impl Deref for ProcessEnvironment { + type Target = T; + + fn deref(&self) -> &T { + &self.0 + } +} + +impl DerefMut for ProcessEnvironment { + fn deref_mut(&mut self) -> &mut T { + &mut self.0 + } +} + +impl fmt::Debug for ProcessEnvironment { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "ProcessEnvironment({} entries; redacted)", + self.0.environment_entries() + ) + } +} + +impl IntoIterator for ProcessEnvironment { + type Item = T::Item; + type IntoIter = T::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.0.into_iter() + } +} + +impl<'a, T> IntoIterator for &'a ProcessEnvironment +where + &'a T: IntoIterator, +{ + type Item = <&'a T as IntoIterator>::Item; + type IntoIter = <&'a T as IntoIterator>::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + (&self.0).into_iter() + } +} + +/// The entry count of a stored environment, the only detail [`ProcessEnvironment`] renders. +pub trait EnvironmentEntries { + /// Number of variables held; an absent environment counts as zero. + fn environment_entries(&self) -> usize; +} + +impl EnvironmentEntries for HashMap { + fn environment_entries(&self) -> usize { + self.len() + } +} + +impl EnvironmentEntries for BTreeMap { + fn environment_entries(&self) -> usize { + self.len() + } +} + +impl EnvironmentEntries for Vec<(K, V)> { + fn environment_entries(&self) -> usize { + self.len() + } +} + +impl EnvironmentEntries for Option { + fn environment_entries(&self) -> usize { + self.as_ref() + .map_or(0, EnvironmentEntries::environment_entries) + } +} + +#[cfg(test)] +mod storage_guard; +#[cfg(test)] +mod tests; diff --git a/rust/src/process_environment/storage_guard.rs b/rust/src/process_environment/storage_guard.rs new file mode 100644 index 00000000000..2c8dd627486 --- /dev/null +++ b/rust/src/process_environment/storage_guard.rs @@ -0,0 +1,695 @@ +//! Lexical tripwire for stored process environments (upstream 0.70.0 +//! `ProcessEnvironmentStorageTests`). +//! +//! Scans the shipped Rust sources for struct, union and enum-variant fields whose name contains +//! `env` and whose type is a string map or a string pair list, including `Option`, `Box`, `Arc`, +//! reference, slice and local `type` alias spellings. Each one must use `ProcessEnvironment`. +//! Function parameters and locals are transient, not storage. This is not a type checker: +//! generic parameters, differently named fields and explicit logging of an unwrapped map still +//! need review. + +use std::collections::{BTreeSet, HashMap}; +use std::path::{Path, PathBuf}; +use std::sync::OnceLock; + +use regex_lite::Regex; + +/// Shipped source roots, relative to the repository root. +const SCANNED_ROOTS: &[&str] = &["rust/src", "apps/desktop-tauri/src-tauri/src"]; + +/// Reviewed fields that may keep a plain environment, as `(path, "Owner.field: Type")`. +/// +/// Each entry must match exactly one declaration; stale or duplicate matches fail the guard. +const REVIEWED_EXCEPTIONS: &[(&str, &str)] = &[]; + +/// Optional path segments before a type name, such as `std::collections::`. +const PATH: &str = r"(?:[A-Za-z_][A-Za-z0-9_]*::)*"; + +#[derive(Debug, Clone, PartialEq, Eq)] +struct Field { + line: usize, + declaration: String, +} + +/// Scanner patterns, compiled once per test binary. +struct Patterns { + item: Regex, + alias: Regex, + field: Regex, + variant_name: Regex, + reference: Regex, + protected: Regex, + container: Regex, + map: Regex, + pairs: Regex, +} + +fn patterns() -> &'static Patterns { + static PATTERNS: OnceLock = OnceLock::new(); + PATTERNS.get_or_init(|| { + let compile = |pattern: &str| { + Regex::new(pattern).unwrap_or_else(|error| panic!("pattern {pattern}: {error}")) + }; + // A string-like key or value: String, OsString, &str, &OsStr, Box/Arc/Rc, Cow. + let text = r"(?:{PATH}(?:String|OsString)|&(?:'[A-Za-z_][A-Za-z0-9_]* )?(?:str|OsStr)|{PATH}(?:Box|Arc|Rc)<(?:str|OsStr)>|{PATH}Cow<(?:'[A-Za-z_][A-Za-z0-9_]*,)?(?:str|OsStr)>)" + .replace("{PATH}", PATH); + Patterns { + item: compile(r"\b(struct|union|enum)\s+([A-Za-z_][A-Za-z0-9_]*)"), + alias: compile(r"\btype\s+([A-Za-z_][A-Za-z0-9_]*)\s*(?:<[^=;]*>)?\s*=\s*([^;]+);"), + field: compile( + r"(?s)^\s*(?:#\[[^\]]*\]\s*)*(?:pub\b\s*(?:\([^)]*\)\s*)?)?([A-Za-z_][A-Za-z0-9_]*)\s*:\s*(.+?)\s*$", + ), + variant_name: compile(r"([A-Za-z_][A-Za-z0-9_]*)\s*$"), + reference: compile(r"^&(?:'[A-Za-z_][A-Za-z0-9_]* ?)?(?:mut )?"), + protected: compile(&format!(r"^{PATH}ProcessEnvironment<")), + container: compile(&format!( + r"^{PATH}(?:Option|Box|Arc|Rc|Mutex|RwLock|RefCell|Cell)<(.*)>$" + )), + map: compile(&format!( + r"^{PATH}(?:HashMap|BTreeMap|IndexMap)<{text},{text}(?:,.*)?>$" + )), + pairs: compile(&format!( + r"^(?:{PATH}(?:Vec|VecDeque|Box)<\({text},{text}\)>|{PATH}Box<\[\({text},{text}\)\]>|\[\({text},{text}\)(?:;[^\]]+)?\])$" + )), + } + }) +} + +#[test] +fn shipped_environment_storage_uses_the_redacting_wrapper() { + let repo = Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .expect("the codexbar crate lives one level below the repository root"); + let mut files = Vec::new(); + for root in SCANNED_ROOTS { + let directory = repo.join(root); + assert!( + directory.is_dir(), + "missing scan root {}", + directory.display() + ); + collect_rust_files(&directory, &mut files); + } + files.sort(); + assert!( + files.len() > 100, + "expected the full source tree, found {} files", + files.len() + ); + + let sources: Vec<(String, String)> = files + .iter() + .map(|path| { + let text = std::fs::read_to_string(path) + .unwrap_or_else(|error| panic!("read {}: {error}", path.display())); + (relative_path(repo, path), sanitize(&text)) + }) + .collect(); + let aliases = environment_aliases(sources.iter().map(|(_, source)| source.as_str())); + let findings: Vec<(String, Field)> = sources + .iter() + .flat_map(|(path, source)| { + unprotected_fields(source, &aliases) + .into_iter() + .map(move |field| (path.clone(), field)) + }) + .collect(); + + let problems = review(&findings, REVIEWED_EXCEPTIONS); + assert!(problems.is_empty(), "{}", problems.join("\n")); +} + +#[test] +fn scanner_recognizes_storage_spellings_without_flagging_parameters_or_wrapped_fields() { + let source = sanitize(SPELLINGS); + let aliases = environment_aliases([source.as_str()]); + assert_eq!( + aliases, + BTreeSet::from(["EnvMap".to_string(), "EnvPairs".to_string()]) + ); + let found: Vec = unprotected_fields(&source, &aliases) + .into_iter() + .map(|field| field.declaration) + .collect(); + assert_eq!( + found, + [ + "Example.env: HashMap", + "Example.environment: std::collections::HashMap", + "Example.env_override: Option>", + "Example.base_env: BTreeMap", + "Example.child_env: Vec<(OsString, OsString)>", + "Example.borrowed_env: &'a HashMap", + "Example.shared_env: Arc>", + "Example.alias_env: EnvMap", + "Example.qualified_alias_env: roots::EnvPairs", + "Example.slice_env: &'a [(&'static str, &'static str)]", + "Generic.generic_env: HashMap", + "Launch::Child.env: BTreeMap", + ] + ); +} + +#[test] +fn field_lines_point_at_the_declaration() { + let source = sanitize( + "struct A {\n // env: HashMap,\n env:\n HashMap,\n}\n", + ); + assert_eq!( + unprotected_fields(&source, &BTreeSet::new()), + [Field { + line: 3, + declaration: "A.env: HashMap".to_string(), + }] + ); +} + +#[test] +fn reviewed_exceptions_must_match_exactly_one_declaration() { + let field = |line, declaration: &str| Field { + line, + declaration: declaration.to_string(), + }; + let findings = [ + ( + "a.rs".to_string(), + field(3, "Kept.env: HashMap"), + ), + ( + "a.rs".to_string(), + field(9, "Kept.env: HashMap"), + ), + ( + "b.rs".to_string(), + field(4, "Open.env: Vec<(OsString, OsString)>"), + ), + ]; + let exceptions = [ + ("a.rs", "Kept.env: HashMap"), + ("c.rs", "Gone.env: HashMap"), + ]; + assert_eq!( + review(&findings, &exceptions), + [ + "Duplicate reviewed exception match: a.rs:9: Kept.env: HashMap", + "Unprotected environment storage: b.rs:4: Open.env: Vec<(OsString, OsString)>", + "Remove stale reviewed exception: c.rs: Gone.env: HashMap", + ] + ); + assert!(review(&findings[..1], &exceptions[..1]).is_empty()); +} + +#[test] +fn sanitize_blanks_comments_and_literals_but_keeps_offsets() { + let source = "let a = \"{ env }\"; // { env }\nlet b = r#\"{\"#; /* { /* } */ } */ let c = '{';\nlet d: &'static str = \"\\\"}\";"; + let sanitized = sanitize(source); + assert_eq!(sanitized.len(), source.len()); + assert_eq!(sanitized.matches('\n').count(), 2); + assert!(!sanitized.contains('{') && !sanitized.contains('}')); + assert!(!sanitized.contains("env")); + assert!(sanitized.contains("&'static str")); +} + +/// Storage spellings for the self-test, including declarations the guard must ignore. +const SPELLINGS: &str = r###" +type EnvMap = HashMap; +pub type EnvPairs = Vec<(OsString, OsString)>; +type Callback = Box u32>; + +#[derive(Debug)] +pub struct Example<'a> { + env: HashMap, + pub environment: std::collections::HashMap, + pub(crate) env_override: Option>, + #[serde(default, rename = "x,}")] + base_env: + BTreeMap, + child_env: Vec<(OsString, OsString)>, + borrowed_env: &'a HashMap, + shared_env: Arc>, + alias_env: EnvMap, + qualified_alias_env: roots::EnvPairs, + slice_env: &'a [(&'static str, &'static str)], + protected_env: ProcessEnvironment>, + protected_optional_env: Option>, + env_names: Vec, + headers: HashMap, + callback_env: Box HashMap>, + // commented_env: HashMap, + /* block_env: HashMap, */ +} + +pub struct Wrapper(HashMap, T); + +pub struct Generic +where + F: Fn(u32) -> u32, +{ + generic_env: HashMap, + callback: F, +} + +enum Launch { + Child { program: String, env: BTreeMap }, + Plain(HashMap), + Idle, +} + +fn run(environment: HashMap) -> HashMap { + let env: HashMap = environment; + let text = "struct Fake { env: HashMap }"; + let quote = '"'; + env +} +"###; + +fn collect_rust_files(directory: &Path, files: &mut Vec) { + let entries = std::fs::read_dir(directory) + .unwrap_or_else(|error| panic!("read {}: {error}", directory.display())); + for entry in entries { + let path = entry.expect("directory entry").path(); + if path.is_dir() { + collect_rust_files(&path, files); + } else if path.extension().is_some_and(|extension| extension == "rs") { + files.push(path); + } + } +} + +fn relative_path(repo: &Path, path: &Path) -> String { + path.strip_prefix(repo) + .unwrap_or(path) + .to_string_lossy() + .replace('\\', "/") +} + +/// Blanks comments and the contents of string and char literals. Byte offsets and newlines are +/// kept, so braces, commas and keywords inside them cannot confuse the scan and line numbers +/// still match the file. +fn sanitize(source: &str) -> String { + let bytes = source.as_bytes(); + let mut out = bytes.to_vec(); + let blank = |out: &mut Vec, from: usize, to: usize| { + for byte in &mut out[from..to.min(bytes.len())] { + if *byte != b'\n' { + *byte = b' '; + } + } + }; + let mut index = 0; + while index < bytes.len() { + let next = bytes.get(index + 1).copied(); + if bytes[index] == b'/' && next == Some(b'/') { + let end = bytes[index..] + .iter() + .position(|&byte| byte == b'\n') + .map_or(bytes.len(), |offset| index + offset); + blank(&mut out, index, end); + index = end; + } else if bytes[index] == b'/' && next == Some(b'*') { + let mut depth = 0usize; + let mut end = index; + while end < bytes.len() { + if bytes[end] == b'/' && bytes.get(end + 1) == Some(&b'*') { + depth += 1; + end += 2; + } else if bytes[end] == b'*' && bytes.get(end + 1) == Some(&b'/') { + depth -= 1; + end += 2; + if depth == 0 { + break; + } + } else { + end += 1; + } + } + blank(&mut out, index, end); + index = end; + } else if let Some((content, hashes)) = raw_string_start(bytes, index) { + let mut end = content; + while end < bytes.len() + && !(bytes[end] == b'"' + && bytes[end + 1..] + .iter() + .take(hashes) + .filter(|&&byte| byte == b'#') + .count() + == hashes) + { + end += 1; + } + blank(&mut out, content, end); + index = end + 1 + hashes; + } else if bytes[index] == b'"' { + let mut end = index + 1; + while end < bytes.len() && bytes[end] != b'"' { + end += if bytes[end] == b'\\' { 2 } else { 1 }; + } + blank(&mut out, index + 1, end); + index = end + 1; + } else if bytes[index] == b'\'' { + index = char_literal_end(bytes, index).map_or(index + 1, |end| { + blank(&mut out, index + 1, end - 1); + end + }); + } else { + index += 1; + } + } + String::from_utf8(out).expect("blanking whole literals keeps UTF-8 valid") +} + +/// The content start and `#` count of a raw string literal (`r"`, `r#"`, `br"`, `cr"`) at `index`. +fn raw_string_start(bytes: &[u8], index: usize) -> Option<(usize, usize)> { + let previous_is_identifier = + index > 0 && (bytes[index - 1].is_ascii_alphanumeric() || bytes[index - 1] == b'_'); + if previous_is_identifier { + return None; + } + let mut cursor = index; + if matches!(bytes.get(cursor), Some(b'b' | b'c')) { + cursor += 1; + } + if bytes.get(cursor) != Some(&b'r') { + return None; + } + cursor += 1; + let hashes = bytes[cursor..] + .iter() + .take_while(|&&byte| byte == b'#') + .count(); + cursor += hashes; + (bytes.get(cursor) == Some(&b'"')).then_some((cursor + 1, hashes)) +} + +/// The end (one past the closing quote) of a char literal at `index`, or `None` for a lifetime. +fn char_literal_end(bytes: &[u8], index: usize) -> Option { + let first = *bytes.get(index + 1)?; + if first == b'\\' { + let close = bytes + .get(index + 3..)? + .iter() + .position(|&byte| byte == b'\'')?; + return Some(index + 3 + close + 1); + } + let width = match first { + 0x00..=0x7f => 1, + 0xc0..=0xdf => 2, + 0xe0..=0xef => 3, + _ => 4, + }; + (bytes.get(index + 1 + width) == Some(&b'\'')).then_some(index + 2 + width) +} + +/// Local `type` aliases that name an environment map, resolved through aliases of aliases. +fn environment_aliases<'a>(sources: impl IntoIterator) -> BTreeSet { + let definitions: Vec<(String, String)> = sources + .into_iter() + .flat_map(|source| { + patterns() + .alias + .captures_iter(source) + .map(|captures| (captures[1].to_string(), captures[2].to_string())) + .collect::>() + }) + .collect(); + let mut names = BTreeSet::new(); + loop { + let before = names.len(); + for (name, target) in &definitions { + if is_environment_type(target, &names) == Some(true) { + names.insert(name.clone()); + } + } + if names.len() == before { + return names; + } + } +} + +/// Environment-named fields of every struct, union and enum variant in `source` whose type is an +/// unwrapped environment. +fn unprotected_fields(source: &str, aliases: &BTreeSet) -> Vec { + let mut fields = Vec::new(); + for captures in patterns().item.captures_iter(source) { + let keyword = &captures[1]; + let owner = &captures[2]; + let header_end = captures.get(0).expect("whole match").end(); + let Some(open) = body_open(source.as_bytes(), header_end) else { + continue; + }; + let close = matching_brace(source.as_bytes(), open); + if keyword == "enum" { + for (variant, start, end) in enum_variant_bodies(source, open + 1, close) { + fields.extend(fields_in( + source, + start, + end, + &format!("{owner}::{variant}"), + aliases, + )); + } + } else { + fields.extend(fields_in(source, open + 1, close, owner, aliases)); + } + } + fields +} + +/// The opening brace of a named-field body after an item name, skipping generics and a `where` +/// clause. Tuple and unit items have no named fields and return `None`. +fn body_open(bytes: &[u8], mut index: usize) -> Option { + let skip_space = |index: &mut usize| { + while bytes.get(*index).is_some_and(u8::is_ascii_whitespace) { + *index += 1; + } + }; + skip_space(&mut index); + if bytes.get(index) == Some(&b'<') { + let mut depth = 0usize; + while index < bytes.len() { + match bytes[index] { + b'-' if bytes.get(index + 1) == Some(&b'>') => index += 1, + b'<' => depth += 1, + b'>' => { + depth -= 1; + if depth == 0 { + index += 1; + break; + } + } + _ => {} + } + index += 1; + } + skip_space(&mut index); + } + match bytes.get(index)? { + b'{' => Some(index), + b'(' | b';' => None, + _ => { + // A `where` clause: its bounds may hold parentheses and arrows, never braces. + let offset = bytes[index..] + .iter() + .position(|&byte| byte == b'{' || byte == b';')?; + (bytes[index + offset] == b'{').then_some(index + offset) + } + } +} + +fn matching_brace(bytes: &[u8], open: usize) -> usize { + let mut depth = 0usize; + for (index, &byte) in bytes.iter().enumerate().skip(open) { + match byte { + b'{' => depth += 1, + b'}' => { + depth -= 1; + if depth == 0 { + return index; + } + } + _ => {} + } + } + bytes.len() +} + +/// `(variant, start, end)` for each enum variant with named fields between `start` and `end`. +fn enum_variant_bodies(source: &str, start: usize, end: usize) -> Vec<(String, usize, usize)> { + let bytes = source.as_bytes(); + let mut bodies = Vec::new(); + let mut nesting = 0usize; + let mut segment = start; + let mut index = start; + while index < end { + match bytes[index] { + b'(' | b'[' => nesting += 1, + b')' | b']' => nesting = nesting.saturating_sub(1), + b',' if nesting == 0 => segment = index + 1, + b'{' if nesting == 0 => { + let close = matching_brace(bytes, index); + if let Some(name) = patterns().variant_name.captures(&source[segment..index]) { + bodies.push((name[1].to_string(), index + 1, close)); + } + index = close; + } + _ => {} + } + index += 1; + } + bodies +} + +/// Environment-named fields between `start` and `end` whose type is an unwrapped environment. +fn fields_in( + source: &str, + start: usize, + end: usize, + owner: &str, + aliases: &BTreeSet, +) -> Vec { + split_top_level(source, start, end) + .into_iter() + .filter_map(|(segment_start, segment_end)| { + let captures = patterns() + .field + .captures(&source[segment_start..segment_end])?; + let name = captures.get(1).expect("field name"); + let ty = &captures[2]; + if !name.as_str().to_ascii_lowercase().contains("env") + || is_environment_type(ty, aliases) != Some(true) + { + return None; + } + let offset = segment_start + name.start(); + Some(Field { + line: source[..offset].matches('\n').count() + 1, + declaration: format!("{owner}.{}: {}", name.as_str(), display_type(ty)), + }) + }) + .collect() +} + +/// Comma-separated ranges between `start` and `end`, ignoring commas nested in brackets. +fn split_top_level(source: &str, start: usize, end: usize) -> Vec<(usize, usize)> { + let bytes = source.as_bytes(); + let mut ranges = Vec::new(); + let mut nesting = 0usize; + let mut segment = start; + let mut index = start; + while index < end { + match bytes[index] { + b'-' if bytes.get(index + 1) == Some(&b'>') => index += 1, + b'(' | b'[' | b'{' | b'<' => nesting += 1, + b')' | b']' | b'}' | b'>' => nesting = nesting.saturating_sub(1), + b',' if nesting == 0 => { + ranges.push((segment, index)); + segment = index + 1; + } + _ => {} + } + index += 1; + } + if !source[segment..end].trim().is_empty() { + ranges.push((segment, end)); + } + ranges +} + +/// `Some(true)` for an unwrapped environment type, `Some(false)` for one already inside +/// `ProcessEnvironment`, `None` for anything else. +fn is_environment_type(ty: &str, aliases: &BTreeSet) -> Option { + let patterns = patterns(); + let mut current = patterns + .reference + .replace(&compact_type(ty), "") + .into_owned(); + loop { + if patterns.protected.is_match(¤t) { + return Some(false); + } + let Some(inner) = patterns + .container + .captures(¤t) + .map(|captures| captures[1].to_string()) + else { + break; + }; + current = patterns.reference.replace(&inner, "").into_owned(); + } + let alias = current.rsplit("::").next().unwrap_or(¤t); + (patterns.map.is_match(¤t) + || patterns.pairs.is_match(¤t) + || aliases.contains(alias)) + .then_some(true) +} + +/// Drops whitespace, keeping one space only between identifier characters (`'a mut`, `'static str`). +fn compact_type(ty: &str) -> String { + let is_word = |c: char| c.is_ascii_alphanumeric() || c == '_' || c == '\''; + let mut out = String::new(); + let mut pending_space = false; + for c in ty.chars() { + if c.is_whitespace() { + pending_space = true; + continue; + } + if pending_space && out.chars().last().is_some_and(is_word) && is_word(c) { + out.push(' '); + } + pending_space = false; + out.push(c); + } + out +} + +/// The type as written, with whitespace runs collapsed for stable reports. +fn display_type(ty: &str) -> String { + ty.split_whitespace() + .collect::>() + .join(" ") + .replace("< ", "<") + .replace(" >", ">") + .replace("( ", "(") + .replace(" )", ")") + .replace(" ,", ",") +} + +/// Problems for unreviewed findings, plus stale or duplicate reviewed exceptions. +fn review(findings: &[(String, Field)], exceptions: &[(&str, &str)]) -> Vec { + let mut matches: HashMap<(&str, &str), usize> = HashMap::new(); + let mut problems = Vec::new(); + for (path, field) in findings { + let reviewed = exceptions + .iter() + .find(|(exception_path, declaration)| { + exception_path == path && *declaration == field.declaration + }) + .copied(); + match reviewed { + Some(exception) => { + let count = matches.entry(exception).or_default(); + *count += 1; + if *count > 1 { + problems.push(format!( + "Duplicate reviewed exception match: {path}:{}: {}", + field.line, field.declaration + )); + } + } + None => problems.push(format!( + "Unprotected environment storage: {path}:{}: {}", + field.line, field.declaration + )), + } + } + for exception in exceptions { + if !matches.contains_key(exception) { + problems.push(format!( + "Remove stale reviewed exception: {}: {}", + exception.0, exception.1 + )); + } + } + problems +} diff --git a/rust/src/process_environment/tests.rs b/rust/src/process_environment/tests.rs new file mode 100644 index 00000000000..373b44ed84d --- /dev/null +++ b/rust/src/process_environment/tests.rs @@ -0,0 +1,208 @@ +//! Redaction and value behaviour of [`ProcessEnvironment`] and of the structs that store one +//! (upstream 0.70.0 `ProcessEnvironmentTests`). + +use std::collections::{BTreeMap, HashMap}; +use std::ffi::OsString; +use std::panic::{self, AssertUnwindSafe}; + +use super::ProcessEnvironment; +use crate::cli::tty_runner::TtyCommandOptions; +use crate::core::{ProviderId, TokenAccount, TokenAccountOverride}; + +const SENTINEL: &str = "sentinel-environment-value-must-not-be-rendered"; +const SENTINEL_KEY: &str = "CODEXBAR_TEST_SENTINEL_SECRET"; +const ORDINARY_KEY: &str = "ORDINARY_NAME"; +const TWO_ENTRIES: &str = "ProcessEnvironment(2 entries; redacted)"; + +/// A synthetic environment: a secret-looking variable and an ordinary one, both holding the +/// sentinel value. +fn synthetic_environment() -> HashMap { + HashMap::from([ + (SENTINEL_KEY.to_string(), SENTINEL.to_string()), + (ORDINARY_KEY.to_string(), SENTINEL.to_string()), + ]) +} + +fn single_entry() -> HashMap { + HashMap::from([(SENTINEL_KEY.to_string(), SENTINEL.to_string())]) +} + +/// Neither values nor variable names may be rendered. +fn assert_redacted(output: &str) { + for hidden in [SENTINEL, SENTINEL_KEY, ORDINARY_KEY] { + assert!(!output.contains(hidden), "rendered {hidden}: {output}"); + } +} + +/// `{:?}` and `{:#?}` of a storing struct show the count and nothing else of the environment. +fn assert_debug_shows_only_two_entries(value: &impl std::fmt::Debug) { + for output in [format!("{value:?}"), format!("{value:#?}")] { + assert_redacted(&output); + assert!(output.contains(TWO_ENTRIES), "{output}"); + } +} + +#[derive(Debug, Clone, Default, PartialEq, Eq)] +struct OptionalConfiguration { + environment: ProcessEnvironment>>, +} + +impl OptionalConfiguration { + fn with(environment: HashMap) -> Self { + Self { + environment: Some(environment).into(), + } + } +} + +#[test] +fn structs_storing_an_environment_render_only_its_entry_count() { + let tty = TtyCommandOptions { + env: synthetic_environment().into(), + ..TtyCommandOptions::default() + }; + assert_debug_shows_only_two_entries(&tty); + let token_override = TokenAccountOverride { + provider: ProviderId::OpenRouter, + account: TokenAccount::new("synthetic", "synthetic-token"), + env_override: Some(synthetic_environment()).into(), + cookie_header: None, + }; + assert_debug_shows_only_two_entries(&token_override); +} + +#[cfg(windows)] +#[test] +fn managed_process_config_renders_only_its_entry_count() { + let managed = crate::managed_process::ManagedProcessConfig { + program: std::path::PathBuf::from("synthetic-helper.exe"), + args: Vec::new(), + env: synthetic_environment() + .into_iter() + .map(|(name, value)| (OsString::from(name), OsString::from(value))) + .collect::>() + .into(), + cwd: None, + pty_rows: 24, + pty_cols: 80, + label: "synthetic".to_string(), + }; + assert_debug_shows_only_two_entries(&managed); +} + +#[test] +fn failed_assertions_hide_captured_environment_contents() { + let captured = OptionalConfiguration::with(synthetic_environment()); + let other = OptionalConfiguration::default(); + let payload = panic::catch_unwind(AssertUnwindSafe(|| assert_eq!(captured, other))) + .expect_err("different environments must not compare equal"); + let message = payload + .downcast_ref::() + .expect("assert_eq! panics with a formatted message"); + assert!(message.contains(TWO_ENTRIES), "{message}"); + assert!( + message.contains("ProcessEnvironment(0 entries; redacted)"), + "{message}" + ); + assert_redacted(message); +} + +#[test] +fn wrapper_preserves_map_access_and_reports_the_current_count() { + let mut environment = ProcessEnvironment::new(single_entry()); + assert_eq!( + environment.get(SENTINEL_KEY).map(String::as_str), + Some(SENTINEL) + ); + environment.insert(ORDINARY_KEY.to_string(), SENTINEL.to_string()); + assert_eq!(environment.len(), 2); + assert_eq!(format!("{environment:?}"), TWO_ENTRIES); + assert_eq!(format!("{environment:#?}"), TWO_ENTRIES); + + let mut names = Vec::new(); + for (name, value) in &environment { + assert_eq!(value, SENTINEL); + names.push(name.as_str()); + } + names.sort_unstable(); + assert_eq!(names, [SENTINEL_KEY, ORDINARY_KEY]); + assert_eq!(environment.into_inner(), synthetic_environment()); +} + +#[test] +fn pair_lists_and_ordered_maps_count_their_entries() { + let pairs = ProcessEnvironment::new(vec![( + OsString::from(SENTINEL_KEY), + OsString::from(SENTINEL), + )]); + assert_eq!( + format!("{pairs:?}"), + "ProcessEnvironment(1 entries; redacted)" + ); + let ordered = ProcessEnvironment::new(BTreeMap::from([ + (SENTINEL_KEY, SENTINEL), + (ORDINARY_KEY, SENTINEL), + ])); + assert_eq!(format!("{ordered:?}"), TWO_ENTRIES); + let handed_to_child: Vec<(OsString, OsString)> = pairs.into_iter().collect(); + assert_eq!( + handed_to_child, + [(OsString::from(SENTINEL_KEY), OsString::from(SENTINEL))] + ); +} + +#[test] +fn optional_environments_preserve_absence_mutation_and_value_equality() { + let mut absent = OptionalConfiguration::default(); + let empty = OptionalConfiguration::with(HashMap::new()); + assert!(absent.environment.is_none()); + assert_ne!(absent, empty); + assert_eq!(absent, OptionalConfiguration::default()); + + *absent.environment = Some(single_entry()); + assert_eq!(absent, OptionalConfiguration::with(single_entry())); + let mut copy = absent.clone(); + copy.environment + .as_mut() + .expect("environment present") + .insert(ORDINARY_KEY.to_string(), SENTINEL.to_string()); + assert_ne!(copy, absent); + assert_eq!(absent.environment.as_ref().map(HashMap::len), Some(1)); + assert_eq!(copy.environment.as_ref().map(HashMap::len), Some(2)); + + // Equal counts render identically whatever the contents. + let unrelated = OptionalConfiguration::with(HashMap::from([ + ("unrelated".to_string(), "value".to_string()), + ("different".to_string(), "contents".to_string()), + ])); + assert_eq!(format!("{copy:?}"), format!("{unrelated:?}")); + assert_redacted(&format!("{copy:#?}")); + + *absent.environment = None; + assert_eq!(absent, OptionalConfiguration::default()); + assert_eq!( + format!("{absent:?}"), + "OptionalConfiguration { environment: ProcessEnvironment(0 entries; redacted) }" + ); +} + +#[test] +fn optional_wrapper_counts_track_mutations_without_equating_missing_and_empty() { + let mut environment = ProcessEnvironment::new(None::>); + assert_eq!( + format!("{environment:?}"), + "ProcessEnvironment(0 entries; redacted)" + ); + assert_ne!(environment, ProcessEnvironment::new(Some(HashMap::new()))); + + *environment = Some(single_entry()); + assert_eq!( + format!("{environment:?}"), + "ProcessEnvironment(1 entries; redacted)" + ); + environment + .as_mut() + .expect("environment present") + .insert(ORDINARY_KEY.to_string(), SENTINEL.to_string()); + assert_eq!(format!("{environment:?}"), TWO_ENTRIES); +} diff --git a/rust/src/providers/antigravity/mod.rs b/rust/src/providers/antigravity/mod.rs index 5f357646639..12610082eee 100755 --- a/rust/src/providers/antigravity/mod.rs +++ b/rust/src/providers/antigravity/mod.rs @@ -528,7 +528,8 @@ impl AntigravityProvider { env: vec![ (OsString::from("TERM"), OsString::from("xterm-256color")), (OsString::from("COLORTERM"), OsString::from("truecolor")), - ], + ] + .into(), cwd: dirs::home_dir().filter(|path| path.is_dir()), pty_rows: 30, pty_cols: 120, diff --git a/rust/src/providers/claude/mod.rs b/rust/src/providers/claude/mod.rs index 22da9fe951e..93038f334e3 100755 --- a/rust/src/providers/claude/mod.rs +++ b/rust/src/providers/claude/mod.rs @@ -416,7 +416,7 @@ async fn run_claude_pty_probe( if let Some((trigger, keys)) = probe.send_on_substring { options = options.with_send_on_substring(trigger, keys); } - options.env = env; + options.env = env.into(); TtyCommandRunner::new() .run(&claude_path.to_string_lossy(), probe.script, options)