fix(codex): stop 502ing empty CLI output, GET /file no longer mkdirs or leaks paths
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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: {
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user