fix(security): close MCP-surface secret-leak gaps from post-ship review (H1/M1/M3/L1)

src/__tests__/commands/up.test.ts

@@ -131,6 +131,21 @@ describe("handleUp", () => {
     );
   });
 
+  // M2: the MCP surface passes a hard trust floor; a low-trust server must be
+  // blocked even when the stack file declares no policy (the caller controls the
+  // stack, so a missing/zero policy must not bypass the floor).
+  it("blocks a server below minTrustFloor even with no stack policy (M2)", async () => {
+    const stackPath = await writeStackAndLock(basicStack, basicLock);
+    const deps = makeDeps({ computeTrustScore: vi.fn().mockReturnValue(lowTrust) });
+
+    await expect(
+      handleUp({ stackFile: stackPath, minTrustFloor: 50 }, deps)
+    ).rejects.toThrow(/could not be installed/);
+
+    const adapter = (deps.getAdapter as ReturnType<typeof vi.fn>).mock.results[0].value;
+    expect(adapter.addServer).not.toHaveBeenCalled();
+  });
+
   it("auto-runs lock when no lock file exists", async () => {
     const dir = await mkdtemp(path.join(os.tmpdir(), "mcpm-up-nolock-"));
     const stackPath = path.join(dir, "mcpm.yaml");

src/__tests__/server/handlers-up-wiring.test.ts

@@ -0,0 +1,52 @@
+/**
+ * Wiring test for handleMcpUp — proves the MCP (untrusted-caller) surface hands
+ * handleUp the locked-down options. Unlike handlers-up.test.ts (which runs the REAL
+ * handleUp), this mocks handleUp and captures exactly what handleMcpUp passes, so
+ * deleting any lockdown option (e.g. `minTrustFloor`) makes this test fail — the
+ * gap review finding H2/M2 flagged (the wiring was otherwise unguarded).
+ */
+
+import { describe, it, expect, vi } from "vitest";
+import type { ClientId } from "../../config/paths.js";
+import type { ServerDeps } from "../../server/handlers.js";
+
+// Capture the options handleMcpUp passes to handleUp without executing the real one.
+const handleUpSpy = vi.fn().mockResolvedValue(undefined);
+vi.mock("../../commands/up.js", () => ({ handleUp: handleUpSpy }));
+
+// Imported after vi.mock so the spy is in effect.
+const { handleMcpUp } = await import("../../server/handlers.js");
+
+function makeServerDeps(): ServerDeps {
+  return {
+    registrySearch: vi.fn().mockResolvedValue([]),
+    registryGetServer: vi.fn(),
+    detectClients: vi
+      .fn<() => Promise<ClientId[]>>()
+      .mockResolvedValue(["claude-desktop"]),
+    getAdapter: vi.fn(),
+    getConfigPath: vi.fn().mockReturnValue("/mock/config.json"),
+    scanTier1: vi.fn().mockReturnValue([]),
+    computeTrustScore: vi.fn(),
+    addToStore: vi.fn().mockResolvedValue(undefined),
+    removeFromStore: vi.fn().mockResolvedValue(undefined),
+  };
+}
+
+describe("handleMcpUp — MCP surface lockdown wiring", () => {
+  it("hands handleUp the locked-down options (no ambient env, no URL servers, hard trust floor)", async () => {
+    handleUpSpy.mockClear();
+
+    await handleMcpUp({}, makeServerDeps());
+
+    expect(handleUpSpy).toHaveBeenCalledTimes(1);
+    const opts = handleUpSpy.mock.calls[0][0];
+    expect(opts.allowProcessEnv).toBe(false);
+    expect(opts.allowEnvFile).toBe(false);
+    expect(opts.allowUrlServers).toBe(false);
+    // HARD_TRUST_FLOOR (issue #24). Deleting `minTrustFloor` from handleMcpUp's
+    // handleUp call fails here even though the rest of the suite stays green.
+    expect(opts.minTrustFloor).toBe(25);
+    expect(opts.ci).toBe(true);
+  });
+});

src/__tests__/server/handlers-up.test.ts

@@ -170,8 +170,14 @@ servers:
     expect(result.error).toBeUndefined();
   });
 
