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.

Author: Alex-Vol-SV

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/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 );