From 7b4083cc1612f1ec99e21f8be169d7de52abe982 Mon Sep 17 00:00:00 2001 From: Jianjun Chen Date: Sun, 20 Sep 2026 12:47:26 +0800 Subject: [PATCH] fix(mcp): close the symlink escape in the write guard + more validation (#171) confine_to_root only normalized paths lexically, so a `.codeweb` symlink inside the project pointing elsewhere passed the check and the store write landed outside the permitted directory. Verified as Red first: with a symlinked .codeweb the guard accepted the path. The guard now also canonicalizes the deepest existing ancestor of the target and compares it against the canonicalized root. Both sides are canonicalized, so legitimate paths that merely traverse a symlink (e.g. /tmp on macOS) are still accepted. Additional validation added while checking the finished work: - corrupt store: queries report it explicitly and codeweb_analyze self-heals - pipelined analyze+analyze+stats requests all answer (no lock deadlock) - --project pointing at a not-yet-existing directory works with codeweb_init - init_at path branches: empty dir list, absolute source dirs preserved --- docs/DeveloperGuide.md | 2 +- ...026-09-20-issue-171-mcp-lifecycle-tools.md | 14 +- src/mcp/tools.rs | 86 +++++++- src/project/mod.rs | 32 +++ tests/mcp_test.rs | 190 ++++++++++++++++++ 5 files changed, 313 insertions(+), 11 deletions(-) diff --git a/docs/DeveloperGuide.md b/docs/DeveloperGuide.md index 5eed75f..2a9ead9 100644 --- a/docs/DeveloperGuide.md +++ b/docs/DeveloperGuide.md @@ -391,7 +391,7 @@ codeweb 提供四种 MCP/外部集成方式： - 查询工具读取 `Arc` 快照后立即释放锁，长查询不会阻塞 `codeweb_analyze`。 - `codeweb_analyze` 在 `tokio::task::spawn_blocking` 中运行 `Project::analyze`（CPU 密集、同步），完成后把新 store 换入快照。 - 未初始化目录不再导致进程退出：查询返回 `status: "uninitialized"`，引导调用 `codeweb_init`。 -- 写守卫 `confine_to_root` 做词法归一化后校验路径在 `permitted_root` 内；`store.path` 逃逸时 analyze 直接返回错误。 +- 写守卫 `confine_to_root` 双层校验：词法归一化拦住 `..` 逃逸；再对最深已存在祖先做 `canonicalize` 比较，拦住 `.codeweb` 指向目录外的符号链接。`store.path` 逃逸时 analyze 直接返回错误，且不产生任何写入。 - stdout 只用于 JSON-RPC：进度条与报告一律走 stderr，且 MCP 不调用 CLI 的 `print_analyze_report`。 ### 程序化 API 示例 diff --git a/docs/plans/2026-09-20-issue-171-mcp-lifecycle-tools.md b/docs/plans/2026-09-20-issue-171-mcp-lifecycle-tools.md index d27a504..7fe929c 100644 --- a/docs/plans/2026-09-20-issue-171-mcp-lifecycle-tools.md +++ b/docs/plans/2026-09-20-issue-171-mcp-lifecycle-tools.md @@ -35,8 +35,15 @@ - 未初始化：`--project` 参数指向的目录（canonicalize 后）。 所有写操作（`codeweb.toml`、`.codeweb/`、store、manifest、`parse.log`）都必须落在该目录树内。 -`confine_to_root(root, candidate)` 做词法归一化（处理 `.` / `..`）后校验 `starts_with(root)`，防止 -`store.path = "../../escape.bincode"` 这类配置把写操作带出许可目录。读路径不校验。 +`confine_to_root(root, candidate)` 做两层校验： + +1. 词法归一化（处理 `.` / `..`）后校验 `starts_with(root)`，拦住 `store.path = "../../escape.bincode"` + 以及尚未存在的路径； +2. 对**最深已存在祖先**做 `canonicalize` 后再比较，拦住 `.codeweb` 是指向目录外符号链接的情况 + （纯词法检查看不见符号链接）。比较双方都取 canonical 形式，因此经过符号链接到达同一目录的 + 合法路径（如 macOS 上经 `/tmp`）不会被误拒。 + +读路径不校验。 ### 2. 状态改为可变，查询走快照 @@ -87,9 +94,11 @@ MCP 的 stdout 是 JSON-RPC 通道。analyze 进度条（indicatif）与报告 | 6 | `codeweb_diff` 返回变更文件分类 | 集成 | | 7 | `store.path` 逃逸许可目录时 analyze 返回错误且不写盘 | 集成 | | 8 | 已分析且无变更的项目调用 analyze 报 `is_up_to_date:true` 且给出真实 nodes/edges（而非 up-to-date 短路返回的 0） | 集成 | +| 9 | `.codeweb` 为指向目录外的符号链接时 analyze 拒绝且目标目录无写入 | 单元 + 集成 | 循环 7 的 Red 通过临时禁用 `confine_to_root` 验证：无守卫时 analyze 返回 `ready` 并在服务目录外写出文件。 循环 8 的 Red 通过临时改用 `report.nodes/edges` 验证：此时报告为 `0 nodes`，测试失败。 +循环 9 的 Red 是真实缺口：加固前 `confine_rejects_symlinked_subdir_escaping_root` 直接失败（词法检查通过）。 测试权限：`test_mcp_tools_list` 的期望工具数由 8 变 11 是本次 feature 的必然结果， 更新时保持「精确集合」断言而非放宽为子集断言，并在提交信息中说明。 @@ -123,6 +132,7 @@ cargo fmt --all -- --check - 分支：`feat/issue-171-mcp-lifecycle-tools` - PR：https://github.com/c2j/codeweb/pull/172 - 门禁结果：`cargo build --features full` 通过；`cargo test --features full -- --skip test_path_mapping_applied --skip test_serve_` 全绿（`mcp_test` 13 passed）；`cargo clippy --features full -- -D warnings` 干净；`cargo fmt --all -- --check` 干净；GitHub CI（Lint / Test ubuntu full）通过。 +- 后续加固（PR #174）：`confine_to_root` 增加 canonicalize 祖先校验，堵住符号链接逃逸；新增 corrupt store 自愈、请求流水线、`--project` 指向不存在目录、`init_at` 路径分支等验证用例。 - 已知遗留：默认（非 mcp）构建下 `node_sub_type_tag`、`TreeNode::has_more/more_count` 报 dead_code，为既有 mcp-gated 代码，与本次改动无关。 - `tests/mcp_test.rs::test_mcp_tools_list` 期望工具集 8 → 11 为 feature 必然结果，保持精确集合断言。 diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 8064be2..3b2b13c 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -1068,11 +1068,24 @@ pub(super) fn normalize_lexically(path: &Path) -> std::path::PathBuf { out } +/// Canonicalize `path`, or if it does not exist yet, its deepest existing ancestor. +fn canonicalize_existing_ancestor(path: &Path) -> Option { + let mut current = path; + loop { + if let Ok(canonical) = std::fs::canonicalize(current) { + return Some(canonical); + } + current = current.parent()?; + } +} + /// Resolve `candidate` against `root` and accept it only if it stays inside. /// -/// `root` is expected to be absolute (the MCP server canonicalizes it at -/// startup). Resolution is lexical, so a symlink *inside* the root pointing -/// outside is not caught here. +/// `root` is expected to be absolute (the MCP server absolutizes and normalizes +/// it at startup). Two checks run: a lexical one that catches `..` escapes even +/// for paths that do not exist yet, and a symlink-aware one that canonicalizes +/// the deepest existing ancestor of the target (so `.codeweb` being a symlink to +/// a directory outside the root is rejected). fn confine_to_root(root: &Path, candidate: &Path) -> Result { let root_norm = normalize_lexically(root); let joined = if candidate.is_absolute() { @@ -1082,15 +1095,31 @@ fn confine_to_root(root: &Path, candidate: &Path) -> Result 0, + "analyze must rebuild a corrupted store, got: {analyzed}" + ); + + mcp.send( + r#"{"jsonrpc":"2.0","id":4,"method":"tools/call","params":{"name":"codeweb_stats","arguments":{}}}"#, + ); + let resp = mcp.recv_response(4); + let text = resp["result"]["content"][0]["text"] + .as_str() + .expect("stats text"); + let healed: serde_json::Value = serde_json::from_str(text).expect("stats JSON"); + assert_eq!(healed["status"], "ready", "got: {healed}"); + } + + #[test] + fn test_mcp_handles_pipelined_requests() { + // Requests are written back-to-back without waiting. Two analyzes in a + // row plus a read must all answer (no deadlock between the project mutex + // and the graph lock) and agree on the final state. + let (_tmpdir, project) = create_analyzed_project(); + let mut mcp = McpChild::start(&project); + handshake(&mut mcp); + + mcp.send( + r#"{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"codeweb_analyze","arguments":{}}}"#, + ); + mcp.send( + r#"{"jsonrpc":"2.0","id":3,"method":"tools/call","params":{"name":"codeweb_analyze","arguments":{}}}"#, + ); + mcp.send( + r#"{"jsonrpc":"2.0","id":4,"method":"tools/call","params":{"name":"codeweb_stats","arguments":{}}}"#, + ); + + let mut seen = std::collections::BTreeMap::new(); + // Notifications may be interleaved; keep reading until all three ids land. + for _ in 0..12 { + if seen.len() == 3 { + break; + } + let line = mcp + .read_line() + .expect("stdout closed while handling pipelined requests"); + let json: serde_json::Value = serde_json::from_str(&line).expect("valid JSON"); + if let Some(id) = json["id"].as_i64() { + seen.insert(id, json); + } + } + + for id in [2, 3, 4] { + let resp = seen + .get(&id) + .unwrap_or_else(|| panic!("no response for id {id}: {seen:?}")); + let text = resp["result"]["content"][0]["text"] + .as_str() + .expect("tool response text"); + let body: serde_json::Value = serde_json::from_str(text).expect("tool response JSON"); + assert_eq!(body["status"], "ready", "id {id} returned: {body}"); + assert!( + body["edges"].as_u64().unwrap_or(0) > 0, + "id {id} returned an empty graph: {body}" + ); + } + } + + #[test] + fn test_mcp_init_creates_missing_project_directory() { + // `--project` may point at a directory that does not exist yet. + let tmpdir = TempDir::new().expect("failed to create temp dir"); + let project = tmpdir.path().join("new").join("nested"); + assert!(!project.exists()); + + let mut mcp = McpChild::start(&project); + handshake(&mut mcp); + + mcp.send( + r#"{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"codeweb_init","arguments":{"name":"fresh","paths":["."]}}}"#, + ); + let resp = mcp.recv_response(2); + let text = resp["result"]["content"][0]["text"] + .as_str() + .expect("init text"); + let init: serde_json::Value = serde_json::from_str(text).expect("init JSON"); + + assert_eq!(init["status"], "initialized", "got: {init}"); + assert!( + project.join("codeweb.toml").exists(), + "init must create the served directory and its config" + ); + } + + #[test] + fn test_mcp_analyze_rejects_symlinked_store_dir() { + // Lexical path checks cannot see symlinks: `.codeweb` looks like it is + // inside the project, but it points outside. The write guard must still + // refuse, and nothing may be written into the symlink target. + let tmpdir = TempDir::new().expect("failed to create temp dir"); + let project = tmpdir.path().join("proj"); + let outside = tmpdir.path().join("outside"); + std::fs::create_dir_all(&project).expect("create project dir"); + std::fs::create_dir_all(&outside).expect("create outside dir"); + copy_serve_demo_fixture(&project); + + let toml = "[project]\n\ + name = \"symlink\"\n\ + \n\ + [analysis]\n\ + paths = [\"sql\"]\n\ + \n\ + [store]\n\ + path = \".codeweb/store.bincode\"\n\ + format = \"bincode\"\n"; + std::fs::write(project.join("codeweb.toml"), toml).expect("write codeweb.toml"); + std::os::unix::fs::symlink(&outside, project.join(".codeweb")).expect("symlink .codeweb"); + + let mut mcp = McpChild::start(&project); + handshake(&mut mcp); + + mcp.send( + r#"{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"codeweb_analyze","arguments":{}}}"#, + ); + let resp = mcp.recv_response(2); + let text = resp["result"]["content"][0]["text"] + .as_str() + .expect("analyze text"); + let result: serde_json::Value = serde_json::from_str(text).expect("analyze JSON"); + + assert_eq!( + result["status"], "error", + "a symlinked store directory must be refused, got: {result}" + ); + assert!( + result["error"] + .as_str() + .is_some_and(|e| e.contains("outside")), + "the error should say the path resolves outside the root, got: {result}" + ); + let leaked: Vec = std::fs::read_dir(&outside) + .expect("read outside dir") + .filter_map(|e| e.ok().map(|e| e.file_name().to_string_lossy().to_string())) + .collect(); + assert!( + leaked.is_empty(), + "nothing may be written through the symlink, found: {leaked:?}" + ); + } + #[test] fn test_mcp_call_stats() { let (_tmpdir, project) = create_test_project();