diff --git a/server/__tests__/codex-cli.test.js b/server/__tests__/codex-cli.test.js new file mode 100644 index 00000000..03a6a09a --- /dev/null +++ b/server/__tests__/codex-cli.test.js @@ -0,0 +1,202 @@ +/** + * @file Tests for server/lib/codex-cli.js — the `codex mcp`/`codex plugin` + * wrapper that backs the Codex config panel's MCP-server and plugin + * management. Uses a fake `codex` script on PATH (the fixture style + * server/__tests__/cc-plugin-cli.test.js uses) rather than mocking + * child_process, and pins the exact argv each operation builds — that argv + * IS the security boundary. + * @author Nguyễn Ngọc Trí Vĩ + */ +const { describe, it, after } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); + +const ROOT = fs.mkdtempSync(path.join(os.tmpdir(), "agenthub-codexcli-")); +const cli = require("../lib/codex-cli"); + +/** + * Put a fake `codex` on PATH. The default body echoes the argv, one element + * per line, so a test can assert the exact array that was spawned. + */ +function stubCodex(scriptBody = 'for a in "$@"; do echo "$a"; done') { + const bin = path.join(ROOT, `bin-${process.hrtime.bigint()}`); + fs.mkdirSync(bin, { recursive: true }); + const file = path.join(bin, "codex"); + fs.writeFileSync(file, `#!/usr/bin/env bash\n${scriptBody}\n`); + fs.chmodSync(file, 0o755); + const originalPath = process.env.PATH; + process.env.PATH = `${bin}${path.delimiter}${originalPath}`; + return () => { + process.env.PATH = originalPath; + }; +} + +/** PATH with no `codex` on it at all. */ +function stubMissingCodex() { + const empty = path.join(ROOT, `empty-${process.hrtime.bigint()}`); + fs.mkdirSync(empty, { recursive: true }); + const originalPath = process.env.PATH; + process.env.PATH = empty; + return () => { + process.env.PATH = originalPath; + }; +} + +const argvOf = (result) => result.stdout.trim().split("\n"); + +after(() => fs.rmSync(ROOT, { recursive: true, force: true, maxRetries: 5 })); + +describe("codex-cli argument validation", () => { + it("refuses a name that could be read as a flag or a path", async () => { + await assert.rejects(() => cli.mcpRemove("--force"), /name/i); + await assert.rejects(() => cli.mcpRemove("../../etc"), /name/i); + await assert.rejects(() => cli.pluginRemove("--force"), /name/i); + await assert.rejects(() => cli.marketplaceRemove("--force"), /name/i); + await assert.rejects(() => cli.mcpAdd({ name: "--force", command: "node" }), /name/i); + }); + + it("accepts a plain identifier as a name", () => { + assert.match("docs", cli.NAME_RE); + assert.doesNotMatch("--force", cli.NAME_RE); + assert.doesNotMatch("../../etc", cli.NAME_RE); + }); +}); + +describe("codex-cli argv construction", () => { + it("passes the server name as one argv entry, never through a shell", async () => { + const restore = stubCodex(); + try { + const result = await cli.mcpAdd({ name: "docs", command: "node", args: ["server.js"] }); + assert.deepEqual(argvOf(result), ["mcp", "add", "docs", "--", "node", "server.js"]); + } finally { + restore(); + } + }); + + it("builds mcp list / remove argvs", async () => { + const restore = stubCodex(); + try { + assert.deepEqual(argvOf(await cli.mcpList()), ["mcp", "list", "--json"]); + assert.deepEqual(argvOf(await cli.mcpRemove("docs")), ["mcp", "remove", "docs"]); + } finally { + restore(); + } + }); + + it("builds plugin list / add / remove argvs", async () => { + const restore = stubCodex(); + try { + assert.deepEqual(argvOf(await cli.pluginList()), ["plugin", "list", "--json"]); + assert.deepEqual(argvOf(await cli.pluginAdd("demo@sg")), ["plugin", "add", "demo@sg"]); + assert.deepEqual(argvOf(await cli.pluginRemove("demo")), ["plugin", "remove", "demo"]); + } finally { + restore(); + } + }); + + it("builds marketplace list / add / remove argvs, nested under plugin", async () => { + const restore = stubCodex(); + try { + assert.deepEqual(argvOf(await cli.marketplaceList()), [ + "plugin", + "marketplace", + "list", + "--json", + ]); + assert.deepEqual(argvOf(await cli.marketplaceAdd("https://example.com/mp.git")), [ + "plugin", + "marketplace", + "add", + "https://example.com/mp.git", + ]); + assert.deepEqual(argvOf(await cli.marketplaceRemove("sg")), [ + "plugin", + "marketplace", + "remove", + "sg", + ]); + } finally { + restore(); + } + }); + + it("never lets a rejected name reach a shell", async () => { + // If the value were interpolated into a shell string this would create + // the file; the NAME_RE guard rejects it before any spawn happens. + const canary = path.join(ROOT, "pwned"); + const restore = stubCodex(); + try { + await assert.rejects(() => cli.mcpRemove("docs; touch " + canary), /name/i); + assert.equal(fs.existsSync(canary), false); + } finally { + restore(); + } + }); +}); + +describe("codex-cli run outcomes", () => { + it("surfaces a non-zero exit as ok:false with stderr kept", async () => { + const restore = stubCodex('echo "no such marketplace" >&2\nexit 2'); + try { + const res = await cli.marketplaceRemove("nope"); + assert.equal(res.ok, false); + assert.equal(res.code, 2); + assert.match(res.stderr, /no such marketplace/); + } finally { + restore(); + } + }); + + it("throws a clear error when the codex CLI is not on PATH", async () => { + const restore = stubMissingCodex(); + try { + await assert.rejects( + () => cli.mcpList(), + (err) => { + assert.equal(err.code, "ECLINOTFOUND"); + assert.match(err.message, /not found on PATH/); + return true; + } + ); + } finally { + restore(); + } + }); + + it("parses JSON output into `data`", async () => { + const restore = stubCodex(`cat <<'JSON'\n{"servers":[{"name":"docs"}]}\nJSON`); + try { + const res = await cli.mcpList(); + assert.equal(res.ok, true); + assert.deepEqual(res.data, { servers: [{ name: "docs" }] }); + } finally { + restore(); + } + }); + + it("leaves `data` undefined when stdout is not JSON", async () => { + const restore = stubCodex('echo "removed docs"'); + try { + const res = await cli.mcpRemove("docs"); + assert.equal(res.ok, true); + assert.equal(res.data, undefined); + } finally { + restore(); + } + }); + + it("serializes mutating calls so two clicks cannot interleave", async () => { + const log = path.join(ROOT, `order-${process.hrtime.bigint()}.txt`); + const restore = stubCodex(`echo "start $2" >> ${log}\nsleep 0.2\necho "end $2" >> ${log}`); + try { + await Promise.all([cli.mcpRemove("first"), cli.mcpRemove("second")]); + const lines = fs.readFileSync(log, "utf8").trim().split("\n"); + assert.equal(lines[0].replace("start ", ""), lines[1].replace("end ", "")); + assert.equal(lines[2].replace("start ", ""), lines[3].replace("end ", "")); + } finally { + restore(); + } + }); +}); diff --git a/server/lib/codex-cli.js b/server/lib/codex-cli.js new file mode 100644 index 00000000..c2b7940a --- /dev/null +++ b/server/lib/codex-cli.js @@ -0,0 +1,212 @@ +/** + * @file codex-cli.js + * @description Drives the official `codex mcp …` / `codex plugin …` CLI so + * the Codex config panel can add/remove MCP servers, marketplaces and + * plugins without the dashboard touching Codex's TOML config directly. + * + * Why the CLI and not direct TOML edits: the CLI owns config.toml's schema + * (MCP server table, plugin/marketplace registration) and rewrites it in + * place. Reproducing that here would drift the moment the CLI's format + * moves — the same reasoning `cc-plugin-cli.js` follows for `claude plugin`. + * + * Hard constraints (do not relax without a follow-up review): + * - `execFile` with an explicit argv array. NEVER a shell string — a + * 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. + * - 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 + * the CLI writes where the dashboard looks. + * + * A non-zero CLI exit resolves as `{ok: false}` (data, not an error) so a + * route can map it to HTTP 422. Only a rejected name or a missing binary + * throws — those are programming/environment errors, not CLI outcomes. + * + * @author Nguyễn Ngọc Trí Vĩ + */ + +const { execFile } = require("node:child_process"); +const { getCodexHome } = require("./codex-home"); + +const DEFAULT_TIMEOUT_MS = 120_000; +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._-]*$/; + +function makeError(code, message) { + const err = new Error(message); + err.code = code; + return err; +} + +/** + * Baseline check applied to every argv element we interpolate. A value that + * starts with `-` would be parsed as a flag by the CLI even inside an argv + * array, so it is rejected rather than escaped. + */ +function assertArg(value, label) { + if (typeof value !== "string" || !value.trim()) { + throw makeError("EBADARG", `${label} is required`); + } + if (value.length > 512) throw makeError("EBADARG", `${label} is too long`); + // eslint-disable-next-line no-control-regex + if (/[\u0000-\u001f\u007f]/.test(value)) { + throw makeError("EBADARG", `${label} contains control characters`); + } + if (value.startsWith("-")) throw makeError("EBADARG", `${label} may not start with "-"`); + return value; +} + +/** An MCP server / plugin / marketplace identifier — the routes' shared guard. */ +function assertName(value, label = "name") { + assertArg(value, label); + if (!NAME_RE.test(value)) { + throw makeError("EBADARG", `${label} is not a valid identifier`); + } + 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 +// a human running `codex …` in a terminal at the same moment. +let mutationQueue = Promise.resolve(); + +function serialize(fn) { + const run = mutationQueue.then(fn, fn); + mutationQueue = run.then( + () => undefined, + () => undefined + ); + return run; +} + +/** + * Run `codex ` and resolve with the outcome. A non-zero exit is a + * normal result (`ok:false`), not a rejection. When stdout parses as JSON + * the parsed value is attached as `data`. + * + * @param {string[]} args argv AFTER the binary name; every element pre-validated. + * @param {{timeoutMs?:number}} [opts] + * @returns {Promise<{ok:boolean, code:number|null, stdout:string, stderr:string, data?:object}>} + */ +function runCodex(args, opts = {}) { + const timeout = opts.timeoutMs || DEFAULT_TIMEOUT_MS; + return new Promise((resolve, reject) => { + const child = execFile( + "codex", + args, + { + timeout, + maxBuffer: MAX_OUTPUT_BYTES, + // Pin the CLI to the same home codex-discovery reads, so a write + // made here is a write the dashboard can see. + env: { ...process.env, CODEX_HOME: getCodexHome() }, + }, + (err, stdout, stderr) => { + const out = String(stdout || ""); + const errOut = String(stderr || ""); + if (err && (err.code === "ENOENT" || err.code === "EACCES")) { + return reject( + makeError( + "ECLINOTFOUND", + "the `codex` CLI was not found on PATH — install Codex or add it to the dashboard's PATH" + ) + ); + } + if (err && err.killed) { + return reject(makeError("ETIMEOUT", `codex ${args[0]} timed out after ${timeout}ms`)); + } + const code = err ? (typeof err.code === "number" ? err.code : 1) : 0; + const result = { ok: !err, code, stdout: out, stderr: errOut }; + try { + result.data = JSON.parse(out.trim()); + } catch { + /* not every command emits JSON */ + } + resolve(result); + } + ); + // Every command here is non-interactive. Closing stdin turns a would-be + // prompt into an immediate CLI error instead of a hang until `timeout`. + try { + child.stdin?.end(); + } catch { + /* the child may already have exited */ + } + }); +} + +// ── MCP servers ──────────────────────────────────────────────────────── + +async function mcpList() { + return runCodex(["mcp", "list", "--json"]); +} + +async function mcpAdd({ name, command, args = [], env }) { + const n = assertName(name); + 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`); + } + assertArg(String(v), `env.${k}`); + return ["--env", `${k}=${v}`]; + }); + return serialize(() => runCodex(["mcp", "add", n, ...envFlags, "--", cmd, ...cmdArgs])); +} + +async function mcpRemove(name) { + const n = assertName(name); + return serialize(() => runCodex(["mcp", "remove", n])); +} + +// ── Plugins ──────────────────────────────────────────────────────────── + +async function pluginList() { + return runCodex(["plugin", "list", "--json"]); +} + +async function pluginAdd(spec) { + const s = assertArg(spec, "plugin spec"); + return serialize(() => runCodex(["plugin", "add", s])); +} + +async function pluginRemove(name) { + const n = assertName(name); + return serialize(() => runCodex(["plugin", "remove", n])); +} + +// ── Marketplaces ─────────────────────────────────────────────────────── + +async function marketplaceList() { + return runCodex(["plugin", "marketplace", "list", "--json"]); +} + +async function marketplaceAdd(url) { + const u = assertArg(url, "marketplace url"); + return serialize(() => runCodex(["plugin", "marketplace", "add", u])); +} + +async function marketplaceRemove(name) { + const n = assertName(name); + return serialize(() => runCodex(["plugin", "marketplace", "remove", n])); +} + +module.exports = { + runCodex, + mcpList, + mcpAdd, + mcpRemove, + pluginList, + pluginAdd, + pluginRemove, + marketplaceList, + marketplaceAdd, + marketplaceRemove, + NAME_RE, +};