Eliminate false positives by detecting non-stream objects returned from pipe() calls based on accessed properties

Author: Napalys

javascript/ql/src/Quality/UnhandledStreamPipe.ql

@@ -48,6 +48,21 @@ string getNonchainableStreamMethodName() {
   result = ["read", "write", "end", "pipe", "unshift", "push", "isPaused", "wrap", "emit"]
 }
 
+/**
+ * Gets the property names commonly found on Node.js streams.
+ */
+string getStreamPropertyName() {
+  result =
+    [
+      "readable", "writable", "destroyed", "closed", "readableHighWaterMark", "readableLength",
+      "readableObjectMode", "readableEncoding", "readableFlowing", "readableEnded", "flowing",
+      "writableHighWaterMark", "writableLength", "writableObjectMode", "writableFinished",
+      "writableCorked", "writableEnded", "defaultEncoding", "allowHalfOpen", "objectMode",
+      "errored", "pending", "autoDestroy", "encoding", "path", "fd", "bytesRead", "bytesWritten",
+      "_readableState", "_writableState"
+    ]
+}
+
 /**
  * Gets all method names commonly found on Node.js streams.
  */
@@ -109,6 +124,25 @@ predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) {
   )
 }
 
+/**
+ * Holds if the pipe call result is used to access a property that is not typical of streams.
+ */
+predicate isPipeFollowedByNonStreamProperty(PipeCall pipeCall) {
+  exists(DataFlow::PropRef propRef |
+    propRef = pipeResultRef(pipeCall).getAPropertyRead() and
+    not propRef.getPropertyName() = getStreamPropertyName()
+  )
+}
+
+/**
+ * Holds if the pipe call result is used in a non-stream-like way,
+ * either by calling non-stream methods or accessing non-stream properties.
+ */
+predicate isPipeFollowedByNonStreamAccess(PipeCall pipeCall) {
+  isPipeFollowedByNonStreamMethod(pipeCall) or
+  isPipeFollowedByNonStreamProperty(pipeCall)
+}
+
 /**
  * Gets a reference to a stream that may be the source of the given pipe call.
  * Uses type back-tracking to trace stream references in the data flow.
@@ -145,6 +179,6 @@ predicate hasErrorHandlerRegistered(PipeCall pipeCall) {
 from PipeCall pipeCall
 where
   not hasErrorHandlerRegistered(pipeCall) and
-  not isPipeFollowedByNonStreamMethod(pipeCall)
+  not isPipeFollowedByNonStreamAccess(pipeCall)
 select pipeCall,
   "Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped."

javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected

@@ -11,18 +11,11 @@
 | test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:171:17:171:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:175:17:175:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:190:17:190:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:195:17:195:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:199:5:199:22 | notStream.pipe({}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:203:5:203:26 | notStre ... ()=>{}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:207:5:207:31 | getStre ... mber()) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:207:5:207:42 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:207:5:207:53 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:207:5:207:64 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:212:5:212:23 | getStream().pipe(p) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:212:5:212:34 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:212:5:212:45 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:212:5:212:56 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |

javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js

@@ -168,7 +168,7 @@ function test() {
   }
   { // Member access on a non-stream after pipe
     const notStream = getNotAStream();
-    const val = notStream.pipe(writable).someMember; // $SPURIOUS:Alert
+    const val = notStream.pipe(writable).someMember;
   }
   { // Member access on a stream after pipe
     const notStream = getNotAStream();