From a400a1e9d4cd4542767a8e78e7b2afb8c73fb4a3 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 10:17:57 +0200
Subject: [PATCH 01/16] split the markdown steps into a separate class

---
 .../semmle/javascript/frameworks/Markdown.qll | 247 ++++++++++--------
 1 file changed, 134 insertions(+), 113 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
index 743341325a7a..788e28e2e9e2 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
@@ -2,159 +2,180 @@
  * Provides classes for modelling common markdown parsers and generators.
  */
 
+import semmle.javascript.Unit
 import javascript
 
 /**
- * A taint step for the `marked` library, that converts markdown to HTML.
+ * A module providing taint-steps for common markdown parsers and generators.
  */
-private class MarkedStep extends TaintTracking::SharedTaintStep {
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(DataFlow::CallNode call |
-      call = DataFlow::globalVarRef("marked").getACall()
-      or
-      call = DataFlow::moduleImport("marked").getACall()
-    |
-      succ = call and
-      pred = call.getArgument(0)
-    )
+module Markdown {
+  /**
+   * A taint-step that parses a markdown document, but preserves taint.import
+   */
+  class MarkdownStep extends Unit {
+    abstract predicate step(DataFlow::Node pred, DataFlow::Node succ);
   }
-}
 
-/**
- * A taint step for the `markdown-table` library.
- */
-private class MarkdownTableStep extends TaintTracking::SharedTaintStep {
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(DataFlow::CallNode call | call = DataFlow::moduleImport("markdown-table").getACall() |
-      succ = call and
-      pred = call.getArgument(0)
-    )
+  private class MarkdownStepAsTaintStep extends TaintTracking::SharedTaintStep {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      any(MarkdownStep step).step(pred, succ)
+    }
   }
-}
 
-/**
- * A taint step for the `showdown` library.
- */
-private class ShowDownStep extends TaintTracking::SharedTaintStep {
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(DataFlow::CallNode call |
-      call =
-        [DataFlow::globalVarRef("showdown"), DataFlow::moduleImport("showdown")]
-            .getAConstructorInvocation("Converter")
-            .getAMemberCall(["makeHtml", "makeMd"])
-    |
-      succ = call and
-      pred = call.getArgument(0)
-    )
+  /**
+   * A taint step for the `marked` library, that converts markdown to HTML.
+   */
+  private class MarkedStep extends MarkdownStep {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      exists(DataFlow::CallNode call |
+        call = DataFlow::globalVarRef("marked").getACall()
+        or
+        call = DataFlow::moduleImport("marked").getACall()
+      |
+        succ = call and
+        pred = call.getArgument(0)
+      )
+    }
   }
-}
 
-/**
- * Classes and predicates for modelling taint steps in `unified` and `remark`.
- */
-private module Unified {
   /**
-   * The creation of a parser from `unified`.
-   * The `remark` module is a shorthand that initializes `unified` with a markdown parser.
+   * A taint step for the `markdown-table` library.
    */
-  DataFlow::CallNode unified() { result = DataFlow::moduleImport(["unified", "remark"]).getACall() }
+  private class MarkdownTableStep extends MarkdownStep {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      exists(DataFlow::CallNode call | call = DataFlow::moduleImport("markdown-table").getACall() |
+        succ = call and
+        pred = call.getArgument(0)
+      )
+    }
+  }
 
   /**
-   * A chain of method calls that process an input with `unified`.
+   * A taint step for the `showdown` library.
    */
-  class UnifiedChain extends DataFlow::CallNode {
-    DataFlow::CallNode root;
-
-    UnifiedChain() {
-      root = unified() and
-      this = root.getAChainedMethodCall(["process", "processSync"])
+  private class ShowDownStep extends MarkdownStep {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      exists(DataFlow::CallNode call |
+        call =
+          [DataFlow::globalVarRef("showdown"), DataFlow::moduleImport("showdown")]
+              .getAConstructorInvocation("Converter")
+              .getAMemberCall(["makeHtml", "makeMd"])
+      |
+        succ = call and
+        pred = call.getArgument(0)
+      )
     }
+  }
 
+  /**
+   * Classes and predicates for modelling taint steps in `unified` and `remark`.
+   */
+  private module Unified {
     /**
-     * Gets a plugin that is used in this chain.
+     * The creation of a parser from `unified`.
+     * The `remark` module is a shorthand that initializes `unified` with a markdown parser.
      */
-    DataFlow::Node getAUsedPlugin() { result = root.getAChainedMethodCall("use").getArgument(0) }
+    DataFlow::CallNode unified() {
+      result = DataFlow::moduleImport(["unified", "remark"]).getACall()
+    }
 
     /**
-     * Gets the input that is processed.
+     * A chain of method calls that process an input with `unified`.
      */
-    DataFlow::Node getInput() { result = getArgument(0) }
+    class UnifiedChain extends DataFlow::CallNode {
+      DataFlow::CallNode root;
+
+      UnifiedChain() {
+        root = unified() and
+        this = root.getAChainedMethodCall(["process", "processSync"])
+      }
+
+      /**
+       * Gets a plugin that is used in this chain.
+       */
+      DataFlow::Node getAUsedPlugin() { result = root.getAChainedMethodCall("use").getArgument(0) }
+
+      /**
+       * Gets the input that is processed.
+       */
+      DataFlow::Node getInput() { result = getArgument(0) }
+
+      /**
+       * Gets the processed output.
+       */
+      DataFlow::Node getOutput() {
+        this.getCalleeName() = "process" and result = getABoundCallbackParameter(1, 1)
+        or
+        this.getCalleeName() = "processSync" and result = this
+      }
+    }
 
     /**
-     * Gets the processed output.
+     * A taint step for the `unified` library.
      */
-    DataFlow::Node getOutput() {
-      this.getCalleeName() = "process" and result = getABoundCallbackParameter(1, 1)
-      or
-      this.getCalleeName() = "processSync" and result = this
+    class UnifiedStep extends MarkdownStep {
+      override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+        exists(UnifiedChain chain |
+          // sanitizer. Mostly looking for `rehype-sanitize`, but also other plugins with `sanitize` in their name.
+          not chain.getAUsedPlugin().getALocalSource() =
+            DataFlow::moduleImport(any(string s | s.matches("%sanitize%")))
+        |
+          pred = chain.getInput() and
+          succ = chain.getOutput()
+        )
+      }
     }
   }
 
   /**
-   * A taint step for the `unified` library.
+   * A taint step for the `snarkdown` library.
    */
-  class UnifiedStep extends TaintTracking::SharedTaintStep {
+  private class SnarkdownStep extends MarkdownStep {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      exists(UnifiedChain chain |
-        // sanitizer. Mostly looking for `rehype-sanitize`, but also other plugins with `sanitize` in their name.
-        not chain.getAUsedPlugin().getALocalSource() =
-          DataFlow::moduleImport(any(string s | s.matches("%sanitize%")))
-      |
-        pred = chain.getInput() and
-        succ = chain.getOutput()
+      exists(DataFlow::CallNode call | call = DataFlow::moduleImport("snarkdown").getACall() |
+        call = succ and
+        pred = call.getArgument(0)
       )
     }
   }
-}
 
-/**
- * A taint step for the `snarkdown` library.
- */
-private class SnarkdownStep extends TaintTracking::SharedTaintStep {
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(DataFlow::CallNode call | call = DataFlow::moduleImport("snarkdown").getACall() |
-      call = succ and
-      pred = call.getArgument(0)
-    )
-  }
-}
-
-/**
- * Classes and predicates for modelling taint steps the `markdown-it` library.
- */
-private module MarkdownIt {
   /**
-   * The creation of a parser from `markdown-it`.
+   * Classes and predicates for modelling taint steps the `markdown-it` library.
    */
-  private API::Node markdownIt() {
-    exists(API::InvokeNode call |
-      call = API::moduleImport("markdown-it").getAnInvocation()
+  private module MarkdownIt {
+    /**
+     * The creation of a parser from `markdown-it`.
+     */
+    private API::Node markdownIt() {
+      exists(API::InvokeNode call |
+        call = API::moduleImport("markdown-it").getAnInvocation()
+        or
+        call = API::moduleImport("markdown-it").getMember("Markdown").getAnInvocation()
+      |
+        call.getParameter(0).getMember("html").getARhs().mayHaveBooleanValue(true) and
+        result = call.getReturn()
+      )
       or
-      call = API::moduleImport("markdown-it").getMember("Markdown").getAnInvocation()
-    |
-      call.getParameter(0).getMember("html").getARhs().mayHaveBooleanValue(true) and
-      result = call.getReturn()
-    )
-    or
-    exists(API::CallNode call |
-      call = markdownIt().getMember(["use", "set", "configure", "enable", "disable"]).getACall() and
-      result = call.getReturn() and
-      not call.getParameter(0).getAValueReachingRhs() =
-        DataFlow::moduleImport("markdown-it-sanitizer")
-    )
-  }
-
-  /**
-   * A taint step for the `markdown-it` library.
-   */
-  private class MarkdownItStep extends TaintTracking::SharedTaintStep {
-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       exists(API::CallNode call |
-        call = markdownIt().getMember(["render", "renderInline"]).getACall()
-      |
-        succ = call and
-        pred = call.getArgument(0)
+        call = markdownIt().getMember(["use", "set", "configure", "enable", "disable"]).getACall() and
+        result = call.getReturn() and
+        not call.getParameter(0).getAValueReachingRhs() =
+          DataFlow::moduleImport("markdown-it-sanitizer")
       )
     }
+
+    /**
+     * A taint step for the `markdown-it` library.
+     */
+    private class MarkdownItStep extends MarkdownStep {
+      override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+        exists(API::CallNode call |
+          call = markdownIt().getMember(["render", "renderInline"]).getACall()
+        |
+          succ = call and
+          pred = call.getArgument(0)
+        )
+      }
+    }
   }
 }

