Add merlyn.md which explains the changes on this branch.

This reverts commit 50a015c73e.

change-notes/1.26/analysis-javascript.md

@@ -70,4 +70,3 @@
 
 ## 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, subclasses should instead be based on `HtmlInjectionConfiguration`. To use both configurations in a query, see [Xss.ql](https://github.com/github/codeql/blob/main/javascript/ql/src/Security/CWE-079/Xss.ql) for an example.

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

@@ -15,13 +15,8 @@ import javascript
 import semmle.javascript.security.dataflow.DomBasedXss::DomBasedXss
 import DataFlow::PathGraph
 
-from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
-where
-  (
-    cfg instanceof HtmlInjectionConfiguration or
-    cfg instanceof JQueryHtmlOrSelectorInjectionConfiguration
-  ) and
-  cfg.hasFlowPath(source, sink)
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
   sink.getNode().(Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(),
   "user-provided value"

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

@@ -105,35 +105,6 @@ module Angular2 {
     result = urlSegment().getAPropertyRead("parameters") and kind.isPath()
   }
 
-  /** Gets a reference to a `Params` object, usually containing values from the URL. */
-  DataFlow::SourceNode paramDictionaryObject() { result = paramDictionaryObject(_) }
-
-  /**
-   * A value from `@angular/router` derived from the URL.
-   */
-  class AngularSource extends ClientSideRemoteFlowSource {
-    ClientSideRemoteFlowKind kind;
-
-    AngularSource() {
-      this = paramMap(kind).getAMethodCall(["get", "getAll"])
-      or
-      this = paramDictionaryObject(kind)
-      or
-      this = activatedRouteProp("fragment") and kind.isFragment()
-      or
-      this = urlSegment().getAPropertyRead("path") and kind.isPath()
-      or
-      // Note that Router.url and RouterStateSnapshot.url are strings, not UrlSegment[]
-      this = router().getAPropertyRead("url") and kind.isUrl()
-      or
-      this = routerStateSnapshot().getAPropertyRead("url") and kind.isUrl()
-    }
-
-    override string getSourceType() { result = "Angular route parameter" }
-
-    override ClientSideRemoteFlowKind getKind() { result = kind }
-  }
-
   /** Gets a reference to a `DomSanitizer` object. */
   DataFlow::SourceNode domSanitizer() {
     result.hasUnderlyingType(["@angular/platform-browser", "@angular/core"], "DomSanitizer")

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

@@ -489,39 +489,32 @@ module Express {
     string kind;
 
     RequestInputAccess() {
-      kind = "parameter" and
-      this =
-        [
-          getAQueryObjectReference(DataFlow::TypeTracker::end(), rh),
-          getAParamsObjectReference(DataFlow::TypeTracker::end(), rh)
-        ].getAPropertyRead()
-      or
-      exists(DataFlow::SourceNode request | request = rh.getARequestSource().ref() |
+      exists(DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
         kind = "parameter" and
-        this = request.getAMethodCall("param")
+        (
+          this.(DataFlow::MethodCallNode).calls(request, "param")
+          or
+          exists(DataFlow::PropRead base, string propName |
+            // `req.params.name` or `req.query.name`
+            base.accesses(request, propName) and
+            this = base.getAPropertyReference(_)
+          |
+            propName = "params" or
+            propName = "query"
+          )
+        )
         or
         // `req.originalUrl`
         kind = "url" and
-        this = request.getAPropertyRead("originalUrl")
+        this.(DataFlow::PropRef).accesses(request, "originalUrl")
         or
         // `req.cookies`
         kind = "cookie" and
-        this = request.getAPropertyRead("cookies")
-        or
-        // `req.files`, treated the same as `req.body`.
-        // `express-fileupload` uses .files, and `multer` uses .files or .file
-        kind = "body" and
-        this = request.getAPropertyRead(["files", "file"])
+        this.(DataFlow::PropRef).accesses(request, "cookies")
       )
       or
       kind = "body" and
       this.asExpr() = rh.getARequestBodyAccess()
-      or
-      // `value` in `router.param('foo', (req, res, next, value) => { ... })`
-      kind = "parameter" and
-      exists(RouteSetup setup | rh = setup.getARouteHandler() |
-        this = DataFlow::parameterNode(rh.getRouteHandlerParameter("parameter"))
-      )
     }
 
     override RouteHandler getRouteHandler() { result = rh }

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

@@ -125,42 +125,6 @@ module Fastify {
     }
   }
 
-  /**
-   * An access to a user-controlled Fastify request input.
-   */
-  private class RequestInputAccess extends HTTP::RequestInputAccess {
-    RouteHandler rh;
-    string kind;
-
-    RequestInputAccess() {
-      exists(string name | this = rh.getARequestSource().ref().getAPropertyRead(name) |
-        kind = "parameter" and
-        name = ["params", "query"]
-        or
-        kind = "body" and
-        name = "body"
-      )
-    }
-
-    override RouteHandler getRouteHandler() { result = rh }
-
-    override string getKind() { result = kind }
-
-    override predicate isUserControlledObject() {
-      kind = "body" and
-      (
-        usesFastifyPlugin(rh,
-          DataFlow::moduleImport(["fastify-xml-body-parser", "fastify-formbody"]))
-        or
-        usesMiddleware(rh,
-          any(ExpressLibraries::BodyParser bodyParser | bodyParser.producesUserControlledObjects()))
-      )
-      or
-      kind = "parameter" and
-      usesFastifyPlugin(rh, DataFlow::moduleImport("fastify-qs"))
-    }
-  }
-
   /**
    * Holds if `rh` uses `plugin`.
    */
@@ -193,25 +157,6 @@ module Fastify {
     )
   }
 
-  /**
-   * An access to a header on a Fastify request.
-   */
-  private class RequestHeaderAccess extends HTTP::RequestHeaderAccess {
-    RouteHandler rh;
-
-    RequestHeaderAccess() {
-      this = rh.getARequestSource().ref().getAPropertyRead("headers").getAPropertyRead()
-    }
-
-    override string getAHeaderName() {
-      result = this.(DataFlow::PropRead).getPropertyName().toLowerCase()
-    }
-
-    override RouteHandler getRouteHandler() { result = rh }
-
-    override string getKind() { result = "header" }
-  }
-
   /**
    * An argument passed to the `send` or `end` method of an HTTP response object.
    */

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

@@ -4,60 +4,3 @@
 
 import javascript
 
-/**
- * A source of remote flow from the `Busboy` library.
- */
-private class BusBoyRemoteFlow extends RemoteFlowSource {
-  BusBoyRemoteFlow() {
-    this =
-      API::moduleImport("busboy")
-          .getInstance()
-          .getMember("on")
-          .getParameter(1)
-          .getAParameter()
-          .getAnImmediateUse()
-  }
-
-  override string getSourceType() { result = "parsed user value from Busbuy" }
-}
-
-/**
- * A source of remote flow from the `Formidable` library parsing a HTTP request.
- */
-private class FormidableRemoteFlow extends RemoteFlowSource {
-  FormidableRemoteFlow() {
-    exists(API::Node formidable |
-      formidable = API::moduleImport("formidable").getReturn()
-      or
-      formidable = API::moduleImport("formidable").getMember("formidable").getReturn()
-      or
-      formidable =
-        API::moduleImport("formidable").getMember(["IncomingForm", "Formidable"]).getInstance()
-    |
-      this =
-        formidable.getMember("parse").getACall().getABoundCallbackParameter(1, any(int i | i > 0))
-    )
-  }
-
-  override string getSourceType() { result = "parsed user value from Formidable" }
-}
-
-/**
- * A source of remote flow from the `Multiparty` library.
- */
-private class MultipartyRemoteFlow extends RemoteFlowSource {
-  MultipartyRemoteFlow() {
-    exists(API::Node form | form = API::moduleImport("multiparty").getMember("Form").getInstance() |
-      exists(API::CallNode parse | parse = form.getMember("parse").getACall() |
-        this = parse.getParameter(1).getAParameter().getAnImmediateUse()
-      )
-      or
-      exists(API::CallNode on | on = form.getMember("on").getACall() |
-        on.getArgument(0).mayHaveStringValue(["part", "file", "field"]) and
-        this = on.getParameter(1).getAParameter().getAnImmediateUse()
-      )
-    )
-  }
-
-  override string getSourceType() { result = "parsed user value from Multiparty" }
-}

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

@@ -475,23 +475,6 @@ module HTTP {
        */
       abstract Expr getServer();
     }
-
-    /**
-     * A parameter containing data received by a NodeJS HTTP server.
-     * E.g. `chunk` in: `http.createServer().on('request', (req, res) => req.on("data", (chunk) => ...))`.
-     */
-    private class ServerRequestDataEvent extends RemoteFlowSource, DataFlow::ParameterNode {
-      RequestSource req;
-
-      ServerRequestDataEvent() {
-        exists(DataFlow::MethodCallNode mcn | mcn = req.ref().getAMethodCall(EventEmitter::on()) |
-          mcn.getArgument(0).mayHaveStringValue("data") and
-          this = mcn.getABoundCallbackParameter(1, 0)
-        )
-      }
-
-      override string getSourceType() { result = "NodeJS HTTP server data event" }
-    }
   }
 
   /**

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

@@ -82,23 +82,6 @@ private module Micro {
     )
   }
 
-  class MicroBodyParserCall extends HTTP::RequestInputAccess, DataFlow::CallNode {
-    string name;
-
-    MicroBodyParserCall() {
-      name = ["buffer", "text", "json"] and
-      this = moduleMember("micro", name).getACall()
-    }
-
-    override string getKind() { result = "body" }
-
-    override HTTP::RouteHandler getRouteHandler() {
-      result = getRouteHandlerFromReqRes(getArgument(0))
-    }
-
-    override predicate isUserControlledObject() { name = "json" }
-  }
-
   class MicroSendArgument extends HTTP::ResponseSendArgument {
     CallNode send;
 

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

@@ -43,22 +43,6 @@ module NextJS {
     )
   }
 
-  /**
-   * User defined path parameter in `Next.js`.
-   */
-  class NextParams extends RemoteFlowSource {
-    NextParams() {
-      this =
-        getAModuleWithFallbackPaths()
-            .getAnExportedValue("getStaticProps")
-            .getAFunctionValue()
-            .getParameter(0)
-            .getAPropertyRead("params")
-    }
-
-    override string getSourceType() { result = "Next request parameter" }
-  }
-
   /**
    * Gets the `getStaticProps` function in a Next.js page.
    * This function is executed at build time, or when a page with a new URL is requested for the first time (if `fallback` is not false).

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

@@ -940,7 +940,7 @@ module NodeJSLib {
   private class ClientRequestDataEvent extends RemoteFlowSource {
     ClientRequestDataEvent() {
       exists(DataFlow::MethodCallNode mcn, ClientRequestResponseEvent cr |
-        cr.getAMethodCall(EventEmitter::on()) = mcn and
+        cr.getAMethodCall("on") = mcn and
         mcn.getArgument(0).mayHaveStringValue("data") and
         this = mcn.getCallback(1).getParameter(0)
       )
@@ -1175,17 +1175,6 @@ module NodeJSLib {
     NodeJSNetServerRegistration() { this = emitter.ref().getAMethodCall(EventEmitter::on()) }
   }
 
-  /**
-   * A data flow node representing data received from a client to a NodeJS net server, viewed as remote user input.
-   */
-  private class NodeJSNetServerItemAsRemoteFlow extends RemoteFlowSource {
-    NodeJSNetServerRegistration reg;
-
-    NodeJSNetServerItemAsRemoteFlow() { this = reg.getReceivedItem(_) }
-
-    override string getSourceType() { result = "NodeJS server" }
-  }
-
   /**
    * An instantiation of the `respjs` library, which is an EventEmitter.
    */

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

@@ -691,33 +691,6 @@ private DataFlow::SourceNode reactRouterDom() {
   result = DataFlow::moduleImport("react-router-dom")
 }
 
-private DataFlow::SourceNode reactRouterMatchObject() {
-  result = reactRouterDom().getAMemberCall(["useRouteMatch", "matchPath"])
-  or
-  exists(ReactComponent c |
-    dependedOnByReactRouterClient(c.getTopLevel()) and
-    result = c.getAPropRead("match")
-  )
-}
-
-private class ReactRouterSource extends ClientSideRemoteFlowSource {
-  ClientSideRemoteFlowKind kind;
-
-  ReactRouterSource() {
-    this = reactRouterDom().getAMemberCall("useParams") and kind.isPath()
-    or
-    exists(string prop | this = reactRouterMatchObject().getAPropertyRead(prop) |
-      prop = "params" and kind.isPath()
-      or
-      prop = "url" and kind.isUrl()
-    )
-  }
-
-  override string getSourceType() { result = "react-router path parameters" }
-
-  override ClientSideRemoteFlowKind getKind() { result = kind }
-}
-
 /**
  * Holds if `mod` transitively depends on `react-router-dom`.
  */

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

@@ -100,13 +100,4 @@ private module ServerLess {
       result = mod.getAnExportedValue(handler).getAFunctionValue()
     )
   }
