From 2160a1588258ffffd4c2738ef00d355f6b9818f7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 15 Aug 2026 11:13:40 +0000 Subject: [PATCH] fix(file): do not collapse missing path components onto ancestor files write_file treated an existing ancestor as the fopen target when the requested nested path did not exist, truncating a file used as a directory or overwriting a same-named file in a parent directory. Co-authored-by: esadrianno --- src/tools/file.c | 100 ++++++++++++++++++++++++++++++++-------------- tests/test_file.c | 93 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 31 deletions(-) diff --git a/src/tools/file.c b/src/tools/file.c index e8170b1..183bc4d 100644 --- a/src/tools/file.c +++ b/src/tools/file.c @@ -31,6 +31,22 @@ void tool_file_set_config(const config_t *cfg) g_file_cfg = cfg; } +static int resolved_is_under_workspace(const char *resolved) +{ + char ws_resolved[PATH_MAX]; + const char *workspace; + size_t ws_len; + + if (!resolved || !g_file_cfg) return 0; + workspace = config_workspace_path(g_file_cfg); + if (!workspace || workspace[0] == '\0') return 0; + if (realpath(workspace, ws_resolved) == NULL) return 0; + ws_len = strlen(ws_resolved); + if (strncmp(resolved, ws_resolved, ws_len) != 0) return 0; + if (resolved[ws_len] != '\0' && resolved[ws_len] != '/') return 0; + return 1; +} + static int path_within_workspace(const char *path, char *resolved, size_t resolved_size) { if (!path || path[0] == '\0') return 0; @@ -42,38 +58,68 @@ static int path_within_workspace(const char *path, char *resolved, size_t resolv if (!workspace || workspace[0] == '\0') { return 0; /* Deny: cannot validate without workspace path */ } - char ws_resolved[PATH_MAX]; - if (realpath(workspace, ws_resolved) == NULL) return 0; - if (realpath(path, resolved) != NULL) { - size_t ws_len = strlen(ws_resolved); - if (strncmp(resolved, ws_resolved, ws_len) != 0) return 0; - if (resolved[ws_len] != '\0' && resolved[ws_len] != '/') return 0; - return 1; - } + if (realpath(workspace, resolved) == NULL) return 0; + if (realpath(path, resolved) != NULL) + return resolved_is_under_workspace(resolved); char path_copy[PATH_MAX]; snprintf(path_copy, sizeof(path_copy), "%s", path); for (;;) { char *dir = dirname(path_copy); if (!dir || dir[0] == '\0') break; - if (realpath(dir, resolved) != NULL) { - size_t ws_len = strlen(ws_resolved); - if (strncmp(resolved, ws_resolved, ws_len) != 0) return 0; - if (resolved[ws_len] != '\0' && resolved[ws_len] != '/') return 0; - return 1; - } + if (realpath(dir, resolved) != NULL) + return resolved_is_under_workspace(resolved); if (strcmp(dir, ".") == 0 || strcmp(dir, "/") == 0) break; snprintf(path_copy, sizeof(path_copy), "%s", dir); } return 0; } +/* + * Map a workspace write to the intended path. Ancestor lookup in + * path_within_workspace is only a membership check: using that ancestor as + * the fopen target truncates an existing file treated as a directory, or + * overwrites a same-named file in a parent when intermediate dirs are missing. + */ +static int resolve_workspace_write_path(const char *path, char *safe_path, size_t cap) +{ + char parent[PATH_MAX]; + char path_copy[PATH_MAX]; + struct stat st; + const char *base; + char *dir; + int n; + + if (realpath(path, safe_path) != NULL) { + if (stat(safe_path, &st) != 0 || !S_ISREG(st.st_mode)) return 0; + return resolved_is_under_workspace(safe_path); + } + if (snprintf(path_copy, sizeof(path_copy), "%s", path) >= (int)sizeof(path_copy)) + return 0; + dir = dirname(path_copy); + if (!dir || realpath(dir, parent) == NULL) return 0; + if (stat(parent, &st) != 0 || !S_ISDIR(st.st_mode)) return 0; + if (!resolved_is_under_workspace(parent)) return 0; + base = strrchr(path, '/'); + base = base ? base + 1 : path; + if (base[0] == '\0' || strcmp(base, ".") == 0 || strcmp(base, "..") == 0) + return 0; + n = snprintf(safe_path, cap, "%s/%s", parent, base); + return n > 0 && (size_t)n < cap; +} + static int file_read(const char *path, char *result_buf, size_t max_len) { char resolved[PATH_MAX]; + int ws_only; if (!path_within_workspace(path, resolved, sizeof(resolved))) { snprintf(result_buf, max_len, "{\"error\":\"path outside workspace\"}"); return -1; } + ws_only = g_file_cfg ? config_workspace_only(g_file_cfg) : 0; + if (ws_only && realpath(path, resolved) == NULL) { + snprintf(result_buf, max_len, "{\"error\":\"cannot open file\"}"); + return -1; + } FILE *f = fopen(resolved, "rb"); if (!f) { snprintf(result_buf, max_len, "{\"error\":\"cannot open file\"}"); @@ -106,23 +152,9 @@ static int file_write(const char *path, const char *content, char *result_buf, s char safe_path[PATH_MAX]; if (!ws_only) { snprintf(safe_path, sizeof(safe_path), "%s", path); - } else { - struct stat st; - if (stat(resolved, &st) == 0 && S_ISREG(st.st_mode)) { - snprintf(safe_path, sizeof(safe_path), "%s", resolved); - } else { - const char *base = strrchr(path, '/'); - base = base ? base + 1 : path; - size_t res_len = strlen(resolved); - size_t base_len = strlen(base); - if (res_len + 1 + base_len >= sizeof(safe_path)) { - snprintf(result_buf, max_len, "{\"error\":\"path too long\"}"); - return -1; - } - memcpy(safe_path, resolved, res_len); - safe_path[res_len] = '/'; - memcpy(safe_path + res_len + 1, base, base_len + 1); - } + } else if (!resolve_workspace_write_path(path, safe_path, sizeof(safe_path))) { + snprintf(result_buf, max_len, "{\"error\":\"cannot write file\"}"); + return -1; } FILE *f = fopen(safe_path, "w"); if (!f) { @@ -145,10 +177,16 @@ static int file_write(const char *path, const char *content, char *result_buf, s static int file_list(const char *path, char *result_buf, size_t max_len) { char resolved[PATH_MAX]; + int ws_only; if (!path_within_workspace(path, resolved, sizeof(resolved))) { snprintf(result_buf, max_len, "{\"error\":\"path outside workspace\"}"); return -1; } + ws_only = g_file_cfg ? config_workspace_only(g_file_cfg) : 0; + if (ws_only && realpath(path, resolved) == NULL) { + snprintf(result_buf, max_len, "{\"error\":\"cannot list directory\"}"); + return -1; + } DIR *d = opendir(resolved); if (!d) { snprintf(result_buf, max_len, "{\"error\":\"cannot list directory\"}"); diff --git a/tests/test_file.c b/tests/test_file.c index b4087ac..099452a 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -196,6 +196,97 @@ static void test_symlink_escape_rejected(void) rmdir(tmpdir); } +static int slurp_file(const char *path, char *buf, size_t cap) +{ + FILE *f = fopen(path, "rb"); + size_t n; + if (!f) return -1; + n = fread(buf, 1, cap - 1, f); + fclose(f); + buf[n] = '\0'; + return 0; +} + +static void test_write_does_not_truncate_file_used_as_directory(void) +{ + char tmpdir[PATH_MAX]; + char victim[PATH_MAX]; + char nested[PATH_MAX]; + char config_path[PATH_MAX]; + char args[PATH_MAX + 128]; + char buf[256]; + char kept[64]; + config_t *cfg; + const tool_t *t; + FILE *f; + int r; + + snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_filedir_%d", (int)getpid()); + if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; + snprintf(victim, sizeof(victim), "%s/important.md", tmpdir); + f = fopen(victim, "w"); + MU_ASSERT(f != NULL, "create victim file"); + fputs("KEEP", f); + fclose(f); + cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); + MU_ASSERT(cfg != NULL, "file-as-dir: load config"); + tool_file_set_config(cfg); + t = tool_file_get(); + snprintf(nested, sizeof(nested), "%s/important.md/nested.txt", tmpdir); + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"PWNED\"}", nested); + r = t->execute(args, buf, sizeof(buf)); + MU_ASSERT(r == -1, "write through file-as-directory is rejected"); + MU_ASSERT(slurp_file(victim, kept, sizeof(kept)) == 0, "victim still readable"); + MU_ASSERT(strcmp(kept, "KEEP") == 0, "victim content preserved"); + snprintf(args, sizeof(args), "{\"operation\":\"read_file\",\"path\":\"%s\"}", nested); + r = t->execute(args, buf, sizeof(buf)); + MU_ASSERT(r == -1, "read through file-as-directory is rejected"); + MU_ASSERT(strstr(buf, "KEEP") == NULL, "read does not leak victim contents"); + config_free(cfg); + unlink(config_path); + unlink(victim); + rmdir(tmpdir); +} + +static void test_write_does_not_collapse_missing_parent_onto_basename(void) +{ + char tmpdir[PATH_MAX]; + char victim[PATH_MAX]; + char nested[PATH_MAX]; + char config_path[PATH_MAX]; + char args[PATH_MAX + 128]; + char buf[256]; + char kept[64]; + config_t *cfg; + const tool_t *t; + FILE *f; + int r; + + snprintf(tmpdir, sizeof(tmpdir), "/tmp/sc_test_missdir_%d", (int)getpid()); + if (mkdir(tmpdir, 0755) != 0 && errno != EEXIST) return; + snprintf(victim, sizeof(victim), "%s/notes.md", tmpdir); + f = fopen(victim, "w"); + MU_ASSERT(f != NULL, "create same-basename victim"); + fputs("KEEP", f); + fclose(f); + cfg = make_ws_config(tmpdir, config_path, sizeof(config_path)); + MU_ASSERT(cfg != NULL, "missing-parent: load config"); + tool_file_set_config(cfg); + t = tool_file_get(); + snprintf(nested, sizeof(nested), "%s/missing_dir/notes.md", tmpdir); + snprintf(args, sizeof(args), + "{\"operation\":\"write_file\",\"path\":\"%s\",\"content\":\"PWNED\"}", nested); + r = t->execute(args, buf, sizeof(buf)); + MU_ASSERT(r == -1, "write with missing parent is rejected"); + MU_ASSERT(slurp_file(victim, kept, sizeof(kept)) == 0, "workspace notes.md still readable"); + MU_ASSERT(strcmp(kept, "KEEP") == 0, "missing parent does not overwrite same basename"); + config_free(cfg); + unlink(config_path); + unlink(victim); + rmdir(tmpdir); +} + int main(void) { MU_RUN(test_file_read_write_list); @@ -203,6 +294,8 @@ int main(void) MU_RUN(test_file_outside_workspace_rejected); MU_RUN(test_path_traversal_rejected); MU_RUN(test_symlink_escape_rejected); + MU_RUN(test_write_does_not_truncate_file_used_as_directory); + MU_RUN(test_write_does_not_collapse_missing_parent_onto_basename); printf("%d tests run, %d failed\n", tests_run, tests_failed); return tests_failed ? 1 : 0; }