Clarify sandbox permission override helper semantics (#13703)
## Summary Today `SandboxPermissions::requires_additional_permissions()` does not actually mean "is `WithAdditionalPermissions`". It returns `true` for any non-default sandbox override, including `RequireEscalated`. That broad behavior is relied on in multiple `main` callsites. The naming is security-sensitive because `SandboxPermissions` is used on shell-like tool calls to tell the executor how a single command should relate to the turn sandbox: - `UseDefault`: run with the turn sandbox unchanged - `RequireEscalated`: request execution outside the sandbox - `WithAdditionalPermissions`: stay sandboxed but widen permissions for that command only ## Problem The old helper name reads as if it only applies to the `WithAdditionalPermissions` variant. In practice it means "this command requested any explicit sandbox override." That ambiguity made it easy to read production checks incorrectly and made the guardian change look like a standalone `main` fix when it is not. On `main` today: - `shell` and `unified_exec` intentionally reject any explicit `sandbox_permissions` request unless approval policy is `OnRequest` - `exec_policy` intentionally treats any explicit sandbox override as prompt-worthy in restricted sandboxes - tests intentionally serialize both `RequireEscalated` and `WithAdditionalPermissions` as explicit sandbox override requests So changing those callsites from the broad helper to a narrow `WithAdditionalPermissions` check would be a behavior change, not a pure cleanup. ## What This PR Does - documents `SandboxPermissions` as a per-command sandbox override, not a generic permissions bag - adds `requests_sandbox_override()` for the broad meaning: anything except `UseDefault` - adds `uses_additional_permissions()` for the narrow meaning: only `WithAdditionalPermissions` - keeps `requires_additional_permissions()` as a compatibility alias to the broad meaning for now - updates the current broad callsites to use the accurately named broad helper - adds unit coverage that locks in the semantics of all three helpers ## What This PR Does Not Do This PR does not change runtime behavior. That is intentional. --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
Charley Cunningham
GitHub
parent
c8f4b5bc1e
commit
cb1a182bbe
5 changed files with 55 additions and 14 deletions
|
|
@ -538,7 +538,7 @@ pub fn render_decision_for_unmatched_command(
|
|||
// In restricted sandboxes (ReadOnly/WorkspaceWrite), do not prompt for
|
||||
// non‑escalated, non‑dangerous commands — let the sandbox enforce
|
||||
// restrictions (e.g., block network/write) without a user prompt.
|
||||
if sandbox_permissions.requires_additional_permissions() {
|
||||
if sandbox_permissions.requests_sandbox_override() {
|
||||
Decision::Prompt
|
||||
} else {
|
||||
Decision::Allow
|
||||
|
|
@ -553,7 +553,7 @@ pub fn render_decision_for_unmatched_command(
|
|||
Decision::Allow
|
||||
}
|
||||
SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. } => {
|
||||
if sandbox_permissions.requires_additional_permissions() {
|
||||
if sandbox_permissions.requests_sandbox_override() {
|
||||
Decision::Prompt
|
||||
} else {
|
||||
Decision::Allow
|
||||
|
|
|
|||
|
|
@ -342,9 +342,7 @@ impl ShellHandler {
|
|||
.map_err(FunctionCallError::RespondToModel)?;
|
||||
|
||||
// Approval policy guard for explicit escalation in non-OnRequest modes.
|
||||
if exec_params
|
||||
.sandbox_permissions
|
||||
.requires_additional_permissions()
|
||||
if exec_params.sandbox_permissions.requests_sandbox_override()
|
||||
&& !matches!(
|
||||
turn.approval_policy.value(),
|
||||
codex_protocol::protocol::AskForApproval::OnRequest
|
||||
|
|
|
|||
|
|
@ -171,7 +171,7 @@ impl ToolHandler for UnifiedExecHandler {
|
|||
let request_permission_enabled =
|
||||
session.features().enabled(Feature::RequestPermissions);
|
||||
|
||||
if sandbox_permissions.requires_additional_permissions()
|
||||
if sandbox_permissions.requests_sandbox_override()
|
||||
&& !matches!(
|
||||
context.turn.approval_policy.value(),
|
||||
codex_protocol::protocol::AskForApproval::OnRequest
|
||||
|
|
|
|||
|
|
@ -239,7 +239,7 @@ fn shell_event_with_prefix_rule(
|
|||
"command": command,
|
||||
"timeout_ms": timeout_ms,
|
||||
});
|
||||
if sandbox_permissions.requires_additional_permissions() {
|
||||
if sandbox_permissions.requests_sandbox_override() {
|
||||
args["sandbox_permissions"] = json!(sandbox_permissions);
|
||||
}
|
||||
if let Some(prefix_rule) = prefix_rule {
|
||||
|
|
@ -262,7 +262,7 @@ fn exec_command_event(
|
|||
if let Some(yield_time_ms) = yield_time_ms {
|
||||
args["yield_time_ms"] = json!(yield_time_ms);
|
||||
}
|
||||
if sandbox_permissions.requires_additional_permissions() {
|
||||
if sandbox_permissions.requests_sandbox_override() {
|
||||
args["sandbox_permissions"] = json!(sandbox_permissions);
|
||||
let reason = justification.unwrap_or(DEFAULT_UNIFIED_EXEC_JUSTIFICATION);
|
||||
args["justification"] = json!(reason);
|
||||
|
|
|
|||
|
|
@ -28,18 +28,19 @@ use schemars::JsonSchema;
|
|||
|
||||
use crate::mcp::CallToolResult;
|
||||
|
||||
/// Controls whether a command should use the session sandbox or bypass it.
|
||||
/// Controls the per-command sandbox override requested by a shell-like tool call.
|
||||
#[derive(
|
||||
Debug, Clone, Copy, Default, Eq, Hash, PartialEq, Serialize, Deserialize, JsonSchema, TS,
|
||||
)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum SandboxPermissions {
|
||||
/// Run with the configured sandbox
|
||||
/// Run with the turn's configured sandbox policy unchanged.
|
||||
#[default]
|
||||
UseDefault,
|
||||
/// Request to run outside the sandbox
|
||||
/// Request to run outside the sandbox.
|
||||
RequireEscalated,
|
||||
/// Request to run in the sandbox with additional per-command permissions.
|
||||
/// Request to stay in the sandbox while widening permissions for this
|
||||
/// command only.
|
||||
WithAdditionalPermissions,
|
||||
}
|
||||
|
||||
|
|
@ -49,10 +50,17 @@ impl SandboxPermissions {
|
|||
matches!(self, SandboxPermissions::RequireEscalated)
|
||||
}
|
||||
|
||||
/// True if SandboxPermissions requires permissions beyond UseDefault
|
||||
pub fn requires_additional_permissions(self) -> bool {
|
||||
/// True if SandboxPermissions requests any explicit per-command override
|
||||
/// beyond `UseDefault`.
|
||||
pub fn requests_sandbox_override(self) -> bool {
|
||||
!matches!(self, SandboxPermissions::UseDefault)
|
||||
}
|
||||
|
||||
/// True if SandboxPermissions uses the sandboxed per-command permission
|
||||
/// widening flow.
|
||||
pub fn uses_additional_permissions(self) -> bool {
|
||||
matches!(self, SandboxPermissions::WithAdditionalPermissions)
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Default, Eq, Hash, PartialEq, Serialize, Deserialize, JsonSchema, TS)]
|
||||
|
|
@ -1305,6 +1313,41 @@ mod tests {
|
|||
use std::path::PathBuf;
|
||||
use tempfile::tempdir;
|
||||
|
||||
#[test]
|
||||
fn sandbox_permissions_helpers_match_documented_semantics() {
|
||||
let cases = [
|
||||
(SandboxPermissions::UseDefault, false, false, false),
|
||||
(SandboxPermissions::RequireEscalated, true, true, false),
|
||||
(
|
||||
SandboxPermissions::WithAdditionalPermissions,
|
||||
false,
|
||||
true,
|
||||
true,
|
||||
),
|
||||
];
|
||||
|
||||
for (
|
||||
sandbox_permissions,
|
||||
requires_escalated_permissions,
|
||||
requests_sandbox_override,
|
||||
uses_additional_permissions,
|
||||
) in cases
|
||||
{
|
||||
assert_eq!(
|
||||
sandbox_permissions.requires_escalated_permissions(),
|
||||
requires_escalated_permissions
|
||||
);
|
||||
assert_eq!(
|
||||
sandbox_permissions.requests_sandbox_override(),
|
||||
requests_sandbox_override
|
||||
);
|
||||
assert_eq!(
|
||||
sandbox_permissions.uses_additional_permissions(),
|
||||
uses_additional_permissions
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn convert_mcp_content_to_items_preserves_data_urls() {
|
||||
let contents = vec![serde_json::json!({
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue