diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 35e1b08b61..5a87a10b38 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -295,6 +295,12 @@ Example with notification opt-out: - `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk, with optional `reloadUserConfig: true` to hot-reload loaded threads, including multiple `desktop.*` edits. Session-static model, reasoning-effort, Plan-mode reasoning-effort, service-tier, and personality defaults do not reload existing threads. - `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including exact managed values (`cliAuthCredentialsStore`, `chatgptBaseUrl`, `sqliteHome`, `logDir`, `modelCatalogJson`, `checkForUpdateOnStartup`, `allowLoginShell`, `feedback.enabled`, and `windowsSandboxPrivateDesktop`), requirements-only developer instructions (`additionalDeveloperInstructions`, supplied independently of ordinary developer instructions), allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), the layered permission-profile allow map (`allowedPermissionProfiles`), the managed permission-profile default (`defaultPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), remote-control policy (`allowRemoteControl`; `false` force-disables remote control while `true` or `null` preserves existing behavior), the Browser/Computer Use umbrella policy (`allowBrowserAndComputerUse`), computer use policy (`computerUse`, including persistent approval, default application access, and per-platform application rules), Browser Use policy (`browserUse`, including history access, origin rules, auto-review, and approval controls), interactive browser import policy (`inAppBrowser.allowExternalBrowserSettingsImport`), pinned feature values (`featureRequirements`, including the default-allowed `in_app_updates` policy that administrators can set to `false`), managed lifecycle hooks (`hooks`, including command handlers with optional `additionalContextLimit` and `mcp_tool` handlers with `server`, `tool`, `input`, `timeoutSec`, and `statusMessage`), `enforceResidency`, managed automatic review (`autoReview.requiredOnModels` and `autoReview.ignoreRules`), model defaults (`models.newThread.model`, `models.newThread.modelReasoningEffort`, and `models.newThread.serviceTier`), and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`. +`mcpServer/resource/read` and `mcpServer/tool/call` preserve MCP protocol errors +with their original `code`, `message`, and `data`, including authentication +metadata in `data._meta`. Other operation failures retain the existing +internal-error response. Tool results with `isError: true` remain results, +including their `_meta`. + ### Plugin configuration scope Plugin activation and MCP settings use the existing merged configuration, including diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 299d7626fb..405a1fe8c3 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -509,7 +509,7 @@ impl McpRequestProcessor { origin_call_id: Option, ) { let result = result - .map_err(|error| internal_error(format!("{error:#}"))) + .map_err(mcp_operation_error) .and_then(|result| { serde_json::from_value::(result).map_err(|error| { internal_error(format!( @@ -541,13 +541,24 @@ impl McpRequestProcessor { .call_mcp_tool(¶ms.server, ¶ms.tool, params.arguments, meta) .await .map(McpServerToolCallResponse::from) - .map_err(|error| internal_error(format!("{error:#}"))); + .map_err(mcp_operation_error); outgoing.send_result(request_id, result).await; }); Ok(()) } } +fn mcp_operation_error(error: anyhow::Error) -> JSONRPCErrorError { + match codex_rmcp_client::mcp_error(&error) { + Some(error) => JSONRPCErrorError { + code: i64::from(error.code.0), + message: error.message.to_string(), + data: error.data.clone(), + }, + None => internal_error(format!("{error:#}")), + } +} + fn with_mcp_tool_call_thread_id_meta( meta: Option, thread_id: &str, diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index ca534a201d..71094e2b58 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -149,6 +149,7 @@ struct MockResponsesState { root_worker: bool, root_user_restriction: bool, root_user_input_restriction: bool, + late_root_restriction: bool, user_input_restriction: bool, } @@ -198,6 +199,7 @@ enum ThreadLifecycle { Fork, RootRollback, RootRestriction, + RootRestrictionDuringClassification, RootTrustedSkill, RootUserRestriction, RootUserInputRestriction, @@ -210,6 +212,7 @@ impl ThreadLifecycle { self, Self::RootRollback | Self::RootRestriction + | Self::RootRestrictionDuringClassification | Self::RootTrustedSkill | Self::RootUserInputRestriction | Self::RootUserInputHookBlocked @@ -316,6 +319,17 @@ async fn submit_user_input_response(app_server: &mut TestAppServer, answers: Val .await } +async fn wait_for_guardian_reviews(state: &MockResponsesState, expected: usize) -> Result<()> { + timeout(TIMEOUT, async { + while state.guardian_reviews.load(Ordering::SeqCst) < expected { + tokio::task::yield_now().await; + } + }) + .await?; + assert_eq!(state.guardian_reviews.load(Ordering::SeqCst), expected); + Ok(()) +} + async fn parent_response( State(state): State>, Json(request): Json, @@ -363,7 +377,7 @@ async fn parent_response( let root_request = state.root_requests.fetch_add(1, Ordering::SeqCst); match root_request { 1 if state.root_user_input_restriction => user_input_request_events(), - 0 | 2 => { + 0 | 2 if root_request == 0 || !state.late_root_restriction => { let (call_id, tool_name, arguments) = if root_request == 0 { ( "guardian-spawn-worker", @@ -536,8 +550,19 @@ async fn guardian_v2_routes_scoped_tool_approvals( }; let node_repl_review_required = matches!(requirement, ModelReviewRequirement::Required) && codex_protocol::mcp::is_node_repl_backed_server(server_name); + let late_root_restriction = matches!( + lifecycle, + ThreadLifecycle::RootRestrictionDuringClassification + ); let (luna_score, expected_guardian_reviews) = match risk { - GuardianRisk::Low if classifier_in_scope && sensitive_action != Some(true) => (0.25, 1), + GuardianRisk::Low + if classifier_in_scope + && sensitive_action != Some(true) + && !lifecycle.has_user_answer() + && !late_root_restriction => + { + (0.25, 1) + } GuardianRisk::Low | GuardianRisk::InvalidResponse => (0.25, 2), GuardianRisk::Threshold => (0.5, 2), GuardianRisk::High => (0.95, 2), @@ -557,6 +582,7 @@ async fn guardian_v2_routes_scoped_tool_approvals( root_worker: lifecycle.uses_root_worker(), root_user_restriction: matches!(lifecycle, ThreadLifecycle::RootUserRestriction), root_user_input_restriction: lifecycle.has_root_user_input(), + late_root_restriction, user_input_restriction: lifecycle.has_user_input(), ..Default::default() }); @@ -669,6 +695,7 @@ async fn guardian_v2_routes_scoped_tool_approvals( | ThreadLifecycle::UserInputHookBlocked | ThreadLifecycle::RootRollback | ThreadLifecycle::RootRestriction + | ThreadLifecycle::RootRestrictionDuringClassification | ThreadLifecycle::RootTrustedSkill | ThreadLifecycle::RootUserRestriction | ThreadLifecycle::RootUserInputRestriction @@ -715,6 +742,7 @@ async fn guardian_v2_routes_scoped_tool_approvals( | ThreadLifecycle::UserInputHookBlocked | ThreadLifecycle::RootRollback | ThreadLifecycle::RootRestriction + | ThreadLifecycle::RootRestrictionDuringClassification | ThreadLifecycle::RootTrustedSkill | ThreadLifecycle::RootUserRestriction | ThreadLifecycle::RootUserInputRestriction @@ -878,6 +906,27 @@ async fn guardian_v2_routes_scoped_tool_approvals( }) }) ); + if late_root_restriction { + // The worker's first classifier stays in flight while only root authorization changes. + let completed: TurnCompletedNotification = + timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??; + assert_eq!(completed.thread_id, thread_id); + let request_id = app_server + .send_turn_start_request(TurnStartParams { + thread_id: thread_id.clone(), + input: vec![UserInput::Text { + text: ROOT_RESTRICTION.to_owned(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let _: TurnStartResponse = + timeout(TIMEOUT, app_server.read_response(request_id)).await??; + let completed: TurnCompletedNotification = + timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??; + assert_eq!(completed.thread_id, thread_id); + } responses_state.allow_luna.notify_one(); timeout(TIMEOUT, responses_state.classification_completed.notified()).await?; responses_state.allow_guardian_review.notify_one(); @@ -920,6 +969,11 @@ async fn guardian_v2_routes_scoped_tool_approvals( "the configured hook must replace or reject the visible tool output" ); } + } else if late_root_restriction { + assert!( + reviews.is_empty(), + "the first review predates root revocation" + ); } else if matches!(review_outcome, ReviewOutcome::Malformed) { assert!( reviews.is_empty(), @@ -984,6 +1038,11 @@ async fn guardian_v2_routes_scoped_tool_approvals( ); } } + if matches!(risk, GuardianRisk::Low) + && (lifecycle.has_user_answer() || late_root_restriction) + { + wait_for_guardian_reviews(responses_state.as_ref(), expected_guardian_reviews).await?; + } responses_state.allow_luna.notify_one(); } else { responses_state.allow_guardian_review.notify_one(); @@ -1032,10 +1091,10 @@ async fn guardian_v2_routes_scoped_tool_approvals( ); } let requires_strict_review = classifier_in_scope - && matches!( + && (matches!( risk, GuardianRisk::Threshold | GuardianRisk::High | GuardianRisk::InvalidResponse - ); + ) || matches!(risk, GuardianRisk::Low) && lifecycle.has_user_answer()); let strict_review_count = app_server .pending_notification_methods() .into_iter() @@ -1063,7 +1122,10 @@ async fn guardian_v2_routes_scoped_tool_approvals( ); } - if classifier_in_scope && !matches!(risk, GuardianRisk::InvalidResponse) { + if classifier_in_scope + && !matches!(risk, GuardianRisk::InvalidResponse) + && !late_root_restriction + { let state_db = StateRuntime::init( codex_state::SqliteConfig::new_for_testing(codex_home.path().abs()), "mock_provider".to_owned(), @@ -1100,7 +1162,10 @@ async fn guardian_v2_routes_scoped_tool_approvals( ); } - if lifecycle.uses_root_worker() || matches!(lifecycle, ThreadLifecycle::RootUserRestriction) { + if !late_root_restriction + && (lifecycle.uses_root_worker() + || matches!(lifecycle, ThreadLifecycle::RootUserRestriction)) + { if matches!(lifecycle, ThreadLifecycle::RootRollback) { let rollback_id = app_server .send_thread_rollback_request(ThreadRollbackParams { @@ -1190,6 +1255,11 @@ async fn guardian_v2_routes_scoped_tool_approvals( "the worker classifier must see the new root-user restriction" ); } + // Keep the fresh classifier pending: a new review must not be skipped using the old low score. + if matches!(risk, GuardianRisk::Low) { + wait_for_guardian_reviews(responses_state.as_ref(), expected_guardian_reviews + 1) + .await?; + } responses_state.allow_luna.notify_one(); } @@ -1655,3 +1725,28 @@ async fn guardian_v2_discards_sync_reviews_after_authorization_changes( ) .await } + +#[test_case(ThreadLifecycle::RootUserRestriction; "new user turn")] +#[test_case(ThreadLifecycle::RootRestriction; "worker root restriction")] +#[test_case(ThreadLifecycle::RootUserInputRestriction; "worker root answer")] +#[test_case(ThreadLifecycle::UserInputRestriction; "user input answer")] +#[test_case(ThreadLifecycle::UserInputEmpty; "empty answer preserves cache")] +#[test_case(ThreadLifecycle::RootRestrictionDuringClassification; "late score after root revocation")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_v2_low_scores_require_current_authorization( + lifecycle: ThreadLifecycle, +) -> Result<()> { + skip_if_no_network!(Ok(())); + guardian_v2_routes_scoped_tool_approvals( + GuardianRisk::Low, + lifecycle, + ModelReviewRequirement::Optional, + ReviewOutcome::Allow, + TranscriptContent::Normal, + GuardianToolScope::ComputerUseOnly { + server_name: "node_repl", + }, + /*sensitive_action*/ None, + ) + .await +} diff --git a/codex-rs/app-server/tests/suite/v2/mcp_resource.rs b/codex-rs/app-server/tests/suite/v2/mcp_resource.rs index 2064b4bf07..ba6295ddf9 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_resource.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_resource.rs @@ -60,6 +60,7 @@ use rmcp::transport::StreamableHttpService; use rmcp::transport::streamable_http_server::session::local::LocalSessionManager; use serde_json::json; use tempfile::TempDir; +use test_case::test_case; use tokio::net::TcpListener; use tokio::task::JoinHandle; use tokio::time::timeout; @@ -70,6 +71,7 @@ pub(super) const TEST_WIDGET_RESOURCE_URI: &str = "ui://widget/checkout-session. const TEST_BLOB_RESOURCE_URI: &str = "test://codex/resource.bin"; const TEST_RESOURCE_BLOB: &str = "YmluYXJ5LXJlc291cmNl"; const TEST_RESOURCE_TEXT: &str = "Resource body from the MCP server."; +const TEST_ERROR_RESOURCE_URI: &str = "test://codex/error"; const TEST_ELICITATION_RESOURCE_URI: &str = "test://codex/elicitation"; const TEST_ELICITATION_RESOURCE_TEXT: &str = "Threadless elicitation was declined."; const SKILL_NAME: &str = "demo-plugin:deploy"; @@ -133,6 +135,77 @@ async fn mcp_resource_read_returns_resource_contents() -> Result<()> { Ok(()) } +#[test_case(ProtocolVersion::V_2025_06_18; "legacy")] +#[test_case(ProtocolVersion::V_2026_07_28; "modern")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn mcp_resource_read_preserves_protocol_errors(protocol: ProtocolVersion) -> Result<()> { + let responses_server = responses::start_mock_server().await; + let (apps_server_url, _calls, apps_server_handle) = start_resource_apps_mcp_server().await?; + let codex_home = TempDir::new()?; + let config = MockResponsesConfig::new(&responses_server.uri()).with_extra_config(&format!( + "[mcp_servers.resource_server]\nurl = \"{apps_server_url}/api/codex/ps/mcp\"" + )); + let config = if protocol == ProtocolVersion::V_2026_07_28 { + config.enable_feature(Feature::Mcp20260728) + } else { + config.disable_feature(Feature::Mcp20260728) + }; + config.write(codex_home.path())?; + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized() + .await?; + let ThreadStartResponse { thread, .. } = mcp.start_thread(ThreadStartParams::default()).await?; + + for (server, expected_error) in [ + ( + "resource_server", + json!({ + "code": -32042, + "message": "resource authorization required", + "data": { + "uri": TEST_ERROR_RESOURCE_URI, + "protocolVersion": protocol, + "_meta": {"_codex_apps": {"connector_auth_failure": { + "is_auth_failure": true, + "connector_id": "calendar", + "requested_scopes": ["calendar.read"], + }}}, + }, + }), + ), + ( + "missing", + json!({"code": -32603, "message": "unknown MCP server 'missing'"}), + ), + ] { + for thread_id in [Some(thread.id.clone()), None] { + let request_id = mcp + .send_mcp_resource_read_request(McpResourceReadParams { + thread_id, + origin_call_id: None, + server: server.to_string(), + uri: TEST_ERROR_RESOURCE_URI.to_string(), + connector_id: None, + }) + .await?; + let error = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert_eq!( + serde_json::to_value(error)?, + json!({"id": request_id, "error": expected_error}) + ); + } + } + + apps_server_handle.abort(); + let _ = apps_server_handle.await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn orchestrator_skill_can_read_referenced_resource_without_an_executor() -> Result<()> { let responses_server = responses::start_mock_server().await; @@ -951,6 +1024,21 @@ impl ServerHandler for ResourceAppsMcpServer { context: RequestContext, ) -> Result { let uri = request.uri; + if uri == TEST_ERROR_RESOURCE_URI { + return Err(rmcp::ErrorData::new( + rmcp::model::ErrorCode(-32042), + "resource authorization required", + Some(json!({ + "uri": uri, + "protocolVersion": context.protocol_version(), + "_meta": {"_codex_apps": {"connector_auth_failure": { + "is_auth_failure": true, + "connector_id": "calendar", + "requested_scopes": ["calendar.read"], + }}}, + })), + )); + } if uri == TEST_WIDGET_RESOURCE_URI { let request_meta = context .meta diff --git a/codex-rs/app-server/tests/suite/v2/mcp_tool.rs b/codex-rs/app-server/tests/suite/v2/mcp_tool.rs index 49b886e1e5..6b67fb195e 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_tool.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_tool.rs @@ -63,6 +63,7 @@ use rmcp::model::JsonObject; use rmcp::model::ListToolsResult; use rmcp::model::MetaObject; use rmcp::model::PrimitiveSchemaDefinition; +use rmcp::model::ProtocolVersion; use rmcp::model::RequestMetaObject; use rmcp::model::ServerCapabilities; use rmcp::model::ServerInfo; @@ -75,6 +76,7 @@ use rmcp::transport::StreamableHttpService; use rmcp::transport::streamable_http_server::session::local::LocalSessionManager; use serde_json::json; use tempfile::TempDir; +use test_case::test_case; use tokio::net::TcpListener; use tokio::sync::oneshot; use tokio::task::JoinHandle; @@ -90,6 +92,7 @@ const LARGE_OUTPUT_AUTO_COMPACT_LIMIT: i64 = 1_000_000; pub(super) const TEST_SERVER_NAME: &str = "tool_server"; pub(super) const TEST_TOOL_NAME: &str = "echo_tool"; const LARGE_RESPONSE_MESSAGE: &str = "large"; +const PROTOCOL_ERROR_MESSAGE: &str = "protocol-error"; const ELICITATION_TRIGGER_MESSAGE: &str = "confirm"; const ELICITATION_MESSAGE: &str = "Allow this request?"; const URL_ELICITATION_TRIGGER_MESSAGE: &str = "auth"; @@ -163,6 +166,73 @@ async fn mcp_server_tool_call_returns_tool_result() -> Result<()> { Ok(()) } +#[test_case(ProtocolVersion::V_2025_06_18; "legacy")] +#[test_case(ProtocolVersion::V_2026_07_28; "modern")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn mcp_server_tool_call_preserves_protocol_errors(protocol: ProtocolVersion) -> Result<()> { + let responses_server = responses::start_mock_server().await; + let (mcp_server_url, mcp_server_handle) = start_mcp_server(/*sensitive_action*/ None).await?; + let codex_home = TempDir::new()?; + let config = mcp_tool_config(&responses_server.uri(), &mcp_server_url, AUTO_COMPACT_LIMIT); + let config = if protocol == ProtocolVersion::V_2026_07_28 { + config.enable_feature(Feature::Mcp20260728) + } else { + config.disable_feature(Feature::Mcp20260728) + }; + config.write(codex_home.path())?; + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized() + .await?; + let ThreadStartResponse { thread, .. } = mcp.start_thread(ThreadStartParams::default()).await?; + + for (server, expected_error) in [ + ( + TEST_SERVER_NAME, + json!({ + "code": -32043, + "message": "tool authorization required", + "data": { + "tool": TEST_TOOL_NAME, + "protocolVersion": protocol, + "_meta": {"_codex_apps": {"connector_auth_failure": { + "is_auth_failure": true, + "connector_id": "calendar", + "requested_scopes": ["calendar.read"], + }}}, + }, + }), + ), + ( + "missing", + json!({"code": -32603, "message": "unknown MCP server 'missing'"}), + ), + ] { + let request_id = mcp + .send_mcp_server_tool_call_request(McpServerToolCallParams { + thread_id: thread.id.clone(), + server: server.to_string(), + tool: TEST_TOOL_NAME.to_string(), + arguments: Some(json!({"message": PROTOCOL_ERROR_MESSAGE})), + meta: None, + }) + .await?; + let error = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert_eq!( + serde_json::to_value(error)?, + json!({"id": request_id, "error": expected_error}) + ); + } + + mcp_server_handle.abort(); + let _ = mcp_server_handle.await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn mcp_server_tool_call_forwards_only_server_extensions() -> Result<()> { let responses_server = responses::start_mock_server().await; @@ -1312,6 +1382,22 @@ impl ServerHandler for ToolAppsMcpServer { ); } + if message == PROTOCOL_ERROR_MESSAGE { + return Err(rmcp::ErrorData::new( + rmcp::model::ErrorCode(-32043), + "tool authorization required", + Some(json!({ + "tool": request.name, + "protocolVersion": context.protocol_version(), + "_meta": {"_codex_apps": {"connector_auth_failure": { + "is_auth_failure": true, + "connector_id": "calendar", + "requested_scopes": ["calendar.read"], + }}}, + })), + )); + } + if message == LARGE_RESPONSE_MESSAGE { let large_text = "large-mcp-content-".repeat(DEFAULT_OUTPUT_BYTES_CAP / 8); let mut result = CallToolResult::structured(json!({ diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index c053c1d618..3527df18d5 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -1,6 +1,7 @@ use crate::agent::AgentStatus; use crate::config::ConstraintResult; use crate::context::ContextualUserFragment; +use crate::context::GuardianReviewEvidence; use crate::elicitation::ElicitationRegistration; use crate::session::SessionIo; use crate::session::SessionSettingsUpdate; @@ -808,6 +809,17 @@ impl CodexThread { self.session.multi_agent_version() } + /// Returns the current history generation and genuine user-input counts for Guardian. + pub async fn guardian_authorization_version(&self) -> GuardianAuthorizationVersion { + let history = self.session.conversation_history_snapshot().await; + self.thread_extension_data() + .get::() + .map_or_else( + || GuardianAuthorizationVersion::from_history(history.as_ref()), + |evidence| evidence.authorization_version(history.as_ref()), + ) + } + /// Returns bounded root conversation evidence and its authorization version atomically. pub async fn guardian_root_snapshot(&self) -> Option { self.session diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/authorization.rs b/codex-rs/ext/guardian-v2/src/async_scorer/authorization.rs new file mode 100644 index 0000000000..2dcf369a34 --- /dev/null +++ b/codex-rs/ext/guardian-v2/src/async_scorer/authorization.rs @@ -0,0 +1,23 @@ +//! Binds cached classifier results to the user authorization they evaluated. + +use codex_core::CodexThread; +use codex_core::GuardianAuthorizationVersion; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(super) struct ScoreAuthorization { + pub(super) local: GuardianAuthorizationVersion, + pub(super) root: Option, +} + +impl ScoreAuthorization { + pub(super) async fn current(thread: &CodexThread) -> Self { + let root = thread + .guardian_root_snapshot() + .await + .map(|snapshot| snapshot.authorization_version); + Self { + local: thread.guardian_authorization_version().await, + root, + } + } +} diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index 6260214b19..42d60041c8 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -1,5 +1,6 @@ use std::collections::BTreeMap; use std::sync::Arc; +use std::sync::Mutex; use std::sync::Weak; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -44,6 +45,7 @@ use codex_protocol::security_risk::SecurityRiskScore; use super::action::GuardianAction; use super::action::RenderedAction; +use super::authorization::ScoreAuthorization; use super::config::GuardianV2Config; use super::config::GuardianV2ReviewScope; use super::metrics::REVIEW_FALLBACK_METRIC; @@ -113,6 +115,8 @@ struct GuardianV2ScoreProgress { latest_tool_call: AtomicUsize, latest_scored_tool_call: AtomicUsize, latest_failed_tool_call: AtomicUsize, + // Serialize successful score publication with its authorization metadata. + authorization: Mutex>, metrics: Option>, } @@ -262,6 +266,17 @@ impl ApprovalReviewContributor for GuardianV2Extension { record_fast_decision(extension_metrics.as_deref(), "deferred", "missing_score"); return None; }; + let manager = self.thread_manager.upgrade()?; + let thread_id = ThreadId::from_string(thread_store.level_id()).ok()?; + let Ok(thread) = manager.get_thread(thread_id).await else { + record_fast_decision(extension_metrics.as_deref(), "deferred", "scoring_failure"); + return None; + }; + let current_authorization = ScoreAuthorization::current(&thread).await; + let scored_authorization = score_progress + .authorization + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let latest_scored_tool_call = score_progress .latest_scored_tool_call .load(Ordering::Acquire); @@ -306,6 +321,15 @@ impl ApprovalReviewContributor for GuardianV2Extension { return None; }; if score < guardian_config.review_threshold { + if scored_authorization.as_ref() != Some(¤t_authorization) { + thread_store.insert(StrictReviewReason::StaleScore); + record_fast_decision( + extension_metrics.as_deref(), + "deferred", + "authorization_changed", + ); + return None; + } record_fast_decision(extension_metrics.as_deref(), "approved", "low_risk"); return Some(ReviewDecision::Approved); } @@ -515,6 +539,10 @@ impl GuardianV2Extension { let root_conversation = root_snapshot.map(|snapshot| snapshot.messages); let authorization_version = guardian_evidence.authorization_version(conversation_history.as_ref()); + let score_authorization = ScoreAuthorization { + local: authorization_version, + root: root_authorization_version, + }; let trusted_user_inputs = guardian_evidence.user_input_fragments(conversation_history.as_ref()); let transcript = guardian_config @@ -658,12 +686,26 @@ impl GuardianV2Extension { ), sampled_at: Some(sampled_at.into()), }; - let accepted = - thread - .thread_extension_data() - .insert_if(score.clone(), |previous| { - previous.is_none_or(|previous| previous.sampled_at < score.sampled_at) - }); + if score_authorization != ScoreAuthorization::current(&thread).await { + return Ok(ClassificationOutcome::Superseded); + } + let accepted = { + let mut scored_authorization = score_progress + .authorization + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let accepted = + thread + .thread_extension_data() + .insert_if(score.clone(), |previous| { + previous + .is_none_or(|previous| previous.sampled_at < score.sampled_at) + }); + if accepted { + *scored_authorization = Some(score_authorization); + } + accepted + }; tracing::info!( %thread_id, %turn_id, diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs index 1144907c84..aade245d9d 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs @@ -937,6 +937,13 @@ async fn sample_configured_conversation_history_with_source( let tool_payload = ToolPayload::Function { arguments: arguments.to_owned(), }; + if !conversation_history.is_empty() { + Box::pin( + test.codex + .inject_response_items(conversation_history.clone()), + ) + .await?; + } let conversation_history = TestConversationHistory(conversation_history); registry.tool_lifecycle_contributors()[0] @@ -2153,6 +2160,11 @@ async fn contributor_skips_required_models_in_standard_scope() -> Result<()> { .await; model_info.slug = "protected-model".to_owned(); thread_store.insert(model_info); + let authorization = super::ScoreAuthorization::current(&test.codex).await; + let progress = thread_store + .get::() + .expect("Guardian v2 should track score progress per thread"); + *progress.authorization.lock().unwrap() = Some(authorization); thread_store.insert(SecurityRiskScore { scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]), call_id: None, @@ -2597,7 +2609,6 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result< let session_store = ExtensionData::new("session-1"); let thread_store = test.codex.thread_extension_data(); let metrics = Arc::new(RecordingMetrics::default()); - thread_store.insert(parent_model); registry.thread_lifecycle_contributors()[0] .on_thread_start(ThreadStartInput { config: &config, @@ -2643,6 +2654,13 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result< }, ]); + Box::pin( + test.codex + .inject_response_items(conversation_history.0.clone()), + ) + .await?; + thread_store.insert(parent_model); + registry.tool_lifecycle_contributors()[0] .on_tool_start(ToolStartInput { session_store: &session_store, diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs b/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs index 4ce01e4500..5fcb6da9d3 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/mod.rs @@ -1,4 +1,5 @@ mod action; +mod authorization; mod config; mod extension; mod metrics; diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 511d84557d..83e53099cf 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -364,6 +364,29 @@ impl ReadDenyMatcher { /// Returns whether `path` is denied by the policy used to build this matcher. pub fn is_read_denied(&self, path: &Path) -> bool { + if self.invalid_pattern { + return true; + } + self.is_read_denied_candidates(&normalized_and_canonical_candidates(path)) + } + + /// Checks an enumerated path using a canonical location already resolved by + /// the caller, without reopening the file to canonicalize it. + /// + /// Bulk filesystem walkers may derive this location from a freshly resolved + /// parent and a non-symlink directory entry. Symlinks and Windows junctions + /// must be resolved separately. Do not reuse these locations across walks: + /// a later operation must observe newly created files and changed links. + pub fn is_read_denied_with_canonical_path(&self, path: &Path, canonical_path: &Path) -> bool { + let candidates = [path, canonical_path].map(|candidate| { + AbsolutePathBuf::from_absolute_path(candidate) + .map(AbsolutePathBuf::into_path_buf) + .unwrap_or_else(|_| candidate.to_path_buf()) + }); + self.is_read_denied_candidates(&candidates) + } + + fn is_read_denied_candidates(&self, path_candidates: &[PathBuf]) -> bool { if self.invalid_pattern { // Direct tool reads fail closed on malformed deny patterns. Silent // allow would turn a config typo into a policy bypass. @@ -373,7 +396,6 @@ impl ReadDenyMatcher { // Check exact roots against each candidate spelling before evaluating // glob matchers. Exact entries are subtree denies; glob entries match // according to the pattern compiler's path-separator rules. - let path_candidates = normalized_and_canonical_candidates(path); if self.denied_candidates.iter().any(|denied_candidates| { path_candidates.iter().any(|candidate| { denied_candidates.iter().any(|denied_candidate| { diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index 06ba74a7e0..9b3a2ae6ce 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -20,6 +20,7 @@ mod perform_oauth_login; mod program_resolver; mod protocol_mode; mod rmcp_client; +mod service_error; mod startup_error; mod stdio_server_launcher; mod utils; @@ -72,6 +73,7 @@ pub use rmcp_client::RmcpClient; pub use rmcp_client::SendElicitation; pub use rmcp_client::StreamableHttpBearerToken; pub use rmcp_client::ToolWithConnectorId; +pub use service_error::mcp_error; pub use startup_error::is_authentication_required_error; pub use stdio_server_launcher::ExecutorStdioServerLauncher; pub use stdio_server_launcher::LocalStdioServerLauncher; diff --git a/codex-rs/rmcp-client/src/service_error.rs b/codex-rs/rmcp-client/src/service_error.rs new file mode 100644 index 0000000000..65da799f14 --- /dev/null +++ b/codex-rs/rmcp-client/src/service_error.rs @@ -0,0 +1,15 @@ +//! Access structured MCP errors without dropping their original protocol data. + +use anyhow::Error; +use rmcp::ErrorData; +use rmcp::service::ServiceError; + +use crate::rmcp_client::ClientOperationError; + +/// Returns the server's structured protocol error, including its original data. +pub fn mcp_error(error: &Error) -> Option<&ErrorData> { + let ClientOperationError::Service(ServiceError::McpError(error)) = error.downcast_ref()? else { + return None; + }; + Some(error) +} diff --git a/codex-rs/rmcp-client/tests/resources.rs b/codex-rs/rmcp-client/tests/resources.rs index be1755f4a5..da280ee291 100644 --- a/codex-rs/rmcp-client/tests/resources.rs +++ b/codex-rs/rmcp-client/tests/resources.rs @@ -7,8 +7,10 @@ use codex_rmcp_client::ElicitationAction; use codex_rmcp_client::ElicitationResponse; use codex_rmcp_client::LocalStdioServerLauncher; use codex_rmcp_client::RmcpClient; +use codex_rmcp_client::mcp_error; use codex_utils_cargo_bin::CargoBinError; use futures::FutureExt as _; +use pretty_assertions::assert_eq; use rmcp::model::ClientCapabilities; use rmcp::model::ElicitationCapability; use rmcp::model::FormElicitationCapability; @@ -37,8 +39,7 @@ fn init_params() -> InitializeRequestParams { .with_protocol_version(ProtocolVersion::V_2025_06_18) } -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> { +async fn resource_client() -> anyhow::Result { let client = RmcpClient::new_stdio_client( stdio_server_bin()?.into(), Vec::::new(), @@ -66,6 +67,12 @@ async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> { ) .await?; + Ok(client) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> { + let client = resource_client().await?; let list = client .list_resources(/*params*/ None, Some(Duration::from_secs(5))) .await?; @@ -112,3 +119,26 @@ async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn rmcp_client_preserves_each_resource_error() -> anyhow::Result<()> { + let client = resource_client().await?; + for uri in ["memo://codex/missing-first", "memo://codex/missing-second"] { + let error = client + .read_resource( + ReadResourceRequestParams::new(uri), + Some(Duration::from_secs(5)), + ) + .await + .expect_err("missing resource must return a protocol error") + .context("resources/read failed"); + assert_eq!( + mcp_error(&error), + Some(&rmcp::ErrorData::resource_not_found( + "resource_not_found", + Some(json!({ "uri": uri })), + )) + ); + } + Ok(()) +} diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs index 498ceabfd0..57a19d0720 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs @@ -5,12 +5,20 @@ use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::ReadDenyMatcher; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashSet; -use std::path::Path; use std::path::PathBuf; +use std::process::Command; +#[path = "deny_read_walker.rs"] +mod walker; + +use walker::DirectoryScanMode; +use walker::collect_existing_glob_directory_matches; + +#[derive(Debug, Eq, PartialEq)] struct GlobScanPlan { root: PathBuf, max_depth: Option, + globs: Vec, } /// Resolve split filesystem `None` read entries into concrete Windows ACL targets. @@ -52,87 +60,107 @@ pub fn resolve_windows_deny_read_paths( return Ok(paths); }; - let scan_plans = unreadable_globs - .iter() - .map(|pattern| { - let scan_plan = glob_scan_plan(pattern, file_system_sandbox_policy.glob_scan_max_depth); - if scan_plan.max_depth.is_none() && scan_plan.root.parent().is_none() { - return Err(format!( - "unreadable glob `{pattern}` cannot be safely expanded from a filesystem root without `glob_scan_max_depth`; configure `glob_scan_max_depth` or use a non-root directory prefix" - )); - } - Ok(scan_plan) - }) - .collect::, String>>()?; + let scan_plans = glob_scan_plans( + &unreadable_globs, + file_system_sandbox_policy.glob_scan_max_depth, + )?; for scan_plan in scan_plans { - let mut seen_scan_dirs = HashSet::new(); - collect_existing_glob_matches( + if !scan_plan.root.exists() { + continue; + } + + let directory_scan_mode = if let Some(file_paths) = ripgrep_files(&scan_plan)? { + for path in file_paths { + if matcher.is_read_denied(&path) { + push_absolute_path(&mut paths, &mut seen, path)?; + } + } + DirectoryScanMode::DirectoriesOnly + } else { + // Rebuild the complete accessible snapshot with the policy matcher + // when ripgrep is missing or reports an incomplete traversal. Never + // apply ACLs from a failed scan's partial stdout. + DirectoryScanMode::IncludeAllFiles + }; + + collect_existing_glob_directory_matches( &scan_plan.root, &matcher, &mut paths, &mut seen, - &mut seen_scan_dirs, scan_plan.max_depth, - /*depth*/ 0, + directory_scan_mode, )?; } Ok(paths) } -fn collect_existing_glob_matches( - path: &Path, - matcher: &ReadDenyMatcher, - paths: &mut Vec, - seen_paths: &mut HashSet, - seen_scan_dirs: &mut HashSet, - max_depth: Option, - depth: usize, -) -> Result<(), String> { - if !path.exists() { - return Ok(()); +fn ripgrep_files(scan_plan: &GlobScanPlan) -> Result>, String> { + let mut command = Command::new("rg"); + command + .arg("--files") + .arg("--hidden") + .arg("--no-ignore") + .arg("--null"); + if let Some(max_depth) = scan_plan.max_depth { + command.arg("--max-depth").arg(max_depth.to_string()); } - - if matcher.is_read_denied(path) { - push_absolute_path(paths, seen_paths, path.to_path_buf())?; + for glob in &scan_plan.globs { + command.arg("--glob").arg(glob); } + command.arg("--").arg(&scan_plan.root); - let Ok(metadata) = path.metadata() else { - return Ok(()); + let output = match command.output() { + Ok(output) => output, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(err) => { + return Err(format!( + "failed to run bundled ripgrep for unreadable glob scan under {}: {err}", + scan_plan.root.display() + )); + } }; - if !metadata.is_dir() { - return Ok(()); + if !output.status.success() { + if output.status.code() == Some(1) && output.stderr.is_empty() { + return Ok(Some(Vec::new())); + } + if output.status.code() == Some(2) { + // Ripgrep uses exit 2 for traversal errors, including protected + // Windows directories. Its output may be incomplete. The caller + // must enumerate again using the matcher-backed walker; policy + // syntax has already been validated by ReadDenyMatcher::try_new. + return Ok(None); + } + + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(format!( + "ripgrep unreadable glob scan failed under {}: {stderr}", + scan_plan.root.display() + )); } - // Canonical directory keys keep recursive scans from following a symlink or - // junction cycle forever while preserving the original matched path for the - // ACL layer. - let scan_key = dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); - if !seen_scan_dirs.insert(scan_key) { - return Ok(()); - } - - if max_depth.is_some_and(|max_depth| depth >= max_depth) { - return Ok(()); - } - - let Ok(entries) = std::fs::read_dir(path) else { - return Ok(()); - }; - for entry in entries.flatten() { - collect_existing_glob_matches( - &entry.path(), - matcher, - paths, - seen_paths, - seen_scan_dirs, - max_depth, - depth + 1, - )?; - } - - Ok(()) + output + .stdout + .split(|byte| *byte == b'\0') + .filter(|path| !path.is_empty()) + .map(|path| { + let path = std::str::from_utf8(path).map_err(|err| { + format!( + "ripgrep returned a non-UTF-8 path under {}: {err}", + scan_plan.root.display() + ) + })?; + let path = PathBuf::from(path); + Ok(if path.is_absolute() { + path + } else { + scan_plan.root.join(path) + }) + }) + .collect::, _>>() + .map(Some) } fn push_absolute_path( @@ -148,6 +176,37 @@ fn push_absolute_path( Ok(()) } +fn glob_scan_plans( + patterns: &[String], + configured_max_depth: Option, +) -> Result, String> { + let mut scan_plans: Vec = Vec::new(); + + for pattern in patterns { + let mut scan_plan = glob_scan_plan(pattern, configured_max_depth); + if scan_plan.max_depth.is_none() && scan_plan.root.parent().is_none() { + return Err(format!( + "unreadable glob `{pattern}` cannot be safely expanded from a filesystem root without `glob_scan_max_depth`; configure `glob_scan_max_depth` or use a non-root directory prefix" + )); + } + + if let Some(existing) = scan_plans + .iter_mut() + .find(|existing| existing.root == scan_plan.root) + { + existing.max_depth = match (existing.max_depth, scan_plan.max_depth) { + (Some(existing_depth), Some(new_depth)) => Some(existing_depth.max(new_depth)), + _ => None, + }; + existing.globs.append(&mut scan_plan.globs); + } else { + scan_plans.push(scan_plan); + } + } + + Ok(scan_plans) +} + fn glob_scan_plan(pattern: &str, configured_max_depth: Option) -> GlobScanPlan { // Start scanning at the deepest literal directory prefix before the first // glob metacharacter. For example, `C:\repo\**\*.env` only scans `C:\repo` @@ -162,6 +221,7 @@ fn glob_scan_plan(pattern: &str, configured_max_depth: Option) -> GlobSca return GlobScanPlan { root: PathBuf::from("."), max_depth: effective_glob_scan_max_depth(pattern, configured_max_depth), + globs: vec![ripgrep_glob(pattern)], }; }; let pattern_suffix = &pattern[separator_index + 1..]; @@ -174,11 +234,51 @@ fn glob_scan_plan(pattern: &str, configured_max_depth: Option) -> GlobSca return GlobScanPlan { root: PathBuf::from(&literal_prefix[..=separator_index]), max_depth: effective_glob_scan_max_depth(pattern_suffix, configured_max_depth), + globs: vec![ripgrep_glob(pattern_suffix)], }; } GlobScanPlan { root: PathBuf::from(literal_prefix[..separator_index].to_string()), max_depth: effective_glob_scan_max_depth(pattern_suffix, configured_max_depth), + globs: vec![ripgrep_glob(pattern_suffix)], + } +} + +fn ripgrep_glob(pattern: &str) -> String { + let pattern = pattern.replace('\\', "/"); + let mut escaped = String::with_capacity(pattern.len()); + let mut chars = pattern.chars(); + + while let Some(ch) = chars.next() { + if ch != '[' { + escaped.push(ch); + continue; + } + + let mut class = String::new(); + let mut closed = false; + for class_ch in chars.by_ref() { + if class_ch == ']' { + closed = true; + break; + } + class.push(class_ch); + } + + if closed { + escaped.push('['); + escaped.push_str(&class); + escaped.push(']'); + } else { + escaped.push_str(r"\["); + escaped.push_str(&class); + } + } + + if escaped.starts_with("**/") { + escaped + } else { + format!("**/{escaped}") } } @@ -198,10 +298,17 @@ fn effective_glob_scan_max_depth( })) } +#[cfg(test)] +#[path = "deny_read_resolver_access_tests.rs"] +mod access_tests; + #[cfg(test)] mod tests { + use super::GlobScanPlan; use super::glob_scan_plan; + use super::glob_scan_plans; use super::resolve_windows_deny_read_paths; + use super::ripgrep_glob; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; @@ -283,6 +390,102 @@ mod tests { ); } + #[test] + fn glob_patterns_with_the_same_root_and_depth_share_one_scan() { + assert_eq!( + glob_scan_plans( + &[ + "/tmp/work/**/*.env".to_string(), + "/tmp/work/**/*.pem".to_string(), + "/tmp/work/**/*.secret".to_string(), + ], + /*configured_max_depth*/ Some(3), + ) + .expect("combined scan plans"), + vec![GlobScanPlan { + root: PathBuf::from("/tmp/work"), + max_depth: Some(3), + globs: vec![ + "**/*.env".to_string(), + "**/*.pem".to_string(), + "**/*.secret".to_string(), + ], + }] + ); + } + + #[test] + fn bounded_globs_with_the_same_root_share_the_deepest_scan() { + assert_eq!( + glob_scan_plans( + &[ + "/tmp/work/*.env".to_string(), + "/tmp/work/*/*.pem".to_string(), + ], + /*configured_max_depth*/ None, + ) + .expect("combined scan plans"), + vec![GlobScanPlan { + root: PathBuf::from("/tmp/work"), + max_depth: Some(2), + globs: vec!["**/*.env".to_string(), "**/*/*.pem".to_string()], + }] + ); + } + + #[test] + fn recursive_and_bounded_globs_with_the_same_root_share_one_scan() { + assert_eq!( + glob_scan_plans( + &[ + "/tmp/work/*.env".to_string(), + "/tmp/work/**/*.pem".to_string(), + ], + /*configured_max_depth*/ None, + ) + .expect("combined scan plans"), + vec![GlobScanPlan { + root: PathBuf::from("/tmp/work"), + max_depth: None, + globs: vec!["**/*.env".to_string(), "**/*.pem".to_string()], + }] + ); + } + + #[test] + fn project_recursive_globs_without_depth_expand_existing_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let secret = tmp.path().join("nested").join("secret.env"); + std::fs::create_dir_all(secret.parent().expect("secret parent")).expect("create parent"); + std::fs::write(&secret, "secret").expect("write secret"); + let pattern = tmp.path().join("**").join("*.env").display().to_string(); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(pattern)]); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("unbounded project glob"), + vec![AbsolutePathBuf::from_absolute_path(secret).expect("absolute secret")] + ); + } + + #[test] + fn relative_recursive_globs_without_depth_expand_existing_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let secret = tmp.path().join("nested").join("secret.env"); + std::fs::create_dir_all(secret.parent().expect("secret parent")).expect("create parent"); + std::fs::write(&secret, "secret").expect("write secret"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry( + "**/*.env".to_string(), + )]); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd) + .expect("unbounded relative project glob"), + vec![AbsolutePathBuf::from_absolute_path(secret).expect("absolute secret")] + ); + } + #[test] fn root_recursive_globs_without_depth_fail_before_expansion() { let tmp = TempDir::new().expect("tempdir"); @@ -358,6 +561,127 @@ mod tests { assert_eq!(actual, expected); } + #[test] + fn recursive_globs_include_hidden_and_ignored_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let hidden_secret = tmp.path().join("nested").join(".env"); + let ignored_secret = tmp.path().join("nested").join("ignored.env"); + std::fs::create_dir_all(hidden_secret.parent().expect("secret parent")) + .expect("create parent"); + std::fs::write(tmp.path().join(".ignore"), "ignored.env\n").expect("write ignore file"); + std::fs::write(&hidden_secret, "secret").expect("write hidden secret"); + std::fs::write(&ignored_secret, "secret").expect("write ignored secret"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry( + "**/*.env".to_string(), + )]); + + let actual: HashSet = resolve_windows_deny_read_paths(&policy, &cwd) + .expect("resolve hidden and ignored matches") + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect(); + + assert_eq!( + actual, + [hidden_secret, ignored_secret].into_iter().collect() + ); + } + + #[test] + fn recursive_globs_preserve_matching_empty_directories() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let root_secret = tmp.path().join("root.env"); + let nested_secret = tmp.path().join("nested").join("empty.env"); + std::fs::create_dir_all(&root_secret).expect("create root secret directory"); + std::fs::create_dir_all(&nested_secret).expect("create nested secret directory"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry( + "**/*.env".to_string(), + )]); + + let actual: HashSet = resolve_windows_deny_read_paths(&policy, &cwd) + .expect("resolve matching directories") + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect(); + + assert_eq!(actual, [root_secret, nested_secret].into_iter().collect()); + } + + #[test] + fn configured_depth_excludes_deeper_recursive_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let shallow_secret = tmp.path().join("shallow.env"); + let deep_secret = tmp.path().join("nested").join("deep.env"); + std::fs::create_dir_all(deep_secret.parent().expect("secret parent")) + .expect("create parent"); + std::fs::write(&shallow_secret, "secret").expect("write shallow secret"); + std::fs::write(&deep_secret, "secret").expect("write deep secret"); + let mut policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry( + "**/*.env".to_string(), + )]); + policy.glob_scan_max_depth = Some(1); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve depth-limited glob"), + vec![AbsolutePathBuf::from_absolute_path(shallow_secret).expect("absolute secret")] + ); + } + + #[test] + fn recursive_globs_rescan_for_new_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + std::fs::write(tmp.path().join("notes.txt"), "notes").expect("write notes"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry( + "**/*.env".to_string(), + )]); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve unmatched glob"), + Vec::::new() + ); + let secret = tmp.path().join("new.env"); + std::fs::write(&secret, "new secret").expect("create matching file"); + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("fresh resolution"), + vec![AbsolutePathBuf::from_absolute_path(secret).expect("secret path")] + ); + } + + #[test] + fn ripgrep_globs_normalize_windows_separators_and_unclosed_classes() { + assert_eq!(ripgrep_glob(r"**\*.env"), "**/*.env"); + assert_eq!(ripgrep_glob("nested/*.env"), "**/nested/*.env"); + assert_eq!(ripgrep_glob("[*.env"), r"**/\[*.env"); + } + + #[test] + fn shared_scan_roots_preserve_matches_at_each_required_depth() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let env = tmp.path().join(".env"); + let pem = tmp.path().join("nested").join("credentials.pem"); + std::fs::create_dir_all(pem.parent().expect("pem parent")).expect("create pem parent"); + std::fs::write(&env, "secret").expect("write env"); + std::fs::write(&pem, "secret").expect("write pem"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + unreadable_glob_entry(format!("{}/*.env", tmp.path().display())), + unreadable_glob_entry(format!("{}/*/*.pem", tmp.path().display())), + ]); + + let actual: HashSet = resolve_windows_deny_read_paths(&policy, &cwd) + .expect("resolve shared root") + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect(); + let expected = [env, pem].into_iter().collect(); + + assert_eq!(actual, expected); + } + #[test] fn invalid_glob_patterns_fail_before_expansion() { let tmp = TempDir::new().expect("tempdir"); @@ -424,4 +748,23 @@ mod tests { assert_eq!(actual, expected); } + + #[cfg(unix)] + #[test] + fn recursive_globs_preserve_symlinked_file_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let target = tmp.path().join("target.txt"); + let alias = tmp.path().join("linked.env"); + std::fs::write(&target, "secret").expect("write symlink target"); + symlink(&target, &alias).expect("create symlink"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry( + "**/*.env".to_string(), + )]); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve symlinked secret"), + vec![AbsolutePathBuf::from_absolute_path(alias).expect("absolute symlink")] + ); + } } diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_resolver_access_tests.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver_access_tests.rs new file mode 100644 index 0000000000..f473c9f96e --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver_access_tests.rs @@ -0,0 +1,242 @@ +use super::resolve_windows_deny_read_paths; +use super::walker::DirectoryScanMode; +use super::walker::collect_existing_glob_directory_matches; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::ReadDenyMatcher; +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; +use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; +use tempfile::TempDir; + +fn policy() -> FileSystemSandboxPolicy { + FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::Deny, + missing_path_behavior: None, + }]) +} + +struct ProtectedDirectory { + path: PathBuf, + #[cfg(unix)] + permissions: std::fs::Permissions, +} + +impl ProtectedDirectory { + fn new(path: &Path) -> Self { + #[cfg(unix)] + let permissions = { + use std::os::unix::fs::PermissionsExt; + + let permissions = path.metadata().expect("directory metadata").permissions(); + std::fs::set_permissions(path, std::fs::Permissions::from_mode(/*mode*/ 0o000)) + .expect("protect directory"); + permissions + }; + #[cfg(windows)] + assert!( + std::process::Command::new("icacls.exe") + .arg(path) + .args(["/deny", "*S-1-1-0:(OI)(CI)R"]) + .output() + .expect("protect directory for every account") + .status + .success() + ); + Self { + path: path.to_path_buf(), + #[cfg(unix)] + permissions, + } + } +} + +impl Drop for ProtectedDirectory { + fn drop(&mut self) { + #[cfg(unix)] + std::fs::set_permissions(&self.path, self.permissions.clone()) + .expect("restore directory permissions"); + #[cfg(windows)] + assert!( + std::process::Command::new("icacls.exe") + .arg(&self.path) + .args(["/remove:d", "*S-1-1-0"]) + .output() + .expect("restore directory permissions") + .status + .success() + ); + } +} + +#[test] +fn protected_directory_does_not_discard_accessible_deny_matches() { + #[cfg(windows)] + if let Some(root) = std::env::var_os("CODEX_TEST_DENY_READ_RESTRICTED_ROOT") { + assert_accessible_deny_matches(Path::new(&root)); + return; + } + + let temp = TempDir::new().expect("tempdir"); + let protected = temp.path().join("protected"); + let readable = temp.path().join("readable"); + std::fs::create_dir(&protected).expect("protected directory"); + std::fs::create_dir(&readable).expect("readable directory"); + std::fs::write(protected.join("hidden.env"), "protected").expect("protected file"); + std::fs::write(readable.join("secret.env"), "secret").expect("readable secret"); + std::fs::create_dir(readable.join("empty.env")).expect("matching empty directory"); + std::fs::write(readable.join("public.txt"), "public").expect("public file"); + let guard = ProtectedDirectory::new(&protected); + + #[cfg(windows)] + { + // CI enables backup privileges, which let directory enumeration bypass + // this fixture's ACL. Restrict a separate process, including its rg child, + // rather than changing the shared test process's privileges. The parent + // owns ACL setup and cleanup, which the restricted child cannot perform. + let codex_home = TempDir::new().expect("restricted codex home"); + let cwd = AbsolutePathBuf::from_absolute_path(temp.path()).expect("workspace"); + let mut env = std::collections::HashMap::from([ + ( + "CODEX_TEST_DENY_READ_RESTRICTED_ROOT".to_string(), + cwd.display().to_string(), + ), + ("TEMP".to_string(), cwd.display().to_string()), + ("TMP".to_string(), cwd.display().to_string()), + ]); + for name in ["PATH", "SystemRoot", "COMSPEC", "USERPROFILE"] { + if let Ok(value) = std::env::var(name) { + env.insert(name.to_string(), value); + } + } + let result = crate::run_windows_sandbox_capture( + &codex_protocol::models::PermissionProfile::workspace_write(), + std::slice::from_ref(&cwd), + codex_home.path(), + vec![ + std::env::current_exe() + .expect("test binary") + .display() + .to_string(), + "--exact".to_string(), + std::thread::current() + .name() + .expect("test name") + .to_string(), + ], + cwd.as_path(), + env, + /*timeout_ms*/ Some(30_000), + /*cancellation*/ None, + /*use_private_desktop*/ true, + ) + .expect("run protected-directory regression without backup privileges"); + let stdout = String::from_utf8_lossy(&result.stdout); + let stderr = String::from_utf8_lossy(&result.stderr); + assert_eq!( + (result.exit_code, result.timed_out), + (0, false), + "{stdout}\n{stderr}" + ); + assert!(stdout.contains("test result: ok. 1 passed"), "{stdout}"); + } + + #[cfg(not(windows))] + assert_accessible_deny_matches(temp.path()); + drop(guard); +} + +fn assert_accessible_deny_matches(root: &Path) { + let cwd = AbsolutePathBuf::from_absolute_path(root).expect("cwd"); + let protected = root.join("protected"); + let enumeration = std::fs::read_dir(&protected) + .and_then(std::iter::Iterator::collect::>>); + assert_eq!( + enumeration + .expect_err("fixture must deny directory enumeration") + .kind(), + std::io::ErrorKind::PermissionDenied + ); + + let actual = resolve_windows_deny_read_paths(&policy(), &cwd) + .expect("protected branches must not abort expansion of accessible paths") + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect::>(); + assert_eq!( + actual, + HashSet::from([ + root.join("readable/secret.env"), + root.join("readable/empty.env"), + ]) + ); +} + +#[test] +fn native_walk_preserves_canonical_matches_through_directory_links_and_cycles() { + let temp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(temp.path()).expect("cwd"); + let walk = temp.path().join("walk"); + let target = temp.path().join("target"); + std::fs::create_dir(&walk).expect("walk root"); + std::fs::create_dir(&target).expect("link target"); + std::fs::write(target.join("secret.env"), "secret").expect("secret"); + std::fs::write(target.join("public.txt"), "public").expect("public"); + let alias = walk.join("alias"); + let cycle = target.join("cycle"); + for (link, destination) in [(&alias, &target), (&cycle, &walk)] { + #[cfg(unix)] + std::os::unix::fs::symlink(destination, link).expect("directory symlink"); + #[cfg(windows)] + assert!( + std::process::Command::new("cmd.exe") + .args(["/d", "/c", "mklink", "/J"]) + .arg(link) + .arg(destination) + .output() + .expect("directory junction") + .status + .success() + ); + } + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: target + .canonicalize() + .expect("canonical target") + .join("**") + .join("*.env") + .display() + .to_string(), + }, + access: FileSystemAccessMode::Deny, + missing_path_behavior: None, + }]); + let matcher = ReadDenyMatcher::try_new(&policy, cwd.as_path()) + .expect("valid matcher") + .expect("deny restriction"); + assert!(matcher.is_read_denied(&alias.join("secret.env"))); + let mut paths = Vec::new(); + collect_existing_glob_directory_matches( + &walk, + &matcher, + &mut paths, + &mut HashSet::new(), + /*max_depth*/ None, + DirectoryScanMode::IncludeAllFiles, + ) + .expect("walk through directory alias and cycle"); + assert_eq!( + paths, + vec![ + AbsolutePathBuf::from_absolute_path(alias.join("secret.env")).expect("lexical target") + ] + ); +} diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_walker.rs b/codex-rs/windows-sandbox-rs/src/deny_read_walker.rs new file mode 100644 index 0000000000..cb3e911ae0 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_walker.rs @@ -0,0 +1,151 @@ +//! Matcher-backed snapshot traversal for Windows deny-read ACL targets. +//! +//! The accessible-directory behavior is the same as the original native +//! resolver: disappearing and inaccessible branches cannot be enumerated, but +//! a matching directory is retained before reading its children. This does not +//! grant access to a skipped branch. Unexpected I/O errors abort the snapshot. +//! Canonical parents are reused only within this walk; links are resolved anew. + +use super::push_absolute_path; +use codex_protocol::permissions::ReadDenyMatcher; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashSet; +use std::fs::FileType; +use std::io; +use std::path::Path; +use std::path::PathBuf; + +#[derive(Clone, Copy)] +pub(super) enum DirectoryScanMode { + DirectoriesOnly, + IncludeAllFiles, +} + +pub(super) fn collect_existing_glob_directory_matches( + root: &Path, + matcher: &ReadDenyMatcher, + paths: &mut Vec, + seen_paths: &mut HashSet, + max_depth: Option, + mode: DirectoryScanMode, +) -> Result<(), String> { + let Some(metadata) = accessible_entry(root.metadata(), root)? else { + return Ok(()); + }; + let canonical = accessible_entry(root.canonicalize(), root)?; + let mut walker = GlobWalker { + matcher, + paths, + seen_paths, + seen_directories: HashSet::new(), + max_depth, + }; + walker.visit( + root, + canonical.as_deref(), + metadata.file_type(), + /*depth*/ 0, + mode, + ) +} + +struct GlobWalker<'a> { + matcher: &'a ReadDenyMatcher, + paths: &'a mut Vec, + seen_paths: &'a mut HashSet, + seen_directories: HashSet, + max_depth: Option, +} + +impl GlobWalker<'_> { + fn visit( + &mut self, + path: &Path, + canonical: Option<&Path>, + file_type: FileType, + depth: usize, + mode: DirectoryScanMode, + ) -> Result<(), String> { + let denied = match canonical { + Some(canonical) => self + .matcher + .is_read_denied_with_canonical_path(path, canonical), + None => self.matcher.is_read_denied(path), + }; + if denied { + push_absolute_path(self.paths, self.seen_paths, path.to_path_buf())?; + } + if !file_type.is_dir() + || self.max_depth.is_some_and(|limit| depth >= limit) + || !self + .seen_directories + .insert(canonical.unwrap_or(path).to_path_buf()) + { + return Ok(()); + } + let Some(entries) = accessible_entry(std::fs::read_dir(path), path)? else { + return Ok(()); + }; + for entry in entries { + let Some(entry) = accessible_entry(entry, path)? else { + continue; + }; + let entry_path = entry.path(); + let Some(file_type) = accessible_entry(entry.file_type(), &entry_path)? else { + continue; + }; + if file_type.is_symlink() { + // Name-surrogate reparse points (including Windows junctions) + // cannot inherit the parent's canonical location. Resolve the + // target, then retain lexical matches and detect target cycles. + let target = accessible_entry(entry_path.canonicalize(), &entry_path)?; + let Some(metadata) = accessible_entry(entry_path.metadata(), &entry_path)? else { + continue; + }; + self.visit( + &entry_path, + target.as_deref(), + metadata.file_type(), + depth + 1, + DirectoryScanMode::IncludeAllFiles, + )?; + } else if file_type.is_dir() || matches!(mode, DirectoryScanMode::IncludeAllFiles) { + // Directory entries already carry their type on Windows. A + // normal child has the resolved parent's location plus its + // enumerated name; do not stat/canonicalize every ordinary file. + self.visit( + &entry_path, + canonical + .map(|parent| parent.join(entry.file_name())) + .as_deref(), + file_type, + depth + 1, + mode, + )?; + } + } + Ok(()) + } +} + +fn accessible_entry(result: io::Result, path: &Path) -> Result, String> { + match result { + Ok(value) => Ok(Some(value)), + Err(err) + if matches!( + err.kind(), + io::ErrorKind::NotFound | io::ErrorKind::PermissionDenied + ) => + { + Ok(None) + } + Err(err) => Err(format!( + "failed to enumerate unreadable glob paths under {}: {err}", + path.display() + )), + } +} + +#[cfg(test)] +#[path = "deny_read_walker_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_walker_tests.rs b/codex-rs/windows-sandbox-rs/src/deny_read_walker_tests.rs new file mode 100644 index 0000000000..8836197762 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_walker_tests.rs @@ -0,0 +1,17 @@ +use super::accessible_entry; +use pretty_assertions::assert_eq; +use std::io; +use std::path::Path; + +#[test] +fn unexpected_io_errors_abort_the_snapshot() { + let error = io::Error::other("directory read interrupted"); + + assert_eq!( + accessible_entry::<()>(Err(error), Path::new("scan-root")), + Err( + "failed to enumerate unreadable glob paths under scan-root: directory read interrupted" + .to_string() + ) + ); +}