Merge remote-tracking branch 'upstream/master' into SimpleRangeAnalysis-use-after-cast

change-notes/1.21/analysis-cpp.md

@@ -12,6 +12,11 @@
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | Fewer false positive results | Fixed an issue where functions were being identified as allocation functions inappropriately.  Also affects `cpp/new-array-delete-mismatch` and `cpp/new-delete-array-mismatch`. |
+| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | More correct results | This query has been reworked so that it can find a wider variety of results. |
+| Memory may not be freed (`cpp/memory-may-not-be-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
+| Memory is never freed (`cpp/memory-never-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
 | Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Resource allocation and deallocation functions are now determined more accurately. |
+| Comparison result is always the same | Fewer false positive results | The range analysis library is now more conservative about floating point values being possibly `NaN` |
+| Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | More correct results and fewer false positive results | This query now more accurately identifies wide and non-wide string/character format arguments on different platforms.  Platform detection has also been made more accurate for the purposes of this query. |
 
 ## Changes to QL libraries

change-notes/1.21/analysis-csharp.md

@@ -0,0 +1,17 @@
+# Improvements to C# analysis
+
+## Changes to existing queries
+
+| **Query**                    | **Expected impact**    | **Change**                        |
+|------------------------------|------------------------|-----------------------------------|
+
+
+## Changes to code extraction
+
+* Named attribute arguments are now extracted.
+
+## Changes to QL libraries
+
+* The class `Attribute` has two new predicates: `getConstructorArgument()` and `getNamedArgument()`. The first predicate returns arguments to the underlying constructor call and the latter returns named arguments for initializing fields and properties.
+
+## Changes to autobuilder

change-notes/1.21/analysis-javascript.md

@@ -18,7 +18,9 @@
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Expression has no effect       | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
-| Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. |
+| Useless assignment to property | Fewer false-positive results | This rule now ignores reads of additional getters. |
 | Arbitrary file write during zip extraction ("Zip Slip") | More results | This rule now considers more libraries, including tar as well as zip. |
+| Client-side URL redirect       | Fewer false-positive results | This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |
+| Server-side URL redirect       | Fewer false-positive results | This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |
 
 ## Changes to QL libraries

cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql

@@ -12,6 +12,7 @@
  *       readability
  */
 import cpp
+private import semmle.code.cpp.commons.Exclusions
 private import semmle.code.cpp.rangeanalysis.PointlessComparison
 private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
 import UnsignedGEZero
@@ -31,6 +32,7 @@ from
 where
   not cmp.isInMacroExpansion() and
   not cmp.isFromTemplateInstantiation(_) and
+  not functionContainsDisabledCode(cmp.getEnclosingFunction()) and
   reachablePointlessComparison(cmp, left, right, value, ss) and
 
   // a comparison between an enum and zero is always valid because whether

cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql

@@ -11,6 +11,7 @@
  *       external/cwe/cwe-561
  */
 import cpp
+private import semmle.code.cpp.commons.Exclusions
 
 class PureExprInVoidContext extends ExprInVoidContext {
   PureExprInVoidContext() { this.isPure() }
@@ -23,71 +24,29 @@ predicate accessInInitOfForStmt(Expr e) {
                                  s.getExpr() = e)
 }
 
-/**
- * Holds if the preprocessor branch `pbd` is on line `pbdStartLine` in file `file`.
- */
-predicate pbdLocation(PreprocessorBranchDirective pbd, string file, int pbdStartLine) {
-  pbd.getLocation().hasLocationInfo(file, pbdStartLine, _, _, _)
-}
-
-/**
- * Holds if the body of the function `f` is on lines `fBlockStartLine` to `fBlockEndLine` in file `file`.
- */
-predicate functionLocation(Function f, string file, int fBlockStartLine, int fBlockEndLine) {
-  f.getBlock().getLocation().hasLocationInfo(file, fBlockStartLine, _, fBlockEndLine, _)
-}
 /**
  * Holds if the function `f`, or a function called by it, contains
  * code excluded by the preprocessor.
  */
-predicate containsDisabledCode(Function f) {
+predicate functionContainsDisabledCodeRecursive(Function f) {
-  // `f` contains a preprocessor branch that was not taken
+  functionContainsDisabledCode(f) or
-  exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine  |
-    functionLocation(f, file, fBlockStartLine, fBlockEndLine) and
-    pbdLocation(pbd, file, pbdStartLine) and
-    pbdStartLine <= fBlockEndLine and
-    pbdStartLine >= fBlockStartLine and
-    (
-      pbd.(PreprocessorBranch).wasNotTaken() or
-
-      // an else either was not taken, or it's corresponding branch
-      // was not taken.
-      pbd instanceof PreprocessorElse
-    )
-  ) or
-
   // recurse into function calls
   exists(FunctionCall fc |
     fc.getEnclosingFunction() = f and
-    containsDisabledCode(fc.getTarget())
+    functionContainsDisabledCodeRecursive(fc.getTarget())
   )
 }
 
-
 /**
  * Holds if the function `f`, or a function called by it, is inside a
  * preprocessor branch that may have code in another arm
  */
-predicate definedInIfDef(Function f) {
+predicate functionDefinedInIfDefRecursive(Function f) {
-  exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int pbdEndLine, int fBlockStartLine, int fBlockEndLine  |
+  functionDefinedInIfDef(f) or
-    functionLocation(f, file, fBlockStartLine, fBlockEndLine) and
-    pbdLocation(pbd, file, pbdStartLine) and
-    pbdLocation(pbd.getNext(), file, pbdEndLine) and
-    pbdStartLine <= fBlockStartLine and
-    pbdEndLine >= fBlockEndLine and
-    // pbd is a preprocessor branch where multiple branches exist
-    (
-      pbd.getNext() instanceof PreprocessorElse or
-      pbd instanceof PreprocessorElse or
-      pbd.getNext() instanceof PreprocessorElif or
-      pbd instanceof PreprocessorElif
-    )
-  ) or
-
   // recurse into function calls
   exists(FunctionCall fc |
     fc.getEnclosingFunction() = f and
-    definedInIfDef(fc.getTarget())
+    functionDefinedInIfDefRecursive(fc.getTarget())
   )
 }
 
@@ -121,8 +80,8 @@ where // EQExprs are covered by CompareWhereAssignMeant.ql
       not parent instanceof PureExprInVoidContext and
       not peivc.getEnclosingFunction().isCompilerGenerated() and
       not peivc.getType() instanceof UnknownType and
-      not containsDisabledCode(peivc.(FunctionCall).getTarget()) and
+      not functionContainsDisabledCodeRecursive(peivc.(FunctionCall).getTarget()) and
-      not definedInIfDef(peivc.(FunctionCall).getTarget()) and
+      not functionDefinedInIfDefRecursive(peivc.(FunctionCall).getTarget()) and
       if peivc instanceof FunctionCall then
         exists(Function target |
           target = peivc.(FunctionCall).getTarget() and

cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql

@@ -14,15 +14,20 @@
 import cpp
 import semmle.code.cpp.security.TaintTracking
 
-from Expr source, Expr tainted, BinaryArithmeticOperation oper,
+predicate taintedAllocSize(Expr e, Expr source, string taintCause) {
-     SizeofOperator sizeof, string taintCause
+  (
-where tainted(source, tainted)
+    isAllocationExpr(e) or
-  and oper.getAnOperand() = tainted
+    any(MulExpr me | me.getAChild() instanceof SizeofOperator) = e
-  and oper.getOperator() = "*"
+  ) and
-  and oper.getAnOperand() = sizeof
+  exists(Expr tainted |
-  and oper != tainted
+    tainted = e.getAChild() and
-  and sizeof.getValue().toInt() > 1
+    tainted.getType().getUnspecifiedType() instanceof IntegralType and
-  and isUserInput(source, taintCause)
+    isUserInput(source, taintCause) and
-select
+    tainted(source, tainted)
-  oper, "This allocation size is derived from $@ and might overflow",
+  )
-  source, "user input (" + taintCause + ")"
+}
+
+from Expr e, Expr source, string taintCause
+where taintedAllocSize(e, source, taintCause)
+select e, "This allocation size is derived from $@ and might overflow", source,
+  "user input (" + taintCause + ")"

cpp/ql/src/jsf/4.07 Header Files/AV Rule 35.qhelp

@@ -68,6 +68,9 @@ some are after the final <code>#endif</code>. All three of these things must be
 <li>
   <a href="http://www.cplusplus.com/forum/articles/10627/">Headers and Includes: Why and How</a>
 </li>
+<li>
+  <a href="https://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html">The Multiple-Include Optimization</a>
+</li>
 
 
 </references>

cpp/ql/src/semmle/code/cpp/File.qll

@@ -302,7 +302,13 @@ class File extends Container, @file {
   predicate compiledAsMicrosoft() {
     exists(Compilation c |
       c.getAFileCompiled() = this and
-      c.getAnArgument() = "--microsoft"
+      (
+        c.getAnArgument() = "--microsoft" or
+        c.getAnArgument().toLowerCase().replaceAll("\\", "/").matches("%/cl.exe")
+      )
+    ) or exists(File parent |
+      parent.compiledAsMicrosoft() and
+      parent.getAnIncludedFile() = this
     )
   }
 

cpp/ql/src/semmle/code/cpp/commons/Alloc.qll

@@ -39,7 +39,16 @@ predicate allocationFunction(Function f)
       name = "MmAllocateNodePagesForMdlEx" or
       name = "MmMapLockedPagesWithReservedMapping" or
       name = "MmMapLockedPages" or
-      name = "MmMapLockedPagesSpecifyCache"
+      name = "MmMapLockedPagesSpecifyCache" or
+      name = "LocalAlloc" or
+      name = "LocalReAlloc" or
+      name = "GlobalAlloc" or
+      name = "GlobalReAlloc" or
+      name = "HeapAlloc" or
+      name = "HeapReAlloc" or
+      name = "VirtualAlloc" or
+      name = "CoTaskMemAlloc" or
+      name = "CoTaskMemRealloc"
     )
   )
 }
@@ -81,7 +90,17 @@ predicate freeFunction(Function f, int argNum)
       (name = "MmFreeMappingAddress" and argNum = 0) or
       (name = "MmFreePagesFromMdl" and argNum = 0) or
       (name = "MmUnmapReservedMapping" and argNum = 0) or
-      (name = "MmUnmapLockedPages" and argNum = 0)
+      (name = "MmUnmapLockedPages" and argNum = 0) or
+      (name = "LocalFree" and argNum = 0) or
+      (name = "GlobalFree" and argNum = 0) or
+      (name = "HeapFree" and argNum = 2) or
+      (name = "VirtualFree" and argNum = 0) or
+      (name = "CoTaskMemFree" and argNum = 0) or
+      (name = "SysFreeString" and argNum = 0) or
+      (name = "LocalReAlloc" and argNum = 0) or
+      (name = "GlobalReAlloc" and argNum = 0) or
+      (name = "HeapReAlloc" and argNum = 2) or
+      (name = "CoTaskMemRealloc" and argNum = 0)
     )
   )
 }

cpp/ql/src/semmle/code/cpp/commons/Exclusions.qll

@@ -0,0 +1,60 @@
+/**
+ * Common predicates used to exclude results from a query based on heuristics.
+ */
+
+import cpp
+
+/**
+ * Holds if the preprocessor branch `pbd` is on line `pbdStartLine` in file `file`.
+ */
+private predicate pbdLocation(PreprocessorBranchDirective pbd, string file, int pbdStartLine) {
+  pbd.getLocation().hasLocationInfo(file, pbdStartLine, _, _, _)
+}
+
+/**
+ * Holds if the body of the function `f` is on lines `fBlockStartLine` to `fBlockEndLine` in file `file`.
+ */
+private predicate functionLocation(Function f, string file, int fBlockStartLine, int fBlockEndLine) {
+  f.getBlock().getLocation().hasLocationInfo(file, fBlockStartLine, _, fBlockEndLine, _)
+}
+
+/**
+ * Holds if the function `f` is inside a preprocessor branch that may have code in another arm.
+ */
+predicate functionDefinedInIfDef(Function f) {
+  exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int pbdEndLine, int fBlockStartLine,
+      int fBlockEndLine  |
+    functionLocation(f, file, fBlockStartLine, fBlockEndLine) and
+    pbdLocation(pbd, file, pbdStartLine) and
+    pbdLocation(pbd.getNext(), file, pbdEndLine) and
+    pbdStartLine <= fBlockStartLine and
+    pbdEndLine >= fBlockEndLine and
+    // pbd is a preprocessor branch where multiple branches exist
+    (
+      pbd.getNext() instanceof PreprocessorElse or
+      pbd instanceof PreprocessorElse or
+      pbd.getNext() instanceof PreprocessorElif or
+      pbd instanceof PreprocessorElif
+    )
+  )
+}
+
+/**
+ * Holds if the function `f` contains code excluded by the preprocessor.
+ */
+predicate functionContainsDisabledCode(Function f) {
+  // `f` contains a preprocessor branch that was not taken
+  exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine |
+    functionLocation(f, file, fBlockStartLine, fBlockEndLine) and
+    pbdLocation(pbd, file, pbdStartLine) and
+    pbdStartLine <= fBlockEndLine and
+    pbdStartLine >= fBlockStartLine and
+    (
+      pbd.(PreprocessorBranch).wasNotTaken() or
+
+      // an else either was not taken, or it's corresponding branch
+      // was not taken.
+      pbd instanceof PreprocessorElse
+    )
+  )
+}

cpp/ql/src/semmle/code/cpp/commons/Printf.qll

