mirror of
https://github.com/openai/codex.git
synced 2026-10-21 05:22:21 +00:00
Scope session MCP approvals to app account links (#42133)
## Why Remembering an app tool approval by connector and tool alone could reuse an approval when the same tool was called with a different selected account. ## What changed Include the app `link_id` in MCP tool approval keys so remembered session approvals apply only to the account link that was approved. Calls for another link, or calls without a link selector, request their own approval. ## Testing Add an integration test covering calls across two link IDs and the legacy no-selector case. GitOrigin-RevId: 7dd88330b75de312469020d5892dfb2fa5d5fada
This commit is contained in:
committed by
copyberry
parent
8436b749a4
commit
2393b5c920
@@ -1373,6 +1373,7 @@ const MCP_TOOL_APPROVAL_CANCEL: &str = "Cancel";
|
||||
struct McpToolApprovalKey {
|
||||
server: String,
|
||||
connector_id: Option<String>,
|
||||
link_id: Option<String>,
|
||||
tool_name: String,
|
||||
}
|
||||
|
||||
@@ -1636,6 +1637,7 @@ fn session_mcp_tool_approval_key(
|
||||
Some(McpToolApprovalKey {
|
||||
server: invocation.server.clone(),
|
||||
connector_id,
|
||||
link_id: metadata.and_then(|metadata| metadata.link_id.clone()),
|
||||
tool_name: invocation.tool.clone(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -911,6 +911,7 @@ fn custom_servers_support_session_and_persistent_approval() {
|
||||
let expected = McpToolApprovalKey {
|
||||
server: "custom_server".to_string(),
|
||||
connector_id: None,
|
||||
link_id: None,
|
||||
tool_name: "run_action".to_string(),
|
||||
};
|
||||
|
||||
@@ -935,16 +936,18 @@ fn codex_apps_connectors_support_persistent_approval() {
|
||||
tool: "calendar/list_events".to_string(),
|
||||
arguments: None,
|
||||
};
|
||||
let metadata = approval_metadata(
|
||||
let mut metadata = approval_metadata(
|
||||
Some("calendar"),
|
||||
Some("Calendar"),
|
||||
/*connector_description*/ None,
|
||||
/*tool_title*/ None,
|
||||
/*tool_description*/ None,
|
||||
);
|
||||
metadata.link_id = Some("link_a".to_string());
|
||||
let expected = McpToolApprovalKey {
|
||||
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
link_id: Some("link_a".to_string()),
|
||||
tool_name: "calendar/list_events".to_string(),
|
||||
};
|
||||
|
||||
@@ -2339,6 +2342,7 @@ async fn maybe_persist_mcp_tool_approval_reloads_session_config() {
|
||||
let key = McpToolApprovalKey {
|
||||
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
link_id: None,
|
||||
tool_name: "calendar/list_events".to_string(),
|
||||
};
|
||||
|
||||
@@ -2389,6 +2393,7 @@ async fn maybe_persist_mcp_tool_approval_reloads_session_config_for_custom_serve
|
||||
let key = McpToolApprovalKey {
|
||||
server: "docs".to_string(),
|
||||
connector_id: None,
|
||||
link_id: None,
|
||||
tool_name: "search".to_string(),
|
||||
};
|
||||
|
||||
@@ -2445,6 +2450,7 @@ enabled = true
|
||||
let key = McpToolApprovalKey {
|
||||
server: "sample".to_string(),
|
||||
connector_id: None,
|
||||
link_id: None,
|
||||
tool_name: "search".to_string(),
|
||||
};
|
||||
|
||||
@@ -2501,6 +2507,7 @@ async fn maybe_persist_mcp_tool_approval_writes_project_config_for_project_serve
|
||||
let key = McpToolApprovalKey {
|
||||
server: "docs".to_string(),
|
||||
connector_id: None,
|
||||
link_id: None,
|
||||
tool_name: "search".to_string(),
|
||||
};
|
||||
|
||||
|
||||
@@ -56,6 +56,8 @@ use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
use test_case::test_case;
|
||||
use wiremock::Mock;
|
||||
use wiremock::Request;
|
||||
@@ -64,6 +66,177 @@ use wiremock::matchers::body_partial_json;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path_regex;
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn auto_session_approval_is_scoped_to_tool_link_id() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let apps_server = AppsTestServer::mount(&server).await?;
|
||||
let has_link_selector = Arc::new(Mutex::new(true));
|
||||
let lists_link_selector = Arc::clone(&has_link_selector);
|
||||
Mock::given(method("POST"))
|
||||
.and(path_regex("^/api/codex/ps/mcp/?$"))
|
||||
.and(body_partial_json(json!({ "method": "tools/list" })))
|
||||
.respond_with(move |request: &Request| {
|
||||
let body: Value = serde_json::from_slice(&request.body).expect("valid tools/list");
|
||||
let mut tool = json!({
|
||||
"name": "calendar_create_event",
|
||||
"description": "Create a calendar event.",
|
||||
"annotations": { "readOnlyHint": false },
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"title": { "type": "string" },
|
||||
"starts_at": { "type": "string" }
|
||||
},
|
||||
"required": ["title", "starts_at"]
|
||||
},
|
||||
"_meta": {
|
||||
"connector_id": "calendar",
|
||||
"connector_name": "Calendar",
|
||||
"_codex_apps": {
|
||||
"resource_uri": "connector://calendar/tools/calendar_create_event",
|
||||
"contains_mcp_source": true,
|
||||
"connector_id": "calendar"
|
||||
}
|
||||
}
|
||||
});
|
||||
if *lists_link_selector.lock().unwrap() {
|
||||
// The catalog account stays fixed while each call selects its account.
|
||||
tool["_meta"]["link_id"] = json!("link_a");
|
||||
tool["_meta"]["_codex_apps"]["requires_explicit_link_id"] = json!(true);
|
||||
tool["inputSchema"]["properties"]["link_id"] = json!({ "type": "string" });
|
||||
tool["inputSchema"]["required"] = json!(["title", "starts_at", "link_id"]);
|
||||
}
|
||||
ResponseTemplate::new(/*status*/ 200).set_body_json(json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": body["id"],
|
||||
"result": { "tools": [tool] }
|
||||
}))
|
||||
})
|
||||
.with_priority(/*priority*/ 1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let cases = [
|
||||
(Some("link_a"), true),
|
||||
(Some("link_b"), true),
|
||||
(None, true),
|
||||
(Some("link_a"), false),
|
||||
];
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
cases
|
||||
.iter()
|
||||
.enumerate()
|
||||
.flat_map(|(index, (selected_link_id, _))| {
|
||||
let call_id = format!("calendar-call-{index}");
|
||||
let mut calendar_args = json!({
|
||||
"title": "Lunch",
|
||||
"starts_at": "2026-03-10T12:00:00Z"
|
||||
});
|
||||
if let Some(selected_link_id) = selected_link_id {
|
||||
calendar_args["link_id"] = json!(selected_link_id);
|
||||
}
|
||||
[
|
||||
sse(vec![
|
||||
ev_response_created(&call_id),
|
||||
ev_function_call_with_namespace(
|
||||
&call_id,
|
||||
SEARCH_CALENDAR_NAMESPACE,
|
||||
SEARCH_CALENDAR_CREATE_TOOL,
|
||||
&calendar_args.to_string(),
|
||||
),
|
||||
ev_completed(&call_id),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-done"),
|
||||
ev_assistant_message("msg-done", "done"),
|
||||
ev_completed("resp-done"),
|
||||
]),
|
||||
]
|
||||
})
|
||||
.collect(),
|
||||
)
|
||||
.await;
|
||||
let test = search_capable_apps_builder(apps_server.chatgpt_base_url)
|
||||
.with_config(move |config| {
|
||||
set_calendar_approval_mode(config, AppToolApproval::Auto);
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::ToolCallMcpElicitation)
|
||||
.expect("test config should allow feature update");
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await?;
|
||||
|
||||
for (index, (selected_link_id, expects_prompt)) in cases.into_iter().enumerate() {
|
||||
*has_link_selector.lock().unwrap() = selected_link_id.is_some();
|
||||
// Reconnect to publish the account selector or legacy metadata without a link.
|
||||
test.codex.submit(Op::RefreshMcpServers).await?;
|
||||
submit_user_turn(
|
||||
&test,
|
||||
"Use [$calendar](app://calendar) to create a calendar event.",
|
||||
AskForApproval::OnRequest,
|
||||
PermissionProfile::Disabled,
|
||||
/*collaboration_mode*/ None,
|
||||
)
|
||||
.await?;
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
assert_eq!(
|
||||
matches!(event, EventMsg::ElicitationRequest(_)),
|
||||
expects_prompt,
|
||||
"unexpected approval for call {index} with link {selected_link_id:?}"
|
||||
);
|
||||
if let EventMsg::ElicitationRequest(request) = event {
|
||||
test.codex
|
||||
.submit(Op::ResolveElicitation {
|
||||
server_name: request.server_name,
|
||||
request_id: request.id,
|
||||
decision: ElicitationAction::Accept,
|
||||
content: None,
|
||||
meta: Some(json!({ "persist": "session" })),
|
||||
})
|
||||
.await?;
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
}
|
||||
}
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), cases.len() * 2);
|
||||
for (index, request) in requests.iter().skip(1).step_by(2).enumerate() {
|
||||
let call_id = format!("calendar-call-{index}");
|
||||
let output = request
|
||||
.function_call_output_text(&call_id)
|
||||
.expect("MCP tool output");
|
||||
let (_, result) = output.split_once("\nOutput:\n").expect("MCP output header");
|
||||
let result: Value = serde_json::from_str(result)?;
|
||||
let mut expected_result = json!({
|
||||
"_codex_apps": {
|
||||
"call_id": call_id,
|
||||
"connector_id": "calendar",
|
||||
"contains_mcp_source": true,
|
||||
"resource_uri": "connector://calendar/tools/calendar_create_event"
|
||||
}
|
||||
});
|
||||
if cases[index].0.is_some() {
|
||||
expected_result["_codex_apps"]["requires_explicit_link_id"] = json!(true);
|
||||
}
|
||||
assert_eq!(result, expected_result);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn set_calendar_approval_mode(config: &mut Config, approval_mode: AppToolApproval) {
|
||||
let approval_mode = match approval_mode {
|
||||
AppToolApproval::Auto => "auto",
|
||||
|
||||
Reference in New Issue
Block a user