feat(analyzer): reduce false positive for emoji-zwj-sequence#260
feat(analyzer): reduce false positive for emoji-zwj-sequence#260badnoodle wants to merge 3 commits into
Conversation
Signed-off-by: Yangnian <[email protected]>
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Requesting changes. The emoji carve-out misses common valid ZWJ sequences containing skin-tone modifiers or VS16, and the PR adds no regressions. Handle the full emoji sequence around ZWJ, add representative positive/negative tests, declare regex as a direct dependency, and run formatting.
… unit test Signed-off-by: Yangnian <[email protected]>
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review of head d2dd3e6. The PR carves ZWJ (U+200D) out of the P2 hidden-instruction rule when it joins a valid emoji sequence (fixes #259). The new commit substantially addresses the sequence-handling feedback and adds good test coverage, but one prior blocker remains open and the new code introduces a crash on attacker-controllable input.
Prior-issue resolution checklist
| Prior issue | Status |
|---|---|
| Handle full emoji sequence around ZWJ (Emoji_Modifier, FE0F), e.g. 👩🏽💻, ❤️🔥 | Resolved — is_emoji_zwj_sequence now walks left past U+1F3FB–U+1F3FF and FE0E/FE0F to the \p{Extended_Pictographic} base; tests cover those exact emoji |
| Representative positive/negative tests incl. non-emoji ZWJ staying detectable | Resolved — added in tests/unit/test_patterns.py and tests/nodes/analyzers/test_static_patterns.py, incl. ignore\u200dall system\u200d\u200dprompt still yielding P2 |
Declare regex as a direct dependency |
Not resolved — it was added to the dev optional-dependency group in pyproject.toml, but common.py (runtime code imported by every analyzer) does import regex at module level. A plain pip install skillspector now fails at import time. Move it to [project] dependencies |
| Run formatting | Not resolved — leftover debug print(findings) in a test, test-name typo seuquence, inline-comment style, fullwidth colon in a comment, missing return-type annotations |
New blockers
- Crash on ZWJ near start of file (
src/skillspector/nodes/analyzers/common.py)._readreturns""for out-of-range indexes, but_check_modifier/_check_selectorcallord(cp)unconditionally, andord("")raisesTypeError. Reproduced: content starting with\u200d(ZWJ at index 0), or a modifier at index 0 followed by ZWJ, crashesis_emoji_zwj_sequence. Neitheranalyze()norstatic_runner.run_static_patterns()catches analyzer exceptions, so a skill file beginning with a ZWJ takes down the entire prompt-injection node — an attacker can prepend one character to suppress all P1–P4 scanning. Guard the empty-string case (e.g. returnFalsefrom the_check_*helpers whencpis empty) and add a test with ZWJ as the first character of the file. - Runtime
ImportErrorin production installs (same as unresolved prior issue above):regexmust move fromdevextras to[project] dependencies.
Non-blocking nits
_check_emoji_zwj_sequence:content[match.start() : match.end()]is identical tomatch.group(0); the condition can simply bematch.group(0) != "\u200d". Prefer conventional operand order over Yoda conditions, and add-> boolannotations (repo convention).tests/nodes/analyzers/test_static_patterns.py: remove debugprint(findings); fix test-name typotest_p2_emoji_zwj_seuquence_still_keep_detectable;zwj_with_modifieractually contains no skin-tone modifier (VS16 only) — rename or use a modifier-bearing sequence.- Consider precompiling the
\p{Extended_Pictographic}pattern at module level instead of per-characterregex.matchcalls.
Once the crash is fixed (with a boundary test) and regex is a runtime dependency, this looks mergeable.
Note for maintainers: PR #264 fixes the same issue #259 with a stdlib-only carve-out in the same hunk — the two PRs conflict and only one should land.
… zero index test; add crash problem; precompile pattern; move regex to project level; and all relative test passed. Signed-off-by: Yangnian <[email protected]>
Fixes #259 , check the valid Emoji ZWJ sequences to reduce false positive