Static-pattern false positives found scanning public skills (P2 BOM, RA2 plist, EA3 OFL, plus design-level items)

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 BF before <?xml in ECMA-376 XSD files) is reported as Hidden Instructions. Mid-file U+FEFF and other zero-width characters are still flagged.
A2. RA2 matches plist inside identifiers. (?:defaults\s+write|plist|launchctl\s+load) with re.IGNORECASE matches relationshipList (JS) and CT_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.txt is reported as Scope Creep. The fix(analyzer): filter license boilerplate from EA3 static findings (#312) #328 filter only knows Apache/MIT/BSD ranges and LICENSE/COPYING/NOTICE basenames.

B. Not changed. Policy or design questions

B1. P2 HTML comments. Visible layout comments (, a backticked  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:latest

Suggestion: 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 it

and 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.

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…

B7 is on rules I added (PE5 #214, TM4 #220). Reproduced on v2.12.0 / main @ 8831219 with three synthetic skills, scan --no-llm:

skill	PE5 tags	score
`references/*.md`, indented blocks	none	56 HIGH DO_NOT_INSTALL
`references/*.md`, fenced blocks + "for example"	`contextual-triage`, `likely-benign-context`	56 HIGH DO_NOT_INSTALL
same content as a `SKILL.md` instruction	none	56 HIGH DO_NOT_INSTALL

Findings 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_score in nodes/report.py computes base_points * weight * confidence and does not read tags; contextual-triage is consumed only by _deduplicate_view_findings in static_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.py delegates example filtering to the runner, and the runner drops non-executable documents, but references/*.md is part of the skill and is not dropped. That delegation was mine in #237.

One detail narrower than the report: _is_documentation_example requires file_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.md and scripts as instruction and references/ as description, reusing the SKILL.md-as-instruction-file distinction already in static_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.

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:

PR #682 — open, non-draft; GitHub review decision: changes requested; latest reported check rollup: success.

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

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.

Implementation-link update checked on 2026-10-08: the remaining work now has additional open PRs: #782 for identifying Docker's actual image operand after options, and #783 for umask-masked modes and permissive umask values.

Keeping this broader report open. Neither proposal has merged. Merged #645 resolved A1–A3, and merged #682 provides reference-material triage tags without changing severity/confidence/scoring. The other B-series requirements remain outstanding; the new ignore-file PE3 fix in #787 is not a general resolution of them.

Navigation Menu

Repository navigation

Static-pattern false positives found scanning public skills (P2 BOM, RA2 plist, EA3 OFL, plus design-level items) #644

Summary

A. Fixed in the linked PR

B. Not changed. Policy or design questions

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Static-pattern false positives found scanning public skills (P2 BOM, RA2 plist, EA3 OFL, plus design-level items) #644

Description

Summary

A. Fixed in the linked PR

B. Not changed. Policy or design questions

Activity

rng1995 commented on Sep 28, 2026

CharmingGroot commented on Sep 29, 2026

rng1995 commented on Oct 4, 2026

rng1995 commented on Oct 6, 2026

rng1995 commented on Oct 8, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions