Conversation
CBM_WS_DENY_TOO_SHALLOW ("path is too broad to index as one root") has
no override today: cbm_workspace_verdict_is_overridable() only lifts
CBM_WS_DENY_SENSITIVE, and `allow-root --list` already documents
shallow/absolute refusals as "always-refused" rather than configurable.
That default is right -- a bare top-level tree like "/etc" or "/home"
is almost always a mistake -- but it leaves no way to say "I mean this
one specific broad root, on purpose." That's a real case: a person who
deliberately wants one combined project spanning several sibling repos
under a shared parent has no path forward today short of physically
restructuring their checkout.
PATH_ALLOW_BROAD names one exact canonical path that lifts a
CBM_WS_DENY_TOO_SHALLOW verdict, and only that verdict:
- exact match only, not a prefix -- naming one broad root must not
quietly approve every root below it too, the same "/Users"-style
breadth this depth rule exists to catch in the first place
- process environment, not a recorded grant: it never appears in
cbm_workspace_grant_list, and cbm_workspace_verdict_is_overridable()
is untouched, so CBM_WS_DENY_ABSOLUTE and CBM_WS_DENY_SENSITIVE stay
exactly as unliftable as before
- read once per call site with getenv(), mirroring how CBM_ALLOWED_ROOT
is already threaded into cbm_workspace_root_allowed() from each of
its four callers, rather than read inside
cbm_workspace_classify_root() itself -- that function is documented
as a pure function of its arguments precisely so it stays
host-independent and directly testable
Also updates the refusal message to name the fix, the same way the
sensitive-root refusal already names `allow-root --approve-sensitive`.
Adds ws_allow_broad_root_lifts_too_shallow_for_an_exact_match_only
alongside the existing too-shallow coverage, checking the exact-match
requirement, the untouched sensitive/absolute paths, and that nothing
is written to the grant store.
Signed-off-by: Mark Murawski <github@kobaz.net>
|
Actionable note ahead of the review verdict, so you are not blocked on me for it.
(For context, since it has caught others: our gate requires the Homebrew LLVM On the change itself, I have verified the three boundary claims in your description and they hold:
That is a carefully drawn boundary and it is the reason this is reviewable at all. What I cannot decide on my own is whether we want a new environment variable that lifts a workspace security refusal. A new env var is a one-way door, and this one weakens a guard by design, however narrowly. That is a maintainer call and I have put it in front of ours; I will come back with an answer rather than leaving it open-ended. |
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
DeusData
left a comment
There was a problem hiding this comment.
First, an apology: this has been open since 2 September with only a formatting note from me and no verdict. That is too long for a PR this small and this carefully argued, and it is on me, not you.
The need is real and I want it solved. "Too shallow" on POSIX means fewer than two path components, and that does not only catch /etc and /home — it also refuses /app, /code, /work, /src and /workspace, which is where a great many container images and build machines keep exactly one project. Your sibling-repos-under-a-shared-parent case is the same problem from the other side. Today the only answer is "restructure your checkout", and that is not a good answer.
You also got the hard parts right, and I want to say so specifically: exact match rather than prefix (so one allowance cannot quietly cover everything beneath it), leaving cbm_workspace_verdict_is_overridable() and the absolute/sensitive verdicts untouched, and keeping cbm_workspace_classify_root() a pure function by threading the value in from the callers instead of reading it inside. The test name says what it proves. All of that carries over unchanged to what I am about to ask for.
What I would like changed is the lever, not the rule. The comment on the verdict enum in workspace.h states the principle this boundary is built on: a refusal is "overridable by an explicit human action, never by a manifest or a tool call." An environment variable does not meet that bar here, for reasons specific to how this server runs:
- It lives in the MCP client's config file — a manifest, and one that a coding agent with file access can edit. The grant store exists precisely so that widening the boundary takes a person running a command.
- It leaves no record. You describe "never appears in
cbm_workspace_grant_list" as a property; for a security boundary it is the drawback —allow-root --listwould stop being the complete answer to "what can this installation index?". - In daemon mode
getenv()insrc/daemon/application.creads the daemon's environment, fixed when it started, not the client's. The allowed root is carried per session (srv->allowed_root, withCBM_ALLOWED_ROOTonly as the fallback) for exactly this reason;PATH_ALLOW_BROADis not, so a user who sets it in their client config can find that a daemon started earlier never sees it — with nothing telling them why. - The new refusal text hands the bypass to whoever made the request. When that requester is an agent calling
index_repository, "To index it anyway, set PATH_ALLOW_BROAD=…" is an instruction it may try to follow.
The shape I would merge: the same override, as a recorded grant —
codebase-memory-mcp allow-root --approve-broad <path>
mirroring --approve-sensitive end to end: parsed next to it in main.c (~line 1165), carried into cbm_workspace_grant_add() as a second explicit-approval flag, stored with the grant, and honoured in cbm_workspace_root_allowed() by the exact-match rule you already wrote — only for CBM_WS_DENY_TOO_SHALLOW, only for that exact canonical path. It then shows up in allow-root --list (ideally marked as a broad approval — the store already keeps an explicit-approval bit per grant for the sensitive case, match.exact_sensitive), it is revocable the same way, it behaves identically over stdio and through the daemon, and the refusal message can name the command — re-run allow-root with --approve-broad if that is intended — which a human runs in a terminal. CBM_WS_DENY_ABSOLUTE stays unliftable. Your test moves over almost as it is: grant without the flag → still refused; grant with it → allowed for the exact path, refused for a child and for a sibling; absolute and sensitive verdicts unaffected by it.
Two smaller things that apply either way:
lint / lintis still red from the formatting note on 2 September (the pinnedclang-formaton the lines you added). Note thatmainitself currently has an unrelated lint failure that a fix in flight (#2257) clears — once that lands, a rebase will show you only your own.- If any environment variable does survive in a later iteration, it needs the
CBM_prefix every other knob here carries;PATH_*reads like something the shell or a build system owns.
If you would rather not rework it, say so and I will carry it forward from your branch with you credited as the author of the design — but I would much prefer it to land as yours. Thank you for the care in this one; it shows.
|
Hi @kobaz, a quick status note so nothing here is ambiguous. The review from 20 September still stands as the path forward: Two mechanical things, whichever way it goes:
Thanks again for how carefully you drew this boundary. |
CBM_WS_DENY_TOO_SHALLOW ("path is too broad to index as one root") has no override today: cbm_workspace_verdict_is_overridable() only lifts CBM_WS_DENY_SENSITIVE, and
allow-root --listalready documents shallow/absolute refusals as "always-refused" rather than configurable. That default is right -- a bare top-level tree like "/etc" or "/home" is almost always a mistake -- but it leaves no way to say "I mean this one specific broad root, on purpose." That's a real case: a person who deliberately wants one combined project spanning several sibling repos under a shared parent has no path forward today short of physically restructuring their checkout.PATH_ALLOW_BROAD names one exact canonical path that lifts a CBM_WS_DENY_TOO_SHALLOW verdict, and only that verdict:
Also updates the refusal message to name the fix, the same way the sensitive-root refusal already names
allow-root --approve-sensitive.Adds ws_allow_broad_root_lifts_too_shallow_for_an_exact_match_only alongside the existing too-shallow coverage, checking the exact-match requirement, the untouched sensitive/absolute paths, and that nothing is written to the grant store.
What does this PR do?
Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)