diff --git a/server/__tests__/cc-config.test.js b/server/__tests__/cc-config.test.js index 0edc21ae..84566466 100644 --- a/server/__tests__/cc-config.test.js +++ b/server/__tests__/cc-config.test.js @@ -551,16 +551,70 @@ describe("/api/cc-config", () => { }); it("PUT /file edits memory CLAUDE.md without a name", async () => { - const { status, body } = await fetchJson( - `/api/cc-config/file?cwd=${encodeURIComponent(FAKE_PROJECT)}`, - { + // chdir, not `?cwd=`: project-scope writes resolve against the SERVER's + // cwd on purpose, so a request cannot pick the destination. Passing + // `?cwd=` here would have written into the repo's own CLAUDE.md. + const prev = process.cwd(); + process.chdir(FAKE_PROJECT); + try { + const { status, body } = await fetchJson("/api/cc-config/file", { method: "PUT", body: { scope: "project", type: "memory", content: "# new project memory" }, - } - ); - assert.equal(status, 200); - assert.ok(body.backupPath, "previous CLAUDE.md should be backed up"); - assert.equal(fs.readFileSync(body.file, "utf8"), "# new project memory"); + }); + assert.equal(status, 200); + assert.ok(body.backupPath, "previous CLAUDE.md should be backed up"); + assert.equal(fs.readFileSync(body.file, "utf8"), "# new project memory"); + assert.equal(body.file, path.join(FAKE_PROJECT, "CLAUDE.md")); + } finally { + process.chdir(prev); + } + }); + + it("PUT /file ignores ?cwd= — a caller cannot choose where a write lands", async () => { + // The hole this closes: `?cwd=` with scope "project" dropped a + // file into any directory on disk. PATCH /settings already refused it; + // PUT/DELETE did not. + const victim = path.join(TMP, "victim-project"); + fs.mkdirSync(victim, { recursive: true }); + fs.writeFileSync(path.join(victim, "CLAUDE.md"), "UNTOUCHED"); + + const prev = process.cwd(); + process.chdir(FAKE_PROJECT); + try { + const { status, body } = await fetchJson( + `/api/cc-config/file?cwd=${encodeURIComponent(victim)}`, + { + method: "PUT", + body: { scope: "project", type: "memory", content: "PWNED" }, + } + ); + assert.equal(status, 200); + assert.equal(body.file, path.join(FAKE_PROJECT, "CLAUDE.md"), "must ignore the query cwd"); + assert.equal(fs.readFileSync(path.join(victim, "CLAUDE.md"), "utf8"), "UNTOUCHED"); + } finally { + process.chdir(prev); + } + }); + + it("DELETE /file ignores ?cwd= too", async () => { + const victim = path.join(TMP, "victim-delete"); + fs.mkdirSync(victim, { recursive: true }); + fs.writeFileSync(path.join(victim, "CLAUDE.md"), "UNTOUCHED"); + fs.writeFileSync(path.join(FAKE_PROJECT, "CLAUDE.md"), "# server-cwd memory"); + + const prev = process.cwd(); + process.chdir(FAKE_PROJECT); + try { + const { status } = await fetchJson(`/api/cc-config/file?cwd=${encodeURIComponent(victim)}`, { + method: "DELETE", + body: { scope: "project", type: "memory" }, + }); + assert.equal(status, 200); + assert.equal(fs.existsSync(path.join(victim, "CLAUDE.md")), true, "victim must survive"); + assert.equal(fs.existsSync(path.join(FAKE_PROJECT, "CLAUDE.md")), false); + } finally { + process.chdir(prev); + } }); it("DELETE /file backs up and removes a single-file agent", async () => { diff --git a/server/__tests__/cc-mutate-containment.test.js b/server/__tests__/cc-mutate-containment.test.js new file mode 100644 index 00000000..73b772b4 --- /dev/null +++ b/server/__tests__/cc-mutate-containment.test.js @@ -0,0 +1,143 @@ +/** + * @file cc-mutate-containment.test.js + * @description Path-containment tests for cc-mutate.js's artifact writer. + * + * `isUnder` compares strings, which a symlink defeats: a link planted at + * `~/.claude/skills/` — or at any ancestor of it — is lexically inside + * the root while the write lands wherever the link points. The Codex writer + * hit both variants (leaf first, then ancestor on a fresh create, where + * realpath fails with ENOENT and a naive check waves it through), and the + * backup DESTINATION is a third path with the same exposure. + * + * Every case here asserts the refusal AND that nothing appeared outside the + * root — a thrown error with the write already done is not a fix. + * @author Nguyễn Ngọc Trí Vĩ + */ + +const { describe, it, beforeEach, after } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const path = require("node:path"); +const os = require("node:os"); + +const TMP = fs.mkdtempSync(path.join(os.tmpdir(), "cc-mutate-containment-")); +const FAKE_HOME = path.join(TMP, "home", ".claude"); +const OUTSIDE = path.join(TMP, "outside"); + +fs.mkdirSync(FAKE_HOME, { recursive: true }); +fs.mkdirSync(OUTSIDE, { recursive: true }); + +process.env.CLAUDE_HOME = FAKE_HOME; +const ccMutate = require("../lib/cc-mutate"); + +after(() => fs.rmSync(TMP, { recursive: true, force: true, maxRetries: 5 })); + +beforeEach(() => { + fs.rmSync(FAKE_HOME, { recursive: true, force: true }); + fs.rmSync(OUTSIDE, { recursive: true, force: true }); + fs.mkdirSync(FAKE_HOME, { recursive: true }); + fs.mkdirSync(OUTSIDE, { recursive: true }); +}); + +const write = (over = {}) => + ccMutate.writeArtifact({ + scope: "user", + type: "skills", + name: "victim", + content: "NEW", + ...over, + }); + +describe("cc-mutate: symlink containment on the write path", () => { + it("refuses a leaf symlink pointing out of the root, and writes nothing there", () => { + const outsideDir = path.join(OUTSIDE, "target"); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(outsideDir, "SKILL.md"), "ORIGINAL"); + fs.mkdirSync(path.join(FAKE_HOME, "skills"), { recursive: true }); + fs.symlinkSync(outsideDir, path.join(FAKE_HOME, "skills", "victim")); + + assert.throws( + () => write(), + (e) => e.code === "EOUTOFROOT" + ); + assert.equal(fs.readFileSync(path.join(outsideDir, "SKILL.md"), "utf8"), "ORIGINAL"); + }); + + it("refuses a symlinked ANCESTOR even when the artifact itself is new", () => { + // The case a leaf-only check misses: realpath on a not-yet-existing leaf + // throws ENOENT, so the walk has to climb to the symlinked parent. + fs.symlinkSync(OUTSIDE, path.join(FAKE_HOME, "skills")); + + assert.throws( + () => write({ name: "fresh" }), + (e) => e.code === "EOUTOFROOT" + ); + assert.equal(fs.existsSync(path.join(OUTSIDE, "fresh")), false); + }); + + it("refuses to back up through a symlinked backup root", () => { + // rmSync/unlink only remove the link, but the backup runs first and + // copies THROUGH it — so the destination needs the guard too. + fs.mkdirSync(path.join(FAKE_HOME, "skills", "victim"), { recursive: true }); + fs.writeFileSync(path.join(FAKE_HOME, "skills", "victim", "SKILL.md"), "OLD"); + fs.symlinkSync(OUTSIDE, path.join(FAKE_HOME, "cc-config-backups")); + + assert.throws( + () => write(), + (e) => e.code === "EOUTOFROOT" + ); + assert.deepEqual(fs.readdirSync(OUTSIDE), []); + // the original is untouched — the backup failing must not have written + assert.equal( + fs.readFileSync(path.join(FAKE_HOME, "skills", "victim", "SKILL.md"), "utf8"), + "OLD" + ); + }); + + it("refuses a delete whose target is a symlink out of the root", () => { + const outsideFile = path.join(OUTSIDE, "SKILL.md"); + fs.writeFileSync(outsideFile, "ORIGINAL"); + fs.mkdirSync(path.join(FAKE_HOME, "skills"), { recursive: true }); + fs.symlinkSync(OUTSIDE, path.join(FAKE_HOME, "skills", "victim")); + + assert.throws( + () => ccMutate.deleteArtifact({ scope: "user", type: "skills", name: "victim" }), + (e) => e.code === "EOUTOFROOT" + ); + assert.equal(fs.readFileSync(outsideFile, "utf8"), "ORIGINAL"); + }); +}); + +describe("cc-mutate: symlinks that stay inside the root still work", () => { + it("writes through a symlink whose target is inside the root", () => { + // Reorganising within ~/.claude is legitimate; the rule is where the path + // lands, not whether a link was involved. + const real = path.join(FAKE_HOME, "real-skills"); + fs.mkdirSync(real, { recursive: true }); + fs.symlinkSync(real, path.join(FAKE_HOME, "skills")); + + const res = write({ name: "ok" }); + assert.equal(res.created, true); + assert.equal(fs.readFileSync(path.join(real, "ok", "SKILL.md"), "utf8"), "NEW"); + }); + + it("works when CLAUDE_HOME itself is a symlink — the dotfiles case", () => { + // A dotfiles-managed ~/.claude is ordinary. Comparing an unresolved root + // against a resolved target would reject every write on such a machine. + const realHome = path.join(TMP, "real-home"); + fs.rmSync(realHome, { recursive: true, force: true }); + fs.mkdirSync(realHome, { recursive: true }); + const linkedHome = path.join(TMP, "linked-home"); + fs.rmSync(linkedHome, { recursive: true, force: true }); + fs.symlinkSync(realHome, linkedHome); + + process.env.CLAUDE_HOME = linkedHome; + try { + const res = write({ name: "ok" }); + assert.equal(res.created, true); + assert.equal(fs.readFileSync(path.join(realHome, "skills", "ok", "SKILL.md"), "utf8"), "NEW"); + } finally { + process.env.CLAUDE_HOME = FAKE_HOME; + } + }); +}); diff --git a/server/lib/cc-mutate.js b/server/lib/cc-mutate.js index aa7d2af2..8abe8142 100644 --- a/server/lib/cc-mutate.js +++ b/server/lib/cc-mutate.js @@ -31,7 +31,12 @@ const fs = require("node:fs"); const path = require("node:path"); const { getClaudeHome } = require("./claude-home"); const { isUnder, MAX_FILE_BYTES } = require("./cc-discovery"); -const { timestamp, createBackup, atomicWriteFile } = require("./config-write"); +const { + timestamp, + createBackup, + atomicWriteFile, + assertNoSymlinkEscape, +} = require("./config-write"); const NAME_RE = /^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$/; // Auto-memory files are arbitrary flat *.md filenames inside a project's @@ -118,6 +123,7 @@ function resolveTarget(scope, type, name, opts = {}) { target: filePath, filePath, containmentRoot: path.dirname(filePath), + trustedRoot: path.dirname(filePath), }; } @@ -127,7 +133,15 @@ function resolveTarget(scope, type, name, opts = {}) { throw makeError("EBADNAME", `auto-memory name must be a flat *.md filename`); } const target = path.join(memDir, name); - return { kind: "file", target, filePath: target, containmentRoot: memDir }; + // The projects root, not memDir: `//memory` is itself + // replaceable with a link, so it cannot be its own comparison base. + return { + kind: "file", + target, + filePath: target, + containmentRoot: memDir, + trustedRoot: path.join(getClaudeHome(), "projects"), + }; } if (typeof name !== "string" || !NAME_RE.test(name)) { @@ -144,6 +158,7 @@ function resolveTarget(scope, type, name, opts = {}) { target, filePath: path.join(target, spec.filename), containmentRoot: subdirAbs, + trustedRoot: root, }; } @@ -154,6 +169,7 @@ function resolveTarget(scope, type, name, opts = {}) { target, filePath: target, containmentRoot: subdirAbs, + trustedRoot: root, }; } @@ -184,11 +200,14 @@ function rmTreeSync(p) { * copies the whole tree. Returns the backup path (or null if there was * nothing to back up — e.g. brand-new file). */ -function backupTarget({ scope, type, target, opts }) { +function backupTarget({ scope, type, target, opts, trustedRoot }) { let root; if (type === "memory") root = memoryBackupRoot(scope, opts); else if (type === "auto-memory") root = autoMemoryBackupRoot(path.dirname(target)); else root = backupRoot(scope, type, opts); + // The backup DESTINATION needs the same guard as the artifact: a link at + // `/cc-config-backups` sends the copied contents outside the root. + if (trustedRoot) assertNoSymlinkEscape(trustedRoot, root, { label: "Claude" }); return createBackup({ source: target, backupRoot: root, type, name: path.basename(target) }); } @@ -214,6 +233,9 @@ function writeArtifact(args) { if (!isUnder(r.containmentRoot, r.target)) { throw makeError("EOUTOFROOT", "resolved path is outside containment root"); } + // The lexical check above is defeated by a symlink at the target or at any + // ancestor of it — see config-write.js. Resolve before writing. + assertNoSymlinkEscape(r.trustedRoot, r.target, { label: "Claude" }); const existedBefore = fs.existsSync(r.filePath); const backupPath = existedBefore @@ -222,6 +244,7 @@ function writeArtifact(args) { type, target: r.kind === "dir" ? r.target : r.filePath, opts: { cwd }, + trustedRoot: r.trustedRoot, }) : null; @@ -250,6 +273,9 @@ function deleteArtifact(args) { if (!isUnder(r.containmentRoot, r.target)) { throw makeError("EOUTOFROOT", "resolved path is outside containment root"); } + // rmSync/unlinkSync only unlink the link itself, but the backup that runs + // first copies THROUGH it — so the same guard has to cover delete. + assertNoSymlinkEscape(r.trustedRoot, r.target, { label: "Claude" }); if (!fs.existsSync(r.target)) { throw makeError("ENOTFOUND", `${type}/${name || "CLAUDE.md"} does not exist`); @@ -260,6 +286,7 @@ function deleteArtifact(args) { type, target: r.target, opts: { cwd }, + trustedRoot: r.trustedRoot, }); if (r.kind === "dir") { diff --git a/server/lib/codex-mutate.js b/server/lib/codex-mutate.js index 13fe4768..1833727d 100644 --- a/server/lib/codex-mutate.js +++ b/server/lib/codex-mutate.js @@ -35,7 +35,12 @@ const fs = require("node:fs"); const path = require("node:path"); const { getCodexHome, getProjectCodexDir, getProjectRoot } = require("./codex-home"); const { MAX_FILE_BYTES, isUnder } = require("./cc-discovery"); -const { createBackup, atomicWriteFile, assertUnder } = require("./config-write"); +const { + createBackup, + atomicWriteFile, + assertUnder, + assertNoSymlinkEscape, +} = require("./config-write"); const NAME_RE = /^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$/; @@ -165,28 +170,9 @@ function rmTreeSync(p) { * scope for this task (see task-5 fix round 2). */ function assertNotSymlinkEscape(trustedRoot, target, opts = {}) { - if (opts.create !== false) fs.mkdirSync(trustedRoot, { recursive: true }); - const realRoot = fs.realpathSync(trustedRoot); - - let dir = target; - let real; - for (;;) { - try { - real = fs.realpathSync(dir); - break; - } catch { - const parent = path.dirname(dir); - if (parent === dir) { - real = dir; // hit the filesystem root without finding anything real - break; - } - dir = parent; - } - } - - if (!isUnder(realRoot, real)) { - throw makeError("EOUTOFROOT", `resolved path escapes the Codex root: ${real}`); - } + // Lives in config-write.js so the Claude writer runs the identical check — + // this guard cost four fix rounds to get right and must not exist twice. + assertNoSymlinkEscape(trustedRoot, target, { ...opts, label: "Codex" }); } // ── Public API ───────────────────────────────────────────────────────── diff --git a/server/lib/config-write.js b/server/lib/config-write.js index 00e5bbf2..608768f0 100644 --- a/server/lib/config-write.js +++ b/server/lib/config-write.js @@ -98,4 +98,66 @@ function assertUnder(root, target) { } } -module.exports = { timestamp, createBackup, atomicWriteFile, assertUnder }; +/** + * The containment check for WRITE paths. `assertUnder` compares strings, which + * a symlink defeats: a link planted at `/skills/` — or at any + * ancestor of it — is lexically inside the root while the write lands wherever + * the link points. + * + * So resolve instead of compare. Walk up from `target` to the first component + * that actually exists, realpath THAT, and require the result inside the + * realpath'd root. Checking only `target` is not enough: on a create the leaf + * does not exist yet, `realpathSync` fails with ENOENT, and a symlinked + * ancestor sails through. + * + * A symlink that stays inside the root still works — the rule is where the + * path lands, not whether a link was involved. + * + * Read paths deliberately do NOT use this. Reading through a link shows you a + * file your own config directory points at, which is what the CLIs do; + * writing through one clobbers something unrelated. + * + * ponytail: check-then-use, so a local attacker who swaps a directory for a + * symlink between this call and the write still wins. Closing that needs + * O_NOFOLLOW-style fd work with real platform variance. + * + * @param {string} trustedRoot the outermost root the write must stay inside — + * never a subdirectory an attacker could replace with a link. + * @param {string} target the path about to be written, created or removed. + * @param {{create?: boolean, label?: string}} [opts] `create: false` skips + * materialising the root (a read-side caller must not have side effects). + */ +function assertNoSymlinkEscape(trustedRoot, target, opts = {}) { + if (opts.create !== false) fs.mkdirSync(trustedRoot, { recursive: true }); + const realRoot = fs.realpathSync(trustedRoot); + + let dir = target; + let real; + for (;;) { + try { + real = fs.realpathSync(dir); + break; + } catch { + const parent = path.dirname(dir); + if (parent === dir) { + real = dir; // hit the filesystem root without finding anything real + break; + } + dir = parent; + } + } + + if (!isUnder(realRoot, real)) { + const err = new Error(`resolved path escapes the ${opts.label || "config"} root: ${real}`); + err.code = "EOUTOFROOT"; + throw err; + } +} + +module.exports = { + timestamp, + createBackup, + atomicWriteFile, + assertUnder, + assertNoSymlinkEscape, +}; diff --git a/server/routes/cc-config.js b/server/routes/cc-config.js index 7e6e711b..64c67e17 100644 --- a/server/routes/cc-config.js +++ b/server/routes/cc-config.js @@ -178,7 +178,17 @@ router.put("/file", (req, res) => { .json({ error: { code: "EBADREQ", message: "scope and type are required" } }); } try { - const result = ccMutate.writeArtifact({ scope, type, name, content, project, cwd: cwdOf(req) }); + // process.cwd(), never ?cwd= — the same rule PATCH /settings already + // follows: a caller-chosen write target lets any request drop a file + // anywhere on disk. Reads keep the override; writes do not. + const result = ccMutate.writeArtifact({ + scope, + type, + name, + content, + project, + cwd: process.cwd(), + }); emitChanged({ action: "write", scope, type, name: name || null, project: project || null }); res.json(result); } catch (err) { @@ -194,7 +204,8 @@ router.delete("/file", (req, res) => { .json({ error: { code: "EBADREQ", message: "scope and type are required" } }); } try { - const result = ccMutate.deleteArtifact({ scope, type, name, project, cwd: cwdOf(req) }); + // Pinned for the same reason as PUT above. + const result = ccMutate.deleteArtifact({ scope, type, name, project, cwd: process.cwd() }); emitChanged({ action: "delete", scope, type, name: name || null, project: project || null }); res.json(result); } catch (err) {