diff --git a/server/__tests__/codex-cli.test.js b/server/__tests__/codex-cli.test.js index 03a6a09a..e0802ef8 100644 --- a/server/__tests__/codex-cli.test.js +++ b/server/__tests__/codex-cli.test.js @@ -62,6 +62,45 @@ describe("codex-cli argument validation", () => { assert.doesNotMatch("--force", cli.NAME_RE); assert.doesNotMatch("../../etc", cli.NAME_RE); }); + + it("accepts and @ plugin specs only", () => { + assert.equal(cli.assertPluginSpec("agenthub"), "agenthub"); + assert.equal(cli.assertPluginSpec("agenthub@sg"), "agenthub@sg"); + for (const bad of ["../agenthub", "agenthub agenthub", "@sg", "-agenthub", "a/b"]) { + assert.throws(() => cli.assertPluginSpec(bad), undefined, `accepted ${bad}`); + } + }); + + it("accepts only http(s) URLs", () => { + assert.equal(cli.assertUrl("https://example.com/mp.git", "url"), "https://example.com/mp.git"); + for (const bad of ["ssh://example.com/mp.git", "not a url", "file:///tmp/x"]) { + assert.throws(() => cli.assertUrl(bad, "url"), /http\(s\)/, `accepted ${bad}`); + } + // rejected earlier, by the shared leading-dash guard, not the URL check + assert.throws(() => cli.assertUrl("--force", "url"), /may not start with/); + }); + + it("rejects mcpAdd with neither or both of url/command", async () => { + await assert.rejects(() => cli.mcpAdd({ name: "docs" }), /requires either/); + await assert.rejects( + () => cli.mcpAdd({ name: "docs", url: "https://x", command: "node" }), + /either url or command, not both/ + ); + }); + + it("rejects env together with url — --env is invalid on the url form", async () => { + await assert.rejects( + () => cli.mcpAdd({ name: "docs", url: "https://x", env: { A: "1" } }), + /stdio-only/ + ); + }); + + it("rejects an env key that is not a valid identifier", async () => { + await assert.rejects( + () => cli.mcpAdd({ name: "docs", command: "node", env: { "1bad": "1" } }), + /identifier/ + ); + }); }); describe("codex-cli argv construction", () => { @@ -75,6 +114,54 @@ describe("codex-cli argv construction", () => { } }); + it("puts --env flags between NAME and -- for a stdio server", async () => { + const restore = stubCodex(); + try { + const result = await cli.mcpAdd({ + name: "docs", + command: "node", + args: ["server.js"], + env: { API_KEY: "secret", MODE: "prod" }, + }); + assert.deepEqual(argvOf(result), [ + "mcp", + "add", + "docs", + "--env", + "API_KEY=secret", + "--env", + "MODE=prod", + "--", + "node", + "server.js", + ]); + } finally { + restore(); + } + }); + + it("builds the --url form with an optional --bearer-token-env-var", async () => { + const restore = stubCodex(); + try { + const result = await cli.mcpAdd({ + name: "docs", + url: "https://example.com/mcp", + bearerTokenEnvVar: "DOCS_TOKEN", + }); + assert.deepEqual(argvOf(result), [ + "mcp", + "add", + "docs", + "--bearer-token-env-var", + "DOCS_TOKEN", + "--url", + "https://example.com/mcp", + ]); + } finally { + restore(); + } + }); + it("builds mcp list / remove argvs", async () => { const restore = stubCodex(); try { diff --git a/server/lib/codex-cli.js b/server/lib/codex-cli.js index c2b7940a..ba00d271 100644 --- a/server/lib/codex-cli.js +++ b/server/lib/codex-cli.js @@ -14,7 +14,8 @@ * server/plugin/marketplace name comes straight from a user-typed field. * - A name is validated against `NAME_RE` BEFORE spawning: no leading `-` * (never a flag), no `/` (never a path). This is the same guard the - * HTTP routes re-check. + * HTTP routes re-check. Plugin specs and URLs get their own purpose-built + * guards (`assertPluginSpec`, `assertUrl`) — same idea, looser shape. * - Mutating calls are serialized in-process; two dashboard clicks must * not interleave writes to the same config.toml. * - `CODEX_HOME` is pinned to the same home `codex-discovery` reads, so @@ -35,6 +36,10 @@ const MAX_OUTPUT_BYTES = 1024 * 1024; /** An MCP server / plugin / marketplace name. No slashes: never a path. */ const NAME_RE = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; +/** `name` or `name@marketplace` — same shape as cc-plugin-cli's PLUGIN_SPEC_RE. */ +const PLUGIN_SPEC_RE = /^[A-Za-z0-9][A-Za-z0-9._-]*(@[A-Za-z0-9][A-Za-z0-9._-]*)?$/; +/** An environment variable name — used for both `--env K=V` and `--bearer-token-env-var`. */ +const ENV_KEY_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; function makeError(code, message) { const err = new Error(message); @@ -69,6 +74,39 @@ function assertName(value, label = "name") { return value; } +/** `` or `@`, as `codex plugin add` accepts. */ +function assertPluginSpec(value) { + assertArg(value, "plugin spec"); + if (!PLUGIN_SPEC_RE.test(value)) { + throw makeError("EBADARG", "plugin spec must be or @"); + } + return value; +} + +/** An environment variable NAME (never a literal secret). */ +function assertEnvKey(value, label) { + assertArg(value, label); + if (!ENV_KEY_RE.test(value)) { + throw makeError("EBADARG", `${label} is not a valid identifier`); + } + return value; +} + +/** An http(s) URL — `new URL()` both validates and normalizes the parse. */ +function assertUrl(value, label) { + assertArg(value, label); + let parsed; + try { + parsed = new URL(value); + } catch { + throw makeError("EBADARG", `${label} must be a valid http(s) URL`); + } + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { + throw makeError("EBADARG", `${label} must be a valid http(s) URL`); + } + return value; +} + // One in-process queue for every mutating CLI call — the same reasoning as // cc-plugin-cli.js's `mutationQueue`: this dashboard is the only Node writer, // so a promise chain is the whole locking story. It does NOT protect against @@ -146,16 +184,37 @@ async function mcpList() { return runCodex(["mcp", "list", "--json"]); } -async function mcpAdd({ name, command, args = [], env }) { +/** + * `codex mcp add` has two mutually exclusive forms: a streamable HTTP server + * (`--url`, optionally `--bearer-token-env-var`) or a stdio server + * (`-- `, optionally `--env K=V`). `--env` is invalid on + * the `--url` form per the CLI's own `--help`, so `{url, env}` together is + * rejected rather than silently dropping one of them. + */ +async function mcpAdd({ name, url, bearerTokenEnvVar, command, args = [], env }) { const n = assertName(name); + if (url && command) { + throw makeError("EBADARG", "mcpAdd accepts either url or command, not both"); + } + if (!url && !command) { + throw makeError("EBADARG", "mcpAdd requires either url or command"); + } + + if (url) { + if (env) throw makeError("EBADARG", "env is not valid together with url (stdio-only)"); + const u = assertUrl(url, "url"); + const flags = bearerTokenEnvVar + ? ["--bearer-token-env-var", assertEnvKey(bearerTokenEnvVar, "bearerTokenEnvVar")] + : []; + return serialize(() => runCodex(["mcp", "add", n, ...flags, "--url", u])); + } + const cmd = assertArg(command, "command"); const cmdArgs = args.map((a, i) => assertArg(a, `args[${i}]`)); const envFlags = Object.entries(env || {}).flatMap(([k, v]) => { - if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(k)) { - throw makeError("EBADARG", `env key "${k}" is not a valid identifier`); - } + const key = assertEnvKey(k, "env key"); assertArg(String(v), `env.${k}`); - return ["--env", `${k}=${v}`]; + return ["--env", `${key}=${v}`]; }); return serialize(() => runCodex(["mcp", "add", n, ...envFlags, "--", cmd, ...cmdArgs])); } @@ -172,7 +231,7 @@ async function pluginList() { } async function pluginAdd(spec) { - const s = assertArg(spec, "plugin spec"); + const s = assertPluginSpec(spec); return serialize(() => runCodex(["plugin", "add", s])); } @@ -188,7 +247,7 @@ async function marketplaceList() { } async function marketplaceAdd(url) { - const u = assertArg(url, "marketplace url"); + const u = assertUrl(url, "marketplace url"); return serialize(() => runCodex(["plugin", "marketplace", "add", u])); } @@ -209,4 +268,7 @@ module.exports = { marketplaceAdd, marketplaceRemove, NAME_RE, + PLUGIN_SPEC_RE, + assertPluginSpec, + assertUrl, };