@@ -32,19 +32,19 @@ class AttributeFormattingFunction extends FormattingFunction {
  * A standard function such as `vprintf` that has a format parameter
  * and a variable argument list of type `va_arg`.
  */
-predicate primitiveVariadicFormatter(TopLevelFunction f, int formatParamIndex, boolean wide) {
+predicate primitiveVariadicFormatter(TopLevelFunction f, int formatParamIndex) {
   f.getName().regexpMatch("_?_?va?[fs]?n?w?printf(_s)?(_p)?(_l)?")
   and (
     if f.getName().matches("%\\_l")
     then formatParamIndex = f.getNumberOfParameters() - 3
     else formatParamIndex = f.getNumberOfParameters() - 2
-  ) and if f.getName().matches("%w%") then wide = true else wide = false
+  )
 }
 
 private
-predicate callsVariadicFormatter(Function f, int formatParamIndex, boolean wide) {
+predicate callsVariadicFormatter(Function f, int formatParamIndex) {
   exists(FunctionCall fc, int i |
-    variadicFormatter(fc.getTarget(), i, wide)
+    variadicFormatter(fc.getTarget(), i)
     and fc.getEnclosingFunction() = f
     and fc.getArgument(i) = f.getParameter(formatParamIndex).getAnAccess()
   )
@@ -54,11 +54,11 @@ predicate callsVariadicFormatter(Function f, int formatParamIndex, boolean wide)
  * Holds if `f` is a function such as `vprintf` that has a format parameter
  * (at `formatParamIndex`) and a variable argument list of type `va_arg`.  
  */
-predicate variadicFormatter(Function f, int formatParamIndex, boolean wide) {
+predicate variadicFormatter(Function f, int formatParamIndex) {
-  primitiveVariadicFormatter(f, formatParamIndex, wide)
+  primitiveVariadicFormatter(f, formatParamIndex)
   or (
     not f.isVarargs()
-    and callsVariadicFormatter(f, formatParamIndex, wide)
+    and callsVariadicFormatter(f, formatParamIndex)
   )
 }
 
@@ -68,12 +68,10 @@ predicate variadicFormatter(Function f, int formatParamIndex, boolean wide) {
  */
 class UserDefinedFormattingFunction extends FormattingFunction {
   UserDefinedFormattingFunction() {
-    isVarargs() and callsVariadicFormatter(this, _, _)
+    isVarargs() and callsVariadicFormatter(this, _)
   }
 
-  override int getFormatParameterIndex() { callsVariadicFormatter(this, result, _) }
+  override int getFormatParameterIndex() { callsVariadicFormatter(this, result) }
-
-  override predicate isWideCharDefault() { callsVariadicFormatter(this, _, true) }
 }
 
 /**
@@ -674,8 +672,8 @@ class FormatLiteral extends Literal {
   /**
    * Gets the char type required by the nth conversion specifier.
    *  - in the base case this is the default for the formatting function
-   *    (e.g. `char` for `printf`, `wchar_t` for `wprintf`).
+   *    (e.g. `char` for `printf`, `char` or `wchar_t` for `wprintf`).
-   *  - the `%S` format character reverses wideness.
+   *  - the `%C` format character reverses wideness.
    *  - the size prefixes 'l'/'w' and 'h' override the type character
    *    to wide or single-byte characters respectively.
    */
@@ -721,8 +719,8 @@ class FormatLiteral extends Literal {
   /**
    * Gets the string type required by the nth conversion specifier.
    *  - in the base case this is the default for the formatting function
-   *    (e.g. `char` for `printf`, `wchar_t` for `wprintf`).
+   *    (e.g. `char *` for `printf`, `char *` or `wchar_t *` for `wprintf`).
-   *  - the `%S` format character reverses wideness.
+   *  - the `%S` format character reverses wideness on some platforms.
    *  - the size prefixes 'l'/'w' and 'h' override the type character
    *    to wide or single-byte characters respectively.
    */

cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll

@@ -22,7 +22,7 @@ private Type stripTopLevelSpecifiersOnly(Type t) {
  */
 Type getAFormatterWideType() {
   exists(FormattingFunction ff |
-    result = stripTopLevelSpecifiersOnly(ff.getDefaultCharType()) and
+    result = stripTopLevelSpecifiersOnly(ff.getFormatCharType()) and
     result.getSize() != 1
   )
 }
@@ -46,6 +46,14 @@ abstract class FormattingFunction extends Function {
   /** Gets the position at which the format parameter occurs. */
   abstract int getFormatParameterIndex();
 
+  /**
+   * Holds if this `FormattingFunction` is in a context that supports
+   * Microsoft rules and extensions.
+   */
+  predicate isMicrosoft() {
+    getFile().compiledAsMicrosoft()
+  }
+
   /**
    * Holds if the default meaning of `%s` is a `wchar_t *`, rather than
    * a `char *` (either way, `%S` will have the opposite meaning).
@@ -55,11 +63,10 @@ abstract class FormattingFunction extends Function {
   deprecated predicate isWideCharDefault() { none() }
 
   /**
-   * Gets the default character type expected for `%s` by this function.  Typically
+   * Gets the character type used in the format string for this function.
-   * `char` or `wchar_t`.
    */
-  Type getDefaultCharType() {
+  Type getFormatCharType() {
-    result = 
+    result =
       stripTopLevelSpecifiersOnly(
         stripTopLevelSpecifiersOnly(
           getParameter(getFormatParameterIndex()).getType().getUnderlyingType()
@@ -67,19 +74,33 @@ abstract class FormattingFunction extends Function {
       )
   }
 
+  /**
+   * Gets the default character type expected for `%s` by this function.  Typically
+   * `char` or `wchar_t`.
+   */
+  Type getDefaultCharType() {
+    (
+      isMicrosoft() and
+      result = getFormatCharType()
+    ) or (
+      not isMicrosoft() and
+      result instanceof PlainCharType
+    )
+  }
+
   /**
    * Gets the non-default character type expected for `%S` by this function.  Typically
    * `wchar_t` or `char`.  On some snapshots there may be multiple results where we can't tell
    * which is correct for a particular function.
    */
   Type getNonDefaultCharType() {
-  	(
+    (
-  	  getDefaultCharType().getSize() = 1 and
+      getDefaultCharType().getSize() = 1 and
-  	  result = getAFormatterWideTypeOrDefault()
+      result = getWideCharType()
-  	) or (
+    ) or (
-  	  getDefaultCharType().getSize() > 1 and
+      not getDefaultCharType().getSize() = 1 and
-  	  result instanceof PlainCharType
+      result instanceof PlainCharType
-  	)
+    )
   }
 
   /**
@@ -89,10 +110,12 @@ abstract class FormattingFunction extends Function {
    */
   Type getWideCharType() {
     (
-      result = getDefaultCharType() or
+      result = getFormatCharType() and
-      result = getNonDefaultCharType()
+      result.getSize() > 1
-    ) and
+    ) or (
-    result.getSize() > 1
+      not getFormatCharType().getSize() > 1 and
+      result = getAFormatterWideTypeOrDefault() // may have more than one result
+    )
   }
 
   /**

cpp/ql/src/semmle/code/cpp/rangeanalysis/NanAnalysis.qll

@@ -0,0 +1,41 @@
+import cpp
+private import semmle.code.cpp.rangeanalysis.RangeSSA
+
+/**
+ * Holds if `guard` won't return the value `polarity` when either
+ * operand is NaN.
+ */
+predicate nanExcludingComparison(ComparisonOperation guard, boolean polarity) {
+  polarity = true and
+  (
+    guard instanceof LTExpr or
+    guard instanceof LEExpr or
+    guard instanceof GTExpr or
+    guard instanceof GEExpr or
+    guard instanceof EQExpr
+  )
+  or
+  polarity = false and
+  guard instanceof NEExpr
+}
+
+/**
+ * Holds if `v` is a use of an SSA definition in `def` which cannot be NaN,
+ * by virtue of the guard in `def`.
+ */
+private predicate excludesNan(RangeSsaDefinition def, VariableAccess v) {
+  exists(VariableAccess inCond, ComparisonOperation guard, boolean branch, LocalScopeVariable lsv |
+    def.isGuardPhi(inCond, guard, branch) and
+    inCond.getTarget() = lsv and
+    v = def.getAUse(lsv) and
+    guard.getAnOperand() = inCond and
+    nanExcludingComparison(guard, branch)
+  )
+}
+
+/**
+ * A variable access which cannot be NaN.
+ */
+class NonNanVariableAccess extends VariableAccess {
+  NonNanVariableAccess() { excludesNan(_, this) }
+}

cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

@@ -45,6 +45,7 @@ import cpp
 private import RangeAnalysisUtils
 import RangeSSA
 import SimpleRangeAnalysisCached
+private import NanAnalysis
 
 /**
  * This fixed set of lower bounds is used when the lower bounds of an
@@ -993,6 +994,25 @@ predicate unanalyzableDefBounds(
   ub = varMaxVal(v)
 }
 
+/**
+ * Holds if in the `branch` branch of a guard `guard` involving `v`,
+ * we know that `v` is not NaN, and therefore it is safe to make range
+ * inferences about `v`.
+ */
+bindingset[guard, v, branch]
+predicate nonNanGuardedVariable(ComparisonOperation guard, VariableAccess v, boolean branch) {
+  v.getType().getUnspecifiedType() instanceof IntegralType
+  or
+  v.getType().getUnspecifiedType() instanceof FloatingPointType and v instanceof NonNanVariableAccess
+  or
+  // The reason the following case is here is to ensure that when we say
+  // `if (x > 5) { ...then... } else { ...else... }`
+  // it is ok to conclude that `x > 5` in the `then`, (though not safe
+  // to conclude that x <= 5 in `else`) even if we had no prior
+  // knowledge of `x` not being `NaN`.
+  nanExcludingComparison(guard, branch)
+}
+
 /**
  * If the guard is a comparison of the form `p*v + q <CMP> r`, then this
  * predicate uses the bounds information for `r` to compute a lower bound
@@ -1004,10 +1024,12 @@ predicate lowerBoundFromGuard(
 ) {
   exists (float childLB, RelationStrictness strictness
   | boundFromGuard(guard, v, childLB, true, strictness, branch)
-  | if (strictness = Nonstrict() or
+  | if nonNanGuardedVariable(guard, v, branch)
-        not (v.getType().getUnspecifiedType() instanceof IntegralType))
+    then (if (strictness = Nonstrict() or
-      then lb = childLB
+            not (v.getType().getUnspecifiedType() instanceof IntegralType))
-      else lb = childLB+1)
+          then lb = childLB
+          else lb = childLB+1)
+    else lb = varMinVal(v.getTarget()))
 }
 
 /**
@@ -1021,10 +1043,12 @@ predicate upperBoundFromGuard(
 ) {
   exists (float childUB, RelationStrictness strictness
   | boundFromGuard(guard, v, childUB, false, strictness, branch)
-  | if (strictness = Nonstrict() or
+  | if nonNanGuardedVariable(guard, v, branch)
-        not (v.getType().getUnspecifiedType() instanceof IntegralType))
+    then (if (strictness = Nonstrict() or
-      then ub = childUB
+            not (v.getType().getUnspecifiedType() instanceof IntegralType))
-      else ub = childUB-1)
+          then ub = childUB
+          else ub = childUB-1)
+    else ub = varMaxVal(v.getTarget()))
 }
 
 /**

cpp/ql/src/semmle/code/cpp/security/PrintfLike.qll

@@ -4,7 +4,7 @@ import external.ExternalArtifact
 predicate printfLikeFunction(Function func, int formatArg) {
   (formatArg = func.(FormattingFunction).getFormatParameterIndex() and not func instanceof UserDefinedFormattingFunction)
   or
-  primitiveVariadicFormatter(func, formatArg, _)
+  primitiveVariadicFormatter(func, formatArg)
   or
   exists(ExternalData data |
     // TODO Do this \ to / conversion in the toolchain?

cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll

@@ -245,9 +245,14 @@ predicate insideFunctionValueMoveTo(Element src, Element dest)
       and format.getConversionChar(arg - formattingSend.getTarget().getNumberOfParameters()) = argFormat
       and (argFormat = "s" or argFormat = "S" or argFormat = "@"))
     // Expressions computed from tainted data are also tainted
-    or (exists (FunctionCall call | dest = call and isPureFunction(call.getTarget().getName()) |
+    or exists(FunctionCall call | dest = call and isPureFunction(call.getTarget().getName()) |
-      call.getAnArgument() = src
+      call.getAnArgument() = src and
-      and forall(Expr arg | arg = call.getAnArgument() | arg = src or predictable(arg))))
+      forall(Expr arg | arg = call.getAnArgument() | arg = src or predictable(arg)) and
+
+      // flow through `strlen` tends to cause dubious results, if the length is
+      // bounded.
+      not call.getTarget().getName() = "strlen"
+    )
     or exists(Element a, Element b |
       moveToDependingOnSide(a, b) and
       if insideValueSource(a) then

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.c

@@ -315,3 +315,30 @@ int signedness_cast2(signed char c) {
   }
   return 0;
 }
+
+int nan1(double x) {
+  if (x < 0.0) {
+    return 100;
+  }
+  else if (x >= 0.0) { // GOOD [x could be NaN]
+    return 200;
+  }
+  else {
+    return 300;
+  }
+}
+
+int nan2(double x) {
+  if (x == x) {
+    // If x compares with anything at all, it's not NaN
+    if (x < 0.0) {
+      return 100;
+    }
+    else if (x >= 0.0) { // BAD [Always true]
+      return 200;
+    }
+    else {
+      return 300;
+    }
+  }
+}

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected

@@ -36,5 +36,6 @@
 | PointlessComparison.c:273:9:273:18 | ... > ... | Comparison is always false because c <= 0. |
 | PointlessComparison.c:283:13:283:19 | ... >= ... | Comparison is always true because c >= 11. |
 | PointlessComparison.c:294:9:294:16 | ... >= ... | Comparison is always false because ui1 <= 0. |
+| PointlessComparison.c:337:14:337:21 | ... >= ... | Comparison is always true because x >= 0. |
 | RegressionTests.cpp:57:7:57:22 | ... <= ... | Comparison is always true because * ... <= 4294967295. |
 | Templates.cpp:9:10:9:24 | ... <= ... | Comparison is always true because local <= 32767. |

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/RegressionTests.cpp

@@ -66,3 +66,17 @@ int regression_test_01(unsigned long bb) {
     return 1;
   }
 }
+
+int containsIfDef(int x) {
+  int result = 0;
+  if (x > 0) {
+    result = 1;
+  }
+#if _CONDITION
+  if (x < 0) {
+    result = -1;
+  }
+#endif
+
+  return result >= 0;
+}

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/WrongTypeFormatArguments.expected

@@ -1,10 +1,14 @@
 | tests.cpp:18:15:18:22 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
 | tests.cpp:19:15:19:22 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
-| tests.cpp:25:17:25:23 | Hello | This argument should be of type 'wchar_t *' but is of type 'char *' |
+| tests.cpp:26:17:26:24 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
-| tests.cpp:26:17:26:24 | Hello | This argument should be of type 'wchar_t *' but is of type 'char16_t *' |
+| tests.cpp:27:17:27:24 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
-| tests.cpp:30:17:30:24 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
+| tests.cpp:29:17:29:23 | Hello | This argument should be of type 'wchar_t *' but is of type 'char *' |
-| tests.cpp:31:17:31:24 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
+| tests.cpp:30:17:30:24 | Hello | This argument should be of type 'wchar_t *' but is of type 'char16_t *' |
-| tests.cpp:33:36:33:42 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
+| tests.cpp:34:36:34:43 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
-| tests.cpp:35:36:35:43 | Hello | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |
+| tests.cpp:35:36:35:43 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
-| tests.cpp:38:36:38:43 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
+| tests.cpp:37:36:37:42 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
-| tests.cpp:39:36:39:43 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
+| tests.cpp:39:36:39:43 | Hello | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |
+| tests.cpp:42:37:42:44 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
+| tests.cpp:43:37:43:44 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
+| tests.cpp:45:37:45:43 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
+| tests.cpp:47:37:47:44 | Hello | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/formattingFunction.expected

@@ -1,3 +1,3 @@
-| tests.cpp:8:5:8:10 | printf | char | char16_t, wchar_t | char16_t, wchar_t |
+| tests.cpp:8:5:8:10 | printf | char | char | char16_t, wchar_t | char16_t, wchar_t |
-| tests.cpp:9:5:9:11 | wprintf | wchar_t | char | wchar_t |
+| tests.cpp:9:5:9:11 | wprintf | wchar_t | char | wchar_t | wchar_t |
-| tests.cpp:10:5:10:12 | swprintf | char16_t | char | char16_t |
+| tests.cpp:10:5:10:12 | swprintf | char16_t | char | char16_t | char16_t |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/formattingFunction.ql

@@ -3,6 +3,7 @@ import cpp
 from FormattingFunction f
 select
   f,
+  concat(f.getFormatCharType().toString(), ", "),
   concat(f.getDefaultCharType().toString(), ", "),
   concat(f.getNonDefaultCharType().toString(), ", "),
   concat(f.getWideCharType().toString(), ", ")

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/tests.cpp

@@ -22,19 +22,27 @@ void tests() {
 	printf("%S", u"Hello"); // GOOD
 	printf("%S", L"Hello"); // GOOD
 
-	wprintf(L"%s", "Hello"); // BAD: expecting wchar_t
+	wprintf(L"%s", "Hello"); // GOOD
-	wprintf(L"%s", u"Hello"); // BAD: expecting wchar_t
+	wprintf(L"%s", u"Hello"); // BAD: expecting char
-	wprintf(L"%s", L"Hello"); // GOOD
+	wprintf(L"%s", L"Hello"); // BAD: expecting char
 
-	wprintf(L"%S", "Hello"); // GOOD
+	wprintf(L"%S", "Hello"); // BAD: expecting wchar_t
-	wprintf(L"%S", u"Hello"); // BAD: expecting char
+	wprintf(L"%S", u"Hello"); // BAD: expecting wchar_t
-	wprintf(L"%S", L"Hello"); // BAD: expecting char
+	wprintf(L"%S", L"Hello"); // GOOD
 
-	swprintf(buffer, BUF_SIZE, u"%s", "Hello"); // BAD: expecting char16_t
+	swprintf(buffer, BUF_SIZE, u"%s", "Hello"); // GOOD
-	swprintf(buffer, BUF_SIZE, u"%s", u"Hello"); // GOOD
+	swprintf(buffer, BUF_SIZE, u"%s", u"Hello"); // BAD: expecting char
-	swprintf(buffer, BUF_SIZE, u"%s", L"Hello"); // BAD: expecting char16_t
+	swprintf(buffer, BUF_SIZE, u"%s", L"Hello"); // BAD: expecting char
 
-	swprintf(buffer, BUF_SIZE, u"%S", "Hello"); // GOOD
+	swprintf(buffer, BUF_SIZE, u"%S", "Hello"); // BAD: expecting char16_t
-	swprintf(buffer, BUF_SIZE, u"%S", u"Hello"); // BAD: expecting char
+	swprintf(buffer, BUF_SIZE, u"%S", u"Hello"); // GOOD
-	swprintf(buffer, BUF_SIZE, u"%S", L"Hello"); // BAD: expecting char
+	swprintf(buffer, BUF_SIZE, u"%S", L"Hello"); // BAD: expecting char16_t
+
+	swprintf(buffer, BUF_SIZE, u"%hs", "Hello"); // GOOD
+	swprintf(buffer, BUF_SIZE, u"%hs", u"Hello"); // BAD: expecting char
+	swprintf(buffer, BUF_SIZE, u"%hs", L"Hello"); // BAD: expecting char
+
+	swprintf(buffer, BUF_SIZE, u"%ls", "Hello"); // BAD: expecting char16_t
+	swprintf(buffer, BUF_SIZE, u"%ls", u"Hello"); // GOOD
+	swprintf(buffer, BUF_SIZE, u"%ls", L"Hello"); // BAD: expecting char16_t
 }

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_two_byte_wprintf/WrongTypeFormatArguments.expected

@@ -1,2 +1,3 @@
+| printf.cpp:33:31:33:37 | test | This argument should be of type 'char *' but is of type 'char16_t *' |
 | printf.cpp:45:29:45:35 | test | This argument should be of type 'char *' but is of type 'char16_t *' |
 | printf.cpp:52:29:52:35 | test | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_two_byte_wprintf/formattingFunction.expected

@@ -1,2 +1,2 @@
-| printf.cpp:15:5:15:12 | swprintf | char16_t | char | char16_t |
+| printf.cpp:15:5:15:12 | swprintf | char | char16_t | char16_t |
 | printf.cpp:26:5:26:11 | sprintf | char | char16_t | char16_t |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_two_byte_wprintf/printf.cpp

@@ -30,7 +30,7 @@ int sprintf(char *dest, char *format, ...);
 void test1() {
 	WCHAR string[20];
 
-	swprintf(string, u"test %s", u"test"); // GOOD
+	swprintf(string, u"test %s", u"test"); // BAD: `char16_t` string parameter read as `char` string
 }
 
 void test2() {

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_unsigned_chars/WrongTypeFormatArguments.expected

@@ -11,6 +11,8 @@
 | printf1.h:45:18:45:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
 | printf1.h:46:18:46:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
 | printf1.h:47:19:47:21 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
+| printf1.h:126:18:126:19 | wc | This argument should be of type 'char *' but is of type 'wchar_t *' |
+| printf1.h:127:18:127:18 | c | This argument should be of type 'wchar_t *' but is of type 'char *' |
 | real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
 | real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
 | real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_unsigned_chars/formattingFunction.expected

@@ -1,5 +1,5 @@
 | common.h:12:12:12:17 | printf | char | wchar_t | wchar_t |
-| common.h:15:12:15:18 | wprintf | wchar_t | char | wchar_t |
+| common.h:15:12:15:18 | wprintf | char | wchar_t | wchar_t |
 | format.h:4:13:4:17 | error | char | wchar_t | wchar_t |
 | real_world.h:8:12:8:18 | fprintf | char | wchar_t | wchar_t |
 | real_world.h:33:6:33:12 | msg_out | char | wchar_t | wchar_t |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_unsigned_chars/printf1.h

@@ -119,3 +119,11 @@ void test_chars(char c, wchar_t wc, wint_t wt)
   wprintf(L"%C", wc); // GOOD (converts to wint_t)
   wprintf(L"%C", wt); // GOOD
 }
+
+void test_ws(char *c, wchar_t *wc)
+{
+  wprintf(L"%s", c); // GOOD
+  wprintf(L"%s", wc); // BAD
+  wprintf(L"%S", c); // BAD
+  wprintf(L"%S", wc); // GOOD
+}

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Microsoft/WrongTypeFormatArguments.expected

@@ -17,8 +17,11 @@
 | printf1.h:74:19:74:22 | C_ST | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
 | printf1.h:75:19:75:28 | sizeof(<expr>) | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
 | printf1.h:84:23:84:35 | ... - ... | This argument should be of type 'ssize_t' but is of type 'long long' |
+| printf1.h:125:18:125:18 | c | This argument should be of type '__wchar_t *' but is of type 'char *' |
+| printf1.h:128:18:128:19 | wc | This argument should be of type 'char *' but is of type '__wchar_t *' |
 | real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
 | real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
 | real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |
 | real_world.h:64:22:64:24 | & ... | This argument should be of type 'short *' but is of type 'signed int *' |
 | wide_string.h:25:18:25:20 | c | This argument should be of type 'char' but is of type 'char *' |
+| wide_string.h:29:19:29:22 | c | This argument should be of type 'wchar_t' but is of type '__wchar_t *' |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Microsoft/printf1.h

@@ -119,3 +119,11 @@ void test_chars(char c, wchar_t wc, wint_t wt)
   wprintf(L"%C", wc); // BAD [NOT DETECTED]
   wprintf(L"%C", wt); // BAD [NOT DETECTED]
 }
+
+void test_ws(char *c, wchar_t *wc, wint_t *wt)
+{
+  wprintf(L"%s", c); // BAD
+  wprintf(L"%s", wc); // GOOD
+  wprintf(L"%S", c); // GOOD
+  wprintf(L"%S", wc); // BAD
+}

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Microsoft/wide_string.h

@@ -26,5 +26,5 @@ void test_wchar4(char c, const char cc, wchar_t wc, const wchar_t wcc) {
     printf("%wc", wc);             // GOOD
     printf("%wc", wcc);            // GOOD
     printf("%wc", L'c');           // GOOD
-    printf("%wc", L"c");           // BAD [NOT DETECTED]
+    printf("%wc", L"c");           // BAD
 }

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected

@@ -0,0 +1,6 @@
+| test.cpp:42:31:42:36 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:43:38:43:63 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:48:25:48:30 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:49:17:49:30 | new[] | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:52:35:52:60 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
+| test.cpp:55:11:55:24 | new[] | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-190/TaintedAllocationSize.ql

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp

@@ -0,0 +1,58 @@
+// Associated with CWE-190: Integer Overflow or Wraparound. http://cwe.mitre.org/data/definitions/190.html
+
+typedef unsigned long size_t;
+typedef struct {} FILE;
+
+void *malloc(size_t size);
+void *realloc(void *ptr, size_t size);
+int atoi(const char *nptr);
+
+struct MyStruct
+{
+	char data[256];
+};
+
+namespace std
+{
+	template<class charT> struct char_traits;
+
+	template <class charT, class traits = char_traits<charT> >
+	class basic_istream /*: virtual public basic_ios<charT,traits> - not needed for this test */ {
+	public:
+		basic_istream<charT,traits>& operator>>(int& n);
+	};
+
+	typedef basic_istream<char> istream;
+
+	extern istream cin;
+}
+
+int getTainted() {
+	int i;
+	
+	std::cin >> i;
+
+	return i;
+}
+
+int main(int argc, char **argv) {
+	int tainted = atoi(argv[1]);
+
+	MyStruct *arr1 = (MyStruct *)malloc(sizeof(MyStruct)); // GOOD
+	MyStruct *arr2 = (MyStruct *)malloc(tainted); // BAD
+	MyStruct *arr3 = (MyStruct *)malloc(tainted * sizeof(MyStruct)); // BAD
+	MyStruct *arr4 = (MyStruct *)malloc(getTainted() * sizeof(MyStruct)); // BAD [NOT DETECTED]
+	MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // BAD [NOT DETECTED]
+
+	int size = tainted * 8;
+	char *chars1 = (char *)malloc(size); // BAD
+	char *chars2 = new char[size]; // BAD
+	char *chars3 = new char[8]; // GOOD
+
+	arr1 = (MyStruct *)realloc(arr1, sizeof(MyStruct) * tainted); // BAD
+
+	size = 8;
+	chars3 = new char[size]; // GOOD [FALSE POSITIVE]
+
+	return 0;
+}

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected

@@ -8,6 +8,3 @@
 | test.c:14:15:14:28 | maxConnections | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:11:29:11:32 | argv | User-provided value |
 | test.c:44:7:44:10 | len2 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:41:17:41:20 | argv | User-provided value |
 | test.c:54:7:54:10 | len3 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:51:17:51:20 | argv | User-provided value |
-| test.c:74:7:74:10 | len5 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:71:19:71:22 | argv | User-provided value |
-| test.c:84:7:84:10 | len6 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:81:19:81:22 | argv | User-provided value |
-| test.c:94:7:94:10 | len7 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:91:19:91:22 | argv | User-provided value |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/test.c

@@ -71,7 +71,7 @@ int main(int argc, char** argv) {
     len5 = strlen(argv[1]);
     while (len5)
     {
-      len5--; // GOOD: can't underflow [FALSE POSITIVE]
+      len5--; // GOOD: can't underflow
     }
   }
 
@@ -81,7 +81,7 @@ int main(int argc, char** argv) {
     len6 = strlen(argv[1]);
     while (len6 != 0)
     {
-      len6--; // GOOD: can't underflow [FALSE POSITIVE]
+      len6--; // GOOD: can't underflow
     }
   }
 
@@ -91,7 +91,7 @@ int main(int argc, char** argv) {
     len7 = strlen(argv[1]);
     while ((len7) && (1))
     {
-      len7--; // GOOD: can't underflow [FALSE POSITIVE]
+      len7--; // GOOD: can't underflow
     }
   }
 

csharp/extractor/Semmle.Extraction.CSharp/Entities/Attribute.cs

@@ -51,9 +51,12 @@ namespace Semmle.Extraction.CSharp.Entities
                     int child = 0;
                     foreach (var arg in syntax.ArgumentList.Arguments)
                     {
-                        Expression.Create(cx, arg.Expression, this, child++);
+                        var expr = Expression.Create(cx, arg.Expression, this, child++);
+                        if (!(arg.NameEquals is null))
+                        {
+                            cx.Emit(Tuples.expr_argument_name(expr, arg.NameEquals.Name.Identifier.Text));
+                        }
                     }
-                    // !! Handle named arguments
                 });
             }
         }
@@ -66,14 +69,6 @@ namespace Semmle.Extraction.CSharp.Entities
             }
         }
 
-        public static void ExtractAttributes(Context cx, IEnumerable<AttributeListSyntax> attributes, IEntity entity)
-        {
-            foreach (var attributeSyntax in attributes.SelectMany(l => l.Attributes))
-            {
-                new Attribute(cx, attributeSyntax, entity);
-            }
-        }
-
         public override TrapStackBehaviour TrapStackBehaviour => TrapStackBehaviour.OptionalLabel;
     }
 }

csharp/ql/src/semmle/code/csharp/Attribute.qll

@@ -28,7 +28,10 @@ class Attributable extends @attributable {
   predicate hasLocationInfo(
     string filepath, int startline, int startcolumn, int endline, int endcolumn
   ) {
-    this.(Element).getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    this
+        .(Element)
+        .getLocation()
+        .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
 }
 
@@ -51,9 +54,37 @@ class Attribute extends TopLevelExprParent, @attribute {
   /** Gets the element that this attribute is attached to. */
   Attributable getTarget() { attributes(this, _, result) }
 
-  /** Gets the `i`th argument of this attribute. */
+  /**
+   * Gets the `i`th argument of this attribute. This includes both constructor
+   * arguments and named arguments.
+   */
   Expr getArgument(int i) { result = this.getChildExpr(i) }
 
+  /**
+   * Gets the `i`th constructor argument of this attribute. For example, only
+   * `true` is a constructor argument in
+   *
+   * ```
+   * MyAttribute[true, Foo = 0]
+   * ```
+   */
+  Expr getConstructorArgument(int i) {
+    result = this.getArgument(i) and not exists(result.getExplicitArgumentName())
+  }
+
+  /**
+   * Gets the named argument `name` of this attribute. For example, only
+   * `0` is a named argument in
+   *
+   * ```
+   * MyAttribute[true, Foo = 0]
+   * ```
+   */
+  Expr getNamedArgument(string name) {
+    result = this.getArgument(_) and
+    result.getExplicitArgumentName() = name
+  }
+
   override Location getALocation() { attribute_location(this, result) }
 
   override string toString() {

csharp/ql/src/semmle/code/csharp/dispatch/Dispatch.qll

@@ -202,7 +202,7 @@ private module Internal {
      * Gets a non-exact (see `hasQualifierType()`) qualifier type of this call
      * that does not contain type parameters.
      */
-    TypeWithoutTypeParameters getANonExactQualifierTypeWithoutTypeParameters() {
+    private TypeWithoutTypeParameters getANonExactQualifierTypeWithoutTypeParameters() {
       exists(Type qualifierType | hasQualifierType(qualifierType, false) |
         // Qualifier type contains no type parameters: use it
         result = qualifierType
@@ -220,6 +220,20 @@ private module Internal {
         result = qualifierType.(QualifierTypeWithTypeParameters).getAPotentialInstance()
       )
     }
+
+    /**
+     * Gets a non-exact (see `hasQualifierType()`) qualifier type of this call.
+     */
+    ValueOrRefType getANonExactQualifierType() {
+      exists(TypeWithoutTypeParameters t |
+        t = this.getANonExactQualifierTypeWithoutTypeParameters()
+      |
+        result.(ConstructedType).getUnboundGeneric() = t
+        or
+        not t instanceof UnboundGenericType and
+        result = t
+      )
+    }
   }
 
   private class DynamicFieldOrProperty extends Assignable {
@@ -480,9 +494,9 @@ private module Internal {
      * qualifier types are `B.M` and `C.M`, `C.M`, and none, respectively.
      */
     private RuntimeInstanceMethod getAViableOverrider() {
-      exists(TypeWithoutTypeParameters t, NonConstructedOverridableMethod m |
+      exists(ValueOrRefType t, NonConstructedOverridableMethod m |
-        t = getANonExactQualifierTypeWithoutTypeParameters() and
+        t = this.getANonExactQualifierType() and
-        getAStaticTarget() = m.getAConstructingMethodOrSelf() and
+        this.getAStaticTarget() = m.getAConstructingMethodOrSelf() and
         result = m.getAnOverrider(t)
       )
     }
@@ -620,9 +634,9 @@ private module Internal {
      * respectively.
      */
     private RuntimeAccessor getAViableOverrider() {
-      exists(TypeWithoutTypeParameters t |
+      exists(ValueOrRefType t |
-        t = getANonExactQualifierTypeWithoutTypeParameters() and
+        t = this.getANonExactQualifierType() |
-        result = getAStaticTarget().(OverridableAccessor).getAnOverrider(t)
+        result = this.getAStaticTarget().(OverridableAccessor).getAnOverrider(t)
       )
     }
   }

csharp/ql/src/semmle/code/csharp/dispatch/OverridableCallable.qll

@@ -135,27 +135,33 @@ class OverridableCallable extends Callable {
    * - `C2.M = C2.M.getInherited(C3)`.
    */
   Callable getInherited(SourceDeclarationType t) {
-    exists(Callable sourceDecl | result = getInherited1(t, sourceDecl) |
+    exists(Callable sourceDecl | result = this.getInherited2(t, sourceDecl) |
       hasSourceDeclarationCallable(t, sourceDecl)
     )
   }
 
-  private Callable getInherited0(SourceDeclarationType t) {
+  private Callable getInherited0(ValueOrRefType t) {
     // A (transitive, reflexive) overrider
-    t = this.hasOverrider(result).getASubType*().getSourceDeclaration()
+    t = this.hasOverrider(result)
+    or
+    // A (transitive) overrider of an interface implementation
+    t = this.hasOverridingImplementor(result)
+    or
+    exists(ValueOrRefType mid | result = this.getInherited0(mid) | t = mid.getASubType())
+  }
+
+  private Callable getInherited1(SourceDeclarationType t) {
+    exists(ValueOrRefType t0 | result = getInherited0(t0) | t = t0.getSourceDeclaration())
     or
     // An interface implementation
     exists(ValueOrRefType s |
       result = getAnImplementorSubType(s) and
       t = s.getSourceDeclaration()
     )
-    or
-    // A (transitive) overrider of an interface implementation
-    t = this.hasOverridingImplementor(result).getASubType*().getSourceDeclaration()
   }
 
-  private Callable getInherited1(SourceDeclarationType t, Callable sourceDecl) {
+  private Callable getInherited2(SourceDeclarationType t, Callable sourceDecl) {
-    result = this.getInherited0(t) and
+    result = this.getInherited1(t) and
     sourceDecl = result.getSourceDeclaration()
   }
 
@@ -171,9 +177,42 @@ class OverridableCallable extends Callable {
     result = c.getDeclaringType()
   }
 
+  private predicate isDeclaringSubType(ValueOrRefType t) {
+    t = this.getDeclaringType()
+    or
+    exists(ValueOrRefType mid | isDeclaringSubType(mid) | t = mid.getASubType())
+  }
+
+  pragma[noinline]
+  private Callable getAnOverrider0(ValueOrRefType t) {
+    not this.declaredInTypeWithTypeParameters() and
+    (
+      // A (transitive) overrider
+      result = this.getAnOverrider+() and
+      t = result.getDeclaringType()
+      or
+      // An interface implementation
+      result = this.getAnImplementorSubType(t)
+      or
+      // A (transitive) overrider of an interface implementation
+      result = this.getAnOverridingImplementor() and
+      t = result.getDeclaringType()
+    )
+  }
+
+  private Callable getAnOverrider1(ValueOrRefType t) {
+    result = this.getAnOverrider0(t)
+    or
+    exists(ValueOrRefType mid | result = this.getAnOverrider1(mid) |
+      t = mid.getABaseType() and
+      this.isDeclaringSubType(t)
+    )
+  }
+
   /**
-   * Gets a callable defined in a sub type of `t` that overrides/implements
+   * Gets a callable defined in a sub type of `t` (which is itself a sub type
-   * this callable, if any.
+   * of this callable's declaring type) that overrides/implements this callable,
+   * if any.
    *
    * The type `t` may be a constructed type: For example, if `t = C<int>`,
    * then only callables defined in sub types of `C<int>` (and e.g. not
@@ -181,39 +220,7 @@ class OverridableCallable extends Callable {
    * contains a callable that overrides this callable, then only if `C2<int>`
    * is ever constructed will the callable in `C2` be considered valid.
    */
-  Callable getAnOverrider(TypeWithoutTypeParameters t) {
+  Callable getAnOverrider(ValueOrRefType t) { result = this.getABoundInstance().getAnOverrider1(t) }
-    exists(OverridableCallable oc, ValueOrRefType sub |
-      result = oc.getAnOverriderAux(sub) and
-      t = oc.getAnOverriderBaseType(sub) and
-      oc = getABoundInstance()
-    )
-  }
-
-  // predicate folding to get proper join order
-  private Callable getAnOverriderAux(ValueOrRefType t) {
-    not declaredInTypeWithTypeParameters() and
-    (
-      // A (transitive) overrider
-      result = getAnOverrider+() and
-      t = result.getDeclaringType()
-      or
-      // An interface implementation
-      result = getAnImplementorSubType(t)
-      or
-      // A (transitive) overrider of an interface implementation
-      result = getAnOverridingImplementor() and
-      t = result.getDeclaringType()
-    )
-  }
-
-  private TypeWithoutTypeParameters getAnOverriderBaseType(ValueOrRefType t) {
-    exists(getAnOverriderAux(t)) and
-    exists(Type t0 | t0 = t.getABaseType*() |
-      result = t0
-      or
-      result = t0.(ConstructedType).getUnboundGeneric()
-    )
-  }
 
   /**
    * Gets a bound instance of this callable.
@@ -264,7 +271,7 @@ class OverridableMethod extends Method, OverridableCallable {
     result = OverridableCallable.super.getInherited(t)
   }
 
-  override Method getAnOverrider(TypeWithoutTypeParameters t) {
+  override Method getAnOverrider(ValueOrRefType t) {
     result = OverridableCallable.super.getAnOverrider(t)
   }
 }
@@ -311,7 +318,7 @@ class OverridableAccessor extends Accessor, OverridableCallable {
     result = OverridableCallable.super.getInherited(t)
   }
 
-  override Accessor getAnOverrider(TypeWithoutTypeParameters t) {
+  override Accessor getAnOverrider(ValueOrRefType t) {
     result = OverridableCallable.super.getAnOverrider(t)
   }
 }

csharp/ql/test/library-tests/arguments/argumentName.expected

@@ -7,3 +7,5 @@
 | arguments.cs:39:27:39:27 | 0 | o |
 | arguments.cs:40:18:40:35 | array creation of type Int32[] | args |
 | arguments.cs:40:41:40:41 | 0 | o |
+| arguments.cs:68:28:68:29 | "" | y |
+| arguments.cs:68:36:68:36 | 0 | x |

csharp/ql/test/library-tests/arguments/arguments.cs

@@ -61,4 +61,17 @@ class ArgumentsTest
         var tuple = (13, 14);
         (Prop, this[15, 16]) = tuple;
     }
+
+    [MyAttribute(false)]
+    void f6() { }
+
+    [MyAttribute(true, y = "", x = 0)]
+    void f7() { }
+}
+
+class MyAttribute : Attribute
+{
+    public int x;
+    public string y { get; set; }
+    public MyAttribute(bool b) { }
 }

csharp/ql/test/library-tests/attributes/AttributeArguments.expected

@@ -1,3 +1,4 @@
+arguments
 | attributes.cs:10:12:10:24 | [AssemblyTitle(...)] | 0 | attributes.cs:10:26:10:45 | "C# attributes test" |
 | attributes.cs:11:12:11:30 | [AssemblyDescription(...)] | 0 | attributes.cs:11:32:11:56 | "A test of C# attributes" |
 | attributes.cs:12:12:12:32 | [AssemblyConfiguration(...)] | 0 | attributes.cs:12:34:12:35 | "" |
@@ -12,3 +13,27 @@
 | attributes.cs:38:12:38:30 | [AssemblyFileVersion(...)] | 0 | attributes.cs:38:32:38:40 | "1.0.0.0" |
 | attributes.cs:40:2:40:22 | [AttributeUsage(...)] | 0 | attributes.cs:40:24:40:50 | access to constant All |
 | attributes.cs:43:6:43:16 | [Conditional(...)] | 0 | attributes.cs:43:18:43:25 | "DEBUG2" |
+| attributes.cs:51:6:51:16 | [My(...)] | 0 | attributes.cs:51:18:51:22 | false |
+| attributes.cs:54:6:54:16 | [My(...)] | 0 | attributes.cs:54:18:54:21 | true |
+| attributes.cs:54:6:54:16 | [My(...)] | 1 | attributes.cs:54:28:54:29 | "" |
+| attributes.cs:54:6:54:16 | [My(...)] | 2 | attributes.cs:54:36:54:36 | 0 |
+constructorArguments
+| attributes.cs:10:12:10:24 | [AssemblyTitle(...)] | 0 | attributes.cs:10:26:10:45 | "C# attributes test" |
+| attributes.cs:11:12:11:30 | [AssemblyDescription(...)] | 0 | attributes.cs:11:32:11:56 | "A test of C# attributes" |
+| attributes.cs:12:12:12:32 | [AssemblyConfiguration(...)] | 0 | attributes.cs:12:34:12:35 | "" |
+| attributes.cs:13:12:13:26 | [AssemblyCompany(...)] | 0 | attributes.cs:13:28:13:39 | "Semmle Plc" |
+| attributes.cs:14:12:14:26 | [AssemblyProduct(...)] | 0 | attributes.cs:14:28:14:34 | "Odasa" |
+| attributes.cs:15:12:15:28 | [AssemblyCopyright(...)] | 0 | attributes.cs:15:30:15:54 | "Copyright \ufffd Semmle 2018" |
+| attributes.cs:16:12:16:28 | [AssemblyTrademark(...)] | 0 | attributes.cs:16:30:16:31 | "" |
+| attributes.cs:17:12:17:26 | [AssemblyCulture(...)] | 0 | attributes.cs:17:28:17:29 | "" |
+| attributes.cs:22:12:22:21 | [ComVisible(...)] | 0 | attributes.cs:22:23:22:27 | false |
+| attributes.cs:25:12:25:15 | [Guid(...)] | 0 | attributes.cs:25:17:25:54 | "2f70fdd6-14aa-4850-b053-d547adb1f476" |
+| attributes.cs:37:12:37:26 | [AssemblyVersion(...)] | 0 | attributes.cs:37:28:37:36 | "1.0.0.0" |
+| attributes.cs:38:12:38:30 | [AssemblyFileVersion(...)] | 0 | attributes.cs:38:32:38:40 | "1.0.0.0" |
+| attributes.cs:40:2:40:22 | [AttributeUsage(...)] | 0 | attributes.cs:40:24:40:50 | access to constant All |
+| attributes.cs:43:6:43:16 | [Conditional(...)] | 0 | attributes.cs:43:18:43:25 | "DEBUG2" |
+| attributes.cs:51:6:51:16 | [My(...)] | 0 | attributes.cs:51:18:51:22 | false |
+| attributes.cs:54:6:54:16 | [My(...)] | 0 | attributes.cs:54:18:54:21 | true |
+namedArguments
+| attributes.cs:54:6:54:16 | [My(...)] | x | attributes.cs:54:36:54:36 | 0 |
+| attributes.cs:54:6:54:16 | [My(...)] | y | attributes.cs:54:28:54:29 | "" |

csharp/ql/test/library-tests/attributes/AttributeArguments.ql

@@ -1,4 +1,13 @@
 import csharp
 
-from Attribute attribute, int index
+query predicate arguments(Attribute attribute, int index, Expr e) {
-select attribute, index, attribute.getArgument(index)
+  e = attribute.getArgument(index)
+}
+
+query predicate constructorArguments(Attribute attribute, int index, Expr e) {
+  e = attribute.getConstructorArgument(index)
+}
+
+query predicate namedArguments(Attribute attribute, string name, Expr e) {
+  e = attribute.getNamedArgument(name)
+}

csharp/ql/test/library-tests/attributes/AttributeElements.expected

@@ -1,6 +1,8 @@
 | attributes.cs:41:7:41:9 | Foo | attributes.cs:40:2:40:22 | [AttributeUsage(...)] | System.AttributeUsageAttribute |
 | attributes.cs:44:17:44:19 | foo | attributes.cs:43:6:43:16 | [Conditional(...)] | System.Diagnostics.ConditionalAttribute |
 | attributes.cs:49:23:49:23 | x | attributes.cs:49:14:49:16 | [Foo(...)] | Foo |
+| attributes.cs:52:10:52:11 | M1 | attributes.cs:51:6:51:16 | [My(...)] | MyAttribute |
+| attributes.cs:55:10:55:11 | M2 | attributes.cs:54:6:54:16 | [My(...)] | MyAttribute |
 | attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:10:12:10:24 | [AssemblyTitle(...)] | System.Reflection.AssemblyTitleAttribute |
 | attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:11:12:11:30 | [AssemblyDescription(...)] | System.Reflection.AssemblyDescriptionAttribute |
 | attributes.dll:0:0:0:0 | attributes, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null | attributes.cs:12:12:12:32 | [AssemblyConfiguration(...)] | System.Reflection.AssemblyConfigurationAttribute |

csharp/ql/test/library-tests/attributes/attributes.cs

@@ -47,4 +47,17 @@ class Foo : Attribute
 class Bar
 {
     int inc([Foo] int x) { return x + 1; }
+
+    [MyAttribute(false)]
+    void M1() { }
+
+    [MyAttribute(true, y = "", x = 0)]
+    void M2() { }
+}
+
+class MyAttribute : Attribute
+{
+    public int x;
+    public string y { get; set; }
+    public MyAttribute(bool b) { }
 }

javascript/ql/src/javascript.qll

@@ -69,6 +69,7 @@ import semmle.javascript.frameworks.CryptoLibraries
 import semmle.javascript.frameworks.DigitalOcean
 import semmle.javascript.frameworks.Electron
 import semmle.javascript.frameworks.Files
+import semmle.javascript.frameworks.Firebase
 import semmle.javascript.frameworks.jQuery
 import semmle.javascript.frameworks.LodashUnderscore
 import semmle.javascript.frameworks.Logging

javascript/ql/src/semmle/javascript/AMD.qll

@@ -170,20 +170,19 @@ class AMDModuleDefinition extends CallExpr {
   }
 }
 
-/** A path expression appearing in the list of dependencies of an AMD module. */
+/** An AMD dependency, considered as a path expression. */
-private class AMDDependencyPath extends PathExprInModule, ConstantString {
+private class AmdDependencyPath extends PathExprCandidate {
-  AMDDependencyPath() {
+  AmdDependencyPath() {
-    exists(AMDModuleDefinition amd | this.getParentExpr*() = amd.getDependencies().getAnElement())
+    exists(AMDModuleDefinition amd |
+      this = amd.getDependencies().getAnElement() or
+      this = amd.getARequireCall().getAnArgument()
+    )
   }
-
-  override string getValue() { result = this.(ConstantString).getStringValue() }
 }
 
-/** A path expression appearing in a `require` call in an AMD module. */
+/** A constant path element appearing in an AMD dependency expression. */
-private class AMDRequirePath extends PathExprInModule, ConstantString {
+private class ConstantAmdDependencyPathElement extends PathExprInModule, ConstantString {
-  AMDRequirePath() {
+  ConstantAmdDependencyPathElement() { this = any(AmdDependencyPath amd).getAPart() }
-    exists(AMDModuleDefinition amd | this.getParentExpr*() = amd.getARequireCall().getAnArgument())
-  }
 
   override string getValue() { result = this.(ConstantString).getStringValue() }
 }

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

@@ -64,7 +64,7 @@ module Closure {
    * a top-level expression statement.
    */
   private predicate isTopLevelExpr(DataFlow::Node node) {
-    node.getTopLevel().getAChildStmt().(ExprStmt).getExpr().flow() = node
+    any(TopLevel tl).getAChildStmt().(ExprStmt).getExpr().flow() = node
   }
 
   /**

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

@@ -121,6 +121,23 @@ private predicate isGeneratedHtml(File f) {
     e.getName() = "meta" and
     e.getAttributeByName("name").getValue() = "generator"
   )
+  or
+  20 < countStartingHtmlElements(f, _)
+}
+
+/**
+ * Gets an element that starts at line `l` in file `f`.
+ */
+private HTML::Element getAStartingElement(File f, int l) {
+  result.getFile() = f and result.getLocation().getStartLine() = l
+}
+
+
+/**
+ * Gets the number of HTML elements that start at line `l` in file `f`.
+ */
+private int countStartingHtmlElements(File f, int l) {
+  result = strictcount(getAStartingElement(f, l))
 }
 
 /**

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

@@ -239,22 +239,22 @@ class Require extends CallExpr, Import {
   }
 }
 
-/** A literal path expression appearing in a `require` import. */
+/** An argument to `require` or `require.resolve`, considered as a path expression. */
-private class LiteralRequiredPath extends PathExprInModule, ConstantString {
+private class RequirePath extends PathExprCandidate {
-  LiteralRequiredPath() { exists(Require req | this.getParentExpr*() = req.getArgument(0)) }
+  RequirePath() {
-
+    this = any(Require req).getArgument(0)
-  override string getValue() { result = this.getStringValue() }
+    or
-}
-
-/** A literal path expression appearing in a call to `require.resolve`. */
-private class LiteralRequireResolvePath extends PathExprInModule, ConstantString {
-  LiteralRequireResolvePath() {
     exists(RequireVariable req, MethodCallExpr reqres |
       reqres.getReceiver() = req.getAnAccess() and
       reqres.getMethodName() = "resolve" and
-      this.getParentExpr*() = reqres.getArgument(0)
+      this = reqres.getArgument(0)
     )
   }
+}
+
+/** A constant path element appearing in a call to `require` or `require.resolve`. */
+private class ConstantRequirePathElement extends PathExprInModule, ConstantString {
+  ConstantRequirePathElement() { this = any(RequirePath rp).getAPart() }
 
   override string getValue() { result = this.getStringValue() }
 }

javascript/ql/src/semmle/javascript/Paths.qll

@@ -236,3 +236,20 @@ private class ConcatPath extends PathExpr {
     result = this.(AddExpr).getAnOperand().(PathExpr).getSearchRoot(priority)
   }
 }
+
+/**
+ * An expression that appears in a syntactic position where it may represent a path.
+ *
+ * Examples include arguments to the CommonJS `require` function or AMD dependency arguments.
+ */
+abstract class PathExprCandidate extends Expr {
+  /**
+   * Gets an expression that is nested inside this expression.
+   *
+   * Equivalent to `getAChildExpr*()`, but useful to enforce a better join order (in spite of
+   * what the optimizer thinks, there are generally far fewer `PathExprCandidate`s than
+   * `ConstantString`s).
+   */
+  pragma[nomagic]
+  Expr getAPart() { result = this or result = getAPart().getAChildExpr() }
+}

javascript/ql/src/semmle/javascript/StandardLibrary.qll

@@ -189,7 +189,7 @@ private class PromiseFlowStep extends DataFlow::AdditionalFlowStep {
 /**
  * Holds if taint propagates from `pred` to `succ` through promises.
  */
-private predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
   // from `x` to `new Promise((res, rej) => res(x))`
   pred = succ.(PromiseDefinition).getResolveParameter().getACall().getArgument(0)
   or

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

@@ -162,10 +162,11 @@ class SourceNode extends DataFlow::Node {
    *
    * See `TypeTracker` for more details about how to use this.
    */
+  pragma[inline]
   DataFlow::SourceNode track(TypeTracker t2, TypeTracker t) {
     exists(StepSummary summary |
       StepSummary::step(this, result, summary) and
-      t = StepSummary::append(t2, summary)
+      t = t2.append(summary)
     )
   }
 
@@ -176,10 +177,11 @@ class SourceNode extends DataFlow::Node {
    *
    * See `TypeBackTracker` for more details about how to use this.
    */
+  pragma[inline]
   DataFlow::SourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) {
     exists(StepSummary summary |
       StepSummary::step(result, this, summary) and
-      t = StepSummary::prepend(summary, t2)
+      t = t2.prepend(summary)
     )
   }
 }

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

@@ -9,133 +9,87 @@
 import javascript
 private import internal.FlowSteps
 
-/**
+private class PropertyName extends string {
- * A pair of booleans, indicating whether a path goes through a return and/or a call.
+  PropertyName() { this = any(DataFlow::PropRef pr).getPropertyName() }
- *
- * Identical to `TPathSummary` except without flow labels.
- */
-private newtype TStepSummary = MkStepSummary(boolean hasReturn, boolean hasCall) {
-  (hasReturn = true or hasReturn = false) and
-  (hasCall = true or hasCall = false)
 }
 
+private class OptionalPropertyName extends string {
+  OptionalPropertyName() { this instanceof PropertyName or this = "" }
+}
+
+/**
+ * A description of a step on an inter-procedural data flow path.
+ */
+private newtype TStepSummary =
+  LevelStep() or
+  CallStep() or
+  ReturnStep() or
+  StoreStep(PropertyName prop) or
+  LoadStep(PropertyName prop)
+
 /**
  * INTERNAL: Use `TypeTracker` or `TypeBackTracker` instead.
  *
- * Summary of the steps needed to track a value to a given dataflow node.
+ * A description of a step on an inter-procedural data flow path.
  */
 class StepSummary extends TStepSummary {
-  Boolean hasReturn;
+  /** Gets a textual representation of this step summary. */
-
-  Boolean hasCall;
-
-  StepSummary() { this = MkStepSummary(hasReturn, hasCall) }
-
-  /** Indicates whether the path represented by this summary contains any return steps. */
-  boolean hasReturn() { result = hasReturn }
-
-  /** Indicates whether the path represented by this summary contains any call steps. */
-  boolean hasCall() { result = hasCall }
-
-  /**
-   * Gets the summary for the path obtained by appending `that` to `this`.
-   *
-   * Note that a path containing a `return` step cannot be appended to a path containing
-   * a `call` step in order to maintain well-formedness.
-   */
-  StepSummary append(StepSummary that) {
-    exists(Boolean hasReturn2, Boolean hasCall2 |
-      that = MkStepSummary(hasReturn2, hasCall2)
-    |
-      result = MkStepSummary(hasReturn.booleanOr(hasReturn2), hasCall.booleanOr(hasCall2)) and
-      // avoid constructing invalid paths
-      not (hasCall = true and hasReturn2 = true)
-    )
-  }
-
-  /**
-   * Gets the summary for the path obtained by appending `this` to `that`.
-   */
-  StepSummary prepend(StepSummary that) { result = that.append(this) }
-
-  /** Gets a textual representation of this path summary. */
   string toString() {
-    exists(string withReturn, string withCall |
+    this instanceof LevelStep and result = "level"
-      (if hasReturn = true then withReturn = "with" else withReturn = "without") and
+    or
-      (if hasCall = true then withCall = "with" else withCall = "without")
+    this instanceof CallStep and result = "call"
-    |
+    or
-      result = "path " + withReturn + " return steps and " + withCall + " call steps"
+    this instanceof ReturnStep and result = "return"
+    or
+    exists(string prop | this = StoreStep(prop) |
+      result = "store " + prop
+    )
+    or
+    exists(string prop | this = LoadStep(prop) |
+      result = "load" + prop
     )
   }
 }
 
 module StepSummary {
-  /**
-   * Gets a summary describing a path without any calls or returns.
-   */
-  StepSummary level() { result = MkStepSummary(false, false) }
-
-  /**
-   * Gets a summary describing a path with one or more calls, but no returns.
-   */
-  StepSummary call() { result = MkStepSummary(false, true) }
-
-  /**
-   * Gets a summary describing a path with one or more returns, but no calls.
-   */
-  StepSummary return() { result = MkStepSummary(true, false) }
-
   /**
    * INTERNAL: Use `SourceNode.track()` or `SourceNode.backtrack()` instead.
    */
   predicate step(DataFlow::SourceNode pred, DataFlow::SourceNode succ, StepSummary summary) {
-    exists (DataFlow::Node predNode | pred.flowsTo(predNode) |
+    exists(DataFlow::Node predNode | pred.flowsTo(predNode) |
       // Flow through properties of objects
       propertyFlowStep(predNode, succ) and
-      summary = level()
+      summary = LevelStep()
       or
       // Flow through global variables
       globalFlowStep(predNode, succ) and
-      summary = level()
+      summary = LevelStep()
       or
       // Flow into function
       callStep(predNode, succ) and
-      summary = call()
+      summary = CallStep()
       or
       // Flow out of function
       returnStep(predNode, succ) and
-      summary = return()
+      summary = ReturnStep()
       or
       // Flow through an instance field between members of the same class
       DataFlow::localFieldStep(predNode, succ) and
-      summary = level()
+      summary = LevelStep()
+      or
+      exists(string prop |
+        basicStoreStep(predNode, succ, prop) and
+        summary = StoreStep(prop)
+        or
+        loadStep(predNode, succ, prop) and
+        summary = LoadStep(prop)
+      )
     )
   }
-
-  /**
-   * INTERNAL. Do not use.
-   *
-   * Appends a step summary onto a type-tracking summary.
-   */
-  TypeTracker append(TypeTracker type, StepSummary summary) {
-    not (type.hasCall() = true and summary.hasReturn() = true) and
-    result.hasCall() = type.hasCall().booleanOr(summary.hasCall())
-  }
-
-  /**
-   * INTERNAL. Do not use.
-   *
-   * Prepends a step summary before a backwards type-tracking summary.
-   */
-  TypeBackTracker prepend(StepSummary summary, TypeBackTracker type) {
-    not (type.hasReturn() = true and summary.hasCall() = true) and
-    result.hasReturn() = type.hasReturn().booleanOr(summary.hasReturn())
-  }
 }
 
-private newtype TTypeTracker = MkTypeTracker(boolean hasCall) {
+private newtype TTypeTracker =
-  hasCall = true or hasCall = false
+  MkTypeTracker(Boolean hasCall, OptionalPropertyName prop)
-}
 
 /**
  * EXPERIMENTAL.
@@ -159,7 +113,7 @@ private newtype TTypeTracker = MkTypeTracker(boolean hasCall) {
  *   )
  * }
  *
- * DataFlow::SourceNode myType() { result = myType(_) }
+ * DataFlow::SourceNode myType() { result = myType(DataFlow::TypeTracker::end()) }
  * ```
  *
  * To track values backwards, which can be useful for tracking
@@ -168,35 +122,56 @@ private newtype TTypeTracker = MkTypeTracker(boolean hasCall) {
 class TypeTracker extends TTypeTracker {
   Boolean hasCall;
 
-  TypeTracker() { this = MkTypeTracker(hasCall) }
+  string prop;
+
+  TypeTracker() { this = MkTypeTracker(hasCall, prop) }
+
+  TypeTracker append(StepSummary step) {
+    step = LevelStep() and result = this
+    or
+    step = CallStep() and result = MkTypeTracker(true, prop)
+    or
+    step = ReturnStep() and hasCall = false and result = this
+    or
+    step = LoadStep(prop) and result = MkTypeTracker(hasCall, "")
+    or
+    exists(string p |
+      step = StoreStep(p) and prop = "" and result = MkTypeTracker(hasCall, p)
+    )
+  }
 
   string toString() {
-    hasCall = true and result = "type tracker with call steps"
+    exists(string withCall, string withProp |
-    or
+      (if hasCall = true then withCall = "with" else withCall = "without") and
-    hasCall = false and result = "type tracker without call steps"
+      (if prop != "" then withProp = " with property " + prop else withProp = "") and
+      result = "type tracker " + withCall + " call steps" + withProp
+    )
   }
 
   /**
    * Holds if this is the starting point of type tracking.
    */
-  predicate start() {
+  predicate start() { hasCall = false and prop = "" }
-    hasCall = false
+
-  }
+  predicate end() { prop = "" }
 
   /**
    * INTERNAL. DO NOT USE.
    *
    * Holds if this type has been tracked into a call.
    */
-  boolean hasCall() {
+  boolean hasCall() { result = hasCall }
-    result = hasCall
+
-  }
+  string getProp() { result = prop }
 }
 
-private newtype TTypeBackTracker = MkTypeBackTracker(boolean hasReturn) {
+module TypeTracker {
-  hasReturn = true or hasReturn = false
+  TypeTracker end() { result.end() }
 }
 
+private newtype TTypeBackTracker =
+  MkTypeBackTracker(Boolean hasReturn, OptionalPropertyName prop)
+
 /**
  * EXPERIMENTAL.
  *
@@ -221,33 +196,55 @@ private newtype TTypeBackTracker = MkTypeBackTracker(boolean hasReturn) {
  *   )
  * }
  *
- * DataFlow::SourceNode myCallback() { result = myCallback(_) }
+ * DataFlow::SourceNode myCallback() { result = myCallback(DataFlow::TypeBackTracker::end()) }
  * ```
  */
 class TypeBackTracker extends TTypeBackTracker {
   Boolean hasReturn;
 
-  TypeBackTracker() { this = MkTypeBackTracker(hasReturn) }
+  string prop;
+
+  TypeBackTracker() { this = MkTypeBackTracker(hasReturn, prop) }
+
+  TypeBackTracker prepend(StepSummary step) {
+    step = LevelStep() and result = this
+    or
+    step = CallStep() and hasReturn = false and result = this
+    or
+    step = ReturnStep() and result = MkTypeBackTracker(true, prop)
+    or
+    exists(string p |
+      step = LoadStep(p) and prop = "" and result = MkTypeBackTracker(hasReturn, p)
+    )
+    or
+    step = StoreStep(prop) and result = MkTypeBackTracker(hasReturn, "")
+  }
 
   string toString() {
-    hasReturn = true and result = "type back-tracker with return steps"
+    exists(string withReturn, string withProp |
-    or
+      (if hasReturn = true then withReturn = "with" else withReturn = "without") and
-    hasReturn = false and result = "type back-tracker without return steps"
+      (if prop != "" then withProp = " with property " + prop else withProp = "") and
+      result = "type back-tracker " + withReturn + " return steps" + withProp
+    )
   }
 
   /**
    * Holds if this is the starting point of type tracking.
    */
-  predicate start() {
+  predicate start() { hasReturn = false and prop = "" }
-    hasReturn = false
+
-  }
+  predicate end() { prop = "" }
 
   /**
    * INTERNAL. DO NOT USE.
    *
    * Holds if this type has been back-tracked into a call through return edge.
    */
-  boolean hasReturn() {
+  boolean hasReturn() { result = hasReturn }
-    result = hasReturn
+
-  }
+  string getProp() { result = prop }
+}
+
+module TypeBackTracker {
+  TypeBackTracker end() { result.end() }
 }

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

@@ -17,36 +17,6 @@ predicate shouldTrackProperties(AbstractValue obj) {
   obj instanceof AbstractModuleObject
 }
 
-/**
- * Holds if `invk` may invoke `f`.
- */
-predicate calls(DataFlow::InvokeNode invk, Function f) { f = invk.getACallee(0) }
-
-/**
- * Holds if `invk` may invoke `f` indirectly through the given `callback` argument.
- *
- * This only holds for explicitly modeled partial calls.
- */
-private predicate partiallyCalls(
-  DataFlow::AdditionalPartialInvokeNode invk, DataFlow::AnalyzedNode callback, Function f
-) {
-  invk.isPartialArgument(callback, _, _) and
-  exists(AbstractFunction callee | callee = callback.getAValue() |
-    if callback.getAValue().isIndefinite("global")
-    then f = callee.getFunction() and f.getFile() = invk.getFile()
-    else f = callee.getFunction()
-  )
-}
-
-/**
- * Holds if `f` captures the variable defined by `def` in `cap`.
- */
-cached
-predicate captures(Function f, SsaExplicitDefinition def, SsaVariableCapture cap) {
-  def.getSourceVariable() = cap.getSourceVariable() and
-  f = cap.getContainer()
-}
-
 /**
  * Holds if `source` corresponds to an expression returned by `f`, and
  * `sink` equals `source`.
@@ -84,206 +54,255 @@ predicate localFlowStep(
 }
 
 /**
- * Holds if `arg` is passed as an argument into parameter `parm`
+ * Implements a set of data flow predicates that are used by multiple predicates and
- * through invocation `invk` of function `f`.
+ * hence should only be computed once.
  */
-predicate argumentPassing(
+cached
-  DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::ParameterNode parm
+private module CachedSteps {
-) {
+  /**
-  calls(invk, f) and
+   * Holds if `f` captures the variable defined by `def` in `cap`.
-  exists(int i |
+   */
-    f.getParameter(i) = parm.getParameter() and
+  cached
-    not parm.isRestParameter() and
+  predicate captures(Function f, SsaExplicitDefinition def, SsaVariableCapture cap) {
-    arg = invk.getArgument(i)
+    def.getSourceVariable() = cap.getSourceVariable() and
-  )
+    f = cap.getContainer()
-  or
+  }
-  exists(DataFlow::Node callback, int i |
-    invk.(DataFlow::AdditionalPartialInvokeNode).isPartialArgument(callback, arg, i) and
-    partiallyCalls(invk, callback, f) and
-    parm.getParameter() = f.getParameter(i) and
-    not parm.isRestParameter()
-  )
-}
 
-/**
+  /**
- * Holds if there is a flow step from `pred` to `succ` through parameter passing
+   * Holds if `invk` may invoke `f`.
- * to a function call.
+   */
- */
+  cached
-predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { argumentPassing(_, pred, _, succ) }
+  predicate calls(DataFlow::InvokeNode invk, Function f) { f = invk.getACallee(0) }
 
-/**
+  /**
- * Holds if there is a flow step from `pred` to `succ` through returning
+   * Holds if `invk` may invoke `f` indirectly through the given `callback` argument.
- * from a function call or the receiver flowing out of a constructor call.
+   *
- */
+   * This only holds for explicitly modeled partial calls.
-predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
+   */
-  exists(Function f | calls(succ, f) |
+  private predicate partiallyCalls(
-    returnExpr(f, pred, _)
+    DataFlow::AdditionalPartialInvokeNode invk, DataFlow::AnalyzedNode callback, Function f
+  ) {
+    invk.isPartialArgument(callback, _, _) and
+    exists(AbstractFunction callee | callee = callback.getAValue() |
+      if callback.getAValue().isIndefinite("global")
+      then f = callee.getFunction() and f.getFile() = invk.getFile()
+      else f = callee.getFunction()
+    )
+  }
+
+  /**
+   * Holds if `arg` is passed as an argument into parameter `parm`
+   * through invocation `invk` of function `f`.
+   */
+  cached
+  predicate argumentPassing(
+    DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::ParameterNode parm
+  ) {
+    calls(invk, f) and
+    exists(int i |
+      f.getParameter(i) = parm.getParameter() and
+      not parm.isRestParameter() and
+      arg = invk.getArgument(i)
+    )
     or
-    succ instanceof DataFlow::NewNode and
+    exists(DataFlow::Node callback, int i |
-    DataFlow::thisNode(pred, f)
+      invk.(DataFlow::AdditionalPartialInvokeNode).isPartialArgument(callback, arg, i) and
-  )
+      partiallyCalls(invk, callback, f) and
-}
+      parm.getParameter() = f.getParameter(i) and
+      not parm.isRestParameter()
+    )
+  }
 
-/**
+  /**
- * Holds if there is an assignment to property `prop` of an object represented by `obj`
+   * Holds if there is a flow step from `pred` to `succ` through parameter passing
- * with right hand side `rhs` somewhere, and properties of `obj` should be tracked.
+   * to a function call.
- */
+   */
-pragma[noinline]
+  cached
-private predicate trackedPropertyWrite(AbstractValue obj, string prop, DataFlow::Node rhs) {
+  predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { argumentPassing(_, pred, _, succ) }
-  exists(AnalyzedPropertyWrite pw |
-    pw.writes(obj, prop, rhs) and
-    shouldTrackProperties(obj) and
-    // avoid introducing spurious global flow
-    not pw.baseIsIncomplete("global")
-  )
-}
 
-/**
+  /**
- * Holds if there is a flow step from `pred` to `succ` through an object property.
+   * Holds if there is a flow step from `pred` to `succ` through returning
- */
+   * from a function call or the receiver flowing out of a constructor call.
-predicate propertyFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+   */
-  exists(AbstractValue obj, string prop |
+  cached
-    trackedPropertyWrite(obj, prop, pred) and
+  predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
-    succ.(AnalyzedPropertyRead).reads(obj, prop)
+    exists(Function f | calls(succ, f) |
-  )
+      returnExpr(f, pred, _)
-}
+      or
+      succ instanceof DataFlow::NewNode and
+      DataFlow::thisNode(pred, f)
+    )
+  }
 
-/**
+  /**
- * Gets a node whose value is assigned to `gv` in `f`.
+   * Holds if there is an assignment to property `prop` of an object represented by `obj`
- */
+   * with right hand side `rhs` somewhere, and properties of `obj` should be tracked.
-pragma[noinline]
+   */
-private DataFlow::ValueNode getADefIn(GlobalVariable gv, File f) {
+  pragma[noinline]
-  exists(VarDef def |
+  private predicate trackedPropertyWrite(AbstractValue obj, string prop, DataFlow::Node rhs) {
-    def.getFile() = f and
+    exists(AnalyzedPropertyWrite pw |
-    def.getTarget() = gv.getAReference() and
+      pw.writes(obj, prop, rhs) and
-    result = DataFlow::valueNode(def.getSource())
+      shouldTrackProperties(obj) and
-  )
+      // avoid introducing spurious global flow
-}
+      not pw.baseIsIncomplete("global")
+    )
+  }
 
-/**
+  /**
- * Gets a use of `gv` in `f`.
+   * Holds if there is a flow step from `pred` to `succ` through an object property.
- */
+   */
-pragma[noinline]
+  cached
-DataFlow::ValueNode getAUseIn(GlobalVariable gv, File f) {
+  predicate propertyFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
-  result.getFile() = f and
+    exists(AbstractValue obj, string prop |
-  result = DataFlow::valueNode(gv.getAnAccess())
+      trackedPropertyWrite(obj, prop, pred) and
-}
+      succ.(AnalyzedPropertyRead).reads(obj, prop)
+    )
+  }
 
-/**
+  /**
- * Holds if there is a flow step from `pred` to `succ` through a global
+   * Gets a node whose value is assigned to `gv` in `f`.
- * variable. Both `pred` and `succ` must be in the same file.
+   */
- */
+  pragma[noinline]
-predicate globalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+  private DataFlow::ValueNode getADefIn(GlobalVariable gv, File f) {
-  exists(GlobalVariable gv, File f |
+    exists(VarDef def |
-    pred = getADefIn(gv, f) and
+      def.getFile() = f and
-    succ = getAUseIn(gv, f)
+      def.getTarget() = gv.getAReference() and
-  )
+      result = DataFlow::valueNode(def.getSource())
-}
+    )
+  }
 
-/**
+  /**
- * Holds if there is a write to property `prop` of global variable `gv`
+   * Gets a use of `gv` in `f`.
- * in file `f`, where the right-hand side of the write is `rhs`.
+   */
- */
+  pragma[noinline]
-pragma[noinline]
+  private DataFlow::ValueNode getAUseIn(GlobalVariable gv, File f) {
-predicate globalPropertyWrite(GlobalVariable gv, File f, string prop, DataFlow::Node rhs) {
+    result.getFile() = f and
-  exists(DataFlow::PropWrite pw | pw.writes(getAUseIn(gv, f), prop, rhs))
+    result = DataFlow::valueNode(gv.getAnAccess())
-}
+  }
 
-/**
+  /**
- * Holds if there is a read from property `prop` of `base`, which is
+   * Holds if there is a flow step from `pred` to `succ` through a global
- * an access to global variable `base` in file `f`.
+   * variable. Both `pred` and `succ` must be in the same file.
- */
+   */
-pragma[noinline]
+  cached
-predicate globalPropertyRead(GlobalVariable gv, File f, string prop, DataFlow::Node base) {
+  predicate globalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
-  exists(DataFlow::PropRead pr |
+    exists(GlobalVariable gv, File f |
-    base = getAUseIn(gv, f) and
+      pred = getADefIn(gv, f) and
-    pr.accesses(base, prop)
+      succ = getAUseIn(gv, f)
-  )
+    )
-}
+  }
 
-/**
+  /**
- * Holds if there is a store step from `pred` to `succ` under property `prop`,
+   * Holds if there is a write to property `prop` of global variable `gv`
- * that is, `succ` is the local source of the base of a write of property
+   * in file `f`, where the right-hand side of the write is `rhs`.
- * `prop` with right-hand side `pred`.
+   */
- *
+  pragma[noinline]
- * For example, for this code snippet:
+  private predicate globalPropertyWrite(GlobalVariable gv, File f, string prop, DataFlow::Node rhs) {
- *
+    exists(DataFlow::PropWrite pw | pw.writes(getAUseIn(gv, f), prop, rhs))
- * ```
+  }
- * var a = new A();
- * a.p = e;
- * ```
- *
- * there is a store step from `e` to `new A()` under property `prop`.
- *
- * As a special case, if the base of the property write is a global variable,
- * then there is a store step from the right-hand side of the write to any
- * read of the same property from the same global variable in the same file.
- */
-predicate basicStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-  succ.(DataFlow::SourceNode).hasPropertyWrite(prop, pred)
-  or
-  exists(GlobalVariable gv, File f |
-    globalPropertyWrite(gv, f, prop, pred) and
-    globalPropertyRead(gv, f, prop, succ)
-  )
-}
 
-/**
+  /**
- * Holds if there is a load step from `pred` to `succ` under property `prop`,
+   * Holds if there is a read from property `prop` of `base`, which is
- * that is, `succ` is a read of property `prop` from `pred`.
+   * an access to global variable `base` in file `f`.
- */
+   */
-predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {
+  pragma[noinline]
-  succ.accesses(pred, prop)
+  private predicate globalPropertyRead(GlobalVariable gv, File f, string prop, DataFlow::Node base) {
-}
+    exists(DataFlow::PropRead pr |
+      base = getAUseIn(gv, f) and
+      pr.accesses(base, prop)
+    )
+  }
 
-/**
+  /**
- * Holds if there is a higher-order call with argument `arg`, and `cb` is the local
+   * Holds if there is a store step from `pred` to `succ` under property `prop`,
- * source of an argument that flows into the callee position of that call:
+   * that is, `succ` is the local source of the base of a write of property
- *
+   * `prop` with right-hand side `pred`.
- * ```
+   *
- * function f(x, g) {
+   * For example, for this code snippet:
- *   g(
+   *
- *     x                 // arg
+   * ```
- *   );
+   * var a = new A();
- * }
+   * a.p = e;
- *
+   * ```
- * function cb() {      // cb
+   *
- * }
+   * there is a store step from `e` to `new A()` under property `prop`.
- *
+   *
- * f(arg, cb);
+   * As a special case, if the base of the property write is a global variable,
- *
+   * then there is a store step from the right-hand side of the write to any
- * This is an over-approximation of a possible data flow step through a callback
+   * read of the same property from the same global variable in the same file.
- * invocation.
+   */
- */
+  cached
-predicate callback(DataFlow::Node arg, DataFlow::SourceNode cb) {
+  predicate basicStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-  exists(DataFlow::InvokeNode invk, DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
+    succ.(DataFlow::SourceNode).hasPropertyWrite(prop, pred)
-    arg = invk.getAnArgument() and
+    or
-    cbParm.flowsTo(invk.getCalleeNode()) and
+    exists(GlobalVariable gv, File f |
-    callStep(cbArg, cbParm) and
+      globalPropertyWrite(gv, f, prop, pred) and
-    cb.flowsTo(cbArg)
+      globalPropertyRead(gv, f, prop, succ)
-  )
+    )
-  or
+  }
-  exists(DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
-    callback(arg, cbParm) and
-    callStep(cbArg, cbParm) and
-    cb.flowsTo(cbArg)
-  )
-}
 
-/**
+  /**
- * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`.
+   * Holds if there is a load step from `pred` to `succ` under property `prop`,
- */
+   * that is, `succ` is a read of property `prop` from `pred`.
-predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, DataFlow::Node rhs) {
+   */
-  base.hasPropertyWrite(prop, rhs) and
+  cached
-  base.flowsToExpr(f.getAReturnedExpr())
+  predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {
-}
+    succ.accesses(pred, prop)
+  }
 
-/**
+  /**
- * Holds if `f` may assign `rhs` to `this.prop`.
+   * Holds if there is a higher-order call with argument `arg`, and `cb` is the local
- */
+   * source of an argument that flows into the callee position of that call:
-predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) {
+   *
-  DataFlow::thisNode(f).hasPropertyWrite(prop, rhs)
+   * ```
+   * function f(x, g) {
+   *   g(
+   *     x                 // arg
+   *   );
+   * }
+   *
+   * function cb() {      // cb
+   * }
+   *
+   * f(arg, cb);
+   *
+   * This is an over-approximation of a possible data flow step through a callback
+   * invocation.
+   */
+  cached
+  predicate callback(DataFlow::Node arg, DataFlow::SourceNode cb) {
+    exists(DataFlow::InvokeNode invk, DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
+      arg = invk.getAnArgument() and
+      cbParm.flowsTo(invk.getCalleeNode()) and
+      callStep(cbArg, cbParm) and
+      cb.flowsTo(cbArg)
+    )
+    or
+    exists(DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
+      callback(arg, cbParm) and
+      callStep(cbArg, cbParm) and
+      cb.flowsTo(cbArg)
+    )
+  }
+
+  /**
+   * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`.
+   */
+  cached
+  predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, DataFlow::Node rhs) {
+    base.hasPropertyWrite(prop, rhs) and
+    base.flowsToExpr(f.getAReturnedExpr())
+  }
+
+  /**
+   * Holds if `f` may assign `rhs` to `this.prop`.
+   */
+  cached
+  predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) {
+    DataFlow::thisNode(f).hasPropertyWrite(prop, rhs)
+  }
 }
+import CachedSteps
 
 /**
  * A utility class that is equivalent to `boolean` but does not require type joining.

javascript/ql/src/semmle/javascript/frameworks/Connect.qll

@@ -118,8 +118,16 @@ module Connect {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result.(DataFlow::SourceNode).flowsTo(getARouteHandlerExpr().flow()) or
+      result = getARouteHandler(DataFlow::TypeBackTracker::end())
-      result.(DataFlow::TrackedNode).flowsTo(getARouteHandlerExpr().flow())
+    }
+
+    private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = getARouteHandlerExpr().flow().getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker t2 |
+        result = getARouteHandler(t2).backtrack(t2, t)
+      )
     }
 
     override Expr getServer() { result = server }
@@ -169,24 +177,13 @@ module Connect {
   }
 
   /**
-   * Tracking for `RouteHandlerCandidate`.
+   * A function that flows to a route setup.
-   */
-  private class TrackedRouteHandlerCandidate extends DataFlow::TrackedNode {
-    TrackedRouteHandlerCandidate() { this instanceof RouteHandlerCandidate }
-  }
-
-  /**
-   * A function that looks like a Connect route handler and flows to a route setup.
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
-    HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
+    HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
-    override Function astNode;
 
     TrackedRouteHandlerCandidateWithSetup() {
-      exists(TrackedRouteHandlerCandidate tracked |
+      this = any(RouteSetup s).getARouteHandler()
-        tracked.flowsTo(any(RouteSetup s).getARouteHandlerExpr().flow()) and
-        this = tracked
-      )
     }
 
     override SimpleParameter getRouteHandlerParameter(string kind) {

javascript/ql/src/semmle/javascript/frameworks/Electron.qll

@@ -16,7 +16,7 @@ module Electron {
   /**
    * An instantiation of `BrowserWindow` or `BrowserView`.
    */
-  abstract private class NewBrowserObject extends BrowserObject, DataFlow::SourceNode {
+  abstract private class NewBrowserObject extends BrowserObject {
     DataFlow::NewNode self;
 
     NewBrowserObject() { this = self }
@@ -56,11 +56,20 @@ module Electron {
     }
   }
 
+  private DataFlow::SourceNode browserObject(DataFlow::TypeTracker t) {
+    t.start() and
+    result instanceof NewBrowserObject
+    or
+    exists(DataFlow::TypeTracker t2 |
+      result = browserObject(t2).track(t2, t)
+    )
+  }
+
   /**
    * A data flow node whose value may originate from a browser object instantiation.
    */
   private class BrowserObjectByFlow extends BrowserObject {
-    BrowserObjectByFlow() { any(NewBrowserObject nbo).flowsTo(this) }
+    BrowserObjectByFlow() { browserObject(DataFlow::TypeTracker::end()).flowsTo(this) }
   }
 
   /**

javascript/ql/src/semmle/javascript/frameworks/Express.qll

@@ -111,8 +111,16 @@ module Express {
     Expr getLastRouteHandlerExpr() { result = max(int i | | getRouteHandlerExpr(i) order by i) }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result.(DataFlow::SourceNode).flowsTo(getARouteHandlerExpr().flow()) or
+      result = getARouteHandler(DataFlow::TypeBackTracker::end())
-      result.(DataFlow::TrackedNode).flowsTo(getARouteHandlerExpr().flow())
+    }
+
+    private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = getARouteHandlerExpr().flow().getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker t2 |
+        result = getARouteHandler(t2).backtrack(t2, t)
+      )
     }
 
     override Expr getServer() { result.(Application).getARouteHandler() = getARouteHandler() }
@@ -766,24 +774,13 @@ module Express {
   }
 
   /**
-   * Tracking for `RouteHandlerCandidate`.
+   * A function that flows to a route setup.
-   */
-  private class TrackedRouteHandlerCandidate extends DataFlow::TrackedNode {
-    TrackedRouteHandlerCandidate() { this instanceof RouteHandlerCandidate }
-  }
-
-  /**
-   * A function that looks like an Express route handler and flows to a route setup.
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
-    HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
+    HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
-    override Function astNode;
 
     TrackedRouteHandlerCandidateWithSetup() {
-      exists(TrackedRouteHandlerCandidate tracked |
+      this = any(RouteSetup s).getARouteHandler()
-        tracked.flowsTo(any(RouteSetup s).getARouteHandlerExpr().flow()) and
-        this = tracked
-      )
     }
 
     override SimpleParameter getRouteHandlerParameter(string kind) {

javascript/ql/src/semmle/javascript/frameworks/Firebase.qll

@@ -0,0 +1,296 @@
+/**
+ * Provides classes and predicates for reasoning about code using the Firebase API.
+ */
+import javascript
+
+module Firebase {
+  /** Gets a reference to the Firebase API object. */
+  private DataFlow::SourceNode firebase(DataFlow::TypeTracker t) {
+    t.start() and
+    (
+      result = DataFlow::moduleImport("firebase/app")
+      or
+      result = DataFlow::moduleImport("firebase-admin")
+      or
+      result = DataFlow::globalVarRef("firebase")
+    )
+    or
+    exists (DataFlow::TypeTracker t2 |
+      result = firebase(t2).track(t2, t)
+    )
+  }
+
+  /** Gets a reference to the `firebase/app` or `firebase-admin` API object. */
+  DataFlow::SourceNode firebase() {
+    result = firebase(DataFlow::TypeTracker::end())
+  }
+
+  /** Gets a reference to a Firebase app created with `initializeApp`. */
+  private DataFlow::SourceNode initApp(DataFlow::TypeTracker t) {
+    result = firebase().getAMethodCall("initializeApp") and t.start()
+    or
+    exists (DataFlow::TypeTracker t2 |
+      result = initApp(t2).track(t2, t)
+    )
+  }
+
+  /**
+   * Gets a reference to a Firebase app, either the `firebase` object or an
+   * app created explicitly with `initializeApp()`.
+   */
+  DataFlow::SourceNode app() {
+    result = firebase(DataFlow::TypeTracker::end()) or result = initApp(DataFlow::TypeTracker::end())
+  }
+
+  module Database {
+
+    /** Gets a reference to a Firebase database object, such as `firebase.database()`. */
+    private DataFlow::SourceNode database(DataFlow::TypeTracker t) {
+      result = app().getAMethodCall("database") and t.start()
+      or
+      exists (DataFlow::TypeTracker t2 |
+        result = database(t2).track(t2, t)
+      )
+    }
+
+    /** Gets a reference to a Firebase database object, such as `firebase.database()`. */
+    DataFlow::SourceNode database() {
+      result = database(DataFlow::TypeTracker::end())
+    }
+
+    /** Gets a node that refers to a `Reference` object, such as `firebase.database().ref()`. */
+    private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
+      t.start() and
+      (
+        exists (string name | result = database().getAMethodCall(name) |
+          name = "ref" or
+          name = "refFromURL"
+        )
+        or
+        exists (string name | result = ref().getAMethodCall(name) |
+          name = "push" or
+          name = "child"
+        )
+        or
+        exists (string name | result = ref().getAPropertyRead(name) |
+          name = "parent" or
+          name = "root"
+        )
+        or
+        result = snapshot().getAPropertyRead("ref")
+      )
+      or
+      exists (DataFlow::TypeTracker t2 |
+        result = ref(t2).track(t2, t)
+      )
+    }
+
+    /** Gets a node that refers to a `Reference` object, such as `firebase.database().ref()`. */
+    DataFlow::SourceNode ref() {
+      result = ref(DataFlow::TypeTracker::end())
+    }
+
+    /** Gets a node that refers to a `Query` or `Reference` object. */
+    private DataFlow::SourceNode query(DataFlow::TypeTracker t) {
+      t.start() and
+      (
+        result = ref(t) // a Reference can be used as a Query
+        or
+        exists (string name | result = query().getAMethodCall(name) |
+          name = "endAt" or
+          name = "limitTo" + any(string s) or
+          name = "orderBy" + any(string s) or
+          name = "startAt"
+        )
+      )
+      or
+      exists (DataFlow::TypeTracker t2 |
+        result = query(t2).track(t2, t)
+      )
+    }
+
+    /** Gets a node that refers to a `Query` or `Reference` object. */
+    DataFlow::SourceNode query() {
+      result = query(DataFlow::TypeTracker::end())
+    }
+
+    /**
+     * A call of form `query.on(...)` or `query.once(...)`.
+     */
+    class QueryListenCall extends DataFlow::MethodCallNode {
+      QueryListenCall() {
+        this = query().getAMethodCall() and
+        (getMethodName() = "on" or getMethodName() = "once")
+      }
+
+      /**
+       * Gets the argument in which the callback is passed.
+       */
+      DataFlow::Node getCallbackNode() {
+        result = getArgument(1)
+      }
+    }
+
+    /**
+     * Gets a node that is passed as the callback to a `Reference.transaction` call.
+     */
+    private DataFlow::SourceNode transactionCallback(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = ref().getAMethodCall("transaction").getArgument(0).getALocalSource()
+      or
+      exists (DataFlow::TypeBackTracker t2 |
+        result = transactionCallback(t2).backtrack(t2, t)
+      )
+    }
+
+    /**
+     * Gets a node that is passed as the callback to a `Reference.transaction` call.
+     */
+    DataFlow::SourceNode transactionCallback() {
+      result = transactionCallback(DataFlow::TypeBackTracker::end())
+    }
+  }
+
+  /**
+   * Provides predicates for reasoning about the the Firebase Cloud Functions API,
+   * sometimes referred to just as just "Firebase Functions".
+   */
+  module CloudFunctions {
+    /** Gets a reference to the Cloud Functions namespace. */
+    private DataFlow::SourceNode namespace(DataFlow::TypeTracker t) {
+      t.start() and
+      result = DataFlow::moduleImport("firebase-functions")
+      or
+      exists (DataFlow::TypeTracker t2 |
+        result = namespace(t2).track(t2, t)
+      )
+    }
+
+    /** Gets a reference to the Cloud Functions namespace. */
+    DataFlow::SourceNode namespace() {
+      result = namespace(DataFlow::TypeTracker::end())
+    }
+
+    /** Gets a reference to a Cloud Functions database object. */
+    private DataFlow::SourceNode database(DataFlow::TypeTracker t) {
+      t.start() and
+      result = namespace().getAPropertyRead("database")
+      or
+      exists (DataFlow::TypeTracker t2 |
+        result = database(t2).track(t2, t)
+      )
+    }
+
+    /** Gets a reference to a Cloud Functions database object. */
+    DataFlow::SourceNode database() {
+      result = database(DataFlow::TypeTracker::end())
+    }
+
+    /** Gets a data flow node holding a `RefBuilder` object. */
+    private DataFlow::SourceNode refBuilder(DataFlow::TypeTracker t) {
+      t.start() and
+      result = database().getAMethodCall("ref")
+      or
+      exists (DataFlow::TypeTracker t2 |
+        result = refBuilder(t2).track(t2, t)
+      )
+    }
+
+    /** Gets a data flow node holding a `RefBuilder` object. */
+    DataFlow::SourceNode ref() {
+      result = refBuilder(DataFlow::TypeTracker::end())
+    }
+
+    /** Gets a call that registers a listener on a `RefBuilder`, such as `ref.onCreate(...)`. */
+    class RefBuilderListenCall extends DataFlow::MethodCallNode {
+      RefBuilderListenCall() {
+        this = ref().getAMethodCall() and
+        getMethodName() = "on" + any(string s)
+      }
+
+      /**
+       * Gets the data flow node holding the listener callback.
+       */
+      DataFlow::Node getCallbackNode() {
+        result = getArgument(0)
+      }
+    }
+  }
+
+  /**
+   * Gets a value that will be invoked with a `DataSnapshot` value as its first parameter.
+   */
+  private DataFlow::SourceNode snapshotCallback(DataFlow::TypeBackTracker t) {
+    t.start() and
+    (
+      result = any(Database::QueryListenCall call).getCallbackNode().getALocalSource()
+      or
+      result = any(CloudFunctions::RefBuilderListenCall call).getCallbackNode().getALocalSource()
+    )
+    or
+    exists (DataFlow::TypeBackTracker t2 |
+      result = snapshotCallback(t2).backtrack(t2, t)
+    )
+  }
+
+  /**
+   * Gets a value that will be invoked with a `DataSnapshot` value as its first parameter.
+   */
+  DataFlow::SourceNode snapshotCallback() {
+    result = snapshotCallback(DataFlow::TypeBackTracker::end())
+  }
+
+  /**
+   * Gets a node that refers to a `DataSnapshot` value or a promise or `Change`
+   * object containing `DataSnapshot`s.
+   */
+  private DataFlow::SourceNode snapshot(DataFlow::TypeTracker t) {
+    t.start() and
+    (
+      result = snapshotCallback().(DataFlow::FunctionNode).getParameter(0)
+      or
+      result instanceof Database::QueryListenCall // returns promise
+      or
+      result = snapshot().getAMethodCall("child")
+      or
+      result = snapshot().getAMethodCall("forEach").getCallback(0).getParameter(0)
+      or
+      exists (string prop | result = snapshot().getAPropertyRead(prop) |
+        prop = "before" or // only defined on Change objects
+        prop = "after"
+      )
+    )
+    or
+    promiseTaintStep(snapshot(t), result)
+    or
+    exists (DataFlow::TypeTracker t2 |
+      result = snapshot(t2).track(t2, t)
+    )
+  }
+
+  /**
+   * Gets a node that refers to a `DataSnapshot` value, such as `x` in
+   * `firebase.database().ref().on('value', x => {...})`.
+   */
+  DataFlow::SourceNode snapshot() {
+    result = snapshot(DataFlow::TypeTracker::end())
+  }
+
+  /**
+   * A reference to a value obtained from a Firebase database.
+   */
+  class FirebaseVal extends RemoteFlowSource {
+    FirebaseVal() {
+      exists (string name | this = snapshot().getAMethodCall(name) |
+        name = "val" or
+        name = "exportVal"
+      )
+      or
+      this = Database::transactionCallback().(DataFlow::FunctionNode).getParameter(0)
+    }
+
+    override string getSourceType() {
+      result = "Firebase database"
+    }
+  }
+}

javascript/ql/src/semmle/javascript/frameworks/HTTP.qll

@@ -276,11 +276,20 @@ module HTTP {
      * a request object enters the flow graph, such as the request
      * parameter of a route handler.
      */
-    abstract class RequestSource extends DataFlow::TrackedNode {
+    abstract class RequestSource extends DataFlow::Node {
       /**
        * Gets the route handler that handles this request.
        */
       abstract RouteHandler getRouteHandler();
+
+      predicate flowsTo(DataFlow::Node nd) { ref(DataFlow::TypeTracker::end()).flowsTo(nd) }
+
+      private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
+        t.start() and
+        result = this
+        or
+        exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
+      }
     }
 
     /**
@@ -288,11 +297,20 @@ module HTTP {
      * a response object enters the flow graph, such as the response
      * parameter of a route handler.
      */
-    abstract class ResponseSource extends DataFlow::TrackedNode {
+    abstract class ResponseSource extends DataFlow::Node {
       /**
        * Gets the route handler that provides this response.
        */
       abstract RouteHandler getRouteHandler();
+
+      predicate flowsTo(DataFlow::Node nd) { ref(DataFlow::TypeTracker::end()).flowsTo(nd) }
+
+      private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
+        t.start() and
+        result = this
+        or
+        exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
+      }
     }
 
     /**

javascript/ql/src/semmle/javascript/frameworks/Hapi.qll

@@ -193,8 +193,16 @@ module Hapi {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result.(DataFlow::SourceNode).flowsTo(handler.flow()) or
+      result = getARouteHandler(DataFlow::TypeBackTracker::end())
-      result.(DataFlow::TrackedNode).flowsTo(handler.flow())
+    }
+
+    private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = handler.flow().getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker t2 |
+        result = getARouteHandler(t2).backtrack(t2, t)
+      )
     }
 
     Expr getRouteHandlerExpr() { result = handler }
@@ -223,25 +231,13 @@ module Hapi {
     }
   }
 
-  /**
-   * Tracking for `RouteHandlerCandidate`.
-   */
-  private class TrackedRouteHandlerCandidate extends DataFlow::TrackedNode {
-    TrackedRouteHandlerCandidate() { this instanceof RouteHandlerCandidate }
-  }
-
   /**
    * A function that looks like a Hapi route handler and flows to a route setup.
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
-    HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
+    HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
-    override Function astNode;
-
     TrackedRouteHandlerCandidateWithSetup() {
-      exists(TrackedRouteHandlerCandidate tracked |
+      this = any(RouteSetup s).getARouteHandler()
-        tracked.flowsTo(any(RouteSetup s).getRouteHandlerExpr().flow()) and
-        this = tracked
-      )
     }
   }
 }

javascript/ql/src/semmle/javascript/frameworks/Koa.qll

@@ -64,7 +64,7 @@ module Koa {
    * A Koa context source, that is, the context parameter of a
    * route handler, or a `this` access in a route handler.
    */
-  private class ContextSource extends DataFlow::TrackedNode {
+  private class ContextSource extends DataFlow::Node {
     RouteHandler rh;
 
     ContextSource() {
@@ -77,6 +77,19 @@ module Koa {
      * Gets the route handler that handles this request.
      */
     RouteHandler getRouteHandler() { result = rh }
+
+    predicate flowsTo(DataFlow::Node nd) {
+      ref(DataFlow::TypeTracker::end()).flowsTo(nd)
+    }
+
+    private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
+      t.start() and
+      result = this
+      or
+      exists(DataFlow::TypeTracker t2 |
+        result = ref(t2).track(t2, t)
+      )
+    }
   }
 
   /**

javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll

@@ -189,8 +189,16 @@ module NodeJSLib {
     }
 
     override DataFlow::SourceNode getARouteHandler() {
-      result.(DataFlow::SourceNode).flowsTo(handler.flow()) or
+      result = getARouteHandler(DataFlow::TypeBackTracker::end())
-      result.(DataFlow::TrackedNode).flowsTo(handler.flow())
+    }
+
+    private DataFlow::SourceNode getARouteHandler(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result = handler.flow().getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker t2 |
+        result = getARouteHandler(t2).backtrack(t2, t)
+      )
     }
 
     override Expr getServer() { result = server }
@@ -613,24 +621,12 @@ module NodeJSLib {
   }
 
   /**
-   * Tracking for `RouteHandlerCandidate`.
+   * A function that flows to a route setup.
-   */
-  private class TrackedRouteHandlerCandidate extends DataFlow::TrackedNode {
-    TrackedRouteHandlerCandidate() { this instanceof RouteHandlerCandidate }
-  }
-
-  /**
-   * A function that looks like a Node.js route handler and flows to a route setup.
    */
   private class TrackedRouteHandlerCandidateWithSetup extends RouteHandler,
-    HTTP::Servers::StandardRouteHandler, DataFlow::ValueNode {
+    HTTP::Servers::StandardRouteHandler, DataFlow::FunctionNode {
-    override Function astNode;
-
     TrackedRouteHandlerCandidateWithSetup() {
-      exists(TrackedRouteHandlerCandidate tracked |
+      this = any(RouteSetup s).getARouteHandler()
-        tracked.flowsTo(any(RouteSetup s).getRouteHandlerExpr().flow()) and
-        this = tracked
-      )
     }
   }
 

javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll

@@ -24,16 +24,19 @@ module SocketIO {
     result = DataFlow::moduleImport("socket.io").getAMemberCall("listen")
   }
 
-  /** A data flow node that may produce (that is, create or return) a socket.io server. */
+  /**
-  class ServerNode extends DataFlow::SourceNode {
+   * Gets a data flow node that may refer to the socket.io server created at `srv`.
-    ServerObject srv;
+   */
-
+  private DataFlow::SourceNode server(ServerObject srv, DataFlow::TypeTracker t) {
-    ServerNode() {
+    result = newServer() and
-      this = newServer() and
+    srv = MkServer(result) and
-      srv = MkServer(this)
+    t.start()
+    or
+    exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | pred = server(srv, t2) |
+      result = pred.track(t2, t)
       or
       // invocation of a chainable method
-      exists(ServerNode base, DataFlow::MethodCallNode mcn, string m |
+      exists(DataFlow::MethodCallNode mcn, string m |
         m = "adapter" or
         m = "attach" or
         m = "bind" or
@@ -44,96 +47,116 @@ module SocketIO {
         m = "serveClient" or
         m = "set"
       |
-        mcn = base.getAMethodCall(m) and
+        mcn = pred.getAMethodCall(m) and
         // exclude getter versions
         exists(mcn.getAnArgument()) and
-        this = mcn and
+        result = mcn and
-        srv = base.getServer()
+        t = t2
       )
-    }
+    )
+  }
+
+  /** A data flow node that may produce (that is, create or return) a socket.io server. */
+  class ServerNode extends DataFlow::SourceNode {
+    ServerObject srv;
+
+    ServerNode() { this = server(srv, DataFlow::TypeTracker::end()) }
 
     /** Gets the server to which this node refers. */
     ServerObject getServer() { result = srv }
   }
 
+  /**
+   * Gets the name of a chainable method on socket.io namespace objects, which servers forward
+   * to their default namespace.
+   */
+  private string namespaceChainableMethod() {
+    result = "binary" or
+    result = "clients" or
+    result = "compress" or
+    result = "emit" or
+    result = "in" or
+    result = "send" or
+    result = "to" or
+    result = "use" or
+    result = "write" or
+    result = EventEmitter::chainableMethod()
+  }
+
+  /**
+   * Gets a data flow node that may refer to the socket.io namespace created at `ns`.
+   */
+  private DataFlow::SourceNode namespace(NamespaceObject ns, DataFlow::TypeTracker t) {
+    t.start() and
+    exists(ServerNode srv |
+      // namespace lookup on `srv`
+      result = srv.getAPropertyRead("sockets") and
+      ns = srv.getServer().getDefaultNamespace()
+      or
+      exists(DataFlow::MethodCallNode mcn, string path |
+        mcn = srv.getAMethodCall("of") and
+        mcn.getArgument(0).mayHaveStringValue(path) and
+        result = mcn and
+        ns = MkNamespace(srv.getServer(), path)
+      )
+      or
+      // invocation of a method that `srv` forwards to its default namespace
+      result = srv.getAMethodCall(namespaceChainableMethod()) and
+      ns = srv.getServer().getDefaultNamespace()
+    )
+    or
+    exists(DataFlow::SourceNode pred, DataFlow::TypeTracker t2 | pred = namespace(ns, t2) |
+      result = pred.track(t2, t)
+      or
+      // invocation of a chainable method
+      result = pred.getAMethodCall(namespaceChainableMethod()) and
+      t = t2
+      or
+      // invocation of chainable getter method
+      exists(string m |
+        m = "json" or
+        m = "local" or
+        m = "volatile"
+      |
+        result = pred.getAPropertyRead(m) and
+        t = t2
+      )
+    )
+  }
+
   /** A data flow node that may produce a namespace object. */
   class NamespaceNode extends DataFlow::SourceNode {
     NamespaceObject ns;
 
-    NamespaceNode() {
+    NamespaceNode() { this = namespace(ns, DataFlow::TypeTracker::end()) }
-      // namespace lookup
-      exists(ServerNode srv |
-        this = srv.getAPropertyRead("sockets") and
-        ns = srv.getServer().getDefaultNamespace()
-        or
-        exists(DataFlow::MethodCallNode mcn, string path |
-          mcn = srv.getAMethodCall("of") and
-          mcn.getArgument(0).mayHaveStringValue(path) and
-          this = mcn and
-          ns = MkNamespace(srv.getServer(), path)
-        )
-      )
-      or
-      // invocation of a chainable method
-      exists(string m |
-        m = "binary" or
-        m = "clients" or
-        m = "compress" or
-        m = "emit" or
-        m = "in" or
-        m = "send" or
-        m = "to" or
-        m = "use" or
-        m = "write" or
-        m = EventEmitter::chainableMethod()
-      |
-        exists(NamespaceNode base |
-          this = base.getAMethodCall(m) and
-          ns = base.getNamespace()
-        )
-        or
-        // server objects forward these methods to their default namespace
-        exists(ServerNode srv |
-          this = srv.getAMethodCall(m) and
-          ns = srv.getServer().getDefaultNamespace()
-        )
-      )
-      or
-      // invocation of chainable getter method
-      exists(NamespaceNode base, string m |
-        m = "json" or
-        m = "local" or
-        m = "volatile"
-      |
-        this = base.getAPropertyRead(m) and
-        ns = base.getNamespace()
-      )
-    }
 
     /** Gets the namespace to which this node refers. */
     NamespaceObject getNamespace() { result = ns }
   }
 
-  /** A data flow node that may produce a socket object. */
+  /**
-  class SocketNode extends DataFlow::SourceNode {
+   * Gets a data flow node that may refer to a socket.io socket belonging to namespace `ns`.
-    NamespaceObject ns;
+   */
-
+  private DataFlow::SourceNode socket(NamespaceObject ns, DataFlow::TypeTracker t) {
-    SocketNode() {
+    // callback accepting a socket
-      // callback accepting a socket
+    t.start() and
-      exists(DataFlow::SourceNode base, string connect, DataFlow::MethodCallNode on |
+    exists(DataFlow::SourceNode base, string connect, DataFlow::MethodCallNode on |
-        (
+      (
-          ns = base.(ServerNode).getServer().getDefaultNamespace() or
+        ns = base.(ServerNode).getServer().getDefaultNamespace() or
-          ns = base.(NamespaceNode).getNamespace()
+        ns = base.(NamespaceNode).getNamespace()
-        ) and
+      ) and
-        (connect = "connect" or connect = "connection")
+      (connect = "connect" or connect = "connection")
-      |
+    |
-        on = base.getAMethodCall(EventEmitter::on()) and
+      on = base.getAMethodCall(EventEmitter::on()) and
-        on.getArgument(0).mayHaveStringValue(connect) and
+      on.getArgument(0).mayHaveStringValue(connect) and
-        this = on.getCallback(1).getParameter(0)
+      result = on.getCallback(1).getParameter(0)
-      )
+    )
+    or
+    exists(DataFlow::SourceNode pred, DataFlow::TypeTracker t2 | pred = socket(ns, t2) |
+      result = pred.track(t2, t)
       or
       // invocation of a chainable method
-      exists(SocketNode base, string m |
+      exists(string m |
         m = "binary" or
         m = "compress" or
         m = "disconnect" or
@@ -147,21 +170,28 @@ module SocketIO {
         m = "write" or
         m = EventEmitter::chainableMethod()
       |
-        this = base.getAMethodCall(m) and
+        result = pred.getAMethodCall(m) and
-        ns = base.getNamespace()
+        t = t2
       )
       or
       // invocation of a chainable getter method
-      exists(SocketNode base, string m |
+      exists(string m |
         m = "broadcast" or
         m = "json" or
         m = "local" or
         m = "volatile"
       |
-        this = base.getAPropertyRead(m) and
+        result = pred.getAPropertyRead(m) and
-        ns = base.getNamespace()
+        t = t2
       )
-    }
+    )
+  }
+
+  /** A data flow node that may produce a socket object. */
+  class SocketNode extends DataFlow::SourceNode {
+    NamespaceObject ns;
+
+    SocketNode() { this = socket(ns, DataFlow::TypeTracker::end()) }
 
     /** Gets the namespace to which this socket belongs. */
     NamespaceObject getNamespace() { result = ns }
@@ -361,21 +391,29 @@ module SocketIO {
  * (npm package `socket.io-client`).
  */
 module SocketIOClient {
+  /**
+   * Gets a data flow node that may refer to the socket.io socket created at `invk`.
+   */
+  private DataFlow::SourceNode socket(DataFlow::InvokeNode invk, DataFlow::TypeTracker t) {
+    t.start() and
+    exists(DataFlow::SourceNode io |
+      io = DataFlow::globalVarRef("io") or
+      io = DataFlow::globalVarRef("io").getAPropertyRead("connect") or
+      io = DataFlow::moduleImport("socket.io-client") or
+      io = DataFlow::moduleMember("socket.io-client", "connect")
+    |
+      invk = io.getAnInvocation() and
+      result = invk
+    )
+    or
+    exists(DataFlow::TypeTracker t2 | result = socket(invk, t2).track(t2, t))
+  }
+
   /** A data flow node that may produce a socket object. */
   class SocketNode extends DataFlow::SourceNode {
     DataFlow::InvokeNode invk;
 
-    SocketNode() {
+    SocketNode() { this = socket(invk, DataFlow::TypeTracker::end()) }
-      exists(DataFlow::SourceNode io |
-        io = DataFlow::globalVarRef("io") or
-        io = DataFlow::globalVarRef("io").getAPropertyRead("connect") or
-        io = DataFlow::moduleImport("socket.io-client") or
-        io = DataFlow::moduleMember("socket.io-client", "connect")
-      |
-        invk = io.getAnInvocation() and
-        this = invk
-      )
-    }
 
     /** Gets the path of the namespace this socket belongs to, if it can be determined. */
     string getNamespacePath() {

javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll

@@ -43,12 +43,13 @@ predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
  * - `?` (any suffix becomes part of query)
  * - `#` (any suffix becomes part of fragment)
  * - `/` or `\`, immediately prefixed by a character other than `:`, `/`, or `\` (any suffix becomes part of the path)
+ * - a leading `/` or `\` followed by a character other than `/` or `\` (any suffix becomes part of the path)
  *
- * In the latter case, the additional prefix check is necessary to avoid a `/` that could be interpreted as
+ * In the latter two cases, the additional check is necessary to avoid a `/` that could be interpreted as
  * the `//` separating the (optional) scheme from the hostname.
  */
 private predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
-  nd.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*")
+  nd.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*")
   or
   hasHostnameSanitizingSubstring(StringConcatenation::getAnOperand(nd))
   or

javascript/ql/test/library-tests/TypeTracking/ClassStyle.ql

@@ -6,86 +6,68 @@ string chainableMethod() {
 }
 
 class ApiObject extends DataFlow::NewNode {
-  ApiObject() {
+  ApiObject() { this = DataFlow::moduleImport("@test/myapi").getAnInstantiation() }
-    this = DataFlow::moduleImport("@test/myapi").getAnInstantiation()
-  }
 
   DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
     t.start() and
     result = this
     or
     t.start() and
-    result = ref(_).getAMethodCall(chainableMethod())
+    result = ref(DataFlow::TypeTracker::end()).getAMethodCall(chainableMethod())
     or
-    exists(DataFlow::TypeTracker t2 |
+    exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
-      result = ref(t2).track(t2, t)
-    )
-  }
-  
-  DataFlow::SourceNode ref() {
-    result = ref(_)
   }
+
+  DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
 }
 
 class Connection extends DataFlow::SourceNode {
   ApiObject api;
 
-  Connection() {
+  Connection() { this = api.ref().getAMethodCall("createConnection") }
-    this = api.ref().getAMethodCall("createConnection")
-  }
 
   DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
     t.start() and
     result = this
     or
-    exists(DataFlow::TypeTracker t2 |
+    exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
-      result = ref(t2).track(t2, t)
-    )
-  }
-  
-  DataFlow::SourceNode ref() {
-    result = ref(_)
   }
 
+  DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
+
   DataFlow::SourceNode getACallbackNode(DataFlow::TypeBackTracker t) {
     t.start() and
     result = ref().getAMethodCall("getData").getArgument(0).getALocalSource()
     or
-    exists(DataFlow::TypeBackTracker t2 |
+    exists(DataFlow::TypeBackTracker t2 | result = getACallbackNode(t2).backtrack(t2, t))
-      result = getACallbackNode(t2).backtrack(t2, t)
-    )
   }
 
   DataFlow::FunctionNode getACallback() {
-    result = getACallbackNode(_).getAFunctionValue()
+    result = getACallbackNode(DataFlow::TypeBackTracker::end()).getAFunctionValue()
   }
 }
 
 class DataValue extends DataFlow::SourceNode {
   Connection connection;
 
-  DataValue() {
+  DataValue() { this = connection.getACallback().getParameter(0) }
-    this = connection.getACallback().getParameter(0)
-  }
 
   DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
     t.start() and
     result = this
     or
-    exists(DataFlow::TypeTracker t2 |
+    exists(DataFlow::TypeTracker t2 | result = ref(t2).track(t2, t))
-      result = ref(t2).track(t2, t)
-    )
-  }
-  
-  DataFlow::SourceNode ref() {
-    result = ref(_)
   }
+
+  DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
 }
 
 query DataFlow::SourceNode test_ApiObject() { result = any(ApiObject obj).ref() }
 
 query DataFlow::SourceNode test_Connection() { result = any(Connection c).ref() }
 
-query DataFlow::SourceNode test_DataCallback() { result = any(Connection c).getACallbackNode(_) }
+query DataFlow::SourceNode test_DataCallback() {
+  result = any(Connection c).getACallbackNode(DataFlow::TypeBackTracker::end())
+}
 
 query DataFlow::SourceNode test_DataValue() { result = any(DataValue v).ref() }

javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected

@@ -5,6 +5,7 @@ apiObject
 connection
 | type tracker with call steps | tst.js:6:15:6:18 | conn |
 | type tracker with call steps | tst.js:10:5:10:19 | this.connection |
+| type tracker with call steps with property connection | tst.js:6:14:6:13 | this |
 | type tracker without call steps | tst.js:15:10:15:49 | api.cha ... ction() |
 | type tracker without call steps | tst.js:18:7:18:21 | getConnection() |
 | type tracker without call steps | tst.js:30:9:30:23 | getConnection() |

javascript/ql/test/library-tests/TypeTracking/PredicateStyle.ql

@@ -10,52 +10,38 @@ DataFlow::SourceNode apiObject(DataFlow::TypeTracker t) {
   result = DataFlow::moduleImport("@test/myapi").getAnInstantiation()
   or
   t.start() and
-  result = apiObject(_).getAMethodCall(chainableMethod())
+  result = apiObject(DataFlow::TypeTracker::end()).getAMethodCall(chainableMethod())
   or
-  exists(DataFlow::TypeTracker t2 |
+  exists(DataFlow::TypeTracker t2 | result = apiObject(t2).track(t2, t))
-    result = apiObject(t2).track(t2, t)
-  )
 }
 
-query DataFlow::SourceNode apiObject() {
+query DataFlow::SourceNode apiObject() { result = apiObject(DataFlow::TypeTracker::end()) }
-  result = apiObject(_)
-}
 
 query DataFlow::SourceNode connection(DataFlow::TypeTracker t) {
   t.start() and
   result = apiObject().getAMethodCall("createConnection")
   or
-  exists(DataFlow::TypeTracker t2 |
+  exists(DataFlow::TypeTracker t2 | result = connection(t2).track(t2, t))
-    result = connection(t2).track(t2, t)
-  )
 }
 
-DataFlow::SourceNode connection() {
+DataFlow::SourceNode connection() { result = connection(DataFlow::TypeTracker::end()) }
-  result = connection(_)
-}
 
 DataFlow::SourceNode dataCallback(DataFlow::TypeBackTracker t) {
   t.start() and
   result = connection().getAMethodCall("getData").getArgument(0).getALocalSource()
   or
-  exists(DataFlow::TypeBackTracker t2 |
+  exists(DataFlow::TypeBackTracker t2 | result = dataCallback(t2).backtrack(t2, t))
-    result = dataCallback(t2).backtrack(t2, t)
-  )
 }
 
 query DataFlow::SourceNode dataCallback() {
-  result = dataCallback(_)
+  result = dataCallback(DataFlow::TypeBackTracker::end())
 }
 
 DataFlow::SourceNode dataValue(DataFlow::TypeTracker t) {
   t.start() and
   result = dataCallback().getAFunctionValue().getParameter(0)
   or
-  exists(DataFlow::TypeTracker t2 |
+  exists(DataFlow::TypeTracker t2 | result = dataValue(t2).track(t2, t))
-    result = dataValue(t2).track(t2, t)
-  )
 }
 
-query DataFlow::SourceNode dataValue() {
+query DataFlow::SourceNode dataValue() { result = dataValue(DataFlow::TypeTracker::end()) }
-  result = dataValue(_)
-}

javascript/ql/test/library-tests/frameworks/Electron/BrowserObject.expected

@@ -4,6 +4,8 @@
 | electron.js:3:10:3:48 | new Bro ... s: {}}) |
 | electron.js:4:5:4:46 | bv |
 | electron.js:4:10:4:46 | new Bro ... s: {}}) |
+| electron.js:35:14:35:14 | x |
+| electron.js:36:12:36:12 | x |
 | electron.js:39:5:39:6 | bw |
 | electron.js:40:5:40:6 | bv |
 | electron.ts:3:12:3:13 | bw |

javascript/ql/test/library-tests/frameworks/Firebase/FirebaseRef.expected

@@ -0,0 +1,17 @@
+| tst.js:5:1:5:22 | fb.data ... ef('x') |
+| tst.js:7:3:7:7 | x.ref |
+| tst.js:7:3:7:14 | x.ref.parent |
+| tst.js:10:1:10:25 | admin.d ... ef('x') |
+| tst.js:12:3:12:7 | x.ref |
+| tst.js:12:3:12:14 | x.ref.parent |
+| tst.js:17:3:17:7 | x.ref |
+| tst.js:17:3:17:14 | x.ref.parent |
+| tst.js:23:3:23:7 | x.ref |
+| tst.js:23:3:23:14 | x.ref.parent |
+| tst.js:32:12:32:42 | this.fi ... .ref(x) |
+| tst.js:46:12:46:42 | this.fi ... .ref(x) |
+| tst.js:50:12:50:25 | this.getRef(x) |
+| tst.js:50:12:50:34 | this.ge ... hild(x) |
+| tst.js:54:5:54:37 | this.fi ... ef('x') |
+| tst.js:58:1:58:61 | new Fir ... /news') |
+| tst.js:59:1:59:38 | new Fir ... /news') |

javascript/ql/test/library-tests/frameworks/Firebase/FirebaseRef.ql

@@ -0,0 +1,3 @@
+import javascript
+
+select Firebase::Database::ref()

javascript/ql/test/library-tests/frameworks/Firebase/FirebaseSnapshot.expected

@@ -0,0 +1,10 @@
+| tst.js:5:1:8:2 | fb.data ... ent;\\n}) |
+| tst.js:5:38:5:38 | x |
+| tst.js:10:1:13:2 | admin.d ... ent;\\n}) |
+| tst.js:10:41:10:41 | x |
+| tst.js:15:38:15:38 | x |
+| tst.js:20:38:20:38 | x |
+| tst.js:21:3:21:10 | x.before |
+| tst.js:22:3:22:9 | x.after |
+| tst.js:50:12:50:48 | this.ge ... value') |
+| tst.js:60:1:60:39 | new Fir ... em('x') |

javascript/ql/test/library-tests/frameworks/Firebase/FirebaseSnapshot.ql

@@ -0,0 +1,3 @@
+import javascript
+
+select Firebase::snapshot()

javascript/ql/test/library-tests/frameworks/Firebase/FirebaseVal.expected

@@ -0,0 +1,6 @@
+| tst.js:6:3:6:9 | x.val() |
+| tst.js:11:3:11:9 | x.val() |
+| tst.js:16:3:16:9 | x.val() |
+| tst.js:21:3:21:16 | x.before.val() |
+| tst.js:22:3:22:15 | x.after.val() |
+| tst.js:61:36:61:36 | x |

javascript/ql/test/library-tests/frameworks/Firebase/FirebaseVal.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from Firebase::FirebaseVal val
+select val

javascript/ql/test/library-tests/frameworks/Firebase/tst.js

@@ -0,0 +1,70 @@
+import * as fb from 'firebase/app';
+import * as admin from 'firebase-admin';
+import * as functions from 'firebase-functions';
+
+fb.database().ref('x').once('value', x => {
+  x.val();
+  x.ref.parent;
+});
+
+admin.database().ref('x').once('value', x => {
+  x.val();
+  x.ref.parent;
+});
+
+functions.database.ref('x').onCreate(x => {
+  x.val();
+  x.ref.parent;
+});
+
+functions.database.ref('x').onUpdate(x => {
+  x.before.val();
+  x.after.val();
+  x.ref.parent;
+});
+
+class FirebaseWrapper {
+  constructor(firebase) {
+    this.firebase = firebase;
+  }
+
+  getRef(x) {
+    return this.firebase.database().ref(x);
+  }
+}
+
+class FirebaseWrapper2 {
+  constructor() {
+    this.init();
+  }
+
+  init() {
+    this.firebase = fb.initializeApp();
+  }
+
+  getRef(x) {
+    return this.firebase.database().ref(x);
+  }
+
+  getNewsItem(x) {
+    return this.getRef(x).child(x).once('value');
+  }
+
+  adjustValue(fn) {
+    this.firebase.database().ref('x').transaction(fn);
+  }
+}
+
+new FirebaseWrapper(firebase.initializeApp()).getRef('/news');
+new FirebaseWrapper2().getRef('/news');
+new FirebaseWrapper2().getNewsItem('x');
+new FirebaseWrapper2().adjustValue(x => x + 1);
+
+class Box {
+  constructor(x) {
+    this.x = x;
+  }
+}
+let box1 = new Box(fb.database());
+let box2 = new Box(whatever());
+box2.x.ref(); // not a firebase ref

javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteHandlerCandidate.expected

@@ -3,6 +3,9 @@
 | src/hapi.js:1:1:1:30 | functio ... t, h){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/iterated-handlers.js:4:2:4:22 | functio ...  res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/middleware-attacher-getter.js:29:32:29:51 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/nodejs.js:5:22:5:41 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/nodejs.js:11:23:11:42 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/nodejs.js:12:25:12:44 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:7:19:7:38 | function(req, res){} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:8:12:10:5 | (req, res) {\\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:20:16:22:9 | (req, r ...       } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
@@ -10,8 +13,20 @@
 | src/route-objects.js:40:12:42:5 | (req, res) {\\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:50:12:52:5 | (req, res) {\\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/route-objects.js:56:12:58:5 | functio ... ;\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:6:32:6:52 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:8:32:8:61 | functio ... nse) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:30:36:30:56 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:33:18:33:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:34:18:34:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:37:5:37:25 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:38:5:38:25 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:43:18:43:38 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:46:1:46:23 | functio ... res) {} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:52:1:54:1 | functio ... req()\\n} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:61:1:63:1 | functio ... turn;\\n} | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:70:5:72:5 | functio ... \\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:79:5:81:5 | functio ... \\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:84:5:86:5 | functio ... \\n\\n    } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
 | src/tst.js:109:16:111:9 | functio ...       } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:124:16:126:9 | functio ...       } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |
+| src/tst.js:132:16:134:9 | functio ...       } | A `RouteHandlerCandidate` that did not get promoted to `RouteHandler`, and it is not used in a `RouteSetupCandidate`. |

javascript/ql/test/library-tests/frameworks/HTTP-heuristics/UnpromotedRouteSetupCandidate.expected

@@ -1,17 +0,0 @@
-| src/nodejs.js:5:1:5:42 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/nodejs.js:11:1:11:43 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/nodejs.js:12:1:12:45 | unknown ... res){}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:6:1:6:53 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:8:1:8:62 | someOth ... se) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:30:1:30:57 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:32:1:34:39 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:36:1:39:2 | someOth ... ) {}\\n]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:41:1:43:39 | someOth ... es) {}) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:87:5:87:57 | unknown ... cSetup) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:96:5:96:36 | unknown ... h', rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:98:5:98:38 | unknown ... , [rh]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:104:5:104:45 | unknown ... wn, rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:137:5:137:57 | unknown ... cSetup) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:149:5:149:36 | unknown ... h', rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:151:5:151:38 | unknown ... , [rh]) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |
-| src/tst.js:157:5:157:45 | unknown ... wn, rh) | A `RouteSetupCandidate` that did not get promoted to `RouteSetup`, and it is using at least one `RouteHandlerCandidate`. |

javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected

@@ -22,6 +22,18 @@ nodes
 | tst9.js:2:21:2:37 | document.location |
 | tst9.js:2:21:2:42 | documen ... on.hash |
 | tst9.js:2:21:2:55 | documen ... ring(1) |
+| tst10.js:5:17:5:46 | '/' + d ... .search |
+| tst10.js:5:23:5:39 | document.location |
+| tst10.js:5:23:5:46 | documen ... .search |
+| tst10.js:8:17:8:47 | '//' +  ... .search |
+| tst10.js:8:24:8:40 | document.location |
+| tst10.js:8:24:8:47 | documen ... .search |
+| tst10.js:11:17:11:50 | '//foo' ... .search |
+| tst10.js:11:27:11:43 | document.location |
+| tst10.js:11:27:11:50 | documen ... .search |
+| tst10.js:14:17:14:56 | 'https: ... .search |
+| tst10.js:14:33:14:49 | document.location |
+| tst10.js:14:33:14:56 | documen ... .search |
 | tst.js:2:19:2:69 | /.*redi ... n.href) |
 | tst.js:2:19:2:72 | /.*redi ... ref)[1] |
 | tst.js:2:47:2:63 | document.location |
@@ -46,6 +58,14 @@ edges
 | tst9.js:2:21:2:37 | document.location | tst9.js:2:21:2:42 | documen ... on.hash |
 | tst9.js:2:21:2:42 | documen ... on.hash | tst9.js:2:21:2:55 | documen ... ring(1) |
 | tst9.js:2:21:2:55 | documen ... ring(1) | tst9.js:2:21:2:37 | document.location |
