Merge pull request #2003 from snyk/fix/ignore-yaml-semantic-errors-iac

fix: Skip specific errors when parsing yaml IaC files

src/cli/commands/test/iac-local-execution/file-parser.ts

@@ -23,6 +23,7 @@ import {
 import * as analytics from '../../../../lib/analytics';
 import { CustomError } from '../../../../lib/errors';
 import { getErrorStringCode } from './error-utils';
+import { shouldThrowErrorFor } from './file-utils';
 
 export async function parseFiles(
   filesData: IacFileData[],
@@ -69,7 +70,7 @@ function parseYAMLOrJSONFileData(fileData: IacFileData): any[] {
     // the YAML library can parse both YAML and JSON content, as well as content with singe/multiple YAMLs
     // by using this library we don't have to disambiguate between these different contents ourselves
     yamlDocuments = YAML.parseAllDocuments(fileData.fileContent).map((doc) => {
-      if (doc.errors.length !== 0) {
+      if (shouldThrowErrorFor(doc)) {
         throw doc.errors[0];
       }
       return doc.toJSON();

src/cli/commands/test/iac-local-execution/file-utils.ts

@@ -2,6 +2,7 @@ import * as fs from 'fs';
 import * as tar from 'tar';
 import * as path from 'path';
 import { FailedToInitLocalCacheError } from './local-cache';
+import * as YAML from 'yaml';
 
 export function createIacDir(): void {
   // this path will be able to be customised by the user in the future
@@ -29,3 +30,18 @@ export function extractBundle(response: NodeJS.ReadableStream): Promise<void> {
       .on('error', reject);
   });
 }
+
+const errorsToSkip = [
+  'Insufficient indentation in flow collection',
+  'Map keys must be unique',
+];
+
+// the YAML Parser is more strict than the Golang one in Policy Engine,
+// so we decided to skip specific errors in order to be consistent.
+// this function checks if the current error is one them
+export function shouldThrowErrorFor(doc: YAML.Document.Parsed) {
+  return (
+    doc.errors.length !== 0 &&
+    !errorsToSkip.some((e) => doc.errors[0].message.includes(e))
+  );
+}

src/lib/iac/iac-parser.ts

@@ -14,6 +14,7 @@ import {
   IacValidateTerraformResponse,
   IacValidationResponse,
 } from './constants';
+import { shouldThrowErrorFor } from '../../cli/commands/test/iac-local-execution/file-utils';
 
 const debug = debugLib('snyk-detect');
 
@@ -31,7 +32,7 @@ function parseYamlOrJson(fileContent: string, filePath: string): any {
     case 'yml':
       try {
         return YAML.parseAllDocuments(fileContent).map((doc) => {
-          if (doc.errors.length !== 0) {
+          if (shouldThrowErrorFor(doc)) {
             throw doc.errors[0];
           }
           return doc.toJSON();

test/jest/unit/iac-unit-tests/file-parser.fixtures.ts

@@ -84,6 +84,73 @@ const invalidYamlFile = `
 foo: "bar
 `;
 
+const semanticYamlWithDuplicateKeyFile = `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: myapp-pod
+spec:
+  containers:
+  something: here
+metadata:
+  another: thing
+`;
+
+const semanticYamlErrorFileJSON = {
+  apiVersion: 'v1',
+  kind: 'Pod',
+  metadata: {
+    another: 'thing', //in a case of a duplicate key, the last one will be the one used when parsed
+  },
+  spec: {
+    containers: null,
+    something: 'here',
+  },
+};
+
+const yamlWithInsufficientIndentationFile = `
+Resources:
+ Denied:
+   Type: "AWS::IAM::Role"
+   Properties:
+     AssumeRolePolicyDocument: {
+ "Version": "2012-10-17",
+ "Statement": [
+   {
+     "Action": "sts:AssumeRole",
+     "Principal": {
+       "AWS": "arn:aws:iam::123456789012:root"
+     },
+     "Effect": "Allow",
+     "Sid": ""
+   }
+ ]
+}
+`;
+
+const yamlWithInsufficientIndentationFileJSON = {
+  Resources: {
+    Denied: {
+      Type: 'AWS::IAM::Role',
+      Properties: {
+        AssumeRolePolicyDocument: {
+          Version: '2012-10-17',
+          Statement: [
+            {
+              Action: 'sts:AssumeRole',
+              Principal: {
+                AWS: 'arn:aws:iam::123456789012:root',
+              },
+              Effect: 'Allow',
+              Sid: '',
+            },
+          ],
+        },
+      },
+    },
+  },
+};
+
 export const kubernetesYamlFileDataStub: IacFileData = {
   fileContent: kubernetesYamlFileContent,
   filePath: 'dont-care',
@@ -120,6 +187,33 @@ export const invalidYamlFileDataStub: IacFileData = {
   fileType: 'yml',
 };
 
+export const duplicateKeyYamlErrorFileDataStub: IacFileData = {
+  fileContent: semanticYamlWithDuplicateKeyFile,
+  filePath: 'dont-care',
+  fileType: 'yml',
+};
+
+export const expectedDuplicateKeyYamlErrorFileParsingResult: IacFileParsed = {
+  ...duplicateKeyYamlErrorFileDataStub,
+  docId: 0,
+  projectType: IacProjectType.K8S,
+  engineType: EngineType.Kubernetes,
+  jsonContent: semanticYamlErrorFileJSON,
+};
+
+export const insufficientIndentationYamlErrorFileDataStub: IacFileData = {
+  fileContent: yamlWithInsufficientIndentationFile,
+  filePath: 'dont-care',
+  fileType: 'yml',
+};
+export const expectedInsufficientIndentationYamlErrorFileParsingResult: IacFileParsed = {
+  ...insufficientIndentationYamlErrorFileDataStub,
+  docId: 0,
+  projectType: IacProjectType.CLOUDFORMATION,
+  engineType: EngineType.CloudFormation,
+  jsonContent: yamlWithInsufficientIndentationFileJSON,
+};
+
 export const expectedKubernetesYamlParsingResult: IacFileParsed = {
   ...kubernetesYamlFileDataStub,
   docId: 0,

test/jest/unit/iac-unit-tests/file-parser.spec.ts

@@ -27,6 +27,10 @@ import {
   expectedMultipleKubernetesYamlsParsingResult,
   invalidYamlFileDataStub,
   invalidJsonFileDataStub,
+  duplicateKeyYamlErrorFileDataStub,
+  expectedDuplicateKeyYamlErrorFileParsingResult,
+  expectedInsufficientIndentationYamlErrorFileParsingResult,
+  insufficientIndentationYamlErrorFileDataStub,
 } from './file-parser.fixtures';
 import { IacFileData } from '../../../../src/cli/commands/test/iac-local-execution/types';
 import { IacFileTypes } from '../../../../dist/lib/iac/constants';
@@ -119,12 +123,35 @@ describe('parseFiles', () => {
     );
   });
 
-  it('throws an error for invalid YAML file types', async () => {
+  it('throws an error for invalid (syntax) YAML file types', async () => {
     await expect(parseFiles([invalidYamlFileDataStub])).rejects.toThrow(
       InvalidYamlFileError,
     );
   });
 
+  // the npm yaml parser by default fails on SemanticErrors like duplicate keys
+  // but we decided to skip this error in order to be consistent with the Policy Engine
+  it.each([
+    [
+      {
+        fileStub: duplicateKeyYamlErrorFileDataStub,
+        expectedParsingResult: expectedDuplicateKeyYamlErrorFileParsingResult,
+      },
+    ],
+    [
+      {
+        fileStub: insufficientIndentationYamlErrorFileDataStub,
+        expectedParsingResult: expectedInsufficientIndentationYamlErrorFileParsingResult,
+      },
+    ],
+  ])(
+    `given an $fileStub with one of the errors to skip, it returns $expectedParsingResult`,
+    async ({ fileStub, expectedParsingResult }) => {
+      const { parsedFiles } = await parseFiles([fileStub]);
+      expect(parsedFiles[0]).toEqual(expectedParsingResult);
+    },
+  );
+
   it('throws an error for a Helm file', async () => {
     const helmFileData: IacFileData = {
       fileContent: ' {{ something }}',