mirror of
https://github.com/openai/codex.git
synced 2026-10-26 18:38:47 +00:00
[codex] preserve optional auth refresh and WIF denies [ci changed_files]
This commit is contained in:
@@ -90,7 +90,7 @@ impl AppsRequestProcessor {
|
||||
.await
|
||||
.map_err(|err| internal_error(format!("failed to resolve auth: {err}")))?
|
||||
} else {
|
||||
self.auth_manager.auth_cached()
|
||||
self.auth_manager.auth_for_optional_use().await
|
||||
};
|
||||
if !config
|
||||
.features
|
||||
|
||||
@@ -340,6 +340,53 @@ async fn workload_credential_file_is_denied_even_with_danger_full_access() -> an
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn workload_credential_deny_preserves_an_existing_user_deny() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let token_file = codex_home.path().join("subject-token");
|
||||
let token_path = AbsolutePathBuf::try_from(token_file.clone())?;
|
||||
let user_deny = FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path {
|
||||
path: token_path.clone(),
|
||||
},
|
||||
access: FileSystemAccessMode::Deny,
|
||||
};
|
||||
let permission_profile = PermissionProfile::from_runtime_permissions(
|
||||
&FileSystemSandboxPolicy::restricted(vec![user_deny.clone()]),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
);
|
||||
let cfg = ConfigToml {
|
||||
workload_identity: Some(WorkloadIdentityConfig {
|
||||
identity_provider_id: "idp_example".to_string(),
|
||||
identity_provider_mapping_id: "idpm_example".to_string(),
|
||||
audience: "openai-audience".to_string(),
|
||||
token_url: "https://auth.openai.com/oauth/token".to_string(),
|
||||
credential_source: CredentialSourceConfig::File { path: token_file },
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let config = Config::load_from_base_config_with_overrides(
|
||||
cfg,
|
||||
ConfigOverrides {
|
||||
permission_profile: Some(permission_profile),
|
||||
..Default::default()
|
||||
},
|
||||
codex_home.abs(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
assert!(
|
||||
config
|
||||
.permissions
|
||||
.file_system_sandbox_policy()
|
||||
.entries
|
||||
.contains(&user_deny)
|
||||
);
|
||||
assert!(config.workload_identity_credential_deny_paths.is_empty());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn workload_identity_rejects_forced_api_key_login() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
@@ -810,8 +810,8 @@ pub struct Config {
|
||||
/// Workload identity federation configuration for external ChatGPT auth.
|
||||
pub workload_identity: Option<WorkloadIdentityConfig>,
|
||||
|
||||
/// Runtime-only paths added to the sandbox for workload credential isolation.
|
||||
/// These enforcement details must not be exposed in model-visible context.
|
||||
/// Runtime-only paths added to the sandbox solely for workload credential isolation.
|
||||
/// Pre-existing user or managed denies are excluded so they remain model-visible.
|
||||
#[doc(hidden)]
|
||||
pub workload_identity_credential_deny_paths: Vec<AbsolutePathBuf>,
|
||||
|
||||
@@ -2370,6 +2370,21 @@ fn apply_workload_identity_filesystem_constraints(
|
||||
.preserve_deny_read_restrictions_from(&FileSystemSandboxPolicy::restricted(entries));
|
||||
}
|
||||
|
||||
fn file_system_policy_has_exact_deny_path(
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy,
|
||||
path: &AbsolutePathBuf,
|
||||
) -> bool {
|
||||
file_system_sandbox_policy.entries.iter().any(|entry| {
|
||||
entry.access == codex_protocol::permissions::FileSystemAccessMode::Deny
|
||||
&& matches!(
|
||||
&entry.path,
|
||||
codex_protocol::permissions::FileSystemPath::Path {
|
||||
path: denied_path,
|
||||
} if denied_path == path
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
fn workload_identity_credential_deny_paths(
|
||||
workload_identity: &WorkloadIdentityConfig,
|
||||
) -> Vec<AbsolutePathBuf> {
|
||||
@@ -3642,11 +3657,21 @@ impl Config {
|
||||
})?;
|
||||
let credential_deny_paths =
|
||||
workload_identity_credential_deny_paths(workload_identity);
|
||||
let enforcement_only_paths = credential_deny_paths
|
||||
.iter()
|
||||
.filter(|path| {
|
||||
!file_system_policy_has_exact_deny_path(
|
||||
&effective_file_system_sandbox_policy,
|
||||
path,
|
||||
)
|
||||
})
|
||||
.cloned()
|
||||
.collect();
|
||||
apply_workload_identity_filesystem_constraints(
|
||||
&mut effective_file_system_sandbox_policy,
|
||||
&credential_deny_paths,
|
||||
);
|
||||
credential_deny_paths
|
||||
enforcement_only_paths
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
|
||||
@@ -607,7 +607,7 @@ impl Session {
|
||||
let auth = if config_for_mcp.model_provider.requires_openai_auth {
|
||||
auth_manager_clone.auth_result().await?
|
||||
} else {
|
||||
auth_manager_clone.auth_cached()
|
||||
auth_manager_clone.auth_for_optional_use().await
|
||||
};
|
||||
let mcp_config = mcp_manager_for_mcp
|
||||
.runtime_config_for_thread(&config_for_mcp, mcp_thread_init_for_startup)
|
||||
|
||||
@@ -8278,6 +8278,33 @@ async fn turn_context_item_redacts_enforcement_only_paths_from_split_policy() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn turn_context_item_preserves_user_deny_overlapping_workload_credential() {
|
||||
let (_session, mut turn_context) = make_session_and_context().await;
|
||||
let credential_path = turn_context.config.codex_home.join("workload-token");
|
||||
let mut file_system_policy = file_system_policy_with_unreadable_glob(&turn_context);
|
||||
file_system_policy.entries.push(FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path {
|
||||
path: credential_path.clone(),
|
||||
},
|
||||
access: FileSystemAccessMode::Deny,
|
||||
});
|
||||
turn_context.permission_profile = PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
turn_context.permission_profile.enforcement(),
|
||||
&file_system_policy,
|
||||
turn_context.network_sandbox_policy(),
|
||||
);
|
||||
|
||||
let item = turn_context.to_turn_context_item();
|
||||
|
||||
assert_eq!(item.file_system_sandbox_policy, Some(file_system_policy));
|
||||
assert!(
|
||||
serde_json::to_string(&item)
|
||||
.expect("serialize turn context item")
|
||||
.contains(credential_path.to_string_lossy().as_ref())
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn record_context_updates_and_set_reference_context_item_injects_full_context_when_baseline_missing()
|
||||
{
|
||||
|
||||
@@ -2,9 +2,11 @@
|
||||
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use codex_config::CONFIG_TOML_FILE;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -16,6 +18,7 @@ use core_test_support::apps_test_server::SEARCH_CALENDAR_EXTRACT_TEXT_TOOL as DO
|
||||
use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE as DOCUMENT_EXTRACT_NAMESPACE;
|
||||
use core_test_support::apps_test_server::apps_enabled_builder;
|
||||
use core_test_support::apps_test_server::recorded_apps_tool_call_by_name;
|
||||
use core_test_support::apps_test_server::recorded_apps_tool_calls;
|
||||
use core_test_support::hooks::trust_discovered_hooks;
|
||||
use core_test_support::responses::ResponseMock;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
@@ -29,6 +32,7 @@ use core_test_support::test_codex::TestCodex;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use tempfile::TempDir;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
@@ -136,8 +140,8 @@ async fn mount_file_upload_mocks(server: &MockServer, file_size_bytes: u64) {
|
||||
.await;
|
||||
}
|
||||
|
||||
async fn run_extract_turn(test: &TestCodex, server: &MockServer) -> Result<ResponseMock> {
|
||||
let mock = mount_sse_sequence(
|
||||
async fn mount_extract_responses(server: &MockServer) -> ResponseMock {
|
||||
mount_sse_sequence(
|
||||
server,
|
||||
vec![
|
||||
sse(vec![
|
||||
@@ -157,12 +161,20 @@ async fn run_extract_turn(test: &TestCodex, server: &MockServer) -> Result<Respo
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
.await
|
||||
}
|
||||
|
||||
async fn run_extract_turn(
|
||||
test: &TestCodex,
|
||||
server: &MockServer,
|
||||
permission_profile: PermissionProfile,
|
||||
) -> Result<ResponseMock> {
|
||||
let mock = mount_extract_responses(server).await;
|
||||
|
||||
test.submit_turn_with_approval_and_permission_profile(
|
||||
"Extract the report text with the app tool.",
|
||||
AskForApproval::Never,
|
||||
PermissionProfile::Disabled,
|
||||
permission_profile,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -187,7 +199,7 @@ async fn codex_apps_file_params_upload_environment_files_before_mcp_tool_call()
|
||||
Ok(())
|
||||
});
|
||||
let test = builder.build_with_remote_env(&server).await?;
|
||||
let mock = run_extract_turn(&test, &server).await?;
|
||||
let mock = run_extract_turn(&test, &server, PermissionProfile::Disabled).await?;
|
||||
|
||||
let requests = mock.requests();
|
||||
let body = requests[0].body_json();
|
||||
@@ -243,7 +255,7 @@ async fn codex_apps_file_params_pass_uploaded_file_to_post_tool_use_hook() -> Re
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
tokio::fs::write(test.cwd.path().join("report.txt"), b"hello world").await?;
|
||||
let _responses = run_extract_turn(&test, &server).await?;
|
||||
let _responses = run_extract_turn(&test, &server, PermissionProfile::Disabled).await?;
|
||||
|
||||
let hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
@@ -255,3 +267,78 @@ async fn codex_apps_file_params_pass_uploaded_file_to_post_tool_use_hook() -> Re
|
||||
server.verify().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn codex_apps_file_params_reject_wif_credential_symlink_before_upload() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let apps_server = AppsTestServer::mount(&server).await?;
|
||||
let home = Arc::new(TempDir::new()?);
|
||||
let credential_path = home.path().join("subject-token");
|
||||
let credential_contents = "external.subject.token.must-not-leak";
|
||||
fs::write(&credential_path, credential_contents)?;
|
||||
|
||||
let credential_path_for_config = credential_path.clone();
|
||||
let mut builder = apps_enabled_builder(apps_server.chatgpt_base_url.clone())
|
||||
.with_home(Arc::clone(&home))
|
||||
.with_pre_build_hook(move |home| {
|
||||
let config = format!(
|
||||
r#"[workload_identity]
|
||||
identity_provider_id = "idp_example"
|
||||
identity_provider_mapping_id = "idpm_example"
|
||||
audience = "openai-audience"
|
||||
token_url = "https://auth.openai.com/oauth/token"
|
||||
|
||||
[workload_identity.credential_source]
|
||||
type = "file"
|
||||
path = "{}"
|
||||
|
||||
[apps.calendar]
|
||||
default_tools_approval_mode = "approve"
|
||||
"#,
|
||||
credential_path_for_config.display()
|
||||
);
|
||||
fs::write(home.join(CONFIG_TOML_FILE), config).expect("write workload identity config");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
let upload_path = test.cwd.path().join("report.txt");
|
||||
std::os::unix::fs::symlink(&credential_path, &upload_path)?;
|
||||
|
||||
assert!(
|
||||
test.config
|
||||
.workload_identity_credential_deny_paths
|
||||
.iter()
|
||||
.any(|path| path.as_path() == credential_path)
|
||||
);
|
||||
let responses = run_extract_turn(
|
||||
&test,
|
||||
&server,
|
||||
test.config.permissions.permission_profile().clone(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
let output = responses
|
||||
.function_call_output_text("extract-call-1")
|
||||
.context("Apps file tool failure should be returned to the model")?;
|
||||
assert!(output.contains("path is not readable under the current sandbox policy"));
|
||||
assert!(!output.contains(credential_contents));
|
||||
assert!(!output.contains(credential_path.to_string_lossy().as_ref()));
|
||||
assert_eq!(recorded_apps_tool_calls(&server).await, Vec::<Value>::new());
|
||||
|
||||
let upload_requests = server
|
||||
.received_requests()
|
||||
.await
|
||||
.unwrap_or_default()
|
||||
.into_iter()
|
||||
.filter(|request| {
|
||||
matches!(
|
||||
request.url.path(),
|
||||
"/files" | "/upload/file_123" | "/files/file_123/uploaded"
|
||||
)
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
assert!(upload_requests.is_empty(), "unexpected file upload request");
|
||||
|
||||
server.verify().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -801,6 +801,30 @@ async fn auth_result_preserves_external_auth_resolution_errors() {
|
||||
assert!(manager.auth().await.is_err());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn optional_auth_does_not_resolve_required_external_auth() {
|
||||
let cached = CodexAuth::create_dummy_chatgpt_auth_for_testing();
|
||||
let manager = AuthManager::from_auth_for_testing(cached.clone());
|
||||
manager.set_external_auth(Arc::new(FailingExternalChatgptAuth(
|
||||
"required external auth must not run",
|
||||
)));
|
||||
|
||||
let resolved = manager
|
||||
.auth_for_optional_use()
|
||||
.await
|
||||
.expect("cached ChatGPT auth should remain available");
|
||||
|
||||
assert_eq!(resolved.get_token().unwrap(), cached.get_token().unwrap());
|
||||
assert_eq!(
|
||||
manager
|
||||
.auth()
|
||||
.await
|
||||
.expect_err("normal auth should still require external resolution")
|
||||
.to_string(),
|
||||
"required external auth must not run"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn replace_non_required_external_auth_preserves_required_provider() {
|
||||
let manager = AuthManager::shared(
|
||||
|
||||
@@ -1731,6 +1731,30 @@ impl AuthManager {
|
||||
Ok(self.auth_cached())
|
||||
}
|
||||
|
||||
/// Resolves auth for optional OpenAI-backed features while another model provider is active.
|
||||
///
|
||||
/// A required external auth source belongs to the primary model provider and must not be
|
||||
/// invoked merely to initialize an optional feature. Managed ChatGPT auth and optional
|
||||
/// external auth still use the normal resolution path, including proactive token refresh.
|
||||
pub async fn auth_for_optional_use(&self) -> Option<CodexAuth> {
|
||||
if self
|
||||
.external_auth()
|
||||
.is_some_and(|external_auth| external_auth.requires_successful_resolution())
|
||||
{
|
||||
return self.auth_cached();
|
||||
}
|
||||
|
||||
match self.auth().await {
|
||||
Ok(auth) => auth,
|
||||
Err(error) => {
|
||||
// The external auth registration can race with the check above. Optional features
|
||||
// should keep using the last managed credential instead of failing startup.
|
||||
tracing::error!("Failed to resolve optional feature auth: {error}");
|
||||
self.auth_cached()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Backwards-compatible explicit name for callers that already handle auth resolution errors.
|
||||
pub async fn auth_result(&self) -> std::io::Result<Option<CodexAuth>> {
|
||||
self.auth().await
|
||||
|
||||
@@ -180,6 +180,16 @@ async fn refresh_token_refreshes_when_auth_is_unchanged() -> Result<()> {
|
||||
#[serial_test::serial(auth_env)]
|
||||
#[tokio::test]
|
||||
async fn auth_refreshes_when_access_token_is_near_expiry() -> Result<()> {
|
||||
assert_auth_refreshes_when_access_token_is_near_expiry(/*optional*/ false).await
|
||||
}
|
||||
|
||||
#[serial_test::serial(auth_env)]
|
||||
#[tokio::test]
|
||||
async fn optional_auth_refreshes_when_access_token_is_near_expiry() -> Result<()> {
|
||||
assert_auth_refreshes_when_access_token_is_near_expiry(/*optional*/ true).await
|
||||
}
|
||||
|
||||
async fn assert_auth_refreshes_when_access_token_is_near_expiry(optional: bool) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = MockServer::start().await;
|
||||
@@ -208,12 +218,18 @@ async fn auth_refreshes_when_access_token_is_near_expiry() -> Result<()> {
|
||||
};
|
||||
ctx.write_auth(&initial_auth).await?;
|
||||
|
||||
let cached_auth = ctx
|
||||
.auth_manager
|
||||
.auth()
|
||||
.await
|
||||
.context("auth should resolve")?
|
||||
.context("auth should be cached")?;
|
||||
let cached_auth = if optional {
|
||||
ctx.auth_manager
|
||||
.auth_for_optional_use()
|
||||
.await
|
||||
.context("optional auth should be cached")?
|
||||
} else {
|
||||
ctx.auth_manager
|
||||
.auth()
|
||||
.await
|
||||
.context("auth should resolve")?
|
||||
.context("auth should be cached")?
|
||||
};
|
||||
|
||||
let refreshed_tokens = TokenData {
|
||||
access_token: "new-access-token".to_string(),
|
||||
|
||||
Reference in New Issue
Block a user