fix(agents): canonicalize portable env keys

Co-authored-by: Atish Patel <atish@squareup.com>
Signed-off-by: Atish Patel <atish@squareup.com>
This commit is contained in:
Hardworking Honey
2026-08-06 10:12:11 -05:00
co-authored by Atish Patel
parent 8e4358b314
commit 2a116bfb9e
2 changed files with 88 additions and 16 deletions
@@ -170,18 +170,38 @@ pub fn validate_user_env_keys(env_vars: &BTreeMap<String, String>) -> Result<(),
Ok(()) Ok(())
} }
/// Explicitly portable, non-secret settings. Keep this separate from the
/// structured provider/model fields: those are derived at spawn/deploy time and
/// therefore must not be imported from a snapshot environment map.
const PORTABLE_ENV_KEYS: &[&str] = &[
"BUZZ_AGENT_THINKING_EFFORT",
"CLAUDE_CODE_EFFORT_LEVEL",
"GOOSE_THINKING_EFFORT",
"DATABRICKS_HOST",
"DATABRICKS_MODEL",
];
/// Returns the canonical spelling of an explicitly portable, non-secret key.
/// Snapshot input is untrusted and Windows treats environment names
/// case-insensitively, so callers must not retain a sender-controlled spelling.
fn portable_env_key(key: &str) -> Option<&'static str> {
PORTABLE_ENV_KEYS
.iter()
.copied()
.find(|portable| portable.eq_ignore_ascii_case(key))
}
/// Returns `true` when `key` is safe to show verbatim — not a credential. /// Returns `true` when `key` is safe to show verbatim — not a credential.
/// ///
/// Default-deny: every key NOT in this explicit allowlist is masked. Callers /// Default-deny: every key NOT in this explicit allowlist is masked. Callers
/// that display env values (baked-env UI, spawn-diff tooltip) share this /// that display env values (baked-env UI, spawn-diff tooltip) share this
/// single authority — no second list. /// single authority — no second list.
/// ///
/// Allowlist (case-insensitive): /// This display policy also includes derived provider/model values so users can
/// - `BUZZ_AGENT_PROVIDER`, `BUZZ_AGENT_MODEL` — agent runtime selection /// inspect the configuration Buzz supplies from structured fields. Those keys
/// - `BUZZ_AGENT_THINKING_EFFORT` — non-secret enum (none/minimal/low/medium/high/xhigh/max) /// are intentionally excluded from portable snapshots; see [`portable_env_key`].
/// - `DATABRICKS_HOST`, `DATABRICKS_MODEL` — Block non-secret defaults
pub(crate) fn is_safe_to_reveal(key: &str) -> bool { pub(crate) fn is_safe_to_reveal(key: &str) -> bool {
const SAFE_KEYS: &[&str] = &[ const SAFE_DISPLAY_KEYS: &[&str] = &[
"BUZZ_AGENT_PROVIDER", "BUZZ_AGENT_PROVIDER",
"BUZZ_AGENT_MODEL", "BUZZ_AGENT_MODEL",
"BUZZ_AGENT_THINKING_EFFORT", "BUZZ_AGENT_THINKING_EFFORT",
@@ -190,8 +210,9 @@ pub(crate) fn is_safe_to_reveal(key: &str) -> bool {
"DATABRICKS_HOST", "DATABRICKS_HOST",
"DATABRICKS_MODEL", "DATABRICKS_MODEL",
]; ];
let upper = key.to_ascii_uppercase(); SAFE_DISPLAY_KEYS
SAFE_KEYS.iter().any(|safe| upper == *safe) .iter()
.any(|safe| safe.eq_ignore_ascii_case(key))
} }
/// Project only explicitly approved, non-secret environment settings for a /// Project only explicitly approved, non-secret environment settings for a
@@ -200,16 +221,25 @@ pub(crate) fn is_safe_to_reveal(key: &str) -> bool {
pub(crate) fn portable_env_for_export( pub(crate) fn portable_env_for_export(
env_vars: &BTreeMap<String, String>, env_vars: &BTreeMap<String, String>,
) -> BTreeMap<String, String> { ) -> BTreeMap<String, String> {
env_vars let mut projected = BTreeMap::new();
.iter() for (key, value) in env_vars {
.filter(|(key, _)| is_safe_to_reveal(key)) if let Some(canonical_key) = portable_env_key(key) {
.map(|(key, value)| (key.clone(), value.clone())) // Persisted maps should already use canonical spellings. If a legacy
.collect() // record contains aliases, prefer the canonical key deterministically
// rather than exporting two keys that collide on Windows.
if key == canonical_key || !projected.contains_key(canonical_key) {
projected.insert(canonical_key.to_string(), value.clone());
}
}
}
projected
} }
/// Accept the portable snapshot projection only after applying the same /// Accept the portable snapshot projection only after applying the explicit
/// allowlist and normal save-time environment validation used by UI input. /// portable-key allowlist and normal save-time environment validation used by
/// Unsafe crafted keys are dropped rather than becoming configuration. /// UI input. Keys are canonicalized to prevent case-insensitive collisions in a
/// Windows child-process environment; unknown and derived model/provider keys
/// are dropped rather than becoming persisted configuration.
pub(crate) fn portable_env_for_import( pub(crate) fn portable_env_for_import(
portable_env: &BTreeMap<String, String>, portable_env: &BTreeMap<String, String>,
) -> Result<BTreeMap<String, String>, String> { ) -> Result<BTreeMap<String, String>, String> {
@@ -282,6 +312,7 @@ pub(crate) fn portable_config_for_import(
runtime.thinking_config_json_env_var, runtime.thinking_config_json_env_var,
runtime.thinking_config_json_key, runtime.thinking_config_json_key,
) { ) {
remove_env_key_case_insensitive(&mut env_vars, env_key);
env_vars.insert( env_vars.insert(
env_key.to_string(), env_key.to_string(),
serde_json::json!({ json_key: effort }).to_string(), serde_json::json!({ json_key: effort }).to_string(),
@@ -289,12 +320,18 @@ pub(crate) fn portable_config_for_import(
} else if let Some(env_key) = runtime.thinking_env_var { } else if let Some(env_key) = runtime.thinking_env_var {
// The first-class snapshot field is authoritative when the two values // The first-class snapshot field is authoritative when the two values
// differ, while the allowlisted env map remains available for all other // differ, while the allowlisted env map remains available for all other
// non-secret runtime configuration. // non-secret runtime configuration. Remove aliases first because
// Windows collapses environment names case-insensitively.
remove_env_key_case_insensitive(&mut env_vars, env_key);
env_vars.insert(env_key.to_string(), effort.to_string()); env_vars.insert(env_key.to_string(), effort.to_string());
} }
Ok(env_vars) Ok(env_vars)
} }
fn remove_env_key_case_insensitive(env_vars: &mut BTreeMap<String, String>, key: &str) {
env_vars.retain(|existing, _| !existing.eq_ignore_ascii_case(key));
}
/// The complete set of effort tiers Buzz will write into a harness variable. /// The complete set of effort tiers Buzz will write into a harness variable.
/// ///
/// Default-deny, and deliberately a closed list: `thinking_effort` arriving from /// Default-deny, and deliberately a closed list: `thinking_effort` arriving from
@@ -583,6 +583,41 @@ fn codex_effort_reader_ignores_malformed_or_non_string_json_values() {
); );
} }
#[test]
fn portable_import_canonicalizes_effort_and_first_class_value_wins() {
let source = map(&[("claude_code_effort_level", "low")]);
let env_vars = portable_config_for_import(Some("claude"), &source, Some("high")).unwrap();
assert_eq!(env_vars.len(), 1);
assert_eq!(
env_vars.get("CLAUDE_CODE_EFFORT_LEVEL").map(String::as_str),
Some("high")
);
assert!(!env_vars
.keys()
.any(|key| key.eq_ignore_ascii_case("claude_code_effort_level")
&& key != "CLAUDE_CODE_EFFORT_LEVEL"));
}
#[test]
fn portable_import_drops_derived_model_provider_aliases() {
let imported = portable_env_for_import(&map(&[
("BUZZ_AGENT_MODEL", "attacker-model"),
("buzz_agent_provider", "attacker-provider"),
("GOOSE_MODEL", "attacker-model"),
("goose_provider", "attacker-provider"),
("GOOSE_THINKING_EFFORT", "high"),
]))
.unwrap();
assert_eq!(imported.len(), 1);
assert_eq!(
imported.get("GOOSE_THINKING_EFFORT").map(String::as_str),
Some("high")
);
}
#[test] #[test]
fn portable_import_drops_crafted_secret_without_persisting_it() { fn portable_import_drops_crafted_secret_without_persisting_it() {
let imported = portable_env_for_import(&map(&[ let imported = portable_env_for_import(&map(&[