Merge pull request #603 from xiemaisi/js/fix-inconsistent-new

Approved by asger-semmle, esben-semmle

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -45,6 +45,7 @@
 | Conflicting HTML element attributes | Lower severity | The severity of this rule has been revised to "warning". |
 | Duplicate 'if' condition | Lower severity | The severity of this rule has been revised to "warning". |
 | Duplicate switch case | Lower severity | The severity of this rule has been revised to "warning". |
+| Inconsistent use of 'new' | Simpler result presentation | This rule now only shows one call with `new` and one without. |
 | Information exposure through a stack trace | More results | This rule now also flags cases where the entire exception object (including the stack trace) may be exposed. |
 | Missing 'this' qualifier | Fewer false-positive results | This rule now recognizes additional intentional calls to global functions. | 
 | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |

javascript/ql/src/LanguageFeatures/InconsistentNew.ql

@@ -56,26 +56,60 @@ predicate calls(DataFlow::InvokeNode cs, Function callee, int imprecision) {
 
 /**
  * Gets a function that may be invoked at `cs`, preferring callees that
- * are less likely to be derived due to analysis imprecision.
+ * are less likely to be derived due to analysis imprecision and excluding
+ * whitelisted call sites and callees. Additionally, `isNew` is bound to
+ * `true` if `cs` is a `new` expression, and to `false` otherwise.
  */
-Function getALikelyCallee(DataFlow::InvokeNode cs) {
-  calls(cs, result, min(int p | calls(cs, _, p)))
+Function getALikelyCallee(DataFlow::InvokeNode cs, boolean isNew) {
+  calls(cs, result, min(int p | calls(cs, _, p))) and
+  not cs.isUncertain() and
+  not whitelistedCall(cs) and
+  not whitelistedCallee(result) and
+  (cs instanceof DataFlow::NewNode and isNew = true
+   or
+   cs instanceof DataFlow::CallNode and isNew = false)
+}
+
+/**
+ * Holds if `f` should be whitelisted, either because it guards against
+ * inconsistent `new` or we do not want to report it.
+ */
+predicate whitelistedCallee(Function f) {
+  // externs are special, so don't flag them
+  f.inExternsFile() or
+  // illegal constructor calls are flagged by query 'Illegal invocation',
+  // so don't flag them
+  f instanceof Constructor or
+  // if `f` itself guards against missing `new`, don't flag it
+  guardsAgainstMissingNew(f)
+}
+
+/**
+ * Holds if `call` should be whitelisted because it cannot cause problems
+ * with inconsistent `new`.
+ */
+predicate whitelistedCall(DataFlow::CallNode call) {
+  // super constructor calls behave more like `new`, so don't flag them
+  call.asExpr() instanceof SuperCall or
+  // don't flag if there is a receiver object
+  exists(call.getReceiver())
+}
+
+/**
+ * Get the `new` or call (depending on whether `isNew` is true or false) of `f`
+ * that comes first under a lexicographical ordering by file path, start line
+ * and start column.
+ */
+DataFlow::InvokeNode getFirstInvocation(Function f, boolean isNew) {
+  result = min(DataFlow::InvokeNode invk, string path, int line, int col |
+    f = getALikelyCallee(invk, isNew) and invk.hasLocationInfo(path, line, col, _, _) |
+    invk order by path, line, col
+  )
 }
 
 from Function f, DataFlow::NewNode new, DataFlow::CallNode call
-where // externs are special, so don't flag them
-      not f.inExternsFile() and
-      // illegal constructor calls are flagged by query 'Illegal invocation',
-      // so don't flag them
-      not f instanceof Constructor and
-      f = getALikelyCallee(new) and
-      f = getALikelyCallee(call) and
-      not guardsAgainstMissingNew(f) and
-      not new.isUncertain() and
-      not call.isUncertain() and
-      // super constructor calls behave more like `new`, so don't flag them
-      not call.asExpr() instanceof SuperCall and
-      // don't flag if there is a receiver object
-      not exists(call.getReceiver())
-select (FirstLineOf)f, capitalize(f.describe()) + " is invoked as a constructor here $@, " +
-      "and as a normal function here $@.", new, new.toString(), call, call.toString()
+where new = getFirstInvocation(f, true) and
+      call = getFirstInvocation(f, false)
+select (FirstLineOf)f, capitalize(f.describe()) + " is sometimes invoked as a constructor " +
+      "(for example $@), and sometimes as a normal function (for example $@).",
+      new, "here", call, "here"

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

@@ -1,2 +1,2 @@
-| m.js:1:8:1:22 | functio ...  = x;\\n} | Function A is invoked as a constructor here $@, and as a normal function here $@. | c1.js:2:1:2:9 | new A(42) | new A(42) | c2.js:2:1:2:5 | A(23) | A(23) |
-| tst.js:1:1:1:22 | functio ...  = y;\\n} | Function Point is invoked as a constructor here $@, and as a normal function here $@. | tst.js:6:1:6:17 | new Point(23, 42) | new Point(23, 42) | tst.js:7:1:7:13 | Point(56, 72) | Point(56, 72) |
+| m.js:1:8:1:22 | functio ...  = x;\\n} | Function A is sometimes invoked as a constructor (for example $@), and sometimes as a normal function (for example $@). | c1.js:2:1:2:9 | new A(42) | here | c2.js:2:1:2:5 | A(23) | here |
+| tst.js:1:1:1:22 | functio ...  = y;\\n} | Function Point is sometimes invoked as a constructor (for example $@), and sometimes as a normal function (for example $@). | tst.js:6:1:6:17 | new Point(23, 42) | here | tst.js:7:1:7:13 | Point(56, 72) | here |

javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/tst.js

@@ -63,3 +63,6 @@ C(); // NOT OK, but flagged by IllegalInvocation
 	new A(42);
 	A.call({}, 23);
 })();
+
+new Point(42, 23); // NOT OK, but not flagged since line 6 above was already flagged
+Point(56, 72);     // NOT OK, but not flagged since line 7 above was already flagged