fix(rules): improve precision and recall of java opengrep rules

docs/java-sast-benchmark.md

@@ -0,0 +1,139 @@
+# Java SAST rule benchmarking
+
+This document records how `socket_basics/rules/java.yml` is measured and what the
+current numbers are. It exists so the next person changing a Java rule can tell
+whether they improved it or just moved the noise around.
+
+## Why
+
+A customer SAST evaluation reported roughly 90% false positives from our Java
+rules, and compared us unfavourably to CodeQL. Reproducing that on real code
+confirmed it: on six mature, heavily reviewed open source Java projects
+(~17,400 Java files) the rule set emitted **1,631 findings**, and a hand
+adjudicated random sample of 40 of them contained **zero true positives**.
+
+## Corpora
+
+Two corpora, because they answer different questions.
+
+| Corpus | What it answers | Source |
+|---|---|---|
+| OWASP Benchmark v1.2 | Precision and recall against ground truth | `github.com/OWASP-Benchmark/BenchmarkJava` |
+| Mature OSS Java projects | How much noise a real user has to triage | guava, netty, spring-framework, commons-lang, commons-io, spring-petclinic |
+| WebGoat | Whether we still catch deliberately planted vulnerabilities | `github.com/WebGoat/WebGoat` |
+
+OWASP Benchmark ships 2,740 annotated servlets (1,415 real vulnerabilities,
+1,325 deliberate non-vulnerabilities) with an `expectedresults-1.2.csv` giving
+the category, CWE, and whether each case is genuinely vulnerable. It is the
+standard scoring corpus for Java SAST.
+
+The mature-OSS corpus is the honest proxy for customer experience. These
+libraries are not web applications and contain essentially no reachable
+instances of these vulnerability classes, so nearly every finding is noise.
+Alert volume there is the number that maps to triage burden.
+
+## Running it
+
+```bash
+git clone --depth 1 https://github.com/OWASP-Benchmark/BenchmarkJava.git
+
+opengrep --json --dataflow-traces --quiet -a --no-git-ignore \
+  --config socket_basics/rules/java.yml \
+  --output results.json \
+  BenchmarkJava/src/main/java
+
+python3 scripts/score_owasp_benchmark.py results.json \
+  BenchmarkJava/expectedresults-1.2.csv
+```
+
+The scorer reports per-category precision, recall, false positive rate and the
+OWASP Benchmark score (`TPR - FPR`), plus per-rule TP/FP counts so you can see
+which rule is responsible for a regression.
+
+## Results
+
+Measured with opengrep 1.25.0.
+
+### OWASP Benchmark v1.2 (ground truth)
+
+| | Before | After |
+|---|---|---|
+| Precision | 64.5% | **76.5%** |
+| Recall | 12.4% | **63.4%** |
+| False positive rate | 7.3% | 20.8% |
+| Benchmark score (TPR - FPR) | 5.1 | **42.6** |
+| True positives found | 176 | **897** |
+
+Per category, after the change:
+
+| Category | Precision | Recall |
+|---|---|---|
+| securecookie | 100.0% | 100.0% |
+| weakrand | 100.0% | 91.7% |
+| crypto | 100.0% | 74.6% |
+| hash | 100.0% | 69.0% |
+| xpathi | 60.0% | 80.0% |
+| ldapi | 58.3% | 77.8% |
+| xss | 67.4% | 70.7% |
+| pathtraver | 56.1% | 69.2% |
+| cmdi | 63.6% | 44.4% |
+| sqli | 64.9% | 44.1% |
+
+The headline false positive rate rises because the rule set now detects seven
+categories it previously scored zero on. Precision, which is the share of
+emitted findings that are real, is the comparable number and it improved.
+
+### Mature open source Java projects (triage burden)
+
+| | Before | After | Change |
+|---|---|---|---|
+| Total findings | 1,631 | 130 | **-92%** |
+| Unique findings, mature libraries only | 1,536 | 85 | **-94.5%** |
+
+Three rules produced 74% of the original noise: `java-empty-catch-block` (645),
+`java-reflection-injection` (296) and `java-system-out-usage` (263). All three
+now emit zero findings on the mature-library corpus.
+
+### WebGoat (deliberately vulnerable)
+
+87 findings before, 45 after. The removed findings were lint noise
+(`java-system-out-usage` 26, `java-hardcoded-ip` 5, `java-empty-catch-block` 4)
+plus three `java-reflection-injection` matches on factory `newInstance()` calls
+and a JDK dynamic proxy. The security findings, including the Zip Slip in
+`ProfileZipSlip`, the default credentials in `DefaultCredentialsTask`, and the
+weak PRNG in `PasswordResetLink`, are still reported.
+
+## Known limits
+
+**OWASP Benchmark's designated false positives are adversarial toward
+pattern-based engines.** A large share of them are unreachable-branch traps:
+
+```java
+String guess = "ABC";
+char switchTarget = guess.charAt(1);   // always 'B', the safe branch
+switch (switchTarget) {
+  case 'A': bar = param; break;        // tainted, but dead code
+  case 'B': bar = "bob"; break;        // always taken
+}
+```
+
+Resolving these requires constant propagation plus path sensitivity. opengrep's
+taint analysis is path insensitive, so it reports the dead tainted branch. This
+is the structural difference behind the "Semgrep matches patterns, CodeQL traces
+paths" comparison, and it caps achievable precision on `sqli`, `cmdi` and
+`pathtraver` regardless of how the rules are written. Do not read the remaining
+FPs in those categories as fixable rule defects without checking the test case
+first.
+
+**Remaining noise not addressed here.** On the mature-library corpus these rules
+were untouched by this change and are still the largest remaining sources:
+
+| Rule | Findings | Cause |
+|---|---|---|
+| `java-template-injection` | 20 | Matches any `.process(...)` call |
+| `java-xxe-vulnerability` | 14 | Matches `DocumentBuilderFactory.newInstance()` unconditionally, ignoring whether secure features are set |
+| `java-unsafe-deserialization` | 14 | Library serialization helpers that accept a caller-supplied `ObjectInputStream` |
+| `java-jndi-injection` | 8 | Matches any `.lookup(...)` call |
+| `java-sql-injection` | 4 | `$STMT.execute(...)` and `$TEMPLATE.query(...)` sinks match any method of those names |
+
+`trustbound` (CWE-501) has no rule at all; OWASP Benchmark scores 126 cases for it.

