mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
fix(terminal-browser): approve human navigation network access
This commit is contained in:
@@ -396,7 +396,12 @@ pub async fn exec_approval(
|
||||
}
|
||||
match decision {
|
||||
ReviewDecision::Abort => {
|
||||
sess.interrupt_task().await;
|
||||
if !sess
|
||||
.notify_out_of_turn_network_approval(&approval_id, ReviewDecision::Abort)
|
||||
.await
|
||||
{
|
||||
sess.interrupt_task().await;
|
||||
}
|
||||
}
|
||||
other => sess.notify_approval(&approval_id, other).await,
|
||||
}
|
||||
|
||||
@@ -2187,17 +2187,30 @@ impl Session {
|
||||
let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone());
|
||||
// Add the tx_approve callback to the map before sending the request.
|
||||
let (tx_approve, rx_approve) = oneshot::channel();
|
||||
let mut tx_approve = Some(tx_approve);
|
||||
let prev_entry = {
|
||||
let mut active = self.active_turn.lock().await;
|
||||
match active.as_mut() {
|
||||
Some(at) => {
|
||||
let mut ts = at.turn_state.lock().await;
|
||||
ts.insert_pending_approval(effective_approval_id.clone(), tx_approve)
|
||||
tx_approve.take().and_then(|tx_approve| {
|
||||
ts.insert_pending_approval(effective_approval_id.clone(), tx_approve)
|
||||
})
|
||||
}
|
||||
None => None,
|
||||
}
|
||||
};
|
||||
if prev_entry.is_some() {
|
||||
let previous_out_of_turn = if network_approval_context.is_some()
|
||||
&& let Some(tx_approve) = tx_approve
|
||||
{
|
||||
self.services
|
||||
.network_approval
|
||||
.insert_out_of_turn_approval(effective_approval_id.clone(), tx_approve)
|
||||
.await
|
||||
} else {
|
||||
None
|
||||
};
|
||||
if prev_entry.is_some() || previous_out_of_turn.is_some() {
|
||||
warn!("Overwriting existing pending approval for call_id: {effective_approval_id}");
|
||||
}
|
||||
|
||||
@@ -2755,11 +2768,29 @@ impl Session {
|
||||
tx_approve.send(decision).ok();
|
||||
}
|
||||
None => {
|
||||
warn!("No pending approval found for call_id: {approval_id}");
|
||||
if !self
|
||||
.services
|
||||
.network_approval
|
||||
.notify_out_of_turn_approval(approval_id, decision)
|
||||
.await
|
||||
{
|
||||
warn!("No pending approval found for call_id: {approval_id}");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn notify_out_of_turn_network_approval(
|
||||
&self,
|
||||
approval_id: &str,
|
||||
decision: ReviewDecision,
|
||||
) -> bool {
|
||||
self.services
|
||||
.network_approval
|
||||
.notify_out_of_turn_approval(approval_id, decision)
|
||||
.await
|
||||
}
|
||||
|
||||
/// Records conversation items: append to history, persist to rollout, and
|
||||
/// notify clients observing raw response items.
|
||||
pub(crate) fn prepare_conversation_items_for_history<'a>(
|
||||
|
||||
@@ -35,6 +35,7 @@ use tokio::sync::Mutex;
|
||||
use tokio::sync::Notify;
|
||||
use tokio::sync::OnceCell;
|
||||
use tokio::sync::RwLock;
|
||||
use tokio::sync::oneshot;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
use tracing::warn;
|
||||
use uuid::Uuid;
|
||||
@@ -250,6 +251,7 @@ struct NetworkApprovalCallState {
|
||||
pub(crate) struct NetworkApprovalService {
|
||||
calls: Mutex<NetworkApprovalCallState>,
|
||||
pending_host_approvals: Mutex<HashMap<HostApprovalKey, Arc<PendingHostApproval>>>,
|
||||
pending_out_of_turn_approvals: Mutex<HashMap<String, oneshot::Sender<ReviewDecision>>>,
|
||||
session_approved_hosts: Mutex<HashSet<HostApprovalKey>>,
|
||||
session_denied_hosts: Mutex<HashSet<HostApprovalKey>>,
|
||||
}
|
||||
@@ -259,6 +261,7 @@ impl Default for NetworkApprovalService {
|
||||
Self {
|
||||
calls: Mutex::new(NetworkApprovalCallState::default()),
|
||||
pending_host_approvals: Mutex::new(HashMap::new()),
|
||||
pending_out_of_turn_approvals: Mutex::new(HashMap::new()),
|
||||
session_approved_hosts: Mutex::new(HashSet::new()),
|
||||
session_denied_hosts: Mutex::new(HashSet::new()),
|
||||
}
|
||||
@@ -266,6 +269,30 @@ impl Default for NetworkApprovalService {
|
||||
}
|
||||
|
||||
impl NetworkApprovalService {
|
||||
pub(crate) async fn insert_out_of_turn_approval(
|
||||
&self,
|
||||
approval_id: String,
|
||||
sender: oneshot::Sender<ReviewDecision>,
|
||||
) -> Option<oneshot::Sender<ReviewDecision>> {
|
||||
self.pending_out_of_turn_approvals
|
||||
.lock()
|
||||
.await
|
||||
.insert(approval_id, sender)
|
||||
}
|
||||
|
||||
pub(crate) async fn notify_out_of_turn_approval(
|
||||
&self,
|
||||
approval_id: &str,
|
||||
decision: ReviewDecision,
|
||||
) -> bool {
|
||||
let sender = self
|
||||
.pending_out_of_turn_approvals
|
||||
.lock()
|
||||
.await
|
||||
.remove(approval_id);
|
||||
sender.is_some_and(|sender| sender.send(decision).is_ok())
|
||||
}
|
||||
|
||||
/// Replace the target session's approval cache with the source session's
|
||||
/// currently approved hosts.
|
||||
pub(crate) async fn sync_session_approved_hosts_to(&self, other: &Self) {
|
||||
@@ -453,7 +480,14 @@ impl NetworkApprovalService {
|
||||
}
|
||||
}
|
||||
};
|
||||
let turn_context = Self::active_turn_context(session.as_ref()).await;
|
||||
let turn_context = match Self::active_turn_context(session.as_ref()).await {
|
||||
Some(turn_context) => Some(turn_context),
|
||||
// The untagged proxy listener is exposed to trusted host clients such as the
|
||||
// terminal browser. Environment-tagged listeners belong to sandboxed commands and
|
||||
// remain fail-closed after their originating turn ends.
|
||||
None if request.environment_id.is_none() => Some(session.new_default_turn().await),
|
||||
None => None,
|
||||
};
|
||||
let Some(environment_id) = active_environment_id.or_else(|| {
|
||||
turn_context
|
||||
.as_ref()
|
||||
|
||||
@@ -234,6 +234,39 @@ async fn pending_waiters_receive_owner_decision() {
|
||||
assert_eq!(decision, PendingApprovalDecision::AllowOnce);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn out_of_turn_approval_receives_the_submitted_decision() {
|
||||
let service = NetworkApprovalService::default();
|
||||
let (sender, receiver) = oneshot::channel();
|
||||
assert!(
|
||||
service
|
||||
.insert_out_of_turn_approval("approval-1".to_string(), sender)
|
||||
.await
|
||||
.is_none()
|
||||
);
|
||||
|
||||
assert!(
|
||||
service
|
||||
.notify_out_of_turn_approval("approval-1", ReviewDecision::Approved)
|
||||
.await
|
||||
);
|
||||
assert_eq!(
|
||||
receiver.await.expect("approval sender should remain open"),
|
||||
ReviewDecision::Approved
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn out_of_turn_approval_rejects_unknown_ids() {
|
||||
let service = NetworkApprovalService::default();
|
||||
|
||||
assert!(
|
||||
!service
|
||||
.notify_out_of_turn_approval("missing", ReviewDecision::Approved)
|
||||
.await
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn allow_once_and_allow_for_session_both_allow_network() {
|
||||
assert_eq!(
|
||||
|
||||
@@ -55,6 +55,100 @@ use tempfile::TempDir;
|
||||
|
||||
const NETWORK_TEST_HOST: &str = "codex-network-test.invalid";
|
||||
const NETWORK_TEST_TARGET: &str = "http://codex-network-test.invalid:80";
|
||||
const NETWORK_ABORT_HOST: &str = "codex-network-abort.invalid";
|
||||
const NETWORK_ABORT_TARGET: &str = "http://codex-network-abort.invalid:80";
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn host_proxy_request_can_be_approved_without_an_active_turn() -> Result<()> {
|
||||
skip_if_target_windows!(Ok(()), "uses a local managed-network proxy");
|
||||
skip_if_host_windows!(Ok(()));
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let home = Arc::new(TempDir::new()?);
|
||||
fs::write(
|
||||
home.path().join("config.toml"),
|
||||
r#"default_permissions = "workspace"
|
||||
|
||||
[permissions.workspace.filesystem]
|
||||
":minimal" = "read"
|
||||
|
||||
[permissions.workspace.network]
|
||||
enabled = true
|
||||
mode = "limited"
|
||||
allow_local_binding = true
|
||||
"#,
|
||||
)?;
|
||||
let permission_profile = PermissionProfile::workspace_write_with(
|
||||
&[],
|
||||
NetworkSandboxPolicy::Enabled,
|
||||
/*exclude_tmpdir_env_var*/ false,
|
||||
/*exclude_slash_tmp*/ false,
|
||||
);
|
||||
let test = test_codex()
|
||||
.with_home(home)
|
||||
.with_cloud_config_bundle(managed_network_requirements_loader())
|
||||
.with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile(permission_profile)
|
||||
.expect("set permission profile");
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await?;
|
||||
let http_addr = test
|
||||
.session_configured
|
||||
.network_proxy
|
||||
.as_ref()
|
||||
.context("expected runtime managed network proxy addresses")?
|
||||
.http_addr
|
||||
.clone();
|
||||
let client = reqwest::Client::builder()
|
||||
.proxy(reqwest::Proxy::http(format!("http://{http_addr}"))?)
|
||||
.build()?;
|
||||
let request_client = client.clone();
|
||||
let request = tokio::spawn(async move { request_client.get(NETWORK_TEST_TARGET).send().await });
|
||||
|
||||
let approval = expect_network_approval(&test, LOCAL_ENVIRONMENT_ID).await?;
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: Some(approval.turn_id),
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
.await?;
|
||||
|
||||
tokio::time::timeout(Duration::from_secs(/*secs*/ 5), request)
|
||||
.await
|
||||
.context("host proxy request did not resume after approval")??
|
||||
.context("host proxy request failed before receiving a proxy response")?;
|
||||
|
||||
let request = tokio::spawn(async move { client.get(NETWORK_ABORT_TARGET).send().await });
|
||||
let approval = expect_network_approval_for(
|
||||
&test,
|
||||
LOCAL_ENVIRONMENT_ID,
|
||||
NETWORK_ABORT_HOST,
|
||||
NETWORK_ABORT_TARGET,
|
||||
)
|
||||
.await?;
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: Some(approval.turn_id),
|
||||
decision: ReviewDecision::Abort,
|
||||
})
|
||||
.await?;
|
||||
let response = tokio::time::timeout(Duration::from_secs(/*secs*/ 5), request)
|
||||
.await
|
||||
.context("host proxy request did not resume after abort")??
|
||||
.context("host proxy request failed before receiving its denial")?;
|
||||
assert_eq!(response.status(), reqwest::StatusCode::FORBIDDEN);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn approved_network_host_for_one_environment_still_prompts_in_another() -> Result<()> {
|
||||
@@ -284,6 +378,21 @@ async fn submit_managed_network_turn(
|
||||
async fn expect_network_approval(
|
||||
test: &TestCodex,
|
||||
expected_environment_id: &str,
|
||||
) -> Result<ExecApprovalRequestEvent> {
|
||||
expect_network_approval_for(
|
||||
test,
|
||||
expected_environment_id,
|
||||
NETWORK_TEST_HOST,
|
||||
NETWORK_TEST_TARGET,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn expect_network_approval_for(
|
||||
test: &TestCodex,
|
||||
expected_environment_id: &str,
|
||||
expected_host: &str,
|
||||
expected_target: &str,
|
||||
) -> Result<ExecApprovalRequestEvent> {
|
||||
let deadline = std::time::Instant::now() + Duration::from_secs(30);
|
||||
let remaining = deadline
|
||||
@@ -304,15 +413,12 @@ async fn expect_network_approval(
|
||||
EventMsg::ExecApprovalRequest(approval) => {
|
||||
assert_eq!(
|
||||
approval.command,
|
||||
vec![
|
||||
"network-access".to_string(),
|
||||
NETWORK_TEST_TARGET.to_string()
|
||||
]
|
||||
vec!["network-access".to_string(), expected_target.to_string()]
|
||||
);
|
||||
assert_eq!(
|
||||
approval.network_approval_context,
|
||||
Some(NetworkApprovalContext {
|
||||
host: NETWORK_TEST_HOST.to_string(),
|
||||
host: expected_host.to_string(),
|
||||
protocol: NetworkApprovalProtocol::Http,
|
||||
})
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user