JS: Move ".."-parsing trick into AccessPathSyntax.qll

Author: asgerf

javascript/ql/lib/semmle/javascript/frameworks/data/internal/AccessPathSyntax.qll

@@ -22,15 +22,31 @@ module AccessPath {
 class AccessPath extends string instanceof AccessPath::Range {
   /** Gets the `n`th token on the access path as a string. */
   string getRawToken(int n) {
-    this != "" and // The empty path should have zero tokens, not a single empty token
-    result = this.splitAt(".", n)
+    // Avoid splitting by '.' since tokens may contain dots, e.g. `Field[foo.Bar.x]`.
+    // Instead use regexpFind to match valid tokens, and supplement with a final length
+    // check to ensure all characters were included in a token.
+    result = this.regexpFind("\\w+(?:\\[[^\\]]*\\])?(?=\\.|$)", n, _)
   }
 
-  /** Gets the `n`th token on the access path. */
-  AccessPathToken getToken(int n) { result = this.getRawToken(n) }
+  /** Holds if this string is not a syntactically valid access path. */
+  predicate hasSyntaxError() {
+    // If the lengths match, all characters must haven been included in a token
+    // or seen by the `.` lookahead pattern.
+    this != "" and
+    not this.length() = sum(int n | | getRawToken(n).length() + 1) - 1
+  }
+
+  /** Gets the `n`th token on the access path (if there are no syntax errors). */
+  AccessPathToken getToken(int n) {
+    result = this.getRawToken(n) and
+    not hasSyntaxError()
+  }
 
-  /** Gets the number of tokens on the path. */
-  int getNumToken() { result = count(int n | exists(this.getRawToken(n))) }
+  /** Gets the number of tokens on the path (if there are no syntax errors). */
+  int getNumToken() {
+    result = count(int n | exists(this.getRawToken(n))) and
+    not hasSyntaxError()
+  }
 }
 
 /**

javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll

@@ -163,19 +163,13 @@ private predicate summaryModel(string row) { any(SummaryModelCsv s).row(inverseP
 
 private predicate typeModel(string row) { any(TypeModelCsv s).row(inversePad(row)) }
 
-/**
- * Replaces `..` with `-->` in order to simplify subsequent parsing.
- */
-bindingset[path]
-private string normalizePath(string path) { result = path.replaceAll("..", "-->") }
-
 /** Holds if a source model exists for the given parameters. */
 predicate sourceModel(string package, string type, string path, string kind) {
   exists(string row |
     sourceModel(row) and
     row.splitAt(";", 0) = package and
     row.splitAt(";", 1) = type and
-    normalizePath(row.splitAt(";", 2)) = path and
+    row.splitAt(";", 2) = path and
     row.splitAt(";", 3) = kind
   )
 }
@@ -186,7 +180,7 @@ private predicate sinkModel(string package, string type, string path, string kin
     sinkModel(row) and
     row.splitAt(";", 0) = package and
     row.splitAt(";", 1) = type and
-    normalizePath(row.splitAt(";", 2)) = path and
+    row.splitAt(";", 2) = path and
     row.splitAt(";", 3) = kind
   )
 }
@@ -199,9 +193,9 @@ private predicate summaryModel(
     summaryModel(row) and
     row.splitAt(";", 0) = package and
     row.splitAt(";", 1) = type and
-    normalizePath(row.splitAt(";", 2)) = path and
-    normalizePath(row.splitAt(";", 3)) = input and
-    normalizePath(row.splitAt(";", 4)) = output and
+    row.splitAt(";", 2) = path and
+    row.splitAt(";", 3) = input and
+    row.splitAt(";", 4) = output and
     row.splitAt(";", 5) = kind
   )
 }
@@ -216,7 +210,7 @@ private predicate typeModel(
     row.splitAt(";", 1) = type1 and
     row.splitAt(";", 2) = package2 and
     row.splitAt(";", 3) = type2 and
-    normalizePath(row.splitAt(";", 4)) = path
+    row.splitAt(";", 4) = path
   )
 }
 
@@ -434,9 +428,9 @@ bindingset[arg]
 private int getAnIntFromString(string arg) {
   result = arg.toInt()
   or
-  // Match "n1..n2", where ".." has previously been replaced with "-->" to simplify parsing
+  // Match "n1..n2"
   exists(string lo, string hi |
-    regexpCaptureTwo(arg, "(\\d+)-->(\\d+)", lo, hi) and
+    regexpCaptureTwo(arg, "(\\d+)\\.\\.(\\d+)", lo, hi) and
     result = [lo.toInt() .. hi.toInt()]
   )
 }
