From 6699855c931657c7af7860e9bdd097da303a3a26 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Mon, 3 Aug 2026 14:48:20 +0200 Subject: [PATCH] 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. --- .../middleware/adminAuthRoleFallback.test.js | 112 ++++++++++++++++++ .../middleware/apiTokenRoleFallback.test.js | 40 +++++++ backend/src/middleware/apiTokenAuth.js | 19 +-- backend/src/middleware/auth.js | 10 ++ backend/src/utils/dbErrors.js | 35 +++++- 5 files changed, 197 insertions(+), 19 deletions(-) create mode 100644 backend/__tests__/middleware/adminAuthRoleFallback.test.js diff --git a/backend/__tests__/middleware/adminAuthRoleFallback.test.js b/backend/__tests__/middleware/adminAuthRoleFallback.test.js new file mode 100644 index 00000000..867cb1ab --- /dev/null +++ b/backend/__tests__/middleware/adminAuthRoleFallback.test.js @@ -0,0 +1,112 @@ +/** + * The roles-join fallback in adminAuth fabricates `role_name = 'super_admin'` + * to keep existing sessions working across the RBAC upgrade window. The catch + * around it used to be unconditional, so ANY transient database failure — + * connection reset, deadlock, statement timeout, pool exhaustion — took the + * same branch and handed the caller super_admin for the duration of the fault. + * + * `roleName` is the sole discriminator for every ownership check (ownership.js, + * adminProjects, adminUsers, adminApiTokens, projectService, ...), so that + * inverted the whole authorization model rather than failing the request. + * Issue #968. Same treatment apiTokenAuth already got for the v1 surface. + */ + +const jwt = require('jsonwebtoken'); + +jest.mock('../../src/utils/tokenRevocation', () => ({ isTokenRevoked: jest.fn().mockResolvedValue(false) })); +jest.mock('../../src/utils/sessionCutoff', () => ({ isTokenBeforeCutoff: jest.fn().mockResolvedValue(false) })); +jest.mock('../../src/utils/logger', () => ({ warn: jest.fn(), error: jest.fn(), debug: jest.fn(), info: jest.fn() })); + +// The joined query throws whatever the test stages; the role-less fallback +// query (no .leftJoin) always succeeds, which is what made the original bug +// reachable — it is the cheaper single-table read. +// `mock`-prefixed so jest's module-factory hoisting allows the reference. +let mockJoinError = null; +const mockAdminRow = { id: 7, username: 'scoped', email: 's@example.com', password_changed_at: null }; + +jest.mock('../../src/database/db', () => ({ + db: () => ({ + _joined: false, + leftJoin() { this._joined = true; return this; }, + where() { return this; }, + select() { return this; }, + first() { + if (this._joined && mockJoinError) return Promise.reject(mockJoinError); + return Promise.resolve({ ...mockAdminRow }); + }, + }), +})); + +const { adminAuth } = require('../../src/middleware/auth'); + +const SECRET = 'test-secret-for-admin-auth-fallback'; + +function makeReq() { + const token = jwt.sign( + { id: mockAdminRow.id, type: 'admin' }, + SECRET, + { algorithm: 'HS256', issuer: 'picpeak-auth' }, + ); + return { headers: { authorization: `Bearer ${token}` }, ip: '127.0.0.1', connection: {} }; +} + +function makeRes() { + return { + statusCode: null, + body: null, + status(code) { this.statusCode = code; return this; }, + json(payload) { this.body = payload; return this; }, + }; +} + +describe('adminAuth roles-join fallback (#968)', () => { + const OLD_SECRET = process.env.JWT_SECRET; + beforeAll(() => { process.env.JWT_SECRET = SECRET; }); + afterAll(() => { process.env.JWT_SECRET = OLD_SECRET; }); + beforeEach(() => { mockJoinError = null; }); + + it('grants the upgrade-window fallback only for a genuinely missing roles table', async () => { + mockJoinError = new Error('SQLITE_ERROR: no such table: roles'); + const req = makeReq(); + const res = makeRes(); + const next = jest.fn(); + + await adminAuth(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.admin.roleName).toBe('super_admin'); + }); + + it.each([ + ['connection reset', new Error('Connection terminated unexpectedly')], + ['deadlock', new Error('deadlock detected')], + ['pool exhaustion', new Error('Knex: Timeout acquiring a connection')], + ['statement timeout', new Error('canceling statement due to statement timeout')], + ])('does NOT fabricate super_admin on a transient failure (%s)', async (_label, err) => { + mockJoinError = err; + const req = makeReq(); + const res = makeRes(); + const next = jest.fn(); + + await adminAuth(req, res, next); + + // Fails closed: request rejected, req.admin never populated. The specific + // status is 401 (adminAuth's blanket outer catch) — what matters is that + // the caller is not elevated and does not reach the route. + expect(next).not.toHaveBeenCalled(); + expect(req.admin).toBeUndefined(); + expect(res.statusCode).toBe(401); + }); + + it('does NOT fabricate super_admin when an unrelated table is missing', async () => { + mockJoinError = new Error('SQLITE_ERROR: no such table: admin_sessions'); + const req = makeReq(); + const res = makeRes(); + const next = jest.fn(); + + await adminAuth(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(req.admin).toBeUndefined(); + }); +}); diff --git a/backend/__tests__/middleware/apiTokenRoleFallback.test.js b/backend/__tests__/middleware/apiTokenRoleFallback.test.js index 8a7fc910..fb757861 100644 --- a/backend/__tests__/middleware/apiTokenRoleFallback.test.js +++ b/backend/__tests__/middleware/apiTokenRoleFallback.test.js @@ -29,4 +29,44 @@ describe('apiTokenAuth roles-schema fallback predicate (GHSA-9697)', () => { it('rejects a missing-table error for an unrelated table', () => { expect(isMissingRolesSchema(new Error('SQLITE_ERROR: no such table: api_tokens'))).toBe(false); }); + + // knex prefixes the failing SQL to err.message, and that SQL always names + // `roles` on this join — so the message substring proves nothing about the + // error, and only an exact driver phrase (or a SQLSTATE) may be trusted. + // These are real knex message shapes, captured from the actual query. + describe('with knex\'s SQL prefix on the message (#968)', () => { + const withSql = (driverMessage) => new Error( + 'select `roles`.`name` as `role_name` from `admin_users` ' + + 'left join `roles` on `roles`.`id` = `admin_users`.`role_id` ' + + `where \`admin_users\`.\`id\` = 1 limit 1 - ${driverMessage}`, + ); + + it('accepts both legitimate upgrade-window states', () => { + // pre-054: the roles table does not exist yet + expect(isMissingRolesSchema( + Object.assign(withSql('SQLITE_ERROR: no such table: roles'), { code: 'SQLITE_ERROR' }), + )).toBe(true); + // post-054, pre-057: roles exists, admin_users.role_id not added yet + expect(isMissingRolesSchema( + Object.assign(withSql('SQLITE_ERROR: no such column: admin_users.role_id'), { code: 'SQLITE_ERROR' }), + )).toBe(true); + }); + + it('rejects an unrelated "does not exist" fault despite the SQL naming roles', () => { + // pgbouncer transaction pooling loses a named prepared statement + // (SQLSTATE 26000). Transient — the fallback query would succeed on a + // fresh connection, so accepting this would fabricate super_admin. + expect(isMissingRolesSchema( + Object.assign(withSql('prepared statement "S_1" does not exist'), { code: '26000' }), + )).toBe(false); + // The DB role/user, not the roles table. + expect(isMissingRolesSchema( + Object.assign(withSql('role "picpeak" does not exist'), { code: '28000' }), + )).toBe(false); + expect(isMissingRolesSchema( + Object.assign(withSql('database "picpeak" does not exist'), { code: '3D000' }), + )).toBe(false); + expect(isMissingRolesSchema(withSql('Connection terminated unexpectedly'))).toBe(false); + }); + }); }); diff --git a/backend/src/middleware/apiTokenAuth.js b/backend/src/middleware/apiTokenAuth.js index 7056df04..f4ffc9b0 100644 --- a/backend/src/middleware/apiTokenAuth.js +++ b/backend/src/middleware/apiTokenAuth.js @@ -1,6 +1,7 @@ const crypto = require('crypto'); const { db } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); +const { isMissingRolesSchema } = require('../utils/dbErrors'); const logger = require('../utils/logger'); const TOKEN_PREFIX = 'pp_live_'; @@ -32,24 +33,6 @@ function parseScopes(raw) { .filter((s) => VALID_SCOPES.includes(s)); } -/** - * 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 the fallback below grants super_admin: a - * catch-all would turn any transient failure — connection reset, deadlock, - * statement timeout — into a privilege escalation that hands a demoted viewer - * exactly the access GHSA-9697 closes. - */ -function isMissingRolesSchema(err) { - const message = String(err?.message || ''); - if (!/roles/i.test(message)) return false; - // PG: 42P01 undefined_table / 42703 undefined_column. SQLite carries no - // codes, so match its wording too. - return err?.code === '42P01' || err?.code === '42703' - || /no such table|no such column|does not exist|unknown column/i.test(message); -} - /** * Middleware: authenticate via API token. Maps the token to its owner * admin user, attaches { req.admin, req.apiToken }, then defers to the diff --git a/backend/src/middleware/auth.js b/backend/src/middleware/auth.js index ed6faba5..a96478b4 100644 --- a/backend/src/middleware/auth.js +++ b/backend/src/middleware/auth.js @@ -1,6 +1,7 @@ const jwt = require('jsonwebtoken'); const { db } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); +const { isMissingRolesSchema } = require('../utils/dbErrors'); const { isTokenRevoked } = require('../utils/tokenRevocation'); const { isTokenBeforeCutoff } = require('../utils/sessionCutoff'); const logger = require('../utils/logger'); @@ -83,6 +84,15 @@ async function adminAuth(req, res, next) { ) .first(); } catch (joinError) { + // Fail CLOSED on anything that isn't a genuinely missing roles schema: + // the fallback below fabricates super_admin, so a transient query failure + // (connection reset, deadlock, statement timeout, pool exhaustion) must + // not become a free privilege upgrade for every scoped admin. Rethrow → + // outer catch → 401, which is already how every other transient DB fault + // in this try block behaves (isTokenRevoked / isTokenBeforeCutoff both + // hit the DB here). apiTokenAuth takes the same posture on the v1 + // surface, differing only in its 500. + if (!isMissingRolesSchema(joinError)) throw joinError; // Fallback: roles table may not exist yet during upgrade // Query without role join - user will have no role info but can still authenticate logger.debug('Roles table not available, falling back to basic auth', { error: joinError.message }); diff --git a/backend/src/utils/dbErrors.js b/backend/src/utils/dbErrors.js index d4424598..cfb997ae 100644 --- a/backend/src/utils/dbErrors.js +++ b/backend/src/utils/dbErrors.js @@ -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 };