Reformats vulnerability section

__tests__/fixtures/create-test-change.ts

@@ -1,4 +1,5 @@
 import {Change} from '../../src/schemas'
+import {createTestVulnerability} from './create-test-vulnerability'
 
 const defaultChange: Change = {
   change_type: 'added',
@@ -11,19 +12,19 @@ const defaultChange: Change = {
   source_repository_url: 'https://github.com/lodash/lodash',
   scope: 'runtime',
   vulnerabilities: [
-    {
+    createTestVulnerability({
       severity: 'high',
       advisory_ghsa_id: 'GHSA-35jh-r3h4-6jhm',
       advisory_summary: 'Command Injection in lodash',
       advisory_url: 'https://github.com/advisories/GHSA-35jh-r3h4-6jhm'
-    },
+    }),
-    {
+    createTestVulnerability({
       severity: 'moderate',
       advisory_ghsa_id: 'GHSA-29mw-wpgm-hmr9',
       advisory_summary:
         'Regular Expression Denial of Service (ReDoS) in lodash',
       advisory_url: 'https://github.com/advisories/GHSA-29mw-wpgm-hmr9'
-    }
+    })
   ]
 }
 

__tests__/fixtures/create-test-vulnerability.ts

@@ -0,0 +1,19 @@
+import {Change} from '../../src/schemas'
+
+type Vulnerability = Change['vulnerabilities'][0]
+
+const defaultTestVulnerability: Vulnerability = {
+  severity: 'high',
+  advisory_ghsa_id: 'GHSA-35jh-r3h4-6jhm',
+  advisory_summary: 'Command Injection in lodash',
+  advisory_url: 'https://github.com/advisories/GHSA-35jh-r3h4-6jhm'
+}
+
+const createTestVulnerability = (
+  overwrites: Partial<Vulnerability> = {}
+): Vulnerability => ({
+  ...defaultTestVulnerability,
+  ...overwrites
+})
+
+export {createTestVulnerability}

__tests__/summary.test.ts

@@ -1,21 +1,21 @@
 import {expect, jest, test} from '@jest/globals'
-import {Change, Changes, ConfigurationOptions} from '../src/schemas'
+import {Changes, ConfigurationOptions} from '../src/schemas'
-import * as summary from '../src/summary';
+import * as summary from '../src/summary'
-import * as core from '@actions/core';
+import * as core from '@actions/core'
-import { createTestChange } from './fixtures/mock-change';
+import {createTestChange} from './fixtures/create-test-change'
-
+import {createTestVulnerability} from './fixtures/create-test-vulnerability'
 
 afterEach(() => {
-  jest.clearAllMocks();
+  jest.clearAllMocks()
-  core.summary.emptyBuffer();
+  core.summary.emptyBuffer()
-});
+})
 
-const emptyChanges: Changes = [];
+const emptyChanges: Changes = []
 const emptyInvalidLicenseChanges = {
   forbidden: [],
   unresolved: [],
   unlicensed: []
-};
+}
 const defaultConfig: ConfigurationOptions = {
   vulnerability_check: true,
   license_check: true,
@@ -24,87 +24,191 @@ const defaultConfig: ConfigurationOptions = {
   allow_ghsas: [],
   allow_licenses: [],
   deny_licenses: [],
-  comment_summary_in_pr: true,
+  comment_summary_in_pr: true
 }
 
 test('prints headline as h2', () => {
-  summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, defaultConfig);
+  summary.addSummaryToSummary(
-  const text = core.summary.stringify();
+    emptyChanges,
+    emptyInvalidLicenseChanges,
+    defaultConfig
+  )
+  const text = core.summary.stringify()
 
-  expect(text).toContain('<h2>Dependency Review</h2>');
+  expect(text).toContain('<h2>Dependency Review</h2>')
-});
+})
 
 test('only includes "No vulnerabilities or license issues found"-message if both are configured and nothing was found', () => {
-  summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, defaultConfig);
+  summary.addSummaryToSummary(
-  const text = core.summary.stringify();
+    emptyChanges,
+    emptyInvalidLicenseChanges,
+    defaultConfig
+  )
+  const text = core.summary.stringify()
 
-  expect(text).toContain('✅ No vulnerabilities or license issues found.');
+  expect(text).toContain('✅ No vulnerabilities or license issues found.')
-});
+})
 
 test('only includes "No vulnerabilities found"-message if "license_check" is set to false and nothing was found', () => {
-  const config = {...defaultConfig, license_check: false};
+  const config = {...defaultConfig, license_check: false}
-  summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, config);
+  summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, config)
-  const text = core.summary.stringify();
+  const text = core.summary.stringify()
 
