From ea297dd4424d03718a9063dcb3dd9e47a933f179 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Wed, 10 Oct 2018 16:06:44 +0100
Subject: [PATCH 01/10] JS: bugfix in handling of custom flow labels

---
 .../semmle/javascript/dataflow/internal/FlowSteps.qll  | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
index 69646cec816..95fb763f8cd 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
@@ -74,10 +74,12 @@ predicate localFlowStep(DataFlow::Node pred, DataFlow::Node succ,
   any(DataFlow::AdditionalFlowStep afs).step(pred, succ) and predlbl = succlbl
   or
   exists (boolean vp | configuration.isAdditionalFlowStep(pred, succ, vp) |
-    if vp = false and (predlbl = FlowLabel::data() or predlbl = FlowLabel::taint()) then
-      succlbl = FlowLabel::taint()
-    else
-      predlbl = succlbl
+    vp = true and
+    predlbl = succlbl
+    or
+    vp = false and
+    (predlbl = FlowLabel::data() or predlbl = FlowLabel::taint()) and
+    succlbl = FlowLabel::taint()
   )
   or
   configuration.isAdditionalFlowStep(pred, succ, predlbl, succlbl)

From 03b479114fa08c2a7a368229d1dcfc6e7d1cf8ac Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Wed, 10 Oct 2018 17:05:39 +0100
Subject: [PATCH 02/10] JS: preserve document.url label out of .href property

---
 .../javascript/security/dataflow/ClientSideUrlRedirect.qll   | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll
index 35bb5fa950d..dc331b7d2a8 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll
@@ -65,6 +65,11 @@ module ClientSideUrlRedirect {
       queryAccess(pred, succ) and
       f instanceof DocumentUrl and
       g = DataFlow::FlowLabel::taint()
+      or
+      // preserve document.url label in step from `location` to `location.href`
+      f instanceof DocumentUrl and
+      g instanceof DocumentUrl and
+      succ.(DataFlow::PropRead).accesses(pred, "href")
     }
   }
 

From 46b20150652be5f39d922fb30fe2c217c97832f8 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Wed, 10 Oct 2018 16:08:04 +0100
Subject: [PATCH 03/10] JS: fix an outdated comment

---
 javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
