Merge remote-tracking branch 'upstream/master' into UrlSearch

javascript/ql/examples/queries/dataflow/StoredXss/StoredXss.ql

@@ -22,7 +22,8 @@ import DataFlow::PathGraph
  */
 class MysqlSource extends StoredXss::Source {
   MysqlSource() {
-    this = moduleImport("mysql")
+    this =
+      moduleImport("mysql")
           .getAMemberCall("createConnection")
           .getAMethodCall("query")
           .getCallback(1)

javascript/ql/src/AngularJS/DependencyMismatch.ql

@@ -24,8 +24,9 @@ where
     exists(string n | p = f.getDependencyParameter(n) |
       p.getName() != n and
       exists(f.getDependencyParameter(p.getName())) and
-      msg = "This parameter is named '" + p.getName() + "', " +
-          "but actually refers to dependency '" + n + "'."
+      msg =
+        "This parameter is named '" + p.getName() + "', " + "but actually refers to dependency '" +
+          n + "'."
     )
   )
 select p, msg

javascript/ql/src/AngularJS/DoubleCompilation.ql

@@ -15,7 +15,8 @@ import javascript
 from AngularJS::ServiceReference compile, SimpleParameter elem, CallExpr c
 where
   compile.getName() = "$compile" and
-  elem = any(AngularJS::CustomDirective d)
+  elem =
+    any(AngularJS::CustomDirective d)
         .getALinkFunction()
         .(AngularJS::LinkFunction)
         .getElementParameter() and

javascript/ql/src/AngularJS/IncompatibleService.ql

@@ -130,7 +130,8 @@ where
   kind = getServiceKind(request, name) and
   exists(request.getAServiceDefinition(name)) and // ignore unknown/undefined services
   not isCompatibleRequestedService(request, kind) and
-  compatibleWithString = concat(string compatibleKind |
+  compatibleWithString =
+    concat(string compatibleKind |
       isCompatibleRequestedService(request, compatibleKind) and
       not isWildcardKind(compatibleKind)
     |

javascript/ql/src/AngularJS/UnusedAngularDependency.ql

@@ -34,8 +34,9 @@ predicate isMissingParameter(AngularJS::InjectableFunction f, string msg, ASTNod
         then dependenciesAreString = "dependency is"
         else dependenciesAreString = "dependencies are"
       ) and
-      msg = "This function has " + paramCount + " " + parametersString + ", but " + injectionCount +
-          " " + dependenciesAreString + " injected into it."
+      msg =
+        "This function has " + paramCount + " " + parametersString + ", but " + injectionCount + " "
+          + dependenciesAreString + " injected into it."
     )
   )
 }

javascript/ql/src/Declarations/Declarations.qll

