From cd2991a394277d29dc1b6df2d4a993a8ada345d5 Mon Sep 17 00:00:00 2001 From: Shusetsu Toda Date: Thu, 5 Sep 2019 16:04:39 +0200 Subject: [PATCH 1/5] :bug: Fix invalid event handling on network event --- framework/src/modules/chain/chain.js | 39 ++++++++++++------- .../src/modules/chain/transport/transport.js | 27 +++---------- .../functional/ws/transport/transport.js | 22 ++++++++++- 3 files changed, 51 insertions(+), 37 deletions(-) diff --git a/framework/src/modules/chain/chain.js b/framework/src/modules/chain/chain.js index 7c9d5a7e797..080b029855a 100644 --- a/framework/src/modules/chain/chain.js +++ b/framework/src/modules/chain/chain.js @@ -170,21 +170,30 @@ module.exports = class Chain { // Avoid receiving blocks/transactions from the network during snapshotting process if (!this.options.loading.rebuildUpToRound) { - this.channel.subscribe('network:event', ({ data: { event, data } }) => { - if (event === 'postTransactions') { - this.transport.postTransactions(data); - return; - } - if (event === 'postSignatures') { - this.transport.postSignatures(data); - return; - } - if (event === 'postBlock') { - this.transport.postBlock(data); - // eslint-disable-next-line no-useless-return - return; - } - }); + this.channel.subscribe( + 'network:event', + async ({ data: { event, data } }) => { + try { + if (event === 'postTransactions') { + await this.transport.postTransactions(data); + return; + } + if (event === 'postSignatures') { + await this.transport.postSignatures(data); + return; + } + if (event === 'postBlock') { + await this.transport.postBlock(data); + return; + } + } catch (error) { + this.logger.info( + { error, event, data }, + 'Received invalid event message', + ); + } + }, + ); } } catch (error) { this.logger.fatal('Chain initialization', { diff --git a/framework/src/modules/chain/transport/transport.js b/framework/src/modules/chain/transport/transport.js index ed7543da5c5..4213c0a78a3 100644 --- a/framework/src/modules/chain/transport/transport.js +++ b/framework/src/modules/chain/transport/transport.js @@ -363,28 +363,16 @@ class Transport { query, }, ); - throw new Error(errors); + throw errors; } let block; - let success = true; - try { - block = blocksUtils.addBlockProperties(query.block); + block = blocksUtils.addBlockProperties(query.block); - // Instantiate transaction classes - block.transactions = this.interfaceAdapters.transactions.fromBlock(block); - - block = blocksUtils.objectNormalize(block); - } catch (e) { - success = false; - this.logger.debug('Block normalization failed', { - err: e.toString(), - module: 'transport', - block: query.block, - }); + // Instantiate transaction classes + block.transactions = this.interfaceAdapters.transactions.fromBlock(block); - // TODO: If there is an error, invoke the applyPenalty action on the Network module once it is implemented. - } + block = blocksUtils.objectNormalize(block); // TODO: endpoint should be protected before if (this.loaderModule.syncing()) { return this.logger.debug( @@ -392,10 +380,7 @@ class Transport { block.id, ); } - if (success) { - return this.blocksModule.receiveBlockFromNetwork(block); - } - return null; + return this.blocksModule.receiveBlockFromNetwork(block); } /** diff --git a/framework/test/mocha/functional/ws/transport/transport.js b/framework/test/mocha/functional/ws/transport/transport.js index e65b23bdea7..f260461bcce 100644 --- a/framework/test/mocha/functional/ws/transport/transport.js +++ b/framework/test/mocha/functional/ws/transport/transport.js @@ -32,7 +32,7 @@ describe('WS transport', () => { let p2p; function postTransaction(transactionToPost) { - p2p.send({ + return p2p.send({ event: 'postTransactions', data: { nonce: 'sYHEDBKcScaAAAYg', @@ -72,6 +72,10 @@ describe('WS transport', () => { goodTransactions.push(transaction); done(); }); + + it('should not crash the application', async () => { + await postTransaction('Invalid transaction'); + }); }); describe('confirmation', () => { @@ -379,6 +383,22 @@ describe('WS transport', () => { }); await p2p.send({ event: 'postBlock', data: { block: testBlock } }); }); + + it('should not crash the application when sending the invalid block', async () => { + await p2p.send({ + event: 'postBlock', + data: { block: { generatorPublicKey: '123' } }, + }); + }); + }); + + describe('postSignatures', () => { + it('should not crash the application when sending the invalid signatures', async () => { + await p2p.send({ + event: 'postSignatures', + data: { signatures: ['not object signature'] }, + }); + }); }); }); }); From 756522d3283360683bf28e2138a82789cac561c4 Mon Sep 17 00:00:00 2001 From: Shusetsu Toda Date: Thu, 5 Sep 2019 16:44:08 +0200 Subject: [PATCH 2/5] :white_check_mark: Fix unit test --- .../unit/modules/chain/transport/transport.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/framework/test/mocha/unit/modules/chain/transport/transport.js b/framework/test/mocha/unit/modules/chain/transport/transport.js index df6457f5fb9..46cb08fb739 100644 --- a/framework/test/mocha/unit/modules/chain/transport/transport.js +++ b/framework/test/mocha/unit/modules/chain/transport/transport.js @@ -895,19 +895,15 @@ describe('transport', () => { beforeEach(async () => { sinonSandbox .stub(blocksModule, 'objectNormalize') - .throws(blockValidationError); - transportModule.postBlock(postBlockQuery); + .throws(new Error(blockValidationError)); }); - it('should call transportModule.logger.debug with "Block normalization failed" and {err: error, module: "transport", block: query.block }', async () => { - expect(transportModule.logger.debug).to.be.calledWith( - 'Block normalization failed', - { - err: blockValidationError.toString(), - module: 'transport', - block: blockMock, - }, - ); + it('should throw an error', async () => { + try { + await transportModule.postBlock(postBlockQuery); + } catch (err) { + expect(err.message).to.equal(blockValidationError); + } }); }); From c8270fb0bea7b5bc7c4471625802e688f74df8a9 Mon Sep 17 00:00:00 2001 From: Shusetsu Toda Date: Fri, 6 Sep 2019 09:05:46 +0200 Subject: [PATCH 3/5] :recycle: Remove data and increase log level --- framework/src/modules/chain/chain.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/framework/src/modules/chain/chain.js b/framework/src/modules/chain/chain.js index 080b029855a..524ef807bd3 100644 --- a/framework/src/modules/chain/chain.js +++ b/framework/src/modules/chain/chain.js @@ -187,8 +187,8 @@ module.exports = class Chain { return; } } catch (error) { - this.logger.info( - { error, event, data }, + this.logger.warn( + { error, event }, 'Received invalid event message', ); } From 203478fd0e7573580ac3cebde52623bc4d5c4a93 Mon Sep 17 00:00:00 2001 From: Shusetsu Toda Date: Fri, 6 Sep 2019 12:11:19 +0200 Subject: [PATCH 4/5] :recycle: Add comment back --- framework/src/modules/chain/transport/transport.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/framework/src/modules/chain/transport/transport.js b/framework/src/modules/chain/transport/transport.js index 4213c0a78a3..5bf2e03e9e7 100644 --- a/framework/src/modules/chain/transport/transport.js +++ b/framework/src/modules/chain/transport/transport.js @@ -363,6 +363,7 @@ class Transport { query, }, ); + // TODO: If there is an error, invoke the applyPenalty action on the Network module once it is implemented. throw errors; } @@ -434,6 +435,7 @@ class Transport { if (errors.length) { this.logger.debug('Invalid signatures body', errors); + // TODO: If there is an error, invoke the applyPenalty action on the Network module once it is implemented. throw errors; } @@ -528,6 +530,7 @@ class Transport { if (errors.length) { this.logger.debug('Invalid transactions body', errors); + // TODO: If there is an error, invoke the applyPenalty action on the Network module once it is implemented. throw errors; } From 85af0f5f0898a7ecb9fa688e378718e6aa5f9e54 Mon Sep 17 00:00:00 2001 From: Shusetsu Toda Date: Fri, 6 Sep 2019 13:42:19 +0200 Subject: [PATCH 5/5] :fire: Remove unnecessary declaration --- framework/src/modules/chain/transport/transport.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/framework/src/modules/chain/transport/transport.js b/framework/src/modules/chain/transport/transport.js index 5bf2e03e9e7..2871039ba9d 100644 --- a/framework/src/modules/chain/transport/transport.js +++ b/framework/src/modules/chain/transport/transport.js @@ -344,13 +344,12 @@ class Transport { * @todo Add @returns tag * @todo Add description of the function */ - async postBlock(query) { + async postBlock(query = {}) { if (!this.constants.broadcasts.active) { return this.logger.debug( 'Receiving blocks disabled by user through config.json', ); } - query = query || {}; const errors = validator.validate(definitions.WSBlocksBroadcast, query); @@ -367,8 +366,7 @@ class Transport { throw errors; } - let block; - block = blocksUtils.addBlockProperties(query.block); + let block = blocksUtils.addBlockProperties(query.block); // Instantiate transaction classes block.transactions = this.interfaceAdapters.transactions.fromBlock(block);