fix(codex): close parent-dir symlink escape in the containment check

server/__tests__/codex-mutate.test.js

@@ -207,6 +207,65 @@ describe("codex-mutate", () => {
     fs.rmSync(outside, { recursive: true, force: true });
   });
 
+  it("refuses a brand-new artifact whose subdir (not the leaf) is a symlink escaping the root", () => {
+    const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-outside-"));
+    const linkPath = path.join(TMP, "agents");
+    // "agents" doesn't exist yet in this fresh suite of tests for this scope —
+    // remove it first if an earlier test already created it as a real dir.
+    fs.rmSync(linkPath, { recursive: true, force: true });
+    fs.symlinkSync(outside, linkPath, "dir");
+
+    assert.throws(
+      () =>
+        cm.writeArtifact({
+          scope: "user",
+          type: "agents",
+          name: "brand-new",
+          content: "pwned",
+          cwd: REPO,
+        }),
+      (e) => e.code === "EOUTOFROOT"
+    );
+    assert.equal(fs.existsSync(path.join(outside, "brand-new.toml")), false);
+    fs.rmSync(linkPath, { force: true });
+    fs.rmSync(outside, { recursive: true, force: true });
+  });
+
+  it("still writes correctly when $CODEX_HOME itself is a symlink (a dotfiles checkout)", () => {
+    const realHome = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-realhome-"));
+    const linkHome = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-linkhome-"));
+    fs.rmdirSync(linkHome); // symlinkSync needs the link path itself to not exist
+    fs.symlinkSync(realHome, linkHome, "dir");
+
+    const prev = process.env.CODEX_HOME;
+    process.env.CODEX_HOME = linkHome;
+    delete require.cache[require.resolve("../lib/codex-home")];
+    delete require.cache[require.resolve("../lib/codex-mutate")];
+    const linkedCm = require("../lib/codex-mutate");
+    try {
+      const { file, created } = linkedCm.writeArtifact({
+        scope: "user",
+        type: "rules",
+        name: "via-link",
+        content: "ok",
+        cwd: REPO,
+      });
+      assert.equal(created, true);
+      assert.equal(fs.readFileSync(file, "utf8"), "ok");
+      assert.equal(
+        fs.readFileSync(path.join(realHome, "rules", "via-link.rules"), "utf8"),
+        "ok",
+        "the write must land in the real directory the symlinked CODEX_HOME points to"
+      );
+    } finally {
+      process.env.CODEX_HOME = prev;
+      delete require.cache[require.resolve("../lib/codex-home")];
+      delete require.cache[require.resolve("../lib/codex-mutate")];
+      fs.rmSync(linkHome, { force: true });
+      fs.rmSync(realHome, { recursive: true, force: true });
+    }
+  });
+
   for (const badName of ["..", ".", "/etc/passwd", "a/b", "a\0b"]) {
     it(`rejects the bad name ${JSON.stringify(badName)} with EBADNAME and writes nothing`, () => {
       const target = path.join(TMP, "agents", `${badName.replace(/[/\0]/g, "_")}.toml`);

server/lib/codex-mutate.js

@@ -18,11 +18,14 @@
  *   - 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. 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.
+ *     any I/O. That string-only check does not see through a symlink placed
+ *     anywhere along the path — including the type's own subdir (e.g.
+ *     `<root>/skills`) — so the nearest existing ancestor of the target is
+ *     additionally realpath-resolved and required to land inside the
+ *     realpath'd scope root (see `assertNotSymlinkEscape`). A symlink that
+ *     merely reorganises files within the root (a supported install
+ *     pattern, and a symlinked CODEX_HOME itself) 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>
@@ -75,10 +78,17 @@ function fixedPathFor(type, scope, opts = {}) {
 }
 
 /**
- * Resolve the on-disk target for a (scope, type, name) tuple AND the
- * containment root used for path-traversal checks.
+ * Resolve the on-disk target for a (scope, type, name) tuple, the
+ * containment root used for string-only path-traversal checks, and the
+ * trusted root used for the realpath-based symlink-escape check.
  *
- * Returns: { kind: "file" | "dir", target, filePath, containmentRoot }
+ * `trustedRoot` is deliberately the OUTER root (CODEX_HOME / project .codex
+ * dir / project root), not `containmentRoot`: for skills/agents/rules,
+ * `containmentRoot` is `<root>/<subdir>` (e.g. `<root>/skills`), and that
+ * subdir itself is exactly what an attacker could replace with a symlink —
+ * checking a target against its own compromised parent would always pass.
+ *
+ * Returns: { kind: "file" | "dir", target, filePath, containmentRoot, trustedRoot }
  */
 function resolveTarget(scope, type, name, opts = {}) {
   const spec = TYPES[type];
@@ -90,14 +100,18 @@ function resolveTarget(scope, type, name, opts = {}) {
       throw makeError("EBADNAME", `${type} is a fixed file and takes no name`);
     }
     const filePath = fixedPathFor(type, scope, opts);
-    return { kind: "file", target: filePath, filePath, containmentRoot: path.dirname(filePath) };
+    // No attacker-placeable segment between the trusted root and the fixed
+    // filename, so the containment root doubles as the trusted root.
+    const root = path.dirname(filePath);
+    return { kind: "file", target: filePath, filePath, containmentRoot: root, trustedRoot: root };
   }
 
   if (typeof name !== "string" || !NAME_RE.test(name)) {
     throw makeError("EBADNAME", `name must match ${NAME_RE}`);
   }
 
-  const subdirAbs = path.join(rootForScope(scope, opts), spec.subdir);
+  const root = rootForScope(scope, opts);
+  const subdirAbs = path.join(root, spec.subdir);
 
   if (spec.kind === "dir") {
     const target = path.join(subdirAbs, name);
@@ -106,11 +120,12 @@ function resolveTarget(scope, type, name, opts = {}) {
       target,
       filePath: path.join(target, spec.filename),
       containmentRoot: subdirAbs,
+      trustedRoot: root,
     };
   }
 
   const target = path.join(subdirAbs, name + spec.ext);
-  return { kind: "file", target, filePath: target, containmentRoot: subdirAbs };
+  return { kind: "file", target, filePath: target, containmentRoot: subdirAbs, trustedRoot: root };
 }
 
 /** <root>/codex-config-backups/<type>/, always rooted the same way as the read side's <root>. */
@@ -122,33 +137,51 @@ 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.
+ * so a symlink under `trustedRoot` is not rejected outright — only one that
+ * dereferences outside it. `trustedRoot` itself may be a symlink (a
+ * dotfiles-managed CODEX_HOME is a normal setup) — that's fine, it just
+ * means everything is compared against wherever it really points.
+ *
+ * `target` may not exist yet (a fresh create). Realpath can't resolve a
+ * path that isn't there, so this walks up to the nearest ancestor that DOES
+ * exist, realpath's that, and requires it to land inside the realpath'd
+ * root. Everything below that existing ancestor is plain path segments this
+ * module built itself from a NAME_RE-validated name or a fixed literal, so
+ * they can't smuggle in a symlink — only an EXISTING ancestor can.
+ *
+ * `trustedRoot` is created first (mkdirSync recursive — safe, it is never
+ * attacker-controlled) so the walk is guaranteed to bottom out at or below
+ * it, never wander past it into whatever happens to contain it.
+ *
+ * ponytail: this check and the write/delete that follows are not atomic —
+ * a symlink swapped in between them could still redirect one write. Closing
+ * that needs O_NOFOLLOW-style fd work with real platform variance; out of
+ * scope for this task (see task-5 fix round 2).
  */
-function assertNotSymlinkEscape(target, cwd) {
+function assertNotSymlinkEscape(trustedRoot, target) {
+  fs.mkdirSync(trustedRoot, { recursive: true });
+  const realRoot = fs.realpathSync(trustedRoot);
+
+  let dir = target;
   let real;
-  try {
-    real = fs.realpathSync(target);
-  } catch {
-    return;
+  for (;;) {
+    try {
+      real = fs.realpathSync(dir);
+      break;
+    } catch {
+      const parent = path.dirname(dir);
+      if (parent === dir) {
+        real = dir; // hit the filesystem root without finding anything real
+        break;
+      }
+      dir = parent;
+    }
   }
-  const roots = [safeRealpath(getCodexHome()), safeRealpath(getProjectCodexDir(cwd))];
-  if (!roots.some((root) => isUnder(root, real))) {
-    throw makeError("EOUTOFROOT", `resolved path escapes the Codex roots: ${real}`);
+
+  if (!isUnder(realRoot, real)) {
+    throw makeError("EOUTOFROOT", `resolved path escapes the Codex root: ${real}`);
   }
 }
 
@@ -169,7 +202,7 @@ function writeArtifact(args) {
 
   const r = resolveTarget(scope, type, name, { cwd });
   assertUnder(r.containmentRoot, r.target);
-  assertNotSymlinkEscape(r.target, cwd);
+  assertNotSymlinkEscape(r.trustedRoot, r.target);
 
   const existedBefore = fs.existsSync(r.filePath);
   const backup = existedBefore
@@ -201,7 +234,7 @@ function deleteArtifact(args) {
   if (!fs.existsSync(r.target)) {
     throw makeError("ENOTFOUND", `${type}/${name || TYPES[type].file} does not exist`);
   }
-  assertNotSymlinkEscape(r.target, cwd);
+  assertNotSymlinkEscape(r.trustedRoot, r.target);
 
   const backup = createBackup({
     source: r.target,