-
-  /**
-   * A serverless request handler event, seen as a RemoteFlowSource.
-   */
-  private class ServerlessHandlerEventAsRemoteFlow extends RemoteFlowSource {
-    ServerlessHandlerEventAsRemoteFlow() { this = getAServerlessHandler().getParameter(0) }
-
-    override string getSourceType() { result = "Serverless event" }
-  }
 }

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

@@ -655,17 +655,4 @@ module Vue {
 
   /** Gets a data flow node that refers to a `Route` object from `vue-router`. */
   DataFlow::SourceNode routeObject() { result = routeObject(DataFlow::TypeTracker::end()) }
-
-  private class VueRouterFlowSource extends RemoteFlowSource {
-    VueRouterFlowSource() {
-      this = routeObject().getAPropertyRead(["params", "query", "hash", "path", "fullPath"])
-      or
-      exists(Instance i, string prop |
-        this = i.getWatchHandler(prop).getParameter([0, 1]) and
-        prop.regexpMatch("\\$route\\.(params|query|hash|path|fullPath)\\b.*")
-      )
-    }
-
-    override string getSourceType() { result = "Vue route parameter" }
-  }
 }

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

@@ -318,15 +318,4 @@ module ServerWebSocket {
       result = this.getCallback(1).getParameter(0)
     }
   }
