fix(security): close MCP-surface secret-leak gaps from post-ship review (H1/M1/M3/L1)#65
Conversation
…ew (H1/M1/M3/L1) The #64 hardening blocked process.env on the MCP surface but a post-ship security review surfaced gaps that survive it: - H1 (HIGH): the working-directory `.env` was read ungated, so an attacker- controlled stack file could still siphon the host's `.env` into an installed server config even with allowProcessEnv:false. Add `allowEnvFile` (default true = CLI); the MCP surface passes false and skips reading `.env`. - M3 (MED): containment used lexical path.resolve only, so an in-cwd symlink pointing outside cwd passed the check and the reader followed it. Add a realpath re-check (ENOENT-tolerant). - M1 (MED): a whole-batch throw pushed its error *message* into `failed`, which is contracted to hold server names. Surface it via the `error` field only. - L1 (LOW): env resolution used truthiness, dropping legitimate empty-string values. Compare against undefined. Also corrects the misleading blocked-by-policy test (review H2): it mocked deps.computeTrustScore, which handleMcpUp never calls (it builds its own), so the gate was only exercised by coincidence. The test now drives the real trust pipeline; new tests prove the `.env` is not harvested and a symlink is rejected. tsc + tsup clean; full vitest suite green.
The single-install MCP tool (handleInstall) clamps installs to a non-overridable HARD_TRUST_FLOOR (issue #24), but the batch `mcpm_up` MCP path enforced only the stack file's `policy` — which the untrusted caller controls and can omit or set to `minTrustScore: 0`. A prompt-injected agent could therefore install a low-trust server via `mcpm_up` that `mcpm_install` would reject. Add a `minTrustFloor` option to handleUp (default undefined = no floor = CLI behavior); processServer blocks any server scoring below it BEFORE the caller-controlled policy check. handleMcpUp passes the same HARD_TRUST_FLOOR the single-install tool uses, closing the bypass while leaving the CLI unchanged. Test: a score-30 server with no stack policy is blocked by a floor of 50.
|
Added M2 to this PR (it edits the same M2 (MED) — trust-floor bypass on Fix: Updated totals: 6 findings in this PR (H1, M1, M2, M3, L1, + H2 test correction). Full suite green (1300 tests), tsc + tsup clean. |
…test) Multi-agent review follow-ups (no blockers found; these are the should-fix items): - M3 (handlers.ts): the realpath containment catch only swallowed ENOENT, so an untrusted-caller stackFile that is a circular symlink (ELOOP) or has a file as a path component (ENOTDIR) escaped as a raw ErrnoException with an internal stack trace. Treat ENOENT/ELOOP/ENOTDIR alike (fall through); the containment Error has no `.code` so it still propagates. - M2 wiring coverage: add handlers-up-wiring.test.ts — mocks handleUp and asserts handleMcpUp passes allowProcessEnv/allowEnvFile/allowUrlServers:false AND minTrustFloor:25. Deleting the floor from the call site now fails a test. - Nits: missing-env test asserts `failed` contains the server NAME (names-only contract, symmetric with the no-clients test); corrected the H2 test comment (real score is 55/80 = 69%, not ~62%; the override set the unused deps.computeTrustScore); clarified the absolute-vs-percentage floor is intentional. tsc + tsup clean; full suite green (1301).
Multi-agent review pass — addressed (commit 1d05f43)Ran a 3-lens review (security/efficacy · correctness · test-proof) with 2-lens adversarial verification. Verdict: approve-with-nits, no blockers — the five fixes close their holes and introduce no new vuln. (The verifier correctly killed two cross-contaminated M4a/M4b false positives — that code lives in #66, not here.) Addressed the real items:
Full suite green (1301), tsc + tsup clean. |
Summary
Post-ship security review of
main(after #62/#61/#63/#64) found that the #64 MCP hardening — which blocksprocess.envon the untrustedmcpm_upsurface — has gaps that survive it. This PR closes the shipped-diff ones. (Pre-existing items go in a follow-up PR.)Method: 6-dimension multi-agent review → 2-lens adversarial verification (refute + exploit) → manual confirmation against source.
Findings fixed
up.tsgatedprocess.env(475) but read the CWD.envungated (172, 476). An attacker-controlled stack file could still siphon the host's.envinto an installed server config.allowEnvFileoption (defaulttrue= CLI);handleMcpUppassesfalse, skipping the.envread entirely.path.resolveonly — an in-cwd symlink pointing outside cwd passed, and the reader followed it (out-of-tree read).realpathre-check after the lexical check (ENOENT-tolerant).failed[], which is contracted to hold server names — a consumer iterating it got a stray sentence.errorfield only;failed[]stays names-only."") values.undefined.deps.computeTrustScore, whichhandleMcpUpnever calls (it builds its own) — the gate was only exercised by coincidence.Tests
.envis not harvested on the MCP surface (fails on pre-fix code).error(not poisonedfailed).tsc --noEmit+tsupclean.Not in this PR (follow-up)
Pre-existing hardening (http→loopback-only remote URLs, runtime-arg
..rejection, machine-key downgrade warning) and the trust-floor policy decision (M2).