fix(server): register the mcpm_up MCP tool so mcpm serve exposes it#64
Merged
Conversation
mcpm_up was fully built as an MCP tool — schema (tools.ts: UpInput),
handler (handlers.ts: handleMcpUp, with path-traversal validation and
stubbed interactive callbacks), and hardened in v0.7.1 ('mcpm_up no
longer auto-confirms') — but the server.registerTool() call was never
added. So 'mcpm serve' advertised 9 tools (docs/CLAUDE) while exposing 8.
Wire it in, matching the existing tool registrations:
- import UpInput (tools.ts) and handleMcpUp (handlers.ts)
- register 'mcpm_up' with inputSchema UpInput.shape and
annotations.destructiveHint = true, after mcpm_setup
MCP-safe: handleMcpUp stubs confirm/promptEnvVar and passes ci=true, so
no interactive prompt can hang the stdio server; it never calls
process.exit and returns structured output.
Verify: tsc --noEmit clean, tsup build clean, full suite 1287 tests pass
(no test asserts a tool count). serve now registers 9 tools.
The mcpm_up registration was clean, but exposing handleMcpUp to untrusted MCP callers surfaced pre-existing weaknesses. Lock down the MCP surface WITHOUT changing CLI behavior (new options default to current CLI semantics; only the MCP surface opts into the strict path). - silent failure: handleMcpUp's bare catch swallowed handleUp's early throws (no clients / lock failure / missing required env in CI), returning an empty success-looking result. Now surfaces the message via an `error` field and guarantees a non-empty `failed` on throw. Categorization uses a typed recordResult sink instead of scraping the success/block glyphs from output (which can't tell blocked from failed — policy reasons never contain 'blocked' and both use the same mark). - path traversal: replace the dead string-only stackFile guard with unconditional resolved-path containment (path.resolve + cwd boundary), closing Windows-backslash and sibling-prefix bypasses. - env harvesting: new allowProcessEnv option (default true = CLI); the MCP surface passes false so an attacker-controlled stack file can't read ambient process.env secrets into installed configs. - URL servers: new allowUrlServers option (default true = CLI); the MCP surface passes false so URL servers (which bypass the registry trust gate) are recorded as blocked, not installed. - tests: registration-completeness guard (asserts every TOOL_DEFINITIONS name is registered — the gap that hid the missing mcpm_up), plus handleMcpUp dryRun / blocked-by-policy / missing-env / URL-blocked and UpInput default-value assertions. CLI preserved (registerUpCommand sets none of the new options/deps). tsc --noEmit clean, build clean, full suite 1297 tests pass.
m1ngshum
added a commit
that referenced
this pull request
Jun 8, 2026
The earlier doc-drift pass set the MCP tool count to 8 to match main's then-current code (mcpm_up built but unregistered). PR #64 registers mcpm_up, so the correct count is 9. Restore mcpm_up to the tool lists in README and CLAUDE. Merge order: this should land with/after #64 (which registers the tool); main's code exposes 9 only once #64 is merged.
m1ngshum
added a commit
that referenced
this pull request
Jun 8, 2026
* docs: add rendered architecture diagrams (Mermaid) Add four GitHub-native Mermaid diagrams, each grounded in source and render-validated with mermaid-cli: - README 'How it works': system overview (local-first, only the public registry is remote; trust scan + atomic config writes + guard runtime) - ARCHITECTURE 'Data Flow': install + trust-gate flow (replaces ASCII), with the min-trust and already-installed fail branches made explicit - ARCHITECTURE: new 'Secrets resolution data flow' — shows plaintext secrets live only in the guarded child's in-memory env, never on disk - ARCHITECTURE 'Guard data flow': stdio MITM relay as a sequence diagram (replaces ASCII) — bidirectional parent->child / child->parent inspection, schema-drift block path, and event-log sink No code changes. * docs: mirror guard relay sequence diagram into GUARD.md Adds the same render-validated runtime message-flow sequence diagram to GUARD.md's 'Mental model' section (as 'Full message flow'), so the user-facing guard reference shows the bidirectional inspection pipeline alongside the existing ASCII sketch + detection-layers table. Same diagram already lives in ARCHITECTURE.md's 'Guard data flow'. * docs: fix scanner trust model in README overview diagram Audit against src/scanner/ found the overview diagram mislabeled the trust pipeline. Corrected to the real 4-component model from trust-score.ts: - Health Check (0-30, health-check.ts: spawn + verify) - Tier 1: Static Patterns (0-40, tier1.ts: secrets/injection/typosquat/exfil) - Tier 2: External Scan (0-20, tier2.ts: optional MCP-Scan) - Registry Metadata (0-10: publisher/age/downloads) Previously Tier 1/Tier 2 labels were swapped and registry metadata was folded into Tier 1; trust score 'max 80 (100 with external scan)' now reflects the dynamic maxPossible. Install + secrets + guard-relay diagrams audited and confirmed accurate (no change). * docs: fix drift found auditing all docs against source Full-repo doc audit (every implementation doc vs src/), each finding independently confirmed with file:line evidence. Surgical fixes: README.md - add 'mcpm why' to the Commands table (was undocumented) - expand 'mcpm publish' into scaffold / check / submit subcommands - mcpm serve exposes 8 tools, not 9 (mcpm_up is not registered) docs/ARCHITECTURE.md - CI matrix is Node 22/24/26 (was 20/22/24) - commands/ module: 24 commands, guard 12 subcommands, publish 3 - complete the Commands table (add outdated/why/secrets/publish) docs/GUARD.md - wrap transform includes --declared-env and --orig-hash flags - policy-overrides actions are ignore/warn/block/log_only (+ paused_until) docs/POLICY.md - mark signature_overrides as optional (matches Zod schema) docs/SIGNATURES.md - inspection model also scans structuredContent + JSON-RPC errors (tool_response) and title + full inputSchema (tool_description) CLAUDE.md - version v0.8.0, Node >=22.0.0, last-updated 2026-06-03 - local storage: aliases.json (not scans.json, which doesn't exist) - mcpm serve exposes 8 tools; complete the command list src/commands/guard.ts - header comment: all v0.5.0 guard subcommands are implemented (was 'land in subsequent build steps') * docs: serve exposes 9 tools (reconcile with mcpm_up registration) The earlier doc-drift pass set the MCP tool count to 8 to match main's then-current code (mcpm_up built but unregistered). PR #64 registers mcpm_up, so the correct count is 9. Restore mcpm_up to the tool lists in README and CLAUDE. Merge order: this should land with/after #64 (which registers the tool); main's code exposes 9 only once #64 is merged.
The handleMcpUp tests are integration-style (real temp dirs) and the
first one pays a one-time dynamic import("../commands/up.js"). Under CI
coverage instrumentation on the slower Node 22 runner that first test
(dryRun) crossed the 5000ms default and timed out, while Node 24/26
passed. Logic is correct (all 7 pass; ~1s locally) — raise the
per-file testTimeout to 30s to absorb CI-runner variance.
This was referenced Jun 8, 2026
m1ngshum
added a commit
that referenced
this pull request
Jun 8, 2026
…ew (H1/M1/M3/L1) (#65) * fix(security): close MCP-surface secret-leak gaps from post-ship review (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. * fix(security): enforce a hard trust floor on the mcpm_up MCP path (M2) 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. * fix(security): address code-review on #65 (M3 error-leak + M2 wiring 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).
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two commits:
mcpm_upMCP tool — it was fully built (schemaUpInput, handlerhandleMcpUp, hardened in v0.7.1) but neverregisterTool'd, somcpm serveexposed 8 tools while docs claimed 9. Now 9.handleMcpUp/handleUp; locked them down without changing CLI behavior.Commit 1 — registration
server.registerTool("mcpm_up", { inputSchema: UpInput.shape, annotations: { destructiveHint: true } }, handler)+ the two imports, matching the existing 8 tools. Handler/tool-def unchanged.Commit 2 — security hardening
A multi-agent review (correctness / security / tests / conventions, each finding adversarially confirmed) returned REQUEST_CHANGES. The registration diff itself was clean; all findings were about the now-exposed handler. Addressed:
catchswallowedhandleUpearly throws → empty "success"errorfield; guarantee non-emptyfailedon throw; categorize via a typedrecordResultsink (emoji-scraping couldn't tell blocked from failed)if (stackFile !== undefined)path.resolve+ cwd boundary) — closes Windows-backslash & sibling-prefix bypassesprocess.envsecretsallowProcessEnvoption (default true = CLI); MCP surface passesfalseallowUrlServersoption (default true = CLI); MCP surface passesfalse→ URL servers recorded as blocked, not installedregisterTools()extracted + test asserting everyTOOL_DEFINITIONSname is registeredUpInputdefaults unassertedhandleMcpUpsuite + defaults testCLI behavior preserved: every new option/dep is gated on
=== false/ optional and is set only by the MCP surface (handleMcpUp).registerUpCommandpasses none of them.Verification
tsc --noEmitclean ·pnpm buildcleanmcpm serveregisters 9 tools.Known minor behavior
For blocked-by-policy / URL-blocked outcomes,
handleUprecords the server inresult.blockedand also throws its summary, soresult.erroris set and a summary line appears inresult.failedalongside the realblockedentry — accurate (the op didn't fully succeed), if slightly redundant.Related
Complements docs PR #63 (which documents the current 8-tool surface). After this merges, the serve-tool count becomes 9 — see the note there about reconciling.
🤖 Generated with Claude Code