-
-  /**
-   * A data flow node representing data received from a client, viewed as remote user input.
-   */
-  private class ReceivedItemAsRemoteFlow extends RemoteFlowSource {
-    ReceivedItemAsRemoteFlow() { this = any(ReceiveNode rercv).getReceivedItem(_) }
-
-    override string getSourceType() { result = "WebSocket client data" }
-
-    override predicate isUserControlledObject() { any() }
-  }
 }

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

@@ -206,11 +206,6 @@ class PostMessageEventHandler extends Function {
       addEventListener.getArgument(0).mayHaveStringValue("message") and
       addEventListener.getArgument(1).getABoundFunctionValue(paramIndex).getFunction() = this
     )
-    or
-    exists(DataFlow::Node rhs |
-      rhs = DataFlow::globalObjectRef().getAPropertyWrite("onmessage").getRhs() and
-      rhs.getABoundFunctionValue(paramIndex).getFunction() = this
-    )
   }
 
   /**

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

@@ -8,28 +8,15 @@ import javascript
 module DomBasedXss {
   import DomBasedXssCustomizations::DomBasedXss
 
-  /**
-   * DEPRECATED. Use `HtmlInjectionConfiguration` or `JQueryHtmlOrSelectorInjectionConfiguration`.
-   */
-  deprecated class Configuration = HtmlInjectionConfiguration;
-
-  /**
-   * DEPRECATED. Use `Vue::VHtmlSourceWrite` instead.
-   */
-  deprecated class VHtmlSourceWrite = Vue::VHtmlSourceWrite;
-
   /**
    * A taint-tracking configuration for reasoning about XSS.
    */
