From 29ed86d6724b5586110087ccd44a5c6cb97e692a Mon Sep 17 00:00:00 2001 From: nntrivi2001 Date: Tue, 8 Sep 2026 20:07:57 +0700 Subject: [PATCH] fix(codex): close symlink escape on GET /file, pin project writes to server cwd --- openapi.yaml | 47 ++++++----- server/__tests__/codex-config.test.js | 111 ++++++++++++++++++++++++++ server/lib/codex-cli.js | 22 +++++ server/lib/codex-discovery.js | 48 +++++++++++ server/lib/codex-mutate.js | 1 + server/openapi-extra/codex-config.js | 9 ++- server/routes/codex-config.js | 97 +++++++--------------- 7 files changed, 246 insertions(+), 89 deletions(-) diff --git a/openapi.yaml b/openapi.yaml index df8b830a..d959224b 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -9017,16 +9017,8 @@ 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. + description: 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: - - name: cwd - in: query - required: false - schema: - type: string - description: Override the working directory used to resolve the project root and project .codex dir. Defaults to the dashboard server's own cwd. - example: /repo requestBody: required: true content: @@ -9091,16 +9083,8 @@ 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. + description: 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: - - name: cwd - in: query - required: false - schema: - type: string - description: Override the working directory used to resolve the project root and project .codex dir. Defaults to the dashboard server's own cwd. - example: /repo requestBody: required: true content: @@ -9233,6 +9217,15 @@ paths: stdout: '' stderr: | ✘ marketplace "nope" is not registered + '502': + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorResponse' + example: + error: + code: ECLIBADOUTPUT + message: could not parse `codex` JSON output '503': content: application/json: @@ -9287,6 +9280,15 @@ paths: stdout: '' stderr: | ✘ marketplace "nope" is not registered + '502': + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorResponse' + example: + error: + code: ECLIBADOUTPUT + message: could not parse `codex` JSON output '503': content: application/json: @@ -9341,6 +9343,15 @@ paths: stdout: '' stderr: | ✘ marketplace "nope" is not registered + '502': + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorResponse' + example: + error: + code: ECLIBADOUTPUT + message: could not parse `codex` JSON output '503': content: application/json: diff --git a/server/__tests__/codex-config.test.js b/server/__tests__/codex-config.test.js index 797507d7..3b58ae0f 100644 --- a/server/__tests__/codex-config.test.js +++ b/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 { diff --git a/server/lib/codex-cli.js b/server/lib/codex-cli.js index ba00d271..f1750233 100644 --- a/server/lib/codex-cli.js +++ b/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, diff --git a/server/lib/codex-discovery.js b/server/lib/codex-discovery.js index d026dca0..c3f8b0cd 100644 --- a/server/lib/codex-discovery.js +++ b/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, }; diff --git a/server/lib/codex-mutate.js b/server/lib/codex-mutate.js index 29b644f6..c28862c8 100644 --- a/server/lib/codex-mutate.js +++ b/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, }; diff --git a/server/openapi-extra/codex-config.js b/server/openapi-extra/codex-config.js index 55618932..46de53ab 100644 --- a/server/openapi-extra/codex-config.js +++ b/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"), }, }, diff --git a/server/routes/codex-config.js b/server/routes/codex-config.js index bccf048e..ade362a3 100644 --- a/server/routes/codex-config.js +++ b/server/routes/codex-config.js @@ -20,14 +20,10 @@ * @author Nguyễn Ngọc Trí Vĩ */ -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. */