fix(mcp): take the outermost repository as the trust boundary (WALM-684) - #986
Conversation
`enclosingProjectRoot` stopped at the nearest ancestor carrying a `.git`
*or* a `package.json`, so in a monorepo a client started in `apps/web`
treated `apps/web` as the entire project. `assertRuntimeRootLocation`
compares `MEMWAL_MCP_RUNTIME_DIR` against exactly that directory, so an
override naming a sibling — `<repo>/.memwal-runtime`, `apps/other/runtime` —
sat outside the nested package and passed, even though the same repository
that supplied the override also controls the tree it points at. That is the
control the function exists to enforce: absoluteness is not trust, because
a client expands `${workspaceFolder}` to an absolute path.
Walk to the outermost marker instead of returning at the first. `.git`
wins over `package.json` because it marks the checkout boundary rather
than a workspace member; a package tree with no repository around it still
counts, so a non-git monorepo keeps a boundary too. Continuing the walk
past a match is also what lets a worktree's or submodule's `.git` *file*
be found from inside a nested package.
This widens what gets refused, so it only ever rejects more. The default
`~/.memwal/runtime` is unaffected — home is still not a project — and a
runtime genuinely outside the repository still resolves.
Verified: 6 new tests (nested package boundary, sibling override refused
from both the member and the repo root, legitimate external runtime still
accepted, worktree `.git` file, no-git package tree, symlinked cwd).
Full package suite 310 pass, 0 fail.
|
@ducnmm flagging you on the release blocker in the description above, since it is a release decision rather than something this PR can fix. Short version: the plugin's
One more thing worth timing into that decision: the MW-05 launcher fix in this PR is not in any published build yet, so a candidate cut before this merges still carries the nested-package boundary gap. |
Review follow-up on #986 (Harry). Walking to the *outermost* marker introduced a regression the nearest-marker walk never had: a stray `/package.json` or `/.git` — common in container images built with WORKDIR / — outranked the real checkout below it, and a project of `/` hits the `project === dirname(project)` branch, which returns null and turns the location guard off entirely. From `/srv/app/packages/web` with markers at `/` and `/srv/app`, the previous commit returned null; this returns `/srv/app`. The root is now excluded from the walk the same way home already was. `enclosingProjectRoot` takes an injectable `exists` so the case can be tested without writing to `/`. Verified: two new tests (root marker above a real checkout; marker only at root falls back to cwd, not to "no project"). Package suite passes.
harrymove-ctrl
left a comment
harrymove-ctrl
left a comment
There was a problem hiding this comment.
Approved. Head identifies the outermost repository root (.git, then package.json) as the trust boundary rather than stopping at nested workspace packages. Fixes monorepo override escape. Tests and CI are green.
2 participants
Closes WALM-684. Triage of the 22 Sep static security review (WALM-679); MW-05 confirmed against source.
What was wrong
enclosingProjectRootstopped at the nearest ancestor carrying a.gitor apackage.json, so in a monorepo a client started inapps/webtreatedapps/webas the entire project.assertRuntimeRootLocationcomparesMEMWAL_MCP_RUNTIME_DIRagainst exactly that directory, so an override naming a sibling —<repo>/.memwal-runtime,apps/other/runtime— sat outside the nested package and passed, even though the same repository that supplied the override also controls the tree it points at.That is the control the function exists to enforce: absoluteness is not trust, because a client expands
${workspaceFolder}to an absolute path.The fix
Walk to the outermost marker instead of returning at the first.
.gitwins overpackage.jsonbecause it marks the checkout boundary rather than a workspace member; a package tree with no repository around it still counts, so a non-git monorepo keeps a boundary too. Continuing the walk past a match is also what lets a worktree's or submodule's.gitfile be found from inside a nested package.This only ever rejects more. The default
~/.memwal/runtimeis unaffected — home is still not a project — and a runtime genuinely outside the repository still resolves.Verification
6 new tests: nested package boundary, sibling override refused from both the member and the repo root, legitimate external runtime still accepted, worktree
.gitfile, no-git package tree, symlinked cwd. Full package suite 310 pass, 0 fail.This is not fixed by this PR and needs a release decision. MW-06 (WALM-685) asks that what is published and installed be checked, not only what is merged to
dev. Checking the actual published tarballs rather than inferring from dates:06d75e2f(origin/dev)packages/mcpversion0.0.140.0.14mcpPackageVersion)0.0.14-dev.2.mcp.json,.codex-mcp.json,.cursor-mcp.jsonall runnode ${PLUGIN_ROOT}/scripts/launch_mcp.mjs— the absolute launcher, so WALM-640 is in effect0.0.14-dev.2is missingconsent.jsandmemory-policy.js— the WALM-642 secret-filtering and consent work. Its publisheddist/carries 14 modules against 19 insrcondev. The pin was published before the WALM-642 merge landed, so installing the plugin from this candidate gives an MCP server without the secret-exclusion policy, even thoughdevhas it.0.0.14-dev.6contains all four missing modules.0.0.14-rc.0contains three (nohealth-probe).0.0.14, so moving the pin is allowed.Either move
mcpPackageVersionto a build that contains those modules, or cut a freshrcfrom currentdev. I did not pick, since that is a release call.Also note: the MW-05 fix in this PR is not in any published build yet, so a candidate cut before it merges still has the nested-package boundary gap.
All six fixes MW-06 lists were confirmed present on
devand absent on the reviewedmainsnapshot (0ffbacbb), which has no launcher file at all and still selects project credentials with no approval gate.devis 173 commits ahead, 57 touchingpackages/mcp.