Merge rc/1.18 into master.

javascript/ql/src/Expressions/SuspiciousInvocation.ql

@@ -15,5 +15,6 @@ private import semmle.javascript.dataflow.InferredTypes
 
 from InvokeExpr invk, DataFlow::AnalyzedNode callee
 where callee.asExpr() = invk.getCallee() and
-      forex (InferredType tp | tp = callee.getAType() | tp != TTFunction() and tp != TTClass())
+      forex (InferredType tp | tp = callee.getAType() | tp != TTFunction() and tp != TTClass()) and
+      not invk.isAmbient()
 select invk, "Callee is not a function: it has type " + callee.ppTypes() + "."

javascript/ql/src/Expressions/SuspiciousPropAccess.ql

@@ -31,5 +31,6 @@ predicate namespaceOrConstEnumAccess(VarAccess e) {
 from PropAccess pacc, DataFlow::AnalyzedNode base
 where base.asExpr() = pacc.getBase() and
       forex (InferredType tp | tp = base.getAType() | tp = TTNull() or tp = TTUndefined()) and
-      not namespaceOrConstEnumAccess(pacc.getBase())
+      not namespaceOrConstEnumAccess(pacc.getBase()) and
+      not pacc.isAmbient()
 select pacc, "The base expression of this property access is always " + base.ppTypes() + "."

javascript/ql/src/Security/CWE-116/IncompleteSanitization.qhelp

@@ -20,6 +20,10 @@ sanitization. In the latter case, preceding a meta-character with a backslash le
 backslash being escaped, but the meta-character appearing un-escaped, which again makes the
 sanitization ineffective.
 </p>
+<p>
+Even if the escaped string is not used in a security-critical context, incomplete escaping may
+still have undesirable effects, such as badly rendered or confusing output.
+</p>
 </overview>
 
 <recommendation>

javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql

@@ -1,7 +1,7 @@
 /**
- * @name Incomplete sanitization
- * @description A sanitizer that does not replace or escape all occurrences of a
- *              problematic substring may be ineffective.
+ * @name Incomplete string escaping or encoding
+ * @description A string transformer that does not replace or escape all occurrences of a
+ *              meta-character may be ineffective.
  * @kind problem
  * @problem.severity warning
  * @precision high
@@ -16,9 +16,6 @@ import javascript
 
 /**
  * Gets a character that is commonly used as a meta-character.
- *
- * We heuristically assume that string replacements involving one of these
- * characters are meant to be sanitizers.
  */
 string metachar() {
   result = "'\"\\&<>\n\r\t*|{}[]%$".charAt(_)
@@ -75,7 +72,7 @@ predicate isBackslashEscape(MethodCallExpr mce, RegExpLiteral re) {
 }
 
 /**
- * Holds if data flowing into `nd` has no unescaped backslashes.
+ * Holds if data flowing into `nd` has no un-escaped backslashes.
  */
 predicate allBackslashesEscaped(DataFlow::Node nd) {
   // `JSON.stringify` escapes backslashes

javascript/ql/src/semmle/javascript/AST.qll

@@ -208,6 +208,11 @@ class TopLevel extends @toplevel, StmtContainer {
   override string toString() {
     result = "<toplevel>"
   }
+
+  override predicate isAmbient() {
+    getFile().getFileType().isTypeScript() and
+    getFile().getBaseName().matches("%.d.ts")
+  }
 }
 
 /**

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -214,6 +214,9 @@ module TaintTracking {
           m.getMethodName() = "map" and
           m.getArgument(0) = f and // Require the argument to be a closure to avoid spurious call/return flow
           pred = f.getAReturnedExpr().flow())
+        or
+        // `array.push(e)`: if `e` is tainted, then so is `array`
+        succ.(DataFlow::SourceNode).getAMethodCall("push").getAnArgument() = pred
       )
       or
       // reading from a tainted object yields a tainted result
@@ -508,6 +511,19 @@ module TaintTracking {
     }
   }
 
+  /**
+   * A taint propagating data flow edge arising from sorting.
+   */
+  private class SortTaintStep extends AdditionalTaintStep, DataFlow::MethodCallNode {
+    SortTaintStep() {
+      getMethodName() = "sort"
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      pred = getReceiver() and succ = this
+    }
+  }
+
   /**
    * A conditional checking a tainted string against a regular expression, which is
    * considered to be a sanitizer for all configurations.

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -1,2 +1,5 @@
 | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |
 | tst.js:2:13:2:20 | source() | tst.js:5:10:5:22 | "/" + x + "!" |
+| tst.js:2:13:2:20 | source() | tst.js:14:10:14:17 | x.sort() |
+| tst.js:2:13:2:20 | source() | tst.js:17:10:17:10 | a |
+| tst.js:2:13:2:20 | source() | tst.js:19:10:19:10 | a |

javascript/ql/test/library-tests/TaintTracking/tst.js

@@ -10,4 +10,12 @@ function test() {
     sink(x === 1); // OK
     sink(undefined == x); // OK
     sink(x === x); // OK
+
+    sink(x.sort()); // NOT OK
+
+    var a = [];
+    sink(a); // NOT OK (flow-insensitive treatment of `a`)
+    a.push(x);
+    sink(a); // NOT OK
+
 }

javascript/ql/test/query-tests/Expressions/SuspiciousInvocation/ambient_extends.d.ts

@@ -0,0 +1,3 @@
+export class Subclass extends BaseClass {} // OK - ambient context
+
+export class BaseClass {}