diff --git a/src/agents/skills.loadworkspaceskillentries.test.ts b/src/agents/skills.loadworkspaceskillentries.test.ts index 96fa9f7e9c3..97d4ef8e741 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", diff --git a/src/agents/skills/workspace.ts b/src/agents/skills/workspace.ts index 84c8ea78df3..8e71fdc7cd1 100644 --- a/src/agents/skills/workspace.ts +++ b/src/agents/skills/workspace.ts @@ -198,6 +198,17 @@ 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 resolveContainedSkillPath(params: { source: string; rootDir: string; @@ -300,36 +311,37 @@ 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({ - source: params.source, - rootDir, - rootRealPath: baseDirRealPath, - candidatePath: rootSkillMd, - }); - if (!rootSkillRealPath) { - return []; - } + const rootSkillPathForSize = enforceContainedRealpath + ? resolveContainedSkillPath({ + source: params.source, + rootDir, + rootRealPath: baseDirRealPath, + candidatePath: rootSkillMd, + }) + : rootSkillMd; + 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 +356,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 +394,32 @@ 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({ - source: params.source, - rootDir, - rootRealPath: baseDirRealPath, - candidatePath: skillMd, - }); - if (!skillMdRealPath) { - continue; - } + const skillMdPathForSize = enforceContainedRealpath + ? resolveContainedSkillPath({ + source: params.source, + rootDir, + rootRealPath: baseDirRealPath, + candidatePath: skillMd, + }) + : skillMd; + 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 +435,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) {