fix: follow up on linux sandbox review nits (#14440)
## Summary - address the follow-up review nits from #13996 in a separate PR - make the approvals test command a raw string and keep the managed-network path using env proxy routing - inline `--apply-seccomp-then-exec` in the Linux sandbox inner command builder - remove the bubblewrap-specific sandbox metric tag path and drop the `use_legacy_landlock` shim from `sandbox_tag`/`TurnMetadataState::new` - restore the `Feature` import that `origin/main` currently still needs in `connectors.rs` ## Testing - `cargo test -p codex-linux-sandbox` - focused `codex-core` tests were rerun/started, but the final verification pass was interrupted when I pushed at request
This commit is contained in:
viyatb-oai
GitHub
parent
04892b4ceb
commit
e99e8e4a6b
7 changed files with 20 additions and 71 deletions
|
|
@ -1290,7 +1290,6 @@ impl Session {
|
|||
cwd.clone(),
|
||||
session_configuration.sandbox_policy.get(),
|
||||
session_configuration.windows_sandbox_level,
|
||||
per_turn_config.features.use_legacy_landlock(),
|
||||
));
|
||||
let (current_date, timezone) = local_time_context();
|
||||
TurnContext {
|
||||
|
|
@ -5211,7 +5210,6 @@ async fn spawn_review_thread(
|
|||
parent_turn_context.cwd.clone(),
|
||||
parent_turn_context.sandbox_policy.get(),
|
||||
parent_turn_context.windows_sandbox_level,
|
||||
parent_turn_context.features.use_legacy_landlock(),
|
||||
));
|
||||
|
||||
let review_turn_context = TurnContext {
|
||||
|
|
|
|||
|
|
@ -31,6 +31,7 @@ use crate::config::types::AppsConfigToml;
|
|||
use crate::default_client::create_client;
|
||||
use crate::default_client::is_first_party_chat_originator;
|
||||
use crate::default_client::originator;
|
||||
use crate::features::Feature;
|
||||
use crate::mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use crate::mcp::McpManager;
|
||||
use crate::mcp::ToolPluginProvenance;
|
||||
|
|
|
|||
|
|
@ -6,7 +6,6 @@ use codex_protocol::config_types::WindowsSandboxLevel;
|
|||
pub(crate) fn sandbox_tag(
|
||||
policy: &SandboxPolicy,
|
||||
windows_sandbox_level: WindowsSandboxLevel,
|
||||
use_legacy_landlock: bool,
|
||||
) -> &'static str {
|
||||
if matches!(policy, SandboxPolicy::DangerFullAccess) {
|
||||
return "none";
|
||||
|
|
@ -18,9 +17,6 @@ pub(crate) fn sandbox_tag(
|
|||
{
|
||||
return "windows_elevated";
|
||||
}
|
||||
if cfg!(target_os = "linux") && !use_legacy_landlock {
|
||||
return "linux_bubblewrap";
|
||||
}
|
||||
|
||||
get_platform_sandbox(windows_sandbox_level != WindowsSandboxLevel::Disabled)
|
||||
.map(SandboxType::as_metric_tag)
|
||||
|
|
@ -38,41 +34,34 @@ mod tests {
|
|||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn danger_full_access_is_untagged_even_when_bubblewrap_is_default() {
|
||||
fn danger_full_access_is_untagged_even_when_linux_sandbox_defaults_apply() {
|
||||
let actual = sandbox_tag(
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
WindowsSandboxLevel::Disabled,
|
||||
false,
|
||||
);
|
||||
assert_eq!(actual, "none");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn external_sandbox_keeps_external_tag_when_bubblewrap_is_default() {
|
||||
fn external_sandbox_keeps_external_tag_when_linux_sandbox_defaults_apply() {
|
||||
let actual = sandbox_tag(
|
||||
&SandboxPolicy::ExternalSandbox {
|
||||
network_access: NetworkAccess::Enabled,
|
||||
},
|
||||
WindowsSandboxLevel::Disabled,
|
||||
false,
|
||||
);
|
||||
assert_eq!(actual, "external");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bubblewrap_default_sets_distinct_linux_tag() {
|
||||
fn default_linux_sandbox_uses_platform_sandbox_tag() {
|
||||
let actual = sandbox_tag(
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
WindowsSandboxLevel::Disabled,
|
||||
false,
|
||||
);
|
||||
let expected = if cfg!(target_os = "linux") {
|
||||
"linux_bubblewrap"
|
||||
} else {
|
||||
get_platform_sandbox(false)
|
||||
.map(SandboxType::as_metric_tag)
|
||||
.unwrap_or("none")
|
||||
};
|
||||
let expected = get_platform_sandbox(false)
|
||||
.map(SandboxType::as_metric_tag)
|
||||
.unwrap_or("none");
|
||||
assert_eq!(actual, expected);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -173,7 +173,6 @@ impl ToolRegistry {
|
|||
sandbox_tag(
|
||||
&invocation.turn.sandbox_policy,
|
||||
invocation.turn.windows_sandbox_level,
|
||||
invocation.turn.features.use_legacy_landlock(),
|
||||
),
|
||||
),
|
||||
(
|
||||
|
|
@ -498,12 +497,8 @@ async fn dispatch_after_tool_use_hook(
|
|||
success: dispatch.success,
|
||||
duration_ms: u64::try_from(dispatch.duration.as_millis()).unwrap_or(u64::MAX),
|
||||
mutating: dispatch.mutating,
|
||||
sandbox: sandbox_tag(
|
||||
&turn.sandbox_policy,
|
||||
turn.windows_sandbox_level,
|
||||
turn.features.use_legacy_landlock(),
|
||||
)
|
||||
.to_string(),
|
||||
sandbox: sandbox_tag(&turn.sandbox_policy, turn.windows_sandbox_level)
|
||||
.to_string(),
|
||||
sandbox_policy: sandbox_policy_tag(&turn.sandbox_policy).to_string(),
|
||||
output_preview: dispatch.output_preview.clone(),
|
||||
},
|
||||
|
|
|
|||
|
|
@ -132,12 +132,9 @@ impl TurnMetadataState {
|
|||
cwd: PathBuf,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
windows_sandbox_level: WindowsSandboxLevel,
|
||||
use_legacy_landlock: bool,
|
||||
) -> Self {
|
||||
let repo_root = get_git_repo_root(&cwd).map(|root| root.to_string_lossy().into_owned());
|
||||
let sandbox = Some(
|
||||
sandbox_tag(sandbox_policy, windows_sandbox_level, use_legacy_landlock).to_string(),
|
||||
);
|
||||
let sandbox = Some(sandbox_tag(sandbox_policy, windows_sandbox_level).to_string());
|
||||
let base_metadata = build_turn_metadata_bag(Some(turn_id), sandbox, None, None);
|
||||
let base_header = base_metadata
|
||||
.to_header_value()
|
||||
|
|
@ -295,53 +292,23 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn turn_metadata_state_respects_legacy_landlock_flag() {
|
||||
fn turn_metadata_state_uses_platform_sandbox_tag() {
|
||||
let temp_dir = TempDir::new().expect("temp dir");
|
||||
let cwd = temp_dir.path().to_path_buf();
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
|
||||
let default_bubblewrap = TurnMetadataState::new(
|
||||
let state = TurnMetadataState::new(
|
||||
"turn-a".to_string(),
|
||||
cwd.clone(),
|
||||
&sandbox_policy,
|
||||
WindowsSandboxLevel::Disabled,
|
||||
false,
|
||||
);
|
||||
let legacy_landlock = TurnMetadataState::new(
|
||||
"turn-b".to_string(),
|
||||
cwd,
|
||||
&sandbox_policy,
|
||||
WindowsSandboxLevel::Disabled,
|
||||
true,
|
||||
);
|
||||
|
||||
let default_bubblewrap_header = default_bubblewrap
|
||||
.current_header_value()
|
||||
.expect("default_bubblewrap_header");
|
||||
let legacy_landlock_header = legacy_landlock
|
||||
.current_header_value()
|
||||
.expect("legacy_landlock_header");
|
||||
let header = state.current_header_value().expect("header");
|
||||
let json: Value = serde_json::from_str(&header).expect("json");
|
||||
let sandbox_name = json.get("sandbox").and_then(Value::as_str);
|
||||
|
||||
let default_bubblewrap_json: Value =
|
||||
serde_json::from_str(&default_bubblewrap_header).expect("default_bubblewrap_json");
|
||||
let legacy_landlock_json: Value =
|
||||
serde_json::from_str(&legacy_landlock_header).expect("legacy_landlock_json");
|
||||
|
||||
let default_bubblewrap_sandbox = default_bubblewrap_json
|
||||
.get("sandbox")
|
||||
.and_then(Value::as_str);
|
||||
let legacy_landlock_sandbox = legacy_landlock_json.get("sandbox").and_then(Value::as_str);
|
||||
|
||||
let expected_default_bubblewrap =
|
||||
sandbox_tag(&sandbox_policy, WindowsSandboxLevel::Disabled, false);
|
||||
assert_eq!(
|
||||
default_bubblewrap_sandbox,
|
||||
Some(expected_default_bubblewrap)
|
||||
);
|
||||
|
||||
if cfg!(target_os = "linux") {
|
||||
assert_eq!(default_bubblewrap_sandbox, Some("linux_bubblewrap"));
|
||||
assert_ne!(default_bubblewrap_sandbox, legacy_landlock_sandbox);
|
||||
}
|
||||
let expected_sandbox = sandbox_tag(&sandbox_policy, WindowsSandboxLevel::Disabled);
|
||||
assert_eq!(sandbox_name, Some(expected_sandbox));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2298,9 +2298,8 @@ allow_local_binding = true
|
|||
let call_id_first = "allow-network-first";
|
||||
// Use urllib without overriding proxy settings so managed-network sessions
|
||||
// continue to exercise the env-based proxy routing path under bubblewrap.
|
||||
let fetch_command =
|
||||
"python3 -c \"import urllib.request; opener = urllib.request.build_opener(urllib.request.ProxyHandler()); print('OK:' + opener.open('http://codex-network-test.invalid', timeout=30).read().decode(errors='replace'))\""
|
||||
.to_string();
|
||||
let fetch_command = r#"python3 -c "import urllib.request; opener = urllib.request.build_opener(urllib.request.ProxyHandler()); print('OK:' + opener.open('http://codex-network-test.invalid', timeout=30).read().decode(errors='replace'))""#
|
||||
.to_string();
|
||||
let first_event = shell_event(
|
||||
call_id_first,
|
||||
&fetch_command,
|
||||
|
|
|
|||
|
|
@ -514,8 +514,8 @@ fn build_inner_seccomp_command(args: InnerSeccompCommandArgs<'_>) -> Vec<String>
|
|||
file_system_policy_json,
|
||||
"--network-sandbox-policy".to_string(),
|
||||
network_policy_json,
|
||||
"--apply-seccomp-then-exec".to_string(),
|
||||
];
|
||||
inner.push("--apply-seccomp-then-exec".to_string());
|
||||
if allow_network_for_proxy {
|
||||
inner.push("--allow-network-for-proxy".to_string());
|
||||
let proxy_route_spec = proxy_route_spec
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue