refactor: make the protocol implementation stricter

This commit handles several edge cases that were silently ignored
before:

- receiving several CONNECT packets during a session
- receiving any packet without CONNECT packet first

Author: darrachequesne

lib/client.ts

@@ -248,6 +248,7 @@ export class Client<
     try {
       this.decoder.add(data);
     } catch (e) {
+      debug("invalid packet format");
       this.onerror(e);
     }
   }
@@ -258,22 +259,31 @@ export class Client<
    * @private
    */
   private ondecoded(packet: Packet): void {
-    if (PacketType.CONNECT === packet.type) {
-      if (this.conn.protocol === 3) {
-        const parsed = url.parse(packet.nsp, true);
-        this.connect(parsed.pathname!, parsed.query);
-      } else {
-        this.connect(packet.nsp, packet.data);
-      }
+    let namespace: string;
+    let authPayload;
+    if (this.conn.protocol === 3) {
+      const parsed = url.parse(packet.nsp, true);
+      namespace = parsed.pathname!;
+      authPayload = parsed.query;
     } else {
-      const socket = this.nsps.get(packet.nsp);
-      if (socket) {
-        process.nextTick(function () {
-          socket._onpacket(packet);
-        });
-      } else {
-        debug("no socket for namespace %s", packet.nsp);
-      }
+      namespace = packet.nsp;
+      authPayload = packet.data;
+    }
+    const socket = this.nsps.get(namespace);
+
+    if (!socket && packet.type === PacketType.CONNECT) {
+      this.connect(namespace, authPayload);
+    } else if (
+      socket &&
+      packet.type !== PacketType.CONNECT &&
+      packet.type !== PacketType.CONNECT_ERROR
+    ) {
+      process.nextTick(function () {
+        socket._onpacket(packet);
+      });
+    } else {
+      debug("invalid state (packet type: %s)", packet.type);
+      this.close();
     }
   }
 

lib/socket.ts

@@ -407,9 +407,6 @@ export class Socket<
       case PacketType.DISCONNECT:
         this.ondisconnect();
         break;
-
-      case PacketType.CONNECT_ERROR:
-        this._onerror(new Error(packet.data));
     }
   }
 

test/socket.io.ts

@@ -47,6 +47,44 @@ const getPort = (io: Server): number => {
   return io.httpServer.address().port;
 };
 
+// TODO: update superagent as latest release now supports promises
+const eioHandshake = (httpServer): Promise<string> => {
+  return new Promise((resolve) => {
+    request(httpServer)
+      .get("/socket.io/")
+      .query({ transport: "polling", EIO: 4 })
+      .end((err, res) => {
+        const sid = JSON.parse(res.text.substring(1)).sid;
+        resolve(sid);
+      });
+  });
+};
+
+const eioPush = (httpServer, sid: string, body: string): Promise<void> => {
+  return new Promise((resolve) => {
+    request(httpServer)
+      .post("/socket.io/")
+      .send(body)
+      .query({ transport: "polling", EIO: 4, sid })
+      .expect(200)
+      .end(() => {
+        resolve();
+      });
+  });
+};
+
+const eioPoll = (httpServer, sid): Promise<string> => {
+  return new Promise((resolve) => {
+    request(httpServer)
+      .get("/socket.io/")
+      .query({ transport: "polling", EIO: 4, sid })
+      .expect(200)
+      .end((err, res) => {
+        resolve(res.text);
+      });
+  });
+};
+
 describe("socket.io", () => {
   it("should be the same version as client", () => {
     const version = require("../package").version;
@@ -378,6 +416,66 @@ describe("socket.io", () => {
         exec(fixture("server-close.ts"), done);
       });
     });
+
+    describe("protocol violations", () => {
+      it("should close the connection when receiving several CONNECT packets", async () => {
+        const httpServer = createServer();
+        const io = new Server(httpServer);
+
+        httpServer.listen(0);
+
+        const sid = await eioHandshake(httpServer);
+        // send a first CONNECT packet
+        await eioPush(httpServer, sid, "40");
+        // send another CONNECT packet
+        await eioPush(httpServer, sid, "40");
+        // session is cleanly closed (not discarded, see 'client.close()')
+        // first, we receive the Socket.IO handshake response
+        await eioPoll(httpServer, sid);
+        // then a close packet
+        const body = await eioPoll(httpServer, sid);
+        expect(body).to.be("6\u001e1");
+
+        io.close();
+      });
+
+      it("should close the connection when receiving an EVENT packet while not connected", async () => {
+        const httpServer = createServer();
+        const io = new Server(httpServer);
+
+        httpServer.listen(0);
+
+        const sid = await eioHandshake(httpServer);
+        // send an EVENT packet
+        await eioPush(httpServer, sid, '42["some event"]');
+        // session is cleanly closed, we receive a close packet
+        const body = await eioPoll(httpServer, sid);
+        expect(body).to.be("6\u001e1");
+
+        io.close();
+      });
+
+      it("should close the connection when receiving an invalid packet", async () => {
+        const httpServer = createServer();
+        const io = new Server(httpServer);
+
+        httpServer.listen(0);
+
+        const sid = await eioHandshake(httpServer);
+        // send a CONNECT packet
+        await eioPush(httpServer, sid, "40");
+        // send an invalid packet
+        await eioPush(httpServer, sid, "4abc");
+        // session is cleanly closed (not discarded, see 'client.close()')
+        // first, we receive the Socket.IO handshake response
+        await eioPoll(httpServer, sid);
+        // then a close packet
+        const body = await eioPoll(httpServer, sid);
+        expect(body).to.be("6\u001e1");
+
+        io.close();
+      });
+    });
   });
 
   describe("namespaces", () => {