From 7f518590d99e3fb2a4bbc6ac3c7c7692a796ecfb Mon Sep 17 00:00:00 2001 From: Loonride Date: Mon, 1 Jul 2019 10:12:23 -0500 Subject: [PATCH] fix(server): fix header check for socket server (#2077) * fix(server): fix header check for socket server * test(server): tests to check header error and server methods --- lib/Server.js | 11 +-- lib/servers/SockJSServer.js | 10 +- lib/servers/WebsocketServer.js | 8 +- .../serverMode-option.test.js.snap | 9 ++ test/server/serverMode-option.test.js | 92 ++++++++++++++++++- test/server/servers/SockJSServer.test.js | 28 +++++- test/server/servers/WebsocketServer.test.js | 28 +++++- .../__snapshots__/SockJSServer.test.js.snap | 4 +- .../WebsocketServer.test.js.snap | 4 +- 9 files changed, 177 insertions(+), 17 deletions(-) create mode 100644 test/server/__snapshots__/serverMode-option.test.js.snap diff --git a/lib/Server.js b/lib/Server.js index 28986dfdaa..597a4c61ce 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -677,25 +677,22 @@ class Server { const SocketServerImplementation = this.socketServerImplementation; this.socketServer = new SocketServerImplementation(this); - this.socketServer.onConnection((connection) => { + this.socketServer.onConnection((connection, headers) => { if (!connection) { return; } - if ( - !this.checkHost(connection.headers) || - !this.checkOrigin(connection.headers) - ) { + if (headers && (!this.checkHost(headers) || !this.checkOrigin(headers))) { this.sockWrite([connection], 'error', 'Invalid Host/Origin header'); - connection.close(); + this.socketServer.close(connection); return; } this.sockets.push(connection); - connection.on('close', () => { + this.socketServer.onConnectionClose(connection, () => { const idx = this.sockets.indexOf(connection); if (idx >= 0) { diff --git a/lib/servers/SockJSServer.js b/lib/servers/SockJSServer.js index 8c67abd4e5..49c42538d2 100644 --- a/lib/servers/SockJSServer.js +++ b/lib/servers/SockJSServer.js @@ -57,8 +57,14 @@ module.exports = class SockJSServer extends BaseServer { connection.close(); } - // f should return the resulting connection + // f should return the resulting connection and, optionally, the connection headers onConnection(f) { - this.socket.on('connection', f); + this.socket.on('connection', (connection) => { + f(connection, connection.headers); + }); + } + + onConnectionClose(connection, f) { + connection.on('close', f); } }; diff --git a/lib/servers/WebsocketServer.js b/lib/servers/WebsocketServer.js index 0b282cd8c4..03c5a56e46 100644 --- a/lib/servers/WebsocketServer.js +++ b/lib/servers/WebsocketServer.js @@ -29,6 +29,12 @@ module.exports = class WebsocketServer extends BaseServer { // f should return the resulting connection onConnection(f) { - this.wsServer.on('connection', f); + this.wsServer.on('connection', (connection, req) => { + f(connection, req.headers); + }); + } + + onConnectionClose(connection, f) { + connection.on('close', f); } }; diff --git a/test/server/__snapshots__/serverMode-option.test.js.snap b/test/server/__snapshots__/serverMode-option.test.js.snap new file mode 100644 index 0000000000..a7ad43ca4e --- /dev/null +++ b/test/server/__snapshots__/serverMode-option.test.js.snap @@ -0,0 +1,9 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`serverMode option with a bad host header results in an error 1`] = ` +Array [ + "open", + "{\\"type\\":\\"error\\",\\"data\\":\\"Invalid Host/Origin header\\"}", + "close", +] +`; diff --git a/test/server/serverMode-option.test.js b/test/server/serverMode-option.test.js index b0ef7357fb..8a660ddab6 100644 --- a/test/server/serverMode-option.test.js +++ b/test/server/serverMode-option.test.js @@ -5,6 +5,7 @@ */ const request = require('supertest'); const sockjs = require('sockjs'); +const SockJS = require('sockjs-client/dist/sockjs'); const SockJSServer = require('../../lib/servers/SockJSServer'); const config = require('../fixtures/simple-config/webpack.config'); const testServer = require('../helpers/test-server'); @@ -77,6 +78,7 @@ describe('serverMode option', () => { describe('as a class (custom "sockjs" implementation)', () => { it('uses supplied server implementation', (done) => { + let sockPath; server = testServer.start( config, { @@ -102,7 +104,7 @@ describe('serverMode option', () => { prefix: this.server.sockPath, }); - expect(server.options.sockPath).toEqual('/foo/test/bar/'); + sockPath = server.options.sockPath; } send(connection, message) { @@ -114,11 +116,20 @@ describe('serverMode option', () => { } onConnection(f) { - this.socket.on('connection', f); + this.socket.on('connection', (connection) => { + f(connection, connection.headers); + }); + } + + onConnectionClose(connection, f) { + connection.on('close', f); } }, }, - done + () => { + expect(sockPath).toEqual('/foo/test/bar/'); + done(); + } ); }); }); @@ -137,4 +148,79 @@ describe('serverMode option', () => { }).toThrow(/serverMode must be a string/); }); }); + + describe('with a bad host header', () => { + beforeAll((done) => { + server = testServer.start( + config, + { + port, + serverMode: class MySockJSServer extends BaseServer { + constructor(serv) { + super(serv); + this.socket = sockjs.createServer({ + // Use provided up-to-date sockjs-client + sockjs_url: '/__webpack_dev_server__/sockjs.bundle.js', + // Limit useless logs + log: (severity, line) => { + if (severity === 'error') { + this.server.log.error(line); + } else { + this.server.log.debug(line); + } + }, + }); + + this.socket.installHandlers(this.server.listeningApp, { + prefix: this.server.sockPath, + }); + } + + send(connection, message) { + connection.write(message); + } + + close(connection) { + connection.close(); + } + + onConnection(f) { + this.socket.on('connection', (connection) => { + f(connection, { + host: null, + }); + }); + } + + onConnectionClose(connection, f) { + connection.on('close', f); + } + }, + }, + done + ); + }); + + it('results in an error', (done) => { + const data = []; + const client = new SockJS(`http://localhost:${port}/sockjs-node`); + + client.onopen = () => { + data.push('open'); + }; + + client.onmessage = (e) => { + data.push(e.data); + }; + + client.onclose = () => { + data.push('close'); + }; + + setTimeout(() => { + expect(data).toMatchSnapshot(); + done(); + }, 5000); + }); + }); }); diff --git a/test/server/servers/SockJSServer.test.js b/test/server/servers/SockJSServer.test.js index 9ea6d39b2a..9c61c67e14 100644 --- a/test/server/servers/SockJSServer.test.js +++ b/test/server/servers/SockJSServer.test.js @@ -35,10 +35,13 @@ describe('SockJSServer', () => { it('should recieve connection, send message, and close client', (done) => { const data = []; - socketServer.onConnection((connection) => { + let headers; + socketServer.onConnection((connection, h) => { + headers = h; data.push('open'); socketServer.send(connection, 'hello world'); setTimeout(() => { + // the server closes the connection with the client socketServer.close(connection); }, 1000); }); @@ -54,10 +57,33 @@ describe('SockJSServer', () => { }; setTimeout(() => { + expect(headers.host).toMatchSnapshot(); expect(data).toMatchSnapshot(); done(); }, 3000); }); + + it('should receive client close event', (done) => { + let receivedClientClose = false; + socketServer.onConnection((connection) => { + socketServer.onConnectionClose(connection, () => { + receivedClientClose = true; + }); + }); + + // eslint-disable-next-line new-cap + const client = new SockJS(`http://localhost:${port}/sockjs-node`); + + setTimeout(() => { + // the client closes itself, the server does not close it + client.close(); + }, 1000); + + setTimeout(() => { + expect(receivedClientClose).toBeTruthy(); + done(); + }, 3000); + }); }); afterAll((done) => { diff --git a/test/server/servers/WebsocketServer.test.js b/test/server/servers/WebsocketServer.test.js index 705bb548fa..e8e59a786b 100644 --- a/test/server/servers/WebsocketServer.test.js +++ b/test/server/servers/WebsocketServer.test.js @@ -35,10 +35,13 @@ describe('WebsocketServer', () => { it('should recieve connection, send message, and close client', (done) => { const data = []; - socketServer.onConnection((connection) => { + let headers; + socketServer.onConnection((connection, h) => { + headers = h; data.push('open'); socketServer.send(connection, 'hello world'); setTimeout(() => { + // the server closes the connection with the client socketServer.close(connection); }, 1000); }); @@ -55,10 +58,33 @@ describe('WebsocketServer', () => { }; setTimeout(() => { + expect(headers.host).toMatchSnapshot(); expect(data).toMatchSnapshot(); done(); }, 3000); }); + + it('should receive client close event', (done) => { + let receivedClientClose = false; + socketServer.onConnection((connection) => { + socketServer.onConnectionClose(connection, () => { + receivedClientClose = true; + }); + }); + + // eslint-disable-next-line new-cap + const client = new ws(`http://localhost:${port}/ws-server`); + + setTimeout(() => { + // the client closes itself, the server does not close it + client.close(); + }, 1000); + + setTimeout(() => { + expect(receivedClientClose).toBeTruthy(); + done(); + }, 3000); + }); }); afterAll((done) => { diff --git a/test/server/servers/__snapshots__/SockJSServer.test.js.snap b/test/server/servers/__snapshots__/SockJSServer.test.js.snap index 34076e2176..8bc35a0b99 100644 --- a/test/server/servers/__snapshots__/SockJSServer.test.js.snap +++ b/test/server/servers/__snapshots__/SockJSServer.test.js.snap @@ -1,6 +1,8 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`SockJSServer server should recieve connection, send message, and close client 1`] = ` +exports[`SockJSServer server should recieve connection, send message, and close client 1`] = `"localhost:8083"`; + +exports[`SockJSServer server should recieve connection, send message, and close client 2`] = ` Array [ "open", "hello world", diff --git a/test/server/servers/__snapshots__/WebsocketServer.test.js.snap b/test/server/servers/__snapshots__/WebsocketServer.test.js.snap index ade8385a89..abaaaa8c48 100644 --- a/test/server/servers/__snapshots__/WebsocketServer.test.js.snap +++ b/test/server/servers/__snapshots__/WebsocketServer.test.js.snap @@ -1,6 +1,8 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`WebsocketServer server should recieve connection, send message, and close client 1`] = ` +exports[`WebsocketServer server should recieve connection, send message, and close client 1`] = `"localhost:8121"`; + +exports[`WebsocketServer server should recieve connection, send message, and close client 2`] = ` Array [ "open", "hello world",