Merge pull request #2056 from erik-krogh/suspiciousMethodName

JS: add query for detecting suspicious method names in TypeScript

Author: Esben Sparre Andreasen

change-notes/1.23/analysis-javascript.md

@@ -16,6 +16,7 @@
 |---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Unused index variable (`js/unused-index-variable`)                        | correctness                                                       | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
 | Loop bound injection (`js/loop-bound-injection`)                          | security, external/cwe/cwe-834                                      | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
+| Suspicious method name (`js/suspicious-method-name-declaration`)          | correctness, typescript, methods                                  | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

javascript/ql/src/Declarations/SuspiciousMethodNameDeclaration.qhelp

@@ -0,0 +1,55 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+In TypeScript the keywords <code>constructor</code> and <code>new</code> for 
+member declarations are used to declare constructors in classes and interfaces 
+respectively. 
+However, a member declaration with the name <code>new</code> in an interface 
+or <code>constructor</code> in a class, will declare an ordinary method named 
+<code>new</code> or <code>constructor</code> rather than a constructor.
+Similarly, the keyword <code>function</code> is used to declare functions in 
+some contexts. However, using the name <code>function</code> for a class 
+or interface member declaration declares a method named <code>function</code>.    
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Declare classes as classes and not as interfaces.
+Use the keyword <code>constructor</code> to declare constructors in a class,
+use the keyword <code>new</code> to declare constructors inside interfaces, 
+and don't use <code>function</code> when declaring a call signature in an 
+interface.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+The below example declares an interface <code>Point</code> with 2 fields 
+and a method called <code>constructor</code>. The interface does not declare
+a class <code>Point</code> with a constructor, which was likely what the 
+developer meant to create.
+</p>
+<sample src="examples/SuspiciousMethodNameDeclaration.ts" />
+
+<p>
+The below example is a fixed version of the above, where the interface is 
+instead declared as a class, thereby describing the type the developer meant 
+in the first place.
+</p>
+
+<sample src="examples/SuspiciousMethodNameDeclarationFixed.ts" />
+
+</example>
+<references>
+
+<li>TypeScript specification: <a href="https://github.com/microsoft/TypeScript/blob/master/doc/spec.md#3.8.9">Constructor Type Literals</a>.</li>
+<li>TypeScript specification: <a href="https://github.com/microsoft/TypeScript/blob/master/doc/spec.md#8.3.1">Constructor Parameters</a>.</li>
+
+</references>
+</qhelp>

javascript/ql/src/Declarations/SuspiciousMethodNameDeclaration.ql

@@ -0,0 +1,69 @@
+/**
+ * @name Suspicious method name declaration
+ * @description A method declaration with a name that is a special keyword in another 
+ *              context is suspicious. 
+ * @kind problem
+ * @problem.severity warning
+ * @id js/suspicious-method-name-declaration
+ * @precision high
+ * @tags correctness
+ *       typescript
+ *       methods
+ */
+
+import javascript
+
+/**
+ * Holds if the method name on the given container is likely to be a mistake.
+ */
+predicate isSuspiciousMethodName(string name, ClassOrInterface container) {
+  name = "function"
+  or
+  // "constructor" is only suspicious outside a class.  
+  name = "constructor" and not container instanceof ClassDefinition
+  or
+  // "new" is only suspicious inside a class.
+  name = "new" and container instanceof ClassDefinition
+}
+
+from MethodDeclaration member, ClassOrInterface container, string name, string msg
+where
+  container.getLocation().getFile().getFileType().isTypeScript() and
+  container.getMember(name) = member and
+  isSuspiciousMethodName(name, container) and
+  
+  // Cases to ignore.
+  not (
+    // Assume that a "new" method is intentional if the class has an explicit constructor.
+    name = "new" and
+    container instanceof ClassDefinition and
+    exists(ConstructorDeclaration constructor |
+      container.getMember("constructor") = constructor and
+      not constructor.isSynthetic() 
+    ) 
+    or
+    // Explicitly declared static methods are fine.
+    container instanceof ClassDefinition and
+    member.isStatic()
+    or
+    // Only looking for declared methods. Methods with a body are OK. 
+    exists(member.getBody().getBody())
+    or
+    // The developer was not confused about "function" when there are other methods in the interface.
+    name = "function" and 
+    exists(MethodDeclaration other | other = container.getAMethod() |
+      other.getName() != "function" and
+      not other.(ConstructorDeclaration).isSynthetic()
+    )
+  )
+  
+  and
+  
+  (
+    name = "constructor" and msg = "The member name 'constructor' does not declare a constructor in interfaces, but it does in classes." 
+    or
+    name = "new" and msg = "The member name 'new' does not declare a constructor, but 'constructor' does in class declarations."
+    or
+    name = "function" and msg = "The member name 'function' does not declare a function, it declares a method named 'function'."
+  )
+select member, msg

