From 050eaf6481b1d70db120da1d99321548756cd030 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:22:04 +0200 Subject: [PATCH] fix(backend): require actor to hold every permission of a role they grant (stable) (#1380) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(backend): require actor to hold every permission of a role they grant Any admin with `users.edit` could grant an arbitrary non-super_admin role — including one carrying far more permissions than they themselves hold — via PUT /api/admin/users/:id. The role-change path never called the existing assertActorMayGrant() guard that already protects role create/edit. * fix(backend): require actor to hold every permission of a role they grant Backport of the same fix on main (#1378), adapted to stable's schema — stable has no custom-role-creation service or roles.manage containment helper yet, so assertActorMayGrant() is added locally in userManagementService.js instead of reused from elsewhere. Any admin with `users.edit` could grant an arbitrary non-super_admin role — including one carrying far more permissions than they themselves hold — via PUT /api/admin/users/:id. * fix(backend): apply the same role-grant guard to admin invitations (stable) Stable counterpart of the main-branch fix: createInvitation() only blocked granting super_admin — the same users.create-holder-can-invite- into-any-role escalation that updateAdminUser() was fixed for (GHSA-rv8w-m6mx-7j4q) was still open via POST /admin/users/invite. Reuses stable's local assertActorMayGrant(). --------- Co-authored-by: Paul Nothaft --- ...erManagementService.roleGrantGuard.test.js | 235 ++++++++++++++++++ backend/src/services/userManagementService.js | 59 ++++- 2 files changed, 293 insertions(+), 1 deletion(-) create mode 100644 backend/__tests__/services/userManagementService.roleGrantGuard.test.js diff --git a/backend/__tests__/services/userManagementService.roleGrantGuard.test.js b/backend/__tests__/services/userManagementService.roleGrantGuard.test.js new file mode 100644 index 00000000..688aba5a --- /dev/null +++ b/backend/__tests__/services/userManagementService.roleGrantGuard.test.js @@ -0,0 +1,235 @@ +/** + * Privilege-escalation guard for PUT /api/admin/users/:id and + * POST /api/admin/users/invite (GHSA-rv8w-m6mx-7j4q). + * + * updateAdminUser's role-change path previously enforced only: + * (a) non-super_admin actors can't grant the super_admin role + * (b) no self-role-update / demoting the last super_admin + * It never checked whether the ACTOR's own permission set covers the + * permissions carried by the role being granted — so an admin holding + * only `users.edit` could hand any other admin a role (including the + * built-in `admin` role) carrying far more permissions than the actor + * itself held. + * + * createInvitation() had the identical gap: it only ever blocked + * granting super_admin, so an admin holding only `users.create` could + * invite a brand-new admin into any other role — including one carrying + * far more permissions than the inviter itself held — via + * POST /admin/users/invite. + * + * The fix adds assertActorMayGrant() — a local containment guard, since + * stable does not yet have main's custom-role-creation service or its + * roles.manage equivalent — inside both updateAdminUser's role_id branch + * and createInvitation(). + * + * Both describe blocks below share a single bootCrmDb() call: the + * `db` module (`src/database/db.js`) is a singleton keyed off + * TEST_DATABASE_PATH at first require, and bootCrmDb's own comment + * warns that a second call after the first's cleanup() destroys the + * pool, leaving "Unable to acquire a connection" for every later query. + */ +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-rolegrantguard-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'rolegrantguard-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-rolegrantguard-storage-')); + +const { bootCrmDb, seedMinimal, assignAdminRole } = require('../integration/helpers/crmDb'); +const svc = require('../../src/services/userManagementService'); +const { clearPermissionCache } = require('../../src/middleware/permissions'); + +// Stable has no custom-role-creation service (that's main-only); build a role +// directly against the roles/permissions/role_permissions schema instead. +async function createRole(db, name, permissionNames) { + const [roleRow] = await db('roles').insert({ + name, display_name: name, is_system: false, priority: 10, created_at: new Date(), updated_at: new Date(), + }).returning('id'); + const roleId = roleRow?.id ?? roleRow; + if (permissionNames.length > 0) { + const perms = await db('permissions').whereIn('name', permissionNames).select('id', 'name'); + if (perms.length !== permissionNames.length) { + throw new Error(`Missing seeded permission(s) for: ${permissionNames.join(', ')}`); + } + await db('role_permissions').insert(perms.map((p) => ({ role_id: roleId, permission_id: p.id }))); + } + return { id: roleId }; +} + +let db; let cleanup; +let superId; + +beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ adminId: superId } = await seedMinimal(db)); + await assignAdminRole(db, superId, 'super_admin'); + clearPermissionCache(); +}, 120000); + +afterAll(async () => { if (cleanup) await cleanup(); }); + +describe('updateAdminUser — role-grant privilege-escalation guard (GHSA-rv8w-m6mx-7j4q)', () => { + let limitedRoleId; let limitedId; // holds only users.edit + events.view + let powerfulRoleId; // carries settings.edit, which limitedId does NOT hold + let modestRoleId; // carries only events.view, a subset of what limitedId holds + let targetId; // account whose role limitedId will try to change + + beforeAll(async () => { + // The attacker in GHSA-rv8w-m6mx-7j4q: users.edit only, nothing else. + const limitedRole = await createRole(db, 'limited_user_editor', ['users.edit', 'events.view']); + limitedRoleId = limitedRole.id; + const limitedIns = await db('admin_users').insert({ + username: 'limited', email: 'limited@example.com', password_hash: 'x', + role_id: limitedRoleId, must_change_password: false, created_at: new Date(), + }).returning('id'); + limitedId = limitedIns[0]?.id ?? limitedIns[0]; + + // A role carrying a permission the limited actor does not hold. + const powerfulRole = await createRole(db, 'powerful_role', ['users.edit', 'settings.edit']); + powerfulRoleId = powerfulRole.id; + + // A role whose permissions ARE a subset of what the limited actor holds. + const modestRole = await createRole(db, 'modest_role', ['events.view']); + modestRoleId = modestRole.id; + + clearPermissionCache(); + }, 120000); + + beforeEach(async () => { + // Fresh target for every test, role reset to modestRole so role-change + // assertions always start from a known baseline. + const existing = await db('admin_users').where({ username: 'target' }).first(); + if (existing) { + targetId = existing.id; + await db('admin_users').where({ id: targetId }).update({ role_id: modestRoleId }); + } else { + const ins = await db('admin_users').insert({ + username: 'target', email: 'target@example.com', password_hash: 'x', + role_id: modestRoleId, must_change_password: false, created_at: new Date(), + }).returning('id'); + targetId = ins[0]?.id ?? ins[0]; + } + }); + + it('refuses to let an admin grant a role carrying permissions the admin lacks', async () => { + await expect( + svc.updateAdminUser( + targetId, + { role_id: powerfulRoleId }, + limitedId, + { roleName: 'limited_user_editor' }, + ), + ).rejects.toThrow(/only grant permissions your own role/i); + + // Target's role must be unchanged. + const row = await db('admin_users').where({ id: targetId }).first(); + expect(row.role_id).toBe(modestRoleId); + }); + + it('refuses to let an admin grant the built-in admin role beyond its own permissions', async () => { + const adminRole = await db('roles').where({ name: 'admin' }).first(); + await expect( + svc.updateAdminUser( + targetId, + { role_id: adminRole.id }, + limitedId, + { roleName: 'limited_user_editor' }, + ), + ).rejects.toThrow(/only grant permissions your own role/i); + }); + + it('allows an admin to grant a role whose permissions it already holds', async () => { + const updated = await svc.updateAdminUser( + targetId, + { role_id: limitedRoleId }, + limitedId, + { roleName: 'limited_user_editor' }, + ); + expect(updated.role_id).toBe(limitedRoleId); + }); + + it('super_admin can still grant any role, including one carrying more permissions than a limited actor holds', async () => { + const updated = await svc.updateAdminUser( + targetId, + { role_id: powerfulRoleId }, + superId, + { roleName: 'super_admin' }, + ); + expect(updated.role_id).toBe(powerfulRoleId); + }); +}); + +describe('createInvitation — role-grant privilege-escalation guard (GHSA-rv8w-m6mx-7j4q)', () => { + let limitedRoleId; let limitedId; // holds only users.create + events.view + let powerfulRoleId; // carries settings.edit, which limitedId does NOT hold + let modestRoleId; // carries only events.view, a subset of what limitedId holds + let inviteCounter = 0; + + beforeAll(async () => { + const limitedRole = await createRole(db, 'limited_inviter', ['users.create', 'events.view']); + limitedRoleId = limitedRole.id; + const limitedIns = await db('admin_users').insert({ + username: 'limited_inviter', email: 'limited_inviter@example.com', password_hash: 'x', + role_id: limitedRoleId, must_change_password: false, created_at: new Date(), + }).returning('id'); + limitedId = limitedIns[0]?.id ?? limitedIns[0]; + + const powerfulRole = await createRole(db, 'powerful_invite_role', ['users.create', 'settings.edit']); + powerfulRoleId = powerfulRole.id; + + const modestRole = await createRole(db, 'modest_invite_role', ['events.view']); + modestRoleId = modestRole.id; + + clearPermissionCache(); + }, 120000); + + function nextEmail() { + inviteCounter += 1; + return `invitee-${inviteCounter}@example.com`; + } + + it('refuses to let an admin invite someone into a role carrying permissions the admin lacks', async () => { + await expect( + svc.createInvitation({ + email: nextEmail(), + roleId: powerfulRoleId, + invitedById: limitedId, + inviterRoleName: 'limited_inviter', + }), + ).rejects.toThrow(/only grant permissions your own role/i); + }); + + it('allows an admin to invite someone into a role whose permissions it already holds', async () => { + const invitation = await svc.createInvitation({ + email: nextEmail(), + roleId: limitedRoleId, + invitedById: limitedId, + inviterRoleName: 'limited_inviter', + }); + expect(invitation.role).toBeTruthy(); + }); + + it('allows an admin to invite someone into a role that is a subset of its own permissions', async () => { + const invitation = await svc.createInvitation({ + email: nextEmail(), + roleId: modestRoleId, + invitedById: limitedId, + inviterRoleName: 'limited_inviter', + }); + expect(invitation.role).toBeTruthy(); + }); + + it('super_admin can still invite into any role, including one carrying more permissions than a limited actor holds', async () => { + const invitation = await svc.createInvitation({ + email: nextEmail(), + roleId: powerfulRoleId, + invitedById: superId, + inviterRoleName: 'super_admin', + }); + expect(invitation.role).toBeTruthy(); + }); +}); diff --git a/backend/src/services/userManagementService.js b/backend/src/services/userManagementService.js index fc51a34f..635883d1 100644 --- a/backend/src/services/userManagementService.js +++ b/backend/src/services/userManagementService.js @@ -11,7 +11,44 @@ const { generateReadablePassword } = require('../utils/passwordGenerator'); const { getBcryptRounds } = require('../utils/passwordValidation'); const { queueEmail } = require('./emailProcessor'); const logger = require('../utils/logger'); -const { ConflictError, NotFoundError, ValidationError } = require('../utils/errors'); +const { ConflictError, ForbiddenError, NotFoundError, ValidationError } = require('../utils/errors'); + +/** + * Guard against granting a role whose permissions exceed the actor's own. + * Super admins may grant any role. Everyone else may only grant permissions + * they already hold themselves (GHSA-rv8w-m6mx-7j4q). + * @param {number} actorId - ID of the admin performing the grant + * @param {string[]} permissionNames - permission names carried by the target role + */ +async function assertActorMayGrant(actorId, permissionNames) { + if (!permissionNames || permissionNames.length === 0) { + return; + } + + const actor = await db('admin_users').where('id', actorId).first(); + if (!actor) { + throw new NotFoundError('Admin user', actorId); + } + + const actorRole = await db('roles').where('id', actor.role_id).first(); + if (actorRole && actorRole.name === 'super_admin') { + return; + } + + const actorPermissions = await db('role_permissions') + .join('permissions', 'permissions.id', 'role_permissions.permission_id') + .where('role_permissions.role_id', actor.role_id) + .pluck('permissions.name'); + + const actorPermissionSet = new Set(actorPermissions); + const missing = permissionNames.filter((name) => !actorPermissionSet.has(name)); + + if (missing.length > 0) { + throw new ForbiddenError( + `You can only grant permissions your own role already holds. Missing: ${missing.join(', ')}` + ); + } +} /** * Create a new admin user invitation @@ -47,6 +84,16 @@ async function createInvitation({ email, roleId, invitedById, inviterRoleName }) throw new ValidationError('Only Super Admins can invite new Super Admins'); } + // Privilege-escalation guard (GHSA-rv8w-m6mx-7j4q): holding `users.create` + // must not let an actor invite someone into a role carrying permissions + // they don't themselves have — same containment updateAdminUser already + // gives role assignment, reused here for invitations. + const targetRolePermissions = await db('role_permissions') + .join('permissions', 'permissions.id', 'role_permissions.permission_id') + .where('role_permissions.role_id', role.id) + .pluck('permissions.name'); + await assertActorMayGrant(invitedById, targetRolePermissions); + // Generate secure invitation token (64 characters hex = 32 bytes) const token = crypto.randomBytes(32).toString('hex'); const expiresAt = new Date(Date.now() + 7 * 24 * 60 * 60 * 1000); // 7 days @@ -263,6 +310,16 @@ async function updateAdminUser(id, updates, updatedById, requestingAdmin = {}) { throw new ValidationError('Only Super Admins can assign the Super Admin role'); } + // Privilege-escalation guard (GHSA-rv8w-m6mx-7j4q): holding `users.edit` + // must not let an actor hand out a role carrying permissions they don't + // themselves have — same containment assertActorMayGrant already gives + // `roles.manage` for role create/edit, reused here for role assignment. + const targetRolePermissions = await db('role_permissions') + .join('permissions', 'permissions.id', 'role_permissions.permission_id') + .where('role_permissions.role_id', role.id) + .pluck('permissions.name'); + await assertActorMayGrant(updatedById, targetRolePermissions); + // Prevent self-role-update if (id === updatedById) { throw new ValidationError('Cannot change your own role');