-  expect(text).toContain('✅ No vulnerabilities found.');
+  expect(text).toContain('✅ No vulnerabilities found.')
-});
+})
 
 test('only includes "No license issues found"-message if "vulnerability_check" is set to false and nothing was found', () => {
-  const config = {...defaultConfig, vulnerability_check: false};
+  const config = {...defaultConfig, vulnerability_check: false}
-  summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, config);
+  summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, config)
-  const text = core.summary.stringify();
+  const text = core.summary.stringify()
 
-  expect(text).toContain('✅ No license issues found.');
+  expect(text).toContain('✅ No license issues found.')
-});
+})
 
 test('does not include status section if nothing was found', () => {
-  summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, defaultConfig);
+  summary.addSummaryToSummary(
-  const text = core.summary.stringify();
+    emptyChanges,
-  
+    emptyInvalidLicenseChanges,
-  expect(text).not.toContain('The following issues were found:');
+    defaultConfig
-});
+  )
+  const text = core.summary.stringify()
 
+  expect(text).not.toContain('The following issues were found:')
+})
 
 test('includes count and status icons for all findings', () => {
   const vulnerabilities = [
-    createTestChange({ name: 'lodash'}),
+    createTestChange({name: 'lodash'}),
-    createTestChange({ name: 'underscore', package_url: 'test-url'}),
+    createTestChange({name: 'underscore', package_url: 'test-url'})
-  ];
+  ]
   const licenseIssues = {
     forbidden: [createTestChange()],
     unresolved: [createTestChange(), createTestChange()],
-    unlicensed: [createTestChange(), createTestChange(), createTestChange()],
+    unlicensed: [createTestChange(), createTestChange(), createTestChange()]
-  };
+  }
 
-  summary.addSummaryToSummary(vulnerabilities, licenseIssues, defaultConfig);
+  summary.addSummaryToSummary(vulnerabilities, licenseIssues, defaultConfig)
 
-  const text = core.summary.stringify();
+  const text = core.summary.stringify()
-  expect(text).toContain('❌ 2 vulnerable package(s)');
+  expect(text).toContain('❌ 2 vulnerable package(s)')
-  expect(text).toContain('❌ 2 package(s) with invalid SPDX license definitions');
+  expect(text).toContain(
-  expect(text).toContain('❌ 1 package(s) with incompatible licenses');
+    '❌ 2 package(s) with invalid SPDX license definitions'
-  expect(text).toContain('⚠️ 3 package(s) with unknown licenses');
+  )
-});
+  expect(text).toContain('❌ 1 package(s) with incompatible licenses')
+  expect(text).toContain('⚠️ 3 package(s) with unknown licenses')
+})
 
 test('uses checkmarks for license issues if only vulnerabilities were found', () => {
-  const vulnerabilities = [ createTestChange() ];
+  const vulnerabilities = [createTestChange()]
 
-  summary.addSummaryToSummary(vulnerabilities, emptyInvalidLicenseChanges, defaultConfig);
+  summary.addSummaryToSummary(
+    vulnerabilities,
+    emptyInvalidLicenseChanges,
+    defaultConfig
+  )
 
-  const text = core.summary.stringify();
+  const text = core.summary.stringify()
-  expect(text).toContain('❌ 1 vulnerable package(s)');
+  expect(text).toContain('❌ 1 vulnerable package(s)')
-  expect(text).toContain('✅ 0 package(s) with invalid SPDX license definitions');
+  expect(text).toContain(
-  expect(text).toContain('✅ 0 package(s) with incompatible licenses');
+    '✅ 0 package(s) with invalid SPDX license definitions'
-  expect(text).toContain('✅ 0 package(s) with unknown licenses');
+  )
-});
+  expect(text).toContain('✅ 0 package(s) with incompatible licenses')
+  expect(text).toContain('✅ 0 package(s) with unknown licenses')
+})
 