+| tst10.js:5:23:5:39 | document.location | tst10.js:5:23:5:46 | documen ... .search |
+| tst10.js:5:23:5:46 | documen ... .search | tst10.js:5:17:5:46 | '/' + d ... .search |
+| tst10.js:8:24:8:40 | document.location | tst10.js:8:24:8:47 | documen ... .search |
+| tst10.js:8:24:8:47 | documen ... .search | tst10.js:8:17:8:47 | '//' +  ... .search |
+| tst10.js:11:27:11:43 | document.location | tst10.js:11:27:11:50 | documen ... .search |
+| tst10.js:11:27:11:50 | documen ... .search | tst10.js:11:17:11:50 | '//foo' ... .search |
+| tst10.js:14:33:14:49 | document.location | tst10.js:14:33:14:56 | documen ... .search |
+| tst10.js:14:33:14:56 | documen ... .search | tst10.js:14:17:14:56 | 'https: ... .search |
 | tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] |
 | tst.js:2:47:2:63 | document.location | tst.js:2:47:2:68 | documen ... on.href |
 | tst.js:2:47:2:68 | documen ... on.href | tst.js:2:19:2:69 | /.*redi ... n.href) |
@@ -59,4 +79,8 @@ edges
 | tst7.js:5:27:5:50 | documen ... .search | tst7.js:5:27:5:43 | document.location | tst7.js:5:27:5:50 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:5:27:5:43 | document.location | user-provided value |
 | tst9.js:2:21:2:55 | documen ... ring(1) | tst9.js:2:21:2:37 | document.location | tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:37 | document.location | user-provided value |
 | tst9.js:2:21:2:55 | documen ... ring(1) | tst9.js:2:21:2:37 | document.location | tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:37 | document.location | user-provided value |
