diff --git a/AGENTS.md b/AGENTS.md index d7733a4..5a4753d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -155,8 +155,9 @@ and a name defined at a higher level wins, so a project skill overrides the user copy of the same name rather than merging; the prompt groups the listing by level so each skill's provenance is visible. Only `name`/`description` enter the system prompt — progressive disclosure — and the body is fetched on demand; the read tool -is confined to the skill's own directory, and a malformed `SKILL.md` is warned about -and skipped rather than failing the run. +is confined to the skill's own directory — real paths are resolved, so a symlink out is +refused, not followed — and a malformed `SKILL.md` is warned about and skipped rather +than failing the run. **Delegation** (`src/delegation.ts`) is a `task` tool, off unless `--task`: a sub-agent that runs in its own session and returns only its result. Two rules diff --git a/README.md b/README.md index 35623dd..4a5d812 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ do nothing at all in the REPL.) file) is read through the `skills_read_skill` tool when the work calls for it, so a large skill library costs almost nothing until it is used. A malformed `SKILL.md` is reported and skipped rather than failing the run, and the read tool cannot leave - a skill's own directory. + a skill's own directory — real paths are resolved, so a symlink out is refused. - **Delegation.** `--task` exposes a `task` tool: the model can hand a self-contained job to a **sub-agent** that runs in its own session and returns only its result — so a broad search or a long review cannot flood the caller's diff --git a/docs/archetypes/skills-agent.md b/docs/archetypes/skills-agent.md index 6d7f26f..a29eb74 100644 --- a/docs/archetypes/skills-agent.md +++ b/docs/archetypes/skills-agent.md @@ -102,7 +102,8 @@ The loader is deliberately outside the runtime: a skill set becomes an ordinary **toolset** (`skillsToolset`), so reading one takes the same path as any MCP tool — approval gate, telemetry, execution graph — and no protocol noun was added (ADR-004). A malformed `SKILL.md` warns and is skipped rather than failing the -run, and the read tool cannot leave a skill's directory. +run, and the read tool cannot leave a skill's directory: real paths are resolved, +so a symlink out is refused rather than followed. ## Gap diff --git a/src/skills.test.ts b/src/skills.test.ts index ad954be..e373996 100644 --- a/src/skills.test.ts +++ b/src/skills.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -110,6 +110,33 @@ describe("skills", () => { expect(skill?.files).toEqual(["checklist.md", "scripts/run.sh"]); }); + + it("never lists a symlink, or recurses through one, out of the skill", () => { + const dir = makeSkill(root, "deploy", "Deploy"); + const outside = join(root, "outside"); + mkdirSync(outside); + writeFileSync(join(outside, "secret.txt"), "s3cret"); + symlinkSync(join(outside, "secret.txt"), join(dir, "leak")); + symlinkSync(outside, join(dir, "elsewhere")); + writeFileSync(join(dir, "real.md"), "- item"); + + const [skill] = discoverSkills(dirs(root)); + + // The real file is listed; neither link is — not even as a directory to walk. + expect(skill?.files).toEqual(["real.md"]); + }); + + it("skips a skill whose SKILL.md is a symlink out of its directory, with a warning", () => { + const borrowed = join(root, "borrowed.md"); + writeFileSync(borrowed, "---\nname: borrowed\ndescription: someone else's file\n---\nbody"); + const dir = join(root, "borrowed"); + mkdirSync(dir); + symlinkSync(borrowed, join(dir, SKILL_FILE)); + const warnings: string[] = []; + + expect(discoverSkills(dirs(root), (message) => warnings.push(message))).toEqual([]); + expect(warnings[0]).toContain("not a regular file"); + }); }); describe("read_skill", () => { @@ -167,6 +194,45 @@ describe("skills", () => { expect(result.ok).toBe(false); }); + it("refuses to read through a symlink that points out of the skill", async () => { + // The lexical check the read used to do would pass this: the link *is* inside + // the directory by name. Only resolving the real path catches the target. + const dir = makeSkill(root, "deploy", "Deploy"); + writeFileSync(join(root, "secret.txt"), "s3cret"); + symlinkSync(join(root, "secret.txt"), join(dir, "leak")); + const toolset = skillsToolset(discoverSkills(dirs(root))); + + const result = await toolset.callTool("read_skill", { skill: "deploy", file: "leak" }); + + expect(result.ok).toBe(false); + expect(result.error).toContain("not a regular file"); + expect(result.output).toBeUndefined(); + }); + + it("still reads a symlink that stays inside the skill", async () => { + const dir = makeSkill(root, "deploy", "Deploy"); + writeFileSync(join(dir, "notes.md"), "- item"); + symlinkSync(join(dir, "notes.md"), join(dir, "alias.md")); + const toolset = skillsToolset(discoverSkills(dirs(root))); + + const result = await toolset.callTool("read_skill", { skill: "deploy", file: "alias.md" }); + + expect(result.ok).toBe(true); + expect(String(result.output)).toBe("- item"); + }); + + it("refuses a directory as a bundled file", async () => { + const dir = makeSkill(root, "deploy", "Deploy"); + mkdirSync(join(dir, "scripts")); + writeFileSync(join(dir, "scripts", "run.sh"), "echo hi"); + const toolset = skillsToolset(discoverSkills(dirs(root))); + + const result = await toolset.callTool("read_skill", { skill: "deploy", file: "scripts" }); + + expect(result.ok).toBe(false); + expect(result.error).toContain("not a regular file"); + }); + it("reports an unknown skill instead of throwing", async () => { makeSkill(root, "deploy", "Deploy"); const toolset = skillsToolset(discoverSkills(dirs(root))); diff --git a/src/skills.ts b/src/skills.ts index bb83946..26b5904 100644 --- a/src/skills.ts +++ b/src/skills.ts @@ -12,7 +12,8 @@ * Skills are **not a protocol concept** (ADR-004): there is no `adaptiveSkill`, * no new noun. This is a host-side loader over a filesystem convention, and its * files are treated as untrusted input — the read tool cannot escape a skill's - * own directory. + * own directory. That guarantee is `resolveInside`'s: a symlink defeats a string + * check, so paths are resolved for real and required to be regular files. * * Three levels are searched, highest precedence first: the explicit `--skills` * directories, the project's `./skills`, then the user-level default. Order *is* @@ -20,7 +21,7 @@ * rather than the two merging — and the prompt says which level each one came * from, so a name that looks local but is not cannot be mistaken for one. */ -import { existsSync, readdirSync, readFileSync, statSync } from "node:fs"; +import { existsSync, lstatSync, readdirSync, readFileSync, realpathSync, statSync } from "node:fs"; import { homedir } from "node:os"; import { join, resolve, sep } from "node:path"; import { load as parseYaml } from "js-yaml"; @@ -152,10 +153,22 @@ export function discoverSkills( for (const entry of entries) { const dirPath = join(dir, entry); const filePath = join(dirPath, SKILL_FILE); + let root: string; let text: string; try { - if (!statSync(dirPath).isDirectory()) continue; - text = readFileSync(filePath, "utf8"); + root = realpathSync(dirPath); + if (!statSync(root).isDirectory()) continue; + const skillFile = resolveInside(root, filePath); + if (!skillFile) { + // Something is there, but it is not a regular file inside the skill — + // a `SKILL.md` symlinked out of the tree, or a device. Refuse it + // loudly rather than reading whatever it points at. + if (existsSync(filePath)) { + warn(`${filePath}: ${SKILL_FILE} is not a regular file inside the skill; skipped`); + } + continue; + } + text = readFileSync(skillFile, "utf8"); } catch { continue; // no SKILL.md here — just not a skill } @@ -172,7 +185,6 @@ export function discoverSkills( ); continue; } - const root = resolve(dirPath); found.set(parsed.name, { name: parsed.name, description: parsed.description, @@ -248,12 +260,19 @@ export function skillsToolset(skills: Skill[]): Toolset { if (args.file !== undefined) { if (typeof args.file !== "string") return { ok: false, error: "file must be a string" }; - const target = resolve(skill.dir, args.file); - // Untrusted input: a skill may only read inside its own directory, so a - // `../` in the argument cannot reach the rest of the filesystem. - if (target !== skill.dir && !target.startsWith(skill.dir + sep)) { + const lexical = resolve(skill.dir, args.file); + if (lexical !== skill.dir && !lexical.startsWith(skill.dir + sep)) { return { ok: false, error: `refused: "${args.file}" is outside the skill directory` }; } + // The lexical check is not enough on its own: a symlink in the skill + // directory is inside by string, and the *target* is where it really goes. + const target = resolveInside(skill.dir, lexical); + if (!target) { + return { + ok: false, + error: `refused: "${args.file}" is not a regular file inside the skill directory`, + }; + } try { return { ok: true, output: bounded(readFileSync(target, "utf8")) }; } catch { @@ -262,8 +281,10 @@ export function skillsToolset(skills: Skill[]): Toolset { } let body: string; + const skillFile = resolveInside(skill.dir, join(skill.dir, SKILL_FILE)); + if (!skillFile) return { ok: false, error: `cannot read ${skill.name}/${SKILL_FILE}` }; try { - const parsed = parseSkill(readFileSync(join(skill.dir, SKILL_FILE), "utf8")); + const parsed = parseSkill(readFileSync(skillFile, "utf8")); if (!parsed) return { ok: false, error: `"${skill.name}" no longer loads` }; body = parsed.body; } catch { @@ -281,6 +302,35 @@ export function skillsToolset(skills: Skill[]): Toolset { }; } +/** + * The real path of `path` when it is a **regular file inside** `root`, else + * `undefined`. + * + * `resolve` alone is not confinement: it is purely lexical, so a symlink sitting + * in a skill directory passes a string-prefix check and `readFileSync` follows it + * straight out of the tree — which is exactly how a copied or vendored skill (the + * untrusted case) would read a key or `/etc/passwd`. Resolving for real *and* + * requiring a regular file is what makes "cannot leave the skill's own directory" + * true rather than aspirational. + */ +function resolveInside(root: string, path: string): string | undefined { + let real: string; + try { + real = realpathSync(path); + } catch { + return undefined; + } + if (real !== root && !real.startsWith(root + sep)) return undefined; + try { + // A directory, socket, fifo or device is not a bundled file; reading one + // either fails or hangs. + if (!statSync(real).isFile()) return undefined; + } catch { + return undefined; + } + return real; +} + /** Split a `SKILL.md` into its frontmatter and body, or `undefined` if malformed. */ function parseSkill( text: string, @@ -338,11 +388,13 @@ function readNameList(value: unknown): string[] | undefined | null { */ export function agentDefinition(skill: Skill): AgentDefinition { let instructions = ""; - try { - const parsed = parseSkill(readFileSync(join(skill.dir, SKILL_FILE), "utf8")); - instructions = parsed?.body ?? ""; - } catch { - // A skill that vanished between discovery and use simply has no instructions. + const skillFile = resolveInside(skill.dir, join(skill.dir, SKILL_FILE)); + if (skillFile) { + try { + instructions = parseSkill(readFileSync(skillFile, "utf8"))?.body ?? ""; + } catch { + // A skill that vanished between discovery and use simply has no instructions. + } } return { name: skill.name, @@ -367,14 +419,17 @@ function listSkillFiles(dir: string, prefix = "", depth = 0): string[] { if (files.length >= MAX_FILES) break; const path = join(dir, entry); const relative = prefix ? `${prefix}/${entry}` : entry; - let isDirectory = false; + let info: ReturnType; try { - isDirectory = statSync(path).isDirectory(); + // `lstat`, never `stat`: following a link here would both list files from + // outside the skill and recurse into a directory it points at. + info = lstatSync(path); } catch { continue; } - if (isDirectory) files.push(...listSkillFiles(path, relative, depth + 1)); - else if (relative !== SKILL_FILE) files.push(relative); + if (info.isSymbolicLink()) continue; // a link is how a bundled file escapes + if (info.isDirectory()) files.push(...listSkillFiles(path, relative, depth + 1)); + else if (info.isFile() && relative !== SKILL_FILE) files.push(relative); } return files.slice(0, MAX_FILES); }