index 8a33ebb5c80..bef9a766b17 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
@@ -130,7 +130,7 @@ module TaintTracking {
    * configurations it is used in.
    *
    * Note: For performance reasons, all subclasses of this class should be part
-   * of the standard library. Override `Configuration::isTaintSanitizerGuard`
+   * of the standard library. Override `Configuration::isSanitizer`
    * for analysis-specific taint steps.
    */
   abstract class AdditionalSanitizerGuardNode extends SanitizerGuardNode {

From 396ad336a3f0e2c346b6aec8c6fca498f92818f0 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Wed, 10 Oct 2018 16:09:43 +0100
Subject: [PATCH 04/10] JS: add RemoteFlowSource.isDeepObject() and populate it

---
 .../semmle/javascript/frameworks/Express.qll  | 14 ++++++
 .../javascript/frameworks/ExpressModules.qll  | 48 +++++++++++++++++++
 .../security/dataflow/RemoteFlowSources.qll   |  5 ++
 3 files changed, 67 insertions(+)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
index 484ffaf0c49..c654314cb79 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -488,6 +488,20 @@ module Express {
     override string getKind() {
       result = kind
     }
+
+    override predicate isDeepObject() {
+      kind = "body" and
+      exists (ExpressLibraries::BodyParser bodyParser, RouteHandlerExpr expr |
+        expr.getBody() = rh and
+        bodyParser.isDeepObject() and
+        bodyParser.flowsToExpr(expr.getAMatchingAncestor())
+      )
+      or
+      // If we can't find the middlewares for the route handler,
+      // but all known body parsers are deep, assume req.body is a deep object.
+      kind = "body" and
+      forall(ExpressLibraries::BodyParser bodyParser | bodyParser.isDeepObject())
+    }
   }
 
   /**
diff --git a/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll b/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll
index e7538c54612..2a7beadee5c 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll
@@ -230,4 +230,52 @@ module ExpressLibraries {
 
   }
 
+  /**
+   * An instance of the Express `body-parser` middleware.
+   */
+  class BodyParser extends DataFlow::InvokeNode {
+    string kind;
+
+    BodyParser() {
+      this = DataFlow::moduleImport("body-parser").getACall() and kind = "json"
+      or
+      exists (string moduleName |
+        (moduleName = "body-parser" or moduleName = "express") and
+        (kind = "json" or kind = "urlencoded") and
+        this = DataFlow::moduleMember(moduleName, kind).getACall()
+      )
+    }
+
+    /**
+     * Holds if this is a JSON body parser.
+     */
+    predicate isJson() {
+      kind = "json"
+    }
+
+    /**
+     * Holds if this is a URL-encoded body parser.
+     */
+    predicate isUrlEncoded() {
+      kind = "urlencoded"
+    }
+
+    /**
+     * Holds if this is an extended URL-encoded body parser.
+     *
+     * The extended URL-encoding allows for nested objects, like JSON.
+     */
+    predicate isExtendedUrlEncoded() {
+      kind = "urlencoded" and
+      not getOptionArgument(0, "extended").mayHaveBooleanValue(false)
+    }
+
+    /**
+     * Holds if this parses the input as JSON or extended URL-encoding.
+     */
+    predicate isDeepObject() {
+      isJson() or isExtendedUrlEncoded()
+    }
+  }
+
 }
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll
index a1138a0eb5e..c63a80cad64 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll
@@ -10,6 +10,11 @@ import semmle.javascript.security.dataflow.DOM
 abstract class RemoteFlowSource extends DataFlow::Node {
   /** Gets a string that describes the type of this remote flow source. */
   abstract string getSourceType();
+
+  /**
+   * Holds if this can be a user-controlled deep object, such as a JSON object parsed from user-controlled data.
+   */
+  predicate isDeepObject() { none() }
 }
 
 /**

From b70f70f7227d9540ff274434c0b0f4a2a89ef2c7 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Wed, 10 Oct 2018 16:37:11 +0100
Subject: [PATCH 05/10] JS: Add TaintedObject flow label library

---
 .../javascript/security/TaintedObject.qll     | 84 +++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100644 javascript/ql/src/semmle/javascript/security/TaintedObject.qll

diff --git a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
new file mode 100644
index 00000000000..fb45e6dd753
--- /dev/null
+++ b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
@@ -0,0 +1,84 @@
+/**
+ * Provides methods for reasoning about the flow of deeply tainted objects, such as JSON objects
+ * parsed from user-controlled data.
+ *
+ * Deeply tainted objects are arrays or objects with user-controlled property names, containing
+ * tainted values or deeply tainted objects in their properties.
+ *
+ * To track deeply tainted objects, a flow-tracking configuration should generally include the following:
+ *
+ * 1. One or more sinks associated with the label `TaintedObject::label()`.
+ * 2. The sources from `TaintedObject::isSource`.
+ * 3. The flow steps from `TaintedObject::step`.
+ */
+import javascript
+
+module TaintedObject {
+  private import DataFlow
+
+  private class TaintedObjectLabel extends FlowLabel {
+    TaintedObjectLabel() { this = "tainted-object" }
+  }
+
+  /**
+   * Gets the flow label representing a deeply tainted objects.
+   *
+   * A "tainted object" is an array or object whose values are all assumed to be tainted as well.
+   *
+   * Note that the presence of the `object-taint` label generally implies the presence of the `taint` label as well.
+   */
+  FlowLabel label() { result instanceof TaintedObjectLabel  }
+
+  /**
+   * Holds for the flows steps that are relevant for tracking user-controlled JSON objects.
+   */
+  predicate step(Node src, Node trg, FlowLabel inlbl, FlowLabel outlbl) {
+    // JSON parsers map tainted inputs to tainted JSON
+    (inlbl = FlowLabel::data() or inlbl = FlowLabel::taint()) and
+    outlbl = label() and
+    exists (JsonParserCall parse |
+      src = parse.getInput() and
+      trg = parse.getOutput())
+    or
+    // Property reads preserve deep object taint.
+    inlbl = label() and
+    outlbl = label() and
+    trg.(PropRead).getBase() = src
+    or
+    // Property projection preserves deep object taint
+    inlbl = label() and
+    outlbl = label() and
+    trg.(PropertyProjection).getObject() = src
+    or
+    // Extending objects preserves deep object taint
+    inlbl = label() and
+    outlbl = label() and
+    exists (ExtendCall call |
+      src = call.getAnOperand() and
+      trg = call
+      or
+      src = call.getASourceOperand() and
+      trg = call.getDestinationOperand().getALocalSource())
+  }
+
+  /**
+   * Holds if `node` is a source of JSON taint and label is the JSON taint label.
+   */
+  predicate isSource(Node source, FlowLabel label) {
+    source instanceof Source and label = label()
+  }
+
+  /**
+   * A source of a user-controlled deep object object.
+   */
+  abstract class Source extends DataFlow::Node {}
+
+  /** Request input accesses as a JSON source. */
+  private class RequestInputAsSource extends Source {
+    RequestInputAsSource() {
+      this.(HTTP::RequestInputAccess).isDeepObject()
+    }
+  }
+
+  // TODO: string tests should be classified as sanitizer guards; need support for flow labels on guards
+}

From d72d7345b8c89f213fbbad5259a82937d9e7599b Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Wed, 10 Oct 2018 16:39:11 +0100
Subject: [PATCH 06/10] JS: make NosqlInjection use object taint

---
 .../src/Security/CWE-089/SqlInjection.actual  |  0
 .../security/dataflow/NosqlInjection.qll      | 30 ++++++++++++++----
 .../Security/CWE-089/SqlInjection.expected    |  1 -
 .../query-tests/Security/CWE-089/mongodb.js   |  8 ++++-
 .../Security/CWE-089/mongodb_bodySafe.js      | 31 +++++++++++++++++++
 5 files changed, 62 insertions(+), 8 deletions(-)
 create mode 100644 javascript/ql/src/Security/CWE-089/SqlInjection.actual
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-089/mongodb_bodySafe.js

diff --git a/javascript/ql/src/Security/CWE-089/SqlInjection.actual b/javascript/ql/src/Security/CWE-089/SqlInjection.actual
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll
index 9c031728d7e..571ca4d4964 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll
@@ -4,6 +4,7 @@
  */
 
 import javascript
+import semmle.javascript.security.TaintedObject
 
 module NosqlInjection {
   /**
@@ -14,7 +15,16 @@ module NosqlInjection {
   /**
    * A data flow sink for SQL-injection vulnerabilities.
    */
-  abstract class Sink extends DataFlow::Node { }
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets a flow label relevant for this sink.
+     *
+     * Defaults to deeply tainted objects only.
+     */
+    DataFlow::FlowLabel getAFlowLabel() {
+      result = TaintedObject::label()
+    }
+  }
 
   /**
    * A sanitizer for SQL-injection vulnerabilities.
@@ -31,8 +41,12 @@ module NosqlInjection {
       source instanceof Source
     }
 
-    override predicate isSink(DataFlow::Node sink) {
-      sink instanceof Sink
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      TaintedObject::isSource(source, label)
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink.(Sink).getAFlowLabel() = label
     }
 
     override predicate isSanitizer(DataFlow::Node node) {
@@ -40,12 +54,16 @@ module NosqlInjection {
       node instanceof Sanitizer
     }
 
-    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl) {
+      TaintedObject::step(src, trg, inlbl, outlbl)
+      or
       // additional flow step to track taint through NoSQL query objects
+      inlbl = TaintedObject::label() and
+      outlbl = TaintedObject::label() and
       exists (NoSQL::Query query, DataFlow::SourceNode queryObj |
         queryObj.flowsToExpr(query) and
-        queryObj.flowsTo(succ) and
-        pred = queryObj.getAPropertyWrite().getRhs()
+        queryObj.flowsTo(trg) and
+        src = queryObj.getAPropertyWrite().getRhs()
       )
     }
   }
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
index 7a99482adb8..d432a2a9363 100644
--- a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
@@ -1,5 +1,4 @@
 | mongodb.js:18:16:18:20 | query | This query depends on $@. | mongodb.js:13:19:13:26 | req.body | a user-provided value |
-| mongodb.js:39:16:39:20 | query | This query depends on $@. | mongodb.js:34:19:34:33 | req.query.title | a user-provided value |
 | mongoose.js:27:20:27:24 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
 | mongoose.js:30:25:30:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
 | mongoose.js:33:24:33:28 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js b/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
index 900cad1cbba..f8c41a55f76 100644
--- a/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
+++ b/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
@@ -16,6 +16,12 @@ app.post('/documents/find', (req, res) => {
 
       // NOT OK: query is tainted by user-provided object value
       doc.find(query);
+
+      // OK: user-data is coerced to a string
+      doc.find({ title: '' + query.body.title });
+
+      // OK: throws unless user-data is a string
+      doc.find({ title: query.body.title.substr(1) });
     });
 });
 
@@ -36,6 +42,6 @@ app.post('/documents/find', (req, res) => {
       let doc = db.collection('doc');
 
       // NOT OK: query is tainted by user-provided object value
-      doc.find(query);
+      doc.find(query); // Not currently detected 
     });
 });
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/mongodb_bodySafe.js b/javascript/ql/test/query-tests/Security/CWE-089/mongodb_bodySafe.js
new file mode 100644
index 00000000000..61a87bfaa5a
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-089/mongodb_bodySafe.js
@@ -0,0 +1,31 @@
+const express = require('express'),
+      mongodb = require('mongodb'),
+      bodyParser = require('body-parser');
+
+const MongoClient = mongodb.MongoClient;
+
+const app = express();
+
+app.use(bodyParser.urlencoded({ extended: false }));
+
+app.post('/documents/find', (req, res) => {
+    const query = {};
+    query.title = req.body.title;
+    MongoClient.connect('mongodb://localhost:27017/test', (err, db) => {
+      let doc = db.collection('doc');
+
+      // OK: req.body is safe
+      doc.find(query);
+    });
+});
+
+app.post('/documents/find', (req, res) => {
+    const query = {};
+    query.title = req.query.title;
+    MongoClient.connect('mongodb://localhost:27017/test', (err, db) => {
+      let doc = db.collection('doc');
+
+      // NOT OK: regardless of body parser, query value is still tainted
+      doc.find(query);
+    });
+});

From 5e720486d54b74b45d7601c31a227bd903df5b34 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Wed, 10 Oct 2018 17:15:56 +0100
Subject: [PATCH 07/10] JS: recognize req.query.x as deep object taint

---
 .../ql/src/semmle/javascript/frameworks/Express.qll   | 11 +++++++++++
 .../Security/CWE-089/SqlInjection.expected            |  2 ++
 .../ql/test/query-tests/Security/CWE-089/mongodb.js   |  2 +-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
index c654314cb79..6a2c7af99d0 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -501,6 +501,17 @@ module Express {
       // but all known body parsers are deep, assume req.body is a deep object.
       kind = "body" and
       forall(ExpressLibraries::BodyParser bodyParser | bodyParser.isDeepObject())
+      or
+      kind = "parameter" and
+      exists (DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
+        this.(DataFlow::MethodCallNode).calls(request, "param")
+        or
+        exists (DataFlow::PropRead base |
+          // `req.query.name`
+          base.accesses(request, "query") and
+          this = base.getAPropertyReference(_)
+        )
+      )
     }
   }
 
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
index d432a2a9363..f5279170af9 100644
--- a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
@@ -1,4 +1,6 @@
 | mongodb.js:18:16:18:20 | query | This query depends on $@. | mongodb.js:13:19:13:26 | req.body | a user-provided value |
+| mongodb.js:45:16:45:20 | query | This query depends on $@. | mongodb.js:40:19:40:33 | req.query.title | a user-provided value |
+| mongodb_bodySafe.js:29:16:29:20 | query | This query depends on $@. | mongodb_bodySafe.js:24:19:24:33 | req.query.title | a user-provided value |
 | mongoose.js:27:20:27:24 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
 | mongoose.js:30:25:30:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
 | mongoose.js:33:24:33:28 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js b/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
index f8c41a55f76..3c40b43a13e 100644
--- a/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
+++ b/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
@@ -42,6 +42,6 @@ app.post('/documents/find', (req, res) => {
       let doc = db.collection('doc');
 
       // NOT OK: query is tainted by user-provided object value
-      doc.find(query); // Not currently detected 
+      doc.find(query);
     });
 });

From 9b10254cd498a37aa51440c2c81df4c7215e7d08 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Wed, 10 Oct 2018 18:01:57 +0100
Subject: [PATCH 08/10] JS: support label-specific sanitizer guards

---
 .../javascript/dataflow/Configuration.qll     | 31 +++++++++++++--
 .../javascript/security/TaintedObject.qll     | 39 ++++++++++++++++++-
 .../security/dataflow/NosqlInjection.qll      |  4 ++
 .../Security/CWE-089/SqlInjection.expected    |  3 +-
 .../query-tests/Security/CWE-089/mongodb.js   |  9 +++++
 5 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
index 8125784a5e3..6dc998c2d49 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -146,6 +146,7 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node node) {
     exists (BarrierGuardNode guard |
+      guard.blocksAllLabels() and
       isBarrierGuard(guard) and
       guard.blocks(node)
     )
@@ -161,6 +162,17 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node src, DataFlow::Node trg, FlowLabel lbl) { none() }
 
+  /**
+   * Holds if flow with label `lbl` cannot flow into `node`.
+   */
+  predicate isBarrierForLabel(DataFlow::Node node, FlowLabel lbl) {
+    exists (BarrierGuardNode guard |
+      guard.blocksSpecificLabel(lbl) and
+      isBarrierGuard(guard) and
+      guard.blocks(node)
+    )
+  }
+
   /**
    * Holds if data flow node `guard` can act as a barrier when appearing
    * in a condition.
@@ -298,6 +310,15 @@ abstract class BarrierGuardNode extends DataFlow::Node {
    */
   abstract predicate blocks(boolean outcome, Expr e);
 
+  /**
+   * Holds if this barrier guard blocks all labels.
+   */
+  predicate blocksAllLabels() { any() }
+
+  /**
+   * Holds if this barrier guard only blocks specific labels, and `label` is one of them.
+   */
+  predicate blocksSpecificLabel(FlowLabel label) { none() }
 }
 
 /**
@@ -570,7 +591,8 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
     ret.asExpr() = f.getAReturnedExpr() and
     calls(invk, f) and // Do not consider partial calls
     reachableFromInput(f, invk, input, ret, cfg, summary) and
-    not cfg.isBarrier(ret, invk)
+    not cfg.isBarrier(ret, invk) and
+    not cfg.isBarrierForLabel(invk, summary.getEndLabel())
   )
 }
 
@@ -641,7 +663,8 @@ private predicate flowStep(DataFlow::Node pred, DataFlow::Configuration cfg,
    flowThroughProperty(pred, succ, cfg, summary)
   ) and
   not cfg.isBarrier(succ) and
-  not cfg.isBarrier(pred, succ)
+  not cfg.isBarrier(pred, succ) and
+  not cfg.isBarrierForLabel(succ, summary.getEndLabel())
 }
 
 /**
@@ -666,6 +689,7 @@ private predicate reachableFromSource(DataFlow::Node nd, DataFlow::Configuration
   exists (FlowLabel lbl |
     isSource(nd, cfg, lbl) and
     not cfg.isBarrier(nd) and
+    not cfg.isBarrierForLabel(nd, lbl) and
     summary = MkPathSummary(false, false, lbl, lbl)
   )
   or
@@ -684,7 +708,8 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg,
                          PathSummary summary) {
   reachableFromSource(nd, cfg, summary) and
   isSink(nd, cfg, summary.getEndLabel()) and
-  not cfg.isBarrier(nd)
+  not cfg.isBarrier(nd) and
+  not cfg.isBarrierForLabel(nd, summary.getEndLabel())
   or
   exists (DataFlow::Node mid, PathSummary stepSummary |
     reachableFromSource(nd, cfg, summary) and
diff --git a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
index fb45e6dd753..4f7d715b30b 100644
--- a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
+++ b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
@@ -10,6 +10,7 @@
  * 1. One or more sinks associated with the label `TaintedObject::label()`.
  * 2. The sources from `TaintedObject::isSource`.
  * 3. The flow steps from `TaintedObject::step`.
+ * 4. The sanitizing guards `TaintedObject::SanitizerGuard`.
  */
 import javascript
 
