mirror of
https://github.com/usestrix/strix.git
synced 2026-11-14 09:01:25 +00:00
fix(sarif): qualify CWE rule ids with the finding's vulnerability class
Two findings sharing one CWE but belonging to distinct vulnerability classes collapsed into a single SARIF rule (65 findings -> 13 rules in the issue's measurement), so consumers keying on (ruleId, location) could not tell distinct bugs apart. _rule_id() now returns class-qualified ids, e.g. CWE-639/insecure-direct-object-reference instead of bare CWE-639, with the qualifier taken from the closed curated class-keyword catalogue only - never from the raw LLM title. Findings with no curated keyword keep the bare CWE id. Tags keep the bare CWE-NNN tag and helpUri keeps pointing at the CWE definition page; fingerprints keep keying on the bare CWE so refining a rule id does not churn existing alerts or break dismissal carryover. Fixes #1575. AI assistance disclosure: this change was implemented with AI assistance; the fix was reproduced before editing, and the new regression tests plus the existing SARIF suites were run against the change. Signed-off-by: Bharath Vemula <12749607+bharathvemula@users.noreply.github.com>
This commit is contained in:
Bharath Vemula
parent
278b6a280b
commit
3b7b7e4e7e
2 changed files with 163 additions and 13 deletions
|
|
@ -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/<class>``) 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
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue