From 61b949259ed966ef6fc8bfd61f14d1a2ef06d319 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Mon, 10 Feb 2020 13:24:04 +0100 Subject: [PATCH] feat: use the cors module to handle cross-origin requests We'll now rely on the standard cors module (https://github.com/expressjs/cors), instead of the custom implementation that is error-prone and not really user-friendly. Breaking change: the handlePreflightRequest option is removed by the change. Before: ``` new Server({ handlePreflightRequest: (req, res) => { res.writeHead(200, { "Access-Control-Allow-Origin": 'https://example.com', "Access-Control-Allow-Methods": 'GET', "Access-Control-Allow-Headers": 'Authorization', "Access-Control-Allow-Credentials": true }); res.end(); } }) ``` After: ``` new Server({ cors: { origin: "https://example.com", methods: ["GET"], allowedHeaders: ["Authorization"], credentials: true } }) ``` --- README.md | 2 +- lib/server.js | 39 ++++--- lib/transports/index.js | 2 +- lib/transports/polling-xhr.js | 43 -------- package-lock.json | 17 ++- package.json | 5 +- test/server.js | 199 +++++++++++++++++++++------------- 7 files changed, 164 insertions(+), 143 deletions(-) delete mode 100644 lib/transports/polling-xhr.js diff --git a/README.md b/README.md index dfd6332ac..e77973510 100644 --- a/README.md +++ b/README.md @@ -249,6 +249,7 @@ to a single process. headers. This cookie might be used for sticky-session. Defaults to not sending any cookie (`false`). See [here](https://github.com/jshttp/cookie#options-1) for all supported options. - `wsEngine` (`String`): what WebSocket server implementation to use. Specified module must conform to the `ws` interface (see [ws module api docs](https://github.com/websockets/ws/blob/master/doc/ws.md)). Default value is `ws`. An alternative c++ addon is also available by installing `uws` module. + - `cors` (`Object`): the options that will be forwarded to the cors module. See [there](https://github.com/expressjs/cors#configuration-options) for all available options. Defaults to no CORS allowed. - `initialPacket` (`Object`): an optional packet which will be concatenated to the handshake packet emitted by Engine.IO. - `close` - Closes all clients @@ -277,7 +278,6 @@ to a single process. - `path` (`String`): name of the path to capture (`/engine.io`). - `destroyUpgrade` (`Boolean`): destroy unhandled upgrade requests (`true`) - `destroyUpgradeTimeout` (`Number`): milliseconds after which unhandled requests are ended (`1000`) - - `handlePreflightRequest` (`Boolean|Function`): whether to let engine.io handle the OPTIONS requests. You can also pass a custom function to handle the requests (`true`) - `generateId` - Generate a socket id. - Overwrite this method to generate your custom socket id. diff --git a/lib/server.js b/lib/server.js index 1632de0dc..527c7a0f9 100644 --- a/lib/server.js +++ b/lib/server.js @@ -34,7 +34,8 @@ class Server extends EventEmitter { }, httpCompression: { threshold: 1024 - } + }, + cors: false }, opts ); @@ -51,6 +52,10 @@ class Server extends EventEmitter { ); } + if (this.opts.cors) { + this.corsMiddleware = require("cors")(this.opts.cors); + } + this.init(); } @@ -183,8 +188,7 @@ class Server extends EventEmitter { this.prepare(req); req.res = res; - const self = this; - this.verify(req, false, function(err, success) { + const callback = (err, success) => { if (!success) { sendErrorMessage(req, res, err); return; @@ -192,11 +196,19 @@ class Server extends EventEmitter { if (req._query.sid) { debug("setting new request for existing client"); - self.clients[req._query.sid].transport.onRequest(req); + this.clients[req._query.sid].transport.onRequest(req); } else { - self.handshake(req._query.transport, req); + this.handshake(req._query.transport, req); } - }); + }; + + if (this.corsMiddleware) { + this.corsMiddleware.call(null, req, res, () => { + this.verify(req, false, callback); + }); + } else { + this.verify(req, false, callback); + } } /** @@ -380,12 +392,6 @@ class Server extends EventEmitter { path += "/"; function check(req) { - if ( - "OPTIONS" === req.method && - false === options.handlePreflightRequest - ) { - return false; - } return path === req.url.substr(0, path.length); } @@ -399,14 +405,7 @@ class Server extends EventEmitter { server.on("request", function(req, res) { if (check(req)) { debug('intercepting request for path "%s"', path); - if ( - "OPTIONS" === req.method && - "function" === typeof options.handlePreflightRequest - ) { - options.handlePreflightRequest.call(server, req, res); - } else { - self.handleRequest(req, res); - } + self.handleRequest(req, res); } else { let i = 0; const l = listeners.length; diff --git a/lib/transports/index.js b/lib/transports/index.js index ea106551e..98e0ab537 100644 --- a/lib/transports/index.js +++ b/lib/transports/index.js @@ -1,4 +1,4 @@ -const XHR = require("./polling-xhr"); +const XHR = require("./polling"); const JSONP = require("./polling-jsonp"); /** diff --git a/lib/transports/polling-xhr.js b/lib/transports/polling-xhr.js deleted file mode 100644 index c4a63e26b..000000000 --- a/lib/transports/polling-xhr.js +++ /dev/null @@ -1,43 +0,0 @@ -const Polling = require("./polling"); - -class XHR extends Polling { - /** - * Overrides `onRequest` to handle `OPTIONS`.. - * - * @param {http.IncomingMessage} - * @api private - */ - onRequest(req) { - if ("OPTIONS" === req.method) { - const res = req.res; - const headers = this.headers(req); - headers["Access-Control-Allow-Headers"] = "Content-Type"; - res.writeHead(200, headers); - res.end(); - } else { - super.onRequest(req); - } - } - - /** - * Returns headers for a response. - * - * @param {http.IncomingMessage} request - * @param {Object} extra headers - * @api private - */ - headers(req, headers) { - headers = headers || {}; - - if (req.headers.origin) { - headers["Access-Control-Allow-Credentials"] = "true"; - headers["Access-Control-Allow-Origin"] = req.headers.origin; - } else { - headers["Access-Control-Allow-Origin"] = "*"; - } - - return super.headers(req, headers); - } -} - -module.exports = XHR; diff --git a/package-lock.json b/package-lock.json index 1706ad680..917a71cbe 100644 --- a/package-lock.json +++ b/package-lock.json @@ -465,6 +465,15 @@ "integrity": "sha1-tf1UIgqivFq1eqtxQMlAdUUDwac=", "dev": true }, + "cors": { + "version": "2.8.5", + "resolved": "https://registry.npmjs.org/cors/-/cors-2.8.5.tgz", + "integrity": "sha512-KIHbLJqu73RGr/hnbrO9uBeixNGuvSQjul/jdFvS/KFSIH1hWVd1ng7zOHx+YrEfInLG7q4n6GHQ9cDtxv/P6g==", + "requires": { + "object-assign": "^4", + "vary": "^1" + } + }, "cross-spawn": { "version": "5.1.0", "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-5.1.0.tgz", @@ -1206,8 +1215,7 @@ "object-assign": { "version": "4.1.1", "resolved": "https://registry.npmjs.org/object-assign/-/object-assign-4.1.1.tgz", - "integrity": "sha1-IQmtx5ZYh8/AXLvUQsrIv7s2CGM=", - "dev": true + "integrity": "sha1-IQmtx5ZYh8/AXLvUQsrIv7s2CGM=" }, "once": { "version": "1.4.0", @@ -1635,6 +1643,11 @@ "from": "github:mmdevries/uws#2.4.1", "dev": true }, + "vary": { + "version": "1.1.2", + "resolved": "https://registry.npmjs.org/vary/-/vary-1.1.2.tgz", + "integrity": "sha1-IpnwLG3tMNSllhsLn3RSShj2NPw=" + }, "which": { "version": "1.3.1", "resolved": "https://registry.npmjs.org/which/-/which-1.3.1.tgz", diff --git a/package.json b/package.json index 3aef222b6..83e7b766e 100644 --- a/package.json +++ b/package.json @@ -28,6 +28,7 @@ "accepts": "~1.3.4", "base64id": "2.0.0", "cookie": "0.3.1", + "cors": "~2.8.5", "debug": "~4.1.0", "engine.io-parser": "git+https://github.com/socketio/engine.io-parser.git#v4", "ws": "^7.1.2" @@ -57,7 +58,7 @@ "files": [ "lib/" ], - "engines" : { - "node" : ">=8.0.0" + "engines": { + "node": ">=8.0.0" } } diff --git a/test/server.js b/test/server.js index 56ac84d9b..a770d7169 100644 --- a/test/server.js +++ b/test/server.js @@ -2881,85 +2881,136 @@ describe("server", function() { }); describe("cors", function() { - it("should handle OPTIONS requests", function(done) { - listen({ handlePreflightRequest: true }, function(port) { - request - .options("http://localhost:%d/engine.io/default/".s(port)) - .set("Origin", "http://engine.io") - .query({ transport: "polling" }) - .end(function(err, res) { - expect(err).to.be.an(Error); - expect(res.status).to.be(400); - expect(res.body.code).to.be(2); - expect(res.body.message).to.be("Bad handshake method"); - expect(res.header["access-control-allow-credentials"]).to.be( - "true" - ); - expect(res.header["access-control-allow-origin"]).to.be( - "http://engine.io" - ); - done(); - }); - }); + it("should allow CORS from the current origin (preflight request)", done => { + listen( + { cors: { origin: true, headers: ["my-header"], credentials: true } }, + port => { + request + .options("http://localhost:%d/engine.io/default/".s(port)) + .set("Origin", "http://engine.io") + .query({ transport: "polling" }) + .end(function(err, res) { + expect(err).to.be(null); + expect(res.status).to.be(204); + expect(res.body).to.be.empty(); + expect(res.header["access-control-allow-origin"]).to.be( + "http://engine.io" + ); + expect(res.header["access-control-allow-methods"]).to.be( + "GET,HEAD,PUT,PATCH,POST,DELETE" + ); + expect(res.header["access-control-allow-headers"]).to.be( + "my-header" + ); + expect(res.header["access-control-allow-credentials"]).to.be( + "true" + ); + done(); + }); + } + ); }); - it("should not handle OPTIONS requests", function(done) { - listen({ handlePreflightRequest: false }, function(port) { - request - .options("http://localhost:%d/engine.io/default/".s(port)) - .set("Origin", "http://engine.io") - .query({ transport: "polling" }) - .end(function(err, res) { - expect(err).to.be.an(Error); - expect(res.status).to.be(501); - expect(res.body.code).to.be(undefined); - done(); - }); - }); + it("should allow CORS from the current origin (actual request)", done => { + listen( + { cors: { origin: true, headers: ["my-header"], credentials: true } }, + port => { + request + .get("http://localhost:%d/engine.io/default/".s(port)) + .set("Origin", "http://engine.io") + .query({ transport: "polling" }) + .end(function(err, res) { + expect(err).to.be(null); + expect(res.status).to.be(200); + expect(res.body).to.be.empty(); + expect(res.header["access-control-allow-origin"]).to.be( + "http://engine.io" + ); + expect(res.header["access-control-allow-methods"]).to.be( + undefined + ); + expect(res.header["access-control-allow-headers"]).to.be( + undefined + ); + expect(res.header["access-control-allow-credentials"]).to.be( + "true" + ); + done(); + }); + } + ); }); - it("should handle OPTIONS requests with the given function", function(done) { - var handlePreflightRequest = function(req, res) { - var headers = {}; - if (req.headers.origin) { - headers["Access-Control-Allow-Credentials"] = "true"; - headers["Access-Control-Allow-Origin"] = req.headers.origin; - } else { - headers["Access-Control-Allow-Origin"] = "*"; + it("should disallow CORS from a bad origin", done => { + listen( + { + cors: { + origin: ["http://good-domain.com"] + } + }, + port => { + request + .options("http://localhost:%d/engine.io/default/".s(port)) + .set("Origin", "http://bad-domain.com") + .query({ transport: "polling" }) + .end(function(err, res) { + expect(err).to.be(null); + expect(res.status).to.be(204); + expect(res.body).to.be.empty(); + expect(res.header["access-control-allow-origin"]).to.be( + undefined + ); + expect(res.header["access-control-allow-credentials"]).to.be( + undefined + ); + done(); + }); } - headers["Access-Control-Allow-Methods"] = - "GET,HEAD,PUT,PATCH,POST,DELETE"; - headers["Access-Control-Allow-Headers"] = - "origin, content-type, accept"; - res.writeHead(200, headers); - res.end(); - }; - listen({ handlePreflightRequest: handlePreflightRequest }, function( - port - ) { - request - .options("http://localhost:%d/engine.io/default/".s(port)) - .set("Origin", "http://engine.io") - .query({ transport: "polling" }) - .end(function(err, res) { - expect(err).to.be(null); - expect(res.status).to.be(200); - expect(res.body).to.be.empty(); - expect(res.header["access-control-allow-credentials"]).to.be( - "true" - ); - expect(res.header["access-control-allow-origin"]).to.be( - "http://engine.io" - ); - expect(res.header["access-control-allow-methods"]).to.be( - "GET,HEAD,PUT,PATCH,POST,DELETE" - ); - expect(res.header["access-control-allow-headers"]).to.be( - "origin, content-type, accept" - ); - done(); - }); - }); + ); + }); + + it("should forward the configuration to the cors module", done => { + listen( + { + cors: { + origin: "http://good-domain.com", + methods: ["GET", "PUT", "POST"], + allowedHeaders: ["my-header"], + exposedHeaders: ["my-exposed-header"], + credentials: true, + maxAge: 123, + optionsSuccessStatus: 200 + } + }, + port => { + request + .options("http://localhost:%d/engine.io/default/".s(port)) + .set("Origin", "http://good-domain.com") + .query({ transport: "polling" }) + .end(function(err, res) { + expect(err).to.be(null); + expect(res.status).to.be(200); + expect(res.body).to.be.empty(); + expect(res.header["access-control-allow-origin"]).to.be( + "http://good-domain.com" + ); + expect(res.header["access-control-allow-methods"]).to.be( + "GET,PUT,POST" + ); + expect(res.header["access-control-allow-headers"]).to.be( + "my-header" + ); + expect(res.header["access-control-expose-headers"]).to.be( + "my-exposed-header" + ); + expect(res.header["access-control-allow-credentials"]).to.be( + "true" + ); + expect(res.header["access-control-max-age"]).to.be("123"); + done(); + }); + } + ); }); });