modified query and added tests

Author: Naman-ntc

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

@@ -58,5 +58,12 @@ class Configuration extends TaintTracking::Configuration {
       // avoid overlapping results with unsafe dynamic method access query
       not PropertyInjection::hasUnsafeMethods(read.getBase().getALocalSource())
     )
+    or
+    exists(DataFlow::SourceNode base, DataFlow::CallNode get | get = base.getAMethodCall("get") |
+      src = get.getArgument(0) and
+      dst = get
+    ) and
+    srclabel.isTaint() and
+    dstlabel instanceof MaybeNonFunction
   }
 }

javascript/ql/src/Security/CWE-754/examples/UnvalidatedDynamicMethodCallGood.js

@@ -11,7 +11,9 @@ actions.put("pause", function pause(data) {
 
 app.get('/perform/:action/:payload', function(req, res) {
   if (actions.has(req.params.action)) {
-    let action = actions.get(req.params.action);
+    if (typeof actions.get(req.params.action) === 'function'){
+      let action = actions.get(req.params.action);
+    }
     // GOOD: `action` is either the `play` or the `pause` function from above
     res.end(action(req.params.payload));
   } else {

javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCall2.js

@@ -0,0 +1,15 @@
+var express = require('express');
+var app = express();
+
+var actions = new Map();
+actions.put("play", function play(data) {
+  // ...
+});
+actions.put("pause", function pause(data) {
+  // ...
+});
+
+app.get('/perform/:action/:payload', function(req, res) {
+  let action = actions.get(req.params.action);
+  res.end(action.get(req.params.payload)); // NOT OK 
+});

javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCall3.js

@@ -0,0 +1,17 @@
+var express = require('express');
+var app = express();
+
+var actions = new Map();
+actions.put("play", function play(data) {
+  // ...
+});
+actions.put("pause", function pause(data) {
+  // ...
+});
+
+app.get('/perform/:action/:payload', function(req, res) {
+  if (actions.has(req.params.action)){
+    let action = actions.get(req.params.action);
+    res.end(action.get(req.params.payload)); // NOT OK, but not flagged [INCONSISTENCY]
+  }
+});

javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCallGood.js

@@ -2,17 +2,20 @@ var express = require('express');
 var app = express();
 
 var actions = new Map();
-actions.put("play", function (data) {
+actions.put("play", function play(data) {
   // ...
 });
-actions.put("pause", function(data) {
+actions.put("pause", function pause(data) {
   // ...
 });
 
-app.get('/perform/:action/:payload', function(req, res) {
+app.get('/perform/:action/:payload', function (req, res) {
   if (actions.has(req.params.action)) {
-    let action = actions.get(req.params.action);
-    res.end(action(req.params.payload));
+    if (typeof actions.get(req.params.action) === 'function') {
+      let action = actions.get(req.params.action);
+      // GOOD: `action` is either the `play` or the `pause` function from above
+      res.end(action(req.params.payload));
+    }
   } else {
     res.end("Unsupported action.");
   }

javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCallGood3.js

@@ -0,0 +1,20 @@
+var express = require('express');
+var app = express();
+
+var actions = new Map();
+actions.put("play", function play(data) {
+    // ...
+});
+actions.put("pause", function pause(data) {
+    // ...
+});
+
+app.get('/perform/:action/:payload', function (req, res) {
+    if (typeof actions.get(req.params.action) === 'function') {
+        let action = actions.get(req.params.action);
+        // GOOD: `action` is either the `play` or the `pause` function from above
+        res.end(action(req.params.payload));
+    } else {
+        res.end("Unsupported action.");
+    }
+});