From 7fd965f481b21cb1b345025971ef936b669900c2 Mon Sep 17 00:00:00 2001 From: Alex Volanis Date: Mon, 23 Dec 2019 16:27:08 -0500 Subject: [PATCH 1/3] Update .travis.yml to solve PostgreSQL issues with new base images Travis CI updated the base images so the hacky PG 10 install is no longer needed and on top of that is failing the builds. --- .travis-postgres10.sh | 20 -------------------- .travis.yml | 9 +++++---- 2 files changed, 5 insertions(+), 24 deletions(-) delete mode 100755 .travis-postgres10.sh diff --git a/.travis-postgres10.sh b/.travis-postgres10.sh deleted file mode 100755 index bca31aa..0000000 --- a/.travis-postgres10.sh +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env bash -# -# Installs PostgreSQL 10 on Travis CI, as the default build images don't support it yet. -# -# @see https://github.com/travis-ci/travis-ci/issues/8537#issuecomment-354020356 - -set -euxo pipefail - -echo "Installing Postgres 10..." -sudo service postgresql stop -sudo apt-get remove --quiet 'postgresql-*' -sudo apt-get update --quiet -sudo apt-get install --quiet postgresql-10 postgresql-client-10 -sudo cp /etc/postgresql/{9.6,10}/main/pg_hba.conf - -echo "Restarting Postgres 10..." -sudo service postgresql restart - -echo "Adding Travis role..." -sudo psql --command="CREATE ROLE travis SUPERUSER LOGIN CREATEDB;" --username="postgres" diff --git a/.travis.yml b/.travis.yml index e96d681..4b04d29 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,7 +16,11 @@ services: # @see https://github.com/travis-ci/travis-ci/issues/8537#issuecomment-354020356 sudo: required addons: - postgresql: 9.6 + postgresql: "10" + apt: + packages: + - postgresql-10 + - postgresql-client-10 cache: yarn: true @@ -29,9 +33,6 @@ before_install: - curl --location https://yarnpkg.com/install.sh | bash -s -- --version "$( node -e "console.log(require('./package.json').engines.yarn)" )" - export PATH="$HOME/.yarn/bin:$PATH" - # Upgrade to PostgreSQL 10. - - ./.travis-postgres10.sh - install: - yarn --frozen-lockfile From 6fa326b3f362c12215c662ad1125d57d3d9d511a Mon Sep 17 00:00:00 2001 From: Alex Volanis Date: Mon, 23 Dec 2019 02:53:42 -0500 Subject: [PATCH 2/3] Eliminate potential SQL injection from database queries The TODO markers indicating the possibility of SQL injection issues were used to guide this implementation. Fixed by applying parameterized queries. Found a unitest issue that was masked by the use of concatenation in SQL and fixed the unit tests to match the runtime code execution. --- src/points.js | 22 +++++++++++++--------- tests/integration-tests.js | 18 ++++++++---------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/points.js b/src/points.js index abc201b..e4c4071 100644 --- a/src/points.js +++ b/src/points.js @@ -73,17 +73,21 @@ const updateScore = async( item, operation ) => { ' ); // Atomically record the action. - // TODO: Fix potential SQL injection issues here, even though we know the input should be safe. - await dbClient.query( '\ - INSERT INTO ' + scoresTableName + ' VALUES (\'' + item + '\', ' + operation + '1) \ - ON CONFLICT (item) DO UPDATE SET score = ' + scoresTableName + '.score ' + operation + ' 1; \ - ' ); + const insertOrUpdateScore = { + text: '\ + INSERT INTO ' + scoresTableName + ' VALUES ($1, $2) \ + ON CONFLICT (item) DO UPDATE SET score = ' + scoresTableName + '.score ' + operation + ' 1; \ + ', + values: [ item, operation + '1' ] + }; + await dbClient.query( insertOrUpdateScore ); // Get the new value. - // TODO: Fix potential SQL injection issues here, even though we know the input should be safe. - const dbSelect = await dbClient.query( '\ - SELECT score FROM ' + scoresTableName + ' WHERE item = \'' + item + '\'; \ - ' ); + const getCurrentScore = { + text: 'SELECT score FROM ' + scoresTableName + ' WHERE item = $1;', + values: [ item ] + }; + const dbSelect = await dbClient.query( getCurrentScore ); await dbClient.release(); const score = dbSelect.rows[0].score; diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 21524cc..52a617a 100644 --- a/tests/integration-tests.js +++ b/tests/integration-tests.js @@ -214,11 +214,12 @@ describe( 'The database', () => { it( 'creates the ' + config.scoresTableName + ' table on the first request', async() => { expect.hasAssertions(); - await points.updateScore( defaultItem, '++' ); + const newScore = await points.updateScore( defaultItem, '+' ); const dbClient = await postgres.connect(); const query = await dbClient.query( tableExistsQuery ); await dbClient.release(); expect( query.rows[0].exists ).toBeTrue(); + expect( newScore ).toBe( 1 ); }); it( 'also creates the case-insensitive extension on the first request', async() => { @@ -229,14 +230,11 @@ describe( 'The database', () => { expect( query.rowCount ).toBe( 1 ); }); - /* eslint-disable jest/expect-expect */ - // TODO: This test really should have an assertion, but I can't figure out how to catch the error - // properly... it's possible that updateScore needs rewriting to catch properly. In the - // meantime, this test *does* actually work like expected. it( 'does not cause any errors on a second request when everything already exists', async() => { - await points.updateScore( defaultItem, '++' ); + expect.hasAssertions(); + const newScore = await points.updateScore( defaultItem, '+' ); + expect( newScore ).toBe( 2 ); }); - /* eslint-enable jest/expect-expect */ it( 'returns a list of top scores in the correct order', async() => { expect.hasAssertions(); @@ -253,9 +251,9 @@ describe( 'The database', () => { ]; // Give us a few additional scores so we can check the order works. - await points.updateScore( defaultUser, '++' ); - await points.updateScore( defaultUser, '++' ); - await points.updateScore( defaultUser, '++' ); + await points.updateScore( defaultUser, '+' ); + await points.updateScore( defaultUser, '+' ); + await points.updateScore( defaultUser, '+' ); const topScores = await points.retrieveTopScores(); expect( topScores ).toEqual( expectedScores ); From 8d507dd3fd7009c2945a20515865ff6535f3b7d0 Mon Sep 17 00:00:00 2001 From: Alex Volanis Date: Sun, 22 Dec 2019 16:35:19 -0500 Subject: [PATCH 3/3] Enhance the app to see multiple mentions in one message Enabled the code to process multiple occurances of the ++/-- syntax in one message iteratively thus handling score updates for multiple recipients in one message. Changed the handler for selfPlus to be processed within the handlePlusMinus handler an simply not increment the score while providing the disapproving random message to the instigator. --- src/events.js | 56 +++++++++++------------- src/helpers.js | 30 ++++++++----- tests/events.js | 109 +++++++++++++++++++++++++++++++---------------- tests/helpers.js | 64 +++++++++++++++++++--------- 4 files changed, 161 insertions(+), 98 deletions(-) diff --git a/src/events.js b/src/events.js index 1611268..fcc487b 100644 --- a/src/events.js +++ b/src/events.js @@ -17,37 +17,36 @@ const slack = require( './slack' ), const camelCase = require( 'lodash.camelcase' ); /** + * Handles a plus or minus against a user, and then notifies the channel of the new score. * Handles an attempt by a user to 'self plus' themselves, which includes both logging the attempt * and letting the user know it wasn't successful. * - * @param {object} user The ID of the user (Uxxxxxxxx) who tried to self plus. - * @param {object} channel The ID of the channel (Cxxxxxxxx for public channels or Gxxxxxxxx for - * private channels - aka groups) that the message was sent from. - * @return {Promise} A Promise to send a Slack message back to the requesting channel. - */ -const handleSelfPlus = ( user, channel ) => { - console.log( user + ' tried to alter their own score.' ); - const message = messages.getRandomMessage( operations.operations.SELF, user ); - return slack.sendMessage( message, channel ); -}; - -/** - * Handles a plus or minus against a user, and then notifies the channel of the new score. - * - * @param {string} item The Slack user ID (if user) or name (if thing) of the item being - * operated on. - * @param {string} operation The mathematical operation performed on the item's score. + * @param {object} mentions Array of parsed objects from a Slack message with objects containing + * the Slack user ID (if user) or name (if thing) of the item being + * operated on and the +/- operation performed on the item's score. + * @param {object} user The ID of the user (Uxxxxxxxx) who sent the message. * @param {object} channel The ID of the channel (Cxxxxxxxx for public channels or Gxxxxxxxx for * private channels - aka groups) that the message was sent from. * @return {Promise} A Promise to send a Slack message back to the requesting channel after the * points have been updated. */ -const handlePlusMinus = async( item, operation, channel ) => { - const score = await points.updateScore( item, operation ), - operationName = operations.getOperationName( operation ), - message = messages.getRandomMessage( operationName, item, score ); +const handlePlusMinus = async( mentions, user, channel ) => { + + const messageLines = []; + for ( const mention of mentions ) { + + // Handle self plus as an event avoiding incrementing the score + if ( mention.item === user && '+' === mention.operation ) { + console.log( user + ' tried to alter their own score.' ); + messageLines.push( messages.getRandomMessage( operations.operations.SELF, user ) ); + } else { + const score = await points.updateScore( mention.item, mention.operation ), + operationName = operations.getOperationName( mention.operation ); + messageLines.push( messages.getRandomMessage( operationName, mention.item, score ) ); + } + } - return slack.sendMessage( message, channel ); + return slack.sendMessage( messageLines.join( '\n' ), channel ); }; /** @@ -122,20 +121,14 @@ const handlers = { message: ( event ) => { // Extract the relevant data from the message text. - const { item, operation } = helpers.extractPlusMinusEventData( event.text ); - - if ( ! item || ! operation ) { - return false; - } + const mentions = helpers.extractPlusMinusEventData( event.text ); - // Bail if the user is trying to ++ themselves... - if ( item === event.user && '+' === operation ) { - handleSelfPlus( event.user, event.channel ); + if ( ! mentions ) { return false; } // Otherwise, let's go! - return handlePlusMinus( item, operation, event.channel ); + return handlePlusMinus( mentions, event.user, event.channel ); }, // Message event. @@ -225,7 +218,6 @@ const handleEvent = ( event, request ) => { }; // HandleEvent. module.exports = { - handleSelfPlus, handlePlusMinus, sayThankyou, sendHelp, diff --git a/src/helpers.js b/src/helpers.js index b5dfeed..c0ca0c6 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -55,22 +55,32 @@ const extractCommand = ( message, commands ) => { * We take the operation down to one character, and also support — due to iOS' replacement of --. * * @param {string} text The message text sent through in the event. - * @returns {object} An object containing both the 'item' being referred to - either a Slack user - * ID (eg. 'U12345678') or the name of a 'thing' (eg. 'NameOfThing'); and the - * 'operation' being done on it - expressed as a valid mathematical operation - * (i.e. + or -). + * @returns {object} An array of objects containing both the 'item' being referred to - either a + * Slack user ID (eg. 'U12345678') or the name of a 'thing' (eg. 'NameOfThing'); + * and the 'operation' being done on it - expressed as a valid mathematical + * operation (i.e. + or -). */ const extractPlusMinusEventData = ( text ) => { - const data = text.match( /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/ ); + const matchAll = /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/g; + const matchOne = /@([A-Za-z0-9]+?)>?\s*(\+{2}|-{2}|—{1})/; - if ( ! data ) { + const matches = text.match( matchAll ); + + if ( null === matches ) { return false; } - return { - item: data[1], - operation: data[2].substring( 0, 1 ).replace( '—', '-' ) - }; + const data = []; + for ( const match of matches ) { + const parts = match.match( matchOne ); + data.push( + { + item: parts[1], + operation: parts[2].substring( 0, 1 ).replace( '—', '-' ) + } + ); + } + return data; }; // ExtractPlusMinusEventData. diff --git a/tests/events.js b/tests/events.js index 8c3b64a..2eb573b 100644 --- a/tests/events.js +++ b/tests/events.js @@ -37,18 +37,38 @@ beforeEach( () => { slack.sendMessage.mockClear(); }); -describe( 'handleSelfPlus', () => { +describe( 'handlePlusMinus', () => { const user = 'U12345678', - channel = 'C12345678'; + item = 'SomeRandomThing', + item2 = 'SomeOtherThing', + channel = 'C12345678', + score = 5; + + /** @returns {integer} Returns a fake score. */ + const updateScoreMock = () => { + return score; + }; it( 'logs an attempt by a user to increment their own score', () => { - events.handleSelfPlus( user, channel ); + const mentions = [ + { + item: user, + operation: '+' + } + ]; + events.handlePlusMinus( mentions, user, channel ); expect( console.log ).toHaveBeenCalledTimes( 1 ); }); it( 'gets a message from the \'self plus\' collection', () => { - events.handleSelfPlus( user, channel ); + const mentions = [ + { + item: user, + operation: '+' + } + ]; + events.handlePlusMinus( mentions, user, channel ); expect( messages.getRandomMessage ) .toHaveBeenCalledTimes( 1 ) @@ -62,26 +82,19 @@ describe( 'handleSelfPlus', () => { slack.sendMessage = jest.fn(); slack.setSlackClient( slackClientMock ); - events.handleSelfPlus( user, channel ); + const mentions = [ + { + item: user, + operation: '+' + } + ]; + events.handlePlusMinus( mentions, user, channel ); expect( slack.sendMessage ) .toHaveBeenCalledTimes( 1 ) .toHaveBeenCalledWith( expect.stringContaining( user ), channel ); }); -}); - -describe( 'handlePlusMinus', () => { - - const item = 'SomeRandomThing', - channel = 'C12345678', - score = 5; - - /** @returns {integer} Returns a fake score. */ - const updateScoreMock = () => { - return score; - }; - it( 'calls the score updater to update an item\'s score', () => { const slack = require( '../src/slack' ), points = require( '../src/points' ), @@ -90,11 +103,24 @@ describe( 'handlePlusMinus', () => { slack.setSlackClient( slackClientMock ); points.updateScore = jest.fn(); - events.handlePlusMinus( item, '+', channel ); + const operation = '+'; + const mentions = [ + { + item, + operation + }, + { + item: item2, + operation + } + ]; + events.handlePlusMinus( mentions, user, channel ).then( () => { + expect( points.updateScore ) + .toHaveBeenCalledTimes( 2 ) + .toHaveBeenNthCalledWith( 1, item, '+' ) + .toHaveBeenNthCalledWith( 2, item2, '+' ); + }); - expect( points.updateScore ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( item, '+' ); }); it.each([ [ 'plus', '+' ], [ 'minus', '-' ] ])( @@ -111,10 +137,21 @@ describe( 'handlePlusMinus', () => { points.updateScore = jest.fn( updateScoreMock ); messages.getRandomMessage = jest.fn(); - return events.handlePlusMinus( item, operation, channel ).then( () => { + const mentions = [ + { + item, + operation + }, + { + item: item2, + operation + } + ]; + return events.handlePlusMinus( mentions, user, channel ).then( () => { expect( messages.getRandomMessage ) - .toHaveBeenCalledTimes( 1 ) - .toHaveBeenCalledWith( operationName, item, score ); + .toHaveBeenCalledTimes( 2 ) + .toHaveBeenNthCalledWith( 1, operationName, item, score ) + .toHaveBeenNthCalledWith( 2, operationName, item2, score ); }); } ); @@ -130,7 +167,17 @@ describe( 'handlePlusMinus', () => { points.updateScore = jest.fn(); slack.sendMessage = jest.fn(); - return events.handlePlusMinus( item, '+', channel ).then( () => { + const mentions = [ + { + item, + operation: '+' + }, + { + item: item2, + operation: '+' + } + ]; + return events.handlePlusMinus( mentions, user, channel ).then( () => { expect( slack.sendMessage ) .toHaveBeenCalledTimes( 1 ) .toHaveBeenCalledWith( expect.stringContaining( item ), channel ); @@ -161,16 +208,6 @@ describe( 'handlers.message', () => { expect( handlers.message( event ) ).toBeFalse(); }); - it( 'returns false if a user trying to ++ themselves', () => { - const event = { - type: eventType, - text: '<@U12345678>++', - user: 'U12345678' - }; - - expect( handlers.message( event ) ).toBeFalse(); - }); - }); // HandleMessageEvent. describe( 'handlers.appMention', () => { diff --git a/tests/helpers.js b/tests/helpers.js index bc50cb3..6c27661 100644 --- a/tests/helpers.js +++ b/tests/helpers.js @@ -74,31 +74,53 @@ describe( 'extractPlusMinusEventData', () => { }); it( 'extracts a \'thing\' and operation from the start of a message', () => { - expect( helpers.extractPlusMinusEventData( '@SomethingRandom++ that was awesome' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); + expect( helpers.extractPlusMinusEventData( '@SomethingRandom++ that was awesome' ) ).toEqual([ + { + item: 'SomethingRandom', + operation: '+' + } + ]); }); it( 'extracts a user and operation from the start of a message', () => { - expect( helpers.extractPlusMinusEventData( '<@U87654321>++ that was awesome' ) ).toEqual({ - item: 'U87654321', - operation: '+' - }); + expect( helpers.extractPlusMinusEventData( '<@U87654321>++ that was awesome' ) ).toEqual([ + { + item: 'U87654321', + operation: '+' + } + ]); }); it( 'extracts data in the middle of a message', () => { - expect( helpers.extractPlusMinusEventData( 'Hey @SomethingRandom++ you\'re great' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); + expect( helpers.extractPlusMinusEventData( 'Hey @SomethingRandom++ you\'re great' ) ).toEqual([ + { + item: 'SomethingRandom', + operation: '+' + } + ]); }); it( 'extracts data at the end of a message', () => { - expect( helpers.extractPlusMinusEventData( 'Awesome work @SomethingRandom++' ) ).toEqual({ - item: 'SomethingRandom', - operation: '+' - }); + expect( helpers.extractPlusMinusEventData( 'Awesome work @SomethingRandom++' ) ).toEqual([ + { + item: 'SomethingRandom', + operation: '+' + } + ]); + }); + + it( 'extracts multiple mentions in one message', () => { + const multiMentions = 'Thing one @SomethingRandom++ and thing two @SomethingElse--'; + expect( helpers.extractPlusMinusEventData( multiMentions ) ).toEqual([ + { + item: 'SomethingRandom', + operation: '+' + }, + { + item: 'SomethingElse', + operation: '-' + } + ]); }); const itemsToMatch = [ @@ -149,10 +171,12 @@ describe( 'extractPlusMinusEventData', () => { it( testName, () => { const result = helpers.extractPlusMinusEventData( messageText ); - expect( result ).toEqual({ - item: item.expected, - operation: operation.expected - }); + expect( result ).toEqual([ + { + item: item.expected, + operation: operation.expected + } + ]); }); } // For iterator.