@@ -80,5 +81,41 @@ module TaintedObject {
     }
   }
 
-  // TODO: string tests should be classified as sanitizer guards; need support for flow labels on guards
+  /**
+   * Sanitizer guard that blocks deep object taint.
+   */
+  abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode {
+    override predicate blocksAllLabels() { none() }
+
+    override predicate blocksSpecificLabel(FlowLabel label) {
+      label = label()
+    }
+  }
+
+  /**
+   * A test of form `typeof x === "something"`, preventing `x` from being an object in some cases.
+   */
+  private class TypeTestGuard extends SanitizerGuard, ValueNode {
+    override EqualityTest astNode;
+    TypeofExpr typeof;
+    boolean polarity;
+
+    TypeTestGuard() {
+      astNode.getAnOperand() = typeof and
+      (
+        // typeof x === "object" sanitizes `x` when it evaluates to false
+        astNode.getAnOperand().getStringValue() = "object" and
+        polarity = astNode.getPolarity().booleanNot()
+        or
+        // typeof x === "string" sanitizes `x` when it evaluates to true
+        astNode.getAnOperand().getStringValue() != "object" and
+        polarity = astNode.getPolarity()
+      )
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      polarity = outcome and
+      e = typeof.getOperand()
+    }
+  }
 }
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll
index 571ca4d4964..c338e6ced50 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll
@@ -54,6 +54,10 @@ module NosqlInjection {
       node instanceof Sanitizer
     }
 
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof TaintedObject::SanitizerGuard
+    }
+
     override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl) {
       TaintedObject::step(src, trg, inlbl, outlbl)
       or
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
index f5279170af9..e6c84156dd0 100644
--- a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected
@@ -1,5 +1,6 @@
 | mongodb.js:18:16:18:20 | query | This query depends on $@. | mongodb.js:13:19:13:26 | req.body | a user-provided value |
-| mongodb.js:45:16:45:20 | query | This query depends on $@. | mongodb.js:40:19:40:33 | req.query.title | a user-provided value |
+| mongodb.js:32:18:32:45 | { title ... itle) } | This query depends on $@. | mongodb.js:26:19:26:26 | req.body | a user-provided value |
+| mongodb.js:54:16:54:20 | query | This query depends on $@. | mongodb.js:49:19:49:33 | req.query.title | a user-provided value |
 | mongodb_bodySafe.js:29:16:29:20 | query | This query depends on $@. | mongodb_bodySafe.js:24:19:24:33 | req.query.title | a user-provided value |
 | mongoose.js:27:20:27:24 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
 | mongoose.js:30:25:30:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js b/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
