Skip to content

fix(security): align audit symlink_escape boundary with skill loader#73948

Open
openclaw-clownfish[bot] wants to merge 2 commits into
mainfrom
clownfish/ghcrawl-156898-autonomous-smoke
Open

fix(security): align audit symlink_escape boundary with skill loader#73948
openclaw-clownfish[bot] wants to merge 2 commits into
mainfrom
clownfish/ghcrawl-156898-autonomous-smoke

Conversation

@openclaw-clownfish

@openclaw-clownfish openclaw-clownfish Bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • align the skills.workspace.symlink_escape audit probe with the workspace skills-directory realpath enforced by the skill loader
  • update title/detail/remediation copy so the finding consistently names the skills directory boundary
  • add the inside-workspace/outside-skills symlink regression test without lockfile or unrelated churn

Credit

This carries forward the narrow fix idea from @menhguin / Minh Nguyen in #60155 and the later duplicate source PR #60513.

Review notes

Addresses Greptile's source-PR findings by making the symlink test comment match the actual symlink target and by updating the stale remediation wording.

Validation

  • pnpm check:changed

ProjectClownfish replacement details:

@openclaw-clownfish
openclaw-clownfish Bot requested a review from a team as a code owner April 29, 2026 01:58
@openclaw-clownfish openclaw-clownfish Bot added the clawsweeper Tracked by ClawSweeper automation label Apr 29, 2026
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation gateway Gateway runtime size: XS r: too-many-prs Auto-close: author has more than twenty active PRs. labels Apr 29, 2026
@openclaw-barnacle

openclaw-barnacle Bot commented Apr 29, 2026

Copy link
Copy Markdown

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

@greptile-apps

greptile-apps Bot commented Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR narrows the skills.workspace.symlink_escape audit probe boundary from the workspace root to the workspace skills/ subdirectory, aligning it with the skill loader's own realpath enforcement. It updates the finding title, detail, and remediation copy consistently, adds a regression test for the inside-workspace/outside-skills symlink case, and reformats the audit-checks documentation table.

Confidence Score: 5/5

This PR is safe to merge — the logic change is narrow, well-tested, and the fallback path for a missing skills directory is handled correctly.

No P0 or P1 issues found. The boundary change is logically correct, all updated text assertions in tests match the new copy, the skillsRootRealPath fallback is safe, and the docs table change is purely cosmetic whitespace reformatting.

No files require special attention.

Reviews (2): Last reviewed commit: "fix(clownfish): address review for ghcra..." | Re-trigger Greptile

@vincentkoc vincentkoc reopened this Apr 29, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the r: too-many-prs Auto-close: author has more than twenty active PRs. label Apr 29, 2026
@clawsweeper

clawsweeper Bot commented Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed July 3, 2026, 4:13 PM ET / 20:13 UTC.

Summary
The PR changes the workspace skill symlink_escape audit check, related regression coverage, audit-check docs, and changelog copy to use the workspace skills/ directory as the symlink boundary.

PR surface: Source +1, Tests +28, Docs +1. Total +30 across 4 files.

Reproducibility: yes. from source inspection. Current main's active audit accepts skill realpaths inside the broader workspace root while the loader checks the configured skills root plus trusted symlink targets; I did not run a filesystem repro because this review is read-only.

Review metrics: 1 noteworthy metric.

  • Audit trust predicate: 1 changed predicate. This predicate decides whether loader-accepted symlinked skill layouts are reported as security escapes, so compatibility needs maintainer attention before merge.

Stored data model
Persistent data-model change detected: migration/backfill/repair: CHANGELOG.md, serialized state: src/security/audit-workspace-skill-escape.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: canonical
Canonical: #73948
Summary: This PR is the current open candidate for the workspace skill audit/loader symlink-boundary mismatch; earlier source attempts are closed, and broader skill symlink/orphan reports only partially overlap.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🌊 off-meta tidepool
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Port the change and tests to src/skills/security/workspace-audit.ts and src/skills/security/workspace-audit.test.ts.
  • Mirror the loader behavior for skills.load.allowSymlinkTargets.
  • Remove the normal-PR CHANGELOG.md edit.

Risk before merge

  • [P1] The branch is dirty against current main and changes obsolete src/security/audit-workspace-* files, so the active audit path remains unfixed until the change is ported.
  • [P1] Porting the predicate unchanged would warn on documented skills.load.allowSymlinkTargets layouts that the current loader accepts.
  • [P1] The PR edits CHANGELOG.md, which OpenClaw policy keeps release-owned for normal PRs.

Maintainer options:

  1. Port And Mirror Loader Trust (recommended)
    Move the audit change into the active skills security module and treat configured allowed symlink target realpaths as audit-compatible before merge.
  2. Document A Stricter Audit Policy
    If maintainers intentionally want audit warnings for loader-accepted trusted targets, document that divergence and its operator impact before merge.
  3. Replace The Stale Branch
    If this branch cannot be repaired cleanly against current files, open a narrow replacement that credits the source work and close this PR afterward.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Port the audit-boundary change to `src/skills/security/workspace-audit.ts` and `src/skills/security/workspace-audit.test.ts`. Match the skill loader's trusted realpath contract, including `skills.load.allowSymlinkTargets`; add warning and no-warning regressions; update `docs/gateway/security/audit-checks.md`; remove the `CHANGELOG.md` edit.

Next step before merge

  • [P2] A bounded mechanical repair is clear: port the credited fix to current files, mirror loader-trust compatibility, and drop release-owned changelog churn.

