fix(security): bound skill-file audit read (restore removed fs dependency)#110589
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: 889a003f7e
ℹ️ 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".
| fs.readFile(params.skillFilePath, "utf-8"), | ||
| readRegularFile({ | ||
| filePath: params.skillFilePath, | ||
| maxBytes: MAX_SKILL_AUDIT_FILE_BYTES, |
There was a problem hiding this comment.
When skills.limits.maxSkillFileBytes is raised above the default, loadWorkspaceSkillEntries still loads those larger SKILL.md files, but the audit now always reads them with this fixed 256 KB cap. In that configuration a loadable >256 KB skill makes readRegularFile reject, the catch in collectInstalledSkillsCodeSafetyFindings emits only skills.code_safety.scan_failed, and scanSkillContent/scanSource never report critical SKILL.md findings such as pipe-to-shell instructions. Please use the same effective maxSkillFileBytes limit that was used to load the skills instead of hard-coding the default here.
Useful? React with 👍 / 👎.
889a003 to
d4733b0
Compare
…emoved fs dependency)
clawsweeper
Bot
added
rating: 🧂 unranked krab
d4733b0 to
847f5a1
Compare
clawsweeper
Bot
added
P1
|
Codex review: needs real behavior proof before merge. Reviewed July 18, 2026, 5:39 AM ET / 09:39 UTC. Summary PR surface: Source +6. Total +6 across 1 file. Reproducibility: yes. from source: configure 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:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Derive the audit read limit from the same canonical resolved skill-file limit used by the workspace loader, add coverage for both the default and an overridden limit, then show a real Do we have a high-confidence way to reproduce the issue? Yes, from source: configure Is this the best way to solve the issue? No. Bounded reads are the right direction, but the cap must follow the same effective configured limit that admitted the skill rather than silently overriding that contract in the audit path. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f14947d256d8. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +6. Total +6 across 1 file. View PR surface stats
Security concerns:
Acceptance criteria:
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
|
|
Merged via squash.
|
…emoved fs dependency) (openclaw#110589)
* fix(plugin-sdk): reject failed raw channel sends * fix(plugin-sdk): preserve raw result helper compatibility * test(plugin-sdk): cover raw send fallback errors Co-authored-by: 杨浩宇0668001029 <[email protected]> * fix(security): bound skill-file audit read via safe reader (restore removed fs dependency) (#110589) --------- Co-authored-by: Peter Steinberger <[email protected]>
…emoved fs dependency) (openclaw#110589)
…9906) * fix(plugin-sdk): reject failed raw channel sends * fix(plugin-sdk): preserve raw result helper compatibility * test(plugin-sdk): cover raw send fallback errors Co-authored-by: 杨浩宇0668001029 <[email protected]> * fix(security): bound skill-file audit read via safe reader (restore removed fs dependency) (openclaw#110589) --------- Co-authored-by: Peter Steinberger <[email protected]>
1 participant
What Problem This Solves
mainis broken:src/security/audit-extra.async.tscallsfs.readFile(params.skillFilePath, ...)at line ~196 but has nofsimport, so typecheck fails (Cannot find name 'fs') and blocks CI.Why This Change Was Made
The "bound file reads in audit" series (#101773, #101775) converted the plugin-manifest read to the safe
readRegularFilehelper and removed the now-unusedimport fs from "node:fs/promises"— but missed the SKILL.md read added by #109363, which still usedfs.readFile. That left an orphaned reference and a broken build.User Impact
Restores a green build. Consistent with the series intent, the skill-file audit read is now bounded via
readRegularFile(cap256_000bytes, matching the workspace loader'sDEFAULT_MAX_SKILL_FILE_BYTES) instead of an unboundedfs.readFile, so an oversized attacker-controlledSKILL.mdcannot force an unbounded read during the code-safety scan.Evidence
MAX_SKILL_AUDIT_FILE_BYTES = 256_000and converts the one orphanedfs.readFiletoreadRegularFile({...}).then(({buffer}) => buffer.toString("utf-8")), mirroring the existing manifest-read pattern at line ~114. No remainingfs.method()calls; nofsimport needed.import fs); this restores correctness after the import was removed.