Fix first instance rule being used as rule description for all violations of that rule and other SARIF improvements

AUTHORS

@@ -405,6 +405,7 @@ Tomo Dote
 Toralf Förster
 Troshin V.S.
 Tyson Nottingham
+Usman Majid
 Valentin Batz
 Valerii Lashmanov
 Vasily Maslyukov

Makefile

@@ -171,7 +171,7 @@ ifndef INCLUDE_FOR_CLI
 endif
 
 ifndef INCLUDE_FOR_TEST
-    INCLUDE_FOR_TEST=-Ilib -Ifrontend -Icli -isystem externals/simplecpp -isystem externals/tinyxml2
+    INCLUDE_FOR_TEST=-Ilib -Ifrontend -Icli -isystem externals/simplecpp -isystem externals/tinyxml2 -isystem externals/picojson
 endif
 
 BIN=$(DESTDIR)$(PREFIX)/bin
@@ -316,6 +316,7 @@ TESTOBJ =     test/fixture.o \
               test/testpreprocessor.o \
               test/testprocessexecutor.o \
               test/testprogrammemory.o \
+              test/testsarif.o \
               test/testsettings.o \
               test/testsimplifytemplate.o \
               test/testsimplifytokens.o \
@@ -834,6 +835,9 @@ test/testprocessexecutor.o: test/testprocessexecutor.cpp cli/executor.h cli/proc
 test/testprogrammemory.o: test/testprogrammemory.cpp lib/addoninfo.h lib/check.h lib/checkers.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/programmemory.h lib/settings.h lib/standards.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h test/helpers.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testprogrammemory.cpp
 
+test/testsarif.o: test/testsarif.cpp lib/config.h lib/json.h test/fixture.h test/helpers.h
+	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testsarif.cpp
+
 test/testsettings.o: test/testsettings.cpp lib/addoninfo.h lib/check.h lib/checkers.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/tokenize.h lib/tokenlist.h lib/utils.h test/fixture.h test/helpers.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testsettings.cpp
 

cli/cppcheckexecutor.cpp

