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.
This commit is contained in:
@@ -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 };
|
||||
|
||||
Reference in New Issue
Block a user