fix(skills): allow symlinked managed skill directories

This commit is contained in:
kingdess 2026-03-13 19:45:18 +08:00
parent f9ea879729
commit f8c8845b77
2 changed files with 110 additions and 69 deletions

View File

@ -14,6 +14,10 @@ async function createTempWorkspaceDir() {
return workspaceDir; return workspaceDir;
} }
async function symlinkDir(targetDir: string, linkPath: string) {
await fs.symlink(targetDir, linkPath, process.platform === "win32" ? "junction" : "dir");
}
afterEach(async () => { afterEach(async () => {
await Promise.all( await Promise.all(
tempDirs.splice(0, tempDirs.length).map((dir) => fs.rm(dir, { recursive: true, force: true })), 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"); expect(entries.map((entry) => entry.skill.name)).not.toContain("diffs");
}); });
it.runIf(process.platform !== "win32")( it("allows managed skill directories that resolve outside the managed root", async () => {
"skips workspace skill directories that resolve outside the workspace root", const workspaceDir = await createTempWorkspaceDir();
async () => { const managedDir = path.join(workspaceDir, ".managed");
const workspaceDir = await createTempWorkspaceDir(); const outsideDir = await createTempWorkspaceDir();
const outsideDir = await createTempWorkspaceDir(); const externalSkillDir = path.join(outsideDir, "outside-skill");
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");
const entries = loadWorkspaceSkillEntries(workspaceDir, { await writeSkill({
managedSkillsDir: path.join(workspaceDir, ".managed"), dir: externalSkillDir,
bundledSkillsDir: path.join(workspaceDir, ".bundled"), 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")( it.runIf(process.platform !== "win32")(
"skips workspace skill files that resolve outside the workspace root", "skips workspace skill files that resolve outside the workspace root",

View File

@ -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: { function resolveContainedSkillPath(params: {
source: string; source: string;
rootDir: string; rootDir: string;
@ -300,36 +311,37 @@ function loadSkillEntries(
const limits = resolveSkillsLimits(opts?.config); const limits = resolveSkillsLimits(opts?.config);
const loadSkills = (params: { dir: string; source: string }): Skill[] => { const loadSkills = (params: { dir: string; source: string }): Skill[] => {
const enforceContainedRealpath = shouldEnforceContainedSkillPaths(params.source);
const rootDir = path.resolve(params.dir); const rootDir = path.resolve(params.dir);
const rootRealPath = tryRealpath(rootDir) ?? rootDir; const rootRealPath = tryRealpath(rootDir) ?? rootDir;
const resolved = resolveNestedSkillsRoot(params.dir, { const resolved = resolveNestedSkillsRoot(params.dir, {
maxEntriesToScan: limits.maxCandidatesPerRoot, maxEntriesToScan: limits.maxCandidatesPerRoot,
}); });
const baseDir = resolved.baseDir; const baseDir = resolved.baseDir;
const baseDirRealPath = resolveContainedSkillPath({ const baseDirRealPath = enforceContainedRealpath
source: params.source, ? resolveContainedSkillPath({
rootDir, source: params.source,
rootRealPath, rootDir,
candidatePath: baseDir, rootRealPath,
}); candidatePath: baseDir,
if (!baseDirRealPath) { })
return []; : (tryRealpath(baseDir) ?? path.resolve(baseDir));
} if (!baseDirRealPath) return [];
// If the root itself is a skill directory, just load it directly (but enforce size cap). // If the root itself is a skill directory, just load it directly (but enforce size cap).
const rootSkillMd = path.join(baseDir, "SKILL.md"); const rootSkillMd = path.join(baseDir, "SKILL.md");
if (fs.existsSync(rootSkillMd)) { if (fs.existsSync(rootSkillMd)) {
const rootSkillRealPath = resolveContainedSkillPath({ const rootSkillPathForSize = enforceContainedRealpath
source: params.source, ? resolveContainedSkillPath({
rootDir, source: params.source,
rootRealPath: baseDirRealPath, rootDir,
candidatePath: rootSkillMd, rootRealPath: baseDirRealPath,
}); candidatePath: rootSkillMd,
if (!rootSkillRealPath) { })
return []; : rootSkillMd;
} if (!rootSkillPathForSize) return [];
try { try {
const size = fs.statSync(rootSkillRealPath).size; const size = fs.statSync(rootSkillPathForSize).size;
if (size > limits.maxSkillFileBytes) { if (size > limits.maxSkillFileBytes) {
skillsLogger.warn("Skipping skills root due to oversized SKILL.md.", { skillsLogger.warn("Skipping skills root due to oversized SKILL.md.", {
dir: baseDir, dir: baseDir,
@ -344,12 +356,14 @@ function loadSkillEntries(
} }
const loaded = loadSkillsFromDir({ dir: baseDir, source: params.source }); const loaded = loadSkillsFromDir({ dir: baseDir, source: params.source });
return filterLoadedSkillsInsideRoot({ return enforceContainedRealpath
skills: unwrapLoadedSkills(loaded), ? filterLoadedSkillsInsideRoot({
source: params.source, skills: unwrapLoadedSkills(loaded),
rootDir, source: params.source,
rootRealPath: baseDirRealPath, rootDir,
}); rootRealPath: baseDirRealPath,
})
: unwrapLoadedSkills(loaded);
} }
const childDirs = listChildDirectories(baseDir); 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. // Only consider immediate subfolders that look like skills (have SKILL.md) and are under size cap.
for (const name of limitedChildren) { for (const name of limitedChildren) {
const skillDir = path.join(baseDir, name); const skillDir = path.join(baseDir, name);
const skillDirRealPath = resolveContainedSkillPath({ if (enforceContainedRealpath) {
source: params.source, const skillDirRealPath = resolveContainedSkillPath({
rootDir, source: params.source,
rootRealPath: baseDirRealPath, rootDir,
candidatePath: skillDir, rootRealPath: baseDirRealPath,
}); candidatePath: skillDir,
if (!skillDirRealPath) { });
continue; if (!skillDirRealPath) {
continue;
}
} }
const skillMd = path.join(skillDir, "SKILL.md"); const skillMd = path.join(skillDir, "SKILL.md");
if (!fs.existsSync(skillMd)) { if (!fs.existsSync(skillMd)) {
continue; continue;
} }
const skillMdRealPath = resolveContainedSkillPath({ const skillMdPathForSize = enforceContainedRealpath
source: params.source, ? resolveContainedSkillPath({
rootDir, source: params.source,
rootRealPath: baseDirRealPath, rootDir,
candidatePath: skillMd, rootRealPath: baseDirRealPath,
}); candidatePath: skillMd,
if (!skillMdRealPath) { })
continue; : skillMd;
} if (!skillMdPathForSize) continue;
try { try {
const size = fs.statSync(skillMdRealPath).size; const size = fs.statSync(skillMdPathForSize).size;
if (size > limits.maxSkillFileBytes) { if (size > limits.maxSkillFileBytes) {
skillsLogger.warn("Skipping skill due to oversized SKILL.md.", { skillsLogger.warn("Skipping skill due to oversized SKILL.md.", {
skill: name, skill: name,
@ -419,12 +435,14 @@ function loadSkillEntries(
const loaded = loadSkillsFromDir({ dir: skillDir, source: params.source }); const loaded = loadSkillsFromDir({ dir: skillDir, source: params.source });
loadedSkills.push( loadedSkills.push(
...filterLoadedSkillsInsideRoot({ ...(enforceContainedRealpath
skills: unwrapLoadedSkills(loaded), ? filterLoadedSkillsInsideRoot({
source: params.source, skills: unwrapLoadedSkills(loaded),
rootDir, source: params.source,
rootRealPath: baseDirRealPath, rootDir,
}), rootRealPath: baseDirRealPath,
})
: unwrapLoadedSkills(loaded)),
); );
if (loadedSkills.length >= limits.maxSkillsLoadedPerSource) { if (loadedSkills.length >= limits.maxSkillsLoadedPerSource) {