From e86a3b5e577fc603c5cf1dd171c191b7e692e373 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 10:21:25 +0200
Subject: [PATCH 02/16] add js/html-constructed-from-input query

---
 .../CWE-079/UnsafeHtmlConstruction.ql         |  22 +++
 .../semmle/javascript/frameworks/Markdown.qll |   3 +
 .../dataflow/UnsafeHtmlConstruction.qll       |  32 ++++
 .../UnsafeHtmlConstructionCustomizations.qll  | 168 ++++++++++++++++++
 4 files changed, 225 insertions(+)
 create mode 100644 javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql
 create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
 create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll

diff --git a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql
new file mode 100644
index 000000000000..464f4951307a
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql
@@ -0,0 +1,22 @@
+/**
+ * @name Unsafe HTML constructed from library input
+ * @description Using externally controlled strings to construct HTML might allow a malicious
+ *              user to perform an cross-site scripting attack.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/html-constructed-from-input
+ * @tags security
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import javascript
+import DataFlow::PathGraph
+import semmle.javascript.security.dataflow.UnsafeHtmlConstruction::UnsafeHtmlConstruction
+
+from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
+where cfg.hasFlowPath(source, sink) and sink.getNode() = sinkNode
+select sinkNode, source, sink, "$@ based on $@ might later cause $@.", sinkNode,
+  sinkNode.describe(), source.getNode(), "library input", sinkNode.getSink(),
+  sinkNode.getVulnerabilityKind().toLowerCase()
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
index 788e28e2e9e2..c98243e4fbbd 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
@@ -13,6 +13,9 @@ module Markdown {
    * A taint-step that parses a markdown document, but preserves taint.import
    */
   class MarkdownStep extends Unit {
+    /**
+     * Holds if there is a taint-step from `pred` to `succ` for a taint-preserving markdown parser.
+     */
     abstract predicate step(DataFlow::Node pred, DataFlow::Node succ);
   }
 
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
new file mode 100644
index 000000000000..3e2bf53bde98
--- /dev/null
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
@@ -0,0 +1,32 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about
+ * unsafe HTML constructed from library input vulnerabilities.
+ */
+
+import javascript
+
+/**
+ * Classes and predicates for the unsafe HTML constructed from library input query.
+ */
+module UnsafeHtmlConstruction {
+  private import semmle.javascript.security.dataflow.DomBasedXssCustomizations::DomBasedXss as DomBasedXss
+  private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQueryPlugin
+  import UnsafeHtmlConstructionCustomizations::UnsafeHtmlConstruction
+
+  /**
+   * A taint-tracking configuration for reasoning about unsafe HTML constructed from library input vulnerabilities.
+   */
+  class Configration extends TaintTracking::Configuration {
+    Configration() { this = "UnsafeHtmlConstruction" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) }
+
+    override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
+      DomBasedXss::isOptionallySanitizedEdge(pred, succ)
+    }
+  }
+}
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
new file mode 100644
index 000000000000..b72df1b74212
--- /dev/null
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -0,0 +1,168 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * unsafe HTML constructed from library input, as well as extension points
+ * for adding your own.
+ */
+
+import javascript
+
+/**
+ * Module containing sources, sinks, and sanitizers for unsafe HTML constructed from library input.
+ */
+module UnsafeHtmlConstruction {
+  private import semmle.javascript.security.dataflow.DomBasedXssCustomizations::DomBasedXss as DomBasedXss
+  private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQueryPlugin
+  private import semmle.javascript.PackageExports as Exports
+
+  /**
+   * A source for unsafe HTML constructed from library input.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A parameter of an exported function, seen as a source for usnafe HTML constructed from input.
+   */
+  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+    ExternalInputSource() {
+      this = Exports::getALibraryInputParameter() and
+      not this = JQuery::dollarSource()
+    }
+  }
+
+  /**
+   * A sink for unsafe HTML constructed from library input.
+   * This sink somehow transforms its input into a value that can cause XSS if it ends up in a XSS sink.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets the kind of vulnerability to report in the alert message.
+     *
+     * Defaults to `Cross-site scripting`, but may be overriden for sinks
+     * that do not allow script injection, but injection of other undesirable HTML elements.
+     */
+    abstract string getVulnerabilityKind();
+
+    /**
+     * Gets the XSS sink that this transformed input ends up in.
+     */
+    abstract DataFlow::Node getSink();
+
+    /**
+     * Gets a string describing the transformation that this sink represents.
+     */
+    abstract string describe();
+  }
+
+  /**
+   * A sink for `js/html-constructed-from-input` that constructs some HTML where
+   * that HTML is later used in `xssSink`.
+   */
+  abstract class XssSink extends Sink {
+    DomBasedXss::Sink xssSink;
+
+    final override string getVulnerabilityKind() { result = xssSink.getVulnerabilityKind() }
+
+    final override DomBasedXss::Sink getSink() { result = xssSink }
+  }
+
+  /**
+   * Gets a dataflow node that flows to `sink` tracked by `t`.
+   */
+  private DataFlow::Node isUsedInXssSink(DataFlow::TypeBackTracker t, DomBasedXss::Sink sink) {
+    t.start() and
+    result = sink
+    or
+    exists(DataFlow::TypeBackTracker t2 | t = t2.smallstep(result, isUsedInXssSink(t2, sink)))
+    or
+    exists(DataFlow::TypeBackTracker t2 |
+      t.continue() = t2 and
+      domBasedTaintStep(result, isUsedInXssSink(t2, sink))
+    )
+  }
+
+  /**
+   * Gets a dataflow node that flows to `sink`.
+   */
+  DataFlow::Node isUsedInXssSink(DomBasedXss::Sink sink) {
+    result = isUsedInXssSink(DataFlow::TypeBackTracker::end(), sink)
+  }
+
+  /**
+   * Holds if there is a taint step from `pred` to `succ` for DOM strings/nodes.
+   * These steps are mostly relevant for DOM nodes that are created by an XML parser.
+   */
+  predicate domBasedTaintStep(DataFlow::Node pred, DataFlow::SourceNode succ) {
+    // node.appendChild(newChild) and similar
+    exists(DataFlow::MethodCallNode call |
+      call.getMethodName() = ["insertBefore", "replaceChild", "appendChild"]
+    |
+      pred = call.getArgument(0) and
+      succ = [call.getReceiver().getALocalSource(), call]
+    )
+    or
+    // element.{prepend,append}(node) and similar
+    exists(DataFlow::MethodCallNode call |
+      call.getMethodName() = ["prepend", "append", "replaceWith", "replaceChildren"]
+    |
+      pred = call.getAnArgument() and
+      succ = call.getReceiver().getALocalSource()
+    )
+    or
+    // node.insertAdjacentElement("location", newChild)
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "insertAdjacentElement" |
+      pred = call.getArgument(1) and
+      succ = call.getReceiver().getALocalSource()
+    )
+    or
+    // clone = node.cloneNode()
+    exists(DataFlow::MethodCallNode cloneNode | cloneNode.getMethodName() = "cloneNode" |
+      pred = cloneNode.getReceiver() and
+      succ = cloneNode
+    )
+    or
+    // var succ = pred.documentElement;
+    // documentElement is the root element of the document, and childNodes is the list of all children
+    exists(DataFlow::PropRead read | read.getPropertyName() = ["documentElement", "childNodes"] |
+      pred = read.getBase() and
+      succ = read
+    )
+  }
+
+  /**
+   * A string-concatenation of HTML, where the result is used as an XSS sink.
+   */
+  class HTMLConcatenationSink extends XssSink, StringOps::HtmlConcatenationLeaf {
+    HTMLConcatenationSink() { isUsedInXssSink(xssSink) = this.getRoot() }
+
+    override string describe() { result = "HTML construction" }
+  }
+
+  /**
+   * A string parsed as XML, which is later used in an XSS sink.
+   */
+  class XMLParsedSink extends XssSink {
+    XML::ParserInvocation parser;
+
+    XMLParsedSink() {
+      this.asExpr() = parser.getSourceArgument() and
+      isUsedInXssSink(xssSink) = parser.getAResult()
+    }
+
+    override string describe() { result = "XML parsing" }
+  }
+
+  /**
+   * A string rendered as markdown, where the rendering preserves HTML.
+   */
+  class MarkdownSink extends XssSink {
+    MarkdownSink() {
+      exists(DataFlow::Node pred, DataFlow::Node succ, Markdown::MarkdownStep step |
+        step.step(pred, succ) and
+        this = pred and
+        succ = isUsedInXssSink(xssSink)
+      )
+    }
+
+    override string describe() { result = "Markdown rendering" }
+  }
+}

