test: harden dependency check approval flow

This commit is contained in:
Casey Silver
2026-07-07 22:19:13 -07:00
parent ba40f20930
commit 48cdaf27d1
16 changed files with 395 additions and 50 deletions
+1
View File
@@ -2859,6 +2859,7 @@ dependencies = [
"thiserror 2.0.18",
"tokio",
"url",
"wiremock",
]
[[package]]
@@ -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<AbsolutePathBuf>) -> 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
);
}
}
@@ -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))
@@ -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!(
@@ -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(
+6 -2
View File
@@ -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<FunctionToolOutput, Func
)
.map_err(FunctionCallError::RespondToModel)?;
let dependency_project_root = turn_environment
.cwd()
.to_abs_path()
.unwrap_or_else(|_| exec_params.cwd.clone());
if session.features().enabled(Feature::DependencyCheck)
&& dependency_permissions_overlap_project(
effective_additional_permissions.sandbox_permissions,
normalized_additional_permissions.as_ref(),
&exec_params.cwd,
&dependency_project_root,
)
{
return Err(FunctionCallError::RespondToModel(
@@ -332,11 +332,15 @@ impl ExecCommandHandler {
}
};
let dependency_project_root = turn_environment
.cwd()
.to_abs_path()
.unwrap_or_else(|_| turn.config.cwd.clone());
if session.features().enabled(Feature::DependencyCheck)
&& dependency_permissions_overlap_project(
effective_additional_permissions.sandbox_permissions,
normalized_additional_permissions.as_ref(),
permission_cwd,
&dependency_project_root,
)
{
manager.release_process_id(process_id).await;
+34 -3
View File
@@ -157,8 +157,11 @@ impl ToolOrchestrator {
let otel_ci = &tool_ctx.call_id;
let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await;
let force_user_review = tool.approval_review_mode(req) == ApprovalReviewMode::User;
let use_guardian =
!force_user_review && (routes_approval_to_guardian(turn_ctx) || strict_auto_review);
let use_guardian = use_guardian_for_review_mode(
routes_approval_to_guardian(turn_ctx),
strict_auto_review,
tool.approval_review_mode(req),
);
// 1) Approval
let mut already_approved = false;
@@ -221,7 +224,8 @@ impl ToolOrchestrator {
tool_ctx.call_id.as_str(),
approval_ctx,
tool_ctx,
/*evaluate_permission_request_hooks*/ !strict_auto_review || force_user_review,
/*evaluate_permission_request_hooks*/
!strict_auto_review || force_user_review,
&otel,
)
.await?;
@@ -637,6 +641,14 @@ impl ToolOrchestrator {
}
}
fn use_guardian_for_review_mode(
configured_for_guardian: bool,
strict_auto_review: bool,
review_mode: ApprovalReviewMode,
) -> 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,
));
}
}
@@ -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(
@@ -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<F, Fut>(test: F) -> Result<()>
where
F: FnOnce() -> Fut + Send + 'static,
Fut: std::future::Future<Output = Result<()>> + '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(())
})
}
+1
View File
@@ -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;
+1
View File
@@ -25,3 +25,4 @@ url = { workspace = true }
[dev-dependencies]
pretty_assertions = { workspace = true }
tokio = { workspace = true, features = ["macros", "rt"] }
wiremock = { workspace = true }
+19
View File
@@ -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(
+55
View File
@@ -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(_)));
}
}
+1 -1
View File
@@ -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;
+11 -5
View File
@@ -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