Report generation refactoring

bandit/bandit_report.js

@@ -1,7 +1,6 @@
 // Copyright 2018 VMware, Inc.
 // SPDX-License-Identifier: BSD-2-Clause
 
-const { config } = require('../config')
 const { getAnnotation } = require('./bandit_annotations')
 const { countIssueLevels } = require('../annotations_levels')
 
@@ -23,23 +22,13 @@ function createMoreInfoLinks (issues) {
 }
 
 /**
- * Convert bandit output into valid 'output' object for check run conclusion
+ * Process Bandit output (generate annotations, count issue levels)
  * @param {any} results Bandit json output
  */
 module.exports = (results) => {
-  let title = ''
-  let summary = ''
-  let annotations = []
-  let moreInfo = ''
+  const annotations = results.results.map(issue => getAnnotation(issue))
+  const issueCount = countIssueLevels(annotations)
+  const moreInfo = annotations.length > 0 ? createMoreInfoLinks(results.results) : ''
 
-  if (results && results.results.length !== 0) {
-    title = config.issuesFoundResultTitle
-    annotations = results.results.map(issue => getAnnotation(issue))
-    summary = countIssueLevels(annotations)
-    moreInfo = createMoreInfoLinks(results.results)
-  } else {
-    title = config.noIssuesResultTitle
-    summary = config.noIssuesResultSummary
-  }
-  return { title, summary, annotations, moreInfo }
+  return { annotations, issueCount, moreInfo }
 }

github_api_helper.js

