From 4366dbae42c015a363c2238839de69e3a20d4270 Mon Sep 17 00:00:00 2001 From: Federico Builes Date: Fri, 24 Nov 2023 14:40:18 +0100 Subject: [PATCH] Advisory filters should not drop entire dependencies. --- __tests__/filter.test.ts | 128 +++++++++++++++++++++++++++++++++------ src/filter.ts | 19 +++--- 2 files changed, 118 insertions(+), 29 deletions(-) diff --git a/__tests__/filter.test.ts b/__tests__/filter.test.ts index 8caab409a..5ee904338 100644 --- a/__tests__/filter.test.ts +++ b/__tests__/filter.test.ts @@ -19,7 +19,7 @@ const npmChange: Change = { vulnerabilities: [ { severity: 'critical', - advisory_ghsa_id: 'first-random_string', + advisory_ghsa_id: 'vulnerable-ghsa-id', advisory_summary: 'very dangerous', advisory_url: 'github.com/future-funk' } @@ -39,13 +39,13 @@ const rubyChange: Change = { vulnerabilities: [ { severity: 'moderate', - advisory_ghsa_id: 'second-random_string', + advisory_ghsa_id: 'moderate-ghsa-id', advisory_summary: 'not so dangerous', advisory_url: 'github.com/future-funk' }, { severity: 'low', - advisory_ghsa_id: 'third-random_string', + advisory_ghsa_id: 'low-ghsa-id', advisory_summary: 'dont page me', advisory_url: 'github.com/future-funk' } @@ -65,6 +65,64 @@ const noVulnNpmChange: Change = { vulnerabilities: [] } +const lodashChange: Change = { + change_type: 'added', + manifest: 'package.json', + ecosystem: 'npm', + name: 'lodash', + version: '4.17.0', + package_url: 'pkg:npm/lodash@4.17.0', + license: 'MIT', + source_repository_url: 'https://github.com/lodash/lodash', + scope: 'runtime', + vulnerabilities: [ + { + severity: 'critical', + advisory_ghsa_id: 'GHSA-jf85-cpcp-j695', + advisory_summary: 'Prototype Pollution in lodash', + advisory_url: 'https://github.com/advisories/GHSA-jf85-cpcp-j695' + }, + { + severity: 'high', + advisory_ghsa_id: 'GHSA-4xc9-xhrj-v574', + advisory_summary: 'Prototype Pollution in lodash', + advisory_url: 'https://github.com/advisories/GHSA-4xc9-xhrj-v574' + }, + { + 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' + }, + { + severity: 'high', + advisory_ghsa_id: 'GHSA-p6mc-m468-83gw', + advisory_summary: 'Prototype Pollution in lodash', + advisory_url: 'https://github.com/advisories/GHSA-p6mc-m468-83gw' + }, + { + severity: 'moderate', + advisory_ghsa_id: 'GHSA-x5rq-j2xg-h7qm', + advisory_summary: + 'Regular Expression Denial of Service (ReDoS) in lodash', + advisory_url: 'https://github.com/advisories/GHSA-x5rq-j2xg-h7qm' + }, + { + 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' + }, + { + severity: 'low', + advisory_ghsa_id: 'GHSA-fvqr-27wr-82fm', + advisory_summary: 'Prototype Pollution in lodash', + advisory_url: 'https://github.com/advisories/GHSA-fvqr-27wr-82fm' + } + ] +} + test('it properly filters changes by severity', async () => { const changes = [npmChange, rubyChange] let result = filterChangesBySeverity('high', changes) @@ -99,25 +157,61 @@ test('it properly handles undefined advisory IDs', async () => { test('it properly filters changes with allowed vulnerabilities', async () => { const changes = [npmChange, rubyChange, noVulnNpmChange] - let result = filterAllowedAdvisories(['notrealGHSAID'], changes) - expect(result).toEqual([npmChange, rubyChange, noVulnNpmChange]) + const fakeGHSAChanges = filterAllowedAdvisories(['notrealGHSAID'], changes) + expect(fakeGHSAChanges).toEqual([npmChange, rubyChange, noVulnNpmChange]) +}) + +test('it properly filters only allowed vulnerabilities', async () => { + const changes = [npmChange, rubyChange, noVulnNpmChange] + const oldVulns = [ + ...npmChange.vulnerabilities, + ...rubyChange.vulnerabilities, + ...noVulnNpmChange.vulnerabilities + ] - result = filterAllowedAdvisories(['first-random_string'], changes) - expect(result).toEqual([rubyChange, noVulnNpmChange]) + const vulnerable = filterAllowedAdvisories(['vulnerable-ghsa-id'], changes) - result = filterAllowedAdvisories( - ['second-random_string', 'third-random_string'], - changes + const newVulns = vulnerable.map(change => change.vulnerabilities).flat() + + expect(newVulns.length).toEqual(oldVulns.length - 1) + expect(newVulns).not.toContainEqual( + expect.objectContaining({advisory_ghsa_id: 'vulnerable-ghsa-id'}) ) - expect(result).toEqual([npmChange, noVulnNpmChange]) +}) - result = filterAllowedAdvisories( - ['first-random_string', 'second-random_string', 'third-random_string'], +test('does not drop dependencies when filtering by GHSA', async () => { + const changes = [npmChange, rubyChange, noVulnNpmChange] + const result = filterAllowedAdvisories( + ['moderate-ghsa-id', 'low-ghsa-id', 'GHSA-jf85-cpcp-j695'], changes ) - expect(result).toEqual([noVulnNpmChange]) - // if we have a change with multiple vulnerabilities but only one is allowed, we still should not filter out that change - result = filterAllowedAdvisories(['second-random_string'], changes) - expect(result).toEqual([npmChange, rubyChange, noVulnNpmChange]) + expect(result.map(change => change.name)).toEqual( + changes.map(change => change.name) + ) +}) + +test('it properly filters multiple GHSAs', async () => { + const allowedGHSAs = ['vulnerable-ghsa-id', 'moderate-ghsa-id', 'low-ghsa-id'] + const changes = [npmChange, rubyChange, noVulnNpmChange] + const oldVulns = changes.map(change => change.vulnerabilities).flat() + + const result = filterAllowedAdvisories(allowedGHSAs, changes) + + const newVulns = result.map(change => change.vulnerabilities).flat() + + expect(newVulns.length).toEqual(oldVulns.length - 3) +}) + +test('it properly filters multiple GHSAs', async () => { + const lodash = filterAllowedAdvisories( + ['GHSA-jf85-cpcp-j695'], + [lodashChange] + )[0] + // the filter should have removed a single GHSA from the list + const expected = lodashChange.vulnerabilities.filter( + vuln => vuln.advisory_ghsa_id !== 'GHSA-jf85-cpcp-j695' + ) + expect(expected.length).toEqual(lodashChange.vulnerabilities.length - 1) + expect(lodash.vulnerabilities).toEqual(expected) }) diff --git a/src/filter.ts b/src/filter.ts index 6921c204d..5e1f0a546 100644 --- a/src/filter.ts +++ b/src/filter.ts @@ -82,25 +82,20 @@ export function filterAllowedAdvisories( return changes } - const filteredChanges = changes.filter(change => { + const filteredChanges = changes.map(change => { const noAdvisories = change.vulnerabilities === undefined || change.vulnerabilities.length === 0 if (noAdvisories) { - return true + return change } + const newChange = {...change} + newChange.vulnerabilities = change.vulnerabilities.filter( + vuln => !ghsas.includes(vuln.advisory_ghsa_id) + ) - let allAllowedAdvisories = true - // if there's at least one advisory that is not allowlisted, we will keep the change - for (const vulnerability of change.vulnerabilities) { - if (!ghsas.includes(vulnerability.advisory_ghsa_id)) { - allAllowedAdvisories = false - } - if (!allAllowedAdvisories) { - return true - } - } + return newChange }) return filteredChanges