From bb85a28001418b2e47cbe250cc70f1d7621efe6b Mon Sep 17 00:00:00 2001 From: Sebastian Krauskopf Date: Sat, 25 May 2024 22:12:52 +0200 Subject: [PATCH 1/3] fix: catch exception when server sends invalid/incomplete json in subscription (Bug849302) --- README.md | 1 + lib/CtrlxDatalayerSubscription.js | 34 ++++++++------- package.json | 2 +- test/ctrlx-datalayer-subscribe.test.js | 57 ++++++++++++++++++++++++++ test/helper/CtrlxMockupV2.js | 11 +++++ test/helper/benchmark.js | 1 - 6 files changed, 90 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 2930e08..d6f3aef 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,7 @@ Any use of the source code and related documents of this repository in applicati * 2023-09-04: 1.9.3 - fix: subscription with ipv6 address that contains a zone index * 2024-03-13: 1.9.4 - fix: timeout of connection with Node.js v19. "Read timeout, received no data in undefinedms, assuming connection is dead" * 2024-03-19: 1.9.5 - fix: update dependencies in package-lock +* 2024-05-24: 1.9.6 - fix: catch exception when server sends invalid/incomplete json in subscription (Bug849302) ``` ## About diff --git a/lib/CtrlxDatalayerSubscription.js b/lib/CtrlxDatalayerSubscription.js index e69e15d..f24c12b 100644 --- a/lib/CtrlxDatalayerSubscription.js +++ b/lib/CtrlxDatalayerSubscription.js @@ -320,7 +320,7 @@ class CtrlxDatalayerSubscription extends EventEmitter { } }, agent: new https.Agent({ keepAlive: false }) // create a dedicated agent to have dedicated connection instance. Also disable the agent-keep-alive explicitly. - // This is necessary because since node.js 19 the default behaviour was changed. + // This is necessary because since node.js 19 the default behaviour was changed. // https://nodejs.org/en/blog/announcements/v19-release-announce#https11-keepalive-by-default }; @@ -428,12 +428,16 @@ class CtrlxDatalayerSubscription extends EventEmitter { debugUpdate(`update(${e.data})`); } - let payload = CtrlxDatalayer._parseData(e.data); - - if (!this.emit('update', payload, e.lastEventId)) { - // Listener seems not yet to be attached. Retry on next tick. - setTimeout(()=>this.emit('update', payload, e.lastEventId), 0); + try { + let payload = CtrlxDatalayer._parseData(e.data); + if (!this.emit('update', payload, e.lastEventId)) { + // Listener seems not yet to be attached. Retry on next tick. + setTimeout(()=>this.emit('update', payload, e.lastEventId), 0); + } + } catch (err) { + this.emit('error', new Error(`Error parsing update event: ${err.message}`)); } + }); this._es.addEventListener('keepalive', (e) => { @@ -441,11 +445,14 @@ class CtrlxDatalayerSubscription extends EventEmitter { debugUpdate(`keepalive(${e.data})`); } - let payload = CtrlxDatalayer._parseData(e.data); - - if (!this.emit('keepalive', payload, e.lastEventId)) { - // Listener seems not yet to be attached. Retry on next tick. - setTimeout(()=>this.emit('keepalive', payload, e.lastEventId), 0); + try { + let payload = CtrlxDatalayer._parseData(e.data); + if (!this.emit('keepalive', payload, e.lastEventId)) { + // Listener seems not yet to be attached. Retry on next tick. + setTimeout(()=>this.emit('keepalive', payload, e.lastEventId), 0); + } + } catch (err) { + this.emit('error', new Error(`Error parsing keepalive event: ${err.message}`)); } }); @@ -476,10 +483,9 @@ class CtrlxDatalayerSubscription extends EventEmitter { // @ts-ignore this._es.onretrying = undefined; // @ts-ignore - this._es.onerror = (e) => { - // ignore any pending errors in the pipeline, which might get emitted and result in an uncaught exception + this._es.onerror = (e) => { + // ignore any pending errors in the pipeline, which might get emitted and result in an uncaught exception debug(`onerror(${e.type})`); - console.log("error after"); }; // @ts-ignore this._es.onmessage = undefined; diff --git a/package.json b/package.json index ef77c2e..d0fa292 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "node-red-contrib-ctrlx-automation", - "version": "1.9.5", + "version": "1.9.6", "description": "Node-RED nodes for ctrlX AUTOMATION", "repository": { "type": "git", diff --git a/test/ctrlx-datalayer-subscribe.test.js b/test/ctrlx-datalayer-subscribe.test.js index 41e2131..a2b5c60 100644 --- a/test/ctrlx-datalayer-subscribe.test.js +++ b/test/ctrlx-datalayer-subscribe.test.js @@ -464,6 +464,63 @@ describe('ctrlx-datalayer-subscribe', function () { }); + describe('ctrlx-datalayer-subscribe: Error Handling', function () { + it('should handle invalid send json messages', function (done) { + let flow = [ + { "id": "f1", "type": "tab", "label": "Test flow"}, + { "id": "h1", "z":"f1", "type": "helper" }, + { "id": "n1", "z":"f1", "type": "ctrlx-datalayer-subscribe", "subscription": "s1", "path": "test/invalid/json", "name": "subscribe", "wires": [["h1"]] }, + { "id": "s1", "z":"f1", "type": "ctrlx-config-subscription", "device": "c1", "name": "sub1", "publishIntervalMs": "1000" }, + { "id": "c1", "z":"f1", "type": "ctrlx-config", "name": "ctrlx", "hostname": getHostname(), "debug": true }, + ]; + let credentials = { + c1: { + username: getUsername(), + password: getPassword() + } + }; + + helper.load([ctrlxConfigNode, ctrlxConfigSubscriptionNode, ctrlxDatalayerSubscribeNode], flow, credentials, () => { + + let s1 = helper.getNode("s1"); + let h1 = helper.getNode("h1"); + let n1 = helper.getNode("n1"); + + let numErrors = 0; + + // @ts-ignore + h1.on("input", (msg) => { + try { + expect(msg).to.have.property('topic').to.be.a('string').eql('test/invalid/json'); + expect(msg).to.have.property('timestamp').to.be.a('number'); + expect(msg).to.have.property('type').to.be.a('string').eql('object'); + expect(msg).to.have.property('payload').to.be.a('object').eql({ + valid: "data" + }); + // Make sure we received an error before we reveice the valid data. + expect(numErrors).eql(1); + s1.subscription.close(); + done(); + } + catch (err) { + s1.subscription.close(); + done(err); + } + }); + + // We expect to reveive an error message, because the mockup will send an invalid JSON message. + n1.on('call:error', call => { + numErrors++; + try { + expect(call.firstArg).eql('Error parsing update event: Unexpected end of JSON input'); + } catch (err) { + done(err); + } + }); + + }); + }); + }); }); diff --git a/test/helper/CtrlxMockupV2.js b/test/helper/CtrlxMockupV2.js index a09c303..e82e9b5 100644 --- a/test/helper/CtrlxMockupV2.js +++ b/test/helper/CtrlxMockupV2.js @@ -523,6 +523,17 @@ class CtrlxMockupV2 { data.value = options; data.type = 'object'; break; + case 'test/invalid/json': + // This is a special node which sends an invalid and malformed json to for example + // simulate a broken connection. + sseStream.write({ + id: id++, + event: 'update', + data: `{"type": "object", "value": { "invalid formed json` + }); + data.value = { "valid": "data" }; + data.type = 'object'; + break; default: data.value = 'error: unknown value'; diff --git a/test/helper/benchmark.js b/test/helper/benchmark.js index 6dc901d..8e4f168 100644 --- a/test/helper/benchmark.js +++ b/test/helper/benchmark.js @@ -27,7 +27,6 @@ const CtrlxCore = require('../../lib/CtrlxCore') const { performance, PerformanceObserver } = require('perf_hooks') -const async = require('async'); const { assert } = require('console'); From f205dd7ed8fded217efcfceaced2e3207a378501 Mon Sep 17 00:00:00 2001 From: Sebastian Krauskopf Date: Sun, 26 May 2024 22:47:33 +0200 Subject: [PATCH 2/3] fix: add more tests to handle node address with invalid symbols --- README.md | 3 +- test/ctrlx-datalayer-request.test.js | 43 ++++++++++++++++++++++ test/ctrlx-datalayer-subscribe.test.js | 50 ++++++++++++++++++++++++++ test/helper/CtrlxMockupV2.js | 12 +++++++ 4 files changed, 107 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d6f3aef..b53efcf 100644 --- a/README.md +++ b/README.md @@ -117,7 +117,8 @@ Any use of the source code and related documents of this repository in applicati * 2023-09-04: 1.9.3 - fix: subscription with ipv6 address that contains a zone index * 2024-03-13: 1.9.4 - fix: timeout of connection with Node.js v19. "Read timeout, received no data in undefinedms, assuming connection is dead" * 2024-03-19: 1.9.5 - fix: update dependencies in package-lock -* 2024-05-24: 1.9.6 - fix: catch exception when server sends invalid/incomplete json in subscription (Bug849302) +* 2024-05-24: 1.9.6 - fix: catch exception when server sends invalid/incomplete json in subscription (Bug849302). + - fix: add more tests to handle node address with invalid symbols ``` ## About diff --git a/test/ctrlx-datalayer-request.test.js b/test/ctrlx-datalayer-request.test.js index ebe8b96..e544e00 100644 --- a/test/ctrlx-datalayer-request.test.js +++ b/test/ctrlx-datalayer-request.test.js @@ -165,6 +165,49 @@ describe('ctrlx-datalayer-request', function () { }); + it('should read a node with strange address', function (done) { + + let path = 'with/strange/symbols/abc=1;nichts-ist.wahr:("alles[ist]erlaubt")42/x.y.z'; + + let flow = [ + { "id": "h1", "type": "helper" }, + { "id": "n1", "type": "ctrlx-datalayer-request", "device": "c1", "method": "READ", "path": path, "name": "request", "wires": [["h1"]] }, + { "id": "c1", "type": "ctrlx-config", "name": "ctrlx", "hostname": getHostname(), "debug": true } + ]; + let credentials = { + c1: { + username: getUsername(), + password: getPassword() + } + }; + + helper.load([ctrlxConfigNode, ctrlxDatalayerRequestNode], flow, credentials, () => { + + let n1 = helper.getNode("n1"); + let h1 = helper.getNode("h1"); + + // @ts-ignore + h1.on("input", (msg) => { + try { + expect(msg).to.have.property('payload').with.property('value').that.is.a('string').eql('/automation/api/v2/nodes/' + encodeURI(path)); + expect(msg).to.have.property('payload').with.property('type').that.is.a('string').eql('string'); + + done(); + } + catch (err) { + done(err); + } + }); + n1.on('call:error', (call) => { + done(call.firstArg); + }); + + // @ts-ignore + n1.receive({ payload: "" }); + }); + }); + + it('should read a value and set the empty msg.topic', function (done) { let flow = [ diff --git a/test/ctrlx-datalayer-subscribe.test.js b/test/ctrlx-datalayer-subscribe.test.js index a2b5c60..569abf3 100644 --- a/test/ctrlx-datalayer-subscribe.test.js +++ b/test/ctrlx-datalayer-subscribe.test.js @@ -462,6 +462,56 @@ describe('ctrlx-datalayer-subscribe', function () { }); }); + + it('should subscribe a node with strange address', function (done) { + + let path = 'with/strange/symbols/abc=1;nichts-ist.wahr:("alles[ist]erlaubt")42/x.y.z'; + + let flow = [ + { "id": "f1", "type": "tab", "label": "Test flow"}, + { "id": "h1", "z":"f1", "type": "helper" }, + { "id": "n1", "z":"f1", "type": "ctrlx-datalayer-subscribe", "subscription": "s1", "path": path, "name": "subscribe", "wires": [["h1"]] }, + { "id": "s1", "z":"f1", "type": "ctrlx-config-subscription", "device": "c1", "name": "sub1", "publishIntervalMs": "1000" }, + { "id": "c1", "z":"f1", "type": "ctrlx-config", "name": "ctrlx", "hostname": getHostname(), "debug": true }, + ]; + let credentials = { + c1: { + username: getUsername(), + password: getPassword() + } + }; + + helper.load([ctrlxConfigNode, ctrlxConfigSubscriptionNode, ctrlxDatalayerSubscribeNode], flow, credentials, () => { + + let s1 = helper.getNode("s1"); + let h1 = helper.getNode("h1"); + let n1 = helper.getNode("n1"); + + // @ts-ignore + h1.on("input", (msg) => { + try { + expect(msg).to.have.property('topic').to.be.a('string').eql(path); + expect(msg).to.have.property('timestamp').to.be.a('number'); + expect(msg).to.have.property('type').to.be.a('string').eql('double'); + expect(msg).to.have.property('payload').to.be.a('number').eql(23); + + s1.subscription.close(); + done(); + } + catch (err) { + s1.subscription.close(); + done(err); + } + }); + + // We expect to reveive an error message, because the mockup will send an invalid JSON message. + n1.on('call:error', call => { + //numErrors++; + }); + + }); + }); + }); describe('ctrlx-datalayer-subscribe: Error Handling', function () { diff --git a/test/helper/CtrlxMockupV2.js b/test/helper/CtrlxMockupV2.js index e82e9b5..f4cd949 100644 --- a/test/helper/CtrlxMockupV2.js +++ b/test/helper/CtrlxMockupV2.js @@ -274,6 +274,14 @@ class CtrlxMockupV2 { }, 10); }); + this.app.get('/automation/api/v2/nodes/with/strange/symbols/*', authenticateJWT, (req, res) => { + res.statusCode = 200; + res.json({ + value: req.path, + type: 'string' + }); + }); + // // Builtin Data Mockups - Create/Delete @@ -534,6 +542,10 @@ class CtrlxMockupV2 { data.value = { "valid": "data" }; data.type = 'object'; break; + case 'with/strange/symbols/abc=1;nichts-ist.wahr:("alles[ist]erlaubt")42/x.y.z': + data.value = 23; + data.type = 'double'; + break; default: data.value = 'error: unknown value'; From 402efc961f5de407cd9e4847b6258b0f4f292206 Mon Sep 17 00:00:00 2001 From: "Krauskopf Sebastian (DC-AE/EAR2)" Date: Mon, 27 May 2024 17:17:07 +0200 Subject: [PATCH 3/3] fix: adjust unit-test to node20 --- test/ctrlx-datalayer-subscribe.test.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/ctrlx-datalayer-subscribe.test.js b/test/ctrlx-datalayer-subscribe.test.js index 569abf3..f784801 100644 --- a/test/ctrlx-datalayer-subscribe.test.js +++ b/test/ctrlx-datalayer-subscribe.test.js @@ -494,7 +494,6 @@ describe('ctrlx-datalayer-subscribe', function () { expect(msg).to.have.property('timestamp').to.be.a('number'); expect(msg).to.have.property('type').to.be.a('string').eql('double'); expect(msg).to.have.property('payload').to.be.a('number').eql(23); - s1.subscription.close(); done(); } @@ -504,9 +503,9 @@ describe('ctrlx-datalayer-subscribe', function () { } }); - // We expect to reveive an error message, because the mockup will send an invalid JSON message. + // The node should not return any errors, but just accept the path n1.on('call:error', call => { - //numErrors++; + done(call.firstArg); }); }); @@ -563,7 +562,7 @@ describe('ctrlx-datalayer-subscribe', function () { n1.on('call:error', call => { numErrors++; try { - expect(call.firstArg).eql('Error parsing update event: Unexpected end of JSON input'); + expect(call.firstArg).oneOf(['Error parsing update event: Unexpected end of JSON input', 'Error parsing update event: Unterminated string in JSON at position 50']); } catch (err) { done(err); }