-  class HtmlInjectionConfiguration extends TaintTracking::Configuration {
-    HtmlInjectionConfiguration() { this = "HtmlInjection" }
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "DomBasedXss" }
 
     override predicate isSource(DataFlow::Node source) { source instanceof Source }
 
-    override predicate isSink(DataFlow::Node sink) {
-      sink instanceof Sink and
-      not sink instanceof JQueryHtmlOrSelectorSink // Handled by JQueryHtmlOrSelectorInjectionConfiguration below
-    }
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
     override predicate isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node)
@@ -41,66 +28,59 @@ module DomBasedXss {
       guard instanceof SanitizerGuard
     }
 
-    override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-      DomBasedXss::isOptionallySanitizedEdge(pred, succ)
-    }
-  }
-
-  /**
-   * A taint-tracking configuration for reasoning about injection into the jQuery `$` function
-   * or similar, where the interpretation of the input string depends on its first character.
-   *
-   * Values are only considered tainted if they can start with the `<` character.
-   */
-  class JQueryHtmlOrSelectorInjectionConfiguration extends TaintTracking::Configuration {
-    JQueryHtmlOrSelectorInjectionConfiguration() { this = "JQueryHtmlOrSelectorInjection" }
-
-    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
-      // Reuse any source not derived from location
-      source instanceof Source and
-      not source = [DOM::locationRef(), DOM::locationRef().getAPropertyRead()] and
-      label.isTaint()
-      or
-      source = DOM::locationSource() and
-      label.isData() // Require transformation before reaching sink
-      or
-      source = DOM::locationRef().getAPropertyRead(["hash", "search"]) and
-      label.isData() // Require transformation before reaching sink
-    }
-
-    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
-      sink instanceof JQueryHtmlOrSelectorSink and label.isTaint()
-    }
-
-    override predicate isAdditionalFlowStep(
-      DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
-      DataFlow::FlowLabel succlbl
+    override predicate isAdditionalStoreStep(
+      DataFlow::Node pred, DataFlow::SourceNode succ, string prop
     ) {
-      TaintTracking::sharedTaintStep(pred, succ) and
-      predlbl.isData() and
-      succlbl.isTaint()
-    }
-
-    override predicate isSanitizer(DataFlow::Node node) {
-      super.isSanitizer(node)
-      or
-      node instanceof Sanitizer
-    }
-
-    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
-      guard instanceof SanitizerGuard
-    }
-
-    override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-      DomBasedXss::isOptionallySanitizedEdge(pred, succ)
-      or
-      // Avoid stepping from location -> location.hash, as the .hash is already treated as a source
-      // (with a different flow label)
       exists(DataFlow::PropRead read |
-        read = DOM::locationRef().getAPropertyRead(["hash", "search"]) and
         pred = read.getBase() and
-        succ = read
+        succ = read and
+        read.getPropertyName() = "hash" and
+        prop = urlSuffixPseudoProperty()
       )
     }