@@ -446,8 +440,8 @@ private int getAnIntFromString(string arg) {
  */
 bindingset[arg]
 private int getLowerBoundFromString(string arg) {
-  // Match "n..", where ".." has previously been replaced with "-->" to simplify parsing
-  result = arg.regexpCapture("(\\d+)-->", 1).toInt()
+  // Match "n.."
+  result = arg.regexpCapture("(\\d+)\\.\\.", 1).toInt()
 }
 
 /**
@@ -479,22 +473,22 @@ private int getAnIntFromStringWithArity(string arg, int arity) {
     result = arity - lo.toInt()
     or
     // N-x..
-    lo = arg.regexpCapture("N-(\\d+)-->", 1) and
+    lo = arg.regexpCapture("N-(\\d+)\\.\\.", 1) and
     result = [arity - lo.toInt(), arity - 1]
   )
   or
   exists(string lo, string hi |
     // x..N-y
-    regexpCaptureTwo(arg, "(\\d+)-->N-(\\d+)", lo, hi) and
+    regexpCaptureTwo(arg, "(\\d+)\\.\\.N-(\\d+)", lo, hi) and
     result = [lo.toInt() .. arity - hi.toInt()]
     or
     // N-x..Ny
-    regexpCaptureTwo(arg, "N-(\\d+)-->N-(\\d+)", lo, hi) and
+    regexpCaptureTwo(arg, "N-(\\d+)\\.\\.N-(\\d+)", lo, hi) and
     result = [arity - lo.toInt() .. arity - hi.toInt()] and
     result >= 0
     or
     // N-x..y
-    regexpCaptureTwo(arg, "N-(\\d+)-->(\\d+)", lo, hi) and
+    regexpCaptureTwo(arg, "N-(\\d+)\\.\\.(\\d+)", lo, hi) and
     result = [arity - lo.toInt() .. hi.toInt()] and
     result >= 0
   )

javascript/ql/test/library-tests/frameworks/data/test.expected

@@ -71,3 +71,14 @@ isSink
 | test.js:78:34:78:34 | 3 | test-sink |
 | test.js:81:28:81:35 | source() | test-sink |
 | test.js:82:28:82:28 | 1 | test-sink |
+syntaxErrors
+| Member[foo |
+| Member[foo] .Member[bar] |
+| Member[foo] Member[bar] |
+| Member[foo], Member[bar] |
+| Member[foo],Member[bar] |
+| Member[foo]. Member[bar] |
+| Member[foo]..Member[bar] |
+| Member[foo]Member[bar] |
+| Member[foo]] |
+| Member[foo]].Member[bar] |

javascript/ql/test/library-tests/frameworks/data/test.ql

@@ -1,5 +1,6 @@
 import javascript
 import testUtilities.ConsistencyChecking
+import semmle.javascript.frameworks.data.internal.AccessPathSyntax as AccessPathSyntax
 
 class Steps extends ModelInput::SummaryModelCsv {
   override predicate row(string row) {
@@ -54,3 +55,24 @@ query predicate taintFlow(DataFlow::Node source, DataFlow::Node sink) {
 query predicate isSink(DataFlow::Node node, string kind) {
   node = ModelOutput::getASinkNode(kind).getARhs()
 }
+
+class SyntaxErrorTest extends ModelInput::SinkModelCsv {
+  override predicate row(string row) {
+    row = [
+      "testlib;;Member[foo],Member[bar];test-sink",
+      "testlib;;Member[foo] Member[bar];test-sink",
+      "testlib;;Member[foo]. Member[bar];test-sink",
+      "testlib;;Member[foo], Member[bar];test-sink",
+      "testlib;;Member[foo]..Member[bar];test-sink",
+      "testlib;;Member[foo] .Member[bar];test-sink",
+      "testlib;;Member[foo]Member[bar];test-sink",
+      "testlib;;Member[foo;test-sink",
+      "testlib;;Member[foo]];test-sink",
+      "testlib;;Member[foo]].Member[bar];test-sink"
+    ]
+  }
+}
+
+query predicate syntaxErrors(AccessPathSyntax::AccessPath path) {
+  path.hasSyntaxError()
+}