From b3766add457f92bda6f5d5dbdfa4f474bbd48f8b Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Thu, 15 Oct 2026 19:25:58 -0700 Subject: [PATCH] mysql: quote column names and bind every WHERE value insert(), update() and whereConditions() put object keys into the SQL as column names, and those keys can come from a request payload. The Joi schemas reject unknown keys today, so nothing reaches them, but the builders relied on that. They now quote each column with escapeId. update() takes its WHERE as an object of bound values; the stores passed `nt_x_id=${id}`. A string WHERE still works for SQL whose placeholders the caller binds. The group and user subgroup IN lists bind their ids. Clears CodeQL js/sql-injection (alerts 15 and 28). --- lib/group/store/mysql.js | 15 +++++--- lib/mysql.js | 42 +++++++++++--------- lib/mysql.test.js | 70 +++++++++++++++++++++++++--------- lib/nameserver/store/mysql.js | 12 ++++-- lib/permission/store/mysql.js | 12 ++++-- lib/session/store/mysql.js | 2 +- lib/user/store/mysql.js | 17 ++++++--- lib/zone/store/mysql.js | 12 ++++-- lib/zone_record/store/mysql.js | 10 +++-- 9 files changed, 127 insertions(+), 65 deletions(-) diff --git a/lib/group/store/mysql.js b/lib/group/store/mysql.js index cb9422c..a8b99fd 100644 --- a/lib/group/store/mysql.js +++ b/lib/group/store/mysql.js @@ -83,7 +83,8 @@ class Group extends GroupBase { [args.id], ) const gids = [args.id, ...subgroupRows.map((r) => r.nt_subgroup_id)] - where.push(`g.nt_group_id IN (${gids.join(',')})`) + where.push(`g.nt_group_id IN (${gids.map(() => '?').join(',')})`) + params.push(...gids) } else { where.push('g.nt_group_id = ?') params.push(args.id) @@ -159,16 +160,20 @@ class Group extends GroupBase { if (Object.keys(args).length === 0) return true const r = await Mysql.execute( - ...Mysql.update(`nt_group`, `nt_group_id=${id}`, mapToDbColumn(args, groupDbMap)), + ...Mysql.update(`nt_group`, { nt_group_id: id }, mapToDbColumn(args, groupDbMap)), ) return r.changedRows === 1 } async delete(args) { const r = await Mysql.execute( - ...Mysql.update(`nt_group`, `nt_group_id=${args.id}`, { - deleted: args.deleted ?? 1, - }), + ...Mysql.update( + `nt_group`, + { nt_group_id: args.id }, + { + deleted: args.deleted ?? 1, + }, + ), ) return r.changedRows === 1 } diff --git a/lib/mysql.js b/lib/mysql.js index fd8cad2..8daa8c8 100644 --- a/lib/mysql.js +++ b/lib/mysql.js @@ -77,9 +77,12 @@ class Mysql { return shapeRows(query, rows) } + // Column names can come from payload keys, so they are quoted as + // identifiers; values are always bound. insert(table, params = {}) { + const columns = Object.keys(params).map(escapeId) return [ - `INSERT INTO ${table} (${Object.keys(params).join(',')}) VALUES(${Object.keys(params).map(() => '?')})`, + `INSERT INTO ${table} (${columns.join(',')}) VALUES(${columns.map(() => '?')})`, Object.values(params), ] } @@ -88,8 +91,15 @@ class Mysql { return this.whereConditions(query, params) } + // where is an object of column values, or SQL whose placeholders the + // caller binds after the SET values update(table, where, params = {}) { - return [`UPDATE ${table} SET ${Object.keys(params).join('=?,')}=? WHERE ${where}`, Object.values(params)] + const set = Object.keys(params).map((col) => `${escapeId(col)}=?`) + const [conditions, whereValues] = typeof where === 'string' ? [where, []] : equalities(where) + return [ + `UPDATE ${table} SET ${set.join(',')} WHERE ${conditions}`, + [...Object.values(params), ...whereValues], + ] } delete(table, params) { @@ -97,23 +107,10 @@ class Mysql { } whereConditions(query, params) { - let newQuery = query - let paramsArray = [] - - if (Array.isArray(params)) { - paramsArray = [...params] - } else if (typeof params === 'object' && !Array.isArray(params)) { - // Object to WHERE conditions - let first = true - for (const p in params) { - if (first) newQuery += ' WHERE' - if (!first) newQuery += ' AND' - newQuery += ` ${p}=?` - paramsArray.push(params[p]) - first = false - } - } - return [newQuery, paramsArray] + if (Array.isArray(params)) return [query, [...params]] + if (typeof params !== 'object' || Object.keys(params).length === 0) return [query, []] + const [conditions, values] = equalities(params) + return [`${query} WHERE ${conditions}`, values] } async disconnect(dbh) { @@ -133,4 +130,11 @@ class Mysql { } } +const { escapeId } = mysql + +function equalities(params) { + const columns = Object.keys(params).map((col) => `${escapeId(col)}=?`) + return [columns.join(' AND '), Object.values(params)] +} + export default new Mysql() diff --git a/lib/mysql.test.js b/lib/mysql.test.js index 95e1047..245e27b 100644 --- a/lib/mysql.test.js +++ b/lib/mysql.test.js @@ -25,7 +25,7 @@ describe('mysql', () => { Mysql.select(`SELECT * FROM nt_user`, { last_name: 'Test', }), - [`SELECT * FROM nt_user WHERE last_name=?`, ['Test']], + ['SELECT * FROM nt_user WHERE `last_name`=?', ['Test']], ) }) @@ -34,36 +34,68 @@ describe('mysql', () => { first_name: 'uNite', last_name: 'Test', }) - assert.deepEqual(r, [`INSERT INTO nt_user (first_name,last_name) VALUES(?,?)`, ['uNite', 'Test']]) + assert.deepEqual(r, ['INSERT INTO nt_user (`first_name`,`last_name`) VALUES(?,?)', ['uNite', 'Test']]) }) describe('update', () => { it('formats with one value', () => { - const r = Mysql.update(`nt_user`, `nt_user_id=4096`, { - first_name: 'uNite', - }) - assert.deepEqual(r, [`UPDATE nt_user SET first_name=? WHERE nt_user_id=4096`, ['uNite']]) + const r = Mysql.update( + `nt_user`, + { nt_user_id: 4096 }, + { + first_name: 'uNite', + }, + ) + assert.deepEqual(r, ['UPDATE nt_user SET `first_name`=? WHERE `nt_user_id`=?', ['uNite', 4096]]) }) it('formats with two values', () => { - const r = Mysql.update(`nt_user`, `nt_user_id=4096`, { - last_name: 'Teste', - is_admin: 1, - }) - assert.deepEqual(r, [`UPDATE nt_user SET last_name=?,is_admin=? WHERE nt_user_id=4096`, ['Teste', 1]]) + const r = Mysql.update( + `nt_user`, + { nt_user_id: 4096 }, + { + last_name: 'Teste', + is_admin: 1, + }, + ) + assert.deepEqual(r, [ + 'UPDATE nt_user SET `last_name`=?,`is_admin`=? WHERE `nt_user_id`=?', + ['Teste', 1, 4096], + ]) }) it('formats with three values', () => { - const r = Mysql.update(`nt_user`, `nt_user_id=4096`, { - first_name: 'Unit', - last_name: 'Test', - is_admin: 0, - }) + const r = Mysql.update( + `nt_user`, + { nt_user_id: 4096 }, + { + first_name: 'Unit', + last_name: 'Test', + is_admin: 0, + }, + ) assert.deepEqual(r, [ - `UPDATE nt_user SET first_name=?,last_name=?,is_admin=? WHERE nt_user_id=4096`, - ['Unit', 'Test', 0], + 'UPDATE nt_user SET `first_name`=?,`last_name`=?,`is_admin`=? WHERE `nt_user_id`=?', + ['Unit', 'Test', 0, 4096], ]) }) + + it('binds the SET values before a where clause the caller wrote', () => { + const r = Mysql.update(`nt_user`, 'nt_user_id=? AND deleted=?', { first_name: 'Unit' }) + assert.deepEqual(r, ['UPDATE nt_user SET `first_name`=? WHERE nt_user_id=? AND deleted=?', ['Unit']]) + }) + + it('quotes a column name that tries to be SQL', () => { + const [sql] = Mysql.update(`nt_user`, { nt_user_id: 4096 }, { 'is_admin=1,first_name': 'x' }) + assert.equal(sql, 'UPDATE nt_user SET `is_admin=1,first_name`=? WHERE `nt_user_id`=?') + }) + }) + + describe('insert', () => { + it('quotes a column name that tries to be SQL', () => { + const [sql] = Mysql.insert(`nt_user`, { 'first_name) VALUES (1); --': 'x' }) + assert.equal(sql, 'INSERT INTO nt_user (`first_name) VALUES (1); --`) VALUES(?)') + }) }) describe('delete', () => { @@ -73,7 +105,7 @@ describe('mysql', () => { it('with params', () => { assert.deepEqual(Mysql.delete(`nt_user`, { last_name: 'Test' }), [ - `DELETE FROM nt_user WHERE last_name=?`, + 'DELETE FROM nt_user WHERE `last_name`=?', ['Test'], ]) }) diff --git a/lib/nameserver/store/mysql.js b/lib/nameserver/store/mysql.js index 1246c97..3c04278 100644 --- a/lib/nameserver/store/mysql.js +++ b/lib/nameserver/store/mysql.js @@ -77,16 +77,20 @@ class Nameserver extends NameserverBase { delete args.id // Mysql.debug(1) const r = await Mysql.execute( - ...Mysql.update(`nt_nameserver`, `nt_nameserver_id=${id}`, mapToDbColumn(objectToDb(args), nsDbMap)), + ...Mysql.update(`nt_nameserver`, { nt_nameserver_id: id }, mapToDbColumn(objectToDb(args), nsDbMap)), ) return r.changedRows === 1 } async delete(args) { const r = await Mysql.execute( - ...Mysql.update(`nt_nameserver`, `nt_nameserver_id=${args.id}`, { - deleted: args.deleted ?? 1, - }), + ...Mysql.update( + `nt_nameserver`, + { nt_nameserver_id: args.id }, + { + deleted: args.deleted ?? 1, + }, + ), ) return r.changedRows === 1 } diff --git a/lib/permission/store/mysql.js b/lib/permission/store/mysql.js index a0281f2..e426cb0 100644 --- a/lib/permission/store/mysql.js +++ b/lib/permission/store/mysql.js @@ -121,7 +121,7 @@ class PermissionRepoMySQL extends PermissionBase { delete row.id if (Object.keys(row).length === 0) return false const r = await Mysql.execute( - ...Mysql.update(`nt_perm`, `nt_perm_id=${id}`, mapToDbColumn(row, permDbMap)), + ...Mysql.update(`nt_perm`, { nt_perm_id: id }, mapToDbColumn(row, permDbMap)), ) return r.changedRows === 1 } @@ -129,9 +129,13 @@ class PermissionRepoMySQL extends PermissionBase { async delete(args) { if (!args.id) return false const r = await Mysql.execute( - ...Mysql.update(`nt_perm`, `nt_perm_id=${args.id}`, { - deleted: args.deleted ?? 1, - }), + ...Mysql.update( + `nt_perm`, + { nt_perm_id: args.id }, + { + deleted: args.deleted ?? 1, + }, + ), ) return r.changedRows === 1 } diff --git a/lib/session/store/mysql.js b/lib/session/store/mysql.js index bc45390..94a0ea5 100644 --- a/lib/session/store/mysql.js +++ b/lib/session/store/mysql.js @@ -63,7 +63,7 @@ class SessionRepoMySQL { const id = args.id delete args.id const r = await Mysql.execute( - ...Mysql.update(`nt_user_session`, `nt_user_session_id=${id}`, mapToDbColumn(args, sessionDbMap)), + ...Mysql.update(`nt_user_session`, { nt_user_session_id: id }, mapToDbColumn(args, sessionDbMap)), ) return r.changedRows === 1 } diff --git a/lib/user/store/mysql.js b/lib/user/store/mysql.js index 2a0d56f..7e53c08 100644 --- a/lib/user/store/mysql.js +++ b/lib/user/store/mysql.js @@ -50,7 +50,7 @@ class UserRepoMySQL extends UserBase { if (needsUpgrade) { try { const creds = await Credentials.forStorage(authTry.password) - await Mysql.execute(...Mysql.update('nt_user', `nt_user_id=${u.id}`, creds)) + await Mysql.execute(...Mysql.update('nt_user', { nt_user_id: u.id }, creds)) } catch (err) { console.warn(`password hash upgrade failed for user ${authTry.username}`, err) } @@ -130,7 +130,8 @@ class UserRepoMySQL extends UserBase { [args.gid], ) const gids = [args.gid, ...subgroupRows.map((r) => r.nt_subgroup_id)] - where.push(`nt_group_id IN (${gids.join(',')})`) + where.push(`nt_group_id IN (${gids.map(() => '?').join(',')})`) + params.push(...gids) } else { where.push('nt_group_id = ?') params.push(args.gid) @@ -238,16 +239,20 @@ class UserRepoMySQL extends UserBase { if (Object.keys(args).length === 0) return true const r = await Mysql.execute( - ...Mysql.update(`nt_user`, `nt_user_id=${id}`, mapToDbColumn(args, userDbMap)), + ...Mysql.update(`nt_user`, { nt_user_id: id }, mapToDbColumn(args, userDbMap)), ) return r.changedRows === 1 } async delete(args) { const r = await Mysql.execute( - ...Mysql.update(`nt_user`, `nt_user_id=${args.id}`, { - deleted: args.deleted ?? 1, - }), + ...Mysql.update( + `nt_user`, + { nt_user_id: args.id }, + { + deleted: args.deleted ?? 1, + }, + ), ) return r.changedRows === 1 } diff --git a/lib/zone/store/mysql.js b/lib/zone/store/mysql.js index 39761aa..3bd4dfa 100644 --- a/lib/zone/store/mysql.js +++ b/lib/zone/store/mysql.js @@ -255,7 +255,7 @@ class ZoneRepoMySQL extends ZoneBase { if (!args.id) return false const { id, nameservers, ...changes } = args const update = Object.keys(changes).length - ? Mysql.update(`nt_zone`, `nt_zone_id=${id}`, mapToDbColumn(changes, zoneDbMap)) + ? Mysql.update(`nt_zone`, { nt_zone_id: id }, mapToDbColumn(changes, zoneDbMap)) : undefined const deleted = changes.deleted === undefined ? undefined : isTrue(changes.deleted) @@ -298,9 +298,13 @@ class ZoneRepoMySQL extends ZoneBase { async delete(args) { if (args.deleted === false || args.deleted === 0) return this.put({ id: args.id, deleted: false }) const r = await Mysql.execute( - ...Mysql.update(`nt_zone`, `nt_zone_id=${args.id}`, { - deleted: args.deleted ?? 1, - }), + ...Mysql.update( + `nt_zone`, + { nt_zone_id: args.id }, + { + deleted: args.deleted ?? 1, + }, + ), ) return r.changedRows === 1 } diff --git a/lib/zone_record/store/mysql.js b/lib/zone_record/store/mysql.js index 86027d4..16f25e1 100644 --- a/lib/zone_record/store/mysql.js +++ b/lib/zone_record/store/mysql.js @@ -203,9 +203,13 @@ class ZoneRecordMySQL extends ZoneRecordBase { async delete(args) { const r = await Mysql.execute( - ...Mysql.update(`nt_zone_record`, `nt_zone_record_id=${args.id}`, { - deleted: args.deleted ?? 1, - }), + ...Mysql.update( + `nt_zone_record`, + { nt_zone_record_id: args.id }, + { + deleted: args.deleted ?? 1, + }, + ), ) return r.changedRows === 1 }