From 32b1acf6bd319b61e8a886df4282b413f68e6a65 Mon Sep 17 00:00:00 2001 From: Bharath Vemula <12749607+bharathvemula@users.noreply.github.com> Date: Sat, 31 Oct 2026 04:53:04 +0000 Subject: [PATCH] fix(sarif): canonicalise vulnerability class for CWE rule ids Greptile P1 on #1618: rewordings of the same finding diverged in rule identity. 'SQL Injection in get_user' qualified to CWE-89/sql-injection, while 'SQLi via string-formatted query in get_user' matched no curated keyword and fell back to bare CWE-89, so a consumer keying alerts on (ruleId, location) would treat the re-scan as a new alert and lose dismissal carryover (partialFingerprints already key on the bare CWE, so they could not preserve rule identity). _curated_class_keyword() now resolves equivalent class names to one canonical keyword via a closed _CLASS_ALIASES map (sqli -> sql injection, idor -> insecure direct object reference, directory traversal -> path traversal, redos -> regex denial of service, cross-site scripting -> xss, and other unambiguous acronym/full-form synonyms), with aliases matched at their canonical keyword's catalogue position and titles normalised over hyphens/underscores. 'regex denial of service' now precedes 'denial of service' in the catalogue, as the precise-before-fuzzy ordering intends. Near-synonyms that may denote distinct sub-classes (bola vs insecure direct object reference, weak random vs insecure random, auth bypass vs missing authentication) are deliberately not aliased, so distinct classes sharing a CWE still get distinct rules. The same canonicalisation also aligns class fingerprints for reworded titles. AI assistance disclosure: this change was implemented with AI assistance; the bot's claim was reproduced before editing, and the new regression tests plus the existing SARIF/report suites were run against the change. Signed-off-by: Bharath Vemula --- strix/report/sarif.py | 78 ++++++++++++++++++++++++++++++++++-------- tests/test_sarif.py | 79 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 14 deletions(-) diff --git a/strix/report/sarif.py b/strix/report/sarif.py index be0030a97..1b19a865a 100644 --- a/strix/report/sarif.py +++ b/strix/report/sarif.py @@ -874,7 +874,9 @@ def _rule_id(report: dict[str, Any]) -> str: 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 + 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. """ @@ -929,7 +931,9 @@ def _class_rule_slug(report: dict[str, Any]) -> str | None: 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. + 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) @@ -941,7 +945,8 @@ def _class_rule_slug(report: dict[str, Any]) -> str | None: # 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 = ( @@ -958,7 +963,6 @@ _VULN_CLASS_KEYWORDS = ( "session fixation", "open redirect", "path traversal", - "directory traversal", "command injection", "sql injection", "code injection", @@ -978,7 +982,6 @@ _VULN_CLASS_KEYWORDS = ( "toctou", "information disclosure", "insecure direct object reference", - "idor", "bola", "bfla", "cross-tenant", @@ -986,20 +989,61 @@ _VULN_CLASS_KEYWORDS = ( "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], @@ -1109,18 +1153,24 @@ def _class_fingerprint(rule_id: str, report: dict[str, Any]) -> str | None: def _curated_class_keyword(title: str) -> str: - """First matching curated class keyword in ``title``, or "". + """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. + 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 "" - lower = title.lower() - for kw in _VULN_CLASS_KEYWORDS: - if kw in lower: - return kw + normalised = re.sub(r"\s+", " ", title.lower().replace("-", " ").replace("_", " ")).strip() + for pattern, canonical in _CLASS_MATCH_PATTERNS: + if pattern in normalised: + return canonical return "" diff --git a/tests/test_sarif.py b/tests/test_sarif.py index 851c8420c..a77e95e0e 100644 --- a/tests/test_sarif.py +++ b/tests/test_sarif.py @@ -458,3 +458,82 @@ def test_class_qualified_rule_keeps_bare_cwe_fingerprints(tmp_path: Path) -> Non 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", + ]