From 0189009822a66058fdbd58d055f7187ba76c3ae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Sat, 19 Sep 2026 03:00:18 -0400 Subject: [PATCH] fix(config): a symlinked ~/.kigi/config.toml survives a save Every config.toml writer wrote a tmp beside the path and renamed it over the path. With config.toml symlinked into a dotfiles repo, the rename replaced the link with a regular file: the repo copy went stale and the link was gone. `config_write_dest` resolves the destination before a writer names its tmp. For the user's own config path a symlink resolves to its referent, so the tmp sits on the referent's filesystem and the rename stays atomic. A dangling link is an error, not a clobber. All eight writers call it: `save_config`, the four MCP writers, and the three import writers. Only the user's config path is followed, and only while `user_kigi_home()` resolves. `kigi mcp add --scope project` and the Claude project import write `.kigi/config.toml` inside the repo; a hostile repo could ship that file as a symlink to any file the user owns. With no home, `kigi_home()` falls back to `./.kigi`, which is that same project tree, so nothing is followed then. Untrusted paths come back unchanged and the link itself is replaced, as before. AGENTS.md records the rule. Tests (unix; run on Linux under WSL): helper cases including the no-home case, a project-scope save that spares the link's target, and an end-to-end save under KIGI_SHARE_DIR. The end-to-end test fails with following disabled. Windows symlinks were not exercised: the host lacks the privilege. Ported from upstream (grok-build config/fs_atomic.rs LeafSymlink::Follow, 1.0.35): the rule, not the 1375-line module. --- AGENTS.md | 9 ++ .../codegen/kigi-shell/src/claude_import.rs | 2 + crates/codegen/kigi-shell/src/kimi_import.rs | 1 + .../codegen/kigi-shell/src/util/config/mcp.rs | 4 + .../codegen/kigi-shell/src/util/config/mod.rs | 2 + .../kigi-shell/src/util/config/persist.rs | 1 + .../kigi-shell/src/util/config/write_dest.rs | 110 ++++++++++++++++++ .../test_config_symlink_survives_save.rs | 35 ++++++ 8 files changed, 164 insertions(+) create mode 100644 crates/codegen/kigi-shell/src/util/config/write_dest.rs create mode 100644 crates/codegen/kigi-shell/tests/test_config_symlink_survives_save.rs diff --git a/AGENTS.md b/AGENTS.md index 52b5406..6e645d0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,6 +64,15 @@ The cure is rmcp 3.x (upstream runs 3.2), a major bump not yet taken. switch that never stuck). Plain rename stays correct only for true moves whose destination doesn't pre-exist (worktree-pool markers, corrupt-file backups). Write failures must at least `warn!` — never `let _ =`. +- **config.toml writers resolve their destination through + `util::config::config_write_dest` before they name the tmp file.** A + symlinked `~/.kigi/config.toml` (dotfiles) keeps its link: the tmp sits + beside the referent and replaces it. Only that one path is followed, and + only while `user_kigi_home()` resolves (with no home, `kigi_home()` is the + project's own `./.kigi`). A project `.kigi/config.toml` is + repo-controlled, and following its symlink would overwrite whatever file + a hostile repo points it at; there the link itself is replaced. + Credential and managed stores never follow symlinks. - The root `Cargo.toml` is hand-maintained (upstream's generator is not in this repo). Members sorted; versions inherited from `workspace.package.version` — the single source of truth for the release diff --git a/crates/codegen/kigi-shell/src/claude_import.rs b/crates/codegen/kigi-shell/src/claude_import.rs index 5aa65f8..a3f2649 100644 --- a/crates/codegen/kigi-shell/src/claude_import.rs +++ b/crates/codegen/kigi-shell/src/claude_import.rs @@ -638,6 +638,7 @@ fn write_import_marker(config_path: &Path) -> anyhow::Result<()> { if let Some(parent) = config_path.parent() { std::fs::create_dir_all(parent)?; } + let config_path = &crate::util::config::config_write_dest(config_path)?; let tmp = config_path.with_extension("toml.tmp"); // Best-effort cleanup of the .tmp file if either write or rename fails so // a failed marker write doesn't leave a stale artefact next to the real @@ -820,6 +821,7 @@ fn apply_items_to_config(config_path: &Path, items: &[ImportableItem]) -> anyhow if count > 0 { let toml_str = toml::to_string_pretty(&root)?; + let config_path = &crate::util::config::config_write_dest(config_path)?; let tmp = config_path.with_extension("toml.tmp"); if let Some(parent) = config_path.parent() { std::fs::create_dir_all(parent)?; diff --git a/crates/codegen/kigi-shell/src/kimi_import.rs b/crates/codegen/kigi-shell/src/kimi_import.rs index 09fe4b3..57b554f 100644 --- a/crates/codegen/kigi-shell/src/kimi_import.rs +++ b/crates/codegen/kigi-shell/src/kimi_import.rs @@ -513,6 +513,7 @@ pub fn apply_at(plan: &KimiImportPlan, kigi_home: &Path) -> anyhow::Result 0 { // Atomic write: tmp + rename (same pattern as save_mcp_server_config). let toml_str = toml::to_string_pretty(&root)?; + let config_path = crate::util::config::config_write_dest(&config_path)?; let tmp = config_path.with_extension("toml.tmp"); if let Err(e) = std::fs::write(&tmp, &toml_str) { let _ = std::fs::remove_file(&tmp); diff --git a/crates/codegen/kigi-shell/src/util/config/mcp.rs b/crates/codegen/kigi-shell/src/util/config/mcp.rs index e9067fe..1d4b9c5 100644 --- a/crates/codegen/kigi-shell/src/util/config/mcp.rs +++ b/crates/codegen/kigi-shell/src/util/config/mcp.rs @@ -359,6 +359,7 @@ pub async fn save_mcp_disabled_tools(server_name: &str, disabled_tools: &[String } let toml_str = toml::to_string_pretty(&root)?; + let path = super::config_write_dest(&path)?; let tmp = path.with_extension("toml.tmp"); if let Some(parent) = path.parent() { let _ = tokio::fs::create_dir_all(parent).await; @@ -413,6 +414,7 @@ pub async fn save_mcp_server_enabled(server_name: &str, enabled: bool) -> Result } let toml_str = toml::to_string_pretty(&root)?; + let path = super::config_write_dest(&path)?; let tmp = path.with_extension("toml.tmp"); if let Some(parent) = path.parent() { let _ = tokio::fs::create_dir_all(parent).await; @@ -472,6 +474,7 @@ pub async fn save_mcp_server_config_at( } let toml_str = toml::to_string_pretty(&root)?; + let path = super::config_write_dest(path)?; let tmp = path.with_extension("toml.tmp"); if let Some(parent) = path.parent() { let _ = tokio::fs::create_dir_all(parent).await; @@ -552,6 +555,7 @@ pub async fn delete_mcp_server_config_at( } let toml_str = toml::to_string_pretty(&root)?; + let path = super::config_write_dest(path)?; let tmp = path.with_extension("toml.tmp"); if let Some(parent) = path.parent() { let _ = tokio::fs::create_dir_all(parent).await; diff --git a/crates/codegen/kigi-shell/src/util/config/mod.rs b/crates/codegen/kigi-shell/src/util/config/mod.rs index 73e90b8..6d18ac8 100644 --- a/crates/codegen/kigi-shell/src/util/config/mod.rs +++ b/crates/codegen/kigi-shell/src/util/config/mod.rs @@ -10,6 +10,7 @@ mod resolve; mod settings_writes; mod tips; mod worktree; +mod write_dest; pub use campaigns::{ load_effective_config, load_effective_config_disk_only, persist_models_default, @@ -30,3 +31,4 @@ pub use resolve::*; pub use settings_writes::*; pub use tips::*; pub use worktree::*; +pub(crate) use write_dest::config_write_dest; diff --git a/crates/codegen/kigi-shell/src/util/config/persist.rs b/crates/codegen/kigi-shell/src/util/config/persist.rs index 27851d0..9e8c347 100644 --- a/crates/codegen/kigi-shell/src/util/config/persist.rs +++ b/crates/codegen/kigi-shell/src/util/config/persist.rs @@ -52,6 +52,7 @@ pub async fn save_config(config: &Config) -> Result<()> { } let toml_str = toml::to_string_pretty(&root)?; + let path = super::config_write_dest(&path)?; if let Some(parent) = path.parent() { let _ = tokio::fs::create_dir_all(parent).await; } diff --git a/crates/codegen/kigi-shell/src/util/config/write_dest.rs b/crates/codegen/kigi-shell/src/util/config/write_dest.rs new file mode 100644 index 0000000..4ab41cc --- /dev/null +++ b/crates/codegen/kigi-shell/src/util/config/write_dest.rs @@ -0,0 +1,110 @@ +//! Where a config write lands when the path is a symlink. + +use std::io; +use std::path::{Path, PathBuf}; + +/// The user's own config.toml keeps its symlink; every other path comes back unchanged. +pub(crate) fn config_write_dest(path: &Path) -> io::Result { + // With no home, `kigi_home()` is `./.kigi`: the project's own tree, never trusted. + let user_config = kigi_config::user_kigi_home().map(|home| home.join("config.toml")); + write_dest(path, user_config.as_deref()) +} + +/// A project config is repo-controlled: following its symlink would overwrite any file it names. +fn write_dest(path: &Path, user_config: Option<&Path>) -> io::Result { + if user_config != Some(path) { + return Ok(path.to_path_buf()); + } + match std::fs::symlink_metadata(path) { + Ok(meta) if meta.file_type().is_symlink() => dunce::canonicalize(path).map_err(|e| { + let context = format!("config symlink {} does not resolve: {e}", path.display()); + io::Error::new(e.kind(), context) + }), + Ok(_) => Ok(path.to_path_buf()), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(path.to_path_buf()), + Err(e) => Err(e), + } +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use crate::util::config::{McpServerConfig, save_mcp_server_config_at}; + + fn symlinked_config(dir: &Path, referent_content: &str) -> (PathBuf, PathBuf) { + let referent = dir.join("dotfiles.toml"); + std::fs::write(&referent, referent_content).unwrap(); + let link = dir.join("config.toml"); + std::os::unix::fs::symlink(&referent, &link).unwrap(); + (link, referent) + } + + #[test] + fn user_config_symlink_resolves_to_its_referent() { + let dir = tempfile::tempdir().unwrap(); + let (link, referent) = symlinked_config(dir.path(), ""); + + let dest = write_dest(&link, Some(&link)).unwrap(); + assert_eq!(dest, dunce::canonicalize(&referent).unwrap()); + } + + #[test] + fn regular_and_missing_user_config_keep_their_path() { + let dir = tempfile::tempdir().unwrap(); + let regular = dir.path().join("config.toml"); + std::fs::write(®ular, "").unwrap(); + let missing = dir.path().join("absent.toml"); + + assert_eq!(write_dest(®ular, Some(®ular)).unwrap(), regular); + assert_eq!(write_dest(&missing, Some(&missing)).unwrap(), missing); + } + + #[test] + fn dangling_user_config_symlink_is_an_error_not_a_clobber() { + let dir = tempfile::tempdir().unwrap(); + let link = dir.path().join("config.toml"); + std::os::unix::fs::symlink(dir.path().join("gone.toml"), &link).unwrap(); + + let err = write_dest(&link, Some(&link)).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::NotFound); + assert!(err.to_string().contains("does not resolve"), "{err}"); + } + + #[test] + fn project_config_symlink_is_never_followed() { + let dir = tempfile::tempdir().unwrap(); + let (link, _referent) = symlinked_config(dir.path(), ""); + let user_config = dir.path().join("home").join("config.toml"); + + assert_eq!(write_dest(&link, Some(&user_config)).unwrap(), link); + } + + #[test] + fn nothing_is_followed_when_no_user_home_resolves() { + let dir = tempfile::tempdir().unwrap(); + let (link, _referent) = symlinked_config(dir.path(), ""); + + assert_eq!(write_dest(&link, None).unwrap(), link); + } + + #[tokio::test] + async fn project_scope_save_replaces_the_link_and_spares_its_target() { + let dir = tempfile::tempdir().unwrap(); + let (link, target) = symlinked_config(dir.path(), "victim = true\n"); + + let server: McpServerConfig = + toml::from_str("command = \"echo\"\nargs = [\"hi\"]\n").unwrap(); + save_mcp_server_config_at(&link, "demo", &server) + .await + .unwrap(); + + let meta = std::fs::symlink_metadata(&link).unwrap(); + assert!( + meta.file_type().is_file(), + "the link is replaced, not followed" + ); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "victim = true\n"); + let saved = std::fs::read_to_string(&link).unwrap(); + assert!(saved.contains("[mcp_servers.demo]"), "{saved}"); + } +} diff --git a/crates/codegen/kigi-shell/tests/test_config_symlink_survives_save.rs b/crates/codegen/kigi-shell/tests/test_config_symlink_survives_save.rs new file mode 100644 index 0000000..4fef2ef --- /dev/null +++ b/crates/codegen/kigi-shell/tests/test_config_symlink_survives_save.rs @@ -0,0 +1,35 @@ +//! A symlinked `~/.kigi/config.toml` survives a save: the write lands on the referent. +#![cfg(unix)] + +use kigi_shell::util::config::{McpServerConfig, save_mcp_server_config}; + +#[tokio::test] +async fn user_config_symlink_survives_a_save() { + let home = tempfile::tempdir().unwrap(); + let dotfiles = tempfile::tempdir().unwrap(); + // SAFETY: the only test in this binary; set before `kigi_home()` first runs. + unsafe { std::env::set_var("KIGI_SHARE_DIR", home.path()) }; + + let referent = dotfiles.path().join("kigi.toml"); + std::fs::write(&referent, "[ui]\ntheme = \"dark\"\n").unwrap(); + let link = home.path().join("config.toml"); + std::os::unix::fs::symlink(&referent, &link).unwrap(); + + let server: McpServerConfig = toml::from_str("command = \"echo\"\nargs = [\"hi\"]\n").unwrap(); + save_mcp_server_config("demo", &server).await.unwrap(); + + let meta = std::fs::symlink_metadata(&link).unwrap(); + assert!(meta.file_type().is_symlink(), "the link must survive"); + let saved = std::fs::read_to_string(&referent).unwrap(); + assert!(saved.contains("[mcp_servers.demo]"), "{saved}"); + assert!(saved.contains("theme = \"dark\""), "{saved}"); + let leftovers: Vec<_> = std::fs::read_dir(home.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .filter(|name| name != "config.toml") + .collect(); + assert!( + leftovers.is_empty(), + "no tmp beside the link: {leftovers:?}" + ); +}