-test('uses checkmarks for vulnerabilities if only license issues were found.', () => {
+test('uses checkmarks for vulnerabilities if only license issues were found', () => {
-  const licenseIssues = { forbidden: [createTestChange()], unresolved: [], unlicensed: [] };
+  const licenseIssues = {
+    forbidden: [createTestChange()],
+    unresolved: [],
+    unlicensed: []
+  }
 
-  summary.addSummaryToSummary(emptyChanges, licenseIssues, defaultConfig);
+  summary.addSummaryToSummary(emptyChanges, licenseIssues, defaultConfig)
 
-  const text = core.summary.stringify();
+  const text = core.summary.stringify()
-  expect(text).toContain('✅ 0 vulnerable package(s)');
+  expect(text).toContain('✅ 0 vulnerable package(s)')
-  expect(text).toContain('✅ 0 package(s) with invalid SPDX license definitions');
+  expect(text).toContain(
-  expect(text).toContain('❌ 1 package(s) with incompatible licenses');
+    '✅ 0 package(s) with invalid SPDX license definitions'
-  expect(text).toContain('✅ 0 package(s) with unknown licenses');
+  )
-});
+  expect(text).toContain('❌ 1 package(s) with incompatible licenses')
+  expect(text).toContain('✅ 0 package(s) with unknown licenses')
+})
+
+test('addChangeVulnerabilitiesToSummary() - only includes section if any vulnerabilites found', () => {
+  summary.addChangeVulnerabilitiesToSummary(emptyChanges, 'low')
+  const text = core.summary.stringify()
+  expect(text).toEqual('')
+})
+
+test('addChangeVulnerabilitiesToSummary() - includes all vulnerabilities', () => {
+  const changes = [
+    createTestChange({name: 'lodash'}),
+    createTestChange({name: 'underscore', package_url: 'test-url'})
+  ]
+
+  summary.addChangeVulnerabilitiesToSummary(changes, 'low')
+
+  const text = core.summary.stringify()
+  expect(text).toContain('<h3>Vulnerabilities</h3>')
+  expect(text).toContain('lodash')
+  expect(text).toContain('underscore')
+})
+
+test('addChangeVulnerabilitiesToSummary() - includes advisory url if available', () => {
+  const changes = [
+    createTestChange({
+      name: 'underscore',
+      vulnerabilities: [
+        createTestVulnerability({
+          advisory_summary: 'test-summary',
+          advisory_url: 'test-url'
+        })
+      ]
+    })
+  ]
+
+  summary.addChangeVulnerabilitiesToSummary(changes, 'low')
+
+  const text = core.summary.stringify()
+  expect(text).toContain('lodash')
+  expect(text).toContain('<a href="test-url">test-summary</a>')
+})
+
+test('addChangeVulnerabilitiesToSummary() - groups vulnerabilities of a single package', () => {
+  const changes = [
+    createTestChange({
+      name: 'package-with-multiple-vulnerabilities',
+      vulnerabilities: [
+        createTestVulnerability({advisory_summary: 'test-summary-1'}),
+        createTestVulnerability({advisory_summary: 'test-summary-2'})
+      ]
+    })
+  ]
+
+  summary.addChangeVulnerabilitiesToSummary(changes, 'low')
+
+  const text = core.summary.stringify()
+  expect(text.match('package-with-multiple-vulnerabilities')).toHaveLength(1)
+  expect(text).toContain('test-summary-1')
+  expect(text).toContain('test-summary-2')
+})
+
+test('addChangeVulnerabilitiesToSummary() - prints severity statement if above low', () => {
+  const changes = [createTestChange()]
+
+  summary.addChangeVulnerabilitiesToSummary(changes, 'medium')
+
+  const text = core.summary.stringify()
+  expect(text).toContain(
+    'Only included vulnerabilities with severity <strong>medium</strong> or higher.'
+  )
+})
+
+test('addChangeVulnerabilitiesToSummary() - does not print severity statment if it is set to "low"', () => {
+  const changes = [createTestChange()]
+
+  summary.addChangeVulnerabilitiesToSummary(changes, 'low')
+
+  const text = core.summary.stringify()
+  expect(text).not.toContain('Only included vulnerabilities')
+})

