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

[jwcastillo]

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 (<!-- 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: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.

[rng1995]

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.

[CharmingGroot]

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.