Merge pull request #623 from actions/fix-advisory-filters

Fix GHSA Filtering

__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/[email protected]',
+  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)
+  const fakeGHSAChanges = filterAllowedAdvisories(['notrealGHSAID'], changes)
-  expect(result).toEqual([npmChange, rubyChange, noVulnNpmChange])
+  expect(fakeGHSAChanges).toEqual([npmChange, rubyChange, noVulnNpmChange])
+})
 
-  result = filterAllowedAdvisories(['first-random_string'], changes)
+test('it properly filters only allowed vulnerabilities', async () => {
-  expect(result).toEqual([rubyChange, noVulnNpmChange])
+  const changes = [npmChange, rubyChange, noVulnNpmChange]
+  const oldVulns = [
+    ...npmChange.vulnerabilities,
+    ...rubyChange.vulnerabilities,
+    ...noVulnNpmChange.vulnerabilities
+  ]
 
-  result = filterAllowedAdvisories(
+  const vulnerable = filterAllowedAdvisories(['vulnerable-ghsa-id'], changes)
-    ['second-random_string', 'third-random_string'],
+
+  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'})
+  )
+})
+
+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([npmChange, noVulnNpmChange])
 
-  result = filterAllowedAdvisories(
+  expect(result.map(change => change.name)).toEqual(
-    ['first-random_string', 'second-random_string', 'third-random_string'],
+    changes.map(change => change.name)
-    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
+test('it properly filters multiple GHSAs', async () => {
-  result = filterAllowedAdvisories(['second-random_string'], changes)
+  const allowedGHSAs = ['vulnerable-ghsa-id', 'moderate-ghsa-id', 'low-ghsa-id']
-  expect(result).toEqual([npmChange, rubyChange, noVulnNpmChange])
+  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 filters out GHSA dependencies', 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)
 })

dist/index.js

@@ -606,12 +606,10 @@ function run() {
                 core.info('No Dependency Changes found. Skipping Dependency Review.');
                 return;
             }
-            const minSeverity = config.fail_on_severity;
             const scopedChanges = (0, filter_1.filterChangesByScopes)(config.fail_on_scopes, changes);
             const filteredChanges = (0, filter_1.filterAllowedAdvisories)(config.allow_ghsas, scopedChanges);
-            const vulnerableChanges = (0, filter_1.filterChangesBySeverity)(minSeverity, filteredChanges).filter(change => change.change_type === 'added' &&
+            const minSeverity = config.fail_on_severity;
-                change.vulnerabilities !== undefined &&
+            const vulnerableChanges = (0, filter_1.filterChangesBySeverity)(minSeverity, filteredChanges);
-                change.vulnerabilities.length > 0);
             const invalidLicenseChanges = yield (0, licenses_1.getInvalidLicenseChanges)(filteredChanges, {
                 allow: config.allow_licenses,
                 deny: config.deny_licenses,
@@ -55933,6 +55931,14 @@ function validatePURL(allow_dependencies_licenses) {
 Object.defineProperty(exports, "__esModule", ({ value: true }));
 exports.filterAllowedAdvisories = exports.filterChangesByScopes = exports.filterChangesBySeverity = void 0;
 const schemas_1 = __nccwpck_require__(1129);
+/**
+ * Filters changes by a severity level. Only vulnerable
+ * dependencies will be returned.
+ *
+ * @param severity - The severity level to filter by.
+ * @param changes - The array of changes to filter.
+ * @returns The filtered array of changes that match the specified severity level and have vulnerabilities.
+ */
 function filterChangesBySeverity(severity, changes) {
     const severityIdx = schemas_1.SEVERITIES.indexOf(severity);
     let filteredChanges = [];
@@ -55952,7 +55958,10 @@ function filterChangesBySeverity(severity, changes) {
     }
     // don't want to deal with changes with no vulnerabilities
     filteredChanges = filteredChanges.filter(change => change.vulnerabilities.length > 0);
-    return filteredChanges;
+    // only report vulnerability additions
+    return filteredChanges.filter(change => change.change_type === 'added' &&
+        change.vulnerabilities !== undefined &&
+        change.vulnerabilities.length > 0);
 }
 exports.filterChangesBySeverity = filterChangesBySeverity;
 function filterChangesByScopes(scopes, changes) {
@@ -55979,22 +55988,15 @@ function filterAllowedAdvisories(ghsas, changes) {
     if (ghsas === undefined) {
         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;
-        }
-        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;
-            }
         }
+        const newChange = Object.assign({}, change);
+        newChange.vulnerabilities = change.vulnerabilities.filter(vuln => !ghsas.includes(vuln.advisory_ghsa_id));
+        return newChange;
     });
     return filteredChanges;
 }

src/filter.ts

@@ -1,5 +1,13 @@
 import {Changes, Severity, SEVERITIES, Scope} from './schemas'
 
+/**
+ * Filters changes by a severity level. Only vulnerable
+ * dependencies will be returned.
+ *
+ * @param severity - The severity level to filter by.
+ * @param changes - The array of changes to filter.
+ * @returns The filtered array of changes that match the specified severity level and have vulnerabilities.
+ */
 export function filterChangesBySeverity(
   severity: Severity,
   changes: Changes
@@ -31,7 +39,14 @@ export function filterChangesBySeverity(
   filteredChanges = filteredChanges.filter(
     change => change.vulnerabilities.length > 0
   )
-  return filteredChanges
+
+  // only report vulnerability additions
+  return filteredChanges.filter(
+    change =>
+      change.change_type === 'added' &&
+      change.vulnerabilities !== undefined &&
+      change.vulnerabilities.length > 0
+  )
 }
 
 export function filterChangesByScopes(
@@ -67,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
+    return newChange
-    // 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 filteredChanges

src/main.ts

@@ -80,21 +80,17 @@ async function run(): Promise<void> {
       return
     }
 
-    const minSeverity = config.fail_on_severity
     const scopedChanges = filterChangesByScopes(config.fail_on_scopes, changes)
+
     const filteredChanges = filterAllowedAdvisories(
       config.allow_ghsas,
       scopedChanges
     )
 
+    const minSeverity = config.fail_on_severity
     const vulnerableChanges = filterChangesBySeverity(
       minSeverity,
       filteredChanges
-    ).filter(
-      change =>
-        change.change_type === 'added' &&
-        change.vulnerabilities !== undefined &&
-        change.vulnerabilities.length > 0
     )
 
     const invalidLicenseChanges = await getInvalidLicenseChanges(