fix(skills): make the read confinement real — a symlink could leave the directory

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

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

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
 

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)));

src/skills.ts

@@ -12,15 +12,16 @@
  * 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*
  * the precedence — a project skill overrides the user's copy of the same name
  * 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<typeof lstatSync>;
     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);
 }