diff --git a/openapi.yaml b/openapi.yaml index d959224b..c6a45277 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -8981,7 +8981,7 @@ paths: tags: - CodexConfig summary: Read a single file body - description: 'Read-only. Returns the body of one file. Strict path containment: the resolved absolute path must live under CODEX_HOME, the project .codex dir, or (only for a file literally named AGENTS.md) the project root.' + description: 'Read-only, no filesystem side effect. Returns the body of one file. Strict path containment: the resolved absolute path must live under CODEX_HOME, the project .codex dir, or (only for a file literally named AGENTS.md) the project root, and must not escape that root through a symlink. Error messages never reveal what does or does not exist outside the root.' operationId: codexConfigGetFile parameters: - name: path @@ -9012,7 +9012,16 @@ paths: example: error: code: READ_DENIED - message: path is outside allowed roots + message: path escapes the allowed root + '404': + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorResponse' + example: + error: + code: NOT_FOUND + message: file not found put: tags: - CodexConfig diff --git a/server/__tests__/codex-config.test.js b/server/__tests__/codex-config.test.js index 3b58ae0f..5593afef 100644 --- a/server/__tests__/codex-config.test.js +++ b/server/__tests__/codex-config.test.js @@ -247,6 +247,26 @@ describe("/api/codex-config", () => { } }); + it("file endpoint 404s on a missing file inside an existing root", async () => { + const target = path.join(FAKE_HOME, "rules", "does-not-exist.rules"); + const { status, body } = await fetchJson( + `/api/codex-config/file?cwd=${encodeURIComponent(FAKE_PROJECT)}&path=${encodeURIComponent(target)}` + ); + assert.equal(status, 404); + assert.equal(body.error.code, "NOT_FOUND"); + }); + + it("file endpoint 404s on a ?cwd= that doesn't exist yet, and creates nothing", async () => { + const missingCwd = path.join(TMP, `never-existed-${process.hrtime.bigint()}`); + const target = path.join(missingCwd, "AGENTS.md"); + const { status, body } = await fetchJson( + `/api/codex-config/file?cwd=${encodeURIComponent(missingCwd)}&path=${encodeURIComponent(target)}` + ); + assert.equal(status, 404); + assert.equal(body.error.code, "NOT_FOUND"); + assert.equal(fs.existsSync(missingCwd), false, "GET /file must not create the missing root"); + }); + it("file endpoint requires a path", async () => { const { status, body } = await fetchJson("/api/codex-config/file"); assert.equal(status, 400); @@ -450,6 +470,23 @@ describe("/api/codex-config", () => { } }); + it("GET /mcp|/plugins|/marketplaces treat empty stdout as an empty list (fresh-install case), not a 502", async () => { + const restore = stubCodex(""); // exits 0, prints nothing — the normal fresh-install state + try { + for (const [route, key] of [ + ["/api/codex-config/mcp", "mcp"], + ["/api/codex-config/plugins", "plugins"], + ["/api/codex-config/marketplaces", "marketplaces"], + ]) { + const { status, body } = await fetchJson(route); + assert.equal(status, 200, route); + assert.deepEqual(body[key], [], route); + } + } finally { + restore(); + } + }); + it("GET /mcp reports unparseable CLI JSON as 502, not a type-confused 200", async () => { const restore = stubCodex('echo "not json"'); try { diff --git a/server/lib/codex-discovery.js b/server/lib/codex-discovery.js index c3f8b0cd..b2b3a8f7 100644 --- a/server/lib/codex-discovery.js +++ b/server/lib/codex-discovery.js @@ -300,6 +300,16 @@ function readOverview(opts = {}) { * 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. + * + * A GET has no filesystem side effect: `assertNotSymlinkEscape` is called + * with `{create: false}` so it never mkdirs a caller-chosen root, and a + * missing root or file both come back as `{notFound: true}` rather than the + * write side's `EOUTOFROOT`. Failure messages never include the resolved + * path — a `GET /file` response is not the place to reveal what does or + * does not exist outside the root. + * + * @returns {{ok:true,file:string,size:number,mtime:number,truncated:boolean,text:string} + * | {notFound:true} | {error:string}} */ function readFileSafe(absPath, opts = {}) { const codexHome = getCodexHome(); @@ -323,13 +333,16 @@ function readFileSafe(absPath, opts = {}) { ? projectCodexDir : projectRoot; try { - assertNotSymlinkEscape(trustedRoot, resolved); + assertNotSymlinkEscape(trustedRoot, resolved, { create: false }); } catch (err) { - return { error: err.message }; + // A root that doesn't exist yet (ENOENT from realpathSync, since this + // call never mkdirs it) means there is nothing to find, not a refusal. + if (err.code === "ENOENT") return { notFound: true }; + return { error: "path escapes the allowed root" }; } const r = safeReadText(resolved); - if (!r) return { error: "file not readable" }; + if (!r) return { notFound: true }; return { ok: true, file: resolved, ...r }; } diff --git a/server/lib/codex-mutate.js b/server/lib/codex-mutate.js index c28862c8..2d593e9c 100644 --- a/server/lib/codex-mutate.js +++ b/server/lib/codex-mutate.js @@ -153,15 +153,19 @@ function rmTreeSync(p) { * * `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. + * it, never wander past it into whatever happens to contain it. Pass + * `{create: false}` for a read-only caller (codex-discovery.js's + * readFileSafe): a GET must have no filesystem side effect, so it skips the + * mkdir and instead lets a missing `trustedRoot` surface as the native + * `ENOENT` from `realpathSync` — the caller treats that as "not found". * * 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(trustedRoot, target) { - fs.mkdirSync(trustedRoot, { recursive: true }); +function assertNotSymlinkEscape(trustedRoot, target, opts = {}) { + if (opts.create !== false) fs.mkdirSync(trustedRoot, { recursive: true }); const realRoot = fs.realpathSync(trustedRoot); let dir = target; diff --git a/server/openapi-extra/codex-config.js b/server/openapi-extra/codex-config.js index 46de53ab..091c71ea 100644 --- a/server/openapi-extra/codex-config.js +++ b/server/openapi-extra/codex-config.js @@ -370,7 +370,7 @@ const paths = { tags: ["CodexConfig"], summary: "Read a single file body", description: - "Read-only. Returns the body of one file. Strict path containment: the resolved absolute path must live under CODEX_HOME, the project .codex dir, or (only for a file literally named AGENTS.md) the project root.", + "Read-only, no filesystem side effect. Returns the body of one file. Strict path containment: the resolved absolute path must live under CODEX_HOME, the project .codex dir, or (only for a file literally named AGENTS.md) the project root, and must not escape that root through a symlink. Error messages never reveal what does or does not exist outside the root.", operationId: "codexConfigGetFile", parameters: [ { @@ -391,7 +391,8 @@ const paths = { }, }, }, - 400: errorResponse("READ_DENIED", "path is outside allowed roots"), + 400: errorResponse("READ_DENIED", "path escapes the allowed root"), + 404: errorResponse("NOT_FOUND", "file not found"), }, }, put: { diff --git a/server/routes/codex-config.js b/server/routes/codex-config.js index ade362a3..896a9e7b 100644 --- a/server/routes/codex-config.js +++ b/server/routes/codex-config.js @@ -107,6 +107,8 @@ router.get("/file", (req, res) => { return res.status(400).json({ error: { code: "BAD_PATH", message: "path is required" } }); } const result = codexDiscovery.readFileSafe(p, { cwd: cwdOf(req) }); + if (result.notFound) + return res.status(404).json({ error: { code: "NOT_FOUND", message: "file not found" } }); if (result.error) return res.status(400).json({ error: { code: "READ_DENIED", message: result.error } }); res.json(result); @@ -201,14 +203,16 @@ function cliFailure(res, result) { /** * 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. + * is only set when stdout parsed as JSON (see runCodex). A fresh install with + * zero MCP servers/plugins/marketplaces is the normal case and prints nothing + * — that's an empty list, not a parse failure. Only stdout that has content + * and still doesn't parse (the CLI's output shape changed under us) is a 502, + * never 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); if (result.data === undefined) { + if (!result.stdout.trim()) return res.json({ [key]: [] }); return res.status(502).json({ error: { code: "ECLIBADOUTPUT",