From 7378b40e955d879843512aad869bbcf3bff68e09 Mon Sep 17 00:00:00 2001 From: Isaac Date: Sun, 19 Sep 2021 23:40:39 -0400 Subject: [PATCH] Bugfix/all node ssl options (#105) * Update README for http ssl option * Add sslOptions attribute and spread that for the https server options * Update https test server to use ssl option * Add new emptyBuffer method to dump the contents of the message buffer * Push request chunks to message buffer and validate entire buffer - no delimiter used since one is not typically sent for large json data * Add test for emptyBuffer * Register 'end' event for http listener to emptyBuffer on response complete * Call on('end') listener outside of data listener and check if buffer is empty - prevents maxEventListeners warning - dont try to parse empty buffer string * Add generic _removeFromArray method - used to remove clients from this.clients - used to remove pendingRequests from this.pendingRequests * - Return false from _maybeHandleRequest if notification received - Dont send a response to a batch request if the respnse is empty - Only attempt to get results for batch request messages if they are not notifications --- README.md | 23 +++++++++------- src/client/protocol/http.js | 13 ++++++--- src/server/http.js | 46 ++++++------------------------- src/server/index.js | 25 +++++++++-------- src/server/protocol/base.js | 19 +++++++++---- src/server/protocol/http.js | 14 ++++++---- src/util/buffer.js | 14 ++++++++++ tests/client/http-client.test.js | 4 +++ tests/client/tcp-client.test.js | 2 +- tests/server/http-server.test.js | 4 +-- tests/server/tcp-server.test.js | 4 +-- tests/test-server.js | 8 ++++-- tests/util/message-buffer.test.js | 11 +++++++- 13 files changed, 105 insertions(+), 82 deletions(-) diff --git a/README.md b/README.md index a7824f42..04343cf8 100644 --- a/README.md +++ b/README.md @@ -6,8 +6,8 @@ - [Download & Installation](#download--installation) - [Class Documentation](#class-documentation) - [CLI tool](#cli-tool) - - [Install](#install) - - [Usage](#usage) + - [Install](#install) + - [Usage](#usage) - [Use of the library as a standalone module](#use-of-the-library-as-a-standalone-module) - [Initialization](#initialization) - [WS Client for browser](#ws-client-for-browser) @@ -18,10 +18,10 @@ - [Other client and server options](#other-client-and-server-options) - [Code Demos](#code-demos) - [Initialization](#initialization-1) - - [TCP](#tcp) - - [HTTP](#http) - - [HTTPS](#https) - - [Websocket](#websocket) + - [TCP](#tcp) + - [HTTP](#http) + - [HTTPS](#https) + - [Websocket](#websocket) - [Server side](#server-side) - [Instantiation and Listening](#instantiation-and-listening) - [Closing the connection](#closing-the-connection) @@ -297,7 +297,8 @@ const server = new Jaysonic.server.http() Pass `https` to the `sheme` option to use an https client or server. -The https server requires an SSL key and certificate file. +The https server requires an SSL key and certificate file. Valid options for the `ssl` object are any of the options +given by https://nodejs.org/api/tls.html#tls_tls_createsecurecontext_options. ```js const Jaysonic = require("jaysonic"); @@ -307,9 +308,11 @@ const client = new Jaysonic.client.http({ scheme: "https" }); // https server const server = new Jaysonic.server.http({ - scheme: "http", - key: fs.readFileSync("key.pem"), - cert: fs.readFileSync("server.crt") + scheme: "https", + ssl: { + key: fs.readFileSync("key.pem"), + cert: fs.readFileSync("server.crt") + } }); ``` diff --git a/src/client/protocol/http.js b/src/client/protocol/http.js index 4ffe8fbc..a022ad06 100644 --- a/src/client/protocol/http.js +++ b/src/client/protocol/http.js @@ -74,14 +74,19 @@ class HttpClientProtocol extends JsonRpcClientProtocol { /** * Setup `this.listener.on("data")` event to listen for data coming into the client. * - * The HTTP client does not use the messageBuffer since each request should - * only receive one response at a time. + * The HTTP client does not use the delimiter since the completion of a response indicates + * the end of data. * * Calls [_waitForData]{@link JsonRpcClientProtocol#_waitForData} */ listen() { - this.listener.on("data", (data) => { - this._waitForData(data); + this.listener.on("data", (chunk) => { + this.messageBuffer.push(chunk); + }); + this.listener.on("end", () => { + if (this.messageBuffer.buffer !== "") { + this._waitForData(this.messageBuffer.emptyBuffer()); + } }); } diff --git a/src/server/http.js b/src/server/http.js index 0a2e33b0..5c3de952 100644 --- a/src/server/http.js +++ b/src/server/http.js @@ -14,15 +14,14 @@ class HttpServerFactory extends JsonRpcServerFactory { * the WsServerProtocol has the following properties: * * @property {'http'|'https'} scheme The scheme to allow connections with - * @property {file} key The private SSL key file - * @property {file} cert The SSL certificate file + * @property {object} sslOptions Any of the ssl options for the http server according to https://nodejs.org/api/tls.html#tls_tls_createsecurecontext_options */ constructor(options) { super(options); this.scheme = this.options.scheme || "http"; - this.key = this.options.key; - this.cert = this.options.cert; + this.sslOptions = this.options.ssl; this.protocol = HttpServerProtocol; + this.pendingRequests = []; } /** @inheritdoc */ @@ -31,8 +30,7 @@ class HttpServerFactory extends JsonRpcServerFactory { this.server = new http.Server(); } else if (this.scheme === "https") { this.server = new https.Server({ - key: this.key, - cert: this.cert + ...this.sslOptions }); } else { throw Error("Invalid scheme"); @@ -58,47 +56,19 @@ class HttpServerFactory extends JsonRpcServerFactory { this.options.delimiter ); pcol.clientConnected(); + this.pendingRequests.push(pcol); }); } /** - * Called when client receives a `connection` event. + * Called when the server receives a `connection` event. * - * @param {stream.Duplex} client Instance of `stream.Duplex` - * @returns {stream.Duplex} Returns an instance of `stream.Duplex` + * @param {net.Socket} client Instance of `net.Socket` + * @returns {net.Socket} Returns an instance of `net.Socket` */ clientConnected(client) { return client; } - - /** - * Called when client disconnects from server. - * - * @param {stream.Duplex} client Instance of `stream.Duplex` - * @returns {object|error} Returns an object of {host, port} for the given protocol instance, or {error} - * if there was an error retrieving the client - */ - clientDisconnected(client) { - return this.removeDisconnectedClient(client); - } - - /** - * Removes disconnected client from `this.clients` list - * - * @param {stream.Duplex} client Instance of `stream.Duplex` - * @returns {object|error} Returns an object of {host, port} for the given protocol instance, or {error} - * if there was an error retrieving the client - */ - removeDisconnectedClient(client) { - const clientIndex = this.clients.findIndex(c => c === client); - if (clientIndex === -1) { - return { - error: `Unknown client ${JSON.stringify(client)}` - }; - } - const [disconnectedClient] = this.clients.splice(clientIndex, 1); - return disconnectedClient; - } } module.exports = HttpServerFactory; diff --git a/src/server/index.js b/src/server/index.js index 62e353af..d4c4e842 100644 --- a/src/server/index.js +++ b/src/server/index.js @@ -295,33 +295,36 @@ class JsonRpcServerFactory extends EventEmitter { /** * Called when client disconnects from server. * - * If overwriting, its recommended to call {@link JsonRpcServerFactory.removeDisconnectedClient} manually + * If overwriting, its recommended to call {@link JsonRpcServerFactory._removeFromArray} manually * to ensure `this.clients` is cleaned up * + * Calls `this._removeFromArray` and removes disconnected client from `this.clients` list + * * @param {JsonRpcServerProtocol} pcol A {@link JsonRpcServerProtocol} instance * @returns {object|error} Returns an object of {host, port} for the given protocol instance, or {error} * if there was an error retrieving the client */ clientDisconnected(pcol) { - return this.removeDisconnectedClient(pcol); + return this._removeFromArray(pcol, this.clients); } /** - * Removes disconnected client from `this.clients` list + * Removes item from the given list * - * @param {JsonRpcServerProtocol} pcol A {@link JsonRpcServerProtocol} instance - * @returns {object|error} Returns an object of {host, port} for the given protocol instance, or {error} + * @param {*} item Any item in the list + * @param {array} array The array to remove the item from + * @returns {*|error} Returns the item that was removed from the array or {error} * */ - removeDisconnectedClient(pcol) { - const clientIndex = this.clients.findIndex(p => p === pcol); - if (clientIndex === -1) { + _removeFromArray(item, array) { + const itemIndex = array.findIndex(c => c === item); + if (itemIndex === -1) { return { - error: `Unknown client ${JSON.stringify(pcol)}` + error: `Unable to remove ${JSON.stringify(item)}` }; } - const [protocol] = this.clients.splice(clientIndex, 1); - return protocol.client; + const [removedItem] = array.splice(itemIndex, 1); + return removedItem; } /** diff --git a/src/server/protocol/base.js b/src/server/protocol/base.js index 8c5db01b..a358dce7 100644 --- a/src/server/protocol/base.js +++ b/src/server/protocol/base.js @@ -127,11 +127,16 @@ class JsonRpcServerProtocol { } // possible batch request this.gotBatchRequest(result).then((res) => { - this.writeToClient(JSON.stringify(res) + this.delimiter); + if (res.length !== 0) { + // if all the messages in the batch were notifications, + // then we wouldnt want to return anything + this.writeToClient(JSON.stringify(res) + this.delimiter); + } }); } else if (result === Object(result) && !("id" in result)) { // no id, so assume notification this.gotNotification(result); + return false; } else { this.validateMessage(result); return true; @@ -243,10 +248,14 @@ class JsonRpcServerProtocol { const batchResponses = requests .map((request) => { try { - this._maybeHandleRequest(request); - return this.getResult(request) - .then(result => JSON.parse(result)) - .catch(error => JSON.parse(error)); + const isMessage = this._maybeHandleRequest(request); + if (isMessage) { + // if its a notification we dont want to return anything + return this.getResult(request) + .then(result => JSON.parse(result)) + .catch(error => JSON.parse(error)); + } + return null; } catch (e) { // basically reject the whole batch if any one thing fails return JSON.parse(e.message); diff --git a/src/server/protocol/http.js b/src/server/protocol/http.js index 9913ef47..338823dc 100644 --- a/src/server/protocol/http.js +++ b/src/server/protocol/http.js @@ -65,14 +65,18 @@ class HttpServerProtocol extends JsonRpcServerProtocol { * Pushes received data into `messageBuffer` and calls * [_waitForData]{@link JsonRpcServerProtocol#_waitForData}. * + * The HTTP server does not use the delimiter since the completion of a request indicates + * the end of data. + * * @extends JsonRpcServerProtocol.clientConnected */ clientConnected() { - this.client.on(this.event, (data) => { - this.client.on("end", () => { - this.messageBuffer.push(data); - this._waitForData(); - }); + this.client.on(this.event, (chunk) => { + this.messageBuffer.push(chunk); + }); + this.client.on("end", () => { + this._validateData(this.messageBuffer.emptyBuffer()); + this.factory._removeFromArray(this, this.factory.pendingRequests); }); } diff --git a/src/util/buffer.js b/src/util/buffer.js index 63fe2374..26c7fbb4 100644 --- a/src/util/buffer.js +++ b/src/util/buffer.js @@ -81,6 +81,20 @@ class MessageBuffer { return null; } + /** + * + * Returns the contents of `this.buffer`. + * + * Particularily useful for http buffering, where delimiters might not be used. + * + * @returns {string} message + */ + emptyBuffer() { + const data = this.buffer; + this.buffer = ""; + return data; + } + /** * @returns {function} MessageBuffer.getMessage() */ diff --git a/tests/client/http-client.test.js b/tests/client/http-client.test.js index 805561f3..7515eb08 100644 --- a/tests/client/http-client.test.js +++ b/tests/client/http-client.test.js @@ -1,8 +1,12 @@ const { expect } = require("chai"); +const chai = require("chai"); +const spies = require("chai-spies"); const Jaysonic = require("../../src"); const data = require("../large-data.json"); const { serverHttp } = require("../test-server"); +chai.use(spies); + const clientHttp = new Jaysonic.client.http(); describe("HTTP Client", () => { diff --git a/tests/client/tcp-client.test.js b/tests/client/tcp-client.test.js index bca2cf49..a2f5bb5d 100644 --- a/tests/client/tcp-client.test.js +++ b/tests/client/tcp-client.test.js @@ -287,7 +287,7 @@ describe("TCP Client", () => { }); }); }); - describe("notifications", () => { + describe("receiving notifications", () => { it("should handle receiving a notification", (done) => { const callback = (message) => { expect(message).to.be.eql({ diff --git a/tests/server/http-server.test.js b/tests/server/http-server.test.js index e7ca6e46..dc10773e 100644 --- a/tests/server/http-server.test.js +++ b/tests/server/http-server.test.js @@ -29,11 +29,11 @@ describe("Http Server", () => { it("should return 'Unknown client' from clientDisconnected if client was not found", (done) => { let res; serverHttp.clientDisconnected = (client) => { - res = serverHttp.removeDisconnectedClient(client); + res = serverHttp._removeFromArray(client, serverHttp.clients); }; serverHttp.clientDisconnected({}); try { - expect(res).to.be.eql({ error: "Unknown client {}" }); + expect(res).to.be.eql({ error: "Unable to remove {}" }); done(); } catch (e) { console.log(e); diff --git a/tests/server/tcp-server.test.js b/tests/server/tcp-server.test.js index 688d1ec1..660fc380 100644 --- a/tests/server/tcp-server.test.js +++ b/tests/server/tcp-server.test.js @@ -71,10 +71,10 @@ describe("TCP Server", () => { it("should return 'Unknown client' from clientDisconnected if client was not found", (done) => { let res; server.clientDisconnected = (pcol) => { - res = server.removeDisconnectedClient(pcol); + res = server._removeFromArray(pcol, server.clients); }; server.clientDisconnected({}); - expect(res).to.be.eql({ error: "Unknown client {}" }); + expect(res).to.be.eql({ error: "Unable to remove {}" }); done(); }); it("should be unable to listen multiple times", (done) => { diff --git a/tests/test-server.js b/tests/test-server.js index b4ddb96b..5f5a1a7d 100644 --- a/tests/test-server.js +++ b/tests/test-server.js @@ -13,9 +13,11 @@ const serverHttp = new Jaysonic.server.http(); const serverHttpV1 = new Jaysonic.server.http({ version: 1 }); const serverHttps = new Jaysonic.server.http({ scheme: "https", - key: fs.readFileSync("tests/key.pem"), - cert: fs.readFileSync("tests/server.crt"), - ca: "selfSignedRootCaPemCrtBuffer" + ssl: { + key: fs.readFileSync("tests/key.pem"), + cert: fs.readFileSync("tests/server.crt"), + ca: "selfSignedRootCaPemCrtBuffer" + } }); const wss = new Jaysonic.server.ws(); diff --git a/tests/util/message-buffer.test.js b/tests/util/message-buffer.test.js index 3b48696b..fddc4930 100644 --- a/tests/util/message-buffer.test.js +++ b/tests/util/message-buffer.test.js @@ -33,7 +33,7 @@ const res2 = { }; describe("Message Buffer", () => { - describe("get.message()", () => { + describe("getMessage()", () => { it("should return null if no delimiter found", (done) => { const buffer = new MessageBuffer("\n"); buffer.push("foo"); @@ -41,6 +41,15 @@ describe("Message Buffer", () => { done(); }); }); + describe("emptyBuffer()", () => { + it("should return the message buffer and set it to empty", (done) => { + const buffer = new MessageBuffer(); + buffer.push("foo\r\nbar"); + expect(buffer.emptyBuffer()).to.equal("foo\r\nbar"); + expect(buffer.buffer).to.equal(""); + done(); + }); + }); describe("server side", () => { before((done) => { server.listen().then(() => {