Fix content_scanner.py: only require SKILL.md frontmatter on SKILL.md itself - #47
Merged
Merged
Conversation
… itself
Found while actually running content_scanner.py against all 145 SKILL.md
files of an external skill repo (uphiago/recon-skills), per the "don't
trust unreviewed external content" gap flagged earlier -- scan_skill_file()
required YAML frontmatter on every *.md file it was pointed at, not just
files literally named SKILL.md. That repo's references/*.md and top-level
docs legitimately have none (only SKILL.md is the frontmatter contract,
per its own AGENTS.md convention, matching this repo's own
.claude/skills/<name>/{SKILL.md, references/} layout). Produced 27 false
positives out of 33 findings on the real scan -- prompt-injection-pattern
and hidden-Unicode checks still apply to non-SKILL.md files; only the
frontmatter-specific checks are now scoped to SKILL.md.
Full external-repo scan result after the fix: 5 HIGH + 1 MEDIUM, all
manually verified as legitimate (skills that hunt FOR prompt injection
necessarily document injection phrasing as example payloads -- same
pattern as this repo's own emerging-surfaces false positive; the one
base64 blob is a synthetic ASP.NET ViewState example). No genuine
malicious content found.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Directly implements the review I flagged earlier ("Unreviewed external content" —
content_scanner.pyexists exactly for this, so 150+ external skill files shouldn't be trusted without actually scanning them). Cloneduphiago/recon-skillsand rancontent_scanner.pyagainst its full 145-SKILL.md tree.Found a real bug in the scanner itself:
scan_skill_file()required YAML frontmatter on every.mdfile it was pointed at, not just files literally namedSKILL.md. That produced 27 false positives (allreferences/*.mdand top-level docs likeREADME.md/AGENTS.md, which legitimately have no frontmatter — onlySKILL.mdis the frontmatter contract, matching this repo's own.claude/skills/<name>/{SKILL.md, references/}layout).Fixed by scoping the frontmatter-specific checks to
os.path.basename(path) == "SKILL.md"— the content-safety checks (prompt-injection phrasing, hidden Unicode, base64 blobs) still apply to every.mdfile regardless of filename.Scan result after the fix: 5 HIGH + 1 MEDIUM, all manually verified as legitimate — every "prompt-injection" hit is inside a skill that specifically hunts for prompt injection (
hunt-llm-ai,llm-prompt-injection,hunt-mcp-security, etc.), so it necessarily documents the injection phrasing as an example test payload — the exact same pattern as this repo's own knownemerging-surfacesfalse positive. The one base64 blob is a synthetic ASP.NET__VIEWSTATEexample. No genuine malicious content found.Test plan
references/*.mdwithout frontmatter is no longer flagged; the same file still gets flagged for genuine content issues (prompt-injection phrasing)content_scannertests still pass (all useSKILL.mdas the filename, so the fix doesn't change their behavior)