diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 5e985da92b..5876743504 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2859,6 +2859,7 @@ dependencies = [ "thiserror 2.0.18", "tokio", "url", + "wiremock", ] [[package]] diff --git a/codex-rs/core/src/tools/handlers/dependency_check.rs b/codex-rs/core/src/tools/handlers/dependency_check.rs index 96c57d2612..1921498d9d 100644 --- a/codex-rs/core/src/tools/handlers/dependency_check.rs +++ b/codex-rs/core/src/tools/handlers/dependency_check.rs @@ -435,11 +435,7 @@ impl DependencyCommandRunner { .additional_permissions_preapproved, justification: Some(justification.to_string()), exec_approval_requirement, - approval_review_mode: match working_directory_access { - WorkingDirectoryAccess::ProjectDependencyFiles => ApprovalReviewMode::User, - WorkingDirectoryAccess::Default - | WorkingDirectoryAccess::PreapprovedScratch => ApprovalReviewMode::Configured, - }, + approval_review_mode: approval_review_mode(working_directory_access), }; let tool_ctx = ToolCtx { session: self.session.clone(), @@ -448,8 +444,7 @@ impl DependencyCommandRunner { tool_name: self.tool_name.clone(), }; let mut orchestrator = ToolOrchestrator::new(); - let mut runtime = - ShellRuntime::for_shell_command(ShellRuntimeBackend::ShellCommandClassic); + let mut runtime = ShellRuntime::for_shell_command(ShellRuntimeBackend::ShellCommandClassic); orchestrator .run( &mut runtime, @@ -468,6 +463,15 @@ impl DependencyCommandRunner { } } +fn approval_review_mode(working_directory_access: WorkingDirectoryAccess) -> ApprovalReviewMode { + match working_directory_access { + WorkingDirectoryAccess::ProjectDependencyFiles => ApprovalReviewMode::User, + WorkingDirectoryAccess::Default | WorkingDirectoryAccess::PreapprovedScratch => { + ApprovalReviewMode::Configured + } + } +} + fn command_permissions( cwd: &Path, working_directory_access: WorkingDirectoryAccess, @@ -580,6 +584,22 @@ async fn copy_resolution_inputs( mod tests { use super::*; + fn write_permissions(paths: Vec) -> AdditionalPermissionProfile { + AdditionalPermissionProfile { + file_system: Some(FileSystemPermissions { + entries: paths + .into_iter() + .map(|path| FileSystemSandboxEntry { + path: FileSystemPath::Path { path }, + access: FileSystemAccessMode::Write, + }) + .collect(), + glob_scan_max_depth: None, + }), + ..Default::default() + } + } + #[test] fn raw_install_redirect_names_package_manager() { let message = dependency_install_redirect(&[ @@ -607,16 +627,9 @@ mod tests { assert!(permissions.additional_permissions_preapproved); assert_eq!( permissions.additional_permissions, - Some(PermissionProfile { - file_system: Some(FileSystemPermissions { - read: None, - write: Some(vec![ - AbsolutePathBuf::from_absolute_path(scratch.path()) - .expect("absolute scratch path") - ]), - }), - ..Default::default() - }) + Some(write_permissions(vec![ + AbsolutePathBuf::from_absolute_path(scratch.path()).expect("absolute scratch path") + ])) ); } @@ -637,20 +650,16 @@ mod tests { assert!(!permissions.additional_permissions_preapproved); assert_eq!( permissions.additional_permissions, - Some(PermissionProfile { - file_system: Some(FileSystemPermissions { - read: None, - write: Some(vec![ - AbsolutePathBuf::from_absolute_path(project.path().join("package.json")) - .expect("absolute package.json path"), - AbsolutePathBuf::from_absolute_path( - project.path().join("package-lock.json") - ) - .expect("absolute package-lock.json path"), - ]), - }), - ..Default::default() - }) + Some(write_permissions(vec![ + AbsolutePathBuf::from_absolute_path(project.path().join("package.json")) + .expect("absolute package.json path"), + AbsolutePathBuf::from_absolute_path(project.path().join("package-lock.json")) + .expect("absolute package-lock.json path"), + ])) + ); + assert_eq!( + approval_review_mode(WorkingDirectoryAccess::ProjectDependencyFiles), + ApprovalReviewMode::User ); } } diff --git a/codex-rs/core/src/tools/handlers/dependency_check_output.rs b/codex-rs/core/src/tools/handlers/dependency_check_output.rs index 3d351476d2..b627d0313b 100644 --- a/codex-rs/core/src/tools/handlers/dependency_check_output.rs +++ b/codex-rs/core/src/tools/handlers/dependency_check_output.rs @@ -1,4 +1,3 @@ -use codex_protocol::exec_output::ExecToolCallOutput; use crate::tools::context::FunctionToolOutput; use codex_dependency_check::DependencyCheckRequest; use codex_dependency_check::DependencyPolicyAction; @@ -7,6 +6,7 @@ use codex_dependency_check::DependencyRisk; use codex_dependency_check::DependencyRiskKind; use codex_dependency_check::NpmGraphMismatch; use codex_dependency_check::NpmInstalledGraphMismatch; +use codex_protocol::exec_output::ExecToolCallOutput; pub(super) fn blocked_output(message: String) -> FunctionToolOutput { FunctionToolOutput::from_text(message, Some(false)) diff --git a/codex-rs/core/src/tools/handlers/dependency_guard.rs b/codex-rs/core/src/tools/handlers/dependency_guard.rs index 5fd16cf2f9..a197acfc71 100644 --- a/codex-rs/core/src/tools/handlers/dependency_guard.rs +++ b/codex-rs/core/src/tools/handlers/dependency_guard.rs @@ -87,7 +87,7 @@ mod tests { assert!(dependency_permissions_overlap_project( SandboxPermissions::RequireEscalated, - None, + /*additional_permissions*/ None, &root, )); assert!(dependency_permissions_overlap_project( @@ -101,8 +101,8 @@ mod tests { fn permits_unrelated_write_permissions() { let root = AbsolutePathBuf::from_absolute_path("/workspace/project") .expect("absolute project root"); - let cache = AbsolutePathBuf::from_absolute_path("/var/cache/example") - .expect("absolute cache path"); + let cache = + AbsolutePathBuf::from_absolute_path("/var/cache/example").expect("absolute cache path"); let profile = write_profile(FileSystemPath::Path { path: cache }); assert_eq!( diff --git a/codex-rs/core/src/tools/handlers/request_permissions.rs b/codex-rs/core/src/tools/handlers/request_permissions.rs index 3d7afc6b30..e2bf016a04 100644 --- a/codex-rs/core/src/tools/handlers/request_permissions.rs +++ b/codex-rs/core/src/tools/handlers/request_permissions.rs @@ -6,11 +6,11 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; +use crate::tools::handlers::dependency_guard::dependency_permission_request_message; +use crate::tools::handlers::dependency_guard::dependency_permissions_overlap_project; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_tool_environment; -use crate::tools::handlers::dependency_guard::dependency_permission_request_message; -use crate::tools::handlers::dependency_guard::dependency_permissions_overlap_project; use crate::tools::handlers::shell_spec::create_request_permissions_tool; use crate::tools::handlers::shell_spec::request_permissions_tool_description; use crate::tools::registry::CoreToolRuntime; @@ -93,7 +93,10 @@ impl RequestPermissionsHandler { "request_permissions requires at least one permission".to_string(), )); } - if session.features().enabled(codex_features::Feature::DependencyCheck) { + if session + .features() + .enabled(codex_features::Feature::DependencyCheck) + { let permissions: codex_protocol::models::AdditionalPermissionProfile = args.permissions.clone().into(); if dependency_permissions_overlap_project( diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 191ab1d38c..3e14f05f85 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -26,8 +26,8 @@ use crate::tools::orchestrator::ToolOrchestrator; use crate::tools::runtimes::shell::ShellRequest; use crate::tools::runtimes::shell::ShellRuntime; use crate::tools::runtimes::shell::ShellRuntimeBackend; -use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ApprovalReviewMode; +use crate::tools::sandboxing::ToolCtx; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::ExecCommandSource; use codex_tools::ToolName; @@ -129,11 +129,15 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result bool { + review_mode == ApprovalReviewMode::Configured && (configured_for_guardian || strict_auto_review) +} + fn sandbox_outcome_from_tool_error(err: &ToolError) -> Option<&'static str> { match err { ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { .. })) => Some("denied"), @@ -651,3 +663,22 @@ fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String { // output so we can evolve heuristics later without touching call sites. "command failed; retry without sandbox?".to_string() } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn user_review_mode_bypasses_guardian_and_strict_auto_review() { + assert!(!use_guardian_for_review_mode( + /*configured_for_guardian*/ true, + /*strict_auto_review*/ true, + ApprovalReviewMode::User, + )); + assert!(use_guardian_for_review_mode( + /*configured_for_guardian*/ true, + /*strict_auto_review*/ false, + ApprovalReviewMode::Configured, + )); + } +} diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index d19744be46..49c79ae530 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -749,6 +749,20 @@ async fn sleep_tool_follows_current_time_config() { ); } +#[tokio::test] +async fn dependency_check_tool_requires_feature_flag() { + let disabled = probe(|_| {}).await; + disabled.assert_visible_lacks(&["dependency_check"]); + disabled.assert_registered_lacks(&["dependency_check"]); + + let enabled = probe(|turn| { + set_feature(turn, Feature::DependencyCheck, /*enabled*/ true); + }) + .await; + enabled.assert_visible_contains(&["dependency_check"]); + enabled.assert_registered_contains(&["dependency_check"]); +} + #[tokio::test] async fn mcp_and_tool_search_follow_direct_and_deferred_tool_exposure() { let direct_mcp = probe_with( diff --git a/codex-rs/core/tests/suite/dependency_check.rs b/codex-rs/core/tests/suite/dependency_check.rs new file mode 100644 index 0000000000..9ce2957acb --- /dev/null +++ b/codex-rs/core/tests/suite/dependency_check.rs @@ -0,0 +1,197 @@ +use anyhow::Result; +use codex_features::Feature; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::request_permissions::RequestPermissionProfile; +use core_test_support::responses::ev_apply_patch_custom_tool_call; +use core_test_support::responses::ev_assistant_message; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_sse_sequence; +use core_test_support::responses::sse; +use core_test_support::test_codex::TestCodexBuilder; +use core_test_support::test_codex::TestCodexHarness; +use core_test_support::test_codex::test_codex; +use serde_json::json; + +fn run_dependency_e2e(test: F) -> Result<()> +where + F: FnOnce() -> Fut + Send + 'static, + Fut: std::future::Future> + 'static, +{ + std::thread::Builder::new() + .name("dependency-check-e2e".to_string()) + .stack_size(32 * 1024 * 1024) + .spawn(move || { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build()? + .block_on(test()) + })? + .join() + .map_err(|_| anyhow::anyhow!("dependency check test thread panicked"))? +} + +fn dependency_check_builder() -> TestCodexBuilder { + test_codex().with_model("gpt-5.4").with_config(|config| { + config + .features + .enable(Feature::DependencyCheck) + .expect("test config should enable dependency_check"); + }) +} + +async fn mount_function_call( + harness: &TestCodexHarness, + call_id: &str, + tool_name: &str, + arguments: serde_json::Value, +) -> Result<()> { + mount_sse_sequence( + harness.server(), + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, tool_name, &serde_json::to_string(&arguments)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + Ok(()) +} + +#[test] +fn shell_dependency_install_redirects_without_running_npm() -> Result<()> { + run_dependency_e2e(|| async { + let harness = TestCodexHarness::with_builder(dependency_check_builder()).await?; + let call_id = "dependency-shell-redirect"; + mount_function_call( + &harness, + call_id, + "shell_command", + json!({"command": "npm install zod@3.23.8"}), + ) + .await?; + + harness.submit("add zod").await?; + + let output = harness.function_call_stdout(call_id).await; + assert!(output.contains("Dependency Check is enabled")); + assert!(output.contains("dependency_check")); + assert!(!harness.path("package-lock.json").exists()); + Ok(()) + }) +} + +#[test] +fn unified_exec_dependency_install_redirects_without_allocating_a_process() -> Result<()> { + run_dependency_e2e(|| async { + let builder = dependency_check_builder().with_config(|config| { + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should enable unified exec"); + }); + let harness = TestCodexHarness::with_builder(builder).await?; + let call_id = "dependency-unified-redirect"; + mount_function_call( + &harness, + call_id, + "exec_command", + json!({"cmd": "npm install zod@3.23.8", "yield_time_ms": 1_000}), + ) + .await?; + + harness.submit("add zod").await?; + + let output = harness.function_call_stdout(call_id).await; + assert!(output.contains("Dependency Check is enabled")); + assert!(!output.contains("Process running with session ID")); + assert!(!harness.path("package-lock.json").exists()); + Ok(()) + }) +} + +#[test] +fn apply_patch_rejects_dependency_manifest_edits_without_mutation() -> Result<()> { + run_dependency_e2e(|| async { + let harness = TestCodexHarness::with_builder(dependency_check_builder()).await?; + harness + .write_file("package.json", br#"{"name":"fixture","version":"1.0.0"}"#) + .await?; + let before = std::fs::read_to_string(harness.path("package.json"))?; + let call_id = "dependency-apply-patch-rejection"; + let patch = "*** Begin Patch\n*** Update File: package.json\n@@\n-{\"name\":\"fixture\",\"version\":\"1.0.0\"}\n+{\"name\":\"fixture\",\"version\":\"1.0.1\"}\n*** End Patch"; + mount_sse_sequence( + harness.server(), + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_apply_patch_custom_tool_call(call_id, patch), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + harness.submit("edit package.json").await?; + + let output = harness.apply_patch_output(call_id).await; + assert!(output.contains("Do not edit JavaScript dependency manifests")); + assert_eq!( + std::fs::read_to_string(harness.path("package.json"))?, + before + ); + Ok(()) + }) +} + +#[test] +fn request_permissions_rejects_project_write_grants() -> Result<()> { + run_dependency_e2e(|| async { + let builder = dependency_check_builder().with_config(|config| { + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("test config should enable request_permissions"); + }); + let harness = TestCodexHarness::with_builder(builder).await?; + let permissions = RequestPermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_roots( + /*read*/ None, + Some(vec![harness.path_abs("package.json")]), + )), + network: None, + }; + let call_id = "dependency-request-permissions-rejection"; + mount_function_call( + &harness, + call_id, + "request_permissions", + json!({ + "reason": "make the project writable", + "permissions": permissions, + }), + ) + .await?; + + harness.submit("request project write access").await?; + + let output = harness.function_call_stdout(call_id).await; + assert!( + output.contains("Generic tools cannot request permissions"), + "unexpected request_permissions output: {output}" + ); + Ok(()) + }) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index e8bb48856c..c8b4dbdf51 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -49,6 +49,7 @@ mod compact_remote; mod compact_remote_parity; mod compact_resume_fork; mod current_time_reminder; +mod dependency_check; mod deprecation_notice; mod exec; mod exec_policy; diff --git a/codex-rs/dependency-check/Cargo.toml b/codex-rs/dependency-check/Cargo.toml index a650e800b8..5593595be5 100644 --- a/codex-rs/dependency-check/Cargo.toml +++ b/codex-rs/dependency-check/Cargo.toml @@ -25,3 +25,4 @@ url = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } tokio = { workspace = true, features = ["macros", "rt"] } +wiremock = { workspace = true } diff --git a/codex-rs/dependency-check/src/npm.rs b/codex-rs/dependency-check/src/npm.rs index c5a956e387..b69c0701d5 100644 --- a/codex-rs/dependency-check/src/npm.rs +++ b/codex-rs/dependency-check/src/npm.rs @@ -428,6 +428,25 @@ mod tests { assert_eq!(graph.compare_installed(&installed), Ok(())); } + #[test] + fn reports_lock_and_installed_graph_mismatches() { + let checked = NpmGraph::from_query_json( + r#"[{"name":"zod","version":"3.23.8","location":"node_modules/zod","resolved":"https://registry.npmjs.org/zod/-/zod-3.23.8.tgz","integrity":"sha512-checked"}]"#, + ) + .expect("checked graph"); + let changed = NpmGraph::from_query_json( + r#"[{"name":"zod","version":"3.24.0","location":"node_modules/zod","resolved":"https://registry.npmjs.org/zod/-/zod-3.24.0.tgz","integrity":"sha512-changed"}]"#, + ) + .expect("changed graph"); + let installed = NpmInstalledGraph::from_query_json( + r#"[{"name":"zod","version":"3.24.0","location":"node_modules/zod","resolved":"https://registry.npmjs.org/zod/-/zod-3.24.0.tgz"}]"#, + ) + .expect("installed graph"); + + assert!(checked.compare(&changed).is_err()); + assert!(checked.compare_installed(&installed).is_err()); + } + #[test] fn rejects_unverifiable_sources() { let err = NpmGraph::from_query_json( diff --git a/codex-rs/dependency-check/src/osv.rs b/codex-rs/dependency-check/src/osv.rs index 36e2e5b2c4..a191834afa 100644 --- a/codex-rs/dependency-check/src/osv.rs +++ b/codex-rs/dependency-check/src/osv.rs @@ -191,6 +191,18 @@ struct Vulnerability { mod tests { use super::*; use pretty_assertions::assert_eq; + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::method; + use wiremock::matchers::path; + + fn graph() -> NpmGraph { + NpmGraph::from_query_json( + r#"[{"name":"example","version":"1.0.0","location":"node_modules/example","resolved":"https://registry.npmjs.org/example/-/example-1.0.0.tgz","integrity":"sha512-example"}]"#, + ) + .expect("parse graph") + } #[test] fn maps_malware_to_block_and_cves_to_warn() { @@ -242,4 +254,47 @@ mod tests { .expect_err("partial response should fail"); assert!(matches!(err, OsvError::ResultCountMismatch { .. })); } + + #[tokio::test] + async fn mocked_malware_response_blocks_dependency_graph() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/querybatch")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "results": [{ + "vulns": [{"id": "MAL-2026-1", "summary": "malware"}] + }] + }))) + .mount(&server) + .await; + let endpoint = + Url::parse(&format!("{}/v1/querybatch", server.uri())).expect("mock endpoint"); + let report = OsvClient::with_endpoint(endpoint) + .expect("client") + .evaluate(&graph()) + .await + .expect("policy report"); + + assert_eq!(report.action, DependencyPolicyAction::Block); + assert_eq!(report.risks[0].kind, DependencyRiskKind::Malware); + } + + #[tokio::test] + async fn mocked_provider_failure_fails_closed() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/querybatch")) + .respond_with(ResponseTemplate::new(503)) + .mount(&server) + .await; + let endpoint = + Url::parse(&format!("{}/v1/querybatch", server.uri())).expect("mock endpoint"); + let err = OsvClient::with_endpoint(endpoint) + .expect("client") + .evaluate(&graph()) + .await + .expect_err("provider failure should fail closed"); + + assert!(matches!(err, OsvError::Request(_))); + } } diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index 07d181e7fd..63e200d77d 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -26,8 +26,8 @@ pub use code_mode::code_mode_name_for_tool_name; pub use code_mode::collect_code_mode_exec_prompt_tool_definitions; pub use code_mode::collect_code_mode_tool_definitions; pub use code_mode::tool_spec_to_code_mode_tool_definition; -pub use dependency_check_tool::create_dependency_check_tool; pub use codex_protocol::ToolName; +pub use dependency_check_tool::create_dependency_check_tool; pub use dynamic_tool::parse_dynamic_tool; pub use function_call_error::FunctionCallError; pub use image_detail::can_request_original_image_detail; diff --git a/docs/dependency-check.md b/docs/dependency-check.md index d610f343f6..931721a906 100644 --- a/docs/dependency-check.md +++ b/docs/dependency-check.md @@ -50,12 +50,17 @@ not a general assertion that a package is trustworthy. When the feature is enabled, Codex rejects recognizable direct dependency-add commands such as `npm install package@version`, `pnpm add`, `yarn add`, and `bun add`, directing the model to `dependency_check`. It also rejects direct -`apply_patch` changes to JavaScript package manifests and lockfiles. +`apply_patch` changes to JavaScript package manifests and lockfiles. Generic +shell tools cannot request full sandbox bypass or additional write permissions +that overlap the active project, and `request_permissions` cannot grant write +access to dependency manifests. The real project mutation still uses the existing shell sandbox and approval -orchestrator. A permission profile can therefore keep the root manifest and -lockfile read-only inside the sandbox so the checked update produces a user -approval prompt: +orchestrator. Its request is limited to `package.json` and `package-lock.json` +and is always routed to the user, even when the session normally uses automatic +approval review. A permission profile should keep the root manifest and +lockfile read-only inside the ambient sandbox so only the structured tool can +request that exact write access: ```toml default_permissions = "dependency-check" @@ -93,7 +98,8 @@ filesystem enforcement. The tool grants its own temporary resolution directory as a preapproved, per-command write path; that does not widen access to the project. The checked project update separately requests per-command write access to `package.json` and `package-lock.json`; with `on-request` -approval, that request produces the user prompt. +approval, that request produces the human prompt. Denial stops before the +project command runs. ## Current limitations