diff --git a/strix/report/sarif.py b/strix/report/sarif.py index 821fffbc03..1b19a865ac 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,24 @@ 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 equivalent class names resolve + to one canonical keyword (``_CLASS_ALIASES``), so rewordings of the + same class keep the same rule id. 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,10 +909,44 @@ 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. Equivalent + names map to their canonical keyword first (``_CLASS_ALIASES``), + so e.g. "SQLi" yields the "sql injection" slug, not a bare id. + """ + 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 -# "access control"). Keep this list closed and curated; a future +# "access control", "regex denial of service" before +# "denial of service"). Keep this list closed and curated; a future # maintainer adding sloppy entries could collapse distinct findings # to the same class hash. _VULN_CLASS_KEYWORDS = ( @@ -912,7 +963,6 @@ def _normalise_cwe(value: str) -> str | None: "session fixation", "open redirect", "path traversal", - "directory traversal", "command injection", "sql injection", "code injection", @@ -932,7 +982,6 @@ def _normalise_cwe(value: str) -> str | None: "toctou", "information disclosure", "insecure direct object reference", - "idor", "bola", "bfla", "cross-tenant", @@ -940,20 +989,61 @@ def _normalise_cwe(value: str) -> str | None: "tenant bypass", "auth bypass", "rate limiting", - "rate limit", "weak cryptography", "weak hash", "weak random", "insecure random", "tls verification", "certificate verification", - "denial of service", "regex denial of service", - "redos", + "denial of service", "supply chain", ) +# Equivalent names for a canonical class, keyed by that canonical +# keyword. A rule id (and a class fingerprint) must be a property of +# the vulnerability class, not of the LLM's phrasing of it: "SQLi", +# "directory traversal", "IDOR", "ReDoS" and "cross-site scripting" +# are the same classes as "sql injection", "path traversal", +# "insecure direct object reference", "regex denial of service" and +# "xss", so they must resolve to the same canonical keyword — and +# hence the same rule id — rather than to a bare CWE id or a +# different slug. Only unambiguous acronyms / full-form synonyms +# belong here. Near-synonyms that may denote distinct sub-classes +# (e.g. "bola" vs "insecure direct object reference", "weak random" +# vs "insecure random", "auth bypass" vs "missing authentication") +# are deliberately NOT aliased, so distinct classes sharing one CWE +# still get distinct rules. +_CLASS_ALIASES: dict[str, tuple[str, ...]] = { + "sql injection": ("sqli",), + "command injection": ("cmd injection",), + "template injection": ("ssti",), + "xss": ("cross site scripting",), + "csrf": ("cross site request forgery",), + "ssrf": ("server side request forgery",), + "xxe": ("xml external entity", "xml external entities"), + "toctou": ("time of check time of use", "time of check to time of use"), + "insecure direct object reference": ("idor",), + "path traversal": ("directory traversal",), + "rate limiting": ("rate limit",), + "regex denial of service": ("redos",), + "open redirect": ("unvalidated redirect",), + "information disclosure": ("info disclosure",), +} + + +# (pattern, canonical keyword) pairs in match order: each canonical +# keyword's aliases sit at that keyword's own position, so an alias +# and its canonical form have identical priority against every other +# class. First match wins, as in the catalogue itself. +_CLASS_MATCH_PATTERNS: tuple[tuple[str, str], ...] = tuple( + (pattern, canonical) + for canonical in _VULN_CLASS_KEYWORDS + for pattern in (*_CLASS_ALIASES.get(canonical, ()), canonical) +) + + def _primary_fingerprint( rule_id: str, report: dict[str, Any], @@ -1062,6 +1152,28 @@ class fingerprint is a tiebreaker, not a primary reconciliation key. return hashlib.sha256(composite.encode("utf-8")).hexdigest() +def _curated_class_keyword(title: str) -> str: + """Canonical curated class keyword for ``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. Equivalent names resolve to their + canonical keyword (``_CLASS_ALIASES``), so rewordings of the same + class — "SQL Injection" vs "SQLi", "IDOR" vs "insecure direct + object reference" — return the same value instead of diverging + into a qualified id and a bare CWE id. Titles are normalised + (hyphens/underscores become spaces) before matching, so + "sql-injection" and "cross-site scripting" match too. + """ + if not title: + return "" + normalised = re.sub(r"\s+", " ", title.lower().replace("-", " ").replace("_", " ")).strip() + for pattern, canonical in _CLASS_MATCH_PATTERNS: + if pattern in normalised: + return canonical + 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 +1183,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 +1217,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 61ffc39bbd..a77e95e0ea 100644 --- a/tests/test_sarif.py +++ b/tests/test_sarif.py @@ -371,3 +371,169 @@ 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 + + +def _rule_and_location(run_dir: Path) -> tuple[str, dict[str, Any], dict[str, Any]]: + result = _read(run_dir)["runs"][0]["results"][0] + loc = result["locations"][0]["physicalLocation"] + return result["ruleId"], loc, result["partialFingerprints"] + + +def test_title_rewording_keeps_same_rule_id(tmp_path: Path) -> None: + # Greptile P1 on #1618: "SQL Injection in get_user" and "SQLi via + # string-formatted query in get_user" are the same bug, but the + # first qualified to CWE-89/sql-injection while the second fell + # back to bare CWE-89, so a consumer keying alerts on + # (ruleId, location) would treat the re-scan as a new alert. + # Both rewordings must now resolve to the same canonical class. + 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="SQLi via string-formatted query in get_user")]) + + rule_a, loc_a, fp_a = _rule_and_location(a) + rule_b, loc_b, fp_b = _rule_and_location(b) + + assert rule_a == rule_b == "CWE-89/sql-injection" + assert ( + loc_a + == loc_b + == { + "artifactLocation": {"uri": "app.py"}, + "region": {"startLine": 4}, + } + ) + assert fp_a == fp_b + + +def test_class_alias_rewordings_share_one_rule_id(tmp_path: Path) -> None: + # The fix is a canonical-class mapping, not a one-off for "SQLi": + # every equivalent-name pair below must share one rule id. + pairs = [ + ( + "IDOR via order parameter", + "Insecure Direct Object Reference via order parameter", + "CWE-639", + ), + ("Cross-Site Scripting in search", "XSS in search", "CWE-79"), + ("Directory traversal in download", "Path traversal in download", "CWE-22"), + ("ReDoS in search regex", "Regex denial of service in search regex", "CWE-1333"), + ("SQL-injection in login", "SQL Injection in login", "CWE-89"), + ] + for i, (alias_title, canonical_title, cwe) in enumerate(pairs): + a = tmp_path / f"a{i}" + b = tmp_path / f"b{i}" + a.mkdir() + b.mkdir() + write_sarif(a, [_finding(title=alias_title, cwe=cwe)]) + write_sarif(b, [_finding(title=canonical_title, cwe=cwe)]) + rule_a, _, _ = _rule_and_location(a) + rule_b, _, _ = _rule_and_location(b) + assert rule_a == rule_b, (alias_title, canonical_title, rule_a, rule_b) + + +def test_distinct_class_still_gets_distinct_rule_id(tmp_path: Path) -> None: + # Negative control: canonicalisation must not collapse genuinely + # different classes. Same CWE, different class -> different rule. + write_sarif( + tmp_path, + [ + _finding(id="vuln-0001", title="SQL Injection in get_user", cwe="CWE-89"), + _finding(id="vuln-0002", title="Command Injection in get_user", cwe="CWE-89"), + ], + ) + run = _read(tmp_path)["runs"][0] + + assert [result["ruleId"] for result in run["results"]] == [ + "CWE-89/sql-injection", + "CWE-89/command-injection", + ]