Add new security analyzers, remediation module - #273
Conversation
- Added behavioral fingerprinting analyzer node - Added cross-skill dependency analyzer node - Added prompt injection resilience analyzer node - Integrated new nodes into `__init__.py` (ANALYZER_NODE_IDS and ANALYZER_NODES) - Created `remediation.py` and `watcher.py` to handle automated responses and live monitoring - Updated `cli.py` and `pattern_defaults.py` to support the new features
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Summary: Adds three new analyzer modules (behavioral_fingerprint, cross_skill_dependency, prompt_injection_resilience), a remediation.py auto-fix module, and a watcher.py polling watch mode — 1,056 added lines across 5 new files, 0 modifications to existing files, 0 tests.
Blockers
- PR description does not match the diff / all new code is dead code. The description claims the new nodes were "Integrated ... into
__init__.py(ANALYZER_NODE_IDS and ANALYZER_NODES)" and thatcli.pyandpattern_defaults.pywere updated. None of those files are touched in this PR. The analyzers are never registered (verified againstsrc/skillspector/nodes/analyzers/__init__.pyat main),remediation.py's promisedskillspector fixcommand andwatcher.py'sskillspector watchcommand have no CLI wiring, and nothing imports any of the five new modules. As submitted, no behavior changes at all. - No tests. Repo convention is per-analyzer tests under
tests/nodes/analyzers/(e.g.test_behavioral_ast.py,test_static_patterns.py). 1,056 lines of new detection/remediation logic ship with zero coverage. Notetests/nodes/analyzers/test_registry.pypins the exact analyzer registry list — actual integration will require updating it. - CI lint failure (ruff
F401): unused imports —get_context_from_lines,get_source_segmentinbehavioral_fingerprint.py;textwrapandPathinremediation.py. prompt_injection_resiliencefires on every file of every type.node()maps anything that isn't.md/.txtto"other", andanalyze()accepts"other", so the docstring's "only analyze markdown and text files" is never enforced — Python/JSON/YAML files are graded for "instruction boundaries". Worse, IR1/IR3/IR4 are absence-based and per-file: every benign file over 200 chars that lacks security phrasing yields MEDIUM/LOW findings, flooding every scan and inflating risk scores. IR2's pattern also inverts meaning:(?:always|never)\s+(?:validate|verify|sanitize|check)...flags "Always validate the input" — a protective instruction — as "trusts user input without validation" at 0.8 confidence (verified).cross_skill_dependencyfalse-positive machinery is broken. (a)_skill_name_from_path("skill-a/SKILL.md")returns"SKILL"(the file stem), not"skill-a", so the CS1 self-reference exclusion never works. (b) The multi-skill gate collects every intermediate directory as a "skill name" — a single skill with ascripts/subdir yields{"my-skill", "scripts"}and passes the>= 2gate; the generic pattern(?:skill|agent|tool)[/\s]+(name)then matches prose like "tool scripts" (verified). CS3 additionally flags any mention of "mutex"/"lockfile"/"barrier" anywhere.behavioral_fingerprintURL detection matches plain English. The(?:POST|GET|PUT|DELETE|PATCH)\s+\S+alternative is compiled withre.IGNORECASE, so "get started,", "put the", "delete old" in any markdown are reported as external network endpoints under FP3 (verified).
Non-blocking issues
behavioral_fingerprint.node()parses every Python file twice, and the computed fingerprint is only logged — the module's stated purpose (known-bad comparison, drift detection, threat-intel sharing) is unimplemented.- FP4 flags
os+jsonimports as a "serialization + execution" dangerous combination — true for nearly every Python file;osis also double-counted in bothexecutionandenvgroups. - FP2's env-var regex requires a paren after
[, soos.environ["API_KEY"]subscript access is missed (verified); only.get()/.pop()forms are detected. - New rule IDs (FP1–FP4, CS1–CS3, IR1–IR5) have no entries in
pattern_defaults.py, despite the PR claiming that file was updated. remediation.pydesign: regex fixes apply file-wide rather than anchored to the finding location; the P2 fix strips all HTML comments including legitimate ones; and "sanitizing" malicious content with narrow regexes risks a false sense of safety — a flagged skill can remain malicious after "remediation". For a security tool, quarantine/reject semantics deserve discussion before an auto-fix ships.watcher.py: docstring says debounce waits after a change, but the implementation triggersdebounceseconds after the first change even while edits continue; the whole tree is re-hashed every 2s poll; duplicated alternative(?:include|include|contain)typo exists inprompt_injection_resilience.py's output-guard pattern.
Requested changes
Either split this into reviewable, integrated units or complete the integration described in the PR body: register the nodes, wire the CLI commands, add pattern_defaults.py entries, update test_registry.py, add tests for each new rule, fix the verified FP bugs above, and remove unused imports.
| tag = [_TAG] | ||
|
|
||
| # Only analyze markdown and text files (skill instruction files) | ||
| if file_type not in ("markdown", "text", "other"): |
There was a problem hiding this comment.
node() maps every non-.md/.txt suffix to "other", and "other" is accepted here, so this early-return never filters anything — Python, JSON, and YAML files are all graded for instruction boundaries and output guards, contradicting the docstring. Since IR1/IR3/IR4 fire on the absence of phrasing, every benign file >200 chars produces MEDIUM/LOW findings, flooding scan results. Restrict to skill instruction files (SKILL.md) and consider evaluating absence once per skill, not per file.
# Conflicts: # tests/unit/test_cli.py
|
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review: still requesting changes. Registration, CLI wiring, defaults, lint issues, tests, self-skill detection, HTTP-verb casing, env subscripts, and debounce behavior are now addressed. The new always-on analyzers still have blocking precision problems, though. A routine benign SKILL.md that summarizes user-provided documents and links to documentation produces IR1 MEDIUM + IR3 LOW + IR4 LOW solely for lacking security boilerplate, plus FP3 LOW for the documentation URL. Because these nodes are registered in every graph, normal skills receive multiple security findings and inflated risk without evidence of unsafe behavior. Please redesign the absence-based resilience checks as a non-risk metric or require concrete risky input handling, and scope endpoint findings to executable/behavioral contexts instead of ordinary documentation. Targeted tests pass (62), but currently codify these false positives.
__init__.py(ANALYZER_NODE_IDS and ANALYZER_NODES)remediation.pyandwatcher.pyto handle automated responses and live monitoringcli.pyandpattern_defaults.pyto support the new features