diff --git a/src/agents/skills.loadworkspaceskillentries.test.ts b/src/agents/skills.loadworkspaceskillentries.test.ts index 96fa9f7e9c3..d717fba03d1 100644 --- a/src/agents/skills.loadworkspaceskillentries.test.ts +++ b/src/agents/skills.loadworkspaceskillentries.test.ts @@ -14,6 +14,10 @@ async function createTempWorkspaceDir() { return workspaceDir; } +async function symlinkDir(targetDir: string, linkPath: string) { + await fs.symlink(targetDir, linkPath, process.platform === "win32" ? "junction" : "dir"); +} + afterEach(async () => { await Promise.all( tempDirs.splice(0, tempDirs.length).map((dir) => fs.rm(dir, { recursive: true, force: true })), @@ -130,28 +134,47 @@ describe("loadWorkspaceSkillEntries", () => { expect(entries.map((entry) => entry.skill.name)).not.toContain("diffs"); }); - it.runIf(process.platform !== "win32")( - "skips workspace skill directories that resolve outside the workspace root", - async () => { - const workspaceDir = await createTempWorkspaceDir(); - const outsideDir = await createTempWorkspaceDir(); - const escapedSkillDir = path.join(outsideDir, "outside-skill"); - await writeSkill({ - dir: escapedSkillDir, - name: "outside-skill", - description: "Outside", - }); - await fs.mkdir(path.join(workspaceDir, "skills"), { recursive: true }); - await fs.symlink(escapedSkillDir, path.join(workspaceDir, "skills", "escaped-skill"), "dir"); + it("allows managed skill directories that resolve outside the managed root", async () => { + const workspaceDir = await createTempWorkspaceDir(); + const managedDir = path.join(workspaceDir, ".managed"); + const outsideDir = await createTempWorkspaceDir(); + const externalSkillDir = path.join(outsideDir, "outside-skill"); - const entries = loadWorkspaceSkillEntries(workspaceDir, { - managedSkillsDir: path.join(workspaceDir, ".managed"), - bundledSkillsDir: path.join(workspaceDir, ".bundled"), - }); + await writeSkill({ + dir: externalSkillDir, + name: "outside-skill", + description: "Outside managed root", + }); + await fs.mkdir(managedDir, { recursive: true }); + await symlinkDir(externalSkillDir, path.join(managedDir, "outside-skill")); - expect(entries.map((entry) => entry.skill.name)).not.toContain("outside-skill"); - }, - ); + const entries = loadWorkspaceSkillEntries(workspaceDir, { + managedSkillsDir: managedDir, + bundledSkillsDir: path.join(workspaceDir, ".bundled"), + }); + + expect(entries.map((entry) => entry.skill.name)).toContain("outside-skill"); + }); + + it("skips workspace skill directories that resolve outside the workspace root", async () => { + const workspaceDir = await createTempWorkspaceDir(); + const outsideDir = await createTempWorkspaceDir(); + const escapedSkillDir = path.join(outsideDir, "outside-skill"); + await writeSkill({ + dir: escapedSkillDir, + name: "outside-skill", + description: "Outside", + }); + await fs.mkdir(path.join(workspaceDir, "skills"), { recursive: true }); + await symlinkDir(escapedSkillDir, path.join(workspaceDir, "skills", "escaped-skill")); + + const entries = loadWorkspaceSkillEntries(workspaceDir, { + managedSkillsDir: path.join(workspaceDir, ".managed"), + bundledSkillsDir: path.join(workspaceDir, ".bundled"), + }); + + expect(entries.map((entry) => entry.skill.name)).not.toContain("outside-skill"); + }); it.runIf(process.platform !== "win32")( "skips workspace skill files that resolve outside the workspace root", @@ -175,4 +198,41 @@ describe("loadWorkspaceSkillEntries", () => { expect(entries.map((entry) => entry.skill.name)).not.toContain("outside-file-skill"); }, ); + + it.runIf(process.platform !== "win32")( + "skips managed skill files that resolve outside the managed skill root", + async () => { + const workspaceDir = await createTempWorkspaceDir(); + const managedDir = path.join(workspaceDir, ".managed"); + const outsideDir = await createTempWorkspaceDir(); + const escapedTargetDir = path.join(outsideDir, "outside-file-skill"); + await writeSkill({ + dir: escapedTargetDir, + name: "outside-file-skill", + description: "Outside file", + }); + + const externalManagedSkillDir = path.join(outsideDir, "managed-skill"); + await writeSkill({ + dir: externalManagedSkillDir, + name: "managed-skill", + description: "Managed skill", + }); + await fs.rm(path.join(externalManagedSkillDir, "SKILL.md")); + await fs.symlink( + path.join(escapedTargetDir, "SKILL.md"), + path.join(externalManagedSkillDir, "SKILL.md"), + ); + await fs.mkdir(managedDir, { recursive: true }); + await symlinkDir(externalManagedSkillDir, path.join(managedDir, "managed-skill")); + + const entries = loadWorkspaceSkillEntries(workspaceDir, { + managedSkillsDir: managedDir, + bundledSkillsDir: path.join(workspaceDir, ".bundled"), + }); + + expect(entries.map((entry) => entry.skill.name)).not.toContain("outside-file-skill"); + expect(entries.map((entry) => entry.skill.name)).not.toContain("managed-skill"); + }, + ); }); diff --git a/src/agents/skills/workspace.ts b/src/agents/skills/workspace.ts index 80624a30139..663a532b091 100644 --- a/src/agents/skills/workspace.ts +++ b/src/agents/skills/workspace.ts @@ -198,6 +198,31 @@ function warnEscapedSkillPath(params: { }); } +function shouldEnforceContainedSkillPaths(source: string): boolean { + // Repo-scoped and explicitly configured external roots are treated as untrusted + // boundaries. Managed/personal skill roots are user-owned and may legitimately + // expose symlinked skill directories. + return ( + source === "openclaw-extra" || + source === "openclaw-workspace" || + source === "agents-skills-project" + ); +} + +function resolveContainedPathWithinSkillRoot(params: { + source: string; + skillRootDir: string; + candidatePath: string; +}): string | null { + const skillRootRealPath = tryRealpath(params.skillRootDir) ?? path.resolve(params.skillRootDir); + return resolveContainedSkillPath({ + source: params.source, + rootDir: params.skillRootDir, + rootRealPath: skillRootRealPath, + candidatePath: params.candidatePath, + }); +} + function resolveContainedSkillPath(params: { source: string; rootDir: string; @@ -300,36 +325,34 @@ function loadSkillEntries( const limits = resolveSkillsLimits(opts?.config); const loadSkills = (params: { dir: string; source: string }): Skill[] => { + const enforceContainedRealpath = shouldEnforceContainedSkillPaths(params.source); const rootDir = path.resolve(params.dir); const rootRealPath = tryRealpath(rootDir) ?? rootDir; const resolved = resolveNestedSkillsRoot(params.dir, { maxEntriesToScan: limits.maxCandidatesPerRoot, }); const baseDir = resolved.baseDir; - const baseDirRealPath = resolveContainedSkillPath({ - source: params.source, - rootDir, - rootRealPath, - candidatePath: baseDir, - }); - if (!baseDirRealPath) { - return []; - } + const baseDirRealPath = enforceContainedRealpath + ? resolveContainedSkillPath({ + source: params.source, + rootDir, + rootRealPath, + candidatePath: baseDir, + }) + : (tryRealpath(baseDir) ?? path.resolve(baseDir)); + if (!baseDirRealPath) return []; // If the root itself is a skill directory, just load it directly (but enforce size cap). const rootSkillMd = path.join(baseDir, "SKILL.md"); if (fs.existsSync(rootSkillMd)) { - const rootSkillRealPath = resolveContainedSkillPath({ + const rootSkillPathForSize = resolveContainedPathWithinSkillRoot({ source: params.source, - rootDir, - rootRealPath: baseDirRealPath, + skillRootDir: baseDir, candidatePath: rootSkillMd, }); - if (!rootSkillRealPath) { - return []; - } + if (!rootSkillPathForSize) return []; try { - const size = fs.statSync(rootSkillRealPath).size; + const size = fs.statSync(rootSkillPathForSize).size; if (size > limits.maxSkillFileBytes) { skillsLogger.warn("Skipping skills root due to oversized SKILL.md.", { dir: baseDir, @@ -344,12 +367,14 @@ function loadSkillEntries( } const loaded = loadSkillsFromDir({ dir: baseDir, source: params.source }); - return filterLoadedSkillsInsideRoot({ - skills: unwrapLoadedSkills(loaded), - source: params.source, - rootDir, - rootRealPath: baseDirRealPath, - }); + return enforceContainedRealpath + ? filterLoadedSkillsInsideRoot({ + skills: unwrapLoadedSkills(loaded), + source: params.source, + rootDir, + rootRealPath: baseDirRealPath, + }) + : unwrapLoadedSkills(loaded); } const childDirs = listChildDirectories(baseDir); @@ -380,30 +405,29 @@ function loadSkillEntries( // Only consider immediate subfolders that look like skills (have SKILL.md) and are under size cap. for (const name of limitedChildren) { const skillDir = path.join(baseDir, name); - const skillDirRealPath = resolveContainedSkillPath({ - source: params.source, - rootDir, - rootRealPath: baseDirRealPath, - candidatePath: skillDir, - }); - if (!skillDirRealPath) { - continue; + if (enforceContainedRealpath) { + const skillDirRealPath = resolveContainedSkillPath({ + source: params.source, + rootDir, + rootRealPath: baseDirRealPath, + candidatePath: skillDir, + }); + if (!skillDirRealPath) { + continue; + } } const skillMd = path.join(skillDir, "SKILL.md"); if (!fs.existsSync(skillMd)) { continue; } - const skillMdRealPath = resolveContainedSkillPath({ + const skillMdPathForSize = resolveContainedPathWithinSkillRoot({ source: params.source, - rootDir, - rootRealPath: baseDirRealPath, + skillRootDir: skillDir, candidatePath: skillMd, }); - if (!skillMdRealPath) { - continue; - } + if (!skillMdPathForSize) continue; try { - const size = fs.statSync(skillMdRealPath).size; + const size = fs.statSync(skillMdPathForSize).size; if (size > limits.maxSkillFileBytes) { skillsLogger.warn("Skipping skill due to oversized SKILL.md.", { skill: name, @@ -419,12 +443,14 @@ function loadSkillEntries( const loaded = loadSkillsFromDir({ dir: skillDir, source: params.source }); loadedSkills.push( - ...filterLoadedSkillsInsideRoot({ - skills: unwrapLoadedSkills(loaded), - source: params.source, - rootDir, - rootRealPath: baseDirRealPath, - }), + ...(enforceContainedRealpath + ? filterLoadedSkillsInsideRoot({ + skills: unwrapLoadedSkills(loaded), + source: params.source, + rootDir, + rootRealPath: baseDirRealPath, + }) + : unwrapLoadedSkills(loaded)), ); if (loadedSkills.length >= limits.maxSkillsLoadedPerSource) {