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 240d5b078d486d79f5bfc90f2a1bf84ed7159d99 Mon Sep 17 00:00:00 2001 From: Alex Volanis Date: Fri, 27 Dec 2019 20:56:24 -0500 Subject: [PATCH 3/3] Added requested in depth testing. Query the DB to verify operation. As requested I added an actual DB query in the test. I pretty much copied the query code from the runtime to use in the test but it gets the job done. --- tests/integration-tests.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/integration-tests.js b/tests/integration-tests.js index 52a617a..ac4c120 100644 --- a/tests/integration-tests.js +++ b/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(); @@ -215,11 +221,13 @@ describe( 'The database', () => { it( 'creates the ' + config.scoresTableName + ' table on the first request', async() => { expect.hasAssertions(); 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(); - expect( newScore ).toBe( 1 ); + 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() => { @@ -234,6 +242,10 @@ describe( 'The database', () => { 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(); }); it( 'returns a list of top scores in the correct order', async() => {