mirror of
https://github.com/openai/codex.git
synced 2026-11-12 21:20:34 +00:00
Normalize permission updates at session settings admission (#61134)
## What changed Move permission normalization into `Session::apply_session_settings`, leaving `SessionConfiguration::apply` to install the normalized snapshot. Preserve existing deny-read restrictions and infer active profile identity before merging those restrictions. Convert legacy `sandbox_policy` updates into permission profiles using the updated working directory, clearing active profile identity and profile workspace roots. Normalize a copy so previews preserve the original compatibility input for subsequent admission. ## Testing Extend session tests to cover deny-read preservation for both update forms, Git metadata protection when changing the working directory, and preview/commit behavior for legacy updates and active profile identity. GitOrigin-RevId: 8b3a0f630ab56aca6ad75cf16ad432f7fdc08255
This commit is contained in:
@@ -10,6 +10,10 @@ use codex_features::Feature;
|
||||
use codex_protocol::capabilities::CapabilityRootLocation;
|
||||
use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::error::Result as CodexResult;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::SandboxEnforcement;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::AgentStatus;
|
||||
use codex_protocol::protocol::EnvironmentConfig;
|
||||
use codex_protocol::protocol::EnvironmentConfigState;
|
||||
@@ -183,7 +187,52 @@ impl Session {
|
||||
ensure_configs_stay_owner_provided(current_environments, &environments.environments)?;
|
||||
}
|
||||
|
||||
current.apply(updates, current_environments)
|
||||
if updates.permission_profile.is_none() && updates.sandbox_policy.is_none() {
|
||||
return current.apply(updates, current_environments);
|
||||
}
|
||||
|
||||
// Normalize a copy so preview leaves compatibility input available for admission.
|
||||
let mut updates = updates.clone();
|
||||
let sandbox_policy = updates.sandbox_policy.take();
|
||||
let current_file_system = current.file_system_sandbox_policy(current_environments);
|
||||
if let Some(profile) = updates.permission_profile.take() {
|
||||
// Infer identity from the requested profile before merging current denials.
|
||||
updates.active_permission_profile = updates.active_permission_profile.or_else(|| {
|
||||
(profile == current.permission_profile())
|
||||
.then(|| current.active_permission_profile())
|
||||
.flatten()
|
||||
});
|
||||
let (mut file_system, network) = profile.to_runtime_permissions();
|
||||
file_system.preserve_deny_read_restrictions_from(¤t_file_system);
|
||||
updates.permission_profile = Some(
|
||||
PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
profile.enforcement(),
|
||||
&file_system,
|
||||
network,
|
||||
),
|
||||
);
|
||||
} else if let Some(policy) = sandbox_policy {
|
||||
let cwd = updates.environments.as_ref().map_or_else(
|
||||
|| current.cwd(),
|
||||
|environments| &environments.legacy_fallback_cwd,
|
||||
);
|
||||
let file_system =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_preserving_deny_entries(
|
||||
&policy,
|
||||
cwd,
|
||||
¤t_file_system,
|
||||
);
|
||||
updates.permission_profile = Some(
|
||||
PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
SandboxEnforcement::from_legacy_sandbox_policy(&policy),
|
||||
&file_system,
|
||||
NetworkSandboxPolicy::from(&policy),
|
||||
),
|
||||
);
|
||||
updates.active_permission_profile = None;
|
||||
updates.profile_workspace_roots = Some(Vec::new());
|
||||
}
|
||||
current.apply(&updates, current_environments)
|
||||
}
|
||||
|
||||
/// Activates the environments accepted for a new task. Configuration may have arrived for
|
||||
|
||||
@@ -433,6 +433,7 @@ impl SessionConfiguration {
|
||||
}
|
||||
}
|
||||
|
||||
/// Applies fields normalized against this configuration at admission.
|
||||
pub(super) fn apply(
|
||||
&self,
|
||||
updates: &SessionSettingsUpdate,
|
||||
@@ -481,20 +482,17 @@ impl SessionConfiguration {
|
||||
}
|
||||
|
||||
if let Some(permission_profile) = updates.permission_profile.clone() {
|
||||
let active_permission_profile =
|
||||
updates.active_permission_profile.clone().or_else(|| {
|
||||
if permission_profile == self.permission_profile() {
|
||||
self.active_permission_profile()
|
||||
} else {
|
||||
None
|
||||
}
|
||||
});
|
||||
next_configuration.set_permission_profile_projection(
|
||||
permission_profile,
|
||||
active_permission_profile,
|
||||
updates.profile_workspace_roots.clone().unwrap_or_default(),
|
||||
Some(¤t_file_system_sandbox_policy),
|
||||
)?;
|
||||
let snapshot = match updates.active_permission_profile.clone() {
|
||||
Some(active) => PermissionProfileSnapshot::active_with_profile_workspace_roots(
|
||||
permission_profile,
|
||||
active,
|
||||
updates.profile_workspace_roots.clone().unwrap_or_default(),
|
||||
),
|
||||
None => PermissionProfileSnapshot::legacy(permission_profile),
|
||||
};
|
||||
next_configuration
|
||||
.permission_profile_state
|
||||
.set_permission_profile_snapshot(snapshot)?;
|
||||
if let Some(active_permission_profile) = next_configuration.active_permission_profile()
|
||||
{
|
||||
let mut config = (*next_configuration.original_config_do_not_use).clone();
|
||||
@@ -522,23 +520,6 @@ impl SessionConfiguration {
|
||||
)?;
|
||||
next_configuration.original_config_do_not_use = Arc::new(config);
|
||||
}
|
||||
} else if let Some(sandbox_policy) = updates.sandbox_policy.clone() {
|
||||
let file_system_sandbox_policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_preserving_deny_entries(
|
||||
&sandbox_policy,
|
||||
next_configuration.cwd(),
|
||||
¤t_file_system_sandbox_policy,
|
||||
);
|
||||
let network_sandbox_policy = NetworkSandboxPolicy::from(&sandbox_policy);
|
||||
next_configuration
|
||||
.permission_profile_state
|
||||
.set_legacy_permission_profile(
|
||||
PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
SandboxEnforcement::from_legacy_sandbox_policy(&sandbox_policy),
|
||||
&file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
),
|
||||
)?;
|
||||
} else if cwd_changed && file_system_policy_has_rebindable_project_root_write {
|
||||
// Compatibility projection can resolve filesystem paths. Only compute it
|
||||
// when a cwd-bound legacy policy might need rebinding.
|
||||
@@ -624,42 +605,6 @@ impl SessionConfiguration {
|
||||
}
|
||||
Ok(next_configuration)
|
||||
}
|
||||
|
||||
fn set_permission_profile_projection(
|
||||
&mut self,
|
||||
permission_profile: PermissionProfile,
|
||||
active_permission_profile: Option<ActivePermissionProfile>,
|
||||
profile_workspace_roots: Vec<ProfileWorkspaceRoot>,
|
||||
preserve_deny_reads_from: Option<&FileSystemSandboxPolicy>,
|
||||
) -> ConstraintResult<()> {
|
||||
let enforcement = permission_profile.enforcement();
|
||||
let (mut file_system_sandbox_policy, network_sandbox_policy) =
|
||||
permission_profile.to_runtime_permissions();
|
||||
if let Some(existing_file_system_policy) = preserve_deny_reads_from {
|
||||
file_system_sandbox_policy
|
||||
.preserve_deny_read_restrictions_from(existing_file_system_policy);
|
||||
}
|
||||
let effective_permission_profile =
|
||||
PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
enforcement,
|
||||
&file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
);
|
||||
|
||||
let permission_snapshot = match active_permission_profile {
|
||||
Some(active_permission_profile) => {
|
||||
PermissionProfileSnapshot::active_with_profile_workspace_roots(
|
||||
effective_permission_profile,
|
||||
active_permission_profile,
|
||||
profile_workspace_roots,
|
||||
)
|
||||
}
|
||||
None => PermissionProfileSnapshot::legacy(effective_permission_profile),
|
||||
};
|
||||
|
||||
self.permission_profile_state
|
||||
.set_permission_profile_snapshot(permission_snapshot)
|
||||
}
|
||||
}
|
||||
|
||||
/// The configuration and public snapshot published by one settings commit.
|
||||
@@ -679,6 +624,7 @@ pub(crate) struct SessionSettingsUpdate {
|
||||
pub(crate) environments: Option<TurnEnvironmentSelections>,
|
||||
pub(crate) runtime_workspace_roots: Option<Vec<AbsolutePathBuf>>,
|
||||
pub(crate) profile_workspace_roots: Option<Vec<ProfileWorkspaceRoot>>,
|
||||
/// Compatibility input consumed when normalizing settings at admission.
|
||||
pub(crate) sandbox_policy: Option<SandboxPolicy>,
|
||||
pub(crate) permission_profile: Option<PermissionProfile>,
|
||||
pub(crate) active_permission_profile: Option<ActivePermissionProfile>,
|
||||
|
||||
@@ -5538,11 +5538,19 @@ async fn session_configuration_apply_preserves_profile_file_system_policy_on_cwd
|
||||
|
||||
#[tokio::test]
|
||||
async fn session_configuration_apply_permission_profile_preserves_existing_deny_read_entries() {
|
||||
let mut session_configuration = make_session_configuration_for_tests().await;
|
||||
let cwd = tempfile::tempdir().expect("create temp dir");
|
||||
session_configuration.legacy_fallback_cwd = cwd.path().abs();
|
||||
const GLOB_SCAN_MAX_DEPTH: usize = 2;
|
||||
|
||||
let workspace_policy = SandboxPolicy::new_workspace_write_policy();
|
||||
let (session, _turn_context) = make_session_and_context().await;
|
||||
let mut config = session.state.lock().await.session_configuration.clone();
|
||||
let cwd = tempfile::tempdir().expect("create temp dir");
|
||||
config.legacy_fallback_cwd = cwd.path().abs();
|
||||
|
||||
let workspace_policy = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![config.cwd().clone()],
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
};
|
||||
let deny_entry = FileSystemSandboxEntry {
|
||||
path: FileSystemPath::GlobPattern {
|
||||
pattern: "**/*.env".to_string(),
|
||||
@@ -5550,48 +5558,136 @@ async fn session_configuration_apply_permission_profile_preserves_existing_deny_
|
||||
access: FileSystemAccessMode::Deny,
|
||||
missing_path_behavior: None,
|
||||
};
|
||||
let mut existing_file_system_policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(
|
||||
&workspace_policy,
|
||||
session_configuration.cwd().as_path(),
|
||||
);
|
||||
existing_file_system_policy.glob_scan_max_depth = Some(2);
|
||||
existing_file_system_policy.entries.push(deny_entry.clone());
|
||||
session_configuration
|
||||
.set_permission_profile_for_tests(
|
||||
PermissionProfile::from_runtime_permissions_with_enforcement(
|
||||
SandboxEnforcement::from_legacy_sandbox_policy(&workspace_policy),
|
||||
&existing_file_system_policy,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
),
|
||||
)
|
||||
.expect("set permission profile");
|
||||
|
||||
let requested_file_system_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(
|
||||
let requested = FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(
|
||||
&workspace_policy,
|
||||
session_configuration.cwd().as_path(),
|
||||
config.cwd().as_path(),
|
||||
);
|
||||
let permission_profile = codex_protocol::models::PermissionProfile::from_runtime_permissions(
|
||||
&requested_file_system_policy,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
);
|
||||
let updated = session_configuration
|
||||
.apply(
|
||||
let mut existing = requested.clone();
|
||||
existing.glob_scan_max_depth = Some(GLOB_SCAN_MAX_DEPTH);
|
||||
existing.entries.push(deny_entry.clone());
|
||||
config
|
||||
.set_permission_profile_for_tests(PermissionProfile::from_runtime_permissions(
|
||||
&existing,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
))
|
||||
.unwrap();
|
||||
|
||||
let permission_profile =
|
||||
PermissionProfile::from_runtime_permissions(&requested, NetworkSandboxPolicy::Restricted);
|
||||
let mut expected = requested.materialize_project_roots_with_workspace_roots(&[]);
|
||||
expected.glob_scan_max_depth = Some(GLOB_SCAN_MAX_DEPTH);
|
||||
expected.entries.push(deny_entry);
|
||||
for updates in [
|
||||
SessionSettingsUpdate {
|
||||
permission_profile: Some(permission_profile),
|
||||
..Default::default()
|
||||
},
|
||||
SessionSettingsUpdate {
|
||||
sandbox_policy: Some(workspace_policy.clone()),
|
||||
..Default::default()
|
||||
},
|
||||
] {
|
||||
let updated = session.apply_session_settings(&config, &updates).unwrap();
|
||||
assert_eq!(updated.file_system_sandbox_policy(&[]), expected);
|
||||
}
|
||||
let next_cwd = tempfile::tempdir().unwrap();
|
||||
let git_dir = cwd.path().join("git-metadata");
|
||||
std::fs::create_dir(&git_dir).unwrap();
|
||||
std::fs::write(
|
||||
next_cwd.path().join(".git"),
|
||||
format!("gitdir: {}\n", git_dir.display()),
|
||||
)
|
||||
.unwrap();
|
||||
let updated = session
|
||||
.apply_session_settings(
|
||||
&config,
|
||||
&SessionSettingsUpdate {
|
||||
permission_profile: Some(permission_profile),
|
||||
environments: Some(TurnEnvironmentSelections::new(
|
||||
next_cwd.path().abs(),
|
||||
Vec::new(),
|
||||
)),
|
||||
sandbox_policy: Some(workspace_policy),
|
||||
..Default::default()
|
||||
},
|
||||
&[],
|
||||
)
|
||||
.expect("permission profile update should succeed");
|
||||
|
||||
let mut expected_file_system_policy =
|
||||
requested_file_system_policy.materialize_project_roots_with_workspace_roots(&[]);
|
||||
expected_file_system_policy.glob_scan_max_depth = Some(2);
|
||||
expected_file_system_policy.entries.push(deny_entry);
|
||||
.unwrap();
|
||||
let filesystem = updated.file_system_sandbox_policy(&[]);
|
||||
assert_eq!(
|
||||
updated.file_system_sandbox_policy(&[]),
|
||||
expected_file_system_policy
|
||||
(
|
||||
updated.cwd(),
|
||||
filesystem.can_write_local_path_with_cwd(&git_dir, next_cwd.path()),
|
||||
filesystem
|
||||
.can_write_local_path_with_cwd(&cwd.path().join("ordinary-file"), next_cwd.path()),
|
||||
),
|
||||
(&next_cwd.path().abs(), false, true)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn session_settings_preview_preserves_legacy_input_and_identity_clearing() {
|
||||
let (session, context) = make_session_and_context().await;
|
||||
let profile = PermissionProfile::read_only();
|
||||
let selected = PermissionProfileSnapshot::active_with_profile_workspace_roots(
|
||||
profile.clone(),
|
||||
ActivePermissionProfile::read_only(),
|
||||
vec![
|
||||
context
|
||||
.initial_environments
|
||||
.primary()
|
||||
.unwrap()
|
||||
.cwd()
|
||||
.clone()
|
||||
.into(),
|
||||
],
|
||||
);
|
||||
session
|
||||
.state
|
||||
.lock()
|
||||
.await
|
||||
.session_configuration
|
||||
.permission_profile_state
|
||||
.set_permission_profile_snapshot(selected.clone())
|
||||
.unwrap();
|
||||
let canonical = session
|
||||
.update_settings(SessionSettingsUpdate {
|
||||
permission_profile: Some(profile.clone()),
|
||||
profile_workspace_roots: Some(selected.profile_workspace_roots().to_vec()),
|
||||
..Default::default()
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
canonical.configuration.permission_profile_state.snapshot(),
|
||||
selected
|
||||
);
|
||||
|
||||
let legacy = SessionSettingsUpdate {
|
||||
sandbox_policy: Some(SandboxPolicy::new_read_only_policy()),
|
||||
..Default::default()
|
||||
};
|
||||
let preview = session.preview_settings(&legacy).await.unwrap();
|
||||
assert_eq!(
|
||||
(
|
||||
preview.permission_profile,
|
||||
preview.active_permission_profile,
|
||||
preview.profile_workspace_roots
|
||||
),
|
||||
(profile.clone(), None, Vec::new()),
|
||||
);
|
||||
assert_eq!(
|
||||
session
|
||||
.state
|
||||
.lock()
|
||||
.await
|
||||
.session_configuration
|
||||
.permission_profile_state
|
||||
.snapshot(),
|
||||
selected
|
||||
);
|
||||
let committed = session.update_settings(legacy).await.unwrap();
|
||||
assert_eq!(
|
||||
committed.configuration.permission_profile_state.snapshot(),
|
||||
PermissionProfileSnapshot::legacy(profile),
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user