Feat: Preserve network access on read-only sandbox policies (#13409)

## Summary

`PermissionProfile.network` could not be preserved when additional or
compiled permissions resolved to
`SandboxPolicy::ReadOnly`, because `ReadOnly` had no network_access
field. This change makes read-only + network
enabled representable directly and threads that through the protocol,
app-server v2 mirror, and permission-
  merging logic.

## What changed

- Added `network_access: bool` to `SandboxPolicy::ReadOnly` in the core
protocol and app-server v2 protocol.
- Kept backward compatibility by defaulting the new field to false, so
legacy read-only payloads still
    deserialize unchanged.
- Updated `has_full_network_access()` and sandbox summaries to respect
read-only network access.
  - Preserved PermissionProfile.network when:
      - compiling skill permission profiles into sandbox policies
      - normalizing additional permissions
      - merging additional permissions into existing sandbox policies
- Updated the approval overlay to show network in the rendered
permission rule when requested.
  - Regenerated app-server schema fixtures for the new v2 wire shape.
This commit is contained in:
Celia Chen 2026-03-03 18:41:57 -08:00 committed by GitHub
parent 2d8c1575b8
commit e6773f856c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
20 changed files with 218 additions and 26 deletions

View file

@ -1637,6 +1637,10 @@
"type": "fullAccess"
}
},
"networkAccess": {
"default": false,
"type": "boolean"
},
"type": {
"enum": [
"readOnly"

View file

@ -5350,6 +5350,10 @@
],
"description": "Read access granted while running under this policy."
},
"network_access": {
"description": "When set to `true`, outbound network access is allowed. `false` by default.",
"type": "boolean"
},
"type": {
"enum": [
"read-only"

View file

@ -11963,6 +11963,10 @@
"type": "fullAccess"
}
},
"networkAccess": {
"default": false,
"type": "boolean"
},
"type": {
"enum": [
"readOnly"

View file

@ -9743,6 +9743,10 @@
"type": "fullAccess"
}
},
"networkAccess": {
"default": false,
"type": "boolean"
},
"type": {
"enum": [
"readOnly"

View file

@ -89,6 +89,10 @@
"type": "fullAccess"
}
},
"networkAccess": {
"default": false,
"type": "boolean"
},
"type": {
"enum": [
"readOnly"

View file

@ -653,6 +653,10 @@
"type": "fullAccess"
}
},
"networkAccess": {
"default": false,
"type": "boolean"
},
"type": {
"enum": [
"readOnly"

View file

@ -653,6 +653,10 @@
"type": "fullAccess"
}
},
"networkAccess": {
"default": false,
"type": "boolean"
},
"type": {
"enum": [
"readOnly"

View file

@ -653,6 +653,10 @@
"type": "fullAccess"
}
},
"networkAccess": {
"default": false,
"type": "boolean"
},
"type": {
"enum": [
"readOnly"

View file

@ -214,6 +214,10 @@
"type": "fullAccess"
}
},
"networkAccess": {
"default": false,
"type": "boolean"
},
"type": {
"enum": [
"readOnly"

View file

@ -12,7 +12,12 @@ export type SandboxPolicy = { "type": "danger-full-access" } | { "type": "read-o
/**
* Read access granted while running under this policy.
*/
access?: ReadOnlyAccess, } | { "type": "external-sandbox",
access?: ReadOnlyAccess,
/**
* When set to `true`, outbound network access is allowed. `false` by
* default.
*/
network_access?: boolean, } | { "type": "external-sandbox",
/**
* Whether the external sandbox permits outbound network traffic.
*/

View file

@ -5,4 +5,4 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf";
import type { NetworkAccess } from "./NetworkAccess";
import type { ReadOnlyAccess } from "./ReadOnlyAccess";
export type SandboxPolicy = { "type": "dangerFullAccess" } | { "type": "readOnly", access: ReadOnlyAccess, } | { "type": "externalSandbox", networkAccess: NetworkAccess, } | { "type": "workspaceWrite", writableRoots: Array<AbsolutePathBuf>, readOnlyAccess: ReadOnlyAccess, networkAccess: boolean, excludeTmpdirEnvVar: boolean, excludeSlashTmp: boolean, };
export type SandboxPolicy = { "type": "dangerFullAccess" } | { "type": "readOnly", access: ReadOnlyAccess, networkAccess: boolean, } | { "type": "externalSandbox", networkAccess: NetworkAccess, } | { "type": "workspaceWrite", writableRoots: Array<AbsolutePathBuf>, readOnlyAccess: ReadOnlyAccess, networkAccess: boolean, excludeTmpdirEnvVar: boolean, excludeSlashTmp: boolean, };

View file

@ -950,6 +950,8 @@ pub enum SandboxPolicy {
ReadOnly {
#[serde(default)]
access: ReadOnlyAccess,
#[serde(default)]
network_access: bool,
},
#[serde(rename_all = "camelCase")]
#[ts(rename_all = "camelCase")]
@ -979,11 +981,13 @@ impl SandboxPolicy {
SandboxPolicy::DangerFullAccess => {
codex_protocol::protocol::SandboxPolicy::DangerFullAccess
}
SandboxPolicy::ReadOnly { access } => {
codex_protocol::protocol::SandboxPolicy::ReadOnly {
access: access.to_core(),
}
}
SandboxPolicy::ReadOnly {
access,
network_access,
} => codex_protocol::protocol::SandboxPolicy::ReadOnly {
access: access.to_core(),
network_access: *network_access,
},
SandboxPolicy::ExternalSandbox { network_access } => {
codex_protocol::protocol::SandboxPolicy::ExternalSandbox {
network_access: match network_access {
@ -1015,11 +1019,13 @@ impl From<codex_protocol::protocol::SandboxPolicy> for SandboxPolicy {
codex_protocol::protocol::SandboxPolicy::DangerFullAccess => {
SandboxPolicy::DangerFullAccess
}
codex_protocol::protocol::SandboxPolicy::ReadOnly { access } => {
SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::from(access),
}
}
codex_protocol::protocol::SandboxPolicy::ReadOnly {
access,
network_access,
} => SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::from(access),
network_access,
},
codex_protocol::protocol::SandboxPolicy::ExternalSandbox { network_access } => {
SandboxPolicy::ExternalSandbox {
network_access: match network_access {
@ -4342,6 +4348,7 @@ mod tests {
include_platform_defaults: false,
readable_roots: vec![readable_root.clone()],
},
network_access: true,
};
let core_policy = v2_policy.to_core();
@ -4352,6 +4359,7 @@ mod tests {
include_platform_defaults: false,
readable_roots: vec![readable_root],
},
network_access: true,
}
);
@ -4402,6 +4410,7 @@ mod tests {
policy,
SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::FullAccess,
network_access: false,
}
);
}

View file

@ -589,6 +589,7 @@ fn trigger_zsh_fork_multi_cmd_approval(
turn_params.approval_policy = Some(AskForApproval::OnRequest);
turn_params.sandbox_policy = Some(SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::FullAccess,
network_access: false,
});
let turn_response = client.turn_start(turn_params)?;
@ -722,6 +723,7 @@ fn trigger_cmd_approval(
Some(AskForApproval::OnRequest),
Some(SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::FullAccess,
network_access: false,
}),
dynamic_tools,
)
@ -744,6 +746,7 @@ fn trigger_patch_approval(
Some(AskForApproval::OnRequest),
Some(SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::FullAccess,
network_access: false,
}),
dynamic_tools,
)

View file

@ -31,6 +31,7 @@ use codex_protocol::models::PermissionProfile;
pub use codex_protocol::models::SandboxPermissions;
use codex_protocol::protocol::ReadOnlyAccess;
use codex_utils_absolute_path::AbsolutePathBuf;
use dunce::canonicalize;
use std::collections::HashMap;
use std::collections::HashSet;
use std::path::Path;
@ -110,6 +111,7 @@ pub(crate) fn normalize_additional_permissions(
.write
.map(|paths| normalize_permission_paths(paths, "file_system.write"));
Ok(PermissionProfile {
network: additional_permissions.network,
file_system: Some(FileSystemPermissions { read, write }),
..Default::default()
})
@ -123,9 +125,7 @@ fn normalize_permission_paths(
let mut seen = HashSet::new();
for path in paths {
let canonicalized = path
.as_path()
.canonicalize()
let canonicalized = canonicalize(path.as_path())
.ok()
.and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok())
.unwrap_or(path);
@ -189,6 +189,13 @@ fn merge_read_only_access_with_additional_reads(
}
}
fn merge_network_access(
base_network_access: bool,
additional_permissions: &PermissionProfile,
) -> bool {
base_network_access || matches!(additional_permissions.network, Some(true))
}
fn sandbox_policy_with_additional_permissions(
sandbox_policy: &SandboxPolicy,
additional_permissions: &PermissionProfile,
@ -218,15 +225,19 @@ fn sandbox_policy_with_additional_permissions(
read_only_access,
extra_reads,
),
network_access: *network_access,
network_access: merge_network_access(*network_access, additional_permissions),
exclude_tmpdir_env_var: *exclude_tmpdir_env_var,
exclude_slash_tmp: *exclude_slash_tmp,
}
}
SandboxPolicy::ReadOnly { access } => {
SandboxPolicy::ReadOnly {
access,
network_access,
} => {
if extra_writes.is_empty() {
SandboxPolicy::ReadOnly {
access: merge_read_only_access_with_additional_reads(access, extra_reads),
network_access: merge_network_access(*network_access, additional_permissions),
}
} else {
// todo(dylan) - for now, this grants more access than the request. We should restrict this,
@ -238,7 +249,7 @@ fn sandbox_policy_with_additional_permissions(
access,
extra_reads,
),
network_access: false,
network_access: merge_network_access(*network_access, additional_permissions),
exclude_tmpdir_env_var: false,
exclude_slash_tmp: false,
}
@ -412,11 +423,19 @@ pub async fn execute_env(
#[cfg(test)]
mod tests {
use super::SandboxManager;
use super::normalize_additional_permissions;
use super::sandbox_policy_with_additional_permissions;
use crate::exec::SandboxType;
use crate::protocol::ReadOnlyAccess;
use crate::protocol::SandboxPolicy;
use crate::tools::sandboxing::SandboxablePreference;
use codex_protocol::config_types::WindowsSandboxLevel;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::PermissionProfile;
use codex_utils_absolute_path::AbsolutePathBuf;
use dunce::canonicalize;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
#[test]
fn danger_full_access_defaults_to_no_sandbox_without_network_requirements() {
@ -442,4 +461,69 @@ mod tests {
);
assert_eq!(sandbox, expected);
}
#[test]
fn normalize_additional_permissions_preserves_network() {
let temp_dir = TempDir::new().expect("create temp dir");
let path = AbsolutePathBuf::from_absolute_path(
canonicalize(temp_dir.path()).expect("canonicalize temp dir"),
)
.expect("absolute temp dir");
let permissions = normalize_additional_permissions(PermissionProfile {
network: Some(true),
file_system: Some(FileSystemPermissions {
read: Some(vec![path.clone()]),
write: Some(vec![path.clone()]),
}),
..Default::default()
})
.expect("permissions");
assert_eq!(permissions.network, Some(true));
assert_eq!(
permissions.file_system,
Some(FileSystemPermissions {
read: Some(vec![path.clone()]),
write: Some(vec![path]),
})
);
}
#[test]
fn read_only_additional_permissions_can_enable_network_without_writes() {
let temp_dir = TempDir::new().expect("create temp dir");
let path = AbsolutePathBuf::from_absolute_path(
canonicalize(temp_dir.path()).expect("canonicalize temp dir"),
)
.expect("absolute temp dir");
let policy = sandbox_policy_with_additional_permissions(
&SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::Restricted {
include_platform_defaults: true,
readable_roots: vec![path.clone()],
},
network_access: false,
},
&PermissionProfile {
network: Some(true),
file_system: Some(FileSystemPermissions {
read: Some(vec![path.clone()]),
write: Some(Vec::new()),
}),
..Default::default()
},
)
.expect("policy");
assert_eq!(
policy,
SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::Restricted {
include_platform_defaults: true,
readable_roots: vec![path],
},
network_access: true,
}
);
}
}

View file

@ -44,6 +44,7 @@ pub(crate) fn compile_permission_profile(
file_system,
macos,
} = permissions?;
let network_access = network.unwrap_or_default();
let file_system = file_system.unwrap_or_default();
let fs_read = normalize_permission_paths(
file_system.read.as_deref().unwrap_or_default(),
@ -64,7 +65,7 @@ pub(crate) fn compile_permission_profile(
readable_roots: fs_read,
}
},
network_access: network.unwrap_or_default(),
network_access,
exclude_tmpdir_env_var: false,
exclude_slash_tmp: false,
}
@ -74,6 +75,12 @@ pub(crate) fn compile_permission_profile(
include_platform_defaults: true,
readable_roots: fs_read,
},
network_access,
}
} else if network_access {
SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::FullAccess,
network_access: true,
}
} else {
// Default sandbox policy
@ -320,7 +327,10 @@ mod tests {
profile,
Permissions {
approval_policy: Constrained::allow_any(AskForApproval::Never),
sandbox_policy: Constrained::allow_any(SandboxPolicy::new_read_only_policy()),
sandbox_policy: Constrained::allow_any(SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::FullAccess,
network_access: true,
}),
network: None,
allow_login_shell: true,
shell_environment_policy: ShellEnvironmentPolicy::default(),
@ -366,6 +376,7 @@ mod tests {
.expect("absolute read path")
],
},
network_access: true,
}),
network: None,
allow_login_shell: true,