src/summary.ts

@@ -52,47 +52,49 @@ export function addSummaryToSummary(
         `${icons.check} No vulnerabilities or license issues found.`
       )
     }
-  } else {
+
-    core.summary
+    return
-      .addRaw('The following issues were found:')
-      .addList([
-        ...(config.vulnerability_check
-          ? [
-              `${checkOrFail(addedPackages.length)} ${
-                addedPackages.length
-              } vulnerable package(s)`
-            ]
-          : []),
-        ...(config.license_check
-          ? [
-              `${checkOrFail(invalidLicenseChanges.unresolved.length)} ${
-                invalidLicenseChanges.unresolved.length
-              } package(s) with invalid SPDX license definitions`,
-              `${checkOrFail(invalidLicenseChanges.forbidden.length)} ${
-                invalidLicenseChanges.forbidden.length
-              } package(s) with incompatible licenses`,
-              `${checkOrWarn(invalidLicenseChanges.unlicensed.length)} ${
-                invalidLicenseChanges.unlicensed.length
-              } package(s) with unknown licenses.`
-            ]
-          : [])
-      ])
   }
+
+  core.summary
+    .addRaw('The following issues were found:')
+    .addList([
+      ...(config.vulnerability_check
+        ? [
+            `${checkOrFailIcon(addedPackages.length)} ${
+              addedPackages.length
+            } vulnerable package(s)`
+          ]
+        : []),
+      ...(config.license_check
+        ? [
+            `${checkOrFailIcon(invalidLicenseChanges.unresolved.length)} ${
+              invalidLicenseChanges.unresolved.length
+            } package(s) with invalid SPDX license definitions`,
+            `${checkOrFailIcon(invalidLicenseChanges.forbidden.length)} ${
+              invalidLicenseChanges.forbidden.length
+            } package(s) with incompatible licenses`,
+            `${checkOrWarnIcon(invalidLicenseChanges.unlicensed.length)} ${
+              invalidLicenseChanges.unlicensed.length
+            } package(s) with unknown licenses.`
+          ]
+        : [])
+    ])
 }
 
 export function addChangeVulnerabilitiesToSummary(
   addedPackages: Changes,
   severity: string
 ): void {
+  if (addedPackages.length === 0) {
+    return
+  }
+
   const rows: SummaryTableRow[] = []
 
   const manifests = getManifestsSet(addedPackages)
 
-  core.summary
+  core.summary.addHeading('Vulnerabilities', 3)
-    .addHeading('Vulnerabilities', 3)
-    .addQuote(
-      `Vulnerabilities were filtered by minimum severity <strong>${severity}</strong>.`
-    )
 
   for (const manifest of manifests) {
     for (const change of addedPackages.filter(
@@ -133,6 +135,12 @@ export function addChangeVulnerabilitiesToSummary(
       ...rows
     ])
   }
+
+  if (severity !== 'low') {
+    core.summary.addQuote(
+      `Only included vulnerabilities with severity <strong>${severity}</strong> or higher.`
+    )
+  }
 }
 
 export function addLicensesToSummary(
@@ -235,10 +243,10 @@ function countLicenseIssues(
   )
 }
 
-function checkOrFail(count: number): string {
+function checkOrFailIcon(count: number): string {
   return count === 0 ? icons.check : icons.cross
 }
 
-function checkOrWarn(count: number): string {
+function checkOrWarnIcon(count: number): string {
   return count === 0 ? icons.check : icons.warning
 }