From 8566273fa446b571607026c29d6c68495aa803a1 Mon Sep 17 00:00:00 2001
From: Craig Furman <craig.furman@snyk.io>
Date: Wed, 28 Jul 2021 11:51:46 +0100
Subject: [PATCH] fix: IaC path parsing

Fix a bug in which IaC resource paths (more context below) was naively
split by dot, without distinguishing between separator dots and dots
inside string literals.

For example: "foo.bar.baz" was split into 3 words correctly, but
"metadata.annotations['container.apparmor.security.beta.kubernetes.io/web']"
was not.

The "path" / "cloud config path" is a sort of address used to identify
locations in files where vulnerabilities reside. It is very similar to a
jsonpath. The user receives it as an array of components, which in the
future can be useful for building tooling around, e.g. the upcoming CLI
ignores.

The code that evaluates policies against source files is also
responsible for returning a representation of this path, which it does
as a dot-separated string.
---
 package.json                                  |  1 +
 .../test/iac-local-execution/parsers/path.ts  | 35 +++++++++++++++++++
 .../iac-local-execution/results-formatter.ts  |  3 +-
 .../unit/iac-unit-tests/path-parser.spec.ts   | 19 ++++++++++
 4 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 src/cli/commands/test/iac-local-execution/parsers/path.ts
 create mode 100644 test/jest/unit/iac-unit-tests/path-parser.spec.ts

diff --git a/package.json b/package.json
index f853f69f601..18ae43fe233 100644
--- a/package.json
+++ b/package.json
@@ -114,6 +114,7 @@
     "open": "^7.0.3",
     "ora": "5.4.0",
     "os-name": "^3.0.0",
+    "pegjs": "^0.10.0",
     "promise-queue": "^2.2.5",
     "proxy-from-env": "^1.0.0",
     "rimraf": "^2.6.3",
diff --git a/src/cli/commands/test/iac-local-execution/parsers/path.ts b/src/cli/commands/test/iac-local-execution/parsers/path.ts
new file mode 100644
index 00000000000..aa53f8e474a
--- /dev/null
+++ b/src/cli/commands/test/iac-local-execution/parsers/path.ts
@@ -0,0 +1,35 @@
+import * as peg from 'pegjs';
+
+const grammar = `
+start
+  = element+
+
+element
+  = component:component "."? { return component; }
+
+component
+  = $(identifier index?)
+
+identifier
+  = $([^'"\\[\\]\\.]+)
+
+index
+  = $("[" ['"]? [^'"\\]]+ ['"]? "]")
+`;
+
+export const parsePath = createPathParser();
+
+function createPathParser(): (expr: string) => string[] {
+  const parser = peg.generate(grammar);
+  return (expr: string) => {
+    try {
+      return parser.parse(expr);
+    } catch (e) {
+      // I haven't actually been able to write a testcase that triggers this
+      // code path, but I've included it anyway as a fallback to allow users to
+      // keep using the CLI even if this does occur. Their paths might look
+      // strange, but that's better than nothing.
+      return expr.split('.');
+    }
+  };
+}
diff --git a/src/cli/commands/test/iac-local-execution/results-formatter.ts b/src/cli/commands/test/iac-local-execution/results-formatter.ts
index 92dc80cc1bc..f171a2c1d19 100644
--- a/src/cli/commands/test/iac-local-execution/results-formatter.ts
+++ b/src/cli/commands/test/iac-local-execution/results-formatter.ts
@@ -7,6 +7,7 @@ import {
   PolicyMetadata,
   TestMeta,
 } from './types';
+import { parsePath } from './parsers/path';
 import * as path from 'path';
 import { SEVERITY } from '../../../../lib/snyk-test/common';
 import { IacProjectType } from '../../../../lib/iac/constants';
@@ -55,7 +56,7 @@ function formatScanResult(
   const formattedIssues = scanResult.violatedPolicies.map((policy) => {
     const cloudConfigPath =
       scanResult.docId !== undefined
-        ? [`[DocId: ${scanResult.docId}]`].concat(policy.msg.split('.'))
+        ? [`[DocId: ${scanResult.docId}]`].concat(parsePath(policy.msg))
         : policy.msg.split('.');
 
     const flagsRequiringLineNumber = [
diff --git a/test/jest/unit/iac-unit-tests/path-parser.spec.ts b/test/jest/unit/iac-unit-tests/path-parser.spec.ts
new file mode 100644
index 00000000000..b8e2e9bb9aa
--- /dev/null
+++ b/test/jest/unit/iac-unit-tests/path-parser.spec.ts
@@ -0,0 +1,19 @@
+import { parsePath } from '../../../../src/cli/commands/test/iac-local-execution/parsers/path';
+
+describe('parsing cloudConfigPath', () => {
+  it.each([
+    ['foo', ['foo']],
+    ['foo.bar.baz', ['foo', 'bar', 'baz']],
+    ['foo_1._bar2.baz3_', ['foo_1', '_bar2', 'baz3_']],
+    ['foo.bar[abc].baz', ['foo', 'bar[abc]', 'baz']],
+    ['foo.bar[abc.def].baz', ['foo', 'bar[abc.def]', 'baz']],
+    ["foo.bar['abc.def'].baz", ['foo', "bar['abc.def']", 'baz']],
+    ['foo.bar["abc.def"].baz', ['foo', 'bar["abc.def"]', 'baz']],
+    ["foo.bar['abc/def'].baz", ['foo', "bar['abc/def']", 'baz']],
+    ["foo.bar['abcdef'].baz", ['foo', "bar['abcdef']", 'baz']],
+    ["bar['abc.def']", ["bar['abc.def']"]],
+    ["fo%o.bar['ab$c/def'].baz", ['fo%o', "bar['ab$c/def']", 'baz']],
+  ])('%s', (input, expected) => {
+    expect(parsePath(input)).toEqual(expected);
+  });
+});