index 3c40b43a13e..00f3422ca40 100644
--- a/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
+++ b/javascript/ql/test/query-tests/Security/CWE-089/mongodb.js
@@ -22,6 +22,15 @@ app.post('/documents/find', (req, res) => {
 
       // OK: throws unless user-data is a string
       doc.find({ title: query.body.title.substr(1) });
+
+      let title = req.body.title;
+      if (typeof title === "string") {
+        // OK: input checked to be a string
+        doc.find({ title: title });
+
+        // NOT OK: input is parsed as JSON after string check
+        doc.find({ title: JSON.parse(title) });
+      }
     });
 });
 

From da3e960e39c63b23e827f0a72a48dfbd94c92efb Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Thu, 11 Oct 2018 12:45:45 +0100
Subject: [PATCH 09/10] JS: address review comments

---
 .../ql/src/semmle/javascript/dataflow/Configuration.qll    | 7 ++++++-
 .../ql/src/semmle/javascript/security/TaintedObject.qll    | 6 +++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
index 6dc998c2d49..6daa44daa6c 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -311,12 +311,17 @@ abstract class BarrierGuardNode extends DataFlow::Node {
   abstract predicate blocks(boolean outcome, Expr e);
 
   /**
-   * Holds if this barrier guard blocks all labels.
+   * Holds if this barrier guard should block all labels.
+   *
+   * To block specific labels only, subclasses should override this with `none()` and
+   * also override `blocksSpecificLabel`.
    */
   predicate blocksAllLabels() { any() }
 
   /**
    * Holds if this barrier guard only blocks specific labels, and `label` is one of them.
+   *
+   * Subclasses that override this predicate should also override `blocksAllLabels`.
    */
   predicate blocksSpecificLabel(FlowLabel label) { none() }
 }
