changes based on review feedback.

Author: erik-krogh

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

@@ -73,7 +73,7 @@ module Electron {
   /**
    * A reference to the `webContents` property of a browser object.
    */
-  class WebContents extends DataFlow::SourceNode, EventEmitter::EventEmitterRange::NodeJSEventEmitter {
+  class WebContents extends DataFlow::SourceNode, EventEmitter::NodeJSEventEmitter {
     WebContents() { this.(DataFlow::PropRead).accesses(any(BrowserObject bo), "webContents") }
   }
 
@@ -89,9 +89,9 @@ module Electron {
     /**
      * A model for the Main and Renderer process in an Electron app.
      */
-    abstract class Process extends EventEmitter::EventEmitterRange::Range {
+    abstract class Process extends EventEmitter::Range {
       /**
-       * Type tracking on a process. The type tracking tracks through chainable methods.
+       * Gets a node that refers to a Process object.
        */
       DataFlow::SourceNode ref() { result = EventEmitter::trackEventEmitter(this) }
     }
@@ -128,7 +128,7 @@ module Electron {
      * Does mostly the same as an EventEmitter event handler,
      * except that values can be returned through the `event.returnValue` property.
      */
-    class IPCSendRegistration extends EventEmitter::EventRegistration::Range,
+    class IPCSendRegistration extends EventRegistration::Range,
       DataFlow::MethodCallNode {
       override Process emitter;
 
@@ -138,17 +138,17 @@ module Electron {
         result = this.getABoundCallbackParameter(1, 0).getAPropertyWrite("returnValue").getRhs()
       }
 
-      override predicate canReturnTo(EventEmitter::EventDispatch dispatch) {
-        dispatch.(IPCDispatch).getCalleeName() = "sendSync"
+      override IPCDispatch getAReturnDispatch() {
+        result.getCalleeName() = "sendSync"
       }
     }
 
     /**
      * A dispatch of an IPC event.
-     * An IPC event is sent from the Renderer to the Main process.
+     * An IPC event is sent from the renderer to the main process.
      * And a value can be returned through the `returnValue` property of the event (first parameter in the callback).
      */
-    class IPCDispatch extends EventEmitter::EventDispatch::Range, DataFlow::InvokeNode {
+    class IPCDispatch extends EventDispatch::Range, DataFlow::InvokeNode {
       override Process emitter;
 
       IPCDispatch() {

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

@@ -19,11 +19,21 @@ module EventEmitter {
     result = "prependOnceListener"
   }
 
+  /**
+   * Gets a node that refers to an EventEmitter object.
+   */
+  DataFlow::SourceNode trackEventEmitter(EventEmitter::Range emitter) {
+    result = trackEventEmitter(DataFlow::TypeTracker::end(), emitter)
+  }
 
-  private DataFlow::SourceNode trackEventEmitter(DataFlow::TypeTracker t, EventEmitterRange::Range emitter) {
+  private DataFlow::SourceNode trackEventEmitter(
+    DataFlow::TypeTracker t, EventEmitter::Range emitter
+  ) {
     t.start() and result = emitter
     or
-    exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | pred = trackEventEmitter(t2, emitter) |
+    exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred |
+      pred = trackEventEmitter(t2, emitter)
+    |
       result = pred.track(t2, t)
       or
       // invocation of a chainable method
@@ -38,192 +48,178 @@ module EventEmitter {
   }
 
   /**
-   * Type tracking of an EventEmitter. Types are tracked through the chainable methods in the NodeJS eventEmitter.
+   * An object that implements the EventEmitter API.
+   * Extending this class does nothing, its mostly to indicate intent.
+   * The magic only happens when extending EventRegistration::Range and EventDispatch::Range.
    */
-  DataFlow::SourceNode trackEventEmitter(EventEmitterRange::Range emitter) {
-    result = trackEventEmitter(DataFlow::TypeTracker::end(), emitter)
-  }
+  abstract class Range extends DataFlow::Node { }
 
   /**
-   * An EventEmitter instance that implements the NodeJS EventEmitter API. 
-   * Extend EventEmitter::Range to mark something as being an EventEmitter. 
+   * An NodeJS EventEmitter instance.
+   * Events dispatched on this EventEmitter will be handled by event handlers registered on this EventEmitter.
+   * (That is opposed to e.g. SocketIO, which implements the same interface, but where events cross object boundaries).
    */
-  class EventEmitter extends DataFlow::Node {
-    EventEmitterRange::Range range;
-
-    EventEmitter() { this = range }
-  }
-
-  module EventEmitterRange {
+  abstract class NodeJSEventEmitter extends Range {
     /**
-     * An object that implements the EventEmitter API.
-     * Extending this class does nothing, its mostly to indicate intent.
-     * The magic only happens when extending EventRegistration::Range and EventDispatch::Range.
+     * Get a Node that refers to a NodeJS EventEmitter instance.
      */
-    abstract class Range extends DataFlow::Node {}
-
-    /**
-     * An NodeJS EventEmitter instance.
-     * Events dispatched on this EventEmitter will be handled by event handlers registered on this EventEmitter.
-     * (That is opposed to e.g. SocketIO, which implements the same interface, but where events cross object boundaries).
-     */
-    abstract class NodeJSEventEmitter extends Range {
-      DataFlow::SourceNode ref() { result = trackEventEmitter(this) }
-    }
+    DataFlow::SourceNode ref() { result = trackEventEmitter(this) }
+  }
 
-    private class ImportedNodeJSEventEmitter extends NodeJSEventEmitter {
-      ImportedNodeJSEventEmitter() {
-        exists(DataFlow::SourceNode clazz |
-          clazz = DataFlow::moduleImport("events") or
-          clazz = DataFlow::moduleMember("events", "EventEmitter")
-        |
-          this = clazz.getAnInstantiation()
-        )
-      }
+  private class ImportedNodeJSEventEmitter extends NodeJSEventEmitter {
+    ImportedNodeJSEventEmitter() {
+      exists(DataFlow::SourceNode clazz |
+        clazz = DataFlow::moduleImport("events") or
+        clazz = DataFlow::moduleMember("events", "EventEmitter")
+      |
+        this = clazz.getAnInstantiation()
+      )
     }
   }
+}
 
-  /**
-   * A registration of an event handler on an EventEmitter.
-   */
-  class EventRegistration extends DataFlow::Node {
-    EventRegistration::Range range;
+/**
+ * An EventEmitter instance that implements the NodeJS EventEmitter API.
+ * Extend EventEmitter::Range to mark something as being an EventEmitter.
+ */
+class EventEmitter extends DataFlow::Node {
+  EventEmitter::Range range;
 
-    EventRegistration() { this = range }
+  EventEmitter() { this = range }
+}
 
-    /** Gets the EventEmitter that the event handler is registered on. */
-    final EventEmitter getEmitter() { result = range.getEmitter() }
+/**
+ * A registration of an event handler on an EventEmitter.
+ */
+class EventRegistration extends DataFlow::Node {
+  EventRegistration::Range range;
 
-    /** Gets the name of the channel if possible. */
-    string getChannel() { result = range.getChannel() }
+  EventRegistration() { this = range }
 
-    /** Gets the `i`th parameter in the event handler. */
-    DataFlow::Node getReceivedItem(int i) { result = range.getReceivedItem(i) }
+  /** Gets the EventEmitter that the event handler is registered on. */
+  final EventEmitter getEmitter() { result = range.getEmitter() }
 
-    /**
-     * Gets a value that is returned by the event handler.
-     * The default implementation is that no value can be returned.
-     */
-    DataFlow::Node getAReturnedValue() { result = range.getAReturnedValue() }
+  /** Gets the name of the channel if possible. */
+  string getChannel() { result = range.getChannel() }
 
-    /**
-     * Holds if this event handler can return a value to the given `dispatch`.
-     * The default implementation is that there exists no such dispatch.
-     */
-    predicate canReturnTo(EventDispatch dispatch) { range.canReturnTo(dispatch) }
-  }
+  /** Gets the `i`th parameter in the event handler. */
+  DataFlow::Node getReceivedItem(int i) { result = range.getReceivedItem(i) }
 
-  module EventRegistration {
-    /**
-     * A registration of an event handler on an EventEmitter.
-     * The default implementation assumes that `this` is a DataFlow::InvokeNode where the
-     * first argument is a string describing which channel is registered, and the second
-     * argument is the event handler callback.
-     */
-    abstract class Range extends DataFlow::Node {
-      EventEmitterRange::Range emitter;
+  /**
+   * Gets a value that is returned by the event handler.
+   * The default implementation is that no value can be returned.
+   */
+  DataFlow::Node getAReturnedValue() { result = range.getAReturnedValue() }
 
-      final EventEmitter getEmitter() { result = emitter }
+  /**
+   * Get a dispatch that this event handler can return a value to. 
+   * The default implementation is that there exists no such dispatch.
+   */
+  EventDispatch getAReturnDispatch() { result = range.getAReturnDispatch() }
+}
 
-      string getChannel() {
-        this.(DataFlow::InvokeNode).getArgument(0).mayHaveStringValue(result)
-      }
+module EventRegistration {
+  /**
+   * A registration of an event handler on an EventEmitter.
+   * The default implementation assumes that `this` is a DataFlow::InvokeNode where the
+   * first argument is a string describing which channel is registered, and the second
+   * argument is the event handler callback.
+   */
+  abstract class Range extends DataFlow::Node {
+    EventEmitter::Range emitter;
 
-      DataFlow::Node getReceivedItem(int i) {
-        result = this.(DataFlow::InvokeNode).getABoundCallbackParameter(1, i)
-      }
+    final EventEmitter getEmitter() { result = emitter }
 
-      DataFlow::Node getAReturnedValue() { none() }
+    string getChannel() { this.(DataFlow::InvokeNode).getArgument(0).mayHaveStringValue(result) }
 
-      predicate canReturnTo(EventDispatch dispatch) { none() }
+    DataFlow::Node getReceivedItem(int i) {
+      result = this.(DataFlow::InvokeNode).getABoundCallbackParameter(1, i)
     }
 
-    private class NodeJSEventRegistration extends Range, DataFlow::MethodCallNode {
-      override EventEmitterRange::NodeJSEventEmitter emitter;
+    DataFlow::Node getAReturnedValue() { none() }
 
-      NodeJSEventRegistration() { this = emitter.ref().getAMethodCall(EventEmitter::on()) }
-    }
+    EventDispatch::Range getAReturnDispatch() { none() }
   }
 
-  /**
-   * A dispatch of an event on an EventEmitter.
-   */
-  class EventDispatch extends DataFlow::Node {
-    EventDispatch::Range range;
+  private class NodeJSEventRegistration extends Range, DataFlow::MethodCallNode {
+    override EventEmitter::NodeJSEventEmitter emitter;
 
-    EventDispatch() { this = range }
+    NodeJSEventRegistration() { this = emitter.ref().getAMethodCall(EventEmitter::on()) }
+  }
+}
 
-    /** Gets the emitter that the event dispatch happens on. */
-    EventEmitter getEmitter() { result = range.getEmitter() }
+/**
+ * A dispatch of an event on an EventEmitter.
+ */
+class EventDispatch extends DataFlow::Node {
+  EventDispatch::Range range;
 
-    /** Gets the name of the channel if possible. */
-    string getChannel() { result = range.getChannel() }
+  EventDispatch() { this = range }
 
-    /** Gets the `i`th argument that is send to the event handler. */
-    DataFlow::Node getSentItem(int i) { result = range.getSentItem(i) }
+  /** Gets the emitter that the event dispatch happens on. */
+  EventEmitter getEmitter() { result = range.getEmitter() }
 
-    /**
-     * Get an EventRegistration that this event dispatch can send an event to.
-     * The default implementation is that the emitters of the dispatch and registration have to be equal.
-     * Channels are by default ignored.
-     */
-    EventRegistration getAReceiver() { result = range.getAReceiver() }
-  }
+  /** Gets the name of the channel if possible. */
+  string getChannel() { result = range.getChannel() }
 
-  module EventDispatch {
-    /**
-     * A dispatch of an event on an EventEmitter.
-     * The default implementation assumes that the dispatch is a DataFlow::InvokeNode,
-     * where the first argument is a string describing the channel, and the `i`+1 argument
-     * is the `i`th item sent to the event handler.
-     */
-    abstract class Range extends DataFlow::Node {
-      EventEmitterRange::Range emitter;
+  /** Gets the `i`th argument that is send to the event handler. */
+  DataFlow::Node getSentItem(int i) { result = range.getSentItem(i) }
 
-      final EventEmitter getEmitter() { result = emitter }
+  /**
+   * Get an EventRegistration that this event dispatch can send an event to.
+   * The default implementation is that the emitters of the dispatch and registration have to be equal.
+   * Channels are by default ignored.
+   */
+  EventRegistration getAReceiver() { result = range.getAReceiver() }
+}
 
-      string getChannel() {
-        this.(DataFlow::InvokeNode).getArgument(0).mayHaveStringValue(result)
-      }
+module EventDispatch {
+  /**
+   * A dispatch of an event on an EventEmitter.
+   * The default implementation assumes that the dispatch is a DataFlow::InvokeNode,
+   * where the first argument is a string describing the channel, and the `i`+1 argument
+   * is the `i`th item sent to the event handler.
+   */
+  abstract class Range extends DataFlow::Node {
+    EventEmitter::Range emitter;
 
-      DataFlow::Node getSentItem(int i) {
-        result = this.(DataFlow::InvokeNode).getArgument(i + 1)
-      }
+    final EventEmitter getEmitter() { result = emitter }
 
-      EventRegistration::Range getAReceiver() {
-        this.getEmitter() = result.getEmitter()
-      }
-    }
+    string getChannel() { this.(DataFlow::InvokeNode).getArgument(0).mayHaveStringValue(result) }
 
-    private class NodeJSEventDispatch extends Range, DataFlow::MethodCallNode {
-      override EventEmitterRange::NodeJSEventEmitter emitter;
+    DataFlow::Node getSentItem(int i) { result = this.(DataFlow::InvokeNode).getArgument(i + 1) }
 
-      NodeJSEventDispatch() { this = emitter.ref().getAMethodCall("emit") }
-    }
+    EventRegistration::Range getAReceiver() { this.getEmitter() = result.getEmitter() }
   }
 
-  /**
-   * A taint-step that models data-flow between event handlers and event dispatchers.
-   */
-  private class EventEmitterTaintStep extends DataFlow::AdditionalFlowStep {
-    EventRegistration reg;
-    EventDispatch dispatch;
-
-    EventEmitterTaintStep() {
-      this = dispatch and
-      reg = dispatch.getAReceiver() and
-      not dispatch.getChannel() != reg.getChannel()
-    }
+  private class NodeJSEventDispatch extends Range, DataFlow::MethodCallNode {
+    override EventEmitter::NodeJSEventEmitter emitter;
 
-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      exists(int i | i >= 0 |
-        pred = dispatch.getSentItem(i) and
-        succ = reg.getReceivedItem(i)
-      )
-      or
-      reg.canReturnTo(dispatch) and
-      pred = reg.getAReturnedValue() and
-      succ = dispatch
-    }
+    NodeJSEventDispatch() { this = emitter.ref().getAMethodCall("emit") }
+  }
+}
+
+/**
+ * A taint-step that models data-flow between event handlers and event dispatchers.
+ */
+private class EventEmitterTaintStep extends DataFlow::AdditionalFlowStep {
+  EventRegistration reg;
+  EventDispatch dispatch;
+
+  EventEmitterTaintStep() {
+    this = dispatch and
+    reg = dispatch.getAReceiver() and
+    not dispatch.getChannel() != reg.getChannel()
+  }
+
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(int i | i >= 0 |
+      pred = dispatch.getSentItem(i) and
+      succ = reg.getReceivedItem(i)
+    )
+    or
+    dispatch = reg.getAReturnDispatch() and
+    pred = reg.getAReturnedValue() and
+    succ = dispatch
   }
 }