Fix sql injection and add multiple mentions

.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
 

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,

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.
 

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;

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', () => {