Merge pull request github#3721 from asger-semmle/js/non-linear-pattern-msg

JS: Improve alert message in js/non-linear-pattern

Author: erik-krogh

change-notes/1.25/analysis-javascript.md

@@ -46,6 +46,7 @@
 | Hard-coded credentials (`js/hardcoded-credentials`) | More results | This query now recognizes hard-coded credentials sent via HTTP authorization headers. |
 | Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |
 | Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
+| Non-linear pattern (`js/non-linear-pattern`) | Fewer duplicates and message changed | This query now generates fewer duplicate alerts and has a clearer explanation in case of type annotations used in a pattern. |
 | Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |
 | Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
 | Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |

javascript/ql/src/LanguageFeatures/NonLinearPattern.qhelp

@@ -8,6 +8,11 @@ If the same pattern variable is bound multiple times in the same object or array
 binding overwrites all of the earlier ones. This is most likely unintended and should be avoided.
 </p>
 
+<p>
+In TypeScript, a common mistake is to try to write type annotations inside a pattern. This is not
+possible, and the type annotation should come after the pattern.
+</p>
+
 </overview>
 <recommendation>
 
@@ -34,6 +39,21 @@ From context, it appears that the second binding should have been for variable <
 
 <sample src="examples/NonLinearPatternGood.js" />
 
+<p>
+This can sometimes happen in TypeScript, due to the apparant similarity between property patterns
+and type annotations. In the following example, the function uses a pattern parameter with properties <code>x</code>
+and <code>y</code>. These appear to have type <code>number</code>, but are in fact untyped properties both stored in a variable named <code>number</code>.
+</p>
+
+<sample src="examples/NonLinearPatternTS.ts" />
+
+<p>
+It is not possible to specify type annotations inside a pattern. The correct way is to specify the type
+after the parameter:
+</p>
+
+<sample src="examples/NonLinearPatternTSGood.ts" />
+
 </example>
 <references>
 <li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment">Destructuring assignment</a>.</li>

javascript/ql/src/LanguageFeatures/NonLinearPattern.ql

@@ -14,12 +14,55 @@
 
 import javascript
 
-from BindingPattern p, string n, VarDecl v, VarDecl w
+class RootDestructuringPattern extends BindingPattern {
+  RootDestructuringPattern() {
+    this instanceof DestructuringPattern and
+    not this = any(PropertyPattern p).getValuePattern() and
+    not this = any(ArrayPattern p).getAnElement()
+  }
+
+  /** Holds if this pattern has multiple bindings for `name`. */
+  predicate hasConflictingBindings(string name) {
+    exists(VarRef v, VarRef w |
+      v = getABindingVarRef() and
+      w = getABindingVarRef() and
+      name = v.getName() and
+      name = w.getName() and
+      v != w
+    )
+  }
+
+  /** Gets the first occurrence of the conflicting binding `name`. */
+  VarDecl getFirstClobberedVarDecl(string name) {
+    hasConflictingBindings(name) and
+    result =
+      min(VarDecl decl |
+        decl = getABindingVarRef() and decl.getName() = name
+      |
+        decl order by decl.getLocation().getStartLine(), decl.getLocation().getStartColumn()
+      )
+  }
+
+  /** Holds if variables in this pattern may resemble type annotations. */
+  predicate resemblesTypeAnnotation() {
+    hasConflictingBindings(_) and // Restrict size of predicate.
+    this instanceof Parameter and
+    this instanceof ObjectPattern and
+    not exists(getTypeAnnotation()) and
+    getFile().getFileType().isTypeScript()
+  }
+}
+
+from RootDestructuringPattern p, string n, VarDecl v, VarDecl w, string message
 where
-  v = p.getABindingVarRef() and
+  v = p.getFirstClobberedVarDecl(n) and
   w = p.getABindingVarRef() and
-  v.getName() = n and
   w.getName() = n and
   v != w and
-  v.getLocation().startsBefore(w.getLocation())
-select w, "Repeated binding of pattern variable '" + n + "' previously bound $@.", v, "here"
+  if p.resemblesTypeAnnotation()
+  then
+    message =
+      "The pattern variable '" + n +
+        "' appears to be a type, but is a variable previously bound $@."
+  else message = "Repeated binding of pattern variable '" + n + "' previously bound $@."
+select w, message, v, "here"

javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTS.ts

@@ -0,0 +1,3 @@
+function distance({x: number, y: number}) {
+    return Math.sqrt(x*x + y*y);
+}

javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTSGood.ts

@@ -0,0 +1,3 @@
+function distance({x, y}: {x: number, y: number}) {
+    return Math.sqrt(x*x + y*y);
+}

javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPattern.expected

@@ -1,6 +1,11 @@
+| NonLinearPatternTS.ts:1:34:1:39 | number | The pattern variable 'number' appears to be a type, but is a variable previously bound $@. | NonLinearPatternTS.ts:1:23:1:28 | number | here |
 | ts-test.ts:3:13:3:13 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:3:10:3:10 | x | here |
 | ts-test.ts:8:16:8:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:8:10:8:10 | x | here |
 | ts-test.ts:11:10:11:10 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:11:7:11:7 | x | here |
+| ts-test.ts:21:8:21:13 | string | The pattern variable 'string' appears to be a type, but is a variable previously bound $@. | ts-test.ts:20:8:20:13 | string | here |
+| ts-test.ts:32:16:32:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:30:12:30:12 | x | here |
+| ts-test.ts:34:20:34:20 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:30:12:30:12 | x | here |
+| ts-test.ts:40:27:40:32 | string | Repeated binding of pattern variable 'string' previously bound $@. | ts-test.ts:40:16:40:21 | string | here |
 | tst.js:3:13:3:13 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:3:10:3:10 | x | here |
 | tst.js:8:16:8:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:8:10:8:10 | x | here |
 | tst.js:11:10:11:10 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:11:7:11:7 | x | here |

javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTS.ts

@@ -0,0 +1,3 @@
+function distance({x: number, y: number}) {
+    return Math.sqrt(x*x + y*y);
+}

javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTSGood.ts

@@ -0,0 +1,3 @@
+function distance({x, y}: {x: number, y: number}) {
+    return Math.sqrt(x*x + y*y);
+}

javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/ts-test.ts

@@ -15,3 +15,27 @@ var { x: x, x: y } = o;
 
 // OK
 var { p = x, q = x } = o;
+
+function f({
+    x: string,
+    y: string  // NOT OK
+}) {
+}
+
+function g({x, y}: {x: string, y: string}) { // OK
+}
+
+function blah(arg) {
+    var {
+        x: x,
+        y: {
+            x: x, // NOT OK
+            y: {
+                x: x // NOT OK
+            }
+        }
+    } = arg;
+}
+
+function h({x: string, y: string}: any) {  // NOT OK
+}