diff --git a/backend/__tests__/middleware/adminAuthRoleFallback.test.js b/backend/__tests__/middleware/adminAuthRoleFallback.test.js new file mode 100644 index 00000000..1ea5100d --- /dev/null +++ b/backend/__tests__/middleware/adminAuthRoleFallback.test.js @@ -0,0 +1,111 @@ +/** + * 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/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 da5ec767..623c5ace 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 ccb49903..df1321b0 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 logger = require('../utils/logger'); const { getAdminTokenFromRequest, getGalleryTokenFromRequest } = require('../utils/tokenUtils'); @@ -75,6 +76,14 @@ 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 a transient DB fault in this + // try block behaves (isTokenRevoked hits 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 };