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

Closes #968 on stable. Backport of #974.

The roles-join fallback in adminAuth fabricated role_name='super_admin' on ANY database error, so a transient fault silently granted super_admin for its duration. Gate it on isMissingRolesSchema(), moved to utils/dbErrors.js and shared with apiTokenAuth, with the predicate tightened to trust SQLSTATE 42P01/42703 on Postgres and exact driver phrasing on SQLite.

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: '[email protected]', 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();
+  });
+});

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

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

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

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