mirror of
https://github.com/openai/codex.git
synced 2026-09-08 15:50:34 +00:00
fix: address sandbox violation review feedback
Co-authored-by: Codex noreply@openai.com
This commit is contained in:
@@ -31,6 +31,7 @@ use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_sandboxing::SandboxType;
|
||||
use codex_sandboxing::SandboxablePreference;
|
||||
use codex_sandboxing::policy_transforms::effective_permission_profile;
|
||||
use codex_sandboxing::record_filesystem_sandbox_violation;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use futures::future::BoxFuture;
|
||||
use std::path::PathBuf;
|
||||
@@ -267,6 +268,7 @@ impl ToolRuntime<ApplyPatchRequest, ApplyPatchRuntimeOutput> for ApplyPatchRunti
|
||||
timed_out: false,
|
||||
};
|
||||
if failed && is_likely_sandbox_denied(attempt.sandbox, &output) {
|
||||
record_filesystem_sandbox_violation(attempt.sandbox, &output);
|
||||
return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
|
||||
output: Box::new(output),
|
||||
network_policy_decision: None,
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use crate::SandboxType;
|
||||
use codex_network_proxy::BlockedRequest;
|
||||
use codex_network_proxy::NetworkMode;
|
||||
use codex_protocol::exec_output::ExecToolCallOutput;
|
||||
use tracing::warn;
|
||||
|
||||
@@ -86,6 +87,7 @@ pub struct NetworkSandboxViolation {
|
||||
pub reason: String,
|
||||
pub client: Option<String>,
|
||||
pub method: Option<String>,
|
||||
pub mode: Option<NetworkMode>,
|
||||
pub protocol: String,
|
||||
pub decision: Option<String>,
|
||||
pub source: Option<String>,
|
||||
@@ -100,6 +102,7 @@ impl NetworkSandboxViolation {
|
||||
reason: blocked.reason.clone(),
|
||||
client: blocked.client.clone(),
|
||||
method: blocked.method.clone(),
|
||||
mode: blocked.mode,
|
||||
protocol: blocked.protocol.clone(),
|
||||
decision: blocked.decision.clone(),
|
||||
source: blocked.source.clone(),
|
||||
@@ -179,12 +182,13 @@ pub fn record_sandbox_violation(event: &SandboxViolationEvent) {
|
||||
}
|
||||
SandboxViolationEvent::Network(violation) => {
|
||||
warn!(
|
||||
"recorded sandbox violation: resource=network protocol={} host={} port={:?} reason={} method={:?} client={:?} decision={:?} source={:?}",
|
||||
"recorded sandbox violation: resource=network protocol={} host={} port={:?} reason={} method={:?} mode={:?} client={:?} decision={:?} source={:?}",
|
||||
violation.protocol,
|
||||
violation.host,
|
||||
violation.port,
|
||||
violation.reason,
|
||||
violation.method,
|
||||
violation.mode,
|
||||
violation.client,
|
||||
violation.decision,
|
||||
violation.source
|
||||
@@ -228,9 +232,13 @@ fn extract_denied_path_from_text(text: &str) -> Option<String> {
|
||||
];
|
||||
|
||||
for line in text.lines() {
|
||||
let lower = line.to_lowercase();
|
||||
for marker in PATH_MARKERS {
|
||||
let Some(marker_start) = lower.find(marker) else {
|
||||
let Some(marker_start) = line.match_indices(':').find_map(|(marker_start, _)| {
|
||||
line.get(marker_start..)
|
||||
.and_then(|suffix| suffix.get(..marker.len()))
|
||||
.is_some_and(|candidate| candidate.eq_ignore_ascii_case(marker))
|
||||
.then_some(marker_start)
|
||||
}) else {
|
||||
continue;
|
||||
};
|
||||
let candidate_prefix = &line[..marker_start];
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use super::*;
|
||||
use codex_network_proxy::BlockedRequest;
|
||||
use codex_network_proxy::BlockedRequestArgs;
|
||||
use codex_network_proxy::NetworkMode;
|
||||
use codex_protocol::exec_output::ExecToolCallOutput;
|
||||
use codex_protocol::exec_output::StreamOutput;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -97,6 +98,27 @@ fn classifies_filesystem_violation_with_path() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn classifies_filesystem_violation_with_unicode_before_marker() {
|
||||
let output = make_exec_output(
|
||||
/*exit_code*/ 1,
|
||||
"",
|
||||
"bash: /private/tmp/\u{130}-denied: Operation not permitted",
|
||||
"",
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
classify_filesystem_sandbox_violation(SandboxType::MacosSeatbelt, &output),
|
||||
Some(FileSystemSandboxViolation {
|
||||
sandbox_type: SandboxType::MacosSeatbelt,
|
||||
reason: FileSystemSandboxViolationReason::OperationNotPermitted,
|
||||
path: Some("/private/tmp/\u{130}-denied".to_string()),
|
||||
output_snippet: "bash: /private/tmp/\u{130}-denied: Operation not permitted"
|
||||
.to_string(),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn classifies_filesystem_violation_from_aggregated_output() {
|
||||
let output = make_exec_output(
|
||||
@@ -152,7 +174,7 @@ fn converts_blocked_request_to_network_violation() {
|
||||
reason: "not_allowed".to_string(),
|
||||
client: Some("curl".to_string()),
|
||||
method: Some("CONNECT".to_string()),
|
||||
mode: None,
|
||||
mode: Some(NetworkMode::Limited),
|
||||
protocol: "https".to_string(),
|
||||
decision: Some("block".to_string()),
|
||||
source: Some("policy".to_string()),
|
||||
@@ -166,6 +188,7 @@ fn converts_blocked_request_to_network_violation() {
|
||||
reason: "not_allowed".to_string(),
|
||||
client: Some("curl".to_string()),
|
||||
method: Some("CONNECT".to_string()),
|
||||
mode: Some(NetworkMode::Limited),
|
||||
protocol: "https".to_string(),
|
||||
decision: Some("block".to_string()),
|
||||
source: Some("policy".to_string()),
|
||||
|
||||
Reference in New Issue
Block a user