From edce333620a1981faf72550f241e63bf7fbaba44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Sun, 13 Sep 2026 13:04:46 -0400 Subject: [PATCH] fix(subagent): project-agent mcpServers follow folder trust; agent names are one path component A project-scoped agent definition from an untrusted repository could declare inline stdio mcpServers and the child session started that command, while inline hooks on the same definition were already gated on folder trust. The mcpServers list now uses the same gate. By-name agent lookup joined the requested name onto each searched directory, so a name carrying a separator or ".." resolved a file outside that directory under its trust scope; both lookup entry points now reject any name that is not a single path component. Ported from upstream (grok-build subagent/handle_request.rs mcpServers gate), extended with the name guard. Claude-Session: https://claude.ai/code/session_01RUqV8HVGXcAETyYbZ2tPQ4 --- crates/codegen/kigi-agent/src/discovery.rs | 37 +++++++++++++++++++ .../src/agent/subagent/handle_request.rs | 23 ++++++++---- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/crates/codegen/kigi-agent/src/discovery.rs b/crates/codegen/kigi-agent/src/discovery.rs index 740e97c..9f5cae4 100644 --- a/crates/codegen/kigi-agent/src/discovery.rs +++ b/crates/codegen/kigi-agent/src/discovery.rs @@ -252,11 +252,20 @@ pub fn by_name(name: &str) -> Option { by_name_with_home(name, dirs::home_dir().as_deref(), kigi.as_deref()) } +/// An agent name is one file stem: a separator or `..` would resolve a file outside the searched directory with that directory's trust scope. +fn is_single_path_component(name: &str) -> bool { + !name.is_empty() && name != "." && name != ".." && !name.contains(['/', '\\']) +} + fn by_name_with_home( name: &str, home: Option<&Path>, kigi_home: Option<&Path>, ) -> Option { + if !is_single_path_component(name) { + tracing::warn!(name, "rejected agent name: not a single path component"); + return None; + } // Check built-ins first — type-safe via BuiltinAgentName strum enum. // Legacy pre-rebrand agent types (persisted in old session files) are // mapped onto their current names first; see `canonical_agent_type`. @@ -298,6 +307,10 @@ fn by_name_in_cwd_with_home( home: Option<&Path>, kigi_home: Option<&Path>, ) -> Option { + if !is_single_path_component(name) { + tracing::warn!(name, "rejected agent name: not a single path component"); + return None; + } if let Some(def) = load_project_definition_by_name(name, cwd) { return Some(def); } @@ -827,6 +840,30 @@ mod tests { assert!(def.is_none()); } + /// `../outside` must not load a file beyond the agents directory under that directory's scope. + #[test] + fn by_name_rejects_names_that_leave_the_agents_directory() { + let tmp = tempfile::tempdir().unwrap(); + let project = tmp.path().join("project"); + let agents_dir = project.join(".kigi").join("agents"); + fs::create_dir_all(&agents_dir).unwrap(); + write_agent_file( + &project, + "outside.md", + "outside", + "Lives outside the agents dir", + ); + write_agent_file(&agents_dir, "inside.md", "inside", "Lives inside"); + assert!(by_name_in_cwd_with_home("../../outside", &project, None, None).is_none()); + assert!(by_name_in_cwd_with_home("..", &project, None, None).is_none()); + assert!(by_name_in_cwd_with_home("a/b", &project, None, None).is_none()); + assert!(by_name_with_home("../../outside", None, None).is_none()); + assert_eq!( + by_name_in_cwd_with_home("inside", &project, None, None).map(|d| d.name), + Some("inside".to_string()) + ); + } + #[test] fn test_discover_finds_md_files() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/codegen/kigi-shell/src/agent/subagent/handle_request.rs b/crates/codegen/kigi-shell/src/agent/subagent/handle_request.rs index 73c6d5d..43cbb2d 100644 --- a/crates/codegen/kigi-shell/src/agent/subagent/handle_request.rs +++ b/crates/codegen/kigi-shell/src/agent/subagent/handle_request.rs @@ -988,13 +988,22 @@ pub(crate) async fn handle_subagent_request( } } } - let agent_mcp_servers: Vec<_> = if is_plugin_agent { - if !definition.mcp_servers.is_empty() { - tracing::warn!( - agent = % definition.name, plugin = ? definition.plugin_name, - "ignoring mcpServers on plugin agent (not supported for security)" - ); - } + let agent_mcp_servers: Vec<_> = if definition.mcp_servers.is_empty() { + vec![] + } else if is_plugin_agent { + tracing::warn!( + agent = % definition.name, plugin = ? definition.plugin_name, + "ignoring mcpServers on plugin agent (not supported for security)" + ); + vec![] + } else if !crate::agent::folder_trust::agent_inline_hooks_allowed(definition.scope, || { + crate::agent::folder_trust::project_scope_allowed(&ctx.parent_cwd) + }) { + // An untrusted repo's inline stdio MCP command would run like an inline hook. + tracing::warn!( + agent = % definition.name, + "ignoring mcpServers on untrusted project agent (folder not trusted; re-run with --trust)" + ); vec![] } else { definition