+| tst10.js:5:17:5:46 | '/' + d ... .search | tst10.js:5:23:5:39 | document.location | tst10.js:5:17:5:46 | '/' + d ... .search | Untrusted URL redirection due to $@. | tst10.js:5:23:5:39 | document.location | user-provided value |
+| tst10.js:8:17:8:47 | '//' +  ... .search | tst10.js:8:24:8:40 | document.location | tst10.js:8:17:8:47 | '//' +  ... .search | Untrusted URL redirection due to $@. | tst10.js:8:24:8:40 | document.location | user-provided value |
+| tst10.js:11:17:11:50 | '//foo' ... .search | tst10.js:11:27:11:43 | document.location | tst10.js:11:17:11:50 | '//foo' ... .search | Untrusted URL redirection due to $@. | tst10.js:11:27:11:43 | document.location | user-provided value |
+| tst10.js:14:17:14:56 | 'https: ... .search | tst10.js:14:33:14:49 | document.location | tst10.js:14:17:14:56 | 'https: ... .search | Untrusted URL redirection due to $@. | tst10.js:14:33:14:49 | document.location | user-provided value |
 | tst.js:2:19:2:72 | /.*redi ... ref)[1] | tst.js:2:47:2:63 | document.location | tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:63 | document.location | user-provided value |

javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst10.js

@@ -0,0 +1,14 @@
+// OK - cannot affect hostname
+location.href = '/foo' + document.location.search;
+
+// NOT OK
+location.href = '/' + document.location.search;
+
+// NOT OK
+location.href = '//' + document.location.search;
+
+// NOT OK
+location.href = '//foo' + document.location.search;
+
+// NOT OK
+location.href = 'https://foo' + document.location.search;

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected

@@ -22,12 +22,6 @@ nodes
 | express.js:135:23:135:37 | req.params.user |
 | express.js:136:16:136:36 | 'u' + r ... ms.user |
 | express.js:136:22:136:36 | req.params.user |
-| express.js:138:16:138:45 | '/' + ( ... s.user) |
-| express.js:138:22:138:45 | ('/u' + ... s.user) |
-| express.js:138:23:138:44 | '/u' +  ... ms.user |
-| express.js:138:30:138:44 | req.params.user |
-| express.js:139:16:139:37 | '/u' +  ... ms.user |
-| express.js:139:23:139:37 | req.params.user |
 | node.js:6:7:6:52 | target |
 | node.js:6:16:6:39 | url.par ... , true) |
 | node.js:6:16:6:45 | url.par ... ).query |
@@ -66,10 +60,6 @@ edges
 | express.js:134:22:134:36 | req.params.user | express.js:134:16:134:36 | '/' + r ... ms.user |
 | express.js:135:23:135:37 | req.params.user | express.js:135:16:135:37 | '//' +  ... ms.user |
 | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user |
