fix(skills): replace readFileSync with symlink-safe, root-confined skill file loader (#57519)

* fix: replace readFileSync with symlink-safe, root-confined skill file loader

* fix(skills): preserve directory-name fallback when frontmatter omits name

* fix: harden skill loader path containment

---------

Co-authored-by: Jacob Tomlinson <jacobtomlinson@users.noreply.github.com>

src/agents/skills.loadworkspaceskillentries.test.ts

@@ -4,6 +4,7 @@ import path from "node:path";
 import { afterEach, describe, expect, it } from "vitest";
 import { writeSkill } from "./skills.e2e-test-helpers.js";
 import { loadWorkspaceSkillEntries } from "./skills.js";
+import { readSkillFrontmatterSafe } from "./skills/local-loader.js";
 import { writePluginWithSkill } from "./test-helpers/skill-plugin-fixtures.js";
 
 const tempDirs: string[] = [];
@@ -130,6 +131,24 @@ describe("loadWorkspaceSkillEntries", () => {
     expect(entries.map((entry) => entry.skill.name)).not.toContain("diffs");
   });
 
+  it("falls back to the skill directory name when frontmatter omits name", async () => {
+    const workspaceDir = await createTempWorkspaceDir();
+    const skillDir = path.join(workspaceDir, "skills", "fallback-name");
+    await fs.mkdir(skillDir, { recursive: true });
+    await fs.writeFile(
+      path.join(skillDir, "SKILL.md"),
+      ["---", "description: Skill without explicit name", "---", "", "# Fallback"].join("\n"),
+      "utf8",
+    );
+
+    const entries = loadWorkspaceSkillEntries(workspaceDir, {
+      managedSkillsDir: path.join(workspaceDir, ".managed"),
+      bundledSkillsDir: path.join(workspaceDir, ".bundled"),
+    });
+
+    expect(entries.map((entry) => entry.skill.name)).toContain("fallback-name");
+  });
+
   it.runIf(process.platform !== "win32")(
     "skips workspace skill directories that resolve outside the workspace root",
     async () => {
@@ -175,4 +194,51 @@ describe("loadWorkspaceSkillEntries", () => {
       expect(entries.map((entry) => entry.skill.name)).not.toContain("outside-file-skill");
     },
   );
+
+  it.runIf(process.platform !== "win32")(
+    "skips symlinked SKILL.md even when the target stays inside the workspace root",
+    async () => {
+      const workspaceDir = await createTempWorkspaceDir();
+      const targetDir = path.join(workspaceDir, "safe-target");
+      await writeSkill({
+        dir: targetDir,
+        name: "symlink-target",
+        description: "Target skill",
+      });
+
+      const skillDir = path.join(workspaceDir, "skills", "symlinked");
+      await fs.mkdir(skillDir, { recursive: true });
+      await fs.symlink(path.join(targetDir, "SKILL.md"), path.join(skillDir, "SKILL.md"));
+
+      const entries = loadWorkspaceSkillEntries(workspaceDir, {
+        managedSkillsDir: path.join(workspaceDir, ".managed"),
+        bundledSkillsDir: path.join(workspaceDir, ".bundled"),
+      });
+
+      expect(entries.map((entry) => entry.skill.name)).not.toContain("symlink-target");
+    },
+  );
+
+  it.runIf(process.platform !== "win32")(
+    "reads skill frontmatter when the allowed root is the filesystem root",
+    async () => {
+      const workspaceDir = await createTempWorkspaceDir();
+      const skillDir = path.join(workspaceDir, "skills", "root-allowed");
+      await writeSkill({
+        dir: skillDir,
+        name: "root-allowed",
+        description: "Readable from filesystem root",
+      });
+
+      const frontmatter = readSkillFrontmatterSafe({
+        rootDir: path.parse(skillDir).root,
+        filePath: path.join(skillDir, "SKILL.md"),
+      });
+
+      expect(frontmatter).toMatchObject({
+        name: "root-allowed",
+        description: "Readable from filesystem root",
+      });
+    },
+  );
 });

src/agents/skills/bundled-context.ts

@@ -1,6 +1,6 @@
-import { loadSkillsFromDir } from "@mariozechner/pi-coding-agent";
 import { createSubsystemLogger } from "../../logging/subsystem.js";
 import { resolveBundledSkillsDir, type BundledSkillsResolveOptions } from "./bundled-dir.js";
+import { loadSkillsFromDirSafe } from "./local-loader.js";
 
 const skillsLogger = createSubsystemLogger("skills");
 let hasWarnedMissingBundledDir = false;
@@ -29,7 +29,7 @@ export function resolveBundledSkillsContext(
   if (cachedBundledContext?.dir === dir) {
     return { dir, names: new Set(cachedBundledContext.names) };
   }
-  const result = loadSkillsFromDir({ dir, source: "openclaw-bundled" });
+  const result = loadSkillsFromDirSafe({ dir, source: "openclaw-bundled" });
   for (const skill of result.skills) {
     if (skill.name.trim()) {
       names.add(skill.name);

src/agents/skills/local-loader.ts

@@ -0,0 +1,161 @@
+import fs from "node:fs";
+import path from "node:path";
+import { createSyntheticSourceInfo, type Skill } from "@mariozechner/pi-coding-agent";
+import { openVerifiedFileSync } from "../../infra/safe-open-sync.js";
+import { parseFrontmatter, resolveSkillInvocationPolicy } from "./frontmatter.js";
+
+function isPathWithinRoot(rootRealPath: string, candidatePath: string): boolean {
+  const relative = path.relative(rootRealPath, candidatePath);
+  return (
+    relative === "" ||
+    (!relative.startsWith(`..${path.sep}`) && relative !== ".." && !path.isAbsolute(relative))
+  );
+}
+
+function readSkillFileSync(params: {
+  rootRealPath: string;
+  filePath: string;
+  maxBytes?: number;
+}): string | null {
+  const opened = openVerifiedFileSync({
+    filePath: params.filePath,
+    rejectPathSymlink: true,
+    maxBytes: params.maxBytes,
+  });
+  if (!opened.ok) {
+    return null;
+  }
+  try {
+    if (!isPathWithinRoot(params.rootRealPath, opened.path)) {
+      return null;
+    }
+    return fs.readFileSync(opened.fd, "utf8");
+  } finally {
+    fs.closeSync(opened.fd);
+  }
+}
+
+function loadSingleSkillDirectory(params: {
+  skillDir: string;
+  source: string;
+  rootRealPath: string;
+  maxBytes?: number;
+}): Skill | null {
+  const skillFilePath = path.join(params.skillDir, "SKILL.md");
+  const raw = readSkillFileSync({
+    rootRealPath: params.rootRealPath,
+    filePath: skillFilePath,
+    maxBytes: params.maxBytes,
+  });
+  if (!raw) {
+    return null;
+  }
+
+  let frontmatter: Record<string, string>;
+  try {
+    frontmatter = parseFrontmatter(raw);
+  } catch {
+    return null;
+  }
+
+  const fallbackName = path.basename(params.skillDir).trim();
+  const name = frontmatter.name?.trim() || fallbackName;
+  const description = frontmatter.description?.trim();
+  if (!name || !description) {
+    return null;
+  }
+  const invocation = resolveSkillInvocationPolicy(frontmatter);
+  const filePath = path.resolve(skillFilePath);
+  const baseDir = path.resolve(params.skillDir);
+
+  return {
+    name,
+    description,
+    filePath,
+    baseDir,
+    source: params.source,
+    sourceInfo: createSyntheticSourceInfo(filePath, {
+      source: params.source,
+      baseDir,
+      scope: "project",
+      origin: "top-level",
+    }),
+    disableModelInvocation: invocation.disableModelInvocation,
+  };
+}
+
+function listCandidateSkillDirs(dir: string): string[] {
+  try {
+    return fs
+      .readdirSync(dir, { withFileTypes: true })
+      .filter(
+        (entry) =>
+          entry.isDirectory() && !entry.name.startsWith(".") && entry.name !== "node_modules",
+      )
+      .map((entry) => path.join(dir, entry.name))
+      .sort((left, right) => left.localeCompare(right));
+  } catch {
+    return [];
+  }
+}
+
+export function loadSkillsFromDirSafe(params: { dir: string; source: string; maxBytes?: number }): {
+  skills: Skill[];
+} {
+  const rootDir = path.resolve(params.dir);
+  let rootRealPath: string;
+  try {
+    rootRealPath = fs.realpathSync(rootDir);
+  } catch {
+    return { skills: [] };
+  }
+
+  const rootSkill = loadSingleSkillDirectory({
+    skillDir: rootDir,
+    source: params.source,
+    rootRealPath,
+    maxBytes: params.maxBytes,
+  });
+  if (rootSkill) {
+    return { skills: [rootSkill] };
+  }
+
+  const skills = listCandidateSkillDirs(rootDir)
+    .map((skillDir) =>
+      loadSingleSkillDirectory({
+        skillDir,
+        source: params.source,
+        rootRealPath,
+        maxBytes: params.maxBytes,
+      }),
+    )
+    .filter((skill): skill is Skill => skill !== null);
+
+  return { skills };
+}
+
+export function readSkillFrontmatterSafe(params: {
+  rootDir: string;
+  filePath: string;
+  maxBytes?: number;
+}): Record<string, string> | null {
+  let rootRealPath: string;
+  try {
+    rootRealPath = fs.realpathSync(path.resolve(params.rootDir));
+  } catch {
+    return null;
+  }
+  const raw = readSkillFileSync({
+    rootRealPath,
+    filePath: path.resolve(params.filePath),
+    maxBytes: params.maxBytes,
+  });
+  if (!raw) {
+    return null;
+  }
+  try {
+    return parseFrontmatter(raw);
+  } catch {
+    return null;
+  }
+}

src/agents/skills/workspace.ts

@@ -1,11 +1,7 @@
 import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
-import {
-  formatSkillsForPrompt,
-  loadSkillsFromDir,
-  type Skill,
-} from "@mariozechner/pi-coding-agent";
+import { formatSkillsForPrompt, type Skill } from "@mariozechner/pi-coding-agent";
 import type { OpenClawConfig } from "../../config/config.js";
 import { isPathInside } from "../../infra/path-guards.js";
 import { createSubsystemLogger } from "../../logging/subsystem.js";
@@ -15,11 +11,8 @@ import { resolveSandboxPath } from "../sandbox-paths.js";
 import { resolveBundledSkillsDir } from "./bundled-dir.js";
 import { shouldIncludeSkill } from "./config.js";
 import { normalizeSkillFilter } from "./filter.js";
-import {
-  parseFrontmatter,
-  resolveOpenClawMetadata,
-  resolveSkillInvocationPolicy,
-} from "./frontmatter.js";
+import { resolveOpenClawMetadata, resolveSkillInvocationPolicy } from "./frontmatter.js";
+import { loadSkillsFromDirSafe, readSkillFrontmatterSafe } from "./local-loader.js";
 import { resolvePluginSkillDirs } from "./plugin-skills.js";
 import { serializeByKey } from "./serialize.js";
 import type {
@@ -344,7 +337,11 @@ function loadSkillEntries(
         return [];
       }
 
-      const loaded = loadSkillsFromDir({ dir: baseDir, source: params.source });
+      const loaded = loadSkillsFromDirSafe({
+        dir: baseDir,
+        source: params.source,
+        maxBytes: limits.maxSkillFileBytes,
+      });
       return filterLoadedSkillsInsideRoot({
         skills: unwrapLoadedSkills(loaded),
         source: params.source,
@@ -418,7 +415,11 @@ function loadSkillEntries(
         continue;
       }
 
-      const loaded = loadSkillsFromDir({ dir: skillDir, source: params.source });
+      const loaded = loadSkillsFromDirSafe({
+        dir: skillDir,
+        source: params.source,
+        maxBytes: limits.maxSkillFileBytes,
+      });
       loadedSkills.push(
         ...filterLoadedSkillsInsideRoot({
           skills: unwrapLoadedSkills(loaded),
@@ -510,13 +511,12 @@ function loadSkillEntries(
   }
 
   const skillEntries: SkillEntry[] = Array.from(merged.values()).map((skill) => {
-    let frontmatter: ParsedSkillFrontmatter = {};
-    try {
-      const raw = fs.readFileSync(skill.filePath, "utf-8");
-      frontmatter = parseFrontmatter(raw);
-    } catch {
-      // ignore malformed skills
-    }
+    const frontmatter =
+      readSkillFrontmatterSafe({
+        rootDir: skill.baseDir,
+        filePath: skill.filePath,
+        maxBytes: limits.maxSkillFileBytes,
+      }) ?? ({} as ParsedSkillFrontmatter);
     return {
       skill,
       frontmatter,