diff --git a/src/__tests__/commands/install.test.ts b/src/__tests__/commands/install.test.ts index ebe8665..2f981c3 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,19 @@ 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(); + // 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)", () => { + 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/__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 6af5e1c..5d5af67 100644 --- a/src/commands/install.ts +++ b/src/commands/install.ts @@ -43,6 +43,32 @@ 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. + * 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 ( + 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 +152,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 +548,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..f987394 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,19 @@ 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). + // 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"); if (cursorClients.length === 0) { @@ -434,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) {