diff --git a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
index 4f7d715b30b..6778a65d2e6 100644
--- a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
+++ b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
@@ -22,11 +22,11 @@ module TaintedObject {
   }
 
   /**
-   * Gets the flow label representing a deeply tainted objects.
+   * Gets the flow label representing a deeply tainted object.
    *
-   * A "tainted object" is an array or object whose values are all assumed to be tainted as well.
+   * A "tainted object" is an array or object whose properties values are all assumed to be tainted as well.
    *
-   * Note that the presence of the `object-taint` label generally implies the presence of the `taint` label as well.
+   * Note that the presence of the this label generally implies the presence of the `taint` label as well.
    */
   FlowLabel label() { result instanceof TaintedObjectLabel  }
 

From b72e2aa6029d6998dd8a24b96c957159a66a7730 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Thu, 18 Oct 2018 11:21:27 +0100
Subject: [PATCH 10/10] JS: address comments and introduce
 LabeledBarrierGuardNode

---
 .../javascript/dataflow/Configuration.qll     | 39 ++++++++-----------
 .../javascript/dataflow/TaintTracking.qll     |  6 +++
 .../semmle/javascript/frameworks/Express.qll  |  6 +--
 .../javascript/frameworks/ExpressModules.qll  |  5 ++-
 .../javascript/security/TaintedObject.qll     | 14 +++----
 .../security/dataflow/RemoteFlowSources.qll   |  4 +-
 6 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