scripts/score_owasp_benchmark.py

@@ -0,0 +1,137 @@
+#!/usr/bin/env python3
+"""Score an opengrep JSON run against the OWASP Benchmark v1.2 expected results.
+
+Two scores are produced:
+  1. Category-matched precision/recall (official OWASP Benchmark style): a finding
+     only counts for the CWE the test case actually targets.
+  2. Raw finding volume: everything the tool emitted, which is what a customer
+     actually has to triage.
+"""
+import csv
+import json
+import re
+import sys
+from collections import defaultdict
+from pathlib import Path
+
+# Map socket-basics java rule ids -> OWASP Benchmark category
+RULE_CATEGORY = {
+    "java-sql-injection": "sqli",
+    "java-jpa-sql-injection": "sqli",
+    "java-command-injection": "cmdi",
+    "java-path-traversal": "pathtraver",
+    "java-ldap-injection": "ldapi",
+    "java-insecure-random": "weakrand",
+    "java-weak-crypto-md5": "hash",
+    "java-weak-crypto-sha1": "hash",
+    "java-weak-cipher": "crypto",
+    "java-insecure-cookie": "securecookie",
+    "java-xpath-injection": "xpathi",
+    "java-trust-boundary-violation": "trustbound",
+    "java-xss": "xss",
+    "java-template-injection": "xss",
+}
+
+TEST_RE = re.compile(r"(BenchmarkTest\d{5})")
+
+
+def load_expected(csv_path):
+    expected = {}
+    with open(csv_path) as fh:
+        for row in csv.reader(fh):
+            if not row or row[0].startswith("#"):
+                continue
+            name, category, real, cwe = row[0].strip(), row[1].strip(), row[2].strip(), row[3].strip()
+            expected[name] = {"category": category, "real": real.lower() == "true", "cwe": cwe}
+    return expected
+
+
+def main(results_json, expected_csv):
+    expected = load_expected(expected_csv)
+    data = json.loads(Path(results_json).read_text())
+    findings = data.get("results", [])
+
+    # Deduplicate: one (test case, rule) pair counts once, matching how a
+    # reviewer triages "does this tool flag this file for this issue".
+    hits = set()
+    per_rule_total = defaultdict(int)
+    off_benchmark = 0
+
+    for f in findings:
+        rule = f.get("check_id", "").split(".")[-1]
+        per_rule_total[rule] += 1
+        m = TEST_RE.search(f.get("path", ""))
+        if not m:
+            off_benchmark += 1
+            continue
+        hits.add((m.group(1), rule))
+
+    # Category-matched scoring
+    cat_stats = defaultdict(lambda: {"tp": 0, "fp": 0, "fn": 0, "tn": 0})
+    rule_stats = defaultdict(lambda: {"tp": 0, "fp": 0})
+
+    flagged = defaultdict(set)  # test -> {categories flagged}
+    for test, rule in hits:
+        cat = RULE_CATEGORY.get(rule)
+        if cat is None:
+            continue
+        flagged[test].add(cat)
+        exp = expected.get(test)
+        if not exp or exp["category"] != cat:
+            continue
+        if exp["real"]:
+            rule_stats[rule]["tp"] += 1
+        else:
+            rule_stats[rule]["fp"] += 1
+
+    scored_cats = set(RULE_CATEGORY.values())
+    for test, exp in expected.items():
+        cat = exp["category"]
+        if cat not in scored_cats:
+            continue
+        did_flag = cat in flagged.get(test, set())
+        s = cat_stats[cat]
+        if exp["real"] and did_flag:
+            s["tp"] += 1
+        elif exp["real"] and not did_flag:
+            s["fn"] += 1
+        elif not exp["real"] and did_flag:
+            s["fp"] += 1
+        else:
+            s["tn"] += 1
+
+    def pct(n, d):
+        return f"{100.0 * n / d:5.1f}%" if d else "    -"
+
+    print(f"\n=== {Path(results_json).name} ===")
+    print(f"Total raw findings emitted: {len(findings)}")
+    print(f"Unique (testcase, rule) pairs: {len(hits)}")
+    print(f"Findings outside benchmark test files: {off_benchmark}\n")
+
+    print("--- Per-category (OWASP Benchmark scoring: CWE-matched) ---")
+    print(f"{'category':14} {'TP':>5} {'FP':>5} {'FN':>5} {'TN':>5} {'prec':>7} {'recall':>7} {'FP rate':>8} {'score':>7}")
+    tot = {"tp": 0, "fp": 0, "fn": 0, "tn": 0}
+    for cat in sorted(cat_stats):
+        s = cat_stats[cat]
+        for k in tot:
+            tot[k] += s[k]
+        tpr = s["tp"] / (s["tp"] + s["fn"]) if (s["tp"] + s["fn"]) else 0
+        fpr = s["fp"] / (s["fp"] + s["tn"]) if (s["fp"] + s["tn"]) else 0
+        print(f"{cat:14} {s['tp']:5} {s['fp']:5} {s['fn']:5} {s['tn']:5} "
+              f"{pct(s['tp'], s['tp'] + s['fp'])} {pct(s['tp'], s['tp'] + s['fn'])} "
+              f"{pct(s['fp'], s['fp'] + s['tn'])} {100 * (tpr - fpr):6.1f}")
+    tpr = tot["tp"] / (tot["tp"] + tot["fn"]) if (tot["tp"] + tot["fn"]) else 0
+    fpr = tot["fp"] / (tot["fp"] + tot["tn"]) if (tot["fp"] + tot["tn"]) else 0
+    print(f"{'TOTAL':14} {tot['tp']:5} {tot['fp']:5} {tot['fn']:5} {tot['tn']:5} "
+          f"{pct(tot['tp'], tot['tp'] + tot['fp'])} {pct(tot['tp'], tot['tp'] + tot['fn'])} "
+          f"{pct(tot['fp'], tot['fp'] + tot['tn'])} {100 * (tpr - fpr):6.1f}")
+
+    print("\n--- Per-rule (CWE-matched TP/FP) ---")
+    print(f"{'rule':38} {'TP':>5} {'FP':>5} {'prec':>7} {'raw findings':>13}")
+    for rule in sorted(per_rule_total, key=lambda r: -per_rule_total[r]):
+        s = rule_stats.get(rule, {"tp": 0, "fp": 0})
+        print(f"{rule:38} {s['tp']:5} {s['fp']:5} {pct(s['tp'], s['tp'] + s['fp'])} {per_rule_total[rule]:13}")
+
+
+if __name__ == "__main__":
+    main(sys.argv[1], sys.argv[2])