feat(codex): wrap the codex mcp/plugin CLI in argv-safe calls
This commit is contained in:
@@ -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ĩ <vinnt@smartgift.vn>
|
||||
*/
|
||||
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();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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ĩ <vinnt@smartgift.vn>
|
||||
*/
|
||||
|
||||
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 <args…>` 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,
|
||||
};
|
||||
Reference in New Issue
Block a user