support arrow functions in the callbacks

javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll

@@ -17,13 +17,24 @@ string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) {
       else extraArg = ""
     ) and
     if exists(cat.getCallback())
-    then callback = ", function(" + getCallbackArgs(cat.getCallback()) + ") {...}"
+    then callback = constructCallbackString(cat.getCallback())
     else callback = ""
   |
     result = "fs.readFile" + sync + "(" + cat.getFileArgument().trim() + extraArg + callback + ")"
   )
 }
 
+string constructCallbackString(DataFlow::FunctionNode func) {
+  exists(string args | args = getCallbackArgs(func) |
+    if func.getFunction() instanceof ArrowFunctionExpr
+    then
+      if func.getFunction().getBody() instanceof Expr
+      then result = ", (" + args + ") => ..."
+      else result = ", (" + args + ") => {...}"
+    else result = ", function(" + args + ") {...}"
+  )
+}
+
 /**
  * Gets a string concatenation of the parameters to a function.
  */
@@ -185,13 +196,16 @@ module UselsesCatCandidates {
   bindingset[str]
   private string getSimplifiedStringConcat(string str) {
     // Remove an initial ""+ (e.g. in `""+file`)
-    if str.prefix(3) = "\"\"+" then
-      result = str.suffix(3)
-    // prettify `${newpath}` to just newpath
-    else if str.prefix(3) = "`${" and str.suffix(str.length() - 2) = "}`" and not str.suffix(3).matches("%{%")  then
-      result = str.prefix(str.length() - 2).suffix(3)
+    if str.prefix(3) = "\"\"+"
+    then result = str.suffix(3)
     else
-     result = str
+      // prettify `${newpath}` to just newpath
+      if
+        str.prefix(3) = "`${" and
+        str.suffix(str.length() - 2) = "}`" and
+        not str.suffix(3).matches("%{%")
+      then result = str.prefix(str.length() - 2).suffix(3)
+      else result = str
   }
 
   /**

javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected

@@ -2,7 +2,7 @@ readFile
 | uselesscat.js:10:1:10:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) |
 | uselesscat.js:12:1:14:2 | exec("c ... ut);\\n}) | fs.readFile("/proc/"+id+"/status", function(err, out) {...}) |
 | uselesscat.js:16:1:16:29 | execSyn ... uinfo') | fs.readFileSync("/proc/cpuinfo") |
-| uselesscat.js:18:1:18:26 | execSyn ... path}`) | fs.readFileSync(`${newpath}`) |
+| uselesscat.js:18:1:18:26 | execSyn ... path}`) | fs.readFileSync(newpath) |
 | uselesscat.js:32:1:32:34 | execSyn ... path}`) | fs.readFileSync(`foo/bar/${newpath}`) |
 | uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | fs.readFileSync(`foo/bar/${newpath}`, {encoding: 'utf8'})) |
 | uselesscat.js:51:9:51:31 | execSyn ... + file) | fs.readFileSync(file) |
@@ -15,6 +15,9 @@ readFile
 | uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/"+newPath+"bar", {encoding: 'utf8'})) |
 | uselesscat.js:88:1:88:35 | execSyn ...  + foo) | fs.readFileSync("/proc/cpuinfo"+foo) |
 | uselesscat.js:90:1:90:50 | execFil ... th}` ]) | fs.readFileSync(`foo/bar/${newpath}`) |
+| uselesscat.js:94:1:94:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) |
+| uselesscat.js:96:1:96:53 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) |
+| uselesscat.js:98:1:98:55 | exec("c ... h(out)) | fs.readFile("foo/bar", (err, out) => ...) |
 #select
 | False negative | uselesscat.js:54:42:54:69 | // NOT  ... lagged] |
 | False negative | uselesscat.js:84:118:84:144 | // NOT  ... lagged] |

javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js

@@ -90,3 +90,9 @@ execSync('cat /proc/cpuinfo' + foo).toString(); // NOT OK.
 execFileSync('/bin/cat', [ `foo/bar/${newpath}` ]); // NOT OK
 
 execFileSync('node', [ `foo/bar/${newpath}` ]); // OK - not a call to cat
+
+exec("cat foo/bar", function (err, out) {}); // NOT OK
+
+exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK
+
+exec("cat foo/bar", (err, out) => doSomethingWith(out)); // NOT OK