diff --git a/strix/report/sarif.py b/strix/report/sarif.py index 821fffbc0..be0030a97 100644 --- a/strix/report/sarif.py +++ b/strix/report/sarif.py @@ -493,8 +493,12 @@ def _build_result( # partialFingerprints. Computed from the deterministic primitives # this report already carries (CWE, primary code location, route # tuple) — NOT from the LLM-authored title or message body, which - # vary cosmetically across runs of the same finding. - fp = _primary_fingerprint(rule_id, report, locations, is_synthetic=is_synthetic) + # vary cosmetically across runs of the same finding. Fingerprints + # key on the bare CWE, not the class-qualified rule id, so refining + # a rule id (``CWE-639`` → ``CWE-639/``) does not churn + # existing alerts or break cross-run dismissal carryover. + fp_rule_id = _cwe_base(rule_id) or rule_id + fp = _primary_fingerprint(fp_rule_id, report, locations, is_synthetic=is_synthetic) if fp: result["partialFingerprints"] = {"primaryLocationLineHash": fp} # File-independent class fingerprint as a sibling property: lets @@ -502,7 +506,7 @@ def _build_result( # determinations across file rename refactors where the primary # fingerprint legitimately shifts but the underlying class is # unchanged. - class_fp = _class_fingerprint(rule_id, report) + class_fp = _class_fingerprint(fp_rule_id, report) result["properties"] = _result_properties(report, class_fp, is_synthetic=is_synthetic) return result @@ -865,11 +869,22 @@ def _rule_id(report: dict[str, Any]) -> str: CWE values are normalised from Strix output variants (``CWE-306``, ``cwe: 306``, ``306``) to the canonical ``CWE-NNN`` form. Without normalisation, the same weakness across runs dedups to separate rules. + + A CWE-keyed rule is further qualified with the finding's + vulnerability-class slug (``CWE-639/insecure-direct-object-reference``) + so distinct classes that share one CWE do not collapse into a single + rule. The slug comes from the curated class-keyword catalogue only — + never from the raw LLM title — and findings with no curated keyword + keep the bare ``CWE-NNN`` id, so rule ids stay stable across + title rewordings. """ cwe = _string_value(report.get("cwe")) if cwe: normalised = _normalise_cwe(cwe) if normalised: + class_slug = _class_rule_slug(report) + if class_slug: + return f"{normalised}/{class_slug}" return normalised cve = _string_value(report.get("cve")) @@ -892,6 +907,37 @@ def _normalise_cwe(value: str) -> str | None: return f"CWE-{digits}" +def _cwe_base(rule_id: str) -> str | None: + """``CWE-639/insecure-direct-object-reference`` → ``CWE-639``. + + Returns None for non-CWE rule ids (CVE, finding id, title slug). + Used wherever the bare CWE matters — tags, help URIs, and the + fingerprint composites, which must not churn when a rule id gains + its class qualifier. + """ + if not rule_id.startswith("CWE-"): + return None + head = rule_id.split("/", 1)[0] + if re.fullmatch(r"CWE-[0-9]+", head): + return head + return None + + +def _class_rule_slug(report: dict[str, Any]) -> str | None: + """Slug of the finding's curated vulnerability class, if one matches. + + Uses only the closed class-keyword catalogue (the same notion of + class ``_class_fingerprint`` uses), never the raw title: a title + with no curated keyword yields None and the rule keeps its bare + CWE id rather than gaining a prose-dependent suffix. + """ + title = _string_value(report.get("title")) or "" + keyword = _curated_class_keyword(title) + if not keyword: + return None + return _slugify(keyword) + + # Vulnerability-class keywords for the file-independent class # fingerprint. Order matters — first match wins, so precise terms # come before fuzzy ones (e.g. "broken access control" before @@ -1062,6 +1108,22 @@ def _class_fingerprint(rule_id: str, report: dict[str, Any]) -> str | None: return hashlib.sha256(composite.encode("utf-8")).hexdigest() +def _curated_class_keyword(title: str) -> str: + """First matching curated class keyword in ``title``, or "". + + The catalogue is closed and ordered (first match wins); this is + the stable notion of "the finding's class" shared by fingerprints + and CWE rule-id qualification. + """ + if not title: + return "" + lower = title.lower() + for kw in _VULN_CLASS_KEYWORDS: + if kw in lower: + return kw + return "" + + def _class_keyword(title: str) -> str: """Pick the first matching curated keyword in ``title``, or fall back to the first 5 lowercased alpha-numeric words. @@ -1071,26 +1133,26 @@ def _class_keyword(title: str) -> str: two fingerprints stay aligned on what counts as "the same class". Returns empty string when title is empty or whitespace-only. """ + keyword = _curated_class_keyword(title) + if keyword: + return keyword if not title: return "" - lower = title.lower() - for kw in _VULN_CLASS_KEYWORDS: - if kw in lower: - return kw - words = re.findall(r"[a-z0-9]+", lower)[:5] + words = re.findall(r"[a-z0-9]+", title.lower())[:5] return " ".join(words) def _rule_name(rule_id: str, title: str) -> str: """SARIF rule.name must be a free-form string; prefer the finding title where available, fall back to a snake_case'd form of the rule id.""" - return title or rule_id.replace("-", "_") + return title or rule_id.replace("-", "_").replace("/", "_") def _rule_tags(rule_id: str, report: dict[str, Any]) -> list[str]: tags: list[str] = ["security"] - if rule_id.startswith("CWE-"): - tags.append(rule_id) + cwe_base = _cwe_base(rule_id) + if cwe_base: + tags.append(cwe_base) cve = _string_value(report.get("cve")) if cve and cve not in tags: tags.append(cve) @@ -1105,8 +1167,9 @@ def _rule_tags(rule_id: str, report: dict[str, Any]) -> list[str]: def _help_uri_for(rule_id: str) -> str | None: - if rule_id.startswith("CWE-"): - return f"https://cwe.mitre.org/data/definitions/{rule_id.removeprefix('CWE-')}.html" + cwe_base = _cwe_base(rule_id) + if cwe_base: + return f"https://cwe.mitre.org/data/definitions/{cwe_base.removeprefix('CWE-')}.html" return None diff --git a/tests/test_sarif.py b/tests/test_sarif.py index 61ffc39bb..851c8420c 100644 --- a/tests/test_sarif.py +++ b/tests/test_sarif.py @@ -371,3 +371,90 @@ def test_calibration_metadata_survives_into_result_properties(tmp_path: Path) -> assert strix["confidence_rationale"] == "Reproduced once out of three attempts." assert strix["severity_change_conditions"] == "Critical if the WAF rule is removed." assert strix["fix_verification"] == "Not retested." + + +def test_same_cwe_distinct_classes_get_distinct_rules(tmp_path: Path) -> None: + # Regression for #1575: ruleId was the bare CWE, so distinct findings + # sharing one CWE collapsed into a single rule. + write_sarif( + tmp_path, + [ + _finding( + id="vuln-0001", + title="Insecure Direct Object Reference via login parameter", + cwe="CWE-639", + ), + _finding( + id="vuln-0002", + title="Broken Access Control allows viewing other users' orders", + cwe="CWE-639", + ), + ], + ) + run = _read(tmp_path)["runs"][0] + rules = run["tool"]["driver"]["rules"] + results = run["results"] + + assert [rule["id"] for rule in rules] == [ + "CWE-639/insecure-direct-object-reference", + "CWE-639/broken-access-control", + ] + assert [result["ruleId"] for result in results] == [ + "CWE-639/insecure-direct-object-reference", + "CWE-639/broken-access-control", + ] + for result in results: + assert rules[result["ruleIndex"]]["id"] == result["ruleId"] + # The class qualifier must not leak into the CWE tag or help link. + for rule in rules: + assert "CWE-639" in rule["properties"]["tags"] + assert rule["helpUri"] == "https://cwe.mitre.org/data/definitions/639.html" + + +def test_same_cwe_same_class_shares_one_rule(tmp_path: Path) -> None: + # Class-level dedup is preserved: two SQL-injection findings under + # one CWE still share a single rule. + write_sarif( + tmp_path, + [ + _finding(id="vuln-0001", title="SQL Injection in login", cwe="CWE-89"), + _finding(id="vuln-0002", title="SQL Injection in search", cwe="CWE-89"), + ], + ) + run = _read(tmp_path)["runs"][0] + rules = run["tool"]["driver"]["rules"] + + assert [rule["id"] for rule in rules] == ["CWE-89/sql-injection"] + assert [result["ruleId"] for result in run["results"]] == [ + "CWE-89/sql-injection", + "CWE-89/sql-injection", + ] + + +def test_cwe_rule_without_curated_class_keeps_bare_cwe_id(tmp_path: Path) -> None: + # A title with no curated class keyword must not gain a + # prose-dependent suffix: the rule keeps its bare CWE id. + write_sarif( + tmp_path, + [_finding(title="Something odd happened on the dashboard", cwe="CWE-89")], + ) + run = _read(tmp_path)["runs"][0] + + assert [rule["id"] for rule in run["tool"]["driver"]["rules"]] == ["CWE-89"] + assert run["results"][0]["ruleId"] == "CWE-89" + + +def test_class_qualified_rule_keeps_bare_cwe_fingerprints(tmp_path: Path) -> None: + # Refining a rule id must not churn alerts: fingerprints still key + # on the bare CWE, so the same finding at the same location keeps + # its fingerprint whether or not its title carries a class keyword. + a = tmp_path / "a" + b = tmp_path / "b" + a.mkdir() + b.mkdir() + write_sarif(a, [_finding(title="SQL Injection in get_user")]) + write_sarif(b, [_finding(title="Something odd happened on the dashboard")]) + + fp_a = _read(a)["runs"][0]["results"][0]["partialFingerprints"] + fp_b = _read(b)["runs"][0]["results"][0]["partialFingerprints"] + assert fp_a == fp_b