feat(mysql): Add config option for REQUIRED_SQL_MODES. (#334) r=@philbooth,@vladikoff

This also tweaks the implementation to preserve any existing mode
flags on the connection, rather than overwriting them with just the
modes from the required list.

Author: rfk

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',

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)
+            })
           })
         })
-
       })
     })
   }

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)
           }
         )
     }