fix(security): align audit symlink_escape boundary with skill loader#60144
fix(security): align audit symlink_escape boundary with skill loader#60144menhguin wants to merge 1 commit into
Conversation
Greptile SummaryThis PR fixes a gap between the Confidence Score: 5/5Safe to merge — targeted, correct fix with a new test case covering the exact gap. The change is minimal and mechanically correct: the audit probe now uses realpath(workspace/skills/) as the containment boundary, which matches exactly what the loader (resolveContainedSkillPath) uses via baseDirRealPath. The fallback in fs.realpath(skillsDir).catch(() => skillsDir) is consistent with the pre-existing pattern. The new test verifies the specific gap (inside workspace, outside skills/), and the existing warns outside workspace root test remains a valid superset check. No regressions expected. No files require special attention. Reviews (1): Last reviewed commit: "fix(security): align audit symlink_escap..." | Re-trigger Greptile |
The skills.workspace.symlink_escape audit probe checked whether skill file realpaths escaped the workspace root (~/.openclaw/workspace/), but the skill loader (resolveContainedSkillPath) checks against the skills directory root (~/.openclaw/workspace/skills/). This mismatch meant symlinks like: workspace/skills/my-skill -> workspace/other-dir/skills/my-skill ...would resolve inside the workspace root (audit says OK) but outside the skills directory (loader silently rejects). The skill would fail to load with zero feedback from `openclaw security audit`. Fix: check against the skills directory realpath in the audit probe, matching the loader's boundary. Add test for the in-workspace but outside-skills-dir case. Relates to openclaw#49408
d561e96 to
b40c56a
Compare
openclaw-barnacle
Bot
added
docs
openclaw-barnacle
Bot
added
gateway
|
Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch. |
|
Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch. |
1 participant
Bug
The
skills.workspace.symlink_escapeaudit probe checks whether skill file realpaths escape the workspace root (~/.openclaw/workspace/), but the skill loader (resolveContainedSkillPathinsrc/agents/skills/workspace.ts) checks against the skills directory root (~/.openclaw/workspace/skills/).This means symlinks like:
...resolve to
workspace/ehoy/.claude/skills/my-skill, which is:workspace/skills/→ loader silently rejects itopenclaw security auditreports no findings, but the skill is silently dropped at load time with zero user feedback.How We Found It
We had 10 symlinked skills that were silently invisible for nearly a month after v2026.3.7 shipped the realpath enforcement in the loader. The daily-review cron skill stopped working but the model was finding and executing the skill through manual filesystem search ~80% of the time, masking the fact that formal skill discovery never included it.
openclaw security auditshowed nothing wrong.Fix
Changed the audit probe to check against the skills directory realpath (
workspace/skills/) instead of the workspace root, matching the loader's boundary. Updated finding messages to reflect the tighter boundary.Test
Added a test case for the specific gap: a symlink that resolves inside the workspace root but outside the skills directory now correctly triggers the
skills.workspace.symlink_escapefinding.All existing tests pass. The existing "warns when workspace skill files resolve outside workspace root" test continues to pass (those escapes are a superset of the new check).
AI-Assisted
This PR was authored by an AI agent (Claude Opus via OpenClaw). The code changes were verified against the existing test suite and a new test case was added. The agent reviewed git blame and commit history to confirm this was an oversight (the probe was added Mar 2 before the Mar 8 loader hardening), not intentional behavior.
Relates to #49408