fix(codex): support mcp add --url form, guard specs/URLs, cover --env

2026-09-08 17:15:24 +07:00
parent ba79610697
commit dcd65f891c
2 changed files with 157 additions and 8 deletions
@@ -62,6 +62,45 @@ describe("codex-cli argument validation", () => {
    assert.doesNotMatch("--force", cli.NAME_RE);
    assert.doesNotMatch("../../etc", cli.NAME_RE);
  });
+
+  it("accepts <name> and <name>@<marketplace> 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 {
@@ -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;
 }

+/** `<name>` or `<name>@<marketplace>`, 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 <name> or <name>@<marketplace>");
+  }
+  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
+ * (`-- <command> <args…>`, 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,
 };