diff --git a/backend/__tests__/middleware/authMustChangePassword.test.js b/backend/__tests__/middleware/authMustChangePassword.test.js new file mode 100644 index 00000000..e37cfd08 --- /dev/null +++ b/backend/__tests__/middleware/authMustChangePassword.test.js @@ -0,0 +1,123 @@ +/** + * GHSA-h4w8-57xq-53fx enforcement half: `must_change_password` was written + * by the admin password-reset flow (userManagementService.resetAdminPassword) + * and returned in a few response payloads, but no route-blocking logic ever + * checked it — a reset admin could keep using the old/weak password on every + * protected route indefinitely. adminAuth() is now the server-side backstop: + * a flagged admin gets 403 MUST_CHANGE_PASSWORD on everything except the + * routes they need to clear the flag (change-password) or leave (logout). + * + * Mirrors the mocking shape of adminAuthRoleFallback.test.js — a stub `db` + * chain, no real SQLite needed, so this stays a fast unit test. + */ + +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() })); + +let mockMustChangePassword = false; +const mockAdminRow = { id: 7, username: 'scoped', email: 's@example.com', password_changed_at: null, role_id: 1, role_name: 'editor' }; + +jest.mock('../../src/database/db', () => ({ + db: () => ({ + leftJoin() { return this; }, + where() { return this; }, + select() { return this; }, + first: () => Promise.resolve({ ...mockAdminRow, must_change_password: mockMustChangePassword }), + }), +})); + +const { adminAuth } = require('../../src/middleware/auth'); + +const SECRET = 'test-secret-for-must-change-password'; + +function makeReq(originalUrl) { + 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: {}, originalUrl }; +} + +function makeRes() { + return { + statusCode: null, + body: null, + status(code) { this.statusCode = code; return this; }, + json(payload) { this.body = payload; return this; }, + }; +} + +describe('adminAuth must_change_password enforcement (GHSA-h4w8-57xq-53fx)', () => { + const OLD_SECRET = process.env.JWT_SECRET; + beforeAll(() => { process.env.JWT_SECRET = SECRET; }); + afterAll(() => { process.env.JWT_SECRET = OLD_SECRET; }); + beforeEach(() => { mockMustChangePassword = false; }); + + it('blocks an arbitrary protected route with 403 MUST_CHANGE_PASSWORD when the flag is set', async () => { + mockMustChangePassword = true; + const req = makeReq('/api/admin/dashboard/stats'); + const res = makeRes(); + const next = jest.fn(); + + await adminAuth(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.statusCode).toBe(403); + expect(res.body).toEqual(expect.objectContaining({ code: 'MUST_CHANGE_PASSWORD' })); + expect(req.admin).toBeUndefined(); + }); + + it('does not block when the flag is not set', async () => { + mockMustChangePassword = false; + const req = makeReq('/api/admin/dashboard/stats'); + const res = makeRes(); + const next = jest.fn(); + + await adminAuth(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.admin.mustChangePassword).toBe(false); + }); + + it.each([ + ['/api/admin/auth/change-password'], + ['/api/admin/auth/logout'], + ])('still allows %s through when the flag is set', async (originalUrl) => { + mockMustChangePassword = true; + const req = makeReq(originalUrl); + const res = makeRes(); + const next = jest.fn(); + + await adminAuth(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.admin.mustChangePassword).toBe(true); + expect(res.statusCode).toBeNull(); + }); + + it('allows the exempt change-password path even with a query string', async () => { + mockMustChangePassword = true; + const req = makeReq('/api/admin/auth/change-password?foo=bar'); + const res = makeRes(); + const next = jest.fn(); + + await adminAuth(req, res, next); + + expect(next).toHaveBeenCalled(); + }); + + it('does not exempt a route that merely starts with the change-password path', async () => { + mockMustChangePassword = true; + const req = makeReq('/api/admin/auth/change-password-history'); + const res = makeRes(); + const next = jest.fn(); + + await adminAuth(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.statusCode).toBe(403); + }); +}); diff --git a/backend/__tests__/services/userManagementService.resetPassword.test.js b/backend/__tests__/services/userManagementService.resetPassword.test.js new file mode 100644 index 00000000..a39bac41 --- /dev/null +++ b/backend/__tests__/services/userManagementService.resetPassword.test.js @@ -0,0 +1,100 @@ +/** + * GHSA-h4w8-57xq-53fx entropy half: resetAdminPassword used to mint the + * emailed temp password with generateReadablePassword() — 10 adjectives x + * 10 nouns x crypto.randomInt(1000,9999) x 5 specials, ~2^21 possibilities, + * brute-forceable. It now uses generateSecurePassword(16) (90-char charset), + * same as every other security-sensitive password path in this file. + * + * Verified against a real SQLite DB (full core-migration set) so the + * emailed plaintext, the stored hash, and must_change_password are all + * checked end to end rather than against a mock. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +// bootCrmDb() sets TEST_DATABASE_PATH itself, but only in time for requires +// that happen AFTER it runs (inside beforeAll). userManagementService.js +// requires database/db.js at module load — i.e. before beforeAll — so that +// connection has to be pointed at a fresh, unused test DB up front, or it +// falls back to the shared default path and collides with whatever another +// test file already migrated onto it. Same workaround as +// userManagementService.activateDelete.test.js. +const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-reset-pw-test-')); +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'db.sqlite'); +process.env.STORAGE_PATH = path.join(tmpDir, 'storage'); +fs.mkdirSync(process.env.STORAGE_PATH, { recursive: true }); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'reset-pw-test-secret'; + +const bcrypt = require('bcrypt'); + +const { bootCrmDb, seedMinimal, assignAdminRole } = require('../integration/helpers/crmDb'); +const userManagementService = require('../../src/services/userManagementService'); + +// The wordlist generateReadablePassword() used to produce: +// <4 digits><1 special>, e.g. "SwiftEagle4821!" +const READABLE_WORDLIST_PATTERN = /^(Swift|Bright|Strong|Happy|Clever|Brave|Noble|Quick|Sharp|Bold)(Eagle|Mountain|River|Thunder|Forest|Ocean|Falcon|Dragon|Phoenix|Tiger)\d{4}[!@#$%]$/; + +describe('userManagementService.resetAdminPassword (GHSA-h4w8-57xq-53fx)', () => { + let db; + let cleanup; + let actorId; + let targetId; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ adminId: actorId } = await seedMinimal(db)); + await assignAdminRole(db, actorId, 'super_admin'); + + const editor = await db('roles').where({ name: 'editor' }).first(); + const targetInsert = await db('admin_users').insert({ + username: 'reset-target', email: 'reset-target@example.com', + password_hash: await bcrypt.hash('old-password', 4), + role_id: editor?.id || null, + is_active: 1, must_change_password: false, created_at: new Date().toISOString(), + }).returning('id'); + targetId = targetInsert[0]?.id ?? targetInsert[0]; + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('generates a high-entropy password, not one drawn from the adjective/noun wordlist', async () => { + const before = await db('admin_users').where({ id: targetId }).first(); + + await userManagementService.resetAdminPassword(targetId, actorId); + + const emailRow = await db('email_queue') + .where({ recipient_email: 'reset-target@example.com', email_type: 'admin_password_reset' }) + .orderBy('id', 'desc') + .first(); + expect(emailRow).toBeDefined(); + const emailData = JSON.parse(emailRow.email_data); + const newPassword = emailData.new_password; + + // generateSecurePassword(16): fixed 16-char length, not the wordlist's + // variable-length "WordWord####!" shape. + expect(newPassword).toHaveLength(16); + expect(newPassword).not.toMatch(READABLE_WORDLIST_PATTERN); + // generateSecurePassword guarantees at least one of each character class. + expect(newPassword).toMatch(/[a-z]/); + expect(newPassword).toMatch(/[A-Z]/); + expect(newPassword).toMatch(/[0-9]/); + expect(newPassword).toMatch(/[!@#$%^&*()_+\-=[\]{}|;:,.<>?]/); + + // The emailed plaintext actually matches what got persisted. + const after = await db('admin_users').where({ id: targetId }).first(); + expect(after.password_hash).not.toBe(before.password_hash); + await expect(bcrypt.compare(newPassword, after.password_hash)).resolves.toBe(true); + }); + + it('sets must_change_password so the enforcement backstop kicks in on next login', async () => { + await db('admin_users').where({ id: targetId }).update({ must_change_password: false }); + + await userManagementService.resetAdminPassword(targetId, actorId); + + const after = await db('admin_users').where({ id: targetId }).first(); + expect(after.must_change_password === true || after.must_change_password === 1).toBe(true); + }); +}); diff --git a/backend/src/middleware/auth.js b/backend/src/middleware/auth.js index b1b4f1a5..ec0b9879 100644 --- a/backend/src/middleware/auth.js +++ b/backend/src/middleware/auth.js @@ -6,6 +6,18 @@ const { isTokenRevoked } = require('../utils/tokenRevocation'); const logger = require('../utils/logger'); const { getAdminTokenFromRequest } = require('../utils/tokenUtils'); +// GHSA-h4w8-57xq-53fx: must_change_password was written on reset (and on +// invitation paths) but nothing server-side ever checked it — a forced-reset +// admin could keep using the old/weak password indefinitely because the flag +// only ever reached the frontend as a response field. This is the backstop +// for callers that skip the UI entirely. Every route gated by adminAuth() is +// blocked except the ones a flagged admin needs to clear the flag or leave: +// change their password, and log out. +const MUST_CHANGE_PASSWORD_EXEMPT_PATHS = new Set([ + '/api/admin/auth/change-password', + '/api/admin/auth/logout', +]); + /** * Enhanced admin authentication middleware with revocation checking */ @@ -71,6 +83,7 @@ async function adminAuth(req, res, next) { 'admin_users.username', 'admin_users.email', 'admin_users.password_changed_at', + 'admin_users.must_change_password', 'roles.id as role_id', 'roles.name as role_name' ) @@ -89,7 +102,7 @@ async function adminAuth(req, res, next) { logger.debug('Roles table not available, falling back to basic auth', { error: joinError.message }); admin = await db('admin_users') .where({ id: decoded.id, is_active: formatBoolean(true) }) - .select('id', 'username', 'email', 'password_changed_at') + .select('id', 'username', 'email', 'password_changed_at', 'must_change_password') .first(); if (admin) { admin.role_id = null; @@ -119,13 +132,22 @@ async function adminAuth(req, res, next) { } } + if (admin.must_change_password + && !MUST_CHANGE_PASSWORD_EXEMPT_PATHS.has(req.originalUrl.split('?')[0])) { + return res.status(403).json({ + error: 'Password change required before continuing', + code: 'MUST_CHANGE_PASSWORD' + }); + } + // Add user info to request (enhanced with role) req.admin = { id: admin.id, username: admin.username, email: admin.email, roleId: admin.role_id, - roleName: admin.role_name + roleName: admin.role_name, + mustChangePassword: !!admin.must_change_password }; req.token = token; // Store token for potential revocation diff --git a/backend/src/services/userManagementService.js b/backend/src/services/userManagementService.js index fc51a34f..141d1d77 100644 --- a/backend/src/services/userManagementService.js +++ b/backend/src/services/userManagementService.js @@ -7,7 +7,7 @@ const bcrypt = require('bcrypt'); const crypto = require('crypto'); const { db, logActivity } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); -const { generateReadablePassword } = require('../utils/passwordGenerator'); +const { generateSecurePassword } = require('../utils/passwordGenerator'); const { getBcryptRounds } = require('../utils/passwordValidation'); const { queueEmail } = require('./emailProcessor'); const logger = require('../utils/logger'); @@ -459,7 +459,10 @@ async function resetAdminPassword(id, resetById) { throw new NotFoundError('Admin user', id); } - const newPassword = generateReadablePassword(); + // GHSA-h4w8-57xq-53fx: this password is emailed to the admin and is a live + // credential until they change it, so it needs real entropy — not the + // ~2^21 wordlist-based generateReadablePassword() used for gallery resets. + const newPassword = generateSecurePassword(16); const passwordHash = await bcrypt.hash(newPassword, getBcryptRounds()); await db('admin_users').where('id', id).update({