@@ -89,7 +89,7 @@ function getConclusion (annotations) {
  * Send check run results
  * @param {import('probot').Context} context Probot context
  * @param {Number} runID check run identifier
- * @param {Object} output output from the scan of Gosec and Bandit
+ * @param {Object} output merged scan output
  * @returns {Promise<any>} GitHub response
  * See: https://developer.github.com/v3/checks/runs/#update-a-check-run
  */

gosec/gosec_annotations.js

@@ -11,8 +11,8 @@ const { getAnnotationLevel } = require('../annotations_levels')
  */
 function getAnnotation (issue, directory) {
   const path = issue.file.replace(directory + '/', '')
-  const startLine = issue.line
-  const endLine = issue.line
+  const startLine = Number(issue.line)
+  const endLine = startLine
   const annotationLevel = getAnnotationLevel(issue.severity, issue.confidence)
   const title = `${issue.rule_id}:${issue.details}`
   const message = `The issue is in the code: ${issue.code}`

gosec/gosec_report.js

@@ -1,28 +1,18 @@
 // Copyright 2018 VMware, Inc.
 // SPDX-License-Identifier: BSD-2-Clause
 
-const { config } = require('../config')
 const { getAnnotation } = require('./gosec_annotations')
 const { countIssueLevels } = require('../annotations_levels')
 
 /**
- *
- * @param {*} results results gosec json output
+ * Process Gosec output (generate annotations, count issue levels)
+ * @param {*} results Gosec json output
  * @param {*} directory working directory for Gosec process
  */
 module.exports = (results, directory) => {
-  let title = ''
-  let summary = ''
-  let annotations = []
+  const annotations = results.Issues.map(issue => getAnnotation(issue, directory))
+  const issueCount = countIssueLevels(annotations)
+  const moreInfo = ''
 
-  if (results && results.Issues.length !== 0) {
-    title = config.issuesFoundResultTitle
-    annotations = results.Issues.map(issue => getAnnotation(issue, directory))
-    summary = countIssueLevels(annotations)
-  } else {
-    title = config.noIssuesResultTitle
-    summary = config.noIssuesResultSummary
-  }
-
-  return { title, summary, annotations }
+  return { annotations, issueCount, moreInfo }
 }

linters/bandit.js

@@ -18,7 +18,7 @@ module.exports = class Bandit {
   }
 
   get defaultReport () {
-    return report(null)
+    return { annotations: [], issueCount: { errors: 0, warnings: 0, notices: 0 }, moreInfo: '' }
   }
 
   /**

linters/gosec.js

@@ -18,7 +18,7 @@ module.exports = class Gosec {
   }
 
   get defaultReport () {
-    return report(null)
+    return { annotations: [], issueCount: { errors: 0, warnings: 0, notices: 0 }, moreInfo: '' }
   }
 
   /**

merge_reports.js

@@ -3,71 +3,42 @@
 
 const { config } = require('./config')
 
-/**
- * Creates the final summary message describing the number of errors, warnings and notices.
- * @param {Number} errors the number of errors found
- * @param {Number} warnings the number of warnings found
- * @param {Number} notices the number of notices found
- */
-function createFinalSummaryMessage (errors, warnings, notices) {
-  let summary = ''
-
-  const errorsMessage = errors > 1 ? ':x: ' + errors + ' errors\n'
-    : ':x: 1 error\n'
-  const warningsMessage = warnings > 1 ? ':warning: ' + warnings + ' warnings\n'
-    : ':warning: 1 warning\n'
-  const noticesMessage = notices > 1 ? ':information_source: ' + notices + ' notices\n'
-    : ':information_source: 1 notice\n'
+const fmt = (name, count, symbol) => {
+  if (!count) {
+    return ''
+  }
+  return `:${symbol}: ${count} ${name}${count > 1 ? 's' : ''}\n`
+}
 
-  summary += errors !== 0 ? errorsMessage : ''
-  summary += warnings !== 0 ? warningsMessage : ''
-  summary += notices !== 0 ? noticesMessage : ''
+const mergeSummaries = (summaries) => {
+  let summary = ''
 
-  return summary
-}
+  const errors = summaries.map(s => s.errors).reduce((a, b) => a + b, 0)
+  summary += fmt('error', errors, 'x')
 
-function mergeSummaries (banditSummary, gosecSummary) {
-  let result = { errors: 0, warnings: 0, notices: 0 }
+  const warnings = summaries.map(s => s.warnings).reduce((a, b) => a + b, 0)
+  summary += fmt('warning', warnings, 'warning')
 
-  if (banditSummary === config.noIssuesResultSummary) {
-    result = gosecSummary
-  } else if (gosecSummary === config.noIssuesResultSummary) {
-    result = banditSummary
-  } else {
-    result.errors = banditSummary.errors + gosecSummary.errors
-    result.warnings = banditSummary.warnings + gosecSummary.warnings
-    result.notices = banditSummary.notices + gosecSummary.notices
-  }
+  const notices = summaries.map(s => s.notices).reduce((a, b) => a + b, 0)
+  summary += fmt('notice', notices, 'information_source')
 
-  return createFinalSummaryMessage(result.errors, result.warnings, result.notices)
+  return summary
 }
 
 /**
- * @param banditReport the Bandit output converted into valid 'output' object for check run conclusion
- * @param gosecReport the Gosec output converted into valid 'output' object for check run conclusion
- * for reference of the 'output' object see: https://developer.github.com/v3/checks/runs/#output-object
+ * @param {any[]} reports linter report objects to be merged
+ * @returns merged report ready to be sent to GitHub
+ * for object schema see: https://developer.github.com/v3/checks/runs/#output-object
  */
-module.exports = (banditReport, gosecReport) => {
-  let title = ''
-  let summary = ''
-  let text = ''
-  let annotations = []
+module.exports = (reports) => {
+  const annotations = reports.map(r => r.annotations).flat()
 
-  if (banditReport.title === config.noIssuesResultTitle && banditReport.title === gosecReport.title) {
-    title = config.noIssuesResultTitle
-    summary = config.noIssuesResultSummary
-  } else {
-    title = config.issuesFoundResultTitle
-    summary = mergeSummaries(banditReport.summary, gosecReport.summary)
-    text = banditReport.moreInfo
+  if (!annotations.length) {
+    return { title: config.noIssuesResultTitle, summary: config.noIssuesResultSummary, annotations, text: '' }
   }
 
-  if (!gosecReport.annotations) {
-    annotations = banditReport.annotations
-  } else if (!banditReport.annotations) {
-    annotations = gosecReport.annotations
-  } else {
-    annotations = annotations.concat(gosecReport.annotations, banditReport.annotations)
-  }
-  return { title, summary, annotations, text }
+  const summary = mergeSummaries(reports.map(r => r.issueCount))
+
+  // TODO: handle text merge
+  return { title: config.issuesFoundResultTitle, summary, annotations, text: '' }
 }

runner.js

@@ -17,10 +17,8 @@ const linters = require('./linters')
 async function runLinters (files, repoID, prID) {
   // TODO: Sync directory with file download location resolution
   const reports = Object.values(linters).map((linter) => run(linter, linter.workingDirectoryForPR(repoID, prID), files))
-  const resolved = await Promise.all(reports)
 
-  // TODO: rewrite merge to handle list of reports
-  return merge(resolved[0], resolved[1])
+  return merge(await Promise.all(reports))
 }
 
 /**

test/bandit.report.test.js

@@ -2,16 +2,24 @@
 // SPDX-License-Identifier: BSD-2-Clause
 
 const generateReport = require('../bandit/bandit_report')
-const { config } = require('../config')
+
 const banditResults = require('./fixtures/reports/bandit_vulnerable.json')
 
-describe('Report generation', () => {
+describe('Bandit report generation', () => {
   let report
 
   beforeEach(() => {
     report = generateReport(banditResults)
   })
 
+  test('Creates correct report structure', () => {
+    expect(report).toHaveProperty('annotations')
+    expect(report).toHaveProperty('issueCount.errors')
+    expect(report).toHaveProperty('issueCount.warnings')
+    expect(report).toHaveProperty('issueCount.notices')
+    expect(report).toHaveProperty('moreInfo')
+  })
+
   test('Creates correct annotations', () => {
     expect(report.annotations).toHaveLength(4)
     expect(report.annotations[0].end_line).toBe(8)
@@ -25,28 +33,20 @@ describe('Report generation', () => {
     })
   })
 
-  test('Creates correct report structure', () => {
-    expect(report.summary).not.toBe('')
-    expect(report).toHaveProperty('annotations')
-    expect(report).toHaveProperty('title')
-  })
-
-  test('Handles empty reports', () => {
-    const report = generateReport(null)
-    expect(report).toHaveProperty('annotations')
-    expect(report).toHaveProperty('summary')
-    expect(report).toHaveProperty('title')
+  test('Creates correct issue count', () => {
+    expect(report.issueCount.errors).toBe(1)
+    expect(report.issueCount.warnings).toBe(3)
+    expect(report.issueCount.notices).toBe(0)
   })
 
-  // This test caches a few cases at a time: when a pr scans python files without security issues
-  // and when a PR doesn't have any python files
-  test('Generate report on results from Bandit without security issues', async () => {
-    const results = require('./fixtures/reports/bandit_safe.json')
+  test('Handles no vuln reports', async () => {
+    const jsonResults = require('./fixtures/reports/bandit_safe.json')
 
-    const report = generateReport(results)
+    const report = generateReport(jsonResults)
 
-    expect(report.title).toEqual(config.noIssuesResultTitle)
-    expect(report.summary).toEqual(config.noIssuesResultSummary)
-    expect(report.annotations.length).toEqual(0)
+    expect(report.annotations).toHaveLength(0)
+    expect(report.issueCount.errors).toBe(0)
+    expect(report.issueCount.warnings).toBe(0)
+    expect(report.issueCount.notices).toBe(0)
   })
 })