From 2104cf55e3e50f2893668afdb78792a394fcb440 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Thu, 30 Aug 2018 10:18:49 +0200
Subject: [PATCH 01/28] JS: add models of URL requests

---
 javascript/ql/src/javascript.qll              |   1 +
 .../javascript/frameworks/UrlRequests.qll     | 127 ++++++++++++++++++
 .../UrlRequests/UrlRequest.expected           |  16 +++
 .../frameworks/UrlRequests/UrlRequest.ql      |   4 +
 .../frameworks/UrlRequests/tst.js             |  39 ++++++
 5 files changed, 187 insertions(+)
 create mode 100644 javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
 create mode 100644 javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.expected
 create mode 100644 javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql
 create mode 100644 javascript/ql/test/library-tests/frameworks/UrlRequests/tst.js

diff --git a/javascript/ql/src/javascript.qll b/javascript/ql/src/javascript.qll
index 436520d3fcf1..de99ecb0678b 100644
--- a/javascript/ql/src/javascript.qll
+++ b/javascript/ql/src/javascript.qll
@@ -71,6 +71,7 @@ import semmle.javascript.frameworks.Request
 import semmle.javascript.frameworks.SQL
 import semmle.javascript.frameworks.StringFormatters
 import semmle.javascript.frameworks.UriLibraries
+import semmle.javascript.frameworks.UrlRequests
 import semmle.javascript.frameworks.XmlParsers
 import semmle.javascript.frameworks.xUnit
 import semmle.javascript.linters.ESLint
diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
new file mode 100644
index 000000000000..e23d19fb287f
--- /dev/null
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -0,0 +1,127 @@
+/**
+ * Provides classes for modelling URL requests.
+ *
+ * Subclass `UrlRequest` to refine the behavior of the analysis on existing URL requests.
+ * Subclass `CustomUrlRequest` to introduce new kinds of URL requests.
+ */
+
+import javascript
+
+/**
+ * A call that performs a request to a URL.
+ */
+class CustomUrlRequest extends DataFlow::CallNode {
+
+  /**
+   * Gets the URL of the request.
+   */
+  abstract DataFlow::Node getUrl();
+}
+
+/**
+ * A call that performs a request to a URL.
+ */
+class UrlRequest extends DataFlow::CallNode {
+
+  CustomUrlRequest custom;
+
+  UrlRequest() {
+    this = custom
+  }
+
+  /**
+   * Gets the URL of the request.
+   */
+  DataFlow::Node getUrl() {
+    result = custom.getUrl()
+  }
+}
+
+/**
+ * A simple model of common URL request libraries.
+ */
+private class DefaultUrlRequest extends CustomUrlRequest {
+
+  DataFlow::Node url;
+
+  DefaultUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee, string httpMethodName, string urlName |
+      httpMethodName = any(HTTP::RequestMethodName m).toLowerCase() and
+      (urlName = "url" or urlName = "uri") and // slightly over-approximate, in the name of simplicity
+      this = callee.getACall() |
+      (
+        (
+          moduleName = "request" or
+          moduleName = "request-promise" or
+          moduleName = "request-promise-any" or
+          moduleName = "request-promise-native"
+        ) and
+        (
+          callee = DataFlow::moduleImport(moduleName) or
+          callee = DataFlow::moduleMember(moduleName, httpMethodName)
+        ) and
+        (
+          url = getArgument(0) or
+          url = getOptionArgument(0, urlName)
+        )
+      )
+      or
+      (
+        moduleName = "superagent" and
+        callee = DataFlow::moduleMember(moduleName, httpMethodName) and
+        url = getArgument(0)
+      )
+      or
+      (
+        (moduleName = "http" or moduleName = "https") and
+        callee = DataFlow::moduleMember(moduleName, httpMethodName) and
+        url = getArgument(0)
+      )
+      or
+      (
+        moduleName = "axios" and
+        (
+          callee = DataFlow::moduleImport(moduleName) or
+          callee = DataFlow::moduleMember(moduleName, httpMethodName) or
+          callee = DataFlow::moduleMember(moduleName, "request")
+        ) and
+        (
+          url = getArgument(0) or
+          url = getOptionArgument([0..2], urlName) // slightly over-approximate, in the name of simplicity
+        )
+      )
+      or
+      (
+        moduleName = "got" and
+        (
+          callee = DataFlow::moduleImport(moduleName) or
+          callee = DataFlow::moduleMember(moduleName, "stream")
+        ) and
+        (
+          url = getArgument(0) and not exists (getOptionArgument(1, "baseUrl")) 
+        )
+      )
+      or
+      (
+        (
+          moduleName = "node-fetch" or
+          moduleName = "cross-fetch" or
+          moduleName = "isomorphic-fetch"
+        ) and
+        callee = DataFlow::moduleImport(moduleName) and
+        url = getArgument(0)
+      )
+    )
+    or
+    (
+      this = DataFlow::globalVarRef("fetch").getACall() and
+      url = getArgument(0)
+    )
+
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}
\ No newline at end of file
diff --git a/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.expected b/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.expected
new file mode 100644
index 000000000000..2b14dea32748
--- /dev/null
+++ b/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.expected
@@ -0,0 +1,16 @@
+| tst.js:11:5:11:16 | request(url) | tst.js:11:13:11:15 | url |
+| tst.js:13:5:13:20 | request.get(url) | tst.js:13:17:13:19 | url |
+| tst.js:15:5:15:23 | request.delete(url) | tst.js:15:20:15:22 | url |
+| tst.js:17:5:17:25 | request ...  url }) | tst.js:17:13:17:24 | { url: url } |
+| tst.js:17:5:17:25 | request ...  url }) | tst.js:17:20:17:22 | url |
+| tst.js:19:5:19:23 | requestPromise(url) | tst.js:19:20:19:22 | url |
+| tst.js:21:5:21:23 | superagent.get(url) | tst.js:21:20:21:22 | url |
+| tst.js:23:5:23:17 | http.get(url) | tst.js:23:14:23:16 | url |
+| tst.js:25:5:25:14 | axios(url) | tst.js:25:11:25:13 | url |
+| tst.js:27:5:27:18 | axios.get(url) | tst.js:27:15:27:17 | url |
+| tst.js:29:5:29:23 | axios({ url: url }) | tst.js:29:11:29:22 | { url: url } |
+| tst.js:29:5:29:23 | axios({ url: url }) | tst.js:29:18:29:20 | url |
+| tst.js:31:5:31:12 | got(url) | tst.js:31:9:31:11 | url |
+| tst.js:33:5:33:19 | got.stream(url) | tst.js:33:16:33:18 | url |
+| tst.js:35:5:35:21 | window.fetch(url) | tst.js:35:18:35:20 | url |
+| tst.js:37:5:37:18 | nodeFetch(url) | tst.js:37:15:37:17 | url |
diff --git a/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql b/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql
new file mode 100644
index 000000000000..a1a4de127136
--- /dev/null
+++ b/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql
@@ -0,0 +1,4 @@
+import javascript
+
+from UrlRequest r
+select r, r.getUrl()
\ No newline at end of file
diff --git a/javascript/ql/test/library-tests/frameworks/UrlRequests/tst.js b/javascript/ql/test/library-tests/frameworks/UrlRequests/tst.js
new file mode 100644
index 000000000000..3656e30d9660
--- /dev/null
+++ b/javascript/ql/test/library-tests/frameworks/UrlRequests/tst.js
@@ -0,0 +1,39 @@
+import request from 'request';
+import requestPromise from 'request-promise';
+import superagent from 'superagent';
+import http from 'http';
+import express from 'express';
+import axios from 'axios';
+import got from 'got';
+import nodeFetch from 'node-fetch';
+
+(function() {
+    request(url);
+
+    request.get(url);
+
+    request.delete(url);
+
+    request({ url: url });
+
+    requestPromise(url);
+
+    superagent.get(url);
+
+    http.get(url);
+
+    axios(url);
+
+    axios.get(url);
+
+    axios({ url: url });
+
+    got(url);
+
+    got.stream(url);
+
+    window.fetch(url);
+
+    nodeFetch(url);
+
+});

From f5a6af54e67f277aa56e055fb3155e47c7b4a733 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Thu, 30 Aug 2018 10:19:18 +0200
Subject: [PATCH 02/28] JS: add security query: js/request-forgery

---
 javascript/config/suites/javascript/security  |  1 +
 .../src/Security/CWE-918/RequestForgery.qhelp | 79 +++++++++++++++++
 .../ql/src/Security/CWE-918/RequestForgery.ql | 18 ++++
 .../CWE-918/examples/RequestForgeryBad.js     | 12 +++
 .../CWE-918/examples/RequestForgeryGood.js    | 19 ++++
 .../security/dataflow/RequestForgery.qll      | 87 +++++++++++++++++++
 .../Security/CWE-918/RequestForgery.expected  |  6 ++
 .../Security/CWE-918/RequestForgery.qlref     |  1 +
 .../test/query-tests/Security/CWE-918/tst.js  | 31 +++++++
 9 files changed, 254 insertions(+)
 create mode 100644 javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
 create mode 100644 javascript/ql/src/Security/CWE-918/RequestForgery.ql
 create mode 100644 javascript/ql/src/Security/CWE-918/examples/RequestForgeryBad.js
 create mode 100644 javascript/ql/src/Security/CWE-918/examples/RequestForgeryGood.js
 create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.qlref
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-918/tst.js

diff --git a/javascript/config/suites/javascript/security b/javascript/config/suites/javascript/security
index 48878be87ce1..26debb63a24e 100644
--- a/javascript/config/suites/javascript/security
+++ b/javascript/config/suites/javascript/security
@@ -29,3 +29,4 @@
 + semmlecode-javascript-queries/Security/CWE-807/DifferentKindsComparisonBypass.ql: /Security/CWE/CWE-807
 + semmlecode-javascript-queries/Security/CWE-843/TypeConfusionThroughParameterTampering.ql: /Security/CWE/CWE-834
 + semmlecode-javascript-queries/Security/CWE-916/InsufficientPasswordHash.ql: /Security/CWE/CWE-916
++ semmlecode-javascript-queries/Security/CWE-918/RequestForgery.ql: /Security/CWE/CWE-918
diff --git a/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp b/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
new file mode 100644
index 000000000000..75f0b681d168
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
@@ -0,0 +1,79 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+	<overview>
+		<p>
+
+			Directly incorporating user input into a remote request
+			without validating the input can facilitate different kinds of request
+			forgery attacks, where the attacker essentially controls the request.
+
+			If the vulnerable request is in server-side code, then security
+			mechanisms, such as external firewalls, can be bypassed.
+
+			If the vulnerable request is in client-side code, then unsuspecting
+			users can send malicious requests to other servers, potentially
+			resulting in a DDOS attack.
+
+		</p>
+	</overview>
+
+	<recommendation>
+
+		<p>
+
+			To guard against request forgery, it is advisable to avoid
+			putting user input directly into a remote request. If a flexible
+			remote request mechanism is required, it is recommended to maintain a
+			list of authorized request targets and choose from that list based on
+			the user input provided.
+
+		</p>
+
+	</recommendation>
+
+	<example>
+
+		<p>
+
+			The following example shows an HTTP request parameter
+			being used directly in a URL request without validating the input,
+			which facilitate an SSRF attack. The request
+			<code>http.get(...)</code> is vulnerable since an attacker can choose
+			the value of <code>target</code> to be anything he wants. For
+			instance, the attacker can choose
+			<code>"internal.example.com/#"</code> as the target, causing the URL
+			used in the request to be
+			<code>"https://internal.example.com/#.example.com/data"</code>.
+
+		</p>
+
+		<p>
+
+			A request to <code>https://internal.example.com</code> may
+			be problematic if that server is not meant to be
+			directly accessible from the attacker's machine.
+
+		</p>
+
+		<sample src="examples/RequestForgeryBad.js"/>
+
+		<p>
+
+			One way to remedy the problem is to use the user input to
+			select a known fixed string before performing the request:
+
+		</p>
+
+        <sample src="examples/RequestForgeryGood.js"/>
+
+	</example>
+
+	<references>
+
+		<li>OWASP: <a href="https://www.owasp.org/index.php/Server_Side_Request_Forgery">SSRF</a></li>
+
+	</references>
+</qhelp>
diff --git a/javascript/ql/src/Security/CWE-918/RequestForgery.ql b/javascript/ql/src/Security/CWE-918/RequestForgery.ql
new file mode 100644
index 000000000000..a76307c439aa
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-918/RequestForgery.ql
@@ -0,0 +1,18 @@
+/**
+ * @name Uncontrolled data used in remote request
+ * @description Sending remote requests with user-controlled data allows for request forgery attacks.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/request-forgery
+ * @tags security
+ *       external/cwe/cwe-918
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.RequestForgery::RequestForgery
+
+from Configuration cfg, DataFlow::Node source, Sink sink, DataFlow::Node request
+where cfg.hasFlow(source, sink) and
+      request = sink.getARequest()
+select request, "The $@ of this request depends on $@.", sink, sink.getKind(), source, "a user-provided value"
diff --git a/javascript/ql/src/Security/CWE-918/examples/RequestForgeryBad.js b/javascript/ql/src/Security/CWE-918/examples/RequestForgeryBad.js
new file mode 100644
index 000000000000..d0eb8b56b0fc
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-918/examples/RequestForgeryBad.js
@@ -0,0 +1,12 @@
+import http from 'http';
+import url from 'url';
+
+var server = http.createServer(function(req, res) {
+    var target = url.parse(request.url, true).query.target;
+
+    // BAD: `target` is controlled by the attacker
+    http.get('https://' + target + ".example.com/data/", res => {
+        // process request response ...
+    });
+
+});
diff --git a/javascript/ql/src/Security/CWE-918/examples/RequestForgeryGood.js b/javascript/ql/src/Security/CWE-918/examples/RequestForgeryGood.js
new file mode 100644
index 000000000000..2c1662a5a1e7
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-918/examples/RequestForgeryGood.js
@@ -0,0 +1,19 @@
+import http from 'http';
+import url from 'url';
+
+var server = http.createServer(function(req, res) {
+    var target = url.parse(request.url, true).query.target;
+
+    var subdomain;
+    if (target === 'EU') {
+        subdomain = "europe"
+    } else {
+        subdomain = "world"
+    }
+
+    // GOOD: `subdomain` is controlled by the server
+    http.get('https://' + subdomain + ".example.com/data/", res => {
+        // process request response ...
+    });
+
+});
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
new file mode 100644
index 000000000000..15c239d23765
--- /dev/null
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
@@ -0,0 +1,87 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about request forgery.
+ */
+
+import semmle.javascript.security.dataflow.RemoteFlowSources
+import UrlConcatenation
+
+module RequestForgery {
+
+  /**
+   * A data flow source for request forgery.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for request forgery.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets a request that uses this sink.
+     */
+    abstract DataFlow::Node getARequest();
+
+    /**
+     * Gets the kind of this sink.
+     */
+    abstract string getKind();
+
+  }
+
+  /**
+   * A sanitizer for request forgery.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A taint tracking configuration for request forgery.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() {
+      this = "RequestForgery"
+    }
+
+    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) or
+      node instanceof Sanitizer
+    }
+
+    override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) {
+      sanitizingPrefixEdge(source, sink)
+    }
+
+  }
+
+  /** A source of remote user input, considered as a flow source for request forgery. */
+  private class RemoteFlowSourceAsSource extends Source {
+    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * The URL of a URL request, viewed as a sink for request forgery.
+   */
+  private class UrlRequestUrlAsSink extends Sink {
+
+    UrlRequest request;
+
+    UrlRequestUrlAsSink() {
+      this = request.getUrl()
+    }
+
+    override DataFlow::Node getARequest() {
+      result = request
+    }
+
+    override string getKind() {
+      result = "URL"
+    }
+  }
+}
diff --git a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
new file mode 100644
index 000000000000..d114b6a8e389
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
@@ -0,0 +1,6 @@
+| tst.js:16:5:16:20 | request(tainted) | The $@ of this request depends on $@. | tst.js:16:13:16:19 | tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
+| tst.js:18:5:18:24 | request.get(tainted) | The $@ of this request depends on $@. | tst.js:18:17:18:23 | tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
+| tst.js:22:5:22:20 | request(options) | The $@ of this request depends on $@. | tst.js:21:19:21:25 | tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
+| tst.js:24:5:24:32 | request ... ainted) | The $@ of this request depends on $@. | tst.js:24:13:24:31 | "http://" + tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
+| tst.js:26:5:26:43 | request ... ainted) | The $@ of this request depends on $@. | tst.js:26:13:26:42 | "http:/ ... tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
+| tst.js:28:5:28:44 | request ... ainted) | The $@ of this request depends on $@. | tst.js:28:13:28:43 | "http:/ ... tainted | URL | tst.js:12:29:12:35 | req.url | a user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.qlref b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.qlref
new file mode 100644
index 000000000000..fcb4e41daf88
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.qlref
@@ -0,0 +1 @@
+Security/CWE-918/RequestForgery.ql
diff --git a/javascript/ql/test/query-tests/Security/CWE-918/tst.js b/javascript/ql/test/query-tests/Security/CWE-918/tst.js
new file mode 100644
index 000000000000..b502e56bd12f
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-918/tst.js
@@ -0,0 +1,31 @@
+import request from 'request';
+import requestPromise from 'request-promise';
+import superagent from 'superagent';
+import http from 'http';
+import express from 'express';
+import axios from 'axios';
+import got from 'got';
+import nodeFetch from 'node-fetch';
+import url from 'url';
+
+var server = http.createServer(function(req, res) {
+    var tainted = url.parse(req.url, true).query.url;
+
+    request("example.com"); // OK
+
+    request(tainted); // NOT OK
+
+    request.get(tainted); // NOT OK
+
+    var options = {};
+    options.url = tainted;
+    request(options); // NOT OK
+
+    request("http://" + tainted); // NOT OK
+
+    request("http://example.com" + tainted); // NOT OK
+
+    request("http://example.com/" + tainted); // NOT OK
+
+    request("http://example.com/?" + tainted); // OK
+})

