From 0b35dc77c07bfcf02c28643432affffbfc411038 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Thu, 31 Mar 2022 12:19:18 +0200 Subject: [PATCH] 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 --- lib/client.ts | 40 +++++++++++-------- lib/socket.ts | 3 -- test/socket.io.ts | 98 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 18 deletions(-) diff --git a/lib/client.ts b/lib/client.ts index 2db890fe..c46bc012 100644 --- a/lib/client.ts +++ b/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(); } } diff --git a/lib/socket.ts b/lib/socket.ts index 896b321b..583db00a 100644 --- a/lib/socket.ts +++ b/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)); } } diff --git a/test/socket.io.ts b/test/socket.io.ts index 10fac1cf..f595d0db 100644 --- a/test/socket.io.ts +++ b/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 => { + 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 => { + 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 => { + 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", () => {