Merge pull request github#5794 from erik-krogh/rxPipe

Approved by asgerf

Author: codeql-ci

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

@@ -23,7 +23,7 @@ private class RxJsSubscribeStep extends TaintTracking::SharedTaintStep {
  * created by the `map` call.
  */
 private DataFlow::Node pipeInput(DataFlow::CallNode pipe) {
-  pipe = DataFlow::moduleMember("rxjs/operators", ["map", "filter"]).getACall() and
+  pipe = DataFlow::moduleMember("rxjs/operators", any(string s | not s = "catchError")).getACall() and
   result = pipe.getCallback(0).getParameter(0)
 }
 
@@ -34,7 +34,8 @@ private DataFlow::Node pipeInput(DataFlow::CallNode pipe) {
  * the pipe.
  */
 private DataFlow::Node pipeOutput(DataFlow::CallNode pipe) {
-  pipe = DataFlow::moduleMember("rxjs/operators", "map").getACall() and
+  // we assume if there is a return, it is an output.
+  pipe = DataFlow::moduleMember("rxjs/operators", _).getACall() and
   result = pipe.getCallback(0).getReturnNode()
   or
   pipe = DataFlow::moduleMember("rxjs/operators", "filter").getACall() and
@@ -48,30 +49,56 @@ private DataFlow::Node pipeOutput(DataFlow::CallNode pipe) {
  * be special-cased.
  */
 private predicate isIdentityPipe(DataFlow::CallNode pipe) {
-  pipe = DataFlow::moduleMember("rxjs/operators", "catchError").getACall()
+  pipe = DataFlow::moduleMember("rxjs/operators", ["catchError", "tap"]).getACall()
+}
+
+/**
+ * A call to `pipe`, which is assumed to be an `rxjs/operators` pipe.
+ *
+ * Has utility methods `getInput`/`getOutput` to get the input/output of each
+ * element of the pipe.
+ * These utility methods automatically handle itentity pipes, and the
+ * first/last elements of the pipe.
+ */
+private class RxJSPipe extends DataFlow::MethodCallNode {
+  RxJSPipe() { this.getMethodName() = "pipe" }
+
+  /**
+   * Gets an input to pipe element `i`.
+   * Or if `i` is equal to the number of elements, gets the output of the pipe (the call itself)
+   */
+  DataFlow::Node getInput(int i) {
+    result = pipeInput(this.getArgument(i).getALocalSource())
+    or
+    i = this.getNumArgument() and
+    result = this
+  }
+
+  /**
+   * Gets an output from pipe element `i`.
+   * Handles identity pipes by getting the output from the previous element.
+   * If `i` is -1, gets the receiver to the call, which started the pipe.
+   */
+  DataFlow::Node getOutput(int i) {
+    isIdentityPipe(this.getArgument(i).getALocalSource()) and
+    result = getOutput(i - 1)
+    or
+    not isIdentityPipe(this.getArgument(i).getALocalSource()) and
+    result = pipeOutput(this.getArgument(i).getALocalSource())
+    or
+    i = -1 and
+    result = this.getReceiver()
+  }
 }
 
 /**
  * A step in or out of the map callback in a call of form `x.pipe(map(y => ...))`.
  */
 private class RxJsPipeMapStep extends TaintTracking::SharedTaintStep {
   override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(DataFlow::MethodCallNode call | call.getMethodName() = "pipe" |
-      pred = call.getReceiver() and
-      succ = pipeInput(call.getArgument(0).getALocalSource())
-      or
-      exists(int i |
-        pred = pipeOutput(call.getArgument(i).getALocalSource()) and
-        succ = pipeInput(call.getArgument(i + 1).getALocalSource())
-      )
-      or
-      pred = pipeOutput(call.getLastArgument().getALocalSource()) and
-      succ = call
-      or
-      // Handle a common case where the last step is `catchError`.
-      isIdentityPipe(call.getLastArgument().getALocalSource()) and
-      pred = pipeOutput(call.getArgument(call.getNumArgument() - 2)) and
-      succ = call
+    exists(RxJSPipe pipe, int i |
+      pred = pipe.getOutput(i) and
+      succ = pipe.getInput(i + 1)
     )
   }
 }

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -109,6 +109,11 @@ typeInferenceMismatch
 | promise.js:10:24:10:31 | source() | promise.js:10:8:10:32 | Promise ... urce()) |
 | promise.js:12:20:12:27 | source() | promise.js:13:8:13:23 | resolver.promise |
 | rxjs.js:3:1:3:8 | source() | rxjs.js:10:14:10:17 | data |
+| rxjs.js:13:1:13:8 | source() | rxjs.js:17:23:17:23 | x |
+| rxjs.js:13:1:13:8 | source() | rxjs.js:18:23:18:23 | x |
+| rxjs.js:13:1:13:8 | source() | rxjs.js:22:14:22:17 | data |
+| rxjs.js:27:24:27:32 | source(x) | rxjs.js:29:23:29:23 | x |
+| rxjs.js:27:24:27:32 | source(x) | rxjs.js:34:14:34:17 | data |
 | sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:14:10:14:14 | taint |
 | sanitizer-function.js:12:17:12:24 | source() | sanitizer-function.js:33:14:33:18 | taint |
 | sanitizer-guards.js:2:11:2:18 | source() | sanitizer-guards.js:4:8:4:8 | x |

javascript/ql/test/library-tests/TaintTracking/rxjs.js

@@ -1,4 +1,4 @@
-import { map, catchError } from 'rxjs/operators';
+import { map, tap, catchError, switchMap, filter } from 'rxjs/operators';
 
 source()
     .pipe(
@@ -9,3 +9,27 @@ source()
     .subscribe(data => {
         sink(data)
     });
+
+source()
+    .pipe(
+        map(x => x + 'foo'),
+        // `tap` taps into the source observable, so like `subscribe` but inside the pipe.
+        tap(x => sink(x)),
+        tap(x => sink(x)),
+        catchError(err => {})
+    )
+    .subscribe(data => {
+        sink(data)
+    });
+
+myIdentifier()
+    .pipe(
+        switchMap(x => source(x)),
+        filter(x => myFilter(x)),
+        tap(x => sink(x)),
+        catchError(err => {}),
+        map(x => x + 'foo')
+    )
+    .subscribe(data => {
+        sink(data)
+    });