+
+    override predicate isAdditionalLoadStoreStep(
+      DataFlow::Node pred, DataFlow::Node succ, string predProp, string succProp
+    ) {
+      exists(DataFlow::PropRead read |
+        pred = read.getBase() and
+        succ = read and
+        read.getPropertyName() = "hash" and
+        predProp = "hash" and
+        succProp = urlSuffixPseudoProperty()
+      )
+    }
+
+    override predicate isAdditionalLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = ["substr", "substring", "slice"] and
+        not call.getArgument(0).getIntValue() = 0 and
+        pred = call.getReceiver() and
+        succ = call and
+        prop = urlSuffixPseudoProperty()
+      )
+      or
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = "exec" and pred = call.getArgument(0)
+        or
+        call.getMethodName() = "match" and pred = call.getReceiver()
+      |
+        succ = call and
+        prop = urlSuffixPseudoProperty()
+      )
+      or
+      exists(StringSplitCall split |
+        split.getSeparator() = ["#", "?"] and
+        pred = split.getBaseString() and
+        succ = split.getASubstringRead(1) and
+        prop = urlSuffixPseudoProperty()
+      )
+    }
+
+    override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
+      DomBasedXss::isOptionallySanitizedEdge(pred, succ)
+    }
   }
+
+  private string urlSuffixPseudoProperty() { result = "$UrlSuffix$" }
 }

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

@@ -34,11 +34,18 @@ 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,
-    DomBasedXss::JQueryHtmlOrSelectorArgument {
+  class AmbiguousHtmlOrSelectorArgument extends DataFlow::Node {
     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(getAPrefix())
+      not exists(DataFlow::Node prefix |
+        DomBasedXss::isPrefixOfJQueryHtmlString(this, prefix) and
+        prefix.mayHaveStringValue(_)
+      )
     }
   }
 

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