-  // Fix F.3
-  it("blocked-by-policy: low-trust server with high minTrustScore appears in blocked", async () => {
+  // Fix F.3, corrected per review finding H2: handleMcpUp constructs its own
+  // handleUp deps with the REAL computeTrustScore (cts), so overriding
+  // deps.computeTrustScore (as the original test did) has no effect on this path.
+  // We instead drive the genuine pipeline: the clean mock server scores 55/80 = 69%
+  // (health-null 15 + static 40 + externalScan 0 + registryMeta 0; no scanner ->
+  // maxPossible 80), which is below the 90% policy floor (and the 94% lock snapshot
+  // also blocks it via score-drop), so the real checkTrustPolicy blocks it.
+  it("blocked-by-policy: a server below the trust floor is blocked via the real pipeline", async () => {
     const stackWithPolicy = `
 version: "1"
 policy:
@@ -183,13 +189,6 @@ servers:
     await writeStack(stackWithPolicy, REGISTRY_LOCK);
     const adapter = makeAdapter();
     const deps = makeDeps(adapter);
-    // Trust 75/80 = 94%... so force a low score to be below the 90% floor.
-    (deps.computeTrustScore as ReturnType<typeof vi.fn>).mockReturnValue({
-      score: 30,
-      maxPossible: 80,
-      level: "risky",
-      breakdown: { healthCheck: 0, staticScan: 20, externalScan: 0, registryMeta: 10 },
-    });
 
     const result = await handleMcpUp({}, deps);
 
@@ -221,8 +220,9 @@ servers:
       const result = await handleMcpUp({}, deps);
 
       // Fix A: a thrown failure is surfaced, not swallowed into a clean result.
+      // M1: the failed server lands in `failed` by NAME (not the error message).
       expect(result.error).toBeDefined();
-      expect(result.failed.length).toBeGreaterThan(0);
+      expect(result.failed).toContain("io.github.test/server-a");
       expect(result.installed).toEqual([]);
       // Fix C: the ambient secret was never written into a client config.
       expect(adapter.addServer).not.toHaveBeenCalled();
@@ -269,8 +269,11 @@ servers:
 
     const result = await handleMcpUp({}, deps);
 
+    // M1: a whole-batch throw (no server ever processed) is signaled via the
+    // authoritative `error` field — NOT by poisoning `failed`, which holds server
+    // names only. So `failed` is empty here while `error` carries the message.
     expect(result.error).toBeDefined();
-    expect(result.failed.length).toBeGreaterThan(0);
+    expect(result.failed).toEqual([]);
     expect(result.installed).toEqual([]);
   });
 
@@ -285,6 +288,55 @@ servers:
     ).rejects.toThrow("within the working directory");
   });
 
+  // H1: a `.env` in the working directory is an ambient secret source. On the MCP
+  // surface (allowEnvFile:false) it must NOT be harvested into an installed config.
+  // This fails on the pre-fix code (which read .env ungated) and passes after.
+  it("does not harvest the working-directory .env into installed configs (H1)", async () => {
+    const stackWithEnv = `
+version: "1"
+servers:
+  io.github.test/server-a:
+    version: "^1.0.0"
+    env:
+      LEAKME:
+        required: false
+        secret: false
+`;
+    await writeStack(stackWithEnv, REGISTRY_LOCK);
+    await writeFile(
+      path.join(tmpDir, ".env"),
+      "LEAKME=host-secret-from-dotenv\n",
+      "utf-8"
+    );
+    const adapter = makeAdapter();
+    const deps = makeDeps(adapter);
+
+    const result = await handleMcpUp({}, deps);
+
+    expect(result.installed).toContain("io.github.test/server-a");
+    // Installed, but the .env value must never reach the written config env.
+    const entry = adapter.addServer.mock.calls[0]?.[2] as
+      | { env?: Record<string, string> }
+      | undefined;
+    expect(entry?.env?.LEAKME).toBeUndefined();
+  });
+
+  // M3: lexical containment passes an in-cwd symlink (its own path is inside cwd),
+  // but the reader would follow it out of tree. The realpath re-check must reject it.
+  it("rejects a stackFile that is an in-cwd symlink pointing outside (symlink traversal)", async () => {
+    const { symlink, writeFile: wf, mkdtemp: mkd } = await import("fs/promises");
+    const outsideDir = await mkd(path.join(os.tmpdir(), "mcpm-outside-"));
+    const outsideTarget = path.join(outsideDir, "secret.yaml");
+    await wf(outsideTarget, REGISTRY_STACK, "utf-8");
+    // Plant an in-cwd symlink whose real target is outside cwd.
+    await symlink(outsideTarget, path.join(tmpDir, "link.yaml"));
+    const deps = makeDeps();
+
+    await expect(
+      handleMcpUp({ stackFile: "link.yaml" }, deps)
+    ).rejects.toThrow("within the working directory");
+  });
+
   // Happy path: a registry server installs and is categorized as installed.
   it("installs a registry server and reports it under installed", async () => {
     await writeStack(REGISTRY_STACK, REGISTRY_LOCK);

src/commands/up.ts

@@ -67,6 +67,23 @@ export interface UpOptions {
    * `false` so URL servers are recorded as blocked instead of installed.
    */
   allowUrlServers?: boolean;
+  /**
+   * Whether declared env vars may be auto-read from the working-directory `.env`
+   * file. The `.env` is an ambient secret source just like `process.env`.
+   * DEFAULT (undefined/true) preserves CLI behavior. The MCP surface passes
+   * `false` so an attacker-controlled stack file can't siphon the host's `.env`
+   * into an installed server config.
+   */
+  allowEnvFile?: boolean;
+  /**
+   * Hard, non-overridable trust-score floor (absolute, 0–100). When set, any
+   * server scoring below it is blocked regardless of the stack file's `policy`
+   * (which the caller controls). DEFAULT (undefined) = no floor = CLI behavior.
+   * The MCP surface passes the same hard floor the single-install tool enforces
+   * (issue #24), so a prompt-injected agent can't use the batch `up` path to
+   * install a low-trust server that `mcpm_install` would reject.
+   */
+  minTrustFloor?: number;
 }
 
 export interface UpDeps {
@@ -168,8 +185,16 @@ export async function handleUp(
     return;
   }
 
-  // Step 5: Load .env file for env var resolution
-  const envFileVars = await parseEnvFile(".env");
+  // Step 5: Load .env file for env var resolution.
+  // MCP surface lockdown (fix H1): the working-directory `.env` is an ambient
+  // secret source just like process.env. When allowEnvFile === false (set by the
+  // MCP surface) we skip reading it entirely, so an attacker-controlled stack
+  // file can't harvest the host's `.env` into an installed server config. The CLI
+  // default (undefined/true) reads `.env` as before.
+  const envFileVars =
+    options.allowEnvFile === false
+      ? ({ vars: {}, warnings: [] } as Awaited<ReturnType<typeof parseEnvFile>>)
+      : await parseEnvFile(".env");
 
   // Step 6: Re-assess trust in parallel
   const scannerAvailable = await deps.checkScannerAvailable();
@@ -339,6 +364,24 @@ async function processServer(input: ProcessInput): Promise<ServerResult> {
   };
   const trustScore = deps.computeTrustScore(trustInput);
 
+  // M2: enforce the hard trust floor BEFORE the (caller-controlled) stack policy.
+  // The MCP surface sets this so the batch `up` path honors the same floor the
+  // single-install MCP tool enforces (issue #24) — a stack file with no policy (or
+  // `minTrustScore: 0`) can't lower it. The CLI leaves it undefined (no floor).
+  // Note: this is an ABSOLUTE score floor (matching the sibling handleInstall #24),
+  // whereas checkTrustPolicy below compares normalized percentages — intentional
+  // and consistent; revisit only if the hard floor is ever made percentage-based.
+  if (
+    options.minTrustFloor !== undefined &&
+    trustScore.score < options.minTrustFloor
+  ) {
+    return {
+      name,
+      status: "blocked",
+      message: `trust score ${trustScore.score}/${trustScore.maxPossible} is below the required floor of ${options.minTrustFloor}`,
+    };
+  }
+
   // Policy check
   const policyResult = checkTrustPolicy({
     serverName: name,
@@ -476,12 +519,15 @@ async function resolveEnvVars(
     const fromFile = envFileVars[key];
     const fromDefault = decl.default;
 
+    // L1: compare against undefined, not truthiness — an explicitly-set empty
+    // string ("") is a legitimate value and must not silently fall through to
+    // the next source (or to the required-var prompt/throw).
     let value: string | undefined;
-    if (fromEnv) {
+    if (fromEnv !== undefined) {
       value = fromEnv;
-    } else if (fromFile) {
+    } else if (fromFile !== undefined) {
       value = fromFile;
-    } else if (fromDefault) {
+    } else if (fromDefault !== undefined) {
       value = fromDefault;
     } else if (decl.required) {
       if (options.ci) {

src/server/handlers.ts

@@ -438,6 +438,32 @@ export async function handleMcpUp(
   ) {
     throw new Error("stackFile must be within the working directory");
   }
+  // M3: the lexical check above catches "../" and absolute escapes, but NOT a
+  // symlink that lives inside cwd yet points outside it — the file reader would
+  // follow it (arbitrary out-of-tree read). Resolve the REAL path and re-check.
+  // realpath throws ENOENT when the file does not exist yet; that's fine — handleUp
+  // reports the missing file. A containment failure thrown inside the try is not
+  // an ErrnoException, so the catch re-throws it.
+  {
+    const { realpath } = await import("node:fs/promises");
+    try {
+      const [realStack, realCwd] = await Promise.all([
+        realpath(resolved),
+        realpath(process.cwd()),
+      ]);
+      if (realStack !== realCwd && !realStack.startsWith(realCwd + path.sep)) {
+        throw new Error("stackFile must be within the working directory");
+      }
+    } catch (err) {
+      // ENOENT (no such file), ELOOP (circular symlink), and ENOTDIR (a path
+      // component is a file) all mean "no real path to contain" — fall through and
+      // let handleUp report the missing/invalid file. Re-throwing them would leak a
+      // raw internal ErrnoException (with stack) to the untrusted caller. The
+      // containment Error thrown just above has no `.code`, so it still propagates.
+      const code = (err as NodeJS.ErrnoException).code ?? "";
+      if (!["ENOENT", "ELOOP", "ENOTDIR"].includes(code)) throw err;
+    }
+  }
 
   const { handleUp } = await import("../commands/up.js");
   const { writeFile } = await import("fs/promises");
@@ -462,11 +488,18 @@ export async function handleMcpUp(
         dryRun: args.dryRun,
         ci: true,
         yes: false,
-        // MCP surface lockdown (fixes C & D): never auto-read ambient secrets
-        // from process.env, and never install URL servers (they bypass the
-        // registry trust gate). Both default to true on the CLI.
+        // MCP surface lockdown (fixes C, D & H1): never auto-read ambient
+        // secrets from process.env OR the working-directory .env file, and never
+        // install URL servers (they bypass the registry trust gate). All three
+        // default to true on the CLI; the MCP (untrusted-caller) surface opts in
+        // to the locked-down behavior.
         allowProcessEnv: false,
         allowUrlServers: false,
+        allowEnvFile: false,
+        // M2: the batch `up` path must honor the same non-overridable trust floor
+        // the single-install MCP tool enforces (issue #24), so a low-trust server
+        // an agent could not install via mcpm_install can't slip in via mcpm_up.
+        minTrustFloor: HARD_TRUST_FLOOR,
       },
       {
         detectClients: deps.detectClients,
@@ -543,12 +576,12 @@ export async function handleMcpUp(
     }
   }
 
-  // Fix A: if handleUp threw, the failure MUST be signaled in the result \u2014 never
-  // an empty success. Surface the message via `error`, and ensure `failed` is
-  // non-empty (so a throw that produced no per-server failure can't read as clean).
-  if (thrownError !== undefined && failed.length === 0) {
-    failed.push(thrownError);
-  }
+  // Fix A, refined for M1: a thrown handleUp failure MUST be signaled \u2014 but only
+  // via the top-level `error` field (set in the return below). The previous
+  // version pushed the error *message* into `failed`, which is contracted to hold
+  // server NAMES; a consumer iterating it as names got a stray sentence. `error`
+  // is the authoritative batch-failure signal; `failed` stays names-only (genuine
+  // per-server failures are already recorded into it above via `records`).
 
   return {
     installed,