fix(lp3): remediation and docs name allowed-tools for SKILL.md - #316
fix(lp3): remediation and docs name allowed-tools for SKILL.md#316ppcvote wants to merge 1 commit into
Conversation
LP3's detection logic accepts `allowed-tools` as a valid tool-scope
declaration, but the runtime remediation string, the pattern-defaults
fallback, and docs/B.3.1 still directed authors to add a `permissions`
field. For Claude Code / Agent Skills SKILL.md, `permissions` is not part
of the frontmatter schema and is ignored as unknown, so following the
advice could never resolve the finding.
Aligned the three user-facing strings with the code:
- LP3 remediation (analyzer + pattern_defaults): declare `allowed-tools`
in SKILL.md frontmatter, or a `permissions` list in MCP server
manifests, stating which applies to which manifest type.
- LP3 finding message + description: "declares no tool scope
('permissions' or 'allowed-tools')" instead of "no declared
permissions", matching the actual trigger condition.
- docs/B.3.1 LP3 section: Triggers when / Example / Remediation updated
the same way.
No behavior change; detection logic untouched. tests/test_mcp_least_privilege.py 15/15.
Closes NVIDIA#313
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ppcvote <risky9763@gmail.com>
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Approved. This aligns LP3's finding text, remediation, defaults, and documentation with the detector's existing allowed-tools support. It correctly directs SKILL.md authors to allowed-tools while retaining permissions for MCP manifests, with no detection or schema behavior change. The focused LP suite passes (15 tests).
|
Checked this against current Flagging only because it is easy for an approved PR to fall off the list: this closes #313, and the failure it fixes is that the remediation string tells an author to add a Happy to rebase, split, or adjust anything if it would help. |
Closes #313.
LP3's detection logic on
mainalready acceptsallowed-toolsas a valid declaration (mcp_least_privilege.py—permissions_absent = (permissions is None or permissions == []) and not allowed_tools), but the guidance layer was left behind: the runtime remediation string, thepattern_defaults.pyfallback strings, and thedocs/B.3.1LP3 section all direct authors to add apermissionsfield. As #313 lays out,permissionsis not part of the Claude Code / Agent Skills SKILL.md frontmatter schema — an author who follows the advice adds a key the runtime ignores, and LP3 keeps firing. The finding wasn't actionable by its own remediation.What changed (strings only — no behavior change)
mcp_least_privilege.pyand thepattern_defaults.pyfallback — they'd otherwise drift): names both fields and says which applies to which manifest type:allowed-toolsfrontmatter for Claude Code / Agent Skills SKILL.md,permissionslist for MCP server manifests.'permissions'or'allowed-tools')" instead of "no declared permissions", matching the trigger condition the code actually implements.docs/B.3.1-mcp-least-privilege.mdLP3 section: Triggers when / Example / Remediation updated the same way; the docs note explicitly thatpermissionsis ignored in SKILL.md so nobody round-trips through the dead key again.Scope note: LP1/LP4 doc wording could arguably get the same manifest-type clarification, but #313 is specifically about LP3's dead advice, so this PR stays there — happy to follow up if wanted.
Testing
tests/test_mcp_least_privilege.py: 15/15 pass (no test pins the remediation strings).maincheckout on Windows (test_create_github_release×2,test_build_context_records_non_regular_entries_in_the_ledger, one YARA encoding case) — unrelated to this change, and consistent with the Windows-environment gaps described in claude_cli provider: concurrent temp-file collision on Windows drops most files from LLM analysis ([WinError 32]) #315.🤖 Generated with Claude Code