JS: be less conservative about incomplete nodes in prefix sanitizers

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

@@ -6,6 +6,27 @@
 
 import javascript
 
+/**
+ * Holds if the given data flow node is incomplete or is a string concatenation
+ * involving an incomplete operand.
+ */
+private predicate hasIncompleteSubstring(DataFlow::Node nd) {
+  nd.isIncomplete(_)
+  or
+  hasIncompleteSubstring(StringConcatenation::getAnOperand(nd))
+  or
+  hasIncompleteSubstring(nd.getAPredecessor())
+}
+
+/**
+ * Holds if the given data flow node refers is a string that ends with a slash.
+ */
+private predicate endsWithSlash(DataFlow::Node nd) {
+  nd.getStringValue().matches("%/")
+  or
+  endsWithSlash(StringConcatenation::getLastOperand(nd))
+}
+
 /**
  * Holds if the string value of `nd` prevents anything appended after it
  * from affecting the hostname or path of a URL.
@@ -18,8 +39,6 @@ private predicate hasSanitizingSubstring(DataFlow::Node nd) {
   hasSanitizingSubstring(StringConcatenation::getAnOperand(nd))
   or
   hasSanitizingSubstring(nd.getAPredecessor())
-  or
-  nd.isIncomplete(_)
 }
 
 /**
@@ -31,7 +50,15 @@ private predicate hasSanitizingSubstring(DataFlow::Node nd) {
 predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
   exists(DataFlow::Node operator, int n |
     StringConcatenation::taintStep(source, sink, operator, n) and
-    hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1]))
+    (
+      hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1]))
+      or
+      // If prefixed by an unknown base URL, assume the URL is safe, unless
+      // separated by a slash, such as `${baseUrl}/${taint}`. The slash is a
+      // good indicator that the incoming value is most likely part of the path.
+      hasIncompleteSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1])) and
+      not endsWithSlash(StringConcatenation::getOperand(operator, n - 1))
+    )
   )
 }
 

javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected

@@ -18,6 +18,12 @@ nodes
 | tst.js:36:24:36:30 | tainted |
 | tst.js:37:22:37:37 | new Uri(tainted) |
 | tst.js:37:30:37:36 | tainted |
+| tst.js:41:13:41:51 | `http:/ ... inted}` |
+| tst.js:41:43:41:49 | tainted |
+| tst.js:43:13:43:54 | `http:/ ... inted}` |
+| tst.js:43:46:43:52 | tainted |
+| tst.js:45:13:45:56 | 'http:/ ... tainted |
+| tst.js:45:50:45:56 | tainted |
 edges
 | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
 | tst.js:14:9:14:52 | tainted | tst.js:20:17:20:23 | tainted |
@@ -28,6 +34,9 @@ edges
 | tst.js:14:9:14:52 | tainted | tst.js:34:34:34:40 | tainted |
 | tst.js:14:9:14:52 | tainted | tst.js:36:24:36:30 | tainted |
 | tst.js:14:9:14:52 | tainted | tst.js:37:30:37:36 | tainted |
+| tst.js:14:9:14:52 | tainted | tst.js:41:43:41:49 | tainted |
+| tst.js:14:9:14:52 | tainted | tst.js:43:46:43:52 | tainted |
+| tst.js:14:9:14:52 | tainted | tst.js:45:50:45:56 | tainted |
 | tst.js:14:19:14:42 | url.par ... , true) | tst.js:14:19:14:48 | url.par ... ).query |
 | tst.js:14:19:14:48 | url.par ... ).query | tst.js:14:19:14:52 | url.par ... ery.url |
 | tst.js:14:19:14:52 | url.par ... ery.url | tst.js:14:9:14:52 | tainted |
@@ -37,6 +46,9 @@ edges
 | tst.js:30:37:30:43 | tainted | tst.js:30:13:30:43 | "http:/ ... tainted |
 | tst.js:36:24:36:30 | tainted | tst.js:36:16:36:31 | new Uri(tainted) |
 | tst.js:37:30:37:36 | tainted | tst.js:37:22:37:37 | new Uri(tainted) |
+| tst.js:41:43:41:49 | tainted | tst.js:41:13:41:51 | `http:/ ... inted}` |
+| tst.js:43:46:43:52 | tainted | tst.js:43:13:43:54 | `http:/ ... inted}` |
+| tst.js:45:50:45:56 | tainted | tst.js:45:13:45:56 | 'http:/ ... tainted |
 #select
 | tst.js:18:5:18:20 | request(tainted) | tst.js:14:29:14:35 | req.url | tst.js:18:13:18:19 | tainted | The $@ of this request depends on $@. | tst.js:18:13:18:19 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
 | tst.js:20:5:20:24 | request.get(tainted) | tst.js:14:29:14:35 | req.url | tst.js:20:17:20:23 | tainted | The $@ of this request depends on $@. | tst.js:20:17:20:23 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
@@ -47,3 +59,6 @@ edges
 | tst.js:34:5:34:42 | http.ge ... inted}) | tst.js:14:29:14:35 | req.url | tst.js:34:34:34:40 | tainted | The $@ of this request depends on $@. | tst.js:34:34:34:40 | tainted | host | tst.js:14:29:14:35 | req.url | a user-provided value |
 | tst.js:36:5:36:32 | XhrIo.s ... inted)) | tst.js:14:29:14:35 | req.url | tst.js:36:16:36:31 | new Uri(tainted) | The $@ of this request depends on $@. | tst.js:36:16:36:31 | new Uri(tainted) | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
 | tst.js:37:5:37:38 | new Xhr ... inted)) | tst.js:14:29:14:35 | req.url | tst.js:37:22:37:37 | new Uri(tainted) | The $@ of this request depends on $@. | tst.js:37:22:37:37 | new Uri(tainted) | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
+| tst.js:41:5:41:52 | request ... nted}`) | tst.js:14:29:14:35 | req.url | tst.js:41:13:41:51 | `http:/ ... inted}` | The $@ of this request depends on $@. | tst.js:41:13:41:51 | `http:/ ... inted}` | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
+| tst.js:43:5:43:55 | request ... nted}`) | tst.js:14:29:14:35 | req.url | tst.js:43:13:43:54 | `http:/ ... inted}` | The $@ of this request depends on $@. | tst.js:43:13:43:54 | `http:/ ... inted}` | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
+| tst.js:45:5:45:57 | request ... ainted) | tst.js:14:29:14:35 | req.url | tst.js:45:13:45:56 | 'http:/ ... tainted | The $@ of this request depends on $@. | tst.js:45:13:45:56 | 'http:/ ... tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |

javascript/ql/test/query-tests/Security/CWE-918/tst.js

@@ -35,4 +35,20 @@ var server = http.createServer(function(req, res) {
 
     XhrIo.send(new Uri(tainted)); // NOT OK
     new XhrIo().send(new Uri(tainted)); // NOT OK
+
+    let base = require('./config').base;
+
+    request(`http://example.com/${base}/${tainted}`); // NOT OK
+
+    request(`http://example.com/${base}/v1/${tainted}`); // NOT OK
+
+    request('http://example.com/' + base + '/' + tainted); // NOT OK
+
+    request('http://example.com/' + base + ('/' + tainted)); // NOT OK - but not flagged
+
+    request(`http://example.com/?${base}/${tainted}`); // OK
+
+    request(`http://example.com/${base}${tainted}`); // OK - assumed safe
+
+    request(`${base}${tainted}`); // OK - assumed safe
 })