tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

fix(security): fail closed on invalid Codex ACP mode (#11362)

Browse Source 
Signed-off-by: Jasper Hugo <jasper@spiral.xyz>
This commit is contained in:
Jasper
2026-08-20 00:24:12 +00:00
committed by GitHub
parent bc68049225
commit 01f5ed7f9b
2 changed files with 278 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/goose/src/config/base.rs
+78
View File
@@ -150,7 +150,15 @@ enum SecretStorage {
// Global instance
static GLOBAL_CONFIG: OnceCell<Config> = OnceCell::new();
#[cfg(test)]
pub(crate) const TEST_SYSTEM_CONFIG_PATH_ENV: &str = "GOOSE_TEST_SYSTEM_CONFIG_PATH";
fn system_config_path() -> PathBuf {
#[cfg(test)]
if let Some(path) = env::var_os(TEST_SYSTEM_CONFIG_PATH_ENV) {
return path.into();
}
#[cfg(unix)]
{
PathBuf::from("/etc/goose/config.yaml")
@@ -169,6 +177,25 @@ fn additional_config_paths_from_env() -> Vec<PathBuf> {
.unwrap_or_default()
}
fn metadata_is_symlink_or_reparse_point(metadata: &std::fs::Metadata) -> bool {
if metadata.file_type().is_symlink() {
return true;
}
#[cfg(windows)]
{
use std::os::windows::fs::MetadataExt;
const FILE_ATTRIBUTE_REPARSE_POINT: u32 = 0x400;
metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0
}
#[cfg(not(windows))]
{
false
}
}
impl Default for Config {
fn default() -> Self {
let config_dir = Paths::config_dir();
@@ -519,6 +546,37 @@ impl Config {
Ok(merged)
}
fn load_strict(&self) -> Result<Mapping, ConfigError> {
let mut merged = Mapping::new();
for path in &self.config_paths {
match std::fs::symlink_metadata(path) {
Ok(_) => {}
Err(error) if error.kind() == std::io::ErrorKind::NotFound => {
for ancestor in path.ancestors().skip(1) {
match std::fs::symlink_metadata(ancestor) {
Ok(metadata) if metadata_is_symlink_or_reparse_point(&metadata) => {
std::fs::metadata(ancestor)?;
}
Ok(_) => {}
Err(error) if error.kind() == std::io::ErrorKind::NotFound => {}
Err(error) => return Err(error.into()),
}
}
continue;
}
Err(error) => return Err(error.into()),
}
let content = std::fs::read_to_string(path)?;
let layer = parse_yaml_content(&content)?;
merge_config_values(&mut merged, layer);
}
crate::config::migrations::run_read_migrations(&mut merged);
Ok(merged)
}
pub fn all_values(&self) -> Result<HashMap<String, Value>, ConfigError> {
let config_values = self.load()?;
let mut map = HashMap::from_iter(config_values.into_iter().filter_map(|(k, v)| {
@@ -1107,6 +1165,26 @@ config_value!(CHATGPT_CODEX_REASONING_EFFORT, String, "medium");
config_value!(GOOSE_SEARCH_PATHS, Vec<String>);
config_value!(GOOSE_MODE, GooseMode);
impl Config {
pub(crate) fn get_goose_mode_strict(&self) -> Result<GooseMode, ConfigError> {
match env::var("GOOSE_MODE") {
Ok(value) => {
let value = Self::parse_env_value(&value)?;
Ok(serde_json::from_value(value)?)
}
Err(env::VarError::NotPresent) => {
let values = self.load_strict()?;
let value = values
.get("GOOSE_MODE")
.ok_or_else(|| ConfigError::NotFound("GOOSE_MODE".to_string()))?;
Ok(serde_yaml::from_value(value.clone())?)
}
Err(env::VarError::NotUnicode(_)) => Err(ConfigError::DeserializeError(
"GOOSE_MODE contains non-Unicode data".to_string(),
)),
}
}
}
// GOOSE_PROVIDER and GOOSE_MODEL are handled by crate::config::providers
// which checks the structured `providers:` block first and falls back to
// the legacy flat keys. The accessors below delegate to that module.
crates/goose/src/providers/codex_acp.rs
+200 -2
View File
@@ -7,7 +7,7 @@ use crate::acp::{
extension_configs_to_mcp_servers, AcpProvider, AcpProviderConfig, ACP_CURRENT_MODEL,
};
use crate::config::search_path::SearchPaths;
use crate::config::{Config, GooseMode};
use crate::config::{Config, ConfigError, GooseMode};
use crate::providers::base::{
current_working_dir, ProviderDef, ProviderDescriptor, ProviderMetadata,
};
@@ -18,6 +18,16 @@ const CODEX_ACP_DOC_URL: &str = "https://github.com/agentclientprotocol/codex-ac
pub struct CodexAcpProvider;
fn resolve_goose_mode(
configured_mode: Result<GooseMode, ConfigError>,
) -> Result<GooseMode, ConfigError> {
match configured_mode {
Ok(mode) => Ok(mode),
Err(ConfigError::NotFound(_)) => Ok(GooseMode::Auto),
Err(error) => Err(error),
}
}
impl goose_providers::base::ProviderDescriptor for CodexAcpProvider {
fn metadata() -> ProviderMetadata {
ProviderMetadata::new(
@@ -65,7 +75,7 @@ impl ProviderDef for CodexAcpProvider {
let resolved_command = SearchPaths::builder()
.with_npm()
.resolve(CODEX_ACP_PROVIDER_NAME)?;
let goose_mode = config.get_goose_mode().unwrap_or(GooseMode::Auto);
let goose_mode = resolve_goose_mode(config.get_goose_mode_strict())?;
let mcp_servers = extension_configs_to_mcp_servers(&extensions);
let mode_mapping = HashMap::from([
@@ -94,3 +104,191 @@ impl ProviderDef for CodexAcpProvider {
})
}
}
#[cfg(test)]
mod tests {
use super::*;
#[cfg(unix)]
use std::fs;
#[cfg(unix)]
use std::os::unix::fs::{symlink, PermissionsExt};
#[cfg(unix)]
use std::process::Command;
#[cfg(unix)]
const CHILD_ENV: &str = "GOOSE_CODEX_ACP_MODE_TEST_CHILD";
#[cfg(unix)]
const EXPECT_CONFIG_ERROR_ENV: &str = "GOOSE_CODEX_ACP_EXPECT_CONFIG_ERROR";
#[cfg(unix)]
#[derive(Clone, Copy)]
enum ConfigFixture {
Absent,
File(&'static str),
Directory,
DanglingSymlink,
DanglingParentSymlink,
}
#[test]
fn missing_goose_mode_defaults_to_auto() {
assert_eq!(
resolve_goose_mode(Err(ConfigError::NotFound("GOOSE_MODE".to_string()))).unwrap(),
GooseMode::Auto
);
}
#[test]
fn configured_goose_modes_are_preserved() {
for mode in [
GooseMode::Auto,
GooseMode::SmartApprove,
GooseMode::Approve,
GooseMode::Chat,
] {
assert_eq!(resolve_goose_mode(Ok(mode)).unwrap(), mode);
}
}
#[test]
fn invalid_goose_mode_errors_are_preserved() {
assert!(matches!(
resolve_goose_mode(Err(ConfigError::DeserializeError("invalid".to_string()))),
Err(ConfigError::DeserializeError(_))
));
assert!(matches!(
resolve_goose_mode(Err(ConfigError::FileError(std::io::Error::other(
"unreadable"
)))),
Err(ConfigError::FileError(_))
));
}
#[cfg(unix)]
#[tokio::test]
async fn goose_mode_validation_precedes_codex_acp_launch() {
if std::env::var_os(CHILD_ENV).is_some() {
let error = CodexAcpProvider::from_env(vec![], None)
.await
.expect_err("marker executable should fail ACP initialization");
let is_config_error = matches!(
error.downcast_ref::<ConfigError>(),
Some(ConfigError::DeserializeError(_) | ConfigError::FileError(_))
);
assert_eq!(
is_config_error,
std::env::var_os(EXPECT_CONFIG_ERROR_ENV).is_some(),
"unexpected provider error: {error:#}"
);
return;
}
let fixture = tempfile::tempdir().unwrap();
let executable = fixture.path().join(CODEX_ACP_PROVIDER_NAME);
fs::write(
&executable,
"#!/bin/sh\n: > \"$GOOSE_CODEX_ACP_MARKER\"\nexit 1\n",
)
.unwrap();
fs::set_permissions(&executable, fs::Permissions::from_mode(0o700)).unwrap();
let search_paths = serde_json::to_string(&vec![fixture.path()]).unwrap();
for (name, mode, config_fixture, should_launch) in [
("unset", None, ConfigFixture::Absent, true),
("auto", Some("auto"), ConfigFixture::Absent, true),
(
"smart_approve",
Some("smart_approve"),
ConfigFixture::Absent,
true,
),
("approve", Some("approve"), ConfigFixture::Absent, true),
("chat", Some("chat"), ConfigFixture::Absent, true),
("invalid", Some("invalid"), ConfigFixture::Absent, false),
(
"configured_file",
None,
ConfigFixture::File("GOOSE_MODE: approve\n"),
true,
),
(
"malformed_file",
None,
ConfigFixture::File("GOOSE_MODE: ["),
false,
),
("unreadable_file", None, ConfigFixture::Directory, false),
(
"dangling_symlink",
None,
ConfigFixture::DanglingSymlink,
false,
),
(
"dangling_parent_symlink",
None,
ConfigFixture::DanglingParentSymlink,
false,
),
] {
let marker = fixture.path().join(format!("launched-{name}"));
let path_root = fixture.path().join(format!("config-{name}"));
let config_dir = path_root.join("config");
fs::create_dir_all(&config_dir).unwrap();
let config_path = config_dir.join("config.yaml");
match config_fixture {
ConfigFixture::Absent => {}
ConfigFixture::File(content) => fs::write(&config_path, content).unwrap(),
ConfigFixture::Directory => fs::create_dir(&config_path).unwrap(),
ConfigFixture::DanglingSymlink => {
symlink(config_dir.join("missing.yaml"), &config_path).unwrap()
}
ConfigFixture::DanglingParentSymlink => {
fs::remove_dir(&config_dir).unwrap();
symlink(path_root.join("missing-config"), &config_dir).unwrap();
}
}
let mut command = Command::new(std::env::current_exe().unwrap());
command
.arg("--exact")
.arg("providers::codex_acp::tests::goose_mode_validation_precedes_codex_acp_launch")
.arg("--nocapture")
.env(CHILD_ENV, "1")
.env("GOOSE_SEARCH_PATHS", &search_paths)
.env("GOOSE_CODEX_ACP_MARKER", &marker)
.env("GOOSE_PATH_ROOT", &path_root)
.env(
crate::config::base::TEST_SYSTEM_CONFIG_PATH_ENV,
path_root.join("system-config.yaml"),
)
.env_remove("GOOSE_ADDITIONAL_CONFIG_FILES")
.env("GOOSE_DISABLE_KEYRING", "1");
if should_launch {
command.env_remove(EXPECT_CONFIG_ERROR_ENV);
} else {
command.env(EXPECT_CONFIG_ERROR_ENV, "1");
}
match mode {
Some(mode) => {
command.env("GOOSE_MODE", mode);
}
None => {
command.env_remove("GOOSE_MODE");
}
}
let output = command.output().unwrap();
assert!(
output.status.success(),
"{name} child test failed:\nstdout:\n{}\nstderr:\n{}",
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr)
);
assert_eq!(
marker.exists(),
should_launch,
"unexpected codex-acp launch result for {name} GOOSE_MODE"
);
}
}
}