From 6e754c70aa7f5828799aa3291a84a30bceef1c89 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 10:21:53 +0200
Subject: [PATCH 03/16] add test for js/html-constructed-from-input

---
 ...ConsistencyUnsafeHtmlConstruction.expected |  0
 .../ConsistencyUnsafeHtmlConstruction.ql      |  3 +
 .../UnsafeHtmlConstruction.expected           | 72 +++++++++++++++++++
 .../UnsafeHtmlConstruction.qlref              |  1 +
 .../UnsafeHtmlConstruction/jquery-plugin.js   |  9 +++
 .../CWE-079/UnsafeHtmlConstruction/main.js    | 54 ++++++++++++++
 .../UnsafeHtmlConstruction/package.json       |  4 ++
 .../UnsafeHtmlConstruction/tsconfig.json      |  1 +
 .../CWE-079/UnsafeHtmlConstruction/typed.ts   | 20 ++++++
 9 files changed, 164 insertions(+)
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.expected
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.ql
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.qlref
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/jquery-plugin.js
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/package.json
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/tsconfig.json
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts

diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.expected
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.ql b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.ql
new file mode 100644
index 000000000000..823644730b2c
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/ConsistencyUnsafeHtmlConstruction.ql
@@ -0,0 +1,3 @@
+import javascript
+import testUtilities.ConsistencyChecking
+import semmle.javascript.security.dataflow.UnsafeHtmlConstruction as UnsafeHtmlConstruction
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
new file mode 100644
index 000000000000..0a8c8013cbc1
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
@@ -0,0 +1,72 @@
+nodes
+| main.js:1:55:1:55 | s |
+| main.js:1:55:1:55 | s |
+| main.js:2:29:2:29 | s |
+| main.js:2:29:2:29 | s |
+| main.js:6:49:6:49 | s |
+| main.js:6:49:6:49 | s |
+| main.js:7:49:7:49 | s |
+| main.js:7:49:7:49 | s |
+| main.js:11:60:11:60 | s |
+| main.js:11:60:11:60 | s |
+| main.js:12:49:12:49 | s |
+| main.js:12:49:12:49 | s |
+| main.js:21:47:21:47 | s |
+| main.js:21:47:21:47 | s |
+| main.js:22:34:22:34 | s |
+| main.js:22:34:22:34 | s |
+| typed.ts:1:39:1:39 | s |
+| typed.ts:1:39:1:39 | s |
+| typed.ts:2:29:2:29 | s |
+| typed.ts:2:29:2:29 | s |
+| typed.ts:6:43:6:43 | s |
+| typed.ts:6:43:6:43 | s |
+| typed.ts:8:40:8:40 | s |
+| typed.ts:8:40:8:40 | s |
+| typed.ts:11:20:11:20 | s |
+| typed.ts:11:20:11:20 | s |
+| typed.ts:12:12:12:12 | s |
+| typed.ts:16:11:16:21 | s |
+| typed.ts:16:15:16:21 | id("x") |
+| typed.ts:17:29:17:29 | s |
+| typed.ts:17:29:17:29 | s |
+edges
+| main.js:1:55:1:55 | s | main.js:2:29:2:29 | s |
+| main.js:1:55:1:55 | s | main.js:2:29:2:29 | s |
+| main.js:1:55:1:55 | s | main.js:2:29:2:29 | s |
+| main.js:1:55:1:55 | s | main.js:2:29:2:29 | s |
+| main.js:6:49:6:49 | s | main.js:7:49:7:49 | s |
+| main.js:6:49:6:49 | s | main.js:7:49:7:49 | s |
+| main.js:6:49:6:49 | s | main.js:7:49:7:49 | s |
+| main.js:6:49:6:49 | s | main.js:7:49:7:49 | s |
+| main.js:11:60:11:60 | s | main.js:12:49:12:49 | s |
+| main.js:11:60:11:60 | s | main.js:12:49:12:49 | s |
+| main.js:11:60:11:60 | s | main.js:12:49:12:49 | s |
+| main.js:11:60:11:60 | s | main.js:12:49:12:49 | s |
+| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
+| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
+| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
+| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
+| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
+| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
+| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
+| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
+| typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s |
+| typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s |
+| typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s |
+| typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s |
+| typed.ts:11:20:11:20 | s | typed.ts:12:12:12:12 | s |
+| typed.ts:11:20:11:20 | s | typed.ts:12:12:12:12 | s |
+| typed.ts:12:12:12:12 | s | typed.ts:16:15:16:21 | id("x") |
+| typed.ts:16:11:16:21 | s | typed.ts:17:29:17:29 | s |
+| typed.ts:16:11:16:21 | s | typed.ts:17:29:17:29 | s |
+| typed.ts:16:15:16:21 | id("x") | typed.ts:16:11:16:21 | s |
+#select
+| main.js:2:29:2:29 | s | main.js:1:55:1:55 | s | main.js:2:29:2:29 | s | $@ based on $@ might later cause $@. | main.js:2:29:2:29 | s | HTML construction | main.js:1:55:1:55 | s | library input | main.js:3:49:3:52 | html | cross-site scripting |
+| main.js:7:49:7:49 | s | main.js:6:49:6:49 | s | main.js:7:49:7:49 | s | $@ based on $@ might later cause $@. | main.js:7:49:7:49 | s | XML parsing | main.js:6:49:6:49 | s | library input | main.js:8:48:8:66 | doc.documentElement | cross-site scripting |
+| main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:16:21:16:35 | xml.cloneNode() | cross-site scripting |
+| main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:17:48:17:50 | tmp | cross-site scripting |
+| main.js:22:34:22:34 | s | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | $@ based on $@ might later cause $@. | main.js:22:34:22:34 | s | Markdown rendering | main.js:21:47:21:47 | s | library input | main.js:23:53:23:56 | html | cross-site scripting |
+| typed.ts:2:29:2:29 | s | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | $@ based on $@ might later cause $@. | typed.ts:2:29:2:29 | s | HTML construction | typed.ts:1:39:1:39 | s | library input | typed.ts:3:31:3:34 | html | cross-site scripting |
+| typed.ts:8:40:8:40 | s | typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | $@ based on $@ might later cause $@. | typed.ts:8:40:8:40 | s | HTML construction | typed.ts:6:43:6:43 | s | library input | typed.ts:8:29:8:52 | "<span> ... /span>" | cross-site scripting |
+| typed.ts:17:29:17:29 | s | typed.ts:11:20:11:20 | s | typed.ts:17:29:17:29 | s | $@ based on $@ might later cause $@. | typed.ts:17:29:17:29 | s | HTML construction | typed.ts:11:20:11:20 | s | library input | typed.ts:18:31:18:34 | html | cross-site scripting |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.qlref b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.qlref
new file mode 100644
index 000000000000..0fbe0ed0ba1a
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.qlref
@@ -0,0 +1 @@
+Security/CWE-079/UnsafeHtmlConstruction.ql
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/jquery-plugin.js b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/jquery-plugin.js
new file mode 100644
index 000000000000..07b25b558f9b
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/jquery-plugin.js
@@ -0,0 +1,9 @@
+(function (factory) {
+	if (typeof define === 'function' && define.amd) {
+		define(['jquery', 'jquery-ui'], factory);
+	} else {
+		factory(jQuery);
+	}
+}(function ($) {
+    $("<span>" + $.trim("foo") + "</span>"); // OK
+}));
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
new file mode 100644
index 000000000000..6564cd2dfe75
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
@@ -0,0 +1,54 @@
+module.exports.xssThroughHTMLConstruction = function (s) {
+    const html = "<span>" + s + "</span>";// NOT OK
+    document.querySelector("#html").innerHTML = html;
+}
+ 
+module.exports.xssThroughXMLParsing = function (s) {
+    const doc = new DOMParser().parseFromString(s, "text/xml"); // NOT OK
+    document.querySelector("#xml").appendChild(doc.documentElement);
+}
+
+module.exports.xssThroughMoreComplexXMLParsing = function (s) {
+    const doc = new DOMParser().parseFromString(s, "text/xml"); // NOT OK
+    const xml = doc.documentElement;
+    
+    const tmp = document.createElement('span');
+    tmp.appendChild(xml.cloneNode()); 
+    document.querySelector("#xml").appendChild(tmp);
+}
+
+const markdown = require('markdown-it')({html: true});
+module.exports.xssThroughMarkdown = function (s) {
+    const html = markdown.render(s); // NOT OK
+    document.querySelector("#markdown").innerHTML = html;
+}
+
+const striptags = require('striptags');
+module.exports.sanitizedHTML = function (s) {
+    const html = striptags("<span>" + s + "</span>"); // OK
+    document.querySelector("#sanitized").innerHTML = html;
+}
+
+module.exports.ts = require("./typed");
+
+module.exports.jquery = require("./jquery-plugin");
+
+module.exports.plainDOMXMLParsing = function (s) {
+    const doc = new DOMParser().parseFromString(s, "text/xml"); // OK - is never added to the DOM.
+}
+
+class Foo {
+    constructor(s) {
+        this.step = s;
+    }
+
+    doXss() {
+        // not called here, but still bad.
+        document.querySelector("#class").innerHTML = "<span>" + this.step + "</span>"; // NOT OK - but not flagged [INCONSISTENCY]
+    }
+
+}
+
+module.exports.createsClass = function (s) {
+    return new Foo(s);
+}
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/package.json b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/package.json
new file mode 100644
index 000000000000..f4fa73f79d9a
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/package.json
@@ -0,0 +1,4 @@
+{
+    "name": "my-unsafe-library",
+    "main": "./main.js"
+}
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/tsconfig.json b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/tsconfig.json
new file mode 100644
index 000000000000..9e26dfeeb6e6
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/tsconfig.json
@@ -0,0 +1 @@
+{}
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts
new file mode 100644
index 000000000000..73c035c0ad24
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts
@@ -0,0 +1,20 @@
+export function basicHtmlConstruction(s: string) {
+    const html = "<span>" + s + "</span>"; // NOT OK
+    document.body.innerHTML = html;
+}
+
+export function insertIntoCreatedDocument(s: string) {
+    const newDoc = document.implementation.createHTMLDocument("");
+    newDoc.body.innerHTML = "<span>" + s + "</span>"; // OK - inserted into document disconnected from the main DOM. [INCONSISTENCY]
+}
+
+export function id(s: string) {
+    return s;
+}
+
+export function notVulnerable() {
+    const s = id("x");
+    const html = "<span>" + s + "</span>"; // OK - but flagged due to step with unmatched call [INCONSISTENCY]
+    document.body.innerHTML = html;
+}
+ 
\ No newline at end of file

