Fixed issue where a custom pipe method which returns non stream would be flagged by the query

Author: Napalys

javascript/ql/src/Quality/UnhandledStreamPipe.ql

@@ -41,6 +41,20 @@ string getChainableStreamMethodName() {
     ]
 }
 
+/**
+ * Gets the method names that are not chainable on Node.js streams.
+ */
+string getNonchainableStreamMethodName() {
+  result = ["read", "write", "end", "pipe", "unshift", "push", "isPaused", "wrap", "emit"]
+}
+
+/**
+ * Gets all method names commonly found on Node.js streams.
+ */
+string getStreamMethodName() {
+  result = [getChainableStreamMethodName(), getNonchainableStreamMethodName()]
+}
+
 /**
  * A call to register an event handler on a Node.js stream.
  * This includes methods like `on`, `once`, and `addListener`.
@@ -67,6 +81,34 @@ predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode)
   )
 }
 
+/**
+ * Tracks the result of a pipe call as it flows through the program.
+ */
+private DataFlow::SourceNode pipeResultTracker(DataFlow::TypeTracker t, PipeCall pipe) {
+  t.start() and result = pipe
+  or
+  exists(DataFlow::TypeTracker t2 | result = pipeResultTracker(t2, pipe).track(t2, t))
+}
+
+/**
+ * Gets a reference to the result of a pipe call.
+ */
+private DataFlow::SourceNode pipeResultRef(PipeCall pipe) {
+  result = pipeResultTracker(DataFlow::TypeTracker::end(), pipe)
+}
+
+/**
+ * Holds if the pipe call result is used to call a non-stream method.
+ * Since pipe() returns the destination stream, this finds cases where
+ * the destination stream is used with methods not typical of streams.
+ */
+predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) {
+  exists(DataFlow::MethodCallNode call |
+    call = pipeResultRef(pipeCall).getAMethodCall() and
+    not call.getMethodName() = getStreamMethodName()
+  )
+}
+
 /**
  * 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.
@@ -101,6 +143,8 @@ predicate hasErrorHandlerRegistered(PipeCall pipeCall) {
 }
 
 from PipeCall pipeCall
-where not hasErrorHandlerRegistered(pipeCall)
+where
+  not hasErrorHandlerRegistered(pipeCall) and
+  not isPipeFollowedByNonStreamMethod(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

@@ -9,8 +9,5 @@
 | 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:147:5:147:28 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:151:20:151:43 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
-| test.js:157:47:157:74 | someVar ... ething) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:163:5:163:20 | notStream.pipe() | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
 | test.js:167:5:167:36 | notStre ... , arg3) | 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

@@ -144,17 +144,17 @@ function test() {
   }
   { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method)
     const notStream = getNotAStream();
-    notStream.pipe(writable).subscribe(); // $SPURIOUS:Alert
+    notStream.pipe(writable).subscribe();
   }
   { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method)
     const notStream = getNotAStream();
-    const result = notStream.pipe(writable); // $SPURIOUS:Alert
+    const result = notStream.pipe(writable);
     const dealWithResult = (result) => { result.subscribe(); };
     dealWithResult(result); 
   }
   { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method)
     const notStream = getNotAStream();
-    const pipeIt = (someVariable) => { return someVariable.pipe(something); }; // $SPURIOUS:Alert
+    const pipeIt = (someVariable) => { return someVariable.pipe(something); };
     let x = pipeIt(notStream);
     x.subscribe(); 
   }