Skip to content

Commit 551d423

Browse files
squinard1478claudemichaelbromley
authored
Merge commit from fork
* fix(core): Guard AdministratorService.update against privilege escalation The `password` and `emailAddress` branches of AdministratorService.update had no ownership or SuperAdmin check (only the `roleIds` branch did), so any holder of the delegated `UpdateAdministrator` permission could reset another administrator's password — including the SuperAdmin's — and take over the instance. update() now requires the active user to hold all of the target administrator's permissions on all of the target's channels (the same permission-sufficiency check already used for role granting) before any field is modified. A SuperAdmin target therefore requires a SuperAdmin caller. Validated by administrator-update-privesc-guard.e2e-spec.ts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(core): Strengthen administrator update privilege-escalation guard tests Rework the privilege-escalation guard e2e spec to use the suite's typed GraphQL documents and the assertThrowsWithMessage helper instead of hand-rolled untyped queries and try/catch blocks. Add positive-path coverage that was missing: a non-SuperAdmin holding sufficient permissions can still update a lower-privileged administrator, and a SuperAdmin can still update another administrator. The previous "regression" test only exercised a SuperAdmin editing itself, which the guard never affected. Also trim the duplicated and history-narrating comments around checkActiveUserCanManageAdministrator down to a single evergreen note. Relates to GHSA-v85r-wfgv-jcqc --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Michael Bromley <michael@michaelbromley.co.uk>
1 parent c13811a commit 551d423

2 files changed

Lines changed: 139 additions & 0 deletions