From 68b7a8b57efc769766997468ef2ded9c0e5c88a0 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Thu, 30 Aug 2018 14:26:06 +0200
Subject: [PATCH 03/28] JS: change notes for `UrlRequest` libraries and
 `js/request-forgery`

---
 change-notes/1.18/analysis-javascript.md | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md
index 733e3b25bade..a7c4cc4b944e 100644
--- a/change-notes/1.18/analysis-javascript.md
+++ b/change-notes/1.18/analysis-javascript.md
@@ -19,11 +19,13 @@
 * Type inference for simple function calls has been improved. This may give additional results for queries that rely on type inference.
 
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following libraries:
+  - [axios](https://github.com/axios/axios)
   - [bluebird](https://bluebirdjs.com)
   - [browserid-crypto](https://github.com/mozilla/browserid-crypto)
   - [compose-function](https://github.com/stoeffel/compose-function)
   - [cookie-parser](https://github.com/expressjs/cookie-parser)
   - [cookie-session](https://github.com/expressjs/cookie-session)
+  - [cross-fetch](https://github.com/lquixada/cross-fetch)
   - [crypto-js](https://github.com/https://github.com/brix/crypto-js)
   - [deep-assign](https://github.com/sindresorhus/deep-assign)
   - [deep-extend](https://github.com/unclechu/node-deep-extend)
@@ -45,9 +47,11 @@
   - [fast-json-parse](https://github.com/mcollina/fast-json-parse)
   - [forge](https://github.com/digitalbazaar/forge)
   - [format-util](https://github.com/tmpfs/format-util)
+  - [got](https://github.com/sindresorhus/got)
   - [global](https://github.com/Raynos/global)
   - [he](https://github.com/mathiasbynens/he)
   - [html-entities](https://github.com/mdevils/node-html-entities)
+  - [isomorphic-fetch](https://github.com/matthew-andrews/isomorphic-fetch)
   - [jquery](https://jquery.com)
   - [js-extend](https://github.com/vmattos/js-extend)
   - [json-parse-better-errors](https://github.com/zkat/json-parse-better-errors)
@@ -63,6 +67,7 @@
   - [mixin-object](https://github.com/jonschlinkert/mixin-object)
   - [MySQL2](https://github.com/sidorares/node-mysql2)
   - [node.extend](https://github.com/dreamerslab/node.extend)
+  - [node-fetch](https://github.com/bitinn/node-fetch)
   - [object-assign](https://github.com/sindresorhus/object-assign)
   - [object.assign](https://github.com/ljharb/object.assign)
   - [object.defaults](https://github.com/jonschlinkert/object.defaults)
@@ -71,6 +76,10 @@
   - [printj](https://github.com/SheetJS/printj)
   - [q](https://documentup.com/kriskowal/q/)
   - [ramda](https://ramdajs.com)
+  - [request](https://github.com/request/request)
+  - [request-promise](https://github.com/request/request-promise)
+  - [request-promise-any](https://github.com/request/request-promise-any)
+  - [request-promise-native](https://github.com/request/request-promise-native)
   - [React Native](https://facebook.github.io/react-native/)
   - [safe-json-parse](https://github.com/Raynos/safe-json-parse)
   - [sanitize](https://github.com/pocketly/node-sanitize)
@@ -78,6 +87,7 @@
   - [smart-extend](https://github.com/danielkalen/smart-extend)
   - [sprintf.js](https://github.com/alexei/sprintf.js)
   - [string-template](https://github.com/Matt-Esch/string-template)
+  - [superagent](https://github.com/visionmedia/superagent)
   - [underscore](https://underscorejs.org)
   - [util-extend](https://github.com/isaacs/util-extend)
   - [utils-merge](https://github.com/jaredhanson/utils-merge)
@@ -92,6 +102,7 @@
 | Clear-text logging of sensitive information (`js/clear-text-logging`) | security, external/cwe/cwe-312, external/cwe/cwe-315, external/cwe/cwe-359 | Highlights logging of sensitive information, indicating a violation of [CWE-312](https://cwe.mitre.org/data/definitions/312.html). Results shown on LGTM by default. |
 | Disabling Electron webSecurity (`js/disabling-electron-websecurity`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `webSecurity` property set to false. Results shown on LGTM by default. |
 | Enabling Electron allowRunningInsecureContent (`js/enabling-electron-insecure-content`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `allowRunningInsecureContent` property set to true. Results shown on LGTM by default. |
+| Uncontrolled data used in remote request (`js/request-forgery`) | security, external/cwe/cwe-918 | Highlights remote requests that are built from unsanitized user input, indicating a violation of [CWE-918](https://cwe.mitre.org/data/definitions/918.html). Results shown on LGTM by default. |
 | Use of externally-controlled format string (`js/tainted-format-string`) | security, external/cwe/cwe-134 | Highlights format strings containing user-provided data, indicating a violation of [CWE-134](https://cwe.mitre.org/data/definitions/134.html). Results shown on LGTM by default. |
 
 ## Changes to existing queries

From 80b81b07c5ac09ce8f54c2a48c9e77970e1f930e Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 12:56:28 +0200
Subject: [PATCH 04/28] JS: refactor DefaultUrlRequest: extract names

---
 .../javascript/frameworks/UrlRequests.qll     | 31 +++++++++++++------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index e23d19fb287f..ab4d2335690a 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -37,6 +37,21 @@ class UrlRequest extends DataFlow::CallNode {
   }
 }
 
+/**
+ * Gets name of an HTTP request method, in all-lowercase.
+ */
+private string httpMethodName() {
+  result = any(HTTP::RequestMethodName m).toLowerCase()
+}
+
+/**
+ * Gets the name of a property that likely contains a  URL value.
+ */
+private string urlPropertyName() {
+  result = "uri" or
+  result = "url"
+}
+
 /**
  * A simple model of common URL request libraries.
  */
@@ -45,9 +60,7 @@ private class DefaultUrlRequest extends CustomUrlRequest {
   DataFlow::Node url;
 
   DefaultUrlRequest() {
-    exists (string moduleName, DataFlow::SourceNode callee, string httpMethodName, string urlName |
-      httpMethodName = any(HTTP::RequestMethodName m).toLowerCase() and
-      (urlName = "url" or urlName = "uri") and // slightly over-approximate, in the name of simplicity
+    exists (string moduleName, DataFlow::SourceNode callee |
       this = callee.getACall() |
       (
         (
@@ -58,23 +71,23 @@ private class DefaultUrlRequest extends CustomUrlRequest {
         ) and
         (
           callee = DataFlow::moduleImport(moduleName) or
-          callee = DataFlow::moduleMember(moduleName, httpMethodName)
+          callee = DataFlow::moduleMember(moduleName, httpMethodName())
         ) and
         (
           url = getArgument(0) or
-          url = getOptionArgument(0, urlName)
+          url = getOptionArgument(0, urlPropertyName())
         )
       )
       or
       (
         moduleName = "superagent" and
-        callee = DataFlow::moduleMember(moduleName, httpMethodName) and
+        callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
         url = getArgument(0)
       )
       or
       (
         (moduleName = "http" or moduleName = "https") and
-        callee = DataFlow::moduleMember(moduleName, httpMethodName) and
+        callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
         url = getArgument(0)
       )
       or
@@ -82,12 +95,12 @@ private class DefaultUrlRequest extends CustomUrlRequest {
         moduleName = "axios" and
         (
           callee = DataFlow::moduleImport(moduleName) or
-          callee = DataFlow::moduleMember(moduleName, httpMethodName) or
+          callee = DataFlow::moduleMember(moduleName, httpMethodName()) or
           callee = DataFlow::moduleMember(moduleName, "request")
         ) and
         (
           url = getArgument(0) or
-          url = getOptionArgument([0..2], urlName) // slightly over-approximate, in the name of simplicity
+          url = getOptionArgument([0..2], urlPropertyName()) // slightly over-approximate, in the name of simplicity
         )
       )
       or

From d7a81ef8ef1d70012ae50c0908784e14ae654be5 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 12:58:07 +0200
Subject: [PATCH 05/28] JS: refactor DefaultUrlRequest: extract the `request`
 library

---
 .../javascript/frameworks/UrlRequests.qll     | 51 ++++++++++++-------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index ab4d2335690a..96dc0e3af4b7 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -62,23 +62,6 @@ private class DefaultUrlRequest extends CustomUrlRequest {
   DefaultUrlRequest() {
     exists (string moduleName, DataFlow::SourceNode callee |
       this = callee.getACall() |
-      (
-        (
-          moduleName = "request" or
-          moduleName = "request-promise" or
-          moduleName = "request-promise-any" or
-          moduleName = "request-promise-native"
-        ) and
-        (
-          callee = DataFlow::moduleImport(moduleName) or
-          callee = DataFlow::moduleMember(moduleName, httpMethodName())
-        ) and
-        (
-          url = getArgument(0) or
-          url = getOptionArgument(0, urlPropertyName())
-        )
-      )
-      or
       (
         moduleName = "superagent" and
         callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
@@ -137,4 +120,38 @@ private class DefaultUrlRequest extends CustomUrlRequest {
     result = url
   }
 
+}
+
+
+/**
+ * A model of a URL request in the `request` library.
+ */
+private class RequestUrlRequest extends CustomUrlRequest {
+
+  DataFlow::Node url;
+
+  RequestUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      (
+        moduleName = "request" or
+        moduleName = "request-promise" or
+        moduleName = "request-promise-any" or
+        moduleName = "request-promise-native"
+      ) and
+      (
+        callee = DataFlow::moduleImport(moduleName) or
+        callee = DataFlow::moduleMember(moduleName, httpMethodName())
+      ) and
+      (
+        url = getArgument(0) or
+        url = getOptionArgument(0, urlPropertyName())
+      )
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
 }
\ No newline at end of file

From b3b997ca9107472b67b3c546d7061cc85aa75c57 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 12:59:58 +0200
Subject: [PATCH 06/28] JS: refactor DefaultUrlRequest: extract the `axios`
 library

---
 .../javascript/frameworks/UrlRequests.qll     | 44 +++++++++++++------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index 96dc0e3af4b7..bc7a49c15dc4 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -74,19 +74,6 @@ private class DefaultUrlRequest extends CustomUrlRequest {
         url = getArgument(0)
       )
       or
-      (
-        moduleName = "axios" and
-        (
-          callee = DataFlow::moduleImport(moduleName) or
-          callee = DataFlow::moduleMember(moduleName, httpMethodName()) or
-          callee = DataFlow::moduleMember(moduleName, "request")
-        ) and
-        (
-          url = getArgument(0) or
-          url = getOptionArgument([0..2], urlPropertyName()) // slightly over-approximate, in the name of simplicity
-        )
-      )
-      or
       (
         moduleName = "got" and
         (
@@ -154,4 +141,33 @@ private class RequestUrlRequest extends CustomUrlRequest {
     result = url
   }
 
-}
\ No newline at end of file
+}
+
+/**
+ * A model of a URL request in the `axios` library.
+ */
+private class AxiosUrlRequest extends CustomUrlRequest {
+
+  DataFlow::Node url;
+
+  AxiosUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      moduleName = "axios" and
+      (
+        callee = DataFlow::moduleImport(moduleName) or
+        callee = DataFlow::moduleMember(moduleName, httpMethodName()) or
+        callee = DataFlow::moduleMember(moduleName, "request")
+      ) and
+      (
+        url = getArgument(0) or
+        url = getOptionArgument([0..2], urlPropertyName()) // slightly over-approximate, in the name of simplicity
+      )
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}

From 5f26c23582c84f73cdf882514ce318a51e0ee562 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 13:02:48 +0200
Subject: [PATCH 07/28] JS: refactor DefaultUrlRequest: extract the `fetch` API

---
 .../javascript/frameworks/UrlRequests.qll     | 47 ++++++++++++-------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index bc7a49c15dc4..437266b808cd 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -84,23 +84,7 @@ private class DefaultUrlRequest extends CustomUrlRequest {
           url = getArgument(0) and not exists (getOptionArgument(1, "baseUrl")) 
         )
       )
-      or
-      (
-        (
-          moduleName = "node-fetch" or
-          moduleName = "cross-fetch" or
-          moduleName = "isomorphic-fetch"
-        ) and
-        callee = DataFlow::moduleImport(moduleName) and
-        url = getArgument(0)
-      )
-    )
-    or
-    (
-      this = DataFlow::globalVarRef("fetch").getACall() and
-      url = getArgument(0)
     )
-
   }
 
   override DataFlow::Node getUrl() {
@@ -171,3 +155,34 @@ private class AxiosUrlRequest extends CustomUrlRequest {
   }
 
 }
+
+/**
+ * A model of a URL request in an implementation of the `fetch` API.
+ */
+private class FetchUrlRequest extends CustomUrlRequest {
+
+  DataFlow::Node url;
+
+  FetchUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      (
+        moduleName = "node-fetch" or
+        moduleName = "cross-fetch" or
+        moduleName = "isomorphic-fetch"
+      ) and
+      callee = DataFlow::moduleImport(moduleName) and
+      url = getArgument(0)
+    )
+    or
+    (
+      this = DataFlow::globalVarRef("fetch").getACall() and
+      url = getArgument(0)
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}

From 1abdf2ffd578ef4e2f94d7d19c0812b10f4f9a45 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 13:07:02 +0200
Subject: [PATCH 08/28] JS: refactor DefaultUrlRequest: extract the `http`
 library

---
 .../javascript/frameworks/UrlRequests.qll     | 29 +++++++++++++++----
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index 437266b808cd..7de67faf1616 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -68,12 +68,6 @@ private class DefaultUrlRequest extends CustomUrlRequest {
         url = getArgument(0)
       )
       or
-      (
-        (moduleName = "http" or moduleName = "https") and
-        callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
-        url = getArgument(0)
-      )
-      or
       (
         moduleName = "got" and
         (
@@ -186,3 +180,26 @@ private class FetchUrlRequest extends CustomUrlRequest {
   }
 
 }
+
+
+/**
+ * A model of a URL request in the Node.js `http` library.
+ */
+private class NodeHttpUrlRequest extends CustomUrlRequest {
+
+  DataFlow::Node url;
+
+  NodeHttpUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      (moduleName = "http" or moduleName = "https") and
+      callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
+      url = getArgument(0)
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}

From de6b83548ae182187d971ee7cbc31045ea6c41bb Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 13:08:31 +0200
Subject: [PATCH 09/28] JS: refactor DefaultUrlRequest: extract the `got`
 library

---
 .../javascript/frameworks/UrlRequests.qll     | 37 +++++++++++++------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index 7de67faf1616..0f7bfafd1820 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -67,17 +67,6 @@ private class DefaultUrlRequest extends CustomUrlRequest {
         callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
         url = getArgument(0)
       )
-      or
-      (
-        moduleName = "got" and
-        (
-          callee = DataFlow::moduleImport(moduleName) or
-          callee = DataFlow::moduleMember(moduleName, "stream")
-        ) and
-        (
-          url = getArgument(0) and not exists (getOptionArgument(1, "baseUrl")) 
-        )
-      )
     )
   }
 
@@ -203,3 +192,29 @@ private class NodeHttpUrlRequest extends CustomUrlRequest {
   }
 
 }
+
+
+/**
+ * A model of a URL request in the `got` library.
+ */
+private class GotUrlRequest extends CustomUrlRequest {
+
+  DataFlow::Node url;
+
+  GotUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      moduleName = "got" and
+      (
+        callee = DataFlow::moduleImport(moduleName) or
+        callee = DataFlow::moduleMember(moduleName, "stream")
+      ) and
+      url = getArgument(0) and not exists (getOptionArgument(1, "baseUrl"))
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}

From 0a89f1a4202d182301b1ba0b14f6cf45f184f907 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 13:09:57 +0200
Subject: [PATCH 10/28] JS: eliminate DefaultUrlRequest: extract the `got`
 library

---
 .../javascript/frameworks/UrlRequests.qll     | 47 +++++++++----------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index 0f7bfafd1820..dad03577acd6 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -52,31 +52,6 @@ private string urlPropertyName() {
   result = "url"
 }
 
-/**
- * A simple model of common URL request libraries.
- */
-private class DefaultUrlRequest extends CustomUrlRequest {
-
-  DataFlow::Node url;
-
-  DefaultUrlRequest() {
-    exists (string moduleName, DataFlow::SourceNode callee |
-      this = callee.getACall() |
-      (
-        moduleName = "superagent" and
-        callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
-        url = getArgument(0)
-      )
-    )
-  }
-
-  override DataFlow::Node getUrl() {
-    result = url
-  }
-
-}
-
-
 /**
  * A model of a URL request in the `request` library.
  */
@@ -218,3 +193,25 @@ private class GotUrlRequest extends CustomUrlRequest {
   }
 
 }
+
+/**
+ * A model of a URL request in the `superagent` library.
+ */
+private class SuperAgentUrlRequest extends CustomUrlRequest {
+
+  DataFlow::Node url;
+
+  SuperAgentUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      moduleName = "superagent" and
+      callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
+      url = getArgument(0)
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}

From cb2a6ede590072951682396c50920a603c6102c5 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 13:15:53 +0200
Subject: [PATCH 11/28] JS: support http.request URL requests

---
 .../ql/src/semmle/javascript/frameworks/UrlRequests.qll     | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index dad03577acd6..9ae388cdb5bf 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -157,7 +157,11 @@ private class NodeHttpUrlRequest extends CustomUrlRequest {
     exists (string moduleName, DataFlow::SourceNode callee |
       this = callee.getACall() |
       (moduleName = "http" or moduleName = "https") and
-      callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
+      (
+        callee = DataFlow::moduleMember(moduleName, httpMethodName())
+        or
+        callee = DataFlow::moduleMember(moduleName, "request") 
+      ) and
       url = getArgument(0)
     )
   }

From 6d78350fee98b51c67b401a46c50356ad47a77b0 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 13:56:42 +0200
Subject: [PATCH 12/28] JS: s/URLRequest/ClientRequest, merge with
 NodeJSLib::ClientRequest

---
 .../semmle/javascript/frameworks/Electron.qll | 10 +--
 .../javascript/frameworks/NodeJSLib.qll       | 66 ++++++++-----------
 .../javascript/frameworks/UrlRequests.qll     | 52 ++++-----------
 .../security/dataflow/RequestForgery.qll      |  6 +-
 .../frameworks/Electron/ClientRequest.ql      |  2 +-
 .../frameworks/NodeJSLib/ClientRequest.ql     |  2 +-
 .../frameworks/UrlRequests/UrlRequest.ql      |  2 +-
 7 files changed, 51 insertions(+), 89 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
index a60309c9be10..a2b4e6745d6c 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
@@ -37,12 +37,12 @@ module Electron {
   /**
    * A Node.js-style HTTP or HTTPS request made using an Electron module.
    */
-  abstract class ClientRequest extends NodeJSLib::ClientRequest {}
+  abstract class ElectronClientRequest extends NodeJSLib::NodeJSClientRequest {}
   
   /**
    * A Node.js-style HTTP or HTTPS request made using `electron.net`, for example `net.request(url)`.
    */
-  private class NetRequest extends ClientRequest {
+  private class NetRequest extends ElectronClientRequest {
     NetRequest() {
       this = DataFlow::moduleMember("electron", "net").getAMemberCall("request")
     }
@@ -56,7 +56,7 @@ module Electron {
   /**
    * A Node.js-style HTTP or HTTPS request made using `electron.client`, for example `new client(url)`.
    */
-  private class NewClientRequest extends ClientRequest {
+  private class NewClientRequest extends ElectronClientRequest {
     NewClientRequest() {
       this = DataFlow::moduleMember("electron", "ClientRequest").getAnInstantiation()
     }
@@ -75,12 +75,12 @@ module Electron {
       exists(NodeJSLib::ClientRequestHandler handler |
         this = handler.getParameter(0) and
         handler.getAHandledEvent() = "redirect" and
-        handler.getClientRequest() instanceof ClientRequest
+        handler.getClientRequest() instanceof ElectronClientRequest
       )
     }
     
     override string getSourceType() {
-      result = "Electron ClientRequest redirect event"
+      result = "ElectronClientRequest redirect event"
     }
   }
 }
\ No newline at end of file
diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
index 6a11bef5edc5..6f806d723d70 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -504,7 +504,8 @@ module NodeJSLib {
   /**
    * A data flow node that is an HTTP or HTTPS client request made by a Node.js server, for example `http.request(url)`.
    */
-  abstract class ClientRequest extends DataFlow::DefaultSourceNode {
+  abstract class NodeJSClientRequest extends DataFlow::DefaultSourceNode {
+
     /**
      * Gets the options object or string URL used to make the request.
      */
@@ -512,40 +513,29 @@ module NodeJSLib {
   }
   
   /**
-   * A data flow node that is an HTTP or HTTPS client request made by a Node.js server, for example `http.request(url)`.
+   * A model of a URL request in the Node.js `http` library.
    */
-  private class HttpRequest extends ClientRequest {
-    HttpRequest() {
-      exists(string protocol |
+  private class NodeHttpUrlRequest extends CustomClientRequest, NodeJSClientRequest {
+
+    DataFlow::Node url;
+
+    NodeHttpUrlRequest() {
+      exists (string moduleName, DataFlow::SourceNode callee |
+        this = callee.getACall() |
+        (moduleName = "http" or moduleName = "https") and
         (
-          protocol = "http" or
-          protocol = "https"
-        )
-        and
-        this = DataFlow::moduleImport(protocol).getAMemberCall("request")
+          callee = DataFlow::moduleMember(moduleName, any(HTTP::RequestMethodName m).toLowerCase())
+          or
+          callee = DataFlow::moduleMember(moduleName, "request")
+        ) and
+        url = getArgument(0)
       )
     }
-    
-    override DataFlow::Node getOptions() {
-      result = this.(DataFlow::MethodCallNode).getArgument(0)
-    }
-  }
-  
-  /**
-   * A data flow node that is an HTTP or HTTPS client request made by a Node.js process, for example `https.get(url)`.
-   */
-  private class HttpGet extends ClientRequest {
-    HttpGet() {
-      exists(string protocol |
-        (
-          protocol = "http" or
-          protocol = "https"
-        )
-        and
-        this = DataFlow::moduleImport(protocol).getAMemberCall("get")
-      )
+
+    override DataFlow::Node getUrl() {
+      result = url
     }
-    
+
     override DataFlow::Node getOptions() {
       result = this.(DataFlow::MethodCallNode).getArgument(0)
     }
@@ -556,13 +546,13 @@ module NodeJSLib {
    */
   private class ClientRequestCallbackParam extends DataFlow::ParameterNode, RemoteFlowSource {
     ClientRequestCallbackParam() {
-      exists(ClientRequest req |
+      exists(NodeJSClientRequest req |
         this = req.(DataFlow::MethodCallNode).getCallback(1).getParameter(0)
       )
     }
     
     override string getSourceType() {
-      result = "ClientRequest callback parameter"
+      result = "NodeJSClientRequest callback parameter"
     }
   }
   
@@ -589,7 +579,7 @@ module NodeJSLib {
    */
   class ClientRequestHandler extends DataFlow::FunctionNode {
     string handledEvent;
-    ClientRequest clientRequest;
+    NodeJSClientRequest clientRequest;
     
     ClientRequestHandler() {
       exists(DataFlow::MethodCallNode mcn |
@@ -609,7 +599,7 @@ module NodeJSLib {
     /**
      * Gets a request this callback is registered for.
      */
-    ClientRequest getClientRequest() {
+    NodeJSClientRequest getClientRequest() {
       result = clientRequest
     }
   }
@@ -626,7 +616,7 @@ module NodeJSLib {
     }
     
     override string getSourceType() {
-      result = "ClientRequest response event"
+      result = "NodeJSClientRequest response event"
     }
   }
   
@@ -643,7 +633,7 @@ module NodeJSLib {
     }
     
     override string getSourceType() {
-      result = "ClientRequest data event"
+      result = "NodeJSClientRequest data event"
     }
   }
   
@@ -667,7 +657,7 @@ module NodeJSLib {
     }
     
     override string getSourceType() {
-      result = "ClientRequest login event"
+      result = "NodeJSClientRequest login event"
     }
   }
   
@@ -725,7 +715,7 @@ module NodeJSLib {
     }
     
     override string getSourceType() {
-      result = "ClientRequest error event"
+      result = "NodeJSClientRequest error event"
     }
   }
 }
diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
index 9ae388cdb5bf..19bb80d4fd2e 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
@@ -1,8 +1,8 @@
 /**
- * Provides classes for modelling URL requests.
+ * Provides classes for modelling the client-side of a URL request.
  *
- * Subclass `UrlRequest` to refine the behavior of the analysis on existing URL requests.
- * Subclass `CustomUrlRequest` to introduce new kinds of URL requests.
+ * Subclass `ClientRequest` to refine the behavior of the analysis on existing client requests.
+ * Subclass `CustomClientRequest` to introduce new kinds of client requests.
  */
 
 import javascript
@@ -10,7 +10,7 @@ import javascript
 /**
  * A call that performs a request to a URL.
  */
-class CustomUrlRequest extends DataFlow::CallNode {
+class CustomClientRequest extends DataFlow::CallNode {
 
   /**
    * Gets the URL of the request.
@@ -21,11 +21,11 @@ class CustomUrlRequest extends DataFlow::CallNode {
 /**
  * A call that performs a request to a URL.
  */
-class UrlRequest extends DataFlow::CallNode {
+class ClientRequest extends DataFlow::CallNode {
 
-  CustomUrlRequest custom;
+  CustomClientRequest custom;
 
-  UrlRequest() {
+  ClientRequest() {
     this = custom
   }
 
@@ -55,7 +55,7 @@ private string urlPropertyName() {
 /**
  * A model of a URL request in the `request` library.
  */
-private class RequestUrlRequest extends CustomUrlRequest {
+private class RequestUrlRequest extends CustomClientRequest {
 
   DataFlow::Node url;
 
@@ -88,7 +88,7 @@ private class RequestUrlRequest extends CustomUrlRequest {
 /**
  * A model of a URL request in the `axios` library.
  */
-private class AxiosUrlRequest extends CustomUrlRequest {
+private class AxiosUrlRequest extends CustomClientRequest {
 
   DataFlow::Node url;
 
@@ -117,7 +117,7 @@ private class AxiosUrlRequest extends CustomUrlRequest {
 /**
  * A model of a URL request in an implementation of the `fetch` API.
  */
-private class FetchUrlRequest extends CustomUrlRequest {
+private class FetchUrlRequest extends CustomClientRequest {
 
   DataFlow::Node url;
 
@@ -145,38 +145,10 @@ private class FetchUrlRequest extends CustomUrlRequest {
 
 }
 
-
-/**
- * A model of a URL request in the Node.js `http` library.
- */
-private class NodeHttpUrlRequest extends CustomUrlRequest {
-
-  DataFlow::Node url;
-
-  NodeHttpUrlRequest() {
-    exists (string moduleName, DataFlow::SourceNode callee |
-      this = callee.getACall() |
-      (moduleName = "http" or moduleName = "https") and
-      (
-        callee = DataFlow::moduleMember(moduleName, httpMethodName())
-        or
-        callee = DataFlow::moduleMember(moduleName, "request") 
-      ) and
-      url = getArgument(0)
-    )
-  }
-
-  override DataFlow::Node getUrl() {
-    result = url
-  }
-
-}
-
-
 /**
  * A model of a URL request in the `got` library.
  */
-private class GotUrlRequest extends CustomUrlRequest {
+private class GotUrlRequest extends CustomClientRequest {
 
   DataFlow::Node url;
 
@@ -201,7 +173,7 @@ private class GotUrlRequest extends CustomUrlRequest {
 /**
  * A model of a URL request in the `superagent` library.
  */
-private class SuperAgentUrlRequest extends CustomUrlRequest {
+private class SuperAgentUrlRequest extends CustomClientRequest {
 
   DataFlow::Node url;
 
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
index 15c239d23765..6881382672ab 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
@@ -68,11 +68,11 @@ module RequestForgery {
   /**
    * The URL of a URL request, viewed as a sink for request forgery.
    */
-  private class UrlRequestUrlAsSink extends Sink {
+  private class ClientRequestUrlAsSink extends Sink {
 
-    UrlRequest request;
+    ClientRequest request;
 
-    UrlRequestUrlAsSink() {
+    ClientRequestUrlAsSink() {
       this = request.getUrl()
     }
 
diff --git a/javascript/ql/test/library-tests/frameworks/Electron/ClientRequest.ql b/javascript/ql/test/library-tests/frameworks/Electron/ClientRequest.ql
index 157b158d45dc..ef6eac9f2d4e 100644
--- a/javascript/ql/test/library-tests/frameworks/Electron/ClientRequest.ql
+++ b/javascript/ql/test/library-tests/frameworks/Electron/ClientRequest.ql
@@ -1,4 +1,4 @@
 import javascript
 
-from NodeJSLib::ClientRequest cr
+from Electron::ElectronClientRequest cr
 select cr
\ No newline at end of file
diff --git a/javascript/ql/test/library-tests/frameworks/NodeJSLib/ClientRequest.ql b/javascript/ql/test/library-tests/frameworks/NodeJSLib/ClientRequest.ql
index 157b158d45dc..0813343a6928 100644
--- a/javascript/ql/test/library-tests/frameworks/NodeJSLib/ClientRequest.ql
+++ b/javascript/ql/test/library-tests/frameworks/NodeJSLib/ClientRequest.ql
@@ -1,4 +1,4 @@
 import javascript
 
-from NodeJSLib::ClientRequest cr
+from NodeJSLib::NodeJSClientRequest cr
 select cr
\ No newline at end of file
diff --git a/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql b/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql
index a1a4de127136..00dae0b3b732 100644
--- a/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql
+++ b/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql
@@ -1,4 +1,4 @@
 import javascript
 
-from UrlRequest r
+from ClientRequest r
 select r, r.getUrl()
\ No newline at end of file

From 0da14fccbd88d0932c5ff4287672f406c8e34fd9 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 14:00:04 +0200
Subject: [PATCH 13/28] JS: renaming UrlRequests.qll -> ClientRequests.qll

---
 javascript/ql/src/javascript.qll                                | 2 +-
 .../frameworks/{UrlRequests.qll => ClientRequests.qll}          | 0
 .../ClientRequest.expected}                                     | 0
 .../UrlRequest.ql => ClientRequests/ClientRequest.ql}           | 0
 .../frameworks/{UrlRequests => ClientRequests}/tst.js           | 0
 5 files changed, 1 insertion(+), 1 deletion(-)
 rename javascript/ql/src/semmle/javascript/frameworks/{UrlRequests.qll => ClientRequests.qll} (100%)
 rename javascript/ql/test/library-tests/frameworks/{UrlRequests/UrlRequest.expected => ClientRequests/ClientRequest.expected} (100%)
 rename javascript/ql/test/library-tests/frameworks/{UrlRequests/UrlRequest.ql => ClientRequests/ClientRequest.ql} (100%)
 rename javascript/ql/test/library-tests/frameworks/{UrlRequests => ClientRequests}/tst.js (100%)

diff --git a/javascript/ql/src/javascript.qll b/javascript/ql/src/javascript.qll
index de99ecb0678b..16bdff91f76a 100644
--- a/javascript/ql/src/javascript.qll
+++ b/javascript/ql/src/javascript.qll
@@ -54,6 +54,7 @@ import semmle.javascript.frameworks.AWS
 import semmle.javascript.frameworks.Azure
 import semmle.javascript.frameworks.Babel
 import semmle.javascript.frameworks.ComposedFunctions
+import semmle.javascript.frameworks.ClientRequests
 import semmle.javascript.frameworks.Credentials
 import semmle.javascript.frameworks.CryptoLibraries
 import semmle.javascript.frameworks.DigitalOcean
@@ -71,7 +72,6 @@ import semmle.javascript.frameworks.Request
 import semmle.javascript.frameworks.SQL
 import semmle.javascript.frameworks.StringFormatters
 import semmle.javascript.frameworks.UriLibraries
-import semmle.javascript.frameworks.UrlRequests
 import semmle.javascript.frameworks.XmlParsers
 import semmle.javascript.frameworks.xUnit
 import semmle.javascript.linters.ESLint
diff --git a/javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
similarity index 100%
rename from javascript/ql/src/semmle/javascript/frameworks/UrlRequests.qll
rename to javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
diff --git a/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.expected b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
similarity index 100%
rename from javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.expected
rename to javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
diff --git a/javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.ql
similarity index 100%
rename from javascript/ql/test/library-tests/frameworks/UrlRequests/UrlRequest.ql
rename to javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.ql
diff --git a/javascript/ql/test/library-tests/frameworks/UrlRequests/tst.js b/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js
similarity index 100%
rename from javascript/ql/test/library-tests/frameworks/UrlRequests/tst.js
rename to javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js

From d578c7422d87449a1446e3f2c24c1c15063adff3 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 14:12:49 +0200
Subject: [PATCH 14/28] JS: docstring cleanup

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

diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
index 6f806d723d70..58c98273c8b0 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -502,7 +502,7 @@ module NodeJSLib {
   }
 
   /**
-   * A data flow node that is an HTTP or HTTPS client request made by a Node.js server, for example `http.request(url)`.
+   * A data flow node that is an HTTP or HTTPS client request made by a Node.js application, for example `http.request(url)`.
    */
   abstract class NodeJSClientRequest extends DataFlow::DefaultSourceNode {
 

From 2dd8e95a5109227a7b914b13c32896ef900dcb1f Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 14:13:52 +0200
Subject: [PATCH 15/28] JS: remove unused `getOptions` method

---
 .../ql/src/semmle/javascript/frameworks/Electron.qll      | 8 --------
 .../ql/src/semmle/javascript/frameworks/NodeJSLib.qll     | 7 -------
 2 files changed, 15 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
index a2b4e6745d6c..13eba4772c57 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
@@ -46,10 +46,6 @@ module Electron {
     NetRequest() {
       this = DataFlow::moduleMember("electron", "net").getAMemberCall("request")
     }
-    
-    override DataFlow::Node getOptions() {
-      result = this.(DataFlow::MethodCallNode).getArgument(0)
-    }
   }
   
   
@@ -60,10 +56,6 @@ module Electron {
     NewClientRequest() {
       this = DataFlow::moduleMember("electron", "ClientRequest").getAnInstantiation()
     }
-    
-    override DataFlow::Node getOptions() {
-      result = this.(DataFlow::NewNode).getArgument(0)
-    }
   }
   
     
diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
index 58c98273c8b0..489f1d234285 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -506,10 +506,6 @@ module NodeJSLib {
    */
   abstract class NodeJSClientRequest extends DataFlow::DefaultSourceNode {
 
-    /**
-     * Gets the options object or string URL used to make the request.
-     */
-    abstract DataFlow::Node getOptions();
   }
   
   /**
@@ -536,9 +532,6 @@ module NodeJSLib {
       result = url
     }
 
-    override DataFlow::Node getOptions() {
-      result = this.(DataFlow::MethodCallNode).getArgument(0)
-    }
   }
   
   /**

From 2306afdebf828a82e9b65f6b524cd7f77750b20e Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 3 Sep 2018 14:41:19 +0200
Subject: [PATCH 16/28] JS: use extensible architecture for Electron- and
 NodeClientRequest

---
 .../javascript/frameworks/ClientRequests.qll  |  4 +--
 .../semmle/javascript/frameworks/Electron.qll | 34 +++++++++++++++----
 .../javascript/frameworks/NodeJSLib.qll       | 15 ++++++--
 .../ClientRequests/ClientRequest.expected     |  6 ++++
 .../frameworks/ClientRequests/tst.js          |  9 ++++-
 5 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
index 19bb80d4fd2e..f21be0e710ea 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
@@ -10,7 +10,7 @@ import javascript
 /**
  * A call that performs a request to a URL.
  */
-class CustomClientRequest extends DataFlow::CallNode {
+class CustomClientRequest extends DataFlow::InvokeNode {
 
   /**
    * Gets the URL of the request.
@@ -21,7 +21,7 @@ class CustomClientRequest extends DataFlow::CallNode {
 /**
  * A call that performs a request to a URL.
  */
-class ClientRequest extends DataFlow::CallNode {
+class ClientRequest extends DataFlow::InvokeNode {
 
   CustomClientRequest custom;
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
index 13eba4772c57..b7c80eca8031 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
@@ -33,29 +33,51 @@ module Electron {
       this = DataFlow::moduleMember("electron", "BrowserView").getAnInstantiation()
     }
   }
-  
+
+  /**
+   * A Node.js-style HTTP or HTTPS request made using an Electron module.
+   */
+  abstract class CustomElectronClientRequest extends NodeJSLib::CustomNodeJSClientRequest {}
+
   /**
    * A Node.js-style HTTP or HTTPS request made using an Electron module.
    */
-  abstract class ElectronClientRequest extends NodeJSLib::NodeJSClientRequest {}
+  class ElectronClientRequest extends NodeJSLib::NodeJSClientRequest {
+
+    ElectronClientRequest() {
+      this instanceof CustomElectronClientRequest
+    }
+
+  }
   
   /**
    * A Node.js-style HTTP or HTTPS request made using `electron.net`, for example `net.request(url)`.
    */
-  private class NetRequest extends ElectronClientRequest {
+  private class NetRequest extends CustomElectronClientRequest {
     NetRequest() {
       this = DataFlow::moduleMember("electron", "net").getAMemberCall("request")
     }
+
+    override DataFlow::Node getUrl() {
+      result = getArgument(0) or
+      result = getOptionArgument(0, "url")
+    }
+
   }
-  
-  
+
   /**
    * A Node.js-style HTTP or HTTPS request made using `electron.client`, for example `new client(url)`.
    */
-  private class NewClientRequest extends ElectronClientRequest {
+  private class NewClientRequest extends CustomElectronClientRequest {
     NewClientRequest() {
       this = DataFlow::moduleMember("electron", "ClientRequest").getAnInstantiation()
     }
+
+    override DataFlow::Node getUrl() {
+      result = getArgument(0) or
+      result = getOptionArgument(0, "url")
+    }
+
   }
   
     
diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
index 489f1d234285..ba4dc9731e7f 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -504,14 +504,25 @@ module NodeJSLib {
   /**
    * A data flow node that is an HTTP or HTTPS client request made by a Node.js application, for example `http.request(url)`.
    */
-  abstract class NodeJSClientRequest extends DataFlow::DefaultSourceNode {
+  abstract class CustomNodeJSClientRequest extends CustomClientRequest {
+
+  }
+
+  /**
+   * A data flow node that is an HTTP or HTTPS client request made by a Node.js application, for example `http.request(url)`.
+   */
+  class NodeJSClientRequest extends ClientRequest {
+
+    NodeJSClientRequest() {
+      this instanceof CustomNodeJSClientRequest
+    }
 
   }
   
   /**
    * A model of a URL request in the Node.js `http` library.
    */
-  private class NodeHttpUrlRequest extends CustomClientRequest, NodeJSClientRequest {
+  private class NodeHttpUrlRequest extends CustomNodeJSClientRequest {
 
     DataFlow::Node url;
 
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
index 2b14dea32748..9c12f44e448f 100644
--- a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
+++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
@@ -14,3 +14,9 @@
 | tst.js:33:5:33:19 | got.stream(url) | tst.js:33:16:33:18 | url |
 | tst.js:35:5:35:21 | window.fetch(url) | tst.js:35:18:35:20 | url |
 | tst.js:37:5:37:18 | nodeFetch(url) | tst.js:37:15:37:17 | url |
+| tst.js:39:5:39:20 | net.request(url) | tst.js:39:17:39:19 | url |
+| tst.js:41:5:41:29 | net.req ...  url }) | tst.js:41:17:41:28 | { url: url } |
+| tst.js:41:5:41:29 | net.req ...  url }) | tst.js:41:24:41:26 | url |
+| tst.js:43:5:43:26 | new Cli ... st(url) | tst.js:43:23:43:25 | url |
+| tst.js:45:5:45:35 | new Cli ...  url }) | tst.js:45:23:45:34 | { url: url } |
+| tst.js:45:5:45:35 | new Cli ...  url }) | tst.js:45:30:45:32 | url |
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js b/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js
index 3656e30d9660..68a7c2bd5201 100644
--- a/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js
+++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js
@@ -6,7 +6,7 @@ import express from 'express';
 import axios from 'axios';
 import got from 'got';
 import nodeFetch from 'node-fetch';
-
+import {ClientRequest, net} from 'electron';
 (function() {
     request(url);
 
@@ -36,4 +36,11 @@ import nodeFetch from 'node-fetch';
 
     nodeFetch(url);
 
+    net.request(url);
+
+    net.request({ url: url });
+
+    new ClientRequest(url);
+
+    new ClientRequest({ url: url });
 });

From 89887e7dc8e6f271bab1ce05b5cd112cb0aaea5e Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Tue, 4 Sep 2018 08:21:27 +0200
Subject: [PATCH 17/28] JS: address review comments

---
 .../ql/src/Security/CWE-918/RequestForgery.qhelp    |  2 +-
 .../semmle/javascript/frameworks/ClientRequests.qll | 13 +++++++------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp b/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
index 75f0b681d168..a540278c67db 100644
--- a/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
+++ b/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
@@ -6,7 +6,7 @@
 	<overview>
 		<p>
 
-			Directly incorporating user input into a remote request
+			Directly incorporating user input into an HTTP request
 			without validating the input can facilitate different kinds of request
 			forgery attacks, where the attacker essentially controls the request.
 
diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
index f21be0e710ea..8403a7aec7b2 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
@@ -53,7 +53,7 @@ private string urlPropertyName() {
 }
 
 /**
- * A model of a URL request in the `request` library.
+ * A model of a URL request made using the `request` library.
  */
 private class RequestUrlRequest extends CustomClientRequest {
 
@@ -86,7 +86,7 @@ private class RequestUrlRequest extends CustomClientRequest {
 }
 
 /**
- * A model of a URL request in the `axios` library.
+ * A model of a URL request made using the `axios` library.
  */
 private class AxiosUrlRequest extends CustomClientRequest {
 
@@ -103,7 +103,8 @@ private class AxiosUrlRequest extends CustomClientRequest {
       ) and
       (
         url = getArgument(0) or
-        url = getOptionArgument([0..2], urlPropertyName()) // slightly over-approximate, in the name of simplicity
+        // depends on the method name and the call arity, over-approximating slightly in the name of simplicity
+        url = getOptionArgument([0..2], urlPropertyName())
       )
     )
   }
@@ -115,7 +116,7 @@ private class AxiosUrlRequest extends CustomClientRequest {
 }
 
 /**
- * A model of a URL request in an implementation of the `fetch` API.
+ * A model of a URL request made using an implementation of the `fetch` API.
  */
 private class FetchUrlRequest extends CustomClientRequest {
 
@@ -146,7 +147,7 @@ private class FetchUrlRequest extends CustomClientRequest {
 }
 
 /**
- * A model of a URL request in the `got` library.
+ * A model of a URL request made using the `got` library.
  */
 private class GotUrlRequest extends CustomClientRequest {
 
@@ -171,7 +172,7 @@ private class GotUrlRequest extends CustomClientRequest {
 }
 
 /**
- * A model of a URL request in the `superagent` library.
+ * A model of a URL request made using the `superagent` library.
  */
 private class SuperAgentUrlRequest extends CustomClientRequest {
 

From 6e1846b1ca5e24e5fe76acc3993d164e6f6a8c52 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Tue, 4 Sep 2018 10:15:56 +0200
Subject: [PATCH 18/28] JS: address doc review comments

---
 javascript/ql/src/Security/CWE-918/RequestForgery.qhelp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp b/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
index a540278c67db..83b541d3a24b 100644
--- a/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
+++ b/javascript/ql/src/Security/CWE-918/RequestForgery.qhelp
@@ -40,9 +40,9 @@
 
 			The following example shows an HTTP request parameter
 			being used directly in a URL request without validating the input,
-			which facilitate an SSRF attack. The request
-			<code>http.get(...)</code> is vulnerable since an attacker can choose
-			the value of <code>target</code> to be anything he wants. For
+			which facilitates an SSRF attack. The request
+			<code>http.get(...)</code> is vulnerable since attackers can choose
+			the value of <code>target</code> to be anything they want. For
 			instance, the attacker can choose
 			<code>"internal.example.com/#"</code> as the target, causing the URL
 			used in the request to be

From f63a3b3f399ad40afc0aee2774986e85ac7d2e99 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Tue, 4 Sep 2018 11:44:12 +0200
Subject: [PATCH 19/28] JS: add missing `abstract` modifier

---
 .../javascript/frameworks/ClientRequests.qll  |  2 +-
 .../ClientRequests/ClientRequest.expected     | 40 +++++++++----------
 .../ClientRequests/ClientRequest.ql           |  2 +-
 .../ClientRequest_getUrl.expected             | 22 ++++++++++
 .../ClientRequests/ClientRequest_getUrl.ql    |  4 ++
 .../frameworks/ClientRequests/tst.js          |  4 ++
 6 files changed, 50 insertions(+), 24 deletions(-)
 create mode 100644 javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.expected
 create mode 100644 javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.ql

diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
index 8403a7aec7b2..a19b5effca35 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
@@ -10,7 +10,7 @@ import javascript
 /**
  * A call that performs a request to a URL.
  */
-class CustomClientRequest extends DataFlow::InvokeNode {
+abstract class CustomClientRequest extends DataFlow::InvokeNode {
 
   /**
    * Gets the URL of the request.
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
index 9c12f44e448f..ad9bc6a4d196 100644
--- a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
+++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.expected
@@ -1,22 +1,18 @@
-| tst.js:11:5:11:16 | request(url) | tst.js:11:13:11:15 | url |
-| tst.js:13:5:13:20 | request.get(url) | tst.js:13:17:13:19 | url |
-| tst.js:15:5:15:23 | request.delete(url) | tst.js:15:20:15:22 | url |
-| tst.js:17:5:17:25 | request ...  url }) | tst.js:17:13:17:24 | { url: url } |
-| tst.js:17:5:17:25 | request ...  url }) | tst.js:17:20:17:22 | url |
-| tst.js:19:5:19:23 | requestPromise(url) | tst.js:19:20:19:22 | url |
-| tst.js:21:5:21:23 | superagent.get(url) | tst.js:21:20:21:22 | url |
-| tst.js:23:5:23:17 | http.get(url) | tst.js:23:14:23:16 | url |
-| tst.js:25:5:25:14 | axios(url) | tst.js:25:11:25:13 | url |
-| tst.js:27:5:27:18 | axios.get(url) | tst.js:27:15:27:17 | url |
-| tst.js:29:5:29:23 | axios({ url: url }) | tst.js:29:11:29:22 | { url: url } |
-| tst.js:29:5:29:23 | axios({ url: url }) | tst.js:29:18:29:20 | url |
-| tst.js:31:5:31:12 | got(url) | tst.js:31:9:31:11 | url |
-| tst.js:33:5:33:19 | got.stream(url) | tst.js:33:16:33:18 | url |
-| tst.js:35:5:35:21 | window.fetch(url) | tst.js:35:18:35:20 | url |
-| tst.js:37:5:37:18 | nodeFetch(url) | tst.js:37:15:37:17 | url |
-| tst.js:39:5:39:20 | net.request(url) | tst.js:39:17:39:19 | url |
-| tst.js:41:5:41:29 | net.req ...  url }) | tst.js:41:17:41:28 | { url: url } |
-| tst.js:41:5:41:29 | net.req ...  url }) | tst.js:41:24:41:26 | url |
-| tst.js:43:5:43:26 | new Cli ... st(url) | tst.js:43:23:43:25 | url |
-| tst.js:45:5:45:35 | new Cli ...  url }) | tst.js:45:23:45:34 | { url: url } |
-| tst.js:45:5:45:35 | new Cli ...  url }) | tst.js:45:30:45:32 | url |
+| tst.js:11:5:11:16 | request(url) |
+| tst.js:13:5:13:20 | request.get(url) |
+| tst.js:15:5:15:23 | request.delete(url) |
+| tst.js:17:5:17:25 | request ...  url }) |
+| tst.js:19:5:19:23 | requestPromise(url) |
+| tst.js:21:5:21:23 | superagent.get(url) |
+| tst.js:23:5:23:17 | http.get(url) |
+| tst.js:25:5:25:14 | axios(url) |
+| tst.js:27:5:27:18 | axios.get(url) |
+| tst.js:29:5:29:23 | axios({ url: url }) |
+| tst.js:31:5:31:12 | got(url) |
+| tst.js:33:5:33:19 | got.stream(url) |
+| tst.js:35:5:35:21 | window.fetch(url) |
+| tst.js:37:5:37:18 | nodeFetch(url) |
+| tst.js:39:5:39:20 | net.request(url) |
+| tst.js:41:5:41:29 | net.req ...  url }) |
+| tst.js:43:5:43:26 | new Cli ... st(url) |
+| tst.js:45:5:45:35 | new Cli ...  url }) |
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.ql b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.ql
index 00dae0b3b732..0baf011e1df6 100644
--- a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.ql
+++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest.ql
@@ -1,4 +1,4 @@
 import javascript
 
 from ClientRequest r
-select r, r.getUrl()
\ No newline at end of file
+select r
\ No newline at end of file
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.expected b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.expected
new file mode 100644
index 000000000000..9c12f44e448f
--- /dev/null
+++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.expected
@@ -0,0 +1,22 @@
+| tst.js:11:5:11:16 | request(url) | tst.js:11:13:11:15 | url |
+| tst.js:13:5:13:20 | request.get(url) | tst.js:13:17:13:19 | url |
+| tst.js:15:5:15:23 | request.delete(url) | tst.js:15:20:15:22 | url |
+| tst.js:17:5:17:25 | request ...  url }) | tst.js:17:13:17:24 | { url: url } |
+| tst.js:17:5:17:25 | request ...  url }) | tst.js:17:20:17:22 | url |
+| tst.js:19:5:19:23 | requestPromise(url) | tst.js:19:20:19:22 | url |
+| tst.js:21:5:21:23 | superagent.get(url) | tst.js:21:20:21:22 | url |
+| tst.js:23:5:23:17 | http.get(url) | tst.js:23:14:23:16 | url |
+| tst.js:25:5:25:14 | axios(url) | tst.js:25:11:25:13 | url |
+| tst.js:27:5:27:18 | axios.get(url) | tst.js:27:15:27:17 | url |
+| tst.js:29:5:29:23 | axios({ url: url }) | tst.js:29:11:29:22 | { url: url } |
+| tst.js:29:5:29:23 | axios({ url: url }) | tst.js:29:18:29:20 | url |
+| tst.js:31:5:31:12 | got(url) | tst.js:31:9:31:11 | url |
+| tst.js:33:5:33:19 | got.stream(url) | tst.js:33:16:33:18 | url |
+| tst.js:35:5:35:21 | window.fetch(url) | tst.js:35:18:35:20 | url |
+| tst.js:37:5:37:18 | nodeFetch(url) | tst.js:37:15:37:17 | url |
+| tst.js:39:5:39:20 | net.request(url) | tst.js:39:17:39:19 | url |
+| tst.js:41:5:41:29 | net.req ...  url }) | tst.js:41:17:41:28 | { url: url } |
+| tst.js:41:5:41:29 | net.req ...  url }) | tst.js:41:24:41:26 | url |
+| tst.js:43:5:43:26 | new Cli ... st(url) | tst.js:43:23:43:25 | url |
+| tst.js:45:5:45:35 | new Cli ...  url }) | tst.js:45:23:45:34 | { url: url } |
+| tst.js:45:5:45:35 | new Cli ...  url }) | tst.js:45:30:45:32 | url |
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.ql b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.ql
new file mode 100644
index 000000000000..00dae0b3b732
--- /dev/null
+++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequest_getUrl.ql
@@ -0,0 +1,4 @@
+import javascript
+
+from ClientRequest r
+select r, r.getUrl()
\ No newline at end of file
diff --git a/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js b/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js
index 68a7c2bd5201..025de83861f1 100644
--- a/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js
+++ b/javascript/ql/test/library-tests/frameworks/ClientRequests/tst.js
@@ -43,4 +43,8 @@ import {ClientRequest, net} from 'electron';
     new ClientRequest(url);
 
     new ClientRequest({ url: url });
+
+    unknown(url);
+
+    unknown({ url:url });
 });

From aaf1ac770db100aa50ad32cfaa4f2e4b9ec2d370 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Sun, 9 Sep 2018 21:30:43 +0200
Subject: [PATCH 20/28] JS: reduce declared precision of js/request-forgery

---
 change-notes/1.18/analysis-javascript.md             | 2 +-
 javascript/ql/src/Security/CWE-918/RequestForgery.ql | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md
index a7c4cc4b944e..104d45a88bd8 100644
--- a/change-notes/1.18/analysis-javascript.md
+++ b/change-notes/1.18/analysis-javascript.md
@@ -102,7 +102,7 @@
 | Clear-text logging of sensitive information (`js/clear-text-logging`) | security, external/cwe/cwe-312, external/cwe/cwe-315, external/cwe/cwe-359 | Highlights logging of sensitive information, indicating a violation of [CWE-312](https://cwe.mitre.org/data/definitions/312.html). Results shown on LGTM by default. |
 | Disabling Electron webSecurity (`js/disabling-electron-websecurity`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `webSecurity` property set to false. Results shown on LGTM by default. |
 | Enabling Electron allowRunningInsecureContent (`js/enabling-electron-insecure-content`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `allowRunningInsecureContent` property set to true. Results shown on LGTM by default. |
-| Uncontrolled data used in remote request (`js/request-forgery`) | security, external/cwe/cwe-918 | Highlights remote requests that are built from unsanitized user input, indicating a violation of [CWE-918](https://cwe.mitre.org/data/definitions/918.html). Results shown on LGTM by default. |
+| Uncontrolled data used in remote request (`js/request-forgery`) | security, external/cwe/cwe-918 | Highlights remote requests that are built from unsanitized user input, indicating a violation of [CWE-918](https://cwe.mitre.org/data/definitions/918.html). Results are not shown on LGTM by default. |
 | Use of externally-controlled format string (`js/tainted-format-string`) | security, external/cwe/cwe-134 | Highlights format strings containing user-provided data, indicating a violation of [CWE-134](https://cwe.mitre.org/data/definitions/134.html). Results shown on LGTM by default. |
 
 ## Changes to existing queries
diff --git a/javascript/ql/src/Security/CWE-918/RequestForgery.ql b/javascript/ql/src/Security/CWE-918/RequestForgery.ql
index a76307c439aa..8035f2ef56fb 100644
--- a/javascript/ql/src/Security/CWE-918/RequestForgery.ql
+++ b/javascript/ql/src/Security/CWE-918/RequestForgery.ql
@@ -3,7 +3,7 @@
  * @description Sending remote requests with user-controlled data allows for request forgery attacks.
  * @kind problem
  * @problem.severity error
- * @precision high
+ * @precision medium
  * @id js/request-forgery
  * @tags security
  *       external/cwe/cwe-918

From 620f99c5a30ee7c17a15e3766f410169f16b7982 Mon Sep 17 00:00:00 2001
From: Felicity Chapman <felicity@semmle.com>
Date: Tue, 11 Sep 2018 08:14:17 +0100
Subject: [PATCH 21/28] Remove template text

---
 change-notes/1.18/analysis-csharp.md | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/change-notes/1.18/analysis-csharp.md b/change-notes/1.18/analysis-csharp.md
index df455c4365bb..fe6f9824173e 100644
--- a/change-notes/1.18/analysis-csharp.md
+++ b/change-notes/1.18/analysis-csharp.md
@@ -1,11 +1,5 @@
 # Improvements to C# analysis
 
-> NOTES
->
-> Please describe your changes in terms that are suitable for
-> customers to read. These notes will have only minor tidying up
-> before they are published as part of the release notes.
-
 ## General improvements
 
 * Control flow analysis has been improved for `catch` clauses with filters.
@@ -14,7 +8,7 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| Arbitrary file write during zip extraction ("Zip Slip") (`cs/zipslip`) | security, external/cwe/cwe-022  | Identifies zip extraction routines which allow arbitrary file overwrite vulnerabilities.
+| Arbitrary file write during zip extraction ("Zip Slip") (`cs/zipslip`) | security, external/cwe/cwe-022  | Identifies zip extraction routines which allow arbitrary file overwrite vulnerabilities. |
 | Local scope variable shadows member (`cs/local-shadows-member`) | maintainability, readability | Replaces the existing queries Local variable shadows class member (`cs/local-shadows-class-member`), Local variable shadows struct member (`cs/local-shadows-struct-member`), Parameter shadows class member (`cs/parameter-shadows-class-member`), and Parameter shadows struct member (`cs/parameter-shadows-struct-member`). |
 
 ## Changes to existing queries
@@ -40,8 +34,6 @@
 * The `when` part of constant cases is now extracted.
 * Fixed a bug where `while(x is T y) ...` was not extracted correctly.
 
-* *Series of bullet points*
-
 ## Changes to QL libraries
 
 * A new non-member predicate `mayBeDisposed()` can be used to determine if a variable is potentially disposed inside a library. It will analyse the CIL code in the library to determine this.

From f48317f381f23685de93f8296e7367f2873882db Mon Sep 17 00:00:00 2001
From: Felicity Chapman <felicity@semmle.com>
Date: Tue, 11 Sep 2018 08:27:20 +0100
Subject: [PATCH 22/28] Minor updates to prepare for publication

---
 change-notes/1.18/analysis-csharp.md | 30 +++++++++++++++-------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/change-notes/1.18/analysis-csharp.md b/change-notes/1.18/analysis-csharp.md
index fe6f9824173e..204064e91482 100644
--- a/change-notes/1.18/analysis-csharp.md
+++ b/change-notes/1.18/analysis-csharp.md
@@ -15,18 +15,18 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
-| [Constant condition](https://help.semmle.com/wiki/display/CSHARP/Constant+condition) (`cs/constant-condition`) | More results | The query has been generalized to cover both Null-coalescing left operand is constant (`cs/constant-null-coalescing`) and Switch selector is constant (`cs/constant-switch-selector`). |
+| Constant condition (`cs/constant-condition`) | More results | The query has been generalized to report alerts for the old queries Null-coalescing left operand is constant (`cs/constant-null-coalescing`) and Switch selector is constant (`cs/constant-switch-selector`). |
 | Exposing internal representation (`cs/expose-implementation`) | Different results | The query has been rewritten, based on the [equivalent Java query](https://help.semmle.com/wiki/display/JAVA/Exposing+internal+representation). |
-| Local variable shadows class member(`cs/local-shadows-class-member`) | No results | The query has been replaced by Local scope variable shadows member  (`cs/local-shadows-member`). |
-| Local variable shadows struct member (`cs/local-shadows-struct-member`) | No results | The query has been replaced by Local scope variable shadows member  (`cs/local-shadows-member`). |
-| [Missing Dispose call on local IDisposable](https://help.semmle.com/wiki/display/CSHARP/Missing+Dispose+call+on+local+IDisposable) (`cs/local-not-disposed`) | Fewer results | The query identifies more cases where the local variable may be disposed by a library call. |
-| [Nested loops with same variable](https://help.semmle.com/wiki/display/CSHARP/Nested+loops+with+same+variable) (`cs/nested-loops-with-same-variable`) | Fewer results | Results are no longer highlighted in nested loops that share the same condition, and do not use the variable after the inner loop. |
-| Null-coalescing left operand is constant (`cs/constant-null-coalescing`) | No results | The query has been removed, as it is now covered by Constant condition (`cs/constant-condition`). |
-| Parameter shadows class member (`cs/parameter-shadows-class-member`) | No results | The query has been replaced by Local scope variable shadows member  (`cs/local-shadows-member`). |
-| Parameter shadows struct member (`cs/parameter-shadows-struct-member`) | No results | The query has been replaced by Local scope variable shadows member  (`cs/local-shadows-member`). |
-| [Potentially incorrect CompareTo(...) signature](https://help.semmle.com/wiki/display/CSHARP/Potentially+incorrect+CompareTo%28...%29+signature) (`cs/wrong-compareto-signature`) | Fewer results | Results are no longer highlighted in constructed types. |
-| Switch selector is constant (`cs/constant-switch-selector`) | No results | The query has been removed, as it is now covered by Constant condition (`cs/constant-condition`). |
-| [Useless upcast](https://help.semmle.com/wiki/display/CSHARP/Useless+upcast) (`cs/useless-upcast`) | Fewer results | The query has been improved to cover more cases where upcasts may be needed. |
+| Local variable shadows class member (`cs/local-shadows-class-member`) | No results | The query has been replaced by the new query: Local scope variable shadows member  (`cs/local-shadows-member`). |
+| Local variable shadows struct member (`cs/local-shadows-struct-member`) | No results | The query has been replaced by the new query: Local scope variable shadows member  (`cs/local-shadows-member`). |
+| Missing Dispose call on local IDisposable (`cs/local-not-disposed`) | Fewer false positive results | The query identifies more cases where the local variable may be disposed by a library call. |
+| Nested loops with same variable (`cs/nested-loops-with-same-variable`) | Fewer false positive results | Results are no longer highlighted in nested loops that share the same condition, and do not use the variable after the inner loop. |
+| Null-coalescing left operand is constant (`cs/constant-null-coalescing`) | No results | The query has been removed, as alerts for this problem are now reported by the new query: Constant condition (`cs/constant-condition`). |
+| Parameter shadows class member (`cs/parameter-shadows-class-member`) | No results | The query has been replaced by the new query: Local scope variable shadows member  (`cs/local-shadows-member`). |
+| Parameter shadows struct member (`cs/parameter-shadows-struct-member`) | No results | The query has been replaced by the new query: Local scope variable shadows member  (`cs/local-shadows-member`). |
+| Potentially incorrect CompareTo(...) signature (`cs/wrong-compareto-signature`) | Fewer false positive results | Results are no longer highlighted in constructed types. |
+| Switch selector is constant (`cs/constant-switch-selector`) | No results | The query has been removed, as alerts for this problem are now reported by the new query: Constant condition (`cs/constant-condition`). |
+| Useless upcast (`cs/useless-upcast`) | Fewer false positive results | The query has been improved to cover more cases where upcasts may be needed. |
 
 ## Changes to code extraction
 
@@ -36,8 +36,9 @@
 
 ## Changes to QL libraries
 
-* A new non-member predicate `mayBeDisposed()` can be used to determine if a variable is potentially disposed inside a library. It will analyse the CIL code in the library to determine this.
-* Several control flow graph entities have been renamed (the old names still exist for backwards compatibility):
+* A new non-member predicate `mayBeDisposed()` can be used to determine if a variable is potentially disposed inside a library. It will analyze the CIL code in the library to determine this.
+* The predicate `getCondition()` has been moved from `TypeCase` to `CaseStmt`. It is now possible to get the condition of a `ConstCase` using its `getCondition()` predicate.
+* Several control flow graph entities have been renamed (the old names are deprecated but are still available in this release for backwards compatibility):
   - `ControlFlowNode` has been renamed to `ControlFlow::Node`.
   - `CallableEntryNode` has been renamed to `ControlFlow::Nodes::EntryNode`.
   - `CallableExitNode` has been renamed to `ControlFlow::Nodes::ExitNode`.
@@ -55,4 +56,5 @@
   - `ControlFlowEdgeGotoCase` has been renamed to `ControlFlow::SuccessorTypes::GotoCaseSuccessor`.
   - `ControlFlowEdgeGotoDefault` has been renamed to `ControlFlow::SuccessorTypes::GotoDefaultSuccessor`.
   - `ControlFlowEdgeException` has been renamed to `ControlFlow::SuccessorTypes::ExceptionSuccessor`.
-* The predicate `getCondition()` has been moved from `TypeCase` to `CaseStmt`. It is now possible to get the condition of a `ConstCase` using its `getCondition()` predicate.
+
+> You should update any custom queries that use these entities to ensure that they continue working when the old names are removed in a future release.

From 3d444f3dc672497e0b75f266a4f605923422adf9 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Thu, 6 Sep 2018 10:39:37 +0100
Subject: [PATCH 23/28] JavaScript: fix CFG for EnhancedForStmt

---
 javascript/ql/src/semmle/javascript/Stmt.qll        |  4 ++++
 .../DeadStoreOfLocal/for-of-continue.js             | 13 +++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/for-of-continue.js

diff --git a/javascript/ql/src/semmle/javascript/Stmt.qll b/javascript/ql/src/semmle/javascript/Stmt.qll
index facc438e12b4..486298a38965 100644
--- a/javascript/ql/src/semmle/javascript/Stmt.qll
+++ b/javascript/ql/src/semmle/javascript/Stmt.qll
@@ -607,6 +607,10 @@ abstract class EnhancedForLoop extends LoopStmt {
   override Stmt getBody() {
     result = getChildStmt(2)
   }
+
+  override ControlFlowNode getFirstControlFlowNode() {
+    result = getIteratorExpr().getFirstControlFlowNode()
+  }
 }
 
 /** A `for`-`in` loop. */
diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/for-of-continue.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/for-of-continue.js
new file mode 100644
index 000000000000..da26a3557ade
--- /dev/null
+++ b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/for-of-continue.js
@@ -0,0 +1,13 @@
+function f() {
+  let y = false;
+  for (const x of [1, 2, 3]) {
+    if (x > 0) {
+        y = true; // OK
+        continue;
+    }
+    return;
+  }
+  if (!y) {
+    console.log("Hello");
+  }
+}

From 0a4a5da1f0aa24578e1915b1263eaf895bbb3996 Mon Sep 17 00:00:00 2001
From: Asger F <asger@semmle.com>
Date: Thu, 6 Sep 2018 14:18:03 +0100
Subject: [PATCH 24/28] JavaScript: update output of CFG test

---
 .../ql/test/library-tests/CFG/CFG.expected    | 22 +++++++++----------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/javascript/ql/test/library-tests/CFG/CFG.expected b/javascript/ql/test/library-tests/CFG/CFG.expected
index d2a79ea52a5a..c78a966ed341 100644
--- a/javascript/ql/test/library-tests/CFG/CFG.expected
+++ b/javascript/ql/test/library-tests/CFG/CFG.expected
@@ -644,15 +644,14 @@
 | forof | 1 | entry node of functio ...     ;\\n} | 2 | {\\n    f ...     ;\\n} |
 | forof | 1 | f | 1 | functio ...     ;\\n} |
 | forof | 1 | functio ...     ;\\n} | 10 | exit node of <toplevel> |
-| forof | 2 | {\\n    f ...     ;\\n} | 3 | for (\\n  ... )\\n    ; |
-| forof | 3 | for (\\n  ... )\\n    ; | 7 | [] |
+| forof | 2 | {\\n    f ...     ;\\n} | 7 | [] |
+| forof | 3 | for (\\n  ... )\\n    ; | 4 | var\\n        x |
+| forof | 3 | for (\\n  ... )\\n    ; | 9 | exit node of functio ...     ;\\n} |
 | forof | 4 | var\\n        x | 5 | x |
 | forof | 5 | x | 5 | x |
 | forof | 5 | x | 8 | ; |
-| forof | 7 | [] | 4 | var\\n        x |
-| forof | 7 | [] | 9 | exit node of functio ...     ;\\n} |
-| forof | 8 | ; | 4 | var\\n        x |
-| forof | 8 | ; | 9 | exit node of functio ...     ;\\n} |
+| forof | 7 | [] | 3 | for (\\n  ... )\\n    ; |
+| forof | 8 | ; | 3 | for (\\n  ... )\\n    ; |
 | globals | 1 | entry node of <toplevel> | 14 | g |
 | globals | 1 | var\\n x,\\n y; | 2 | x |
 | globals | 2 | x | 2 | x |
@@ -662,15 +661,14 @@
 | globals | 4 | {\\n    var\\n     z;\\n} | 5 | var\\n     z; |
 | globals | 5 | var\\n     z; | 6 | z |
 | globals | 6 | z | 6 | z |
-| globals | 6 | z | 8 | for (\\n  ... [])\\n  ; |
-| globals | 8 | for (\\n  ... [])\\n  ; | 11 | [] |
+| globals | 6 | z | 11 | [] |
+| globals | 8 | for (\\n  ... [])\\n  ; | 9 | var\\n  a |
+| globals | 8 | for (\\n  ... [])\\n  ; | 13 | function\\n g()\\n{\\n} |
 | globals | 9 | var\\n  a | 10 | a |
 | globals | 10 | a | 10 | a |
 | globals | 10 | a | 12 | ; |
-| globals | 11 | [] | 9 | var\\n  a |
-| globals | 11 | [] | 13 | function\\n g()\\n{\\n} |
-| globals | 12 | ; | 9 | var\\n  a |
-| globals | 12 | ; | 13 | function\\n g()\\n{\\n} |
+| globals | 11 | [] | 8 | for (\\n  ... [])\\n  ; |
+| globals | 12 | ; | 8 | for (\\n  ... [])\\n  ; |
 | globals | 13 | entry node of function\\n g()\\n{\\n} | 15 | {\\n} |
 | globals | 13 | function\\n g()\\n{\\n} | 17 | !\\nfunction\\n h()\\n{\\n}; |
 | globals | 14 | g | 1 | var\\n x,\\n y; |

From 223bf6cf56bb0321175500ff999e43d4ee283a35 Mon Sep 17 00:00:00 2001
From: Felicity Chapman <felicity@semmle.com>
Date: Tue, 11 Sep 2018 22:31:32 +0100
Subject: [PATCH 25/28] Updates for consistency

---
 change-notes/1.18/analysis-javascript.md | 189 +++++++++++------------
 1 file changed, 93 insertions(+), 96 deletions(-)

diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md
index 7278d1e94dc0..01afe4f1fa55 100644
--- a/change-notes/1.18/analysis-javascript.md
+++ b/change-notes/1.18/analysis-javascript.md
@@ -2,15 +2,13 @@
 
 ## General improvements
 
-* Additional heuristics have been added to `semmle.javascript.heuristics`. Add `import semmle.javascript.heuristics.all` to a query in order to activate all of the heuristics at once.
-
-* Modelling of data flow through destructuring assignments has been improved. This may give additional results for the security queries and other queries that rely on data flow.
+* Improved modeling of data flow through destructuring assignments may give additional results for the security queries and other queries that rely on data flow.
 
-* Modelling of global variables has been improved. This may give more true-positive results and fewer false-positive results for a variety of queries.
+* Improved modeling of global variables may give more true-positive results and fewer false-positive results for a variety of queries.
 
-* Modelling of re-export declarations has been improved. This may result in fewer false-positive results for a variety of queries.
+* Improved modeling of re-export declarations may result in fewer false-positive results for a variety of queries.
 
-* Modelling of taint flow through array operations has been improved. This may give additional results for the security queries.
+* Improved modeling of taint flow through array operations may give additional results for the security queries.
 
 * The taint tracking library recognizes more ways in which taint propagates. In particular, some flow through string formatters is now recognized. This may give additional results for the security queries.
 
@@ -18,84 +16,86 @@
 
 * Type inference for simple function calls has been improved. This may give additional results for queries that rely on type inference.
 
+* Additional heuristics have been added to `semmle.javascript.heuristics`. Add `import semmle.javascript.heuristics.all` to a query in order to activate all of the heuristics at once.
+
+* Handling of ambient TypeScript code has been improved. As a result, fewer false-positive results will be reported in `.d.ts` files.
+
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following libraries:
-  - [axios](https://github.com/axios/axios)
-  - [bluebird](https://bluebirdjs.com)
-  - [browserid-crypto](https://github.com/mozilla/browserid-crypto)
-  - [compose-function](https://github.com/stoeffel/compose-function)
-  - [cookie-parser](https://github.com/expressjs/cookie-parser)
-  - [cookie-session](https://github.com/expressjs/cookie-session)
-  - [cross-fetch](https://github.com/lquixada/cross-fetch)
-  - [crypto-js](https://github.com/https://github.com/brix/crypto-js)
-  - [deep-assign](https://github.com/sindresorhus/deep-assign)
-  - [deep-extend](https://github.com/unclechu/node-deep-extend)
-  - [deep-merge](https://github.com/Raynos/deep-merge)
-  - [deep](https://github.com/jeffomatic/deep)
-  - [deepmerge](https://github.com/KyleAMathews/deepmerge)
-  - [defaults-deep](https://github.com/jonschlinkert/defaults-deep)
-  - [defaults](https://github.com/tmpvar/defaults)
-  - [dottie](https://github.com/mickhansen/dottie.js)
-  - [dotty](https://github.com/deoxxa/dotty)
-  - [ent](https://github.com/substack/node-ent)
-  - [entities](https://github.com/fb55/node-entities)
-  - [escape-goat](https://github.com/sindresorhus/escape-goat)
-  - [express-jwt](https://github.com/auth0/express-jwt)
-  - [express-session](https://github.com/expressjs/session)
-  - [extend-shallow](https://github.com/jonschlinkert/extend-shallow)
-  - [extend](https://github.com/justmoon/node-extend)
-  - [extend2](https://github.com/eggjs/extend2)
-  - [fast-json-parse](https://github.com/mcollina/fast-json-parse)
-  - [forge](https://github.com/digitalbazaar/forge)
-  - [format-util](https://github.com/tmpfs/format-util)
-  - [got](https://github.com/sindresorhus/got)
-  - [global](https://github.com/Raynos/global)
-  - [he](https://github.com/mathiasbynens/he)
-  - [html-entities](https://github.com/mdevils/node-html-entities)
-  - [isomorphic-fetch](https://github.com/matthew-andrews/isomorphic-fetch)
-  - [jquery](https://jquery.com)
-  - [js-extend](https://github.com/vmattos/js-extend)
-  - [json-parse-better-errors](https://github.com/zkat/json-parse-better-errors)
-  - [json-parse-safe](https://github.com/joaquimserafim/json-parse-safe)
-  - [json-safe-parse](https://github.com/bahamas10/node-json-safe-parse)
-  - [just-compose](https://github.com/angus-c/just)
-  - [just-extend](https://github.com/angus-c/just)
-  - [lodash](https://lodash.com)
-  - [merge-deep](https://github.com/jonschlinkert/merge-deep)
-  - [merge-options](https://github.com/schnittstabil/merge-options)
-  - [merge](https://github.com/yeikos/js.merge)
-  - [mixin-deep](https://github.com/jonschlinkert/mixin-deep)
-  - [mixin-object](https://github.com/jonschlinkert/mixin-object)
-  - [MySQL2](https://github.com/sidorares/node-mysql2)
-  - [node.extend](https://github.com/dreamerslab/node.extend)
-  - [node-fetch](https://github.com/bitinn/node-fetch)
-  - [object-assign](https://github.com/sindresorhus/object-assign)
-  - [object.assign](https://github.com/ljharb/object.assign)
-  - [object.defaults](https://github.com/jonschlinkert/object.defaults)
-  - [parse-json](https://github.com/sindresorhus/parse-json)
-  - [printf](https://github.com/adaltas/node-printf)
-  - [printj](https://github.com/SheetJS/printj)
-  - [q](https://documentup.com/kriskowal/q/)
-  - [ramda](https://ramdajs.com)
-  - [request](https://github.com/request/request)
-  - [request-promise](https://github.com/request/request-promise)
-  - [request-promise-any](https://github.com/request/request-promise-any)
-  - [request-promise-native](https://github.com/request/request-promise-native)
-  - [React Native](https://facebook.github.io/react-native/)
-  - [safe-json-parse](https://github.com/Raynos/safe-json-parse)
-  - [sanitize](https://github.com/pocketly/node-sanitize)
-  - [sanitizer](https://github.com/theSmaw/Caja-HTML-Sanitizer)
-  - [smart-extend](https://github.com/danielkalen/smart-extend)
-  - [sprintf.js](https://github.com/alexei/sprintf.js)
-  - [string-template](https://github.com/Matt-Esch/string-template)
-  - [superagent](https://github.com/visionmedia/superagent)
-  - [underscore](https://underscorejs.org)
-  - [util-extend](https://github.com/isaacs/util-extend)
-  - [utils-merge](https://github.com/jaredhanson/utils-merge)
-  - [validator](https://github.com/chriso/validator.js)
-  - [xss](https://github.com/leizongmin/js-xss)
-  - [xtend](https://github.com/Raynos/xtend)
-
-* Handling of ambient TypeScript code has been improved. As a result, fewer false positives will be reported in `.d.ts` files.
+ [axios](https://github.com/axios/axios), 
+ [bluebird](https://bluebirdjs.com), 
+ [browserid-crypto](https://github.com/mozilla/browserid-crypto), 
+ [compose-function](https://github.com/stoeffel/compose-function), 
+ [cookie-parser](https://github.com/expressjs/cookie-parser), 
+ [cookie-session](https://github.com/expressjs/cookie-session), 
+ [cross-fetch](https://github.com/lquixada/cross-fetch), 
+ [crypto-js](https://github.com/https://github.com/brix/crypto-js), 
+ [deep-assign](https://github.com/sindresorhus/deep-assign), 
+ [deep-extend](https://github.com/unclechu/node-deep-extend), 
+ [deep-merge](https://github.com/Raynos/deep-merge), 
+ [deep](https://github.com/jeffomatic/deep), 
+ [deepmerge](https://github.com/KyleAMathews/deepmerge), 
+ [defaults-deep](https://github.com/jonschlinkert/defaults-deep), 
+ [defaults](https://github.com/tmpvar/defaults), 
+ [dottie](https://github.com/mickhansen/dottie.js), 
+ [dotty](https://github.com/deoxxa/dotty), 
+ [ent](https://github.com/substack/node-ent), 
+ [entities](https://github.com/fb55/node-entities), 
+ [escape-goat](https://github.com/sindresorhus/escape-goat), 
+ [express-jwt](https://github.com/auth0/express-jwt), 
+ [express-session](https://github.com/expressjs/session), 
+ [extend-shallow](https://github.com/jonschlinkert/extend-shallow), 
+ [extend](https://github.com/justmoon/node-extend), 
+ [extend2](https://github.com/eggjs/extend2), 
+ [fast-json-parse](https://github.com/mcollina/fast-json-parse), 
+ [forge](https://github.com/digitalbazaar/forge), 
+ [format-util](https://github.com/tmpfs/format-util), 
+ [got](https://github.com/sindresorhus/got), 
+ [global](https://github.com/Raynos/global), 
+ [he](https://github.com/mathiasbynens/he), 
+ [html-entities](https://github.com/mdevils/node-html-entities), 
+ [isomorphic-fetch](https://github.com/matthew-andrews/isomorphic-fetch), 
+ [jquery](https://jquery.com), 
+ [js-extend](https://github.com/vmattos/js-extend), 
+ [json-parse-better-errors](https://github.com/zkat/json-parse-better-errors), 
+ [json-parse-safe](https://github.com/joaquimserafim/json-parse-safe), 
+ [json-safe-parse](https://github.com/bahamas10/node-json-safe-parse), 
+ [just-compose](https://github.com/angus-c/just), 
+ [just-extend](https://github.com/angus-c/just), 
+ [lodash](https://lodash.com), 
+ [merge-deep](https://github.com/jonschlinkert/merge-deep), 
+ [merge-options](https://github.com/schnittstabil/merge-options), 
+ [merge](https://github.com/yeikos/js.merge), 
+ [mixin-deep](https://github.com/jonschlinkert/mixin-deep), 
+ [mixin-object](https://github.com/jonschlinkert/mixin-object), 
+ [MySQL2](https://github.com/sidorares/node-mysql2), 
+ [node.extend](https://github.com/dreamerslab/node.extend), 
+ [node-fetch](https://github.com/bitinn/node-fetch), 
+ [object-assign](https://github.com/sindresorhus/object-assign), 
+ [object.assign](https://github.com/ljharb/object.assign), 
+ [object.defaults](https://github.com/jonschlinkert/object.defaults), 
+ [parse-json](https://github.com/sindresorhus/parse-json), 
+ [printf](https://github.com/adaltas/node-printf), 
+ [printj](https://github.com/SheetJS/printj), 
+ [q](https://documentup.com/kriskowal/q/), 
+ [ramda](https://ramdajs.com), 
+ [request](https://github.com/request/request), 
+ [request-promise](https://github.com/request/request-promise), 
+ [request-promise-any](https://github.com/request/request-promise-any), 
+ [request-promise-native](https://github.com/request/request-promise-native), 
+ [React Native](https://facebook.github.io/react-native/), 
+ [safe-json-parse](https://github.com/Raynos/safe-json-parse), 
+ [sanitize](https://github.com/pocketly/node-sanitize), 
+ [sanitizer](https://github.com/theSmaw/Caja-HTML-Sanitizer), 
+ [smart-extend](https://github.com/danielkalen/smart-extend), 
+ [sprintf.js](https://github.com/alexei/sprintf.js), 
+ [string-template](https://github.com/Matt-Esch/string-template), 
+ [superagent](https://github.com/visionmedia/superagent), 
+ [underscore](https://underscorejs.org), 
+ [util-extend](https://github.com/isaacs/util-extend), 
+ [utils-merge](https://github.com/jaredhanson/utils-merge), 
+ [validator](https://github.com/chriso/validator.js), 
+ [xss](https://github.com/leizongmin/js-xss), 
+ [xtend](https://github.com/Raynos/xtend).
 
 ## New queries
 
@@ -104,7 +104,7 @@
 | Clear-text logging of sensitive information (`js/clear-text-logging`) | security, external/cwe/cwe-312, external/cwe/cwe-315, external/cwe/cwe-359 | Highlights logging of sensitive information, indicating a violation of [CWE-312](https://cwe.mitre.org/data/definitions/312.html). Results shown on LGTM by default. |
 | Disabling Electron webSecurity (`js/disabling-electron-websecurity`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `webSecurity` property set to false. Results shown on LGTM by default. |
 | Enabling Electron allowRunningInsecureContent (`js/enabling-electron-insecure-content`) | security, frameworks/electron | Highlights Electron browser objects that are created with the `allowRunningInsecureContent` property set to true. Results shown on LGTM by default. |
-| Uncontrolled data used in remote request (`js/request-forgery`) | security, external/cwe/cwe-918 | Highlights remote requests that are built from unsanitized user input, indicating a violation of [CWE-918](https://cwe.mitre.org/data/definitions/918.html). Results are not shown on LGTM by default. |
+| Uncontrolled data used in remote request (`js/request-forgery`) | security, external/cwe/cwe-918 | Highlights remote requests that are built from unsanitized user input, indicating a violation of [CWE-918](https://cwe.mitre.org/data/definitions/918.html). Results are hidden on LGTM by default. |
 | Use of externally-controlled format string (`js/tainted-format-string`) | security, external/cwe/cwe-134 | Highlights format strings containing user-provided data, indicating a violation of [CWE-134](https://cwe.mitre.org/data/definitions/134.html). Results shown on LGTM by default. |
 
 ## Changes to existing queries
@@ -112,14 +112,13 @@
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Arguments redefined | Fewer results | This rule previously also flagged redefinitions of `eval`. This was an oversight that is now fixed. |
-| Comparison between inconvertible types | Fewer results | This rule now flags fewer comparisons involving parameters. |
-| Comparison between inconvertible types | Lower severity | The severity of this rule has been revised to "warning". |
+| Comparison between inconvertible types | Fewer results | This rule now flags fewer comparisons involving parameters. The severity of this rule has been revised to "warning". |
 | CORS misconfiguration for credentials transfer | More true-positive results | This rule now treats header names case-insensitively. |
 | Hard-coded credentials | More true-positive results | This rule now recognizes secret cryptographic keys. |
-| Incomplete string escaping or encoding | Better name, more true-positive results | This rule has been renamed to more clearly reflect its purpose. Also, it now recognizes incomplete URL encoding and decoding. |
+| Incomplete string escaping or encoding | New name, more true-positive results | This rule now recognizes incomplete URL encoding and decoding. As a consequence, the name was updated to reflect the change in behavior. |
 | Insecure randomness | More true-positive results | This rule now recognizes secret cryptographic keys. |
 | Misleading indentation after control statement | Fewer results | This rule temporarily ignores TypeScript files. |
-| Missing rate limiting | More true-positive results, fewer false-positive results | This rule now recognizes additional rate limiters and expensive route handlers. | 
+| Missing rate limiting | More true-positive results, fewer false-positive results | This rule now recognizes additional rate limiters and expensive route handlers. |
 | Missing X-Frame-Options HTTP header | Fewer false-positive results | This rule now treats header names case-insensitively. |
 | Omitted array element | Fewer results | This rule temporarily ignores TypeScript files. |
 | Reflected cross-site scripting | Fewer false-positive results | This rule now treats header names case-insensitively. |
@@ -128,16 +127,14 @@
 | Superfluous trailing arguments | Fewer false-positive results | This rule now ignores calls to some empty functions. |
 | Type confusion through parameter tampering | Fewer false-positive results | This rule no longer flags emptiness checks. |
 | Uncontrolled command line | More true-positive results | This rule now recognizes indirect command injection through `sh -c` and similar. |
-| Unused variable | Fewer results | This rule no longer flags class expressions that could be made anonymous. While technically true, these results are not interesting. |
-| Unused variable | Renamed | This rule has been renamed to "Unused variable, import, function or class" to reflect the fact that it flags different kinds of unused program elements. |
-| Use of incompletely initialized object| Fewer results | This rule now flags the constructor instead its errorneous `this` or `super` expressions. |
-| Useless conditional | Fewer results | This rule no longer flags uses of boolean return values. | 
-| Useless conditional | Fewer results | This rule now flags fewer comparisons involving parameters. | 
+| Unused variable | New name, fewer results | This rule has been renamed to "Unused variable, import, function or class" to reflect the fact that it flags different kinds of unused program elements. The rule no longer flags class expressions that could be made anonymous. While technically true, these results are not interesting. |
+| Use of incompletely initialized object| Fewer results | This rule now flags the constructor instead of its errorneous `this` or `super` expressions. |
+| Useless conditional | Fewer results | This rule no longer flags uses of boolean return values and highlights fewer comparisons involving parameters. |
 
 ## Changes to QL libraries
 
-* HTTP and HTTPS requests made using the Node.js `http.request` and `https.request` APIs and the Electron `Electron.net.request` and `Electron.ClientRequest` APIs are modeled as `RemoteFlowSources`.
-* HTTP header names are now always normalized to lower case to reflect the fact that they are case insensitive. In particular, the result of `HeaderDefinition.getAHeaderName`, and the first parameter of `HeaderDefinition.defines`, `ExplicitHeaderDefinition.definesExplicitly` and `RouteHandler.getAResponseHeader` is now always a lower-case string.
+* HTTP and HTTPS requests made using the Node.js `http.request` and `https.request` APIs, and the Electron `Electron.net.request` and `Electron.ClientRequest` APIs, are modeled as `RemoteFlowSources`.
+* HTTP header names are now always normalized to lower case to reflect the fact that they are case insensitive. In particular, the result of `HeaderDefinition.getAHeaderName`, and the first parameter of `HeaderDefinition.defines`, `ExplicitHeaderDefinition.definesExplicitly`, and `RouteHandler.getAResponseHeader` are now always a lower-case string.
 * New AST nodes have been added for TypeScript 2.9 and 3.0 features.
-* The class `JsonParseCall` has been deprecated. Use `JsonParserCall` instead.
+* The class `JsonParseCall` has been deprecated. Update your queries to use `JsonParserCall` instead.
 * The handling of spread arguments in the data flow library has been changed: `DataFlow::InvokeNode.getArgument(i)` is now only defined when there is no spread argument at or before argument position `i`, and similarly `InvokeNode.getNumArgument` is only defined for invocations without spread arguments.

From 7dd891d908f327877c8747f3ae1225425eb746e4 Mon Sep 17 00:00:00 2001
From: Felicity Chapman <felicity@semmle.com>
Date: Tue, 11 Sep 2018 22:51:14 +0100
Subject: [PATCH 26/28] Further updates and addition of query @ids

---
 change-notes/1.18/analysis-javascript.md | 38 ++++++++++++------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md
index 01afe4f1fa55..1426db55c916 100644
--- a/change-notes/1.18/analysis-javascript.md
+++ b/change-notes/1.18/analysis-javascript.md
@@ -111,25 +111,25 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
-| Arguments redefined | Fewer results | This rule previously also flagged redefinitions of `eval`. This was an oversight that is now fixed. |
-| Comparison between inconvertible types | Fewer results | This rule now flags fewer comparisons involving parameters. The severity of this rule has been revised to "warning". |
-| CORS misconfiguration for credentials transfer | More true-positive results | This rule now treats header names case-insensitively. |
-| Hard-coded credentials | More true-positive results | This rule now recognizes secret cryptographic keys. |
-| Incomplete string escaping or encoding | New name, more true-positive results | This rule now recognizes incomplete URL encoding and decoding. As a consequence, the name was updated to reflect the change in behavior. |
-| Insecure randomness | More true-positive results | This rule now recognizes secret cryptographic keys. |
-| Misleading indentation after control statement | Fewer results | This rule temporarily ignores TypeScript files. |
-| Missing rate limiting | More true-positive results, fewer false-positive results | This rule now recognizes additional rate limiters and expensive route handlers. |
-| Missing X-Frame-Options HTTP header | Fewer false-positive results | This rule now treats header names case-insensitively. |
-| Omitted array element | Fewer results | This rule temporarily ignores TypeScript files. |
-| Reflected cross-site scripting | Fewer false-positive results | This rule now treats header names case-insensitively. |
-| Semicolon insertion | Fewer results | This rule temporarily ignores TypeScript files. |
-| Server-side URL redirect | More true-positive results | This rule now treats header names case-insensitively. |
-| Superfluous trailing arguments | Fewer false-positive results | This rule now ignores calls to some empty functions. |
-| Type confusion through parameter tampering | Fewer false-positive results | This rule no longer flags emptiness checks. |
-| Uncontrolled command line | More true-positive results | This rule now recognizes indirect command injection through `sh -c` and similar. |
-| Unused variable | New name, fewer results | This rule has been renamed to "Unused variable, import, function or class" to reflect the fact that it flags different kinds of unused program elements. The rule no longer flags class expressions that could be made anonymous. While technically true, these results are not interesting. |
-| Use of incompletely initialized object| Fewer results | This rule now flags the constructor instead of its errorneous `this` or `super` expressions. |
-| Useless conditional | Fewer results | This rule no longer flags uses of boolean return values and highlights fewer comparisons involving parameters. |
+| Arguments redefined (`js/arguments-redefinition`) | Fewer results | This query previously also flagged redefinitions of `eval`. This was an oversight that is now fixed. |
+| Comparison between inconvertible types (`js/comparison-between-incompatible-types`) | Fewer results | This query now flags fewer comparisons involving parameters. The severity of this query has been revised to "warning". |
+| CORS misconfiguration for credentials transfer (`js/cors-misconfiguration-for-credentials`) | More true-positive results | This query now treats header names case-insensitively. |
+| Hard-coded credentials (`js/hardcoded-credentials`) | More true-positive results | This query now recognizes secret cryptographic keys. |
+| Incomplete string escaping or encoding (`js/incomplete-sanitization`) | New name, more true-positive results | The "Incomplete sanitization" query has been renamed to more clearly reflect its purpose. It now recognizes incomplete URL encoding and decoding. |
+| Insecure randomness (`js/insecure-randomness`) | More true-positive results | This query now recognizes secret cryptographic keys. |
+| Misleading indentation after control statement (`js/misleading-indentation-after-control-statement`) | Fewer results | This query temporarily ignores TypeScript files. |
+| Missing rate limiting (`js/missing-rate-limiting`) | More true-positive results, fewer false-positive results | This query now recognizes additional rate limiters and expensive route handlers. |
+| Missing X-Frame-Options HTTP header (`js/missing-x-frame-options`) | Fewer false-positive results | This query now treats header names case-insensitively. |
+| Omitted array element (`js/omitted-array-element`)| Fewer results | This query temporarily ignores TypeScript files. |
+| Reflected cross-site scripting (`js/reflected-xss`) | Fewer false-positive results | This query now treats header names case-insensitively. |
+| Semicolon insertion (`js/automatic-semicolon-insertion`) | Fewer results | This query temporarily ignores TypeScript files. |
+| Server-side URL redirect (`js/server-side-unvalidated-url-redirection`) | More true-positive results | This query now treats header names case-insensitively. |
+| Superfluous trailing arguments (`js/superfluous-trailing-arguments`) | Fewer false-positive results | This query now ignores calls to some empty functions. |
+| Type confusion through parameter tampering (`js/type-confusion-through-parameter-tampering`) | Fewer false-positive results | This query no longer flags emptiness checks. |
+| Uncontrolled command line (`js/command-line-injection`) | More true-positive results | This query now recognizes indirect command injection through `sh -c` and similar. |
+| Unused variable, import, function or class (`js/unused-local-variable`) | New name, fewer results | The "Unused variable" query has been renamed to reflect the fact that it highlights different kinds of unused program elements. In addition, the query no longer highlights class expressions that could be made anonymous. While technically true, these results are not interesting. |
+| Use of incompletely initialized object (`js/incomplete-object-initialization`) | Fewer results | This query now highlights the constructor instead of its erroneous `this` or `super` expressions. |
+| Useless conditional (`js/trivial-conditional`) | Fewer results | This query no longer flags uses of boolean return values and highlights fewer comparisons involving parameters. |
 
 ## Changes to QL libraries
 

From 4d512a5b010897eacb15a2edf1029c4b6edb74e0 Mon Sep 17 00:00:00 2001
From: Felicity Chapman <felicity@semmle.com>
Date: Tue, 11 Sep 2018 22:54:37 +0100
Subject: [PATCH 27/28] Remove non-LGTM query (see following PR)

---
 change-notes/1.18/analysis-javascript.md | 1 -
 1 file changed, 1 deletion(-)

diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md
index 1426db55c916..4be9e6dbf9e7 100644
--- a/change-notes/1.18/analysis-javascript.md
+++ b/change-notes/1.18/analysis-javascript.md
@@ -119,7 +119,6 @@
 | Insecure randomness (`js/insecure-randomness`) | More true-positive results | This query now recognizes secret cryptographic keys. |
 | Misleading indentation after control statement (`js/misleading-indentation-after-control-statement`) | Fewer results | This query temporarily ignores TypeScript files. |
 | Missing rate limiting (`js/missing-rate-limiting`) | More true-positive results, fewer false-positive results | This query now recognizes additional rate limiters and expensive route handlers. |
-| Missing X-Frame-Options HTTP header (`js/missing-x-frame-options`) | Fewer false-positive results | This query now treats header names case-insensitively. |
 | Omitted array element (`js/omitted-array-element`)| Fewer results | This query temporarily ignores TypeScript files. |
 | Reflected cross-site scripting (`js/reflected-xss`) | Fewer false-positive results | This query now treats header names case-insensitively. |
 | Semicolon insertion (`js/automatic-semicolon-insertion`) | Fewer results | This query temporarily ignores TypeScript files. |

From 9fb5fbd995028334a815f47e68ed072effac9f2d Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jonas@semmle.com>
Date: Wed, 12 Sep 2018 09:29:56 +0200
Subject: [PATCH 28/28] C++: Restructure UnsafeUseOfStrcat for performance

This query gets optimized badly, and it has started timing out when we
run it on our own code base. Most of the evaluation time is spent in an
RA predicate named `#select#cpe#1#f#antijoin_rhs#1`, which takes 1m36s a
Wireshark snapshot.

This restructuring of the code makes the problematic RA predicate go
away.
---
 .../Memory Management/UnsafeUseOfStrcat.ql    | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/cpp/ql/src/Likely Bugs/Memory Management/UnsafeUseOfStrcat.ql b/cpp/ql/src/Likely Bugs/Memory Management/UnsafeUseOfStrcat.ql
index b3ab5828094d..51302b710f2c 100644
--- a/cpp/ql/src/Likely Bugs/Memory Management/UnsafeUseOfStrcat.ql	
+++ b/cpp/ql/src/Likely Bugs/Memory Management/UnsafeUseOfStrcat.ql	
@@ -29,11 +29,20 @@ predicate isEffectivelyConstAccess(VariableAccess a)
   )
 }
 
-from FunctionCall fc, VariableAccess src
-where fc.getTarget().hasName("strcat") and
-      src = fc.getArgument(1) and
-      not src.getType() instanceof ArrayType and
+class StrcatSource extends VariableAccess {
+  FunctionCall strcat;
+
+  StrcatSource() {
+    strcat.getTarget().hasName("strcat") and
+    this = strcat.getArgument(1)
+  }
+
+  FunctionCall getStrcatCall() { result = strcat }
+}
+
+from StrcatSource src
+where not src.getType() instanceof ArrayType and
       not exists(BufferSizeExpr bse |
         bse.getArg().(VariableAccess).getTarget() = src.getTarget()) and
       not isEffectivelyConstAccess(src)
-select fc, "Always check the size of the source buffer when using strcat."
+select src.getStrcatCall(), "Always check the size of the source buffer when using strcat."