fix(codex): close symlink escape on GET /file, pin project writes to server cwd

server/__tests__/codex-config.test.js

@@ -211,6 +211,42 @@ describe("/api/codex-config", () => {
     assert.equal(body.error.code, "READ_DENIED");
   });
 
+  it("file endpoint refuses a leaf symlink that escapes CODEX_HOME (no secret leaks)", async () => {
+    const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-config-outside-"));
+    fs.writeFileSync(path.join(outside, "secret.rules"), "TOP SECRET LEAF");
+    const linkPath = path.join(FAKE_HOME, "rules", "leaf-link.rules");
+    fs.symlinkSync(path.join(outside, "secret.rules"), linkPath, "file");
+    try {
+      const { status, body } = await fetchJson(
+        `/api/codex-config/file?cwd=${encodeURIComponent(FAKE_PROJECT)}&path=${encodeURIComponent(linkPath)}`
+      );
+      assert.equal(status, 400);
+      assert.equal(body.error.code, "READ_DENIED");
+      assert.doesNotMatch(JSON.stringify(body), /TOP SECRET LEAF/);
+    } finally {
+      fs.rmSync(outside, { recursive: true, force: true });
+    }
+  });
+
+  it("file endpoint refuses an ancestor symlink that escapes CODEX_HOME (no secret leaks)", async () => {
+    const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-config-outside-"));
+    fs.writeFileSync(path.join(outside, "secret.txt"), "TOP SECRET ANCESTOR");
+    const linkDir = path.join(FAKE_HOME, "evil-dir");
+    fs.symlinkSync(outside, linkDir, "dir");
+    try {
+      const target = path.join(linkDir, "secret.txt");
+      const { status, body } = await fetchJson(
+        `/api/codex-config/file?cwd=${encodeURIComponent(FAKE_PROJECT)}&path=${encodeURIComponent(target)}`
+      );
+      assert.equal(status, 400);
+      assert.equal(body.error.code, "READ_DENIED");
+      assert.doesNotMatch(JSON.stringify(body), /TOP SECRET ANCESTOR/);
+    } finally {
+      fs.rmSync(linkDir, { force: true });
+      fs.rmSync(outside, { recursive: true, force: true });
+    }
+  });
+
   it("file endpoint requires a path", async () => {
     const { status, body } = await fetchJson("/api/codex-config/file");
     assert.equal(status, 400);
@@ -267,6 +303,56 @@ describe("/api/codex-config", () => {
     );
   });
 
+  it("PUT /file ignores ?cwd= for project scope — writes to the server's own cwd, not a caller-chosen dir", async () => {
+    const victim = fs.mkdtempSync(path.join(os.tmpdir(), "codex-config-victim-"));
+    const originalCwd = process.cwd();
+    const serverCwd = fs.mkdtempSync(path.join(os.tmpdir(), "codex-config-servercwd-"));
+    process.chdir(serverCwd);
+    try {
+      const { status, body } = await fetchJson(
+        `/api/codex-config/file?cwd=${encodeURIComponent(victim)}`,
+        {
+          method: "PUT",
+          body: { scope: "project", type: "memory", content: "PWNED" },
+        }
+      );
+      assert.equal(status, 200);
+      assert.equal(fs.existsSync(path.join(victim, "AGENTS.md")), false, "must not write to ?cwd=");
+      assert.equal(body.file, path.join(serverCwd, "AGENTS.md"));
+      assert.equal(fs.readFileSync(body.file, "utf8"), "PWNED");
+    } finally {
+      process.chdir(originalCwd);
+      fs.rmSync(victim, { recursive: true, force: true });
+      fs.rmSync(serverCwd, { recursive: true, force: true });
+    }
+  });
+
+  it("DELETE /file ignores ?cwd= for project scope", async () => {
+    const victim = fs.mkdtempSync(path.join(os.tmpdir(), "codex-config-victim-"));
+    fs.writeFileSync(path.join(victim, ".codex-marker"), "x"); // proves this dir wasn't touched
+    const originalCwd = process.cwd();
+    const serverCwd = fs.mkdtempSync(path.join(os.tmpdir(), "codex-config-servercwd-"));
+    fs.mkdirSync(path.join(serverCwd, ".codex", "rules"), { recursive: true });
+    fs.writeFileSync(path.join(serverCwd, ".codex", "rules", "victim.rules"), "real");
+    process.chdir(serverCwd);
+    try {
+      const { status } = await fetchJson(
+        `/api/codex-config/file?cwd=${encodeURIComponent(victim)}`,
+        {
+          method: "DELETE",
+          body: { scope: "project", type: "rules", name: "victim" },
+        }
+      );
+      assert.equal(status, 200);
+      assert.equal(fs.existsSync(path.join(victim, ".codex-marker")), true, "?cwd= dir left alone");
+      assert.equal(fs.existsSync(path.join(serverCwd, ".codex", "rules", "victim.rules")), false);
+    } finally {
+      process.chdir(originalCwd);
+      fs.rmSync(victim, { recursive: true, force: true });
+      fs.rmSync(serverCwd, { recursive: true, force: true });
+    }
+  });
+
   it("PUT /file rejects a write whose name escapes the root with 400, not 500", async () => {
     const { status, body } = await fetchJson(
       `/api/codex-config/file?cwd=${encodeURIComponent(FAKE_PROJECT)}`,
@@ -364,6 +450,31 @@ describe("/api/codex-config", () => {
     }
   });
 
+  it("GET /mcp reports unparseable CLI JSON as 502, not a type-confused 200", async () => {
+    const restore = stubCodex('echo "not json"');
+    try {
+      const { status, body } = await fetchJson("/api/codex-config/mcp");
+      assert.equal(status, 502);
+      assert.equal(body.error.code, "ECLIBADOUTPUT");
+    } finally {
+      restore();
+    }
+  });
+
+  it("POST /mcp/add rejects a non-array args with 400, not a 500", async () => {
+    const restore = stubCodex(ECHO_ARGV);
+    try {
+      const { status, body } = await fetchJson("/api/codex-config/mcp/add", {
+        method: "POST",
+        body: { name: "docs", command: "node", args: "oops" },
+      });
+      assert.equal(status, 400);
+      assert.equal(body.error.code, "EBADARG");
+    } finally {
+      restore();
+    }
+  });
+
   it("POST /mcp/add runs the CLI with the given argv", async () => {
     const restore = stubCodex(ECHO_ARGV);
     try {

server/lib/codex-cli.js

@@ -199,6 +199,10 @@ async function mcpAdd({ name, url, bearerTokenEnvVar, command, args = [], env })
   if (!url && !command) {
     throw makeError("EBADARG", "mcpAdd requires either url or command");
   }
+  if (!Array.isArray(args)) throw makeError("EBADARG", "args must be an array");
+  if (env !== undefined && (typeof env !== "object" || env === null || Array.isArray(env))) {
+    throw makeError("EBADARG", "env must be an object");
+  }
 
   if (url) {
     if (env) throw makeError("EBADARG", "env is not valid together with url (stdio-only)");
@@ -256,8 +260,26 @@ async function marketplaceRemove(name) {
   return serialize(() => runCodex(["plugin", "marketplace", "remove", n]));
 }
 
+/**
+ * Best-effort human message from a failed run. The CLI prints its errors with
+ * a `✘` marker on either stream; fall back to the last non-empty line.
+ * Unchanged copy of cc-plugin-cli.js's cliMessage — same CLI-output shape,
+ * same heuristic.
+ */
+function cliMessage(res) {
+  const text = `${res.stderr || ""}\n${res.stdout || ""}`;
+  const lines = text
+    .split("\n")
+    .map((l) => l.trim())
+    .filter(Boolean);
+  const marked = lines.find((l) => l.includes("✘"));
+  const picked = marked || lines[lines.length - 1] || "";
+  return picked.replace(/^✘\s*/, "").slice(0, 1000);
+}
+
 module.exports = {
   runCodex,
+  cliMessage,
   mcpList,
   mcpAdd,
   mcpRemove,

server/lib/codex-discovery.js

@@ -22,6 +22,11 @@ const {
   isFileLike,
   MAX_FILE_BYTES,
 } = require("./cc-discovery");
+// Shared symlink-escape guard — the same check writeArtifact/deleteArtifact
+// use, so a read and a write of the same path agree on what "inside the
+// root" means. See codex-mutate.js's assertNotSymlinkEscape for the leaf +
+// ancestor symlink reasoning.
+const { assertNotSymlinkEscape } = require("./codex-mutate");
 
 function safeReadText(absPath) {
   try {
@@ -286,6 +291,48 @@ function readOverview(opts = {}) {
   };
 }
 
+// ── Single-file body endpoint (with strict path containment) ───────────
+
+/**
+ * Read the body of one file living under CODEX_HOME, the project .codex
+ * dir, or (only for a file literally named AGENTS.md) the project root.
+ * Mirrors cc-discovery's readFileSafe, plus the same leaf/ancestor
+ * symlink-escape guard the write side (codex-mutate.js) uses — a string-only
+ * `isUnder` check alone would let a symlink dropped anywhere along the path
+ * read arbitrary files outside the root.
+ */
+function readFileSafe(absPath, opts = {}) {
+  const codexHome = getCodexHome();
+  const projectCodexDir = getProjectCodexDir(opts.cwd);
+  const projectRoot = getProjectRoot(opts.cwd);
+  const resolved = path.resolve(absPath);
+
+  const inside = [codexHome, projectCodexDir, projectRoot].some((root) => isUnder(root, resolved));
+  if (!inside) return { error: "path is outside allowed roots" };
+  if (
+    isUnder(projectRoot, resolved) &&
+    !isUnder(projectCodexDir, resolved) &&
+    path.basename(resolved) !== "AGENTS.md"
+  ) {
+    return { error: "only AGENTS.md is readable from project root" };
+  }
+
+  const trustedRoot = isUnder(codexHome, resolved)
+    ? codexHome
+    : isUnder(projectCodexDir, resolved)
+      ? projectCodexDir
+      : projectRoot;
+  try {
+    assertNotSymlinkEscape(trustedRoot, resolved);
+  } catch (err) {
+    return { error: err.message };
+  }
+
+  const r = safeReadText(resolved);
+  if (!r) return { error: "file not readable" };
+  return { ok: true, file: resolved, ...r };
+}
+
 module.exports = {
   readSkills,
   readAgents,
@@ -293,5 +340,6 @@ module.exports = {
   readMemory,
   readConfig,
   readOverview,
+  readFileSafe,
   readTomlMeta,
 };

server/lib/codex-mutate.js

@@ -300,6 +300,7 @@ module.exports = {
   deleteArtifact,
   listBackups,
   resolveTarget, // exported for tests
+  assertNotSymlinkEscape, // shared with codex-discovery.js's readFileSafe — one check, both verbs
   TYPES,
   NAME_RE,
 };

server/openapi-extra/codex-config.js

@@ -398,9 +398,8 @@ const paths = {
       tags: ["CodexConfig"],
       summary: "Create or overwrite a text artifact",
       description:
-        "Writes skills/agents/rules/memory/config. `name` is required for skills/agents/rules and forbidden for memory/config (fixed filenames). Backs up any existing file first; broadcasts `codex_config_changed` on success.",
+        "Writes skills/agents/rules/memory/config. `name` is required for skills/agents/rules and forbidden for memory/config (fixed filenames). `project` scope always resolves against the dashboard server's own cwd — there is no `?cwd=` override on writes, unlike the read routes, so a caller cannot point a write at an arbitrary directory. Backs up any existing file first; broadcasts `codex_config_changed` on success.",
       operationId: "codexConfigPutFile",
-      parameters: [cwdParam],
       requestBody: {
         required: true,
         content: {
@@ -436,9 +435,8 @@ const paths = {
       tags: ["CodexConfig"],
       summary: "Delete a text artifact",
       description:
-        "Deletes skills/agents/rules/memory/config. Backup is mandatory and runs first. Broadcasts `codex_config_changed` on success.",
+        "Deletes skills/agents/rules/memory/config. `project` scope always resolves against the dashboard server's own cwd — no `?cwd=` override, same as PUT. Backup is mandatory and runs first. Broadcasts `codex_config_changed` on success.",
       operationId: "codexConfigDeleteFile",
-      parameters: [cwdParam],
       requestBody: {
         required: true,
         content: {
@@ -518,6 +516,7 @@ const paths = {
           content: { "application/json": { schema: { type: "object", properties: { mcp: {} } } } },
         },
         422: cliExitResponse,
+        502: errorResponse("ECLIBADOUTPUT", "could not parse `codex` JSON output"),
         503: errorResponse("ECLINOTFOUND", "the `codex` CLI was not found on PATH"),
       },
     },
@@ -537,6 +536,7 @@ const paths = {
           },
         },
         422: cliExitResponse,
+        502: errorResponse("ECLIBADOUTPUT", "could not parse `codex` JSON output"),
         503: errorResponse("ECLINOTFOUND", "the `codex` CLI was not found on PATH"),
       },
     },
@@ -556,6 +556,7 @@ const paths = {
           },
         },
         422: cliExitResponse,
+        502: errorResponse("ECLIBADOUTPUT", "could not parse `codex` JSON output"),
         503: errorResponse("ECLINOTFOUND", "the `codex` CLI was not found on PATH"),
       },
     },

server/routes/codex-config.js

@@ -20,14 +20,10 @@
  * @author Nguyễn Ngọc Trí Vĩ <vinnt@smartgift.vn>
  */
 
-const fs = require("node:fs");
-const path = require("node:path");
 const { Router } = require("express");
 const codexDiscovery = require("../lib/codex-discovery");
 const codexMutate = require("../lib/codex-mutate");
 const codexCli = require("../lib/codex-cli");
-const { getCodexHome, getProjectCodexDir, getProjectRoot } = require("../lib/codex-home");
-const { isUnder, MAX_FILE_BYTES } = require("../lib/cc-discovery");
 const { broadcast } = require("../websocket");
 
 const router = Router();
@@ -100,53 +96,17 @@ router.get("/config", (req, res) => {
 
 // GET /api/codex-config/file?path=/abs/path — return body of a single file.
 // Path must resolve under CODEX_HOME, the project .codex dir, or (only for a
-// file literally named AGENTS.md) the project root. No dedicated reader for
-// this exists in codex-discovery.js (it only reads whole surfaces), so this
-// mirrors cc-discovery's readFileSafe against the Codex roots instead.
-function readCodexFileSafe(absPath, opts = {}) {
-  const codexHome = getCodexHome();
-  const projectCodexDir = getProjectCodexDir(opts.cwd);
-  const projectRoot = getProjectRoot(opts.cwd);
-  const resolved = path.resolve(absPath);
-  const inside = [codexHome, projectCodexDir, projectRoot].some((root) => isUnder(root, resolved));
-  if (!inside) return { error: "path is outside allowed roots" };
-  if (
-    isUnder(projectRoot, resolved) &&
-    !isUnder(projectCodexDir, resolved) &&
-    path.basename(resolved) !== "AGENTS.md"
-  ) {
-    return { error: "only AGENTS.md is readable from project root" };
-  }
-  let stat;
-  try {
-    stat = fs.statSync(resolved);
-  } catch {
-    return { error: "file not readable" };
-  }
-  if (!stat.isFile()) return { error: "file not readable" };
-  let text;
-  try {
-    text = fs.readFileSync(resolved, "utf8");
-  } catch {
-    return { error: "file not readable" };
-  }
-  const truncated = stat.size > MAX_FILE_BYTES;
-  return {
-    ok: true,
-    file: resolved,
-    size: stat.size,
-    mtime: stat.mtimeMs,
-    truncated,
-    text: truncated ? text.slice(0, MAX_FILE_BYTES) : text,
-  };
-}
-
+// file literally named AGENTS.md) the project root. codexDiscovery.readFileSafe
+// re-checks containment with the same leaf/ancestor symlink-escape guard the
+// write side uses (codex-mutate.js's assertNotSymlinkEscape) — a plain
+// string-only path check would let a symlink dropped under the root read
+// arbitrary files outside it.
 router.get("/file", (req, res) => {
   const p = req.query.path;
   if (typeof p !== "string" || !p) {
     return res.status(400).json({ error: { code: "BAD_PATH", message: "path is required" } });
   }
-  const result = readCodexFileSafe(p, { cwd: cwdOf(req) });
+  const result = codexDiscovery.readFileSafe(p, { cwd: cwdOf(req) });
   if (result.error)
     return res.status(400).json({ error: { code: "READ_DENIED", message: result.error } });
   res.json(result);
@@ -157,6 +117,11 @@ router.get("/file", (req, res) => {
 // PUT  /api/codex-config/file   — create or overwrite. Body: { scope, type, name?, content }
 // DELETE /api/codex-config/file — delete. Body: { scope, type, name? }
 // GET  /api/codex-config/backups[?scope=&type=] — list backups
+//
+// Writes resolve "project" scope against the SERVER's cwd, never `?cwd=`: a
+// write target chosen by the caller would let any request create a file
+// anywhere on disk (same reasoning cc-config.js's PATCH /settings gives for
+// refusing `?cwd=` there). Reads keep the override.
 
 router.put("/file", (req, res) => {
   const { scope, type, name, content } = req.body || {};
@@ -166,7 +131,7 @@ router.put("/file", (req, res) => {
       .json({ error: { code: "EBADREQ", message: "scope and type are required" } });
   }
   try {
-    const result = codexMutate.writeArtifact({ scope, type, name, content, cwd: cwdOf(req) });
+    const result = codexMutate.writeArtifact({ scope, type, name, content, cwd: process.cwd() });
     emitChanged({ action: "write", scope, type, name: name || null });
     res.json(result);
   } catch (err) {
@@ -182,7 +147,7 @@ router.delete("/file", (req, res) => {
       .json({ error: { code: "EBADREQ", message: "scope and type are required" } });
   }
   try {
-    const result = codexMutate.deleteArtifact({ scope, type, name, cwd: cwdOf(req) });
+    const result = codexMutate.deleteArtifact({ scope, type, name, cwd: process.cwd() });
     emitChanged({ action: "delete", scope, type, name: name || null });
     res.json(result);
   } catch (err) {
@@ -221,28 +186,12 @@ function cliError(res, err) {
     .json({ error: { code: err.code || "EINTERNAL", message: err.message } });
 }
 
-/**
- * Best-effort human message from a failed run. codex-cli.js does not export
- * one (unlike cc-plugin-cli's cliMessage), so this route-local helper does
- * the same "last non-empty line, prefer a marked one" heuristic.
- */
-function cliMessage(result) {
-  const text = `${result.stderr || ""}\n${result.stdout || ""}`;
-  const lines = text
-    .split("\n")
-    .map((l) => l.trim())
-    .filter(Boolean);
-  const marked = lines.find((l) => l.includes("✘") || l.toLowerCase().includes("error"));
-  const picked = marked || lines[lines.length - 1] || "";
-  return picked.replace(/^✘\s*/, "").slice(0, 1000);
-}
-
 /** A non-zero CLI exit is a 422, not a 500 — the caller can inspect stderr. */
 function cliFailure(res, result) {
   return res.status(422).json({
     error: {
       code: "ECLIEXIT",
-      message: cliMessage(result) || `codex exited with code ${result.code}`,
+      message: codexCli.cliMessage(result) || `codex exited with code ${result.code}`,
     },
     code: result.code,
     stdout: result.stdout,
@@ -250,10 +199,24 @@ function cliFailure(res, result) {
   });
 }
 
-/** Read-through result for a GET route backed by the CLI. */
+/**
+ * Read-through result for a GET route backed by `codex … --json`. `result.data`
+ * is only set when stdout parsed as JSON (see runCodex) — an ok:true result
+ * with unparseable stdout means the CLI's output shape changed under us, so
+ * this is a 502, not a 200 with a string where the OpenAPI fragment promises
+ * an array/object.
+ */
 function cliRead(res, result, key) {
   if (!result.ok) return cliFailure(res, result);
-  res.json({ [key]: result.data !== undefined ? result.data : result.stdout });
+  if (result.data === undefined) {
+    return res.status(502).json({
+      error: {
+        code: "ECLIBADOUTPUT",
+        message: `could not parse \`codex\` JSON output for ${key}`,
+      },
+    });
+  }
+  res.json({ [key]: result.data });
 }
 
 /** Result for a mutating route: broadcast on success, 422 on CLI failure. */