File tree

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import { Permission } from '@vendure/common/lib/generated-types';
2+
import { SUPER_ADMIN_USER_IDENTIFIER } from '@vendure/common/lib/shared-constants';
3+
import { createTestEnvironment } from '@vendure/testing';
4+
import path from 'path';
5+
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
6+
7+
import { initialData } from '../../../e2e-common/e2e-initial-data';
8+
import { TEST_SETUP_TIMEOUT_MS, testConfig } from '../../../e2e-common/test-config';
9+
10+
import {
11+
createAdministratorDocument,
12+
createRoleDocument,
13+
getAdministratorsDocument,
14+
MeDocument,
15+
updateAdministratorDocument,
16+
} from './graphql/shared-definitions';
17+
import { assertThrowsWithMessage } from './utils/assert-throws-with-message';
18+
19+
/**
20+
* `AdministratorService.update` must only allow the active user to modify an administrator over whom
21+
* they already hold sufficient permissions. This uses a dedicated environment rather than folding into
22+
* administrator.e2e-spec.ts, because that suite asserts fixed administrator counts across its sequential
23+
* tests and reassigns the SuperAdmin's identifier via updateActiveAdministrator, which would make
24+
* re-authenticating as the SuperAdmin here unreliable.
25+
*/
26+
describe('AdministratorService.update privilege-escalation guard', () => {
27+
const { server, adminClient } = createTestEnvironment(testConfig());
28+
29+
const manager = { emailAddress: 'admin-manager@test.com', password: 'test-password' };
30+
let superAdminId: string;
31+
let staffId: string;
32+
33+
beforeAll(async () => {
34+
await server.init({
35+
initialData,
36+
productsCsvPath: path.join(__dirname, 'fixtures/e2e-products-minimal.csv'),
37+
customerCount: 1,
38+
});
39+
await adminClient.asSuperAdmin();
40+
41+
const { administrators } = await adminClient.query(getAdministratorsDocument);
42+
superAdminId = administrators.items.find(a => a.user.identifier === SUPER_ADMIN_USER_IDENTIFIER)!.id;
43+
44+
// A "staff" administrator holding a narrow set of permissions.
45+
const { createRole: staffRole } = await adminClient.query(createRoleDocument, {
46+
input: {
47+
code: 'staff',
48+
description: 'Catalog staff',
49+
permissions: [Permission.ReadCatalog],
50+
channelIds: ['T_1'],
51+
},
52+
});
53+
const { createAdministrator: staff } = await adminClient.query(createAdministratorDocument, {
54+
input: {
55+
emailAddress: 'staff@test.com',
56+
firstName: 'Stan',
57+
lastName: 'Staff',
58+
password: 'test-password',
59+
roleIds: [staffRole.id],
60+
},
61+
});
62+
staffId = staff.id;
63+
64+
// An "admin manager": not a SuperAdmin, but holds a superset of the staff permissions plus the
65+
// ability to manage administrators.
66+
const { createRole: managerRole } = await adminClient.query(createRoleDocument, {
67+
input: {
68+
code: 'admin-manager',
69+
description: 'Can manage administrators',
70+
permissions: [
71+
Permission.ReadCatalog,
72+
Permission.ReadAdministrator,
73+
Permission.UpdateAdministrator,
74+
],
75+
channelIds: ['T_1'],
76+
},
77+
});
78+
await adminClient.query(createAdministratorDocument, {
79+
input: {
80+
emailAddress: manager.emailAddress,
81+
firstName: 'Manny',
82+
lastName: 'Manager',
83+
password: manager.password,
84+
roleIds: [managerRole.id],
85+
},
86+
});
87+
}, TEST_SETUP_TIMEOUT_MS);
88+
89+
afterAll(async () => {
90+
await server.destroy();
91+
});
92+
93+
it(
94+
'blocks a non-SuperAdmin from resetting the SuperAdmin password',
95+
assertThrowsWithMessage(async () => {
96+
await adminClient.asUserWithCredentials(manager.emailAddress, manager.password);
97+
await adminClient.query(updateAdministratorDocument, {
98+
input: { id: superAdminId, password: 'pwned' },
99+
});
100+
}, 'does not have sufficient permissions'),
101+
);
102+
103+
it('leaves the SuperAdmin credentials intact after a blocked attempt', async () => {
104+
await adminClient.asSuperAdmin();
105+
const { me } = await adminClient.query(MeDocument);
106+
expect(me?.identifier).toBe(SUPER_ADMIN_USER_IDENTIFIER);
107+
});
108+
109+
it('allows a non-SuperAdmin with sufficient permissions to update a lower-privileged administrator', async () => {
110+
await adminClient.asUserWithCredentials(manager.emailAddress, manager.password);
111+
const { updateAdministrator } = await adminClient.query(updateAdministratorDocument, {
112+
input: { id: staffId, firstName: 'Updated', password: 'new-staff-password' },
113+
});
114+
expect(updateAdministrator.id).toBe(staffId);
115+
expect(updateAdministrator.firstName).toBe('Updated');
116+
});
117+
118+
it('allows a SuperAdmin to update another administrator', async () => {
119+
await adminClient.asSuperAdmin();
120+
const { updateAdministrator } = await adminClient.query(updateAdministratorDocument, {
121+
input: { id: staffId, lastName: 'BySuperAdmin' },
122+
});
123+
expect(updateAdministrator.id).toBe(staffId);
124+
expect(updateAdministrator.lastName).toBe('BySuperAdmin');
125+
});
126+
});

packages/core/src/service/services/administrator.service.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ export class AdministratorService {
219219
if (!administrator) {
220220
throw new EntityNotFoundError('Administrator', input.id);
221221
}
222+
await this.checkActiveUserCanManageAdministrator(ctx, administrator);
222223
if (input.roleIds) {
223224
await this.checkActiveUserCanGrantRoles(ctx, input.roleIds);
224225
}
@@ -299,6 +300,18 @@ export class AdministratorService {
299300
}
300301
}
301302

303+
/**
304+
* Ensures the active user holds all of the target administrator's permissions on all of the target's
305+
* channels before allowing the target to be modified, so a lower-privileged administrator cannot
306+
* modify a higher-privileged one (including a SuperAdmin).
307+
*/
308+
private async checkActiveUserCanManageAdministrator(ctx: RequestContext, administrator: Administrator) {
309+
const targetRoleIds = administrator.user.roles.map(role => role.id);
310+
if (targetRoleIds.length) {
311+
await this.checkActiveUserCanGrantRoles(ctx, targetRoleIds);
312+
}
313+
}
314+
302315
/**
303316
* @description
304317
* Assigns a Role to the Administrator's User entity.

0 commit comments

Comments
 (0)