diff --git a/src/integrations.mjs b/src/integrations.mjs index 186b16b..b05bc0a 100644 --- a/src/integrations.mjs +++ b/src/integrations.mjs @@ -88,6 +88,19 @@ export function parseMcp(tokens) { } } + // A token still starting with `-` at this point was never consumed as a flag, + // so it is a typo or an engine-native flag moshcode does not take (`-s user`). + // Left alone it becomes the server NAME or its command and gets spliced + // straight into every engine's own `mcp add` argv. Everything after `--` is + // the user's command line and is deliberately not second-guessed. + const stray = [name, cmdParts ? null : target] + .find((t) => typeof t === "string" && t.startsWith("-")); + if (stray) { + return { + error: `unknown mcp flag "${stray}" — mcp takes --name, -t/--transport, -e/--env, and -H/--header; put a command's own flags after --`, + }; + } + if (verb === "install" && !name) { if (target && isRemoteTarget(target)) name = deriveName(target); else return { error: "a stdio command server needs an explicit --name" }; diff --git a/test/mcp-stray-flag.test.mjs b/test/mcp-stray-flag.test.mjs new file mode 100644 index 0000000..52ce561 --- /dev/null +++ b/test/mcp-stray-flag.test.mjs @@ -0,0 +1,122 @@ +// An unsupported flag before `--` used to be swallowed as a positional, which +// meant it became the server NAME (and its value became the command). The spec +// is spliced verbatim into every engine's native `mcp add` argv, so +// `mcp add -s user https://…` shipped `claude mcp add -s user -s -- user https://…` +// to Claude: a server called `-s`, a command called `user`, and the real URL +// demoted to an argument. Reject the stray flag instead — but only before `--`, +// because everything after it is the user's own command line. +import test from "node:test"; +import assert from "node:assert/strict"; + +import { parseMcp } from "../src/integrations.mjs"; +import { planMcpAdd } from "../src/mcp.mjs"; + +// ---------- the bug ---------- + +test("an engine-native scope flag is rejected, not turned into the server name", () => { + const { error, spec } = parseMcp(["add", "-s", "user", "https://mcp.sentry.dev/mcp"]); + assert.equal(spec, undefined); + assert.match(error, /unknown mcp flag "-s"/); +}); + +test("a long unsupported flag is rejected too", () => { + const { error, spec } = parseMcp(["add", "--scope", "user", "https://mcp.example.com/mcp"]); + assert.equal(spec, undefined); + assert.match(error, /unknown mcp flag "--scope"/); +}); + +test("a misspelled supported flag is rejected rather than silently accepted", () => { + const { error } = parseMcp(["add", "--transportt", "http", "https://mcp.example.com/mcp"]); + assert.match(error, /unknown mcp flag "--transportt"/); +}); + +test("a stray flag in the command position is rejected", () => { + const { error, spec } = parseMcp(["add", "tools", "--verbose", "npx"]); + assert.equal(spec, undefined); + assert.match(error, /unknown mcp flag "--verbose"/); +}); + +test("install reports the stray flag, not a misleading missing-name error", () => { + const { error } = parseMcp(["install", "--scope", "user", "https://mcp.example.com/mcp"]); + assert.match(error, /unknown mcp flag "--scope"/); + assert.doesNotMatch(error, /explicit --name/); +}); + +test("the error names the flags mcp does take, and points at --", () => { + const { error } = parseMcp(["add", "-s", "user", "https://mcp.example.com/mcp"]); + for (const flag of ["--name", "--transport", "--env", "--header"]) { + assert.ok(error.includes(flag), `expected the error to mention ${flag}`); + } + assert.match(error, /after --/); +}); + +test("no stray flag ever reaches an engine's native argv", () => { + // The end-to-end consequence: nothing to plan, so nothing to splice. + const parsed = parseMcp(["add", "-s", "user", "https://mcp.sentry.dev/mcp"]); + assert.equal(parsed.spec, undefined); + assert.throws(() => planMcpAdd(parsed.spec)); +}); + +test("a stray flag cannot smuggle a bogus name past the catalog lookup", () => { + const { error } = parseMcp(["add", "--scope", "porkbun"]); + assert.match(error, /unknown mcp flag "--scope"/); +}); + +// ---------- controls: the opposite direction ---------- + +test("a command's own flags after -- are still passed through untouched", () => { + const { spec } = parseMcp(["add", "tools", "--", "npx", "-y", "srv", "--port", "3000"]); + assert.equal(spec.name, "tools"); + assert.equal(spec.target, "npx"); + assert.deepEqual(spec.args, ["-y", "srv", "--port", "3000"]); +}); + +test("a command's own flags after the target are still arguments, not strays", () => { + const { spec, error } = parseMcp(["add", "tools", "npx", "-y", "srv"]); + assert.equal(error, undefined); + assert.equal(spec.target, "npx"); + assert.deepEqual(spec.args, ["-y", "srv"]); +}); + +test("every supported flag still parses exactly as before", () => { + const { spec } = parseMcp([ + "install", "https://x.dev/mcp", + "--name", "x", "-t", "http", "-e", "K=v", "-H", "A: b", + ]); + assert.deepEqual(spec, { + name: "x", + target: "https://x.dev/mcp", + args: [], + transport: "http", + env: [["K", "v"]], + headers: ["A: b"], + }); +}); + +test("the catalog shortcut is unaffected", () => { + const { spec, catalog, error } = parseMcp(["add", "porkbun"]); + assert.equal(error, undefined); + assert.equal(catalog.key, "porkbun"); + assert.equal(spec.name, "porkbun"); + assert.equal(spec.target, "npx"); +}); + +test("a bare URL install still derives its name", () => { + const { spec, error } = parseMcp(["install", "https://mcp.sentry.dev/mcp"]); + assert.equal(error, undefined); + assert.equal(spec.target, "https://mcp.sentry.dev/mcp"); + assert.ok(spec.name); +}); + +test("the pre-existing missing-value guards still fire first", () => { + assert.match(parseMcp(["install", "https://x.dev/mcp", "--name"]).error, /--name requires a value/); + assert.match(parseMcp(["install", "https://x.dev/mcp", "--transport", "--"]).error, /--transport requires a value/); +}); + +test("an unknown verb still reports an unknown verb, not an unknown flag", () => { + assert.match(parseMcp(["bogus"]).error, /unknown mcp verb/); +}); + +test("a stdio install with no name still asks for --name", () => { + assert.match(parseMcp(["install", "--", "npx", "srv"]).error, /explicit --name/); +});