From 23908f9ec2ba6166bf161ba25d2f61e3a1f2373a Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 10:23:27 +0200
Subject: [PATCH 04/16] remove flowpaths that has a returns without a matching
 call

---
 .../dataflow/UnsafeHtmlConstruction.qll       | 10 ++++++++
 .../UnsafeHtmlConstructionCustomizations.qll  | 24 +++++++++++++++++++
 .../UnsafeHtmlConstruction.expected           | 15 +++++++++++-
 .../CWE-079/UnsafeHtmlConstruction/main.js    |  2 +-
 .../CWE-079/UnsafeHtmlConstruction/typed.ts   |  2 +-
 5 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
index 3e2bf53bde98..7e0194defd3f 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
@@ -28,5 +28,15 @@ module UnsafeHtmlConstruction {
     override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
       DomBasedXss::isOptionallySanitizedEdge(pred, succ)
     }
+
+    // override to require that there is a path without unmatched return steps
+    override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+      super.hasFlowPath(source, sink) and
+      requireMatchedReturn(source, sink)
+    }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+      classFieldStep(pred, succ)
+    }
   }
 }
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
index b72df1b74212..845438b6b17d 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -165,4 +165,28 @@ module UnsafeHtmlConstruction {
 
     override string describe() { result = "Markdown rendering" }
   }
+
+  /**
+   * A taint step from the write of a field in a constructor to a read of the same field in an instance method.
+   */
+  predicate classFieldStep(DataFlow::Node pred, DataFlow::Node succ) {
+    // flow-step from a property written in the constructor to a use in an instance method.
+    // "simulates" client usage of a class, and regains some flow-steps lost by `requireMatchedReturn` below.
+    exists(DataFlow::ClassNode clazz, string prop |
+      DataFlow::thisNode(clazz.getConstructor().getFunction()).getAPropertyWrite(prop).getRhs() =
+        pred and
+      DataFlow::thisNode(clazz.getAnInstanceMethod().getFunction()).getAPropertyRead(prop) = succ
+    )
+  }
+
+  /**
+   *  Holds if there is a path without unmatched return steps from `source` to `sink`.
+   */
+  predicate requireMatchedReturn(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+    exists(DataFlow::MidPathNode mid |
+      source.getASuccessor*() = mid and
+      sink = mid.getASuccessor() and
+      mid.getPathSummary().hasReturn() = false
+    )
+  }
 }
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
index 0a8c8013cbc1..3f87d690d5bc 100644
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
@@ -15,6 +15,13 @@ nodes
 | main.js:21:47:21:47 | s |
 | main.js:22:34:22:34 | s |
 | main.js:22:34:22:34 | s |
+| main.js:46:17:46:17 | s |
+| main.js:47:21:47:21 | s |
+| main.js:52:65:52:73 | this.step |
+| main.js:52:65:52:73 | this.step |
+| main.js:57:41:57:41 | s |
+| main.js:57:41:57:41 | s |
+| main.js:58:20:58:20 | s |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:2:29:2:29 | s |
@@ -47,6 +54,12 @@ edges
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
+| main.js:46:17:46:17 | s | main.js:47:21:47:21 | s |
+| main.js:47:21:47:21 | s | main.js:52:65:52:73 | this.step |
+| main.js:47:21:47:21 | s | main.js:52:65:52:73 | this.step |
+| main.js:57:41:57:41 | s | main.js:58:20:58:20 | s |
+| main.js:57:41:57:41 | s | main.js:58:20:58:20 | s |
+| main.js:58:20:58:20 | s | main.js:46:17:46:17 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
@@ -67,6 +80,6 @@ edges
 | main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:16:21:16:35 | xml.cloneNode() | cross-site scripting |
 | main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:17:48:17:50 | tmp | cross-site scripting |
 | main.js:22:34:22:34 | s | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | $@ based on $@ might later cause $@. | main.js:22:34:22:34 | s | Markdown rendering | main.js:21:47:21:47 | s | library input | main.js:23:53:23:56 | html | cross-site scripting |
+| main.js:52:65:52:73 | this.step | main.js:57:41:57:41 | s | main.js:52:65:52:73 | this.step | $@ based on $@ might later cause $@. | main.js:52:65:52:73 | this.step | HTML construction | main.js:57:41:57:41 | s | library input | main.js:52:54:52:85 | "<span> ... /span>" | cross-site scripting |
 | typed.ts:2:29:2:29 | s | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | $@ based on $@ might later cause $@. | typed.ts:2:29:2:29 | s | HTML construction | typed.ts:1:39:1:39 | s | library input | typed.ts:3:31:3:34 | html | cross-site scripting |
 | typed.ts:8:40:8:40 | s | typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | $@ based on $@ might later cause $@. | typed.ts:8:40:8:40 | s | HTML construction | typed.ts:6:43:6:43 | s | library input | typed.ts:8:29:8:52 | "<span> ... /span>" | cross-site scripting |
