Ayuriel/openclaw
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(skills): allow managed symlink dirs while enforcing SKILL.md containment

Browse Source
This commit is contained in:
kingdess 2026-03-20 20:50:13 +08:00
parent 4c60956d8e
commit 3d8eccc658
2 changed files with 149 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

100
src/agents/skills.loadworkspaceskillentries.test.ts
View File

@ -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");
},
);
});

112
src/agents/skills/workspace.ts
View File

@ -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) {