fix(backend): apply the same role-grant guard to admin invitations
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 assertActorMayGrant().
This commit is contained in:
@@ -1,5 +1,6 @@
|
|||||||
/**
|
/**
|
||||||
* Privilege-escalation guard for PUT /api/admin/users/:id (GHSA-rv8w-m6mx-7j4q).
|
* 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:
|
* updateAdminUser's role-change path previously enforced only:
|
||||||
* (a) non-super_admin actors can't grant the super_admin role
|
* (a) non-super_admin actors can't grant the super_admin role
|
||||||
@@ -10,9 +11,21 @@
|
|||||||
* built-in `admin` role) carrying far more permissions than the actor
|
* built-in `admin` role) carrying far more permissions than the actor
|
||||||
* itself held.
|
* 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 reuses assertActorMayGrant() — the same containment already
|
* The fix reuses assertActorMayGrant() — the same containment already
|
||||||
* applied to roles.manage (see adminRolesGuards.test.js) — inside the
|
* applied to roles.manage (see adminRolesGuards.test.js) — inside both
|
||||||
* role_id branch of updateAdminUser.
|
* 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 path = require('path');
|
||||||
const fs = require('fs');
|
const fs = require('fs');
|
||||||
@@ -29,19 +42,25 @@ const { bootCrmDb, seedMinimal, assignAdminRole } = require('../integration/help
|
|||||||
const svc = require('../../src/services/userManagementService');
|
const svc = require('../../src/services/userManagementService');
|
||||||
const { clearPermissionCache } = require('../../src/middleware/permissions');
|
const { clearPermissionCache } = require('../../src/middleware/permissions');
|
||||||
|
|
||||||
describe('updateAdminUser — role-grant privilege-escalation guard (GHSA-rv8w-m6mx-7j4q)', () => {
|
|
||||||
let db; let cleanup;
|
let db; let cleanup;
|
||||||
let superId;
|
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 limitedRoleId; let limitedId; // holds only users.edit + events.view
|
||||||
let powerfulRoleId; // carries settings.banking, which limitedId does NOT hold
|
let powerfulRoleId; // carries settings.banking, which limitedId does NOT hold
|
||||||
let modestRoleId; // carries only events.view, a subset of what limitedId holds
|
let modestRoleId; // carries only events.view, a subset of what limitedId holds
|
||||||
let targetId; // account whose role limitedId will try to change
|
let targetId; // account whose role limitedId will try to change
|
||||||
|
|
||||||
beforeAll(async () => {
|
beforeAll(async () => {
|
||||||
({ db, cleanup } = await bootCrmDb());
|
|
||||||
({ adminId: superId } = await seedMinimal(db));
|
|
||||||
await assignAdminRole(db, superId, 'super_admin');
|
|
||||||
|
|
||||||
// The attacker in GHSA-rv8w-m6mx-7j4q: users.edit only, nothing else.
|
// The attacker in GHSA-rv8w-m6mx-7j4q: users.edit only, nothing else.
|
||||||
const limitedRole = await svc.createRole(
|
const limitedRole = await svc.createRole(
|
||||||
{ name: 'limited_user_editor', permissions: ['users.edit', 'events.view'] },
|
{ name: 'limited_user_editor', permissions: ['users.edit', 'events.view'] },
|
||||||
@@ -71,8 +90,6 @@ describe('updateAdminUser — role-grant privilege-escalation guard (GHSA-rv8w-m
|
|||||||
clearPermissionCache();
|
clearPermissionCache();
|
||||||
}, 120000);
|
}, 120000);
|
||||||
|
|
||||||
afterAll(async () => { if (cleanup) await cleanup(); });
|
|
||||||
|
|
||||||
beforeEach(async () => {
|
beforeEach(async () => {
|
||||||
// Fresh target for every test, role reset to modestRole so role-change
|
// Fresh target for every test, role reset to modestRole so role-change
|
||||||
// assertions always start from a known baseline.
|
// assertions always start from a known baseline.
|
||||||
@@ -136,3 +153,83 @@ describe('updateAdminUser — role-grant privilege-escalation guard (GHSA-rv8w-m
|
|||||||
expect(updated.role_id).toBe(powerfulRoleId);
|
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.banking, 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 svc.createRole(
|
||||||
|
{ name: 'limited_inviter', permissions: ['users.create', 'events.view'] },
|
||||||
|
superId,
|
||||||
|
);
|
||||||
|
limitedRoleId = limitedRole.id;
|
||||||
|
const limitedIns = await db('admin_users').insert({
|
||||||
|
username: 'limited_inviter', email: '[email protected]', 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 svc.createRole(
|
||||||
|
{ name: 'powerful_invite_role', permissions: ['users.create', 'settings.banking'] },
|
||||||
|
superId,
|
||||||
|
);
|
||||||
|
powerfulRoleId = powerfulRole.id;
|
||||||
|
|
||||||
|
const modestRole = await svc.createRole(
|
||||||
|
{ name: 'modest_invite_role', permissions: ['events.view'] },
|
||||||
|
superId,
|
||||||
|
);
|
||||||
|
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();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -48,6 +48,16 @@ async function createInvitation({ email, roleId, invitedById, inviterRoleName })
|
|||||||
throw new ValidationError('Only Super Admins can invite new Super Admins');
|
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)
|
// Generate secure invitation token (64 characters hex = 32 bytes)
|
||||||
const token = crypto.randomBytes(32).toString('hex');
|
const token = crypto.randomBytes(32).toString('hex');
|
||||||
const expiresAt = new Date(Date.now() + 7 * 24 * 60 * 60 * 1000); // 7 days
|
const expiresAt = new Date(Date.now() + 7 * 24 * 60 * 60 * 1000); // 7 days
|
||||||
|
|||||||
Reference in New Issue
Block a user