-| typed.ts:17:29:17:29 | s | typed.ts:11:20:11:20 | s | typed.ts:17:29:17:29 | s | $@ based on $@ might later cause $@. | typed.ts:17:29:17:29 | s | HTML construction | typed.ts:11:20:11:20 | s | library input | typed.ts:18:31:18:34 | html | cross-site scripting |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
index 6564cd2dfe75..d3620615c08b 100644
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
@@ -44,7 +44,7 @@ class Foo {
 
     doXss() {
         // not called here, but still bad.
-        document.querySelector("#class").innerHTML = "<span>" + this.step + "</span>"; // NOT OK - but not flagged [INCONSISTENCY]
+        document.querySelector("#class").innerHTML = "<span>" + this.step + "</span>"; // NOT OK
     }
 
 }
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts
index 73c035c0ad24..0f04e92cdc04 100644
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts
@@ -14,7 +14,7 @@ export function id(s: string) {
 
 export function notVulnerable() {
     const s = id("x");
-    const html = "<span>" + s + "</span>"; // OK - but flagged due to step with unmatched call [INCONSISTENCY]
+    const html = "<span>" + s + "</span>"; // OK
     document.body.innerHTML = html;
 }
  
\ No newline at end of file

From ee0140e7044ea81b7b08c20e68dbcce54ab5366b Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 10:23:44 +0200
Subject: [PATCH 05/16] share code between unsafe-shell and unsafe-html queries

---
 .../security/dataflow/UnsafeHtmlConstruction.qll  |  2 +-
 .../UnsafeHtmlConstructionCustomizations.qll      | 13 -------------
 .../dataflow/UnsafeShellCommandConstruction.qll   | 15 +++------------
 3 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
index 7e0194defd3f..8d2855dd766e 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
@@ -36,7 +36,7 @@ module UnsafeHtmlConstruction {
     }
 
     override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-      classFieldStep(pred, succ)
+      DataFlow::localFieldStep(pred, succ)
     }
   }
 }
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
index 845438b6b17d..26da71aa6332 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -166,19 +166,6 @@ module UnsafeHtmlConstruction {
     override string describe() { result = "Markdown rendering" }
   }
 
