Skip to content

Commit 9846c3f

Browse files
DeusDataDeusData
committed
fix(daemon): compare session roots separator-equivalently without respelling the policy root
The first cut on this branch (c1b9c45) respelled the session root and the allowed root with forward slashes on the way into the session policy (cbm_mcp_server_set_session_context) and at the three application.c sites that canonicalize a root. CI's Windows shard then failed two tests: tests/test_daemon_application.c:2256: ASSERT(sensitive_blocked) tests/test_daemon_application.c:2380: ASSERT(sensitive_blocked) Both set HOME to the cbm_canonical_path spelling of a directory - the platform's native form, backslashes on Windows - and expect a session rooted there to be refused for auto-index and for watch. The sensitive-root and allowed-root containment walk (ws_is_ancestor_or_equal in src/foundation/workspace.c) compares prefixes byte-exact, so a root that now read C:/Users/... no longer matched a HOME of C:\Users\...: the sensitive refusal was lost and $HOME was admitted. Respelling the policy root was the wrong layer. This commit keeps the policy roots in their canonical native spelling (mcp.c and the three application.c sites are back to what main runs) and folds the separator spelling at the one comparison the original defect lives in: application_index_args_equal, where the running job's args are compared with an explicit index_repository request's. The auto-index job spells repo_path the way the session policy holds it; the handler spells it with forward slashes. On Windows the two never matched, and the request was refused as OPTIONS_CONFLICT instead of joining the job already running for its root. The fold acts on the parsed copies only (yyjson_mut_obj_replace with a cbm_normalize_path_sep'd string), so nothing the daemon stores changes spelling and every other option stays exact. Why this is correct on Windows without a local Windows run: the policy spelling is untouched, so the containment path is byte-for-byte what main runs there today; cbm_normalize_path_sep folds backslashes on every platform, so the new fold is exercised by the new test wherever the suite runs. Tests: daemon_session_context_keeps_one_spelling_of_a_root asserted that the policy respells a root with forward slashes. That was the wrong contract: the policy's containment checks compare its spelling byte-exact against HOME and the granted roots, so a respelled root is exactly what stops matching them. It is replaced by daemon_session_context_keeps_the_policy_spelling_of_a_root, which binds the opposite: the policy stores the root as given. The comparison is exposed through application_internal.h as cbm_daemon_application_index_args_equal_for_test and bound by daemon_application_index_args_compare_repo_path_separator_equivalently: a backslash and a forward-slash repo_path compare equal; a different path, a sub-path, and a different mode still do not. Refs #1726 Signed-off-by: Martin Vogel <[email protected]>
1 parent c1b9c45 commit 9846c3f

5 files changed

Lines changed: 80 additions & 43 deletions

File tree

‎src/daemon/application.c‎

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -379,20 +379,6 @@ static bool application_canonical_directory_exists(const char *path) {
379379
return cbm_path_info_utf8(path, &info) == 0 && info.is_directory;
380380
}
381381

382-
/* The daemon's one spelling of a repository root. cbm_canonical_path answers in
383-
* the platform's native form (backslashes on Windows) while every tool handler
384-
* normalizes the separators of the paths it canonicalizes. A root kept native
385-
* is a second name for one directory, and the exact comparison downstream -
386-
* index-args equality when a request joins the job already running for that
387-
* root - never matches it. */
388-
static bool application_canonical_root(const char *path, char *out, size_t out_size) {
389-
if (!cbm_canonical_path(path, out, out_size)) {
390-
return false;
391-
}
392-
cbm_normalize_path_sep(out);
393-
return true;
394-
}
395-
396382
static cbm_daemon_application_watch_t *application_find_watch_locked(
397383
cbm_daemon_application_t *application, const char *project) {
398384
for (cbm_daemon_application_watch_t *watch = application->watches; watch; watch = watch->next) {
@@ -1602,6 +1588,33 @@ static bool application_index_args_normalize_defaults(yyjson_mut_val *root) {
16021588
return true;
16031589
}
16041590

1591+
/* One directory is one root. The auto-index job spells repo_path the way the
1592+
* session policy holds it - the platform's native form, backslashes on
1593+
* Windows - while an explicit index_repository request arrives in the
1594+
* handler's forward-slash spelling. Compared byte-exact the two never matched
1595+
* on Windows, and the request was refused as an options conflict instead of
1596+
* joining the job already running for its root. The policy keeps its
1597+
* spelling: the sensitive-root and allowed-root containment checks match it
1598+
* byte-exact against HOME and the granted roots, and respelling it there
1599+
* admitted $HOME. So the fold happens here, on this comparison's private copy,
1600+
* and nothing the daemon stores changes. */
1601+
static bool application_index_args_fold_repo_path(yyjson_mut_doc *document) {
1602+
yyjson_mut_val *root = yyjson_mut_doc_get_root(document);
1603+
yyjson_mut_val *repo_path = yyjson_mut_obj_get(root, "repo_path");
1604+
if (!repo_path || !yyjson_mut_is_str(repo_path)) {
1605+
return true;
1606+
}
1607+
char *folded = strdup(yyjson_mut_get_str(repo_path));
1608+
if (!folded) {
1609+
return false;
1610+
}
1611+
cbm_normalize_path_sep(folded);
1612+
yyjson_mut_val *key = yyjson_mut_str(document, "repo_path");
1613+
yyjson_mut_val *value = yyjson_mut_strcpy(document, folded);
1614+
free(folded);
1615+
return key && value && yyjson_mut_obj_replace(root, key, value);
1616+
}
1617+
16051618
static bool application_index_args_equal(const char *left, const char *right) {
16061619
if (!left || !right) {
16071620
return false;
@@ -1614,6 +1627,8 @@ static bool application_index_args_equal(const char *left, const char *right) {
16141627
yyjson_mut_val *right_root = right_copy ? yyjson_mut_doc_get_root(right_copy) : NULL;
16151628
bool equal = application_index_args_normalize_defaults(left_root) &&
16161629
application_index_args_normalize_defaults(right_root) &&
1630+
application_index_args_fold_repo_path(left_copy) &&
1631+
application_index_args_fold_repo_path(right_copy) &&
16171632
yyjson_mut_equals(left_root, right_root);
16181633
yyjson_mut_doc_free(left_copy);
16191634
yyjson_mut_doc_free(right_copy);
@@ -1622,6 +1637,10 @@ static bool application_index_args_equal(const char *left, const char *right) {
16221637
return equal;
16231638
}
16241639

1640+
bool cbm_daemon_application_index_args_equal_for_test(const char *left, const char *right) {
1641+
return application_index_args_equal(left, right);
1642+
}
1643+
16251644
/* Caller holds application->mutex. Keeping watcher ownership validation and
16261645
* this admission in the same critical section closes the unwatch race. */
16271646
static cbm_daemon_application_job_t *application_job_subscribe_locked(
@@ -2369,10 +2388,9 @@ static cbm_daemon_runtime_application_status_t application_set_context(
23692388
}
23702389
char canonical_root[APPLICATION_PATH_CAP] = {0};
23712390
char canonical_allowed[APPLICATION_PATH_CAP] = {0};
2372-
bool canonical = application_canonical_root(root, canonical_root, sizeof(canonical_root));
2391+
bool canonical = cbm_canonical_path(root, canonical_root, sizeof(canonical_root));
23732392
if (canonical && allowed_present) {
2374-
canonical =
2375-
application_canonical_root(allowed, canonical_allowed, sizeof(canonical_allowed));
2393+
canonical = cbm_canonical_path(allowed, canonical_allowed, sizeof(canonical_allowed));
23762394
}
23772395
canonical = canonical && application_canonical_directory_exists(canonical_root);
23782396
bool set =
@@ -3387,7 +3405,7 @@ static int application_background_index(cbm_daemon_application_t *application,
33873405
return -1;
33883406
}
33893407
char canonical_root[APPLICATION_PATH_CAP];
3390-
if (!application_canonical_root(root_path, canonical_root, sizeof(canonical_root)) ||
3408+
if (!cbm_canonical_path(root_path, canonical_root, sizeof(canonical_root)) ||
33913409
!application_canonical_directory_exists(canonical_root)) {
33923410
return -1;
33933411
}

‎src/daemon/application_internal.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,4 +45,10 @@ int cbm_daemon_application_busy_queue_waits_for_test(void);
4545
char *cbm_daemon_application_framable_response_for_test(char *response,
4646
const cbm_jsonrpc_request_t *request);
4747

48+
/* The job registry's index-argument equality: the running job's args against
49+
* a request's. Exposed because the two repo_path spellings it folds - the
50+
* session policy's native root and a handler's forward-slash one - only differ
51+
* on Windows, while the fold itself runs on every platform. */
52+
bool cbm_daemon_application_index_args_equal_for_test(const char *left, const char *right);
53+
4854
#endif /* CBM_DAEMON_APPLICATION_INTERNAL_H */

‎src/mcp/mcp.c‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1765,18 +1765,7 @@ bool cbm_mcp_server_set_session_context(cbm_mcp_server_t *srv, const char *sessi
17651765
return false;
17661766
}
17671767

1768-
/* Keep both roots in the one spelling every tool handler produces. Each
1769-
* handler normalizes separators after canonicalizing, so a root stored in
1770-
* the platform's native form (backslashes on Windows) is a second name for
1771-
* the same directory - and the daemon compares roots exactly. An explicit
1772-
* index_repository request for this root was refused as an options
1773-
* conflict instead of joining the auto-index job already running for it,
1774-
* because the job's repo_path never matched the request's. */
1775-
char root[sizeof(srv->session_root)];
1776-
snprintf(root, sizeof(root), "%s", session_root);
1777-
cbm_normalize_path_sep(root);
1778-
1779-
char *project = cbm_project_name_from_path(root);
1768+
char *project = cbm_project_name_from_path(session_root);
17801769
if (!project || project[0] == '\0' || strlen(project) >= sizeof(srv->session_project)) {
17811770
free(project);
17821771
return false;
@@ -1787,9 +1776,8 @@ bool cbm_mcp_server_set_session_context(cbm_mcp_server_t *srv, const char *sessi
17871776
free(project);
17881777
return false;
17891778
}
1790-
cbm_normalize_path_sep(allowed_copy);
17911779

1792-
snprintf(srv->session_root, sizeof(srv->session_root), "%s", root);
1780+
snprintf(srv->session_root, sizeof(srv->session_root), "%s", session_root);
17931781
snprintf(srv->session_project, sizeof(srv->session_project), "%s", project);
17941782
free(project);
17951783

‎tests/test_daemon.c‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -467,21 +467,21 @@ TEST(daemon_sessions_keep_distinct_roots_and_allowed_root_policy) {
467467
PASS();
468468
}
469469

470-
/* The daemon canonicalizes a client's root through the platform API, which on
471-
* Windows answers in backslash form, while every tool handler normalizes the
472-
* separators of the paths it canonicalizes. Stored in its native form, one
473-
* directory had two names, and an explicit index_repository request was
474-
* refused as an options conflict instead of joining the auto-index job already
475-
* running for that root. Separators fold on every platform, so the check binds
476-
* wherever the suite runs. */
477-
TEST(daemon_session_context_keeps_one_spelling_of_a_root) {
470+
/* The session policy keeps a root in the spelling the daemon canonicalized it
471+
* to - the platform's native form, backslashes on Windows. The sensitive-root
472+
* and allowed-root containment checks compare that spelling byte-exact against
473+
* HOME and the granted roots, so a root respelled with forward slashes on the
474+
* way in stopped matching them, and $HOME was admitted for auto-index and
475+
* watch on Windows. One directory being one root for the job registry is
476+
* folded at that comparison, never by respelling the policy. */
477+
TEST(daemon_session_context_keeps_the_policy_spelling_of_a_root) {
478478
cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL);
479479
ASSERT_NOT_NULL(srv);
480480
cbm_mcp_server_set_background_tasks(srv, false);
481481

482482
ASSERT_TRUE(cbm_mcp_server_set_session_context(srv, "C:\\repos\\cbm", "C:\\repos"));
483-
ASSERT_STR_EQ(cbm_mcp_server_session_root(srv), "C:/repos/cbm");
484-
ASSERT_STR_EQ(cbm_mcp_server_allowed_root(srv), "C:/repos");
483+
ASSERT_STR_EQ(cbm_mcp_server_session_root(srv), "C:\\repos\\cbm");
484+
ASSERT_STR_EQ(cbm_mcp_server_allowed_root(srv), "C:\\repos");
485485

486486
cbm_mcp_server_free(srv);
487487
PASS();
@@ -500,5 +500,5 @@ SUITE(daemon) {
500500
RUN_TEST(daemon_bridge_rejects_embedded_nul_body);
501501
RUN_TEST(daemon_bridge_rejects_oversized_headers);
502502
RUN_TEST(daemon_sessions_keep_distinct_roots_and_allowed_root_policy);
503-
RUN_TEST(daemon_session_context_keeps_one_spelling_of_a_root);
503+
RUN_TEST(daemon_session_context_keeps_the_policy_spelling_of_a_root);
504504
}

‎tests/test_daemon_application.c‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5382,6 +5382,30 @@ TEST(daemon_application_oversized_reply_is_a_jsonrpc_error_not_a_death) {
53825382
PASS();
53835383
}
53845384

5385+
/* One directory is one root for the job registry. The auto-index job spells
5386+
* repo_path the way the session policy holds it - the platform's native form,
5387+
* backslashes on Windows - while an explicit index_repository request arrives
5388+
* in the handler's forward-slash spelling. Compared byte-exact the two never
5389+
* matched on Windows, and the request was refused as an options conflict
5390+
* instead of joining the job already running for its root. The fold runs on
5391+
* every platform, so this binds wherever the suite runs; every other option
5392+
* stays exact. */
5393+
TEST(daemon_application_index_args_compare_repo_path_separator_equivalently) {
5394+
ASSERT_TRUE(cbm_daemon_application_index_args_equal_for_test(
5395+
"{\"repo_path\":\"C:\\\\repos\\\\cbm\"}", "{\"repo_path\":\"C:/repos/cbm\"}"));
5396+
ASSERT_TRUE(cbm_daemon_application_index_args_equal_for_test(
5397+
"{\"repo_path\":\"C:\\\\repos\\\\cbm\",\"mode\":\"full\"}",
5398+
"{\"mode\":\"full\",\"repo_path\":\"C:/repos/cbm\"}"));
5399+
ASSERT_FALSE(cbm_daemon_application_index_args_equal_for_test(
5400+
"{\"repo_path\":\"C:\\\\repos\\\\cbm\"}", "{\"repo_path\":\"C:/repos/cbm2\"}"));
5401+
ASSERT_FALSE(cbm_daemon_application_index_args_equal_for_test(
5402+
"{\"repo_path\":\"C:\\\\repos\\\\cbm\"}", "{\"repo_path\":\"C:/repos/cbm/sub\"}"));
5403+
ASSERT_FALSE(cbm_daemon_application_index_args_equal_for_test(
5404+
"{\"repo_path\":\"C:\\\\repos\\\\cbm\",\"mode\":\"incremental\"}",
5405+
"{\"repo_path\":\"C:/repos/cbm\"}"));
5406+
PASS();
5407+
}
5408+
53855409
SUITE(daemon_application) {
53865410
RUN_TEST(daemon_application_oversized_reply_is_a_jsonrpc_error_not_a_death);
53875411
RUN_TEST(daemon_application_new_session_does_not_retain_initial_store);
@@ -5400,6 +5424,7 @@ SUITE(daemon_application) {
54005424
RUN_TEST(daemon_application_initialize_coalesces_auto_index_for_full_sessions);
54015425
RUN_TEST(daemon_application_sensitive_root_blocks_auto_index_but_preserves_controls);
54025426
RUN_TEST(daemon_application_sensitive_root_blocks_watch_but_preserves_controls);
5427+
RUN_TEST(daemon_application_index_args_compare_repo_path_separator_equivalently);
54035428
RUN_TEST(daemon_application_auto_index_honors_tracked_file_limit);
54045429
RUN_TEST(daemon_application_auto_index_file_count_handles_literal_metacharacter_path);
54055430
RUN_TEST(daemon_application_auto_index_file_count_supports_non_git_roots);

0 commit comments

Comments
 (0)