@@ -92,33 +92,65 @@ namespace {
                 if (finding.callStack.empty())
                     continue;
                 if (ruleIds.insert(finding.id).second) {
+                    // setting name and description to empty strings will make github default
+                    // to the instance specific violation message and not rule description,
+                    // this makes it so not all the violations have the same description.
                     picojson::object rule;
                     rule["id"] = picojson::value(finding.id);
+                    // rule.name
+                    rule["name"] = picojson::value("");
                     // rule.shortDescription.text
                     picojson::object shortDescription;
-                    shortDescription["text"] = picojson::value(finding.shortMessage());
+                    shortDescription["text"] = picojson::value("");
                     rule["shortDescription"] = picojson::value(shortDescription);
                     // rule.fullDescription.text
                     picojson::object fullDescription;
-                    fullDescription["text"] = picojson::value(finding.verboseMessage());
+                    fullDescription["text"] = picojson::value("");
                     rule["fullDescription"] = picojson::value(fullDescription);
                     // rule.help.text
                     picojson::object help;
-                    help["text"] = picojson::value(finding.verboseMessage()); // FIXME provide proper help text
+                    help["text"] = picojson::value("");
                     rule["help"] = picojson::value(help);
                     // rule.properties.precision, rule.properties.problem.severity
                     picojson::object properties;
                     properties["precision"] = picojson::value(sarifPrecision(finding));
-                    const char* securitySeverity = nullptr;
-                    if (finding.severity == Severity::error && !ErrorLogger::isCriticalErrorId(finding.id))
-                        securitySeverity = "9.9"; // We see undefined behavior
-                    //else if (finding.severity == Severity::warning)
-                    //    securitySeverity = 5.1; // We see potential undefined behavior
-                    if (securitySeverity) {
-                        properties["security-severity"] = picojson::value(securitySeverity);
-                        const picojson::array tags{picojson::value("security")};
+                    // rule.properties.security-severity, rule.properties.tags
+                    picojson::array tags;
+
+                    // If we have a CWE ID, treat it as security-related (CWE is the authoritative source for security weaknesses)
+                    if (finding.cwe.id > 0) {
+                        double securitySeverity = 0;
+                        if (finding.severity == Severity::error && !ErrorLogger::isCriticalErrorId(finding.id)) {
+                            securitySeverity = 9.9; // critical = 9.0+
+                        }
+                        else if (finding.severity == Severity::warning) {
+                            securitySeverity = 8.5; // high = 7.0 to 8.9
+                        }
+                        else if (finding.severity == Severity::performance || finding.severity == Severity::portability ||
+                                 finding.severity == Severity::style) {
+                            securitySeverity = 5.5; // medium = 4.0 to 6.9
+                        }
+                        else if (finding.severity == Severity::information || finding.severity == Severity::internal ||
+                                 finding.severity == Severity::debug || finding.severity == Severity::none) {
+                            securitySeverity = 2.0; // low = 0.1 to 3.9
+                        }
+                        if (securitySeverity > 0.0) {
+                            std::ostringstream ss;
+                            ss << securitySeverity;
+                            properties["security-severity"] = picojson::value(ss.str());
+                            tags.emplace_back("external/cwe/cwe-" + std::to_string(finding.cwe.id));
+                            tags.emplace_back("security");
+                        }
+                    }
+
+                    // Add tags array if it has any content
+                    if (!tags.empty()) {
                         properties["tags"] = picojson::value(tags);
                     }
+
+                    // Set problem.severity for use with github
+                    const std::string problemSeverity = sarifSeverity(finding);
+                    properties["problem.severity"] = picojson::value(problemSeverity);
                     rule["properties"] = picojson::value(properties);
                     // rule.defaultConfiguration.level
                     picojson::object defaultConfiguration;
@@ -198,14 +230,15 @@ namespace {
 
             return picojson::value(doc).serialize(true);
         }
-    private:
 
+    private:
         static std::string sarifSeverity(const ErrorMessage& errmsg) {
             if (ErrorLogger::isCriticalErrorId(errmsg.id))
                 return "error";
             switch (errmsg.severity) {
             case Severity::error:
             case Severity::warning:
+                return "error";
             case Severity::style:
             case Severity::portability:
             case Severity::performance:
@@ -672,19 +705,21 @@ void StdLogger::reportErr(const ErrorMessage &msg)
                                      mGuidelineMapping, msgCopy.severity);
     msgCopy.classification = getClassification(msgCopy.guideline, mSettings.reportType);
 
-    // TODO: there should be no need for verbose and default messages here
-    const std::string msgStr = msgCopy.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation);
+    if (mSettings.outputFormat == Settings::OutputFormat::sarif) {
+        mSarifReport.addFinding(std::move(msgCopy));
+    } else {
+        // TODO: there should be no need for verbose and default messages here
+        const std::string msgStr = msgCopy.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation);
 
-    // Alert only about unique errors
-    if (!mSettings.emitDuplicates && !mShownErrors.insert(msgStr).second)
-        return;
+        // Alert only about unique errors
+        if (!mSettings.emitDuplicates && !mShownErrors.insert(msgStr).second)
+            return;
 
-    if (mSettings.outputFormat == Settings::OutputFormat::sarif)
-        mSarifReport.addFinding(std::move(msgCopy));
-    else if (mSettings.outputFormat == Settings::OutputFormat::xml)
-        reportErr(msgCopy.toXML());
-    else
-        reportErr(msgStr);
+        if (mSettings.outputFormat == Settings::OutputFormat::xml)
+            reportErr(msgCopy.toXML());
+        else
+            reportErr(msgStr);
+    }
 }
 
 /**

runformat

@@ -14,8 +14,8 @@
 UNCRUSTIFY_VERSION="0.72.0"
 UNCRUSTIFY="${UNCRUSTIFY-uncrustify}"
 
-DETECTED_VERSION=$("$UNCRUSTIFY" --version 2>&1 | grep -o -E '[0-9.]+')
-if [ "$DETECTED_VERSION" != "${UNCRUSTIFY_VERSION}" ]; then
+DETECTED_VERSION=$("$UNCRUSTIFY" --version 2>&1 | grep -o -E '[0-9]+\.[0-9]+\.[0-9]+')
+if [[ "$DETECTED_VERSION" != "${UNCRUSTIFY_VERSION}" ]]; then
   echo "You should use version: ${UNCRUSTIFY_VERSION}"
   echo "Detected version: ${DETECTED_VERSION}"
   exit 1

test/CMakeLists.txt

@@ -20,6 +20,7 @@ if (BUILD_TESTS)
         target_include_directories(testrunner SYSTEM PRIVATE ${tinyxml2_INCLUDE_DIRS})
     endif()
     target_externals_include_directories(testrunner PRIVATE ${PROJECT_SOURCE_DIR}/externals/simplecpp/)
+    target_externals_include_directories(testrunner PRIVATE ${PROJECT_SOURCE_DIR}/externals/picojson/)
     if (Boost_FOUND)
         target_include_directories(testrunner SYSTEM PRIVATE ${Boost_INCLUDE_DIRS})
     endif()

test/cli/helloworld_test.py

@@ -377,6 +377,9 @@ def test_sarif():
     assert 'security' in res['runs'][0]['tool']['driver']['rules'][0]['properties']['tags']
     assert re.match(r'[0-9]+(.[0-9]+)+', res['runs'][0]['tool']['driver']['semanticVersion'])
     assert 'level' in res['runs'][0]['tool']['driver']['rules'][0]['defaultConfiguration'] # #13885
+    assert res['runs'][0]['tool']['driver']['rules'][0]['shortDescription']['text'] == ''
+    assert res['runs'][0]['results'][0]['message']['text'] == 'Division by zero.'
+    assert res['runs'][0]['tool']['driver']['rules'][0]['properties']['problem.severity'] == 'error'
 
 
 def test_xml_checkers_report():