Merge pull request #4670 from snyk/fix/container-sarif-replace-colon-…

…within-location-uri fix: container sarif replace colon within location uri
snyk · Jun 23, 2023 · 7e422c0 · 7e422c0
2 parents 26a4247 + 6b3e046
commit 7e422c0
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 76 deletions.
diff --git a/src/lib/formatters/get-sarif-result.ts b/src/lib/formatters/get-sarif-result.ts
@@ -18,7 +18,10 @@ export function getResults(testResult): sarif.Result[] {
         {
           physicalLocation: {
             artifactLocation: {
-              uri: testResult.displayTargetFile || testResult.path,
+              uri: getArtifactLocationUri(
+                testResult.displayTargetFile,
+                testResult.path,
+              ),
             },
             region: {
               startLine: vuln.lineNumber || 1,
@@ -42,3 +45,15 @@ export function getLevel(vuln: AnnotatedIssue) {
       return 'note';
   }
 }
+
+function getArtifactLocationUri(targetFile: string, path: string): string {
+  if (targetFile) {
+    return targetFile;
+  }
+
+  // For container tests there might be cases when the target file (i.e. Dockerfile passed with the --file flag) is not
+  // present. In this case we use the test result path which contains the image reference (e.g. alpine:3.18.0).
+  // Also, Github Code Scanning returns an error when the artifact location uri from the uploaded sarif file contains
+  // a colon (e.g. alpine:3.18.0 is not valid, but alpine_3.18.0 is valid), so we are replacing colon characters.
+  return path.replace(/:/g, '_');
+}
diff --git a/test/jest/unit/lib/formatters/get-sarif-result.spec.ts b/test/jest/unit/lib/formatters/get-sarif-result.spec.ts
@@ -2,89 +2,48 @@ import { getResults } from '../../../../../src/lib/formatters/get-sarif-result';
 import { SEVERITY, TestResult } from '../../../../../src/lib/snyk-test/legacy';
 
 describe('Retrieving sarif result', () => {
-  it('should use the test results path as the location uri when target file is not present', () => {
-    let result = getResults(
-      getTestResult({
-        path: 'alpine:3.18.0',
-      }),
-    );
-    expect(result).toEqual([
+  const cases: Array<[
+    string,
+    { path: string; displayTargetFile?: string },
+    { resultLocationUri: string },
+  ]> = [
+    [
+      'should return the path given there is no target file present',
+      { path: 'alpine' },
+      { resultLocationUri: 'alpine' },
+    ],
+    [
+      'should return the path without colon characters given there is no target file present and the path contains a tag',
+      { path: 'alpine:3.18.0' },
+      { resultLocationUri: 'alpine_3.18.0' },
+    ],
+    [
+      'should return the path without colon characters given there is no target file present and the path contains a digest',
       {
-        ruleId: 'SNYK-LINUX-EXPAT-450908',
-        level: 'error',
-        message: {
-          text:
-            'This file introduces a vulnerable expat package with a critical severity vulnerability.',
-        },
-        locations: [
-          {
-            physicalLocation: {
-              artifactLocation: { uri: 'alpine:3.18.0' },
-              region: { startLine: 1 },
-            },
-          },
-        ],
+        path:
+          'alpine@sha256:c0669ef34cdc14332c0f1ab0c2c01acb91d96014b172f1a76f3a39e63d1f0bda',
       },
-    ]);
-
-    result = getResults(
-      getTestResult({
-        path: 'alpine:3.18.0',
-        displayTargetFile: undefined,
-      }),
-    );
-    expect(result).toEqual([
       {
-        ruleId: 'SNYK-LINUX-EXPAT-450908',
-        level: 'error',
-        message: {
-          text:
-            'This file introduces a vulnerable expat package with a critical severity vulnerability.',
-        },
-        locations: [
-          {
-            physicalLocation: {
-              artifactLocation: { uri: 'alpine:3.18.0' },
-              region: { startLine: 1 },
-            },
-          },
-        ],
+        resultLocationUri:
+          'alpine@sha256_c0669ef34cdc14332c0f1ab0c2c01acb91d96014b172f1a76f3a39e63d1f0bda',
       },
-    ]);
+    ],
+    [
+      'should return the target file given there is a target file present',
+      { path: 'alpine', displayTargetFile: 'Dockerfile.test' },
+      { resultLocationUri: 'Dockerfile.test' },
+    ],
+  ];
 
-    result = getResults(
+  it.each(cases)('%s', (_, input, want) => {
+    const result = getResults(
       getTestResult({
-        path: 'alpine:3.18.0',
-        displayTargetFile: null,
+        displayTargetFile: input.displayTargetFile,
+        path: input.path,
       }),
     );
-    expect(result).toEqual([
-      {
-        ruleId: 'SNYK-LINUX-EXPAT-450908',
-        level: 'error',
-        message: {
-          text:
-            'This file introduces a vulnerable expat package with a critical severity vulnerability.',
-        },
-        locations: [
-          {
-            physicalLocation: {
-              artifactLocation: { uri: 'alpine:3.18.0' },
-              region: { startLine: 1 },
-            },
-          },
-        ],
-      },
-    ]);
-  });
 
-  it('should use the target file as the location uri when target file is present', () => {
-    const actualResult = getResults(
-      getTestResult({
-        displayTargetFile: 'Dockerfile.test',
-      }),
-    );
-    expect(actualResult).toEqual([
+    expect(result).toEqual([
       {
         ruleId: 'SNYK-LINUX-EXPAT-450908',
         level: 'error',
@@ -95,7 +54,7 @@ describe('Retrieving sarif result', () => {
         locations: [
           {
             physicalLocation: {
-              artifactLocation: { uri: 'Dockerfile.test' },
+              artifactLocation: { uri: want.resultLocationUri },
               region: { startLine: 1 },
             },
           },