-  /**
-   * A taint step from the write of a field in a constructor to a read of the same field in an instance method.
-   */
-  predicate classFieldStep(DataFlow::Node pred, DataFlow::Node succ) {
-    // flow-step from a property written in the constructor to a use in an instance method.
-    // "simulates" client usage of a class, and regains some flow-steps lost by `requireMatchedReturn` below.
-    exists(DataFlow::ClassNode clazz, string prop |
-      DataFlow::thisNode(clazz.getConstructor().getFunction()).getAPropertyWrite(prop).getRhs() =
-        pred and
-      DataFlow::thisNode(clazz.getAnInstanceMethod().getFunction()).getAPropertyRead(prop) = succ
-    )
-  }
-
   /**
    *  Holds if there is a path without unmatched return steps from `source` to `sink`.
    */
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll
index 90caf59ad33d..2c7de7555def 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll
@@ -14,6 +14,7 @@ import javascript
  */
 module UnsafeShellCommandConstruction {
   import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
+  import UnsafeHtmlConstructionCustomizations
 
   /**
    * A taint-tracking configuration for reasoning about shell command constructed from library input vulnerabilities.
@@ -35,21 +36,11 @@ module UnsafeShellCommandConstruction {
     // override to require that there is a path without unmatched return steps
     override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
       super.hasFlowPath(source, sink) and
-      exists(DataFlow::MidPathNode mid |
-        source.getASuccessor*() = mid and
-        sink = mid.getASuccessor() and
-        mid.getPathSummary().hasReturn() = false
-      )
+      UnsafeHtmlConstruction::requireMatchedReturn(source, sink)
     }
 
     override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-      // flow-step from a property written in the constructor to a use in an instance method.
-      // "simulates" client usage of a class, and regains some flow-steps lost by `hasFlowPath` above.
-      exists(DataFlow::ClassNode clz, string name |
-        pred =
-          DataFlow::thisNode(clz.getConstructor().getFunction()).getAPropertyWrite(name).getRhs() and
-        succ = DataFlow::thisNode(clz.getInstanceMethod(_).getFunction()).getAPropertyRead(name)
-      )
+      DataFlow::localFieldStep(pred, succ)
     }
   }
 }

From 7ef641e7b256754803b1e0f1531916010181739b Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 11:47:52 +0200
Subject: [PATCH 06/16] add qhelp

---
 .../CWE-079/UnsafeHtmlConstruction.qhelp      | 80 +++++++++++++++++++
 .../examples/unsafe-html-construction.js      |  3 +
 .../examples/unsafe-html-construction_safe.js |  5 ++
 .../unsafe-html-construction_sanitizer.js     |  5 ++
 4 files changed, 93 insertions(+)
 create mode 100644 javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
 create mode 100644 javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction.js
 create mode 100644 javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_safe.js
 create mode 100644 javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_sanitizer.js

diff --git a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
new file mode 100644
index 000000000000..12e030150b65
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
@@ -0,0 +1,80 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+	<p>
+		Dynamically constructing HTML with inputs from exported functions may 
+		inadvertently leave a client open to XSS attacks.
+		
+		Clients using the exported function may use inputs containing unsafe HTML,
+		and if these inputs end up in the DOM, the client may be vulnerable to
+		cross-site scripting attacks.
+	</p>
+
+</overview>
+<recommendation>
+
+	<p>
+		If possible, use safe APIs when inserting HTML into the DOM.  
+		Such as writing to the <code>innerText</code> property instead of <code>innerHTML</code>.
+	</p>
+
+	<p>
+		Alternatively, use a HTML sanitizer to escape/remove unsafe content.
+	</p>
+
+</recommendation>
+<example>
+
+	<p>
+		The following example shows a library function that shows a boldface name
+		by writing to the <code>innerHTML</code> property of an element.
+	</p>
+
+	<sample src="examples/unsafe-html-construction.js" />
+
+	<p>
+		This library function, however, does not escape unsafe HTML, and a client
+		that calls the function with user-supplied input may be vulnerable to
+		cross-site scripting attacks.
+	</p>
+
+	<p>
+		To avoid such attacks, a program can use safe APIs such as <code>innerText</code>.
+	</p>
+
+	<sample src="examples/unsafe-html-construction_safe.js" />
+	
+	<p>
+		Alternatively, use a HTML sanitizer to remove unsafe content.
+	</p>
+
+	<sample src="examples/unsafe-html-construction_sanitizer.js" />
+
+</example>
+<references>
+	<li>
+		OWASP:
+		<a href="https://www.owasp.org/index.php/DOM_based_XSS_Prevention_Cheat_Sheet">DOM based
+		XSS Prevention Cheat Sheet</a>.
+	</li>
+	<li>
+		OWASP:
+		<a href="https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet">XSS
+		(Cross Site Scripting) Prevention Cheat Sheet</a>.
+	</li>
+	<li>
+		OWASP
+		<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.
+	</li>
+	<li>
+		OWASP
+		<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
+		Scripting</a>.
+	</li>
+	<li>
+		Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
+	</li>
+</references>
+</qhelp>
diff --git a/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction.js b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction.js
new file mode 100644
index 000000000000..ab4bfee0a25b
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction.js
@@ -0,0 +1,3 @@
+module.exports = function showBoldName(name) {
+  document.getElementById('name').innerHTML = "<b>" + name + "</b>";
+}
diff --git a/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_safe.js b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_safe.js
new file mode 100644
index 000000000000..b2b3af9bcfbb
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_safe.js
@@ -0,0 +1,5 @@
+module.exports = function showBoldName(name) {
+  const bold = document.createElement('b');
+  bold.innerText = name;
+  document.getElementById('name').appendChild(bold);
+}
diff --git a/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_sanitizer.js b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_sanitizer.js
new file mode 100644
index 000000000000..9c63e278720c
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-079/examples/unsafe-html-construction_sanitizer.js
@@ -0,0 +1,5 @@
+
+const striptags = require('striptags');
+module.exports = function showBoldName(name) {
+  document.getElementById('name').innerHTML = "<b>" + striptags(name) + "</b>";
+}

From 5c37e6a435ae1a1f048d087af68daedd0d9f40ab Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 11:56:44 +0200
Subject: [PATCH 07/16] add change note

---
 javascript/change-notes/2021-04-26-unsafe-html-construction.md | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 javascript/change-notes/2021-04-26-unsafe-html-construction.md

diff --git a/javascript/change-notes/2021-04-26-unsafe-html-construction.md b/javascript/change-notes/2021-04-26-unsafe-html-construction.md
new file mode 100644
index 000000000000..8b0f513d7605
--- /dev/null
+++ b/javascript/change-notes/2021-04-26-unsafe-html-construction.md
@@ -0,0 +1,3 @@
+lgtm,codescanning
+* A new query, `js/html-constructed-from-input`, has been added to the query suite,
+  highlighting libraries that may leave clients vulnerable to cross-site-scripting attacks.

From 8ba5bddae8d99817107ebfea1f3527b27cdd74f7 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 20:03:14 +0200
Subject: [PATCH 08/16] add jQuery options objects as sources

---
 .../UnsafeHtmlConstructionCustomizations.qll  |  8 +++
 .../UnsafeHtmlConstruction.expected           | 51 ++++++++++++++-----
 .../CWE-079/UnsafeHtmlConstruction/main.js    | 10 ++++
 3 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
index 26da71aa6332..49097c813790 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -13,6 +13,7 @@ module UnsafeHtmlConstruction {
   private import semmle.javascript.security.dataflow.DomBasedXssCustomizations::DomBasedXss as DomBasedXss
   private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQueryPlugin
   private import semmle.javascript.PackageExports as Exports
+  private import semmle.javascript.security.dataflow.UnsafeJQueryPlugin::UnsafeJQueryPlugin as UnsafeJQueryPlugin
 
   /**
    * A source for unsafe HTML constructed from library input.
@@ -29,6 +30,13 @@ module UnsafeHtmlConstruction {
     }
   }
 
+  /**
+   * A jQuery plugin options object, seen as a source for unsafe HTML constructed from input.
+   */
+  class JQueryPluginOptionsAsSource extends Source {
+    JQueryPluginOptionsAsSource() { this instanceof UnsafeJQueryPlugin::JQueryPluginOptions }
+  }
+
   /**
    * A sink for unsafe HTML constructed from library input.
    * This sink somehow transforms its input into a value that can cause XSS if it ends up in a XSS sink.
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
index 3f87d690d5bc..73e2177b198f 100644
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
@@ -15,13 +15,24 @@ nodes
 | main.js:21:47:21:47 | s |
 | main.js:22:34:22:34 | s |
 | main.js:22:34:22:34 | s |
-| main.js:46:17:46:17 | s |
-| main.js:47:21:47:21 | s |
-| main.js:52:65:52:73 | this.step |
-| main.js:52:65:52:73 | this.step |
-| main.js:57:41:57:41 | s |
-| main.js:57:41:57:41 | s |
-| main.js:58:20:58:20 | s |
+| main.js:41:17:41:17 | s |
+| main.js:42:21:42:21 | s |
+| main.js:47:65:47:73 | this.step |
+| main.js:47:65:47:73 | this.step |
+| main.js:52:41:52:41 | s |
+| main.js:52:41:52:41 | s |
+| main.js:53:20:53:20 | s |
+| main.js:56:28:56:34 | options |
+| main.js:56:28:56:34 | options |
+| main.js:57:11:59:5 | defaults |
+| main.js:57:22:59:5 | {\\n      ... "\\n    } |
+| main.js:60:11:60:48 | settings |
+| main.js:60:22:60:48 | $.exten ... ptions) |
+| main.js:60:31:60:38 | defaults |
+| main.js:60:41:60:47 | options |
+| main.js:62:19:62:26 | settings |
+| main.js:62:19:62:31 | settings.name |
+| main.js:62:19:62:31 | settings.name |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:2:29:2:29 | s |
@@ -54,12 +65,23 @@ edges
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
-| main.js:46:17:46:17 | s | main.js:47:21:47:21 | s |
-| main.js:47:21:47:21 | s | main.js:52:65:52:73 | this.step |
-| main.js:47:21:47:21 | s | main.js:52:65:52:73 | this.step |
-| main.js:57:41:57:41 | s | main.js:58:20:58:20 | s |
-| main.js:57:41:57:41 | s | main.js:58:20:58:20 | s |
-| main.js:58:20:58:20 | s | main.js:46:17:46:17 | s |
+| main.js:41:17:41:17 | s | main.js:42:21:42:21 | s |
+| main.js:42:21:42:21 | s | main.js:47:65:47:73 | this.step |
+| main.js:42:21:42:21 | s | main.js:47:65:47:73 | this.step |
+| main.js:52:41:52:41 | s | main.js:53:20:53:20 | s |
+| main.js:52:41:52:41 | s | main.js:53:20:53:20 | s |
+| main.js:53:20:53:20 | s | main.js:41:17:41:17 | s |
+| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
+| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options |
+| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults |
+| main.js:57:22:59:5 | {\\n      ... "\\n    } | main.js:57:11:59:5 | defaults |
+| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings |
+| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings |
+| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) |
+| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n      ... "\\n    } |
+| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) |
+| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
+| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
@@ -80,6 +102,7 @@ edges
 | main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:16:21:16:35 | xml.cloneNode() | cross-site scripting |
 | main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:17:48:17:50 | tmp | cross-site scripting |
 | main.js:22:34:22:34 | s | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | $@ based on $@ might later cause $@. | main.js:22:34:22:34 | s | Markdown rendering | main.js:21:47:21:47 | s | library input | main.js:23:53:23:56 | html | cross-site scripting |
-| main.js:52:65:52:73 | this.step | main.js:57:41:57:41 | s | main.js:52:65:52:73 | this.step | $@ based on $@ might later cause $@. | main.js:52:65:52:73 | this.step | HTML construction | main.js:57:41:57:41 | s | library input | main.js:52:54:52:85 | "<span> ... /span>" | cross-site scripting |
+| main.js:47:65:47:73 | this.step | main.js:52:41:52:41 | s | main.js:47:65:47:73 | this.step | $@ based on $@ might later cause $@. | main.js:47:65:47:73 | this.step | HTML construction | main.js:52:41:52:41 | s | library input | main.js:47:54:47:85 | "<span> ... /span>" | cross-site scripting |
+| main.js:62:19:62:31 | settings.name | main.js:56:28:56:34 | options | main.js:62:19:62:31 | settings.name | $@ based on $@ might later cause $@. | main.js:62:19:62:31 | settings.name | HTML construction | main.js:56:28:56:34 | options | library input | main.js:62:11:62:40 | "<b>" + ...  "</b>" | cross-site scripting |
 | typed.ts:2:29:2:29 | s | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | $@ based on $@ might later cause $@. | typed.ts:2:29:2:29 | s | HTML construction | typed.ts:1:39:1:39 | s | library input | typed.ts:3:31:3:34 | html | cross-site scripting |
 | typed.ts:8:40:8:40 | s | typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | $@ based on $@ might later cause $@. | typed.ts:8:40:8:40 | s | HTML construction | typed.ts:6:43:6:43 | s | library input | typed.ts:8:29:8:52 | "<span> ... /span>" | cross-site scripting |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
index d3620615c08b..d0a117c23412 100644
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
@@ -52,3 +52,13 @@ class Foo {
 module.exports.createsClass = function (s) {
     return new Foo(s);
 }
+
+$.fn.xssPlugin = function (options) {
+    const defaults = {
+        name: "name"
+    };
+    const settings = $.extend(defaults, options);
+    return this.each(function () {
+        $("<b>" + settings.name + "</b>").appendTo(this); // NOT OK
+    });
+}
\ No newline at end of file

From 3815797dda55766cc497caa08d789e75c8134f70 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 26 Apr 2021 22:39:59 +0200
Subject: [PATCH 09/16] add sanitizers from DOM and jQuery queries

---
 .../security/dataflow/UnsafeHtmlConstruction.qll  |  8 +++++++-
 .../UnsafeHtmlConstructionCustomizations.qll      |  1 -
 .../UnsafeHtmlConstruction.expected               |  9 +++++++++
 .../CWE-079/UnsafeHtmlConstruction/main.js        | 15 ++++++++++++++-
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
index 8d2855dd766e..699d9d3e5101 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
@@ -23,7 +23,13 @@ module UnsafeHtmlConstruction {
 
     override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-    override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) }
+    override predicate isSanitizer(DataFlow::Node node) {
+      super.isSanitizer(node)
+      or
+      node instanceof DomBasedXss::Sanitizer
+      or
+      node instanceof UnsafeJQueryPlugin::Sanitizer
+    }
 
     override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
       DomBasedXss::isOptionallySanitizedEdge(pred, succ)
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
index 49097c813790..a99b5e7986d5 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -13,7 +13,6 @@ module UnsafeHtmlConstruction {
   private import semmle.javascript.security.dataflow.DomBasedXssCustomizations::DomBasedXss as DomBasedXss
   private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQueryPlugin
   private import semmle.javascript.PackageExports as Exports
-  private import semmle.javascript.security.dataflow.UnsafeJQueryPlugin::UnsafeJQueryPlugin as UnsafeJQueryPlugin
 
   /**
    * A source for unsafe HTML constructed from library input.
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
index 73e2177b198f..7994fbad9342 100644
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
@@ -33,6 +33,10 @@ nodes
 | main.js:62:19:62:26 | settings |
 | main.js:62:19:62:31 | settings.name |
 | main.js:62:19:62:31 | settings.name |
+| main.js:66:35:66:41 | attrVal |
+| main.js:66:35:66:41 | attrVal |
+| main.js:67:63:67:69 | attrVal |
+| main.js:67:63:67:69 | attrVal |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:2:29:2:29 | s |
@@ -82,6 +86,10 @@ edges
 | main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) |
 | main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
 | main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name |
+| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
+| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
+| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
+| main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
@@ -104,5 +112,6 @@ edges
 | main.js:22:34:22:34 | s | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | $@ based on $@ might later cause $@. | main.js:22:34:22:34 | s | Markdown rendering | main.js:21:47:21:47 | s | library input | main.js:23:53:23:56 | html | cross-site scripting |
 | main.js:47:65:47:73 | this.step | main.js:52:41:52:41 | s | main.js:47:65:47:73 | this.step | $@ based on $@ might later cause $@. | main.js:47:65:47:73 | this.step | HTML construction | main.js:52:41:52:41 | s | library input | main.js:47:54:47:85 | "<span> ... /span>" | cross-site scripting |
 | main.js:62:19:62:31 | settings.name | main.js:56:28:56:34 | options | main.js:62:19:62:31 | settings.name | $@ based on $@ might later cause $@. | main.js:62:19:62:31 | settings.name | HTML construction | main.js:56:28:56:34 | options | library input | main.js:62:11:62:40 | "<b>" + ...  "</b>" | cross-site scripting |
+| main.js:67:63:67:69 | attrVal | main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal | $@ based on $@ might later cause $@. | main.js:67:63:67:69 | attrVal | HTML construction | main.js:66:35:66:41 | attrVal | library input | main.js:67:47:67:78 | "<img a ...  "\\"/>" | cross-site scripting |
 | typed.ts:2:29:2:29 | s | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | $@ based on $@ might later cause $@. | typed.ts:2:29:2:29 | s | HTML construction | typed.ts:1:39:1:39 | s | library input | typed.ts:3:31:3:34 | html | cross-site scripting |
 | typed.ts:8:40:8:40 | s | typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | $@ based on $@ might later cause $@. | typed.ts:8:40:8:40 | s | HTML construction | typed.ts:6:43:6:43 | s | library input | typed.ts:8:29:8:52 | "<span> ... /span>" | cross-site scripting |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
index d0a117c23412..1097e126feb2 100644
--- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
@@ -61,4 +61,17 @@ $.fn.xssPlugin = function (options) {
     return this.each(function () {
         $("<b>" + settings.name + "</b>").appendTo(this); // NOT OK
     });
-}
\ No newline at end of file
+}
+
+module.exports.guards = function (attrVal) {
+    document.querySelector("#id").innerHTML = "<img alt=\"" + attrVal + "\"/>"; // NOT OK
+    document.querySelector("#id").innerHTML = "<img alt=\"" + attrVal.replace(/"|'/g, "") + "\"/>"; // OK
+    if (attrVal.indexOf("\"") === -1 && attrVal.indexOf("'") === -1) {
+        document.querySelector("#id").innerHTML = "<img alt=\"" + attrVal + "\"/>"; // OK
+    }
+}
+
+module.exports.intentionalTemplate = function (obj) {
+    const html = "<span>" + obj.spanTemplate + "</span>"; // OK
+    document.querySelector("#template").innerHTML = html;
+}

From 2d1ba59e6d8ccc2bf77e84db9220a7bbb1634dd1 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Thu, 6 May 2021 21:55:30 +0200
Subject: [PATCH 10/16] Apply suggestions from code review

Co-authored-by: Esben Sparre Andreasen <esbena@github.com>
---
 .../ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp      | 4 ++--
 javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
index 12e030150b65..76a7e65fd3f2 100644
--- a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
+++ b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
@@ -4,7 +4,7 @@
 <qhelp>
 <overview>
 	<p>
-		Dynamically constructing HTML with inputs from exported functions may 
+		Dynamically constructing HTML with inputs from library functions may 
 		inadvertently leave a client open to XSS attacks.
 		
 		Clients using the exported function may use inputs containing unsafe HTML,
@@ -28,7 +28,7 @@
 <example>
 
 	<p>
-		The following example shows a library function that shows a boldface name
+		The following example has a library function that renders a boldface name
 		by writing to the <code>innerHTML</code> property of an element.
 	</p>
 
diff --git a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql
index 464f4951307a..d6199af45984 100644
--- a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql
+++ b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.ql
@@ -1,7 +1,7 @@
 /**
  * @name Unsafe HTML constructed from library input
  * @description Using externally controlled strings to construct HTML might allow a malicious
- *              user to perform an cross-site scripting attack.
+ *              user to perform a cross-site scripting attack.
  * @kind path-problem
  * @problem.severity error
  * @precision high

From be69c3a458385525bc893d0dfb8fbf82e9dd05d1 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Thu, 6 May 2021 21:59:35 +0200
Subject: [PATCH 11/16] Apply suggestions from code review

Co-authored-by: Esben Sparre Andreasen <esbena@github.com>
---
 .../dataflow/UnsafeHtmlConstructionCustomizations.qll        | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
index a99b5e7986d5..99e729706426 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -38,7 +38,7 @@ module UnsafeHtmlConstruction {
 
   /**
    * A sink for unsafe HTML constructed from library input.
-   * This sink somehow transforms its input into a value that can cause XSS if it ends up in a XSS sink.
+   * This sink transforms its input into a value that can cause XSS if it ends up in a XSS sink.
    */
   abstract class Sink extends DataFlow::Node {
     /**
@@ -165,6 +165,7 @@ module UnsafeHtmlConstruction {
     MarkdownSink() {
       exists(DataFlow::Node pred, DataFlow::Node succ, Markdown::MarkdownStep step |
         step.step(pred, succ) and
+        step.preservesHtml() and
         this = pred and
         succ = isUsedInXssSink(xssSink)
       )
@@ -176,7 +177,7 @@ module UnsafeHtmlConstruction {
   /**
    *  Holds if there is a path without unmatched return steps from `source` to `sink`.
    */
-  predicate requireMatchedReturn(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+  predicate hasPathWithoutUnmatchedReturn(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
     exists(DataFlow::MidPathNode mid |
       source.getASuccessor*() = mid and
       sink = mid.getASuccessor() and

From b53759c5a059b40cc6865371fd46f0d848e6a737 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Thu, 6 May 2021 22:49:25 +0200
Subject: [PATCH 12/16] corrections after code review

---
 .../ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp    | 4 ++--
 javascript/ql/src/semmle/javascript/frameworks/Markdown.qll | 5 +++++
 .../javascript/security/dataflow/UnsafeHtmlConstruction.qll | 2 +-
 .../dataflow/UnsafeHtmlConstructionCustomizations.qll       | 6 ++++--
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
index 76a7e65fd3f2..619b55e4920b 100644
--- a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
+++ b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
@@ -4,8 +4,8 @@
 <qhelp>
 <overview>
 	<p>
-		Dynamically constructing HTML with inputs from library functions may 
-		inadvertently leave a client open to XSS attacks.
+		Dynamically constructing HTML with inputs from library functions that are 
+		available to external clients may inadvertently leave a client open to XSS attacks.
 		
 		Clients using the exported function may use inputs containing unsafe HTML,
 		and if these inputs end up in the DOM, the client may be vulnerable to
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
index c98243e4fbbd..1a6c9d7a8c2b 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
@@ -17,6 +17,11 @@ module Markdown {
      * Holds if there is a taint-step from `pred` to `succ` for a taint-preserving markdown parser.
      */
     abstract predicate step(DataFlow::Node pred, DataFlow::Node succ);
+
+    /**
+     * Holds if the taint-step preserves HTML.
+     */
+    predicate preservesHTML() { any() }
   }
 
   private class MarkdownStepAsTaintStep extends TaintTracking::SharedTaintStep {
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
index 699d9d3e5101..53debb48ff61 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
@@ -38,7 +38,7 @@ module UnsafeHtmlConstruction {
     // override to require that there is a path without unmatched return steps
     override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
       super.hasFlowPath(source, sink) and
-      requireMatchedReturn(source, sink)
+      hasPathWithoutUnmatchedReturn(source, sink)
     }
 
     override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
index 99e729706426..8546041ef06c 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -165,7 +165,7 @@ module UnsafeHtmlConstruction {
     MarkdownSink() {
       exists(DataFlow::Node pred, DataFlow::Node succ, Markdown::MarkdownStep step |
         step.step(pred, succ) and
-        step.preservesHtml() and
+        step.preservesHTML() and
         this = pred and
         succ = isUsedInXssSink(xssSink)
       )
@@ -177,7 +177,9 @@ module UnsafeHtmlConstruction {
   /**
    *  Holds if there is a path without unmatched return steps from `source` to `sink`.
    */
-  predicate hasPathWithoutUnmatchedReturn(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+  predicate hasPathWithoutUnmatchedReturn(
+    DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink
+  ) {
     exists(DataFlow::MidPathNode mid |
       source.getASuccessor*() = mid and
       sink = mid.getASuccessor() and

From 3fe5dd0f3508525ba9ef9724784d15709dbc04d9 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 10 May 2021 10:05:18 +0200
Subject: [PATCH 13/16] add comment about filtering away jQuery from the source

---
 .../security/dataflow/UnsafeHtmlConstructionCustomizations.qll   | 1 +
 1 file changed, 1 insertion(+)

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
index 8546041ef06c..61ee6cd46221 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -25,6 +25,7 @@ module UnsafeHtmlConstruction {
   class ExternalInputSource extends Source, DataFlow::ParameterNode {
     ExternalInputSource() {
       this = Exports::getALibraryInputParameter() and
+      // An AMD-style module sometimes loads the jQuery library in a way which looks like library input.
       not this = JQuery::dollarSource()
     }
   }

From 646bf994896d951c90a2cc09dc40ef1500625a63 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 10 May 2021 10:45:33 +0200
Subject: [PATCH 14/16] rewrite the qhelp to focus more on documenting unsafe
 functions

---
 .../CWE-079/UnsafeHtmlConstruction.qhelp      | 29 +++++++++----------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
index 619b55e4920b..0bc3bd53a0d0 100644
--- a/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
+++ b/javascript/ql/src/Security/CWE-079/UnsafeHtmlConstruction.qhelp
@@ -4,26 +4,22 @@
 <qhelp>
 <overview>
 	<p>
-		Dynamically constructing HTML with inputs from library functions that are 
-		available to external clients may inadvertently leave a client open to XSS attacks.
-		
-		Clients using the exported function may use inputs containing unsafe HTML,
-		and if these inputs end up in the DOM, the client may be vulnerable to
-		cross-site scripting attacks.
-	</p>
+		When a library function dynamically constructs HTML in a potentially unsafe
+		way, then it's important to document to clients of the library that the function 
+		should only be used with trusted inputs. 
 
+		If the function is not documented as being potentially unsafe, then a client
+		may inadvertently use inputs containing unsafe HTML fragments, and thereby leave
+		the client vulnerable to cross-site scripting attacks.
+	</p>
 </overview>
 <recommendation>
 
 	<p>
-		If possible, use safe APIs when inserting HTML into the DOM.  
-		Such as writing to the <code>innerText</code> property instead of <code>innerHTML</code>.
+		Document all library functions that can lead to cross-site scripting 
+		attacks, and guard against unsafe inputs where dynamic HTML
+		construction is not intended.
 	</p>
-
-	<p>
-		Alternatively, use a HTML sanitizer to escape/remove unsafe content.
-	</p>
-
 </recommendation>
 <example>
 
@@ -41,13 +37,14 @@
 	</p>
 
 	<p>
-		To avoid such attacks, a program can use safe APIs such as <code>innerText</code>.
+		The library could either document that this function should not be used
+		with unsafe inputs, or use safe APIs such as <code>innerText</code>.
 	</p>
 
 	<sample src="examples/unsafe-html-construction_safe.js" />
 	
 	<p>
-		Alternatively, use a HTML sanitizer to remove unsafe content.
+		Alternatively, a HTML sanitizer can be used to remove unsafe content.
 	</p>
 
 	<sample src="examples/unsafe-html-construction_sanitizer.js" />

From b4e35f54d9bc6900f2f881e05366a7dc043b92d9 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 10 May 2021 10:47:34 +0200
Subject: [PATCH 15/16] fix typo

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

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
index 1a6c9d7a8c2b..b2235577b04b 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Markdown.qll
@@ -10,7 +10,7 @@ import javascript
  */
 module Markdown {
   /**
-   * A taint-step that parses a markdown document, but preserves taint.import
+   * A taint-step that parses a markdown document, but preserves taint.
    */
   class MarkdownStep extends Unit {
     /**

From d9136689430f412491f81830bc06801f7b3a40f6 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 10 May 2021 10:55:09 +0200
Subject: [PATCH 16/16] move hasPathWithoutUnmatchedReturn to Configuration.qll

---
 .../semmle/javascript/dataflow/Configuration.qll    | 11 +++++++++++
 .../security/dataflow/UnsafeHtmlConstruction.qll    |  2 +-
 .../UnsafeHtmlConstructionCustomizations.qll        | 13 -------------
 .../dataflow/UnsafeShellCommandConstruction.qll     |  3 +--
 4 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
index b4842026e24c..45e7515df916 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -2068,3 +2068,14 @@ class VarAccessBarrier extends DataFlow::Node {
     )
   }
 }
+
+/**
+ *  Holds if there is a path without unmatched return steps from `source` to `sink`.
+ */
+predicate hasPathWithoutUnmatchedReturn(SourcePathNode source, SinkPathNode sink) {
+  exists(MidPathNode mid |
+    source.getASuccessor*() = mid and
+    sink = mid.getASuccessor() and
+    mid.getPathSummary().hasReturn() = false
+  )
+}
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
index 53debb48ff61..4fe675b16d9c 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
@@ -38,7 +38,7 @@ module UnsafeHtmlConstruction {
     // override to require that there is a path without unmatched return steps
     override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
       super.hasFlowPath(source, sink) and
-      hasPathWithoutUnmatchedReturn(source, sink)
+      DataFlow::hasPathWithoutUnmatchedReturn(source, sink)
     }
 
     override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
index 61ee6cd46221..c660657a322e 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
@@ -174,17 +174,4 @@ module UnsafeHtmlConstruction {
 
     override string describe() { result = "Markdown rendering" }
   }
-
-  /**
-   *  Holds if there is a path without unmatched return steps from `source` to `sink`.
-   */
-  predicate hasPathWithoutUnmatchedReturn(
-    DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink
-  ) {
-    exists(DataFlow::MidPathNode mid |
-      source.getASuccessor*() = mid and
-      sink = mid.getASuccessor() and
-      mid.getPathSummary().hasReturn() = false
-    )
-  }
 }
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll
index 2c7de7555def..d47b3696de61 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstruction.qll
@@ -14,7 +14,6 @@ import javascript
  */
 module UnsafeShellCommandConstruction {
   import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
-  import UnsafeHtmlConstructionCustomizations
 
   /**
    * A taint-tracking configuration for reasoning about shell command constructed from library input vulnerabilities.
@@ -36,7 +35,7 @@ module UnsafeShellCommandConstruction {
     // override to require that there is a path without unmatched return steps
     override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
       super.hasFlowPath(source, sink) and
-      UnsafeHtmlConstruction::requireMatchedReturn(source, sink)
+      DataFlow::hasPathWithoutUnmatchedReturn(source, sink)
     }
 
     override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {