diff --git a/config/config.js b/config/config.js index 2a49de98..a7195994 100644 --- a/config/config.js +++ b/config/config.js @@ -70,6 +70,12 @@ module.exports = function (fs, path, url, convict) { format: 'duration', env: 'SIGNIN_CODES_MAX_AGE', }, + requiredSQLModes: { + doc: 'Comma-separated list of SQL mode flags to enforce on each connection', + default: '', + format: 'String', + env: 'REQUIRED_SQL_MODES', + }, master: { user: { doc: 'The user to connect to for MySql', diff --git a/lib/db/mysql.js b/lib/db/mysql.js index 4395f806..9fe6dc22 100644 --- a/lib/db/mysql.js +++ b/lib/db/mysql.js @@ -72,18 +72,16 @@ module.exports = function (log, error) { options.master.charset = options.charset options.slave.charset = options.charset - var mode = REQUIRED_SQL_MODES.join(',') - if (options.sql_mode && options.sql_mode !== mode) { - log.error('createPoolCluster.invalidSqlMode', { sql_mode: options.sql_mode }) - throw new Error(`You cannot use any sql mode other than ${mode}`) - } else { - options.sql_mode = REQUIRED_SQL_MODES.join(',') + this.requiredModes = REQUIRED_SQL_MODES + if (options.requiredSQLModes) { + this.requiredModes = options.requiredSQLModes.split(',') + this.requiredModes.forEach(mode => { + if (! /^[A-Z0-9_]+$/.test(mode)) { + throw new Error('Invalid SQL mode: ' + mode) + } + }) } - options.master.sql_mode = options.sql_mode - options.slave.sql_mode = options.sql_mode - - // Use separate pools for master and slave connections. this.poolCluster.add('MASTER', options.master) this.poolCluster.add('SLAVE', options.slave) @@ -1155,22 +1153,37 @@ module.exports = function (log, error) { return resolve(connection) } - var mode = REQUIRED_SQL_MODES.join(',') - connection.query(`SET SESSION sql_mode = '${mode}';`, (err) => { + // Enforce sane defaults on every new connection. + // These *should* be set by the database by default, but it's nice + // to have an additional layer of protection here. + connection.query('SELECT @@sql_mode AS mode;', (err, rows) => { if (err) { return reject(err) } - connection.query('SET NAMES utf8mb4 COLLATE utf8mb4_bin;', (err) => { + const currentModes = rows[0]['mode'].split(',') + this.requiredModes.forEach(requiredMode => { + if (currentModes.indexOf(requiredMode) === -1) { + currentModes.push(requiredMode) + } + }) + + const newMode = currentModes.join(',') + connection.query(`SET SESSION sql_mode = '${newMode}';`, (err) => { if (err) { return reject(err) } - connection._fxa_initialized = true - resolve(connection) + connection.query('SET NAMES utf8mb4 COLLATE utf8mb4_bin;', (err) => { + if (err) { + return reject(err) + } + + connection._fxa_initialized = true + resolve(connection) + }) }) }) - }) }) } diff --git a/test/local/mysql_tests.js b/test/local/mysql_tests.js index 9c5ea38e..7967067a 100644 --- a/test/local/mysql_tests.js +++ b/test/local/mysql_tests.js @@ -26,7 +26,7 @@ describe('MySQL', () => { }) it( - 'forces REQUIRED_CHARSET for connections', + 'validates REQUIRED_CHARSET for connections', () => { const configCharset = Object.assign({}, config) configCharset.charset = 'wat' @@ -42,20 +42,51 @@ describe('MySQL', () => { ) it( - 'forces REQUIRED_SQL_MODES for connections', + 'accepts REQUIRED_SQL_MODES from config', () => { - const configSqlMode = Object.assign({}, config) - const REQUIRED_SQL_MODES = [ - 'STRICT_ALL_TABLES', - 'NO_ENGINE_SUBSTITUTION', - ] - var mode = REQUIRED_SQL_MODES.join(',') - configSqlMode.sql_mode = 'xyz' - return DB.connect(configSqlMode) + const configModes = Object.assign({}, config) + configModes.requiredSQLModes = 'STRICT_TRANS_TABLES,NO_ZERO_DATE' + + return DB.connect(configModes) + .then( + db => { + assert.deepEqual(db.requiredModes, [ + 'STRICT_TRANS_TABLES', + 'NO_ZERO_DATE' + ]) + }, + assert.fail + ) + } + ) + + it( + 'rejects unrecognized REQUIRED_SQL_MODES values from config', + () => { + const configModes = Object.assign({}, config) + configModes.requiredSQLModes = 'UNRECOGNIZED_SQL_MODE_NONSENSE' + + return DB.connect(configModes) + .then( + assert.fail, + err => { + assert.equal(err.message, 'ER_WRONG_VALUE_FOR_VAR') + } + ) + } + ) + + it( + 'rejects badly-formed REQUIRED_SQL_MODES from config, for safety', + () => { + const configModes = Object.assign({}, config) + configModes.requiredSQLModes = 'TEST,MODE,\'; DROP TABLE users;' + + return DB.connect(configModes) .then( assert.fail, err => { - assert.equal(err.message, `You cannot use any sql mode other than ${mode}`) + assert.ok(err.message.indexOf('Invalid SQL mode') === 0) } ) }