From c572ddc4f3b619ce8725f5336feb49929a88a029 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 8 Sep 2026 01:37:18 +0200 Subject: [PATCH 1/5] fix(cli): uninstall no longer gated by agent-config cleanup failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single agent configuration the editors refuse to touch (the reporter's ~/.cursor/mcp.json was a symlink into a dotfiles checkout) aborted the uninstall activation BEFORE index and binary removal. `uninstall -y` exited 1 and left a 300 MB executable plus the whole cache behind — the data-loss half of #1954 — while the transcript still read "removed". Agent-config failures are now collected instead of gating: every cleanup runs, the indexes and the executable go, and the command ends with one list of the files that could not be cleaned (agent, operation, path, observed target) and a non-zero exit. The existing per-error stderr line is unchanged. Two tests encoded the old hostage contract ("binary must survive failed cleanup") and move to the new one. The new test drives `uninstall -y` against a sandbox HOME with a dangling ~/.cursor/mcp.json link and asserts binary + index gone, exit non-zero, and the file named on stderr; on main it fails at ASSERT(binary_gone). (#1954) Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- src/cli/cli.c | 101 ++++++++++++++++++++++++++++--- tests/test_cli.c | 154 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 241 insertions(+), 14 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index 249b43e6f..29ddd7f4f 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -7729,6 +7729,75 @@ static cbm_install_plan_t *g_install_plan = NULL; static int g_agent_install_errors = 0; static int g_agent_uninstall_errors = 0; +/* Every agent configuration uninstall could not clean, kept for the closing + * summary. A cleanup failure no longer stops executable and index removal + * (#1954: one symlinked ~/.cursor/mcp.json left a 300 MB binary plus the whole + * cache behind), so the user needs ONE list of what is still theirs to fix, + * with the observed reason next to each file. */ +typedef struct { + char agent[64]; + char operation[48]; + char path[CLI_BUF_1K]; + char reason[160]; + char detail[160]; +} cbm_agent_config_failure_t; + +static cbm_agent_config_failure_t *g_agent_uninstall_failures = NULL; +static int g_agent_uninstall_failure_count = 0; +static int g_agent_uninstall_failure_cap = 0; + +static void agent_uninstall_failures_reset(void) { + free(g_agent_uninstall_failures); + g_agent_uninstall_failures = NULL; + g_agent_uninstall_failure_count = 0; + g_agent_uninstall_failure_cap = 0; +} + +static void agent_uninstall_failure_record(const char *agent, const char *operation, + const char *path, const char *reason, + const char *detail) { + if (g_agent_uninstall_failure_count >= g_agent_uninstall_failure_cap) { + int ncap = g_agent_uninstall_failure_cap ? g_agent_uninstall_failure_cap * 2 : CLI_BUF_16; + cbm_agent_config_failure_t *grown = + realloc(g_agent_uninstall_failures, (size_t)ncap * sizeof(*grown)); + if (!grown) { + return; + } + g_agent_uninstall_failures = grown; + g_agent_uninstall_failure_cap = ncap; + } + cbm_agent_config_failure_t *entry = + &g_agent_uninstall_failures[g_agent_uninstall_failure_count++]; + (void)snprintf(entry->agent, sizeof(entry->agent), "%s", agent ? agent : "unknown"); + (void)snprintf(entry->operation, sizeof(entry->operation), "%s", + operation ? operation : "unknown"); + (void)snprintf(entry->path, sizeof(entry->path), "%s", path ? path : "unknown"); + (void)snprintf(entry->reason, sizeof(entry->reason), "%s", reason ? reason : ""); + (void)snprintf(entry->detail, sizeof(entry->detail), "%s", detail ? detail : ""); +} + +/* The closing list of what uninstall could not clean. Printed AFTER the + * executable and the indexes are gone, so nothing in it is a reason to keep + * the installation around — each line is one file the user removes an entry + * from by hand. */ +static void agent_uninstall_failures_report(bool dry_run) { + if (g_agent_uninstall_failure_count == 0) { + return; + } + (void)fprintf(stderr, "\nerror: uninstall %s with %d agent configuration(s) left uncleaned:\n", + dry_run ? "dry-run finished" : "finished", g_agent_uninstall_failure_count); + for (int i = 0; i < g_agent_uninstall_failure_count; i++) { + const cbm_agent_config_failure_t *entry = &g_agent_uninstall_failures[i]; + (void)fprintf(stderr, " %s (%s): %s", entry->agent, entry->operation, entry->path); + if (entry->reason[0]) { + (void)fprintf(stderr, " reason=%s", entry->reason); + } + (void)fputs(entry->detail, stderr); + (void)fputc('\n', stderr); + } + (void)fputs("Remove the codebase-memory-mcp entries from these files by hand.\n", stderr); +} + static void plan_record(const char *agent, const char *kind, const char *path) { if (!g_install_plan || !path || !path[0]) { return; @@ -7801,6 +7870,9 @@ static void record_agent_config_error_with_reason(bool uninstalling, const char } (void)fputs(detail, stderr); (void)fputc('\n', stderr); + if (uninstalling) { + agent_uninstall_failure_record(agent, operation, path, reason, detail); + } } static void record_agent_config_error(bool uninstalling, const char *agent, const char *operation, @@ -12044,13 +12116,12 @@ static int cli_uninstall_activate(void *opaque) { uninstall_additional_agents(&activation->agents, activation->home, activation->dry_run); uninstall_agent_client_registry(activation->home, activation->dry_run); - if (g_agent_uninstall_errors != 0) { - cli_activation_transaction_abort_or_fail_stop(&activation->binary_transaction, - "uninstall_transaction_config_cleanup_abort"); - (void)fprintf(stderr, "error: one or more agent cleanup operations failed; executable " - "and index removal were not started\n"); - return CLI_ACTIVATION_PARTIAL; - } + /* Agent-config failures are collected, never a gate: an entry the editors + * refuse to touch (a symlinked config, a foreign file, a malformed + * document) is the user's to fix by hand, and leaving a 300 MB executable + * plus every index behind because of it is the data-loss shape of #1954. + * The indexes and the executable go now; the failures are listed at the + * end and decide the exit code. */ if (activation->delete_indexes && !activation->dry_run) { int expected = count_db_indexes(activation->home); @@ -12151,6 +12222,7 @@ int cbm_cmd_uninstall(int argc, char **argv) { printf("codebase-memory-mcp uninstall\n\n"); g_agent_uninstall_errors = 0; + agent_uninstall_failures_reset(); cbm_detected_agents_t agents = cbm_detect_agents(home); /* Confirm index removal outside the startup lock, but defer the mutation @@ -12220,6 +12292,19 @@ int cbm_cmd_uninstall(int argc, char **argv) { (void)cli_activation_transaction_abort(&activation.binary_transaction); } if (activation_rc != CLI_OK) { + agent_uninstall_failures_reset(); + return CLI_TRUE; + } + + if (g_agent_uninstall_errors != 0) { + agent_uninstall_failures_report(dry_run); + agent_uninstall_failures_reset(); + printf("\nUninstall finished with errors; the files listed above still hold " + "codebase-memory-mcp entries. Please restart your coding-agent sessions " + "to properly take this into account.\n"); + if (dry_run) { + printf("(dry-run — no files were modified)\n"); + } return CLI_TRUE; } @@ -12228,7 +12313,7 @@ int cbm_cmd_uninstall(int argc, char **argv) { if (dry_run) { printf("(dry-run — no files were modified)\n"); } - return g_agent_uninstall_errors == 0 ? 0 : CLI_TRUE; + return 0; } /* ── Subcommand: update ───────────────────────────────────────── */ diff --git a/tests/test_cli.c b/tests/test_cli.c index 10ec1097a..aab397da4 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -714,6 +714,59 @@ static void test_rmdir_r(const char *path) { th_rmtree(path); } +/* Capture everything a command writes to one fd (stdout or stderr) so a test + * can assert on the transcript. Restores the fd on end and returns the text. */ +typedef struct { + FILE *file; + FILE *stream; + int target_fd; + int saved_fd; + bool redirected; +} cli_fd_capture_t; + +static void cli_fd_capture_begin(cli_fd_capture_t *capture, FILE *stream, int target_fd) { + memset(capture, 0, sizeof(*capture)); + capture->stream = stream; + capture->target_fd = target_fd; + capture->saved_fd = -1; + capture->file = tmpfile(); + if (!capture->file) { + return; + } + capture->saved_fd = dup(target_fd); + if (capture->saved_fd < 0) { + return; + } + fflush(stream); + capture->redirected = dup2(fileno(capture->file), target_fd) >= 0; +} + +/* Returns the captured text (heap, "" when nothing was written) or NULL when + * the capture never engaged. */ +static char *cli_fd_capture_end(cli_fd_capture_t *capture) { + fflush(capture->stream); + if (capture->saved_fd >= 0) { + (void)dup2(capture->saved_fd, capture->target_fd); + close(capture->saved_fd); + capture->saved_fd = -1; + } + char *text = NULL; + if (capture->file) { + if (capture->redirected) { + rewind(capture->file); + size_t capacity = 65536U; + text = calloc(1U, capacity); + if (text) { + size_t count = fread(text, 1U, capacity - 1U, capture->file); + text[count] = '\0'; + } + } + fclose(capture->file); + capture->file = NULL; + } + return text; +} + /* Mandatory-daemon activation guard fixture. The production path uses the * stable per-account endpoint directly; these callbacks make every race * ordering deterministic without exposing a CLI flag or environment bypass. */ @@ -2269,6 +2322,88 @@ TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain) { PASS(); } +#ifndef _WIN32 +/* #1954: an agent config that cannot be cleaned must not hold the executable + * and the indexes hostage. The fixture's ~/.cursor/mcp.json is a dangling + * symlink (a dotfiles checkout that moved) — a shape every config editor keeps + * refusing. Before the fix the cleanup failure aborted the activation BEFORE + * index and binary removal: `uninstall -y` exited 1 and left a 300 MB binary + * plus the whole cache behind, while the report still read "removed". */ +TEST(cli_uninstall_removes_binary_and_index_when_agent_config_cleanup_fails) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-uninstall-hostage-XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) { + FAIL("cbm_mkdtemp failed"); + } + char *old_home = NULL; + char *old_cache = NULL; + cli_activation_save_env(&old_home, &old_cache); + cbm_setenv("HOME", tmpdir, 1); + /* PATH moves with HOME so no real agent on the developer's PATH is detected. */ + char *old_path = save_test_env("PATH"); + cbm_setenv("PATH", tmpdir, 1); + + char cache_dir[512]; + char index_path[640]; + snprintf(cache_dir, sizeof(cache_dir), "%s/cache", tmpdir); + cbm_setenv("CBM_CACHE_DIR", cache_dir, 1); + test_mkdirp(cache_dir); + snprintf(index_path, sizeof(index_path), "%s/project.db", cache_dir); + write_test_file(index_path, "index must go even when a config cannot be cleaned"); + + char bin_dir[512]; + char bin_target[640]; + snprintf(bin_dir, sizeof(bin_dir), "%s/.local/bin", tmpdir); + test_mkdirp(bin_dir); + snprintf(bin_target, sizeof(bin_target), "%s/codebase-memory-mcp", bin_dir); + write_test_file(bin_target, "binary must go even when a config cannot be cleaned"); + + char cursor_dir[512]; + char cursor_config[640]; + char dangling_target[640]; + snprintf(cursor_dir, sizeof(cursor_dir), "%s/.cursor", tmpdir); + test_mkdirp(cursor_dir); + snprintf(cursor_config, sizeof(cursor_config), "%s/mcp.json", cursor_dir); + snprintf(dangling_target, sizeof(dangling_target), "%s/.dotfiles/cursor/mcp.json", tmpdir); + if (symlink(dangling_target, cursor_config) != 0) { + FAIL("symlink failed"); + } + + cli_activation_fake_t fake = {.mutation_reserve_result = 1}; + cbm_cli_activation_ops_t ops = cli_activation_fake_ops(&fake); + cbm_cli_set_activation_ops_for_test(&ops); + cli_fd_capture_t out_capture; + cli_fd_capture_t err_capture; + cli_fd_capture_begin(&out_capture, stdout, STDOUT_FILENO); + cli_fd_capture_begin(&err_capture, stderr, STDERR_FILENO); + char *argv[] = {"--yes"}; + int rc = cli_test_cmd_uninstall(1, argv); + char *err_text = cli_fd_capture_end(&err_capture); + char *out_text = cli_fd_capture_end(&out_capture); + cbm_cli_set_activation_ops_for_test(NULL); + cbm_set_auto_answer_for_test(0); + + struct stat state; + bool binary_gone = lstat(bin_target, &state) != 0 && errno == ENOENT; + bool index_gone = lstat(index_path, &state) != 0 && errno == ENOENT; + bool link_untouched = lstat(cursor_config, &state) == 0 && S_ISLNK(state.st_mode); + bool failure_named = err_text && strstr(err_text, cursor_config) != NULL; + + cli_activation_restore_env(old_home, old_cache); + restore_test_env("PATH", old_path); + test_rmdir_r(tmpdir); + free(err_text); + free(out_text); + + ASSERT(rc != 0); + ASSERT_TRUE(binary_gone); + ASSERT_TRUE(index_gone); + ASSERT_TRUE(link_untouched); + ASSERT_TRUE(failure_named); + PASS(); +} +#endif + TEST(cli_activation_guard_is_bypassed_for_dry_run_and_plan) { char tmpdir[256]; snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-daemon-stateless-XXXXXX"); @@ -4627,7 +4762,7 @@ TEST(cli_agent_uninstall_reports_safe_editor_refusal) { #else snprintf(bin_path, sizeof(bin_path), "%s/codebase-memory-mcp", bin_dir); #endif - write_test_file(bin_path, "installed binary must remain live\n"); + write_test_file(bin_path, "installed binary goes even when a config cannot be cleaned\n"); char *saved_home = save_test_env("HOME"); char *saved_path = save_test_env("PATH"); @@ -4651,9 +4786,12 @@ TEST(cli_agent_uninstall_reports_safe_editor_refusal) { restore_test_env("HOME", saved_home); restore_test_env("PATH", saved_path); test_rmdir_r(tmpdir); - if (rc == 0 || !preserved || !binary_preserved || fake.mutation_reserve_count != 1 || - fake.mutation_lease_release_count != 1 || !strstr(fake.diagnostic, "executable was kept")) - FAIL("agent uninstall refusal must fail before removing the live binary"); + /* #1954: a refused config is reported and fails the exit code, but it never + * keeps the executable installed — that left a 300 MB binary and every index + * behind for one unreadable mcp.json. */ + if (rc == 0 || !preserved || binary_preserved || fake.mutation_reserve_count != 1 || + fake.mutation_lease_release_count != 1 || fake.diagnostic[0] != '\0') + FAIL("agent uninstall refusal must fail the exit code without keeping the binary"); PASS(); } @@ -10463,7 +10601,7 @@ TEST(cli_codex_migrates_to_single_hook_representation) { #else snprintf(binary_path, sizeof(binary_path), "%s/codebase-memory-mcp", binary_dir); #endif - write_test_file(binary_path, "installed binary must survive failed cleanup\n"); + write_test_file(binary_path, "installed binary goes even when a config cannot be cleaned\n"); char *saved_home = save_test_env("HOME"); char *saved_path = save_test_env("PATH"); @@ -10515,7 +10653,8 @@ TEST(cli_codex_migrates_to_single_hook_representation) { snprintf(agent_path, sizeof(agent_path), "%s/agents/codebase-memory.toml", codex_dir); struct stat state; hooks = read_test_file_alloc(hooks_path); - bool independent_cleanup = uninstall_rc != 0 && stat(binary_path, &state) == 0 && + /* #1954: the ambiguous hook fails the exit code; the binary still goes. */ + bool independent_cleanup = uninstall_rc != 0 && stat(binary_path, &state) != 0 && stat(skill_path, &state) != 0 && stat(agent_path, &state) != 0 && hooks && !strstr(hooks, "hook-augment"); free(hooks); @@ -14539,6 +14678,9 @@ SUITE(cli) { RUN_TEST(cli_update_agent_configs_finish_before_guard_release); RUN_TEST(cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index); RUN_TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain); +#ifndef _WIN32 + RUN_TEST(cli_uninstall_removes_binary_and_index_when_agent_config_cleanup_fails); +#endif RUN_TEST(cli_activation_guard_is_bypassed_for_dry_run_and_plan); #ifdef _WIN32 RUN_TEST(cli_windows_update_hands_off_to_install_script); From e5aabdf86ea86ee27b92477b21aa8d6c754a674d Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 8 Sep 2026 02:04:51 +0200 Subject: [PATCH 2/5] fix(cli): follow user-owned symlinked config files in the editors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every config editor (JSON-like, TOML, YAML, text) opens with O_NOFOLLOW and refuses symlinks, so a link planted at ~/.cursor/mcp.json cannot point a privileged writer at another file. Users keep exactly those files as symlinks into a dotfiles checkout; the blanket refusal made install and uninstall fail on them and, before the previous commit, turned uninstall into data loss (#1954). Decision C: a symlink the INVOKING user owns, resolving to a regular file the same user owns, is followed — as an explicit opt-in of the agent-configuration writers only. cbm_install_agent_configs and the uninstall activation register the user's configuration roots (HOME, XDG_CONFIG_HOME) for the duration of their work; a link whose own directory is outside every registered root, or any link seen by another editor caller, stays refused, so repository paths never follow. One shared helper (src/cli/config_edit_path.c) runs the race-safe sequence: lstat(link) → owner == invoking uid (a root process follows root-owned links only) → realpath → the target's parent directory opened as a dirfd (O_DIRECTORY|O_NOFOLLOW; owned by the invoking user or root, not writable by others unless sticky) → openat(dirfd, base, O_NOFOLLOW) → fstat: regular file, same owner, link count 1, and the same (dev, ino) as stat(realpath). Each editor reads through that validated descriptor, stages its temp file with openat(dirfd, …, O_CREAT|O_EXCL|O_NOFOLLOW), runs its pre-publish snapshot comparison against the resolved path, and commits with renameat on the pinned dirfd, so the link itself is never replaced. Every other link is refused with an observable reason (surfaced in the uninstall diagnostics): another owner, root over a non-root link, dangling, a directory or special target, a hard-linked or swapped target, a foreign or world-writable parent. Whether the target must also stay under a root is a pending user decision behind CBM_CONFIG_EDIT_TARGET_UNDER_ROOT (default: anywhere the user owns). Windows never follows reparse points. Tests: each editor suite gains a same-owner link edited in place under the opt-in (link preserved and still pointing at the same target, temp file staged beside the target, bytes identical to the same edit on a plain file, no stray temp files) and keeps its refusal cases; the JSON-like suite adds no-opt-in, outside-root, dangling, directory, identity-skew and world-writable-parent refusals. The foreign-owner and root-run refusals move the observer through a test seam (a test cannot create a foreign-owned link without root; a real root run demotes the link first). test_cli.c drives `uninstall -y` end to end through a symlinked ~/.cursor/mcp.json. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- Makefile.cbm | 3 +- src/cli/cli.c | 38 +++ src/cli/config_edit_path.c | 434 ++++++++++++++++++++++++++++++++++ src/cli/config_edit_path.h | 109 +++++++++ src/cli/config_json_like.c | 77 +++++- src/cli/config_text_edit.c | 78 +++++- src/cli/config_toml_edit.c | 96 ++++++-- src/cli/config_yaml_edit.c | 75 +++++- tests/test_cli.c | 94 ++++++++ tests/test_config_json_like.c | 409 +++++++++++++++++++++++++++++++- tests/test_config_text_edit.c | 74 +++++- tests/test_config_toml_edit.c | 64 ++++- tests/test_config_yaml_edit.c | 81 ++++++- 13 files changed, 1560 insertions(+), 72 deletions(-) create mode 100644 src/cli/config_edit_path.c create mode 100644 src/cli/config_edit_path.h diff --git a/Makefile.cbm b/Makefile.cbm index 4f1ab1371..f92647685 100644 --- a/Makefile.cbm +++ b/Makefile.cbm @@ -84,6 +84,7 @@ SANITIZE = -fsanitize=address,undefined -fno-omit-frame-pointer EDITOR_TEST_DEFINES = -DCBM_JSON_LIKE_ENABLE_TEST_API=1 \ -DCBM_TOML_EDIT_ENABLE_TEST_API=1 -DCBM_YAML_ENABLE_TEST_API=1 \ -DCBM_TEXT_EDIT_ENABLE_TEST_API=1 -DCBM_CLI_ENABLE_TEST_API=1 \ + -DCBM_CONFIG_EDIT_PATH_ENABLE_TEST_API=1 \ -DCBM_DIAGNOSTICS_ENABLE_TEST_API=1 -DCBM_ENABLE_TEST_SEAMS=1 # The build system is the single source of truth for "is this binary # instrumented": compiler-specific probes (__SANITIZE_ADDRESS__) miss @@ -409,7 +410,7 @@ GIT_SRCS = src/git/git_context.c CLI_SRCS = src/cli/cli.c src/cli/progress_sink.c src/cli/hook_augment.c src/cli/client_adapter.c \ src/cli/agent_clients.c src/cli/agent_profiles.c \ src/cli/config_json_like.c src/cli/config_toml_edit.c src/cli/config_yaml_edit.c \ - src/cli/config_text_edit.c src/cli/activation_transaction.c + src/cli/config_text_edit.c src/cli/config_edit_path.c src/cli/activation_transaction.c # UI module (graph visualization) UI_SRCS = \ diff --git a/src/cli/cli.c b/src/cli/cli.c index 29ddd7f4f..fc1b68e44 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -8,6 +8,7 @@ #include "cli/agent_profiles.h" #include "cli/cli.h" #include "cli/activation_transaction.h" +#include "cli/config_edit_path.h" #include "cli/config_json_like.h" #include "cli/config_text_edit.h" #include "cli/config_toml_edit.h" @@ -7776,6 +7777,21 @@ static void agent_uninstall_failure_record(const char *agent, const char *operat (void)snprintf(entry->detail, sizeof(entry->detail), "%s", detail ? detail : ""); } +/* The agent-configuration writers opt in to following user-owned symlinked + * config files under the user's configuration roots (#1954, decision C); + * every other caller of the config editors keeps refusing links. Cleared by + * the same command when its configuration work is done. */ +static void cli_config_follow_begin(const char *home) { + cbm_config_edit_path_follow_clear(); + if (home && home[0]) { + (void)cbm_config_edit_path_follow_add_root(home); + } + const char *xdg_config = getenv("XDG_CONFIG_HOME"); + if (xdg_config && xdg_config[0]) { + (void)cbm_config_edit_path_follow_add_root(xdg_config); + } +} + /* The closing list of what uninstall could not clean. Printed AFTER the * executable and the indexes are gone, so nothing in it is a reason to keep * the installation around — each line is one file the user removes an entry @@ -7853,6 +7869,14 @@ static void describe_agent_config_target(const char *path, char *out, size_t out : info.is_directory ? "directory" : info.is_regular ? "regular file" : "special file"; + /* A refused symlink names the rule that refused it (#1954): the user + * then knows whether to fix ownership, the target, or the parent. */ + char refusal[160]; + if (info.is_symlink && cbm_config_edit_path_refusal(path, refusal, sizeof(refusal))) { + (void)snprintf(out, out_size, " (target: symlink, %lld bytes; not followed: %s)", + (long long)info.size, refusal); + return; + } (void)snprintf(out, out_size, " (target: %s, %lld bytes)", kind, (long long)info.size); } @@ -9817,12 +9841,24 @@ static void install_additional_agent_configs(const cbm_detected_agents_t *agents } } +static int cbm_install_agent_configs_in_scope(const char *home, const char *binary_path, bool force, + bool dry_run, cbm_detected_agents_t *agents_in); + int cbm_install_agent_configs(const char *home, const char *binary_path, bool force, bool dry_run) { g_agent_install_errors = 0; cbm_detected_agents_t agents = cbm_detect_agents(home); if (g_client_selection && !cli_clients_apply_selection(g_client_selection, &agents)) { return CLI_ERR; } + cli_config_follow_begin(home); + int result = cbm_install_agent_configs_in_scope(home, binary_path, force, dry_run, &agents); + cbm_config_edit_path_follow_clear(); + return result; +} + +static int cbm_install_agent_configs_in_scope(const char *home, const char *binary_path, bool force, + bool dry_run, cbm_detected_agents_t *agents_in) { + cbm_detected_agents_t agents = *agents_in; if (!g_install_plan) { print_detected_agents(&agents, home); } @@ -12108,6 +12144,7 @@ static int cli_uninstall_activate(void *opaque) { return CLI_TRUE; } + cli_config_follow_begin(activation->home); if (activation->agents.claude_code) { uninstall_claude_code(activation->home, activation->bin_path, activation->dry_run); } @@ -12115,6 +12152,7 @@ static int cli_uninstall_activate(void *opaque) { uninstall_editor_agents(&activation->agents, activation->home, activation->dry_run); uninstall_additional_agents(&activation->agents, activation->home, activation->dry_run); uninstall_agent_client_registry(activation->home, activation->dry_run); + cbm_config_edit_path_follow_clear(); /* Agent-config failures are collected, never a gate: an entry the editors * refuse to touch (a symlinked config, a foreign file, a malformed diff --git a/src/cli/config_edit_path.c b/src/cli/config_edit_path.c new file mode 100644 index 000000000..b51c5c2a2 --- /dev/null +++ b/src/cli/config_edit_path.c @@ -0,0 +1,434 @@ +/* + * config_edit_path.c — follow a config symlink only when the agent-config + * writer opted in, the invoking user owns both the link and its regular-file + * target, and the target's parent directory can be pinned. See the header + * for the decision, the sequence and its boundaries (#1954). + */ +#include "cli/config_edit_path.h" + +#include "foundation/compat.h" +#include "foundation/compat_fs.h" + +#include +#include +#include + +#ifndef _WIN32 +#include +#include +#include +#include +#endif + +enum { EDIT_PATH_MAX_ROOTS = 4 }; + +static int edit_path_copy(const char *source, char *out, size_t out_size) { + int written = snprintf(out, out_size, "%s", source); + return written >= 0 && (size_t)written < out_size ? 0 : -1; +} + +static void edit_path_target_reset(cbm_config_edit_target_t *target) { + target->status = CBM_CONFIG_EDIT_PATH_DIRECT; + target->dirfd = -1; + target->fd = -1; + target->base[0] = '\0'; + target->path[0] = '\0'; +} + +static void edit_path_note(char *reason, size_t reason_size, const char *text) { + if (reason && reason_size > 0U) { + (void)snprintf(reason, reason_size, "%s", text); + } +} + +void cbm_config_edit_target_close(cbm_config_edit_target_t *target) { + if (!target) { + return; + } + int saved_errno = errno; +#ifndef _WIN32 + if (target->fd >= 0) { + (void)close(target->fd); + } + if (target->dirfd >= 0) { + (void)close(target->dirfd); + } +#endif + target->fd = -1; + target->dirfd = -1; + errno = saved_errno; +} + +#ifndef _WIN32 + +static CBM_TLS char edit_path_roots[EDIT_PATH_MAX_ROOTS][CBM_CONFIG_EDIT_PATH_MAX]; +static CBM_TLS size_t edit_path_root_count = 0U; + +#ifdef CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API +static CBM_TLS int edit_path_uid_override_set = 0; +static CBM_TLS uid_t edit_path_uid_override = 0; +static CBM_TLS int edit_path_identity_skew = 0; + +void cbm_config_edit_path_set_invoking_uid_for_test(unsigned uid, int set) { + edit_path_uid_override_set = set != 0; + edit_path_uid_override = (uid_t)uid; +} + +void cbm_config_edit_path_set_identity_skew_for_test(int set) { + edit_path_identity_skew = set != 0; +} +#endif + +static uid_t edit_path_invoking_uid(void) { +#ifdef CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API + if (edit_path_uid_override_set) { + return edit_path_uid_override; + } +#endif + return geteuid(); +} + +int cbm_config_edit_path_follow_add_root(const char *root) { + if (!root || !root[0] || edit_path_root_count >= EDIT_PATH_MAX_ROOTS) { + return -1; + } + char canonical[CBM_CONFIG_EDIT_PATH_MAX]; + if (!cbm_canonical_path(root, canonical, sizeof(canonical))) { + return -1; + } + if (edit_path_copy(canonical, edit_path_roots[edit_path_root_count], + sizeof(edit_path_roots[0])) != 0) { + return -1; + } + edit_path_root_count++; + return 0; +} + +void cbm_config_edit_path_follow_clear(void) { + edit_path_root_count = 0U; +} + +static int edit_path_under_root(const char *canonical) { + for (size_t i = 0U; i < edit_path_root_count; i++) { + const char *root = edit_path_roots[i]; + size_t root_length = strlen(root); + if (strcmp(canonical, root) == 0) { + return 1; + } + if (strncmp(canonical, root, root_length) == 0 && + (canonical[root_length] == '/' || root[root_length - 1U] == '/')) { + return 1; + } + } + return 0; +} + +/* Splits a canonical absolute path into its parent and its final name. */ +static int edit_path_split(const char *canonical, char *parent, size_t parent_size, char *base, + size_t base_size) { + const char *slash = strrchr(canonical, '/'); + if (!slash || slash[1] == '\0') { + return -1; + } + size_t parent_length = slash == canonical ? 1U : (size_t)(slash - canonical); + if (parent_length >= parent_size || edit_path_copy(slash + 1, base, base_size) != 0) { + return -1; + } + memcpy(parent, canonical, parent_length); + parent[parent_length] = '\0'; + return 0; +} + +/* The link's OWN directory must sit under a registered root: that is what + * makes the follow an opt-in of the agent-configuration writers rather than + * a property of every editor caller. */ +static int edit_path_link_dir_under_root(const char *path) { + if (edit_path_root_count == 0U) { + return 0; + } + char parent[CBM_CONFIG_EDIT_PATH_MAX]; + if (edit_path_copy(path, parent, sizeof(parent)) != 0) { + return 0; + } + char *slash = strrchr(parent, '/'); + if (!slash) { + parent[0] = '.'; + parent[1] = '\0'; + } else if (slash == parent) { + parent[1] = '\0'; + } else { + *slash = '\0'; + } + char canonical[CBM_CONFIG_EDIT_PATH_MAX]; + if (!cbm_canonical_path(parent, canonical, sizeof(canonical))) { + return 0; + } + return edit_path_under_root(canonical); +} + +static void edit_path_note_owner(char *reason, size_t reason_size, const char *what, + unsigned long owner, unsigned long self) { + if (reason && reason_size > 0U) { + (void)snprintf(reason, reason_size, "%s owned by uid %lu, not the invoking user (uid %lu)", + what, owner, self); + } +} + +/* One classification serves both the resolver and the diagnostic, so the + * reason a user reads is exactly the rule that refused the link. On REFUSED + * every descriptor opened along the way is closed again. */ +static int edit_path_classify(const char *path, cbm_config_edit_target_t *target, char *reason, + size_t reason_size) { + edit_path_target_reset(target); + if (reason && reason_size > 0U) { + reason[0] = '\0'; + } + if (!path || !path[0]) { + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + struct stat link_state; + if (lstat(path, &link_state) != 0 || !S_ISLNK(link_state.st_mode)) { + return edit_path_copy(path, target->path, sizeof(target->path)) == 0 + ? CBM_CONFIG_EDIT_PATH_DIRECT + : CBM_CONFIG_EDIT_PATH_REFUSED; + } + if (!edit_path_link_dir_under_root(path)) { + edit_path_note(reason, reason_size, + edit_path_root_count == 0U + ? "symlinks are not followed by this command" + : "symlink outside the opted-in configuration roots"); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + uid_t self = edit_path_invoking_uid(); + if (self == 0 && link_state.st_uid != 0) { + if (reason && reason_size > 0U) { + (void)snprintf(reason, reason_size, + "symlink owned by uid %lu is not followed by a root process", + (unsigned long)link_state.st_uid); + } + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + if (link_state.st_uid != self) { + edit_path_note_owner(reason, reason_size, "symlink", (unsigned long)link_state.st_uid, + (unsigned long)self); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + char resolved[CBM_CONFIG_EDIT_PATH_MAX]; + struct stat target_state; + if (!cbm_canonical_path(path, resolved, sizeof(resolved)) || + stat(resolved, &target_state) != 0) { + edit_path_note(reason, reason_size, "dangling symlink"); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + if (!S_ISREG(target_state.st_mode)) { + edit_path_note(reason, reason_size, + S_ISDIR(target_state.st_mode) ? "symlink to a directory" + : "symlink to a special file"); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + if (target_state.st_uid != self) { + edit_path_note_owner(reason, reason_size, "symlink target", + (unsigned long)target_state.st_uid, (unsigned long)self); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } +#if CBM_CONFIG_EDIT_TARGET_UNDER_ROOT + if (!edit_path_under_root(resolved)) { + edit_path_note(reason, reason_size, "symlink target outside the configuration roots"); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } +#endif + char parent[CBM_CONFIG_EDIT_PATH_MAX]; + if (edit_path_split(resolved, parent, sizeof(parent), target->base, sizeof(target->base)) != + 0 || + edit_path_copy(resolved, target->path, sizeof(target->path)) != 0) { + edit_path_note(reason, reason_size, "symlink target path is too long"); + edit_path_target_reset(target); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + + int dir_flags = O_RDONLY | O_NOFOLLOW; +#ifdef O_DIRECTORY + dir_flags |= O_DIRECTORY; +#endif +#ifdef O_CLOEXEC + dir_flags |= O_CLOEXEC; +#endif + target->dirfd = open(parent, dir_flags); + struct stat dir_state; + if (target->dirfd < 0 || fstat(target->dirfd, &dir_state) != 0 || !S_ISDIR(dir_state.st_mode)) { + edit_path_note(reason, reason_size, + "parent directory of the symlink target cannot be opened"); + cbm_config_edit_target_close(target); + edit_path_target_reset(target); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + if (dir_state.st_uid != self && dir_state.st_uid != 0) { + if (reason && reason_size > 0U) { + (void)snprintf(reason, reason_size, + "parent directory of the symlink target is owned by uid %lu", + (unsigned long)dir_state.st_uid); + } + cbm_config_edit_target_close(target); + edit_path_target_reset(target); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + if ((dir_state.st_mode & S_IWOTH) != 0 && (dir_state.st_mode & S_ISVTX) == 0) { + edit_path_note(reason, reason_size, + "parent directory of the symlink target is writable by others"); + cbm_config_edit_target_close(target); + edit_path_target_reset(target); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + + int file_flags = O_RDONLY | O_NOFOLLOW | O_NONBLOCK; +#ifdef O_CLOEXEC + file_flags |= O_CLOEXEC; +#endif + target->fd = openat(target->dirfd, target->base, file_flags); + struct stat opened_state; + if (target->fd < 0 || fstat(target->fd, &opened_state) != 0) { + edit_path_note(reason, reason_size, "symlink target cannot be opened beside its parent"); + cbm_config_edit_target_close(target); + edit_path_target_reset(target); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + const char *mismatch = NULL; + if (!S_ISREG(opened_state.st_mode)) { + mismatch = "symlink target is not a regular file"; + } else if (opened_state.st_uid != self) { + mismatch = "symlink target changed owner while it was being checked"; + } else if (opened_state.st_nlink != 1U) { + mismatch = "symlink target is hard-linked"; + } else if (opened_state.st_dev != target_state.st_dev || + opened_state.st_ino != target_state.st_ino) { + mismatch = "symlink target changed while it was being checked"; + } +#ifdef CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API + if (!mismatch && edit_path_identity_skew) { + mismatch = "symlink target changed while it was being checked"; + } +#endif + if (mismatch) { + edit_path_note(reason, reason_size, mismatch); + cbm_config_edit_target_close(target); + edit_path_target_reset(target); + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + target->status = CBM_CONFIG_EDIT_PATH_FOLLOWED; + return CBM_CONFIG_EDIT_PATH_FOLLOWED; +} + +int cbm_config_edit_target_create_temp(const cbm_config_edit_target_t *target, const char *name, + unsigned mode) { + if (!target || target->status != CBM_CONFIG_EDIT_PATH_FOLLOWED || target->dirfd < 0 || !name || + !name[0] || strchr(name, '/')) { + errno = EINVAL; + return -1; + } + int flags = O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW; +#ifdef O_CLOEXEC + flags |= O_CLOEXEC; +#endif + return openat(target->dirfd, name, flags, (mode_t)mode); +} + +int cbm_config_edit_target_commit(const cbm_config_edit_target_t *target, const char *name) { + if (!target || target->status != CBM_CONFIG_EDIT_PATH_FOLLOWED || target->dirfd < 0 || !name || + !name[0] || strchr(name, '/')) { + errno = EINVAL; + return -1; + } + if (renameat(target->dirfd, name, target->dirfd, target->base) != 0) { + return -1; + } + return fsync(target->dirfd) == 0 ? 0 : -1; +} + +int cbm_config_edit_target_unlink(const cbm_config_edit_target_t *target, const char *name) { + if (!target || target->status != CBM_CONFIG_EDIT_PATH_FOLLOWED || target->dirfd < 0 || !name || + !name[0] || strchr(name, '/')) { + errno = EINVAL; + return -1; + } + return unlinkat(target->dirfd, name, 0); +} + +#else /* _WIN32 */ + +#ifdef CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API +void cbm_config_edit_path_set_invoking_uid_for_test(unsigned uid, int set) { + (void)uid; + (void)set; +} + +void cbm_config_edit_path_set_identity_skew_for_test(int set) { + (void)set; +} +#endif + +int cbm_config_edit_path_follow_add_root(const char *root) { + (void)root; + return 0; +} + +void cbm_config_edit_path_follow_clear(void) {} + +/* Reparse points are never followed by the editors on Windows; the path is + * handed back unchanged and FILE_FLAG_OPEN_REPARSE_POINT keeps refusing. */ +static int edit_path_classify(const char *path, cbm_config_edit_target_t *target, char *reason, + size_t reason_size) { + edit_path_target_reset(target); + if (reason && reason_size > 0U) { + reason[0] = '\0'; + } + if (!path || !path[0]) { + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + return edit_path_copy(path, target->path, sizeof(target->path)) == 0 + ? CBM_CONFIG_EDIT_PATH_DIRECT + : CBM_CONFIG_EDIT_PATH_REFUSED; +} + +int cbm_config_edit_target_create_temp(const cbm_config_edit_target_t *target, const char *name, + unsigned mode) { + (void)target; + (void)name; + (void)mode; + errno = EINVAL; + return -1; +} + +int cbm_config_edit_target_commit(const cbm_config_edit_target_t *target, const char *name) { + (void)target; + (void)name; + errno = EINVAL; + return -1; +} + +int cbm_config_edit_target_unlink(const cbm_config_edit_target_t *target, const char *name) { + (void)target; + (void)name; + errno = EINVAL; + return -1; +} + +#endif /* _WIN32 */ + +int cbm_config_edit_target_open(const char *path, cbm_config_edit_target_t *target) { + if (!target) { + return CBM_CONFIG_EDIT_PATH_REFUSED; + } + int status = edit_path_classify(path, target, NULL, 0U); + target->status = status; + return status; +} + +int cbm_config_edit_path_refusal(const char *path, char *out, size_t out_size) { + if (!out || out_size == 0U) { + return 0; + } + cbm_config_edit_target_t target; + (void)edit_path_classify(path, &target, out, out_size); + cbm_config_edit_target_close(&target); + return out[0] != '\0' ? 1 : 0; +} diff --git a/src/cli/config_edit_path.h b/src/cli/config_edit_path.h new file mode 100644 index 000000000..abe3d0e4c --- /dev/null +++ b/src/cli/config_edit_path.h @@ -0,0 +1,109 @@ +/* + * config_edit_path.h — the path a configuration editor may operate on when + * the path it was given is a symlink. + * + * Every config editor (JSON-like, TOML, YAML, text) opens its file with + * O_NOFOLLOW and refuses symlinks: a link planted at ~/.cursor/mcp.json could + * otherwise point a privileged writer at any file its planter can name. Users, + * however, keep exactly those files as symlinks into a dotfiles checkout, and + * a blanket refusal turned `uninstall` into data loss (#1954). + * + * Decision C (2026-09-07): a symlink the INVOKING user owns, resolving to a + * regular file the same user owns, is followed — but only as an explicit + * OPT-IN of the agent-configuration writers, scoped to configuration roots + * (HOME / XDG dirs) they register with cbm_config_edit_path_follow_add_root(). + * With no root registered the editors behave exactly as before; a path whose + * link lives outside every registered root is never followed, so repository + * paths stay refused whatever the caller. + * + * Race-safe sequence for a followed link (POSIX): + * lstat(link) → link owner == invoking uid (a root process follows only + * root-owned links) → realpath → open the target's PARENT DIRECTORY as a + * dirfd (O_DIRECTORY|O_NOFOLLOW; owned by the invoking user or root, not + * writable by others unless sticky) → openat(dirfd, base, O_NOFOLLOW) → + * fstat: regular file, owned by the invoking user, link count 1, and the + * same (st_dev, st_ino) as stat(realpath). + * Everything else is refused with an observable reason + * (cbm_config_edit_path_refusal): another owner, a root process over a + * non-root link, a dangling link, a link to a directory or special file, a + * hard-linked or swapped target, a foreign or world-writable parent. The + * editor then reads through the validated descriptor, stages its temp file + * with openat(dirfd, …, O_CREAT|O_EXCL) and commits with renameat on the same + * dirfd, so the link itself is never replaced. Windows never follows reparse + * points here: the editors keep FILE_FLAG_OPEN_REPARSE_POINT and the helper + * hands the path back unchanged. + * + * The check runs at every open and before every commit, never cached: a link + * swapped mid-edit resolves to a different identity, which also fails the + * editors' own pre-publish snapshot comparison on the resolved path. + */ +#ifndef CBM_CONFIG_EDIT_PATH_H +#define CBM_CONFIG_EDIT_PATH_H + +#include + +enum { + /* Callers pass a buffer of at least this size (cbm_canonical_path's floor). */ + CBM_CONFIG_EDIT_PATH_MAX = 4096, + CBM_CONFIG_EDIT_NAME_MAX = 256, +}; + +/* Pending user decision (#1954): must the resolved TARGET also stay under a + * registered root (1), or may it live anywhere the invoking user owns, such + * as a dotfiles checkout on an external volume (0, the default)? One switch. */ +#ifndef CBM_CONFIG_EDIT_TARGET_UNDER_ROOT +#define CBM_CONFIG_EDIT_TARGET_UNDER_ROOT 0 +#endif + +typedef enum { + CBM_CONFIG_EDIT_PATH_REFUSED = -1, /* a link that must not be followed */ + CBM_CONFIG_EDIT_PATH_DIRECT = 0, /* not a link (or absent): path is used as given */ + CBM_CONFIG_EDIT_PATH_FOLLOWED = 1, /* user-owned link inside a root: the target is used */ +} cbm_config_edit_path_status_t; + +typedef struct { + int status; /* cbm_config_edit_path_status_t */ + int dirfd; /* FOLLOWED: the target's parent directory, pinned; else -1 */ + int fd; /* FOLLOWED: the validated target, O_RDONLY|O_NOFOLLOW; else -1 */ + char base[CBM_CONFIG_EDIT_NAME_MAX]; /* FOLLOWED: the target's name inside dirfd */ + char path[CBM_CONFIG_EDIT_PATH_MAX]; /* the path the editor operates on */ +} cbm_config_edit_target_t; + +/* Opt-in scope (per thread): links under these canonical roots may be + * followed. Up to four roots; returns 0 when registered. Clear when the + * agent-configuration work is done. */ +int cbm_config_edit_path_follow_add_root(const char *root); +void cbm_config_edit_path_follow_clear(void); + +/* Classifies path and, for a followed link, pins its parent directory and + * validates the target. Returns the status; on REFUSED the target holds no + * descriptors and an empty path, and the editor fails closed as before. + * Always pair with cbm_config_edit_target_close(). */ +int cbm_config_edit_target_open(const char *path, cbm_config_edit_target_t *target); +void cbm_config_edit_target_close(cbm_config_edit_target_t *target); + +/* FOLLOWED targets only (return -1 with errno = EINVAL otherwise): + * create a temp file `name` beside the target (openat O_CREAT|O_EXCL| + * O_NOFOLLOW; EEXIST when the name is taken), publish it over the target with + * renameat on the pinned directory (which is then fsync'ed), or discard it. */ +int cbm_config_edit_target_create_temp(const cbm_config_edit_target_t *target, const char *name, + unsigned mode); +int cbm_config_edit_target_commit(const cbm_config_edit_target_t *target, const char *name); +int cbm_config_edit_target_unlink(const cbm_config_edit_target_t *target, const char *name); + +/* Observed-fact reason a symlink at path is refused, for diagnostics + * ("symlink owned by uid 0, not the invoking user (uid 501)", "dangling + * symlink", …). Writes "" and returns 0 when path is not a refused link, 1 + * when a reason was written. Never reports errno — it may be stale (#1537). */ +int cbm_config_edit_path_refusal(const char *path, char *out, size_t out_size); + +#ifdef CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API +/* Test seams (POSIX; no-ops on Windows). A test cannot create a link owned by + * another user without root, so the foreign-owner and root-run refusals move + * the observer instead of the file; the identity skew makes the validated + * descriptor disagree with stat(realpath), the swapped-target refusal. */ +void cbm_config_edit_path_set_invoking_uid_for_test(unsigned uid, int set); +void cbm_config_edit_path_set_identity_skew_for_test(int set); +#endif + +#endif /* CBM_CONFIG_EDIT_PATH_H */ diff --git a/src/cli/config_json_like.c b/src/cli/config_json_like.c index dc16c3c5c..4164fcca1 100644 --- a/src/cli/config_json_like.c +++ b/src/cli/config_json_like.c @@ -8,6 +8,7 @@ */ #include "cli/config_json_like.h" +#include "cli/config_edit_path.h" #include "foundation/compat.h" #include "foundation/compat_fs.h" @@ -1429,11 +1430,24 @@ static int jl_read_file(const char *path, char **content_out, size_t *length_out #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif - int descriptor = open(path, flags); + /* A symlink the invoking user owns, inside an opted-in configuration + * root, is read through the descriptor the helper validated on the + * target's pinned parent directory (#1954); everything else is opened by + * name with O_NOFOLLOW exactly as before. */ + cbm_config_edit_target_t target; + if (cbm_config_edit_target_open(path, &target) < 0) { + return -1; + } + int descriptor = target.fd; + target.fd = -1; + if (target.status != CBM_CONFIG_EDIT_PATH_FOLLOWED) { + descriptor = open(target.path, flags); + } + cbm_config_edit_target_close(&target); if (descriptor < 0) { if (errno == ENOENT) { struct stat path_state; - if (lstat(path, &path_state) == 0 || errno != ENOENT) { + if (lstat(target.path, &path_state) == 0 || errno != ENOENT) { return -1; } *missing_out = true; @@ -1593,10 +1607,31 @@ static int jl_replace_atomic(const char *temp_path, const char *path, bool desti #endif } -static int jl_write_atomic(const char *path, const char *content, size_t length, - const char *expected_content, size_t expected_length, - const jl_file_snapshot_t *expected_snapshot) { - if (jl_ensure_parent(path) != 0) { +static const char *jl_temp_name(const char *temp_path) { + const char *slash = strrchr(temp_path, '/'); + return slash ? slash + 1 : temp_path; +} + +/* Drop a staged temp file: through the pinned parent for a followed link, + * by name otherwise. */ +static void jl_discard_temp(const cbm_config_edit_target_t *target, const char *temp_path) { + if (target->status == CBM_CONFIG_EDIT_PATH_FOLLOWED) { + (void)cbm_config_edit_target_unlink(target, jl_temp_name(temp_path)); + } else { + (void)cbm_unlink(temp_path); + } +} + +/* For a followed link (#1954) the temp file is created with openat() beside + * the target and published with renameat() on the pinned parent, so the link + * itself is never replaced; every pre-publish comparison runs against the + * resolved path. A direct path keeps the by-name sequence unchanged. */ +static int jl_write_atomic_at(const cbm_config_edit_target_t *target, const char *content, + size_t length, const char *expected_content, size_t expected_length, + const jl_file_snapshot_t *expected_snapshot) { + const char *path = target->path; + bool followed = target->status == CBM_CONFIG_EDIT_PATH_FOLLOWED; + if (!followed && jl_ensure_parent(path) != 0) { return -1; } size_t path_length = strlen(path); @@ -1631,13 +1666,15 @@ static int jl_write_atomic(const char *path, const char *content, size_t length, #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif - int descriptor = open(temp_path, flags, 0600); + int descriptor = + followed ? cbm_config_edit_target_create_temp(target, jl_temp_name(temp_path), 0600U) + : open(temp_path, flags, 0600); if (descriptor >= 0) { file = fdopen(descriptor, "wb"); if (!file) { int saved_error = errno; close(descriptor); - (void)cbm_unlink(temp_path); + jl_discard_temp(target, temp_path); errno = saved_error; } } @@ -1680,7 +1717,7 @@ static int jl_write_atomic(const char *path, const char *content, size_t length, failed = true; } if (failed) { - cbm_unlink(temp_path); + jl_discard_temp(target, temp_path); free(temp_path); return -1; } @@ -1692,7 +1729,7 @@ static int jl_write_atomic(const char *path, const char *content, size_t length, temp_missing || temp_length != length || (length != 0U && memcmp(temp_content, content, length) != 0)) { free(temp_content); - cbm_unlink(temp_path); + jl_discard_temp(target, temp_path); free(temp_path); return -1; } @@ -1703,7 +1740,7 @@ static int jl_write_atomic(const char *path, const char *content, size_t length, } #endif if (jl_snapshot_matches_path(path, expected_content, expected_length, expected_snapshot) != 0) { - cbm_unlink(temp_path); + jl_discard_temp(target, temp_path); free(temp_path); return -1; } @@ -1714,8 +1751,9 @@ static int jl_write_atomic(const char *path, const char *content, size_t length, #endif if (jl_snapshot_matches_path(path, expected_content, expected_length, expected_snapshot) != 0 || jl_snapshot_matches_path(temp_path, content, length, &temp_snapshot) != 0 || - jl_replace_atomic(temp_path, path, expected_snapshot->exists) != 0) { - cbm_unlink(temp_path); + (followed ? cbm_config_edit_target_commit(target, jl_temp_name(temp_path)) + : jl_replace_atomic(temp_path, path, expected_snapshot->exists)) != 0) { + jl_discard_temp(target, temp_path); free(temp_path); return -1; } @@ -1723,6 +1761,19 @@ static int jl_write_atomic(const char *path, const char *content, size_t length, return 0; } +static int jl_write_atomic(const char *requested_path, const char *content, size_t length, + const char *expected_content, size_t expected_length, + const jl_file_snapshot_t *expected_snapshot) { + cbm_config_edit_target_t target; + if (cbm_config_edit_target_open(requested_path, &target) < 0) { + return -1; + } + int result = jl_write_atomic_at(&target, content, length, expected_content, expected_length, + expected_snapshot); + cbm_config_edit_target_close(&target); + return result; +} + static int jl_decode_utf8(const unsigned char *text, size_t remaining, uint32_t *codepoint, size_t *byte_count) { if (remaining == 0U) { diff --git a/src/cli/config_text_edit.c b/src/cli/config_text_edit.c index cee66bdfe..ad728f506 100644 --- a/src/cli/config_text_edit.c +++ b/src/cli/config_text_edit.c @@ -3,6 +3,7 @@ */ #include "cli/config_text_edit.h" +#include "cli/config_edit_path.h" #include "foundation/compat.h" #include "foundation/compat_fs.h" @@ -442,13 +443,26 @@ static int text_read_file(const char *path, char **data_out, size_t *len_out, #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif - int descriptor = open(path, flags); + /* A symlink the invoking user owns, inside an opted-in configuration + * root, is read through the descriptor the helper validated on the + * target's pinned parent directory (#1954); everything else is opened by + * name with O_NOFOLLOW exactly as before. */ + cbm_config_edit_target_t target; + if (cbm_config_edit_target_open(path, &target) < 0) { + return TEXT_ERROR; + } + int descriptor = target.fd; + target.fd = -1; + if (target.status != CBM_CONFIG_EDIT_PATH_FOLLOWED) { + descriptor = open(target.path, flags); + } + cbm_config_edit_target_close(&target); if (descriptor < 0) { if (errno != ENOENT) { return TEXT_ERROR; } struct stat path_state; - if (lstat(path, &path_state) == 0 || errno != ENOENT) { + if (lstat(target.path, &path_state) == 0 || errno != ENOENT) { return TEXT_ERROR; } char *empty = (char *)calloc(1U, 1U); @@ -589,14 +603,35 @@ static int text_replace_file(const char *temp_path, const char *path, int destin #endif } -static int text_write_atomic_mode(const char *path, const char *new_data, size_t new_len, - const char *old_data, size_t old_len, - const text_file_snapshot_t *snapshot, int override_mode, - unsigned int requested_mode) { +static const char *text_temp_name(const char *temp_path) { + const char *slash = strrchr(temp_path, '/'); + return slash ? slash + 1 : temp_path; +} + +/* Drop a staged temp file: through the pinned parent for a followed link, + * by name otherwise. */ +static void text_discard_temp(const cbm_config_edit_target_t *target, const char *temp_path) { + if (target->status == CBM_CONFIG_EDIT_PATH_FOLLOWED) { + (void)cbm_config_edit_target_unlink(target, text_temp_name(temp_path)); + } else { + (void)cbm_unlink(temp_path); + } +} + +/* For a followed link (#1954) the temp file is created with openat() beside + * the target and published with renameat() on the pinned parent, so the link + * itself is never replaced; every pre-publish comparison runs against the + * resolved path. A direct path keeps the by-name sequence unchanged. */ +static int text_write_atomic_mode_at(const cbm_config_edit_target_t *target, const char *new_data, + size_t new_len, const char *old_data, size_t old_len, + const text_file_snapshot_t *snapshot, int override_mode, + unsigned int requested_mode) { if (new_len > TEXT_MAX_BYTES || old_len > TEXT_MAX_BYTES || !text_requested_mode_valid(override_mode, requested_mode)) { return TEXT_ERROR; } + const char *path = target->path; + bool followed = target->status == CBM_CONFIG_EDIT_PATH_FOLLOWED; int content_same = new_len == old_len && (new_len == 0U || memcmp(new_data, old_data, new_len) == 0); #ifndef _WIN32 @@ -643,13 +678,15 @@ static int text_write_atomic_mode(const char *path, const char *new_data, size_t #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif - int descriptor = open(temp_path, flags, 0600); + int descriptor = + followed ? cbm_config_edit_target_create_temp(target, text_temp_name(temp_path), 0600U) + : open(temp_path, flags, 0600); if (descriptor >= 0) { file = text_fdopen(descriptor, "wb"); if (!file) { int saved_error = errno; text_close(descriptor); - (void)cbm_unlink(temp_path); + text_discard_temp(target, temp_path); errno = saved_error; } } @@ -702,7 +739,7 @@ static int text_write_atomic_mode(const char *path, const char *new_data, size_t failed = 1; } if (failed) { - (void)cbm_unlink(temp_path); + text_discard_temp(target, temp_path); free(temp_path); return TEXT_ERROR; } @@ -718,7 +755,7 @@ static int text_write_atomic_mode(const char *path, const char *new_data, size_t !text_snapshot_publication_equal(&trusted_temp_snapshot, &temp_snapshot) || temp_len != new_len || (new_len != 0U && memcmp(temp_data, new_data, new_len) != 0)) { free(temp_data); - (void)cbm_unlink(temp_path); + text_discard_temp(target, temp_path); free(temp_path); return TEXT_ERROR; } @@ -730,7 +767,7 @@ static int text_write_atomic_mode(const char *path, const char *new_data, size_t } #endif if (text_snapshot_matches_path(path, old_data, old_len, snapshot) != TEXT_OK) { - (void)cbm_unlink(temp_path); + text_discard_temp(target, temp_path); free(temp_path); return TEXT_ERROR; } @@ -741,8 +778,9 @@ static int text_write_atomic_mode(const char *path, const char *new_data, size_t #endif if (text_snapshot_matches_path(path, old_data, old_len, snapshot) != TEXT_OK || text_snapshot_matches_path(temp_path, new_data, new_len, &temp_snapshot) != TEXT_OK || - text_replace_file(temp_path, path, snapshot->exists) != TEXT_OK) { - (void)cbm_unlink(temp_path); + (followed ? cbm_config_edit_target_commit(target, text_temp_name(temp_path)) + : text_replace_file(temp_path, path, snapshot->exists)) != TEXT_OK) { + text_discard_temp(target, temp_path); free(temp_path); return TEXT_ERROR; } @@ -750,6 +788,20 @@ static int text_write_atomic_mode(const char *path, const char *new_data, size_t return TEXT_OK; } +static int text_write_atomic_mode(const char *requested_path, const char *new_data, size_t new_len, + const char *old_data, size_t old_len, + const text_file_snapshot_t *snapshot, int override_mode, + unsigned int requested_mode) { + cbm_config_edit_target_t target; + if (cbm_config_edit_target_open(requested_path, &target) < 0) { + return TEXT_ERROR; + } + int result = text_write_atomic_mode_at(&target, new_data, new_len, old_data, old_len, snapshot, + override_mode, requested_mode); + cbm_config_edit_target_close(&target); + return result; +} + static int text_write_atomic(const char *path, const char *new_data, size_t new_len, const char *old_data, size_t old_len, const text_file_snapshot_t *snapshot) { diff --git a/src/cli/config_toml_edit.c b/src/cli/config_toml_edit.c index ef73c66fc..745bdda0d 100644 --- a/src/cli/config_toml_edit.c +++ b/src/cli/config_toml_edit.c @@ -3,6 +3,7 @@ */ #include "cli/config_toml_edit.h" +#include "cli/config_edit_path.h" #include "foundation/compat.h" #include "foundation/compat_fs.h" @@ -465,13 +466,26 @@ static int toml_read_file(const char *path, char **out_data, size_t *out_len, #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif - int fd = open(path, flags); + /* A symlink the invoking user owns, inside an opted-in configuration + * root, is read through the descriptor the helper validated on the + * target's pinned parent directory (#1954); everything else is opened by + * name with O_NOFOLLOW exactly as before. */ + cbm_config_edit_target_t target; + if (cbm_config_edit_target_open(path, &target) < 0) { + return TOML_EDIT_ERR; + } + int fd = target.fd; + target.fd = -1; + if (target.status != CBM_CONFIG_EDIT_PATH_FOLLOWED) { + fd = open(target.path, flags); + } + cbm_config_edit_target_close(&target); if (fd < 0) { if (errno != ENOENT) { return TOML_EDIT_ERR; } struct stat path_state; - if (lstat(path, &path_state) == 0 || errno != ENOENT) { + if (lstat(target.path, &path_state) == 0 || errno != ENOENT) { return TOML_EDIT_ERR; } char *empty = (char *)malloc(1U); @@ -612,28 +626,65 @@ static int toml_replace_atomic(const char *temp_path, const char *path, int exis #endif } -static int toml_write_atomic(const char *path, const char *old_data, size_t old_len, - const char *new_data, size_t new_len, - const toml_file_snapshot_t *snapshot) { +static const char *toml_temp_name(const char *temp_path) { + const char *slash = strrchr(temp_path, '/'); + return slash ? slash + 1 : temp_path; +} + +/* Drop a staged temp file: through the pinned parent for a followed link, + * by name otherwise. */ +static void toml_discard_temp(const cbm_config_edit_target_t *target, const char *temp_path) { + if (target->status == CBM_CONFIG_EDIT_PATH_FOLLOWED) { + (void)cbm_config_edit_target_unlink(target, toml_temp_name(temp_path)); + } else { + (void)cbm_unlink(temp_path); + } +} + +/* For a followed link (#1954) the temp file is created with openat() beside + * the target and published with renameat() on the pinned parent, so the link + * itself is never replaced; every pre-publish comparison runs against the + * resolved path. A direct path keeps the mkstemp + rename sequence unchanged. */ +static int toml_write_atomic_at(const cbm_config_edit_target_t *target, const char *old_data, + size_t old_len, const char *new_data, size_t new_len, + const toml_file_snapshot_t *snapshot) { if (old_len > TOML_EDIT_MAX_BYTES || new_len > TOML_EDIT_MAX_BYTES) { return TOML_EDIT_ERR; } + const char *path = target->path; + bool followed = target->status == CBM_CONFIG_EDIT_PATH_FOLLOWED; if (old_len == new_len && (old_len == 0 || memcmp(old_data, new_data, old_len) == 0)) { return TOML_EDIT_OK; } size_t path_len = strlen(path); static const char suffix[] = ".XXXXXX"; - if (path_len > SIZE_MAX - sizeof(suffix)) { + enum { TOML_TEMP_SUFFIX_SPACE = 48, TOML_TEMP_ATTEMPTS = 64 }; + if (path_len > SIZE_MAX - TOML_TEMP_SUFFIX_SPACE) { return TOML_EDIT_ERR; } - char *temp_path = (char *)malloc(path_len + sizeof(suffix)); + size_t temp_capacity = path_len + TOML_TEMP_SUFFIX_SPACE; + char *temp_path = (char *)malloc(temp_capacity); if (!temp_path) { return TOML_EDIT_ERR; } - memcpy(temp_path, path, path_len); - memcpy(temp_path + path_len, suffix, sizeof(suffix)); - int fd = cbm_mkstemp(temp_path); + int fd = -1; + if (followed) { + for (unsigned attempt = 0U; attempt < TOML_TEMP_ATTEMPTS && fd < 0; attempt++) { + int written = snprintf(temp_path, temp_capacity, "%s.cbm-toml-%u.tmp", path, attempt); + if (written < 0 || (size_t)written >= temp_capacity) { + break; + } + fd = cbm_config_edit_target_create_temp(target, toml_temp_name(temp_path), 0600U); + if (fd < 0 && errno != EEXIST) { + break; + } + } + } else { + memcpy(temp_path, path, path_len); + memcpy(temp_path + path_len, suffix, sizeof(suffix)); + fd = cbm_mkstemp(temp_path); + } if (fd < 0) { free(temp_path); return TOML_EDIT_ERR; @@ -641,7 +692,7 @@ static int toml_write_atomic(const char *path, const char *old_data, size_t old_ FILE *file = toml_fdopen(fd, "wb"); if (!file) { (void)toml_close(fd); - (void)cbm_unlink(temp_path); + toml_discard_temp(target, temp_path); free(temp_path); return TOML_EDIT_ERR; } @@ -667,7 +718,7 @@ static int toml_write_atomic(const char *path, const char *old_data, size_t old_ failed = 1; } if (failed) { - (void)cbm_unlink(temp_path); + toml_discard_temp(target, temp_path); free(temp_path); return TOML_EDIT_ERR; } @@ -678,7 +729,7 @@ static int toml_write_atomic(const char *path, const char *old_data, size_t old_ !temp_snapshot.exists || temp_len != new_len || (new_len != 0U && memcmp(temp_data, new_data, new_len) != 0)) { free(temp_data); - (void)cbm_unlink(temp_path); + toml_discard_temp(target, temp_path); free(temp_path); return TOML_EDIT_ERR; } @@ -689,7 +740,7 @@ static int toml_write_atomic(const char *path, const char *old_data, size_t old_ } #endif if (toml_snapshot_matches_path(path, old_data, old_len, snapshot) != TOML_EDIT_OK) { - (void)cbm_unlink(temp_path); + toml_discard_temp(target, temp_path); free(temp_path); return TOML_EDIT_ERR; } @@ -700,8 +751,9 @@ static int toml_write_atomic(const char *path, const char *old_data, size_t old_ #endif if (toml_snapshot_matches_path(path, old_data, old_len, snapshot) != TOML_EDIT_OK || toml_snapshot_matches_path(temp_path, new_data, new_len, &temp_snapshot) != TOML_EDIT_OK || - toml_replace_atomic(temp_path, path, snapshot->exists) != TOML_EDIT_OK) { - (void)cbm_unlink(temp_path); + (followed ? cbm_config_edit_target_commit(target, toml_temp_name(temp_path)) + : toml_replace_atomic(temp_path, path, snapshot->exists)) != TOML_EDIT_OK) { + toml_discard_temp(target, temp_path); free(temp_path); return TOML_EDIT_ERR; } @@ -709,6 +761,18 @@ static int toml_write_atomic(const char *path, const char *old_data, size_t old_ return TOML_EDIT_OK; } +static int toml_write_atomic(const char *requested_path, const char *old_data, size_t old_len, + const char *new_data, size_t new_len, + const toml_file_snapshot_t *snapshot) { + cbm_config_edit_target_t target; + if (cbm_config_edit_target_open(requested_path, &target) < 0) { + return TOML_EDIT_ERR; + } + int result = toml_write_atomic_at(&target, old_data, old_len, new_data, new_len, snapshot); + cbm_config_edit_target_close(&target); + return result; +} + #ifdef CBM_TOML_EDIT_ENABLE_TEST_API void cbm_toml_set_precommit_hook_for_testing(cbm_toml_precommit_test_hook_t hook, void *context) { toml_precommit_test_hook = hook; diff --git a/src/cli/config_yaml_edit.c b/src/cli/config_yaml_edit.c index e084b59c1..e64827325 100644 --- a/src/cli/config_yaml_edit.c +++ b/src/cli/config_yaml_edit.c @@ -7,6 +7,7 @@ */ #include "cli/config_yaml_edit.h" +#include "cli/config_edit_path.h" #include "foundation/compat.h" #include "foundation/compat_fs.h" @@ -694,13 +695,26 @@ static int yaml_read_file(const char *path, char **out_data, size_t *out_len, #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif - int descriptor = open(path, flags); + /* A symlink the invoking user owns, inside an opted-in configuration + * root, is read through the descriptor the helper validated on the + * target's pinned parent directory (#1954); everything else is opened by + * name with O_NOFOLLOW exactly as before. */ + cbm_config_edit_target_t target; + if (cbm_config_edit_target_open(path, &target) < 0) { + return YAML_ERROR; + } + int descriptor = target.fd; + target.fd = -1; + if (target.status != CBM_CONFIG_EDIT_PATH_FOLLOWED) { + descriptor = open(target.path, flags); + } + cbm_config_edit_target_close(&target); if (descriptor < 0) { if (errno != ENOENT) { return YAML_ERROR; } struct stat path_state; - if (lstat(path, &path_state) == 0 || errno != ENOENT) { + if (lstat(target.path, &path_state) == 0 || errno != ENOENT) { return YAML_ERROR; } char *empty = (char *)calloc(YAML_UNIT, YAML_UNIT); @@ -869,9 +883,30 @@ static int yaml_replace_file(const char *temp_path, const char *path, bool desti #endif } -static int yaml_write_atomic(const char *path, const char *data, size_t len, - const char *expected_data, size_t expected_len, - const yaml_file_snapshot_t *expected_snapshot) { +static const char *yaml_temp_name(const char *temp_path) { + const char *slash = strrchr(temp_path, '/'); + return slash ? slash + 1 : temp_path; +} + +/* Drop a staged temp file: through the pinned parent for a followed link, + * by name otherwise. */ +static void yaml_discard_temp(const cbm_config_edit_target_t *target, const char *temp_path) { + if (target->status == CBM_CONFIG_EDIT_PATH_FOLLOWED) { + (void)cbm_config_edit_target_unlink(target, yaml_temp_name(temp_path)); + } else { + (void)cbm_unlink(temp_path); + } +} + +/* For a followed link (#1954) the temp file is created with openat() beside + * the target and published with renameat() on the pinned parent, so the link + * itself is never replaced; every pre-publish comparison runs against the + * resolved path. A direct path keeps the by-name sequence unchanged. */ +static int yaml_write_atomic_at(const cbm_config_edit_target_t *target, const char *data, + size_t len, const char *expected_data, size_t expected_len, + const yaml_file_snapshot_t *expected_snapshot) { + const char *path = target->path; + bool followed = target->status == CBM_CONFIG_EDIT_PATH_FOLLOWED; size_t path_len = 0U; if (yaml_bounded_strlen(path, YAML_OUTPUT_MAX, &path_len) != 0 || path_len > SIZE_MAX - YAML_TMP_SUFFIX_MAX - YAML_UNIT) { @@ -901,12 +936,14 @@ static int yaml_write_atomic(const char *path, const char *data, size_t len, #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif - int descriptor = open(temp_path, flags, YAML_NEW_FILE_MODE); + int descriptor = followed ? cbm_config_edit_target_create_temp( + target, yaml_temp_name(temp_path), YAML_NEW_FILE_MODE) + : open(temp_path, flags, YAML_NEW_FILE_MODE); if (descriptor >= 0) { file = fdopen(descriptor, "wb"); if (!file) { (void)close(descriptor); - (void)cbm_unlink(temp_path); + yaml_discard_temp(target, temp_path); free(temp_path); return YAML_ERROR; } @@ -947,7 +984,7 @@ static int yaml_write_atomic(const char *path, const char *data, size_t len, failed = true; } if (failed) { - (void)cbm_unlink(temp_path); + yaml_discard_temp(target, temp_path); free(temp_path); return YAML_ERROR; } @@ -958,7 +995,7 @@ static int yaml_write_atomic(const char *path, const char *data, size_t len, !temp_snapshot.exists || temp_len != len || (len != 0U && memcmp(temp_data, data, len) != 0)) { free(temp_data); - (void)cbm_unlink(temp_path); + yaml_discard_temp(target, temp_path); free(temp_path); return YAML_ERROR; } @@ -970,7 +1007,7 @@ static int yaml_write_atomic(const char *path, const char *data, size_t len, } #endif if (yaml_snapshot_matches_path(path, expected_data, expected_len, expected_snapshot) != 0) { - (void)cbm_unlink(temp_path); + yaml_discard_temp(target, temp_path); free(temp_path); return YAML_ERROR; } @@ -981,8 +1018,9 @@ static int yaml_write_atomic(const char *path, const char *data, size_t len, #endif if (yaml_snapshot_matches_path(path, expected_data, expected_len, expected_snapshot) != 0 || yaml_snapshot_matches_path(temp_path, data, len, &temp_snapshot) != 0 || - yaml_replace_file(temp_path, path, expected_snapshot->exists) != 0) { - (void)cbm_unlink(temp_path); + (followed ? cbm_config_edit_target_commit(target, yaml_temp_name(temp_path)) + : yaml_replace_file(temp_path, path, expected_snapshot->exists)) != 0) { + yaml_discard_temp(target, temp_path); free(temp_path); return YAML_ERROR; } @@ -990,6 +1028,19 @@ static int yaml_write_atomic(const char *path, const char *data, size_t len, return 0; } +static int yaml_write_atomic(const char *requested_path, const char *data, size_t len, + const char *expected_data, size_t expected_len, + const yaml_file_snapshot_t *expected_snapshot) { + cbm_config_edit_target_t target; + if (cbm_config_edit_target_open(requested_path, &target) < 0) { + return YAML_ERROR; + } + int result = + yaml_write_atomic_at(&target, data, len, expected_data, expected_len, expected_snapshot); + cbm_config_edit_target_close(&target); + return result; +} + #ifdef CBM_YAML_ENABLE_TEST_API void cbm_yaml_set_precommit_hook_for_testing(cbm_yaml_precommit_test_hook_t hook, void *context) { yaml_precommit_test_hook = hook; diff --git a/tests/test_cli.c b/tests/test_cli.c index aab397da4..3be2d7403 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -2402,6 +2402,99 @@ TEST(cli_uninstall_removes_binary_and_index_when_agent_config_cleanup_fails) { ASSERT_TRUE(failure_named); PASS(); } + +/* #1954 end to end: ~/.cursor/mcp.json is a user-owned symlink into a + * dotfiles checkout. The uninstall command opts in to following it, removes + * our entry THROUGH the link, leaves the link pointing where it did, and + * exits 0 with the binary and the index gone. */ +TEST(cli_uninstall_cleans_user_owned_symlinked_config) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-uninstall-symlinked-XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) { + FAIL("cbm_mkdtemp failed"); + } + char *old_home = NULL; + char *old_cache = NULL; + cli_activation_save_env(&old_home, &old_cache); + cbm_setenv("HOME", tmpdir, 1); + char *old_path = save_test_env("PATH"); + cbm_setenv("PATH", tmpdir, 1); + + char cache_dir[512]; + char index_path[640]; + snprintf(cache_dir, sizeof(cache_dir), "%s/cache", tmpdir); + cbm_setenv("CBM_CACHE_DIR", cache_dir, 1); + test_mkdirp(cache_dir); + snprintf(index_path, sizeof(index_path), "%s/project.db", cache_dir); + write_test_file(index_path, "index goes with the uninstall"); + + char bin_dir[512]; + char bin_target[640]; + snprintf(bin_dir, sizeof(bin_dir), "%s/.local/bin", tmpdir); + test_mkdirp(bin_dir); + snprintf(bin_target, sizeof(bin_target), "%s/codebase-memory-mcp", bin_dir); + write_test_file(bin_target, "binary goes with the uninstall"); + + char cursor_dir[512]; + char cursor_config[640]; + char dotfiles_dir[512]; + char dotfiles_config[640]; + snprintf(cursor_dir, sizeof(cursor_dir), "%s/.cursor", tmpdir); + snprintf(cursor_config, sizeof(cursor_config), "%s/mcp.json", cursor_dir); + snprintf(dotfiles_dir, sizeof(dotfiles_dir), "%s/.dotfiles/cursor", tmpdir); + snprintf(dotfiles_config, sizeof(dotfiles_config), "%s/mcp.json", dotfiles_dir); + test_mkdirp(cursor_dir); + test_mkdirp(dotfiles_dir); + write_test_file(dotfiles_config, + "{\n \"mcpServers\": {\n \"keep\": {\"command\": \"x\"}\n }\n}\n"); + int installed = cbm_install_editor_mcp(bin_target, dotfiles_config); + if (symlink(dotfiles_config, cursor_config) != 0) { + FAIL("symlink failed"); + } + + cli_activation_fake_t fake = {.mutation_reserve_result = 1}; + cbm_cli_activation_ops_t ops = cli_activation_fake_ops(&fake); + cbm_cli_set_activation_ops_for_test(&ops); + cli_fd_capture_t out_capture; + cli_fd_capture_t err_capture; + cli_fd_capture_begin(&out_capture, stdout, STDOUT_FILENO); + cli_fd_capture_begin(&err_capture, stderr, STDERR_FILENO); + char *argv[] = {"--yes"}; + int rc = cli_test_cmd_uninstall(1, argv); + char *err_text = cli_fd_capture_end(&err_capture); + char *out_text = cli_fd_capture_end(&out_capture); + cbm_cli_set_activation_ops_for_test(NULL); + cbm_set_auto_answer_for_test(0); + + struct stat state; + bool binary_gone = lstat(bin_target, &state) != 0 && errno == ENOENT; + bool index_gone = lstat(index_path, &state) != 0 && errno == ENOENT; + bool link_intact = lstat(cursor_config, &state) == 0 && S_ISLNK(state.st_mode); + char link_value[640] = {0}; + ssize_t link_length = readlink(cursor_config, link_value, sizeof(link_value) - 1U); + bool link_same = link_length > 0 && strcmp(link_value, dotfiles_config) == 0; + char *after = read_test_file_alloc(dotfiles_config); + bool entry_removed = + after && !strstr(after, "codebase-memory-mcp") && strstr(after, "\"keep\""); + bool no_error = err_text && !strstr(err_text, "error:"); + + cli_activation_restore_env(old_home, old_cache); + restore_test_env("PATH", old_path); + test_rmdir_r(tmpdir); + free(after); + free(err_text); + free(out_text); + + ASSERT_EQ(installed, 0); + ASSERT_EQ(rc, 0); + ASSERT_TRUE(binary_gone); + ASSERT_TRUE(index_gone); + ASSERT_TRUE(link_intact); + ASSERT_TRUE(link_same); + ASSERT_TRUE(entry_removed); + ASSERT_TRUE(no_error); + PASS(); +} #endif TEST(cli_activation_guard_is_bypassed_for_dry_run_and_plan) { @@ -14680,6 +14773,7 @@ SUITE(cli) { RUN_TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain); #ifndef _WIN32 RUN_TEST(cli_uninstall_removes_binary_and_index_when_agent_config_cleanup_fails); + RUN_TEST(cli_uninstall_cleans_user_owned_symlinked_config); #endif RUN_TEST(cli_activation_guard_is_bypassed_for_dry_run_and_plan); #ifdef _WIN32 diff --git a/tests/test_config_json_like.c b/tests/test_config_json_like.c index c6f29a387..08a070378 100644 --- a/tests/test_config_json_like.c +++ b/tests/test_config_json_like.c @@ -8,6 +8,8 @@ #define CBM_JSON_LIKE_ENABLE_TEST_API 1 #include "../src/cli/config_json_like.h" +#define CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API 1 +#include "../src/cli/config_edit_path.h" #include "../src/foundation/compat.h" #include "../src/foundation/compat_fs.h" @@ -281,7 +283,169 @@ TEST(config_json_like_rejects_non_regular_path) { } #ifndef _WIN32 -TEST(config_json_like_rejects_symlink_without_touching_target) { +/* Any editor temp file left anywhere in the fixture directory, whatever + * document it was staged for: a link's target lives in the same directory. */ +static size_t jl_stray_temp_count(const jl_fixture_t *fixture) { + cbm_dir_t *directory = cbm_opendir(fixture->directory); + if (!directory) { + return SIZE_MAX; + } + size_t count = 0U; + cbm_dirent_t *entry = NULL; + while ((entry = cbm_readdir(directory)) != NULL) { + if (strstr(entry->name, ".cbm.tmp.")) { + count++; + } + } + cbm_closedir(directory); + return count; +} + +/* A link owned by someone else is never followed. A test cannot create a + * foreign-owned link without root, so the observer moves instead (test seam): + * the refusal touches neither the link nor its target and stages no temp file. */ +TEST(config_json_like_rejects_foreign_owned_symlink_without_touching_target) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + char target[sizeof(fixture.path) + 32U]; + ASSERT(snprintf(target, sizeof(target), "%s/target.json", fixture.directory) > 0); + const char *original = "{\"target\":true}\n"; + ASSERT_EQ(jl_write(target, original), 0); + ASSERT_EQ(symlink(target, fixture.path), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.directory), 0); + cbm_config_edit_path_set_invoking_uid_for_test((unsigned)geteuid() + 1U, 1); + + const char *root[] = {NULL}; + int upsert_rc = cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"); + char reason[256]; + int refused = cbm_config_edit_path_refusal(fixture.path, reason, sizeof(reason)); + cbm_config_edit_path_set_invoking_uid_for_test(0U, 0); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(upsert_rc, -1); + ASSERT_EQ(refused, 1); + ASSERT_NOT_NULL(strstr(reason, "not the invoking user")); + struct stat link_state; + ASSERT_EQ(lstat(fixture.path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + char *content = jl_read(target); + ASSERT_NOT_NULL(content); + ASSERT_STR_EQ(content, original); + free(content); + ASSERT_EQ(jl_stray_temp_count(&fixture), 0U); + + ASSERT_EQ(cbm_unlink(fixture.path), 0); + ASSERT_EQ(cbm_unlink(target), 0); + jl_fixture_close(&fixture); + PASS(); +} + +/* A root process never follows a link a non-root user owns: a user-planted + * link must not redirect a privileged writer. A real root run (CI containers) + * demotes the link to an unprivileged owner first; elsewhere the observer moves. */ +TEST(config_json_like_root_refuses_user_owned_symlink) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + char target[sizeof(fixture.path) + 32U]; + ASSERT(snprintf(target, sizeof(target), "%s/target.json", fixture.directory) > 0); + const char *original = "{\"target\":true}\n"; + ASSERT_EQ(jl_write(target, original), 0); + ASSERT_EQ(symlink(target, fixture.path), 0); + if (geteuid() == 0 && lchown(fixture.path, 65534, 65534) != 0) { + (void)cbm_unlink(fixture.path); + (void)cbm_unlink(target); + jl_fixture_close(&fixture); + FAIL("lchown to an unprivileged owner failed"); + } + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.directory), 0); + cbm_config_edit_path_set_invoking_uid_for_test(0U, 1); + + const char *root[] = {NULL}; + int upsert_rc = cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"); + char reason[256]; + int refused = cbm_config_edit_path_refusal(fixture.path, reason, sizeof(reason)); + cbm_config_edit_path_set_invoking_uid_for_test(0U, 0); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(upsert_rc, -1); + ASSERT_EQ(refused, 1); + ASSERT_NOT_NULL(strstr(reason, "root process")); + struct stat link_state; + ASSERT_EQ(lstat(fixture.path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + char *content = jl_read(target); + ASSERT_NOT_NULL(content); + ASSERT_STR_EQ(content, original); + free(content); + ASSERT_EQ(jl_stray_temp_count(&fixture), 0U); + + ASSERT_EQ(cbm_unlink(fixture.path), 0); + ASSERT_EQ(cbm_unlink(target), 0); + jl_fixture_close(&fixture); + PASS(); +} + +TEST(config_json_like_refuses_dangling_symlink) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + char missing[sizeof(fixture.path) + 32U]; + ASSERT(snprintf(missing, sizeof(missing), "%s/missing.json", fixture.directory) > 0); + ASSERT_EQ(symlink(missing, fixture.path), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.directory), 0); + + const char *root[] = {NULL}; + int upsert_rc = cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"); + char *document = NULL; + size_t length = 0U; + int read_rc = cbm_json_like_read_document(fixture.path, &document, &length); + free(document); + char reason[256]; + int refused = cbm_config_edit_path_refusal(fixture.path, reason, sizeof(reason)); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(upsert_rc, -1); + ASSERT_EQ(read_rc, -1); + ASSERT_EQ(refused, 1); + ASSERT_NOT_NULL(strstr(reason, "dangling")); + struct stat state; + ASSERT_EQ(lstat(fixture.path, &state), 0); + ASSERT(S_ISLNK(state.st_mode)); + ASSERT(lstat(missing, &state) != 0); + ASSERT_EQ(jl_stray_temp_count(&fixture), 0U); + + ASSERT_EQ(cbm_unlink(fixture.path), 0); + jl_fixture_close(&fixture); + PASS(); +} + +TEST(config_json_like_refuses_symlink_to_directory) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + char sub[sizeof(fixture.path) + 32U]; + ASSERT(snprintf(sub, sizeof(sub), "%s/sub", fixture.directory) > 0); + ASSERT_EQ(cbm_mkdir(sub), 0); + ASSERT_EQ(symlink(sub, fixture.path), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.directory), 0); + + const char *root[] = {NULL}; + int upsert_rc = cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"); + char reason[256]; + int refused = cbm_config_edit_path_refusal(fixture.path, reason, sizeof(reason)); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(upsert_rc, -1); + ASSERT_EQ(refused, 1); + ASSERT_NOT_NULL(strstr(reason, "directory")); + struct stat state; + ASSERT_EQ(lstat(fixture.path, &state), 0); + ASSERT(S_ISLNK(state.st_mode)); + ASSERT_EQ(jl_stray_temp_count(&fixture), 0U); + + ASSERT_EQ(cbm_unlink(fixture.path), 0); + ASSERT_EQ(cbm_rmdir(sub), 0); + jl_fixture_close(&fixture); + PASS(); +} + +/* Without the agent-config writers' opt-in, a same-owner link is still + * refused: the follow is never a property of every editor caller. */ +TEST(config_json_like_refuses_symlink_without_opt_in) { jl_fixture_t fixture; ASSERT_EQ(jl_fixture_open(&fixture), 0); char target[sizeof(fixture.path) + 32U]; @@ -289,9 +453,26 @@ TEST(config_json_like_rejects_symlink_without_touching_target) { const char *original = "{\"target\":true}\n"; ASSERT_EQ(jl_write(target, original), 0); ASSERT_EQ(symlink(target, fixture.path), 0); + cbm_config_edit_path_follow_clear(); const char *root[] = {NULL}; ASSERT_EQ(cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"), -1); + char reason[256]; + ASSERT_EQ(cbm_config_edit_path_refusal(fixture.path, reason, sizeof(reason)), 1); + ASSERT_NOT_NULL(strstr(reason, "not followed by this command")); + + /* An opt-in for a different root does not cover this link either. */ + char elsewhere[sizeof(fixture.path) + 32U]; + ASSERT(snprintf(elsewhere, sizeof(elsewhere), "%s/elsewhere", fixture.directory) > 0); + ASSERT_EQ(cbm_mkdir(elsewhere), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(elsewhere), 0); + int outside_rc = cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"); + int outside_refused = cbm_config_edit_path_refusal(fixture.path, reason, sizeof(reason)); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(outside_rc, -1); + ASSERT_EQ(outside_refused, 1); + ASSERT_NOT_NULL(strstr(reason, "outside the opted-in configuration roots")); + struct stat link_state; ASSERT_EQ(lstat(fixture.path, &link_state), 0); ASSERT(S_ISLNK(link_state.st_mode)); @@ -299,7 +480,42 @@ TEST(config_json_like_rejects_symlink_without_touching_target) { ASSERT_NOT_NULL(content); ASSERT_STR_EQ(content, original); free(content); - ASSERT_EQ(jl_temp_file_count(&fixture), 0U); + ASSERT_EQ(jl_stray_temp_count(&fixture), 0U); + + ASSERT_EQ(cbm_rmdir(elsewhere), 0); + ASSERT_EQ(cbm_unlink(fixture.path), 0); + ASSERT_EQ(cbm_unlink(target), 0); + jl_fixture_close(&fixture); + PASS(); +} + +/* The validated descriptor must be the very file stat(realpath) saw: a + * target swapped between the two checks (seam) is refused, untouched. */ +TEST(config_json_like_refuses_symlink_when_target_identity_skews) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + char target[sizeof(fixture.path) + 32U]; + ASSERT(snprintf(target, sizeof(target), "%s/target.json", fixture.directory) > 0); + const char *original = "{\"target\":true}\n"; + ASSERT_EQ(jl_write(target, original), 0); + ASSERT_EQ(symlink(target, fixture.path), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.directory), 0); + cbm_config_edit_path_set_identity_skew_for_test(1); + + const char *root[] = {NULL}; + int upsert_rc = cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"); + char reason[256]; + int refused = cbm_config_edit_path_refusal(fixture.path, reason, sizeof(reason)); + cbm_config_edit_path_set_identity_skew_for_test(0); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(upsert_rc, -1); + ASSERT_EQ(refused, 1); + ASSERT_NOT_NULL(strstr(reason, "changed while it was being checked")); + char *content = jl_read(target); + ASSERT_NOT_NULL(content); + ASSERT_STR_EQ(content, original); + free(content); + ASSERT_EQ(jl_stray_temp_count(&fixture), 0U); ASSERT_EQ(cbm_unlink(fixture.path), 0); ASSERT_EQ(cbm_unlink(target), 0); @@ -307,6 +523,166 @@ TEST(config_json_like_rejects_symlink_without_touching_target) { PASS(); } +/* The target's parent directory is pinned and must not be writable by others + * (a sticky directory is fine): otherwise anyone could stage a swap there. */ +TEST(config_json_like_refuses_symlink_with_world_writable_parent) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + char dotfiles[sizeof(fixture.path) + 32U]; + char target[sizeof(fixture.path) + 64U]; + ASSERT(snprintf(dotfiles, sizeof(dotfiles), "%s/dotfiles", fixture.directory) > 0); + ASSERT(snprintf(target, sizeof(target), "%s/target.json", dotfiles) > 0); + ASSERT_EQ(cbm_mkdir(dotfiles), 0); + const char *original = "{\"target\":true}\n"; + ASSERT_EQ(jl_write(target, original), 0); + ASSERT_EQ(symlink(target, fixture.path), 0); + ASSERT_EQ(chmod(dotfiles, 0777), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.directory), 0); + + const char *root[] = {NULL}; + int loose_rc = cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"); + char reason[256]; + int loose_refused = cbm_config_edit_path_refusal(fixture.path, reason, sizeof(reason)); + ASSERT_EQ(chmod(dotfiles, 0755), 0); + int tight_rc = cbm_json_like_upsert_entry(fixture.path, root, 0U, "owned", "true"); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(loose_rc, -1); + ASSERT_EQ(loose_refused, 1); + ASSERT_NOT_NULL(strstr(reason, "writable by others")); + ASSERT_EQ(tight_rc, 0); + char *content = jl_read(target); + ASSERT_NOT_NULL(content); + ASSERT_NOT_NULL(strstr(content, "\"owned\"")); + free(content); + + ASSERT_EQ(cbm_unlink(fixture.path), 0); + ASSERT_EQ(cbm_unlink(target), 0); + ASSERT_EQ(cbm_rmdir(dotfiles), 0); + jl_fixture_close(&fixture); + PASS(); +} + +typedef struct { + char seen_path[640]; + size_t temps_beside_target; + size_t calls; +} jl_follow_hook_state_t; + +/* Pre-commit hook: the editor hands over the path it is about to publish — + * for a followed link that must be the RESOLVED target, with the staged + * temp file sitting right beside it, never beside the link. */ +static void jl_follow_precommit_hook(const char *path, void *context) { + jl_follow_hook_state_t *state = context; + state->calls++; + (void)snprintf(state->seen_path, sizeof(state->seen_path), "%s", path); + char directory[640]; + (void)snprintf(directory, sizeof(directory), "%s", path); + char *slash = strrchr(directory, '/'); + if (!slash) { + return; + } + const char *base = slash + 1; + char prefix[320]; + (void)snprintf(prefix, sizeof(prefix), "%s.cbm.tmp.", base); + *slash = '\0'; + cbm_dir_t *handle = cbm_opendir(directory); + if (!handle) { + return; + } + cbm_dirent_t *entry = NULL; + while ((entry = cbm_readdir(handle)) != NULL) { + if (strncmp(entry->name, prefix, strlen(prefix)) == 0) { + state->temps_beside_target++; + } + } + cbm_closedir(handle); +} + +/* Decision C (#1954): a symlink the invoking user owns, pointing at a regular + * file the same user owns, is edited THROUGH once the writer opted in — the + * link survives and still points at the same target, the temp file is staged + * beside the target, and every edit is byte-identical to the same edit on a + * plain file. */ +TEST(config_json_like_follows_user_owned_symlink_in_place) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + char target[sizeof(fixture.path) + 32U]; + char control[sizeof(fixture.path) + 32U]; + ASSERT(snprintf(target, sizeof(target), "%s/target.json", fixture.directory) > 0); + ASSERT(snprintf(control, sizeof(control), "%s/control.json", fixture.directory) > 0); + const char *original = "{\n // dotfiles\n \"mcpServers\": {\n" + " \"keep\": {\"command\": \"x\"}\n }\n}\n"; + ASSERT_EQ(jl_write(target, original), 0); + ASSERT_EQ(jl_write(control, original), 0); + /* Relative, the way a dotfiles checkout links its files. */ + ASSERT_EQ(symlink("target.json", fixture.path), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.directory), 0); + char target_real[CBM_CONFIG_EDIT_PATH_MAX]; + ASSERT_EQ(cbm_canonical_path(target, target_real, sizeof(target_real)), 1); + jl_follow_hook_state_t hook_state = {0}; + cbm_json_like_set_precommit_hook_for_testing(jl_follow_precommit_hook, &hook_state); + + const char *path[] = {"mcpServers"}; + int through_rc = + cbm_json_like_upsert_entry(fixture.path, path, 1U, "owned", "{\"command\":\"y\"}"); + cbm_json_like_set_precommit_hook_for_testing(NULL, NULL); + int plain_rc = cbm_json_like_upsert_entry(control, path, 1U, "owned", "{\"command\":\"y\"}"); + if (through_rc != 0 || plain_rc != 0) { + cbm_config_edit_path_follow_clear(); + } + ASSERT_EQ(through_rc, 0); + ASSERT_EQ(plain_rc, 0); + ASSERT_EQ(hook_state.calls, 1U); + ASSERT_STR_EQ(hook_state.seen_path, target_real); + ASSERT_EQ(hook_state.temps_beside_target, 1U); + struct stat link_state; + ASSERT_EQ(lstat(fixture.path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + char link_value[256] = {0}; + ASSERT(readlink(fixture.path, link_value, sizeof(link_value) - 1U) > 0); + ASSERT_STR_EQ(link_value, "target.json"); + char *through = jl_read(target); + char *plain = jl_read(control); + ASSERT_NOT_NULL(through); + ASSERT_NOT_NULL(plain); + ASSERT_STR_EQ(through, plain); + ASSERT_NOT_NULL(strstr(through, "\"owned\"")); + ASSERT_NOT_NULL(strstr(through, "// dotfiles")); + + /* Reading through the link sees the target, and the compare-and-remove + * form uninstall relies on works through the link as well. */ + char *document = NULL; + size_t length = 0U; + ASSERT_EQ(cbm_json_like_read_document(fixture.path, &document, &length), 0); + ASSERT_NOT_NULL(document); + ASSERT_STR_EQ(document, through); + ASSERT_EQ( + cbm_json_like_remove_entry_if_unchanged(fixture.path, path, 1U, "owned", document, length), + 0); + ASSERT_EQ(cbm_json_like_remove_entry(control, path, 1U, "owned"), 0); + free(document); + free(through); + free(plain); + through = jl_read(target); + plain = jl_read(control); + ASSERT_NOT_NULL(through); + ASSERT_NOT_NULL(plain); + ASSERT_STR_EQ(through, plain); + ASSERT_NULL(strstr(through, "\"owned\"")); + free(through); + free(plain); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(lstat(fixture.path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + ASSERT_EQ(jl_stray_temp_count(&fixture), 0U); + + ASSERT_EQ(cbm_unlink(fixture.path), 0); + ASSERT_EQ(cbm_unlink(target), 0); + ASSERT_EQ(cbm_unlink(control), 0); + jl_fixture_close(&fixture); + PASS(); +} + TEST(config_json_like_rejects_hard_link_without_splitting_identity) { jl_fixture_t fixture; ASSERT_EQ(jl_fixture_open(&fixture), 0); @@ -853,7 +1229,7 @@ TEST(config_json_like_nested_array_fails_closed_on_ambiguous_or_invalid_paths) { } #ifndef _WIN32 -TEST(config_json_like_nested_array_rejects_symlink_and_hardlink) { +TEST(config_json_like_nested_array_rejects_foreign_symlink_and_hardlink) { jl_fixture_t fixture; ASSERT_EQ(jl_fixture_open(&fixture), 0); const char *path[] = {"options"}; @@ -862,12 +1238,18 @@ TEST(config_json_like_nested_array_rejects_symlink_and_hardlink) { ASSERT(snprintf(target, sizeof(target), "%s/target.json", fixture.directory) > 0); ASSERT_EQ(jl_write(target, original), 0); ASSERT_EQ(symlink(target, fixture.path), 0); - ASSERT_EQ( - cbm_json_like_add_unique_string_at_path(fixture.path, path, 1U, "context_paths", "owned"), - -1); + /* Foreign-owned link (observer moved by the test seam): still refused. */ + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.directory), 0); + cbm_config_edit_path_set_invoking_uid_for_test((unsigned)geteuid() + 1U, 1); + int add_rc = + cbm_json_like_add_unique_string_at_path(fixture.path, path, 1U, "context_paths", "owned"); char *workspace = (char *)(uintptr_t)1U; - ASSERT_EQ(cbm_json_like_get_string_at_path(fixture.path, path, 1U, "context_paths", &workspace), - -1); + int get_rc = + cbm_json_like_get_string_at_path(fixture.path, path, 1U, "context_paths", &workspace); + cbm_config_edit_path_set_invoking_uid_for_test(0U, 0); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(add_rc, -1); + ASSERT_EQ(get_rc, -1); ASSERT_NULL(workspace); char *content = jl_read(target); ASSERT_NOT_NULL(content); @@ -1007,7 +1389,14 @@ SUITE(config_json_like) { RUN_TEST(config_json_like_existing_target_swap_after_check_preserves_winner); RUN_TEST(config_json_like_rejects_non_regular_path); #ifndef _WIN32 - RUN_TEST(config_json_like_rejects_symlink_without_touching_target); + RUN_TEST(config_json_like_rejects_foreign_owned_symlink_without_touching_target); + RUN_TEST(config_json_like_root_refuses_user_owned_symlink); + RUN_TEST(config_json_like_refuses_dangling_symlink); + RUN_TEST(config_json_like_refuses_symlink_to_directory); + RUN_TEST(config_json_like_refuses_symlink_without_opt_in); + RUN_TEST(config_json_like_refuses_symlink_when_target_identity_skews); + RUN_TEST(config_json_like_refuses_symlink_with_world_writable_parent); + RUN_TEST(config_json_like_follows_user_owned_symlink_in_place); RUN_TEST(config_json_like_rejects_hard_link_without_splitting_identity); RUN_TEST(config_json_like_preserves_owner_group_and_mode); #endif @@ -1030,7 +1419,7 @@ SUITE(config_json_like) { RUN_TEST(config_json_like_nested_array_preserves_jsonc_and_is_idempotent); RUN_TEST(config_json_like_nested_array_fails_closed_on_ambiguous_or_invalid_paths); #ifndef _WIN32 - RUN_TEST(config_json_like_nested_array_rejects_symlink_and_hardlink); + RUN_TEST(config_json_like_nested_array_rejects_foreign_symlink_and_hardlink); #endif RUN_TEST(config_json_like_nested_array_rejects_precommit_content_and_identity_races); RUN_TEST(config_json_like_nested_string_lookup_decodes_json5_workspace); diff --git a/tests/test_config_text_edit.c b/tests/test_config_text_edit.c index 47135be84..4ce474853 100644 --- a/tests/test_config_text_edit.c +++ b/tests/test_config_text_edit.c @@ -2,6 +2,8 @@ * test_config_text_edit.c — Hardened managed-text editor contracts. */ #include "../src/cli/config_text_edit.h" +#define CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API 1 +#include "../src/cli/config_edit_path.h" #include "test_framework.h" #include "test_helpers.h" @@ -384,8 +386,16 @@ TEST(config_text_rejects_links_privileged_mode_and_preserves_metadata) { ASSERT(snprintf(alias, sizeof(alias), "%s/alias.md", dir) > 0); ASSERT_EQ(th_write_file(target, "target\n"), 0); ASSERT_EQ(symlink(target, path), 0); - ASSERT_EQ(cbm_text_write_owned_document(path, "owned\n"), -1); - ASSERT_EQ(cbm_text_migrate_owned_document_mode(path, "owned\n", NULL, 0U, 0755U), -1); + /* Foreign-owned link (observer moved by the test seam): still refused. */ + ASSERT_EQ(cbm_config_edit_path_follow_add_root(dir), 0); + cbm_config_edit_path_set_invoking_uid_for_test((unsigned)geteuid() + 1U, 1); + int foreign_write_rc = cbm_text_write_owned_document(path, "owned\n"); + int foreign_migrate_rc = cbm_text_migrate_owned_document_mode(path, "owned\n", NULL, 0U, 0755U); + cbm_config_edit_path_set_invoking_uid_for_test(0U, 0); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(foreign_write_rc, -1); + ASSERT_EQ(foreign_migrate_rc, -1); + ASSERT(cte_assert_bytes(target, "target\n", strlen("target\n"))); ASSERT_EQ(cbm_unlink(path), 0); ASSERT_EQ(th_write_file(path, "shared\n"), 0); @@ -414,6 +424,65 @@ TEST(config_text_rejects_links_privileged_mode_and_preserves_metadata) { PASS(); } +/* Decision C (#1954): a managed block behind a user-owned symlink is edited + * through the link — the link survives and the target carries the same bytes + * the same edit produces on a plain file. */ +TEST(config_text_follows_user_owned_symlink_in_place) { + char dir[CTE_PATH_CAP]; + char path[CTE_PATH_CAP]; + char target[CTE_PATH_CAP]; + char control[CTE_PATH_CAP]; + ASSERT_EQ(cte_fixture(dir, sizeof(dir), path, sizeof(path)), 0); + ASSERT(snprintf(target, sizeof(target), "%s/target.md", dir) > 0); + ASSERT(snprintf(control, sizeof(control), "%s/control.md", dir) > 0); + ASSERT_EQ(th_write_file(target, "# user notes\n"), 0); + ASSERT_EQ(th_write_file(control, "# user notes\n"), 0); + ASSERT_EQ(symlink("target.md", path), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(dir), 0); + + int through_rc = cbm_text_upsert_managed_block(path, CTE_BEGIN, CTE_END, "owned"); + int plain_rc = cbm_text_upsert_managed_block(control, CTE_BEGIN, CTE_END, "owned"); + if (through_rc != 0 || plain_rc != 0) { + cbm_config_edit_path_follow_clear(); + } + ASSERT_EQ(through_rc, 0); + ASSERT_EQ(plain_rc, 0); + struct stat link_state; + ASSERT_EQ(lstat(path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + size_t through_len = 0U; + size_t plain_len = 0U; + char *through = cte_read_bytes(target, &through_len); + char *plain = cte_read_bytes(control, &plain_len); + ASSERT_NOT_NULL(through); + ASSERT_NOT_NULL(plain); + ASSERT_EQ(through_len, plain_len); + ASSERT_EQ(memcmp(through, plain, plain_len), 0); + ASSERT_NOT_NULL(strstr(through, "owned")); + free(through); + free(plain); + + int through_remove_rc = cbm_text_remove_managed_block(path, CTE_BEGIN, CTE_END); + int plain_remove_rc = cbm_text_remove_managed_block(control, CTE_BEGIN, CTE_END); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(through_remove_rc, 0); + ASSERT_EQ(plain_remove_rc, 0); + ASSERT_EQ(lstat(path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + through = cte_read_bytes(target, &through_len); + plain = cte_read_bytes(control, &plain_len); + ASSERT_NOT_NULL(through); + ASSERT_NOT_NULL(plain); + ASSERT_EQ(through_len, plain_len); + ASSERT_EQ(memcmp(through, plain, plain_len), 0); + ASSERT_NULL(strstr(through, "owned")); + free(through); + free(plain); + ASSERT_EQ(cte_temp_count(dir), 0U); + th_cleanup(dir); + PASS(); +} + TEST(config_text_owned_document_mode_publishes_exact_bytes_atomically) { char dir[CTE_PATH_CAP]; char path[CTE_PATH_CAP]; @@ -670,6 +739,7 @@ SUITE(config_text_edit) { RUN_TEST(config_text_rejects_non_regular_paths); #ifndef _WIN32 RUN_TEST(config_text_rejects_links_privileged_mode_and_preserves_metadata); + RUN_TEST(config_text_follows_user_owned_symlink_in_place); RUN_TEST(config_text_owned_document_mode_publishes_exact_bytes_atomically); RUN_TEST(config_text_owned_document_mode_rejects_prepublish_replacement); RUN_TEST(config_text_owned_document_mode_rejects_closed_temp_replacement); diff --git a/tests/test_config_toml_edit.c b/tests/test_config_toml_edit.c index 1266556e0..cc64fd576 100644 --- a/tests/test_config_toml_edit.c +++ b/tests/test_config_toml_edit.c @@ -5,6 +5,8 @@ */ #define CBM_TOML_EDIT_ENABLE_TEST_API 1 #include "cli/config_toml_edit.h" +#define CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API 1 +#include "cli/config_edit_path.h" #include "foundation/compat.h" #include "foundation/compat_fs.h" #include "test_framework.h" @@ -312,7 +314,13 @@ TEST(config_toml_rejects_symlink_hardlink_and_preserves_metadata) { ASSERT(snprintf(alias, sizeof(alias), "%s/alias.toml", dir) > 0); ASSERT_EQ(th_write_file(target, "target = true\n"), 0); ASSERT_EQ(symlink(target, path), 0); - ASSERT_EQ(cbm_toml_upsert_managed_block(path, CTE_BEGIN, CTE_END, "owned = true\n"), -1); + /* Foreign-owned link (observer moved by the test seam): still refused. */ + ASSERT_EQ(cbm_config_edit_path_follow_add_root(dir), 0); + cbm_config_edit_path_set_invoking_uid_for_test((unsigned)geteuid() + 1U, 1); + int foreign_rc = cbm_toml_upsert_managed_block(path, CTE_BEGIN, CTE_END, "owned = true\n"); + cbm_config_edit_path_set_invoking_uid_for_test(0U, 0); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(foreign_rc, -1); struct stat link_state; ASSERT_EQ(lstat(path, &link_state), 0); ASSERT(S_ISLNK(link_state.st_mode)); @@ -351,6 +359,59 @@ TEST(config_toml_rejects_symlink_hardlink_and_preserves_metadata) { } #endif +#ifndef _WIN32 +/* Decision C (#1954): a managed block behind a user-owned symlink is edited + * through the link — the link survives and the target carries the same bytes + * the same edit produces on a plain file. */ +TEST(config_toml_follows_user_owned_symlink_in_place) { + char dir[CTE_PATH_CAP]; + char path[CTE_PATH_CAP]; + char target[CTE_PATH_CAP]; + char control[CTE_PATH_CAP]; + char through[CTE_FILE_CAP]; + char plain[CTE_FILE_CAP]; + ASSERT_EQ(cte_fixture(dir, sizeof(dir), path, sizeof(path)), 0); + ASSERT(snprintf(target, sizeof(target), "%s/target.toml", dir) > 0); + ASSERT(snprintf(control, sizeof(control), "%s/control.toml", dir) > 0); + ASSERT_EQ(th_write_file(target, "target = true\n"), 0); + ASSERT_EQ(th_write_file(control, "target = true\n"), 0); + ASSERT_EQ(symlink("target.toml", path), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(dir), 0); + + int through_rc = cbm_toml_upsert_managed_block(path, CTE_BEGIN, CTE_END, "owned = true\n"); + int plain_rc = cbm_toml_upsert_managed_block(control, CTE_BEGIN, CTE_END, "owned = true\n"); + if (through_rc != 0 || plain_rc != 0) { + cbm_config_edit_path_follow_clear(); + } + ASSERT_EQ(through_rc, 0); + ASSERT_EQ(plain_rc, 0); + struct stat link_state; + ASSERT_EQ(lstat(path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + ASSERT_EQ(cte_read(target, through, sizeof(through)), 0); + ASSERT_EQ(cte_read(control, plain, sizeof(plain)), 0); + ASSERT_STR_EQ(through, plain); + ASSERT_NOT_NULL(strstr(through, "owned = true")); + + int through_remove_rc = cbm_toml_remove_managed_block(path, CTE_BEGIN, CTE_END); + int plain_remove_rc = cbm_toml_remove_managed_block(control, CTE_BEGIN, CTE_END); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(through_remove_rc, 0); + ASSERT_EQ(plain_remove_rc, 0); + ASSERT_EQ(lstat(path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + ASSERT_EQ(cte_read(target, through, sizeof(through)), 0); + ASSERT_EQ(cte_read(control, plain, sizeof(plain)), 0); + ASSERT_STR_EQ(through, plain); + ASSERT_NULL(strstr(through, "owned = true")); + ASSERT_EQ(cte_temp_count(dir), 0U); + ASSERT_EQ(cbm_unlink(target), 0); + ASSERT_EQ(cbm_unlink(control), 0); + th_cleanup(dir); + PASS(); +} +#endif + TEST(config_toml_managed_markers_ignore_multiline_strings) { char dir[CTE_PATH_CAP]; char path[CTE_PATH_CAP]; @@ -1411,6 +1472,7 @@ SUITE(config_toml_edit) { RUN_TEST(config_toml_rejects_non_regular_path); #ifndef _WIN32 RUN_TEST(config_toml_rejects_symlink_hardlink_and_preserves_metadata); + RUN_TEST(config_toml_follows_user_owned_symlink_in_place); #endif RUN_TEST(config_toml_managed_markers_ignore_multiline_strings); RUN_TEST(config_toml_managed_rejects_marker_in_block_and_unclosed_multiline); diff --git a/tests/test_config_yaml_edit.c b/tests/test_config_yaml_edit.c index 3e290240c..421c8b79c 100644 --- a/tests/test_config_yaml_edit.c +++ b/tests/test_config_yaml_edit.c @@ -8,6 +8,8 @@ #define CBM_YAML_ENABLE_TEST_API 1 #include +#define CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API 1 +#include #include #include @@ -476,7 +478,13 @@ TEST(config_yaml_edit_rejects_symlinks_without_touching_target) { ASSERT_EQ(th_write_file(target, original), 0); ASSERT_EQ(symlink(target, fixture.path), 0); - ASSERT_EQ(cbm_yaml_upsert_string_list_item(fixture.path, "read", "AGENTS.md"), -1); + /* Foreign-owned link (observer moved by the test seam): still refused. */ + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.dir), 0); + cbm_config_edit_path_set_invoking_uid_for_test((unsigned)geteuid() + 1U, 1); + int foreign_rc = cbm_yaml_upsert_string_list_item(fixture.path, "read", "AGENTS.md"); + cbm_config_edit_path_set_invoking_uid_for_test(0U, 0); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(foreign_rc, -1); struct stat link_state; ASSERT_EQ(lstat(fixture.path, &link_state), 0); ASSERT(S_ISLNK(link_state.st_mode)); @@ -492,6 +500,64 @@ TEST(config_yaml_edit_rejects_symlinks_without_touching_target) { PASS(); } +/* Decision C (#1954): a user-owned symlink is edited through — the link + * survives and the target carries the bytes the same edit yields on a plain file. */ +TEST(config_yaml_edit_follows_user_owned_symlink_in_place) { + const char *original = "model: safe\n"; + yaml_fixture_t fixture; + ASSERT_EQ(yaml_fixture_init(&fixture, NULL), 0); + char target[sizeof(fixture.path) + 32U]; + char control[sizeof(fixture.path) + 32U]; + ASSERT(snprintf(target, sizeof(target), "%s/target.yaml", fixture.dir) > 0); + ASSERT(snprintf(control, sizeof(control), "%s/control.yaml", fixture.dir) > 0); + ASSERT_EQ(th_write_file(target, original), 0); + ASSERT_EQ(th_write_file(control, original), 0); + ASSERT_EQ(symlink("target.yaml", fixture.path), 0); + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.dir), 0); + + int through_rc = cbm_yaml_upsert_string_list_item(fixture.path, "read", "AGENTS.md"); + int plain_rc = cbm_yaml_upsert_string_list_item(control, "read", "AGENTS.md"); + if (through_rc != 0 || plain_rc != 0) { + cbm_config_edit_path_follow_clear(); + } + ASSERT_EQ(through_rc, 0); + ASSERT_EQ(plain_rc, 0); + struct stat link_state; + ASSERT_EQ(lstat(fixture.path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + char *through = yaml_read_alloc(target); + char *plain = yaml_read_alloc(control); + ASSERT_NOT_NULL(through); + ASSERT_NOT_NULL(plain); + ASSERT_STR_EQ(through, plain); + ASSERT_NOT_NULL(strstr(through, "AGENTS.md")); + free(through); + free(plain); + + int through_remove_rc = cbm_yaml_remove_string_list_item(fixture.path, "read", "AGENTS.md"); + int plain_remove_rc = cbm_yaml_remove_string_list_item(control, "read", "AGENTS.md"); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(through_remove_rc, 0); + ASSERT_EQ(plain_remove_rc, 0); + ASSERT_EQ(lstat(fixture.path, &link_state), 0); + ASSERT(S_ISLNK(link_state.st_mode)); + through = yaml_read_alloc(target); + plain = yaml_read_alloc(control); + ASSERT_NOT_NULL(through); + ASSERT_NOT_NULL(plain); + ASSERT_STR_EQ(through, plain); + ASSERT_NULL(strstr(through, "AGENTS.md")); + free(through); + free(plain); + ASSERT_EQ(yaml_temp_file_count(&fixture), 0U); + + ASSERT_EQ(cbm_unlink(fixture.path), 0); + ASSERT_EQ(cbm_unlink(target), 0); + ASSERT_EQ(cbm_unlink(control), 0); + th_cleanup(fixture.dir); + PASS(); +} + TEST(config_yaml_edit_rejects_dangling_symlink) { yaml_fixture_t fixture; ASSERT_EQ(yaml_fixture_init(&fixture, NULL), 0); @@ -1935,9 +2001,15 @@ TEST(config_yaml_edit_nested_sequence_rejects_symlink_byte_identically) { ASSERT(snprintf(target, sizeof(target), "%s/target-hooks.yaml", fixture.dir) > 0); ASSERT_EQ(th_write_file(target, original), 0); ASSERT_EQ(symlink(target, fixture.path), 0); - ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", - yaml_hook_identity, yaml_hook_canonical_item), - CBM_YAML_IDENTITY_EDIT_ERROR); + /* Foreign-owned link (observer moved by the test seam): still refused. */ + ASSERT_EQ(cbm_config_edit_path_follow_add_root(fixture.dir), 0); + cbm_config_edit_path_set_invoking_uid_for_test((unsigned)geteuid() + 1U, 1); + int foreign_rc = + cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item); + cbm_config_edit_path_set_invoking_uid_for_test(0U, 0); + cbm_config_edit_path_follow_clear(); + ASSERT_EQ(foreign_rc, CBM_YAML_IDENTITY_EDIT_ERROR); char *after = yaml_read_alloc(target); ASSERT_NOT_NULL(after); ASSERT_STR_EQ(after, original); @@ -1968,6 +2040,7 @@ SUITE(config_yaml_edit) { RUN_TEST(config_yaml_edit_rejects_non_regular_path); #ifndef _WIN32 RUN_TEST(config_yaml_edit_rejects_symlinks_without_touching_target); + RUN_TEST(config_yaml_edit_follows_user_owned_symlink_in_place); RUN_TEST(config_yaml_edit_rejects_dangling_symlink); RUN_TEST(config_yaml_edit_preserves_owner_group_and_mode); RUN_TEST(config_yaml_edit_rejects_hard_links_without_splitting_identity); From 7bf366f63b3fd5211cec19e483ee2ed27145ca14 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 8 Sep 2026 20:30:17 +0200 Subject: [PATCH 3/5] fix(cli): guard the POSIX-only edit-path note recorder from the Windows build config_edit_path.c's edit_path_note is defined before the '#ifndef _WIN32' follow-resolver block but only called inside it, so the Windows build compiled it dead and failed under -Werror,-Wunused-function (pr-smoke windows-latest). Windows never follows a symlinked config, so guard the recorder with the same condition as its callers. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- src/cli/config_edit_path.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/cli/config_edit_path.c b/src/cli/config_edit_path.c index b51c5c2a2..ec26a5534 100644 --- a/src/cli/config_edit_path.c +++ b/src/cli/config_edit_path.c @@ -35,11 +35,16 @@ static void edit_path_target_reset(cbm_config_edit_target_t *target) { target->path[0] = '\0'; } +#ifndef _WIN32 +/* Only the POSIX symlink-follow resolver records a refusal reason; the Windows + * build never follows, so guard this with the same condition as its callers to + * avoid -Werror,-Wunused-function (#2110 CI). */ static void edit_path_note(char *reason, size_t reason_size, const char *text) { if (reason && reason_size > 0U) { (void)snprintf(reason, reason_size, "%s", text); } } +#endif /* !_WIN32 */ void cbm_config_edit_target_close(cbm_config_edit_target_t *target) { if (!target) { From a3c24a71b4c28e805b912f9f928b6b0b151f9495 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 8 Sep 2026 23:00:59 +0200 Subject: [PATCH 4/5] test(cli): guard POSIX-only fd-capture helpers under _WIN32 (#2110) cli_fd_capture_begin/end use POSIX fd redirection (dup/dup2/tmpfile) and are called only by the uninstall tests, which are themselves #ifndef _WIN32. Defined at top level, the helpers were compiled but unused on Windows, tripping -Werror,-Wunused-function on the test-windows CLANG64 leg. Wrap the struct and both helpers in #ifndef _WIN32 to match their callers. Test-only; no production change. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- tests/test_cli.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_cli.c b/tests/test_cli.c index 3316e7acd..38e2cb35f 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -722,6 +722,7 @@ static void test_rmdir_r(const char *path) { th_rmtree(path); } +#ifndef _WIN32 /* Capture everything a command writes to one fd (stdout or stderr) so a test * can assert on the transcript. Restores the fd on end and returns the text. */ typedef struct { @@ -774,6 +775,7 @@ static char *cli_fd_capture_end(cli_fd_capture_t *capture) { } return text; } +#endif /* !_WIN32 -- POSIX-only fd-capture helpers (#2110) */ /* Mandatory-daemon activation guard fixture. The production path uses the * stable per-account endpoint directly; these callbacks make every race From 91c63be5d4e41d44e0746ef533af4a0b1967d6b3 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 9 Sep 2026 21:03:01 +0200 Subject: [PATCH 5/5] fix(cli): keep config follow-roots off the static TLS block (#2110) #1954's config-symlink follow support stored its opted-in roots in a _Thread_local char[4][4096] -- 16 KiB in the binary's static TLS block. On glibc/x86-64 that block is carved out of every thread's stack, including the parent-death watchdog thread, created with a deliberately tiny 64 KiB stack (PARENT_WATCHDOG_STACK_SIZE, main.c). The extra 16 KiB pushed that thread's pthread_create past the static-TLS + TCB + guard budget, so it failed with EINVAL: client_start_parent_watchdog() returned false and the stdio server exited before answering initialize. The parent-death watchdog regression test (#406/#407, scripts/test.sh Step 5) went red on every Linux x86-64 leg while macOS/arm64 stayed green -- its TLS is not stack-carved. Move the buffer off the static TLS block: keep a _Thread_local POINTER (8 bytes) and calloc the 16 KiB lazily on first add, freeing it in follow_clear(). Per-thread isolation and the exact API/semantics are unchanged; the watchdog thread's static-TLS budget is restored. main.c and the watchdog path are untouched. Evidence (local, Linux x86-64 gcc under QEMU + Linux arm64): - PT_TLS MemSiz 59,889 B -> 43,505 B (-16,384 B, exactly the array). - parent-watchdog test 10/10 red -> 10/10 green; red again on revert. - origin/main 10/10 green on the same env (regression is PR-specific). - config editor + cli suites: 522/522 ASan/UBSan (macOS), 523/523 ASan/LeakSanitizer (Linux arm64), no leak. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- src/cli/config_edit_path.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/cli/config_edit_path.c b/src/cli/config_edit_path.c index ec26a5534..2217f3855 100644 --- a/src/cli/config_edit_path.c +++ b/src/cli/config_edit_path.c @@ -11,6 +11,7 @@ #include #include +#include #include #ifndef _WIN32 @@ -66,7 +67,16 @@ void cbm_config_edit_target_close(cbm_config_edit_target_t *target) { #ifndef _WIN32 -static CBM_TLS char edit_path_roots[EDIT_PATH_MAX_ROOTS][CBM_CONFIG_EDIT_PATH_MAX]; +/* The opted-in follow-roots are per-thread (test isolation), but the storage + * is a heap buffer behind a thread-local POINTER, not a thread-local array. + * A `_Thread_local char[4][4096]` sits in the binary's static TLS block, and + * glibc carves that block out of every thread's stack allocation — including + * the parent-death watchdog's deliberately tiny 64 KiB stack (main.c). The + * 16 KiB array pushed the watchdog's pthread_create over the static-TLS + + * guard budget, so it failed with EINVAL on glibc/x86-64 (green on macOS, + * whose TLS is not stack-carved): #2110 CI. The pointer keeps the static TLS + * block at 8 bytes; the buffer is allocated on first add and freed by clear. */ +static CBM_TLS char (*edit_path_roots)[CBM_CONFIG_EDIT_PATH_MAX] = NULL; static CBM_TLS size_t edit_path_root_count = 0U; #ifdef CBM_CONFIG_EDIT_PATH_ENABLE_TEST_API @@ -101,6 +111,12 @@ int cbm_config_edit_path_follow_add_root(const char *root) { if (!cbm_canonical_path(root, canonical, sizeof(canonical))) { return -1; } + if (!edit_path_roots) { + edit_path_roots = calloc(EDIT_PATH_MAX_ROOTS, sizeof(*edit_path_roots)); + if (!edit_path_roots) { + return -1; + } + } if (edit_path_copy(canonical, edit_path_roots[edit_path_root_count], sizeof(edit_path_roots[0])) != 0) { return -1; @@ -111,6 +127,8 @@ int cbm_config_edit_path_follow_add_root(const char *root) { void cbm_config_edit_path_follow_clear(void) { edit_path_root_count = 0U; + free(edit_path_roots); + edit_path_roots = NULL; } static int edit_path_under_root(const char *canonical) {