Skip to content

Commit fd9fbe9

Browse files
ngallodevclaude
ngallodev
and
claude
committed
fix: address CodeRabbit findings on release tooling (PR #12)
- metrics_out validation (security): the path came straight from the tool args and was written with no workspace check. The front-end index operation now resolves it like repo_path, canonicalizes its parent directory (which must exist), runs cbm_workspace_root_allowed on it with the same allowed_root, rejects empty/"."/".." basenames, and rewrites the worker argument to canonical-parent + basename so the worker writes exactly the validated path. index_args_with_repo_path is now a thin wrapper over a generic index_args_with_string. - Stage file mode (security): on POSIX the main stage file is created with open(O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0600) + fdopen, so it is owner-only regardless of umask. Windows keeps cbm_fopen "wbx". - Stage create retry: retry with a new suffix only when the lock hold fails with EWOULDBLOCK/EAGAIN/EACCES or the main-file create fails with EEXIST; any other errno stops immediately. errno is saved before cbm_pipeline_stage_lock_drop (close/unlink can clobber it) and the saved value is used in the lock_failed warning. Helpers keep create_staging_path under the cognitive-complexity threshold. - .clang-tidy: add asm-generic/.* to misc-include-cleaner.IgnoreHeaders, next to the existing sys/ and _*.h entries, so errno macros used via <errno.h> are not attributed to Linux-internal headers. - index.c: free metrics_out on the invalid-project-name path; a metrics write failure on a successful run no longer changes rc, it logs index.metrics_write_failed and adds a metrics_error field to the response; early cancelled / pipeline-creation-failed exits now still write a metrics artifact (index_write_metrics already accepts a NULL pipeline and emits zero/default pipeline-derived fields with outcome cancelled/failed). Response building was split into helpers to stay under the complexity limit; an allocation failure there now reports "result encoding failed". - main.c: daemon status --json is built with yyjson so semantic_version and build_fingerprint are escaped; keys and value types are unchanged. - Jenkinsfile: rm -f .jenkins-installed-cli moves to the first stage so a failed early stage cannot make post restart a stale CLI; removed the nonexistent daemon_frontend suite from the daemon shard list. - cli.c: the legacy-hook finding is answered with a comment fix only. The CLI-owned hook names have used .cmd on Windows since they were introduced (4727371), so no extensionless CLI-owned file was ever installed and the upgrade cleanup was intentionally removed; the comment no longer claims it. - Tests (index_resilience): metrics_out outside CBM_ALLOWED_ROOT is refused and nothing is written; a successful index with metrics_out writes parseable JSON with outcome success. Validation: scripts/build-dev.sh; make lint-ci lint-tidy-diff; scripts/ security-audit.sh; scripts/test.sh --suites pipeline,index_supervisor, index_resilience,daemon_ipc (336 passed); git diff --check. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent 19bc642 commit fd9fbe9

7 files changed

Lines changed: 318 additions & 66 deletions

File tree

‎.clang-tidy‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ CheckOptions:
5252
readability-magic-numbers.IgnoredIntegerValues: ""
5353
readability-magic-numbers.IgnoredFloatingPointValues: ""
5454
bugprone-easily-swappable-parameters.MinimumLength: 3
55-
misc-include-cleaner.IgnoreHeaders: "sys/.*|mach/.*|dispatch/.*|_types/.*|Availability\\.h|secure/.*|_.*\\.h|zconf\\.h"
55+
misc-include-cleaner.IgnoreHeaders: "sys/.*|asm-generic/.*|mach/.*|dispatch/.*|_types/.*|Availability\\.h|secure/.*|_.*\\.h|zconf\\.h"
5656
readability-function-cognitive-complexity.Threshold: 25
5757
readability-function-size.StatementThreshold: 200
5858
readability-function-size.LineThreshold: 400

‎Jenkinsfile‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ arena hash_table dyn_array str_intern log str_util workspace platform diagnostic
2121
steps {
2222
sh '''
2323
set -eu
24+
rm -f .jenkins-installed-cli
2425
scripts/lint.sh --ci CLANG_FORMAT=clang-format-20
2526
'''
2627
}
@@ -66,7 +67,6 @@ arena hash_table dyn_array str_intern log str_util workspace platform diagnostic
6667
steps {
6768
sh '''
6869
set -eu
69-
rm -f .jenkins-installed-cli
7070
installed_cli="$HOME/.local/bin/codebase-memory-cli"
7171
if [ -x "$installed_cli" ]; then
7272
"$installed_cli" daemon stop >/dev/null 2>&1 || true
@@ -186,7 +186,7 @@ arena hash_table dyn_array str_intern log str_util workspace platform diagnostic
186186
// keep them in one quiet shard and distribute the rest.
187187
def daemonNames = ['cli', 'daemon', 'daemon_ipc', 'daemon_runtime',
188188
'daemon_application', 'daemon_bootstrap',
189-
'daemon_frontend', 'index_supervisor', 'watcher']
189+
'index_supervisor', 'watcher']
190190
def daemon = suites.findAll { it in daemonNames }
191191
def other = suites.findAll { !(it in daemonNames) }
192192
def groups = [daemon, other].findAll { !it.isEmpty() }

‎src/cli/cli.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3558,7 +3558,8 @@ static int cbm_remove_grok_mcp_owned(const char *binary_path, const char *config
35583558
/* #929: extensionless bash shims under %USERPROFILE%\\.claude\\hooks trigger
35593559
* the "How do you want to open this file?" dialog when editors (Cursor) scan
35603560
* the hooks dir, and cannot execute without bash anyway. Windows installs
3561-
* .cmd scripts; the extensionless legacy files are removed on upgrade. */
3561+
* .cmd scripts. The CLI-owned names have used .cmd since they were introduced,
3562+
* so no extensionless CLI-owned file needs cleanup on upgrade. */
35623563
#define CMM_HOOK_GATE_SCRIPT "codebase-memory-cli-discovery-gate.cmd"
35633564
#else
35643565
#define CMM_HOOK_GATE_SCRIPT "codebase-memory-cli-discovery-gate"

‎src/main.c‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2156,13 +2156,31 @@ static void main_daemon_ctl_print_status_json(bool active,
21562156
printf("{\"active\":false,\"status\":\"not_running\"}\n");
21572157
return;
21582158
}
2159-
printf("{\"active\":true,\"status\":\"active\",\"permanent\":%s,"
2160-
"\"pid\":%lu,\"clients\":%u,\"committed_clients\":%u,\"stopping\":%s,"
2161-
"\"version\":\"%s\",\"build\":\"%s\"}\n",
2162-
status->permanent ? "true" : "false", (unsigned long)status->daemon_pid,
2163-
(unsigned)status->client_count, (unsigned)status->committed_clients,
2164-
status->stopping ? "true" : "false", status->semantic_version,
2165-
status->build_fingerprint);
2159+
yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL);
2160+
yyjson_mut_val *root = doc ? yyjson_mut_obj(doc) : NULL;
2161+
if (!root) {
2162+
if (doc) {
2163+
yyjson_mut_doc_free(doc);
2164+
}
2165+
(void)fprintf(stderr, "error: could not allocate status JSON\n");
2166+
return;
2167+
}
2168+
yyjson_mut_doc_set_root(doc, root);
2169+
yyjson_mut_obj_add_bool(doc, root, "active", true);
2170+
yyjson_mut_obj_add_str(doc, root, "status", "active");
2171+
yyjson_mut_obj_add_bool(doc, root, "permanent", status->permanent);
2172+
yyjson_mut_obj_add_uint(doc, root, "pid", (unsigned long)status->daemon_pid);
2173+
yyjson_mut_obj_add_uint(doc, root, "clients", (unsigned)status->client_count);
2174+
yyjson_mut_obj_add_uint(doc, root, "committed_clients", (unsigned)status->committed_clients);
2175+
yyjson_mut_obj_add_bool(doc, root, "stopping", status->stopping);
2176+
yyjson_mut_obj_add_strcpy(doc, root, "version", status->semantic_version);
2177+
yyjson_mut_obj_add_strcpy(doc, root, "build", status->build_fingerprint);
2178+
char *json = yyjson_mut_write(doc, 0, NULL);
2179+
yyjson_mut_doc_free(doc);
2180+
if (json) {
2181+
printf("%s\n", json);
2182+
free(json);
2183+
}
21662184
}
21672185

21682186
#ifdef _WIN32

‎src/operations/index.c‎

Lines changed: 155 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,16 @@ static bool index_bool_arg(const char *args_json, const char *key) {
5959

6060
static bool index_db_path(const char *project, char *out, size_t out_size);
6161

62+
static const char *index_mode_name(const char *mode_text) {
63+
if (mode_text && !strcmp(mode_text, "fast")) {
64+
return "fast";
65+
}
66+
if (mode_text && !strcmp(mode_text, "moderate")) {
67+
return "moderate";
68+
}
69+
return "full";
70+
}
71+
6272
static bool index_write_metrics(const char *path, const char *mode, const char *project,
6373
const cbm_pipeline_t *pipeline, int rc) {
6474
if (!path || !path[0])
@@ -187,7 +197,7 @@ static bool index_resolve_session_path(const cbm_operation_runtime_t *runtime, c
187197
return true;
188198
}
189199

190-
static char *index_args_with_repo_path(const char *args_json, const char *repo_path) {
200+
static char *index_args_with_string(const char *args_json, const char *key, const char *value) {
191201
const char *json = args_json ? args_json : "{}";
192202
yyjson_doc *source = yyjson_read(json, strlen(json), 0);
193203
yyjson_val *source_root = source ? yyjson_doc_get_root(source) : NULL;
@@ -204,8 +214,8 @@ static char *index_args_with_repo_path(const char *args_json, const char *repo_p
204214
yyjson_mut_doc_free(copy);
205215
return NULL;
206216
}
207-
(void)yyjson_mut_obj_remove_key(root, "repo_path");
208-
if (!yyjson_mut_obj_add_strcpy(copy, root, "repo_path", repo_path)) {
217+
(void)yyjson_mut_obj_remove_key(root, key);
218+
if (!yyjson_mut_obj_add_strcpy(copy, root, key, value)) {
209219
yyjson_mut_doc_free(copy);
210220
return NULL;
211221
}
@@ -214,6 +224,10 @@ static char *index_args_with_repo_path(const char *args_json, const char *repo_p
214224
return out;
215225
}
216226

227+
static char *index_args_with_repo_path(const char *args_json, const char *repo_path) {
228+
return index_args_with_string(args_json, "repo_path", repo_path);
229+
}
230+
217231
static bool index_db_path(const char *project, char *out, size_t out_size) {
218232
const char *cache = cbm_resolve_cache_dir();
219233
return project && cbm_validate_project_name(project) && cache && cache[0] &&
@@ -526,6 +540,53 @@ static bool index_build_success(yyjson_mut_doc *doc, yyjson_mut_val *root, const
526540
return degraded;
527541
}
528542

543+
static void index_fill_run_response(yyjson_mut_doc *doc, yyjson_mut_val *root, const char *project,
544+
const char *repo_path, bool persistence,
545+
cbm_pipeline_t *pipeline, int rc, char **excluded,
546+
int excluded_count, const cbm_file_error_t *errors,
547+
int error_count, bool metrics_failed) {
548+
yyjson_mut_obj_add_strcpy(doc, root, "project", project ? project : "");
549+
if (rc == 0) {
550+
char logfile[CBM_SZ_1K] = {0};
551+
bool has_log = index_write_log(project, errors, error_count, logfile, sizeof(logfile));
552+
bool degraded =
553+
index_build_success(doc, root, project, repo_path, persistence, pipeline, excluded,
554+
excluded_count, errors, error_count, has_log ? logfile : NULL);
555+
yyjson_mut_obj_add_str(doc, root, "status", degraded ? "degraded" : "indexed");
556+
if (metrics_failed) {
557+
yyjson_mut_obj_add_str(doc, root, "metrics_error", "failed to write metrics_out");
558+
}
559+
if (cbm_pipeline_had_format_migration(pipeline)) {
560+
yyjson_mut_obj_add_bool(doc, root, "format_migration", true);
561+
}
562+
} else {
563+
yyjson_mut_obj_add_str(doc, root, "status", "error");
564+
yyjson_mut_obj_add_str(doc, root, "hint",
565+
"Pipeline failed. Check repo_path exists and contains source files. "
566+
"Try mode='fast' for a quicker diagnostic run.");
567+
}
568+
}
569+
570+
static char *index_encode_run_response(const char *project, const char *repo_path, bool persistence,
571+
cbm_pipeline_t *pipeline, int rc, char **excluded,
572+
int excluded_count, const cbm_file_error_t *errors,
573+
int error_count, bool metrics_failed) {
574+
yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL);
575+
yyjson_mut_val *root = doc ? yyjson_mut_obj(doc) : NULL;
576+
if (!root) {
577+
if (doc) {
578+
yyjson_mut_doc_free(doc);
579+
}
580+
return NULL;
581+
}
582+
yyjson_mut_doc_set_root(doc, root);
583+
index_fill_run_response(doc, root, project, repo_path, persistence, pipeline, rc, excluded,
584+
excluded_count, errors, error_count, metrics_failed);
585+
char *payload = yyjson_mut_write(doc, 0, NULL);
586+
yyjson_mut_doc_free(doc);
587+
return payload;
588+
}
589+
529590
static cbm_operation_result_t index_run_physical(const char *repo_path, const char *args_json,
530591
const cbm_operation_runtime_t *runtime) {
531592
char *mode_text = index_string_arg(args_json, "mode");
@@ -547,6 +608,8 @@ static cbm_operation_result_t index_run_physical(const char *repo_path, const ch
547608
return index_text_error("index operation blocked by another mutation for this project");
548609
}
549610
if (runtime->cancelled && runtime->cancelled(runtime->cancelled_context)) {
611+
(void)index_write_metrics(metrics_out, index_mode_name(mode_text), mutation_project, NULL,
612+
CBM_PIPELINE_ABORT_PRESERVE_DB);
550613
runtime->mutation_end(runtime->mutation_context, mutation_project);
551614
free(mode_text);
552615
free(name);
@@ -559,13 +622,12 @@ static cbm_operation_result_t index_run_physical(const char *repo_path, const ch
559622
mode = CBM_MODE_FAST;
560623
else if (mode_text && !strcmp(mode_text, "moderate"))
561624
mode = CBM_MODE_MODERATE;
562-
const char *mode_name = mode == CBM_MODE_FAST ? "fast"
563-
: mode == CBM_MODE_MODERATE ? "moderate"
564-
: "full";
625+
const char *mode_name = index_mode_name(mode_text);
565626
free(mode_text);
566627
bool persistence = index_bool_arg(args_json, "persistence");
567628
cbm_pipeline_t *pipeline = cbm_pipeline_new(repo_path, NULL, mode);
568629
if (!pipeline) {
630+
(void)index_write_metrics(metrics_out, mode_name, mutation_project, NULL, CBM_NOT_FOUND);
569631
runtime->mutation_end(runtime->mutation_context, mutation_project);
570632
free(name);
571633
free(metrics_out);
@@ -576,6 +638,7 @@ static cbm_operation_result_t index_run_physical(const char *repo_path, const ch
576638
cbm_pipeline_free(pipeline);
577639
runtime->mutation_end(runtime->mutation_context, mutation_project);
578640
free(name);
641+
free(metrics_out);
579642
free(mutation_project);
580643
return index_text_error("invalid project name");
581644
}
@@ -597,42 +660,16 @@ static cbm_operation_result_t index_run_physical(const char *repo_path, const ch
597660
int error_count = 0;
598661
cbm_pipeline_get_file_errors(pipeline, &errors, &error_count);
599662
cbm_mem_collect();
600-
if (!index_write_metrics(metrics_out, mode_name, project, pipeline, rc))
601-
rc = rc ? rc : CBM_NOT_FOUND;
663+
bool metrics_failed = !index_write_metrics(metrics_out, mode_name, project, pipeline, rc);
664+
if (metrics_failed) {
665+
cbm_log_warn("index.metrics_write_failed", "path", metrics_out);
666+
}
602667
free(metrics_out);
603668
if (runtime->project_invalidate)
604669
runtime->project_invalidate(runtime->project_invalidate_context, project);
605-
yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL);
606-
yyjson_mut_val *root = doc ? yyjson_mut_obj(doc) : NULL;
607-
if (!doc || !root) {
608-
if (doc)
609-
yyjson_mut_doc_free(doc);
610-
if (!cbm_index_worker_active())
611-
cbm_pipeline_free(pipeline);
612-
runtime->mutation_end(runtime->mutation_context, mutation_project);
613-
free(project);
614-
free(mutation_project);
615-
return index_text_error("result allocation failed");
616-
}
617-
yyjson_mut_doc_set_root(doc, root);
618-
yyjson_mut_obj_add_strcpy(doc, root, "project", project ? project : "");
619-
if (rc == 0) {
620-
char logfile[CBM_SZ_1K] = {0};
621-
bool has_log = index_write_log(project, errors, error_count, logfile, sizeof(logfile));
622-
bool degraded =
623-
index_build_success(doc, root, project, repo_path, persistence, pipeline, excluded,
624-
excluded_count, errors, error_count, has_log ? logfile : NULL);
625-
yyjson_mut_obj_add_str(doc, root, "status", degraded ? "degraded" : "indexed");
626-
if (cbm_pipeline_had_format_migration(pipeline))
627-
yyjson_mut_obj_add_bool(doc, root, "format_migration", true);
628-
} else {
629-
yyjson_mut_obj_add_str(doc, root, "status", "error");
630-
yyjson_mut_obj_add_str(doc, root, "hint",
631-
"Pipeline failed. Check repo_path exists and contains source files. "
632-
"Try mode='fast' for a quicker diagnostic run.");
633-
}
634-
char *payload = yyjson_mut_write(doc, 0, NULL);
635-
yyjson_mut_doc_free(doc);
670+
char *payload =
671+
index_encode_run_response(project, repo_path, persistence, pipeline, rc, excluded,
672+
excluded_count, errors, error_count, metrics_failed);
636673
if (cbm_index_worker_active())
637674
cbm_log_info("index.worker.fast_exit", "skip", "pipeline_free");
638675
else
@@ -644,6 +681,79 @@ static cbm_operation_result_t index_run_physical(const char *repo_path, const ch
644681
: index_text_error("result encoding failed");
645682
}
646683

684+
/* Resolve metrics_out, require its parent directory to exist inside the allowed
685+
* roots, and rewrite the worker argument to the canonical parent + basename so
686+
* the worker writes exactly the validated path. */
687+
static bool index_validate_metrics_out(const char *json, const cbm_operation_runtime_t *runtime,
688+
const char *allowed_root, char **worker_args, char *err,
689+
size_t err_size) {
690+
char *metrics = index_string_arg(json, "metrics_out");
691+
if (!metrics || !metrics[0]) {
692+
free(metrics);
693+
return true;
694+
}
695+
cbm_normalize_path_sep(metrics);
696+
if (!index_resolve_session_path(runtime, &metrics)) {
697+
free(metrics);
698+
(void)snprintf(err, err_size, "failed to resolve metrics_out");
699+
return false;
700+
}
701+
char *slash = strrchr(metrics, '/');
702+
const char *base = metrics;
703+
if (slash) {
704+
base = slash;
705+
base++;
706+
}
707+
if (!base[0] || !strcmp(base, ".") || !strcmp(base, "..")) {
708+
free(metrics);
709+
(void)snprintf(err, err_size, "metrics_out must name a file");
710+
return false;
711+
}
712+
char parent[CBM_SZ_4K];
713+
size_t parent_len = slash ? (size_t)(slash - metrics) : 0;
714+
if (!slash) {
715+
(void)snprintf(parent, sizeof(parent), ".");
716+
} else if (parent_len == 0) {
717+
(void)snprintf(parent, sizeof(parent), "/");
718+
} else if (parent_len >= sizeof(parent)) {
719+
free(metrics);
720+
(void)snprintf(err, err_size, "metrics_out path too long");
721+
return false;
722+
} else {
723+
memcpy(parent, metrics, parent_len);
724+
parent[parent_len] = '\0';
725+
}
726+
char real[CBM_SZ_4K];
727+
if (!cbm_canonical_path(parent, real, sizeof(real))) {
728+
free(metrics);
729+
(void)snprintf(err, err_size, "metrics_out parent directory does not exist");
730+
return false;
731+
}
732+
cbm_normalize_path_sep(real);
733+
if (!cbm_workspace_root_allowed(real, cbm_workspace_home_dir(), cbm_workspace_cache_dir(),
734+
allowed_root, err, err_size)) {
735+
free(metrics);
736+
return false;
737+
}
738+
char final_path[CBM_SZ_4K];
739+
const char *last_sep = strrchr(real, '/');
740+
int n = snprintf(final_path, sizeof(final_path), "%s%s%s", real,
741+
last_sep && !strcmp(last_sep, "/") ? "" : "/", base);
742+
free(metrics);
743+
if (n <= 0 || (size_t)n >= sizeof(final_path)) {
744+
(void)snprintf(err, err_size, "metrics_out path too long");
745+
return false;
746+
}
747+
char *rewritten = index_args_with_string(*worker_args, "metrics_out", final_path);
748+
if (!rewritten) {
749+
(void)snprintf(err, err_size, "failed to prepare index request");
750+
return false;
751+
}
752+
free(*worker_args);
753+
*worker_args = rewritten;
754+
return true;
755+
}
756+
647757
cbm_operation_result_t cbm_index_operation_execute(const char *args_json,
648758
const cbm_operation_runtime_t *runtime) {
649759
const char *json = args_json ? args_json : "{}";
@@ -680,6 +790,12 @@ cbm_operation_result_t cbm_index_operation_execute(const char *args_json,
680790
free(repo_path);
681791
return index_text_error("failed to prepare index request");
682792
}
793+
if (!index_validate_metrics_out(json, runtime, allowed_root, &worker_args, boundary,
794+
sizeof(boundary))) {
795+
free(worker_args);
796+
free(repo_path);
797+
return index_text_error(boundary);
798+
}
683799
if (runtime && runtime->index_execute) {
684800
cbm_operation_result_t out =
685801
runtime->index_execute(runtime->index_execute_context, repo_path, worker_args);

0 commit comments

Comments
 (0)