@@ -3,7 +3,6 @@
  */
 
 import javascript
-private import semmle.javascript.dataflow.InferredTypes
 
 /** Provides classes and predicates shared between the XSS queries. */
 module Shared {
@@ -148,9 +147,19 @@ module DomBasedXss {
   class LibrarySink extends Sink {
     LibrarySink() {
       // call to a jQuery method that interprets its argument as HTML
-      exists(JQuery::MethodCall call |
-        call.interpretsArgumentAsHtml(this) and
-        not call.interpretsArgumentAsSelector(this) // Handled by `JQuerySelectorSink`
+      exists(JQuery::MethodCall call | call.interpretsArgumentAsHtml(this) |
+        // either the argument is always interpreted as HTML
+        not call.interpretsArgumentAsSelector(this)
+        or
+        // or it doesn't start with something other than `<`, and so at least
+        // _may_ be interpreted as HTML
+        not exists(DataFlow::Node prefix, string strval |
+          isPrefixOfJQueryHtmlString(this, prefix) and
+          strval = prefix.getStringValue() and
+          not strval = "" and
+          not strval.regexpMatch("\\s*<.*")
+        ) and
+        not DOM::locationRef().flowsTo(this)
       )
       or
       // call to an Angular method that interprets its argument as HTML
@@ -184,54 +193,16 @@ module DomBasedXss {
    * HTML by a jQuery method.
    */
   predicate isPrefixOfJQueryHtmlString(DataFlow::Node htmlString, DataFlow::Node prefix) {
-    prefix = getAPrefixOfJQuerySelectorString(htmlString)
-  }
-
-  /**
-   * Holds if `prefix` is a prefix of `htmlString`, which may be intepreted as
-   * HTML by a jQuery method.
-   */
-  private DataFlow::Node getAPrefixOfJQuerySelectorString(DataFlow::Node htmlString) {
-    any(JQuery::MethodCall call).interpretsArgumentAsSelector(htmlString) and
-    result = htmlString
+    any(JQuery::MethodCall call).interpretsArgumentAsHtml(htmlString) and
+    prefix = htmlString
     or
-    exists(DataFlow::Node pred | pred = getAPrefixOfJQuerySelectorString(htmlString) |
-      result = StringConcatenation::getFirstOperand(pred)
+    exists(DataFlow::Node pred | isPrefixOfJQueryHtmlString(htmlString, pred) |
+      prefix = StringConcatenation::getFirstOperand(pred)
       or
-      result = pred.getAPredecessor()
+      prefix = pred.getAPredecessor()
     )
   }
 
-  /**
-   * 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 JQueryHtmlOrSelectorArgument extends DataFlow::Node {
-    JQueryHtmlOrSelectorArgument() {
-      exists(JQuery::MethodCall call |
-        call.interpretsArgumentAsHtml(this) and
-        call.interpretsArgumentAsSelector(this) and
-        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*<.*|"))
-    }
-  }
-
   /**
    * An expression whose value is interpreted as HTML or CSS
    * and may be inserted into the DOM.
@@ -339,7 +310,30 @@ module DomBasedXss {
    */
   class SafePropertyReadSanitizer extends Sanitizer, DataFlow::Node {
     SafePropertyReadSanitizer() {
-      exists(PropAccess pacc | pacc = this.asExpr() | pacc.getPropertyName() = "length")
+      exists(PropAccess pacc | pacc = this.asExpr() |
+        isSafeLocationProperty(pacc)
+        or
+        // `$(location.hash)` is a fairly common and safe idiom
+        // (because `location.hash` always starts with `#`),
+        // so we mark `hash` as safe for the purposes of this query
+        pacc.getPropertyName() = "hash"
+        or
+        pacc.getPropertyName() = "length"
+      )
+    }
+  }
+
+  /**
+   * A sanitizer that reads the first part a location split by "?", e.g. `location.href.split('?')[0]`.
+   */
+  class QueryPrefixSanitizer extends Sanitizer {
+    StringSplitCall splitCall;
+
+    QueryPrefixSanitizer() {
+      this = splitCall.getASubstringRead(0) and
+      splitCall.getSeparator() = "?" and
+      splitCall.getBaseString().getALocalSource() =
+        [DOM::locationRef(), DOM::locationRef().getAPropertyRead("href")]
     }
   }
 

javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.ql

@@ -16,7 +16,7 @@ import semmle.javascript.security.dataflow.DomBasedXss::DomBasedXss
 import DataFlow::PathGraph
 import semmle.javascript.heuristics.AdditionalSources
 
-from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink) and source.getNode() instanceof HeuristicSource
 select sink.getNode(), source, sink,
   sink.getNode().(Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(),

merlyn.md

@@ -0,0 +1,120 @@
+# merlyn
+
+Similar to
+[benjamin-button](https://github.com/github/codeql/blob/experimental/benjamin-button/benjamin-button.md),
+this branch aims to approximate an older version of the CodeQL JavaScript libraries. While
+benjamin-button removes support for recently added sinks, merlyn removes support for recently added
+sources. The branches should be cleanly mergeable, allowing us to independently or jointly assess
+the ability of an analysis technique to infer sources and sinks.
+
+Like benjamin-button, merlyn focuses on `TaintedPath.ql`, `Xss.ql`, and `SqlInjection.ql` (which we
+sometimes consider as two queries, one for SQL injection and one for NoSQL injection). We aim to
+remove all sources added since 1 January 2020. This is also the cut-off date for `TaintedPath.ql` on
+benjamin-button (the other queries have some even older sinks removed).
+
+Surveying our three queries, we note that they all have `RemoteFlowSource` as their main kind of
+taint source, with `Xss.ql` adding a few more sources besides. Below we detail how we rolled back
+the changes to `RemoteFlowSource`, and then separately talk about the other sources in `Xss.ql`.
+
+## `RemoteFlowSource`
+
+We consider all transitive subclasses of `RemoteFlowSource`, remove those added since the cut-off
+date entirely, and restore the other ones to their older state.
+
+Here is the class hierarchy for `RemoteFlowSource`, with information about relevant changes to each subclass. The effects of these commits have been undone on this branch.
+
+- `RemoteFlowSource`
+  - `AngularSource`:
+    introduced in [afd82e202d JS: Add Angular2 model](https://github.com/github/codeql/commit/afd82e202d)
+  - `BusBoyRemoteFlow`:
+    introduced in [a03f4ed3cd add remote flow source for `busboy`](https://github.com/github/codeql/commit/a03f4ed3cd)
+  - `ClientRequestDataEvent`:
+    modified in [0b55aed626 use the EventEmitter registration methods instead of just "on"](https://github.com/github/codeql/commit/0b55aed626)
+  - `ClientRequestErrorEvent`:
+    no relevant changes since the cut-off date
+  - `ClientRequestLoginEvent`:
+    no relevant changes since the cut-off date
+  - `ClientRequestRedirectEvent`:
+    no relevant changes since the cut-off date
+  - `ClientRequestResponseEvent`:
+    no relevant changes since the cut-off date
+  - `ExternalRemoteFlowSource`:
+    not relevant; this is for user-specified extra flow sources
+  - `FirebaseVal`:
+    no relevant changes since the cut-off date
+  - `FormidableRemoteFlow`:
+    introduced in [61b4ffec3d add remote flow from the `Formidable` library](https://github.com/github/codeql/commit/61b4ffec3d)
+  - `LocationFlowSource`:
+    no relevant changes since the cut-off date
+  - `MicroBodyParserCall`:
+    introduced in [4795b87daa JS: Add model of Micro](https://github.com/github/codeql/commit/4795b87daa)
+  - `MultipartyRemoteFlow`:
+    added in [010d580f8e add model for `multiparty`](https://github.com/github/codeql/commit/010d580f8e)
+  - `NextParams`:
+    introduced in [9d7bb57d8a add parameter values from Next as a RemoteFlowSource](https://github.com/github/codeql/commit/9d7bb57d8a)
+  - `NodeJSNetServerItemAsRemoteFlow`:
+    introduced in [082967a629 add EventEmitter models for `net.createServer()` and `respjs`.](https://github.com/github/codeql/commit/082967a629)
+  - `PostMessageEventParameter`:
+    indirectly modified in [cb7de2714a add `onmessage` handlers registered using global property as `PostMessageEventHandler`](https://github.com/github/codeql/commit/cb7de2714a)
+  - `ReactRouterSource`:
+    introduced in [d116b424f4 JS: Add model of react hooks and react-router](https://github.com/github/codeql/commit/d116b424f4)
+  - `ReceivedItemAsRemoteFlow`:
+    no relevant changes since the cut-off date
+  - `RemoteFlowPassword`:
+    not relevant; this is part of the heuristics library
+  - `RemoteServerResponse`:
+    not relevant; this is part of the heuristics library
+  - `RequestInputAccess`:
+    - `Connect::RequestInputAccess`:
+      no relevant changes since the cut-off date
+    - `Express::RequestInputAccess`: changed in
+      - [e2fbf8a68c add files uploaded with `multer` as RemoteFlowSource](https://github.com/github/codeql/commit/e2fbf8a68c)
+      - [89ef6ea4eb C++/C#/Java/JavaScript/Python: Autoformat set literals.](https://github.com/github/codeql/commit/89ef6ea4eb) (irrelevant, autoformat)
+      - [83f0514475 add req.files as a RequestInputAccess in the Express model](https://github.com/github/codeql/commit/83f0514475)
+      - [2b0a091921 split out type-tracking into two predicates, to avoid catastrophic join-order](https://github.com/github/codeql/commit/2b0a091921) (irrelevant, refactoring)
+      - [ed48efe5b4 recognize access to a query object through function calls](https://github.com/github/codeql/commit/ed48efe5b4)
+      - [d84f1b47c2 JS: Refactor RequestInputAccess to use source nodes](https://github.com/github/codeql/commit/d84f1b47c2)
+      - [c45d84f8f3 JS: Update getRouteHandlerParameter and router tracking](https://github.com/github/codeql/commit/c45d84f8f3)
+      - [9cacfab7c6 JS: Recognize Express param value callback as RemoteFlowSource](https://github.com/github/codeql/commit/9cacfab7c6)
+    - `Fastify::RequestInputAccess`:
+      added in [a76c70d2d7 JS: model fastify](https://github.com/github/codeql/commit/a76c70d2d7)
+    - `Hapi::RequestInputAccess`:
+      no relevant changes since the cut-off date
+    - `Koa::RequestInputAccess`:
+      no relevant changes since the cut-off date
+    - `NodeJSLib::RequestInputAccess`
+      no relevant changes since the cut-off date
+    - `RequestHeaderAccess`:
+      - `Express::RequestHeaderAccess`:
+        no relevant changes since the cut-off date
+      - `Fastify::RequestHeaderAccess`:
+        added in [a76c70d2d7 JS: model fastify](https://github.com/github/codeql/commit/a76c70d2d7)
+      - `Hapi::RequestHeaderAccess`:
+        no relevant changes since the cut-off date
+      - `Koa::RequestHeaderAccess`:
+        no relevant changes since the cut-off date
+      - `NodeJSLib::RequestHeaderAccess`:
+        no relevant changes since the cut-off date
+    - `Restify::RequestInputAccess`
+      no relevant changes since the cut-off date
+  - `RouteParamSource`:
+    no relevant changes since the cut-off date
+  - `ServerlessHandlerEventAsRemoteFlow`:
+    introduced in [1ed026fcce add a RemoteFlowSource for serverless handlers](https://github.com/github/codeql/commit/1ed026fcce)
+  - `ServerRequestDataEvent`:
+    introduced in [4dec2171da add http request server data as a RemoteFlowSource](https://github.com/github/codeql/commit/4dec2171da)
+  - `UserControlledTorrentInfo`:
+    no relevant changes since the cut-off date
+  - `VueRouterFlowSource`:
+    introduced in [5264d24f34 JS: Model vue-router](https://github.com/github/codeql/commit/5264d24f34)
+  - `WindowNameAccess`:
+    no relevant changes since the cut-off date
+
+## `Xss.ql`
+
+The data-flow configuration was split and new sources were introduced in the following two commits:
+
+- [c91cdb5194 JS: Address review comments](https://github.com/github/codeql/commit/c91cdb5194)
+- [50a015c73e JS: Move $() sink into separate dataflow config](https://github.com/github/codeql/commit/50a015c73e)
+
+Both commits have been reverted on this branch.