From 6e66d136ecd61a41445e1ef18679023ed68473e7 Mon Sep 17 00:00:00 2001 From: David Losert Date: Mon, 27 Feb 2023 16:05:59 +0000 Subject: [PATCH] Reformats vulnerability section --- .../{mock-change.ts => create-test-change.ts} | 9 +- .../fixtures/create-test-vulnerability.ts | 19 ++ __tests__/summary.test.ts | 250 +++++++++++++----- src/summary.ts | 72 ++--- 4 files changed, 241 insertions(+), 109 deletions(-) rename __tests__/fixtures/{mock-change.ts => create-test-change.ts} (86%) create mode 100644 __tests__/fixtures/create-test-vulnerability.ts diff --git a/__tests__/fixtures/mock-change.ts b/__tests__/fixtures/create-test-change.ts similarity index 86% rename from __tests__/fixtures/mock-change.ts rename to __tests__/fixtures/create-test-change.ts index ebe7ff7..e3e1547 100644 --- a/__tests__/fixtures/mock-change.ts +++ b/__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' - } + }) ] } diff --git a/__tests__/fixtures/create-test-vulnerability.ts b/__tests__/fixtures/create-test-vulnerability.ts new file mode 100644 index 0000000..26f7490 --- /dev/null +++ b/__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 => ({ + ...defaultTestVulnerability, + ...overwrites +}) + +export {createTestVulnerability} diff --git a/__tests__/summary.test.ts b/__tests__/summary.test.ts index 08f2351..000d33f 100644 --- a/__tests__/summary.test.ts +++ b/__tests__/summary.test.ts @@ -1,21 +1,21 @@ import {expect, jest, test} from '@jest/globals' -import {Change, Changes, ConfigurationOptions} from '../src/schemas' -import * as summary from '../src/summary'; -import * as core from '@actions/core'; -import { createTestChange } from './fixtures/mock-change'; - +import {Changes, ConfigurationOptions} from '../src/schemas' +import * as summary from '../src/summary' +import * as core from '@actions/core' +import {createTestChange} from './fixtures/create-test-change' +import {createTestVulnerability} from './fixtures/create-test-vulnerability' afterEach(() => { - jest.clearAllMocks(); - core.summary.emptyBuffer(); -}); + jest.clearAllMocks() + 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); - const text = core.summary.stringify(); - - expect(text).toContain('

Dependency Review

'); -}); + summary.addSummaryToSummary( + emptyChanges, + emptyInvalidLicenseChanges, + defaultConfig + ) + const text = core.summary.stringify() + + expect(text).toContain('

Dependency Review

') +}) test('only includes "No vulnerabilities or license issues found"-message if both are configured and nothing was found', () => { - summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, defaultConfig); - const text = core.summary.stringify(); - - expect(text).toContain('✅ No vulnerabilities or license issues found.'); -}); + summary.addSummaryToSummary( + emptyChanges, + emptyInvalidLicenseChanges, + defaultConfig + ) + const text = core.summary.stringify() + + 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}; - summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, config); - const text = core.summary.stringify(); - - expect(text).toContain('✅ No vulnerabilities found.'); -}); + const config = {...defaultConfig, license_check: false} + summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, config) + const text = core.summary.stringify() + + 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}; - summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, config); - const text = core.summary.stringify(); - - expect(text).toContain('✅ No license issues found.'); -}); + const config = {...defaultConfig, vulnerability_check: false} + summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, config) + const text = core.summary.stringify() + + expect(text).toContain('✅ No license issues found.') +}) test('does not include status section if nothing was found', () => { - summary.addSummaryToSummary(emptyChanges, emptyInvalidLicenseChanges, defaultConfig); - const text = core.summary.stringify(); - - expect(text).not.toContain('The following issues were found:'); -}); + summary.addSummaryToSummary( + emptyChanges, + emptyInvalidLicenseChanges, + 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: 'underscore', package_url: 'test-url'}), - ]; + createTestChange({name: 'lodash'}), + createTestChange({name: 'underscore', package_url: 'test-url'}) + ] const licenseIssues = { forbidden: [createTestChange()], unresolved: [createTestChange(), createTestChange()], - unlicensed: [createTestChange(), createTestChange(), createTestChange()], - }; - - summary.addSummaryToSummary(vulnerabilities, licenseIssues, defaultConfig); - - const text = core.summary.stringify(); - expect(text).toContain('❌ 2 vulnerable package(s)'); - expect(text).toContain('❌ 2 package(s) with invalid SPDX license definitions'); - expect(text).toContain('❌ 1 package(s) with incompatible licenses'); - expect(text).toContain('⚠️ 3 package(s) with unknown licenses'); -}); + unlicensed: [createTestChange(), createTestChange(), createTestChange()] + } + + summary.addSummaryToSummary(vulnerabilities, licenseIssues, defaultConfig) + + const text = core.summary.stringify() + expect(text).toContain('❌ 2 vulnerable package(s)') + expect(text).toContain( + '❌ 2 package(s) with invalid SPDX license definitions' + ) + 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() ]; - - summary.addSummaryToSummary(vulnerabilities, emptyInvalidLicenseChanges, defaultConfig); - - const text = core.summary.stringify(); - expect(text).toContain('❌ 1 vulnerable package(s)'); - expect(text).toContain('✅ 0 package(s) with invalid SPDX license definitions'); - expect(text).toContain('✅ 0 package(s) with incompatible licenses'); - expect(text).toContain('✅ 0 package(s) with unknown licenses'); -}); + const vulnerabilities = [createTestChange()] -test('uses checkmarks for vulnerabilities if only license issues were found.', () => { - const licenseIssues = { forbidden: [createTestChange()], unresolved: [], unlicensed: [] }; - - summary.addSummaryToSummary(emptyChanges, licenseIssues, defaultConfig); - - const text = core.summary.stringify(); - expect(text).toContain('✅ 0 vulnerable package(s)'); - expect(text).toContain('✅ 0 package(s) with invalid SPDX license definitions'); - expect(text).toContain('❌ 1 package(s) with incompatible licenses'); - expect(text).toContain('✅ 0 package(s) with unknown licenses'); -}); + summary.addSummaryToSummary( + vulnerabilities, + emptyInvalidLicenseChanges, + defaultConfig + ) + + const text = core.summary.stringify() + expect(text).toContain('❌ 1 vulnerable package(s)') + expect(text).toContain( + '✅ 0 package(s) with invalid SPDX license definitions' + ) + 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', () => { + const licenseIssues = { + forbidden: [createTestChange()], + unresolved: [], + unlicensed: [] + } + + summary.addSummaryToSummary(emptyChanges, licenseIssues, defaultConfig) + + const text = core.summary.stringify() + expect(text).toContain('✅ 0 vulnerable package(s)') + expect(text).toContain( + '✅ 0 package(s) with invalid SPDX license definitions' + ) + 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('

Vulnerabilities

') + 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('test-summary') +}) + +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 medium 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') +}) diff --git a/src/summary.ts b/src/summary.ts index 75b5377..6c85798 100644 --- a/src/summary.ts +++ b/src/summary.ts @@ -52,47 +52,49 @@ export function addSummaryToSummary( `${icons.check} No vulnerabilities or license issues found.` ) } - } else { - core.summary - .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.` - ] - : []) - ]) + + return } + + 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 - .addHeading('Vulnerabilities', 3) - .addQuote( - `Vulnerabilities were filtered by minimum severity ${severity}.` - ) + core.summary.addHeading('Vulnerabilities', 3) 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 ${severity} 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 }