This commit is contained in:
2026-03-15 14:40:07 +00:00 committed by GitHub
commit f4e7def409
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
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,9 +134,29 @@ 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 outsideDir = await createTempWorkspaceDir();
const externalSkillDir = path.join(outsideDir, "outside-skill");
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"));
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 workspaceDir = await createTempWorkspaceDir();
const outsideDir = await createTempWorkspaceDir(); const outsideDir = await createTempWorkspaceDir();
const escapedSkillDir = path.join(outsideDir, "outside-skill"); const escapedSkillDir = path.join(outsideDir, "outside-skill");
@ -142,7 +166,7 @@ describe("loadWorkspaceSkillEntries", () => {
description: "Outside", description: "Outside",
}); });
await fs.mkdir(path.join(workspaceDir, "skills"), { recursive: true }); await fs.mkdir(path.join(workspaceDir, "skills"), { recursive: true });
await fs.symlink(escapedSkillDir, path.join(workspaceDir, "skills", "escaped-skill"), "dir"); await symlinkDir(escapedSkillDir, path.join(workspaceDir, "skills", "escaped-skill"));
const entries = loadWorkspaceSkillEntries(workspaceDir, { const entries = loadWorkspaceSkillEntries(workspaceDir, {
managedSkillsDir: path.join(workspaceDir, ".managed"), managedSkillsDir: path.join(workspaceDir, ".managed"),
@ -150,8 +174,7 @@ describe("loadWorkspaceSkillEntries", () => {
}); });
expect(entries.map((entry) => entry.skill.name)).not.toContain("outside-skill"); 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
? resolveContainedSkillPath({
source: params.source, source: params.source,
rootDir, rootDir,
rootRealPath, rootRealPath,
candidatePath: baseDir, candidatePath: baseDir,
}); })
if (!baseDirRealPath) { : (tryRealpath(baseDir) ?? path.resolve(baseDir));
return []; 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
? resolveContainedSkillPath({
source: params.source, source: params.source,
rootDir, rootDir,
rootRealPath: baseDirRealPath, rootRealPath: baseDirRealPath,
candidatePath: rootSkillMd, candidatePath: rootSkillMd,
}); })
if (!rootSkillRealPath) { : rootSkillMd;
return []; 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
? filterLoadedSkillsInsideRoot({
skills: unwrapLoadedSkills(loaded), skills: unwrapLoadedSkills(loaded),
source: params.source, source: params.source,
rootDir, rootDir,
rootRealPath: baseDirRealPath, rootRealPath: baseDirRealPath,
}); })
: unwrapLoadedSkills(loaded);
} }
const childDirs = listChildDirectories(baseDir); const childDirs = listChildDirectories(baseDir);
@ -380,6 +394,7 @@ 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);
if (enforceContainedRealpath) {
const skillDirRealPath = resolveContainedSkillPath({ const skillDirRealPath = resolveContainedSkillPath({
source: params.source, source: params.source,
rootDir, rootDir,
@ -389,21 +404,22 @@ function loadSkillEntries(
if (!skillDirRealPath) { if (!skillDirRealPath) {
continue; 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
? resolveContainedSkillPath({
source: params.source, source: params.source,
rootDir, rootDir,
rootRealPath: baseDirRealPath, rootRealPath: baseDirRealPath,
candidatePath: skillMd, candidatePath: skillMd,
}); })
if (!skillMdRealPath) { : skillMd;
continue; 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
? filterLoadedSkillsInsideRoot({
skills: unwrapLoadedSkills(loaded), skills: unwrapLoadedSkills(loaded),
source: params.source, source: params.source,
rootDir, rootDir,
rootRealPath: baseDirRealPath, rootRealPath: baseDirRealPath,
}), })
: unwrapLoadedSkills(loaded)),
); );
if (loadedSkills.length >= limits.maxSkillsLoadedPerSource) { if (loadedSkills.length >= limits.maxSkillsLoadedPerSource) {