Eliminate potential SQL injection from database queries

.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/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

@@ -196,6 +196,12 @@ describe( 'The database', () => {
 
   const extensionExistsQuery = 'SELECT * FROM pg_extension WHERE extname = \'citext\'';
 
+  // Get the new value.
+  const getCurrentScore = {
+    text: 'SELECT score FROM ' + config.scoresTableName + ' WHERE item = $1;',
+    values: [ defaultItem ]
+  };
+
   it( 'does not yet have the ' + config.scoresTableName + ' table', async() => {
     expect.hasAssertions();
     const dbClient = await postgres.connect();
@@ -214,11 +220,14 @@ 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, '+' );
+    expect( newScore ).toBe( 1 );
     const dbClient = await postgres.connect();
     const query = await dbClient.query( tableExistsQuery );
-    await dbClient.release();
     expect( query.rows[0].exists ).toBeTrue();
+    const queryScore = await dbClient.query( getCurrentScore );
+    expect( queryScore.rows[0].score ).toBe( 1 );
+    await dbClient.release();
   });
 
   it( 'also creates the case-insensitive extension on the first request', async() => {
@@ -229,14 +238,15 @@ 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 );
+    const dbClient = await postgres.connect();
+    const queryScore = await dbClient.query( getCurrentScore );
+    expect( queryScore.rows[0].score ).toBe( 2 );
+    await dbClient.release();
   });
-  /* eslint-enable jest/expect-expect */
 
   it( 'returns a list of top scores in the correct order', async() => {
     expect.hasAssertions();
@@ -253,9 +263,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 );