JS: Fix barrier guards for ServerSideUrlRedirect

The barrier guards for ServerSideUrlRedirect were lost when it was ported to ConfigSig, and the aforementioned spurious alert was a result of that.

The query had two guards: a proper barrier guard and a heuristic one for functions named 'isLocalURL'. We should move away from the heuristic name-based sanitiser guards, so I'm only reinstating the proper barrier guard.

Therefore updating the test to test the real barrier guard.

Author: asgerf

javascript/ql/lib/semmle/javascript/security/dataflow/ServerSideUrlRedirectQuery.qll

@@ -20,7 +20,11 @@ module ServerSideUrlRedirectConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof Sanitizer
+    or
+    node = HostnameSanitizerGuard::getABarrierNode()
+  }
 
   predicate isBarrierOut(DataFlow::Node node) { hostnameSanitizingPrefixEdge(node, _) }
 
@@ -69,10 +73,12 @@ deprecated class Configuration extends TaintTracking::Configuration {
 }
 
 /**
+ * DEPRECATED. This is no longer used as a sanitizer guard.
+ *
  * A call to a function called `isLocalUrl` or similar, which is
  * considered to sanitize a variable for purposes of URL redirection.
  */
-class LocalUrlSanitizingGuard extends DataFlow::CallNode {
+deprecated class LocalUrlSanitizingGuard extends DataFlow::CallNode {
   LocalUrlSanitizingGuard() { this.getCalleeName().regexpMatch("(?i)(is_?)?local_?url") }
 
   /** DEPRECATED. Use `blocksExpr` instead. */

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js

@@ -23,9 +23,9 @@ app.get('/some/other/path2', function(req, res) {
 
 app.get('/some/path', function(req, res) {
   var target = req.param("target");
-  if (isLocalURL(target))
+  if (target.startsWith("http://example.com/"))
     // OK - request parameter is sanitized before incorporating it into the redirect
-    res.redirect(target); // $ SPURIOUS: Alert
+    res.redirect(target);
   else
     res.redirect(target); // $ Alert - sanitization doesn't apply here
   res.redirect(target); // $ Alert - sanitization doesn't apply here