mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat: add Reject approval policy with granular prompt rejection controls (#12087)
## Why
We need a way to auto-reject specific approval prompt categories without
switching all approvals off.
The goal is to let users independently control:
- sandbox escalation approvals,
- execpolicy `prompt` rule approvals,
- MCP elicitation prompts.
## What changed
- Added a new primary approval mode in `protocol/src/protocol.rs`:
```rust
pub enum AskForApproval {
// ...
Reject(RejectConfig),
// ...
}
pub struct RejectConfig {
pub sandbox_approval: bool,
pub rules: bool,
pub mcp_elicitations: bool,
}
```
- Wired `RejectConfig` semantics through approval paths in `core`:
- `core/src/exec_policy.rs`
- rejects rule-driven prompts when `rules = true`
- rejects sandbox/escalation prompts when `sandbox_approval = true`
- preserves rule priority when both rule and sandbox prompt conditions
are present
- `core/src/tools/sandboxing.rs`
- applies `sandbox_approval` to default exec approval decisions and
sandbox-failure retry gating
- `core/src/safety.rs`
- keeps `Reject { all false }` behavior aligned with `OnRequest` for
patch safety
- rejects out-of-root patch approvals when `sandbox_approval = true`
- `core/src/mcp_connection_manager.rs`
- auto-declines MCP elicitations when `mcp_elicitations = true`
- Ensured approval policy used by MCP elicitation flow stays in sync
with constrained session policy updates.
- Updated app-server v2 conversions and generated schema/TypeScript
artifacts for the new `Reject` shape.
## Verification
Added focused unit coverage for the new behavior in:
- `core/src/exec_policy.rs`
- `core/src/tools/sandboxing.rs`
- `core/src/mcp_connection_manager.rs`
- `core/src/safety.rs`
- `core/src/tools/runtimes/apply_patch.rs`
Key cases covered include rule-vs-sandbox prompt precedence, MCP
auto-decline behavior, and patch/sandbox retry behavior under
`RejectConfig`.
This commit is contained in:
committed by
GitHub
Unverified
parent
f6c06108b1
commit
425fff7ad6
@@ -1288,9 +1288,9 @@ impl Session {
|
||||
// before any MCP-related events. It is reasonable to consider
|
||||
// changing this to use Option or OnceCell, though the current
|
||||
// setup is straightforward enough and performs well.
|
||||
mcp_connection_manager: Arc::new(
|
||||
RwLock::new(McpConnectionManager::new_uninitialized()),
|
||||
),
|
||||
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::new_uninitialized(
|
||||
&config.permissions.approval_policy,
|
||||
))),
|
||||
mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()),
|
||||
unified_exec_manager: UnifiedExecProcessManager::new(
|
||||
config.background_terminal_max_timeout,
|
||||
@@ -1417,6 +1417,7 @@ impl Session {
|
||||
&mcp_servers,
|
||||
config.mcp_oauth_credentials_store_mode,
|
||||
auth_statuses.clone(),
|
||||
&session_configuration.approval_policy,
|
||||
tx_event.clone(),
|
||||
sandbox_state,
|
||||
)
|
||||
@@ -1872,6 +1873,11 @@ impl Session {
|
||||
sandbox_policy_changed: bool,
|
||||
) -> Arc<TurnContext> {
|
||||
let per_turn_config = Self::build_per_turn_config(&session_configuration);
|
||||
self.services
|
||||
.mcp_connection_manager
|
||||
.read()
|
||||
.await
|
||||
.set_approval_policy(&session_configuration.approval_policy);
|
||||
|
||||
if sandbox_policy_changed {
|
||||
let sandbox_state = SandboxState {
|
||||
@@ -3059,6 +3065,7 @@ impl Session {
|
||||
&mcp_servers,
|
||||
store_mode,
|
||||
auth_statuses,
|
||||
&turn_context.config.permissions.approval_policy,
|
||||
self.get_tx_event(),
|
||||
sandbox_state,
|
||||
)
|
||||
@@ -7383,7 +7390,9 @@ mod tests {
|
||||
let file_watcher = Arc::new(FileWatcher::noop());
|
||||
let services = SessionServices {
|
||||
mcp_connection_manager: Arc::new(RwLock::new(
|
||||
McpConnectionManager::new_mcp_connection_manager_for_tests(),
|
||||
McpConnectionManager::new_mcp_connection_manager_for_tests(
|
||||
&config.permissions.approval_policy,
|
||||
),
|
||||
)),
|
||||
mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()),
|
||||
unified_exec_manager: UnifiedExecProcessManager::new(
|
||||
@@ -7537,7 +7546,9 @@ mod tests {
|
||||
let file_watcher = Arc::new(FileWatcher::noop());
|
||||
let services = SessionServices {
|
||||
mcp_connection_manager: Arc::new(RwLock::new(
|
||||
McpConnectionManager::new_mcp_connection_manager_for_tests(),
|
||||
McpConnectionManager::new_mcp_connection_manager_for_tests(
|
||||
&config.permissions.approval_policy,
|
||||
),
|
||||
)),
|
||||
mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()),
|
||||
unified_exec_manager: UnifiedExecProcessManager::new(
|
||||
|
||||
@@ -104,6 +104,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options(
|
||||
&mcp_servers,
|
||||
config.mcp_oauth_credentials_store_mode,
|
||||
auth_status_entries,
|
||||
&config.permissions.approval_policy,
|
||||
tx_event,
|
||||
sandbox_state,
|
||||
)
|
||||
|
||||
@@ -32,6 +32,10 @@ use shlex::try_join as shlex_try_join;
|
||||
|
||||
const PROMPT_CONFLICT_REASON: &str =
|
||||
"approval required by policy, but AskForApproval is set to Never";
|
||||
const REJECT_SANDBOX_APPROVAL_REASON: &str =
|
||||
"approval required by policy, but AskForApproval::Reject.sandbox_approval is set";
|
||||
const REJECT_RULES_APPROVAL_REASON: &str =
|
||||
"approval required by policy rule, but AskForApproval::Reject.rules is set";
|
||||
const RULES_DIR_NAME: &str = "rules";
|
||||
const RULE_EXTENSION: &str = "rules";
|
||||
const DEFAULT_POLICY_FILE: &str = "default.rules";
|
||||
@@ -91,6 +95,37 @@ fn is_policy_match(rule_match: &RuleMatch) -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns a rejection reason when `approval_policy` disallows surfacing the
|
||||
/// current prompt to the user.
|
||||
///
|
||||
/// `prompt_is_rule` distinguishes policy-rule prompts from sandbox/escalation
|
||||
/// prompts so `Reject.rules` and `Reject.sandbox_approval` are honored
|
||||
/// independently. When both are present, policy-rule prompts take precedence.
|
||||
fn prompt_is_rejected_by_policy(
|
||||
approval_policy: AskForApproval,
|
||||
prompt_is_rule: bool,
|
||||
) -> Option<&'static str> {
|
||||
match approval_policy {
|
||||
AskForApproval::Never => Some(PROMPT_CONFLICT_REASON),
|
||||
AskForApproval::OnFailure => None,
|
||||
AskForApproval::OnRequest => None,
|
||||
AskForApproval::UnlessTrusted => None,
|
||||
AskForApproval::Reject(reject_config) => {
|
||||
if prompt_is_rule {
|
||||
if reject_config.rejects_rules_approval() {
|
||||
Some(REJECT_RULES_APPROVAL_REASON)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
} else if reject_config.rejects_sandbox_approval() {
|
||||
Some(REJECT_SANDBOX_APPROVAL_REASON)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Error)]
|
||||
pub enum ExecPolicyError {
|
||||
#[error("failed to read rules files from {dir}: {source}")]
|
||||
@@ -196,12 +231,14 @@ impl ExecPolicyManager {
|
||||
reason: derive_forbidden_reason(command, &evaluation),
|
||||
},
|
||||
Decision::Prompt => {
|
||||
if matches!(approval_policy, AskForApproval::Never) {
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: PROMPT_CONFLICT_REASON.to_string(),
|
||||
}
|
||||
} else {
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
let prompt_is_rule = evaluation.matched_rules.iter().any(|rule_match| {
|
||||
is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt
|
||||
});
|
||||
match prompt_is_rejected_by_policy(approval_policy, prompt_is_rule) {
|
||||
Some(reason) => ExecApprovalRequirement::Forbidden {
|
||||
reason: reason.to_string(),
|
||||
},
|
||||
None => ExecApprovalRequirement::NeedsApproval {
|
||||
reason: derive_prompt_reason(command, &evaluation),
|
||||
proposed_execpolicy_amendment: requested_amendment.or_else(|| {
|
||||
if auto_amendment_allowed {
|
||||
@@ -212,7 +249,7 @@ impl ExecPolicyManager {
|
||||
None
|
||||
}
|
||||
}),
|
||||
}
|
||||
},
|
||||
}
|
||||
}
|
||||
Decision::Allow => ExecApprovalRequirement::Skip {
|
||||
@@ -465,6 +502,20 @@ pub fn render_decision_for_unmatched_command(
|
||||
}
|
||||
}
|
||||
}
|
||||
AskForApproval::Reject(_) => match sandbox_policy {
|
||||
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => {
|
||||
// Mirror on-request behavior for unmatched commands; prompt-vs-reject is handled
|
||||
// by `prompt_is_rejected_by_policy`.
|
||||
Decision::Allow
|
||||
}
|
||||
SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. } => {
|
||||
if sandbox_permissions.requires_escalated_permissions() {
|
||||
Decision::Prompt
|
||||
} else {
|
||||
Decision::Allow
|
||||
}
|
||||
}
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -690,6 +741,7 @@ mod tests {
|
||||
use crate::config_loader::ConfigRequirementsToml;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::RejectConfig;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -1197,6 +1249,122 @@ prefix_rule(
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unmatched_reject_policy_still_prompts_for_restricted_sandbox_escalation() {
|
||||
let command = vec!["madeup-cmd".to_string()];
|
||||
|
||||
assert_eq!(
|
||||
Decision::Prompt,
|
||||
render_decision_for_unmatched_command(
|
||||
AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
&command,
|
||||
SandboxPermissions::RequireEscalated,
|
||||
false,
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exec_approval_requirement_rejects_unmatched_prompt_when_sandbox_rejection_enabled() {
|
||||
let command = vec!["madeup-cmd".to_string()];
|
||||
|
||||
let requirement = ExecPolicyManager::default()
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
prefix_rule: None,
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: REJECT_SANDBOX_APPROVAL_REASON.to_string(),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn mixed_rule_and_sandbox_prompt_prioritizes_rule_for_rejection_decision() {
|
||||
let policy_src = r#"prefix_rule(pattern=["git"], decision="prompt")"#;
|
||||
let mut parser = PolicyParser::new();
|
||||
parser
|
||||
.parse("test.rules", policy_src)
|
||||
.expect("parse policy");
|
||||
let manager = ExecPolicyManager::new(Arc::new(parser.build()));
|
||||
let command = vec![
|
||||
"bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"git status && madeup-cmd".to_string(),
|
||||
];
|
||||
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
prefix_rule: None,
|
||||
})
|
||||
.await;
|
||||
|
||||
assert!(matches!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval { .. }
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn mixed_rule_and_sandbox_prompt_rejects_when_rules_rejection_enabled() {
|
||||
let policy_src = r#"prefix_rule(pattern=["git"], decision="prompt")"#;
|
||||
let mut parser = PolicyParser::new();
|
||||
parser
|
||||
.parse("test.rules", policy_src)
|
||||
.expect("parse policy");
|
||||
let manager = ExecPolicyManager::new(Arc::new(parser.build()));
|
||||
let command = vec![
|
||||
"bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"git status && madeup-cmd".to_string(),
|
||||
];
|
||||
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: true,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
sandbox_policy: &SandboxPolicy::new_read_only_policy(),
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
prefix_rule: None,
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: REJECT_RULES_APPROVAL_REASON.to_string(),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exec_approval_requirement_falls_back_to_heuristics() {
|
||||
let command = vec!["cargo".to_string(), "build".to_string()];
|
||||
|
||||
@@ -205,6 +205,7 @@ pub async fn collect_mcp_snapshot(config: &Config) -> McpListToolsResponseEvent
|
||||
&mcp_servers,
|
||||
config.mcp_oauth_credentials_store_mode,
|
||||
auth_status_entries.clone(),
|
||||
&config.permissions.approval_policy,
|
||||
tx_event,
|
||||
sandbox_state,
|
||||
)
|
||||
|
||||
@@ -25,9 +25,11 @@ use anyhow::anyhow;
|
||||
use async_channel::Sender;
|
||||
use codex_async_utils::CancelErr;
|
||||
use codex_async_utils::OrCancelExt;
|
||||
use codex_config::Constrained;
|
||||
use codex_protocol::approvals::ElicitationRequestEvent;
|
||||
use codex_protocol::mcp::CallToolResult;
|
||||
use codex_protocol::mcp::RequestId as ProtocolRequestId;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::Event;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::McpStartupCompleteEvent;
|
||||
@@ -44,6 +46,7 @@ use futures::future::FutureExt;
|
||||
use futures::future::Shared;
|
||||
use rmcp::model::ClientCapabilities;
|
||||
use rmcp::model::CreateElicitationRequestParams;
|
||||
use rmcp::model::ElicitationAction;
|
||||
use rmcp::model::ElicitationCapability;
|
||||
use rmcp::model::FormElicitationCapability;
|
||||
use rmcp::model::Implementation;
|
||||
@@ -182,12 +185,30 @@ static CODEX_APPS_TOOLS_CACHE: LazyLock<StdMutex<Option<CachedCodexAppsTools>>>
|
||||
|
||||
type ResponderMap = HashMap<(String, RequestId), oneshot::Sender<ElicitationResponse>>;
|
||||
|
||||
#[derive(Clone, Default)]
|
||||
fn elicitation_is_rejected_by_policy(approval_policy: AskForApproval) -> bool {
|
||||
match approval_policy {
|
||||
AskForApproval::Never => true,
|
||||
AskForApproval::OnFailure => false,
|
||||
AskForApproval::OnRequest => false,
|
||||
AskForApproval::UnlessTrusted => false,
|
||||
AskForApproval::Reject(reject_config) => reject_config.rejects_mcp_elicitations(),
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
struct ElicitationRequestManager {
|
||||
requests: Arc<Mutex<ResponderMap>>,
|
||||
approval_policy: Arc<StdMutex<AskForApproval>>,
|
||||
}
|
||||
|
||||
impl ElicitationRequestManager {
|
||||
fn new(approval_policy: AskForApproval) -> Self {
|
||||
Self {
|
||||
requests: Arc::new(Mutex::new(HashMap::new())),
|
||||
approval_policy: Arc::new(StdMutex::new(approval_policy)),
|
||||
}
|
||||
}
|
||||
|
||||
async fn resolve(
|
||||
&self,
|
||||
server_name: String,
|
||||
@@ -205,11 +226,23 @@ impl ElicitationRequestManager {
|
||||
|
||||
fn make_sender(&self, server_name: String, tx_event: Sender<Event>) -> SendElicitation {
|
||||
let elicitation_requests = self.requests.clone();
|
||||
let approval_policy = self.approval_policy.clone();
|
||||
Box::new(move |id, elicitation| {
|
||||
let elicitation_requests = elicitation_requests.clone();
|
||||
let tx_event = tx_event.clone();
|
||||
let server_name = server_name.clone();
|
||||
let approval_policy = approval_policy.clone();
|
||||
async move {
|
||||
if approval_policy
|
||||
.lock()
|
||||
.is_ok_and(|policy| elicitation_is_rejected_by_policy(*policy))
|
||||
{
|
||||
return Ok(ElicitationResponse {
|
||||
action: ElicitationAction::Decline,
|
||||
content: None,
|
||||
});
|
||||
}
|
||||
|
||||
let (tx, rx) = oneshot::channel();
|
||||
{
|
||||
let mut lock = elicitation_requests.lock().await;
|
||||
@@ -346,41 +379,49 @@ pub struct SandboxState {
|
||||
}
|
||||
|
||||
/// A thin wrapper around a set of running [`RmcpClient`] instances.
|
||||
#[derive(Default)]
|
||||
pub(crate) struct McpConnectionManager {
|
||||
clients: HashMap<String, AsyncManagedClient>,
|
||||
elicitation_requests: ElicitationRequestManager,
|
||||
}
|
||||
|
||||
impl McpConnectionManager {
|
||||
pub(crate) fn new_uninitialized() -> Self {
|
||||
pub(crate) fn new_uninitialized(approval_policy: &Constrained<AskForApproval>) -> Self {
|
||||
Self {
|
||||
clients: HashMap::new(),
|
||||
elicitation_requests: ElicitationRequestManager::default(),
|
||||
elicitation_requests: ElicitationRequestManager::new(approval_policy.value()),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn new_mcp_connection_manager_for_tests() -> Self {
|
||||
Self::new_uninitialized()
|
||||
pub(crate) fn new_mcp_connection_manager_for_tests(
|
||||
approval_policy: &Constrained<AskForApproval>,
|
||||
) -> Self {
|
||||
Self::new_uninitialized(approval_policy)
|
||||
}
|
||||
|
||||
pub(crate) fn has_servers(&self) -> bool {
|
||||
!self.clients.is_empty()
|
||||
}
|
||||
|
||||
#[allow(clippy::new_ret_no_self)]
|
||||
pub fn set_approval_policy(&self, approval_policy: &Constrained<AskForApproval>) {
|
||||
if let Ok(mut policy) = self.elicitation_requests.approval_policy.lock() {
|
||||
*policy = approval_policy.value();
|
||||
}
|
||||
}
|
||||
|
||||
#[allow(clippy::new_ret_no_self, clippy::too_many_arguments)]
|
||||
pub async fn new(
|
||||
mcp_servers: &HashMap<String, McpServerConfig>,
|
||||
store_mode: OAuthCredentialsStoreMode,
|
||||
auth_entries: HashMap<String, McpAuthStatusEntry>,
|
||||
approval_policy: &Constrained<AskForApproval>,
|
||||
tx_event: Sender<Event>,
|
||||
initial_sandbox_state: SandboxState,
|
||||
) -> (Self, CancellationToken) {
|
||||
let cancel_token = CancellationToken::new();
|
||||
let mut clients = HashMap::new();
|
||||
let mut join_set = JoinSet::new();
|
||||
let elicitation_requests = ElicitationRequestManager::default();
|
||||
let elicitation_requests = ElicitationRequestManager::new(approval_policy.value());
|
||||
let mcp_servers = mcp_servers.clone();
|
||||
for (server_name, cfg) in mcp_servers.into_iter().filter(|(_, cfg)| cfg.enabled) {
|
||||
let cancel_token = cancel_token.child_token();
|
||||
@@ -1331,6 +1372,7 @@ mod mcp_init_error_display_tests {}
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_protocol::protocol::McpAuthStatus;
|
||||
use codex_protocol::protocol::RejectConfig;
|
||||
use rmcp::model::JsonObject;
|
||||
use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
@@ -1370,6 +1412,38 @@ mod tests {
|
||||
result
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn elicitation_reject_policy_defaults_to_prompting() {
|
||||
assert!(!elicitation_is_rejected_by_policy(
|
||||
AskForApproval::OnFailure
|
||||
));
|
||||
assert!(!elicitation_is_rejected_by_policy(
|
||||
AskForApproval::OnRequest
|
||||
));
|
||||
assert!(!elicitation_is_rejected_by_policy(
|
||||
AskForApproval::UnlessTrusted
|
||||
));
|
||||
assert!(!elicitation_is_rejected_by_policy(AskForApproval::Reject(
|
||||
RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
}
|
||||
)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn elicitation_reject_policy_respects_never_and_reject_config() {
|
||||
assert!(elicitation_is_rejected_by_policy(AskForApproval::Never));
|
||||
assert!(elicitation_is_rejected_by_policy(AskForApproval::Reject(
|
||||
RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
mcp_elicitations: true,
|
||||
}
|
||||
)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_qualify_tools_short_non_duplicated_names() {
|
||||
let tools = vec![
|
||||
|
||||
@@ -38,7 +38,10 @@ pub fn assess_patch_safety(
|
||||
}
|
||||
|
||||
match policy {
|
||||
AskForApproval::OnFailure | AskForApproval::Never | AskForApproval::OnRequest => {
|
||||
AskForApproval::OnFailure
|
||||
| AskForApproval::Never
|
||||
| AskForApproval::OnRequest
|
||||
| AskForApproval::Reject(_) => {
|
||||
// Continue to see if this can be auto-approved.
|
||||
}
|
||||
// TODO(ragona): I'm not sure this is actually correct? I believe in this case
|
||||
@@ -48,11 +51,17 @@ pub fn assess_patch_safety(
|
||||
}
|
||||
}
|
||||
|
||||
let rejects_sandbox_approval = matches!(policy, AskForApproval::Never)
|
||||
|| matches!(
|
||||
policy,
|
||||
AskForApproval::Reject(reject_config) if reject_config.sandbox_approval
|
||||
);
|
||||
|
||||
// Even though the patch appears to be constrained to writable paths, it is
|
||||
// possible that paths in the patch are hard links to files outside the
|
||||
// writable roots, so we should still run `apply_patch` in a sandbox in that case.
|
||||
if is_write_patch_constrained_to_writable_paths(action, sandbox_policy, cwd)
|
||||
|| policy == AskForApproval::OnFailure
|
||||
|| matches!(policy, AskForApproval::OnFailure)
|
||||
{
|
||||
if matches!(
|
||||
sandbox_policy,
|
||||
@@ -72,10 +81,20 @@ pub fn assess_patch_safety(
|
||||
sandbox_type,
|
||||
user_explicitly_approved: false,
|
||||
},
|
||||
None => SafetyCheck::AskUser,
|
||||
None => {
|
||||
if rejects_sandbox_approval {
|
||||
SafetyCheck::Reject {
|
||||
reason:
|
||||
"writing outside of the project; rejected by user approval settings"
|
||||
.to_string(),
|
||||
}
|
||||
} else {
|
||||
SafetyCheck::AskUser
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
} else if policy == AskForApproval::Never {
|
||||
} else if rejects_sandbox_approval {
|
||||
SafetyCheck::Reject {
|
||||
reason: "writing outside of the project; rejected by user approval settings"
|
||||
.to_string(),
|
||||
@@ -174,6 +193,7 @@ fn is_write_patch_constrained_to_writable_paths(
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_protocol::protocol::RejectConfig;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use tempfile::TempDir;
|
||||
|
||||
@@ -253,4 +273,79 @@ mod tests {
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn reject_with_all_flags_false_matches_on_request_for_out_of_root_patch() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let cwd = tmp.path().to_path_buf();
|
||||
let parent = cwd.parent().unwrap().to_path_buf();
|
||||
let add_outside =
|
||||
ApplyPatchAction::new_add_for_test(&parent.join("outside.txt"), "".to_string());
|
||||
let policy_workspace_only = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
read_only_access: Default::default(),
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
assess_patch_safety(
|
||||
&add_outside,
|
||||
AskForApproval::OnRequest,
|
||||
&policy_workspace_only,
|
||||
&cwd,
|
||||
WindowsSandboxLevel::Disabled,
|
||||
),
|
||||
SafetyCheck::AskUser,
|
||||
);
|
||||
assert_eq!(
|
||||
assess_patch_safety(
|
||||
&add_outside,
|
||||
AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
&policy_workspace_only,
|
||||
&cwd,
|
||||
WindowsSandboxLevel::Disabled,
|
||||
),
|
||||
SafetyCheck::AskUser,
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn reject_sandbox_approval_rejects_out_of_root_patch() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let cwd = tmp.path().to_path_buf();
|
||||
let parent = cwd.parent().unwrap().to_path_buf();
|
||||
let add_outside =
|
||||
ApplyPatchAction::new_add_for_test(&parent.join("outside.txt"), "".to_string());
|
||||
let policy_workspace_only = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
read_only_access: Default::default(),
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
assess_patch_safety(
|
||||
&add_outside,
|
||||
AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
}),
|
||||
&policy_workspace_only,
|
||||
&cwd,
|
||||
WindowsSandboxLevel::Disabled,
|
||||
),
|
||||
SafetyCheck::Reject {
|
||||
reason: "writing outside of the project; rejected by user approval settings"
|
||||
.to_string(),
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -127,7 +127,13 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
|
||||
}
|
||||
|
||||
fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool {
|
||||
!matches!(policy, AskForApproval::Never)
|
||||
match policy {
|
||||
AskForApproval::Never => false,
|
||||
AskForApproval::Reject(reject_config) => !reject_config.rejects_sandbox_approval(),
|
||||
AskForApproval::OnFailure => true,
|
||||
AskForApproval::OnRequest => true,
|
||||
AskForApproval::UnlessTrusted => true,
|
||||
}
|
||||
}
|
||||
|
||||
// apply_patch approvals are decided upstream by assess_patch_safety.
|
||||
@@ -159,3 +165,29 @@ impl ToolRuntime<ApplyPatchRequest, ExecToolCallOutput> for ApplyPatchRuntime {
|
||||
Ok(out)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_protocol::protocol::RejectConfig;
|
||||
|
||||
#[test]
|
||||
fn wants_no_sandbox_approval_reject_respects_sandbox_flag() {
|
||||
let runtime = ApplyPatchRuntime::new();
|
||||
assert!(runtime.wants_no_sandbox_approval(AskForApproval::OnRequest));
|
||||
assert!(
|
||||
!runtime.wants_no_sandbox_approval(AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
}))
|
||||
);
|
||||
assert!(
|
||||
runtime.wants_no_sandbox_approval(AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
}))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -156,6 +156,8 @@ impl ExecApprovalRequirement {
|
||||
|
||||
/// - Never, OnFailure: do not ask
|
||||
/// - OnRequest: ask unless sandbox policy is DangerFullAccess
|
||||
/// - Reject: ask unless sandbox policy is DangerFullAccess, but auto-reject
|
||||
/// when `sandbox_approval` rejection is enabled.
|
||||
/// - UnlessTrusted: always ask
|
||||
pub(crate) fn default_exec_approval_requirement(
|
||||
policy: AskForApproval,
|
||||
@@ -163,14 +165,23 @@ pub(crate) fn default_exec_approval_requirement(
|
||||
) -> ExecApprovalRequirement {
|
||||
let needs_approval = match policy {
|
||||
AskForApproval::Never | AskForApproval::OnFailure => false,
|
||||
AskForApproval::OnRequest => !matches!(
|
||||
AskForApproval::OnRequest | AskForApproval::Reject(_) => !matches!(
|
||||
sandbox_policy,
|
||||
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. }
|
||||
),
|
||||
AskForApproval::UnlessTrusted => true,
|
||||
};
|
||||
|
||||
if needs_approval {
|
||||
if needs_approval
|
||||
&& matches!(
|
||||
policy,
|
||||
AskForApproval::Reject(reject_config) if reject_config.rejects_sandbox_approval()
|
||||
)
|
||||
{
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: "approval policy rejected sandbox approval prompt".to_string(),
|
||||
}
|
||||
} else if needs_approval {
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
@@ -224,7 +235,13 @@ pub(crate) trait Approvable<Req> {
|
||||
|
||||
/// Decide we can request an approval for no-sandbox execution.
|
||||
fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool {
|
||||
!matches!(policy, AskForApproval::Never | AskForApproval::OnRequest)
|
||||
match policy {
|
||||
AskForApproval::OnFailure => true,
|
||||
AskForApproval::UnlessTrusted => true,
|
||||
AskForApproval::Never => false,
|
||||
AskForApproval::OnRequest => false,
|
||||
AskForApproval::Reject(reject_config) => !reject_config.sandbox_approval,
|
||||
}
|
||||
}
|
||||
|
||||
fn start_approval_async<'a>(
|
||||
@@ -313,6 +330,7 @@ impl<'a> SandboxAttempt<'a> {
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_protocol::protocol::NetworkAccess;
|
||||
use codex_protocol::protocol::RejectConfig;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
@@ -344,4 +362,43 @@ mod tests {
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn default_exec_approval_requirement_rejects_sandbox_prompt_when_configured() {
|
||||
let policy = AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
mcp_elicitations: false,
|
||||
});
|
||||
|
||||
let requirement =
|
||||
default_exec_approval_requirement(policy, &SandboxPolicy::new_read_only_policy());
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: "approval policy rejected sandbox approval prompt".to_string(),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn default_exec_approval_requirement_keeps_prompt_when_sandbox_rejection_is_disabled() {
|
||||
let policy = AskForApproval::Reject(RejectConfig {
|
||||
sandbox_approval: false,
|
||||
rules: true,
|
||||
mcp_elicitations: true,
|
||||
});
|
||||
|
||||
let requirement =
|
||||
default_exec_approval_requirement(policy, &SandboxPolicy::new_read_only_policy());
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user