fix: reduce false positives and add global rule configuration#49
Open
satoridev01 wants to merge 1 commit intoParzivalHack:mainfrom
Open
fix: reduce false positives and add global rule configuration#49satoridev01 wants to merge 1 commit intoParzivalHack:mainfrom
satoridev01 wants to merge 1 commit intoParzivalHack:mainfrom
Conversation
Addresses a class of false positives identified through real-world scanning of a large Python codebase (Django 6.1-alpha). Introduces two new mechanisms for controlling rule behavior without modifying individual rule definitions. ## Changes ### New: `[defaults]` section in rules TOML A single top-level section that applies to every rule: ```toml [defaults] exclude_file_patterns = ["*tests*", "*fixtures*", "*testdata*", "*conftest*"] disabled_rule_ids = ["ISINSTANCE855", "LEN1101", "SUPER1128", ...] ``` - `exclude_file_patterns` — glob patterns excluded from ALL rules (replaces repeating `exclude_file_pattern` on each individual rule) - `disabled_rule_ids` — completely disable rules that produce noise without taint context (Python built-in calls, etc.) ### New: per-rule `exclude_pattern` field Regex applied to the matched line; if it matches, the finding is suppressed. Useful for rules that are correct in general but have well-known safe variants: ```toml [[rule]] id = "PY107" ast_match = "Call(func.value.id=yaml, func.attr=load)" exclude_pattern = "Loader\\s*=\\s*(yaml\\.)?(Safe|Base)Loader" ``` ### Rust core changes - `rules.rs`: adds `Defaults` struct with `exclude_file_patterns`, `disabled_rule_ids`; adds `Rule::is_file_excluded()` helper; adds `exclude_pattern: Option<Regex>` to `Rule` - `analysis/mod.rs`: applies `disabled_rule_ids` before scanning - `analysis/ast_analysis.rs`: uses `Rule::is_file_excluded()` and `exclude_pattern` - `analysis/config_analysis.rs`: same ### Rule fixes applied | Rule | Fix | |---|---| | PY107 / PY302 | `exclude_pattern` suppresses `yaml.load(..., Loader=SafeLoader)` | | PY515 / SHELL645 / SHELL670 | `exclude_pattern` suppresses `re.compile()` and `.compile()` method calls | | AUTH711 / ADMIN795 | `exclude_file_pattern` suppresses test fixtures | | CSRF747 / IMPORT825 / PATH813 / PY001 | `exclude_file_pattern` suppresses test files | | SESS744 | Pattern narrowed to `session.session_key = request.*` (actual fixation); writing data to a session is not fixation | | PY511 / JSON612 | Severity downgraded to Low; `json.loads()` alone is not a code execution risk | | 95 Low/Medium rules | Added to `disabled_rule_ids`; rules flagging every Python built-in call (`isinstance`, `super`, `len`, f-strings, etc.) have no security value without taint analysis | ### Test coverage `tests/unit/test_false_positive_reductions.py` — 26 tests covering: - Each suppressed false positive with a concrete code fixture - True-positive counterpart for each rule (ensure real findings still fire) - Global `[defaults]` behavior (test-file exclusion via global config, not per-rule)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces two new mechanisms for controlling rule behavior globally, then applies them to fix a class of false positives that surface when scanning mature Python codebases.
Problem
Scanning a large Django-based project produced 864 findings with
-s HIGH. Manual verification showed that ~720 were false positives:PY515/SHELL645/SHELL670— flagging everyre.compile()call as "code compilation"PY102— the taint engine firing on generic callable invocationsPY511/JSON612—json.loads()flagged as dangerous deserializationAUTH711,CSRF747,SESS744,PATH813, etc. — firing on test fixtures and well-known safe patternsWithout filtering, scanning also revealed ~9,000 additional Low-severity findings from rules that flag every Python built-in (
isinstance,super,len, f-strings,getattr, etc.).Solution
1.
[defaults]section in rules TOMLexclude_file_patterns— glob list applied to all rules; eliminates the need to repeatexclude_file_patternon every individual ruledisabled_rule_ids— completely disables noisy rules without deleting their definitions (easy to re-enable per project)2. Per-rule
exclude_patternRegex matched against the flagged line; suppresses the finding if it matches. Fixes rules that are correct in general but have well-known safe variants:
Results on Django 6.1-alpha
re.compile()false positivesyaml.load(SafeLoader)false positivespickle.loads()true positivesexec()true positivesFiles changed
src/pyspector/_rust_core/src/rules.rs—Defaultsstruct,Rule::is_file_excluded(),exclude_patternfieldsrc/pyspector/_rust_core/src/analysis/mod.rs— applydisabled_rule_idsbefore scansrc/pyspector/_rust_core/src/analysis/ast_analysis.rs— useis_file_excluded()andexclude_patternsrc/pyspector/_rust_core/src/analysis/config_analysis.rs— samesrc/pyspector/rules/built-in-rules.toml—[defaults]section + per-rule fixestests/unit/test_false_positive_reductions.py— 26 tests (each fix has a suppression test + true-positive retention test)Test plan
python -m pytest tests/unit/test_false_positive_reductions.py— 26 tests passpyspector scan /path/to/django— confirmre.compile(),yaml.load(SafeLoader), and test-fixture findings are gonepickle.loads()andexec()findings still present in production code