Merge pull request #1176 from xiemaisi/js/fix-socket-io-type-tracking

Approved by asger-semmle

Author: semmle-qlci

javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll

@@ -126,6 +126,7 @@ class TypeTracker extends TTypeTracker {
 
   TypeTracker() { this = MkTypeTracker(hasCall, prop) }
 
+  /** Gets the summary resulting from appending `step` to this type-tracking summary. */
   TypeTracker append(StepSummary step) {
     step = LevelStep() and result = this
     or
@@ -140,6 +141,7 @@ class TypeTracker extends TTypeTracker {
     )
   }
 
+  /** Gets a textual representation of this summary. */
   string toString() {
     exists(string withCall, string withProp |
       (if hasCall = true then withCall = "with" else withCall = "without") and
@@ -153,6 +155,9 @@ class TypeTracker extends TTypeTracker {
    */
   predicate start() { hasCall = false and prop = "" }
 
+  /**
+   * Holds if this is the end point of type tracking.
+   */
   predicate end() { prop = "" }
 
   /**
@@ -162,7 +167,13 @@ class TypeTracker extends TTypeTracker {
    */
   boolean hasCall() { result = hasCall }
 
-  string getProp() { result = prop }
+  /**
+   * Gets a type tracker that starts where this one has left off to allow continued
+   * tracking.
+   *
+   * This predicate is only defined if the type has not been tracked into a property.
+   */
+  TypeTracker continue() { prop = "" and result = this }
 }
 
 module TypeTracker {
@@ -206,6 +217,7 @@ class TypeBackTracker extends TTypeBackTracker {
 
   TypeBackTracker() { this = MkTypeBackTracker(hasReturn, prop) }
 
+  /** Gets the summary resulting from prepending `step` to this type-tracking summary. */
   TypeBackTracker prepend(StepSummary step) {
     step = LevelStep() and result = this
     or
@@ -220,6 +232,7 @@ class TypeBackTracker extends TTypeBackTracker {
     step = StoreStep(prop) and result = MkTypeBackTracker(hasReturn, "")
   }
 
+  /** Gets a textual representation of this summary. */
   string toString() {
     exists(string withReturn, string withProp |
       (if hasReturn = true then withReturn = "with" else withReturn = "without") and
@@ -233,6 +246,9 @@ class TypeBackTracker extends TTypeBackTracker {
    */
   predicate start() { hasReturn = false and prop = "" }
 
+  /**
+   * Holds if this is the end point of type tracking.
+   */
   predicate end() { prop = "" }
 
   /**
@@ -242,7 +258,13 @@ class TypeBackTracker extends TTypeBackTracker {
    */
   boolean hasReturn() { result = hasReturn }
 
-  string getProp() { result = prop }
+  /**
+   * Gets a type tracker that starts where this one has left off to allow continued
+   * tracking.
+   *
+   * This predicate is only defined if the type has not been tracked into a property.
+   */
+  TypeBackTracker continue() { prop = "" and result = this }
 }
 
 module TypeBackTracker {

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

@@ -51,7 +51,7 @@ module SocketIO {
         // exclude getter versions
         exists(mcn.getAnArgument()) and
         result = mcn and
-        t = t2
+        t = t2.continue()
       )
     )
   }
@@ -110,7 +110,7 @@ module SocketIO {
       or
       // invocation of a chainable method
       result = pred.getAMethodCall(namespaceChainableMethod()) and
-      t = t2
+      t = t2.continue()
       or
       // invocation of chainable getter method
       exists(string m |
@@ -119,7 +119,7 @@ module SocketIO {
         m = "volatile"
       |
         result = pred.getAPropertyRead(m) and
-        t = t2
+        t = t2.continue()
       )
     )
   }
@@ -171,7 +171,7 @@ module SocketIO {
         m = EventEmitter::chainableMethod()
       |
         result = pred.getAMethodCall(m) and
-        t = t2
+        t = t2.continue()
       )
       or
       // invocation of a chainable getter method
@@ -182,7 +182,7 @@ module SocketIO {
         m = "volatile"
       |
         result = pred.getAPropertyRead(m) and
-        t = t2
+        t = t2.continue()
       )
     )
   }

javascript/ql/test/library-tests/frameworks/SocketIO/tests.expected

@@ -149,6 +149,7 @@ test_ServerNode
 | tst.js:15:1:15:15 | io.attach(http) | tst.js:1:12:1:33 | socket.io server |
 | tst.js:16:1:16:15 | io.bind(engine) | tst.js:1:12:1:33 | socket.io server |
 | tst.js:17:1:17:23 | io.onco ... socket) | tst.js:1:12:1:33 | socket.io server |
+| tst.js:79:1:79:10 | obj.server | tst.js:1:12:1:33 | socket.io server |
 test_ClientSendNode_getAReceiver
 | client2.js:14:1:14:32 | sock.em ... there") | tst.js:72:3:72:43 | socket. ...  => {}) |
 | client2.js:16:1:16:36 | sock.wr ...  => {}) | tst.js:70:3:70:35 | socket. ...  => {}) |

javascript/ql/test/library-tests/frameworks/SocketIO/tst.js

@@ -71,3 +71,11 @@ ns.on('connection', (socket) => {
   socket.once('message', (data1, data2) => {});
   socket.addListener(eventName(), () => {});
 });
+
+var obj = {
+  server: io,
+  serveClient: function() { return null; }
+};
+obj.server;                    // SocketIO::ServerNode
+obj.serveClient(false);        // not a SocketIO::ServerNode
+obj.serveClient(false).server; // not a SocketIO::ServerNode