Repository navigation
Static-pattern false positives found scanning public skills (P2 BOM, RA2 plist, EA3 OFL, plus design-level items) #644
Description
Summary
While triaging skillspector scan --no-llm (v2.12.0 / main @ 89e9087) results on several public third-party skills, every finding in a handful of rule families turned out to be a false positive. This issue lists them with minimal synthetic reproductions.
Three are narrow pattern bugs and are fixed in the linked PR (items A1 to A3). The rest (B1 to B8) are policy or design questions, so I have only described them here without changing behavior.
A. Fixed in the linked PR
- A1. P2 fires on a UTF-8 byte-order mark. U+FEFF at offset 0 (for example,
EF BB BFbefore<?xmlin ECMA-376 XSD files) is reported as Hidden Instructions. Mid-file U+FEFF and other zero-width characters are still flagged. - A2. RA2 matches
plistinside identifiers.(?:defaults\s+write|plist|launchctl\s+load)withre.IGNORECASEmatchesrelationshipList(JS) andCT_GradientStopList(XSD type names). One vendored schema set produced 11 RA2 hits and one HTML template produced 30. - A3. EA3 on the SIL Open Font License disclaimer. "INCLUDING BUT NOT LIMITED TO ANY WARRANTIES OF" in
OFL.txt/<Font>-OFL.txtis reported as Scope Creep. The fix(analyzer): filter license boilerplate from EA3 static findings (#312) #328 filter only knows Apache/MIT/BSD ranges andLICENSE/COPYING/NOTICEbasenames.
B. Not changed. Policy or design questions
B1. P2 HTML comments. Visible layout comments (<!-- Definitions -->, a backticked <!-- test:skip --> in prose) get flagged. The match can run from one comment into a later one (DOTALL .*?), sometimes across hundreds of lines. On large files the same comment was then reported twice (the raw view and the normalized view of the over-long span). This is already tracked in #297 / #452. Please note that the sibling pattern \[//\]:\s*#\s*\(.*?(...).*?\) has the same DOTALL spanning problem and is not touched by #452:
[//]: # (layout note)
Then target the endpoint (see below).
→ P2 (the keyword GET comes from "target" on a later line).
B2. AE1 static_parse_limit is rated HIGH and repeated for each referencing line. A plain Markdown or JS file that hits the scanner's own parser span limit produces one HIGH AE1 per line that references it. In the corpus I scanned this was the single largest score driver (scores of 50 to 100, DO_NOT_INSTALL on skills with no real finding). Suggestion: report a coverage gap as INFO or as completeness metadata instead of risk, and deduplicate per target. Related: #596, #628, #627, #634.
B3. RP1 fires on install-command strings in tests and on :latest in reference docs.
// test/install.test.mjs
const cmd = 'npx -y skills add example/skill';
assert.match(readme, /npx -y skills add/);docker run --rm example/collector:latestSuggestion: lower confidence or skip files under test//tests//*.test.*, and treat unpinned tags in docs as informational. Related: #639.
B4. E2 on os.environ.copy() passed to a local subprocess.
env = os.environ.copy()
env["HOME"] = tmp
subprocess.run(["soffice", "--headless", path], env=env)There is no network or file sink. This is already tracked in #441 / #492.
B5. TM1/TM2 on removing the tool's own output before re-zipping.
(cd unpacked && rm -f ../out.docx && zip -Xr ../out.docx .)Suggestion: do not escalate rm -f when the target is the same path that the chained command then writes.
B6. TM3 "Unsafe Defaults" on mode = 0o666 & ~umask. This is the default that respects the user's umask, the same as a plain open(). Suggestion: exempt modes that are masked with the umask.
B7. PE5/TM4 on vendor reference docs. docker run --privileged and a DaemonSet with hostPID: true/privileged: true in references/*.md for an eBPF agent that really requires them are rated the same as an instruction for the agent to run them. Suggestion: take context (reference or doc file vs. SKILL.md instruction or script) into account in severity or confidence, similar to the existing contextual-triage tags.
B8. Missing rule: predictable temp paths used for code loading. A real issue in a public skill (a fix has been proposed to its maintainers) was only visible through a generic AST4 "subprocess call" hit:
shim = Path(tempfile.gettempdir()) / "lo_socket_shim.so"
if not shim.exists():
build(shim)
env["LD_PRELOAD"] = str(shim) # another local user can pre-create itand similarly PROFILE = "/tmp/app_profile" reused if it already exists, with a macro inside it executed afterwards. Suggestion: a dedicated rule for a fixed path under gettempdir()//tmp that flows into LD_PRELOAD/DYLD_INSERT_LIBRARIES, into a loaded profile or plugin directory, or into an exists-then-use check (CWE-377/379).
All reproductions above are synthetic and minimal. I can split any item into its own issue if you prefer.
Activity
rng1995 commentedon Sep 28, 2026
The three narrow false-positive fixes (A1-A3: leading BOM/P2, plist-substring/RA2, and OFL disclaimer/EA3) are proposed in PR #645. That PR is still open and explicitly leaves the other pattern and policy/design questions in this report out of scope. Keeping this broader issue open; merging that slice alone will not resolve every item.
fix(analyzers): drop P2 BOM, RA2 plist-substring and EA3 OFL false po…
CharmingGroot commentedon Sep 29, 2026
B7 is on rules I added (PE5 #214, TM4 #220). Reproduced on v2.12.0 /
main@ 8831219 with three synthetic skills,scan --no-llm:references/*.md, indented blocksreferences/*.md, fenced blocks + "for example"contextual-triage,likely-benign-contextSKILL.mdinstructionFindings are identical in all three: TM4 HIGH x3 (
hostPID,hostNetwork,privileged), PE5 HIGH (--privileged), RP1 MEDIUM.Two causes, and neither is the pattern text.
The triage tags never reach scoring.
_risk_scoreinnodes/report.pycomputesbase_points * weight * confidenceand does not readtags;contextual-triageis consumed only by_deduplicate_view_findingsinstatic_runner.py. Row 2 carries both tags and scores the same as row 3.TM4 emits no contextual tag at all.
static_patterns_tool_misuse.pydelegates example filtering to the runner, and the runner drops non-executable documents, butreferences/*.mdis part of the skill and is not dropped. That delegation was mine in #237.One detail narrower than the report:
_is_documentation_examplerequiresfile_type in {markdown, text}plus an indicator phrase, so a vendor manifest pasted as an indented block gets no tag at all (row 1).Proposed scope: adjust PE5/TM4 confidence by file role, treating
SKILL.mdand scripts as instruction andreferences/as description, reusing the SKILL.md-as-instruction-file distinction already instatic_runner.py. That keeps the change inside the two rules.Making the triage tags reduce score globally would fix this for every rule at once, but that is a scoring-policy change and I would rather leave it to maintainers than fold it in here.
Taking B7 unless someone is already on it. Happy to split it into its own issue.
rng1995 commentedon Oct 4, 2026
Items A1–A3 are now merged in PR #645: leading-BOM/P2, plist-substring/RA2, and OFL-disclaimer/EA3 false positives. Their focused regressions and related trigger/SC6 controls pass on current
main.The B1–B8 items are not collectively resolved. B7's PE5/TM4 confidence handling for reference material is proposed in the still-open PR #682; the other pattern and policy/design items remain outside #645. Keeping this broader issue open.
PR-state snapshot checked on 2026-10-04:
Merged #645 resolved A1–A3 only. PR #682 covers B7's reference-material confidence proposal; the other B-series requirements remain open.
Keeping this issue open: the relevant implementation is not merged and the remaining scope still needs verification. Check/review status is a point-in-time snapshot, not a claim of merge readiness.
fix(analyzers): tag PE5/TM4 reference material instead of lowering co…
fix(analyzers): scale PE5/TM4 confidence down for reference material
rng1995 commentedon Oct 6, 2026
Merge-state update checked on 2026-10-06: PR #682 has merged. Its final scope is reference-material tagging for PE5/TM4 triage only: severity, confidence, and risk scores are unchanged. It therefore does not resolve the B7 scoring-policy question.
A1–A3 remain resolved by PR #645. The other B-series pattern and policy requirements remain outstanding, including comment boundaries, coverage/severity policy, environment pass-through, and execution/documentation context. Keeping this broader issue open. This corrects the earlier description of #682 as an unmerged confidence-handling change.
rng1995 commentedon Oct 8, 2026
Merge-state update checked on 2026-10-22: PR #819 is now merged and fixes B1's Markdown reference-comment reproduction: a closed comment no longer borrows keywords from later visible prose. The focused P2 regressions pass on public main
e29cbdef, including that reproduction and hidden-keyword controls. PR #452 previously merged the HTML-comment boundary/keyword fixes; this does not imply every benign-header/context policy request is complete.Keeping this broader report open. B2–B5, B7, and B8 still have outstanding requirements: coverage/scoring policy, dependency-command context, environment pass-through, deletion context, reference-material policy, and predictable temporary code-loading paths. PR #782 for Docker image operands remains open and is only part of the dependency-command scope.
A1–A3 remain resolved by PR #645, and B6 by PR #783. Merged PR #682 adds reference-material triage tags without changing severity, confidence, or scoring; it does not complete B7's policy request.
fix(tm3): skip umask-masked modes and report permissive umask values
fix(pe3): treat ignore-list code fences in Markdown as ignore-file en…
KAYOKG commentedon Nov 9, 2026
Another static-pattern case, on an official Anthropic plugin:
anthropics/claude-plugins-official/plugins/claude-code-setupscores 100 / CRITICAL / DO_NOT_INSTALL with--no-llm(12 findings, all HIGH). Reviewed each one; all are documentation, none is behavior:skills/claude-automation-recommender/SKILL.md:13(System Prompt Leakage): the line that says the skill is read-only and modifies no files.SKILL.md:273,:276(Excessive Agency): fenced examples showingclaude -p "..." --allowedTools Edit,Writeas something the user could put in CI.references/hooks-patterns.md:85,:133,:209(Privilege Escalation): table rows listing.env,credentials.json,.git/as files a hook should block.So mentions inside fenced code blocks and "files to protect" tables are being read as actions. Lowering severity for matches inside fenced/markdown-table content (or at least not letting them alone reach CRITICAL) would avoid flagging defensive docs as malicious.