From f3a2f41ed736dc9b4b1b79419cad76c4a21a5f7a Mon Sep 17 00:00:00 2001 From: nntrivi2001 Date: Tue, 8 Sep 2026 17:34:10 +0700 Subject: [PATCH] fix(codex): close symlink write escape and validate listBackups scope/type --- server/__tests__/codex-mutate.test.js | 86 +++++++++++++++++++++++++-- server/lib/codex-mutate.js | 47 ++++++++++++++- 2 files changed, 126 insertions(+), 7 deletions(-) diff --git a/server/__tests__/codex-mutate.test.js b/server/__tests__/codex-mutate.test.js index 0372dbf0..291aa7ad 100644 --- a/server/__tests__/codex-mutate.test.js +++ b/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" + ); + }); }); diff --git a/server/lib/codex-mutate.js b/server/lib/codex-mutate.js index cc95e691..e4d96795 100644 --- a/server/lib/codex-mutate.js +++ b/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ĩ @@ -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);