JS: restructure FileSystemWriteAccess/FileSystemReadAccess API

javascript/ql/src/semmle/javascript/Concepts.qll

@@ -29,19 +29,27 @@ abstract class FileSystemAccess extends DataFlow::Node {
   /** Gets an argument to this file system access that is interpreted as a path. */
   abstract DataFlow::Node getAPathArgument();
   
-  /** Gets a node that represents file system access data, such as buffer the data is copied to. */
-  abstract DataFlow::Node getDataNode();
 }
 
 /**
- * A data flow node that performs read file system access.
+ * A data flow node that reads data from the file system.
  */
-abstract class FileSystemReadAccess extends FileSystemAccess { }
+abstract class FileSystemReadAccess extends FileSystemAccess {
+
+  /** Gets a node that represents data from the file system. */
+  abstract DataFlow::Node getADataNode();
+
+}
 
 /**
- * A data flow node that performs write file system access.
+ * A data flow node that writes data to the file system.
  */
-abstract class FileSystemWriteAccess extends FileSystemAccess { }
+abstract class FileSystemWriteAccess extends FileSystemAccess {
+
+  /** Gets a node that represents data to be written to the file system. */
+  abstract DataFlow::Node getADataNode();
+
+}
 
 /**
  * A data flow node that contains a file name or an array of file names from the local file system.

javascript/ql/src/semmle/javascript/frameworks/Express.qll

@@ -832,10 +832,6 @@ module Express {
         asExpr().(MethodCallExpr).calls(any(ResponseExpr res), name))
     }
 
-    override DataFlow::Node getDataNode() {
-      result = DataFlow::valueNode(astNode)
-    }
-
     override DataFlow::Node getAPathArgument() {
       result = DataFlow::valueNode(astNode.getArgument(0))
     }

javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll

@@ -416,26 +416,6 @@ module NodeJSLib {
         result = methodName
     }
 
-    override DataFlow::Node getDataNode() {
-      (
-        methodName = "readFileSync" and 
-        result = this
-      ) 
-      or
-      exists (int i, string paramName | fsDataParam(methodName, i, paramName) |
-      (
-        paramName = "callback" and
-        exists (DataFlow::ParameterNode p, string n | 
-          p = getCallback(i).getAParameter() and
-          n = p.getName().toLowerCase() and
-          result = p |
-          n = "data" or n = "buffer" or n = "string"
-        )
-      ) 
-      or
-        result = getArgument(i))
-    }
-
     override DataFlow::Node getAPathArgument() {
       exists (int i | fsFileParam(methodName, i) | 
         result = getArgument(i))
@@ -444,24 +424,57 @@ module NodeJSLib {
 
   /** Only NodeJSSystemFileAccessCalls that write data to 'fs' */
   private class NodeJSFileSystemAccessWriteCall extends FileSystemWriteAccess, NodeJSFileSystemAccessCall {
-      NodeJSFileSystemAccessWriteCall () {
-          this.getMethodName() = "appendFile" or
-          this.getMethodName() = "appendFileSync" or
-          this.getMethodName() = "write" or
-          this.getMethodName() = "writeFile" or
-          this.getMethodName() = "writeFileSync" or
-          this.getMethodName() = "writeSync"
-      }
+    NodeJSFileSystemAccessWriteCall () {
+      this.getMethodName() = "appendFile" or
+      this.getMethodName() = "appendFileSync" or
+      this.getMethodName() = "write" or
+      this.getMethodName() = "writeFile" or
+      this.getMethodName() = "writeFileSync" or
+      this.getMethodName() = "writeSync"
+    }
+
+    override DataFlow::Node getADataNode() {
+      exists (int i, string paramName |
+        fsDataParam(methodName, i, paramName) |
+        if paramName = "callback" then
+          exists (DataFlow::ParameterNode p |
+            p = getCallback(i).getAParameter() and
+            p.getName().regexpMatch("(?i)data|buffer|string") and
+            result = p
+          )
+        else
+          result = getArgument(i)
+      )
+    }
+
   }
 
   /** Only NodeJSSystemFileAccessCalls that read data from 'fs' */
   private class NodeJSFileSystemAccessReadCall extends FileSystemReadAccess, NodeJSFileSystemAccessCall {
-      NodeJSFileSystemAccessReadCall () {
-          this.getMethodName() = "read" or
-          this.getMethodName() = "readSync" or
-          this.getMethodName() = "readFile" or
-          this.getMethodName() = "readFileSync" 
-      }
+    NodeJSFileSystemAccessReadCall () {
+      this.getMethodName() = "read" or
+      this.getMethodName() = "readSync" or
+      this.getMethodName() = "readFile" or
+      this.getMethodName() = "readFileSync"
+    }
+
+    override DataFlow::Node getADataNode() {
+      if methodName.regexpMatch(".*Sync") then
+        result = this
+      else
+        exists (int i, string paramName |
+          fsDataParam(methodName, i, paramName) |
+          if paramName = "callback" then
+            exists (DataFlow::ParameterNode p |
+              p = getCallback(i).getAParameter() and
+              p.getName().regexpMatch("(?i)data|buffer|string") and
+              result = p
+            )
+          else
+            result = getArgument(i)
+        )
+    }
+
   }
   
   /**
@@ -479,7 +492,7 @@ module NodeJSLib {
         )
       }
       
-      override DataFlow::Node getDataNode() {
+      override DataFlow::Node getADataNode() {
           result = this.getArgument(0)
       } 
 
@@ -502,7 +515,7 @@ module NodeJSLib {
         )
       }
 
-      override DataFlow::Node getDataNode() {
+      override DataFlow::Node getADataNode() {
         result = this
       }
 
@@ -525,7 +538,7 @@ module NodeJSLib {
         )
       }
 
-      override DataFlow::Node getDataNode() {
+      override DataFlow::Node getADataNode() {
         result = this.getArgument(0)
       }
 
@@ -549,7 +562,7 @@ module NodeJSLib {
       )
     }
 
-    override DataFlow::Node getDataNode() {
+    override DataFlow::Node getADataNode() {
         result = this.getCallback(1).getParameter(0)
     }
     

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

@@ -57,7 +57,7 @@ module FileAccessToHttpDataFlow {
   private class FileAccessArgumentAsSource extends Source {
     FileAccessArgumentAsSource() {  
       exists(FileSystemReadAccess src |
-        this = src.getDataNode().getALocalSource()
+        this = src.getADataNode().getALocalSource()
       )
     }
   }

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

@@ -49,7 +49,7 @@ module HttpToFileAccessFlow {
   class FileAccessAsSink extends Sink {
     FileAccessAsSink () {
       exists(FileSystemWriteAccess src |
-        this = src.getDataNode()
+        this = src.getADataNode()
       )
     }
   }