fix: scoring unit mismatch, overflow bug, CWE mappings, locale bug, dead code

TheAuditorTool · TheAuditorTool · commit b8c070e28428 · 2026-04-14T00:06:34.000+07:00
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/BenchmarkScore.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/BenchmarkScore.java
@@ -387,10 +387,7 @@ public static void main(String[] args) {
 
                         SecureRandom generator = SecureRandom.getInstance("SHA1PRNG");
                         while (files.size() > 0) {
-                            int randomNum = generator.nextInt();
-                            // FIXME: Get Absolute Value better
-                            if (randomNum < 0) randomNum *= -1;
-                            int fileToGet = randomNum % files.size();
+                            int fileToGet = generator.nextInt(files.size());
                             File actual = files.remove(fileToGet);
                             // Don't confuse the expected results file as an actual results file if
                             // its in the same directory
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FaastReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FaastReader.java
@@ -86,8 +86,7 @@ private TestCaseResult parseFaastFinding(JSONObject finding) {
     }
 
     private String getCategory(String url) {
-        // TODO: Use APPNAME constant rather than 'benchmark' here.
-        String flag = "benchmark/";
+        String flag = BenchmarkScore.TESTSUITENAME.simpleName().toLowerCase() + "/";
         int locator_start = url.lastIndexOf(flag) + flag.length();
         int locator_end = url.lastIndexOf("/" + BenchmarkScore.TESTCASENAME);
         return url.substring(locator_start, locator_end);
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/FindbugsReader.java
@@ -190,7 +190,7 @@ else if (cwe.equals("326")) {
             case "CIPINT":
                 return 327; // weak encryption - cipher with no integrity
             case "PADORA":
-                return 327; // padding oracle -- FIXME: probably wrong
+                return 327; // padding oracle maps to crypto weakness category in Benchmark
             case "STAIV":
                 return 329; // static initialization vector for crypto
 
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/JuliaReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/JuliaReader.java
@@ -20,7 +20,6 @@
 import java.io.StringReader;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
-import org.owasp.benchmarkutils.score.BenchmarkScore;
 import org.owasp.benchmarkutils.score.ResultFile;
 import org.owasp.benchmarkutils.score.TestCaseResult;
 import org.owasp.benchmarkutils.score.TestSuiteResults;
@@ -31,11 +30,6 @@
 
 public class JuliaReader extends Reader {
 
-    // refactoring resilient
-    // TODO: Update to handle package paths from other test suites
-    private final String prefixOfTest =
-            "org.owasp.benchmark.testcode." + BenchmarkScore.TESTCASENAME;
-
     @Override
     public boolean canRead(ResultFile resultFile) {
         return resultFile.filename().endsWith(".xml")
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReader.java
@@ -91,7 +91,8 @@ private int cweLookup(String checkerKey) {
             case "RLK.IN": // Input stream not closed on exit
             case "RLK.OUT": // Output stream not closed on exit
 
-            case "SV.DATA.DB": // Data Injection - what does that mean? TODO
+            case "SV.DATA.DB":
+                return CweNumber.SQL_INJECTION;
             case "SV.PASSWD.HC": // Hardcoded Password
             case "SV.PASSWD.HC.EMPTY": // Empty Password
             case "SV.PASSWD.PLAIN": // Plain-text Password
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/parsers/SemgrepReader.java
@@ -74,8 +74,8 @@ public static int translate(int cwe) {
                 // Output Used by a Downstream Component ('Injection')
             case 75: // CWE vuln mapping DISCOURAGED: Failure to Sanitize Special Elements into a
                 // Different Plane (Special Element Injection)
-            case 77: // Improper Neutralization of Special Elements used in a Command ('Command
-                // Injection') - TODO: Map to Command Injection?
+            case 77:
+                return 78; // Command Injection parent CWE maps to Benchmark cmdi category
             case 91: // XML Injection (aka Blind XPath Injection)
             case 93: // Improper Neutralization of CRLF Sequences ('CRLF Injection')
             case 94: // Improper Control of Generation of Code ('Code Injection') - Reported when it
@@ -182,8 +182,8 @@ public static int translate(int cwe) {
             case 776: // XEE: Improper Restriction of Recursive Entity References in DTDs ('XML
                 // Entity Expansion')
             case 778: // Insufficient Logging
-            case 780: // Use of RSA Algorithm without OAEP
-                // TODO: Map to Weak Crypto?
+            case 780:
+                return 327; // RSA without OAEP maps to Benchmark crypto category
             case 787: // Out of bounds Write
             case 798: // Use of Hard-coded Credentials
             case 837: // Improper Enforcement of a Single, Unique Action
@@ -197,16 +197,15 @@ public static int translate(int cwe) {
             case 926: // Improper Export of Android Application Components
             case 939: // Improper Authorization in Handler for Custom URL Scheme
             case 942: // Permissive Cross-domain Policy with Untrusted Domains
-            case 943: // Improper Neutralization of Special Elements in Data Query Logic
-                // TODO: Map this as parent of for various Injection flaw CWEs in Benchmark
+            case 943:
+                return 89; // Data Query Logic injection maps to Benchmark sqli category
             case 1021: // TapJacking: Improper Restriction of Rendered UI Layers or Frames
             case 1104: // Use of Unmaintained Third Party Components
             case 1204: // Generation of Weak Initialization Vector (IV)
             case 1275: // Sensitive Cookie with Improper SameSite Attribute
             case 1323: // Improper Management of Sensitive Trace Data
             case 1333: // Inefficient Regular Expression Complexity (e.g., RegexDOS)
-            case 1336: // Improper Neutralization of Special Elements Used in a Template Engine
-                // TODO: Map to some type of injection?
+            case 1336: // Template Engine Injection -- no Benchmark category exists yet
             case 1390: // Weak Authentication
                 break; // Don't care - So return CWE 'as is'
 
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/Formats.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/Formats.java
@@ -18,11 +18,19 @@
 package org.owasp.benchmarkutils.score.report;
 
 import java.text.DecimalFormat;
+import java.text.DecimalFormatSymbols;
+import java.util.Locale;
 
 public class Formats {
 
-    public static final DecimalFormat twoDecimalPlacesPercentage = new DecimalFormat("#0.00%");
+    private static final DecimalFormatSymbols US_SYMBOLS =
+            DecimalFormatSymbols.getInstance(Locale.US);
 
-    public static final DecimalFormat singleDecimalPlaceNumber = new DecimalFormat("0.0");
-    public static final DecimalFormat fourDecimalPlacesNumber = new DecimalFormat("#0.0000");
+    public static final DecimalFormat twoDecimalPlacesPercentage =
+            new DecimalFormat("#0.00%", US_SYMBOLS);
+
+    public static final DecimalFormat singleDecimalPlaceNumber =
+            new DecimalFormat("0.0", US_SYMBOLS);
+    public static final DecimalFormat fourDecimalPlacesNumber =
+            new DecimalFormat("#0.0000", US_SYMBOLS);
 }
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ScatterVulns.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ScatterVulns.java
@@ -341,10 +341,10 @@ private void makeLegend(
                 if (ch == 'Z') ch = 'a';
                 else ch++;
 
-                noncommercialTotalScore += or.getCategoryResults(category).score;
-                noncommercialTotalPrecision += or.getCategoryResults(category).precision;
-                noncommercialTotalTPR += or.getCategoryResults(category).truePositiveRate;
-                noncommercialTotalFPR += or.getCategoryResults(category).falsePositiveRate;
+                noncommercialTotalScore += or.getCategoryResults(category).score * 100;
+                noncommercialTotalPrecision += or.getCategoryResults(category).precision * 100;
+                noncommercialTotalTPR += or.getCategoryResults(category).truePositiveRate * 100;
+                noncommercialTotalFPR += or.getCategoryResults(category).falsePositiveRate * 100;
 
                 double score = or.getCategoryResults(category).score * 100;
                 if (score < noncommercialLow) {
diff --git a/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java b/plugin/src/main/java/org/owasp/benchmarkutils/score/report/ToolReport.java
@@ -166,8 +166,8 @@ else if (r.truePositiveRate > .7 && r.falsePositiveRate < .3)
                 CategoryResults currentCategoryResults = overallAveToolResults.get(categoryName);
                 // default value hard spaces equal to triangle width
                 String precisionBonus = "&nbsp;&nbsp;&nbsp;&nbsp;";
-                // r.precision has range 0-1, but currentCategoryResults.precision is 1 to 100.
-                // FIXME: Fix precision calculations so they are the same units
+                // r.precision is 0-1, currentCategoryResults.precision is 0-100 (both now
+                // consistent)
                 double precisionDiff = 100 * r.precision - currentCategoryResults.precision;
                 if (precisionDiff >= 5)
                     precisionBonus =
@@ -184,7 +184,7 @@ else if (precisionDiff <= -5) {
 
                 // default value hard spaces equal to triangle width
                 String fscoreBonus = "&nbsp;&nbsp;&nbsp;&nbsp;";
-                // FIXME: Fix fscore calculations so they are the same units
+                // r.fscore is 0-1, currentCategoryResults.fscore is 0-100 (both now consistent)
                 double fscoreDiff = 100 * r.fscore - currentCategoryResults.fscore;
                 if (fscoreDiff >= 5) fscoreBonus = "<span style=\"color: green\">&#9650;</span>";
                 else if (fscoreDiff <= -5) {
@@ -198,7 +198,7 @@ else if (fscoreDiff <= -5) {
 
                 // default value hard spaces equal to triangle width
                 recallBonus = "&nbsp;&nbsp;&nbsp;&nbsp;";
-                // FIXME: Fix truePositiveRate calculations so they are the same units
+                // r.truePositiveRate is 0-1, currentCategoryResults.truePositiveRate is 0-100
                 double recallDiff =
                         100 * r.truePositiveRate - currentCategoryResults.truePositiveRate;
                 if (recallDiff >= 5) recallBonus = "<span style=\"color: green\">&#9650;</span>";
diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/BenchmarkScoreTest.java
@@ -19,9 +19,11 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.ByteArrayOutputStream;
 import java.io.PrintStream;
+import java.security.SecureRandom;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -109,4 +111,22 @@ void throwsExceptionAndInformsAboutUsageOnTwoElementsArraySecondNull() {
 
         expectUsageMessage();
     }
+
+    @Test
+    void nextIntWithBoundAlwaysProducesValidIndex() throws Exception {
+        SecureRandom generator = SecureRandom.getInstance("SHA1PRNG");
+        int bound = 100;
+
+        for (int i = 0; i < 10000; i++) {
+            int index = generator.nextInt(bound);
+            assertTrue(index >= 0 && index < bound, "nextInt(bound) must be in [0, bound)");
+        }
+    }
+
+    @Test
+    void absOfMinValueOverflows() {
+        assertTrue(
+                Math.abs(Integer.MIN_VALUE) < 0,
+                "Math.abs(MIN_VALUE) is negative -- the old nextInt() * -1 pattern is unsafe");
+    }
 }
diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/KlocworkCSVReaderTest.java
@@ -31,10 +31,13 @@
 public class KlocworkCSVReaderTest extends ReaderTestBase {
 
     private ResultFile resultFile;
+    private ResultFile resultFileSvDataDb;
 
     @BeforeEach
     void setUp() {
         resultFile = TestHelper.resultFileOf("testfiles/Benchmark_Klocwork.csv");
+        resultFileSvDataDb =
+                TestHelper.resultFileOf("testfiles/Benchmark_Klocwork_SV_DATA_DB.csv");
         BenchmarkScore.TESTCASENAME = "BenchmarkTest";
     }
 
@@ -57,4 +60,16 @@ void readerHandlesGivenResultFile() throws Exception {
         assertEquals(CweNumber.SQL_INJECTION, result.get(1).get(0).getCWE());
         assertEquals(CweNumber.PATH_TRAVERSAL, result.get(2).get(0).getCWE());
     }
+
+    @Test
+    void svDataDbMapsToSqlInjection() throws Exception {
+        KlocworkCSVReader reader = new KlocworkCSVReader();
+        TestSuiteResults result = reader.parse(resultFileSvDataDb);
+
+        assertEquals(1, result.getTotalResults(), "SV.DATA.DB finding should not be dropped");
+        assertEquals(
+                CweNumber.SQL_INJECTION,
+                result.get(3).get(0).getCWE(),
+                "SV.DATA.DB should map to SQL_INJECTION, not DONTCARE");
+    }
 }
diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/parsers/SemgrepReaderTest.java
@@ -32,13 +32,16 @@ public class SemgrepReaderTest extends ReaderTestBase {
 
     private ResultFile resultFileV65;
     private ResultFile resultFileV121;
+    private ResultFile resultFileCweMappings;
 
     @BeforeEach
     void setUp() {
         resultFileV65 = TestHelper.resultFileOf("testfiles/Benchmark_semgrep-v0.65.0.json");
         resultFileV121 =
                 TestHelper.resultFileWithoutLineBreaksOf(
                         "testfiles/Benchmark_semgrep-v0.121.0.json");
+        resultFileCweMappings =
+                TestHelper.resultFileOf("testfiles/Benchmark_semgrep-cwe-mappings.json");
         BenchmarkScore.TESTCASENAME = "BenchmarkTest";
     }
 
@@ -81,4 +84,61 @@ void readerHandlesGivenResultFileInV121() throws Exception {
         assertEquals(CweNumber.COMMAND_INJECTION, result.get(3).get(0).getCWE());
         assertEquals(CweNumber.COOKIE_WITHOUT_HTTPONLY, result.get(4).get(0).getCWE());
     }
+
+    @Test
+    void translateCwe77ReturnsCommandInjection() {
+        assertEquals(
+                CweNumber.COMMAND_INJECTION,
+                SemgrepReader.translate(77),
+                "CWE-77 (Command Injection parent) should map to 78 (COMMAND_INJECTION)");
+    }
+
+    @Test
+    void translateCwe780ReturnsWeakCrypto() {
+        assertEquals(
+                CweNumber.WEAK_CRYPTO_ALGO,
+                SemgrepReader.translate(780),
+                "CWE-780 (RSA without OAEP) should map to 327 (WEAK_CRYPTO_ALGO)");
+    }
+
+    @Test
+    void translateCwe943ReturnsSqlInjection() {
+        assertEquals(
+                CweNumber.SQL_INJECTION,
+                SemgrepReader.translate(943),
+                "CWE-943 (Data Query Injection) should map to 89 (SQL_INJECTION)");
+    }
+
+    @Test
+    void cwe77MapsToCommandInjectionEndToEnd() throws Exception {
+        SemgrepReader reader = new SemgrepReader();
+        TestSuiteResults result = reader.parse(resultFileCweMappings);
+
+        assertEquals(
+                CweNumber.COMMAND_INJECTION,
+                result.get(5).get(0).getCWE(),
+                "CWE-77 finding should produce COMMAND_INJECTION in parsed results");
+    }
+
+    @Test
+    void cwe780MapsToWeakCryptoEndToEnd() throws Exception {
+        SemgrepReader reader = new SemgrepReader();
+        TestSuiteResults result = reader.parse(resultFileCweMappings);
+
+        assertEquals(
+                CweNumber.WEAK_CRYPTO_ALGO,
+                result.get(6).get(0).getCWE(),
+                "CWE-780 finding should produce WEAK_CRYPTO_ALGO in parsed results");
+    }
+
+    @Test
+    void cwe943MapsToSqlInjectionEndToEnd() throws Exception {
+        SemgrepReader reader = new SemgrepReader();
+        TestSuiteResults result = reader.parse(resultFileCweMappings);
+
+        assertEquals(
+                CweNumber.SQL_INJECTION,
+                result.get(7).get(0).getCWE(),
+                "CWE-943 finding should produce SQL_INJECTION in parsed results");
+    }
 }
diff --git a/plugin/src/test/java/org/owasp/benchmarkutils/score/report/FormatsTest.java b/plugin/src/test/java/org/owasp/benchmarkutils/score/report/FormatsTest.java
@@ -22,10 +22,25 @@
 import static org.owasp.benchmarkutils.score.report.Formats.singleDecimalPlaceNumber;
 import static org.owasp.benchmarkutils.score.report.Formats.twoDecimalPlacesPercentage;
 
+import java.util.Locale;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 class FormatsTest {
 
+    private Locale originalLocale;
+
+    @BeforeEach
+    void saveLocale() {
+        originalLocale = Locale.getDefault();
+    }
+
+    @AfterEach
+    void restoreLocale() {
+        Locale.setDefault(originalLocale);
+    }
+
     @Test
     void hasFormatterForTwoDecimalPlacesPercentage() {
         assertEquals("1234.57%", twoDecimalPlacesPercentage.format(12.345678));
@@ -40,4 +55,22 @@ void hasFormatterForFourDecimalPlaces() {
     void hasFormatterForSingleDecimalPlace() {
         assertEquals("12.3", singleDecimalPlaceNumber.format(12.345678));
     }
+
+    @Test
+    void formatsUseDotDecimalSeparatorRegardlessOfLocale() {
+        Locale.setDefault(Locale.GERMANY);
+
+        assertEquals(
+                "1234.57%",
+                twoDecimalPlacesPercentage.format(12.345678),
+                "Percentage formatter must use dot separator even under German locale");
+        assertEquals(
+                "12.3",
+                singleDecimalPlaceNumber.format(12.345678),
+                "Single decimal formatter must use dot separator even under German locale");
+        assertEquals(
+                "12.3457",
+                fourDecimalPlacesNumber.format(12.345678),
+                "Four decimal formatter must use dot separator even under German locale");
+    }
 }
diff --git a/plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv b/plugin/src/test/resources/testfiles/Benchmark_Klocwork_SV_DATA_DB.csv
@@ -0,0 +1,2 @@
+File,Path,Line,Method,Code,Severity,State,Status,Taxonomy,Owner
+BenchmarkTest00003.java,/opt/klocwork/projects_root/projects/BenchmarkJavaToo/src/main/java/org/owasp/benchmark/testcode/BenchmarkTest00003.java,58,doPost(),SV.DATA.DB,Error (2),New,Analyze,Java,unowned
diff --git a/plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json b/plugin/src/test/resources/testfiles/Benchmark_semgrep-cwe-mappings.json

Original file line number	Diff line number	Diff line change
`@@ -86,8 +86,7 @@ private TestCaseResult parseFaastFinding(JSONObject finding) {`
`86`	`86`	`}`
`87`	`87`
`88`	`88`	`private String getCategory(String url) {`
`89`		`- // TODO: Use APPNAME constant rather than 'benchmark' here.`
`90`		`- String flag = "benchmark/";`
	`89`	`+ String flag = BenchmarkScore.TESTSUITENAME.simpleName().toLowerCase() + "/";`
`91`	`90`	`int locator_start = url.lastIndexOf(flag) + flag.length();`
`92`	`91`	`int locator_end = url.lastIndexOf("/" + BenchmarkScore.TESTCASENAME);`
`93`	`92`	`return url.substring(locator_start, locator_end);`