github/codex
4
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-20 00:18:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Constrain Guardian reviews to parent filesystem permissions (#38377)

Browse Source 
## Why

Guardian review sessions must not gain access to paths that the parent turn is
not allowed to read.

## What changed

- Derive Guardian permissions by intersecting managed parent filesystem rules
  with read-only access, preserving denied paths and restricting network access.
- Offer Guardian execution tools only when a managed sandbox can enforce those
  rules.
- Include the selected environment IDs in the review-session reuse key so a
  session is not reused across different environment sets.

## Testing

Update the Guardian reuse integration test to verify that a review cannot read
a parent-denied file or write a local file while consecutive reviews still
reuse the same session.

GitOrigin-RevId: 20f17a6c379f1eda651e8508459d642a51e4ce94
This commit is contained in:
jif
2026-08-13 13:41:08 +00:00
committed by copyberry
parent a7b8c074b5
commit a7e9fb5480
4 changed files with 140 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/guardian/review_session.rs
+31 -12
View File
@@ -44,6 +44,7 @@ use crate::config::NetworkProxySpec;
use crate::config::Permissions;
use crate::context::ContextualUserFragment;
use crate::context::GuardianFollowupReviewReminder;
use crate::environment_selection::TurnEnvironmentSnapshot;
use crate::session::GitEnrichmentPolicy;
use crate::session::SessionIo;
use crate::session::session::Session;
@@ -184,6 +185,7 @@ struct GuardianReviewSessionReuseKey {
zsh_path: Option<PathBuf>,
features: ManagedFeatures,
use_experimental_unified_exec_tool: bool,
environment_ids: Vec<String>,
}
impl GuardianReviewSessionReuseKey {
@@ -221,8 +223,18 @@ impl GuardianReviewSessionReuseKey {
zsh_path: spawn_config.zsh_path.clone(),
features: spawn_config.features.clone(),
use_experimental_unified_exec_tool: spawn_config.use_experimental_unified_exec_tool,
environment_ids: Vec::new(),
}
}
fn with_environments(mut self, environments: &TurnEnvironmentSnapshot) -> Self {
self.environment_ids = environments
.captured_environments()
.into_keys()
.collect::<Vec<_>>();
self.environment_ids.sort_unstable();
self
}
}
fn encrypted_parent_compaction<'a, I>(items: I) -> Option<ResponseItem>
@@ -359,16 +371,18 @@ impl GuardianReviewSessionManager {
.enabled(Feature::GuardianReuseParentCompaction)
.then(|| encrypted_parent_compaction(parent_history.raw_items()))
.flatten();
let parent_context = GuardianReviewContext::from(parent_turn);
let reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
&spawn_config,
parent_session.user_instructions().await,
parent_history.history_version(),
);
)
.with_environments(parent_context.environments());
let spawn_cancel_token = self.cancellation_token.child_token();
let spawn_cancel_guard = spawn_cancel_token.clone().drop_guard();
let review_session = spawn_guardian_review_session(
&parent_session,
&GuardianReviewContext::from(parent_turn),
&parent_context,
spawn_config,
reuse_key,
spawn_cancel_token.clone(),
@@ -439,8 +453,8 @@ impl GuardianReviewSessionManager {
&params.spawn_config,
params.parent_session.user_instructions().await,
parent_history.history_version(),
);
let mut stale_trunk_to_shutdown = None;
)
.with_environments(params.parent_context.environments());
let mut spawned_trunk = false;
let trunk_candidate = match run_before_review_deadline(
deadline,
@@ -460,8 +474,9 @@ impl GuardianReviewSessionManager {
if let Some(trunk) = state.trunk.as_ref()
&& trunk.reuse_key != next_reuse_key
&& trunk.review_lock.try_acquire().is_ok()
&& let Some(stale_trunk) = state.trunk.take()
{
stale_trunk_to_shutdown = state.trunk.take();
stale_trunk.shutdown_in_background();
}
if state.trunk.is_none() {
@@ -504,10 +519,6 @@ impl GuardianReviewSessionManager {
}
};
if let Some(review_session) = stale_trunk_to_shutdown {
review_session.shutdown_in_background();
}
let Some(trunk) = trunk_candidate else {
return (
GuardianReviewSessionOutcome::Completed(Err(anyhow!(
@@ -880,7 +891,7 @@ async fn run_review_on_session(
.total_token_usage()
.await
.unwrap_or_default();
let guardian_permission_profile = PermissionProfile::read_only();
let guardian_permission_profile = params.spawn_config.permissions.permission_profile().clone();
let parent_turn_environments = params.parent_context.environments().to_selections();
// TODO(anp): Migrate guardian review thread settings to a PathUri fallback cwd so foreign
// parent environments do not fall back to the host-native config cwd.
@@ -1127,9 +1138,16 @@ pub(crate) fn build_guardian_review_session_config(
guardian_config.notify = None;
guardian_config.developer_instructions = None;
guardian_config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never);
let guardian_permission_profile = parent_config
.permissions
.permission_profile()
.intersect_with_read_only()
.unwrap_or(PermissionProfile::External {
network: codex_protocol::permissions::NetworkSandboxPolicy::Restricted,
});
guardian_config
.permissions
.set_permission_profile(PermissionProfile::read_only())
.set_permission_profile(guardian_permission_profile)
.map_err(|err| {
anyhow::anyhow!("guardian review session could not set permission profile: {err}")
})?;
@@ -1931,7 +1949,8 @@ mod tests {
.clone_history()
.await
.history_version(),
);
)
.with_environments(params.parent_context.environments());
let manager = Arc::new(GuardianReviewSessionManager {
state: Arc::new(Mutex::new(GuardianReviewSessionState {
trunk: Some(Arc::new(review_session)),
codex-rs/core/src/tools/spec_plan.rs
+9 -1
View File
@@ -70,6 +70,7 @@ use codex_protocol::dynamic_tools::DynamicToolNamespaceTool;
use codex_protocol::dynamic_tools::DynamicToolSpec;
use codex_protocol::error::CodexErrorDetails;
use codex_protocol::error::Result as CodexResult;
use codex_protocol::models::PermissionProfile;
use codex_protocol::openai_models::ConfigShellToolType;
use codex_protocol::openai_models::InputModality;
use codex_protocol::openai_models::ToolMode;
@@ -890,9 +891,16 @@ fn code_mode_namespace_descriptions(
#[instrument(level = "trace", skip_all)]
fn add_core_tool_sources(context: &CoreToolPlanContext<'_>, registry: &mut ToolRegistry) {
// Guardian reviewers receive only `exec_command`, `write_stdin`, and `view_image`
// when an environment is available; all general tool sources stay excluded.
// when a managed sandbox can enforce the parent's filesystem restrictions;
// all general tool sources stay excluded.
if crate::guardian::is_guardian_reviewer_source(&context.turn_context.session_source) {
let turn_context = context.turn_context;
if !matches!(
turn_context.permission_profile(),
PermissionProfile::Managed { .. }
) {
return;
}
let environment_mode = tool_environment_mode(context.environments);
if environment_mode.has_environment() {
let include_environment_id = matches!(environment_mode, ToolEnvironmentMode::Multiple);
codex-rs/core/tests/suite/guardian_review.rs
+73 -87
View File
@@ -19,8 +19,14 @@ use codex_history::RolloutLine;
use codex_login::CodexAuth;
use codex_protocol::ThreadId;
use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::models::PermissionProfile;
use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER;
use codex_protocol::openai_models::ModelsResponse;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::Op;
@@ -33,7 +39,6 @@ use core_test_support::responses::assert_parent_turn;
use core_test_support::responses::assert_root_turn;
use core_test_support::responses::ev_assistant_message;
use core_test_support::responses::ev_completed;
use core_test_support::responses::ev_custom_tool_call;
use core_test_support::responses::ev_function_call;
use core_test_support::responses::ev_response_created;
use core_test_support::responses::mount_response_once_match;
@@ -353,25 +358,44 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
"Guardian approval actions require host-native paths"
);
const SECRET: &str = "guardian-parent-policy-test-secret";
let server = start_mock_server().await;
let approval_policy = AskForApproval::OnRequest;
let sandbox_policy = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
network_access: false,
exclude_tmpdir_env_var: true,
exclude_slash_tmp: true,
};
let sandbox_policy_for_config = sandbox_policy.clone();
let mut builder = test_codex().with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config
.set_legacy_sandbox_policy(sandbox_policy_for_config)
.expect("set sandbox policy");
});
let mut builder = test_codex()
.with_config(move |config| {
let secret_file = config.cwd.join("guardian-secret.txt");
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
let mut file_system_policy = FileSystemSandboxPolicy::workspace_write(
&[],
/*exclude_tmpdir_env_var*/ true,
/*exclude_slash_tmp*/ true,
);
file_system_policy.entries.push(FileSystemSandboxEntry::new(
FileSystemPath::Path { path: secret_file },
FileSystemAccessMode::Deny,
));
config
.permissions
.set_permission_profile(PermissionProfile::from_runtime_permissions(
&file_system_policy,
NetworkSandboxPolicy::Restricted,
))
.expect("set parent permission profile");
})
.with_workspace_setup(|cwd, fs| async move {
fs.write_file(
&codex_utils_path_uri::PathUri::from_abs_path(&cwd.join("guardian-secret.txt")),
SECRET.as_bytes().to_vec(),
/*sandbox*/ None,
)
.await?;
Ok(())
});
let test = builder.build_with_auto_env(&server).await?;
let secret_file = test.config.cwd.join("guardian-secret.txt");
let guardian_output_file = test.cwd.path().join("guardian-write.txt");
let first_output_file = test.cwd.path().join("guardian-first.txt");
let second_output_file = test.cwd.path().join("guardian-second.txt");
let first_command = format!("printf first > {}", first_output_file.display());
@@ -388,6 +412,14 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
"sandbox_permissions": SandboxPermissions::RequireEscalated,
"justification": "Exercise the second Guardian approval.",
});
let guardian_tool_args = json!({
"cmd": format!(
"cat {}; printf hostile > {}",
secret_file.display(),
guardian_output_file.display()
),
"sandbox_permissions": SandboxPermissions::UseDefault,
});
let responses = mount_sse_sequence(
&server,
vec![
@@ -402,6 +434,15 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
]),
sse(vec![
ev_response_created("resp-guardian-first-review"),
ev_function_call(
"exec-guardian-denied-read",
"exec_command",
&serde_json::to_string(&guardian_tool_args)?,
),
ev_completed("resp-guardian-first-review"),
]),
sse(vec![
ev_response_created("resp-guardian-first-assessment"),
ev_assistant_message(
"msg-guardian-first-review",
&json!({
@@ -412,7 +453,7 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
})
.to_string(),
),
ev_completed("resp-guardian-first-review"),
ev_completed("resp-guardian-first-assessment"),
]),
sse(vec![
ev_response_created("resp-parent-second-tool"),
@@ -456,7 +497,6 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
environments: Some(local_selections(test.config.cwd.clone())),
approval_policy: Some(approval_policy),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
sandbox_policy: Some(sandbox_policy),
..Default::default()
}),
)
@@ -465,25 +505,22 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
matches!(event, EventMsg::TurnComplete(_))
})
.await;
let guardian_requests = responses
.requests()
.into_iter()
let requests = responses.requests();
assert!(
requests
.iter()
.all(|request| !request.body_contains_text(SECRET)),
"Guardian disclosed a file denied by the parent task"
);
let guardian_requests = requests
.iter()
.filter(|request| {
request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian")
})
.collect::<Vec<_>>();
assert_eq!(guardian_requests.len(), 2);
assert_eq!(guardian_requests.len(), 3);
let first_guardian_request = guardian_requests[0].body_json();
let second_guardian_request = guardian_requests[1].body_json();
let parent_request = responses.requests()[0].body_json();
let parent_turn_id = parent_request["client_metadata"]["turn_id"]
.as_str()
.expect("reviewed parent turn id");
for request in [&first_guardian_request, &second_guardian_request] {
assert_parent_turn(request, Some(parent_turn_id))?;
assert_root_turn(request, Some(parent_turn_id))?;
}
let second_guardian_request = guardian_requests[2].body_json();
let first_guardian_thread_id = first_guardian_request["client_metadata"]["thread_id"]
.as_str()
.expect("first Guardian review should have a thread id");
@@ -491,67 +528,16 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
.as_str()
.expect("second Guardian review should have a thread id");
assert_eq!(first_guardian_thread_id, second_guardian_thread_id);
assert!(
!guardian_output_file.exists(),
"Guardian wrote a local file"
);
assert_eq!(fs::read_to_string(first_output_file)?, "first");
assert_eq!(fs::read_to_string(second_output_file)?, "second");
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_review_of_code_mode_exec_command_includes_outer_source() -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
skip_if_wine_exec!(
Ok(()),
"Guardian approval actions require host-native paths"
);
let server = start_mock_server().await;
let mut builder = test_codex()
.with_model("test-gpt-5.1-codex")
.with_config(|config| {
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
let _ = config.features.enable(Feature::CodeMode);
});
let test = builder.build_with_auto_env(&server).await?;
let code = r#"const outerContext = "guardian-code-mode-outer-context";
await tools.exec_command({cmd: "true", sandbox_permissions: "require_escalated", justification: "test"});"#;
let responses = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_custom_tool_call("code-mode-call", "exec", code),
ev_completed("resp-parent"),
]),
sse(vec![
ev_assistant_message("guardian", r#"{"outcome":"allow"}"#),
ev_completed("resp-guardian"),
]),
sse(vec![ev_completed("resp-done")]),
],
)
.await;
let prompt = "run a nested command that requires Guardian review";
test.submit_text_turn(prompt).await?;
let transcript = responses.requests()[1].message_input_texts("user")[2..6].concat();
assert_eq!(
transcript,
format!(
">>> TRANSCRIPT START\n\
[1] user: {prompt}\n\
\n\
[2] tool exec call: {code}\n\
>>> TRANSCRIPT END\n"
)
);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn interrupted_guardian_tool_review_aborts_without_executing_the_command() -> Result<()> {
skip_if_no_network!(Ok(()));
codex-rs/protocol/src/models.rs
+27
View File
@@ -387,6 +387,33 @@ impl PermissionProfile {
}
}
/// Intersects managed filesystem permissions with read-only access and restricts network.
///
/// Returns `None` when filesystem enforcement belongs to an external caller.
pub fn intersect_with_read_only(&self) -> Option<Self> {
let mut file_system = self.file_system_sandbox_policy();
match file_system.kind {
FileSystemSandboxKind::Restricted => {
for entry in &mut file_system.entries {
entry.access = match entry.access {
FileSystemAccessMode::Read | FileSystemAccessMode::Write => {
FileSystemAccessMode::Read
}
FileSystemAccessMode::Deny => FileSystemAccessMode::Deny,
};
}
}
FileSystemSandboxKind::Unrestricted => {
file_system = FileSystemSandboxPolicy::read_only();
}
FileSystemSandboxKind::ExternalSandbox => return None,
}
Some(Self::from_runtime_permissions(
&file_system,
NetworkSandboxPolicy::Restricted,
))
}
/// Managed workspace-write filesystem access with restricted network
/// access.
///