diff --git a/.github/workflows/sigrid-pullrequest.yml b/.github/workflows/sigrid-pullrequest.yml index a1c3c15d..918cdf3f 100644 --- a/.github/workflows/sigrid-pullrequest.yml +++ b/.github/workflows/sigrid-pullrequest.yml @@ -15,7 +15,7 @@ jobs: - name: "Run Sigrid CI" env: SIGRID_CI_TOKEN: "${{ secrets.SIGRID_CI_TOKEN }}" - run: "./sigridci/sigridci.py --customer sig --system sigridci-client --source . --capability maintainability,osh" + run: "./sigridci/sigridci.py --customer sig --system sigridci-client --source . --capability maintainability,osh,security" - name: "Save Sigrid CI results" if: always() uses: actions/upload-artifact@v4 @@ -44,6 +44,6 @@ jobs: with: customer: "sig" system: "sigridci-client" - capability: "maintainability,osh" + capability: "maintainability,osh,security" env: SIGRID_CI_TOKEN: "${{ secrets.SIGRID_CI_TOKEN }}" diff --git a/docs/images/ci/security-feedback.png b/docs/images/ci/security-feedback.png new file mode 100644 index 00000000..965eff81 Binary files /dev/null and b/docs/images/ci/security-feedback.png differ diff --git a/docs/reference/client-script-usage.md b/docs/reference/client-script-usage.md index b51ca357..d9beedb5 100644 --- a/docs/reference/client-script-usage.md +++ b/docs/reference/client-script-usage.md @@ -28,7 +28,7 @@ The script takes a limited number of mandatory arguments. However, Sigrid CI's b | `--system` | Yes | examplesystemname | Name of your system in Sigrid. Contact SIG support if you are not sure about this. [2] | | `--subsystem ` | No | frontend | Used to map between repository directory structure versus the one known by Sigrid. [5] | | `--source` | No | . | Path of your project's source code. Use "." for current directory. | -| `--capability` | No | maintainability | Comma-separated list of Sigrid capabilities (`maintainability,osh`). Default is maintainability. | +| `--capability` | No | maintainability | Comma-separated list of Sigrid capabilities (`maintainability,osh,security`). Default is maintainability. | | `--publish` | No | N/A | Automatically publishes analysis results to Sigrid. [1] | | `--publishonly` | No | N/A | Publishes analysis results to Sigrid, but *does not* provide feedback in the CI environment itself. [3] | | `--exclude` | No | /build/,.png | Comma-separated list of file and/or directory names that should be excluded from the upload. [4, 7] | diff --git a/docs/sigridci-integration/using-sigridci.md b/docs/sigridci-integration/using-sigridci.md index 7b0178c2..0d630e82 100644 --- a/docs/sigridci-integration/using-sigridci.md +++ b/docs/sigridci-integration/using-sigridci.md @@ -98,6 +98,37 @@ Sigrid CI separates vulnerable open source libraries into two categories: line `message-path: sigrid-ci-output/feedback.md`, and change this to `message-path: sigrid-ci-output/*feedback.md`. Adding the asterisk allows you to get feedback on *all* Sigrid capabilities, not just maintainability. +### Security feedback (Beta) + +Sigrid CI provides security feedback based on your [objectives](../capabilities/portfolio-objectives.md). +As with [open source vulnerabilities](#open-source-health-feedback-beta), you do need to fix every single +security finding that does not meet your objective. + + + +When you encounter security findings during code reviews, there are three ways how you can deal with them: + +- **Address the finding in the pull request:** It's always the best course of action to just address the finding + within the pull request. This prevents the finding from ever going into the main/master branch, which is generally + considered a best practice in [shift-left thinking](https://en.wikipedia.org/wiki/Shift-left_testing). +- **Merge the pull request, manage the finding via Sigrid:** In some cases, the pull request author and reviewer might + agree it's not feasible to address the finding right now. In those situations, it's OK to merge the pull request. + This will cause the security finding to appear in Sigrid's + [Security dashboard](../capabilities/portfolio-security.md), where it can be tracked. +- **Merge the pull request, mark the finding as a false positive in Sigrid:** Like any automated check, Sigrid can + produce findings that are false positives. In those situations, if the pull request author and reviewer agree the + finding is *actually* a false positive, it's OK to merge the pull request. You can then mark the finding as a false + positive in Sigrid's [security page](../capabilities/system-security.md). False positives are automatically excluded + from future Sigrid CI feedback. + +#### Adding Security feedback to an existing Sigrid CI configuration + +- **All platforms:** You need to add the option `--capability maintainability,osh,security` to the Sigrid CI step in + your pipeline configuration. +- **GitHub:** In addition to the above, you need one extra step: In your pipeline configuration, look for the + line `message-path: sigrid-ci-output/feedback.md`, and change this to `message-path: sigrid-ci-output/*feedback.md`. + Adding the asterisk allows you to get feedback on *all* Sigrid capabilities, not just maintainability. + ## How do you deal with feedback from Sigrid CI? Feedback from Sigrid CI is intended to be used in the context of a diff --git a/sigridci/sigridci.py b/sigridci/sigridci.py index b6a823ff..b208e476 100755 --- a/sigridci/sigridci.py +++ b/sigridci/sigridci.py @@ -18,7 +18,7 @@ import sys from argparse import ArgumentParser, SUPPRESS -from sigridci.capability import MAINTAINABILITY, OPEN_SOURCE_HEALTH +from sigridci.capability import CAPABILITY_SHORT_NAMES from sigridci.publish_options import PublishOptions, RunMode from sigridci.sigrid_api_client import SigridApiClient from sigridci.platform import Platform @@ -26,9 +26,6 @@ from sigridci.upload_log import UploadLog -CAPABILITIES = {cap.shortName: cap for cap in [MAINTAINABILITY, OPEN_SOURCE_HEALTH]} - - def parsePublishOptions(args): return PublishOptions( partner=args.partner.lower(), @@ -66,7 +63,7 @@ def parseTarget(target): def parseCapabilities(names): try: - return [CAPABILITIES[name.lower().strip()] for name in names.split(",")] + return [CAPABILITY_SHORT_NAMES[name.lower().strip()] for name in names.split(",")] except KeyError as e: print(f"Invalid value for --capability: {str(e)}") sys.exit(1) @@ -81,7 +78,8 @@ def parseCapabilities(names): parser.add_argument("--subsystem", type=str, default="", help="Publishes your code as a subsystem within a Sigrid system.") parser.add_argument("--convert", type=str, default="", help="Code conversion for specific technologies") parser.add_argument("--source", type=str, required=True, help="Path of your project's source code.") - parser.add_argument("--capability", type=str, default="maintainability", help=f"Comma-separated Sigrid capabilities ({','.join(CAPABILITIES.keys())}).") + parser.add_argument("--capability", type=str, default="maintainability", + help=f"Comma-separated Sigrid capabilities ({','.join(CAPABILITY_SHORT_NAMES.keys())}).") parser.add_argument("--publish", action="store_true", help="Publishes analysis results to Sigrid.") parser.add_argument("--publishonly", action="store_true", help="Only publishes to Sigrid without waiting for results.") parser.add_argument("--exclude", type=str, default="", help="Comma-separated list of files/directories to exclude.") diff --git a/sigridci/sigridci/analysisresults/findings_processor.py b/sigridci/sigridci/analysisresults/findings_processor.py new file mode 100644 index 00000000..24ca6b20 --- /dev/null +++ b/sigridci/sigridci/analysisresults/findings_processor.py @@ -0,0 +1,76 @@ +# Copyright Software Improvement Group +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from dataclasses import dataclass + +from ..objective import Objective + + +@dataclass +class Finding: + fingerprint: str + risk: str + description: str + file: str + line: int + partOfObjective: bool + + +class FindingsProcessor: + def extractFindings(self, feedback, objective): + if feedback is None: + return [] + elif "runs" in feedback: + sarifProcessor = SarifProcessor() + return list(sarifProcessor.extractFindings(feedback, objective)) + else: + sigridFindingsProcessor = SigridFindingsProcessor() + return list(sigridFindingsProcessor.extractFindings(feedback, objective)) + + +class SarifProcessor: + def extractFindings(self, feedback, objective): + rules = list(self.getRules(feedback)) + + for run in feedback["runs"]: + for result in run.get("results", []): + fingerprint = result["fingerprints"]["sigFingerprint/v1"] + risk = self.getFindingSeverity(result, rules) + file = result["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] + line = result["locations"][0]["physicalLocation"]["region"]["startLine"] + partOfObjective = Objective.isFindingIncluded(risk, objective) + yield Finding(fingerprint, risk, result["message"]["text"], file, line, partOfObjective) + + def getRules(self, feedback): + for run in feedback["runs"]: + for rule in run.get("rules", []): + properties = rule.get("properties", {}) + if properties.get("severity"): + yield rule + + def getFindingSeverity(self, result, rules): + severity = result.get("properties", {}).get("severity") + if not severity: + for rule in rules: + if rule["id"] == result["ruleId"]: + severity = rule["properties"]["severity"].replace("ERROR", "HIGH").replace("WARNING", "MEDIUM") + return severity.upper() if severity else "UNKNOWN" + + +class SigridFindingsProcessor: + def extractFindings(self, feedback, objective): + for finding in feedback: + partOfObjective = Objective.isFindingIncluded(finding["severity"], objective) + yield Finding(finding["id"], finding["severity"], finding["type"], + finding["filePath"], finding["startLine"], partOfObjective) diff --git a/sigridci/sigridci/capability.py b/sigridci/sigridci/capability.py index 146bcd69..e703d81a 100644 --- a/sigridci/sigridci/capability.py +++ b/sigridci/sigridci/capability.py @@ -27,3 +27,5 @@ class Capability: MAINTAINABILITY = Capability("MAINTAINABILITY", "Maintainability", "maintainability", 2, False) OPEN_SOURCE_HEALTH = Capability("OPEN_SOURCE_HEALTH", "Open Source Health", "osh", 4, True) SECURITY = Capability("SECURITY", "Security", "security", 8, True) + +CAPABILITY_SHORT_NAMES = {cap.shortName: cap for cap in [MAINTAINABILITY, OPEN_SOURCE_HEALTH, SECURITY]} diff --git a/sigridci/sigridci/feedback_provider.py b/sigridci/sigridci/feedback_provider.py index cffa894d..c88d3274 100644 --- a/sigridci/sigridci/feedback_provider.py +++ b/sigridci/sigridci/feedback_provider.py @@ -26,6 +26,7 @@ from .reports.osh_text_report import OpenSourceHealthTextReport from .reports.pipeline_summary_report import PipelineSummaryReport from .reports.security_markdown_report import SecurityMarkdownReport +from .reports.security_text_report import SecurityTextReport from .reports.static_html_report import StaticHtmlReport @@ -96,5 +97,7 @@ def prepareAdditionalReports(self, markdownReport): reports += [AsciiArtReport(), JUnitFormatReport(), StaticHtmlReport(self.objective)] elif self.capability == OPEN_SOURCE_HEALTH: reports += [OpenSourceHealthTextReport(self.objective)] + elif self.capability == SECURITY: + reports += [SecurityTextReport(self.objective)] reports.append(PipelineSummaryReport(markdownReport)) return reports diff --git a/sigridci/sigridci/platform.py b/sigridci/sigridci/platform.py index 5111eaee..cfefb474 100644 --- a/sigridci/sigridci/platform.py +++ b/sigridci/sigridci/platform.py @@ -16,6 +16,13 @@ import sys +DOCS_URL = f"https://docs.sigrid-says.com" +SCOPE_DOCS = f"{DOCS_URL}/reference/analysis-scope-configuration.html" +OSH_EXCLUDE_DOCS = f"{SCOPE_DOCS}#exclude-open-source-health-risks" +SECURITY_EXCLUDE_RULE_DOCS = f"{SCOPE_DOCS}#excluding-security-rules" +SECURITY_EXCLUDE_FILE_DOCS = f"{SCOPE_DOCS}#excluding-files-and-directories-from-security-scanning" + + class Platform: @staticmethod def isGitHub(): diff --git a/sigridci/sigridci/reports/osh_markdown_report.py b/sigridci/sigridci/reports/osh_markdown_report.py index c84da48b..96216f5e 100644 --- a/sigridci/sigridci/reports/osh_markdown_report.py +++ b/sigridci/sigridci/reports/osh_markdown_report.py @@ -19,13 +19,13 @@ from ..analysisresults.cyclonedx_processor import CycloneDXProcessor from ..capability import OPEN_SOURCE_HEALTH from ..objective import Objective +from ..platform import OSH_EXCLUDE_DOCS class OpenSourceHealthMarkdownReport(Report, MarkdownRenderer): MAX_FINDINGS = SecurityMarkdownReport.MAX_FINDINGS SYMBOLS = SecurityMarkdownReport.SEVERITY_SYMBOLS SORT_RISK = list(SecurityMarkdownReport.SEVERITY_SYMBOLS.keys()) - DOCS_LINK = "https://docs.sigrid-says.com/reference/analysis-scope-configuration.html#exclude-open-source-health-risks" def __init__(self, objective = "HIGH"): super().__init__() @@ -56,7 +56,7 @@ def renderMarkdown(self, analysisId, feedback, options): details += "> Consider upgrading to a version that no longer contains the vulnerability.\n\n" details += self.generateFindingsTable(fixable, options) details += "If you believe these findings are false positives, " - details += f"you can [exclude them in the Sigrid configuration]({self.DOCS_LINK}).\n\n" + details += f"you can [exclude them in the Sigrid configuration]({OSH_EXCLUDE_DOCS}).\n\n" if len(unfixable) > 0: details += "## 😑 You have findings that you need to investigate in more depth\n\n" details += f"> You have **{len(unfixable)}** vulnerable open source libraries without a fix available. \n" @@ -84,7 +84,7 @@ def generateFindingsTable(self, libraries, options): for library in sorted(libraries, key=lambda lib: self.SORT_RISK.index(lib.risk))[0:self.MAX_FINDINGS]: symbol = self.SYMBOLS[library.risk] - check = "✅" if Objective.isFindingIncluded(library.risk, self.objective) else "-" + check = "✅" if library.partOfObjective else "-" locations = "
".join(self.decorateLink(options, file, file) for file in library.files) md += f"| {symbol} | {check} | {library.name} {library.version} | {library.latestVersion} | {locations} |\n" diff --git a/sigridci/sigridci/reports/security_markdown_report.py b/sigridci/sigridci/reports/security_markdown_report.py index 452faf51..0dd8c7e5 100644 --- a/sigridci/sigridci/reports/security_markdown_report.py +++ b/sigridci/sigridci/reports/security_markdown_report.py @@ -15,8 +15,10 @@ import os from .report import Report, MarkdownRenderer +from ..analysisresults.findings_processor import FindingsProcessor from ..capability import SECURITY from ..objective import Objective +from ..platform import SECURITY_EXCLUDE_RULE_DOCS, SECURITY_EXCLUDE_FILE_DOCS class SecurityMarkdownReport(Report, MarkdownRenderer): @@ -34,24 +36,31 @@ def __init__(self, objective = "HIGH"): super().__init__() self.objective = objective self.previousFeedback = None + self.processor = FindingsProcessor() def generate(self, analysisId, feedback, options): with open(self.getMarkdownFile(options), "w", encoding="utf-8") as f: f.write(self.renderMarkdown(analysisId, feedback, options)) def renderMarkdown(self, analysisId, feedback, options): - rules = list(self.getRules(feedback)) - introduced = list(self.getIntroducedFindings(feedback, rules)) - fixed = list(self.getFixedFindings(feedback)) + findings = self.processor.extractFindings(feedback, self.objective) + previousFindings = self.processor.extractFindings(self.previousFeedback, self.objective) + + introduced = list(self.getIntroducedFindings(findings, previousFindings)) + fixed = list(self.getFixedFindings(findings, previousFindings)) details = "" details += "## 👍 What went well?\n\n" details += f"> You fixed **{len(fixed)}** security findings.\n\n" - details += self.generateFindingsTable(fixed, rules, options) + details += self.generateFindingsTable(fixed, options) details += "## 👎 What could be better?\n\n" if len(introduced) > 0: details += f"> Unfortunately, you introduced **{len(introduced)}** security findings.\n\n" - details += self.generateFindingsTable(introduced, rules, options) + details += self.generateFindingsTable(introduced, options) + details += "If you believe these findings are false positives,\n" + details += f"you can [exclude the rule]({SECURITY_EXCLUDE_RULE_DOCS}) in the Sigrid configuration.\n" + details += "If you believe these findings are located in files that should not be scanned, you can also\n" + details += f"[exclude the files and/or directories]({SECURITY_EXCLUDE_FILE_DOCS}) in the configuration.\n\n" else: details += "> You did not introduce any security findings during your changes, great job!\n\n" @@ -65,69 +74,31 @@ def getSummary(self, feedback, options): else: return f"⚠️ You did not meet your objective of having {objectiveLabel} security findings" - def generateFindingsTable(self, findings, rules, options): + def generateFindingsTable(self, findings, options): if len(findings) == 0: return "" - md = "| Risk | File | Finding |\n" - md += "|------|------|---------|\n" + md = "| Risk | Part of objective? | File | Finding |\n" + md += "|----|----|----|----|\n" for finding in findings[0:self.MAX_FINDINGS]: - symbol = self.SEVERITY_SYMBOLS[self.getFindingSeverity(finding, rules)] - file = finding["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] - line = finding["locations"][0]["physicalLocation"]["region"]["startLine"] - link = self.decorateLink(options, f"{file}:{line}", file, line) - description = finding["message"]["text"] - md += f"| {symbol} | {link} | {description} |\n" + symbol = self.SEVERITY_SYMBOLS[finding.risk] + check = "✅" if finding.partOfObjective else "-" + link = self.decorateLink(options, f"{finding.file}:{finding.line}", finding.file, finding.line) + md += f"| {symbol} | {check} | {link} | {finding.description} |\n" if len(findings) > self.MAX_FINDINGS: - md += f"| | ... and {len(findings) - self.MAX_FINDINGS} more findings | |\n" + md += f"| | ... and {len(findings) - self.MAX_FINDINGS} more findings | | |\n" return f"{md}\n" - def getRules(self, feedback): - for run in feedback["runs"]: - for rule in run.get("rules", []): - properties = rule.get("properties", {}) - if properties.get("severity"): - yield rule - - def getIntroducedFindings(self, feedback, rules): - previousFingerprints = self.getFingerprints(self.previousFeedback) if self.previousFeedback else [] - - for run in feedback["runs"]: - for result in run.get("results", []): - severity = self.getFindingSeverity(result, rules) - fingerprint = result["fingerprints"]["sigFingerprint/v1"] - if Objective.isFindingIncluded(severity, self.objective) and fingerprint not in previousFingerprints: - yield result - - def getFixedFindings(self, feedback): - if not self.previousFeedback: - return [] - - fingerprints = list(self.getFingerprints(feedback)) - previousRules = list(self.getRules(self.previousFeedback)) - - for run in self.previousFeedback["runs"]: - for result in run.get("results", []): - severity = self.getFindingSeverity(result, previousRules) - fingerprint = result["fingerprints"]["sigFingerprint/v1"] - if Objective.isFindingIncluded(severity, self.objective) and fingerprint not in fingerprints: - yield result - - def getFindingSeverity(self, result, rules): - severity = result.get("properties", {}).get("severity") - if not severity: - for rule in rules: - if rule["id"] == result["ruleId"]: - severity = rule["properties"]["severity"].replace("ERROR", "HIGH").replace("WARNING", "MEDIUM") - return severity.upper() if severity else "UNKNOWN" - - def getFingerprints(self, feedback): - for run in feedback["runs"]: - for result in run.get("results", []): - yield result["fingerprints"]["sigFingerprint/v1"] + def getIntroducedFindings(self, findings, previousFindings): + previousFingerprints = [finding.fingerprint for finding in previousFindings] + return [finding for finding in findings if finding.fingerprint not in previousFingerprints] + + def getFixedFindings(self, findings, previousFindings): + fingerprints = [finding.fingerprint for finding in findings] + return [finding for finding in previousFindings if finding.fingerprint not in fingerprints] def getCapability(self): return SECURITY @@ -136,6 +107,6 @@ def getMarkdownFile(self, options): return os.path.abspath(f"{options.outputDir}/security-feedback.md") def isObjectiveSuccess(self, feedback, options): - rules = list(self.getRules(feedback)) - findings = list(self.getIntroducedFindings(feedback, rules)) - return len(findings) == 0 + findings = self.processor.extractFindings(feedback, self.objective) + relevant = [finding for finding in findings if finding.partOfObjective] + return len(relevant) == 0 diff --git a/sigridci/sigridci/reports/security_text_report.py b/sigridci/sigridci/reports/security_text_report.py new file mode 100644 index 00000000..e667c139 --- /dev/null +++ b/sigridci/sigridci/reports/security_text_report.py @@ -0,0 +1,41 @@ +# Copyright Software Improvement Group +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sys + +from .report import Report +from .security_markdown_report import SecurityMarkdownReport +from ..analysisresults.findings_processor import FindingsProcessor + + +class SecurityTextReport(Report): + + def __init__(self, objective, *, output=sys.stdout): + self.output = output + self.objective = objective + + def generate(self, analysisId, feedback, options): + processor = FindingsProcessor() + allFindings = list(processor.extractFindings(feedback, self.objective)) + relevantFindings = [finding for finding in allFindings if finding.partOfObjective] + + if len(relevantFindings) > 0: + print("", file=self.output) + print("Security findings", file=self.output) + print("", file=self.output) + for finding in relevantFindings: + symbol = SecurityMarkdownReport.SEVERITY_SYMBOLS[finding.risk] + print(f" {symbol} {finding.description}", file=self.output) + print(f" In {finding.file} (line {finding.line})", file=self.output) + print("", file=self.output) diff --git a/sigridci/sigridci_feedback.py b/sigridci/sigridci_feedback.py index 27d8df23..43c79769 100755 --- a/sigridci/sigridci_feedback.py +++ b/sigridci/sigridci_feedback.py @@ -18,13 +18,11 @@ import sys from argparse import ArgumentParser, SUPPRESS -from sigridci.capability import MAINTAINABILITY, OPEN_SOURCE_HEALTH, SECURITY +from sigridci.capability import CAPABILITY_SHORT_NAMES from sigridci.feedback_provider import FeedbackProvider -from sigridci.publish_options import PublishOptions, RunMode, Capability +from sigridci.publish_options import PublishOptions, RunMode from sigridci.sigrid_api_client import SigridApiClient -CAPABILITIES = {cap.shortName: cap for cap in [MAINTAINABILITY, OPEN_SOURCE_HEALTH, SECURITY]} - def parseFeedbackOptions(args): options = PublishOptions( @@ -32,7 +30,7 @@ def parseFeedbackOptions(args): customer=args.customer, system=args.system, runMode=RunMode.FEEDBACK_ONLY, - capabilities=[CAPABILITIES[args.capability.lower()]], + capabilities=[CAPABILITY_SHORT_NAMES[args.capability.lower()]], outputDir=args.out, sigridURL=args.sigridurl ) @@ -58,7 +56,7 @@ def determineObjectives(options): parser.add_argument("--system", type=str, required=True, help="Name of your system in Sigrid, letters/digits/hyphens only.") parser.add_argument("--out", type=str, default="sigrid-ci-output", help="Output directory for Sigrid CI feedback.") parser.add_argument("--sigridurl", type=str, default="https://sigrid-says.com", help="Sigrid base URL.") - parser.add_argument("--capability", type=str, required=True, choices=list(CAPABILITIES.keys())) + parser.add_argument("--capability", type=str, required=True, choices=list(CAPABILITY_SHORT_NAMES.keys())) parser.add_argument("--analysisresults", type=str, required=True, help="Analysis results JSON file.") parser.add_argument("--previousresults", type=str, help="Baseline analysis results JSON file used for comparison.") args = parser.parse_args() @@ -66,7 +64,7 @@ def determineObjectives(options): options = parseFeedbackOptions(args) objectives = determineObjectives(options) - feedbackProvider = FeedbackProvider(CAPABILITIES[args.capability], options, objectives) + feedbackProvider = FeedbackProvider(CAPABILITY_SHORT_NAMES[args.capability], options, objectives) feedbackProvider.loadLocalAnalysisResults(args.analysisresults) if args.previousresults: feedbackProvider.loadPreviousAnalysisResults(args.previousresults) diff --git a/test/test_findings_processor.py b/test/test_findings_processor.py new file mode 100644 index 00000000..01d66696 --- /dev/null +++ b/test/test_findings_processor.py @@ -0,0 +1,49 @@ +# Copyright Software Improvement Group +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import json +import os +from unittest import TestCase + +from sigridci.sigridci.analysisresults.findings_processor import FindingsProcessor + + +class FindingsProcessorTest(TestCase): + + def testExtractAllFindingsSarif(self): + with open(os.path.dirname(__file__) + "/testdata/security.sarif.json", encoding="utf-8", mode="r") as f: + feedback = json.load(f) + + processor = FindingsProcessor() + findings = list(processor.extractFindings(feedback, "HIGH")) + + self.assertEqual(len(findings), 2) + self.assertEqual(findings[0].risk, "CRITICAL") + self.assertEqual(findings[0].description, "Weak Hash algorithm used") + self.assertEqual(findings[1].risk, "MEDIUM") + self.assertEqual(findings[1].description, "Some other finding") + + def testExtractAllFindingsNative(self): + with open(os.path.dirname(__file__) + "/testdata/security.sig.json", encoding="utf-8", mode="r") as f: + feedback = json.load(f) + + processor = FindingsProcessor() + findings = list(processor.extractFindings(feedback, "HIGH")) + + self.assertEqual(len(findings), 1) + self.assertEqual(findings[0].fingerprint, "0006d9dd-5288-424a-bf8b-077c98ef00ee") + self.assertEqual(findings[0].risk, "MEDIUM") + self.assertEqual(findings[0].description, "String comparisons using '===', '!==', '!=' and '==' is vulnerable to timing attacks") + self.assertEqual(findings[0].file, "widgets/com.mendix.widget.native.WebView/com/mendix/widget/native/webview/WebView.android.js") + self.assertEqual(findings[0].line, 853) diff --git a/test/test_security_markdown_report.py b/test/test_security_markdown_report.py index 7bae4738..b75af72b 100644 --- a/test/test_security_markdown_report.py +++ b/test/test_security_markdown_report.py @@ -27,7 +27,7 @@ class SecurityMarkdownReportTest(TestCase): def setUp(self): self.options = PublishOptions("aap", "noot", RunMode.FEEDBACK_ONLY, sourceDir="/tmp", feedbackURL="") - with open(os.path.dirname(__file__) + "/testdata/security.json", encoding="utf-8", mode="r") as f: + with open(os.path.dirname(__file__) + "/testdata/security.sarif.json", encoding="utf-8", mode="r") as f: self.feedback = json.load(f) @mock.patch.dict(os.environ, { @@ -51,12 +51,18 @@ def testCreateTableFromFindings(self): ## 👎 What could be better? - > Unfortunately, you introduced **1** security findings. + > Unfortunately, you introduced **2** security findings. - | Risk | File | Finding | - |------|------|---------| - | 🟣 | [Security.java:33](https://example.com/aap/noot/-/blob/mybranch/Security.java#L33) | Weak Hash algorithm used | + | Risk | Part of objective? | File | Finding | + |----|----|----|----| + | 🟣 | ✅ | [Security.java:33](https://example.com/aap/noot/-/blob/mybranch/Security.java#L33) | Weak Hash algorithm used | + | 🟠 | - | [Aap.java:33](https://example.com/aap/noot/-/blob/mybranch/Aap.java#L33) | Some other finding | + If you believe these findings are false positives, + you can [exclude the rule](https://docs.sigrid-says.com/reference/analysis-scope-configuration.html#excluding-security-rules) in the Sigrid configuration. + If you believe these findings are located in files that should not be scanned, you can also + [exclude the files and/or directories](https://docs.sigrid-says.com/reference/analysis-scope-configuration.html#excluding-files-and-directories-from-security-scanning) in the configuration. + ---- @@ -117,17 +123,22 @@ def testLimitFindingsIfThereAreTooMany(self): > Unfortunately, you introduced **11** security findings. - | Risk | File | Finding | - |------|------|---------| - | 🟣 | Security.java:33 | Weak Hash algorithm used | - | 🟣 | Security.java:33 | Weak Hash algorithm used | - | 🟣 | Security.java:33 | Weak Hash algorithm used | - | 🟣 | Security.java:33 | Weak Hash algorithm used | - | 🟣 | Security.java:33 | Weak Hash algorithm used | - | 🟣 | Security.java:33 | Weak Hash algorithm used | - | 🟣 | Security.java:33 | Weak Hash algorithm used | - | 🟣 | Security.java:33 | Weak Hash algorithm used | - | | ... and 3 more findings | | + | Risk | Part of objective? | File | Finding | + |----|----|----|----| + | 🟣 | ✅ | Security.java:33 | Weak Hash algorithm used | + | 🟣 | ✅ | Security.java:33 | Weak Hash algorithm used | + | 🟣 | ✅ | Security.java:33 | Weak Hash algorithm used | + | 🟣 | ✅ | Security.java:33 | Weak Hash algorithm used | + | 🟣 | ✅ | Security.java:33 | Weak Hash algorithm used | + | 🟣 | ✅ | Security.java:33 | Weak Hash algorithm used | + | 🟣 | ✅ | Security.java:33 | Weak Hash algorithm used | + | 🟣 | ✅ | Security.java:33 | Weak Hash algorithm used | + | | ... and 3 more findings | | | + + If you believe these findings are false positives, + you can [exclude the rule](https://docs.sigrid-says.com/reference/analysis-scope-configuration.html#excluding-security-rules) in the Sigrid configuration. + If you believe these findings are located in files that should not be scanned, you can also + [exclude the files and/or directories](https://docs.sigrid-says.com/reference/analysis-scope-configuration.html#excluding-files-and-directories-from-security-scanning) in the configuration. ---- @@ -153,23 +164,28 @@ def testReportBasedOnDiff(self): # [Sigrid](https://sigrid-says.com/aap/noot/-/security) Security feedback *(Beta)* **⚠️ You did not meet your objective of having no medium-severity security findings** - + ## 👍 What went well? > You fixed **1** security findings. - | Risk | File | Finding | - |------|------|---------| - | 🟣 | [Security.java:33](https://example.com/aap/noot/-/blob/mybranch/Security.java#L33) | This finding has been fixed in the next snapshot. | - + | Risk | Part of objective? | File | Finding | + |----|----|----|----| + | 🟣 | ✅ | [Security.java:33](https://example.com/aap/noot/-/blob/mybranch/Security.java#L33) | This finding has been fixed in the next snapshot. | + ## 👎 What could be better? > Unfortunately, you introduced **1** security findings. - | Risk | File | Finding | - |------|------|---------| - | 🟠 | [Aap.java:33](https://example.com/aap/noot/-/blob/mybranch/Aap.java#L33) | Some other finding | - + | Risk | Part of objective? | File | Finding | + |----|----|----|----| + | 🟠 | ✅ | [Aap.java:33](https://example.com/aap/noot/-/blob/mybranch/Aap.java#L33) | Some other finding | + + If you believe these findings are false positives, + you can [exclude the rule](https://docs.sigrid-says.com/reference/analysis-scope-configuration.html#excluding-security-rules) in the Sigrid configuration. + If you believe these findings are located in files that should not be scanned, you can also + [exclude the files and/or directories](https://docs.sigrid-says.com/reference/analysis-scope-configuration.html#excluding-files-and-directories-from-security-scanning) in the configuration. + ---- diff --git a/test/test_security_text_report.py b/test/test_security_text_report.py new file mode 100644 index 00000000..391993aa --- /dev/null +++ b/test/test_security_text_report.py @@ -0,0 +1,44 @@ +# Copyright Software Improvement Group +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import inspect +import json +import os +from io import StringIO +from unittest import TestCase + +from sigridci.sigridci.publish_options import PublishOptions, RunMode +from sigridci.sigridci.reports.security_text_report import SecurityTextReport + + +class SecurityTextReportTest(TestCase): + + def testPrintFindings(self): + options = PublishOptions("aap", "noot", RunMode.FEEDBACK_ONLY, sourceDir="/aap", feedbackURL="") + + with open(os.path.dirname(__file__) + "/testdata/security.sarif.json", encoding="utf-8", mode="r") as f: + feedback = json.load(f) + + buffer = StringIO() + report = SecurityTextReport("HIGH", output=buffer) + report.generate("1234", feedback, options) + + expected = """ + Security findings + + 🟣 Weak Hash algorithm used + In Security.java (line 33) + """ + + self.assertEqual(inspect.cleandoc(expected), buffer.getvalue().strip()) diff --git a/test/testdata/security-previous.json b/test/testdata/security-previous.json index b4e52349..03c977e5 100644 --- a/test/testdata/security-previous.json +++ b/test/testdata/security-previous.json @@ -19,7 +19,7 @@ "help": {}, "properties": { "tags": [], - "severity": "Medium", + "severity": "Critical", "analyzer": "Google ErrorProne", "language": "Java", "category": "CORRECTNESS", diff --git a/test/testdata/security.json b/test/testdata/security.sarif.json similarity index 98% rename from test/testdata/security.json rename to test/testdata/security.sarif.json index 2dfc0d85..6311a2b6 100644 --- a/test/testdata/security.json +++ b/test/testdata/security.sarif.json @@ -81,7 +81,7 @@ } ], "fingerprints": { - "sigFingerprint/v1": "0fe24dfe0b8c90ae48620e02865b9a3b399b49ea4ef5e020d83e9507d614ebcb" + "sigFingerprint/v1": "0fe24dfe0b8c90ae48620e02865b9a3b399b49ea4ef5e020d83e9507d614ebcb22" }, "properties": { "tags": ["CWE-327"], diff --git a/test/testdata/security.sig.json b/test/testdata/security.sig.json new file mode 100644 index 00000000..4e248e84 --- /dev/null +++ b/test/testdata/security.sig.json @@ -0,0 +1,34 @@ +[ + { + "id": "0006d9dd-5288-424a-bf8b-077c98ef00ee", + "href": "https://sigrid-says.com/aap/mendix-native-file-documents-demo/-/security/0006d9dd-5288-424a-bf8b-077c98ef00ee", + "firstSeenAnalysisDate": "2025-10-24", + "lastSeenAnalysisDate": "2025-10-24", + "firstSeenSnapshotDate": "2025-10-24", + "lastSeenSnapshotDate": "2025-10-24", + "filePath": "widgets/com.mendix.widget.native.WebView/com/mendix/widget/native/webview/WebView.android.js", + "startLine": 853, + "endLine": 855, + "component": "widgets/com.mendix.widget.native.WebView", + "type": "String comparisons using '===', '!==', '!=' and '==' is vulnerable to timing attacks", + "cweId": "CWE-208", + "severity": "MEDIUM", + "impact": "LOW", + "exploitability": "HIGH", + "severityScore": 5.2, + "impactScore": 2.2, + "exploitabilityScore": 3.0, + "status": "RAW", + "remark": null, + "toolName": "NodeJS Scan", + "ruleId": "njsscan.crypto.node_timing_attack", + "weaknessIds": [ + "CWE-208" + ], + "categories": [ + "A1 Broken Access Control" + ], + "isManualFinding": false, + "isSeverityOverridden": false + } +]