-| express.js:138:22:138:45 | ('/u' + ... s.user) | express.js:138:16:138:45 | '/' + ( ... s.user) |
-| express.js:138:23:138:44 | '/u' +  ... ms.user | express.js:138:22:138:45 | ('/u' + ... s.user) |
-| express.js:138:30:138:44 | req.params.user | express.js:138:23:138:44 | '/u' +  ... ms.user |
-| express.js:139:23:139:37 | req.params.user | express.js:139:16:139:37 | '/u' +  ... ms.user |
 | node.js:6:7:6:52 | target | node.js:7:34:7:39 | target |
 | node.js:6:16:6:39 | url.par ... , true) | node.js:6:16:6:45 | url.par ... ).query |
 | node.js:6:16:6:45 | url.par ... ).query | node.js:6:16:6:52 | url.par ... .target |
@@ -105,8 +95,6 @@ edges
 | express.js:134:16:134:36 | '/' + r ... ms.user | express.js:134:22:134:36 | req.params.user | express.js:134:16:134:36 | '/' + r ... ms.user | Untrusted URL redirection due to $@. | express.js:134:22:134:36 | req.params.user | user-provided value |
 | express.js:135:16:135:37 | '//' +  ... ms.user | express.js:135:23:135:37 | req.params.user | express.js:135:16:135:37 | '//' +  ... ms.user | Untrusted URL redirection due to $@. | express.js:135:23:135:37 | req.params.user | user-provided value |
 | express.js:136:16:136:36 | 'u' + r ... ms.user | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user | Untrusted URL redirection due to $@. | express.js:136:22:136:36 | req.params.user | user-provided value |
