fix(auth): fail closed when the adminAuth roles join errors (#974)

Closes #968.

The roles-join fallback in adminAuth fabricated role_name='super_admin' on ANY database error, so a transient fault (connection reset, deadlock, statement timeout, pool exhaustion) silently granted super_admin for its duration. roleName is the sole discriminator for every ownership check, so this inverted the authorization model rather than failing the request.

Gate the fallback on isMissingRolesSchema(), moved to utils/dbErrors.js and shared with apiTokenAuth. The predicate was also tightened: knex prefixes the failing SQL to err.message and that SQL always names `roles`, so the old /roles/i gate was vacuous and a generic /does not exist/ could accept unrelated faults. Now trusts SQLSTATE 42P01/42703 on Postgres and exact driver phrasing on SQLite.
This commit is contained in:
Paul Nothaft
2026-08-03 14:48:20 +02:00
committed by GitHub
parent 569ae39acb
commit 6699855c93
5 changed files with 197 additions and 19 deletions
+34 -1
View File
@@ -12,4 +12,37 @@ function isUniqueViolation(err) {
return /unique/i.test(msg) || /sqlite_constraint/i.test(msg);
}
module.exports = { isUniqueViolation };
/**
* Does this error mean the `roles` table/column genuinely isn't there yet
* (mid-upgrade), as opposed to the database being briefly unhappy?
*
* The distinction matters because both auth paths fall back to granting
* super_admin when the roles join fails: a catch-all would turn any transient
* failure — connection reset, deadlock, statement timeout, pool exhaustion —
* into a privilege escalation that hands a demoted viewer exactly the access
* GHSA-9697 closes. Callers must rethrow anything this returns false for.
*/
function isMissingRolesSchema(err) {
if (!err) return false;
const message = String(err.message || '');
// Postgres is authoritative via SQLSTATE: 42P01 undefined_table, 42703
// undefined_column. Both are schema conditions, never transient.
if (err.code === '42P01' || err.code === '42703') return true;
// SQLite carries no SQLSTATE, so the driver's wording is all there is — but
// it must be matched EXACTLY, naming the object the roles join needs. A
// generic /does not exist/ test would be unsound here: knex prefixes the
// failing SQL to err.message, and that SQL always names `roles` on this
// join, so any "... does not exist" fault on the connection (e.g. pgbouncer
// losing a named prepared statement, SQLSTATE 26000) would read as a missing
// roles schema and fabricate super_admin.
//
// Two states are legitimate, per the migration order:
// pre-054 → roles table absent
// post-054, pre-057 → roles exists, admin_users.role_id not added yet
return /no such table: roles\b/i.test(message)
|| /no such column: (roles\.|admin_users\.role_id\b)/i.test(message);
}
module.exports = { isUniqueViolation, isMissingRolesSchema };