@@ -31,7 +31,8 @@ VarRef refInContainer(Variable var, RefKind kind, StmtContainer sc) {
  * declaration of `var` (if `kind` is `Decl()`) in `sc`.
  */
 VarRef firstRefInContainer(Variable var, RefKind kind, StmtContainer sc) {
-  result = min(refInContainer(var, kind, sc) as ref
+  result =
+    min(refInContainer(var, kind, sc) as ref
       order by
         ref.getLocation().getStartLine(), ref.getLocation().getStartColumn()
     )
@@ -52,7 +53,8 @@ VarRef refInTopLevel(Variable var, RefKind kind, TopLevel tl) {
  * declaration of `var` (if `kind` is `Decl()`) in `tl`.
  */
 VarRef firstRefInTopLevel(Variable var, RefKind kind, TopLevel tl) {
-  result = min(refInTopLevel(var, kind, tl) as ref
+  result =
+    min(refInTopLevel(var, kind, tl) as ref
       order by
         ref.getLocation().getStartLine(), ref.getLocation().getStartColumn()
     )

javascript/ql/src/Declarations/MissingVarDecl.ql

@@ -57,7 +57,8 @@ GlobalVarAccess getAccessIn(GlobalVariable v, Function f) {
  * Gets the (lexically) first access to variable `v` in function `f`.
  */
 GlobalVarAccess getFirstAccessIn(GlobalVariable v, Function f) {
-  result = min(getAccessIn(v, f) as gva
+  result =
+    min(getAccessIn(v, f) as gva
       order by
         gva.getLocation().getStartLine(), gva.getLocation().getStartColumn()
     )

javascript/ql/src/Declarations/SuspiciousMethodNameDeclaration.ql

@@ -1,7 +1,7 @@
 /**
  * @name Suspicious method name declaration
- * @description A method declaration with a name that is a special keyword in another 
- *              context is suspicious. 
+ * @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
@@ -19,7 +19,7 @@ import javascript
 predicate isSuspiciousMethodName(string name, ClassOrInterface container) {
   name = "function"
   or
-  // "constructor" is only suspicious outside a class.  
+  // "constructor" is only suspicious outside a class.
   name = "constructor" and not container instanceof ClassDefinition
   or
   // "new" is only suspicious inside a class.
@@ -31,7 +31,6 @@ 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.
@@ -39,31 +38,34 @@ where
     container instanceof ClassDefinition and
     exists(ConstructorDeclaration constructor |
       container.getMember("constructor") = constructor and
-      not constructor.isSynthetic() 
-    ) 
+      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. 
+    // 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 
+    name = "function" and
     exists(MethodDeclaration other | other = container.getAMethod() |
       other.getName() != "function" and
       not other.(ConstructorDeclaration).isSynthetic()
     )
-  )
-  
-  and
-  
+  ) and
   (
-    name = "constructor" and msg = "The member name 'constructor' does not declare a constructor in interfaces, but it does in classes." 
+    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."
+    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'."
+    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/UnreachableMethodOverloads.ql

@@ -44,43 +44,39 @@ string getKind(MemberDeclaration m) {
  * A call-signature that originates from a MethodSignature in the AST.
  */
 private class MethodCallSig extends CallSignatureType {
-    string name;
-    
-    MethodCallSig() {
-        exists(MethodSignature sig | 
-            this = sig.getBody().getCallSignature() and
-            name = sig.getName()
-        )
-    }
-    
-    /**
-     * Gets the name of any member that has this signature.
-     */
-    string getName() {
-        result = name
-    }
+  string name;
+
+  MethodCallSig() {
+    exists(MethodSignature sig |
+      this = sig.getBody().getCallSignature() and
+      name = sig.getName()
+    )
+  }
+
+  /**
+   * Gets the name of any member that has this signature.
+   */
+  string getName() { result = name }
 }
 
 /**
  * Holds if the two call signatures could be overloads of each other and have the same parameter types.
  */
 predicate matchingCallSignature(MethodCallSig method, MethodCallSig other) {
-  method.getName() = other.getName() and 
-  
+  method.getName() = other.getName() and
   method.getNumOptionalParameter() = other.getNumOptionalParameter() and
   method.getNumParameter() = other.getNumParameter() and
   method.getNumRequiredParameter() = other.getNumRequiredParameter() and
-  // purposely not looking at number of type arguments. 
-  
-  method.getKind() = other.getKind() and 
-  
-  
-  forall(int i | i in [0 .. -1 + method.getNumParameter()] | 
+  // purposely not looking at number of type arguments.
+  method.getKind() = other.getKind() and
+  forall(int i | i in [0 .. -1 + method.getNumParameter()] |
     method.getParameter(i) = other.getParameter(i) // This is sometimes imprecise, so it is still a good idea to compare type annotations.
-  ) and 
-  
-  // shared type parameters are equal. 
-  forall(int i | i in [0 .. -1 + min(int num | num = method.getNumTypeParameter() or num = other.getNumTypeParameter())] |
+  ) and
+  // shared type parameters are equal.
+  forall(int i |
+    i in [0 .. -1 +
+          min(int num | num = method.getNumTypeParameter() or num = other.getNumTypeParameter())]
+  |
     method.getTypeParameterBound(i) = other.getTypeParameterBound(i)
   )
 }
@@ -89,7 +85,7 @@ predicate matchingCallSignature(MethodCallSig method, MethodCallSig other) {
  * Gets which overload index the MethodSignature has among the overloads of the same name.
  */
 int getOverloadIndex(MethodSignature sig) {
-    sig.getDeclaringType().getMethodOverload(sig.getName(), result) = sig
+  sig.getDeclaringType().getMethodOverload(sig.getName(), result) = sig
 }
 
 /**
@@ -100,18 +96,22 @@ predicate signaturesMatch(MethodSignature method, MethodSignature other) {
   method.getDeclaringType() = other.getDeclaringType() and
   // same static modifier.
   getKind(method) = getKind(other) and
-
   // same name.
   method.getName() = other.getName() and
-
   // same number of parameters.
   method.getBody().getNumParameter() = other.getBody().getNumParameter() and
-  
+  // same this parameter (if exists)
+  (
+    not exists(method.getBody().getThisTypeAnnotation()) and
+    not exists(other.getBody().getThisTypeAnnotation())
+    or
+    method.getBody().getThisTypeAnnotation().getType() =
+      other.getBody().getThisTypeAnnotation().getType()
+  ) and
   // The types are compared in matchingCallSignature. This is sanity-check that the textual representation of the type-annotations are somewhat similar.
   forall(int i | i in [0 .. -1 + method.getBody().getNumParameter()] |
-    getParameterTypeAnnotation(method, i) = getParameterTypeAnnotation(other, i)   
+    getParameterTypeAnnotation(method, i) = getParameterTypeAnnotation(other, i)
   ) and
-  
   matchingCallSignature(method.getBody().getCallSignature(), other.getBody().getCallSignature())
 }
 
@@ -119,22 +119,19 @@ from ClassOrInterface decl, string name, MethodSignature previous, MethodSignatu
 where
   previous = decl.getMethod(name) and
   unreachable = getOtherMatchingSignatures(previous) and
-  
   // If the method is part of inheritance between classes/interfaces, then there can sometimes be reasons for having this pattern.
-  not exists(decl.getASuperTypeDeclaration().getMethod(name)) and 
-  not exists(ClassOrInterface sub | 
+  not exists(decl.getASuperTypeDeclaration().getMethod(name)) and
+  not exists(ClassOrInterface sub |
     decl = sub.getASuperTypeDeclaration() and
     exists(sub.getMethod(name))
   ) and
-  
-  
   // If a later method overload has more type parameters, then that overload can be selected by explicitly declaring the type arguments at the callsite.
   // This comparison removes those cases.
   unreachable.getBody().getNumTypeParameter() <= previous.getBody().getNumTypeParameter() and
-  
   // We always select the first of the overloaded methods.
   not exists(MethodSignature later | later = getOtherMatchingSignatures(previous) |
     getOverloadIndex(later) < getOverloadIndex(previous)
   )
 select unreachable,
-  "This overload of " + name + "() is unreachable, the $@ overload will always be selected.", previous, "previous"
+  "This overload of " + name + "() is unreachable, the $@ overload will always be selected.",
+  previous, "previous"

javascript/ql/src/Declarations/UnstableCyclicImport.ql

@@ -74,7 +74,8 @@ class CandidateVarAccess extends VarAccess {
  * We use this to avoid duplicate alerts about the same underlying cyclic import.
  */
 VarAccess getFirstCandidateAccess(ImportDeclaration decl) {
-  result = min(decl.getASpecifier().getLocal().getVariable().getAnAccess().(CandidateVarAccess) as p
+  result =
+    min(decl.getASpecifier().getLocal().getVariable().getAnAccess().(CandidateVarAccess) as p
       order by
         p.getFirstToken().getIndex()
     )
@@ -133,14 +134,15 @@ string pathToModule(Module source, Module destination, int steps) {
     steps > 1 and
     exists(Module next |
       // Only extend the path to one of the potential successors, as we only need one example.
-      next = min(Module mod |
+      next =
+        min(Module mod |
           isImportedAtRuntime(source, mod) and
           numberOfStepsToModule(mod, destination, steps - 1)
         |
           mod order by mod.getName()
         ) and
-      result = repr(getARuntimeImport(source, next)) + " => " +
-          pathToModule(next, destination, steps - 1)
+      result =
+        repr(getARuntimeImport(source, next)) + " => " + pathToModule(next, destination, steps - 1)
     )
   )
 }

javascript/ql/src/Expressions/ExprHasNoEffect.ql

@@ -16,7 +16,6 @@ import javascript
 import ExprHasNoEffect
 import semmle.javascript.RestrictedLocations
 
-
 from Expr e
 where hasNoEffect(e)
 select e.(FirstLineOf), "This expression has no effect."

javascript/ql/src/Expressions/HeterogeneousComparison.ql

@@ -200,10 +200,12 @@ where
   rightExprDescription = getDescription(right.asExpr(), "an expression") and
   leftTypeCount = strictcount(left.getAType()) and
   rightTypeCount = strictcount(right.getAType()) and
-  leftTypeDescription = getTypeDescription("is of type " + leftTypes,
-      "cannot be of type " + rightTypes, leftTypeCount, rightTypeCount) and
-  rightTypeDescription = getTypeDescription("of type " + rightTypes,
-      ", which cannot be of type " + leftTypes, rightTypeCount, leftTypeCount)
+  leftTypeDescription =
+    getTypeDescription("is of type " + leftTypes, "cannot be of type " + rightTypes, leftTypeCount,
+      rightTypeCount) and
+  rightTypeDescription =
+    getTypeDescription("of type " + rightTypes, ", which cannot be of type " + leftTypes,
+      rightTypeCount, leftTypeCount)
 select left,
   leftExprDescription + " " + leftTypeDescription + ", but it is compared to $@ " +
     rightTypeDescription + ".", right, rightExprDescription

javascript/ql/src/Expressions/MissingAwait.ql

@@ -43,7 +43,10 @@ predicate isBadPromiseContext(Expr expr) {
   or
   expr = any(LogicalBinaryExpr e).getLeftOperand()
   or
-  expr = any(UnaryExpr e).getOperand()
+  exists(UnaryExpr e |
+    expr = e.getOperand() and
+    not e instanceof VoidExpr
+  )
   or
   expr = any(UpdateExpr e).getOperand()
   or

javascript/ql/src/Expressions/MissingSpaceInAppend.ql

@@ -23,48 +23,40 @@ Expr leftChild(Expr e) {
 }
 
 predicate isInConcat(Expr e) {
-    exists(ParExpr par | isInConcat(par) and par.getExpression() = e)
-    or 
-    exists(AddExpr a | a.getAnOperand() = e)
+  exists(ParExpr par | isInConcat(par) and par.getExpression() = e)
+  or
+  exists(AddExpr a | a.getAnOperand() = e)
 }
 
 class ConcatenationLiteral extends Expr {
   ConcatenationLiteral() {
     (
-      this instanceof TemplateLiteral 
+      this instanceof TemplateLiteral
       or
       this instanceof Literal
-    )
-    and isInConcat(this)
+    ) and
+    isInConcat(this)
   }
 }
 
 Expr getConcatChild(Expr e) {
-    result = rightChild(e) or 
-    result = leftChild(e)
+  result = rightChild(e) or
+  result = leftChild(e)
 }
 
-Expr getConcatParent(Expr e) {
-    e = getConcatChild(result)
-}
+Expr getConcatParent(Expr e) { e = getConcatChild(result) }
 
 predicate isWordLike(ConcatenationLiteral lit) {
-    lit.getStringValue().regexpMatch("(?i).*[a-z]{3,}.*")
+  lit.getStringValue().regexpMatch("(?i).*[a-z]{3,}.*")
 }
 
 class ConcatRoot extends AddExpr {
-    ConcatRoot() {
-        not isInConcat(this)
-    }
+  ConcatRoot() { not isInConcat(this) }
 }
 
-ConcatRoot getAddRoot(AddExpr e) {
-    result = getConcatParent*(e)
-}
+ConcatRoot getAddRoot(AddExpr e) { result = getConcatParent*(e) }
 
-predicate hasWordLikeFragment(AddExpr e) {
-    isWordLike(getConcatChild*(getAddRoot(e)))
-}
+predicate hasWordLikeFragment(AddExpr e) { isWordLike(getConcatChild*(getAddRoot(e))) }
 
 from AddExpr e, ConcatenationLiteral l, ConcatenationLiteral r, string word
 where
@@ -79,7 +71,6 @@ where
   word = l.getStringValue().regexpCapture(".* (([-A-Za-z/'\\.:,]*[a-zA-Z]|[0-9]+)[\\.:,!?']*)", 1) and
   r.getStringValue().regexpMatch("[a-zA-Z].*") and
   not word.regexpMatch(".*[,\\.:].*[a-zA-Z].*[^a-zA-Z]") and
-  
   // There must be a constant-string in the concatenation that looks like a word.
   hasWordLikeFragment(e)
 select l, "This string appears to be missing a space after '" + word + "'."

javascript/ql/src/Expressions/Misspelling.qll

@@ -91,7 +91,8 @@ private string replaceATypoAndLowerCase(Identifier wrong) {
     idPart(wrong, wrongPart, offset)
   |
     normalized_typos(wrongPart, rightPart, _, _, _, _) and
-    rightName = wrong.getName().substring(0, offset) + rightPart +
+    rightName =
+      wrong.getName().substring(0, offset) + rightPart +
         wrong.getName().suffix(offset + wrongPart.length()) and
     result = rightName.toLowerCase()
   )

javascript/ql/src/Expressions/RedundantExpression.ql

@@ -64,7 +64,10 @@ class RedundantIdemnecantOperand extends RedundantOperand {
  * arguments to integers. For example, `x&x` is a common idiom for converting `x` to an integer.
  */
 class RedundantIdempotentOperand extends RedundantOperand {
-  RedundantIdempotentOperand() { getParent() instanceof LogicalBinaryExpr }
+  RedundantIdempotentOperand() {
+    getParent() instanceof LogicalBinaryExpr and
+    not exists(UpdateExpr e | e.getParentExpr+() = this)
+  }
 }
 
 /**

javascript/ql/src/Expressions/UnboundEventHandlerReceiver.ql

@@ -36,7 +36,8 @@ private predicate isBoundInMethod(MethodDeclaration method) {
       mod = "react-autobind"
     |
       thiz.flowsTo(DataFlow::moduleImport(mod).getACall().getArgument(0))
-    ) or
+    )
+    or
     // heuristic reflective binders
     exists(DataFlow::CallNode binder, string calleeName |
       (
@@ -92,8 +93,8 @@ private DOM::AttributeDefinition getAnEventHandlerAttribute() {
 from MethodDeclaration callback, DOM::AttributeDefinition attribute, ThisExpr unbound
 where
   attribute = getAnEventHandlerAttribute() and
-  attribute.getValueNode().analyze().getAValue().(AbstractFunction).getFunction() = callback
-        .getBody() and
+  attribute.getValueNode().analyze().getAValue().(AbstractFunction).getFunction() =
+    callback.getBody() and
   unbound.getBinder() = callback.getBody() and
   not isBoundInMethod(callback)
 select attribute,

javascript/ql/src/LanguageFeatures/IllegalInvocation.ql

@@ -59,7 +59,8 @@ where
   not isCallToFunction(cs) and
   // conservatively only flag call sites where _all_ callees are illegal
   forex(DataFlow::InvokeNode cs2, Function otherCallee |
-    cs2.getInvokeExpr() = cs.getInvokeExpr() and otherCallee = cs2.getACallee() |
+    cs2.getInvokeExpr() = cs.getInvokeExpr() and otherCallee = cs2.getACallee()
+  |
     illegalInvocation(cs, otherCallee, _, _)
   ) and
   // require that all callees are known

javascript/ql/src/LanguageFeatures/InconsistentNew.ql

@@ -35,7 +35,8 @@ predicate guardsAgainstMissingNew(Function f) {
  * `true` if `cs` is a `new` expression, and to `false` otherwise.
  */
 Function getALikelyCallee(DataFlow::InvokeNode cs, boolean isNew) {
-  result = min(Function callee, int imprecision |
+  result =
+    min(Function callee, int imprecision |
       callee = cs.getACallee(imprecision)
     |
       callee order by imprecision
@@ -84,7 +85,8 @@ predicate whitelistedCall(DataFlow::CallNode call) {
  * and start column.
  */
 DataFlow::InvokeNode getFirstInvocation(Function f, boolean isNew) {
-  result = min(DataFlow::InvokeNode invk, string path, int line, int col |
+  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

javascript/ql/src/LanguageFeatures/SyntaxError.ql

@@ -2,12 +2,12 @@
  * @name Syntax error
  * @description A piece of code could not be parsed due to syntax errors.
  * @kind problem
- * @problem.severity error
+ * @problem.severity recommendation
  * @id js/syntax-error
  * @tags reliability
  *       correctness
  *       language-features
- * @precision high
+ * @precision very-high
  */
 
 import javascript

javascript/ql/src/Metrics/FCyclomaticComplexity.ql

@@ -14,7 +14,8 @@ import javascript
 
 from File f, float n
 where
-  n = avg(Function fun, int toAvg |
+  n =
+    avg(Function fun, int toAvg |
       fun.getTopLevel().getFile() = f and toAvg = fun.getCyclomaticComplexity()
     |
       toAvg

javascript/ql/src/Performance/PolynomialReDoS.qhelp

@@ -0,0 +1,108 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+
+<qhelp>
+
+	<include src="ReDoSIntroduction.qhelp" />
+
+	<example>
+		<p>
+
+			Consider this use of a regular expression, which removes
+			all leading and trailing whitespace in a string:
+
+		</p>
+
+		<sample language="javascript">
+			text.replace(/^\s+|\s+$/g, ''); // BAD
+		</sample>
+
+		<p>
+
+			The sub-expression <code>"\s+$"</code> will match the
+			whitespace characters in <code>text</code> from left to right, but it
+			can start matching anywhere within a whitespace sequence. This is
+			problematic for strings that do <strong>not</strong> end with a whitespace
+			character. Such a string will force the regular expression engine to
+			process each whitespace sequence once per whitespace character in the
+			sequence.
+
+		</p>
+
+		<p>
+
+			This ultimately means that the time cost of trimming a
+			string is quadratic in the length of the string. So a string like
+			<code>"a b"</code> will take milliseconds to process, but a similar
+			string with a million spaces instead of just one will take several
+			minutes.
+
+		</p>
+
+		<p>
+
+			Avoid this problem by rewriting the regular expression to
+			not contain the ambiguity about when to start matching whitespace
+			sequences. For instance, by using a negative look-behind
+			(<code>/^\s+|(?&lt;!\s)\s+$/g</code>), or just by using the built-in trim
+			method (<code>text.trim()</code>).
+
+		</p>
+
+		<p>
+
+			Note that the sub-expression <code>"^\s+"</code> is
+			<strong>not</strong> problematic as the <code>^</code> anchor restricts
+			when that sub-expression can start matching, and as the regular
+			expression engine matches from left to right.
+
+		</p>
+
+	</example>
+
+	<example>
+
+		<p>
+
+			As a similar, but slightly subtler problem, consider the
+			regular expression that matches lines with numbers, possibly written
+			using scientific notation:
+		</p>
+
+		<sample language="javascript">
+			^0\.\d+E?\d+$ // BAD
+		</sample>
+
+		<p>
+
+			The problem with this regular expression is in the
+			sub-expression <code>\d+E?\d+</code> because the second
+			<code>\d+</code> can start matching digits anywhere after the first
+			match of the first <code>\d+</code> if there is no <code>E</code> in
+			the input string.
+
+		</p>
+
+		<p>
+
+			This is problematic for strings that do <strong>not</strong>
+			end with a digit. Such a string will force the regular expression
+			engine to process each digit sequence once per digit in the sequence,
+			again leading to a quadratic time complexity.
+
+		</p>
+
+		<p>
+
+			To make the processing faster, the regular expression
+			should be rewritten such that the two <code>\d+</code> sub-expressions
+			do not have overlapping matches: <code>^0\.\d+(E\d+)?$</code>.
+
+		</p>
+
+	</example>
+
+	<include src="ReDoSReferences.qhelp"/>
+
+</qhelp>

javascript/ql/src/Performance/PolynomialReDoS.ql

@@ -0,0 +1,22 @@
+/**
+ * @name Polynomial regular expression used on uncontrolled data
+ * @description A regular expression that can require polynomial time
+ *              to match user-provided values may be
+ *              vulnerable to denial-of-service attacks.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id js/polynomial-redos
+ * @tags security
+ *       external/cwe/cwe-730
+ *       external/cwe/cwe-400
+ */
+
+import javascript
+import semmle.javascript.security.performance.PolynomialReDoS::PolynomialReDoS
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This expensive $@ use depends on $@.",
+  sink.getNode().(Sink).getRegExp(), "regular expression", source.getNode(), "a user-provided value"

javascript/ql/src/Performance/ReDoS.qhelp

@@ -1,70 +1,34 @@
 <!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+
 <qhelp>
 
-<overview>
-<p>
-Some regular expressions take a very long time to match certain input strings to the point where
-the time it takes to match a string of length <i>n</i> is proportional to <i>2<sup>n</sup></i>.
-Such regular expressions can negatively affect performance, or even allow a malicious user to
-perform a Denial of Service ("DoS") attack by crafting an expensive input string for the regular
-expression to match.
-</p>
-<p>
-The regular expression engines provided by many popular JavaScript platforms use backtracking
-non-deterministic finite automata to implement regular expression matching. While this approach
-is space-efficient and allows supporting advanced features like capture groups, it is not
-time-efficient in general. The worst-case time complexity of such an automaton can be exponential,
-meaning that for strings of a certain shape, increasing the input length by ten characters may
-make the automaton about 1000 times slower.
-</p>
-<p>
-Typically, a regular expression is affected by this problem if it contains a repetition of the
-form <code>r*</code> or <code>r+</code> where the sub-expression <code>r</code> is ambiguous in
-the sense that it can match some string in multiple ways. More information about the precise
-circumstances can be found in the references.
-</p>
-</overview>
+	<include src="ReDoSIntroduction.qhelp" />
 
-<recommendation>
-<p>
-Modify the regular expression to remove the ambiguity.
-</p>
-</recommendation>
+	<example>
+		<p>
+			Consider this regular expression:
+		</p>
+		<sample language="javascript">
+			/^_(__|.)+_$/
+		</sample>
+		<p>
+			Its sub-expression <code>"(__|.)+?"</code> can match the string <code>"__"</code> either by the
+			first alternative <code>"__"</code> to the left of the <code>"|"</code> operator, or by two
+			repetitions of the second alternative <code>"."</code> to the right. Thus, a string consisting
+			of an odd number of underscores followed by some other character will cause the regular
+			expression engine to run for an exponential amount of time before rejecting the input.
+		</p>
+		<p>
+			This problem can be avoided by rewriting the regular expression to remove the ambiguity between
+			the two branches of the alternative inside the repetition:
+		</p>
+		<sample language="javascript">
+			/^_(__|[^_])+_$/
+		</sample>
+	</example>
 
-<example>
-<p>
-Consider this regular expression:
-</p>
-<sample language="javascript">
-/^_(__|.)+_$/
-</sample>
-<p>
-Its sub-expression <code>"(__|.)+?"</code> can match the string <code>"__"</code> either by the
-first alternative <code>"__"</code> to the left of the <code>"|"</code> operator, or by two
-repetitions of the second alternative <code>"."</code> to the right. Thus, a string consisting
-of an odd number of underscores followed by some other character will cause the regular
-expression engine to run for an exponential amount of time before rejecting the input.
-</p>
-<p>
-This problem can be avoided by rewriting the regular expression to remove the ambiguity between
-the two branches of the alternative inside the repetition:
-</p>
-<sample language="javascript">
-/^_(__|[^_])+_$/
-</sample>
-</example>
+	<include src="ReDoSReferences.qhelp"/>
 
-<references>
-<li>
-OWASP:
-<a href="https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS">Regular expression Denial of Service - ReDoS</a>.
-</li>
-<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/ReDoS">ReDoS</a>.</li>
-<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Time_complexity">Time complexity</a>.</li>
-<li>James Kirrage, Asiri Rathnayake, Hayo Thielecke:
-<a href="http://www.cs.bham.ac.uk/~hxt/research/reg-exp-sec.pdf">Static Analysis for Regular Expression Denial-of-Service Attack</a>.
-</li>
-</references>
 </qhelp>

javascript/ql/src/Performance/ReDoS.ql

@@ -292,9 +292,7 @@ class EdgeLabel extends TInputSymbol {
  * Gets the state before matching `t`.
  */
 pragma[inline]
-State before(RegExpTerm t) {
-  result = Match(t, 0)
-}
+State before(RegExpTerm t) { result = Match(t, 0) }
 
 /**
  * Gets a state the NFA may be in after matching `t`.
@@ -337,9 +335,7 @@ predicate delta(State q1, EdgeLabel lbl, State q2) {
     )
   )
   or
-  exists(RegExpDot dot |
-    q1 = before(dot) and q2 = after(dot)
-  |
+  exists(RegExpDot dot | q1 = before(dot) and q2 = after(dot) |
     if dot.getLiteral().isDotAll() then lbl = Any() else lbl = Dot()
   )
   or
@@ -545,7 +541,8 @@ string intersect(InputSymbol c, InputSymbol d) {
  * Gets a character matched by character class `cc`.
  */
 string choose(RegExpCharacterClass cc) {
-  result = min(string c |
+  result =
+    min(string c |
       exists(RegExpTerm child | child = cc.getAChild() |
         c = child.(RegExpConstant).getValue() or
         child.(RegExpCharacterRange).isRange(c, _)

javascript/ql/src/Performance/ReDoSIntroduction.qhelp

@@ -0,0 +1,55 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+	<overview>
+		<p>
+
+			Some regular expressions take a long time to match certain
+			input strings to the point where the time it takes to match a string
+			of length <i>n</i> is proportional to <i>n<sup>k</sup></i> or even
+			<i>2<sup>n</sup></i>.  Such regular expressions can negatively affect
+			performance, or even allow a malicious user to perform a Denial of
+			Service ("DoS") attack by crafting an expensive input string for the
+			regular expression to match.
+
+		</p>
+
+		<p>
+
+			The regular expression engines provided by many popular
+			JavaScript platforms use backtracking non-deterministic finite
+			automata to implement regular expression matching. While this approach
+			is space-efficient and allows supporting advanced features like
+			capture groups, it is not time-efficient in general. The worst-case
+			time complexity of such an automaton can be polynomial or even
+			exponential, meaning that for strings of a certain shape, increasing
+			the input length by ten characters may make the automaton about 1000
+			times slower.
+
+		</p>
+
+		<p>
+
+			Typically, a regular expression is affected by this
+			problem if it contains a repetition of the form <code>r*</code> or
+			<code>r+</code> where the sub-expression <code>r</code> is ambiguous
+			in the sense that it can match some string in multiple ways. More
+			information about the precise circumstances can be found in the
+			references.
+
+		</p>
+	</overview>
+
+	<recommendation>
+
+		<p>
+
+			Modify the regular expression to remove the ambiguity, or
+			ensure that the strings matched with the regular expression are short
+			enough that the time-complexity does not matter.
+
+		</p>
+
+	</recommendation>
+</qhelp>

javascript/ql/src/Performance/ReDoSReferences.qhelp

@@ -0,0 +1,16 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+	<references>
+		<li>
+			OWASP:
+			<a href="https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS">Regular expression Denial of Service - ReDoS</a>.
+		</li>
+		<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/ReDoS">ReDoS</a>.</li>
+		<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Time_complexity">Time complexity</a>.</li>
+		<li>James Kirrage, Asiri Rathnayake, Hayo Thielecke:
+		<a href="http://www.cs.bham.ac.uk/~hxt/research/reg-exp-sec.pdf">Static Analysis for Regular Expression Denial-of-Service Attack</a>.
+		</li>
+	</references>
+</qhelp>

javascript/ql/src/RegExp/BackspaceEscape.ql

@@ -14,6 +14,7 @@
 import javascript
 
 from RegExpCharEscape rece
-where rece.toString() = "\\b"
-  and rece.isPartOfRegExpLiteral()
+where
+  rece.toString() = "\\b" and
+  rece.isPartOfRegExpLiteral()
 select rece, "Backspace escape in regular expression."

javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql

@@ -18,7 +18,8 @@ import javascript
  * Indexing is 1-based.
  */
 predicate constantInCharacterClass(RegExpCharacterClass recc, int i, RegExpConstant cc, string val) {
-  cc = rank[i](RegExpConstant cc2, int j |
+  cc =
+    rank[i](RegExpConstant cc2, int j |
       cc2 = recc.getChild(j) and cc2.isCharacter() and cc2.getValue() = val
     |
       cc2 order by j

javascript/ql/src/RegExp/IdentityReplacement.ql

@@ -58,7 +58,8 @@ predicate regExpMatchesString(RegExpTerm t, string s) {
     or
     // sequences match the concatenation of their elements
     exists(RegExpSequence seq | seq = t |
-      s = concat(int i, RegExpTerm child |
+      s =
+        concat(int i, RegExpTerm child |
           child = seq.getChild(i)
         |
           any(string subs | regExpMatchesString(child, subs)) order by i

javascript/ql/src/RegExp/RegExpAlwaysMatches.ql

@@ -73,13 +73,9 @@ abstract class RegExpQuery extends DataFlow::CallNode {
 class RegExpTestCall extends DataFlow::MethodCallNode, RegExpQuery {
   DataFlow::RegExpCreationNode regexp;
 
-  RegExpTestCall() {
-    this = regexp.getAReference().getAMethodCall("test")
-  }
+  RegExpTestCall() { this = regexp.getAReference().getAMethodCall("test") }
 
-  override RegExpTerm getRegExp() {
-    result = regexp.getRoot()
-  }
+  override RegExpTerm getRegExp() { result = regexp.getRoot() }
 }
 
 /**
@@ -93,9 +89,7 @@ class RegExpSearchCall extends DataFlow::MethodCallNode, RegExpQuery {
     regexp.getAReference().flowsTo(getArgument(0))
   }
 
-  override RegExpTerm getRegExp() {
-    result = regexp.getRoot()
-  }
+  override RegExpTerm getRegExp() { result = regexp.getRoot() }
 }
 
 /**
@@ -116,10 +110,12 @@ where
   (
     call instanceof RegExpTestCall and
     not isPossiblyAnchoredOnBothEnds(term) and
-    message = "This regular expression always matches when used in a test $@, as it can match an empty substring."
+    message =
+      "This regular expression always matches when used in a test $@, as it can match an empty substring."
     or
     call instanceof RegExpSearchCall and
     not term.getAChild*() instanceof RegExpDollar and
-    message = "This regular expression always the matches at index 0 when used $@, as it matches the empty substring."
+    message =
+      "This regular expression always the matches at index 0 when used $@, as it matches the empty substring."
   )
 select term, message, call, "here"

javascript/ql/src/Security/CWE-020/HostnameRegexpShared.qll

@@ -2,6 +2,7 @@
  * Provides predicates for reasoning about regular expressions
  * that match URLs and hostname patterns.
  */
+
 import javascript
 
 /**
@@ -39,9 +40,7 @@ predicate isDotLike(RegExpTerm term) {
 predicate matchesBeginningOfString(RegExpTerm term) {
   term.isRootTerm()
   or
-  exists(RegExpTerm parent |
-    matchesBeginningOfString(parent)
-  |
+  exists(RegExpTerm parent | matchesBeginningOfString(parent) |
     term = parent.(RegExpSequence).getChild(0)
     or
     parent.(RegExpSequence).getChild(0) instanceof RegExpCaret and
@@ -60,7 +59,11 @@ predicate matchesBeginningOfString(RegExpTerm term) {
  * `i` is bound to the index of the last child in the top-level domain part.
  */
 predicate hasTopLevelDomainEnding(RegExpSequence seq, int i) {
-  seq.getChild(i).(RegExpConstant).getValue().regexpMatch("(?i)" + RegExpPatterns::commonTLD() + "(:\\d+)?([/?#].*)?") and
+  seq
+      .getChild(i)
+      .(RegExpConstant)
+      .getValue()
+      .regexpMatch("(?i)" + RegExpPatterns::commonTLD() + "(:\\d+)?([/?#].*)?") and
   isDotLike(seq.getChild(i - 1)) and
   not (i = 1 and matchesBeginningOfString(seq))
 }
@@ -69,9 +72,7 @@ predicate hasTopLevelDomainEnding(RegExpSequence seq, int i) {
  * Holds if the given regular expression term contains top-level domain preceded by a dot,
  * such as `.com`.
  */
-predicate hasTopLevelDomainEnding(RegExpSequence seq) {
-  hasTopLevelDomainEnding(seq, _)
-}
+predicate hasTopLevelDomainEnding(RegExpSequence seq) { hasTopLevelDomainEnding(seq, _) }
 
 /**
  * Holds if `term` will always match a hostname, that is, all disjunctions contain

javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.ql

@@ -61,9 +61,12 @@ predicate isIncompleteHostNameRegExpPattern(RegExpTerm regexp, RegExpSequence se
     unescapedDot = seq.getChild([0 .. i - 1]).getAChild*() and
     unescapedDot != seq.getChild(i - 1) and // Should not be the '.' immediately before the TLD
     not hasConsecutiveDots(unescapedDot.getParent()) and
-    hostname = seq.getChild(i - 2).getRawValue() + seq.getChild(i - 1).getRawValue() + seq.getChild(i).getRawValue()
+    hostname =
+      seq.getChild(i - 2).getRawValue() + seq.getChild(i - 1).getRawValue() +
+        seq.getChild(i).getRawValue()
   |
-    if unescapedDot.getParent() instanceof RegExpQuantifier then (
+    if unescapedDot.getParent() instanceof RegExpQuantifier
+    then
       // `.*\.example.com` can match `evil.com/?x=.example.com`
       //
       // This problem only occurs when the pattern is applied against a full URL, not just a hostname/origin.
@@ -73,16 +76,20 @@ predicate isIncompleteHostNameRegExpPattern(RegExpTerm regexp, RegExpSequence se
       seq.getChild(0) instanceof RegExpCaret and
       not seq.getAChild() instanceof RegExpDollar and
       seq.getChild([i .. i + 1]).(RegExpConstant).getValue().regexpMatch(".*[/?#].*") and
-      msg = "has an unrestricted wildcard '" +
-          unescapedDot.getParent().(RegExpQuantifier).getRawValue() +
-          "' which may cause '" + hostname + "' to be matched anywhere in the URL, outside the hostname."
-    ) else (
-      msg = "has an unescaped '.' before '" + hostname + "', so it might match more hosts than expected."
-    )
+      msg =
+        "has an unrestricted wildcard '" + unescapedDot.getParent().(RegExpQuantifier).getRawValue()
+          + "' which may cause '" + hostname +
+          "' to be matched anywhere in the URL, outside the hostname."
+    else
+      msg =
+        "has an unescaped '.' before '" + hostname +
+          "', so it might match more hosts than expected."
   )
 }
 
-from RegExpPatternSource re, RegExpTerm regexp, RegExpSequence hostSequence, string msg, string kind, DataFlow::Node aux
+from
+  RegExpPatternSource re, RegExpTerm regexp, RegExpSequence hostSequence, string msg, string kind,
+  DataFlow::Node aux
 where
   regexp = re.getRegExpTerm() and
   isIncompleteHostNameRegExpPattern(regexp, hostSequence, msg) and
@@ -96,5 +103,4 @@ where
     )
   ) and
   not CharacterEscapes::hasALikelyRegExpPatternMistake(re)
-select hostSequence,
-  "This " + kind + " " + msg, aux, "here"
+select hostSequence, "This " + kind + " " + msg, aux, "here"

javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql

@@ -20,9 +20,7 @@ class DangerousScheme extends string {
 }
 
 /** Gets a data-flow node that checks `nd` against the given `scheme`. */
-DataFlow::Node schemeCheck(
-  DataFlow::Node nd, DangerousScheme scheme
-) {
+DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
   // check of the form `nd.startsWith(scheme)`
   exists(StringOps::StartsWith sw | sw = result |
     sw.getBaseString() = nd and

javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql

@@ -46,9 +46,7 @@ predicate isInteriorAnchor(RegExpAnchor term) {
  * Holds if `term` contains an anchor that is not the first or last node
  * in its tree, such as `(foo|bar$|baz)`.
  */
-predicate containsInteriorAnchor(RegExpTerm term) {
-  isInteriorAnchor(term.getAChild*())
-}
+predicate containsInteriorAnchor(RegExpTerm term) { isInteriorAnchor(term.getAChild*()) }
 
 /**
  * Holds if `term` starts with a word boundary or lookbehind assertion,
@@ -78,9 +76,7 @@ predicate containsTrailingPseudoAnchor(RegExpSequence term) {
  * Holds if `term` is an empty sequence, usually arising from
  * literals with a trailing alternative such as `foo|`.
  */
-predicate isEmpty(RegExpSequence term) {
-  term.getNumChild() = 0
-}
+predicate isEmpty(RegExpSequence term) { term.getNumChild() = 0 }
 
 /**
  * Holds if `term` contains a letter constant.
@@ -131,14 +127,14 @@ predicate hasMisleadingAnchorPrecedence(RegExpPatternSource src, string msg) {
     (
       anchoredTerm = root.getChild(0) and
       anchoredTerm.getChild(0) instanceof RegExpCaret and
-      not containsLeadingPseudoAnchor(root.getChild([ 1 .. root.getNumChild() - 1 ])) and
-      containsLetters(root.getChild([ 1 .. root.getNumChild() - 1 ])) and
+      not containsLeadingPseudoAnchor(root.getChild([1 .. root.getNumChild() - 1])) and
+      containsLetters(root.getChild([1 .. root.getNumChild() - 1])) and
       direction = "beginning"
       or
       anchoredTerm = root.getLastChild() and
       anchoredTerm.getLastChild() instanceof RegExpDollar and
-      not containsTrailingPseudoAnchor(root.getChild([ 0 .. root.getNumChild() - 2 ])) and
-      containsLetters(root.getChild([ 0 .. root.getNumChild() - 2 ])) and
+      not containsTrailingPseudoAnchor(root.getChild([0 .. root.getNumChild() - 2])) and
+      containsLetters(root.getChild([0 .. root.getNumChild() - 2])) and
       direction = "end"
     ) and
     // is not used for replace
@@ -146,8 +142,10 @@ predicate hasMisleadingAnchorPrecedence(RegExpPatternSource src, string msg) {
       replace.getMethodName() = "replace" and
       src.getARegExpObject().flowsTo(replace.getArgument(0))
     ) and
-    msg = "Misleading operator precedence. The subexpression '" + anchoredTerm.getRawValue() +
-        "' is anchored at the " + direction + ", but the other parts of this regular expression are not"
+    msg =
+      "Misleading operator precedence. The subexpression '" + anchoredTerm.getRawValue() +
+        "' is anchored at the " + direction +
+        ", but the other parts of this regular expression are not"
   )
 }
 
@@ -181,7 +179,8 @@ predicate isSemiAnchoredHostnameRegExp(RegExpPatternSource src, string msg) {
     hasTopLevelDomainEnding(tld, i) and
     isFinalRegExpTerm(tld.getChild(i)) and // nothing is matched after the TLD
     tld.getChild(0) instanceof RegExpCaret and
-    msg = "This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end."
+    msg =
+      "This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end."
   )
 }
 
@@ -214,7 +213,8 @@ predicate isUnanchoredHostnameRegExp(RegExpPatternSource src, string msg) {
         name = "match" and exists(mcn.getAPropertyRead())
       )
     ) and
-    msg = "When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it."
+    msg =
+      "When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it."
   )
 }
 

javascript/ql/src/Security/CWE-020/UselessRegExpCharacterEscape.ql

@@ -105,13 +105,9 @@ class RegExpPatternMistake extends TRegExpPatternMistake {
  */
 class IdentityEscapeInStringMistake extends RegExpPatternMistake, TIdentityEscapeInStringMistake {
   RegExpPatternSource src;
-
   string char;
-
   string mistake;
-
   int index;
-
   ASTNode rawStringNode;
 
   IdentityEscapeInStringMistake() {
@@ -130,19 +126,19 @@ class IdentityEscapeInStringMistake extends RegExpPatternMistake, TIdentityEscap
 }
 
 /**
-  * A backspace as '\b' in a regular expression string, indicating
-  * programmer intent to use the word-boundary assertion '\b'.
-  */
+ * A backspace as '\b' in a regular expression string, indicating
+ * programmer intent to use the word-boundary assertion '\b'.
+ */
 class BackspaceInStringMistake extends RegExpPatternMistake, TBackspaceInStringMistake {
   RegExpPatternSource src;
-
   int index;
-
   ASTNode rawStringNode;
 
   BackspaceInStringMistake() { this = TBackspaceInStringMistake(src, rawStringNode, index) }
 
-  override string getMessage() { result = "'\\b' is a backspace, and not a word-boundary assertion" }
+  override string getMessage() {
+    result = "'\\b' is a backspace, and not a word-boundary assertion"
+  }
 
   override int getIndex() { result = index }
 

javascript/ql/src/Security/CWE-078/CommandInjection.ql

@@ -16,11 +16,16 @@ import javascript
 import semmle.javascript.security.dataflow.CommandInjection::CommandInjection
 import DataFlow::PathGraph
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight, Source sourceNode
+from
+  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight,
+  Source sourceNode
 where
   cfg.hasFlowPath(source, sink) and
-  if cfg.isSinkWithHighlight(sink.getNode(), _)
-  then cfg.isSinkWithHighlight(sink.getNode(), highlight)
-  else highlight = sink.getNode() and
+  (
+    if cfg.isSinkWithHighlight(sink.getNode(), _)
+    then cfg.isSinkWithHighlight(sink.getNode(), highlight)
+    else highlight = sink.getNode()
+  ) and
   sourceNode = source.getNode()
-select highlight, source, sink, "This command depends on $@.", sourceNode, sourceNode.getSourceType()
+select highlight, source, sink, "This command depends on $@.", sourceNode,
+  sourceNode.getSourceType()

javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+<p>Using the unix command <code>cat</code> only to read a file is an 
+unnecessarily complex way to achieve something that can be done in a simpler 
+and safer manner using the Node.js <code>fs.readFile</code> API.
+</p>
+<p>
+The use of <code>cat</code> for simple file reads leads to code that is 
+unportable, inefficient, complex, and can lead to subtle bugs or even 
+security vulnerabilities. 
+</p>
+</overview>
+<recommendation>
+<p>
+Use <code>fs.readFile</code> or <code>fs.readFileSync</code> to read files 
+from the file system. 
+</p>
+</recommendation>
+<example>
+
+<p>The following example shows code that reads a file using <code>cat</code>:</p>
+
+<sample src="examples/useless-cat.js"/>
+
+<p>The code in the example will break if the input <code>name</code> contains 
+special characters (including space). Additionally, it does not work on Windows 
+and if the input is user-controlled, a command injection attack can happen.</p>
+
+<p>The <code>fs.readFile</code> API should be used to avoid these potential issues: </p>
+
+<sample src="examples/useless-cat-fixed.js"/>
+
+</example>
+<references>
+
+<li>OWASP: <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.</li>
+<li>Node.js: <a href="https://nodejs.org/api/fs.html">File System API</a>.</li>
+<li><a href="http://porkmail.org/era/unix/award.html#cat">The Useless Use of Cat Award</a>.</li>
+
+
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql

@@ -0,0 +1,25 @@
+/**
+ * @name Unnecessary use of `cat` process
+ * @description Using the  `cat` process to read a file is unnecessarily complex, inefficient, unportable, and can lead to subtle bugs, or even security vulnerabilities.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/unnecessary-use-of-cat
+ * @tags correctness
+ *       security
+ *       maintainability
+ */
+
+import javascript
+import semmle.javascript.security.UselessUseOfCat
+import semmle.javascript.RestrictedLocations
+
+from UselessCat cat, string message
+where
+  message = " Can be replaced with: " + PrettyPrintCatCall::createReadFileCall(cat)
+  or
+  not exists(PrettyPrintCatCall::createReadFileCall(cat)) and
+  if cat.isSync()
+  then message = " Can be replaced with a call to fs.readFileSync(..)."
+  else message = " Can be replaced with a call to fs.readFile(..)."
+select cat.asExpr().(FirstLineOf), "Unnecessary use of `cat` process." + message

javascript/ql/src/Security/CWE-078/examples/useless-cat-fixed.js

@@ -0,0 +1,5 @@
+var fs = require('fs');
+
+module.exports = function (name) {
+    return fs.readFileSync(name).toString();
+};

javascript/ql/src/Security/CWE-078/examples/useless-cat.js

@@ -0,0 +1,5 @@
+var child_process = require('child_process');
+
+module.exports = function (name) {
+    return child_process.execSync("cat " + name).toString();
+};

javascript/ql/src/Security/CWE-079/ExceptionXss.ql

@@ -15,10 +15,8 @@ import javascript
 import semmle.javascript.security.dataflow.ExceptionXss::ExceptionXss
 import DataFlow::PathGraph
 
-from
-  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
-where
-  cfg.hasFlowPath(source, sink)
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
-  sink.getNode().(Xss::Shared::Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(),
-  "user-provided value"
+  sink.getNode().(Xss::Shared::Sink).getVulnerabilityKind() + " vulnerability due to $@.",
+  source.getNode(), "user-provided value"

javascript/ql/src/Security/CWE-079/UnsafeJQueryPlugin.qhelp

@@ -0,0 +1,101 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+	<overview>
+		<p>
+
+			Library plugins, such as those for the jQuery library, are often
+			configurable through options provided by the clients of the
+			plugin.
+
+
+			Clients, however, do not know the implementation details
+			of the plugin, so it is important to document the capabilities of each
+			option.  The documentation for the plugin options that the client is
+			responsible for sanitizing is of particular importance.
+
+			Otherwise, the plugin may write user input (for example, a URL query
+			parameter) to a web page without properly sanitizing it first,
+			which allows for a cross-site scripting vulnerability in the client
+			application through dynamic HTML construction.
+
+		</p>
+	</overview>
+
+	<recommendation>
+		<p>
+
+			Document all options that can lead to cross-site scripting
+			attacks, and guard against unsafe inputs where dynamic HTML
+			construction is not intended.
+
+		</p>
+	</recommendation>
+
+	<example>
+		<p>
+
+			The following example shows a jQuery plugin that selects a
+			DOM element, and copies its text content to another DOM element. The
+			selection is performed by using the plugin option
+			<code>sourceSelector</code> as a CSS selector.
+
+		</p>
+
+		<sample src="examples/UnsafeJQueryPlugin.js" />
+
+		<p>
+
+			This is, however, not a safe plugin, since the call to
+			<code>jQuery</code> interprets <code>sourceSelector</code> as HTML if
+			it is a string that starts with <code>&lt;</code>.
+
+		</p>
+
+		<p>
+
+			Instead of documenting that the client is responsible for
+			sanitizing <code>sourceSelector</code>, the plugin can use
+			<code>jQuery.find</code> to always interpret
+			<code>sourceSelector</code> as a CSS selector:
+
+		</p>
+
+		<sample src="examples/UnsafeJQueryPlugin_safe.js" />
+
+
+	</example>
+
+	<references>
+		<li>
+			OWASP:
+			<a href="https://www.owasp.org/index.php/DOM_based_XSS_Prevention_Cheat_Sheet">DOM based
+			XSS Prevention Cheat Sheet</a>.
+		</li>
+		<li>
+			OWASP:
+			<a href="https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet">XSS
+			(Cross Site Scripting) Prevention Cheat Sheet</a>.
+		</li>
+		<li>
+			OWASP
+			<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.
+		</li>
+		<li>
+			OWASP
+			<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
+			Scripting</a>.
+		</li>
+		<li>
+			Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
+		</li>
+		<li>
+			jQuery: <a href="https://learn.jquery.com/plugins/basic-plugin-creation/">Plugin creation</a>.
+		</li>
+		<li>
+			Bootstrap:	<a href="https://github.com/twbs/bootstrap/pull/27047">XSS vulnerable bootstrap plugins</a>.
+		</li>
+	</references>
+</qhelp>

javascript/ql/src/Security/CWE-079/UnsafeJQueryPlugin.ql

@@ -0,0 +1,25 @@
+/**
+ * @name Unsafe jQuery plugin
+ * @description A jQuery plugin that unintentionally constructs HTML from some of its options may be unsafe to use for clients.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id js/unsafe-jquery-plugin
+ * @tags security
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ *       frameworks/jquery
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.UnsafeJQueryPlugin::UnsafeJQueryPlugin
+import DataFlow::PathGraph
+
+from
+  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, JQueryPluginMethod plugin
+where
+  cfg.hasFlowPath(source, sink) and
+  source.getNode().(Source).getPlugin() = plugin and
+  not isLikelyIntentionalHtmlSink(plugin, sink.getNode())
+select sink.getNode(), source, sink, "Potential XSS vulnerability in the $@.", plugin,
+  "'$.fn." + plugin.getPluginName() + "' plugin"

javascript/ql/src/Security/CWE-079/examples/UnsafeJQueryPlugin.js

@@ -0,0 +1,6 @@
+jQuery.fn.copyText = function(options) {
+	// BAD may evaluate `options.sourceSelector` as HTML
+	var source = jQuery(options.sourceSelector),
+	    text = source.text();
+	jQuery(this).text(text);
+}

javascript/ql/src/Security/CWE-079/examples/UnsafeJQueryPlugin_safe.js

@@ -0,0 +1,6 @@
+jQuery.fn.copyText = function(options) {
+	// GOOD may not evaluate `options.sourceSelector` as HTML
+	var source = jQuery.find(options.sourceSelector),
+	    text = source.text();
+	jQuery(this).text(text);
+}

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

@@ -44,23 +44,17 @@ predicate escapingScheme(string metachar, string regex) {
  * A call to `String.prototype.replace` that replaces all instances of a pattern.
  */
 class Replacement extends StringReplaceCall {
-  Replacement() {
-    isGlobal()
-  }
+  Replacement() { isGlobal() }
 
   /**
    * Gets the input of this replacement.
    */
-  DataFlow::Node getInput() {
-    result = this.getReceiver()
-  }
+  DataFlow::Node getInput() { result = this.getReceiver() }
 
   /**
    * Gets the output of this replacement.
    */
-  DataFlow::SourceNode getOutput() {
-    result = this
-  }
+  DataFlow::SourceNode getOutput() { result = this }
 
   /**
    * Holds if this replacement escapes `char` using `metachar`.
@@ -93,9 +87,7 @@ class Replacement extends StringReplaceCall {
   /**
    * Gets the previous replacement in this chain of replacements.
    */
-  Replacement getPreviousReplacement() {
-    result.getOutput() = getASimplePredecessor*(getInput())
-  }
+  Replacement getPreviousReplacement() { result.getOutput() = getASimplePredecessor*(getInput()) }
 
   /**
    * Gets an earlier replacement in this chain of replacements that

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

@@ -44,9 +44,7 @@ predicate isSimpleCharacterClass(RegExpCharacterClass t) {
 }
 
 /** Holds if `t` is an alternation of simple terms. */
-predicate isSimpleAlt(RegExpAlt t) {
-  forall(RegExpTerm ch | ch = t.getAChild() | isSimple(ch))
-}
+predicate isSimpleAlt(RegExpAlt t) { forall(RegExpTerm ch | ch = t.getAChild() | isSimple(ch)) }
 
 /**
  * Holds if `mce` is of the form `x.replace(re, new)`, where `re` is a global

javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql

@@ -13,26 +13,20 @@
 import javascript
 
 /** Gets a property name of `req` which refers to data usually derived from cookie data. */
-string cookieProperty() {
-  result = "session" or result = "cookies" or result = "user"
-}
+string cookieProperty() { result = "session" or result = "cookies" or result = "user" }
 
 /** Gets a data flow node that flows to the base of an access to `cookies`, `session`, or `user`. */
 private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) {
   t.start() and
   exists(DataFlow::PropRead value |
     value = result.getAPropertyRead(cookieProperty()).getAPropertyRead() and
-
     // Ignore accesses to values that are part of a CSRF or captcha check
     not value.getPropertyName().regexpMatch("(?i).*(csrf|xsrf|captcha).*") and
-
     // Ignore calls like `req.session.save()`
     not value = any(DataFlow::InvokeNode call).getCalleeNode()
   )
   or
-  exists(DataFlow::TypeBackTracker t2 |
-    result = nodeLeadingToCookieAccess(t2).backtrack(t2, t)
-  )
+  exists(DataFlow::TypeBackTracker t2 | result = nodeLeadingToCookieAccess(t2).backtrack(t2, t))
 }
 
 /** Gets a data flow node that flows to the base of an access to `cookies`, `session`, or `user`. */
@@ -52,9 +46,7 @@ private DataFlow::SourceNode getARouteUsingCookies(DataFlow::TypeTracker t) {
   t.start() and
   isRouteHandlerUsingCookies(result)
   or
-  exists(DataFlow::TypeTracker t2 |
-    result = getARouteUsingCookies(t2).track(t2, t)
-  )
+  exists(DataFlow::TypeTracker t2 | result = getARouteUsingCookies(t2).track(t2, t))
 }
 
 /** Gets a data flow node referring to a route handler that uses cookies. */
@@ -113,7 +105,6 @@ from
 where
   router = setup.getRouter() and
   handler = setup.getARouteHandlerExpr() and
-
   // Require that the handler uses cookies and has cookie middleware.
   //
   // In practice, handlers that use cookies always have the cookie middleware or
@@ -122,10 +113,8 @@ where
   // don't trust it to detect the presence of CSRF middleware either.
   getARouteUsingCookies().flowsToExpr(handler) and
   hasCookieMiddleware(handler, cookie) and
-
   // Only flag the cookie parser registered first.
   not hasCookieMiddleware(cookie, _) and
-
   not hasCsrfMiddleware(handler) and
   // Only warn for the last handler in a chain.
   handler.isLastHandler() and

javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.qhelp

@@ -13,7 +13,8 @@
 
   <p>
     One way to cause prototype pollution is through use of an unsafe <em>merge</em> or <em>extend</em> function
-    to recursively copy properties from one object to another.
+    to recursively copy properties from one object to another, or through the use of a <em>deep assignment</em>
+    function to assign to an unverified chain of property names.
     Such a function has the potential to modify any object reachable from the destination object, and
     the built-in <code>Object.prototype</code> is usually reachable through the special properties
     <code>__proto__</code> and <code>constructor.prototype</code>.
@@ -23,13 +24,13 @@
 <recommendation>
   <p>
     The most effective place to guard against this is in the function that performs
-    the recursive copy.
+    the recursive copy or deep assignment.
   </p>
 
   <p>
-    Only merge a property recursively when it is an own property of the <em>destination</em> object.
+    Only merge or assign a property recursively when it is an own property of the <em>destination</em> object.
     Alternatively, blacklist the property names <code>__proto__</code> and <code>constructor</code>
-    from being merged.
+    from being merged or assigned to.
   </p>
 </recommendation>
 

javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql

@@ -1,7 +1,7 @@
 /**
  * @name Prototype pollution in utility function
- * @description Recursively copying properties between objects may cause
-                accidental modification of a built-in prototype object.
+ * @description Recursively assigning properties on objects may cause
+ *              accidental modification of a built-in prototype object.
  * @kind path-problem
  * @problem.severity warning
  * @precision high
@@ -14,144 +14,139 @@
 import javascript
 import DataFlow
 import PathGraph
-import semmle.javascript.dataflow.InferredTypes
+import semmle.javascript.DynamicPropertyAccess
 
 /**
- * Gets a node that refers to an element of `array`, likely obtained
- * as a result of enumerating the elements of the array.
+ * A call of form `x.split(".")` where `x` is a parameter.
+ *
+ * We restrict this to parameter nodes to focus on "deep assignment" functions.
  */
-SourceNode getAnEnumeratedArrayElement(SourceNode array) {
-  exists(MethodCallNode call, string name |
-    call = array.getAMethodCall(name) and
-    (name = "forEach" or name = "map") and
-    result = call.getCallback(0).getParameter(0)
+class SplitCall extends MethodCallNode {
+  SplitCall() {
+    getMethodName() = "split" and
+    getArgument(0).mayHaveStringValue(".") and
+    getReceiver().getALocalSource() instanceof ParameterNode
+  }
+}
+
+/**
+ * Holds if `pred -> succ` should preserve polluted property names.
+ */
+predicate copyArrayStep(SourceNode pred, SourceNode succ) {
+  // x -> [...x]
+  exists(SpreadElement spread |
+    pred.flowsTo(spread.getOperand().flow()) and
+    succ.asExpr().(ArrayExpr).getAnElement() = spread
   )
   or
-  exists(DataFlow::PropRead read |
-    read = array.getAPropertyRead() and
-    not exists(read.getPropertyName()) and
-    not read.getPropertyNameExpr().analyze().getAType() = TTString() and
-    result = read
+  // `x -> y` in `y.push( x[i] )`
+  exists(MethodCallNode push |
+    push = succ.getAMethodCall("push") and
+    (
+      getAnEnumeratedArrayElement(pred).flowsTo(push.getAnArgument())
+      or
+      pred.flowsTo(push.getASpreadArgument())
+    )
+  )
+  or
+  // x -> x.concat(...)
+  exists(MethodCallNode concat_ |
+    concat_.getMethodName() = "concat" and
+    (pred = concat_.getReceiver() or pred = concat_.getAnArgument()) and
+    succ = concat_
   )
 }
 
 /**
- * A data flow node that refers to the name of a property obtained by enumerating
- * the properties of some object.
+ * Holds if `node` may refer to a `SplitCall` or a copy thereof, possibly
+ * returned through a function call.
  */
-abstract class EnumeratedPropName extends DataFlow::Node {
-  /**
-   * Gets the object whose properties are being enumerated.
-   *
-   * For example, gets `src` in `for (var key in src)`.
-   */
-  abstract DataFlow::Node getSourceObject();
-
-  /**
-   * Gets a property read that accesses the corresponding property value in the source object.
-   *
-   * For example, gets `src[key]` in `for (var key in src) { src[key]; }`.
-   */
-  PropRead getASourceProp() {
-    result = AccessPath::getAnAliasedSourceNode(getSourceObject()).getAPropertyRead() and
-    result.getPropertyNameExpr().flow().getImmediatePredecessor*() = this
-  }
+predicate isSplitArray(SourceNode node) {
+  node instanceof SplitCall
+  or
+  exists(SourceNode pred | isSplitArray(pred) |
+    copyArrayStep(pred, node)
+    or
+    pred.flowsToExpr(node.(CallNode).getACallee().getAReturnedExpr())
+  )
 }
 
 /**
- * Property enumeration through `for-in` for `Object.keys` or similar.
+ * A property name originating from a `x.split(".")` call.
  */
-class ForInEnumeratedPropName extends EnumeratedPropName {
-  DataFlow::Node object;
+class SplitPropName extends SourceNode {
+  SourceNode array;
 
-  ForInEnumeratedPropName() {
-    exists(ForInStmt stmt |
-      this = DataFlow::lvalueNode(stmt.getLValue()) and
-      object = stmt.getIterationDomain().flow()
-    )
-    or
-    exists(CallNode call |
-      call = globalVarRef("Object").getAMemberCall("keys")
-      or
-      call = globalVarRef("Object").getAMemberCall("getOwnPropertyNames")
-      or
-      call = globalVarRef("Reflect").getAMemberCall("ownKeys")
-    |
-      object = call.getArgument(0) and
-      this = getAnEnumeratedArrayElement(call)
-    )
+  SplitPropName() {
+    isSplitArray(array) and
+    this = getAnEnumeratedArrayElement(array)
   }
 
-  override Node getSourceObject() { result = object }
-}
+  /**
+   * Gets the array from which this property name was obtained (the result from `split`).
+   */
+  SourceNode getArray() { result = array }
 
-/**
- * Property enumeration through `Object.entries`.
- */
-class EntriesEnumeratedPropName extends EnumeratedPropName {
-  CallNode entries;
-  SourceNode entry;
-
-  EntriesEnumeratedPropName() {
-    entries = globalVarRef("Object").getAMemberCall("entries") and
-    entry = getAnEnumeratedArrayElement(entries) and
-    this = entry.getAPropertyRead("0")
-  }
-
-  override DataFlow::Node getSourceObject() {
-    result = entries.getArgument(0)
-  }
-
-  override PropRead getASourceProp() {
-    result = super.getASourceProp()
-    or
-    result = entry.getAPropertyRead("1")
-  }
+  /** Gets an element accessed on the same underlying array. */
+  SplitPropName getAnAlias() { result.getArray() = getArray() }
 }
 
 /**
  * Holds if the properties of `node` are enumerated locally.
  */
 predicate arePropertiesEnumerated(DataFlow::SourceNode node) {
-  node = AccessPath::getAnAliasedSourceNode(any(EnumeratedPropName name).getSourceObject())
+  node = any(EnumeratedPropName name).getASourceObjectRef()
 }
 
 /**
- * A dynamic property access that is not obviously an array access.
+ * Holds if `node` is a source of property names that we consider possible
+ * prototype pollution payloads.
  */
-class DynamicPropRead extends DataFlow::SourceNode, DataFlow::ValueNode {
-  // Use IndexExpr instead of PropRead as we're not interested in implicit accesses like
-  // rest-patterns and for-of loops.
-  override IndexExpr astNode;
+predicate isPollutedPropNameSource(DataFlow::Node node) {
+  node instanceof EnumeratedPropName
+  or
+  node instanceof SplitPropName
+}
 
-  DynamicPropRead() {
-    not exists(astNode.getPropertyName()) and
-    // Exclude obvious array access
-    astNode.getPropertyNameExpr().analyze().getAType() = TTString()
-  }
-
-  /** Gets the base of the dynamic read. */
-  DataFlow::Node getBase() { result = astNode.getBase().flow() }
-
-  /**
-   * Holds if the value of this read was assigned to earlier in the same basic block.
-   *
-   * For example, this is true for `dst[x]` on line 2 below:
-   * ```js
-   * dst[x] = {};
-   * dst[x][y] = src[y];
-   * ```
-   */
-  predicate hasDominatingAssignment() {
-    exists(DataFlow::PropWrite write, BasicBlock bb, int i, int j, SsaVariable ssaVar |
-      write = getBase().getALocalSource().getAPropertyWrite() and
-      bb.getNode(i) = write.getWriteNode() and
-      bb.getNode(j) = astNode and
-      i < j and
-      write.getPropertyNameExpr() = ssaVar.getAUse() and
-      astNode.getIndex() = ssaVar.getAUse()
+/**
+ * Holds if `node` may flow from a source of polluted propery names, possibly
+ * into function calls (but not returns).
+ */
+predicate isPollutedPropName(Node node) {
+  isPollutedPropNameSource(node)
+  or
+  exists(Node pred | isPollutedPropName(pred) |
+    node = pred.getASuccessor()
+    or
+    argumentPassingStep(_, pred, _, node)
+    or
+    // Handle one level of callbacks
+    exists(FunctionNode function, ParameterNode callback, int i |
+      pred = callback.getAnInvocation().getArgument(i) and
+      argumentPassingStep(_, function, _, callback) and
+      node = function.getParameter(i)
     )
-  }
+  )
+}
+
+/**
+ * Holds if `node` may refer to `Object.prototype` obtained through dynamic property
+ * read of a property obtained through property enumeration.
+ */
+predicate isPotentiallyObjectPrototype(SourceNode node) {
+  exists(Node base, Node key |
+    dynamicPropReadStep(base, key, node) and
+    isPollutedPropName(key) and
+    // Ignore cases where the properties of `base` are enumerated, to avoid FPs
+    // where the key came from that enumeration (and thus will not return Object.prototype).
+    // For example, `src[key]` in `for (let key in src) { ... src[key] ... }` will generally
+    // not return Object.prototype because `key` is an enumerable property of `src`.
+    not arePropertiesEnumerated(base.getALocalSource())
+  )
+  or
+  exists(Node use | isPotentiallyObjectPrototype(use.getALocalSource()) |
+    argumentPassingStep(_, use, _, node)
+  )
 }
 
 /**
@@ -171,7 +166,17 @@ predicate dynamicPropWrite(DataFlow::Node base, DataFlow::Node prop, DataFlow::N
     prop = index.getPropertyNameExpr().flow() and
     rhs = write.getRhs().flow() and
     not exists(prop.getStringValue()) and
-    not arePropertiesEnumerated(base.getALocalSource())
+    not arePropertiesEnumerated(base.getALocalSource()) and
+    // Prune writes that are unlikely to modify Object.prototype.
+    // This is mainly for performance, but may block certain results due to
+    // not tracking out of function returns and into callbacks.
+    isPotentiallyObjectPrototype(base.getALocalSource()) and
+    // Ignore writes with an obviously safe RHS.
+    not exists(Expr e | e = rhs.asExpr() |
+      e instanceof Literal or
+      e instanceof ObjectExpr or
+      e instanceof ArrayExpr
+    )
   )
 }
 
@@ -227,10 +232,10 @@ class PropNameTracking extends DataFlow::Configuration {
 
   override predicate isSource(DataFlow::Node node, FlowLabel label) {
     label instanceof UnsafePropLabel and
-    exists(EnumeratedPropName prop |
-      node = prop
+    (
+      isPollutedPropNameSource(node)
       or
-      node = prop.getASourceProp()
+      node = any(EnumeratedPropName prop).getASourceProp()
     )
   }
 
@@ -268,12 +273,7 @@ class PropNameTracking extends DataFlow::Configuration {
   override predicate isBarrier(DataFlow::Node node) {
     super.isBarrier(node)
     or
-    exists(ConditionGuardNode guard, SsaRefinementNode refinement |
-      node = DataFlow::ssaDefinitionNode(refinement) and
-      refinement.getGuard() = guard and
-      guard.getTest() instanceof VarAccess and
-      guard.getOutcome() = false
-    )
+    node instanceof DataFlow::VarAccessBarrier
   }
 
   override predicate isBarrierGuard(DataFlow::BarrierGuardNode node) {
@@ -284,7 +284,8 @@ class PropNameTracking extends DataFlow::Configuration {
     node instanceof InstanceOfGuard or
     node instanceof TypeofGuard or
     node instanceof BlacklistInclusionGuard or
-    node instanceof WhitelistInclusionGuard
+    node instanceof WhitelistInclusionGuard or
+    node instanceof IsPlainObjectGuard
   }
 }
 
@@ -459,6 +460,25 @@ class WhitelistInclusionGuard extends DataFlow::LabeledBarrierGuardNode {
   }
 }
 
+/**
+ * A check of form `isPlainObject(e)` or similar, which sanitizes the `constructor`
+ * payload in the true case, since it rejects objects with a non-standard `constructor`
+ * property.
+ */
+class IsPlainObjectGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::CallNode {
+  IsPlainObjectGuard() {
+    exists(string name | name = "is-plain-object" or name = "is-extendable" |
+      this = moduleImport(name).getACall()
+    )
+  }
+
+  override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
+    e = getArgument(0).asExpr() and
+    outcome = true and
+    lbl = "constructor"
+  }
+}
+
 /**
  * Gets a meaningful name for `node` if possible.
  */
@@ -475,22 +495,29 @@ string deriveExprName(DataFlow::Node node) {
   result = getExprName(node)
   or
   not exists(getExprName(node)) and
-  result = "this object"
+  result = "here"
 }
 
 /**
  * Holds if the dynamic property write `base[prop] = rhs` can pollute the prototype
- * of `base` due to flow from `enum`.
+ * of `base` due to flow from `propNameSource`.
  *
  * In most cases this will result in an alert, the exception being the case where
  * `base` does not have a prototype at all.
  */
-predicate isPrototypePollutingAssignment(Node base, Node prop, Node rhs, EnumeratedPropName enum) {
+predicate isPrototypePollutingAssignment(Node base, Node prop, Node rhs, Node propNameSource) {
   dynamicPropWrite(base, prop, rhs) and
+  isPollutedPropNameSource(propNameSource) and
   exists(PropNameTracking cfg |
-    cfg.hasFlow(enum, base) and
-    cfg.hasFlow(enum, prop) and
-    cfg.hasFlow(enum.getASourceProp(), rhs)
+    cfg.hasFlow(propNameSource, base) and
+    if propNameSource instanceof EnumeratedPropName
+    then
+      cfg.hasFlow(propNameSource, prop) and
+      cfg.hasFlow(propNameSource.(EnumeratedPropName).getASourceProp(), rhs)
+    else (
+      cfg.hasFlow(propNameSource.(SplitPropName).getAnAlias(), prop) and
+      rhs.getALocalSource() instanceof ParameterNode
+    )
   )
 }
 
@@ -500,9 +527,7 @@ private DataFlow::SourceNode getANodeLeadingToBase(DataFlow::TypeBackTracker t,
   isPrototypePollutingAssignment(base, _, _, _) and
   result = base.getALocalSource()
   or
-  exists(DataFlow::TypeBackTracker t2 |
-    result = getANodeLeadingToBase(t2, base).backtrack(t2, t)
-  )
+  exists(DataFlow::TypeBackTracker t2 | result = getANodeLeadingToBase(t2, base).backtrack(t2, t))
 }
 
 /**
@@ -537,18 +562,29 @@ class ObjectCreateNullCall extends CallNode {
 }
 
 from
-  PropNameTracking cfg, DataFlow::PathNode source, DataFlow::PathNode sink, EnumeratedPropName enum,
-  Node base
+  PropNameTracking cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Node prop, Node base,
+  string msg, Node col1, Node col2
 where
+  isPollutedPropName(prop) and
   cfg.hasFlowPath(source, sink) and
-  isPrototypePollutingAssignment(base, _, _, enum) and
+  isPrototypePollutingAssignment(base, _, _, prop) and
   sink.getNode() = base and
-  source.getNode() = enum and
+  source.getNode() = prop and
   (
     getANodeLeadingToBaseBase(base) instanceof ObjectLiteralNode
     or
     not getANodeLeadingToBaseBase(base) instanceof ObjectCreateNullCall
+  ) and
+  // Generate different messages for deep merge and deep assign cases.
+  if prop instanceof EnumeratedPropName
+  then (
+    col1 = prop.(EnumeratedPropName).getSourceObject() and
+    col2 = base and
+    msg = "Properties are copied from $@ to $@ without guarding against prototype pollution."
+  ) else (
+    col1 = prop and
+    col2 = base and
+    msg =
+      "The property chain $@ is recursively assigned to $@ without guarding against prototype pollution."
   )
-select base, source, sink,
-  "Properties are copied from $@ to $@ without guarding against prototype pollution.",
-  enum.getSourceObject(), deriveExprName(enum.getSourceObject()), base, deriveExprName(base)
+select base, source, sink, msg, col1, deriveExprName(col1), col2, deriveExprName(col2)

javascript/ql/src/Statements/IgnoreArrayResult.ql

@@ -13,7 +13,7 @@ import javascript
 import Expressions.ExprHasNoEffect
 
 DataFlow::SourceNode callsArray(DataFlow::TypeBackTracker t, DataFlow::MethodCallNode call) {
-  isIgnoredPureArrayCall(call) and 
+  isIgnoredPureArrayCall(call) and
   t.start() and
   result = call.getReceiver().getALocalSource()
   or
@@ -39,7 +39,7 @@ predicate isIgnoredPureArrayCall(DataFlow::MethodCallNode call) {
 }
 
 from DataFlow::MethodCallNode call
-where 
+where
   callsArray(call) instanceof DataFlow::ArrayCreationNode and
   not call.getReceiver().asExpr().(ArrayExpr).getSize() = 0
 select call, "Result from call to " + call.getMethodName() + " ignored."

javascript/ql/src/Statements/ImplicitReturn.ql

@@ -32,7 +32,8 @@ class ValueReturn extends ReturnStmt {
 
 /** Gets the lexically first explicit return statement in function `f`. */
 ValueReturn getFirstExplicitReturn(Function f) {
-  result = min(ValueReturn ret |
+  result =
+    min(ValueReturn ret |
       ret.getContainer() = f
     |
       ret order by ret.getLocation().getStartLine(), ret.getLocation().getStartColumn()

javascript/ql/src/Statements/UseOfReturnlessFunction.ql

@@ -21,40 +21,41 @@ predicate returnsVoid(Function f) {
 }
 
 predicate isStub(Function f) {
-    f.getBody().(BlockStmt).getNumChild() = 0 
-    or
-    f instanceof ExternalDecl
+  f.getBody().(BlockStmt).getNumChild() = 0
+  or
+  f instanceof ExternalDecl
 }
 
 /**
  * Holds if `e` is in a syntactic context where it likely is fine that the value of `e` comes from a call to a returnless function.
  */
 predicate benignContext(Expr e) {
-  inVoidContext(e) or 
-  
+  inVoidContext(e)
+  or
   // A return statement is often used to just end the function.
   e = any(Function f).getBody()
   or
   e = any(ReturnStmt r).getExpr()
-  or 
-  exists(ConditionalExpr cond | cond.getABranch() = e and benignContext(cond)) 
-  or 
-  exists(LogicalBinaryExpr bin | bin.getAnOperand() = e and benignContext(bin)) 
+  or
+  exists(ConditionalExpr cond | cond.getABranch() = e and benignContext(cond))
+  or
+  exists(LogicalBinaryExpr bin | bin.getAnOperand() = e and benignContext(bin))
   or
   exists(Expr parent | parent.getUnderlyingValue() = e and benignContext(parent))
-  or 
+  or
   any(VoidExpr voidExpr).getOperand() = e
   or
   // weeds out calls inside HTML-attributes.
-  e.getParent().(ExprStmt).getParent() instanceof CodeInAttribute or
+  e.getParent().(ExprStmt).getParent() instanceof CodeInAttribute
+  or
   // and JSX-attributes.
-  e = any(JSXAttribute attr).getValue() or 
-  
-  exists(AwaitExpr await | await.getOperand() = e and benignContext(await)) 
+  e = any(JSXAttribute attr).getValue()
+  or
+  exists(AwaitExpr await | await.getOperand() = e and benignContext(await))
   or
   // Avoid double reporting with js/trivial-conditional
   isExplicitConditional(_, e)
-  or 
+  or
   // Avoid double reporting with js/comparison-between-incompatible-types
   any(Comparison binOp).getAnOperand() = e
   or
@@ -62,13 +63,18 @@ predicate benignContext(Expr e) {
   any(PropAccess ac).getBase() = e
   or
   // Avoid double-reporting with js/unused-local-variable
-  exists(VariableDeclarator v | v.getInit() = e and v.getBindingPattern().getVariable() instanceof UnusedLocal)
+  exists(VariableDeclarator v |
+    v.getInit() = e and v.getBindingPattern().getVariable() instanceof UnusedLocal
+  )
   or
   // Avoid double reporting with js/call-to-non-callable
   any(InvokeExpr invoke).getCallee() = e
   or
-  // arguments to Promise.resolve (and promise library variants) are benign. 
+  // arguments to Promise.resolve (and promise library variants) are benign.
   e = any(PromiseCreationCall promise).getValue().asExpr()
+  or
+  // arguments to other (unknown) promise creations.
+  e = any(DataFlow::CallNode call | call.getCalleeName() = "resolve").getAnArgument().asExpr()
 }
 
 predicate oneshotClosure(DataFlow::CallNode call) {
@@ -86,15 +92,13 @@ predicate alwaysThrows(Function f) {
 /**
  * Holds if the last statement in the function is flagged by the js/useless-expression query.
  */
-predicate lastStatementHasNoEffect(Function f) {
-  hasNoEffect(f.getExit().getAPredecessor())
-}
+predicate lastStatementHasNoEffect(Function f) { hasNoEffect(f.getExit().getAPredecessor()) }
 
 /**
  * Holds if `func` is a callee of `call`, and all possible callees of `call` never return a value.
  */
 predicate callToVoidFunction(DataFlow::CallNode call, Function func) {
-  not call.isIncomplete() and 
+  not call.isIncomplete() and
   func = call.getACallee() and
   forall(Function f | f = call.getACallee() |
     returnsVoid(f) and not isStub(f) and not alwaysThrows(f)
@@ -122,22 +126,20 @@ predicate hasNonVoidCallbackMethod(string name) {
 DataFlow::SourceNode array(DataFlow::TypeTracker t) {
   t.start() and result instanceof DataFlow::ArrayCreationNode
   or
-  exists (DataFlow::TypeTracker t2 |
-    result = array(t2).track(t2, t)
-  )
+  exists(DataFlow::TypeTracker t2 | result = array(t2).track(t2, t))
 }
 
 DataFlow::SourceNode array() { result = array(DataFlow::TypeTracker::end()) }
 
 /**
  * Holds if `call` is an Array or Lodash method accepting a callback `func`,
- * where the `call` expects a callback that returns an expression, 
- * but `func` does not return a value. 
+ * where the `call` expects a callback that returns an expression,
+ * but `func` does not return a value.
  */
 predicate voidArrayCallback(DataFlow::CallNode call, Function func) {
   hasNonVoidCallbackMethod(call.getCalleeName()) and
-  exists(int index | 
-    index = min(int i | exists(call.getCallback(i))) and 
+  exists(int index |
+    index = min(int i | exists(call.getCallback(i))) and
     func = call.getCallback(index).getFunction()
   ) and
   returnsVoid(func) and
@@ -151,71 +153,20 @@ predicate voidArrayCallback(DataFlow::CallNode call, Function func) {
 }
 
 predicate hasNonVoidReturnType(Function f) {
-  exists(TypeAnnotation type | type = f.getReturnTypeAnnotation() | 
-    not type.isVoid()
-  )
-}
-
-
-/**
- * Provides classes for working with various Deferred implementations. 
- * It is a heuristic. The heuristic assume that a class is a promise defintion 
- * if the class is called "Deferred" and the method `resolve` is called on an instance.
- *  
- * Removes some false positives in the js/use-of-returnless-function query.  
- */
-module Deferred {
-  /**
-   * An instance of a `Deferred` class. 
-   * For example the result from `new Deferred()` or `new $.Deferred()`.
-   */
-  class DeferredInstance extends DataFlow::NewNode {
-  	// Describes both `new Deferred()`, `new $.Deferred` and other variants. 
-    DeferredInstance() { this.getCalleeName() = "Deferred" }
-
-    private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
-      t.start() and
-      result = this
-      or
-      exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
-    }
-    
-    DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
-  }
-
-  /**
-   * A promise object created by a Deferred constructor
-   */
-  private class DeferredPromiseDefinition extends PromiseDefinition, DeferredInstance {
-    DeferredPromiseDefinition() {
-      // hardening of the "Deferred" heuristic: a method call to `resolve`. 
-      exists(ref().getAMethodCall("resolve"))
-    }
-
-    override DataFlow::FunctionNode getExecutor() { result = getCallback(0) }
-  }
-
-  /**
-   * A resolved promise created by a `new Deferred().resolve()` call.
-   */
-  class ResolvedDeferredPromiseDefinition extends PromiseCreationCall {
-    ResolvedDeferredPromiseDefinition() {
-      this = any(DeferredPromiseDefinition def).ref().getAMethodCall("resolve")
-    }
-
-    override DataFlow::Node getValue() { result = getArgument(0) }
-  }
+  exists(TypeAnnotation type | type = f.getReturnTypeAnnotation() | not type.isVoid())
 }
 
 from DataFlow::CallNode call, Function func, string name, string msg
 where
   (
-    callToVoidFunction(call, func) and 
+    callToVoidFunction(call, func) and
     msg = "the $@ does not return anything, yet the return value is used." and
     name = func.describe()
     or
-    voidArrayCallback(call, func) and 
-    msg = "the $@ does not return anything, yet the return value from the call to " + call.getCalleeName() + " is used." and
+    voidArrayCallback(call, func) and
+    msg =
+      "the $@ does not return anything, yet the return value from the call to " +
+        call.getCalleeName() + " is used." and
     name = "callback function"
   ) and
   not benignContext(call.getEnclosingExpr()) and
@@ -224,5 +175,4 @@ where
   not oneshotClosure(call) and
   not hasNonVoidReturnType(func) and
   not call.getEnclosingExpr() instanceof SuperCall
-select
-  call, msg, func, name
+select call, msg, func, name

javascript/ql/src/Statements/UselessConditional.qll

@@ -1,6 +1,6 @@
-/** 
- * Provides predicates for working with useless conditionals.  
- */ 
+/**
+ * Provides predicates for working with useless conditionals.
+ */
 
 import javascript
 
@@ -18,4 +18,4 @@ predicate isExplicitConditional(ASTNode cond, Expr e) {
   or
   isExplicitConditional(_, cond) and
   e = cond.(Expr).getUnderlyingValue().(LogicalBinaryExpr).getAnOperand()
-}
+}

javascript/ql/src/definitions.ql

@@ -78,7 +78,7 @@ predicate importLookup(ASTNode path, Module target, string kind) {
     or
     exists(ReExportDeclaration red |
       path = red.getImportedPath() and
-      target = red.getImportedModule()
+      target = red.getReExportedModule()
     )
   )
 }

javascript/ql/src/experimental/README.md

@@ -0,0 +1 @@
+This directory contains [experimental](../../../../docs/experimental.md) CodeQL queries and libraries.

javascript/ql/src/experimental/SockJS/SockJS.qll

@@ -0,0 +1,46 @@
+/**
+ * Provides classes for working with [SockJS](http://sockjs.org).
+ */
+
+import javascript
+
+/**
+ * A model of the `SockJS` websocket data handler (https://sockjs.org).
+ */
+module SockJS {
+  /**
+   * Access to user-controlled data object received from websocket
+   * For example:
+   * ```
+   * server.on('connection', function(conn) {
+   *     conn.on('data', function(message) {
+   *       ...
+   *     });
+   *  });
+   * ```
+   */
+  class SourceFromSocketJS extends RemoteFlowSource {
+    SourceFromSocketJS() {
+      exists(
+        DataFlow::CallNode createServer, DataFlow::CallNode connNode,
+        DataFlow::CallNode dataHandlerNode
+      |
+        createServer = appCreation() and
+        connNode = createServer.getAMethodCall("on") and
+        connNode.getArgument(0).getStringValue() = "connection" and
+        dataHandlerNode = connNode.getCallback(1).getParameter(0).getAMethodCall("on") and
+        dataHandlerNode.getArgument(0).getStringValue() = "data" and
+        this = dataHandlerNode.getCallback(1).getParameter(0)
+      )
+    }
+
+    override string getSourceType() { result = "input from SockJS WebSocket" }
+  }
+
+  /**
+   * Gets a new SockJS server.
+   */
+  private DataFlow::CallNode appCreation() {
+    result = DataFlow::moduleImport("sockjs").getAMemberCall("createServer")
+  }
+}

javascript/ql/src/experimental/SockJS/examples/server.js

@@ -0,0 +1,16 @@
+const express = require('express');
+const http = require('http');
+const sockjs = require('sockjs');
+
+const app = express();
+const server = http.createServer(app);
+const sockjs_echo = sockjs.createServer({});
+sockjs_echo.on('connection', function(conn) {
+    conn.on('data', function(message) {
+        var data = JSON.parse(message);
+        conn.write(JSON.stringify(eval(data.test)));
+    });
+});
+
+sockjs_echo.installHandlers(server, {prefix:'/echo'});
+server.listen(9090, '127.0.0.1');

javascript/ql/src/experimental/Summaries/ExtractFlowStepSummaries.ql

@@ -14,8 +14,9 @@ import PortalExitSource
 import PortalEntrySink
 
 from
-  TaintTracking::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink, Portal p1,
-  Portal p2, DataFlow::FlowLabel lbl1, DataFlow::FlowLabel lbl2, DataFlow::MidPathNode last
+  TaintTracking::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
+  Portal p1, Portal p2, DataFlow::FlowLabel lbl1, DataFlow::FlowLabel lbl2,
+  DataFlow::MidPathNode last
 where
   cfg = source.getConfiguration() and
   last = source.getASuccessor*() and

javascript/ql/src/experimental/Summaries/ExtractSinkSummaries.ql

@@ -11,7 +11,8 @@ import Configurations
 import PortalExitSource
 import SinkFromAnnotation
 
-from DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
+from
+  DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
   Portal p, DataFlow::MidPathNode last
 where
   cfg = source.getConfiguration() and

javascript/ql/src/experimental/Summaries/ExtractSourceSummaries.ql

@@ -11,7 +11,8 @@ import Configurations
 import PortalEntrySink
 import SourceFromAnnotation
 
-from DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
+from
+  DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
   Portal p, DataFlow::MidPathNode last
 where
   cfg = source.getConfiguration() and

javascript/ql/src/experimental/Summaries/ImportFromCsv.qll

@@ -127,7 +127,8 @@ class AdditionalStepSpec extends ExternalData {
     exists(string config |
       if getField(4) = "" then config = "any configuration" else config = getConfiguration()
     |
-      result = "edge from " + getStartPortal() + " to " + getEndPortal() + ", transforming " +
+      result =
+        "edge from " + getStartPortal() + " to " + getEndPortal() + ", transforming " +
           getStartFlowLabel() + " into " + getEndFlowLabel() + " for " + config
     )
   }

javascript/ql/src/external/CodeDuplication.qll

@@ -266,7 +266,8 @@ predicate similarLines(File f, int line) {
 }
 
 private predicate similarLinesPerEquivalenceClass(int equivClass, int lines, File f) {
-  lines = strictsum(SimilarBlock b, int toSum |
+  lines =
+    strictsum(SimilarBlock b, int toSum |
       (b.sourceFile() = f and b.getEquivalenceClass() = equivClass) and
       toSum = b.sourceLines()
     |
@@ -278,7 +279,8 @@ pragma[noopt]
 private predicate similarLinesCovered(File f, int coveredLines, File otherFile) {
   exists(int numLines | numLines = f.getNumberOfLines() |
     exists(int coveredApprox |
-      coveredApprox = strictsum(int num |
+      coveredApprox =
+        strictsum(int num |
           exists(int equivClass |
             similarLinesPerEquivalenceClass(equivClass, num, f) and
             similarLinesPerEquivalenceClass(equivClass, num, otherFile) and
@@ -301,7 +303,8 @@ predicate duplicateLines(File f, int line) {
 }
 
 private predicate duplicateLinesPerEquivalenceClass(int equivClass, int lines, File f) {
-  lines = strictsum(DuplicateBlock b, int toSum |
+  lines =
+    strictsum(DuplicateBlock b, int toSum |
       (b.sourceFile() = f and b.getEquivalenceClass() = equivClass) and
       toSum = b.sourceLines()
     |
@@ -313,7 +316,8 @@ pragma[noopt]
 private predicate duplicateLinesCovered(File f, int coveredLines, File otherFile) {
   exists(int numLines | numLines = f.getNumberOfLines() |
     exists(int coveredApprox |
-      coveredApprox = strictsum(int num |
+      coveredApprox =
+        strictsum(int num |
           exists(int equivClass |
             duplicateLinesPerEquivalenceClass(equivClass, num, f) and
             duplicateLinesPerEquivalenceClass(equivClass, num, otherFile) and

javascript/ql/src/external/DefectFilter.qll

@@ -48,7 +48,8 @@ class DefectResult extends int {
 
   /** Gets the URL corresponding to the location of this query result. */
   string getURL() {
-    result = "file://" + getFile().getAbsolutePath() + ":" + getStartLine() + ":" + getStartColumn()
-        + ":" + getEndLine() + ":" + getEndColumn()
+    result =
+      "file://" + getFile().getAbsolutePath() + ":" + getStartLine() + ":" + getStartColumn() + ":" +
+        getEndLine() + ":" + getEndColumn()
   }
 }

javascript/ql/src/external/MetricFilter.qll

@@ -66,7 +66,8 @@ class MetricResult extends int {
 
   /** Gets the URL corresponding to the location of this query result. */
   string getURL() {
-    result = "file://" + getFile().getAbsolutePath() + ":" + getStartLine() + ":" + getStartColumn()
-        + ":" + getEndLine() + ":" + getEndColumn()
+    result =
+      "file://" + getFile().getAbsolutePath() + ":" + getStartLine() + ":" + getStartColumn() + ":" +
+        getEndLine() + ":" + getEndColumn()
   }
 }

javascript/ql/src/javascript.qll

@@ -78,6 +78,7 @@ import semmle.javascript.frameworks.Files
 import semmle.javascript.frameworks.Firebase
 import semmle.javascript.frameworks.jQuery
 import semmle.javascript.frameworks.Handlebars
+import semmle.javascript.frameworks.LazyCache
 import semmle.javascript.frameworks.LodashUnderscore
 import semmle.javascript.frameworks.Logging
 import semmle.javascript.frameworks.HttpFrameworks

javascript/ql/src/meta/analysis-quality/CallGraphQuality.qll

@@ -86,9 +86,7 @@ SourceNode nodeLeadingToInvocation() {
  * Holds if there is a call edge `invoke -> f` between a relevant invocation
  * and a relevant function.
  */
-predicate relevantCall(RelevantInvoke invoke, RelevantFunction f) {
-  FlowSteps::calls(invoke, f)
-}
+predicate relevantCall(RelevantInvoke invoke, RelevantFunction f) { FlowSteps::calls(invoke, f) }
 
 /**
  * A call site that can be resolved to a function in the same project.
@@ -105,9 +103,7 @@ class ResolvableCall extends RelevantInvoke {
  * A call site that could not be resolved.
  */
 class UnresolvableCall extends RelevantInvoke {
-  UnresolvableCall() {
-    not this instanceof ResolvableCall
-  }
+  UnresolvableCall() { not this instanceof ResolvableCall }
 }
 
 /**

javascript/ql/src/meta/analysis-quality/UnresolvableImports.ql

@@ -11,8 +11,6 @@
 import javascript
 import CallGraphQuality
 
-Import unresolvableImport() {
-    not exists(result.getImportedModule())
-}
+Import unresolvableImport() { not exists(result.getImportedModule()) }
 
 select projectRoot(), count(unresolvableImport())

javascript/ql/src/meta/extraction-metrics/MissingMetrics.ql

@@ -12,7 +12,8 @@ import javascript
 import semmle.javascript.meta.ExtractionMetrics::ExtractionMetrics
 
 from File f, string cause
-where not extraction_data(f, _, _, _) and cause = "No extraction_data for this file"
-      or
-      not extraction_time(f, _,_, _) and cause =  "No extraction_time for this file"
-select f, cause
+where
+  not extraction_data(f, _, _, _) and cause = "No extraction_data for this file"
+  or
+  not extraction_time(f, _, _, _) and cause = "No extraction_time for this file"
+select f, cause

javascript/ql/src/meta/extraction-metrics/PhaseTimings.ql

@@ -9,4 +9,5 @@
 import semmle.javascript.meta.ExtractionMetrics::ExtractionMetrics
 
 from PhaseName phaseName
-select phaseName, Aggregated::getCpuTime(phaseName) as CPU_NANO, Aggregated::getWallclockTime(phaseName) as WALLCLOCK_NANO
+select phaseName, Aggregated::getCpuTime(phaseName) as CPU_NANO,
+  Aggregated::getWallclockTime(phaseName) as WALLCLOCK_NANO

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

@@ -125,7 +125,7 @@ class ASTNode extends @ast_node, Locatable {
    * Holds if this is part of an ambient declaration or type annotation in a TypeScript file.
    *
    * A declaration is ambient if it occurs under a `declare` modifier or is
-   * an interface declaration, type alias, or type annotation.
+   * an interface declaration, type alias, type annotation, or type-only import/export declaration.
    *
    * The TypeScript compiler emits no code for ambient declarations, but they
    * can affect name resolution and type checking at compile-time.

javascript/ql/src/semmle/javascript/BasicBlocks.qll

@@ -235,7 +235,8 @@ class BasicBlock extends @cfg_node, Locatable {
    */
   private int nextDefOrUseAfter(PurelyLocalVariable v, int i, VarDef d) {
     defAt(i, v, d) and
-    result = min(int j |
+    result =
+      min(int j |
         (defAt(j, v, _) or useAt(j, v, _) or j = length()) and
         j > i
       )

javascript/ql/src/semmle/javascript/CharacterEscapes.qll

@@ -57,7 +57,8 @@ module CharacterEscapes {
       hasRawStringAndQuote(n, delim, rawStringNode, raw) and
       if rawStringNode instanceof RegExpLiteral
       then
-        additionalEscapeChars = Sets::regexpMetaChars() + Sets::regexpAssertionChars() + Sets::regexpCharClassChars() +
+        additionalEscapeChars =
+          Sets::regexpMetaChars() + Sets::regexpAssertionChars() + Sets::regexpCharClassChars() +
             Sets::regexpBackreferenceChars()
       else additionalEscapeChars = "b"
     |
@@ -92,6 +93,12 @@ module CharacterEscapes {
       // conservative formulation: we do not know in general if the sequence is enclosed in a character class `[...]`
       result = Sets::regexpMetaChars().charAt(_) and
       mistake = "may still represent a meta-character"
+    ) and
+    // avoid the benign case where preceding escaped backslashes turns into backslashes when the regexp is constructed
+    not exists(string raw |
+      not rawStringNode instanceof RegExpLiteral and
+      hasRawStringAndQuote(_, _, rawStringNode, raw) and
+      result = raw.regexpFind("(?<=(^|[^\\\\])((\\\\{3})|(\\\\{7}))).", _, i)
     )
   }
 }

javascript/ql/src/semmle/javascript/Classes.qll

@@ -123,13 +123,14 @@ class ClassOrInterface extends @classorinterface, TypeParameterized {
    * Anonymous classes and interfaces do not have a canonical name.
    */
   TypeName getTypeName() { result.getADefinition() = this }
-  
+
   /**
    * Gets the ClassOrInterface corresponding to either a super type or an implemented interface.
    */
   ClassOrInterface getASuperTypeDeclaration() {
     this.getSuperClass().(VarAccess).getVariable().getADeclaration() = result.getIdentifier() or
-    this.getASuperInterface().(LocalTypeAccess).getLocalTypeName().getADeclaration() = result.getIdentifier()
+    this.getASuperInterface().(LocalTypeAccess).getLocalTypeName().getADeclaration() =
+      result.getIdentifier()
   }
 }
 
@@ -334,11 +335,8 @@ class ClassExpr extends @classexpr, ClassDefinition, Expr {
       else
         if exists(getClassInitializedMember())
         then
-          result = min(ClassInitializedMember m |
-              m = getClassInitializedMember()
-            |
-              m order by m.getIndex()
-            )
+          result =
+            min(ClassInitializedMember m | m = getClassInitializedMember() | m order by m.getIndex())
         else result = this
   }
 
@@ -688,7 +686,8 @@ class MethodDeclaration extends MemberDeclaration {
    */
   int getOverloadIndex() {
     exists(ClassOrInterface type, string name |
-      this = rank[result + 1](MethodDeclaration method, int i |
+      this =
+        rank[result + 1](MethodDeclaration method, int i |
           methodDeclaredInType(type, name, i, method)
         |
           method order by i
@@ -696,7 +695,8 @@ class MethodDeclaration extends MemberDeclaration {
     )
     or
     exists(ClassDefinition type |
-      this = rank[result + 1](ConstructorDeclaration ctor, int i |
+      this =
+        rank[result + 1](ConstructorDeclaration ctor, int i |
           ctor = type.getMemberByIndex(i)
         |
           ctor order by i
@@ -1156,7 +1156,8 @@ class FunctionCallSignature extends @function_call_signature, CallSignature {
   /** Gets the index of this function call signature among the function call signatures in the enclosing type. */
   int getOverloadIndex() {
     exists(ClassOrInterface type | type = getDeclaringType() |
-      this = rank[result + 1](FunctionCallSignature sig, int i |
+      this =
+        rank[result + 1](FunctionCallSignature sig, int i |
           sig = type.getMemberByIndex(i)
         |
           sig order by i
@@ -1186,7 +1187,8 @@ class ConstructorCallSignature extends @constructor_call_signature, CallSignatur
   /** Gets the index of this constructor call signature among the constructor call signatures in the enclosing type. */
   int getOverloadIndex() {
     exists(ClassOrInterface type | type = getDeclaringType() |
-      this = rank[result + 1](ConstructorCallSignature sig, int i |
+      this =
+        rank[result + 1](ConstructorCallSignature sig, int i |
           sig = type.getMemberByIndex(i)
         |
           sig order by i

javascript/ql/src/semmle/javascript/Closure.qll

@@ -267,6 +267,9 @@ module Closure {
       result = this
     }
 
-    override DataFlow::Node getBoundReceiver() { result = getArgument(1) }
+    override DataFlow::Node getBoundReceiver(DataFlow::Node callback) {
+      callback = getArgument(0) and
+      result = getArgument(1)
+    }
   }
 }

javascript/ql/src/semmle/javascript/Concepts.qll

@@ -22,6 +22,14 @@ abstract class SystemCommandExecution extends DataFlow::Node {
    * to the command.
    */
   DataFlow::Node getArgumentList() { none() }
+
+  /** Holds if the command execution happens synchronously. */
+  abstract predicate isSync();
+
+  /**
+   * Gets the data-flow node (if it exists) for an options argument.
+   */
+  abstract DataFlow::Node getOptionsArg();
 }
 
 /**

javascript/ql/src/semmle/javascript/DefUse.qll

@@ -284,7 +284,8 @@ private SsaDefinition getAPseudoDefinitionInput(SsaDefinition nd) {
  */
 private int nextDefAfter(BasicBlock bb, Variable v, int i, VarDef d) {
   bb.defAt(i, v, d) and
-  result = min(int jj |
+  result =
+    min(int jj |
       (bb.defAt(jj, v, _) or jj = bb.length()) and
       jj > i
     )

javascript/ql/src/semmle/javascript/DynamicPropertyAccess.qll

@@ -0,0 +1,222 @@
+/**
+ * Provides classes for working with dynamic property accesses.
+ */
+
+import javascript
+private import semmle.javascript.dataflow.InferredTypes
+private import semmle.javascript.dataflow.DataFlow::DataFlow
+private import semmle.javascript.dataflow.internal.FlowSteps
+
+/**
+ * Gets a node that refers to an element of `array`, likely obtained
+ * as a result of enumerating the elements of the array.
+ */
+SourceNode getAnEnumeratedArrayElement(SourceNode array) {
+  exists(MethodCallNode call, string name |
+    call = array.getAMethodCall(name) and
+    (name = "forEach" or name = "map") and
+    result = call.getCallback(0).getParameter(0)
+  )
+  or
+  exists(DataFlow::PropRead read |
+    read = array.getAPropertyRead() and
+    not exists(read.getPropertyName()) and
+    not read.getPropertyNameExpr().analyze().getAType() = TTString() and
+    result = read
+  )
+}
+
+/**
+ * A data flow node that refers to the name of a property obtained by enumerating
+ * the properties of some object.
+ */
+abstract class EnumeratedPropName extends DataFlow::Node {
+  /**
+   * Gets the data flow node holding the object whose properties are being enumerated.
+   *
+   * For example, gets `src` in `for (var key in src)`.
+   */
+  abstract DataFlow::Node getSourceObject();
+
+  /**
+   * Gets a source node that refers to the object whose properties are being enumerated.
+   */
+  DataFlow::SourceNode getASourceObjectRef() {
+    result = AccessPath::getAnAliasedSourceNode(getSourceObject())
+  }
+
+  /**
+   * Gets a property read that accesses the corresponding property value in the source object.
+   *
+   * For example, gets `src[key]` in `for (var key in src) { src[key]; }`.
+   */
+  SourceNode getASourceProp() {
+    exists(Node base, Node key |
+      dynamicPropReadStep(base, key, result) and
+      getASourceObjectRef().flowsTo(base) and
+      key.getImmediatePredecessor*() = this
+    )
+  }
+}
+
+/**
+ * Property enumeration through `for-in` for `Object.keys` or similar.
+ */
+private class ForInEnumeratedPropName extends EnumeratedPropName {
+  DataFlow::Node object;
+
+  ForInEnumeratedPropName() {
+    exists(ForInStmt stmt |
+      this = DataFlow::lvalueNode(stmt.getLValue()) and
+      object = stmt.getIterationDomain().flow()
+    )
+    or
+    exists(CallNode call |
+      call = globalVarRef("Object").getAMemberCall("keys")
+      or
+      call = globalVarRef("Object").getAMemberCall("getOwnPropertyNames")
+      or
+      call = globalVarRef("Reflect").getAMemberCall("ownKeys")
+    |
+      object = call.getArgument(0) and
+      this = getAnEnumeratedArrayElement(call)
+    )
+  }
+
+  override Node getSourceObject() { result = object }
+}
+
+/**
+ * Property enumeration through `Object.entries`.
+ */
+private class EntriesEnumeratedPropName extends EnumeratedPropName {
+  CallNode entries;
+  SourceNode entry;
+
+  EntriesEnumeratedPropName() {
+    entries = globalVarRef("Object").getAMemberCall("entries") and
+    entry = getAnEnumeratedArrayElement(entries) and
+    this = entry.getAPropertyRead("0")
+  }
+
+  override DataFlow::Node getSourceObject() { result = entries.getArgument(0) }
+
+  override SourceNode getASourceProp() {
+    result = super.getASourceProp()
+    or
+    result = entry.getAPropertyRead("1")
+  }
+}
+
+/**
+ * Gets a function that enumerates object properties when invoked.
+ *
+ * Invocations takes the following form:
+ * ```js
+ * fn(obj, (value, key, o) => { ... })
+ * ```
+ */
+private SourceNode propertyEnumerator() {
+  result = moduleImport("for-own") or
+  result = moduleImport("for-in") or
+  result = moduleMember("ramda", "forEachObjIndexed") or
+  result = LodashUnderscore::member("forEach") or
+  result = LodashUnderscore::member("each")
+}
+
+/**
+ * Property enumeration through a library function taking a callback.
+ */
+private class LibraryCallbackEnumeratedPropName extends EnumeratedPropName {
+  CallNode call;
+  FunctionNode callback;
+
+  LibraryCallbackEnumeratedPropName() {
+    call = propertyEnumerator().getACall() and
+    callback = call.getCallback(1) and
+    this = callback.getParameter(1)
+  }
+
+  override Node getSourceObject() { result = call.getArgument(0) }
+
+  override SourceNode getASourceObjectRef() {
+    result = super.getASourceObjectRef()
+    or
+    result = callback.getParameter(2)
+  }
+
+  override SourceNode getASourceProp() {
+    result = super.getASourceProp()
+    or
+    result = callback.getParameter(0)
+  }
+}
+
+/**
+ * A dynamic property access that is not obviously an array access.
+ */
+class DynamicPropRead extends DataFlow::SourceNode, DataFlow::ValueNode {
+  // Use IndexExpr instead of PropRead as we're not interested in implicit accesses like
+  // rest-patterns and for-of loops.
+  override IndexExpr astNode;
+
+  DynamicPropRead() {
+    not exists(astNode.getPropertyName()) and
+    // Exclude obvious array access
+    astNode.getPropertyNameExpr().analyze().getAType() = TTString()
+  }
+
+  /** Gets the base of the dynamic read. */
+  DataFlow::Node getBase() { result = astNode.getBase().flow() }
+
+  /** Gets the node holding the name of the property. */
+  DataFlow::Node getPropertyNameNode() { result = astNode.getIndex().flow() }
+
+  /**
+   * Holds if the value of this read was assigned to earlier in the same basic block.
+   *
+   * For example, this is true for `dst[x]` on line 2 below:
+   * ```js
+   * dst[x] = {};
+   * dst[x][y] = src[y];
+   * ```
+   */
+  predicate hasDominatingAssignment() {
+    exists(DataFlow::PropWrite write, BasicBlock bb, int i, int j, SsaVariable ssaVar |
+      write = getBase().getALocalSource().getAPropertyWrite() and
+      bb.getNode(i) = write.getWriteNode() and
+      bb.getNode(j) = astNode and
+      i < j and
+      write.getPropertyNameExpr() = ssaVar.getAUse() and
+      astNode.getIndex() = ssaVar.getAUse()
+    )
+  }
+}
+
+/**
+ * Holds if `output` is the result of `base[key]`, either directly or through
+ * one or more function calls, ignoring reads that can't access the prototype chain.
+ */
+predicate dynamicPropReadStep(Node base, Node key, SourceNode output) {
+  exists(DynamicPropRead read |
+    not read.hasDominatingAssignment() and
+    base = read.getBase() and
+    key = read.getPropertyNameNode() and
+    output = read
+  )
+  or
+  // Summarize functions returning a dynamic property read of two parameters, such as `function getProp(obj, prop) { return obj[prop]; }`.
+  exists(
+    CallNode call, Function callee, ParameterNode baseParam, ParameterNode keyParam, Node innerBase,
+    Node innerKey, SourceNode innerOutput
+  |
+    dynamicPropReadStep(innerBase, innerKey, innerOutput) and
+    baseParam.flowsTo(innerBase) and
+    keyParam.flowsTo(innerKey) and
+    innerOutput.flowsTo(callee.getAReturnedExpr().flow()) and
+    call.getACallee() = callee and
+    argumentPassingStep(call, base, callee, baseParam) and
+    argumentPassingStep(call, key, callee, keyParam) and
+    output = call
+  )
+}

javascript/ql/src/semmle/javascript/ES2015Modules.qll

@@ -76,6 +76,14 @@ class ImportDeclaration extends Stmt, Import, @importdeclaration {
     // `import { createServer } from 'http'`
     result = DataFlow::destructuredModuleImportNode(this)
   }
+
+  /** Holds if this is declared with the `type` keyword, so it only imports types. */
+  predicate isTypeOnly() { hasTypeKeyword(this) }
+
+  override predicate isAmbient() {
+    Stmt.super.isAmbient() or
+    isTypeOnly()
+  }
 }
 
 /** A literal path expression appearing in an `import` declaration. */
@@ -256,6 +264,14 @@ abstract class ExportDeclaration extends Stmt, @exportdeclaration {
    * to module `a` or possibly to some other module from which `a` re-exports.
    */
   abstract DataFlow::Node getSourceNode(string name);
+
+  /** Holds if is declared with the `type` keyword, so only types are exported. */
+  predicate isTypeOnly() { hasTypeKeyword(this) }
+
+  override predicate isAmbient() {
+    Stmt.super.isAmbient() or
+    isTypeOnly()
+  }
 }
 
 /**
@@ -273,12 +289,12 @@ class BulkReExportDeclaration extends ReExportDeclaration, @exportalldeclaration
   override ConstantString getImportedPath() { result = getChildExpr(0) }
 
   override predicate exportsAs(LexicalName v, string name) {
-    getImportedModule().exportsAs(v, name) and
+    getReExportedES2015Module().exportsAs(v, name) and
     not isShadowedFromBulkExport(this, name)
   }
 
   override DataFlow::Node getSourceNode(string name) {
-    result = getImportedModule().getAnExport().getSourceNode(name)
+    result = getReExportedES2015Module().getAnExport().getSourceNode(name)
   }
 }
 
@@ -379,7 +395,7 @@ class ExportNamedDeclaration extends ExportDeclaration, @exportnameddeclaration
     exists(ExportSpecifier spec | spec = getASpecifier() and name = spec.getExportedName() |
       v = spec.getLocal().(LexicalAccess).getALexicalName()
       or
-      this.(ReExportDeclaration).getImportedModule().exportsAs(v, spec.getLocalName())
+      this.(ReExportDeclaration).getReExportedES2015Module().exportsAs(v, spec.getLocalName())
     )
   }
 
@@ -393,7 +409,7 @@ class ExportNamedDeclaration extends ExportDeclaration, @exportnameddeclaration
       not exists(getImportedPath()) and result = DataFlow::valueNode(spec.getLocal())
       or
       exists(ReExportDeclaration red | red = this |
-        result = red.getImportedModule().getAnExport().getSourceNode(spec.getLocalName())
+        result = red.getReExportedES2015Module().getAnExport().getSourceNode(spec.getLocalName())
       )
     )
   }
@@ -413,6 +429,18 @@ class ExportNamedDeclaration extends ExportDeclaration, @exportnameddeclaration
   }
 }
 
+/**
+ * An export declaration with the `type` modifier.
+ */
+private class TypeOnlyExportDeclaration extends ExportNamedDeclaration {
+  TypeOnlyExportDeclaration() { isTypeOnly() }
+
+  override predicate exportsAs(LexicalName v, string name) {
+    super.exportsAs(v, name) and
+    not v instanceof Variable
+  }
+}
+
 /**
  * An export specifier in an export declaration.
  *
@@ -545,14 +573,18 @@ class ReExportDefaultSpecifier extends ExportDefaultSpecifier {
 }
 
 /**
- * A namespace export specifier.
+ * A namespace export specifier, that is `*` or `* as x` occuring in an export declaration.
  *
- * Example:
+ * Examples:
  *
  * ```
  * export
  *   *          // namespace export specifier
  *   from 'a';
+ *
+ * export
+ *   * as x     // namespace export specifier
+ *   from 'a';
  * ```
  */
 class ExportNamespaceSpecifier extends ExportSpecifier, @exportnamespacespecifier { }
@@ -564,6 +596,7 @@ class ExportNamespaceSpecifier extends ExportSpecifier, @exportnamespacespecifie
  *
  * ```
  * export * from 'a';               // bulk re-export declaration
+ * export * as x from 'a';          // namespace re-export declaration
  * export { x } from 'a';           // named re-export declaration
  * export x from 'a';               // default re-export declaration
  * ```
@@ -572,8 +605,18 @@ abstract class ReExportDeclaration extends ExportDeclaration {
   /** Gets the path of the module from which this declaration re-exports. */
   abstract ConstantString getImportedPath();
 
+  /**
+   * DEPRECATED. Use `getReExportedES2015Module()` instead.
+   *
+   * Gets the module from which this declaration re-exports.
+   */
+  deprecated ES2015Module getImportedModule() { result = getReExportedModule() }
+
+  /** Gets the module from which this declaration re-exports, if it is an ES2015 module. */
+  ES2015Module getReExportedES2015Module() { result = getReExportedModule() }
+
   /** Gets the module from which this declaration re-exports. */
-  ES2015Module getImportedModule() {
+  Module getReExportedModule() {
     result.getFile() = getEnclosingModule().resolve(getImportedPath().(PathExpr))
     or
     result = resolveFromTypeRoot()
@@ -583,7 +626,8 @@ abstract class ReExportDeclaration extends ExportDeclaration {
    * Gets a module in a `node_modules/@types/` folder that matches the imported module name.
    */
   private Module resolveFromTypeRoot() {
-    result.getFile() = min(TypeRootFolder typeRoot |
+    result.getFile() =
+      min(TypeRootFolder typeRoot |
         |
         typeRoot.getModuleFile(getImportedPath().getStringValue())
         order by
@@ -641,4 +685,4 @@ class OriginalExportDeclaration extends ExportDeclaration {
     result = this.(ExportDefaultDeclaration).getSourceNode(name) or
     result = this.(ExportNamedDeclaration).getSourceNode(name)
   }
-}
+}

javascript/ql/src/semmle/javascript/EmailClients.qll

@@ -43,9 +43,8 @@ abstract class EmailSender extends DataFlow::SourceNode {
  */
 private class NodemailerEmailSender extends EmailSender, DataFlow::MethodCallNode {
   NodemailerEmailSender() {
-    this = DataFlow::moduleMember("nodemailer", "createTransport")
-          .getACall()
-          .getAMethodCall("sendMail")
+    this =
+      DataFlow::moduleMember("nodemailer", "createTransport").getACall().getAMethodCall("sendMail")
   }
 
   override DataFlow::Node getPlainTextBody() { result = getOptionArgument(0, "text") }

javascript/ql/src/semmle/javascript/Expr.qll

@@ -253,12 +253,15 @@ class Expr extends @expr, ExprOrStmt, ExprOrType, AST::ValueNode {
   DataFlow::Node getExceptionTarget() {
     if exists(this.getEnclosingStmt().getEnclosingTryCatchStmt())
     then
-      result = DataFlow::parameterNode(this
+      result =
+        DataFlow::parameterNode(this
               .getEnclosingStmt()
               .getEnclosingTryCatchStmt()
               .getACatchClause()
               .getAParameter())
-    else result = any(DataFlow::FunctionNode f | f.getFunction() = this.getContainer()).getExceptionalReturn()
+    else
+      result =
+        any(DataFlow::FunctionNode f | f.getFunction() = this.getContainer()).getExceptionalReturn()
   }
 }
 

javascript/ql/src/semmle/javascript/Files.qll

@@ -183,11 +183,8 @@ class Folder extends Container, @folder {
    * HTML files will not be found by this method.
    */
   File getJavaScriptFile(string stem) {
-    result = min(int p, string ext |
-        p = getFileExtensionPriority(ext)
-      |
-        getFile(stem, ext) order by p
-      )
+    result =
+      min(int p, string ext | p = getFileExtensionPriority(ext) | getFile(stem, ext) order by p)
   }
 
   /** Gets a subfolder contained in this folder. */

javascript/ql/src/semmle/javascript/Functions.qll

@@ -206,7 +206,8 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
 
   /** Gets the cyclomatic complexity of this function. */
   int getCyclomaticComplexity() {
-    result = 2 +
+    result =
+      2 +
         sum(Expr nd |
           nd.getContainer() = this and nd.isBranch()
         |
@@ -420,9 +421,7 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
   /**
    * Gets the call signature of this function, as determined by the TypeScript compiler, if any.
    */
-  CallSignatureType getCallSignature() {
-    declared_function_signature(this, result)
-  }
+  CallSignatureType getCallSignature() { declared_function_signature(this, result) }
 }
 
 /**

javascript/ql/src/semmle/javascript/GeneratedCode.qll

@@ -35,8 +35,10 @@ class CodeGeneratorMarkerComment extends GeneratedCodeMarkerComment {
  */
 private predicate codeGeneratorMarkerComment(Comment c, string tool) {
   exists(string toolPattern |
-    toolPattern = "js_of_ocaml|CoffeeScript|LiveScript|dart2js|ANTLR|PEG\\.js|Opal|JSX|jison(?:-lex)?|(?:Microsoft \\(R\\) AutoRest Code Generator)|purs" and
-    tool = c
+    toolPattern =
+      "js_of_ocaml|CoffeeScript|LiveScript|dart2js|ANTLR|PEG\\.js|Opal|JSX|jison(?:-lex)?|(?:Microsoft \\(R\\) AutoRest Code Generator)|purs" and
+    tool =
+      c
           .getText()
           .regexpCapture("(?s)[\\s*]*(?:parser |Code )?[gG]eneratedy? (?:from .*)?by (" +
               toolPattern + ")\\b.*", 1)

javascript/ql/src/semmle/javascript/GlobalAccessPaths.qll

@@ -5,23 +5,18 @@
 import javascript
 private import semmle.javascript.dataflow.InferredTypes
 
-deprecated
-module GlobalAccessPath {
+deprecated module GlobalAccessPath {
   /**
    * DEPRECATED. Instead use `AccessPath::getAReferenceTo` with the result and parameter reversed.
    */
   pragma[inline]
-  string fromReference(DataFlow::Node node) {
-    node = AccessPath::getAReferenceTo(result)
-  }
+  string fromReference(DataFlow::Node node) { node = AccessPath::getAReferenceTo(result) }
 
   /**
    * DEPRECATED. Instead use `AccessPath::getAnAssignmentTo` with the result and parameter reversed.
    */
   pragma[inline]
-  string fromRhs(DataFlow::Node node) {
-    node = AccessPath::getAnAssignmentTo(result)
-  }
+  string fromRhs(DataFlow::Node node) { node = AccessPath::getAnAssignmentTo(result) }
 
   /**
    * DEPRECATED. Use `AccessPath::getAReferenceOrAssignmentTo`.
@@ -67,9 +62,7 @@ module AccessPath {
     }
 
     /** Holds if this represents the root of the global access path. */
-    predicate isGlobal() {
-      this = DataFlow::globalAccessPathRootPseudoNode()
-    }
+    predicate isGlobal() { this = DataFlow::globalAccessPathRootPseudoNode() }
   }
 
   /**
@@ -212,7 +205,8 @@ module AccessPath {
    * ```
    */
   private predicate isSelfAssignment(DataFlow::Node rhs) {
-    fromRhs(rhs, DataFlow::globalAccessPathRootPseudoNode()) = fromReference(rhs, DataFlow::globalAccessPathRootPseudoNode())
+    fromRhs(rhs, DataFlow::globalAccessPathRootPseudoNode()) =
+      fromReference(rhs, DataFlow::globalAccessPathRootPseudoNode())
   }
 
   /**
@@ -418,8 +412,8 @@ module AccessPath {
    */
   pragma[inline]
   DataFlow::SourceNode getAnAliasedSourceNode(DataFlow::Node node) {
-    exists(DataFlow::SourceNode root, string accessPath | 
-      node = AccessPath::getAReferenceTo(root, accessPath) and 
+    exists(DataFlow::SourceNode root, string accessPath |
+      node = AccessPath::getAReferenceTo(root, accessPath) and
       result = AccessPath::getAReferenceTo(root, accessPath)
     )
     or

javascript/ql/src/semmle/javascript/HtmlSanitizers.qll

@@ -49,9 +49,8 @@ private class DefaultHtmlSanitizerCall extends HtmlSanitizerCall {
       callee = LodashUnderscore::member("escape")
       or
       exists(string name | name = "encode" or name = "encodeNonUTF" |
-        callee = DataFlow::moduleMember("html-entities", _)
-              .getAnInstantiation()
-              .getAPropertyRead(name) or
+        callee =
+          DataFlow::moduleMember("html-entities", _).getAnInstantiation().getAPropertyRead(name) or
         callee = DataFlow::moduleMember("html-entities", _).getAPropertyRead(name)
       )
       or

javascript/ql/src/semmle/javascript/InclusionTests.qll

@@ -1,6 +1,7 @@
 /**
  * Contains classes for recognizing array and string inclusion tests.
  */
+
 private import javascript
 
 /**
@@ -57,6 +58,40 @@ module InclusionTest {
     boolean getPolarity() { result = true }
   }
 
+  /**
+   * A call to a utility function (`callee`) that performs an InclusionTest (`inner`).
+   */
+  private class IndirectInclusionTest extends Range, DataFlow::CallNode {
+    InclusionTest inner;
+    Function callee;
+
+    IndirectInclusionTest() {
+      inner.getEnclosingExpr() = callee.getAReturnedExpr() and
+      this.getACallee() = callee and
+      count(this.getACallee()) = 1 and
+      count(callee.getAReturnedExpr()) = 1 and
+      not this.isImprecise() and
+      inner.getContainerNode().getALocalSource().getEnclosingExpr() = callee.getAParameter() and
+      inner.getContainedNode().getALocalSource().getEnclosingExpr() = callee.getAParameter()
+    }
+
+    override DataFlow::Node getContainerNode() {
+      exists(int arg |
+        inner.getContainerNode().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
+        result = this.getArgument(arg)
+      )
+    }
+
+    override DataFlow::Node getContainedNode() {
+      exists(int arg |
+        inner.getContainedNode().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
+        result = this.getArgument(arg)
+      )
+    }
+
+    override boolean getPolarity() { result = inner.getPolarity() }
+  }
+
   /**
    * A call to a method named `includes`, assumed to refer to `String.prototype.includes`
    * or `Array.prototype.includes`.

javascript/ql/src/semmle/javascript/Locations.qll

@@ -142,5 +142,6 @@ class Locatable extends @locatable {
  */
 private class FileLocatable extends File, Locatable {
   override Location getLocation() { result = File.super.getLocation() }
+
   override string toString() { result = File.super.toString() }
 }

javascript/ql/src/semmle/javascript/Modules.qll

@@ -140,7 +140,8 @@ abstract class Import extends ASTNode {
    * Gets a module in a `node_modules/@types/` folder that matches the imported module name.
    */
   private Module resolveFromTypeRoot() {
-    result.getFile() = min(TypeRootFolder typeRoot |
+    result.getFile() =
+      min(TypeRootFolder typeRoot |
         |
         typeRoot.getModuleFile(getImportedPath().getValue())
         order by

javascript/ql/src/semmle/javascript/NodeJS.qll

@@ -243,7 +243,8 @@ class Require extends CallExpr, Import {
   private File load(int priority) {
     exists(int r | getEnclosingModule().searchRoot(getImportedPath(), _, r) |
       result = loadAsFile(this, r, priority - prioritiesPerCandidate() * r) or
-      result = loadAsDirectory(this, r,
+      result =
+        loadAsDirectory(this, r,
           priority - (prioritiesPerCandidate() * r + numberOfExtensions() + 1))
     )
   }

javascript/ql/src/semmle/javascript/NodeModuleResolutionImpl.qll

@@ -92,7 +92,7 @@ File resolveMainModule(PackageJSON pkg, int priority) {
       not exists(main.resolve()) and
       not exists(main.getExtension()) and
       exists(int n | n = main.getNumComponent() |
-        result = tryExtensions(main.resolveUpTo(n-1), main.getComponent(n-1), priority)
+        result = tryExtensions(main.resolveUpTo(n - 1), main.getComponent(n - 1), priority)
       )
     )
   else result = tryExtensions(pkg.getFile().getParentContainer(), "index", priority)

javascript/ql/src/semmle/javascript/Promises.qll

@@ -3,6 +3,7 @@
  */
 
 import javascript
+private import dataflow.internal.StepSummary
 
 /**
  * A definition of a `Promise` object.
@@ -121,41 +122,157 @@ class AggregateES2015PromiseDefinition extends PromiseCreationCall {
 }
 
 /**
- * This module defines how data-flow propagates into and out of a Promise.
- * The data-flow is based on pseudo-properties rather than tainting the Promise object (which is what `PromiseTaintStep` does).
+ * Common predicates shared between type-tracking and data-flow for promises.
  */
-private module PromiseFlow {
+module Promises {
   /**
    * Gets the pseudo-field used to describe resolved values in a promise.
    */
-  string resolveField() {
-    result = "$PromiseResolveField$"
-  }
+  string valueProp() { result = "$PromiseResolveField$" }
 
   /**
    * Gets the pseudo-field used to describe rejected values in a promise.
    */
-  string rejectField() {
-    result = "$PromiseRejectField$"
+  string errorProp() { result = "$PromiseRejectField$" }
+}
+
+/**
+ * A module for supporting promises in type-tracking predicates.
+ * The `PromiseTypeTracking::promiseStep` predicate is used for type tracking in and out of promises,
+ * and is included in the standard type-tracking steps (`SourceNode::track`).
+ * The `TypeTracker::startInPromise()` predicate can be used to initiate a type-tracker
+ * where the tracked value is a promise.
+ *
+ * The below is an example of a type-tracking predicate where the initial value is a promise:
+ * ```
+ * DataFlow::SourceNode myType(DataFlow::TypeTracker t) {
+ *  t.startInPromise() and
+ *  result = <the promise value> and
+ *  or
+ *  exists(DataFlow::TypeTracker t2 | result = myType(t2).track(t2, t))
+ * }
+ * ```
+ *
+ * The type-tracking predicate above will only end (`t = DataFlow::TypeTracker::end()`) after the tracked value has been
+ * extracted from the promise.
+ *
+ * The `PromiseTypeTracking::promiseStep` predicate can be used instead of `SourceNode::track`
+ * to get type-tracking only for promise steps.
+ *
+ * Replace `t.startInPromise()` in the above example with `t.start()` to create a type-tracking predicate
+ * where the value is not initially inside a promise.
+ */
+module PromiseTypeTracking {
+  /**
+   * Gets the result from a single step through a promise, from `pred` to `result` summarized by `summary`.
+   * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
+   */
+  DataFlow::SourceNode promiseStep(DataFlow::SourceNode pred, StepSummary summary) {
+    exists(PromiseFlowStep step, string field | field = Promises::valueProp() |
+      summary = LoadStep(field) and
+      step.load(pred, result, field)
+      or
+      summary = StoreStep(field) and
+      step.store(pred, result, field)
+      or
+      summary = LevelStep() and
+      step.loadStore(pred, result, field)
+    )
   }
-  
+
+  /**
+   * Gets the result from a single step through a promise, from `pred` with tracker `t2` to `result` with tracker `t`.
+   * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another.
+   */
+  pragma[inline]
+  DataFlow::SourceNode promiseStep(
+    DataFlow::SourceNode pred, DataFlow::TypeTracker t, DataFlow::TypeTracker t2
+  ) {
+    exists(StepSummary summary |
+      result = PromiseTypeTracking::promiseStep(pred, summary) and
+      t = t2.append(summary)
+    )
+  }
+
+  /**
+   * A class enabling the use of the `resolveField` as a pseudo-property in type-tracking predicates.
+   */
+  private class ResolveFieldAsTypeTrackingProperty extends TypeTrackingPseudoProperty {
+    ResolveFieldAsTypeTrackingProperty() { this = Promises::valueProp() }
+  }
+}
+
+/**
+ * An `AdditionalFlowStep` used to model a data-flow step related to promises.
+ *
+ * The `loadStep`/`storeStep`/`loadStoreStep` methods are overloaded such that the new predicates
+ * `load`/`store`/`loadStore` can be used in the `PromiseTypeTracking` module.
+ * (Thereby avoiding conflicts with a "cousin" `AdditionalFlowStep` implementation.)
+ *
+ * The class is private and is only intended to be used inside the `PromiseTypeTracking` and `PromiseFlow` modules.
+ */
+abstract private class PromiseFlowStep extends DataFlow::AdditionalFlowStep {
+  final override predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
+
+  final override predicate step(
+    DataFlow::Node p, DataFlow::Node s, DataFlow::FlowLabel pl, DataFlow::FlowLabel sl
+  ) {
+    none()
+  }
+
+  /**
+   * Holds if the property `prop` of the object `pred` should be loaded into `succ`.
+   */
+  predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  final override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    this.load(pred, succ, prop)
+  }
+
+  /**
+   * Holds if `pred` should be stored in the object `succ` under the property `prop`.
+   */
+  predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  final override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    this.store(pred, succ, prop)
+  }
+
+  /**
+   * Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
+   */
+  predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  final override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    this.loadStore(pred, succ, prop)
+  }
+}
+
+/**
+ * This module defines how data-flow propagates into and out of a Promise.
+ * The data-flow is based on pseudo-properties rather than tainting the Promise object (which is what `PromiseTaintStep` does).
+ */
+private module PromiseFlow {
+  private predicate valueProp = Promises::valueProp/0;
+
+  private predicate errorProp = Promises::errorProp/0;
+
   /**
    * A flow step describing a promise definition.
    *
    * The resolved/rejected value is written to a pseudo-field on the promise.
    */
-  class PromiseDefitionStep extends DataFlow::AdditionalFlowStep {
+  class PromiseDefitionStep extends PromiseFlowStep {
     PromiseDefinition promise;
-    PromiseDefitionStep() {
-      this = promise
-    }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+    PromiseDefitionStep() { this = promise }
+
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = valueProp() and
       pred = promise.getResolveParameter().getACall().getArgument(0) and
       succ = this
       or
-      prop = rejectField() and
+      prop = errorProp() and
       (
         pred = promise.getRejectParameter().getACall().getArgument(0) or
         pred = promise.getExecutor().getExceptionalReturn()
@@ -163,56 +280,55 @@ private module PromiseFlow {
       succ = this
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       // Copy the value of a resolved promise to the value of this promise.
-      prop = resolveField() and
+      prop = valueProp() and
       pred = promise.getResolveParameter().getACall().getArgument(0) and
       succ = this
     }
   }
-  
+
   /**
    * A flow step describing the a Promise.resolve (and similar) call.
    */
-  class CreationStep extends DataFlow::AdditionalFlowStep {
+  class CreationStep extends PromiseFlowStep {
     PromiseCreationCall promise;
-    CreationStep() {
-      this = promise
-    }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+    CreationStep() { this = promise }
+
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = valueProp() and
       pred = promise.getValue() and
       succ = this
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       // Copy the value of a resolved promise to the value of this promise.
-      prop = resolveField() and
+      prop = valueProp() and
       pred = promise.getValue() and
       succ = this
     }
   }
 
-
   /**
    * A load step loading the pseudo-field describing that the promise is rejected.
    * The rejected value is thrown as a exception.
    */
-  class AwaitStep extends DataFlow::AdditionalFlowStep {
+  class AwaitStep extends PromiseFlowStep {
     DataFlow::Node operand;
     AwaitExpr await;
+
     AwaitStep() {
       this.getEnclosingExpr() = await and
       operand.getEnclosingExpr() = await.getOperand()
     }
 
-    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = valueProp() and
       succ = this and
       pred = operand
       or
-      prop = rejectField() and
+      prop = errorProp() and
       succ = await.getExceptionTarget() and
       pred = operand
     }
@@ -221,40 +337,38 @@ private module PromiseFlow {
   /**
    * A flow step describing the data-flow related to the `.then` method of a promise.
    */
-  class ThenStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    ThenStep() {
-      this.getMethodName() = "then"
-    }
+  class ThenStep extends PromiseFlowStep, DataFlow::MethodCallNode {
+    ThenStep() { this.getMethodName() = "then" }
 
-    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = valueProp() and
       pred = getReceiver() and
       succ = getCallback(0).getParameter(0)
       or
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getReceiver() and
       succ = getCallback(1).getParameter(0)
     }
-    
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
       not exists(this.getArgument(1)) and
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getReceiver() and
       succ = this
       or
       // read the value of a resolved/rejected promise that is returned
-      (prop = rejectField() or prop = resolveField()) and
-      pred = getCallback([0..1]).getAReturn() and
+      (prop = errorProp() or prop = valueProp()) and
+      pred = getCallback([0 .. 1]).getAReturn() and
       succ = this
     }
-    
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
-      pred = getCallback([0..1]).getAReturn() and
+
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = valueProp() and
+      pred = getCallback([0 .. 1]).getAReturn() and
       succ = this
       or
-      prop = rejectField() and
-      pred = getCallback([0..1]).getExceptionalReturn() and
+      prop = errorProp() and
+      pred = getCallback([0 .. 1]).getExceptionalReturn() and
       succ = this
     }
   }
@@ -262,34 +376,32 @@ private module PromiseFlow {
   /**
    * A flow step describing the data-flow related to the `.catch` method of a promise.
    */
-  class CatchStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    CatchStep() {
-      this.getMethodName() = "catch"
-    }
+  class CatchStep extends PromiseFlowStep, DataFlow::MethodCallNode {
+    CatchStep() { this.getMethodName() = "catch" }
 
-    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = rejectField() and
+    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = errorProp() and
       pred = getReceiver() and
       succ = getCallback(0).getParameter(0)
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = resolveField() and
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = valueProp() and
       pred = getReceiver().getALocalSource() and
       succ = this
       or
       // read the value of a resolved/rejected promise that is returned
-      (prop = rejectField() or prop = resolveField()) and
+      (prop = errorProp() or prop = valueProp()) and
       pred = getCallback(0).getAReturn() and
       succ = this
     }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = rejectField() and
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = errorProp() and
       pred = getCallback(0).getExceptionalReturn() and
       succ = this
       or
-      prop = resolveField() and
+      prop = valueProp() and
       pred = getCallback(0).getAReturn() and
       succ = this
     }
@@ -298,24 +410,22 @@ private module PromiseFlow {
   /**
    * A flow step describing the data-flow related to the `.finally` method of a promise.
    */
-  class FinallyStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    FinallyStep() {
-      this.getMethodName() = "finally"
-    }
+  class FinallyStep extends PromiseFlowStep, DataFlow::MethodCallNode {
+    FinallyStep() { this.getMethodName() = "finally" }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      (prop = resolveField() or prop = rejectField()) and
+    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      (prop = valueProp() or prop = errorProp()) and
       pred = getReceiver() and
       succ = this
       or
       // read the value of a rejected promise that is returned
-      prop = rejectField() and
+      prop = errorProp() and
       pred = getCallback(0).getAReturn() and
       succ = this
     }
 
-    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = rejectField() and
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = errorProp() and
       pred = getCallback(0).getExceptionalReturn() and
       succ = this
     }
@@ -332,15 +442,13 @@ predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
   // from `x` to `Promise.resolve(x)`
   pred = succ.(PromiseCreationCall).getValue()
   or
-  exists(DataFlow::MethodCallNode thn |
-    thn.getMethodName() = "then"
-  |
+  exists(DataFlow::MethodCallNode thn | thn.getMethodName() = "then" |
     // from `p` to `x` in `p.then(x => ...)`
     pred = thn.getReceiver() and
     succ = thn.getCallback(0).getParameter(0)
     or
     // from `v` to `p.then(x => return v)`
-    pred = thn.getCallback([0..1]).getAReturn() and
+    pred = thn.getCallback([0 .. 1]).getAReturn() and
     succ = thn
   )
   or
@@ -406,22 +514,19 @@ module Bluebird {
 
     override DataFlow::Node getValue() { result = getArgument(0) }
   }
-  
+
   /**
-   * An aggregated promise produced either by `Promise.all`, `Promise.race` or `Promise.map`. 
+   * An aggregated promise produced either by `Promise.all`, `Promise.race` or `Promise.map`.
    */
   class AggregateBluebirdPromiseDefinition extends PromiseCreationCall {
     AggregateBluebirdPromiseDefinition() {
-      exists(string m | m = "all" or m = "race" or m = "map" | 
-        this = bluebird().getAMemberCall(m)
-      )
+      exists(string m | m = "all" or m = "race" or m = "map" | this = bluebird().getAMemberCall(m))
     }
 
     override DataFlow::Node getValue() {
       result = getArgument(0).getALocalSource().(DataFlow::ArrayCreationNode).getAnElement()
     }
   }
-  
 }
 
 /**