fix(analyzer): gate documentation false positives for PE3/RA1/TM1/AR2#266
Conversation
…NVIDIA#251) Signed-off-by: Rod Boev <[email protected]>
Signed-off-by: Rod Boev <[email protected]>
Signed-off-by: Rod Boev <[email protected]>
Signed-off-by: Rod Boev <[email protected]>
) Signed-off-by: Rod Boev <[email protected]>
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Summary
Adds a shared _is_documentation_context gate in static_runner.py that suppresses findings for four lexical rule families (PE3, RA1, TM1, AR2) when the matched line is documentation prose (in .md/.txt) or a #/// comment line, without execution signals. This directly implements the direction requested in #251 (mass FP flagging of benign security documentation in --no-llm mode). The gate is well scoped: only the four rule_ids are affected, SKILL.md is preserved, executable calls (shutil.rmtree(...), os.environ, subprocess, etc.) stay active even inside .md/.txt, inline /* ... */ block comments do not mask executable content, and JSON/YAML/TOML config data lines remain analyzable. It runs after the existing is_code_example suppression, so fenced examples are still handled upstream. Tests are extended with suppression fixtures for all four families plus negative-space fixtures proving executable/config/SKILL.md content still emits; the two docs-subdir tests are updated from confidence-downweight to full-filter assertions consistent with the new behavior. No schema/SARIF/output-contract changes.
Approving. The design is scoped and errs toward keeping findings; the notes below are non-blocking.
Non-blocking observations
_EXECUTION_SIGNALover-matches common markdown, leaving intended FPs. The[|>]alternation matches markdown table pipes (|) and blockquote markers (>), and\b(?:subprocess|eval|exec)\bmatches those words in ordinary English prose (e.g. #251's own cited "never eval'd" wording). Any governed prose line inside a table, a blockquote, or a sentence mentioning eval/exec/subprocess is therefore treated as an execution context and still fires. This only ever reduces suppression (safe direction, no bypass), but it means the PR's stated FP fix silently misses these very common documentation constructs. Consider tightening the signal to require a shell-command/assignment shape rather than bare tokens.- Suppression now spans all
.md/.txt, not just documentation directories. Previously non-SKILL.mdmarkdown got a confidence downweight; it is now fully dropped for these four rules whenever the line lacks an execution signal.SKILL.mdis preserved and the semantic-LLM analyzers remain excluded from the gate, so full-pipeline (LLM) runs still cover these files. In static-only (--no-llm) mode, prose-based PE3/RA1/TM1/AR2 in an on-demand-loaded, non-SKILL.mdmarkdown/text file now yields no finding at all. This matches the maintainer-facing request in #251 and is a defensible tradeoff, but worth flagging as a known static-mode detection-gap for these lexical rules. content.splitlines()vsget_line_number'scount("\n"). The matched-line lookup splits on all Unicode line boundaries whilestart_lineis computed from\ncounts; a lone\ror form-feed in content could misalign the selected line. Pre-existing inconsistency shared withget_context, low risk, minor.
|
|
||
| _SEMANTIC_STRING_DOC_PRONE_RULES = frozenset({"PE3", "RA1", "TM1", "AR2"}) | ||
| _EXECUTION_SIGNAL = re.compile( | ||
| r"(?:\b\w+\s*=|\bos\.(?:environ|getenv|system)\b|\bshutil\.rmtree\b|\b(?:subprocess|eval|exec)\b|[|>]" |
There was a problem hiding this comment.
The [|>] class matches markdown table pipes and > blockquote markers, and \b(?:subprocess|eval|exec)\b matches those words in plain English prose (e.g. #251's cited "never eval'd"). Governed prose inside a markdown table, blockquote, or a sentence mentioning eval/exec/subprocess is thus treated as an execution context and still fires. This is safe-direction (only reduces suppression, no bypass), but it leaves exactly the doc constructs this PR aims to gate. Consider requiring an assignment/shell-command shape instead of bare tokens.
Summary
Benign security documentation trips several high-confidence static rules at once. Credential-hygiene docs are flagged as PE3 (Credential Access), shell-script security comments as RA1 (Self-Modification), an uninstaller's documentation as TM1 (Tool Parameter Abuse), and tone-guidance prose as AR2 (Anti-Refusal Statement). This is one recurring mechanism: a semantic-string rule matching English/comment vocabulary in a non-executable documentation context, surfacing across four rule families.
Closes #251
Root cause
The four implicated rules gate documentation context inconsistently, and the shared runner only partially covers it.
static_runner.pyalready carries a shared PE3-only.envdocumentation filter (_is_env_file_reference_in_docs) and adocs/-subdir confidence downweight (_is_documentation_markdown), but nothing suppresses a plain-prose or comment hit for the general semantic-string rules. PE3 has a narrow local_is_documentation_example, RA1 has no documentation gating, TM1 gates only container/dockerfile idioms, and AR2'sis_code_examplepenalty misses prose that isn't a fenced code example. The same benign vocabulary therefore lands as HIGH findings depending on the matching family.Diff Notes
static_runner.py, keyed on{PE3, RA1, TM1, AR2}. AST, behavioral, semantic-LLM, and all other rules remain excluded._is_documentation_context) suppresses a governed finding only from the matched finding line: markdown or text prose stays suppressible only when the line lacks executable signals,#and//comment lines stay suppressible, neighboring comments no longer suppress executable findings, inline/* ... */prefixes no longer mask executable content, and executable API calls such asshutil.rmtree(...)stay active even inside.mdand.txtfiles.SKILL.mdremains active, and JSON/YAML/TOML config files stay analyzable..envand example checks, TM1's container and Dockerfile idioms, and AR2's code-example penalty.tests/nodes/analyzers/test_static_runner_filtering.pywith suppression fixtures for all four families and negative-space fixtures proving real malicious content with the same vocabulary still emits.Before / After
docs/credentials.md# security note: this script rewrites itself.shdocs/uninstall.mddocs/tone.mdtoken = os.environ["AWS_SECRET_ACCESS_KEY"].pyrun: rm -rf /opt/example/dataconfig.yamlopen(__file__, "w").py/.shSKILL.mdScope
No analyzer module changes — the PE/RA/TM/AR pattern lists and local heuristics are untouched. The gate is rule-scoped and context-gated: it never suppresses on rule id alone, never touches non-governed rules (SC*, EA*, MP*, behavioral/semantic), and never suppresses in SKILL.md or in any context with a code-execution signal. No severity-model, reporter-schema, or MCP change. Adjacent PR #254 works inside the anti-refusal analyzer on match semantics; this runner-level context filter does not overlap it.
Verification
uv sync --all-extras— completed successfully.uv run pytest tests/nodes/analyzers/test_static_runner_filtering.py tests/nodes/analyzers/test_static_patterns.py tests/nodes/analyzers/test_static_patterns_anti_refusal.py tests/nodes/test_meta_analyzer.py -q—116 passed, 6 xfailed in 0.86s.uv run ruff check src/ tests/—All checks passed!.uv run ruff format --check src/ tests/—144 files already formatted.