javascript/ql/src/Declarations/examples/SuspiciousMethodNameDeclaration.ts

@@ -0,0 +1,6 @@
+declare class Point {
+   x: number;
+   y: number;
+   constructor(x : number, y: number);
+}
+

javascript/ql/src/Declarations/examples/SuspiciousMethodNameDeclarationFixed.ts

@@ -0,0 +1,4 @@
+interface Point {
+   x: number;
+   y: number;
+}

javascript/ql/test/query-tests/Declarations/SuspiciousMethodNameDeclaration/SuspiciousMethodNameDeclaration.expected

@@ -0,0 +1,4 @@
+| tst.ts:7:3:7:24 | constru ... string; | The member name 'constructor' does not declare a constructor in interfaces, but it does in classes. |
+| tst.ts:16:3:16:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:37:3:37:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
+| tst.ts:48:3:48:13 | new(): Quz; | The member name 'new' does not declare a constructor, but 'constructor' does in class declarations. |

javascript/ql/test/query-tests/Declarations/SuspiciousMethodNameDeclaration/SuspiciousMethodNameDeclaration.qlref

@@ -0,0 +1 @@
+Declarations/SuspiciousMethodNameDeclaration.ql

javascript/ql/test/query-tests/Declarations/SuspiciousMethodNameDeclaration/tst.js

@@ -0,0 +1,19 @@
+ // OK: don't report anything in .js files.
+function getStuff(number) {
+    return {
+        "new": function() {
+            
+        },
+        "constructor": 123,
+        "function": "this is a string!"
+    }
+}
+
+class Foobar {
+    new() {
+        return 123;
+    }
+    function() {
+        return "string";
+    }
+}

javascript/ql/test/query-tests/Declarations/SuspiciousMethodNameDeclaration/tst.ts

@@ -0,0 +1,52 @@
+var foo: MyInterface = 123 as any;
+
+interface MyInterface {
+  function (): number; // OK. Highly unlikely that it is an accident when there are other named methods in the interface.
+  (): number; // OK: What was probably meant above. 
+  new:() => void; // OK! This is a property, not a method, we ignore those.
+  constructor(): string; // NOT OK! This a called "constructor"
+  new(): Date; // OK! This a constructor signature.
+
+  myNumber: 123; 
+}
+
+var a : MyFunction = null as any;
+
+interface MyFunction {
+  function(): number; // NOT OK!
+}
+
+
+class Foo {
+  new(): number { // OK! Highly unlikely that a developer confuses "constructor" and "new" when both are present. 
+    return 123;
+  }
+  constructor() { // OK! This is a constructor.
+
+  }
+  myString = "foobar"
+  
+  myMethod(): boolean {
+    return Math.random() > 0.5;
+  }
+}
+
+var b : FunctionClass = new FunctionClass();
+
+declare class FunctionClass {
+  function(): number; // NOT OK:
+}
+
+class Baz {
+    new(): Baz { // OK! When there is a method body I assume the developer knows what they are doing.
+        return null as any;
+    }
+}
+
+
+declare class Quz {
+  new(): Quz; // NOT OK! The developer likely meant to write constructor.
+}
+
+var bla = new Foo();
+var blab = new Baz();