diff --git a/CODEBASE_ANALYSIS.md b/CODEBASE_ANALYSIS.md new file mode 100644 index 00000000..db933450 --- /dev/null +++ b/CODEBASE_ANALYSIS.md @@ -0,0 +1,94 @@ +# Senior Bug Review + +## Scope +Focused review of executable helper code in this repository, prioritizing correctness-impacting bugs over style issues. + +## Bug 1 — SARIF merge drops valid findings (high impact) +**File:** `plugins/static-analysis/skills/semgrep/scripts/merge_sarif.py` + +### Problem +The fallback merge de-duplicates findings using only: +- `ruleId` +- `artifactLocation.uri` +- `region.startLine` + +This key is too coarse. Multiple distinct findings on the same line (same rule, different sink/trace/message) are silently discarded. + +### Why this is a bug +Security tooling must preserve findings unless equivalence is proven. Current logic can under-report vulnerabilities and mislead triage. + +### Evidence +`dedup_key = (rule_id, uri, start_line)` then skip if already seen. + +### Fix direction +Use stable fingerprints when present (`partialFingerprints`/`fingerprints`) and only fall back to a richer composite key including start/end columns and normalized message text. + +--- + +## Bug 2 — Rule collisions across tools corrupt merged metadata (medium impact) +**File:** `plugins/static-analysis/skills/semgrep/scripts/merge_sarif.py` + +### Problem +Merged rules are keyed only by `rule.id`, regardless of tool source. + +### Why this is a bug +Different tools can legitimately share the same rule id string with different semantics. Current behavior keeps the first seen rule and overwrites logical provenance. + +### Evidence +`seen_rules` is a dict keyed by `rule_id` only. + +### Fix direction +Namespace rules by tool identity (e.g., `tool_name:rule_id`) or preserve per-run separation instead of flattening all results into a single run. + +--- + +## Bug 3 — Dedup fingerprint can merge unrelated findings in large repos (medium impact) +**File:** `plugins/static-analysis/skills/sarif-parsing/resources/sarif_helpers.py` + +### Problem +`compute_fingerprint()` uses only: +- `ruleId` +- **filename only** (not path) +- `startLine` +- message prefix (first 50 chars) + +### Why this is a bug +Two different files with the same basename (common in monorepos) can collide and be treated as duplicates, causing false “fixed/unchanged/new” results. + +### Evidence +The fingerprint intentionally truncates path to `Path(file_path).name`. + +### Fix direction +Use normalized relative path (or URI) plus start/end column and full message hash. Keep optional path-stripping as an explicit mode, not default. + +--- + +## Bug 4 — Path normalization mishandles file URIs with authorities (low/medium impact) +**File:** `plugins/static-analysis/skills/sarif-parsing/resources/sarif_helpers.py` + +### Problem +`normalize_path()` strips `file://` using string slicing, which can mis-handle URIs like `file://hostname/path` and certain platform-specific forms. + +### Why this is a bug +Incorrect path normalization can break grouping/filtering and produce unstable fingerprints across environments. + +### Evidence +Logic does `uri = uri[7:]` without URI parsing. + +### Fix direction +Parse file URIs via `urllib.parse.urlparse` and handle netloc/path explicitly. + +--- + +## Priority Order +1. Fix Bug 1 first (finding loss). +2. Fix Bug 3 next (diff accuracy). +3. Fix Bug 2 (metadata correctness in multi-tool merges). +4. Fix Bug 4 (portability/stability). + +## Recommended Validation +- Add regression SARIF fixtures with intentionally colliding findings. +- Assert merged result counts remain stable across: + - same rule + same line + different column/message + - same basename in different directories + - mixed-tool rule-id overlap