mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
permissions: remove legacy read-only access modes (#19449)
## Why `ReadOnlyAccess` was a transitional legacy shape on `SandboxPolicy`: `FullAccess` meant the historical read-only/workspace-write modes could read the full filesystem, while `Restricted` tried to carry partial readable roots. The partial-read model now belongs in `FileSystemSandboxPolicy` and `PermissionProfile`, so keeping it on `SandboxPolicy` makes every legacy projection reintroduce lossy read-root bookkeeping and creates unnecessary noise in the rest of the permissions migration. This PR makes the legacy policy model narrower and explicit: `SandboxPolicy::ReadOnly` and `SandboxPolicy::WorkspaceWrite` represent the old full-read sandbox modes only. Split readable roots, deny-read globs, and platform-default/minimal read behavior stay in the runtime permissions model. ## What changed - Removes `ReadOnlyAccess` from `codex_protocol::protocol::SandboxPolicy`, including the generated `access` and `readOnlyAccess` API fields. - Updates legacy policy/profile conversions so restricted filesystem reads are represented only by `FileSystemSandboxPolicy` / `PermissionProfile` entries. - Keeps app-server v2 compatible with legacy `fullAccess` read-access payloads by accepting and ignoring that no-op shape, while rejecting legacy `restricted` read-access payloads instead of silently widening them to full-read legacy policies. - Carries Windows sandbox platform-default read behavior with an explicit override flag instead of depending on `ReadOnlyAccess::Restricted`. - Refreshes generated app-server schema/types and updates tests/docs for the simplified legacy policy shape. ## Verification - `cargo check -p codex-app-server-protocol --tests` - `cargo check -p codex-windows-sandbox --tests` - `cargo test -p codex-app-server-protocol sandbox_policy_` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19449). * #19395 * #19394 * #19393 * #19392 * #19391 * __->__ #19449
This commit is contained in:
Michael Bolin
@@ -83,7 +83,6 @@ use codex_protocol::protocol::PatchApplyStatus as CorePatchApplyStatus;
|
||||
use codex_protocol::protocol::RateLimitReachedType as CoreRateLimitReachedType;
|
||||
use codex_protocol::protocol::RateLimitSnapshot as CoreRateLimitSnapshot;
|
||||
use codex_protocol::protocol::RateLimitWindow as CoreRateLimitWindow;
|
||||
use codex_protocol::protocol::ReadOnlyAccess as CoreReadOnlyAccess;
|
||||
use codex_protocol::protocol::RealtimeAudioFrame as CoreRealtimeAudioFrame;
|
||||
use codex_protocol::protocol::RealtimeConversationVersion;
|
||||
use codex_protocol::protocol::RealtimeOutputModality;
|
||||
@@ -808,10 +807,6 @@ const fn default_enabled() -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
const fn default_include_platform_defaults() -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS, ExperimentalApi)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
#[ts(export_to = "v2/")]
|
||||
@@ -1719,54 +1714,7 @@ pub enum NetworkAccess {
|
||||
Enabled,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Default, Clone, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(tag = "type", rename_all = "camelCase")]
|
||||
#[ts(tag = "type")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub enum ReadOnlyAccess {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
Restricted {
|
||||
#[serde(default = "default_include_platform_defaults")]
|
||||
include_platform_defaults: bool,
|
||||
#[serde(default)]
|
||||
readable_roots: Vec<AbsolutePathBuf>,
|
||||
},
|
||||
#[default]
|
||||
FullAccess,
|
||||
}
|
||||
|
||||
impl ReadOnlyAccess {
|
||||
pub fn to_core(&self) -> CoreReadOnlyAccess {
|
||||
match self {
|
||||
ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults,
|
||||
readable_roots,
|
||||
} => CoreReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: *include_platform_defaults,
|
||||
readable_roots: readable_roots.clone(),
|
||||
},
|
||||
ReadOnlyAccess::FullAccess => CoreReadOnlyAccess::FullAccess,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl From<CoreReadOnlyAccess> for ReadOnlyAccess {
|
||||
fn from(value: CoreReadOnlyAccess) -> Self {
|
||||
match value {
|
||||
CoreReadOnlyAccess::Restricted {
|
||||
include_platform_defaults,
|
||||
readable_roots,
|
||||
} => ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults,
|
||||
readable_roots,
|
||||
},
|
||||
CoreReadOnlyAccess::FullAccess => ReadOnlyAccess::FullAccess,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[derive(Serialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(tag = "type", rename_all = "camelCase")]
|
||||
#[ts(tag = "type")]
|
||||
#[ts(export_to = "v2/")]
|
||||
@@ -1775,8 +1723,6 @@ pub enum SandboxPolicy {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
ReadOnly {
|
||||
#[serde(default)]
|
||||
access: ReadOnlyAccess,
|
||||
#[serde(default)]
|
||||
network_access: bool,
|
||||
},
|
||||
@@ -1792,7 +1738,36 @@ pub enum SandboxPolicy {
|
||||
#[serde(default)]
|
||||
writable_roots: Vec<AbsolutePathBuf>,
|
||||
#[serde(default)]
|
||||
read_only_access: ReadOnlyAccess,
|
||||
network_access: bool,
|
||||
#[serde(default)]
|
||||
exclude_tmpdir_env_var: bool,
|
||||
#[serde(default)]
|
||||
exclude_slash_tmp: bool,
|
||||
},
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
#[serde(tag = "type", rename_all = "camelCase")]
|
||||
enum SandboxPolicyDeserialize {
|
||||
DangerFullAccess,
|
||||
#[serde(rename_all = "camelCase")]
|
||||
ReadOnly {
|
||||
#[serde(default)]
|
||||
network_access: bool,
|
||||
#[serde(default)]
|
||||
access: Option<LegacyReadOnlyAccess>,
|
||||
},
|
||||
#[serde(rename_all = "camelCase")]
|
||||
ExternalSandbox {
|
||||
#[serde(default)]
|
||||
network_access: NetworkAccess,
|
||||
},
|
||||
#[serde(rename_all = "camelCase")]
|
||||
WorkspaceWrite {
|
||||
#[serde(default)]
|
||||
writable_roots: Vec<AbsolutePathBuf>,
|
||||
#[serde(default)]
|
||||
read_only_access: Option<LegacyReadOnlyAccess>,
|
||||
#[serde(default)]
|
||||
network_access: bool,
|
||||
#[serde(default)]
|
||||
@@ -1802,19 +1777,68 @@ pub enum SandboxPolicy {
|
||||
},
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
#[serde(tag = "type", rename_all = "camelCase")]
|
||||
enum LegacyReadOnlyAccess {
|
||||
FullAccess,
|
||||
Restricted,
|
||||
}
|
||||
|
||||
impl<'de> Deserialize<'de> for SandboxPolicy {
|
||||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
|
||||
where
|
||||
D: serde::Deserializer<'de>,
|
||||
{
|
||||
match SandboxPolicyDeserialize::deserialize(deserializer)? {
|
||||
SandboxPolicyDeserialize::DangerFullAccess => Ok(SandboxPolicy::DangerFullAccess),
|
||||
SandboxPolicyDeserialize::ReadOnly {
|
||||
network_access,
|
||||
access,
|
||||
} => {
|
||||
if matches!(access, Some(LegacyReadOnlyAccess::Restricted)) {
|
||||
return Err(serde::de::Error::custom(
|
||||
"readOnly.access is no longer supported; use permissionProfile for restricted reads",
|
||||
));
|
||||
}
|
||||
Ok(SandboxPolicy::ReadOnly { network_access })
|
||||
}
|
||||
SandboxPolicyDeserialize::ExternalSandbox { network_access } => {
|
||||
Ok(SandboxPolicy::ExternalSandbox { network_access })
|
||||
}
|
||||
SandboxPolicyDeserialize::WorkspaceWrite {
|
||||
writable_roots,
|
||||
read_only_access,
|
||||
network_access,
|
||||
exclude_tmpdir_env_var,
|
||||
exclude_slash_tmp,
|
||||
} => {
|
||||
if matches!(read_only_access, Some(LegacyReadOnlyAccess::Restricted)) {
|
||||
return Err(serde::de::Error::custom(
|
||||
"workspaceWrite.readOnlyAccess is no longer supported; use permissionProfile for restricted reads",
|
||||
));
|
||||
}
|
||||
Ok(SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots,
|
||||
network_access,
|
||||
exclude_tmpdir_env_var,
|
||||
exclude_slash_tmp,
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl SandboxPolicy {
|
||||
pub fn to_core(&self) -> codex_protocol::protocol::SandboxPolicy {
|
||||
match self {
|
||||
SandboxPolicy::DangerFullAccess => {
|
||||
codex_protocol::protocol::SandboxPolicy::DangerFullAccess
|
||||
}
|
||||
SandboxPolicy::ReadOnly {
|
||||
access,
|
||||
network_access,
|
||||
} => codex_protocol::protocol::SandboxPolicy::ReadOnly {
|
||||
access: access.to_core(),
|
||||
network_access: *network_access,
|
||||
},
|
||||
SandboxPolicy::ReadOnly { network_access } => {
|
||||
codex_protocol::protocol::SandboxPolicy::ReadOnly {
|
||||
network_access: *network_access,
|
||||
}
|
||||
}
|
||||
SandboxPolicy::ExternalSandbox { network_access } => {
|
||||
codex_protocol::protocol::SandboxPolicy::ExternalSandbox {
|
||||
network_access: match network_access {
|
||||
@@ -1825,13 +1849,11 @@ impl SandboxPolicy {
|
||||
}
|
||||
SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots,
|
||||
read_only_access,
|
||||
network_access,
|
||||
exclude_tmpdir_env_var,
|
||||
exclude_slash_tmp,
|
||||
} => codex_protocol::protocol::SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: writable_roots.clone(),
|
||||
read_only_access: read_only_access.to_core(),
|
||||
network_access: *network_access,
|
||||
exclude_tmpdir_env_var: *exclude_tmpdir_env_var,
|
||||
exclude_slash_tmp: *exclude_slash_tmp,
|
||||
@@ -1846,13 +1868,9 @@ impl From<codex_protocol::protocol::SandboxPolicy> for SandboxPolicy {
|
||||
codex_protocol::protocol::SandboxPolicy::DangerFullAccess => {
|
||||
SandboxPolicy::DangerFullAccess
|
||||
}
|
||||
codex_protocol::protocol::SandboxPolicy::ReadOnly {
|
||||
access,
|
||||
network_access,
|
||||
} => SandboxPolicy::ReadOnly {
|
||||
access: ReadOnlyAccess::from(access),
|
||||
network_access,
|
||||
},
|
||||
codex_protocol::protocol::SandboxPolicy::ReadOnly { network_access } => {
|
||||
SandboxPolicy::ReadOnly { network_access }
|
||||
}
|
||||
codex_protocol::protocol::SandboxPolicy::ExternalSandbox { network_access } => {
|
||||
SandboxPolicy::ExternalSandbox {
|
||||
network_access: match network_access {
|
||||
@@ -1863,13 +1881,11 @@ impl From<codex_protocol::protocol::SandboxPolicy> for SandboxPolicy {
|
||||
}
|
||||
codex_protocol::protocol::SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots,
|
||||
read_only_access,
|
||||
network_access,
|
||||
exclude_tmpdir_env_var,
|
||||
exclude_slash_tmp,
|
||||
} => SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots,
|
||||
read_only_access: ReadOnlyAccess::from(read_only_access),
|
||||
network_access,
|
||||
exclude_tmpdir_env_var,
|
||||
exclude_slash_tmp,
|
||||
@@ -7578,7 +7594,6 @@ mod tests {
|
||||
use codex_protocol::items::WebSearchItem;
|
||||
use codex_protocol::models::WebSearchAction as CoreWebSearchAction;
|
||||
use codex_protocol::protocol::NetworkAccess as CoreNetworkAccess;
|
||||
use codex_protocol::protocol::ReadOnlyAccess as CoreReadOnlyAccess;
|
||||
use codex_protocol::user_input::UserInput as CoreUserInput;
|
||||
use codex_utils_absolute_path::test_support::PathBufExt;
|
||||
use codex_utils_absolute_path::test_support::test_path_buf;
|
||||
@@ -8784,13 +8799,8 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sandbox_policy_round_trips_read_only_access() {
|
||||
let readable_root = test_absolute_path();
|
||||
fn sandbox_policy_round_trips_read_only_network_access() {
|
||||
let v2_policy = SandboxPolicy::ReadOnly {
|
||||
access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: vec![readable_root.clone()],
|
||||
},
|
||||
network_access: true,
|
||||
};
|
||||
|
||||
@@ -8798,10 +8808,6 @@ mod tests {
|
||||
assert_eq!(
|
||||
core_policy,
|
||||
codex_protocol::protocol::SandboxPolicy::ReadOnly {
|
||||
access: CoreReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: vec![readable_root],
|
||||
},
|
||||
network_access: true,
|
||||
}
|
||||
);
|
||||
@@ -9425,14 +9431,9 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sandbox_policy_round_trips_workspace_write_read_only_access() {
|
||||
let readable_root = test_absolute_path();
|
||||
fn sandbox_policy_round_trips_workspace_write_access() {
|
||||
let v2_policy = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
read_only_access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: vec![readable_root.clone()],
|
||||
},
|
||||
network_access: true,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
@@ -9443,10 +9444,6 @@ mod tests {
|
||||
core_policy,
|
||||
codex_protocol::protocol::SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
read_only_access: CoreReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: vec![readable_root],
|
||||
},
|
||||
network_access: true,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
@@ -9458,40 +9455,78 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sandbox_policy_deserializes_legacy_read_only_without_access_field() {
|
||||
let policy: SandboxPolicy = serde_json::from_value(json!({
|
||||
"type": "readOnly"
|
||||
fn sandbox_policy_deserializes_legacy_read_only_full_access_field() {
|
||||
let policy = serde_json::from_value::<SandboxPolicy>(json!({
|
||||
"type": "readOnly",
|
||||
"access": {
|
||||
"type": "fullAccess"
|
||||
},
|
||||
"networkAccess": true
|
||||
}))
|
||||
.expect("read-only policy should deserialize");
|
||||
.expect("read-only policy should ignore legacy fullAccess field");
|
||||
assert_eq!(
|
||||
policy,
|
||||
SandboxPolicy::ReadOnly {
|
||||
access: ReadOnlyAccess::FullAccess,
|
||||
network_access: false,
|
||||
network_access: true
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sandbox_policy_deserializes_legacy_workspace_write_without_read_only_access_field() {
|
||||
let policy: SandboxPolicy = serde_json::from_value(json!({
|
||||
fn sandbox_policy_deserializes_legacy_workspace_write_full_access_field() {
|
||||
let writable_root = absolute_path("/workspace");
|
||||
let policy = serde_json::from_value::<SandboxPolicy>(json!({
|
||||
"type": "workspaceWrite",
|
||||
"writableRoots": [writable_root],
|
||||
"readOnlyAccess": {
|
||||
"type": "fullAccess"
|
||||
},
|
||||
"networkAccess": true,
|
||||
"excludeTmpdirEnvVar": true,
|
||||
"excludeSlashTmp": true
|
||||
}))
|
||||
.expect("workspace-write policy should ignore legacy fullAccess field");
|
||||
assert_eq!(
|
||||
policy,
|
||||
SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![absolute_path("/workspace")],
|
||||
network_access: true,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sandbox_policy_rejects_legacy_read_only_restricted_access_field() {
|
||||
let err = serde_json::from_value::<SandboxPolicy>(json!({
|
||||
"type": "readOnly",
|
||||
"access": {
|
||||
"type": "restricted",
|
||||
"includePlatformDefaults": false,
|
||||
"readableRoots": []
|
||||
}
|
||||
}))
|
||||
.expect_err("read-only policy should reject removed restricted access field");
|
||||
assert!(err.to_string().contains("readOnly.access"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sandbox_policy_rejects_legacy_workspace_write_restricted_read_access_field() {
|
||||
let err = serde_json::from_value::<SandboxPolicy>(json!({
|
||||
"type": "workspaceWrite",
|
||||
"writableRoots": [],
|
||||
"readOnlyAccess": {
|
||||
"type": "restricted",
|
||||
"includePlatformDefaults": false,
|
||||
"readableRoots": []
|
||||
},
|
||||
"networkAccess": false,
|
||||
"excludeTmpdirEnvVar": false,
|
||||
"excludeSlashTmp": false
|
||||
}))
|
||||
.expect("workspace-write policy should deserialize");
|
||||
assert_eq!(
|
||||
policy,
|
||||
SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
read_only_access: ReadOnlyAccess::FullAccess,
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
}
|
||||
);
|
||||
.expect_err("workspace-write policy should reject removed restricted readOnlyAccess field");
|
||||
assert!(err.to_string().contains("workspaceWrite.readOnlyAccess"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user