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

javascript/ql/src/AlertSuppression.ql

@@ -16,9 +16,11 @@ class SuppressionComment extends Locatable {
 
   SuppressionComment() {
     (
-      text = this.(LineComment).getText() or
+      text = this.(Comment).getText() or
       text = this.(HTML::CommentNode).getText()
     ) and
+    // suppression comments must be single-line
+    not text.matches("%\n%") and
     (
       // match `lgtm[...]` anywhere in the comment
       annotation = text.regexpFind("(?i)\\blgtm\\s*\\[[^\\]]*\\]", _, _)

javascript/ql/src/Declarations/UniqueParameterNames.ql

@@ -36,6 +36,9 @@ where
   i < j and
   j = max(int k | parmBinds(f, k, _, name) | k) and
   not isDummy(p) and
+  // ignore functions without bodies or empty bodies
+  f.hasBody() and
+  exists(f.getABodyStmt()) and
   // duplicate parameters in strict mode functions are flagged by the 'Syntax error' rule
   not f.isStrict()
 select p, "This parameter has the same name as $@ of the same function.", q, "another parameter"

javascript/ql/src/Expressions/ExprHasNoEffect.qll

@@ -40,7 +40,6 @@ predicate inVoidContext(Expr e) {
   exists(LogicalBinaryExpr logical | e = logical.getRightOperand() and inVoidContext(logical))
 }
 
-
 /**
  * Holds if `e` is of the form `x;` or `e.p;` and has a JSDoc comment containing a tag.
  * In that case, it is probably meant as a declaration and shouldn't be flagged by this query.
@@ -155,5 +154,7 @@ predicate hasNoEffect(Expr e) {
   not exists(FunctionExpr fe, ExprStmt es | fe = e |
     fe = es.getExpr() and
     not exists(fe.getName())
-  )	
-}
+  ) and
+  // exclude block-level flow type annotations. For example: `(name: empty)`.
+  not e.(ParExpr).getExpression().getLastToken().getNextToken().getValue() = ":"
+}

javascript/ql/src/Expressions/MissingAwait.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+In JavaScript, <code>async</code> functions always return a promise object.
+To obtain the underlying value of the promise, use the <code>await</code> operator or call the <code>then</code> method.
+Attempting to use a promise object instead of its underlying value can lead to unexpected behavior.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Use the <code>await</code> operator to get the value contained in the promise.
+Alternatively, call <code>then</code> on the promise and use the value passed to the callback.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following example, the <code>getData</code> function returns a promise,
+and the caller checks if the returned promise is <code>null</code>:
+</p>
+
+<sample src="examples/MissingAwait.js" />
+
+<p>
+However, the null check does not work as expected. The <code>return null</code> statement
+on line 2 actually returns a <em>promise</em> containing the <code>null</code> value.
+Since the promise object itself is not equal to <code>null</code>, the error check is bypassed.
+</p>
+
+<p>
+The issue can be corrected by inserting <code>await</code> before the promise:
+</p>
+
+<sample src="examples/MissingAwaitGood.js" />
+
+</example>
+<references>
+<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises">Using promises</a></li>
+<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function">Async functions</a></li>
+<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await">Await operator</a></li>
+</references>
+</qhelp>

javascript/ql/src/Expressions/MissingAwait.ql

@@ -0,0 +1,81 @@
+/**
+ * @name Missing await
+ * @description Using a promise without awaiting its result can lead to unexpected behavior.
+ * @kind problem
+ * @problem.severity warning
+ * @id js/missing-await
+ * @tags correctness
+ * @precision high
+ */
+
+import javascript
+
+/**
+ * Holds if `call` is a call to an `async` function.
+ */
+predicate isAsyncCall(DataFlow::CallNode call) {
+  // If a callee is known, and all known callees are async, assume all
+  // possible callees are async.
+  forex(Function callee | call.getACallee() = callee | callee.isAsync())
+}
+
+/**
+ * Holds if `node` is always a promise.
+ */
+predicate isPromise(DataFlow::SourceNode node, boolean nullable) {
+  isAsyncCall(node) and
+  nullable = false
+  or
+  not isAsyncCall(node) and
+  node.asExpr().getType() instanceof PromiseType and
+  nullable = true
+}
+
+/**
+ * Holds the result of `e` is used in a way that doesn't make sense for Promise objects.
+ */
+predicate isBadPromiseContext(Expr expr) {
+  exists(BinaryExpr binary |
+    expr = binary.getAnOperand() and
+    not binary instanceof LogicalExpr and
+    not binary instanceof InstanceofExpr
+  )
+  or
+  expr = any(LogicalBinaryExpr e).getLeftOperand()
+  or
+  expr = any(UnaryExpr e).getOperand()
+  or
+  expr = any(UpdateExpr e).getOperand()
+  or
+  expr = any(ConditionalExpr e).getCondition()
+  or
+  expr = any(IfStmt stmt).getCondition()
+  or
+  expr = any(ForInStmt stmt).getIterationDomain()
+  or
+  expr = any(IndexExpr e).getIndex()
+}
+
+string tryGetPromiseExplanation(Expr e) {
+  result = "The value '" + e.(VarAccess).getName() + "' is always a promise."
+  or
+  result = "The call to '" + e.(CallExpr).getCalleeName() + "' always returns a promise."
+}
+
+string getPromiseExplanation(Expr e) {
+  result = tryGetPromiseExplanation(e)
+  or
+  not exists(tryGetPromiseExplanation(e)) and
+  result = "This value is always a promise."
+}
+
+from Expr expr, boolean nullable
+where
+  isBadPromiseContext(expr) and
+  isPromise(expr.flow().getImmediatePredecessor*(), nullable) and
+  (
+    nullable = false
+    or
+    expr.inNullSensitiveContext()
+  )
+select expr, "Missing await. " + getPromiseExplanation(expr)

javascript/ql/src/Expressions/examples/MissingAwait.js

@@ -0,0 +1,14 @@
+async function getData(id) {
+  let req = await fetch(`https://example.com/data?id=${id}`);
+  if (!req.ok) return null;
+  return req.json();
+}
+
+async function showData(id) {
+  let data = getData(id);
+  if (data == null) {
+    console.warn("No data for: " + id);
+    return;
+  }
+  // ...
+}

javascript/ql/src/Expressions/examples/MissingAwaitGood.js

@@ -0,0 +1,14 @@
+async function getData(id) {
+  let req = await fetch(`https://example.com/data?id=${id}`);
+  if (!req.ok) return null;
+  return req.json();
+}
+
+async function showData(id) {
+  let data = await getData(id);
+  if (data == null) {
+    console.warn("No data for: " + id);
+    return;
+  }
+  // ...
+}

javascript/ql/src/Performance/ReDoS.ql

@@ -144,7 +144,7 @@ newtype TInputSymbol =
   CharClass(RegExpCharacterClass recc) {
     getRoot(recc).isRelevant() and
     not recc.isInverted() and
-    not isUniversalClass(recc)
+    not recc.isUniversalClass()
   } or
   /** An input symbol representing all characters matched by `.`. */
   Dot() or
@@ -153,23 +153,6 @@ newtype TInputSymbol =
   /** An epsilon transition in the automaton. */
   Epsilon()
 
-/**
- * Holds if character class `cc` matches all characters.
- */
-predicate isUniversalClass(RegExpCharacterClass cc) {
-  // [^]
-  cc.isInverted() and not exists(cc.getAChild())
-  or
-  // [\w\W] and similar
-  not cc.isInverted() and
-  exists(string cce1, string cce2 |
-    cce1 = cc.getAChild().(RegExpCharacterClassEscape).getValue() and
-    cce2 = cc.getAChild().(RegExpCharacterClassEscape).getValue()
-  |
-    cce1 != cce2 and cce1.toLowerCase() = cce2.toLowerCase()
-  )
-}
-
 /**
  * An abstract input symbol, representing a set of concrete characters.
  */
@@ -361,7 +344,7 @@ predicate delta(State q1, EdgeLabel lbl, State q2) {
   )
   or
   exists(RegExpCharacterClass cc |
-    isUniversalClass(cc) and q1 = before(cc) and lbl = Any() and q2 = after(cc)
+    cc.isUniversalClass() and q1 = before(cc) and lbl = Any() and q2 = after(cc)
     or
     q1 = before(cc) and lbl = CharClass(cc) and q2 = after(cc)
   )

javascript/ql/src/RegExp/RegExpAlwaysMatches.qhelp

@@ -0,0 +1,55 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+There are several built-in JavaScript functions that search for a regular expression match within a string,
+such as <code>RegExp.prototype.test</code> and <code>String.prototype.search</code>.
+If the regular expression is not anchored, it only needs to match a substring of the input
+and won't necessarily match the whole string.
+</p>
+
+<p>
+If the regular expression being searched for accepts the empty string, this means it can match an empty
+substring anywhere in the input string, and will thus always find a match.
+In this case, testing if a match exists is redundant and indicates dead code.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Examine the regular expression and determine how it was intended to match:
+</p>
+
+<ul>
+<li>To match the whole input string, add anchors at the beginning and end of the regular expression.</li>
+<li>To search for an occurrence within the input string, consider what the shortest meaningful match is and restrict the
+regular expression accordingly, such as by changing a <code>*</code> to a <code>+</code>.</li>
+</ul>
+
+</recommendation>
+<example>
+<p>
+In the following example, a regular expression is used to check the format of a string <code>id</code>.
+However, the check always passes because the regular expression can match the empty substring.
+For example, it will allow the ID string "<code>%%</code>" by matching an empty string at index 0.
+</p>
+
+<sample src="examples/RegExpAlwaysMatches.js" />
+
+<p>
+To ensure the regular expression matches the whole string, add anchors at the beginning and end:
+</p>
+
+<sample src="examples/RegExpAlwaysMatchesGood.js" />
+
+</example>
+<references>
+
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions">JavaScript Regular Expressions</a>.</li>
+
+</references>
+</qhelp>

javascript/ql/src/RegExp/RegExpAlwaysMatches.ql

@@ -0,0 +1,125 @@
+/**
+ * @name Regular expression always matches
+ * @description Regular expression tests that always find a match indicate dead code or a logic error
+ * @kind problem
+ * @problem.severity warning
+ * @id js/regex/always-matches
+ * @tags correctness
+ *       regular-expressions
+ * @precision high
+ */
+
+import javascript
+
+/**
+ * Gets a node reachable from the given root term through alts and groups only.
+ *
+ * For example, for `/(foo|bar)/` this gets `(foo|bar)`, `foo|bar`, `foo` and `bar`.
+ */
+RegExpTerm getEffectiveRootAux(RegExpTerm actualRoot) {
+  actualRoot.isRootTerm() and
+  result = actualRoot
+  or
+  result = getEffectiveRootAux(actualRoot).(RegExpAlt).getAChild()
+  or
+  result = getEffectiveRootAux(actualRoot).(RegExpGroup).getAChild()
+}
+
+/**
+ * Gets the effective root of the given term.
+ *
+ * For example, for `/(foo|bar)/` this gets `foo` and `bar`.
+ */
+RegExpTerm getEffectiveRoot(RegExpTerm actualRoot) {
+  result = getEffectiveRootAux(actualRoot) and
+  not result instanceof RegExpAlt and
+  not result instanceof RegExpGroup
+}
+
+/**
+ * Holds if `term` contains an anchor on both ends.
+ */
+predicate isPossiblyAnchoredOnBothEnds(RegExpSequence node) {
+  node.getAChild*() instanceof RegExpCaret and
+  node.getAChild*() instanceof RegExpDollar and
+  node.getNumChild() >= 2
+}
+
+/**
+ * Holds if `term` is obviously intended to match any string.
+ */
+predicate isUniversalRegExp(RegExpTerm term) {
+  exists(RegExpTerm child | child = term.(RegExpStar).getAChild() |
+    child instanceof RegExpDot
+    or
+    child.(RegExpCharacterClass).isUniversalClass()
+  )
+}
+
+/**
+ * A call that searches for a regexp match within a string, but does not
+ * extract the capture groups or the matched string itself.
+ *
+ * Because of the longest-match rule, queries that are more than pure tests
+ * aren't necessarily broken just because the regexp can accept the empty string.
+ */
+abstract class RegExpQuery extends DataFlow::CallNode {
+  abstract RegExpTerm getRegExp();
+}
+
+/**
+ * A call to `RegExp.prototype.test`.
+ */
+class RegExpTestCall extends DataFlow::MethodCallNode, RegExpQuery {
+  DataFlow::RegExpCreationNode regexp;
+
+  RegExpTestCall() {
+    this = regexp.getAReference().getAMethodCall("test")
+  }
+
+  override RegExpTerm getRegExp() {
+    result = regexp.getRoot()
+  }
+}
+
+/**
+ * A call to `String.prototype.search`.
+ */
+class RegExpSearchCall extends DataFlow::MethodCallNode, RegExpQuery {
+  DataFlow::RegExpCreationNode regexp;
+
+  RegExpSearchCall() {
+    getMethodName() = "search" and
+    regexp.getAReference().flowsTo(getArgument(0))
+  }
+
+  override RegExpTerm getRegExp() {
+    result = regexp.getRoot()
+  }
+}
+
+/**
+ * Holds if `t` is a zero-width assertion other than an anchor.
+ */
+predicate isAssertion(RegExpTerm t) {
+  t instanceof RegExpSubPattern or
+  t instanceof RegExpWordBoundary or
+  t instanceof RegExpNonWordBoundary
+}
+
+from RegExpTerm term, RegExpQuery call, string message
+where
+  term.isNullable() and
+  not isAssertion(term.getAChild*()) and
+  not isUniversalRegExp(term) and
+  term = getEffectiveRoot(call.getRegExp()) and
+  (
+    call instanceof RegExpTestCall and
+    not isPossiblyAnchoredOnBothEnds(term) and
+    message = "This regular expression always matches when used in a test $@, as it can match an empty substring."
+    or
+    call instanceof RegExpSearchCall and
+    not term.getAChild*() instanceof RegExpDollar and
+    message = "This regular expression always the matches at index 0 when used $@, as it matches the empty substring."
+  )
+select term, message, call, "here"

javascript/ql/src/RegExp/examples/RegExpAlwaysMatches.js

@@ -0,0 +1,3 @@
+if (!/[a-z0-9]*/.test(id)) {
+    throw new Error("Invalid id: " + id);
+}

javascript/ql/src/RegExp/examples/RegExpAlwaysMatchesGood.js

@@ -0,0 +1,3 @@
+if (!/^[a-z0-9]*$/.test(id)) {
+    throw new Error("Invalid id: " + id);
+}

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

@@ -454,16 +454,16 @@ class RegExpLiteral extends @regexpliteral, Literal, RegExpParent {
   string getFlags() { result = getValue().regexpCapture(".*/(\\w*)$", 1) }
 
   /** Holds if this regular expression has an `m` flag. */
-  predicate isMultiline() { getFlags().matches("%m%") }
+  predicate isMultiline() { RegExp::isMultiline(getFlags()) }
 
   /** Holds if this regular expression has a `g` flag. */
-  predicate isGlobal() { getFlags().matches("%g%") }
+  predicate isGlobal() { RegExp::isGlobal(getFlags()) }
 
   /** Holds if this regular expression has an `i` flag. */
-  predicate isIgnoreCase() { getFlags().matches("%i%") }
+  predicate isIgnoreCase() { RegExp::isIgnoreCase(getFlags()) }
 
   /** Holds if this regular expression has an `s` flag. */
-  predicate isDotAll() { getFlags().matches("%s%") }
+  predicate isDotAll() { RegExp::isDotAll(getFlags()) }
 }
 
 /**

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

@@ -88,6 +88,7 @@ abstract class Module extends TopLevel {
         result = f.getJavaScriptFile(path.getBaseName())
         or
         // If a js file was not found look for a file that compiles to js
+        path.getExtension() = ".js" and
         not exists(f.getJavaScriptFile(path.getBaseName())) and
         result = f.getJavaScriptFile(path.getStem())
       )

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

@@ -128,6 +128,14 @@ abstract class PathString extends string {
   /** Gets the path of the parent folder of the folder or file this path refers to. */
   string getDirName() { result = this.regexpCapture(pathRegex(), 1) }
 
+  /**
+   * Gets the extension of the folder or file this path refers to, that is, the suffix of the base name
+   * starting at the last dot character, if there is one.
+   *
+   * Has no result if the base name does not contain a dot.
+   */
+  string getExtension() { result = this.regexpCapture(pathRegex(), 4) }
+
   /**
    * Gets the absolute path that the sub-path consisting of the first `n`
    * components of this path refers to when resolved relative to the
@@ -208,6 +216,14 @@ abstract class PathExpr extends PathExprBase {
   /** Gets the stem, that is, base name without extension, of the folder or file this path refers to. */
   string getStem() { result = getValue().(PathString).getStem() }
 
+  /**
+   * Gets the extension of the folder or file this path refers to, that is, the suffix of the base name
+   * starting at the last dot character, if there is one.
+   *
+   * Has no result if the base name does not contain a dot.
+   */
+  string getExtension() { result = getValue().(PathString).getExtension() }
+
   /**
    * Gets the file or folder that the first `n` components of this path refer to
    * when resolved relative to the root folder of the given `priority`.

javascript/ql/src/semmle/javascript/Regexp.qll

@@ -113,9 +113,7 @@ class RegExpTerm extends Locatable, @regexpterm {
   /**
    * Holds if this is the root term of a regular expression.
    */
-  predicate isRootTerm() {
-    not getParent() instanceof RegExpTerm
-  }
+  predicate isRootTerm() { not getParent() instanceof RegExpTerm }
 
   /**
    * Gets the outermost term of this regular expression.
@@ -130,9 +128,7 @@ class RegExpTerm extends Locatable, @regexpterm {
   /**
    * Holds if this term occurs as part of a regular expression literal.
    */
-  predicate isPartOfRegExpLiteral() {
-    exists(getLiteral())
-  }
+  predicate isPartOfRegExpLiteral() { exists(getLiteral()) }
 
   /**
    * Holds if this term occurs as part of a string literal.
@@ -140,9 +136,7 @@ class RegExpTerm extends Locatable, @regexpterm {
    * This predicate holds regardless of whether the string literal is actually
    * used as a regular expression. See `isUsedAsRegExp`.
    */
-  predicate isPartOfStringLiteral() {
-    getRootTerm().getParent() instanceof StringLiteral
-  }
+  predicate isPartOfStringLiteral() { getRootTerm().getParent() instanceof StringLiteral }
 
   /**
    * Holds if this term is part of a regular expression literal, or a string literal
@@ -344,8 +338,7 @@ class RegExpAnchor extends RegExpTerm, @regexp_anchor {
  * ^
  * ```
  */
-class RegExpCaret extends RegExpAnchor, @regexp_caret {
-}
+class RegExpCaret extends RegExpAnchor, @regexp_caret { }
 
 /**
  * A dollar assertion `$` matching the end of a line.
@@ -356,8 +349,7 @@ class RegExpCaret extends RegExpAnchor, @regexp_caret {
  * $
  * ```
  */
-class RegExpDollar extends RegExpAnchor, @regexp_dollar {
-}
+class RegExpDollar extends RegExpAnchor, @regexp_dollar { }
 
 /**
  * A word boundary assertion.
@@ -772,6 +764,23 @@ class RegExpCharacterClass extends RegExpTerm, @regexp_char_class {
   override string getAMatchedString() {
     not isInverted() and result = getAChild().getAMatchedString()
   }
+
+  /**
+   * Holds if this character class matches any character.
+   */
+  predicate isUniversalClass() {
+    // [^]
+    isInverted() and not exists(getAChild())
+    or
+    // [\w\W] and similar
+    not isInverted() and
+    exists(string cce1, string cce2 |
+      cce1 = getAChild().(RegExpCharacterClassEscape).getValue() and
+      cce2 = getAChild().(RegExpCharacterClassEscape).getValue()
+    |
+      cce1 != cce2 and cce1.toLowerCase() = cce2.toLowerCase()
+    )
+  }
 }
 
 /**
@@ -806,6 +815,16 @@ class RegExpParseError extends Error, @regexp_parse_error {
   override string toString() { result = getMessage() }
 }
 
+/**
+ * Holds if `func` is a method defined on `String.prototype` with name `name`. 
+ */
+private predicate isNativeStringMethod(Function func, string name) {
+  exists(ExternalInstanceMemberDecl decl |
+    decl.hasQualifiedName("String", name) and
+    func = decl.getInit()
+  )
+}
+
 /**
  * Holds if `source` may be interpreted as a regular expression.
  */
@@ -816,18 +835,23 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) {
     source = DataFlow::globalVarRef("RegExp").getAnInvocation().getArgument(0)
     or
     // The argument of a call that coerces the argument to a regular expression.
-    exists(MethodCallExpr mce, string methodName |
+    exists(DataFlow::MethodCallNode mce, string methodName |
       mce.getReceiver().analyze().getAType() = TTString() and
-      mce.getMethodName() = methodName
+      mce.getMethodName() = methodName and
+      not exists(Function func |
+        func = mce.getACallee()
+      |
+        not isNativeStringMethod(func, methodName)
+      )
     |
-      methodName = "match" and source.asExpr() = mce.getArgument(0) and mce.getNumArgument() = 1
+      methodName = "match" and source = mce.getArgument(0) and mce.getNumArgument() = 1
       or
       methodName = "search" and
-      source.asExpr() = mce.getArgument(0) and
+      source = mce.getArgument(0) and
       mce.getNumArgument() = 1 and
       // "search" is a common method name, and so we exclude chained accesses
       // because `String.prototype.search` returns a number
-      not exists(PropAccess p | p.getBase() = mce)
+      not exists(PropAccess p | p.getBase() = mce.getEnclosingExpr())
     )
   )
 }
@@ -940,3 +964,131 @@ private class StringRegExpPatternSource extends RegExpPatternSource {
 
   override RegExpTerm getRegExpTerm() { result = asExpr().(StringLiteral).asRegExp() }
 }
+
+module RegExp {
+  /** Gets the string `"?"` used to represent a regular expression whose flags are unknown. */
+  string unknownFlag() { result = "?" }
+
+  /** Holds if `flags` includes the `m` flag. */
+  bindingset[flags]
+  predicate isMultiline(string flags) { flags.matches("%m%") }
+
+  /** Holds if `flags` includes the `g` flag. */
+  bindingset[flags]
+  predicate isGlobal(string flags) { flags.matches("%g%") }
+
+  /** Holds if `flags` includes the `i` flag. */
+  bindingset[flags]
+  predicate isIgnoreCase(string flags) { flags.matches("%i%") }
+
+  /** Holds if `flags` includes the `s` flag. */
+  bindingset[flags]
+  predicate isDotAll(string flags) { flags.matches("%s%") }
+
+  /** Holds if `flags` includes the `m` flag or is the unknown flag `?`. */
+  bindingset[flags]
+  predicate maybeMultiline(string flags) { flags = unknownFlag() or isMultiline(flags) }
+
+  /** Holds if `flags` includes the `g` flag or is the unknown flag `?`. */
+  bindingset[flags]
+  predicate maybeGlobal(string flags) { flags = unknownFlag() or isGlobal(flags) }
+
+  /** Holds if `flags` includes the `i` flag or is the unknown flag `?`. */
+  bindingset[flags]
+  predicate maybeIgnoreCase(string flags) { flags = unknownFlag() or isIgnoreCase(flags) }
+
+  /** Holds if `flags` includes the `s` flag or is the unknown flag `?`. */
+  bindingset[flags]
+  predicate maybeDotAll(string flags) { flags = unknownFlag() or isDotAll(flags) }
+
+  /** Holds if `term` and all of its disjuncts are anchored on both ends. */
+  predicate isFullyAnchoredTerm(RegExpTerm term) {
+    exists(RegExpSequence seq | term = seq |
+      seq.getChild(0) instanceof RegExpCaret and
+      seq.getLastChild() instanceof RegExpDollar
+    )
+    or
+    isFullyAnchoredTerm(term.(RegExpGroup).getAChild())
+    or
+    isFullyAnchoredAlt(term, term.getNumChild())
+  }
+
+  /** Holds if the first `i` disjuncts of `term` are fully anchored. */
+  private predicate isFullyAnchoredAlt(RegExpAlt term, int i) {
+    isFullyAnchoredTerm(term.getChild(0)) and i = 1
+    or
+    isFullyAnchoredAlt(term, i - 1) and
+    isFullyAnchoredTerm(term.getChild(i - 1))
+  }
+
+  /**
+   * Holds if `term` matches any character except for explicitly listed exceptions.
+   *
+   * For example, holds for `.`, `[^<>]`, or `\W`, but not for `[a-z]`, `\w`, or `[^\W\S]`.
+   */
+  predicate isWildcardLike(RegExpTerm term) {
+    term instanceof RegExpDot
+    or
+    term.(RegExpCharacterClassEscape).getValue().isUppercase()
+    or
+    // [^a-z]
+    exists(RegExpCharacterClass cls | term = cls |
+      cls.isInverted() and
+      not cls.getAChild().(RegExpCharacterClassEscape).getValue().isUppercase()
+    )
+    or
+    // [\W]
+    exists(RegExpCharacterClass cls | term = cls |
+      not cls.isInverted() and
+      cls.getAChild().(RegExpCharacterClassEscape).getValue().isUppercase()
+    )
+  }
+
+  /**
+   * Holds if `term` is a generic sanitizer for strings that match (if `outcome` is true)
+   * or strings that don't match (if `outcome` is false).
+   *
+   * Specifically, whitelisting regexps such as `^(foo|bar)$` sanitize matches in the true case.
+   * Inverted character classes such as `[^a-z]` or `\W` sanitize matches in the false case.
+   */
+  predicate isGenericRegExpSanitizer(RegExpTerm term, boolean outcome) {
+    term.isRootTerm() and
+    (
+      outcome = true and
+      isFullyAnchoredTerm(term) and
+      not isWildcardLike(term.getAChild*())
+      or
+      // Character set restrictions like `/[^a-z]/.test(x)` sanitize in the false case
+      outcome = false and
+      exists(RegExpTerm root |
+        root = term
+        or
+        root = term.(RegExpGroup).getAChild()
+      |
+        isWildcardLike(root)
+        or
+        isWildcardLike(root.(RegExpAlt).getAChild())
+      )
+    )
+  }
+
+  /**
+   * Gets the AST of a regular expression object that can flow to `node`.
+   */
+  RegExpTerm getRegExpObjectFromNode(DataFlow::Node node) {
+    exists(DataFlow::RegExpCreationNode regexp |
+      regexp.getAReference().flowsTo(node) and
+      result = regexp.getRoot()
+    )
+  }
+
+  /**
+   * Gets the AST of a regular expression that can flow to `node`,
+   * including `RegExp` objects as well as strings interpreted as regular expressions.
+   */
+  RegExpTerm getRegExpFromNode(DataFlow::Node node) {
+    result = getRegExpObjectFromNode(node)
+    or
+    result = node.asExpr().(StringLiteral).asRegExp()
+  }
+}

javascript/ql/src/semmle/javascript/SSA.qll

@@ -639,8 +639,16 @@ abstract class SsaPseudoDefinition extends SsaImplicitDefinition {
  * would be visible.
  */
 class SsaPhiNode extends SsaPseudoDefinition, TPhi {
+  /**
+   * Gets the input to this phi node coming from the given predecessor block.
+   */
+  SsaVariable getInputFromBlock(BasicBlock bb) {
+    bb = getBasicBlock().getAPredecessor() and
+    result = getDefReachingEndOf(bb, getSourceVariable())
+  }
+
   override SsaVariable getAnInput() {
-    result = getDefReachingEndOf(getBasicBlock().getAPredecessor(), getSourceVariable())
+    result = getInputFromBlock(_)
   }
 
   override predicate definesAt(ReachableBasicBlock bb, int i, SsaSourceVariable v) {
@@ -662,6 +670,29 @@ class SsaPhiNode extends SsaPseudoDefinition, TPhi {
     endcolumn = startcolumn and
     getBasicBlock().getLocation().hasLocationInfo(filepath, startline, startcolumn, _, _)
   }
+
+  /**
+   * If all inputs to this phi node are (transitive) refinements of the same variable,
+   * gets that variable.
+   */
+  SsaVariable getRephinedVariable() {
+    forex(SsaVariable input | input = getAnInput() |
+      result = getRefinedVariable(input)
+    )
+  }
+}
+
+/**
+ * Gets the input to the given refinement node or rephinement node.
+ */
+private SsaVariable getRefinedVariable(SsaVariable v) {
+  result = getRefinedVariable(v.(SsaRefinementNode).getAnInput())
+  or
+  result = getRefinedVariable(v.(SsaPhiNode).getRephinedVariable())
+  or
+  not v instanceof SsaRefinementNode and
+  not v instanceof SsaPhiNode and
+  result = v
 }
 
 /**

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

@@ -209,6 +209,22 @@ private class PromiseFlowStep extends DataFlow::AdditionalFlowStep {
   }
 }
 
+/**
+ * A data flow edge from the exceptional return of the promise executor to the promise catch handler.
+ * This only adds an edge from the exceptional return of the promise executor to a `.catch()` handler.
+ */
+private class PromiseExceptionalStep extends DataFlow::AdditionalFlowStep {
+  PromiseDefinition promise;
+  PromiseExceptionalStep() {
+    promise = this	
+  }
+
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    pred = promise.getExecutor().getExceptionalReturn() and
+    succ = promise.getACatchHandler().getParameter(0)
+  }
+}
+
 /**
  * Holds if taint propagates from `pred` to `succ` through promises.
  */

javascript/ql/src/semmle/javascript/XML.qll

@@ -2,13 +2,20 @@
  * Provides classes and predicates for working with XML files and their content.
  */
 
-import javascript
+import semmle.files.FileSystem
 
 /** An XML element that has a location. */
 abstract class XMLLocatable extends @xmllocatable {
   /** Gets the source location for this element. */
   Location getLocation() { xmllocations(this, result) }
 
+  /**
+   * DEPRECATED: Use `getLocation()` instead.
+   *
+   * Gets the source location for this element.
+   */
+  deprecated Location getALocation() { result = this.getLocation() }
+
   /**
    * Holds if this element is at the specified location.
    * The location spans column `startcolumn` of line `startline` to
@@ -34,6 +41,12 @@ abstract class XMLLocatable extends @xmllocatable {
  * both of which can contain other elements.
  */
 class XMLParent extends @xmlparent {
+  XMLParent() {
+    // explicitly restrict `this` to be either an `XMLElement` or an `XMLFile`;
+    // the type `@xmlparent` currently also includes non-XML files
+    this instanceof @xmlelement or xmlEncoding(this, _)
+  }
+
   /**
    * Gets a printable representation of this XML parent.
    * (Intended to be overridden in subclasses.)
@@ -82,7 +95,10 @@ class XMLParent extends @xmlparent {
     )
   }
 
-  /** Append all the character sequences of this XML parent from left to right, separated by a space. */
+  /**
+   * Gets the result of appending all the character sequences of this XML parent from
+   * left to right, separated by a space.
+   */
   string allCharactersString() {
     result = concat(string chars, int pos |
         xmlChars(_, chars, this, pos, _, _)
@@ -108,6 +124,20 @@ class XMLFile extends XMLParent, File {
   /** Gets the name of this XML file. */
   override string getName() { result = File.super.getAbsolutePath() }
 
+  /**
+   * DEPRECATED: Use `getAbsolutePath()` instead.
+   *
+   * Gets the path of this XML file.
+   */
+  deprecated string getPath() { result = getAbsolutePath() }
+
+  /**
+   * DEPRECATED: Use `getParentContainer().getAbsolutePath()` instead.
+   *
+   * Gets the path of the folder that contains this XML file.
+   */
+  deprecated string getFolder() { result = getParentContainer().getAbsolutePath() }
+
   /** Gets the encoding of this XML file. */
   string getEncoding() { xmlEncoding(this, result) }
 
@@ -132,7 +162,7 @@ class XMLFile extends XMLParent, File {
  * <!ELEMENT lastName (#PCDATA)>
  * ```
  */
-class XMLDTD extends @xmldtd {
+class XMLDTD extends XMLLocatable, @xmldtd {
   /** Gets the name of the root element of this DTD. */
   string getRoot() { xmlDTDs(this, result, _, _, _) }
 
@@ -148,8 +178,7 @@ class XMLDTD extends @xmldtd {
   /** Gets the parent of this DTD. */
   XMLParent getParent() { xmlDTDs(this, _, _, _, result) }
 
-  /** Gets a printable representation of this DTD. */
-  string toString() {
+  override string toString() {
     this.isPublic() and
     result = this.getRoot() + " PUBLIC '" + this.getPublicId() + "' '" + this.getSystemId() + "'"
     or
@@ -252,7 +281,7 @@ class XMLAttribute extends @xmlattribute, XMLLocatable {
  * xmlns:android="http://schemas.android.com/apk/res/android"
  * ```
  */
-class XMLNamespace extends @xmlnamespace {
+class XMLNamespace extends XMLLocatable, @xmlnamespace {
   /** Gets the prefix of this namespace. */
   string getPrefix() { xmlNs(this, result, _, _) }
 
@@ -262,8 +291,7 @@ class XMLNamespace extends @xmlnamespace {
   /** Holds if this namespace has no prefix. */
   predicate isDefault() { this.getPrefix() = "" }
 
-  /** Gets a printable representation of this XML namespace. */
-  string toString() {
+  override string toString() {
     this.isDefault() and result = this.getURI()
     or
     not this.isDefault() and result = this.getPrefix() + ":" + this.getURI()

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

@@ -363,6 +363,51 @@ private predicate barrierGuardBlocksAccessPath(BarrierGuardNode guard, boolean o
   barrierGuardBlocksExpr(guard, outcome, ap.getAnInstance(), label)
 }
 
+/**
+  * Holds if `guard` should block flow along the edge `pred -> succ`.
+  *
+  * `label` is bound to the blocked label, or the empty string if all labels should be blocked.
+  */
+private predicate barrierGuardBlocksEdge(BarrierGuardNode guard, DataFlow::Node pred, DataFlow::Node succ, string label) {
+  exists(SsaVariable input, SsaPhiNode phi, BasicBlock bb, ConditionGuardNode cond, boolean outcome |
+    pred = DataFlow::ssaDefinitionNode(input) and
+    succ = DataFlow::ssaDefinitionNode(phi) and
+    input = phi.getInputFromBlock(bb) and
+    guard.getEnclosingExpr() = cond.getTest() and
+    outcome = cond.getOutcome() and
+    barrierGuardBlocksExpr(guard, outcome, input.getAUse(), label) and
+    cond.dominates(bb)
+  )
+}
+
+/**
+ * Holds if there is a barrier edge `pred -> succ` in `cfg` either through an explicit barrier edge
+ * or one implied by a barrier guard.
+ *
+ * Only holds for barriers that should apply to all flow labels.
+ */
+private predicate isBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ) {
+  cfg.isBarrierEdge(pred, succ)
+  or
+  exists(DataFlow::BarrierGuardNode guard |
+    cfg.isBarrierGuard(guard) and
+    barrierGuardBlocksEdge(guard, pred, succ, "")
+  )
+}
+
+/**
+ * Holds if there is a labeled barrier edge `pred -> succ` in `cfg` either through an explicit barrier edge
+ * or one implied by a barrier guard.
+ */
+private predicate isLabeledBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel label) {
+  cfg.isBarrierEdge(pred, succ, label)
+  or
+  exists(DataFlow::BarrierGuardNode guard |
+    cfg.isBarrierGuard(guard) and
+    barrierGuardBlocksEdge(guard, pred, succ, label)
+  )
+}
+
 /**
  * A guard node that only blocks specific labels.
  */
@@ -470,7 +515,8 @@ private predicate basicFlowStep(
     // Local flow
     exists(FlowLabel predlbl, FlowLabel succlbl |
       localFlowStep(pred, succ, cfg, predlbl, succlbl) and
-      not cfg.isBarrierEdge(pred, succ, predlbl) and
+      not isLabeledBarrierEdge(cfg, pred, succ, predlbl) and
+      not isBarrierEdge(cfg, pred, succ) and
       summary = MkPathSummary(false, false, predlbl, succlbl)
     )
     or
@@ -584,7 +630,7 @@ private predicate callInputStep(
     )
   ) and
   not cfg.isBarrier(succ) and
-  not cfg.isBarrierEdge(pred, succ)
+  not isBarrierEdge(cfg, pred, succ)
 }
 
 /**
@@ -638,7 +684,8 @@ private predicate flowThroughCall(
     ret.asExpr() = f.getAReturnedExpr() and
     calls(output, f) and // Do not consider partial calls
     reachableFromInput(f, output, input, ret, cfg, summary) and
-    not cfg.isBarrierEdge(ret, output) and
+    not isBarrierEdge(cfg, ret, output) and
+    not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
     not cfg.isLabeledBarrier(output, summary.getEndLabel())
   )
   or
@@ -647,7 +694,8 @@ private predicate flowThroughCall(
     DataFlow::exceptionalInvocationReturnNode(output, invk.asExpr()) and
     calls(invk, f) and
     reachableFromInput(f, invk, input, ret, cfg, summary) and
-    not cfg.isBarrierEdge(ret, output) and
+    not isBarrierEdge(cfg, ret, output) and
+    not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
     not cfg.isLabeledBarrier(output, summary.getEndLabel())
   )
 }
@@ -886,7 +934,8 @@ private predicate flowStep(
     flowIntoHigherOrderCall(pred, succ, cfg, summary)
   ) and
   not cfg.isBarrier(succ) and
-  not cfg.isBarrierEdge(pred, succ) and
+  not isBarrierEdge(cfg, pred, succ) and
+  not isLabeledBarrierEdge(cfg, pred, succ, summary.getEndLabel()) and
   not cfg.isLabeledBarrier(succ, summary.getEndLabel())
 }
 
@@ -938,8 +987,8 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg, PathSum
   or
   exists(DataFlow::Node mid, PathSummary stepSummary |
     reachableFromSource(nd, cfg, summary) and
-    flowStep(nd, cfg, mid, stepSummary) and
-    onPath(mid, cfg, summary.append(stepSummary))
+    flowStep(nd, id(cfg), mid, stepSummary) and
+    onPath(mid, id(cfg), summary.append(stepSummary))
   )
 }
 
@@ -1070,6 +1119,18 @@ private MidPathNode finalMidNode(SinkPathNode snk) {
   )
 }
 
+/**
+ * Holds if `nd` is a mid node wrapping `(predNd, cfg, summary)`, and there is a flow step
+ * from `predNd` to `succNd` under `cfg` with summary `newSummary`.
+ *
+ * This helper predicate exists to clarify the intended join order in `getASuccessor` below.
+ */
+pragma[noinline]
+private predicate midNodeStep(PathNode nd, DataFlow::Node predNd, Configuration cfg, PathSummary summary, DataFlow::Node succNd, PathSummary newSummary) {
+  nd = MkMidNode(predNd, cfg, summary) and
+  flowStep(predNd, id(cfg), succNd, newSummary)
+}
+
 /**
  * Gets a node to which data from `nd` may flow in one step.
  */
@@ -1079,8 +1140,7 @@ private PathNode getASuccessor(PathNode nd) {
   or
   // mid node to mid node
   exists(Configuration cfg, DataFlow::Node predNd, PathSummary summary, DataFlow::Node succNd, PathSummary newSummary |
-    nd = MkMidNode(predNd, cfg, summary) and
-    flowStep(predNd, id(cfg), succNd, newSummary) and
+    midNodeStep(nd, predNd, cfg, summary, succNd, newSummary) and
     result = MkMidNode(succNd, id(cfg), summary.append(newSummary))
   )
   or

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

@@ -208,6 +208,11 @@ module DataFlow {
         result = TSsaDefNode(refinement.getAnInput())
       )
       or
+      exists(SsaPhiNode phi |
+        this = TSsaDefNode(phi) and
+        result = TSsaDefNode(phi.getRephinedVariable())
+      )
+      or
       // IIFE call -> return value of IIFE
       exists(Function fun |
         localCall(this.asExpr(), fun) and

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

@@ -546,6 +546,11 @@ class RegExpLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
 
   /** Gets the root term of this regular expression. */
   RegExpTerm getRoot() { result = astNode.getRoot() }
+
+  /** Gets the flags of this regular expression literal. */
+  string getFlags() {
+    result = astNode.getFlags()
+  }
 }
 
 /**
@@ -1315,3 +1320,110 @@ module PartialInvokeNode {
  * This contributes additional argument-passing flow edges that should be added to all data flow configurations.
  */
 deprecated class AdditionalPartialInvokeNode = PartialInvokeNode::Range;
+
+/**
+ * An invocation of the `RegExp` constructor.
+ *
+ * Example:
+ * ```js
+ * new RegExp("#[a-z]+", "g");
+ * RegExp("^\w*$");
+ * ```
+ */
+class RegExpConstructorInvokeNode extends DataFlow::InvokeNode {
+  RegExpConstructorInvokeNode() {
+    this = DataFlow::globalVarRef("RegExp").getAnInvocation()
+  }
+
+  /**
+   * Gets the AST of the regular expression created here, provided that the
+   * first argument is a string literal.
+   */
+  RegExpTerm getRoot() {
+    result = getArgument(0).asExpr().(StringLiteral).asRegExp()
+  }
+
+  /**
+   * Gets the flags provided in the second argument, or an empty string if no
+   * flags are provided.
+   *
+   * Has no result if the flags are provided but are not constant.
+   */
+  string getFlags() {
+    result = getArgument(1).getStringValue()
+    or
+    not exists(getArgument(1)) and
+    result = ""
+  }
+
+  /**
+   * Gets the flags provided in the second argument, or an empty string if no
+   * flags are provided, or the string `"?"` if the provided flags are not known.
+   */
+  string tryGetFlags() {
+    result = getFlags()
+    or
+    not exists(getFlags()) and
+    result = RegExp::unknownFlag()
+  }
+}
+
+/**
+ * A data flow node corresponding to a regular expression literal or
+ * an invocation of the `RegExp` constructor.
+ *
+ * Examples:
+ * ```js
+ * new RegExp("#[a-z]+", "g");
+ * RegExp("^\w*$");
+ * /[a-z]+/i
+ * ```
+ */
+class RegExpCreationNode extends DataFlow::SourceNode {
+  RegExpCreationNode() {
+    this instanceof RegExpConstructorInvokeNode or
+    this instanceof RegExpLiteralNode
+  }
+
+  /**
+   * Gets the root term of the created regular expression, if it is known.
+   *
+   * Has no result for calls to `RegExp` with a non-constant argument.
+   */
+  RegExpTerm getRoot() {
+    result = this.(RegExpConstructorInvokeNode).getRoot() or
+    result = this.(RegExpLiteralNode).getRoot()
+  }
+
+  /**
+   * Gets the provided regular expression flags, if they are known.
+   */
+  string getFlags() {
+    result = this.(RegExpConstructorInvokeNode).getFlags() or
+    result = this.(RegExpLiteralNode).getFlags()
+  }
+
+  /**
+   * Gets the flags provided in the second argument, or an empty string if no
+   * flags are provided, or the string `"?"` if the provided flags are not known.
+   */
+  string tryGetFlags() {
+    result = this.(RegExpConstructorInvokeNode).tryGetFlags() or
+    result = this.(RegExpLiteralNode).getFlags()
+  }
+
+  /** Gets a data flow node referring to this regular expression. */
+  private DataFlow::SourceNode getAReference(DataFlow::TypeTracker t) {
+    t.start() and
+    result = this
+    or
+    exists(DataFlow::TypeTracker t2 |
+      result = getAReference(t2).track(t2, t)
+    )
+  }
+
+  /** Gets a data flow node referring to this regular expression. */
+  DataFlow::SourceNode getAReference() {
+    result = getAReference(DataFlow::TypeTracker::end())
+  }
+}

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

@@ -696,33 +696,37 @@ module TaintTracking {
    */
   class SanitizingRegExpTest extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
     Expr expr;
+    boolean sanitizedOutcome;
 
     SanitizingRegExpTest() {
       exists(MethodCallExpr mce, Expr base, string m, Expr firstArg |
         mce = astNode and mce.calls(base, m) and firstArg = mce.getArgument(0)
       |
         // /re/.test(u) or /re/.exec(u)
-        base.analyze().getAType() = TTRegExp() and
+        RegExp::isGenericRegExpSanitizer(RegExp::getRegExpObjectFromNode(base.flow()), sanitizedOutcome) and
         (m = "test" or m = "exec") and
         firstArg = expr
         or
         // u.match(/re/) or u.match("re")
         base = expr and
         m = "match" and
-        exists(InferredType firstArgType | firstArgType = firstArg.analyze().getAType() |
-          firstArgType = TTRegExp() or firstArgType = TTString()
-        )
+        RegExp::isGenericRegExpSanitizer(RegExp::getRegExpFromNode(firstArg.flow()), sanitizedOutcome)
       )
       or
       // m = /re/.exec(u) and similar
-      DataFlow::valueNode(astNode.(AssignExpr).getRhs()).(SanitizingRegExpTest).getSanitizedExpr() =
-        expr
+      exists(SanitizingRegExpTest other |
+        other = DataFlow::valueNode(astNode.(AssignExpr).getRhs()) and
+        expr = other.getSanitizedExpr() and
+        sanitizedOutcome = other.getSanitizedOutcome()
+      )
     }
 
     private Expr getSanitizedExpr() { result = expr }
 
+    private boolean getSanitizedOutcome() { result = sanitizedOutcome }
+
     override predicate sanitizes(boolean outcome, Expr e) {
-      (outcome = true or outcome = false) and
+      outcome = sanitizedOutcome and
       e = expr
     }
 

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

@@ -10,7 +10,8 @@ module ExceptionXss {
   import DomBasedXssCustomizations::DomBasedXss as DomBasedXssCustom
   import ReflectedXssCustomizations::ReflectedXss as ReflectedXssCustom
   import Xss as Xss
-  
+  private import semmle.javascript.dataflow.InferredTypes
+
   /**
    * Holds if `node` is unlikely to cause an exception containing sensitive information to be thrown.
    */
@@ -24,16 +25,29 @@ module ExceptionXss {
     node = DataFlow::globalVarRef("console").getAMemberCall(_).getAnArgument()
   }
 
+  /**
+   * Holds if `t` is `null` or `undefined`.
+   */
+  private predicate isNullOrUndefined(InferredType t) {
+    t = TTNull() or
+    t = TTUndefined()
+  }
+
   /**
    * Holds if `node` can possibly cause an exception containing sensitive information to be thrown.
    */
   predicate canThrowSensitiveInformation(DataFlow::Node node) {
-    not isUnlikelyToThrowSensitiveInformation(node) and 
+    not isUnlikelyToThrowSensitiveInformation(node) and
     (
       // in the case of reflective calls the below ensures that both InvokeNodes have no known callee.
-      forex(DataFlow::InvokeNode call | node = call.getAnArgument() | not exists(call.getACallee()))
+      forex(DataFlow::InvokeNode call | call.getAnArgument() = node | not exists(call.getACallee()))
       or
       node.asExpr().getEnclosingStmt() instanceof ThrowStmt
+      or
+      exists(DataFlow::PropRef prop |
+        node = DataFlow::valueNode(prop.getPropertyNameExpr()) and
+        forex(InferredType t | t = prop.getBase().analyze().getAType() | isNullOrUndefined(t))
+      )
     )
   }
 
@@ -47,6 +61,55 @@ module ExceptionXss {
     NotYetThrown() { this = "NotYetThrown" }
   }
 
+  /**
+   * A callback that is the last argument to some call, and the callback has the form:
+   * `function (err, value) {if (err) {...} ... }`
+   */
+  class Callback extends DataFlow::FunctionNode {
+    DataFlow::ParameterNode errorParameter;
+
+    Callback() {
+      exists(DataFlow::CallNode call | call.getLastArgument().getAFunctionValue() = this) and
+      this.getNumParameter() = 2 and
+      errorParameter = this.getParameter(0) and
+      exists(IfStmt ifStmt |
+        ifStmt = this.getFunction().getBodyStmt(0) and
+        errorParameter.flowsToExpr(ifStmt.getCondition())
+      )
+    }
+
+    /**
+     * Get the parameter in the callback that contains an error. 
+     * In the current implementation this is always the first parameter.
+     */
+    DataFlow::Node getErrorParam() { result = errorParameter }
+  }
+
+  /**
+   * Gets the error parameter for a callback that is supplied to the same call as `pred` is an argument to. 
+   * For example: `outerCall(foo, <pred>, bar, (<result>, val) => { ... })`. 
+   */
+  DataFlow::Node getCallbackErrorParam(DataFlow::Node pred) {
+    exists(DataFlow::CallNode call, Callback callback |
+      pred = call.getAnArgument() and
+      call.getLastArgument() = callback and
+      result = callback.getErrorParam() and
+      not pred = callback
+    )
+  }
+
+  /**
+   * Gets the data-flow node to which any exceptions thrown by
+   * this expression will propagate.
+   * This predicate adds, on top of `Expr::getExceptionTarget`, exceptions 
+   * propagated by callbacks. 
+   */
+  private DataFlow::Node getExceptionTarget(DataFlow::Node pred) {
+    result = pred.asExpr().getExceptionTarget()
+    or
+    result = getCallbackErrorParam(pred)
+  }
+
   /**
    * A taint-tracking configuration for reasoning about XSS with possible exceptional flow.
    * Flow labels are used to ensure that we only report taint-flow that has been thrown in
@@ -69,12 +132,15 @@ module ExceptionXss {
       DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl,
       DataFlow::FlowLabel outlbl
     ) {
-      inlbl instanceof NotYetThrown and (outlbl.isTaint() or outlbl instanceof NotYetThrown) and
-      succ = pred.asExpr().getExceptionTarget() and
-      canThrowSensitiveInformation(pred)
+      inlbl instanceof NotYetThrown and
+      (outlbl.isTaint() or outlbl instanceof NotYetThrown) and
+      canThrowSensitiveInformation(pred) and
+      succ = getExceptionTarget(pred)
       or
       // All the usual taint-flow steps apply on data-flow before it has been thrown in an exception.
-      this.isAdditionalFlowStep(pred, succ) and inlbl instanceof NotYetThrown and outlbl instanceof NotYetThrown
+      this.isAdditionalFlowStep(pred, succ) and
+      inlbl instanceof NotYetThrown and
+      outlbl instanceof NotYetThrown
     }
   }
 }