From 403d00fd48d80e708336fbc7f7a5a1111f73e1f9 Mon Sep 17 00:00:00 2001 From: m1ngshum <140998506+m1ngshum@users.noreply.github.com> Date: Mon, 8 Jun 2026 23:11:59 +0800 Subject: [PATCH 1/2] fix(security): harden remote-URL & runtime-arg validation; honest keychain notice (M4/M5/I1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing hardening surfaced by the post-ship security review: - M4a (MED): validateRemoteUrl accepted plaintext http:// to any host, which is interceptable once written to an IDE config. Now https is always allowed; http only for loopback (localhost / 127.0.0.1 / ::1 / *.localhost). Also wire validateRemoteUrl into the `mcpm up` URL-server path, which previously wrote stack-file `url:` servers to client configs unvalidated. - M4b (MED): validateRuntimeArgs' allowlist permits "." and "/" inside values, so a ".." path-traversal segment (e.g. --config=../secret) slipped through. Reject ".." segments up front (bare, after "=", or path-separated); a non-traversal double dot like --range=1..10 is left untouched. - M5/I1 (LOW/INFO): the keychain secret-storage notice claimed "protects against other-user/offline access" unconditionally — false when keychain mode silently falls back to the machine-derived key. Make the notice honest about both backends and recommend `mcpm secrets migrate`. tsc + tsup clean; full vitest suite green. --- src/__tests__/commands/install.test.ts | 21 +++++++++++++-- src/commands/install.ts | 37 ++++++++++++++++++++++++-- src/commands/up.ts | 12 +++++++-- 3 files changed, 64 insertions(+), 6 deletions(-) diff --git a/src/__tests__/commands/install.test.ts b/src/__tests__/commands/install.test.ts index ebe8665..94f3911 100644 --- a/src/__tests__/commands/install.test.ts +++ b/src/__tests__/commands/install.test.ts @@ -1176,7 +1176,15 @@ describe("validateRuntimeArgs", () => { }); it("rejects arguments containing path traversal (..)", () => { - expect(() => validateRuntimeArgs(["../../etc/passwd"])).toThrow(/unrecognized/i); + // M4b: a ".." path segment is rejected up front (more specific than the + // generic "unrecognized" allowlist failure), in bare and flag-value forms. + expect(() => validateRuntimeArgs(["../../etc/passwd"])).toThrow(/traversal/i); + expect(() => validateRuntimeArgs([".."])).toThrow(/traversal/i); + expect(() => validateRuntimeArgs(["a/../../b"])).toThrow(/traversal/i); + expect(() => validateRuntimeArgs(["--config=../secret"])).toThrow(/traversal/i); + expect(() => validateRuntimeArgs(["--config=../../secret"])).toThrow(/traversal/i); + // A non-traversal double-dot inside a token is left alone. + expect(() => validateRuntimeArgs(["--range=1..10"])).not.toThrow(); }); it("rejects shell metacharacters in flag values", () => { @@ -1203,8 +1211,17 @@ describe("validateRemoteUrl", () => { expect(() => validateRemoteUrl("https://api.example.com/mcp")).not.toThrow(); }); - it("accepts http URLs", () => { + it("accepts http URLs only for loopback hosts", () => { expect(() => validateRemoteUrl("http://localhost:3000/mcp")).not.toThrow(); + expect(() => validateRemoteUrl("http://127.0.0.1:8080/mcp")).not.toThrow(); + expect(() => validateRemoteUrl("http://app.localhost/mcp")).not.toThrow(); + }); + + it("rejects plaintext http to non-loopback hosts (M4a)", () => { + expect(() => validateRemoteUrl("http://api.example.com/mcp")).toThrow(/https for non-loopback|interception/i); + expect(() => validateRemoteUrl("http://10.0.0.5/mcp")).toThrow(/https for non-loopback|interception/i); + // https to the same host is always fine. + expect(() => validateRemoteUrl("https://api.example.com/mcp")).not.toThrow(); }); it("rejects file:// URLs", () => { diff --git a/src/commands/install.ts b/src/commands/install.ts index 6af5e1c..cf042ec 100644 --- a/src/commands/install.ts +++ b/src/commands/install.ts @@ -43,6 +43,26 @@ export function validateRemoteUrl(url: string): void { `Remote URL must use http or https protocol, got: "${parsed.protocol}"` ); } + // M4a: plaintext http to a non-loopback host is interceptable once written to an + // IDE config. Allow http only for loopback (local dev servers); require https for + // every other host. https is always allowed. + if (parsed.protocol === "http:" && !isLoopbackHost(parsed.hostname)) { + throw new Error( + `Remote URL must use https for non-loopback hosts (plaintext http is ` + + `vulnerable to interception), got: "${url}"` + ); + } +} + +/** True for localhost / loopback literals, where plaintext http is acceptable. */ +function isLoopbackHost(hostname: string): boolean { + const h = hostname.toLowerCase().replace(/^\[|\]$/g, ""); // strip IPv6 brackets + return ( + h === "localhost" || + h.endsWith(".localhost") || + h === "127.0.0.1" || + h === "::1" + ); } const NPM_IDENTIFIER_RE = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; @@ -126,6 +146,16 @@ const SAFE_ARG_PATTERNS: readonly RegExp[] = [ */ export function validateRuntimeArgs(args: string[]): void { for (const arg of args) { + // Layer 0 (M4b): reject a ".." path-traversal segment anywhere in the argument + // — "../x", "a/../../etc/passwd", "--config=../secret". A ".." segment is one + // bounded by start-of-arg, "=" (flag value), or a path separator on the left, + // and a separator or end-of-arg on the right. The Layer-2 allowlist permits "." + // and "/" inside values, so without this a traversal would slip through; a + // non-traversal double dot like "--range=1..10" is left untouched. + if (/(?:^|[=\\/])\.\.(?:[\\/]|$)/.test(arg)) { + throw new Error(`Rejected path traversal in runtime argument: "${arg}"`); + } + // Layer 1: reject dangerous Node.js flags const isDangerous = DANGEROUS_FLAG_PREFIXES.some( (prefix) => arg === prefix || arg.startsWith(`${prefix}=`) @@ -512,8 +542,11 @@ export async function handleInstall( if (!options.json) { if (secretsMode === "keychain" && storedSecretCount > 0) { output( - `\x1b[32mStored ${storedSecretCount} secret(s) encrypted at rest in ~/.mcpm ` + - "(protects against other-user/offline access, not same-user processes). " + + `\x1b[32mStored ${storedSecretCount} secret(s) encrypted at rest in ~/.mcpm. ` + + "With an OS keychain this protects against other-user/offline access (not " + + "same-user processes); without one a machine-derived key is used that guards " + + "casual local inspection only, NOT file exfiltration — run `mcpm secrets migrate` " + + "once a keychain is available. " + "Run `mcpm guard enable` (then restart your IDE) so they resolve at launch — " + "until guard wraps this server it receives the literal placeholder.\x1b[0m" ); diff --git a/src/commands/up.ts b/src/commands/up.ts index 3ab1d1a..a09ea65 100644 --- a/src/commands/up.ts +++ b/src/commands/up.ts @@ -39,7 +39,7 @@ import { } from "../stack/schema.js"; import { checkTrustPolicy } from "../stack/policy.js"; import { parseEnvFile } from "../stack/env.js"; -import { resolveInstallEntry, parseSecretsMode } from "./install.js"; +import { resolveInstallEntry, parseSecretsMode, validateRemoteUrl } from "./install.js"; import { extractRegistryMeta } from "../utils/format-trust.js"; import { applyKeychainSecrets, type SecretsMode, setSecrets as _setSecrets } from "../store/keychain.js"; @@ -240,7 +240,10 @@ export async function handleUp( ); if (options.secrets === "keychain" && totalSecretsStored > 0 && !options.dryRun) { deps.output( - "Secrets stored encrypted at rest in ~/.mcpm (protects against other-user/offline access, not same-user processes). " + + "Secrets stored encrypted at rest in ~/.mcpm. With an OS keychain this protects " + + "against other-user/offline access (not same-user processes); without one a " + + "machine-derived key is used that guards casual local inspection only, NOT file " + + "exfiltration — run `mcpm secrets migrate` once a keychain is available. " + "Run `mcpm guard enable` (then restart your IDE) so they resolve at launch." ); } @@ -423,6 +426,11 @@ async function processUrlServer( }; } + // M4a: validate the URL before it is written to any client config. The up path + // previously wrote stack-file `url:` servers unvalidated; this rejects non-http(s) + // schemes and non-loopback plaintext http (interceptable once in an IDE config). + validateRemoteUrl(url); + const cursorClients = clients.filter((c) => c === "cursor"); if (cursorClients.length === 0) { From 09849491f13174d3abb55dd868efeb8f1ea75bda Mon Sep 17 00:00:00 2001 From: m1ngshum <140998506+m1ngshum@users.noreply.github.com> Date: Mon, 8 Jun 2026 23:44:48 +0800 Subject: [PATCH 2/2] fix(security): address code-review on #66 (dry-run regression + up URL coverage) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-agent review follow-ups: - MEDIUM dry-run regression: validateRemoteUrl ran before the dryRun guard in processUrlServer, so `mcpm up --dry-run` on a bad stack URL threw and exited non-zero on a read-only pass. Capture the validation result instead of throwing: dry-run now previews it as a "would reject URL …" skip (exit zero), and a real run categorizes the bad URL as `blocked` (mirroring trust-policy blocks) rather than a generic `failed`. - HIGH coverage gap: the up-path URL validation had zero integration tests. Add four handleUp cases — valid https installs to Cursor, non-loopback http blocked, non-http(s) scheme blocked, and the dry-run regression (no throw, "would reject"). - Nits: test IPv6 bracket loopback `http://[::1]:8080` (the bracket-strip is load-bearing); document that isLoopbackHost intentionally over-rejects exotic loopback spellings (never a bypass). tsc + tsup clean; full suite green (1302). --- src/__tests__/commands/install.test.ts | 2 + src/__tests__/commands/up.test.ts | 70 ++++++++++++++++++++++++++ src/commands/install.ts | 8 ++- src/commands/up.ts | 18 ++++++- 4 files changed, 95 insertions(+), 3 deletions(-) diff --git a/src/__tests__/commands/install.test.ts b/src/__tests__/commands/install.test.ts index 94f3911..2f981c3 100644 --- a/src/__tests__/commands/install.test.ts +++ b/src/__tests__/commands/install.test.ts @@ -1215,6 +1215,8 @@ describe("validateRemoteUrl", () => { expect(() => validateRemoteUrl("http://localhost:3000/mcp")).not.toThrow(); expect(() => validateRemoteUrl("http://127.0.0.1:8080/mcp")).not.toThrow(); expect(() => validateRemoteUrl("http://app.localhost/mcp")).not.toThrow(); + // IPv6 loopback — the bracket-strip in isLoopbackHost is load-bearing. + expect(() => validateRemoteUrl("http://[::1]:8080/mcp")).not.toThrow(); }); it("rejects plaintext http to non-loopback hosts (M4a)", () => { diff --git a/src/__tests__/commands/up.test.ts b/src/__tests__/commands/up.test.ts index e062b7d..11c27f0 100644 --- a/src/__tests__/commands/up.test.ts +++ b/src/__tests__/commands/up.test.ts @@ -515,4 +515,74 @@ servers: expect(confirm).not.toHaveBeenCalled(); expect(adapter.removeServer).toHaveBeenCalledWith("/mock/config.json", "extra-server"); }); + + // M4a (PR #66): the up path now validates stack-file `url:` servers before writing. + // These exercise processUrlServer end-to-end (previously untested at this level). + const urlStack = (url: string) => ` +version: "1" +servers: + io.github.test/remote: + url: "${url}" +`; + + it("installs a valid https url: server to Cursor", async () => { + const stackPath = await writeStackAndLock(urlStack("https://api.example.com/mcp"), basicLock); + const adapter = makeAdapter(); + const deps = makeDeps({ + detectClients: vi.fn<() => Promise>().mockResolvedValue(["cursor"]), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await handleUp({ stackFile: stackPath }, deps); + + expect(adapter.addServer).toHaveBeenCalledWith( + "/mock/config.json", + "io.github.test/remote", + { url: "https://api.example.com/mcp" }, + { force: true } + ); + }); + + it("blocks a url: server with a non-loopback plaintext-http URL (M4a)", async () => { + const stackPath = await writeStackAndLock(urlStack("http://10.0.0.5/mcp"), basicLock); + const adapter = makeAdapter(); + const deps = makeDeps({ + detectClients: vi.fn<() => Promise>().mockResolvedValue(["cursor"]), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await expect(handleUp({ stackFile: stackPath }, deps)).rejects.toThrow(/could not be installed/); + expect(adapter.addServer).not.toHaveBeenCalled(); + }); + + it("blocks a url: server with a non-http(s) scheme", async () => { + const stackPath = await writeStackAndLock(urlStack("file:///etc/passwd"), basicLock); + const adapter = makeAdapter(); + const deps = makeDeps({ + detectClients: vi.fn<() => Promise>().mockResolvedValue(["cursor"]), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await expect(handleUp({ stackFile: stackPath }, deps)).rejects.toThrow(/could not be installed/); + expect(adapter.addServer).not.toHaveBeenCalled(); + }); + + // Regression: --dry-run is read-only — an invalid URL must NOT throw or exit + // non-zero; it is previewed as "would reject". + it("dry-run does not fail on an invalid url: server (regression)", async () => { + const stackPath = await writeStackAndLock(urlStack("http://10.0.0.5/mcp"), basicLock); + const adapter = makeAdapter(); + const deps = makeDeps({ + detectClients: vi.fn<() => Promise>().mockResolvedValue(["cursor"]), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await expect( + handleUp({ stackFile: stackPath, dryRun: true }, deps) + ).resolves.toBeUndefined(); + expect(adapter.addServer).not.toHaveBeenCalled(); + + const out = (deps.output as ReturnType).mock.calls.map((c) => c[0]).join("\n"); + expect(out).toMatch(/would reject URL/); + }); }); diff --git a/src/commands/install.ts b/src/commands/install.ts index cf042ec..5d5af67 100644 --- a/src/commands/install.ts +++ b/src/commands/install.ts @@ -54,7 +54,13 @@ export function validateRemoteUrl(url: string): void { } } -/** True for localhost / loopback literals, where plaintext http is acceptable. */ +/** + * True for localhost / loopback literals, where plaintext http is acceptable. + * Recognizes localhost / *.localhost / 127.0.0.1 / ::1. Exotic loopback spellings + * (IPv4-mapped `::ffff:127.0.0.1`, `127.x.x.x`, decimal/octal/hex IPs) are NOT + * recognized and fall through to the https requirement — over-rejection only, never + * a bypass (a non-loopback host can never be mistaken for loopback). + */ function isLoopbackHost(hostname: string): boolean { const h = hostname.toLowerCase().replace(/^\[|\]$/g, ""); // strip IPv6 brackets return ( diff --git a/src/commands/up.ts b/src/commands/up.ts index a09ea65..f987394 100644 --- a/src/commands/up.ts +++ b/src/commands/up.ts @@ -429,7 +429,15 @@ async function processUrlServer( // M4a: validate the URL before it is written to any client config. The up path // previously wrote stack-file `url:` servers unvalidated; this rejects non-http(s) // schemes and non-loopback plaintext http (interceptable once in an IDE config). - validateRemoteUrl(url); + // Capture (don't throw): `--dry-run` must stay a read-only, exit-zero preview, and + // an invalid URL is categorized as `blocked` (mirroring trust-policy blocks) rather + // than crashing the per-server loop into a generic `failed`. + let urlError: string | undefined; + try { + validateRemoteUrl(url); + } catch (err) { + urlError = err instanceof Error ? err.message : String(err); + } const cursorClients = clients.filter((c) => c === "cursor"); @@ -442,7 +450,13 @@ async function processUrlServer( } if (options.dryRun) { - return { name, status: "skipped", message: `would install URL ${url} to Cursor` }; + return urlError + ? { name, status: "skipped", message: `would reject URL ${url}: ${urlError}` } + : { name, status: "skipped", message: `would install URL ${url} to Cursor` }; + } + + if (urlError) { + return { name, status: "blocked", message: urlError }; } for (const clientId of cursorClients) {