fix(security): scan SKILL.md text in skill code-safety audit#109363
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7825336023
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const summary = await getSkillCodeSafetySummary({ | ||
| dirPath: skillDir, | ||
| skillFilePath: entry.skill.filePath, | ||
| summaryCache: params.summaryCache, |
There was a problem hiding this comment.
When a plugin declares skills, loadWorkspaceSkillEntries can expose SKILL.md files rooted under the installed plugin directory, but this collector skips those directories earlier because they are under stateDir/extensions. This new SKILL.md scan is therefore only reached for non-plugin skills; the plugin path still goes through collectPluginsCodeSafetyFindings → scanDirectoryWithSummary, whose scannable extensions exclude .md, so a malicious extensions/<plugin>/skills/.../SKILL.md with the same pipe-to-shell or prompt-injection text remains invisible to security audit --deep.
Useful? React with 👍 / 👎.
801c60c to
12ac206
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 12:33 AM ET / 04:33 UTC. Summary PR surface: Source +30, Tests +49. Total +79 across 2 files. Reproducibility: yes. from source inspection: workspace skills reach the new helper, while plugin-rooted skills are skipped and remain under a directory scanner that excludes Markdown. A live CLI transcript has not been supplied. Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Next step before merge
Security Review findings
Review detailsBest possible solution: Use one skill-Markdown audit path for every discovered non-bundled skill, including skills rooted under installed plugins, with dangerous and clean regression cases for both workspace and plugin sources. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection: workspace skills reach the new helper, while plugin-rooted skills are skipped and remain under a directory scanner that excludes Markdown. A live CLI transcript has not been supplied. Is this the best way to solve the issue? No, not yet. Reusing the existing scanners is appropriate, but the narrowest complete fix must also cover the sibling plugin-provided skill path and prove that path with a regression test. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e3618454f245. Label changesLabel justifications:
Evidence reviewedPR surface: Source +30, Tests +49. Total +79 across 2 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (2 earlier review cycles) |
12ac206 to
5f8dbd6
Compare
|
/allow-security-sensitive-change 5f8dbd6 |
|
/allow-dependencies-change 5f8dbd6 |
`security audit --deep` fed only JS/TS files (SCANNABLE_EXTENSIONS) to the skill scanner, so a malicious workspace SKILL.md (pipe-to-shell, prompt-injection, secret-exfiltration) produced zero findings while the same patterns in a sibling tool.js were flagged critical. The scanner already has skill-text rules and the Workshop path scans PROPOSAL.md markdown — the installed/workspace-skill audit just skipped the markdown. Scan each audited skill's SKILL.md content through scanSkillContent + scanSource (the same scanners the Workshop uses), scoped to the skills audit path so the global directory scanner is unchanged (no README/doc false positives).
5f8dbd6 to
badb34b
Compare
|
/allow-security-sensitive-change 5f8dbd6 |
|
/allow-dependencies-change 5f8dbd6 |
|
Merged via squash.
|
…w#109363) `security audit --deep` fed only JS/TS files (SCANNABLE_EXTENSIONS) to the skill scanner, so a malicious workspace SKILL.md (pipe-to-shell, prompt-injection, secret-exfiltration) produced zero findings while the same patterns in a sibling tool.js were flagged critical. The scanner already has skill-text rules and the Workshop path scans PROPOSAL.md markdown — the installed/workspace-skill audit just skipped the markdown. Scan each audited skill's SKILL.md content through scanSkillContent + scanSource (the same scanners the Workshop uses), scoped to the skills audit path so the global directory scanner is unchanged (no README/doc false positives).
#101773 removed the node:fs/promises import while switching plugin manifest reads to fs-safe helpers, but getSkillCodeSafetySummary (added later by #109363) still reads SKILL.md via fs.readFile. Main has been red since the merge (check-prod-types, check-lint, check-test-types, package-boundary compile, compact-large-5). Restore the import; skill-file reads keep their pre-existing unbounded contract.
openclaw#101773 removed the node:fs/promises import while switching plugin manifest reads to fs-safe helpers, but getSkillCodeSafetySummary (added later by openclaw#109363) still reads SKILL.md via fs.readFile. Main has been red since the merge (check-prod-types, check-lint, check-test-types, package-boundary compile, compact-large-5). Restore the import; skill-file reads keep their pre-existing unbounded contract.
1 participant
What Problem This Solves
openclaw security audit --deepnever scanned a skill's SKILL.md. A malicious workspace SKILL.md containing pipe-to-shell install patterns (curl … | bash),rm -rf ~/, secret-exfiltration, or prompt-injection produced zero audit findings — while the identical patterns placed in a siblingtool.jswere immediately flaggedskills.code_safety critical. Since SKILL.md is the primary (often only) file a skill ships, and manually-authored workspace skills are never scanned at install time, malicious skill text was invisible to the audit.Why This Change Was Made
The scanner already has rules written for skill text (
shell-pipe-to-shell= "Skill text includes pipe-to-shell install pattern",secret-exfiltration,prompt-injection-*), and the Workshop proposal path scans PROPOSAL.md markdown. But the installed/workspace-skill audit (collectInstalledSkillsCodeSafetyFindings) only fedSCANNABLE_EXTENSIONS(JS/TS) to the scanner, skipping the markdown.Fix: each audited skill's SKILL.md content is now scanned through
scanSkillContent+scanSource(the same scanners the Workshop uses), merged into the code-safety summary. Scoped to the skills audit path — the global directory scanner's extension set is unchanged, so READMEs/docs elsewhere are not newly scanned (no false-positive flood).User Impact
security audit --deepnow flags dangerous instructions in skill SKILL.md files (pipe-to-shell, prompt-injection, exfiltration), closing a security blind spot for manually-authored/workspace skills. No change to non-skill scanning.Evidence
audit-extra.async.test.ts"scans SKILL.md text for dangerous skill instructions": red before ("expected skill markdown code-safety finding"), green after. 7 tests pass.node scripts/check-changed.mjsgreen on Testbox;oxfmt --check,git diff --checkclean.