From acff059e92b05f9f425ff3677ea8792de50945dc Mon Sep 17 00:00:00 2001 From: nntrivi2001 Date: Wed, 9 Sep 2026 01:17:06 +0700 Subject: [PATCH] fix(codex-config): close whole-branch review defects in the Codex panel Plugins/marketplaces GET routes were silently empty because codex-cli.js never unwrapped the CLI's {installed,available}/{marketplaces} objects into arrays; backup writes bypassed the symlink-escape guard entirely; readFileSafe was stricter than every other Codex reader, showing a symlinked skill/rule in the list and then refusing to open it; marketplaceAdd only accepted http(s) URLs, rejecting owner/repo sources the CLI itself takes. Also: docs drift, a dead i18n key, and a scope-cast hazard in the shared Editor/ConfirmDelete modals. Fixes, one per finding: unwrap installed/marketplaces in codex-cli.js so every list route keeps returning a bare list; guard the backup path in codex-mutate.js with the same assertNotSymlinkEscape the write path already used (Claude's cc-mutate.js has no such guard on its primary write path either, so left untouched there); relax readFileSafe's containment to lexical-only, matching the other four Codex readers, while leaving the write side's realpath check untouched; reuse cc-plugin-cli's assertSource for marketplaceAdd instead of a third validator (ssh remains out of scope, no credential story for it yet). Server: 1557/1557 (was 1548). Client: tsc -b clean, 469/469 tests. --- .../components/config/ConfirmDeleteModal.tsx | 26 ++++-- client/src/components/config/EditorModal.tsx | 42 +++++++-- client/src/i18n/locales/en/codexConfig.json | 3 +- client/src/i18n/locales/vi/codexConfig.json | 3 +- client/src/pages/CodexConfig.tsx | 15 ++-- docs/API.md | 11 ++- openapi.yaml | 20 +++-- server/README.md | 2 +- server/__tests__/codex-cli.test.js | 53 +++++++++++- server/__tests__/codex-config.test.js | 86 ++++++++++++++++--- server/__tests__/codex-mutate.test.js | 54 ++++++++++++ server/lib/codex-cli.js | 37 ++++++-- server/lib/codex-discovery.js | 47 ++++------ server/lib/codex-mutate.js | 26 ++++-- server/openapi-extra/codex-config.js | 33 +++++-- server/routes/codex-config.js | 9 +- 16 files changed, 359 insertions(+), 108 deletions(-) diff --git a/client/src/components/config/ConfirmDeleteModal.tsx b/client/src/components/config/ConfirmDeleteModal.tsx index 5df74986..3ee617b4 100644 --- a/client/src/components/config/ConfirmDeleteModal.tsx +++ b/client/src/components/config/ConfirmDeleteModal.tsx @@ -11,25 +11,33 @@ import { useTranslation } from "react-i18next"; import { ShieldAlert, Trash2 } from "lucide-react"; import type { CcArtifactType } from "../../lib/api"; -export type ConfirmDeleteState = { +/** See EditorModal.tsx's `DefaultScope` — same reasoning: Codex instantiates + * `TScope` as `"user" | "project"` instead of casting into Claude's wider + * default. */ +export type ConfirmDeleteState< + TType extends string = CcArtifactType, + TScope extends string = "user" | "project" | "auto-memory", +> = { type: TType; - scope: "user" | "project" | "auto-memory"; + scope: TScope; name?: string; path: string; project?: string; // set for type === "auto-memory" } | null; -interface ConfirmDeleteModalProps { - state: NonNullable>; +interface ConfirmDeleteModalProps< + TType extends string = CcArtifactType, + TScope extends string = "user" | "project" | "auto-memory", +> { + state: NonNullable>; onCancel: () => void; onConfirm: () => Promise; } -export function ConfirmDeleteModal({ - state, - onCancel, - onConfirm, -}: ConfirmDeleteModalProps) { +export function ConfirmDeleteModal< + TType extends string = CcArtifactType, + TScope extends string = "user" | "project" | "auto-memory", +>({ state, onCancel, onConfirm }: ConfirmDeleteModalProps) { const { t } = useTranslation("ccConfig"); const [busy, setBusy] = useState(false); diff --git a/client/src/components/config/EditorModal.tsx b/client/src/components/config/EditorModal.tsx index 75066ae0..3be95399 100644 --- a/client/src/components/config/EditorModal.tsx +++ b/client/src/components/config/EditorModal.tsx @@ -12,7 +12,16 @@ import { useTranslation } from "react-i18next"; import { Pencil, X, Save, AlertCircle } from "lucide-react"; import type { CcArtifactType } from "../../lib/api"; -export type EditorState = +/** Claude Code's own scope union, kept as the default so existing callers + * (CcConfig.tsx) don't need to name it. Codex has no `auto-memory` scope — + * its caller instantiates `TScope` as just `"user" | "project"` instead of + * casting its own narrower type into this one. */ +type DefaultScope = "user" | "project" | "auto-memory"; + +export type EditorState< + TType extends string = CcArtifactType, + TScope extends string = DefaultScope, +> = | { mode: "create"; type: TType; @@ -23,7 +32,7 @@ export type EditorState = | { mode: "edit"; type: TType; - scope: "user" | "project" | "auto-memory"; + scope: TScope; name: string; filePath: string; project?: string; // set for type === "auto-memory" @@ -34,13 +43,16 @@ export type EditorState = * AGENTS.md / config.toml) - no `name` field, and no artifact name is ever sent. */ const DEFAULT_NAMELESS_TYPES = ["memory"]; -interface EditorModalProps { - state: NonNullable>; +interface EditorModalProps< + TType extends string = CcArtifactType, + TScope extends string = DefaultScope, +> { + state: NonNullable>; fetchContent: (filePath: string) => Promise; onClose: () => void; onSave: (args: { type: TType; - targetScope: "user" | "project" | "auto-memory"; + targetScope: TScope; name: string | undefined; content: string; project?: string; @@ -50,13 +62,16 @@ interface EditorModalProps { namelessTypes?: readonly string[]; } -export function EditorModal({ +export function EditorModal< + TType extends string = CcArtifactType, + TScope extends string = DefaultScope, +>({ state, fetchContent, onClose, onSave, namelessTypes = DEFAULT_NAMELESS_TYPES, -}: EditorModalProps) { +}: EditorModalProps) { const { t } = useTranslation("ccConfig"); const isCreate = state.mode === "create"; const isNameless = namelessTypes.includes(state.type); @@ -64,7 +79,11 @@ export function EditorModal({ const [content, setContent] = useState(isCreate ? state.template : ""); const [name, setName] = useState(""); const [targetScope, setTargetScope] = useState<"user" | "project">( - isCreate ? state.defaultScope : state.scope === "auto-memory" ? "user" : state.scope + isCreate + ? state.defaultScope + : state.scope === "auto-memory" + ? "user" + : (state.scope as "user" | "project") ); const [loading, setLoading] = useState(!isCreate); const [saving, setSaving] = useState(false); @@ -111,7 +130,12 @@ export function EditorModal({ state.mode === "edit" ? state.name : isNameless ? undefined : createName; await onSave({ type: state.type, - targetScope: isAutoMemory ? "auto-memory" : targetScope, + // Widening back to TScope: this component is the one place that + // legitimately knows about all three scope shapes across both + // callers; a Codex caller's TScope ("user" | "project") never + // actually receives "auto-memory" since isAutoMemory can only be + // true when TType includes "auto-memory" (Claude-only). + targetScope: (isAutoMemory ? "auto-memory" : targetScope) as TScope, name: effectiveName, content, project: state.project, diff --git a/client/src/i18n/locales/en/codexConfig.json b/client/src/i18n/locales/en/codexConfig.json index 8ffef541..447db1c0 100644 --- a/client/src/i18n/locales/en/codexConfig.json +++ b/client/src/i18n/locales/en/codexConfig.json @@ -32,8 +32,7 @@ "common": { "search": "Search by name…", "clearSearch": "Clear search", - "close": "Close", - "empty": "Nothing here yet." + "close": "Close" }, "list": { "newButton": "New", diff --git a/client/src/i18n/locales/vi/codexConfig.json b/client/src/i18n/locales/vi/codexConfig.json index e31d6e9b..5b84dff9 100644 --- a/client/src/i18n/locales/vi/codexConfig.json +++ b/client/src/i18n/locales/vi/codexConfig.json @@ -32,8 +32,7 @@ "common": { "search": "Tìm theo tên…", "clearSearch": "Xóa tìm kiếm", - "close": "Đóng", - "empty": "Chưa có gì ở đây." + "close": "Đóng" }, "list": { "newButton": "Mới", diff --git a/client/src/pages/CodexConfig.tsx b/client/src/pages/CodexConfig.tsx index b00f872b..fc3d84f1 100644 --- a/client/src/pages/CodexConfig.tsx +++ b/client/src/pages/CodexConfig.tsx @@ -159,8 +159,9 @@ export function CodexConfig() { data: CodexFileResponse | null; error: string | null; } | null>(null); - const [editor, setEditor] = useState>(null); - const [confirmDelete, setConfirmDelete] = useState>(null); + const [editor, setEditor] = useState>(null); + const [confirmDelete, setConfirmDelete] = + useState>(null); const [toast, setToast] = useState(null); useEffect(() => { @@ -299,12 +300,12 @@ export function CodexConfig() { const handleSave = useCallback( async (args: { type: CodexArtifactType; - targetScope: "user" | "project" | "auto-memory"; + targetScope: "user" | "project"; name: string | undefined; content: string; }) => { const result: CodexMutationResult = await api.codexConfig.write({ - scope: args.targetScope as "user" | "project", + scope: args.targetScope, type: args.type, name: args.name, content: args.content, @@ -325,7 +326,7 @@ export function CodexConfig() { if (!confirmDelete) return; try { const result = await api.codexConfig.remove({ - scope: confirmDelete.scope as "user" | "project", + scope: confirmDelete.scope, type: confirmDelete.type, name: confirmDelete.name, }); @@ -413,7 +414,7 @@ export function CodexConfig() { )} {viewer && setViewer(null)} />} {editor && ( - + state={editor} fetchContent={(p) => api.codexConfig.file(p).then((r) => r.text)} onClose={() => setEditor(null)} @@ -422,7 +423,7 @@ export function CodexConfig() { /> )} {confirmDelete && ( - + state={confirmDelete} onCancel={() => setConfirmDelete(null)} onConfirm={handleDelete} diff --git a/docs/API.md b/docs/API.md index 34b502ab..478b7942 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1646,7 +1646,7 @@ POST /api/codex-config/marketplaces/action Body: { name, action: remove } `type` is one of `skills`, `agents`, `rules`, `memory`, `config`. `name` is required for `skills`/`agents`/`rules` and forbidden for `memory`/`config` (each is a single fixed file per scope — `AGENTS.md` and `config.toml` respectively). `PUT`/`DELETE` always back the previous content up first (`server/lib/codex-mutate.js`) and always resolve `project` scope against the **dashboard server's own cwd** — unlike the read routes, there is no `?cwd=` override on a write, for the same reason `cc-config.js`'s `PATCH /settings` refuses one. Mutation errors: `EBADTYPE`/`EBADSCOPE`/`EBADNAME`/`EBADCONTENT`/`EOUTOFROOT` (400), `ETOOLARGE` (413), `ENOTFOUND` (404). Every successful write/delete broadcasts `codex_config_changed`. -`GET /mcp`, `/plugins`, `/marketplaces` proxy `codex mcp list --json` / `codex plugin list --json` / `codex marketplace list --json`; a fresh install with none of a kind returns `{ mcp: [] }` etc. rather than a parse error, and the client further coerces a non-array response to `[]` defensively. The add/remove routes shell out to `codex …` with an explicit argv array (never a shell string); a non-zero CLI exit is reported as **422 `ECLIEXIT`** with `stdout`/`stderr` preserved, never a 500. `POST /mcp/add` takes either `{ name, url, bearerTokenEnvVar? }` (HTTP transport) or `{ name, command, args?, env? }` (stdio transport) — never both, mirroring `codex mcp add`'s own mutually-exclusive flags. Other CLI errors: `EBADARG` (400), `ECLINOTFOUND` (503 — the `codex` binary isn't on `PATH`), `ETIMEOUT` (504). +`GET /mcp`, `/plugins`, `/marketplaces` proxy `codex mcp list --json` / `codex plugin list --json` / `codex plugin marketplace list --json`; a fresh install with none of a kind returns `{ mcp: [] }` etc. rather than a parse error. Unlike `mcp list --json` (a bare array), `plugin list --json` and `plugin marketplace list --json` return `{installed, available}` / `{marketplaces}` objects — `server/lib/codex-cli.js` unwraps those down to a plain array before the route ever sees them, so every list route's contract stays "the named field is a list"; the client's `Array.isArray` coercion is only a defensive fallback for CLI output that fails to parse at all. The add/remove routes shell out to `codex …` with an explicit argv array (never a shell string); a non-zero CLI exit is reported as **422 `ECLIEXIT`** with `stdout`/`stderr` preserved, never a 500. `POST /mcp/add` takes either `{ name, url, bearerTokenEnvVar? }` (HTTP transport) or `{ name, command, args?, env? }` (stdio transport) — never both, mirroring `codex mcp add`'s own mutually-exclusive flags. `POST /marketplaces/add`'s `url` field is misnamed for history but accepts the same source shapes as `codex plugin marketplace add`: `owner/repo`, an http(s) git URL, or an absolute local path (ssh remotes are out of scope). Other CLI errors: `EBADARG` (400), `ECLINOTFOUND` (503 — the `codex` binary isn't on `PATH`), `ETIMEOUT` (504). Because `CodexMcpServer`/`CodexPlugin`/`CodexMarketplace` are opaque JSON blobs from the CLI (`[key: string]: unknown`) rather than a shape the dashboard controls, the Codex tab renders them as generic key/value cards instead of the richly-typed plugin/MCP cards the Claude Code tab shows. @@ -1868,6 +1868,15 @@ Broadcast whenever Claude Code configuration changes — either by dashboard mut { "type": "cc_config_changed", "data": { "source": "fs", "paths": ["/Users/foo/.claude/settings.json"] } } ``` +#### codex_config_changed + +The Codex counterpart of `cc_config_changed` — broadcast by `routes/codex-config.js` after every successful mutation: `PUT`/`DELETE /api/codex-config/file`, and the CLI-backed `mcp/add`, `mcp/remove`, `plugins/action`, `marketplaces/add`, `marketplaces/action` routes. Always `source: "dashboard"` — there is no filesystem watcher on `$CODEX_HOME` (unlike the Claude side's `cc-watcher.js`). The Codex Config page subscribes and refetches automatically. + +```json +{ "type": "codex_config_changed", "data": { "source": "dashboard", "action": "write", "scope": "user", "type": "rules", "name": "default" } } +{ "type": "codex_config_changed", "data": { "source": "dashboard", "action": "add", "type": "mcp", "name": "docs" } } +``` + #### remote_data.updated Broadcast once per successful remote sync (background poller, manual **Sync now**, or immediate pull after add/re-enable). Clients use this — and the per-session `session_created` / `session_updated` frames emitted in the same pass — to refetch sessions, costs, and analytics without polling. diff --git a/openapi.yaml b/openapi.yaml index c6a45277..b76a17ff 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -9249,17 +9249,19 @@ paths: tags: - CodexConfig summary: List installed plugins - description: Reads live from `codex plugin list --json`. + description: Reads live from `codex plugin list --json` and unwraps its `{installed, available}` object down to the `installed` array — the route's `plugins` field is always a list. operationId: codexConfigGetPlugins responses: '200': - description: Plugins as reported by the CLI. + description: Installed plugins as reported by the CLI. content: application/json: schema: type: object properties: - plugins: {} + plugins: + type: array + items: {} '422': description: '`ECLIEXIT` — the `codex` CLI exited non-zero. stdout/stderr are preserved.' content: @@ -9312,7 +9314,7 @@ paths: tags: - CodexConfig summary: List registered marketplaces - description: Reads live from `codex plugin marketplace list --json`. + description: Reads live from `codex plugin marketplace list --json` and unwraps its `{marketplaces}` object down to a bare array. operationId: codexConfigGetMarketplaces responses: '200': @@ -9322,7 +9324,9 @@ paths: schema: type: object properties: - marketplaces: {} + marketplaces: + type: array + items: {} '422': description: '`ECLIEXIT` — the `codex` CLI exited non-zero. stdout/stderr are preserved.' content: @@ -9582,7 +9586,7 @@ paths: tags: - CodexConfig summary: Register a marketplace - description: Runs `codex plugin marketplace add `. + description: Runs `codex plugin marketplace add `. `source` is an owner/repo shorthand, an http(s) git URL, or an absolute local path — the same set `claude plugin marketplace add` accepts; ssh remotes are not. operationId: codexConfigPostMarketplacesAdd requestBody: required: true @@ -9596,7 +9600,7 @@ paths: url: type: string example: - url: https://example.com/marketplace.json + url: openai/codex responses: '200': description: Marketplace added. @@ -9612,7 +9616,7 @@ paths: example: error: code: EBADARG - message: marketplace url must be a valid http(s) URL + message: source must be owner/repo, an http(s) URL, or an absolute path '422': description: '`ECLIEXIT` — the `codex` CLI exited non-zero. stdout/stderr are preserved.' content: diff --git a/server/README.md b/server/README.md index 7686fdcb..6f4c3500 100644 --- a/server/README.md +++ b/server/README.md @@ -699,7 +699,7 @@ HTTP surface for the tmux-backed terminal run: one session per lane, named `agen Codex-only spawn options (`sandbox`, `approval`, `resumeLast`) are accepted by the lane action `POST /api/lanes/:id/start`, not by `POST /api/run` — see [`docs/API.md`](../docs/API.md). -The pane itself is not JSON: its bytes stream over `/ws-pty/:runId` as binary WebSocket frames. The JSON WebSocket surface adds `cc_config_changed` (broadcast by `lib/cc-watcher.js` on `fs.watch` events under `~/.claude/` and by `routes/cc-config.js` after every successful PUT/DELETE — debounced at 500 ms, payload `{ source: "dashboard"|"fs", action?, scope?, type?, name?, paths? }`). +The pane itself is not JSON: its bytes stream over `/ws-pty/:runId` as binary WebSocket frames. The JSON WebSocket surface adds `cc_config_changed` (broadcast by `lib/cc-watcher.js` on `fs.watch` events under `~/.claude/` and by `routes/cc-config.js` after every successful PUT/DELETE — debounced at 500 ms, payload `{ source: "dashboard"|"fs", action?, scope?, type?, name?, paths? }`) and its Codex counterpart `codex_config_changed` (broadcast by `routes/codex-config.js` after every successful file mutation and CLI-backed mcp/plugin/marketplace action — always `source: "dashboard"`, there is no `$CODEX_HOME` filesystem watcher). ### Import History diff --git a/server/__tests__/codex-cli.test.js b/server/__tests__/codex-cli.test.js index 37fb58f0..77e8448e 100644 --- a/server/__tests__/codex-cli.test.js +++ b/server/__tests__/codex-cli.test.js @@ -72,7 +72,7 @@ describe("codex-cli argument validation", () => { } }); - it("accepts only http(s) URLs", () => { + it("accepts only http(s) URLs (mcpAdd's --url form)", () => { assert.equal(cli.assertUrl("https://example.com/mp.git", "url"), "https://example.com/mp.git"); for (const bad of ["ssh://example.com/mp.git", "not a url", "file:///tmp/x"]) { assert.throws(() => cli.assertUrl(bad, "url"), /http\(s\)/, `accepted ${bad}`); @@ -81,6 +81,32 @@ describe("codex-cli argument validation", () => { assert.throws(() => cli.assertUrl("--force", "url"), /may not start with/); }); + it("marketplaceAdd accepts the source forms `codex plugin marketplace add` actually takes", async () => { + // Not assertUrl: `codex plugin marketplace add ` also takes + // owner/repo and a local path, which a bare http(s)-only check rejected. + const restore = stubCodex(); + try { + assert.deepEqual(await cli.marketplaceAdd("openai/codex").then(argvOf), [ + "plugin", + "marketplace", + "add", + "openai/codex", + ]); + assert.deepEqual(await cli.marketplaceAdd("/abs/local/path").then(argvOf), [ + "plugin", + "marketplace", + "add", + "/abs/local/path", + ]); + } finally { + restore(); + } + // ssh remotes are deliberately out of scope, same as cc-plugin-cli's + // marketplace source guard — no credential/host-key story for them yet. + await assert.rejects(() => cli.marketplaceAdd("git@github.com:owner/repo.git"), /source/); + await assert.rejects(() => cli.marketplaceAdd("../escape"), /source/); + }); + it("rejects mcpAdd with neither or both of url/command", async () => { await assert.rejects(() => cli.mcpAdd({ name: "docs" }), /requires either/); await assert.rejects( @@ -267,6 +293,31 @@ describe("codex-cli run outcomes", () => { } }); + it("unwraps `codex plugin list --json`'s {installed, available} object into a bare array", async () => { + // Real codex-cli 0.149.0 shape — an object, unlike `mcp list`'s array. + const restore = stubCodex( + `cat <<'JSON'\n{"installed":[{"name":"demo"}],"available":[{"name":"other"}]}\nJSON` + ); + try { + const res = await cli.pluginList(); + assert.equal(res.ok, true); + assert.deepEqual(res.data, [{ name: "demo" }]); + } finally { + restore(); + } + }); + + it("unwraps `codex plugin marketplace list --json`'s {marketplaces} object into a bare array", async () => { + const restore = stubCodex(`cat <<'JSON'\n{"marketplaces":[{"name":"sg"}]}\nJSON`); + try { + const res = await cli.marketplaceList(); + assert.equal(res.ok, true); + assert.deepEqual(res.data, [{ name: "sg" }]); + } finally { + restore(); + } + }); + it("leaves `data` undefined when stdout is not JSON", async () => { const restore = stubCodex('echo "removed docs"'); try { diff --git a/server/__tests__/codex-config.test.js b/server/__tests__/codex-config.test.js index 5593afef..7294f82e 100644 --- a/server/__tests__/codex-config.test.js +++ b/server/__tests__/codex-config.test.js @@ -211,36 +211,50 @@ 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 () => { + it("file endpoint blocks a ?cwd=-driven escape (path outside the roots ?cwd= computes)", async () => { + const otherCwd = fs.mkdtempSync(path.join(os.tmpdir(), "codex-config-othercwd-")); + const { status, body } = await fetchJson( + `/api/codex-config/file?cwd=${encodeURIComponent(otherCwd)}&path=${encodeURIComponent( + path.join(FAKE_HOME, "..", "..", "etc", "passwd") + )}` + ); + assert.equal(status, 400); + assert.equal(body.error.code, "READ_DENIED"); + fs.rmSync(otherCwd, { recursive: true, force: true }); + }); + + it("file endpoint follows a leaf symlink whose LOCATION is inside CODEX_HOME (dotfiles setup)", async () => { + // Ruling: containment applies to the link's location, not where it + // points — the same thing the other four discovery readers already do. const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-config-outside-")); - fs.writeFileSync(path.join(outside, "secret.rules"), "TOP SECRET LEAF"); + fs.writeFileSync(path.join(outside, "secret.rules"), "MY DOTFILES RULE"); 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/); + assert.equal(status, 200); + assert.equal(body.ok, true); + assert.equal(body.text, "MY DOTFILES RULE"); } finally { fs.rmSync(outside, { recursive: true, force: true }); } }); - it("file endpoint refuses an ancestor symlink that escapes CODEX_HOME (no secret leaks)", async () => { + it("file endpoint follows an ancestor symlink whose LOCATION is inside CODEX_HOME", 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.writeFileSync(path.join(outside, "linked.txt"), "MY DOTFILES ANCESTOR"); + const linkDir = path.join(FAKE_HOME, "linked-dir"); fs.symlinkSync(outside, linkDir, "dir"); try { - const target = path.join(linkDir, "secret.txt"); + const target = path.join(linkDir, "linked.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/); + assert.equal(status, 200); + assert.equal(body.ok, true); + assert.equal(body.text, "MY DOTFILES ANCESTOR"); } finally { fs.rmSync(linkDir, { force: true }); fs.rmSync(outside, { recursive: true, force: true }); @@ -470,6 +484,33 @@ describe("/api/codex-config", () => { } }); + it("GET /plugins unwraps the CLI's {installed, available} object into a bare array", async () => { + // Real codex-cli 0.149.0 shape: `codex plugin list --json` returns an + // object, not an array like `mcp list` — without the unwrap the Plugins + // tab silently showed nothing (Array.isArray(res.plugins) was false). + const restore = stubCodex( + `cat <<'JSON'\n{"installed":[{"name":"demo"}],"available":[{"name":"other"}]}\nJSON` + ); + try { + const { status, body } = await fetchJson("/api/codex-config/plugins"); + assert.equal(status, 200); + assert.deepEqual(body.plugins, [{ name: "demo" }]); + } finally { + restore(); + } + }); + + it("GET /marketplaces unwraps the CLI's {marketplaces} object into a bare array", async () => { + const restore = stubCodex(`cat <<'JSON'\n{"marketplaces":[{"name":"sg"}]}\nJSON`); + try { + const { status, body } = await fetchJson("/api/codex-config/marketplaces"); + assert.equal(status, 200); + assert.deepEqual(body.marketplaces, [{ name: "sg" }]); + } finally { + restore(); + } + }); + 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 { @@ -589,7 +630,26 @@ describe("/api/codex-config", () => { } }); - it("POST /marketplaces/add rejects a non-http(s) url", async () => { + it("POST /marketplaces/add accepts an owner/repo source (not just http(s) urls)", async () => { + const restore = stubCodex(ECHO_ARGV); + try { + const { status, body } = await fetchJson("/api/codex-config/marketplaces/add", { + method: "POST", + body: { url: "openai/codex" }, + }); + assert.equal(status, 200); + assert.deepEqual(body.stdout.trim().split("\n"), [ + "plugin", + "marketplace", + "add", + "openai/codex", + ]); + } finally { + restore(); + } + }); + + it("POST /marketplaces/add rejects a source that is none of path/owner-repo/http(s) url", async () => { const { status, body } = await fetchJson("/api/codex-config/marketplaces/add", { method: "POST", body: { url: "not a url" }, diff --git a/server/__tests__/codex-mutate.test.js b/server/__tests__/codex-mutate.test.js index f4f7438f..d145210d 100644 --- a/server/__tests__/codex-mutate.test.js +++ b/server/__tests__/codex-mutate.test.js @@ -266,6 +266,60 @@ describe("codex-mutate", () => { } }); + it("refuses to write (and back up) when /codex-config-backups is a symlink escaping the root", () => { + cm.writeArtifact({ scope: "user", type: "rules", name: "toclobber", content: "v1", cwd: REPO }); + const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-outside-")); + const backupsLink = path.join(TMP, "codex-config-backups"); + fs.rmSync(backupsLink, { recursive: true, force: true }); + fs.symlinkSync(outside, backupsLink, "dir"); + try { + assert.throws( + () => + cm.writeArtifact({ + scope: "user", + type: "rules", + name: "toclobber", + content: "v2 — attacker wants this in `outside`", + cwd: REPO, + }), + (e) => e.code === "EOUTOFROOT" + ); + assert.equal(fs.readdirSync(outside).length, 0, "backup must not land outside the root"); + } finally { + fs.rmSync(backupsLink, { force: true }); + fs.rmSync(outside, { recursive: true, force: true }); + } + }); + + it("refuses to delete (and back up) when /codex-config-backups is a symlink escaping the root", () => { + cm.writeArtifact({ + scope: "user", + type: "rules", + name: "todelete2", + content: "keep-me", + cwd: REPO, + }); + const outside = fs.mkdtempSync(path.join(os.tmpdir(), "codex-mut-outside-")); + const backupsLink = path.join(TMP, "codex-config-backups"); + fs.rmSync(backupsLink, { recursive: true, force: true }); + fs.symlinkSync(outside, backupsLink, "dir"); + try { + assert.throws( + () => cm.deleteArtifact({ scope: "user", type: "rules", name: "todelete2", cwd: REPO }), + (e) => e.code === "EOUTOFROOT" + ); + assert.equal(fs.readdirSync(outside).length, 0, "backup must not land outside the root"); + assert.equal( + fs.readFileSync(path.join(TMP, "rules", "todelete2.rules"), "utf8"), + "keep-me", + "the original file must survive a refused backup" + ); + } finally { + fs.rmSync(backupsLink, { force: true }); + fs.rmSync(outside, { 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`); diff --git a/server/lib/codex-cli.js b/server/lib/codex-cli.js index 1f83223e..7236e3bc 100644 --- a/server/lib/codex-cli.js +++ b/server/lib/codex-cli.js @@ -30,6 +30,8 @@ const { execFile } = require("node:child_process"); const { getCodexHome } = require("./codex-home"); +// Reused, not reimplemented — see marketplaceAdd's doc comment. +const { assertSource } = require("./cc-plugin-cli"); const DEFAULT_TIMEOUT_MS = 120_000; const MAX_OUTPUT_BYTES = 1024 * 1024; @@ -230,8 +232,18 @@ async function mcpRemove(name) { // ── Plugins ──────────────────────────────────────────────────────────── +/** + * `codex plugin list --json` returns `{installed: [...], available: [...]}`, + * not a bare array — unlike `mcp list`. The installed list is what the + * Plugins tab shows; unwrap it here so `cliRead` (codex-config.js) can keep + * treating every list route the same way: `result.data` is the list. + */ async function pluginList() { - return runCodex(["plugin", "list", "--json"]); + const result = await runCodex(["plugin", "list", "--json"]); + if (result.data !== undefined && !Array.isArray(result.data)) { + result.data = Array.isArray(result.data.installed) ? result.data.installed : []; + } + return result; } async function pluginAdd(spec) { @@ -249,13 +261,28 @@ async function pluginRemove(spec) { // ── Marketplaces ─────────────────────────────────────────────────────── +/** Same object-not-array shape as `pluginList` — unwrap `marketplaces`. */ async function marketplaceList() { - return runCodex(["plugin", "marketplace", "list", "--json"]); + const result = await runCodex(["plugin", "marketplace", "list", "--json"]); + if (result.data !== undefined && !Array.isArray(result.data)) { + result.data = Array.isArray(result.data.marketplaces) ? result.data.marketplaces : []; + } + return result; } -async function marketplaceAdd(url) { - const u = assertUrl(url, "marketplace url"); - return serialize(() => runCodex(["plugin", "marketplace", "add", u])); +/** + * `codex plugin marketplace add ` accepts a local path, `owner/repo`, + * an https git URL, or an ssh git URL — not just http(s), so `assertUrl` + * rejected the common `owner/repo` form. Reuse cc-plugin-cli's `assertSource` + * (same shape `claude plugin marketplace add` accepts) instead of writing a + * third validator. That guard also excludes ssh remotes and `..` — kept as + * is here too: ssh needs its own credential/host-key handling this dashboard + * has no story for yet, and every other CLI-facing source guard in this repo + * already draws that same line. + */ +async function marketplaceAdd(source) { + const s = assertSource(source); + return serialize(() => runCodex(["plugin", "marketplace", "add", s])); } async function marketplaceRemove(name) { diff --git a/server/lib/codex-discovery.js b/server/lib/codex-discovery.js index b2b3a8f7..c78de06a 100644 --- a/server/lib/codex-discovery.js +++ b/server/lib/codex-discovery.js @@ -22,11 +22,6 @@ 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 { @@ -296,17 +291,25 @@ function readOverview(opts = {}) { /** * 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. + * Mirrors cc-discovery's readFileSafe. * - * 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. + * Containment here is lexical — `isUnder` after `path.resolve`, never a + * realpath check — and deliberately so: a path whose LOCATION lies inside a + * root is readable even when it is a symlink resolving somewhere else. The + * other four Codex readers (readSkills/readAgents/readRules/readMemory/ + * readConfig) already follow such symlinks — a symlinked dotfiles skill or + * config is the normal setup — so this endpoint refusing the same path + * would show a file in the list, preview its body, and then bare-`READ_DENIED` + * it on click. The write side (codex-mutate.js) keeps its stricter + * realpath-based `assertNotSymlinkEscape`: reading through a link shows you + * your own file, writing through one clobbers something unrelated — that + * asymmetry is intentional, not an inconsistency to "fix" here. + * + * A `path` that resolves outside the roots (e.g. `../../etc/passwd`) or a + * `?cwd=`-driven escape are still refused by the lexical `isUnder` checks + * below — only the symlink-target check is relaxed. 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}} @@ -327,20 +330,6 @@ function readFileSafe(absPath, opts = {}) { 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, { create: false }); - } catch (err) { - // 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 { notFound: true }; return { ok: true, file: resolved, ...r }; diff --git a/server/lib/codex-mutate.js b/server/lib/codex-mutate.js index 2d593e9c..13fe4768 100644 --- a/server/lib/codex-mutate.js +++ b/server/lib/codex-mutate.js @@ -209,14 +209,20 @@ function writeArtifact(args) { assertNotSymlinkEscape(r.trustedRoot, r.target); const existedBefore = fs.existsSync(r.filePath); - const backup = existedBefore - ? createBackup({ - source: r.kind === "dir" ? r.target : r.filePath, - backupRoot: backupRootFor(scope, type, { cwd }), - type, - name: path.basename(r.target), - }) - : null; + let backup = null; + if (existedBefore) { + const backupRoot = backupRootFor(scope, type, { cwd }); + // The backup write never went through the symlink-escape guard: a + // `/codex-config-backups` symlink would send the previous file's + // body wherever it points. Same check as the target above, same reason. + assertNotSymlinkEscape(r.trustedRoot, backupRoot); + backup = createBackup({ + source: r.kind === "dir" ? r.target : r.filePath, + backupRoot, + type, + name: path.basename(r.target), + }); + } if (r.kind === "dir") { fs.mkdirSync(r.target, { recursive: true }); @@ -240,9 +246,11 @@ function deleteArtifact(args) { } assertNotSymlinkEscape(r.trustedRoot, r.target); + const backupRoot = backupRootFor(scope, type, { cwd }); + assertNotSymlinkEscape(r.trustedRoot, backupRoot); const backup = createBackup({ source: r.target, - backupRoot: backupRootFor(scope, type, { cwd }), + backupRoot, type, name: path.basename(r.target), }); diff --git a/server/openapi-extra/codex-config.js b/server/openapi-extra/codex-config.js index 091c71ea..ddec6cb5 100644 --- a/server/openapi-extra/codex-config.js +++ b/server/openapi-extra/codex-config.js @@ -527,13 +527,17 @@ const paths = { get: { tags: ["CodexConfig"], summary: "List installed plugins", - description: "Reads live from `codex plugin list --json`.", + description: + "Reads live from `codex plugin list --json` and unwraps its `{installed, available}` " + + "object down to the `installed` array — the route's `plugins` field is always a list.", operationId: "codexConfigGetPlugins", responses: { 200: { - description: "Plugins as reported by the CLI.", + description: "Installed plugins as reported by the CLI.", content: { - "application/json": { schema: { type: "object", properties: { plugins: {} } } }, + "application/json": { + schema: { type: "object", properties: { plugins: { type: "array", items: {} } } }, + }, }, }, 422: cliExitResponse, @@ -547,13 +551,20 @@ const paths = { get: { tags: ["CodexConfig"], summary: "List registered marketplaces", - description: "Reads live from `codex plugin marketplace list --json`.", + description: + "Reads live from `codex plugin marketplace list --json` and unwraps its " + + "`{marketplaces}` object down to a bare array.", operationId: "codexConfigGetMarketplaces", responses: { 200: { description: "Marketplaces as reported by the CLI.", content: { - "application/json": { schema: { type: "object", properties: { marketplaces: {} } } }, + "application/json": { + schema: { + type: "object", + properties: { marketplaces: { type: "array", items: {} } }, + }, + }, }, }, 422: cliExitResponse, @@ -678,14 +689,17 @@ const paths = { post: { tags: ["CodexConfig"], summary: "Register a marketplace", - description: "Runs `codex plugin marketplace add `.", + description: + "Runs `codex plugin marketplace add `. `source` is an owner/repo shorthand, " + + "an http(s) git URL, or an absolute local path — the same set `claude plugin " + + "marketplace add` accepts; ssh remotes are not.", operationId: "codexConfigPostMarketplacesAdd", requestBody: { required: true, content: { "application/json": { schema: { type: "object", required: ["url"], properties: { url: { type: "string" } } }, - example: { url: "https://example.com/marketplace.json" }, + example: { url: "openai/codex" }, }, }, }, @@ -698,7 +712,10 @@ const paths = { }, }, }, - 400: errorResponse("EBADARG", "marketplace url must be a valid http(s) URL"), + 400: errorResponse( + "EBADARG", + "source must be owner/repo, an http(s) URL, or an absolute path" + ), 422: cliExitResponse, }, }, diff --git a/server/routes/codex-config.js b/server/routes/codex-config.js index 896a9e7b..a28d2642 100644 --- a/server/routes/codex-config.js +++ b/server/routes/codex-config.js @@ -97,10 +97,11 @@ 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. 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. +// checks that containment lexically — a symlink whose LOCATION is inside a +// root is readable even when it resolves outside, matching the other four +// discovery readers. The write side (codex-mutate.js) keeps the stricter +// realpath-based assertNotSymlinkEscape — reads and writes intentionally +// disagree on symlinks; see readFileSafe's doc comment for why. router.get("/file", (req, res) => { const p = req.query.path; if (typeof p !== "string" || !p) {