index 6daa44daa6c..2ef42c450d1 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -93,7 +93,7 @@ abstract class Configuration extends string {
   }
 
   /**
-   * Holds if `source` is a source of flow labelled with `lbl` that is relevant
+   * Holds if `source` is a source of flow labeled with `lbl` that is relevant
    * for this configuration.
    */
   predicate isSource(DataFlow::Node source, FlowLabel lbl) {
@@ -108,7 +108,7 @@ abstract class Configuration extends string {
   }
 
   /**
-   * Holds if `sink` is a sink of flow labelled with `lbl` that is relevant
+   * Holds if `sink` is a sink of flow labeled with `lbl` that is relevant
    * for this configuration.
    */
   predicate isSink(DataFlow::Node sink, FlowLabel lbl) {
@@ -146,7 +146,7 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node node) {
     exists (BarrierGuardNode guard |
-      guard.blocksAllLabels() and
+      not guard instanceof LabeledBarrierGuardNode and
       isBarrierGuard(guard) and
       guard.blocks(node)
     )
@@ -165,9 +165,9 @@ abstract class Configuration extends string {
   /**
    * Holds if flow with label `lbl` cannot flow into `node`.
    */
-  predicate isBarrierForLabel(DataFlow::Node node, FlowLabel lbl) {
-    exists (BarrierGuardNode guard |
-      guard.blocksSpecificLabel(lbl) and
+  predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) {
+    exists (LabeledBarrierGuardNode guard |
+      lbl = guard.getALabel() and
       isBarrierGuard(guard) and
       guard.blocks(node)
     )
@@ -309,21 +309,16 @@ abstract class BarrierGuardNode extends DataFlow::Node {
    * Holds if this node blocks expression `e` provided it evaluates to `outcome`.
    */
   abstract predicate blocks(boolean outcome, Expr e);
+}
 
+/**
+ * A guard node that only blocks specific labels.
+ */
+abstract class LabeledBarrierGuardNode extends BarrierGuardNode {
   /**
-   * Holds if this barrier guard should block all labels.
-   *
-   * To block specific labels only, subclasses should override this with `none()` and
-   * also override `blocksSpecificLabel`.
+   * Get a flow label blocked by this guard node.
    */
-  predicate blocksAllLabels() { any() }
-
-  /**
-   * Holds if this barrier guard only blocks specific labels, and `label` is one of them.
-   *
-   * Subclasses that override this predicate should also override `blocksAllLabels`.
-   */
-  predicate blocksSpecificLabel(FlowLabel label) { none() }
+  abstract FlowLabel getALabel();
 }
 
 /**
@@ -597,7 +592,7 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
     calls(invk, f) and // Do not consider partial calls
     reachableFromInput(f, invk, input, ret, cfg, summary) and
     not cfg.isBarrier(ret, invk) and
-    not cfg.isBarrierForLabel(invk, summary.getEndLabel())
+    not cfg.isLabeledBarrier(invk, summary.getEndLabel())
   )
 }
 
@@ -669,7 +664,7 @@ private predicate flowStep(DataFlow::Node pred, DataFlow::Configuration cfg,
   ) and
   not cfg.isBarrier(succ) and
   not cfg.isBarrier(pred, succ) and
-  not cfg.isBarrierForLabel(succ, summary.getEndLabel())
+  not cfg.isLabeledBarrier(succ, summary.getEndLabel())
 }
 
 /**
@@ -694,7 +689,7 @@ private predicate reachableFromSource(DataFlow::Node nd, DataFlow::Configuration
   exists (FlowLabel lbl |
     isSource(nd, cfg, lbl) and
     not cfg.isBarrier(nd) and
-    not cfg.isBarrierForLabel(nd, lbl) and
+    not cfg.isLabeledBarrier(nd, lbl) and
     summary = MkPathSummary(false, false, lbl, lbl)
   )
   or
@@ -714,7 +709,7 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg,
   reachableFromSource(nd, cfg, summary) and
   isSink(nd, cfg, summary.getEndLabel()) and
   not cfg.isBarrier(nd) and
-  not cfg.isBarrierForLabel(nd, summary.getEndLabel())
+  not cfg.isLabeledBarrier(nd, summary.getEndLabel())
   or
   exists (DataFlow::Node mid, PathSummary stepSummary |
     reachableFromSource(nd, cfg, summary) and
diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
index bef9a766b17..53cf8767f6d 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
@@ -159,6 +159,12 @@ module TaintTracking {
 
   }
 
+  /**
+   * A sanitizer guard node that only blocks specific flow labels.
+   */
+  abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode, DataFlow::LabeledBarrierGuardNode {
+  }
+
   /**
    * DEPRECATED: Override `Configuration::isAdditionalTaintStep` or use
    * `AdditionalTaintStep` instead.
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
index 6a2c7af99d0..de7f12e3f3b 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -489,18 +489,18 @@ module Express {
       result = kind
     }
 
-    override predicate isDeepObject() {
+    override predicate isUserControlledObject() {
       kind = "body" and
       exists (ExpressLibraries::BodyParser bodyParser, RouteHandlerExpr expr |
         expr.getBody() = rh and
-        bodyParser.isDeepObject() and
+        bodyParser.producesUserControlledObjects() and
         bodyParser.flowsToExpr(expr.getAMatchingAncestor())
       )
       or
       // If we can't find the middlewares for the route handler,
       // but all known body parsers are deep, assume req.body is a deep object.
       kind = "body" and
-      forall(ExpressLibraries::BodyParser bodyParser | bodyParser.isDeepObject())
+      forall(ExpressLibraries::BodyParser bodyParser | bodyParser.producesUserControlledObjects())
       or
       kind = "parameter" and
       exists (DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
diff --git a/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll b/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll
index 2a7beadee5c..6f184880b16 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll
@@ -271,9 +271,10 @@ module ExpressLibraries {
     }
 
     /**
-     * Holds if this parses the input as JSON or extended URL-encoding.
+     * Holds if this parses the input as JSON or extended URL-encoding, resulting
+     * in user-controlled objects (as opposed to user-controlled strings).
      */
-    predicate isDeepObject() {
+    predicate producesUserControlledObjects() {
       isJson() or isExtendedUrlEncoded()
     }
   }
diff --git a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
index 6778a65d2e6..efb5545b2c3 100644
--- a/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
+++ b/javascript/ql/src/semmle/javascript/security/TaintedObject.qll
@@ -24,7 +24,7 @@ module TaintedObject {
   /**
    * Gets the flow label representing a deeply tainted object.
    *
-   * A "tainted object" is an array or object whose properties values are all assumed to be tainted as well.
+   * A "tainted object" is an array or object whose property values are all assumed to be tainted as well.
    *
    * Note that the presence of the this label generally implies the presence of the `taint` label as well.
    */
@@ -70,25 +70,23 @@ module TaintedObject {
   }
 
   /**
-   * A source of a user-controlled deep object object.
+   * A source of a user-controlled deep object.
    */
   abstract class Source extends DataFlow::Node {}
 
   /** Request input accesses as a JSON source. */
   private class RequestInputAsSource extends Source {
     RequestInputAsSource() {
-      this.(HTTP::RequestInputAccess).isDeepObject()
+      this.(HTTP::RequestInputAccess).isUserControlledObject()
     }
   }
 
   /**
    * Sanitizer guard that blocks deep object taint.
    */
-  abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode {
-    override predicate blocksAllLabels() { none() }
-
-    override predicate blocksSpecificLabel(FlowLabel label) {
-      label = label()
+  abstract class SanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode {
+    override FlowLabel getALabel() {
+      result = label()
     }
   }
 
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll
index c63a80cad64..964f9e988ef 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll
@@ -12,9 +12,9 @@ abstract class RemoteFlowSource extends DataFlow::Node {
   abstract string getSourceType();
 
   /**
-   * Holds if this can be a user-controlled deep object, such as a JSON object parsed from user-controlled data.
+   * Holds if this can be a user-controlled object, such as a JSON object parsed from user-controlled data.
    */
-  predicate isDeepObject() { none() }
+  predicate isUserControlledObject() { none() }
 }
 
 /**