JS: Address review comments

Author: asgerf

change-notes/1.26/analysis-javascript.md

@@ -58,3 +58,5 @@
 
 ## Changes to libraries
 * The predicate `TypeAnnotation.hasQualifiedName` now works in more cases when the imported library was not present during extraction.
+* The class `DomBasedXss::Configuration` has been deprecated, as it has been split into `DomBasedXss::HtmlInjectionConfiguration` and `DomBasedXss::JQueryHtmlOrSelectorInjectionConfiguration`. Unless specifically working with jQuery sinks, uses of that
+class should be replaced with `HtmlInjectionConfiguration`.

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

@@ -19,7 +19,7 @@ from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode
 where
   (
     cfg instanceof HtmlInjectionConfiguration or
-    cfg instanceof JQuerySelectorInjectionConfiguration
+    cfg instanceof JQueryHtmlOrSelectorInjectionConfiguration
   ) and
   cfg.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,

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

@@ -9,7 +9,7 @@ module DomBasedXss {
   import DomBasedXssCustomizations::DomBasedXss
 
   /**
-   * DEPRECATED. Use `HtmlInjectionConfiguration` or `JQuerySelectorInjectionConfiguration`.
+   * DEPRECATED. Use `HtmlInjectionConfiguration` or `JQueryHtmlOrSelectorInjectionConfiguration`.
    */
   deprecated class Configuration = HtmlInjectionConfiguration;
 
@@ -23,7 +23,7 @@ module DomBasedXss {
 
     override predicate isSink(DataFlow::Node sink) {
       sink instanceof Sink and
-      not sink instanceof JQuerySelectorSink // Handled by JQuerySelectorInjectionConfiguration below
+      not sink instanceof JQueryHtmlOrSelectorSink // Handled by JQueryHtmlOrSelectorInjectionConfiguration below
     }
 
     override predicate isSanitizer(DataFlow::Node node) {
@@ -47,8 +47,8 @@ module DomBasedXss {
    *
    * Values are only considered tainted if they can start with the `<` character.
    */
-  class JQuerySelectorInjectionConfiguration extends TaintTracking::Configuration {
-    JQuerySelectorInjectionConfiguration() { this = "JQuerySelectorInjection" }
+  class JQueryHtmlOrSelectorInjectionConfiguration extends TaintTracking::Configuration {
+    JQueryHtmlOrSelectorInjectionConfiguration() { this = "JQueryHtmlOrSelectorInjection" }
 
     override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
       // Reuse any source not derived from location
@@ -64,7 +64,7 @@ module DomBasedXss {
     }
 
     override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
-      sink instanceof JQuerySelectorSink and label.isTaint()
+      sink instanceof JQueryHtmlOrSelectorSink and label.isTaint()
     }
 
     override predicate isAdditionalFlowStep(

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

@@ -34,18 +34,11 @@ module UnsafeJQueryPlugin {
   /**
    * An argument that may act as a HTML fragment rather than a CSS selector, as a sink for remote unsafe jQuery plugins.
    */
-  class AmbiguousHtmlOrSelectorArgument extends DataFlow::Node {
+  class AmbiguousHtmlOrSelectorArgument extends DataFlow::Node,
+    DomBasedXss::JQueryHtmlOrSelectorArgument {
     AmbiguousHtmlOrSelectorArgument() {
-      exists(JQuery::MethodCall call |
-        call.interpretsArgumentAsSelector(this) and call.interpretsArgumentAsHtml(this)
-      ) and
-      // the $-function in particular will not construct HTML for non-string values
-      analyze().getAType() = TTString() and
       // any fixed prefix makes the call unambiguous
-      not exists(DataFlow::Node prefix |
-        DomBasedXss::isPrefixOfJQueryHtmlString(this, prefix) and
-        prefix.mayHaveStringValue(_)
-      )
+      not exists(getAPrefix())
     }
   }
 

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

@@ -3,6 +3,7 @@
  */
 
 import javascript
+private import semmle.javascript.dataflow.InferredTypes
 
 /** Provides classes and predicates shared between the XSS queries. */
 module Shared {
@@ -201,20 +202,33 @@ module DomBasedXss {
   }
 
   /**
-   * An argument to the jQuery `$` function, which is interpreted as either a selector
+   * An argument to the jQuery `$` function or similar, which is interpreted as either a selector
    * or as an HTML string depending on its first character.
    */
-  class JQuerySelectorSink extends Sink {
-    JQuerySelectorSink() {
+  class JQueryHtmlOrSelectorArgument extends DataFlow::Node {
+    JQueryHtmlOrSelectorArgument() {
       exists(JQuery::MethodCall call |
         call.interpretsArgumentAsHtml(this) and
         call.interpretsArgumentAsSelector(this) and
-        // If a prefix of the string is known, it must start with '<' or be an empty string
-        forall(string strval | strval = getAPrefixOfJQuerySelectorString(this).getStringValue() |
-          strval.regexpMatch("(?s)\\s*<.*|")
-        )
+        analyze().getAType() = TTString()
       )
     }
+
+    /** Gets a string that flows to the prefix of this argument. */
+    string getAPrefix() { result = getAPrefixOfJQuerySelectorString(this).getStringValue() }
+  }
+
+  /**
+   * An argument to the jQuery `$` function or similar, which may be interpreted as HTML.
+   *
+   * This is the same as `JQueryHtmlOrSelectorArgument`, excluding cases where the value
+   * is prefixed by something other than `<`.
+   */
+  class JQueryHtmlOrSelectorSink extends Sink, JQueryHtmlOrSelectorArgument {
+    JQueryHtmlOrSelectorSink() {
+      // If a prefix of the string is known, it must start with '<' or be an empty string
+      forall(string strval | strval = getAPrefix() | strval.regexpMatch("(?s)\\s*<.*|"))
+    }
   }
 
   /**