From 2c5b41d6313bba4dc068f60674d3114c8f000d00 Mon Sep 17 00:00:00 2001
From: Ivan Stanev <istanev93@gmail.com>
Date: Fri, 30 Jul 2021 12:36:48 +0100
Subject: [PATCH] fix: return correct exit code when using
 --exclude-base-image-vulns

We now filter the vulnerabilities when using the --exclude-base-image-vulns flag before we decide if the test response was successful.
---
 src/lib/snyk-test/legacy.ts           | 16 +++++++++-----
 src/lib/snyk-test/run-test.ts         |  7 +-----
 test/jest/acceptance/cli-args.spec.ts | 32 +++++++++++++++++----------
 3 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/src/lib/snyk-test/legacy.ts b/src/lib/snyk-test/legacy.ts
index bda663b4320..44766b1b2a7 100644
--- a/src/lib/snyk-test/legacy.ts
+++ b/src/lib/snyk-test/legacy.ts
@@ -1,7 +1,7 @@
 const values = require('lodash.values');
 import * as depGraphLib from '@snyk/dep-graph';
 import { SupportedPackageManagers } from '../package-managers';
-import { SupportedProjectTypes } from '../types';
+import { Options, SupportedProjectTypes, TestOptions } from '../types';
 import { SEVERITIES } from './common';
 
 interface Pkg {
@@ -334,7 +334,7 @@ function convertTestDepGraphResultToLegacy(
   res: TestDepGraphResponse,
   depGraph: depGraphLib.DepGraph,
   packageManager: SupportedProjectTypes | undefined,
-  severityThreshold?: SEVERITY,
+  options: Options & TestOptions,
 ): LegacyVulnApiResult {
   const result = res.result;
 
@@ -359,7 +359,7 @@ function convertTestDepGraphResultToLegacy(
 
   // generate the legacy vulns array (vuln-data + metada per vulnerable path).
   //   use the upgradePathsMap to find available upgrade-paths
-  const vulns: AnnotatedIssue[] = [];
+  let vulns: AnnotatedIssue[] = [];
 
   for (const pkgInfo of values(result.affectedPkgs)) {
     for (const vulnPkgPath of depGraph.pkgPathsToRoot(pkgInfo.pkg)) {
@@ -422,8 +422,14 @@ function convertTestDepGraphResultToLegacy(
 
   const meta = res.meta || {};
 
-  severityThreshold =
-    severityThreshold === SEVERITY.LOW ? undefined : severityThreshold;
+  const severityThreshold =
+    options.severityThreshold === SEVERITY.LOW
+      ? undefined
+      : options.severityThreshold;
+
+  if (options.docker && options.file && options['exclude-base-image-vulns']) {
+    vulns = vulns.filter((vuln) => (vuln as DockerIssue).dockerfileInstruction);
+  }
 
   const legacyRes: LegacyVulnApiResult = {
     vulnerabilities: vulns,
diff --git a/src/lib/snyk-test/run-test.ts b/src/lib/snyk-test/run-test.ts
index 07d81efb1cf..a6fd599c968 100644
--- a/src/lib/snyk-test/run-test.ts
+++ b/src/lib/snyk-test/run-test.ts
@@ -390,7 +390,7 @@ async function parseRes(
       (res as any) as TestDepGraphResponse, // Double "as" required by Typescript for dodgy assertions
       depGraph,
       pkgManager,
-      options.severityThreshold,
+      options,
     );
     // For Node.js: inject additional information (for remediation etc.) into the response.
     if (payload.modules) {
@@ -447,11 +447,6 @@ async function parseRes(
       return vuln;
     });
   }
-  if (options.docker && options.file && options['exclude-base-image-vulns']) {
-    res.vulnerabilities = res.vulnerabilities.filter(
-      (vuln) => (vuln as DockerIssue).dockerfileInstruction,
-    );
-  }
 
   res.uniqueCount = countUniqueVulns(res.vulnerabilities);
 
diff --git a/test/jest/acceptance/cli-args.spec.ts b/test/jest/acceptance/cli-args.spec.ts
index 99e646a51de..4f5bdb661ac 100644
--- a/test/jest/acceptance/cli-args.spec.ts
+++ b/test/jest/acceptance/cli-args.spec.ts
@@ -10,6 +10,11 @@ const createOutputDirectory = (): string => {
   return outputPath;
 };
 
+const isWindows =
+  require('os-name')()
+    .toLowerCase()
+    .indexOf('windows') === 0;
+
 jest.setTimeout(1000 * 60 * 5);
 
 test('snyk test command should fail when --file is not specified correctly', async () => {
@@ -333,17 +338,20 @@ test('container test --sarif-file-output can be used at the same time as --json'
   expect(code).toEqual(0);
 });
 
-test('bug: container test --file=Dockerfile --exclude-base-image-vulns returns exit code 2', async () => {
-  const dockerfilePath = path.normalize(
-    'test/acceptance/fixtures/docker/Dockerfile.alpine-3.12.0',
-  );
+if (!isWindows) {
+  // Previously we used to have a bug where --exclude-base-image-vulns returned exit code 2.
+  // This test asserts that the bug no longer exists.
+  test('container test --file=Dockerfile --exclude-base-image-vulns returns exit code 0', async () => {
+    const dockerfilePath = path.normalize(
+      'test/acceptance/fixtures/docker/Dockerfile.alpine-3.12.0',
+    );
 
-  const { code, stdout } = await runSnykCLI(
-    `container test alpine:3.12.0 --json --file=${dockerfilePath} --exclude-base-image-vulns`,
-  );
-  const jsonOutput = JSON.parse(stdout);
+    const { code, stdout } = await runSnykCLI(
+      `container test alpine:3.12.0 --json --file=${dockerfilePath} --exclude-base-image-vulns`,
+    );
+    const jsonOutput = JSON.parse(stdout);
 
-  // BUG: it should return ok: true and exit code 0 when all vulns are excluded
-  expect(jsonOutput.ok).toEqual(false);
-  expect(code).toEqual(2);
-});
+    expect(jsonOutput.ok).toEqual(true);
+    expect(code).toEqual(0);
+  });
+}