mirror of
https://github.com/openai/codex.git
synced 2026-11-08 20:40:33 +00:00
Use MCP owner permissions for Guardian reviews and score reuse (#59489)
## Why An MCP server can belong to a different environment from the turn's primary environment. An early Guardian score based on the issuing context must not bypass review when the server owner's permissions differ. ## What changed - Capture the owning environment's permissions when preparing an MCP call and retain them for server-initiated approvals tied to that still-running call. - Use the captured permissions consistently in Guardian review prompts and permission context. - Require a fresh synchronous review when the captured owner permissions do not match the early score's authorization. ## Testing Extend regression coverage for MCP score reuse across environments and for follow-up approvals from a local MCP server during a remote-primary turn. Assert that reviews use the owner's denied paths and require a fresh review when permissions differ. GitOrigin-RevId: eb5eeb1259732c57f39765c38e34148fa0787dae
This commit is contained in:
@@ -129,6 +129,19 @@ pub(crate) fn decide_approval(
|
||||
&assessment.authorization.permissions,
|
||||
)
|
||||
.await;
|
||||
// MCP prepares the call after the response is scored. The score must
|
||||
// also match the rules of the server that will handle the call.
|
||||
if !require_synchronous_review
|
||||
&& let Ok(GuardianApprovalRequest::McpToolCall {
|
||||
prompt_permissions: Some((environment_id, sandbox)),
|
||||
..
|
||||
}) = &request.request
|
||||
{
|
||||
let owner_permissions =
|
||||
super::permissions::for_mcp_environment(environment_id, sandbox).ok();
|
||||
require_synchronous_review =
|
||||
owner_permissions.as_ref() != Some(&assessment.authorization.permissions);
|
||||
}
|
||||
}
|
||||
let retried = reasons.retry.is_some();
|
||||
let decision = codex_guardian_reviewer::ReviewRequest {
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
//! Fresh reviews of environment-owned MCP calls use the prepared environment's rules.
|
||||
//! Other reviews and early scoring use the issuing context.
|
||||
//! Guardian reviews and score reuse use the prepared MCP owner's rules when available.
|
||||
//! Early scoring uses the issuing context, before the MCP call is prepared.
|
||||
|
||||
use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID;
|
||||
use codex_file_system::FileSystemSandboxContext;
|
||||
@@ -7,6 +7,7 @@ use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicyContext;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
||||
use super::GuardianApprovalRequest;
|
||||
use super::GuardianReviewContext;
|
||||
use crate::context::GuardianPermissionContext;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
@@ -25,6 +26,21 @@ pub(super) fn check_environment_id(id: Option<&str>) -> anyhow::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(super) fn for_request(
|
||||
request: &GuardianApprovalRequest,
|
||||
context: Option<&GuardianReviewContext>,
|
||||
) -> anyhow::Result<Option<GuardianPermissionContext>> {
|
||||
match request {
|
||||
GuardianApprovalRequest::McpToolCall {
|
||||
prompt_permissions: Some((environment_id, sandbox)),
|
||||
..
|
||||
} => for_mcp_environment(environment_id, sandbox).map(Some),
|
||||
_ => context
|
||||
.map(|context| for_environment(context, request.target_environment_id()))
|
||||
.transpose(),
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn for_mcp_environment(
|
||||
environment_id: &str,
|
||||
sandbox: &FileSystemSandboxContext,
|
||||
|
||||
@@ -122,20 +122,7 @@ pub(super) async fn collect_guardian_prompt_items_with_parent_turn(
|
||||
&request,
|
||||
reasons.retry.or(reasons.approval),
|
||||
)?;
|
||||
let permissions = match request {
|
||||
GuardianApprovalRequest::McpToolCall {
|
||||
prompt_permissions: Some((environment_id, sandbox)),
|
||||
..
|
||||
} => Some(super::permissions::for_mcp_environment(
|
||||
&environment_id,
|
||||
&sandbox,
|
||||
)?),
|
||||
_ => parent_context
|
||||
.map(|context| {
|
||||
super::permissions::for_environment(context, request.target_environment_id())
|
||||
})
|
||||
.transpose()?,
|
||||
};
|
||||
let permissions = super::permissions::for_request(&request, parent_context)?;
|
||||
let node_repl_snapshot = if node_repl_transcripts_enabled {
|
||||
session
|
||||
.services
|
||||
|
||||
@@ -100,11 +100,9 @@ impl ReviewHost for super::super::runtime::ReviewRuntime {
|
||||
|
||||
fn permissions(&self) -> Option<codex_guardian_context::PermissionContext> {
|
||||
let request = self.request.request.as_ref().ok()?;
|
||||
crate::guardian::permissions::for_environment(
|
||||
&self.context,
|
||||
request.target_environment_id(),
|
||||
)
|
||||
.ok()
|
||||
crate::guardian::permissions::for_request(request, Some(&self.context))
|
||||
.ok()
|
||||
.flatten()
|
||||
}
|
||||
|
||||
async fn servicing_turn(
|
||||
|
||||
@@ -37,6 +37,7 @@ use codex_connectors::AppToolPolicyEvaluator;
|
||||
use codex_connectors::AppToolPolicyInput;
|
||||
use codex_extension_api::McpToolContext;
|
||||
use codex_features::Feature;
|
||||
use codex_file_system::FileSystemSandboxContext;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY;
|
||||
use codex_mcp::McpPermissionPromptAutoApproveContext;
|
||||
@@ -257,6 +258,7 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
&invocation,
|
||||
metadata.clone(),
|
||||
GuardianReviewContext::from(step_context),
|
||||
capture_environment_permissions(prepared_call.permission_source()),
|
||||
);
|
||||
notify_mcp_tool_call_started(
|
||||
sess.as_ref(),
|
||||
@@ -1312,6 +1314,20 @@ pub(crate) struct McpToolApprovalContext {
|
||||
pub(crate) invocation: Option<McpInvocation>,
|
||||
pub(crate) metadata: McpToolApprovalMetadata,
|
||||
pub(crate) review_context: GuardianReviewContext,
|
||||
/// Rules captured when MCP prepared this call, if the server belongs to an environment.
|
||||
pub(crate) environment_permissions: Option<(String, Arc<FileSystemSandboxContext>)>,
|
||||
}
|
||||
|
||||
fn capture_environment_permissions(
|
||||
source: McpPermissionSource<'_>,
|
||||
) -> Option<(String, Arc<FileSystemSandboxContext>)> {
|
||||
match source {
|
||||
McpPermissionSource::Environment {
|
||||
environment_id,
|
||||
sandbox,
|
||||
} => Some((environment_id.to_owned(), Arc::clone(sandbox))),
|
||||
McpPermissionSource::Thread { .. } => None,
|
||||
}
|
||||
}
|
||||
|
||||
impl Session {
|
||||
@@ -1321,6 +1337,7 @@ impl Session {
|
||||
invocation: &McpInvocation,
|
||||
metadata: McpToolApprovalMetadata,
|
||||
review_context: GuardianReviewContext,
|
||||
environment_permissions: Option<(String, Arc<FileSystemSandboxContext>)>,
|
||||
) -> Arc<McpToolApprovalContext> {
|
||||
let key = (invocation.server.clone(), call_id.to_string());
|
||||
let metadata = Arc::new(McpToolApprovalContext {
|
||||
@@ -1329,6 +1346,7 @@ impl Session {
|
||||
.then(|| invocation.clone()),
|
||||
metadata,
|
||||
review_context,
|
||||
environment_permissions,
|
||||
});
|
||||
let mut registry = self
|
||||
.mcp_tool_approval_metadata
|
||||
@@ -1695,13 +1713,7 @@ async fn prepare_mcp_tool_approval(
|
||||
approval_mode: policy.mode,
|
||||
allow_session_remember: session_approval_key.is_some(),
|
||||
allow_persistent_approval: persistent_approval_key.is_some(),
|
||||
prompt_permissions: match permission_source {
|
||||
McpPermissionSource::Environment {
|
||||
environment_id,
|
||||
sandbox,
|
||||
} => Some((environment_id.to_owned(), Arc::clone(sandbox))),
|
||||
McpPermissionSource::Thread { .. } => None,
|
||||
},
|
||||
prompt_permissions: capture_environment_permissions(permission_source),
|
||||
};
|
||||
McpToolApprovalRequirement::Required {
|
||||
action: Box::new(action),
|
||||
|
||||
@@ -3723,6 +3723,7 @@ async fn approval_metadata_is_released_when_the_invocation_future_is_dropped() {
|
||||
&invocation,
|
||||
metadata.clone(),
|
||||
review_context.clone(),
|
||||
/*environment_permissions*/ None,
|
||||
);
|
||||
std::future::pending::<()>().await;
|
||||
});
|
||||
@@ -3732,6 +3733,7 @@ async fn approval_metadata_is_released_when_the_invocation_future_is_dropped() {
|
||||
&invocation,
|
||||
metadata.clone(),
|
||||
review_context.clone(),
|
||||
/*environment_permissions*/ None,
|
||||
);
|
||||
assert_eq!(
|
||||
session
|
||||
@@ -3755,6 +3757,7 @@ async fn approval_metadata_is_released_when_the_invocation_future_is_dropped() {
|
||||
&invocation,
|
||||
metadata,
|
||||
review_context,
|
||||
/*environment_permissions*/ None,
|
||||
);
|
||||
let registry = session.mcp_tool_approval_metadata.lock().unwrap();
|
||||
let mut keys = registry.keys().cloned().collect::<Vec<_>>();
|
||||
|
||||
@@ -1187,6 +1187,15 @@ async fn review_guardian_mcp_elicitation(
|
||||
}
|
||||
};
|
||||
guardian_request.category = guardian_scope;
|
||||
// Keep the action the server asked about, but use its owner's rules from the original call.
|
||||
if let Ok(crate::guardian::GuardianApprovalRequest::McpToolCall {
|
||||
prompt_permissions, ..
|
||||
}) = &mut guardian_request.request
|
||||
&& let Some(permissions) =
|
||||
approval_context.and_then(|context| context.environment_permissions)
|
||||
{
|
||||
*prompt_permissions = Some(permissions);
|
||||
}
|
||||
let declined_reason = guardian_request.request.as_ref().err().cloned();
|
||||
let decision = crate::guardian::decide_approval(
|
||||
session,
|
||||
|
||||
Reference in New Issue
Block a user