Security
Needs attention: The diff changes a security-audit predicate, but it targets inactive current-main files and does not mirror the loader's trusted-target boundary.

Review findings

  • [P1] Port the audit fix to the active skills module — src/security/audit-workspace-skills.ts:130-131
  • [P1] Honor configured trusted symlink targets — src/security/audit-workspace-skills.ts:168
  • [P3] Remove the release-owned changelog edit — CHANGELOG.md:16
Review details

Best possible solution:

Port the fix to src/skills/security/workspace-audit.ts, mirror the loader's skills/ root plus trusted-target contract, update focused tests and audit docs, and remove the changelog edit.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection. Current main's active audit accepts skill realpaths inside the broader workspace root while the loader checks the configured skills root plus trusted symlink targets; I did not run a filesystem repro because this review is read-only.

Is this the best way to solve the issue?

No. The narrow fix idea is right, but the submitted branch patches obsolete files and omits the loader's trusted-target boundary, so the maintainable fix belongs in the active skills audit module.

Full review comments:

  • [P1] Port the audit fix to the active skills module — src/security/audit-workspace-skills.ts:130-131
    Current main routes this audit through src/skills/security/workspace-audit.ts, while this branch changes old src/security/audit-workspace-* files. Until the change is ported, the active audit path remains unfixed and the PR stays dirty against main.
    Confidence: 0.93
  • [P1] Honor configured trusted symlink targets — src/security/audit-workspace-skills.ts:168
    The loader accepts realpaths under skills.load.allowSymlinkTargets, but this predicate only accepts realpaths under the workspace skills directory. Supported trusted symlink layouts would load successfully while still being reported as escapes.
    Confidence: 0.9
  • [P3] Remove the release-owned changelog edit — CHANGELOG.md:16
    OpenClaw keeps CHANGELOG.md release-owned for normal PRs. This release-note context belongs in the PR body or squash/merge context, not in the branch diff.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5361e5a0b455.

Label changes

Label justifications:

  • P2: This is a normal-priority security-audit correctness fix with limited runtime blast radius but real operator diagnostic impact.
  • merge-risk: 🚨 compatibility: The proposed audit predicate can false-positive documented skills.load.allowSymlinkTargets layouts that current main loads successfully.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Not applicable because this is an OpenClaw bot replacement PR; targeted validation is still needed after repair.
Evidence reviewed

PR surface:

Source +1, Tests +28, Docs +1. Total +30 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 1 6 5 +1
Tests 1 29 1 +28
Docs 2 104 103 +1
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 139 109 +30

Security concerns:

  • [medium] Audit fix targets inactive current-main files — src/security/audit-workspace-skills.ts:130
    Current main routes the audit through src/skills/security/workspace-audit.ts, while this PR changes removed src/security/audit-workspace-* paths, leaving the real audit behavior unfixed unless ported.
    Confidence: 0.9
  • [low] Trusted skill targets would false-positive — src/security/audit-workspace-skills.ts:168
    The proposed predicate omits skills.load.allowSymlinkTargets, so documented trusted symlink layouts could be reported as security escapes even though the loader accepts them.
    Confidence: 0.87

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs src/skills/security/workspace-audit.test.ts src/skills/loading/workspace-load.test.ts.
  • [P1] git diff --check.
  • [P1] node scripts/crabbox-wrapper.mjs run --shell -- "pnpm check:changed".

What I checked:

Likely related people:

  • steipete: Commit 132794fe74d233098e71e4cde19a5d3f25473552 added the workspace skill symlink escape audit and touched the original security audit files. (role: introduced audit behavior and adjacent owner; confidence: high; commits: 132794fe74d2; files: src/security/audit-extra.async.ts, src/security/audit.test.ts, src/security/audit.ts)
  • abnershang: Commit 287b10a89507f17d9a75e07918d18ab5be48675e authored skills.load.allowSymlinkTargets, which is the trusted-target compatibility contract this PR must preserve. (role: adjacent feature contributor; confidence: high; commits: 287b10a89507; files: src/skills/loading/symlink-targets.ts, src/skills/loading/workspace.ts, src/skills/runtime/refresh.ts)
  • visionik: Commit f624b1d246a9aae71465e95620ddb9122492f9b0 modified workspace audit traversal, realpath handling, and related regression tests around this check. (role: adjacent security audit contributor; confidence: medium; commits: f624b1d246a9; files: src/security/audit-extra.async.ts, src/security/audit-workspace-skill-escape.test.ts, src/security/scan-paths.ts)
  • menhguin: The PR body and closed source attempts credit this handle for the exact audit-boundary reproduction and narrow fix idea carried forward here. (role: source reproduction contributor; confidence: medium; commits: 0daf50308af1, bf84e3a065d3; files: src/security/audit-extra.async.ts, src/security/audit.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot mentioned this pull request Apr 29, 2026
Open
@github-actions github-actions Bot mentioned this pull request May 2, 2026
Open
@jesse-merhi jesse-merhi added security Security documentation and removed security Security documentation labels May 25, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels May 25, 2026
@clawsweeper

clawsweeper Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress.

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.
What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@openclaw-barnacle

openclaw-barnacle Bot commented Jun 14, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jun 14, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 17, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Jun 18, 2026
This was referenced Jul 20, 2026
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

clawsweeper Tracked by ClawSweeper automation docs Improvements or additions to documentation gateway Gateway runtime merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentkoc @jesse-merhi