diff --git a/Makefile.cbm b/Makefile.cbm index 35844a529..2c2aeb193 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 7bc1474a0..49d433358 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" @@ -7977,6 +7978,90 @@ 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 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 + * 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; @@ -8032,6 +8117,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); } @@ -8049,6 +8142,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, @@ -10083,12 +10179,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); } @@ -12398,6 +12506,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); } @@ -12405,14 +12514,14 @@ 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(); - 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. */ #ifdef _WIN32 /* #2117: install registers the install directory in the persistent @@ -12550,6 +12659,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 @@ -12619,6 +12729,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; } @@ -12627,7 +12750,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/src/cli/config_edit_path.c b/src/cli/config_edit_path.c new file mode 100644 index 000000000..2217f3855 --- /dev/null +++ b/src/cli/config_edit_path.c @@ -0,0 +1,457 @@ +/* + * 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 +#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'; +} + +#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) { + 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 + +/* 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 +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_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; + } + edit_path_root_count++; + return 0; +} + +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) { + 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 19aa86ac3..afd786161 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" @@ -1440,11 +1441,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; @@ -1609,10 +1623,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); @@ -1647,13 +1682,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; } } @@ -1696,7 +1733,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; } @@ -1708,7 +1745,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; } @@ -1719,7 +1756,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; } @@ -1730,8 +1767,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; } @@ -1739,6 +1777,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 788752d3a..38b3b4e33 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 7c58ed7d5..825209768 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -723,6 +723,61 @@ 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 { + 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; +} +#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 * ordering deterministic without exposing a CLI flag or environment bypass. */ @@ -2278,6 +2333,181 @@ 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(); +} + +/* #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) { char tmpdir[256]; snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-daemon-stateless-XXXXXX"); @@ -4881,7 +5111,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"); @@ -4905,9 +5135,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(); } @@ -10917,7 +11150,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"); @@ -10972,7 +11205,9 @@ TEST(cli_codex_migrates_to_single_hook_representation) { struct stat state; hooks = read_test_file_alloc(hooks_path); char *agents_after_uninstall = read_test_file_alloc(agents_path); - bool independent_cleanup = uninstall_rc != 0 && stat(binary_path, &state) == 0 && + /* #1954: the ambiguous/foreign hook still fails the exit code, but the binary + * is removed anyway (uninstall is no longer held hostage by agent-config). */ + 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") && agents_after_uninstall && agents_after_uninstall[0] == '\0'; @@ -15196,6 +15431,10 @@ 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); + RUN_TEST(cli_uninstall_cleans_user_owned_symlinked_config); +#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); diff --git a/tests/test_config_json_like.c b/tests/test_config_json_like.c index 8e6a2b28c..43f325e10 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); @@ -1023,7 +1399,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"}; @@ -1032,12 +1408,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); @@ -1177,7 +1559,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 @@ -1203,7 +1592,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 27530757a..f6f80d47e 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); @@ -2038,9 +2104,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); @@ -2071,6 +2143,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);