-| express.js:138:16:138:45 | '/' + ( ... s.user) | express.js:138:30:138:44 | req.params.user | express.js:138:16:138:45 | '/' + ( ... s.user) | Untrusted URL redirection due to $@. | express.js:138:30:138:44 | req.params.user | user-provided value |
-| express.js:139:16:139:37 | '/u' +  ... ms.user | express.js:139:23:139:37 | req.params.user | express.js:139:16:139:37 | '/u' +  ... ms.user | Untrusted URL redirection due to $@. | express.js:139:23:139:37 | req.params.user | user-provided value |
 | node.js:7:34:7:39 | target | node.js:6:26:6:32 | req.url | node.js:7:34:7:39 | target | Untrusted URL redirection due to $@. | node.js:6:26:6:32 | req.url | user-provided value |
 | node.js:15:34:15:45 | '/' + target | node.js:11:26:11:32 | req.url | node.js:15:34:15:45 | '/' + target | Untrusted URL redirection due to $@. | node.js:11:26:11:32 | req.url | user-provided value |
 | node.js:32:34:32:55 | target  ... =" + me | node.js:29:26:29:32 | req.url | node.js:32:34:32:55 | target  ... =" + me | Untrusted URL redirection due to $@. | node.js:29:26:29:32 | req.url | user-provided value |

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js

@@ -135,6 +135,6 @@ app.get('/redirect/:user', function(req, res) {
   res.redirect('//' + req.params.user); // BAD - could go to //evil.com
   res.redirect('u' + req.params.user); // BAD - could go to u.evil.com
 
-  res.redirect('/' + ('/u' + req.params.user)); // BAD - could go to //u.evil.com
+  res.redirect('/' + ('/u' + req.params.user)); // BAD - could go to //u.evil.com, but not flagged
-  res.redirect('/u' + req.params.user); // GOOD - but flagged anyway
+  res.redirect('/u' + req.params.user); // GOOD
 });

javascript/ql/test/query-tests/filters/ClassifyFiles/AlmostManyElementsOnLine.html

@@ -0,0 +1,4 @@
+<html>
+  <div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div>
+  <script>42</script>
+</html>

javascript/ql/test/query-tests/filters/ClassifyFiles/ClassifyFiles.expected

@@ -1,4 +1,5 @@
 | AutoRest.js:0:0:0:0 | AutoRest.js | generated |
+| ManyElementsOnLine.html:0:0:0:0 | ManyElementsOnLine.html | generated |
 | ai.1.2.3-build0123.js:0:0:0:0 | ai.1.2.3-build0123.js | library |
 | bundle-directive.js:0:0:0:0 | bundle-directive.js | generated |
 | data.js:0:0:0:0 | data.js | generated |

javascript/ql/test/query-tests/filters/ClassifyFiles/ManyElementsOnLine.html

@@ -0,0 +1,4 @@
+<html>
+  <div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div><div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div>
+  <script>42</script>
+</html>