View file

@ -478,6 +478,7 @@ mod tests {
.expect("absolute readable root"),
],
},
network_access: false,
};
let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args");
@ -503,6 +504,7 @@ mod tests {
include_platform_defaults: true,
readable_roots: Vec::new(),
},
network_access: false,
};
// `ReadOnlyAccess::Restricted` always includes `cwd` as a readable

View file

@ -626,6 +626,11 @@ pub enum SandboxPolicy {
skip_serializing_if = "ReadOnlyAccess::has_full_disk_read_access"
)]
access: ReadOnlyAccess,
/// When set to `true`, outbound network access is allowed. `false` by
/// default.
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
network_access: bool,
},
/// Indicates the process is already in an external sandbox. Allows full
@ -715,6 +720,7 @@ impl SandboxPolicy {
pub fn new_read_only_policy() -> Self {
SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::FullAccess,
network_access: false,
}
}
@ -735,7 +741,7 @@ impl SandboxPolicy {
match self {
SandboxPolicy::DangerFullAccess => true,
SandboxPolicy::ExternalSandbox { .. } => true,
SandboxPolicy::ReadOnly { access } => access.has_full_disk_read_access(),
SandboxPolicy::ReadOnly { access, .. } => access.has_full_disk_read_access(),
SandboxPolicy::WorkspaceWrite {
read_only_access, ..
} => read_only_access.has_full_disk_read_access(),
@ -755,7 +761,7 @@ impl SandboxPolicy {
match self {
SandboxPolicy::DangerFullAccess => true,
SandboxPolicy::ExternalSandbox { network_access } => network_access.is_enabled(),
SandboxPolicy::ReadOnly { .. } => false,
SandboxPolicy::ReadOnly { network_access, .. } => *network_access,
SandboxPolicy::WorkspaceWrite { network_access, .. } => *network_access,
}
}
@ -766,7 +772,7 @@ impl SandboxPolicy {
return false;
}
match self {
SandboxPolicy::ReadOnly { access } => access.include_platform_defaults(),
SandboxPolicy::ReadOnly { access, .. } => access.include_platform_defaults(),
SandboxPolicy::WorkspaceWrite {
read_only_access, ..
} => read_only_access.include_platform_defaults(),
@ -782,7 +788,7 @@ impl SandboxPolicy {
pub fn get_readable_roots_with_cwd(&self, cwd: &Path) -> Vec<AbsolutePathBuf> {
let mut roots = match self {
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => Vec::new(),
SandboxPolicy::ReadOnly { access } => access.get_readable_roots_with_cwd(cwd),
SandboxPolicy::ReadOnly { access, .. } => access.get_readable_roots_with_cwd(cwd),
SandboxPolicy::WorkspaceWrite {
read_only_access, ..
} => {
@ -3131,6 +3137,18 @@ mod tests {
assert!(enabled.has_full_network_access());
}
#[test]
fn read_only_reports_network_access_flags() {
let restricted = SandboxPolicy::new_read_only_policy();
assert!(!restricted.has_full_network_access());
let enabled = SandboxPolicy::ReadOnly {
access: ReadOnlyAccess::FullAccess,
network_access: true,
};
assert!(enabled.has_full_network_access());
}
#[test]
fn reject_config_mcp_elicitation_flag_is_field_driven() {
assert!(

View file

@ -641,6 +641,9 @@ fn format_additional_permissions_rule(
additional_permissions: &PermissionProfile,
) -> Option<String> {
let mut parts = Vec::new();
if matches!(additional_permissions.network, Some(true)) {
parts.push("network".to_string());
}
if let Some(file_system) = additional_permissions.file_system.as_ref() {
if let Some(read) = file_system.read.as_ref() {
let reads = read
@ -1074,6 +1077,7 @@ mod tests {
available_decisions: vec![ReviewDecision::Approved, ReviewDecision::Abort],
network_approval_context: None,
additional_permissions: Some(PermissionProfile {
network: Some(true),
file_system: Some(FileSystemPermissions {
read: Some(vec![absolute_path("/tmp/readme.txt")]),
write: Some(vec![absolute_path("/tmp/out.txt")]),
@ -1100,6 +1104,10 @@ mod tests {
.any(|line| line.contains("Permission rule:")),
"expected permission-rule line, got {rendered:?}"
);
assert!(
rendered.iter().any(|line| line.contains("network;")),
"expected network permission text, got {rendered:?}"
);
}
#[test]
@ -1115,6 +1123,7 @@ mod tests {
available_decisions: vec![ReviewDecision::Approved, ReviewDecision::Abort],
network_approval_context: None,
additional_permissions: Some(PermissionProfile {
network: Some(true),
file_system: Some(FileSystemPermissions {
read: Some(vec![absolute_path("/tmp/readme.txt")]),
write: Some(vec![absolute_path("/tmp/out.txt")]),

View file

@ -7,7 +7,7 @@ expression: "render_overlay_lines(&view, 120)"
Reason: need filesystem access
Permission rule: read `/tmp/readme.txt`; write `/tmp/out.txt`
Permission rule: network; read `/tmp/readme.txt`; write `/tmp/out.txt`
$ cat /tmp/readme.txt

View file

@ -4,7 +4,13 @@ use codex_protocol::protocol::SandboxPolicy;
pub fn summarize_sandbox_policy(sandbox_policy: &SandboxPolicy) -> String {
match sandbox_policy {
SandboxPolicy::DangerFullAccess => "danger-full-access".to_string(),
SandboxPolicy::ReadOnly { .. } => "read-only".to_string(),
SandboxPolicy::ReadOnly { network_access, .. } => {
let mut summary = "read-only".to_string();
if *network_access {
summary.push_str(" (network access enabled)");
}
summary
}
SandboxPolicy::ExternalSandbox { network_access } => {
let mut summary = "external-sandbox".to_string();
if matches!(network_access, NetworkAccess::Enabled) {
@ -66,6 +72,15 @@ mod tests {
assert_eq!(summary, "external-sandbox (network access enabled)");
}
#[test]
fn summarizes_read_only_with_enabled_network() {
let summary = summarize_sandbox_policy(&SandboxPolicy::ReadOnly {
access: Default::default(),
network_access: true,
});
assert_eq!(summary, "read-only (network access enabled)");
}
#[test]
fn workspace_write_summary_still_includes_network_access() {
let root = if cfg!(windows) { "C:\\repo" } else { "/repo" };