diff --git a/src/build.ts b/src/build.ts index e3ba7fe..6a63f2b 100644 --- a/src/build.ts +++ b/src/build.ts @@ -26,7 +26,7 @@ export async function build(options: BuildOptions = {}): Promise { const targets = options.target ? [options.target] : targetNames.filter((target) => project.config.targets[target]); - const guard = buildDeleteGuard( + const guard = await buildDeleteGuard( project.rootDir, project.config, project.configPath, diff --git a/src/cleanup.ts b/src/cleanup.ts index 3878e05..0721e28 100644 --- a/src/cleanup.ts +++ b/src/cleanup.ts @@ -26,7 +26,7 @@ export async function prune( } = {}, ): Promise { const project = await loadProjectConfig(options.cwd, options.configPath); - const guard = buildDeleteGuard( + const guard = await buildDeleteGuard( project.rootDir, project.config, project.configPath, @@ -58,7 +58,7 @@ export async function clean( } = {}, ): Promise { const project = await loadProjectConfig(options.cwd, options.configPath); - const guard = buildDeleteGuard( + const guard = await buildDeleteGuard( project.rootDir, project.config, project.configPath, diff --git a/src/config.ts b/src/config.ts index 15f3d37..f9d1797 100644 --- a/src/config.ts +++ b/src/config.ts @@ -161,6 +161,36 @@ async function discoverSourcePlugins( return plugins; } +/** + * Directories under `sourceRoot` that are actually source plugins. + * + * The delete guard needs this to protect source without protecting generated + * output: the recommended layout writes every target's output under + * `plugins//`, which is also the default source-plugin root, so + * protecting that root wholesale refuses to prune the tool's own output. A + * shallow filesystem scan is deliberate — `clean` must keep working when the + * config or source tree no longer loads. + */ +export async function listSourcePluginDirs( + sourceRoot: string, +): Promise { + if (!(await exists(sourceRoot))) { + return []; + } + const dirs: string[] = []; + const entries = await fs.readdir(sourceRoot, { withFileTypes: true }); + for (const entry of entries) { + if (!entry.isDirectory() || entry.name.startsWith(".")) { + continue; + } + const dir = path.join(sourceRoot, entry.name); + if (await isSourcePluginDir(dir)) { + dirs.push(dir); + } + } + return dirs; +} + /** * A source plugin dir declares a manifest or has at least one component dir. * This keeps generated target output (e.g. plugins/cursor/ in a single-repo diff --git a/src/fs.ts b/src/fs.ts index 1667756..3e7c0eb 100644 --- a/src/fs.ts +++ b/src/fs.ts @@ -19,6 +19,61 @@ export async function walkFiles(dir: string): Promise { return entries.sort(); } +/** Whether `candidate` is `root` itself or sits beneath it, comparing already-resolved paths. */ +export function isInside(root: string, candidate: string): boolean { + return candidate === root || candidate.startsWith(`${root}${path.sep}`); +} + +/** + * Resolves `target` with symlinks followed, or returns `null` if the result + * escapes `root`. + * + * `path.resolve` is purely lexical, so a prefix test against it is satisfied by + * a path whose *intermediate* directory is a symlink pointing elsewhere — + * `/link/file` passes the string check while landing wherever `link` + * points. Checking the final entry with `lstat` does not help either, since the + * final entry is an ordinary file. + * + * Both sides go through the same resolution, which matters more than it looks: + * resolving only the target and comparing against a lexical root rejects every + * ordinary write whenever any ancestor of the output directory is itself a + * symlink — `/tmp` is one on macOS — and does so precisely when the output + * directory does not exist yet, i.e. on a first build. + */ +export async function resolveInside( + root: string, + target: string, +): Promise { + const realRoot = await realpathDeep(root); + const realTarget = await realpathDeep(target); + return isInside(realRoot, realTarget) ? realTarget : null; +} + +/** + * `fs.realpath` for a path that may not exist yet: resolves symlinks on the + * deepest ancestor that does exist, then re-attaches the missing tail. + */ +async function realpathDeep(target: string): Promise { + const missing: string[] = []; + let current = path.resolve(target); + for (;;) { + try { + const real = await fs.realpath(current); + return path.join(real, ...missing.reverse()); + } catch (error) { + if (!isNotFoundError(error)) { + throw error; + } + const parent = path.dirname(current); + if (parent === current) { + return path.resolve(target); + } + missing.push(path.basename(current)); + current = parent; + } + } +} + /** Writes every file in `files` under `outDir`, refusing to write outside it. */ export async function writeArtifact( outDir: string, @@ -27,14 +82,18 @@ export async function writeArtifact( const resolvedOut = path.resolve(outDir); for (const [relativePath, value] of files) { const destination = path.resolve(outDir, relativePath); - if ( - destination !== resolvedOut && - !destination.startsWith(resolvedOut + path.sep) - ) { + if (!isInside(resolvedOut, destination)) { throw new Error( `Refusing to write outside the output directory: ${relativePath}`, ); } + // The lexical check above cannot see through a symlinked intermediate + // directory, so confirm the real destination too, before creating anything. + if ((await resolveInside(resolvedOut, destination)) === null) { + throw new Error( + `Refusing to write through a symlink that leaves the output directory: ${relativePath}`, + ); + } await fs.mkdir(path.dirname(destination), { recursive: true }); await fs.writeFile(destination, value); } diff --git a/src/managed.ts b/src/managed.ts index 458b6e7..ea1dcbe 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -1,6 +1,7 @@ import { promises as fs } from "node:fs"; import path from "node:path"; -import { json, toPosix } from "./fs.js"; +import { isInside, json, resolveInside, toPosix } from "./fs.js"; +import { listSourcePluginDirs } from "./config.js"; import type { Artifact, CleanupEntry, @@ -134,23 +135,37 @@ export async function cleanManagedFiles( return { target, outDir, entries }; } -/** Builds the guard that stops prune/clean from deleting paths inside the config's source tree. */ -export function buildDeleteGuard( +/** + * Builds the guard that stops prune/clean from deleting the config or a source tree. + * + * `source.plugins` gets different treatment depending on whether it was set: + * when the user names a directory as their source-plugin root, that whole + * directory is protected. When they don't, the default root is `plugins/` — + * which is also where the recommended layout writes every target's *output*, so + * protecting it wholesale made the documented layout refuse to prune its own + * generated files after any source file was removed. Under the default, protect + * only the directories that really are source plugins. + */ +export async function buildDeleteGuard( rootDir: string, config: PluginpackConfig, configPath: string, force?: boolean, -): DeleteGuard { +): Promise { const protectedRoots: string[] = []; if (config.source?.skills) { protectedRoots.push(path.resolve(rootDir, config.source.skills)); } - // Mirrors loadConfig's default in src/config.ts so the source-plugin - // discovery root is always protected, whether or not it's written out - // explicitly in config. - protectedRoots.push( - path.resolve(rootDir, config.source?.plugins ?? "plugins"), - ); + if (config.source?.partials) { + protectedRoots.push(path.resolve(rootDir, config.source.partials)); + } + if (config.source?.plugins) { + protectedRoots.push(path.resolve(rootDir, config.source.plugins)); + } else { + protectedRoots.push( + ...(await listSourcePluginDirs(path.resolve(rootDir, "plugins"))), + ); + } return { protectedRoots, configPath: path.resolve(configPath), force }; } @@ -163,31 +178,65 @@ function assertNoProtectedDeletions( if (!guard || guard.force) { return; } - const blocked = paths.filter((file) => - isProtectedDeletion(outDir, file, guard), - ); + const blocked = paths + .map((file) => ({ file, root: protectingRoot(outDir, file, guard) })) + .filter( + (entry): entry is { file: string; root: string } => entry.root !== null, + ); if (blocked.length === 0) { return; } throw new Error( `Refusing to ${command} ${blocked.length} path(s) that resolve inside your source tree or config:\n` + - `${blocked.map((file) => ` ${file}`).join("\n")}\n` + + `${blocked.map(({ file, root }) => ` ${file} -> resolves inside ${root}`).join("\n")}\n` + `This usually means a target outDir overlaps source.skills/source.plugins. ` + `Fix the config, or re-run with --force to delete anyway.`, ); } -function isProtectedDeletion( +/** + * The protected root `relativePath` resolves inside, or `null` if it is safe to + * delete. Returns the matching root rather than a boolean so the refusal can + * name what it collided with — "resolves inside " is actionable in a way + * that a bare list of refused paths is not. + * + * Comparison is case- and normalization-folded, not exact. On a case-insensitive + * filesystem (APFS, NTFS) `fs.rm` resolves `Skills/x` and `skills/x` to the same + * file, so an exact-match guard can be walked straight past by one letter of + * case — reachable from a single typo in `source.skills` that the OS forgives, + * or a case-only rename in git history. Folding over-protects on a + * case-sensitive host, which is the correct direction for a guard whose job is + * refusing to delete. + */ +function protectingRoot( outDir: string, relativePath: string, guard: DeleteGuard, -): boolean { +): string | null { const absolute = path.resolve(outDir, normalizeManagedPath(relativePath)); - if (guard.configPath && absolute === guard.configPath) { - return true; + if (guard.configPath && pathsEqual(absolute, guard.configPath)) { + return guard.configPath; } - return guard.protectedRoots.some( - (root) => absolute === root || absolute.startsWith(`${root}${path.sep}`), + return ( + guard.protectedRoots.find( + (root) => pathsEqual(absolute, root) || isUnder(absolute, root), + ) ?? null + ); +} + +/** Case- and normalization-folded form, for comparing paths the filesystem treats as equal. */ +function fold(value: string): string { + return value.normalize("NFC").toLowerCase(); +} + +function pathsEqual(a: string, b: string): boolean { + return a === b || fold(a) === fold(b); +} + +function isUnder(candidate: string, root: string): boolean { + return ( + candidate.startsWith(`${root}${path.sep}`) || + fold(candidate).startsWith(`${fold(root)}${path.sep}`) ); } @@ -218,13 +267,9 @@ async function removeManagedPath( const root = path.resolve(outDir); const normalized = normalizeManagedPath(relativePath); const destination = path.resolve(root, normalized); - if (destination !== root && !destination.startsWith(`${root}${path.sep}`)) { + if (!isInside(root, destination)) { throw new Error(`Managed path escapes output directory: ${relativePath}`); } - // Defense in depth: fs.rm on a symlink unlinks the symlink itself rather - // than following it, so this isn't currently exploitable — but refuse - // outright if the entry is a symlink pointing outside `root`, rather than - // relying on that fs.rm behavior remaining true forever. let stats; try { stats = await fs.lstat(destination); @@ -239,13 +284,30 @@ async function removeManagedPath( path.dirname(destination), await fs.readlink(destination), ); - if (target !== root && !target.startsWith(`${root}${path.sep}`)) { + if (!isInside(root, target)) { throw new Error( `Refusing to remove a symlink pointing outside the output directory: ${relativePath}`, ); } } - await fs.rm(destination, { force: true }); + // The checks above are lexical, or inspect only the final entry. Neither sees + // a path whose *intermediate* directory is a symlink pointing elsewhere: the + // string test passes and the final entry is an ordinary file. Resolve the + // real destination as the catch-all before deleting. + if ((await resolveInside(root, destination)) === null) { + throw new Error( + `Refusing to remove a path that resolves outside the output directory: ${relativePath}`, + ); + } + try { + await fs.rm(destination, { force: true }); + } catch (error) { + throw new Error( + `Failed to remove managed path "${relativePath}" under ${outDir}: ${(error as Error).message}. ` + + `The managed manifest still lists every path, so re-running is safe once the cause is fixed.`, + { cause: error }, + ); + } await removeEmptyParents(path.dirname(destination), root); } diff --git a/tests/destructive.test.ts b/tests/destructive.test.ts index 9fef9c9..be3116d 100644 --- a/tests/destructive.test.ts +++ b/tests/destructive.test.ts @@ -1,3 +1,4 @@ +import { existsSync } from "node:fs"; import { access, mkdir, @@ -7,6 +8,7 @@ import { writeFile, } from "node:fs/promises"; import path from "node:path"; +import { fileURLToPath } from "node:url"; import { Project, type ProjectArgs } from "fixturify-project"; import { afterEach, describe, expect, it } from "vitest"; import { build } from "../src/build.js"; @@ -82,6 +84,18 @@ const cursorTarget = ` cursor: { plugins: { demo: { from: ["demo"], components: ["skills"] } } }`; +/** + * Whether this host's filesystem is case-insensitive (APFS, NTFS). A couple of + * guard behaviours only exist on such a host, and asserting them on a + * case-sensitive one would test the filesystem rather than the guard. + */ +const caseInsensitiveFs = existsSync( + path.join( + path.dirname(fileURLToPath(import.meta.url)), + "DESTRUCTIVE.TEST.TS", + ), +); + /** Rewrites a target's managed manifest, the on-disk record prune/clean/diff act on. */ async function writeManifest( root: string, @@ -307,6 +321,22 @@ describe("the delete guard protects a source tree from clean, not just prune", ( /Refusing to clean 2 path\(s\)[\s\S]*skills\/demo\/SKILL\.md[\s\S]*skills\/demo\/OTHER\.md/, ); }); + + it("names the protected root each blocked path resolved inside", async () => { + // A bare list of refused paths does not tell the user what the collision + // was with. Naming the root does — and when the root came from a mis-cased + // config value, seeing it echoed back points straight at the typo. + const created = await overlappingProject(); + const root = created.baseDir; + await build({ cwd: root, target: "cursor" }); + await writeManifest(root, ".", "cursor", ["skills/demo/SKILL.md"]); + + await expect(clean({ cwd: root, target: "cursor" })).rejects.toThrow( + new RegExp( + `skills/demo/SKILL\\.md -> resolves inside ${root.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}/skills`, + ), + ); + }); }); describe("clean handles targets and manifests that are not there", () => { @@ -463,6 +493,222 @@ describe("build writes every target before pruning any target", () => { }); }); +describe("the documented layouts stay operable through a full lifecycle", () => { + // The shape of test that was missing when this class of bug shipped twice: + // every other layout test does one fresh build and never mutates, prunes, or + // cleans. A guard false positive is invisible until something goes stale. + const layouts: Array<[string, string]> = [ + [ + "README Recommended Shape (output under plugins//, source.plugins unset)", + ` antigravity: { + outDir: "plugins/antigravity", + plugins: { acme: { from: ["core"] } } + }`, + ], + [ + "README single-repo-root shape (outDir '.', output under a pluginRoot)", + ` claude: { + outDir: ".", + pluginRoot: "plugins/claude", + plugins: { acme: { from: ["core"] } } + }`, + ], + [ + "init scaffold shape (dist/)", + ` claude: { + outDir: "dist/claude", + plugins: { acme: { from: ["core"] } } + }`, + ], + ]; + + for (const [label, targetsBlock] of layouts) { + it(`survives build -> delete a skill -> rebuild -> prune -> clean: ${label}`, async () => { + const created = await makeProject( + targetsBlock, + { + skills: { + alpha: { + "SKILL.md": "---\nname: alpha\ndescription: Alpha.\n---\n\nA.\n", + }, + beta: { + "SKILL.md": "---\nname: beta\ndescription: Beta.\n---\n\nB.\n", + }, + }, + }, + `source: { skills: "skills", rootPlugin: { id: "core" } },`, + ); + const root = created.baseDir; + + await build({ cwd: root }); + // A user removes a skill — the step that used to break everything after. + await rm(path.join(root, "skills/beta"), { + recursive: true, + force: true, + }); + + await expect(build({ cwd: root })).resolves.toBeDefined(); + await expect(prune({ cwd: root })).resolves.toBeDefined(); + await expect(clean({ cwd: root })).resolves.toBeDefined(); + + // The source tree is untouched by any of it. + await access(path.join(root, "skills/alpha/SKILL.md")); + await access(path.join(root, "pluginpack.config.ts")); + }); + } + + it("still refuses to delete real source when a target writes into the source tree", async () => { + const created = await makeProject( + ` antigravity: { + outDir: ".", + plugins: { acme: { from: ["core"], path: "skills/generated" } } + }`, + { + skills: { + alpha: { + "SKILL.md": "---\nname: alpha\ndescription: Alpha.\n---\n\nA.\n", + }, + }, + }, + `source: { skills: "skills", rootPlugin: { id: "core" } },`, + ); + const root = created.baseDir; + await build({ cwd: root }); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean .* that resolve inside your source tree or config/, + ); + await access(path.join(root, "skills/alpha/SKILL.md")); + }); + + it("refuses a managed path differing from a protected root only by case", async () => { + // Tests the guard's comparison directly, independent of how the host + // filesystem resolves case: the manifest names `Skills/...` while + // source.skills is `skills`. On a case-insensitive host fs.rm would resolve + // these to the same file, so an exact-match guard deletes real source. + const created = await makeProject( + ` antigravity: { + outDir: ".", + plugins: { acme: { from: ["core"], path: "generated" } } + }`, + { + skills: { + alpha: { + "SKILL.md": "---\nname: alpha\ndescription: Alpha.\n---\n\nA.\n", + }, + }, + }, + `source: { skills: "skills", rootPlugin: { id: "core" } },`, + ); + const root = created.baseDir; + await build({ cwd: root }); + await writeManifest(root, ".", "antigravity", ["Skills/alpha/SKILL.md"]); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean .* that resolve inside your source tree or config/, + ); + await access(path.join(root, "skills/alpha/SKILL.md")); + }); + + it.skipIf(!caseInsensitiveFs)( + "refuses when source.skills differs from the real directory only by case", + async () => { + // The end-to-end version of the above, and the realistic trigger: a config + // typo that a case-insensitive filesystem forgives, so the build succeeds + // against `skills/` while the guard was told `Skills/`. Only meaningful on + // a host that resolves the mismatch — on a case-sensitive one the build + // correctly fails earlier with "Root skills source directory is missing". + const created = await makeProject( + ` antigravity: { + outDir: ".", + plugins: { acme: { from: ["core"], path: "skills/generated" } } + }`, + { + skills: { + alpha: { + "SKILL.md": "---\nname: alpha\ndescription: Alpha.\n---\n\nA.\n", + }, + }, + }, + `source: { skills: "Skills", rootPlugin: { id: "core" } },`, + ); + const root = created.baseDir; + await build({ cwd: root }); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean .* that resolve inside your source tree or config/, + ); + await access(path.join(root, "skills/alpha/SKILL.md")); + }, + ); +}); + +describe("containment follows symlinks instead of trusting the path string", () => { + async function projectWithSymlinkedOutputDir(): Promise<{ + root: string; + outside: string; + }> { + const created = await makeProject( + ` antigravity: { + outDir: "out", + plugins: { acme: { from: ["demo"], path: "shared" } } + }`, + ); + const root = created.baseDir; + const outside = path.join(root, "outside-the-output-dir"); + await mkdir(outside, { recursive: true }); + await writeFile(path.join(outside, "plugin.json"), "USER FILE\n"); + await mkdir(path.join(root, "out"), { recursive: true }); + // An intermediate segment of the output path is a symlink pointing away. + await symlink(outside, path.join(root, "out/shared")); + return { root, outside }; + } + + it("refuses to write through a symlinked intermediate directory", async () => { + const { root, outside } = await projectWithSymlinkedOutputDir(); + + await expect(build({ cwd: root })).rejects.toThrow( + /Refusing to write through a symlink that leaves the output directory/, + ); + expect(await readFile(path.join(outside, "plugin.json"), "utf8")).toBe( + "USER FILE\n", + ); + }); + + it("refuses to delete through a symlinked intermediate directory", async () => { + const { root, outside } = await projectWithSymlinkedOutputDir(); + // A manifest naming a path whose intermediate segment is the symlink. Every + // segment is a safe relative path, so normalizeManagedPath permits it. + await writeManifest(root, "out", "antigravity", ["shared/plugin.json"]); + + await expect(clean({ cwd: root, target: "antigravity" })).rejects.toThrow( + /Refusing to remove a path that resolves outside the output directory/, + ); + await access(path.join(outside, "plugin.json")); + }); + + it("still writes and cleans normally when the output directory itself is a symlink", async () => { + // The legitimate case: outDir is a symlink. Resolving both sides means this + // keeps working rather than being caught as an escape. + const created = await makeProject( + ` antigravity: { + outDir: "linked-out", + plugins: { acme: { from: ["demo"] } } + }`, + ); + const root = created.baseDir; + const real = path.join(root, "real-out"); + await mkdir(real, { recursive: true }); + await symlink(real, path.join(root, "linked-out")); + + await expect(build({ cwd: root })).resolves.toBeDefined(); + await access(path.join(real, "acme/plugin.json")); + await expect( + clean({ cwd: root, target: "antigravity" }), + ).resolves.toBeDefined(); + }); +}); + describe("clean refuses paths another target's manifest also claims", () => { // cursor's outDir contains claude's, so a cursor manifest can name a file // inside dist/claude without any "../" — the shape a pre-collision-check