mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Reject obsolete app-server permission profile fields (#38919)
## Why App-server request deserialization otherwise ignores unknown fields, so clients using the removed `permissionProfile` field could have their requested permission settings silently ignored. ## What changed - Reject `permissionProfile` with an invalid-params error on `thread/start`, `thread/resume`, `thread/fork`, and `turn/start`. - Direct clients to select a named profile through `permissions` instead. - Continue accepting unrelated unknown fields for forward compatibility. ## Testing - Cover rejection of the obsolete field on all four affected methods and verify that the connection remains usable. - Cover successful `thread/start` requests that combine named `permissions` with an unknown field. GitOrigin-RevId: a63764fb899bc5f212d60617dc32e911bde8073e
This commit is contained in:
@@ -8,6 +8,7 @@ use crate::attestation::app_server_attestation_provider;
|
||||
use crate::config_manager::ConfigManager;
|
||||
use crate::connection_rpc_gate::ConnectionRpcGate;
|
||||
use crate::current_time::app_server_time_provider;
|
||||
use crate::error_code::invalid_params;
|
||||
use crate::error_code::invalid_request;
|
||||
use crate::extensions::ThreadExtensionDependencies;
|
||||
use crate::extensions::app_server_extension_event_sink;
|
||||
@@ -97,10 +98,36 @@ use crate::models_refresh_worker::ModelsRefreshWorker;
|
||||
const CONNECTION_RPC_DRAIN_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30);
|
||||
|
||||
fn deserialize_client_request(request: JSONRPCRequest) -> Result<ClientRequest, JSONRPCErrorError> {
|
||||
reject_obsolete_request_fields(&request)?;
|
||||
|
||||
ClientRequest::try_from(request)
|
||||
.map_err(|err| invalid_request(format!("Invalid request: {err}")))
|
||||
}
|
||||
|
||||
fn reject_obsolete_request_fields(request: &JSONRPCRequest) -> Result<(), JSONRPCErrorError> {
|
||||
reject_removed_permission_profile(request)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn reject_removed_permission_profile(request: &JSONRPCRequest) -> Result<(), JSONRPCErrorError> {
|
||||
if matches!(
|
||||
request.method.as_str(),
|
||||
"thread/start" | "thread/resume" | "thread/fork" | "turn/start"
|
||||
) && request
|
||||
.params
|
||||
.as_ref()
|
||||
.and_then(serde_json::Value::as_object)
|
||||
.is_some_and(|params| params.contains_key("permissionProfile"))
|
||||
{
|
||||
let method = request.method.as_str();
|
||||
return Err(invalid_params(format!(
|
||||
"`permissionProfile` is no longer supported for `{method}`; use `permissions` with a named profile id instead"
|
||||
)));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) struct MessageProcessor {
|
||||
outgoing: Arc<OutgoingMessageSender>,
|
||||
models_refresh_worker: ModelsRefreshWorker,
|
||||
|
||||
@@ -1,9 +1,14 @@
|
||||
use anyhow::Result;
|
||||
use app_test_support::TestAppServer;
|
||||
use app_test_support::write_mock_responses_config_toml_with_chatgpt_base_url;
|
||||
use codex_app_server::INVALID_PARAMS_ERROR_CODE;
|
||||
use codex_app_server_protocol::ActivePermissionProfile;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::SandboxPolicy;
|
||||
use codex_app_server_protocol::ThreadLoadedListParams;
|
||||
use codex_app_server_protocol::ThreadLoadedListResponse;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::ThreadStartResponse;
|
||||
use codex_protocol::models::FunctionCallOutputContentItem;
|
||||
@@ -20,6 +25,127 @@ const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
const REMOTE_IMAGE_URL_ERROR: &str =
|
||||
"remote image URLs are not supported; use an inline data URL instead";
|
||||
|
||||
#[tokio::test]
|
||||
async fn legacy_permission_profile_requests_fail_closed() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.build_initialized_with_timeout(DEFAULT_READ_TIMEOUT)
|
||||
.await?;
|
||||
let legacy_permission_profile = json!({
|
||||
"network": { "enabled": false },
|
||||
"fileSystem": {
|
||||
"entries": [{
|
||||
"path": {
|
||||
"type": "special",
|
||||
"value": { "kind": "root" }
|
||||
},
|
||||
"access": "read"
|
||||
}]
|
||||
}
|
||||
});
|
||||
let requests = [
|
||||
(
|
||||
"thread/start",
|
||||
json!({
|
||||
"permissionProfile": legacy_permission_profile.clone()
|
||||
}),
|
||||
),
|
||||
(
|
||||
"thread/resume",
|
||||
json!({
|
||||
"threadId": "missing-thread",
|
||||
"permissions": ":read-only",
|
||||
"permissionProfile": legacy_permission_profile.clone()
|
||||
}),
|
||||
),
|
||||
(
|
||||
"thread/fork",
|
||||
json!({
|
||||
"threadId": "missing-thread",
|
||||
"permissionProfile": legacy_permission_profile.clone()
|
||||
}),
|
||||
),
|
||||
(
|
||||
"turn/start",
|
||||
json!({
|
||||
"threadId": "missing-thread",
|
||||
"input": [],
|
||||
"permissionProfile": legacy_permission_profile
|
||||
}),
|
||||
),
|
||||
];
|
||||
|
||||
for (method, params) in requests {
|
||||
let request_id = mcp.send_raw_request(method, Some(params)).await?;
|
||||
let actual: JSONRPCError = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_error_message(RequestId::Integer(request_id)),
|
||||
)
|
||||
.await??;
|
||||
let expected = JSONRPCError {
|
||||
id: RequestId::Integer(request_id),
|
||||
error: JSONRPCErrorError {
|
||||
code: INVALID_PARAMS_ERROR_CODE,
|
||||
data: None,
|
||||
message: format!(
|
||||
"`permissionProfile` is no longer supported for `{method}`; use `permissions` with a named profile id instead"
|
||||
),
|
||||
},
|
||||
};
|
||||
assert_eq!(actual, expected, "unexpected response for {method}");
|
||||
}
|
||||
|
||||
let request_id = mcp
|
||||
.send_thread_loaded_list_request(ThreadLoadedListParams::default())
|
||||
.await?;
|
||||
let actual: ThreadLoadedListResponse =
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(request_id)).await??;
|
||||
assert_eq!(
|
||||
actual,
|
||||
ThreadLoadedListResponse {
|
||||
data: Vec::new(),
|
||||
next_cursor: None,
|
||||
}
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_keeps_unknown_field_compatibility_with_named_permissions() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.build_initialized_with_timeout(DEFAULT_READ_TIMEOUT)
|
||||
.await?;
|
||||
|
||||
let request_id = mcp
|
||||
.send_raw_request(
|
||||
"thread/start",
|
||||
Some(json!({
|
||||
"permissions": ":read-only",
|
||||
"futureField": { "ignored": true }
|
||||
})),
|
||||
)
|
||||
.await?;
|
||||
let actual: ThreadStartResponse =
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(request_id)).await??;
|
||||
|
||||
assert_eq!(
|
||||
actual.sandbox,
|
||||
SandboxPolicy::ReadOnly {
|
||||
network_access: false,
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
actual.active_permission_profile,
|
||||
Some(ActivePermissionProfile::read_only())
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn request_handlers_reject_remote_image_urls() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
Reference in New Issue
Block a user