From cca4f3c63cc198a3fe3ef5e44042110e4ae8a0b1 Mon Sep 17 00:00:00 2001 From: stainlu Date: Thu, 14 May 2026 12:04:05 +0800 Subject: [PATCH] fix(skills): trust managed skill symlink roots --- CHANGELOG.md | 1 + docs/tools/skills-config.md | 27 +++--- docs/tools/skills.md | 12 +-- .../skills.loadworkspaceskillentries.test.ts | 73 ++++++++++++++++- src/agents/skills/workspace.ts | 82 +++++++++++++++---- 5 files changed, 159 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03a5446f5b6..efeef34697e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,7 @@ Docs: https://docs.openclaw.ai - Control UI: keep shared form, config, and usage text-entry controls at 16px on touch-primary devices while preserving chat composer input sizing, so iOS Safari no longer auto-zooms focused fields. Fixes #64651; carries forward #64673. Thanks @NianJiuZst and @BunsDev. - Codex harness: classify native app-server token-refresh logout and relogin failures as authentication refresh errors, so users get re-authentication guidance instead of a raw runtime failure. - Agents/trajectory: make the trajectory flush cleanup timeout configurable with `OPENCLAW_TRAJECTORY_FLUSH_TIMEOUT_MS`, preserving the 10s default while slower stores drain. Refs #75839. Thanks @BunsDev. +- Skills: load ClawHub and local-manager skill-directory symlinks from managed `~/.openclaw/skills` and personal `~/.agents/skills` roots while keeping workspace, extra, bundled, and per-skill `SKILL.md` containment fail-closed. Fixes #44051. Refs #59219. Thanks @Devattom, @ArthurNie, and @luoxiao6645. - Codex startup: treat selectable configured OpenAI agent models as Codex runtime requirements during plugin auto-enable, startup planning, and doctor install repair, so Anthropic-primary configs can still switch to OpenAI/Codex cleanly. - Agents: preserve source-reply delivery metadata when merging tool-returned media into the final reply, keeping message-tool-only replies deliverable and mirrored. Thanks @pashpashpash and @vincentkoc. - Replies: treat rich presentation, interactive controls, and channel-native payload data as outbound content across follow-up, heartbeat, cron, ACP, and block-streaming delivery paths, preventing card/button-only replies from being dropped as empty. diff --git a/docs/tools/skills-config.md b/docs/tools/skills-config.md index 52bd384b316..65a465f83cb 100644 --- a/docs/tools/skills-config.md +++ b/docs/tools/skills-config.md @@ -90,9 +90,13 @@ Rules: bundled skills in the list are eligible (managed, agent, and workspace skills unaffected). - `load.extraDirs`: additional skill directories to scan (lowest precedence). - `load.allowSymlinkTargets`: trusted real target directories that symlinked - skill folders may resolve into even when the symlink lives outside that - target root. Use this for intentional sibling-repo layouts such as - `~/.agents/skills/manager -> ~/Projects/manager/skills`. + workspace, project-agent, or extra-dir skill folders may resolve into even + when the symlink lives outside that target root. Use this for intentional + sibling-repo layouts such as + `/skills/manager -> ~/Projects/manager/skills`. Managed + `~/.openclaw/skills` and personal `~/.agents/skills` roots may follow + skill-directory symlinks from local skill managers by default, but every + `SKILL.md` still has to resolve inside its own skill directory. - `load.watch`: watch skill folders and refresh the skills snapshot (default: true). - `load.watchDebounceMs`: debounce for skill watcher events in milliseconds (default: 250). - `install.preferBrew`: prefer brew installers when available (default: true). @@ -114,10 +118,10 @@ Rules: ## Symlinked sibling repos -By default, each skill root is a containment boundary. If a skill folder under -`~/.agents/skills` is a symlink that resolves outside `~/.agents/skills`, -OpenClaw skips it and logs `Skipping escaped skill path outside its configured -root`. +By default, workspace, project-agent, extra-dir, and bundled skill roots are +containment boundaries. If a skill folder under `/skills` is a +symlink that resolves outside `/skills`, OpenClaw skips it and logs +`Skipping escaped skill path outside its configured root`. Keep the symlink layout and allow only the trusted target root: @@ -133,10 +137,13 @@ Keep the symlink layout and allow only the trusted target root: ``` With this config, a symlink such as -`~/.agents/skills/manager -> ~/Projects/manager/skills` is accepted after +`/skills/manager -> ~/Projects/manager/skills` is accepted after realpath resolution. `extraDirs` also scans the sibling repo directly, while -`allowSymlinkTargets` preserves the symlinked path for existing agent-skill -layouts. Keep target entries narrow; do not point at broad roots such as `~` or +`allowSymlinkTargets` preserves the symlinked path for existing workspace-skill +layouts. Managed `~/.openclaw/skills` and personal `~/.agents/skills` +directories already accept skill-directory symlinks because those roots are +user-owned local skill-manager surfaces; per-skill `SKILL.md` containment still +applies. Keep target entries narrow; do not point at broad roots such as `~` or `~/Projects` unless every skill tree under that root is trusted. Per-skill fields: diff --git a/docs/tools/skills.md b/docs/tools/skills.md index b21e06050d1..80ab50edcec 100644 --- a/docs/tools/skills.md +++ b/docs/tools/skills.md @@ -170,7 +170,7 @@ Prefer sandboxed runs for untrusted inputs and risky tools. See [Sandboxing](/gateway/sandboxing) for the agent-side controls. -- Workspace and extra-dir skill discovery only accepts skill roots and `SKILL.md` files whose resolved realpath stays inside the configured root. +- Workspace, project-agent, and extra-dir skill discovery only accepts skill roots whose resolved realpath stays inside the configured root unless `skills.load.allowSymlinkTargets` explicitly trusts a target root. Bundled skills always stay contained. Managed `~/.openclaw/skills` and personal `~/.agents/skills` roots may contain symlinked skill folders installed by ClawHub or another local skill manager, but every `SKILL.md` realpath must still stay inside its resolved skill directory. - Gateway private archive installs are off by default. When explicitly enabled, they require a committed zip upload containing `SKILL.md` and reuse the same archive extraction, path traversal, symlink, force, and rollback protections as @@ -448,10 +448,12 @@ when `SKILL.md` files change. Configure under `skills.load`: } ``` -Use `allowSymlinkTargets` for intentional sibling-repo layouts where a built-in -skill root contains a symlink, for example -`~/.agents/skills/manager -> ~/Projects/manager/skills`. The target list is -matched after realpath resolution and should stay narrow. +Use `allowSymlinkTargets` for intentional workspace, project-agent, or extra-dir +layouts where a skill root contains a symlink, for example +`/skills/manager -> ~/Projects/manager/skills`. Managed +`~/.openclaw/skills` and personal `~/.agents/skills` can follow skill-directory +symlinks from local skill managers by default, but the target list is still +matched after realpath resolution and should stay narrow when configured. ### Remote macOS nodes (Linux gateway) diff --git a/src/agents/skills.loadworkspaceskillentries.test.ts b/src/agents/skills.loadworkspaceskillentries.test.ts index 8022197dd14..114b4f4259d 100644 --- a/src/agents/skills.loadworkspaceskillentries.test.ts +++ b/src/agents/skills.loadworkspaceskillentries.test.ts @@ -395,9 +395,9 @@ describe("loadWorkspaceSkillEntries", () => { name: skillName, description: "Manager skill", }); - const personalSkillsDir = path.join(fakeHome, ".agents", "skills"); - await fs.mkdir(personalSkillsDir, { recursive: true }); - const symlinkPath = path.join(personalSkillsDir, skillName); + const workspaceSkillsDir = path.join(workspaceDir, "skills"); + await fs.mkdir(workspaceSkillsDir, { recursive: true }); + const symlinkPath = path.join(workspaceSkillsDir, skillName); await fs.symlink(targetSkillDir, symlinkPath, "dir"); const warn = captureWarningLogger(); @@ -420,6 +420,73 @@ describe("loadWorkspaceSkillEntries", () => { }, ); + it.runIf(process.platform !== "win32")( + "loads managed skill directory symlinks outside the managed root", + async () => { + const workspaceDir = await createTempWorkspaceDir(); + const managedDir = path.join(workspaceDir, ".managed"); + const skillName = `managed-${++workspaceCaseIndex}`; + const targetSkillDir = path.join(tempRoot, `${skillName}-target`, skillName); + await writeSkill({ + dir: targetSkillDir, + name: skillName, + description: "Managed symlink target", + }); + await fs.mkdir(managedDir, { recursive: true }); + const symlinkPath = path.join(managedDir, skillName); + await fs.symlink(targetSkillDir, symlinkPath, "dir"); + const warn = captureWarningLogger(); + + try { + const entries = loadTestWorkspaceSkillEntries(workspaceDir, { + managedSkillsDir: managedDir, + }); + + expect(entries.map((entry) => entry.skill.name)).toContain(skillName); + expect(warn).not.toHaveBeenCalled(); + } finally { + await fs.unlink(symlinkPath).catch(() => undefined); + } + }, + ); + + it.runIf(process.platform !== "win32")( + "keeps SKILL.md containment for managed symlinked skill directories", + async () => { + const workspaceDir = await createTempWorkspaceDir(); + const managedDir = path.join(workspaceDir, ".managed"); + const skillName = `managed-file-link-${++workspaceCaseIndex}`; + const targetSkillDir = path.join(tempRoot, `${skillName}-target`, skillName); + const outsideDir = path.join(tempRoot, `${skillName}-outside`); + await fs.mkdir(targetSkillDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + await writeSkill({ + dir: outsideDir, + name: skillName, + description: "Escaped metadata", + }); + await fs.symlink(path.join(outsideDir, "SKILL.md"), path.join(targetSkillDir, "SKILL.md")); + await fs.mkdir(managedDir, { recursive: true }); + const symlinkPath = path.join(managedDir, skillName); + await fs.symlink(targetSkillDir, symlinkPath, "dir"); + const warn = captureWarningLogger(); + + try { + const entries = loadTestWorkspaceSkillEntries(workspaceDir, { + managedSkillsDir: managedDir, + }); + + expect(entries.map((entry) => entry.skill.name)).not.toContain(skillName); + const warningLine = firstWarningLine(warn); + expect(warningLine).toContain("Skipping escaped skill path outside its configured root:"); + expect(warningLine).toContain("source=openclaw-managed"); + expect(warningLine).toContain("reason=symlink-escape"); + } finally { + await fs.unlink(symlinkPath).catch(() => undefined); + } + }, + ); + it.runIf(process.platform !== "win32")( "calls out bundled symlink escapes with compact home-relative paths", async () => { diff --git a/src/agents/skills/workspace.ts b/src/agents/skills/workspace.ts index d227070f57e..620986e9bb3 100644 --- a/src/agents/skills/workspace.ts +++ b/src/agents/skills/workspace.ts @@ -404,6 +404,53 @@ function isPathInsideAnyRoot(rootRealPaths: readonly string[], candidateRealPath return rootRealPaths.some((rootRealPath) => isPathInside(rootRealPath, candidateRealPath)); } +function shouldEnforceConfiguredSkillRootContainment(source: string): boolean { + return source !== "openclaw-managed" && source !== "agents-skills-personal"; +} + +function shouldUseConfiguredSymlinkTargets(source: string): boolean { + return ( + source === "openclaw-workspace" || + source === "openclaw-extra" || + source === "agents-skills-project" + ); +} + +function resolveSkillRootCandidatePath(params: { + source: string; + rootDir: string; + rootRealPath: string; + candidatePath: string; + allowedSymlinkTargetRealPaths: readonly string[]; +}): string | null { + if (!shouldEnforceConfiguredSkillRootContainment(params.source)) { + return tryRealpath(params.candidatePath); + } + return resolveContainedSkillPath({ + source: params.source, + rootDir: params.rootDir, + rootRealPath: params.rootRealPath, + candidatePath: params.candidatePath, + allowedSymlinkTargetRealPaths: shouldUseConfiguredSymlinkTargets(params.source) + ? params.allowedSymlinkTargetRealPaths + : [], + }); +} + +function resolveSkillFilePath(params: { + source: string; + skillDir: string; + skillDirRealPath: string; + candidatePath: string; +}): string | null { + return resolveContainedSkillPath({ + source: params.source, + rootDir: params.skillDir, + rootRealPath: params.skillDirRealPath, + candidatePath: params.candidatePath, + }); +} + function resolvePluginSkillRootRealPaths(pluginSkillDirs: readonly string[]): string[] { return pluginSkillDirs .map((dir) => tryRealpath(dir)) @@ -535,7 +582,7 @@ function loadSkillEntries( maxEntriesToScan: limits.maxCandidatesPerRoot, }); const baseDir = resolved.baseDir; - const baseDirRealPath = resolveContainedSkillPath({ + const baseDirRealPath = resolveSkillRootCandidatePath({ source: params.source, rootDir, rootRealPath, @@ -549,12 +596,11 @@ function loadSkillEntries( // 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 rootSkillRealPath = resolveSkillFilePath({ source: params.source, - rootDir, - rootRealPath: baseDirRealPath, + skillDir: baseDir, + skillDirRealPath: baseDirRealPath, candidatePath: rootSkillMd, - allowedSymlinkTargetRealPaths, }); if (!rootSkillRealPath) { return []; @@ -642,7 +688,7 @@ function loadSkillEntries( // skill directories (e.g. ~/.openclaw/skills/coze/koze-retrieval/SKILL.md). for (const name of limitedChildren) { const skillDir = path.join(baseDir, name); - const skillDirRealPath = resolveContainedSkillPath({ + const skillDirRealPath = resolveSkillRootCandidatePath({ source: params.source, rootDir, rootRealPath: baseDirRealPath, @@ -654,12 +700,11 @@ function loadSkillEntries( } const skillMd = path.join(skillDir, "SKILL.md"); if (fs.existsSync(skillMd)) { - const skillMdRealPath = resolveContainedSkillPath({ + const skillMdRealPath = resolveSkillFilePath({ source: params.source, - rootDir, - rootRealPath: baseDirRealPath, + skillDir, + skillDirRealPath, candidatePath: skillMd, - allowedSymlinkTargetRealPaths, }); if (skillMdRealPath) { loadCandidateSkill({ skillDir, name, skillMdRealPath }); @@ -701,20 +746,21 @@ function loadSkillEntries( const nestedDir = path.join(skillDir, nestedName); const nestedSkillMd = path.join(nestedDir, "SKILL.md"); if (fs.existsSync(nestedSkillMd)) { - const nestedDirRealPath = resolveContainedSkillPath({ + const nestedDirRealPath = resolveSkillRootCandidatePath({ source: params.source, rootDir, rootRealPath: baseDirRealPath, candidatePath: nestedDir, allowedSymlinkTargetRealPaths, }); - const nestedSkillMdRealPath = resolveContainedSkillPath({ - source: params.source, - rootDir, - rootRealPath: baseDirRealPath, - candidatePath: nestedSkillMd, - allowedSymlinkTargetRealPaths, - }); + const nestedSkillMdRealPath = nestedDirRealPath + ? resolveSkillFilePath({ + source: params.source, + skillDir: nestedDir, + skillDirRealPath: nestedDirRealPath, + candidatePath: nestedSkillMd, + }) + : null; if (nestedDirRealPath && nestedSkillMdRealPath) { loadCandidateSkill({ skillDir: nestedDir,