Decouple request permissions feature and tool (#14426)

This commit is contained in:
Jack Mousseau 2026-03-12 14:47:08 -07:00 committed by GitHub
parent bc48b9289a
commit a314c7d3ae
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 482 additions and 35 deletions

View file

@ -928,7 +928,7 @@ Only the granted subset matters on the wire. Any permissions omitted from `resul
Within the same turn, granted permissions are sticky: later shell-like tool calls can automatically reuse the granted subset without reissuing a separate permission request.
If the session approval policy uses `Reject` with `request_permissions: true`, the server does not send `item/permissions/requestApproval` to the client. Instead, the tool is auto-denied and resolves with an empty granted-permissions payload.
If the session approval policy uses `Reject` with `request_permissions: true`, standalone `request_permissions` tool calls are auto-denied and no `item/permissions/requestApproval` prompt is sent. Inline `with_additional_permissions` command requests remain controlled by `sandbox_approval`, and any previously granted permissions remain sticky for later shell-like calls in the same turn.
### Dynamic tool calls (experimental)

View file

@ -1344,7 +1344,7 @@
},
"request_permissions": {
"default": false,
"description": "Reject approval prompts related to built-in permission requests.",
"description": "Reject `request_permissions` tool requests.",
"type": "boolean"
},
"rules": {
@ -1352,7 +1352,7 @@
"type": "boolean"
},
"sandbox_approval": {
"description": "Reject approval prompts related to sandbox escalation.",
"description": "Reject shell command approval requests, including inline `with_additional_permissions` and `require_escalated` requests.",
"type": "boolean"
},
"skill_approval": {

View file

@ -2306,7 +2306,7 @@ async fn request_permissions_emits_event_when_reject_policy_allows_requests() {
}
#[tokio::test]
async fn request_permissions_returns_empty_grant_when_reject_policy_blocks_requests() {
async fn request_permissions_is_auto_denied_when_reject_policy_blocks_tool_requests() {
let (session, mut turn_context, rx) = make_session_and_context_with_rx().await;
*session.active_turn.lock().await = Some(ActiveTurn::default());
Arc::get_mut(&mut turn_context)
@ -2323,10 +2323,13 @@ async fn request_permissions_returns_empty_grant_when_reject_policy_blocks_reque
))
.expect("test setup should allow updating approval policy");
let session = Arc::new(session);
let turn_context = Arc::new(turn_context);
let call_id = "call-1".to_string();
let response = session
.request_permissions(
&turn_context,
"call-1".to_string(),
turn_context.as_ref(),
call_id,
codex_protocol::request_permissions::RequestPermissionsArgs {
reason: Some("need network".to_string()),
permissions: codex_protocol::models::PermissionProfile {
@ -2349,10 +2352,10 @@ async fn request_permissions_returns_empty_grant_when_reject_policy_blocks_reque
)
);
assert!(
tokio::time::timeout(StdDuration::from_millis(50), rx.recv())
tokio::time::timeout(StdDuration::from_millis(100), rx.recv())
.await
.is_err(),
"unexpected request_permissions event emitted",
"request_permissions should not emit an event when reject.request_permissions is set"
);
}

View file

@ -227,6 +227,85 @@ async fn guardian_allows_unified_exec_additional_permissions_requests_past_polic
);
}
#[tokio::test]
#[cfg(unix)]
async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_permissions_feature() {
let (mut session, turn_context_raw) = make_session_and_context().await;
session
.features
.enable(Feature::RequestPermissionsTool)
.expect("test setup should allow enabling request permissions tool");
*session.active_turn.lock().await = Some(ActiveTurn::default());
{
let mut active_turn = session.active_turn.lock().await;
let active_turn = active_turn.as_mut().expect("active turn");
let mut turn_state = active_turn.turn_state.lock().await;
turn_state.record_granted_permissions(PermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
..Default::default()
});
}
let session = Arc::new(session);
let turn_context = Arc::new(turn_context_raw);
let handler = ShellHandler;
let resp = handler
.handle(ToolInvocation {
session: Arc::clone(&session),
turn: Arc::clone(&turn_context),
tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())),
call_id: "sticky-turn-grant".to_string(),
tool_name: "shell".to_string(),
tool_namespace: None,
payload: ToolPayload::Function {
arguments: serde_json::json!({
"command": [
"/bin/sh",
"-c",
"echo hi",
],
"timeout_ms": 1_000_u64,
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
})
.to_string(),
},
})
.await;
match resp {
Ok(output) => {
let output = expect_text_output(&output);
#[derive(Deserialize, PartialEq, Eq, Debug)]
struct ResponseExecMetadata {
exit_code: i32,
}
#[derive(Deserialize)]
struct ResponseExecOutput {
output: String,
metadata: ResponseExecMetadata,
}
let exec_output: ResponseExecOutput =
serde_json::from_str(&output).expect("valid exec output json");
assert_eq!(exec_output.metadata, ResponseExecMetadata { exit_code: 0 });
assert!(exec_output.output.contains("hi"));
}
Err(FunctionCallError::RespondToModel(output)) => {
assert!(
!output.contains("additional permissions are disabled"),
"sticky turn permissions should bypass inline validation: {output}"
);
}
Err(err) => panic!("unexpected error: {err:?}"),
}
}
#[tokio::test]
async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() {
let codex_home = tempdir().expect("create codex home");

View file

@ -101,7 +101,7 @@ fn resolve_workdir_base_path(
/// Validates feature/policy constraints for `with_additional_permissions` and
/// normalizes any path-based permissions. Errors if the request is invalid.
pub(crate) fn normalize_and_validate_additional_permissions(
request_permission_enabled: bool,
additional_permissions_allowed: bool,
approval_policy: AskForApproval,
sandbox_permissions: SandboxPermissions,
additional_permissions: Option<PermissionProfile>,
@ -113,11 +113,12 @@ pub(crate) fn normalize_and_validate_additional_permissions(
SandboxPermissions::WithAdditionalPermissions
);
if !request_permission_enabled
if !permissions_preapproved
&& !additional_permissions_allowed
&& (uses_additional_permissions || additional_permissions.is_some())
{
return Err(
"additional permissions are disabled; enable `features.request_permission` before using `with_additional_permissions`"
"additional permissions are disabled; enable `features.request_permissions` before using `with_additional_permissions`"
.to_string(),
);
}
@ -164,6 +165,23 @@ pub(super) struct EffectiveAdditionalPermissions {
pub permissions_preapproved: bool,
}
pub(super) fn implicit_granted_permissions(
sandbox_permissions: SandboxPermissions,
additional_permissions: Option<&PermissionProfile>,
effective_additional_permissions: &EffectiveAdditionalPermissions,
) -> Option<PermissionProfile> {
if !sandbox_permissions.uses_additional_permissions()
&& !matches!(sandbox_permissions, SandboxPermissions::RequireEscalated)
&& additional_permissions.is_none()
{
effective_additional_permissions
.additional_permissions
.clone()
} else {
None
}
}
pub(super) async fn apply_granted_turn_permissions(
session: &Session,
sandbox_permissions: SandboxPermissions,
@ -210,3 +228,118 @@ pub(super) async fn apply_granted_turn_permissions(
permissions_preapproved,
}
}
#[cfg(test)]
mod tests {
use super::EffectiveAdditionalPermissions;
use super::implicit_granted_permissions;
use super::normalize_and_validate_additional_permissions;
use crate::sandboxing::SandboxPermissions;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::NetworkPermissions;
use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::RejectConfig;
use codex_utils_absolute_path::AbsolutePathBuf;
use pretty_assertions::assert_eq;
use tempfile::tempdir;
fn network_permissions() -> PermissionProfile {
PermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
..Default::default()
}
}
fn file_system_permissions(path: &std::path::Path) -> PermissionProfile {
PermissionProfile {
file_system: Some(FileSystemPermissions {
read: None,
write: Some(vec![
AbsolutePathBuf::from_absolute_path(path).expect("absolute path"),
]),
}),
..Default::default()
}
}
#[test]
fn preapproved_permissions_work_when_request_permissions_tool_is_enabled_without_inline_feature()
{
let cwd = tempdir().expect("tempdir");
let normalized = normalize_and_validate_additional_permissions(
false,
AskForApproval::Reject(RejectConfig {
sandbox_approval: false,
rules: false,
skill_approval: false,
request_permissions: true,
mcp_elicitations: false,
}),
SandboxPermissions::WithAdditionalPermissions,
Some(network_permissions()),
true,
cwd.path(),
)
.expect("preapproved permissions should be allowed");
assert_eq!(normalized, Some(network_permissions()));
}
#[test]
fn fresh_additional_permissions_still_require_request_permissions_feature() {
let cwd = tempdir().expect("tempdir");
let err = normalize_and_validate_additional_permissions(
false,
AskForApproval::OnRequest,
SandboxPermissions::WithAdditionalPermissions,
Some(network_permissions()),
false,
cwd.path(),
)
.expect_err("fresh inline permission requests should remain disabled");
assert_eq!(
err,
"additional permissions are disabled; enable `features.request_permissions` before using `with_additional_permissions`"
);
}
#[test]
fn implicit_sticky_grants_bypass_inline_permission_validation() {
let cwd = tempdir().expect("tempdir");
let granted_permissions = file_system_permissions(cwd.path());
let implicit_permissions = implicit_granted_permissions(
SandboxPermissions::UseDefault,
None,
&EffectiveAdditionalPermissions {
sandbox_permissions: SandboxPermissions::WithAdditionalPermissions,
additional_permissions: Some(granted_permissions.clone()),
permissions_preapproved: false,
},
);
assert_eq!(implicit_permissions, Some(granted_permissions));
}
#[test]
fn explicit_inline_permissions_do_not_use_implicit_sticky_grant_path() {
let cwd = tempdir().expect("tempdir");
let requested_permissions = file_system_permissions(cwd.path());
let implicit_permissions = implicit_granted_permissions(
SandboxPermissions::WithAdditionalPermissions,
Some(&requested_permissions),
&EffectiveAdditionalPermissions {
sandbox_permissions: SandboxPermissions::WithAdditionalPermissions,
additional_permissions: Some(requested_permissions.clone()),
permissions_preapproved: false,
},
);
assert_eq!(implicit_permissions, None);
}
}

View file

@ -21,6 +21,7 @@ use crate::tools::events::ToolEmitter;
use crate::tools::events::ToolEventCtx;
use crate::tools::handlers::apply_granted_turn_permissions;
use crate::tools::handlers::apply_patch::intercept_apply_patch;
use crate::tools::handlers::implicit_granted_permissions;
use crate::tools::handlers::normalize_and_validate_additional_permissions;
use crate::tools::handlers::parse_arguments_with_base_path;
use crate::tools::handlers::resolve_workdir_base_path;
@ -336,19 +337,33 @@ impl ShellHandler {
}
let request_permission_enabled = session.features().enabled(Feature::RequestPermissions);
let requested_additional_permissions = additional_permissions.clone();
let effective_additional_permissions = apply_granted_turn_permissions(
session.as_ref(),
exec_params.sandbox_permissions,
additional_permissions,
)
.await;
let normalized_additional_permissions = normalize_and_validate_additional_permissions(
request_permission_enabled,
turn.approval_policy.value(),
effective_additional_permissions.sandbox_permissions,
effective_additional_permissions.additional_permissions,
effective_additional_permissions.permissions_preapproved,
&exec_params.cwd,
let additional_permissions_allowed = request_permission_enabled
|| (session.features().enabled(Feature::RequestPermissionsTool)
&& effective_additional_permissions.permissions_preapproved);
let normalized_additional_permissions = implicit_granted_permissions(
exec_params.sandbox_permissions,
requested_additional_permissions.as_ref(),
&effective_additional_permissions,
)
.map_or_else(
|| {
normalize_and_validate_additional_permissions(
additional_permissions_allowed,
turn.approval_policy.value(),
effective_additional_permissions.sandbox_permissions,
effective_additional_permissions.additional_permissions,
effective_additional_permissions.permissions_preapproved,
&exec_params.cwd,
)
},
|permissions| Ok(Some(permissions)),
)
.map_err(FunctionCallError::RespondToModel)?;

View file

@ -12,6 +12,7 @@ use crate::tools::context::ToolInvocation;
use crate::tools::context::ToolPayload;
use crate::tools::handlers::apply_granted_turn_permissions;
use crate::tools::handlers::apply_patch::intercept_apply_patch;
use crate::tools::handlers::implicit_granted_permissions;
use crate::tools::handlers::normalize_and_validate_additional_permissions;
use crate::tools::handlers::parse_arguments;
use crate::tools::handlers::parse_arguments_with_base_path;
@ -171,12 +172,16 @@ impl ToolHandler for UnifiedExecHandler {
let request_permission_enabled =
session.features().enabled(Feature::RequestPermissions);
let requested_additional_permissions = additional_permissions.clone();
let effective_additional_permissions = apply_granted_turn_permissions(
context.session.as_ref(),
sandbox_permissions,
additional_permissions,
)
.await;
let additional_permissions_allowed = request_permission_enabled
|| (session.features().enabled(Feature::RequestPermissionsTool)
&& effective_additional_permissions.permissions_preapproved);
// Sticky turn permissions have already been approved, so they should
// continue through the normal exec approval flow for the command.
@ -200,21 +205,30 @@ impl ToolHandler for UnifiedExecHandler {
let workdir = workdir.map(|dir| context.turn.resolve_path(Some(dir)));
let cwd = workdir.clone().unwrap_or(cwd);
let normalized_additional_permissions =
match normalize_and_validate_additional_permissions(
request_permission_enabled,
context.turn.approval_policy.value(),
effective_additional_permissions.sandbox_permissions,
effective_additional_permissions.additional_permissions,
effective_additional_permissions.permissions_preapproved,
&cwd,
) {
Ok(normalized) => normalized,
Err(err) => {
manager.release_process_id(process_id).await;
return Err(FunctionCallError::RespondToModel(err));
}
};
let normalized_additional_permissions = match implicit_granted_permissions(
sandbox_permissions,
requested_additional_permissions.as_ref(),
&effective_additional_permissions,
)
.map_or_else(
|| {
normalize_and_validate_additional_permissions(
additional_permissions_allowed,
context.turn.approval_policy.value(),
effective_additional_permissions.sandbox_permissions,
effective_additional_permissions.additional_permissions,
effective_additional_permissions.permissions_preapproved,
&cwd,
)
},
|permissions| Ok(Some(permissions)),
) {
Ok(normalized) => normalized,
Err(err) => {
manager.release_process_id(process_id).await;
return Err(FunctionCallError::RespondToModel(err));
}
};
if let Some(output) = intercept_apply_patch(
&command,

View file

@ -10,6 +10,7 @@ use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ExecApprovalRequestEvent;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::RejectConfig;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::SandboxPolicy;
use codex_protocol::request_permissions::PermissionGrantScope;
@ -395,6 +396,95 @@ async fn with_additional_permissions_requires_approval_under_on_request() -> Res
Ok(())
}
#[tokio::test(flavor = "current_thread")]
async fn request_permissions_tool_is_auto_denied_when_reject_request_permissions_is_enabled()
-> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
let server = start_mock_server().await;
let approval_policy = AskForApproval::Reject(RejectConfig {
sandbox_approval: false,
rules: false,
skill_approval: false,
request_permissions: true,
mcp_elicitations: false,
});
let sandbox_policy = SandboxPolicy::new_read_only_policy();
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.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config);
config
.features
.enable(Feature::RequestPermissionsTool)
.expect("test config should allow feature update");
});
let test = builder.build(&server).await?;
let requested_dir = test.workspace_path("request-permissions-reject");
fs::create_dir_all(&requested_dir)?;
let requested_permissions = requested_directory_write_permissions(&requested_dir);
let call_id = "request_permissions_reject_auto_denied";
let event = request_permissions_tool_event(
call_id,
"Request access through the standalone tool",
&requested_permissions,
)?;
let _ = mount_sse_once(
&server,
sse(vec![
ev_response_created("resp-request-permissions-reject-1"),
event,
ev_completed("resp-request-permissions-reject-1"),
]),
)
.await;
let results = mount_sse_once(
&server,
sse(vec![
ev_assistant_message("msg-request-permissions-reject-1", "done"),
ev_completed("resp-request-permissions-reject-2"),
]),
)
.await;
submit_turn(
&test,
"request permissions under reject.request_permissions",
approval_policy,
sandbox_policy,
)
.await?;
let event = wait_for_event(&test.codex, |event| {
matches!(
event,
EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_)
)
})
.await;
assert!(
matches!(event, EventMsg::TurnComplete(_)),
"request_permissions should not emit a prompt when reject.request_permissions is set: {event:?}"
);
let call_output = results.single_request().function_call_output(call_id);
let result: RequestPermissionsResponse =
serde_json::from_str(call_output["output"].as_str().unwrap_or_default())?;
assert_eq!(
result,
RequestPermissionsResponse {
permissions: PermissionProfile::default(),
scope: PermissionGrantScope::Turn,
}
);
Ok(())
}
#[tokio::test(flavor = "current_thread")]
async fn relative_additional_permissions_resolve_against_tool_workdir() -> Result<()> {
skip_if_no_network!(Ok(()));
@ -1254,6 +1344,118 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls() -> Resu
Ok(())
}
#[tokio::test(flavor = "current_thread")]
async fn request_permissions_grants_apply_to_later_shell_command_calls_without_inline_permission_feature()
-> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
let server = start_mock_server().await;
let approval_policy = AskForApproval::OnRequest;
let sandbox_policy = workspace_write_excluding_tmp();
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.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config);
config
.features
.enable(Feature::RequestPermissionsTool)
.expect("test config should allow feature update");
});
let test = builder.build(&server).await?;
let outside_dir = tempfile::tempdir()?;
let outside_write = outside_dir
.path()
.join("sticky-shell-feature-independent.txt");
let command = format!(
"printf {:?} > {:?} && cat {:?}",
"sticky-shell-feature-independent-ok", outside_write, outside_write
);
let requested_permissions = requested_directory_write_permissions(outside_dir.path());
let normalized_requested_permissions =
normalized_directory_write_permissions(outside_dir.path())?;
let responses = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_response_created("resp-sticky-shell-independent-1"),
request_permissions_tool_event(
"permissions-call",
"Allow writing outside the workspace",
&requested_permissions,
)?,
ev_completed("resp-sticky-shell-independent-1"),
]),
sse(vec![
ev_response_created("resp-sticky-shell-independent-2"),
shell_command_event("shell-call", &command)?,
ev_completed("resp-sticky-shell-independent-2"),
]),
sse(vec![
ev_response_created("resp-sticky-shell-independent-3"),
ev_assistant_message("msg-sticky-shell-independent-1", "done"),
ev_completed("resp-sticky-shell-independent-3"),
]),
],
)
.await;
submit_turn(
&test,
"write outside the workspace without inline permission feature",
approval_policy,
sandbox_policy,
)
.await?;
let granted_permissions = expect_request_permissions_event(&test, "permissions-call").await;
assert_eq!(
granted_permissions,
normalized_requested_permissions.clone()
);
test.codex
.submit(Op::RequestPermissionsResponse {
id: "permissions-call".to_string(),
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
},
})
.await?;
if let Some(approval) = wait_for_exec_approval_or_completion(&test).await {
test.codex
.submit(Op::ExecApproval {
id: approval.effective_approval_id(),
turn_id: None,
decision: ReviewDecision::Approved,
})
.await?;
wait_for_completion(&test).await;
}
let shell_output = responses
.function_call_output_text("shell-call")
.map(|output| json!({ "output": output }))
.unwrap_or_else(|| panic!("expected shell-call output"));
let result = parse_result(&shell_output);
assert!(
result.exit_code.is_none_or(|exit_code| exit_code == 0),
"expected success output, got exit_code={:?}, stdout={:?}",
result.exit_code,
result.stdout
);
assert_eq!(result.stdout.trim(), "sticky-shell-feature-independent-ok");
assert_eq!(
fs::read_to_string(&outside_write)?,
"sticky-shell-feature-independent-ok"
);
Ok(())
}
#[tokio::test(flavor = "current_thread")]
async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() -> Result<()> {
skip_if_no_network!(Ok(()));

View file

@ -529,14 +529,15 @@ pub enum AskForApproval {
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema, TS)]
pub struct RejectConfig {
/// Reject approval prompts related to sandbox escalation.
/// Reject shell command approval requests, including inline
/// `with_additional_permissions` and `require_escalated` requests.
pub sandbox_approval: bool,
/// Reject prompts triggered by execpolicy `prompt` rules.
pub rules: bool,
/// Reject approval prompts triggered by skill script execution.
#[serde(default)]
pub skill_approval: bool,
/// Reject approval prompts related to built-in permission requests.
/// Reject `request_permissions` tool requests.
#[serde(default)]
pub request_permissions: bool,
/// Reject MCP elicitation prompts.