JS: use original sanitizer for SSRF query

Author: asger-semmle

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

@@ -58,7 +58,7 @@ module ClientSideUrlRedirect {
     }
 
     override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) {
-      sanitizingPrefixEdge(source, sink)
+      hostnameSanitizingPrefixEdge(source, sink)
     }
 
     override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel f, DataFlow::FlowLabel g) {

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

@@ -43,7 +43,7 @@ module ServerSideUrlRedirect {
     }
 
     override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) {
-      sanitizingPrefixEdge(source, sink)
+      hostnameSanitizingPrefixEdge(source, sink)
     }
 
     override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {

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

@@ -6,6 +6,34 @@
 
 import javascript
 
+/**
+ * Holds if the string value of `nd` prevents anything appended after it
+ * from affecting the hostname or path of a URL.
+ *
+ * Specifically, this holds if the string contains `?` or `#`.
+ */
+private predicate hasSanitizingSubstring(DataFlow::Node nd) {
+  nd.asExpr().getStringValue().regexpMatch(".*[?#].*")
+  or
+  hasSanitizingSubstring(StringConcatenation::getAnOperand(nd))
+  or
+  hasSanitizingSubstring(nd.getAPredecessor())
+  or
+  nd.isIncomplete(_)
+}
+
+/**
+ * Holds if data that flows from `source` to `sink` cannot affect the
+ * path or earlier part of the resulting string when interpreted as a URL.
+ *
+ * This is considered as a sanitizing edge for the URL redirection queries.
+ */
+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])))
+}
+
 /**
  * Holds if the string value of `nd` prevents anything appended after it
  * from affecting the hostname of a URL.
@@ -18,24 +46,24 @@ import javascript
  * In the latter case, the additional prefix check is necessary to avoid a `/` that could be interpreted as
  * the `//` separating the (optional) scheme from the hostname.
  */
-predicate hasSanitizingSubstring(DataFlow::Node nd) {
+private predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
   nd.asExpr().getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*") 
   or
-  hasSanitizingSubstring(StringConcatenation::getAnOperand(nd))
+  hasHostnameSanitizingSubstring(StringConcatenation::getAnOperand(nd))
   or
-  hasSanitizingSubstring(nd.getAPredecessor())
+  hasHostnameSanitizingSubstring(nd.getAPredecessor())
   or
   nd.isIncomplete(_)
 }
 
 /**
  * Holds if data that flows from `source` to `sink` cannot affect the
- * hostname of the resulting string when interpreted as a URL.
+ * hostname or scheme of the resulting string when interpreted as a URL.
  *
  * This is considered as a sanitizing edge for the URL redirection queries.
  */
-predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
+predicate hostnameSanitizingPrefixEdge(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])))
-}
+}

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

@@ -3,4 +3,3 @@
 | 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 |