fix(codex): close symlink write escape and validate listBackups scope/type

server/__tests__/codex-mutate.test.js

@@ -127,7 +127,8 @@ describe("codex-mutate", () => {
     assert.equal(p.file, path.join(REPO, ".codex", "config.toml"));
   });
 
-  it("rejects a scope that is neither user nor project", () => {
+  it("rejects a scope that is neither user nor project, and writes nothing", () => {
+    const target = path.join(TMP, "rules", "x.rules");
     assert.throws(
       () =>
         cm.writeArtifact({
@@ -139,14 +140,71 @@ describe("codex-mutate", () => {
         }),
       (e) => e.code === "EBADSCOPE"
     );
+    assert.equal(fs.existsSync(target), false);
   });
 
-  it("rejects a type outside TYPES", () => {
+  it("rejects a type outside TYPES, and writes nothing", () => {
+    const target = path.join(TMP, "commands", "x.md");
     assert.throws(
       () =>
         cm.writeArtifact({ scope: "user", type: "commands", name: "x", content: "x", cwd: REPO }),
       (e) => e.code === "EBADTYPE"
     );
+    assert.equal(fs.existsSync(target), false);
+  });
+
+  it("refuses a symlinked skill dir that escapes both Codex roots, and writes nothing outside", () => {
+    const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-outside-"));
+    const linkPath = path.join(TMP, "skills", "escape-link");
+    fs.mkdirSync(path.dirname(linkPath), { recursive: true });
+    fs.symlinkSync(outside, linkPath, "dir");
+
+    assert.throws(
+      () =>
+        cm.writeArtifact({
+          scope: "user",
+          type: "skills",
+          name: "escape-link",
+          content: "pwned",
+          cwd: REPO,
+        }),
+      (e) => e.code === "EOUTOFROOT"
+    );
+    assert.equal(fs.existsSync(path.join(outside, "SKILL.md")), false);
+    fs.rmSync(outside, { recursive: true, force: true });
+  });
+
+  it("follows a symlinked skill dir that points back inside the same root", () => {
+    const realDir = path.join(TMP, "skills", "real-dir");
+    fs.mkdirSync(realDir, { recursive: true });
+    const linkPath = path.join(TMP, "skills", "inside-link");
+    fs.symlinkSync(realDir, linkPath, "dir");
+
+    const { file, created } = cm.writeArtifact({
+      scope: "user",
+      type: "skills",
+      name: "inside-link",
+      content: "fine",
+      cwd: REPO,
+    });
+    assert.equal(created, true);
+    assert.equal(fs.readFileSync(file, "utf8"), "fine");
+    assert.equal(fs.readFileSync(path.join(realDir, "SKILL.md"), "utf8"), "fine");
+  });
+
+  it("refuses to back up a symlinked artifact that escapes the roots when deleting it", () => {
+    const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-outside-"));
+    fs.writeFileSync(path.join(outside, "secret.rules"), "top secret");
+    const linkPath = path.join(TMP, "rules", "escape-del.rules");
+    fs.mkdirSync(path.dirname(linkPath), { recursive: true });
+    fs.symlinkSync(path.join(outside, "secret.rules"), linkPath, "file");
+
+    assert.throws(
+      () => cm.deleteArtifact({ scope: "user", type: "rules", name: "escape-del", cwd: REPO }),
+      (e) => e.code === "EOUTOFROOT"
+    );
+    assert.equal(fs.existsSync(path.join(outside, "secret.rules")), true);
+    fs.rmSync(outside, { recursive: true, force: true });
   });
 
   for (const badName of ["..", ".", "/etc/passwd", "a/b", "a\0b"]) {
@@ -167,7 +225,8 @@ describe("codex-mutate", () => {
     });
   }
 
-  it("rejects a name passed to memory", () => {
+  it("rejects a name passed to memory, and writes nothing", () => {
+    const target = path.join(TMP, "AGENTS.md");
     assert.throws(
       () =>
         cm.writeArtifact({
@@ -179,9 +238,12 @@ describe("codex-mutate", () => {
         }),
       (e) => e.code === "EBADNAME"
     );
+    assert.equal(fs.existsSync(target), false);
   });
 
-  it("rejects a name passed to config", () => {
+  it("rejects a name passed to config, and leaves any existing config.toml untouched", () => {
+    const target = path.join(TMP, "config.toml");
+    const before = fs.existsSync(target) ? fs.readFileSync(target, "utf8") : null;
     assert.throws(
       () =>
         cm.writeArtifact({
@@ -193,6 +255,8 @@ describe("codex-mutate", () => {
         }),
       (e) => e.code === "EBADNAME"
     );
+    const after = fs.existsSync(target) ? fs.readFileSync(target, "utf8") : null;
+    assert.equal(after, before);
   });
 
   it("rejects content over the size limit and leaves the existing file untouched", () => {
@@ -234,4 +298,18 @@ describe("codex-mutate", () => {
     assert.equal(rows[0].type, "rules");
     assert.equal(rows[0].name.startsWith("listed.rules."), true);
   });
+
+  it("refuses a listBackups scope that is neither user nor project", () => {
+    assert.throws(
+      () => cm.listBackups({ scope: "../../../../tmp" }),
+      (e) => e.code === "EBADSCOPE"
+    );
+  });
+
+  it("refuses a listBackups type that tries to traverse outside the backups root", () => {
+    assert.throws(
+      () => cm.listBackups({ scope: "user", type: "../../../../tmp" }),
+      (e) => e.code === "EBADTYPE"
+    );
+  });
 });

server/lib/codex-mutate.js

@@ -18,7 +18,11 @@
  *   - Writes are atomic via temp file + fs.renameSync (see config-write.js).
  *   - Names are validated against a strict allowlist regex; resolved paths
  *     are double-checked to live under the expected containment root before
- *     any I/O.
+ *     any I/O. That string-only check does not see through a symlink, so
+ *     any existing target is additionally realpath-resolved and required to
+ *     land inside a real Codex root (see `assertNotSymlinkEscape`) — a
+ *     symlink that merely reorganises files within the roots (a supported
+ *     install pattern) keeps working; one pointing outside is refused.
  *   - Codex has no `project-local` scope and no auto-memory: only "user" and
  *     "project" are valid scopes.
  * @author Nguyễn Ngọc Trí Vĩ <vinnt@smartgift.vn>
@@ -27,7 +31,7 @@
 const fs = require("node:fs");
 const path = require("node:path");
 const { getCodexHome, getProjectCodexDir, getProjectRoot } = require("./codex-home");
-const { MAX_FILE_BYTES } = require("./cc-discovery");
+const { MAX_FILE_BYTES, isUnder } = require("./cc-discovery");
 const { createBackup, atomicWriteFile, assertUnder } = require("./config-write");
 
 const NAME_RE = /^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$/;
@@ -118,6 +122,36 @@ function rmTreeSync(p) {
   fs.rmSync(p, { recursive: true, force: true });
 }
 
+/** realpath if it exists, else the plain resolved path — for roots that may not exist yet. */
+function safeRealpath(p) {
+  try {
+    return fs.realpathSync(p);
+  } catch {
+    return path.resolve(p);
+  }
+}
+
+/**
+ * Symlinks are a supported install pattern (Task 3's reader follows them),
+ * so a symlink at `target` is not rejected outright — only one that
+ * dereferences outside both Codex roots. A missing/dangling symlink is a
+ * fresh create, not an error. Must run before anything reads through
+ * `target` (backup or write), since string-only containment checks never
+ * see through a link.
+ */
+function assertNotSymlinkEscape(target, cwd) {
+  let real;
+  try {
+    real = fs.realpathSync(target);
+  } catch {
+    return;
+  }
+  const roots = [safeRealpath(getCodexHome()), safeRealpath(getProjectCodexDir(cwd))];
+  if (!roots.some((root) => isUnder(root, real))) {
+    throw makeError("EOUTOFROOT", `resolved path escapes the Codex roots: ${real}`);
+  }
+}
+
 // ── Public API ─────────────────────────────────────────────────────────
 
 /**
@@ -135,6 +169,7 @@ function writeArtifact(args) {
 
   const r = resolveTarget(scope, type, name, { cwd });
   assertUnder(r.containmentRoot, r.target);
+  assertNotSymlinkEscape(r.target, cwd);
 
   const existedBefore = fs.existsSync(r.filePath);
   const backup = existedBefore
@@ -166,6 +201,7 @@ function deleteArtifact(args) {
   if (!fs.existsSync(r.target)) {
     throw makeError("ENOTFOUND", `${type}/${name || TYPES[type].file} does not exist`);
   }
+  assertNotSymlinkEscape(r.target, cwd);
 
   const backup = createBackup({
     source: r.target,
@@ -188,9 +224,14 @@ function deleteArtifact(args) {
  * Returns [{ type, name, file, mtime, size }].
  */
 function listBackups(opts = {}) {
-  const out = [];
   const scopes = opts.scope ? [opts.scope] : ["user", "project"];
   const types = opts.type ? [opts.type] : Object.keys(TYPES);
+  for (const scope of scopes) assertScope(scope);
+  for (const type of types) {
+    if (!TYPES[type]) throw makeError("EBADTYPE", `unknown type: ${type}`);
+  }
+
+  const out = [];
   for (const scope of scopes) {
     for (const type of types) {
       const root = backupRootFor(scope, type, opts);