SARIF: duplicate rule IDs in per-vulnerability mode (`in` used on an Array)

[airadier]

Summary

In the per-vulnerability SARIF path (not grouped by package), the rule dedupe guard never fires, so a CVE affecting N packages emits N rules sharing the same id.

src/infrastructure/github/SarifReportPresenter.ts:196-204:

let ruleIds: string[] = [];

filterPackages(data.getPackages(), filters).forEach(pkg => {
  pkg.getVulnerabilities().forEach(vuln => {
    if (!(vuln.cve in ruleIds)) {     // ruleIds is an Array
      ruleIds.push(vuln.cve)
      // ... rules.push(rule)

in tests property keys, not values. On an array those are "0", "1", … plus "length", so a CVE string never matches and the guard always passes.

Evidence

Running the presenter over tests/fixtures/vm/report-test-v1.json with groupByPackage = false:

• 195 rules, 132 distinct → 63 surplus
• 53 CVEs duplicated, worst case 3 copies

CVE-2023-44487 x3:
  copy0  ... Package: org.eclipse.jetty.http2:http2-server
  copy1  ... Package: org.eclipse.jetty.http2:http2-common
  copy2  ... Package: nghttp2-libs

The copies are not identical — shortDescription, fullDescription and help all embed the specific package via getSARIFVulnShortDescription(pkg, vuln) — so each copy describes a different package under one shared ID.

Impact

Results reference their rule by ruleId only (no ruleIndex is emitted), so a consumer cannot pick the right copy. SARIF expects rule IDs to be unique within a toolComponent so that lookup is unambiguous.

GitHub's ingestion is lenient and dedupes rather than rejecting, so in practice:

1. The alert's rule description and help table name one arbitrary affected package — not necessarily the one the result refers to, and not all of them.
2. ~48% bloat in the rules array on the fixture above.

Note this was not covered by #95. That fixed a different duplicate-ID problem in the group-by-package path (bare pkg.name as ID colliding across versions/paths, changed to ${pkg.name}-${pkg.version}-${pkg.path}). The per-vulnerability guard was only reindented in that diff.

Proposal

One-line fix:

if (!ruleIds.includes(vuln.cve)) {

Better: make ruleIds a Set<string> and use .has(), so the mistake is unrepresentable.

Test gap worth closing at the same time

tests/fixtures/vm/sarif-test.json is group-by-package output (49 rules, 49 distinct), so the per-vulnerability path has no fixture at all — which is why this went unnoticed.

That same fixture also bakes in "level": "note" for all 49 results, which hides a second bug of the same class: check_level (line 370) does sev_value in this.LEVELS[key] where LEVELS[key] is an array, so every result gets note regardless of severity — 195/195 per-vuln, 49/49 grouped. It dates to #35 (v4.0.0) and survived the TS rewrite (#54) and the clean-architecture refactor (#89).

Low visibility because GitHub code scanning derives displayed severity from rule.properties["security-severity"], not result.level, so the alert list looks correct today. It does affect PR annotation levels and non-GitHub SARIF consumers.

A per-vulnerability fixture asserting unique rule IDs and a non-note level for High/Critical would catch both. Grepping for other in-on-array uses is probably worth it too — there were two in this one file.

Related: the Go SARIF formatter proposed in draios/vm-libs#396 avoids both (seenRule map[string]bool, explicit levelForSeverity), but the TypeScript presenter keeps shipping them until the action migrates.