From d2a045a237aca2fe5546f0b4a4bb31951c7641fd Mon Sep 17 00:00:00 2001 From: Steve Walsh Date: Mon, 12 Jul 2021 12:47:48 +0100 Subject: [PATCH 1/5] Added logic to fix the org variable if its been changed --- nodes/PayloadValidator.js | 87 ++++++--- package-lock.json | 3 +- package.json | 4 +- test/unit/test-payloadValidator.js | 279 ++++++++++++++++++++--------- 4 files changed, 253 insertions(+), 120 deletions(-) diff --git a/nodes/PayloadValidator.js b/nodes/PayloadValidator.js index 273ee1d40..c8619e72f 100644 --- a/nodes/PayloadValidator.js +++ b/nodes/PayloadValidator.js @@ -1,4 +1,5 @@ const clone = require('clone'); +const _ = require('lodash'); const variablesToCheck = [ 'logger.metadata.organization', @@ -41,40 +42,70 @@ module.exports = class PayloadValidator { } } + /** + * Gets the value at the given location in the given object + * + * @param {*} object + * @param {*} location + * @returns + */ getValue(object, location) { - return location.split('.').reduce((p, c) => (p && p[c]) || null, object); + return _.get(object, location); + } + + /** + * Sets the value at the given location in the given object to the given value + * + * @param {*} object + * @param {*} location + * @param {*} value + * @returns + */ + setValue(object, location, value) { + return _.set(object, location, value); + } + + check(_after) { + let after = _after; + try { + variablesToCheck.forEach((location) => { + const beforeValue = this.getValue(this.before, location); + const afterValue = this.getValue(after, location); + if (beforeValue !== afterValue) { + const details = { + message: `msg.${location} changed from "${beforeValue}" to "${afterValue}" for bot "${this.bot}"`, + nodeId: this.nodeId, + workerId: this.workerId + }; + this.logger.error(details.message); + this.logger.app.platform.organization({ + srn: `srn:botnet:${this.region}:${this.organization}:bot:${this.bot}`, + action: 'exception', + actionType: 'invalid-payload-modification', + details, + conversationId: this.conversationId + }); + after = this.setValue(after, location, beforeValue); + } + }); + } catch (e) { + console.log('Error while trying to verify variable changes'); + console.log(e); + } + return after; } verify(_after) { + const after = _after; if (this.isValidBefore) { - try { - let after = _after; - if (Array.isArray(after)) { - after = after.find((msg) => !!msg); - } - variablesToCheck.forEach((location) => { - if (this.getValue(this.before, location) !== this.getValue(after, location)) { - const details = { - message: `msg.${location} changed from "${this.getValue(this.before, location)}" to "${this.getValue(after, location)}" for bot "${this.bot}"`, - nodeId: this.nodeId, - workerId: this.workerId - }; - this.logger.error(details.message); - this.logger.app.platform.organization({ - srn: `srn:botnet:${this.region}:${this.organization}:bot:${this.bot}`, - action: 'exception', - actionType: 'invalid-payload-modification', - details, - conversationId: this.conversationId - }); - } - }); - } catch (e) { - console.log('Error while trying to verify variable changes'); - console.log(e); + if (Array.isArray(after)) { + const afterIndex = after.findIndex((msg) => !!msg); + after[afterIndex] = this.check(after[afterIndex]); + return after; } - } else { - console.log('Error while trying to verify variable changes, wasn\'t initted with correct object'); + return this.check(after); } + console.log('Error while trying to verify variable changes, wasn\'t initted with correct object'); + return after; } }; diff --git a/package-lock.json b/package-lock.json index 7ef7a2c12..55a2c917f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5104,8 +5104,7 @@ "lodash": { "version": "4.17.21", "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", - "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==", - "dev": true + "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==" }, "lodash.assign": { "version": "4.2.0", diff --git a/package.json b/package.json index bb09a0a71..145f9f1d0 100644 --- a/package.json +++ b/package.json @@ -11,9 +11,8 @@ "main": "red/red.js", "scripts": { "start": "node red.js", - "test": "grunt test-nodes", "build": "grunt build", - "test:unit": "nyc node_modules/.bin/mocha --recursive test/unit/*" + "test": "nyc node_modules/.bin/mocha --recursive test/unit/*" }, "bin": { "node-red": "./red.js", @@ -54,6 +53,7 @@ "js-yaml": "3.11.0", "json-stringify-safe": "5.0.1", "jsonata": "1.5.4", + "lodash": "^4.17.21", "media-typer": "0.3.0", "memorystore": "1.6.0", "mqtt": "2.18.0", diff --git a/test/unit/test-payloadValidator.js b/test/unit/test-payloadValidator.js index 36d7e3f6d..1e046113f 100644 --- a/test/unit/test-payloadValidator.js +++ b/test/unit/test-payloadValidator.js @@ -1,142 +1,192 @@ -const PayloadValidator = require('../../nodes/PayloadValidator') -const orgEvent = require('./fixtures/data/orgEvent') const sinon = require('sinon'); const assert = require('assert'); +const PayloadValidator = require('../../nodes/PayloadValidator'); +const orgEvent = require('./fixtures/data/orgEvent'); -describe.only('Unit: PayloadValidator', () => { +describe('Unit: PayloadValidator', () => { it('Should not log when no changes', () => { - const nodeId = 'abc-id' - const workerId = 'worker-id' + const nodeId = 'abc-id'; + const workerId = 'worker-id'; const beforeEvent = orgEvent('before', workerId); const payloadValidator = new PayloadValidator(beforeEvent, `before-${workerId}-${nodeId}`); payloadValidator.verify(beforeEvent); }); - it('Should warn when org is overwritten', () => { - const nodeId = 'abc-id' - const workerId = 'worker-id' + it('Should warn when org is overwritten and fix it', () => { + const nodeId = 'abc-id'; + const workerId = 'worker-id'; const beforeEvent = orgEvent('before', workerId); - errorLogStub = sinon.stub(); - appLogStub = sinon.stub(); + const errorLogStub = sinon.stub(); + const appLogStub = sinon.stub(); beforeEvent.logger.error = errorLogStub; beforeEvent.logger.app = { - platform:{ + platform: { organization: appLogStub } }; - + const payloadValidator = new PayloadValidator(beforeEvent, `before-${workerId}-${nodeId}`); const modifiedEvent = orgEvent('after'); - payloadValidator.verify(modifiedEvent); - assert(errorLogStub.callCount === 4) - assert(appLogStub.callCount === 4) - const [[log1], [log2], [log3], [log4]] = appLogStub.args - assert(log1.details.message.includes('logger.metadata.organization')) - assert.strictEqual(log1.details.nodeId, nodeId) - assert.strictEqual(log1.details.workerId, workerId) - assert(log2.details.message.includes('payload.system.organization')) - assert.strictEqual(log2.details.nodeId, nodeId) - assert.strictEqual(log2.details.workerId, workerId) - assert(log3.details.message.includes('event.event.organization')) - assert.strictEqual(log3.details.nodeId, nodeId) - assert.strictEqual(log3.details.workerId, workerId) - assert(log4.details.message.includes('event.event.token.contents.organization')) - assert.strictEqual(log4.details.nodeId, nodeId) - assert.strictEqual(log4.details.workerId, workerId) + const result = payloadValidator.verify(modifiedEvent); + + assert(errorLogStub.callCount === 4); + assert(appLogStub.callCount === 4); + const [[log1], [log2], [log3], [log4]] = appLogStub.args; + assert(log1.details.message.includes('logger.metadata.organization')); + assert.strictEqual(log1.details.nodeId, nodeId); + assert.strictEqual(log1.details.workerId, workerId); + assert(log2.details.message.includes('payload.system.organization')); + assert.strictEqual(log2.details.nodeId, nodeId); + assert.strictEqual(log2.details.workerId, workerId); + assert(log3.details.message.includes('event.event.organization')); + assert.strictEqual(log3.details.nodeId, nodeId); + assert.strictEqual(log3.details.workerId, workerId); + assert(log4.details.message.includes('event.event.token.contents.organization')); + assert.strictEqual(log4.details.nodeId, nodeId); + assert.strictEqual(log4.details.workerId, workerId); + // deleting due to sinon funness + delete result.logger.error; + delete result.logger.app; + delete beforeEvent.logger.error; + delete beforeEvent.logger.app; + assert.deepStrictEqual(result, beforeEvent); }); - - it('Should warn when org is deleted', () => { - const nodeId = 'abc-id' - const workerId = 'worker-id' + it('Should warn when org is deleted and fix it', () => { + const nodeId = 'abc-id'; + const workerId = 'worker-id'; const beforeEvent = orgEvent('before', workerId); - errorLogStub = sinon.stub(); - appLogStub = sinon.stub(); + const errorLogStub = sinon.stub(); + const appLogStub = sinon.stub(); beforeEvent.logger.error = errorLogStub; beforeEvent.logger.app = { - platform:{ + platform: { organization: appLogStub } }; const payloadValidator = new PayloadValidator(beforeEvent, `before-${workerId}-${nodeId}`); - delete beforeEvent.logger.metadata.organization; delete beforeEvent.payload.system.organization; delete beforeEvent.event.event.organization; delete beforeEvent.event.event.token.contents.organization; - payloadValidator.verify(beforeEvent); - assert(errorLogStub.callCount === 4) - assert(appLogStub.callCount === 4) - const [[log1], [log2], [log3], [log4]] = appLogStub.args - assert(log1.details.message.includes('logger.metadata.organization')) - assert.strictEqual(log1.details.nodeId, nodeId) - assert.strictEqual(log1.details.workerId, workerId) - assert(log2.details.message.includes('payload.system.organization')) - assert.strictEqual(log2.details.nodeId, nodeId) - assert.strictEqual(log2.details.workerId, workerId) - assert(log3.details.message.includes('event.event.organization')) - assert.strictEqual(log3.details.nodeId, nodeId) - assert.strictEqual(log3.details.workerId, workerId) - assert(log4.details.message.includes('event.event.token.contents.organization')) - assert.strictEqual(log4.details.nodeId, nodeId) - assert.strictEqual(log4.details.workerId, workerId) + const result = payloadValidator.verify(beforeEvent); + assert(errorLogStub.callCount === 4); + assert(appLogStub.callCount === 4); + const [[log1], [log2], [log3], [log4]] = appLogStub.args; + assert(log1.details.message.includes('logger.metadata.organization')); + assert.strictEqual(log1.details.nodeId, nodeId); + assert.strictEqual(log1.details.workerId, workerId); + assert(log2.details.message.includes('payload.system.organization')); + assert.strictEqual(log2.details.nodeId, nodeId); + assert.strictEqual(log2.details.workerId, workerId); + assert(log3.details.message.includes('event.event.organization')); + assert.strictEqual(log3.details.nodeId, nodeId); + assert.strictEqual(log3.details.workerId, workerId); + assert(log4.details.message.includes('event.event.token.contents.organization')); + assert.strictEqual(log4.details.nodeId, nodeId); + assert.strictEqual(log4.details.workerId, workerId); + + // deleting due to sinon funness + delete result.logger.error; + delete result.logger.app; + delete beforeEvent.logger.error; + delete beforeEvent.logger.app; + assert.deepStrictEqual(result, beforeEvent); }); - it('Should handle when after is an array', () =>{ - const nodeId = 'abc-id' - const workerId = 'worker-id' + + it('Should handle when after is an array and not find any issues', () => { + const nodeId = 'abc-id'; + const workerId = 'worker-id'; const beforeEvent = orgEvent('before', workerId); - errorLogStub = sinon.stub(); - appLogStub = sinon.stub(); + const errorLogStub = sinon.stub(); + const appLogStub = sinon.stub(); beforeEvent.logger.error = errorLogStub; beforeEvent.logger.app = { - platform:{ + platform: { organization: appLogStub } }; - + const payloadValidator = new PayloadValidator(beforeEvent, `before-${workerId}-${nodeId}`); - const modifiedEvent = orgEvent('before'); - modifiedEvent.logger.metadata.organization = 'after' - payloadValidator.verify([modifiedEvent]); - assert(errorLogStub.callCount === 1) - assert(appLogStub.callCount === 1) - const [[log1]] = appLogStub.args - assert(log1.details.message.includes('logger.metadata.organization')) - assert.strictEqual(log1.details.nodeId, nodeId) - assert.strictEqual(log1.details.workerId, workerId) - }) + const after = orgEvent('before'); + const [result] = payloadValidator.verify([after]); + // deleting due to sinon funness + delete result.logger.error; + delete result.logger.app; + delete beforeEvent.logger.error; + delete beforeEvent.logger.app; + assert.deepStrictEqual(result, beforeEvent); + }); - it('Should handle when after is an array in something other than first position', () =>{ - const nodeId = 'abc-id' - const workerId = 'worker-id' + it('Should handle when after is an array', () => { + const nodeId = 'abc-id'; + const workerId = 'worker-id'; const beforeEvent = orgEvent('before', workerId); - errorLogStub = sinon.stub(); - appLogStub = sinon.stub(); + const errorLogStub = sinon.stub(); + const appLogStub = sinon.stub(); beforeEvent.logger.error = errorLogStub; beforeEvent.logger.app = { - platform:{ + platform: { organization: appLogStub } }; - + const payloadValidator = new PayloadValidator(beforeEvent, `before-${workerId}-${nodeId}`); const modifiedEvent = orgEvent('before'); - modifiedEvent.logger.metadata.organization = 'after' - payloadValidator.verify([null, null, modifiedEvent, null]); - assert(errorLogStub.callCount === 1) - assert(appLogStub.callCount === 1) - const [[log1]] = appLogStub.args - assert(log1.details.message.includes('logger.metadata.organization')) - assert.strictEqual(log1.details.nodeId, nodeId) - assert.strictEqual(log1.details.workerId, workerId) - }) - + modifiedEvent.logger.metadata.organization = 'after'; + const [result] = payloadValidator.verify([modifiedEvent]); + assert(errorLogStub.callCount === 1); + assert(appLogStub.callCount === 1); + const [[log1]] = appLogStub.args; + assert(log1.details.message.includes('logger.metadata.organization')); + assert.strictEqual(log1.details.nodeId, nodeId); + assert.strictEqual(log1.details.workerId, workerId); + // deleting due to sinon funness + delete result.logger.error; + delete result.logger.app; + delete beforeEvent.logger.error; + delete beforeEvent.logger.app; + assert.deepStrictEqual(result, beforeEvent); + }); + + it('Should handle when after is an array in something other than first position', () => { + const nodeId = 'abc-id'; + const workerId = 'worker-id'; + const beforeEvent = orgEvent('before', workerId); + const errorLogStub = sinon.stub(); + const appLogStub = sinon.stub(); + beforeEvent.logger.error = errorLogStub; + beforeEvent.logger.app = { + platform: { + organization: appLogStub + } + }; + + const payloadValidator = new PayloadValidator(beforeEvent, `before-${workerId}-${nodeId}`); + + const modifiedEvent = orgEvent('before'); + modifiedEvent.logger.metadata.organization = 'after'; + const [,, result] = payloadValidator.verify([null, null, modifiedEvent, null]); + assert(errorLogStub.callCount === 1); + assert(appLogStub.callCount === 1); + const [[log1]] = appLogStub.args; + assert(log1.details.message.includes('logger.metadata.organization')); + assert.strictEqual(log1.details.nodeId, nodeId); + assert.strictEqual(log1.details.workerId, workerId); + // deleting due to sinon funness + delete result.logger.error; + delete result.logger.app; + delete beforeEvent.logger.error; + delete beforeEvent.logger.app; + assert.deepStrictEqual(result, beforeEvent); + }); + it('Should not die when error', () => { const beforeEvent = orgEvent('before'); const payloadValidator = new PayloadValidator(beforeEvent); @@ -155,5 +205,58 @@ describe.only('Unit: PayloadValidator', () => { payloadValidator.verify({}); }); - -}); \ No newline at end of file + describe('Get Value', () => { + let payloadValidator; + before(() => { + const event = orgEvent('event'); + payloadValidator = new PayloadValidator(event); + }); + it('Should set a root level variable', () => { + const location = 'hello'; + const value = 'world'; + const object = { + [location]: value + }; + const result = payloadValidator.getValue(object, location); + assert.strictEqual(result, value); + }); + + it('Should set a nested variable', () => { + const value = 'world'; + const object = { + should: { + have: { + hello: value + } + } + }; + const location = 'should.have.hello'; + + const result = payloadValidator.getValue(object, location); + assert.strictEqual(result, value); + }); + }); + + describe('Set Value', () => { + let payloadValidator; + before(() => { + const event = orgEvent('event'); + payloadValidator = new PayloadValidator(event); + }); + it('Should set a root level variable', () => { + const object = {}; + const location = 'hello'; + const value = 'world'; + const result = payloadValidator.setValue(object, location, value); + assert.strictEqual(result[location], value); + }); + + it('Should set a nested variable', () => { + const object = {}; + const location = 'should.set.hello'; + const value = 'world'; + const result = payloadValidator.setValue(object, location, value); + assert.strictEqual(result.should.set.hello, value); + }); + }); +}); From 10771a20199941b23b1ed06ba9d5f11f068275c5 Mon Sep 17 00:00:00 2001 From: Steve Walsh Date: Tue, 13 Jul 2021 16:10:42 +0100 Subject: [PATCH 2/5] Updated logic to handle when parts of the msg object are changed to strings --- nodes/PayloadValidator.js | 19 ++++++++ test/unit/test-payloadValidator.js | 69 ++++++++++++++++++++++++------ 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/nodes/PayloadValidator.js b/nodes/PayloadValidator.js index c8619e72f..b7f6c0c89 100644 --- a/nodes/PayloadValidator.js +++ b/nodes/PayloadValidator.js @@ -65,6 +65,21 @@ module.exports = class PayloadValidator { return _.set(object, location, value); } + /** + * lodash set will always pass but might not actually set the value + * So here we test that it was actually set and use that to confirm we can set + * + * @param {*} object + * @param {*} location + * @returns + */ + canSet(object, location) { + const test = 'test'; + let clonedObject = clone(object); + clonedObject = this.setValue(clonedObject, location, test); + return test === this.getValue(clonedObject, location); + } + check(_after) { let after = _after; try { @@ -85,6 +100,10 @@ module.exports = class PayloadValidator { details, conversationId: this.conversationId }); + + if (!this.canSet(after, location)) { + throw new Error(`Cant set value as ${location} is no longer present in after`); + } after = this.setValue(after, location, beforeValue); } }); diff --git a/test/unit/test-payloadValidator.js b/test/unit/test-payloadValidator.js index 1e046113f..66bfba502 100644 --- a/test/unit/test-payloadValidator.js +++ b/test/unit/test-payloadValidator.js @@ -26,9 +26,7 @@ describe('Unit: PayloadValidator', () => { }; const payloadValidator = new PayloadValidator(beforeEvent, `before-${workerId}-${nodeId}`); - const modifiedEvent = orgEvent('after'); - const result = payloadValidator.verify(modifiedEvent); assert(errorLogStub.callCount === 4); @@ -54,6 +52,38 @@ describe('Unit: PayloadValidator', () => { assert.deepStrictEqual(result, beforeEvent); }); + it('Should warn when org is overwritten and fail to fix it due to overwriting payload', () => { + const nodeId = 'abc-id'; + const workerId = 'worker-id'; + const beforeEvent = orgEvent('before', workerId); + const errorLogStub = sinon.stub(); + const appLogStub = sinon.stub(); + beforeEvent.logger.error = errorLogStub; + beforeEvent.logger.app = { + platform: { + organization: appLogStub + } + }; + + const payloadValidator = new PayloadValidator(beforeEvent, `before-${workerId}-${nodeId}`); + const modifiedEvent = orgEvent('before'); + modifiedEvent.payload = 'some text'; + const result = payloadValidator.verify(modifiedEvent); + console.log(errorLogStub.callCount); + assert(errorLogStub.callCount === 1); + assert(appLogStub.callCount === 1); + const [[log]] = appLogStub.args; + assert(log.details.message.includes('msg.payload.system.organization')); + assert.strictEqual(log.details.nodeId, nodeId); + assert.strictEqual(log.details.workerId, workerId); + // deleting due to sinon funness + delete result.logger.error; + delete result.logger.app; + delete modifiedEvent.logger.error; + delete modifiedEvent.logger.app; + assert.deepStrictEqual(result, modifiedEvent); + }); + it('Should warn when org is deleted and fix it', () => { const nodeId = 'abc-id'; const workerId = 'worker-id'; @@ -187,15 +217,6 @@ describe('Unit: PayloadValidator', () => { assert.deepStrictEqual(result, beforeEvent); }); - it('Should not die when error', () => { - const beforeEvent = orgEvent('before'); - const payloadValidator = new PayloadValidator(beforeEvent); - - const modifiedEvent = orgEvent('after'); - - payloadValidator.verify(modifiedEvent); - }); - it('Should not die with initiating the class with bad object', () => { const payloadValidator = new PayloadValidator({}); }); @@ -209,7 +230,7 @@ describe('Unit: PayloadValidator', () => { let payloadValidator; before(() => { const event = orgEvent('event'); - payloadValidator = new PayloadValidator(event); + payloadValidator = new PayloadValidator(event, 'before-worker-id-nodeId'); }); it('Should set a root level variable', () => { const location = 'hello'; @@ -241,7 +262,7 @@ describe('Unit: PayloadValidator', () => { let payloadValidator; before(() => { const event = orgEvent('event'); - payloadValidator = new PayloadValidator(event); + payloadValidator = new PayloadValidator(event, 'before-worker-id-nodeId'); }); it('Should set a root level variable', () => { const object = {}; @@ -259,4 +280,26 @@ describe('Unit: PayloadValidator', () => { assert.strictEqual(result.should.set.hello, value); }); }); + + describe('Can set', () => { + let payloadValidator; + before(() => { + const event = orgEvent('event'); + payloadValidator = new PayloadValidator(event, 'before-worker-id-nodeId'); + }); + + it('Should get back false for canSet when object is a string', () => { + const object = 'some string'; + const location = 'hello'; + const result = payloadValidator.canSet(object, location); + assert.strictEqual(result, false); + }); + + it('Should get back true for canSet', () => { + const object = {}; + const location = 'should.set.hello'; + const result = payloadValidator.canSet(object, location); + assert.strictEqual(result, true); + }); + }); }); From 9a435217eca613a360f9e03a51e11ea4ca6ee533 Mon Sep 17 00:00:00 2001 From: Steve Walsh Date: Tue, 13 Jul 2021 16:40:32 +0100 Subject: [PATCH 3/5] Updated the logic to be easier to follow --- nodes/PayloadValidator.js | 21 ++++----------------- test/unit/test-payloadValidator.js | 22 ---------------------- 2 files changed, 4 insertions(+), 39 deletions(-) diff --git a/nodes/PayloadValidator.js b/nodes/PayloadValidator.js index b7f6c0c89..61e77b2aa 100644 --- a/nodes/PayloadValidator.js +++ b/nodes/PayloadValidator.js @@ -65,21 +65,6 @@ module.exports = class PayloadValidator { return _.set(object, location, value); } - /** - * lodash set will always pass but might not actually set the value - * So here we test that it was actually set and use that to confirm we can set - * - * @param {*} object - * @param {*} location - * @returns - */ - canSet(object, location) { - const test = 'test'; - let clonedObject = clone(object); - clonedObject = this.setValue(clonedObject, location, test); - return test === this.getValue(clonedObject, location); - } - check(_after) { let after = _after; try { @@ -101,10 +86,12 @@ module.exports = class PayloadValidator { conversationId: this.conversationId }); - if (!this.canSet(after, location)) { + + after = this.setValue(after, location, beforeValue); + + if (!_.has(after, location, beforeValue)) { throw new Error(`Cant set value as ${location} is no longer present in after`); } - after = this.setValue(after, location, beforeValue); } }); } catch (e) { diff --git a/test/unit/test-payloadValidator.js b/test/unit/test-payloadValidator.js index 66bfba502..8d01ddd59 100644 --- a/test/unit/test-payloadValidator.js +++ b/test/unit/test-payloadValidator.js @@ -280,26 +280,4 @@ describe('Unit: PayloadValidator', () => { assert.strictEqual(result.should.set.hello, value); }); }); - - describe('Can set', () => { - let payloadValidator; - before(() => { - const event = orgEvent('event'); - payloadValidator = new PayloadValidator(event, 'before-worker-id-nodeId'); - }); - - it('Should get back false for canSet when object is a string', () => { - const object = 'some string'; - const location = 'hello'; - const result = payloadValidator.canSet(object, location); - assert.strictEqual(result, false); - }); - - it('Should get back true for canSet', () => { - const object = {}; - const location = 'should.set.hello'; - const result = payloadValidator.canSet(object, location); - assert.strictEqual(result, true); - }); - }); }); From 3596c030c80513b2cb909c2826c8da4d66a6f4f7 Mon Sep 17 00:00:00 2001 From: Steve Walsh Date: Tue, 13 Jul 2021 17:06:37 +0100 Subject: [PATCH 4/5] Updated the logic to be easier to follow --- nodes/PayloadValidator.js | 60 ++++++++++++++++++------------ test/unit/test-payloadValidator.js | 1 - 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/nodes/PayloadValidator.js b/nodes/PayloadValidator.js index 61e77b2aa..73483b242 100644 --- a/nodes/PayloadValidator.js +++ b/nodes/PayloadValidator.js @@ -65,32 +65,47 @@ module.exports = class PayloadValidator { return _.set(object, location, value); } - check(_after) { - let after = _after; + /** + * Log that we had a invalid payload modification + * @param {*} beforeValue + * @param {*} afterValue + * @param {*} location + */ + logException(beforeValue, afterValue, location) { + const details = { + message: `msg.${location} changed from "${beforeValue}" to "${afterValue}" for bot "${this.bot}"`, + nodeId: this.nodeId, + workerId: this.workerId + }; + this.logger.error(details.message); + this.logger.app.platform.organization({ + srn: `srn:botnet:${this.region}:${this.organization}:bot:${this.bot}`, + action: 'exception', + actionType: 'invalid-payload-modification', + details, + conversationId: this.conversationId + }); + } + + /** + * Ensures that the organization in the before matches the org in the after + * If they are different then it attempts to set the organization back to the correct one + * @param {*} after + * @returns + */ + ensureOrganizationNotModified(after) { try { variablesToCheck.forEach((location) => { const beforeValue = this.getValue(this.before, location); const afterValue = this.getValue(after, location); if (beforeValue !== afterValue) { - const details = { - message: `msg.${location} changed from "${beforeValue}" to "${afterValue}" for bot "${this.bot}"`, - nodeId: this.nodeId, - workerId: this.workerId - }; - this.logger.error(details.message); - this.logger.app.platform.organization({ - srn: `srn:botnet:${this.region}:${this.organization}:bot:${this.bot}`, - action: 'exception', - actionType: 'invalid-payload-modification', - details, - conversationId: this.conversationId - }); + this.logException(beforeValue, afterValue, location); - + // attempt to set the value back to its correct one after = this.setValue(after, location, beforeValue); - + if (!_.has(after, location, beforeValue)) { - throw new Error(`Cant set value as ${location} is no longer present in after`); + this.logger.error(`Cant set value as ${location} is no longer assessable in after`); } } }); @@ -101,15 +116,14 @@ module.exports = class PayloadValidator { return after; } - verify(_after) { - const after = _after; + verify(after) { if (this.isValidBefore) { if (Array.isArray(after)) { - const afterIndex = after.findIndex((msg) => !!msg); - after[afterIndex] = this.check(after[afterIndex]); + const afterIndex = after.findIndex(msg => !!msg); + after[afterIndex] = this.ensureOrganizationNotModified(after[afterIndex]); return after; } - return this.check(after); + return this.ensureOrganizationNotModified(after); } console.log('Error while trying to verify variable changes, wasn\'t initted with correct object'); return after; diff --git a/test/unit/test-payloadValidator.js b/test/unit/test-payloadValidator.js index 8d01ddd59..7c8334e06 100644 --- a/test/unit/test-payloadValidator.js +++ b/test/unit/test-payloadValidator.js @@ -69,7 +69,6 @@ describe('Unit: PayloadValidator', () => { const modifiedEvent = orgEvent('before'); modifiedEvent.payload = 'some text'; const result = payloadValidator.verify(modifiedEvent); - console.log(errorLogStub.callCount); assert(errorLogStub.callCount === 1); assert(appLogStub.callCount === 1); const [[log]] = appLogStub.args; From 4a9002afa5d0bc6423d612193ad0ec28f64d5a1c Mon Sep 17 00:00:00 2001 From: Steve Walsh Date: Tue, 13 Jul 2021 18:25:05 +0100 Subject: [PATCH 5/5] PR feedback --- nodes/PayloadValidator.js | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/nodes/PayloadValidator.js b/nodes/PayloadValidator.js index 73483b242..ef90079b2 100644 --- a/nodes/PayloadValidator.js +++ b/nodes/PayloadValidator.js @@ -94,25 +94,21 @@ module.exports = class PayloadValidator { * @returns */ ensureOrganizationNotModified(after) { - try { - variablesToCheck.forEach((location) => { - const beforeValue = this.getValue(this.before, location); - const afterValue = this.getValue(after, location); - if (beforeValue !== afterValue) { - this.logException(beforeValue, afterValue, location); + variablesToCheck.forEach((location) => { + const beforeValue = this.getValue(this.before, location); + const afterValue = this.getValue(after, location); + if (beforeValue !== afterValue) { + this.logException(beforeValue, afterValue, location); - // attempt to set the value back to its correct one - after = this.setValue(after, location, beforeValue); + // attempt to set the value back to its correct one + after = this.setValue(after, location, beforeValue); - if (!_.has(after, location, beforeValue)) { - this.logger.error(`Cant set value as ${location} is no longer assessable in after`); - } + if (!_.has(after, location, beforeValue)) { + this.logger.error(`Cant set value as ${location} is no longer assessable in after`); } - }); - } catch (e) { - console.log('Error while trying to verify variable changes'); - console.log(e); - } + } + }); + return after; }