mirror of
https://github.com/ever-co/ever-gauzy.git
synced 2026-10-02 01:54:50 +08:00
fix(security): Role-reference normalization + credential leak (GHSA-x4mv, GHSA-hh83, GHSA-hjcg) (#10245)
* fix(security): resolve every form of a role reference before authorizing it GHSA-x4mv-fhwj-g3rp (high): the role-change check read entity.role?.id and entity.roleId, but TypeORM also accepts a relation given as a bare id string, so a role sent as a plain SUPER_ADMIN uuid was invisible to the check and still persisted — self-escalation through the profile update. extractRoleIds and normalizeRolePayload now read every representation, reject a present but unresolvable role, refuse a role/roleId pair naming two different roles, and normalise the payload so the value that was checked is the value that is saved. Wired into updateProfile, UserCreateHandler, the register handler, invites (which previously stored a role the check never saw), employee and candidate creation, and the DTO validator. GHSA-hh83-hq74-gh9f (low): under DB_ORM=mikro-orm, wrap(entity).toJSON() ignores class-transformer, so a populated User in a listing carried its credential columns. refreshToken, code and codeExpireAt are hidden at the ORM level, and a last-line scrub removes credential keys from User-shaped objects in responses. GHSA-hjcg-633x-qq74 hardening: the register guard tenant-checks every role identifier in the body instead of only the first one it finds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(cspell): add the new vocabulary and use US spellings Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(invite): validate the role payload before branching on the inviter `createBulk` only read the body's role references inside the fallback branch, so the checks rode on the inviter's own role: an EMPLOYEE inviter, who is force-assigned the EMPLOYEE role, had a malformed or self-contradicting payload (`roleId` and `role` naming two different roles, `role: {}`, `roleId: ''`) silently accepted, while every other inviter got a 400 for the same body. That was never an escalation — the EMPLOYEE branch persists the checked role, which is the point of GHSA-x4mv-fhwj-g3rp — but an answer that depends on who is asking is a bad place to keep input validation, and it leaves the one caller whose role is overridden as the only one whose payload nobody parses. The extraction now runs once, before the branch, and a body naming two different roles is refused for everyone. The fallback keeps its own "exactly one" rule, since an invitation there cannot be issued without a role. Regression coverage added for the EMPLOYEE inviter, plus a control that a body with no role at all still issues an EMPLOYEE invitation. Also splits `scrubNode()` out of `scrubUserCredentials()` so the walker stays under SonarCloud's cognitive-complexity threshold (17 -> 9). No behaviour change: the same nodes are visited and the same keys deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(cspell): use the US spelling in the new invite comment Cspell flagged "licence" in the comment added by the previous commit; the wording now avoids the word entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Ruslan Konviser
co-authored by
Claude Opus 5
Claude Opus 5
parent
ebf676b2f8
commit
bf5aea9a7b
@@ -6,6 +6,7 @@ import { AuthService } from '../../auth.service';
|
||||
import { getORMType, MultiORMEnum } from '../../../core/utils';
|
||||
import { RequestContext } from '../../../core/context';
|
||||
import { UserService } from '../../../user/user.service';
|
||||
import { extractRoleIds, normalizeRolePayload } from '../../../user/role-assignment.helper';
|
||||
import { TypeOrmRoleRepository } from '../../../role/repository/type-orm-role.repository';
|
||||
import { MikroOrmRoleRepository } from '../../../role/repository/mikro-orm-role.repository';
|
||||
|
||||
@@ -36,11 +37,12 @@ export class AuthRegisterHandler implements ICommandHandler<AuthRegisterCommand>
|
||||
// enough either: the `role` RELATION wins over the flat `roleId` when the row is persisted
|
||||
// (AuthService.register pins roleId = role.id), so a body pairing a harmless `roleId` with a
|
||||
// privileged `role: { id }` would be validated as the harmless one and registered as the
|
||||
// privileged one.
|
||||
const targetRoleIds = [input.user?.roleId, input.user?.role?.id].filter((roleId) => !!roleId);
|
||||
if (input.user?.role && !targetRoleIds.length) {
|
||||
throw new BadRequestException('The specified role does not reference a valid role.');
|
||||
}
|
||||
// privileged one. `role` may also arrive as a bare id STRING, which `role?.id` never saw
|
||||
// (GHSA-x4mv-fhwj-g3rp): the shared helpers read every form, refuse a role key that references
|
||||
// nothing (400), refuse a `role`/`roleId` pair that disagrees (400), and pin both fields to the
|
||||
// one id checked below.
|
||||
normalizeRolePayload(input.user);
|
||||
const targetRoleIds = extractRoleIds(input.user);
|
||||
|
||||
if (targetRoleIds.length) {
|
||||
// Get tenant id from request context
|
||||
|
||||
@@ -41,7 +41,11 @@ export class CandidateCreateHandler implements ICommandHandler<CandidateCreateCo
|
||||
const user = await this._commandBus.execute(
|
||||
new UserCreateCommand({
|
||||
...input.user,
|
||||
// The role is decided here, server-side. Pin BOTH role fields to it: the spread above can
|
||||
// carry a body `user.roleId` (or a string `user.role`), which would otherwise sit next to the
|
||||
// trusted role and could be what gets persisted (GHSA-x4mv-fhwj-g3rp).
|
||||
role,
|
||||
roleId: role?.id,
|
||||
hash: await this._authService.getPasswordHash(input.password),
|
||||
preferredLanguage: languageCode || LanguagesEnum.ENGLISH,
|
||||
preferredComponentLayout: ComponentLayoutStyleEnum.TABLE
|
||||
|
||||
@@ -9,6 +9,11 @@ type CommonColumnOptions<T> = Omit<MikroORMColumnOptions<T>, 'type' | 'default'>
|
||||
// declare it) so the option is documented rather than silently inherited: MultiORMColumn
|
||||
// honors it on BOTH sides - TypeORM's @Column({ primary: true }) and MikroORM's @PrimaryKey().
|
||||
primary?: boolean;
|
||||
// MikroORM-only, restated for the same reason: `hidden: true` reaches MikroORM's @Property() and
|
||||
// drops the property from `wrap(entity).toJSON()` - which `CrudService.serialize()` applies to every
|
||||
// read under DB_ORM=mikro-orm, so a hidden column is not readable from those results either.
|
||||
// MultiORMColumn does not forward it to TypeORM.
|
||||
hidden?: boolean;
|
||||
};
|
||||
|
||||
// Represents MikroORM-specific column options, using MikroORM's PropertyOptions.
|
||||
|
||||
@@ -0,0 +1,116 @@
|
||||
import 'reflect-metadata';
|
||||
import { MikroORM, PrimaryKey, wrap } from '@mikro-orm/core';
|
||||
import { BetterSqliteDriver } from '@mikro-orm/better-sqlite';
|
||||
import { getMetadataArgsStorage } from 'typeorm';
|
||||
import { MultiORMColumn } from './column.decorator';
|
||||
import { MultiORMEntity } from './entity.decorator';
|
||||
|
||||
/**
|
||||
* GHSA-hh83-hq74-gh9f — `@MultiORMColumn({ hidden: true })` must reach MikroORM's `@Property()`.
|
||||
*
|
||||
* Under DB_ORM=mikro-orm, `CrudService.serialize()` returns `wrap(entity).toJSON()` plain objects, so
|
||||
* class-transformer's `@Exclude` never applies; MikroORM's own `hidden` is what keeps a credential
|
||||
* column out of that output. This drives the REAL decorator against an in-memory SQLite database and
|
||||
* also pins down the one side effect callers must know: `hidden` does not stop the column being
|
||||
* loaded (the entity still carries it), but it IS dropped from every `toJSON()` result — including
|
||||
* the ones services read back from `CrudService.find*`.
|
||||
*
|
||||
* The decorator reads `DB_ORM` when the class is DECORATED, so the fixtures are declared inside
|
||||
* functions after the variable is set, and the variable is restored straight away.
|
||||
*/
|
||||
function declareUnder<T>(orm: 'mikro-orm' | 'typeorm', declare: () => T): T {
|
||||
const previous = process.env.DB_ORM;
|
||||
process.env.DB_ORM = orm;
|
||||
try {
|
||||
return declare();
|
||||
} finally {
|
||||
if (previous === undefined) {
|
||||
delete process.env.DB_ORM;
|
||||
} else {
|
||||
process.env.DB_ORM = previous;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const HiddenFixture = declareUnder('mikro-orm', () => {
|
||||
@MultiORMEntity('multi_orm_hidden_fixture')
|
||||
class HiddenFixture {
|
||||
@PrimaryKey({ type: 'varchar' })
|
||||
id!: string;
|
||||
|
||||
@MultiORMColumn({ type: 'varchar' })
|
||||
email!: string;
|
||||
|
||||
@MultiORMColumn({ type: 'varchar', nullable: true, hidden: true })
|
||||
secret?: string;
|
||||
|
||||
// CONTROL: the same column without `hidden`, which toJSON() emits.
|
||||
@MultiORMColumn({ type: 'varchar', nullable: true })
|
||||
visibleSecret?: string;
|
||||
}
|
||||
return HiddenFixture;
|
||||
});
|
||||
|
||||
describe('MultiORMColumn hidden (GHSA-hh83-hq74-gh9f)', () => {
|
||||
describe('MikroORM', () => {
|
||||
let orm: MikroORM;
|
||||
|
||||
beforeAll(async () => {
|
||||
orm = await MikroORM.init({
|
||||
driver: BetterSqliteDriver,
|
||||
dbName: ':memory:',
|
||||
entities: [HiddenFixture],
|
||||
allowGlobalContext: true,
|
||||
discovery: { warnWhenNoEntities: false }
|
||||
});
|
||||
await orm.getSchemaGenerator().createSchema();
|
||||
const em = orm.em.fork();
|
||||
em.create(HiddenFixture, { id: 'row-1', email: 'ada@example.com', secret: 'secret-value', visibleSecret: 'secret-value' });
|
||||
await em.flush();
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await orm?.close(true);
|
||||
});
|
||||
|
||||
it('forwards hidden to the MikroORM property metadata', () => {
|
||||
const meta = orm.getMetadata().get(HiddenFixture.name);
|
||||
expect(meta.properties['secret'].hidden).toBe(true);
|
||||
expect(meta.properties['visibleSecret'].hidden).toBeFalsy();
|
||||
});
|
||||
|
||||
it('still loads the column: the entity carries it for server-side reads', async () => {
|
||||
const row = await orm.em.fork().findOneOrFail(HiddenFixture, { id: 'row-1' });
|
||||
expect(row.secret).toBe('secret-value');
|
||||
});
|
||||
|
||||
it('drops it from toJSON(), the shape CrudService.serialize() returns', async () => {
|
||||
const row = await orm.em.fork().findOneOrFail(HiddenFixture, { id: 'row-1' });
|
||||
const json: any = wrap(row).toJSON();
|
||||
|
||||
expect(json).not.toHaveProperty('secret');
|
||||
// CONTROL: without `hidden`, the value is serialized verbatim.
|
||||
expect(json.visibleSecret).toBe('secret-value');
|
||||
expect(json.email).toBe('ada@example.com');
|
||||
});
|
||||
});
|
||||
|
||||
describe('TypeORM', () => {
|
||||
it('does not forward the MikroORM-only option to @Column()', () => {
|
||||
const TypeOrmFixture = declareUnder('typeorm', () => {
|
||||
class TypeOrmHiddenFixture {
|
||||
@MultiORMColumn({ type: 'varchar', nullable: true, hidden: true })
|
||||
secret?: string;
|
||||
}
|
||||
return TypeOrmHiddenFixture;
|
||||
});
|
||||
|
||||
const column = getMetadataArgsStorage().columns.find(
|
||||
(args) => args.target === TypeOrmFixture && args.propertyName === 'secret'
|
||||
);
|
||||
expect(column).toBeDefined();
|
||||
expect(column.options).not.toHaveProperty('hidden');
|
||||
expect(column.options.nullable).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -47,7 +47,10 @@ export function MultiORMColumn<T>(
|
||||
|
||||
// Apply TypeORM decorator when using TypeORM
|
||||
if (ormType === MultiORMEnum.TypeORM) {
|
||||
TypeORMColumn({ type, ...options })(target, propertyKey);
|
||||
// `hidden` is MikroORM-only (it drops the property from `wrap(entity).toJSON()`); TypeORM has
|
||||
// no such column option, so it is not forwarded there.
|
||||
const { hidden: _hidden, ...typeOrmOptions } = options;
|
||||
TypeORMColumn({ type, ...typeOrmOptions })(target, propertyKey);
|
||||
}
|
||||
|
||||
// Apply MikroORM decorator when using MikroORM
|
||||
|
||||
@@ -3,6 +3,7 @@ import { Observable } from 'rxjs';
|
||||
import { catchError, map } from 'rxjs/operators';
|
||||
import { instanceToPlain } from 'class-transformer';
|
||||
import { toSafeHttpException } from './safe-http-exception';
|
||||
import { scrubUserCredentials } from './user-credential-scrub';
|
||||
|
||||
@Injectable()
|
||||
export class TransformInterceptor implements NestInterceptor {
|
||||
@@ -16,8 +17,10 @@ export class TransformInterceptor implements NestInterceptor {
|
||||
*/
|
||||
intercept(ctx: ExecutionContext, next: CallHandler): Observable<any> {
|
||||
return next.handle().pipe(
|
||||
// Transform the data using class-transformer's instanceToPlain
|
||||
map((data) => instanceToPlain(data)),
|
||||
// Transform the data using class-transformer's instanceToPlain, then strip credential columns
|
||||
// from any user that reached it WITHOUT its prototype (object spread, MikroORM `toJSON()`),
|
||||
// where `@Exclude` cannot apply (GHSA-hh83-hq74-gh9f)
|
||||
map((data) => scrubUserCredentials(instanceToPlain(data))),
|
||||
// Catch and handle errors
|
||||
// One rule for every error that escapes a controller — see `toSafeHttpException`:
|
||||
// BadRequest bodies intact, other HTTP exceptions keep their STRUCTURED body minus
|
||||
|
||||
@@ -0,0 +1,145 @@
|
||||
import { lastValueFrom, of } from 'rxjs';
|
||||
import { Exclude, instanceToPlain } from 'class-transformer';
|
||||
import { scrubUserCredentials, USER_CREDENTIAL_KEYS } from './user-credential-scrub';
|
||||
import { TransformInterceptor } from './transform.interceptor';
|
||||
|
||||
/**
|
||||
* GHSA-hh83-hq74-gh9f — credential columns leaking through prototype-less users.
|
||||
*
|
||||
* `@Exclude` is found through the prototype, so a user that reaches `TransformInterceptor` as a plain
|
||||
* object — an object spread of an entity, or what `CrudService.serialize()` returns under
|
||||
* DB_ORM=mikro-orm (`wrap(entity).toJSON()`) — was serialized with its password hash, refresh token
|
||||
* and one-time codes. The interceptor now scrubs every user-shaped object after `instanceToPlain`.
|
||||
*/
|
||||
|
||||
/** Minimal stand-in for the entity: same `@Exclude` set as `User`. */
|
||||
class UserLike {
|
||||
id: string;
|
||||
email: string;
|
||||
@Exclude({ toPlainOnly: true }) hash?: string;
|
||||
@Exclude({ toPlainOnly: true }) refreshToken?: string;
|
||||
@Exclude({ toPlainOnly: true }) code?: string;
|
||||
@Exclude({ toPlainOnly: true }) codeExpireAt?: Date;
|
||||
@Exclude({ toPlainOnly: true }) emailToken?: string;
|
||||
@Exclude({ toPlainOnly: true }) emailVerifiedAt?: Date;
|
||||
|
||||
constructor(input: Partial<UserLike>) {
|
||||
Object.assign(this, input);
|
||||
}
|
||||
}
|
||||
|
||||
const credentials = () => ({
|
||||
hash: '$2b$12$digest',
|
||||
refreshToken: 'hashed-refresh',
|
||||
code: '123456',
|
||||
codeExpireAt: new Date('2026-01-01T00:00:00Z'),
|
||||
emailToken: 'hashed-email-token',
|
||||
emailVerifiedAt: new Date('2026-01-01T00:00:00Z')
|
||||
});
|
||||
|
||||
/** A paginated `/user-organization` body whose users lost their prototype (MikroORM toJSON). */
|
||||
const leakyBody = () => ({
|
||||
items: [
|
||||
{
|
||||
id: 'uo-1',
|
||||
user: { id: 'u-1', email: 'ada@example.com', firstName: 'Ada', ...credentials() },
|
||||
createdByUser: { id: 'u-2', email: 'root@example.com', ...credentials() }
|
||||
}
|
||||
],
|
||||
total: 1
|
||||
});
|
||||
|
||||
async function runInterceptor(data: unknown): Promise<any> {
|
||||
const interceptor = new TransformInterceptor();
|
||||
return lastValueFrom(interceptor.intercept({} as any, { handle: () => of(data) }));
|
||||
}
|
||||
|
||||
describe('user credential scrub (GHSA-hh83-hq74-gh9f)', () => {
|
||||
it('CONTROL: instanceToPlain alone serializes a prototype-less user verbatim', () => {
|
||||
const plain: any = instanceToPlain(leakyBody());
|
||||
expect(plain.items[0].user.hash).toBe('$2b$12$digest');
|
||||
expect(plain.items[0].createdByUser.refreshToken).toBe('hashed-refresh');
|
||||
});
|
||||
|
||||
it('CONTROL: instanceToPlain does redact a real instance (why the leak needs a lost prototype)', () => {
|
||||
const plain: any = instanceToPlain(new UserLike({ id: 'u-1', email: 'ada@example.com', ...credentials() }));
|
||||
for (const key of USER_CREDENTIAL_KEYS) {
|
||||
expect(plain).not.toHaveProperty(key);
|
||||
}
|
||||
});
|
||||
|
||||
it('TransformInterceptor strips every credential column from nested prototype-less users', async () => {
|
||||
const body = await runInterceptor(leakyBody());
|
||||
|
||||
for (const user of [body.items[0].user, body.items[0].createdByUser]) {
|
||||
for (const key of USER_CREDENTIAL_KEYS) {
|
||||
expect(user).not.toHaveProperty(key);
|
||||
}
|
||||
}
|
||||
// Everything else is untouched.
|
||||
expect(body.items[0].user).toEqual({ id: 'u-1', email: 'ada@example.com', firstName: 'Ada' });
|
||||
expect(body.total).toBe(1);
|
||||
});
|
||||
|
||||
it('handles a top-level array and a top-level user', async () => {
|
||||
expect(await runInterceptor([{ email: 'a@example.com', hash: 'x', name: 'A' }])).toEqual([
|
||||
{ email: 'a@example.com', name: 'A' }
|
||||
]);
|
||||
expect(scrubUserCredentials({ email: 'a@example.com', hash: null, code: null })).toEqual({ email: 'a@example.com' });
|
||||
});
|
||||
|
||||
it('leaves `code` and `token` alone on objects that are not users', async () => {
|
||||
const body = {
|
||||
currency: { code: 'USD', name: 'Dollar' },
|
||||
product: { code: 'SKU-1', email: 'sales@example.com' },
|
||||
invite: { email: 'x@example.com', code: 'ABC123', token: 't' }
|
||||
};
|
||||
expect(await runInterceptor(body)).toEqual(body);
|
||||
});
|
||||
|
||||
it('passes primitives, empty bodies and Dates through', async () => {
|
||||
expect(await runInterceptor('ok')).toBe('ok');
|
||||
expect(await runInterceptor(undefined)).toBeUndefined();
|
||||
expect(scrubUserCredentials(null)).toBeNull();
|
||||
const date = new Date();
|
||||
expect(scrubUserCredentials(date)).toBe(date);
|
||||
});
|
||||
|
||||
/**
|
||||
* The shape test keys off the columns only `User` has, not off `hash` alone: under DB_ORM=mikro-orm
|
||||
* a projected read (`?select[...]`) can serialize a user WITHOUT `hash` but WITH `emailToken` or
|
||||
* `refreshToken`, and a `hash`-only test let that through.
|
||||
*/
|
||||
describe('a user projected without hash', () => {
|
||||
/** Verbatim copy of the narrower shape test, to show what it missed. */
|
||||
const hashOnlyShapeTest = (value: Record<string, unknown>) => 'email' in value && 'hash' in value;
|
||||
|
||||
it.each([['emailToken', 'hashed-email-token'], ['refreshToken', 'hashed-refresh']])(
|
||||
'CONTROL: the hash-only shape test did not recognize a user carrying only %s',
|
||||
(key, value) => {
|
||||
expect(hashOnlyShapeTest({ id: 'u-1', email: 'ada@example.com', [key]: value })).toBe(false);
|
||||
}
|
||||
);
|
||||
|
||||
it.each([['emailToken', 'hashed-email-token'], ['refreshToken', 'hashed-refresh']])(
|
||||
'is still scrubbed when it carries only %s',
|
||||
async (key, value) => {
|
||||
const body = await runInterceptor({ items: [{ id: 'u-1', email: 'ada@example.com', [key]: value }] });
|
||||
expect(body.items[0]).toEqual({ id: 'u-1', email: 'ada@example.com' });
|
||||
}
|
||||
);
|
||||
|
||||
it('still leaves an invite (email + code, no user-only column) alone', async () => {
|
||||
const invite = { email: 'x@example.com', code: 'ABC123', token: 't' };
|
||||
expect(await runInterceptor({ invite })).toEqual({ invite });
|
||||
});
|
||||
});
|
||||
|
||||
it('survives a cyclic structure', () => {
|
||||
const user: any = { email: 'a@example.com', hash: 'x' };
|
||||
const node: any = { user };
|
||||
user.self = node;
|
||||
expect(() => scrubUserCredentials(node)).not.toThrow();
|
||||
expect(user).not.toHaveProperty('hash');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,108 @@
|
||||
/**
|
||||
* The `User` columns that must never leave the API. They mirror the entity's
|
||||
* `@Exclude({ toPlainOnly: true })` set.
|
||||
*/
|
||||
export const USER_CREDENTIAL_KEYS: ReadonlyArray<string> = [
|
||||
'hash',
|
||||
'refreshToken',
|
||||
'code',
|
||||
'codeExpireAt',
|
||||
'emailToken',
|
||||
'emailVerifiedAt'
|
||||
];
|
||||
|
||||
/**
|
||||
* True for a plain object or array: the only shapes `instanceToPlain` output needs walking through.
|
||||
* Dates, buffers, streams and other class instances are left alone.
|
||||
*/
|
||||
function isWalkable(value: unknown): value is Record<string, unknown> | unknown[] {
|
||||
if (value === null || typeof value !== 'object') {
|
||||
return false;
|
||||
}
|
||||
if (Array.isArray(value)) {
|
||||
return true;
|
||||
}
|
||||
const prototype = Object.getPrototypeOf(value);
|
||||
return prototype === Object.prototype || prototype === null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Credential columns whose NAME belongs to `User` alone (no other entity or response DTO in the
|
||||
* repository declares one), so their presence identifies a serialized user.
|
||||
*
|
||||
* `code`, `codeExpireAt` and `emailVerifiedAt` are deliberately NOT in this list: `code` is an
|
||||
* ordinary field on `Invite`, `OrganizationTeamJoinRequest`, currencies and products — and the first
|
||||
* two carry an `email` too, so keying off it would strip a legitimate field.
|
||||
*/
|
||||
const USER_IDENTIFYING_KEYS: ReadonlyArray<string> = ['hash', 'refreshToken', 'emailToken'];
|
||||
|
||||
/**
|
||||
* True for an object shaped like a serialized `User`: it has an `email` plus at least one column
|
||||
* that only `User` has.
|
||||
*
|
||||
* Deliberately narrow, and matched on more than `hash` alone: a MikroORM read that projects only
|
||||
* some columns (`?select[...]`) can hand back a user WITHOUT `hash` but with `emailToken` or
|
||||
* `refreshToken`, which a `hash`-only test would wave through.
|
||||
*/
|
||||
function isUserShaped(value: Record<string, unknown>): boolean {
|
||||
return 'email' in value && USER_IDENTIFYING_KEYS.some((key) => key in value);
|
||||
}
|
||||
|
||||
/**
|
||||
* Scrubs ONE node and reports the values that still have to be walked.
|
||||
*
|
||||
* An array holds no keys of its own, so only its items are handed back; a user-shaped object loses
|
||||
* its credential columns first, so they are never returned as children either.
|
||||
*
|
||||
* @param node A plain object or array from the response body (mutated in place).
|
||||
* @returns The node's child values, to be walked in turn.
|
||||
*/
|
||||
function scrubNode(node: Record<string, unknown> | unknown[]): unknown[] {
|
||||
if (Array.isArray(node)) {
|
||||
return node;
|
||||
}
|
||||
if (isUserShaped(node)) {
|
||||
for (const key of USER_CREDENTIAL_KEYS) {
|
||||
delete node[key];
|
||||
}
|
||||
}
|
||||
return Object.values(node);
|
||||
}
|
||||
|
||||
/**
|
||||
* Last line of defense for GHSA-hh83-hq74-gh9f: removes the credential columns from every
|
||||
* `User`-shaped object in an already-serialized response body (mutated in place).
|
||||
*
|
||||
* `@Exclude` only works on a real `User` instance, because class-transformer finds the metadata
|
||||
* through the prototype. A prototype-less user — an object spread of an entity, or the plain objects
|
||||
* that `CrudService.serialize()` returns under DB_ORM=mikro-orm (`wrap(entity).toJSON()`) — was
|
||||
* serialized with its password hash, refresh token and one-time codes. This runs AFTER
|
||||
* `instanceToPlain`, so real instances are already clean and this is a no-op for them.
|
||||
*
|
||||
* @param data The response body produced by `instanceToPlain`.
|
||||
* @returns The same value, with credentials removed from every user-shaped object.
|
||||
*/
|
||||
export function scrubUserCredentials<T>(data: T): T {
|
||||
if (!isWalkable(data)) {
|
||||
return data;
|
||||
}
|
||||
|
||||
const seen = new WeakSet<object>();
|
||||
const stack: Array<Record<string, unknown> | unknown[]> = [data];
|
||||
|
||||
while (stack.length) {
|
||||
const node = stack.pop();
|
||||
if (!node || seen.has(node)) {
|
||||
continue;
|
||||
}
|
||||
seen.add(node);
|
||||
|
||||
for (const child of scrubNode(node)) {
|
||||
if (isWalkable(child)) {
|
||||
stack.push(child);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return data;
|
||||
}
|
||||
@@ -114,7 +114,11 @@ export class EmployeeCreateHandler implements ICommandHandler<EmployeeCreateComm
|
||||
const user = await this._commandBus.execute(
|
||||
new UserCreateCommand({
|
||||
...input.user,
|
||||
// The role is decided here, server-side. Pin BOTH role fields to it: the spread above can
|
||||
// carry a body `user.roleId` (or a string `user.role`), which would otherwise sit next to the
|
||||
// trusted role and could be what gets persisted (GHSA-x4mv-fhwj-g3rp).
|
||||
role,
|
||||
roleId: role?.id,
|
||||
hash: passwordHash,
|
||||
preferredLanguage: languageCode,
|
||||
preferredComponentLayout: ComponentLayoutStyleEnum.TABLE
|
||||
|
||||
@@ -0,0 +1,151 @@
|
||||
/**
|
||||
* 🛑 This import must stay FIRST, before any import that pulls a core service or handler — the
|
||||
* entity graph has to finish initializing before anything applies the custom validators.
|
||||
* See invite-accept.security.spec.ts.
|
||||
*/
|
||||
import '../core/entities/internal';
|
||||
import { BadRequestException, UnauthorizedException } from '@nestjs/common';
|
||||
import { RolesEnum } from '@gauzy/contracts';
|
||||
import { RequestContext } from '../core/context';
|
||||
import { InviteService } from './invite.service';
|
||||
|
||||
/**
|
||||
* GHSA-x4mv-fhwj-g3rp sibling — `POST /invite/emails` (ORG_INVITE_EDIT or ORG_TEAM_ADD).
|
||||
*
|
||||
* The service decided which role an invitation may carry (an EMPLOYEE inviter is limited to the
|
||||
* EMPLOYEE role; others to the role they asked for, SUPER_ADMIN only for a super admin) and then
|
||||
* persisted the BODY `roleId` regardless. An employee could therefore mint SUPER_ADMIN invitations.
|
||||
* The invitation now carries the role that was checked.
|
||||
*/
|
||||
describe('InviteService.createBulk role (GHSA-x4mv-fhwj-g3rp)', () => {
|
||||
const TENANT = 'tenant-1';
|
||||
const ORGANIZATION = 'org-1';
|
||||
const ROLES: Record<string, { id: string; name: RolesEnum }> = {
|
||||
'role-employee': { id: 'role-employee', name: RolesEnum.EMPLOYEE },
|
||||
'role-manager': { id: 'role-manager', name: RolesEnum.MANAGER },
|
||||
'role-super-admin': { id: 'role-super-admin', name: RolesEnum.SUPER_ADMIN }
|
||||
};
|
||||
|
||||
/** Thrown by the stubbed saveMany so the test stops before the e-mail side effects. */
|
||||
class Saved extends Error {
|
||||
constructor(readonly invites: any[]) {
|
||||
super('saved');
|
||||
}
|
||||
}
|
||||
|
||||
function build(callerRoleId: string) {
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT);
|
||||
jest.spyOn(RequestContext, 'currentUserId').mockReturnValue('inviter-1');
|
||||
jest.spyOn(RequestContext, 'currentRoleId').mockReturnValue(callerRoleId);
|
||||
|
||||
const roleService = {
|
||||
// The real call is tenant-scoped; `where: { name }` narrows the caller-role probe to EMPLOYEE.
|
||||
findOneByIdString: jest.fn(async (id: string, options?: any) => {
|
||||
const role = ROLES[id];
|
||||
if (!role || (options?.where?.name && options.where.name !== role.name)) {
|
||||
throw new Error('EntityNotFound');
|
||||
}
|
||||
return role;
|
||||
})
|
||||
};
|
||||
|
||||
const service: InviteService = Object.create(InviteService.prototype);
|
||||
Object.assign(service, {
|
||||
configService: { get: () => 'http://localhost:4200' },
|
||||
fetchInvitesRelations: jest.fn(async () => ({
|
||||
projects: [],
|
||||
departments: [],
|
||||
organizationContacts: [],
|
||||
organizationTeams: []
|
||||
})),
|
||||
userService: {
|
||||
findOneByIdString: jest.fn(async () => ({ id: 'inviter-1', role: ROLES[callerRoleId] }))
|
||||
},
|
||||
roleService,
|
||||
organizationService: { findOneByIdString: jest.fn(async () => ({ id: ORGANIZATION, inviteExpiryPeriod: 7 })) },
|
||||
findAll: jest.fn(async () => ({ items: [], total: 0 })),
|
||||
typeOrmOrganizationTeamEmployeeRepository: { findBy: jest.fn(async () => []) },
|
||||
saveMany: jest.fn(async (invites: any[]) => {
|
||||
throw new Saved(invites);
|
||||
})
|
||||
});
|
||||
return { service, roleService };
|
||||
}
|
||||
|
||||
async function invitesOf(service: InviteService, body: Record<string, unknown>): Promise<any[]> {
|
||||
try {
|
||||
await service.createBulk(
|
||||
{ emailIds: ['new@example.com'], organizationId: ORGANIZATION, tenantId: TENANT, ...body } as any,
|
||||
'en' as any
|
||||
);
|
||||
} catch (error) {
|
||||
if (error instanceof Saved) {
|
||||
return error.invites;
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
throw new Error('saveMany was not reached');
|
||||
}
|
||||
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
it('an EMPLOYEE inviter asking for SUPER_ADMIN issues an EMPLOYEE invitation', async () => {
|
||||
const { service } = build('role-employee');
|
||||
|
||||
const [invite] = await invitesOf(service, { roleId: 'role-super-admin' });
|
||||
|
||||
// CONTROL: the body asked for SUPER_ADMIN; pre-fix this is exactly what was persisted.
|
||||
expect(invite.roleId).not.toBe('role-super-admin');
|
||||
expect(invite.roleId).toBe('role-employee');
|
||||
});
|
||||
|
||||
it('a manager still invites with the role it asked for', async () => {
|
||||
const { service } = build('role-manager');
|
||||
|
||||
const [invite] = await invitesOf(service, { roleId: 'role-employee' });
|
||||
|
||||
expect(invite.roleId).toBe('role-employee');
|
||||
});
|
||||
|
||||
it('still refuses a non-super-admin asking for SUPER_ADMIN, in any form', async () => {
|
||||
for (const body of [{ roleId: 'role-super-admin' }, { role: 'role-super-admin' }, { role: { id: 'role-super-admin' } }]) {
|
||||
const { service } = build('role-manager');
|
||||
await expect(invitesOf(service, body)).rejects.toBeInstanceOf(UnauthorizedException);
|
||||
jest.restoreAllMocks();
|
||||
}
|
||||
});
|
||||
|
||||
it('refuses a body naming two different roles, or none', async () => {
|
||||
for (const body of [{ roleId: 'role-employee', role: { id: 'role-super-admin' } }, {}, { role: {} }]) {
|
||||
const { service } = build('role-manager');
|
||||
await expect(invitesOf(service, body)).rejects.toBeInstanceOf(BadRequestException);
|
||||
jest.restoreAllMocks();
|
||||
}
|
||||
});
|
||||
|
||||
/**
|
||||
* The EMPLOYEE branch is force-assigned the EMPLOYEE role, so a bad payload was never a privilege
|
||||
* escalation here — but it was silently accepted, and the same body answered 400 for every other
|
||||
* inviter. Input validation now runs before the branch, so the answer no longer depends on who asks.
|
||||
*/
|
||||
it('refuses a malformed or self-contradicting body from an EMPLOYEE inviter too', async () => {
|
||||
for (const body of [
|
||||
{ roleId: 'role-employee', role: { id: 'role-super-admin' } },
|
||||
{ roleId: 'role-manager', role: 'role-super-admin' },
|
||||
{ role: {} },
|
||||
{ roleId: '' }
|
||||
]) {
|
||||
const { service } = build('role-employee');
|
||||
await expect(invitesOf(service, body)).rejects.toBeInstanceOf(BadRequestException);
|
||||
jest.restoreAllMocks();
|
||||
}
|
||||
});
|
||||
|
||||
it('still lets an EMPLOYEE inviter send a body with no role at all', async () => {
|
||||
const { service } = build('role-employee');
|
||||
|
||||
const [invite] = await invitesOf(service, {});
|
||||
|
||||
expect(invite.roleId).toBe('role-employee');
|
||||
});
|
||||
});
|
||||
@@ -51,6 +51,7 @@ import { LIKE_OPERATOR } from './../core/util';
|
||||
import { EmailService } from './../email-send/email.service';
|
||||
import { UserService } from '../user/user.service';
|
||||
import { RoleService } from './../role/role.service';
|
||||
import { extractRoleIds } from '../user/role-assignment.helper';
|
||||
import { OrganizationService } from './../organization/organization.service';
|
||||
import { OrganizationTeamService } from './../organization-team/organization-team.service';
|
||||
import { OrganizationDepartmentService } from './../organization-department/organization-department.service';
|
||||
@@ -162,7 +163,6 @@ export class InviteService extends TenantAwareCrudService<Invite> {
|
||||
organizationContactIds = [],
|
||||
departmentIds = [],
|
||||
teamIds = [],
|
||||
roleId,
|
||||
organizationId,
|
||||
startedWorkOn,
|
||||
appliedDate,
|
||||
@@ -192,6 +192,17 @@ export class InviteService extends TenantAwareCrudService<Invite> {
|
||||
relations: { role: true }
|
||||
});
|
||||
|
||||
// The role the body asks for, read in every form it can carry it (`roleId`, `role` as an id
|
||||
// string or `{ id }`). Validated BEFORE the inviter's own role is looked at, so a malformed or
|
||||
// self-contradicting payload is refused for every caller and not just for the ones that reach
|
||||
// the fallback below — an EMPLOYEE inviter is force-assigned the EMPLOYEE role, but that is an
|
||||
// authorization decision and must not double as permission to ignore bad input.
|
||||
// `extractRoleIds` itself throws on a role key that is present but references nothing.
|
||||
const requestedRoleIds = extractRoleIds(input);
|
||||
if (requestedRoleIds.length > 1) {
|
||||
throw new BadRequestException('The role and roleId fields must reference the same role.');
|
||||
}
|
||||
|
||||
// Invited Role
|
||||
let role: IRole;
|
||||
|
||||
@@ -203,8 +214,13 @@ export class InviteService extends TenantAwareCrudService<Invite> {
|
||||
where: { name: RolesEnum.EMPLOYEE }
|
||||
});
|
||||
} catch (error) {
|
||||
// If the current role is not an 'EMPLOYEE' role, fallback to specified 'roleId'
|
||||
role = await this.roleService.findOneByIdString(roleId);
|
||||
// If the current role is not an 'EMPLOYEE' role, fallback to the requested role. Exactly one
|
||||
// role must be named: a second, unchecked identifier must not ride along, and an invitation
|
||||
// cannot be issued for no role at all (GHSA-x4mv-fhwj-g3rp).
|
||||
if (requestedRoleIds.length !== 1) {
|
||||
throw new BadRequestException('Exactly one valid role must be specified for the invitation.');
|
||||
}
|
||||
role = await this.roleService.findOneByIdString(requestedRoleIds[0]);
|
||||
|
||||
// Handle unauthorized access if the invitedByUser is not a 'SUPER_ADMIN'
|
||||
if (role.name === RolesEnum.SUPER_ADMIN && invitedByUser.role.name !== RolesEnum.SUPER_ADMIN) {
|
||||
@@ -287,7 +303,10 @@ export class InviteService extends TenantAwareCrudService<Invite> {
|
||||
new Invite({
|
||||
token,
|
||||
email,
|
||||
roleId,
|
||||
// The role that was CHECKED above — never the body `roleId`. For an EMPLOYEE inviter that
|
||||
// is the EMPLOYEE role whatever the body asked for; persisting the body value let an
|
||||
// employee issue invitations for any role, SUPER_ADMIN included (GHSA-x4mv-fhwj-g3rp).
|
||||
roleId: role.id,
|
||||
organizationId,
|
||||
tenantId,
|
||||
invitedByUserId,
|
||||
|
||||
@@ -0,0 +1,84 @@
|
||||
/**
|
||||
* This import must stay FIRST: the entity graph has to finish initializing before anything applies
|
||||
* the custom validators (pre-existing circular import; see invite-accept.security.spec.ts).
|
||||
*/
|
||||
import '../../core/entities/internal';
|
||||
import { plainToInstance } from 'class-transformer';
|
||||
import { IsNotEmpty, validate, ValidateIf, ValidationError } from 'class-validator';
|
||||
import { RequestContext } from '../../core/context';
|
||||
import { CreateUserDTO } from '../../user/dto/create-user.dto';
|
||||
import { UpdateUserDTO } from '../../user/dto/update-user.dto';
|
||||
import { CreateInviteDTO } from '../../invite/dto/create-invite.dto';
|
||||
|
||||
/**
|
||||
* GHSA-x4mv-fhwj-g3rp — the edge half of the fix: `RoleFeatureDTO.role` (inherited by the user
|
||||
* create/update, register and invite DTOs) accepts only an object carrying a UUID `id`.
|
||||
*
|
||||
* Only the `isRoleReference` constraint is asserted here. `IsRoleShouldExist` needs a database and
|
||||
* reports its own error; it is left to fail closed (no tenant context in this test).
|
||||
*/
|
||||
const EMP = '44444444-4444-4444-8444-444444444444';
|
||||
const SA = '55555555-5555-4555-8555-555555555555';
|
||||
|
||||
/** Constraint names reported for `role`, across the (possibly nested) validation errors. */
|
||||
function roleConstraints(errors: ValidationError[]): string[] {
|
||||
return errors.filter((error) => error.property === 'role').flatMap((error) => Object.keys(error.constraints ?? {}));
|
||||
}
|
||||
|
||||
async function validateAs(cls: any, payload: Record<string, unknown>): Promise<ValidationError[]> {
|
||||
return validate(plainToInstance(cls, payload) as object);
|
||||
}
|
||||
|
||||
describe('RoleFeatureDTO.role (GHSA-x4mv-fhwj-g3rp)', () => {
|
||||
beforeEach(() => {
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(undefined as any);
|
||||
});
|
||||
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
describe('CONTROL: the pre-fix declaration', () => {
|
||||
/** Verbatim copy of the old `role` decorators, minus the database-backed existence check. */
|
||||
class PreFixRoleFeatureDTO {
|
||||
@ValidateIf((it) => !it.role)
|
||||
@IsNotEmpty()
|
||||
readonly roleId: string;
|
||||
|
||||
@ValidateIf((it) => !it.roleId)
|
||||
@IsNotEmpty()
|
||||
readonly role: unknown;
|
||||
}
|
||||
|
||||
it('accepted a bare id string as the role', async () => {
|
||||
expect(roleConstraints(await validateAs(PreFixRoleFeatureDTO, { role: SA }))).toEqual([]);
|
||||
});
|
||||
|
||||
it('skipped every validator on role whenever roleId was present', async () => {
|
||||
expect(roleConstraints(await validateAs(PreFixRoleFeatureDTO, { roleId: EMP, role: 42 }))).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe.each([
|
||||
['CreateUserDTO', CreateUserDTO],
|
||||
['UpdateUserDTO', UpdateUserDTO],
|
||||
['CreateInviteDTO', CreateInviteDTO]
|
||||
])('%s', (_name, dto) => {
|
||||
it.each([[SA], [42], [[SA]], [{}], [{ id: 'not-a-uuid' }]])('refuses role %p', async (role) => {
|
||||
expect(roleConstraints(await validateAs(dto, { role }))).toContain('isRoleReference');
|
||||
});
|
||||
|
||||
it('validates role even when roleId is present', async () => {
|
||||
expect(roleConstraints(await validateAs(dto, { roleId: EMP, role: SA }))).toContain('isRoleReference');
|
||||
});
|
||||
|
||||
it('accepts the role object the UI sends', async () => {
|
||||
const errors = await validateAs(dto, { role: { id: EMP, name: 'EMPLOYEE', tenantId: EMP } });
|
||||
expect(roleConstraints(errors)).not.toContain('isRoleReference');
|
||||
});
|
||||
|
||||
it('leaves role alone when only roleId is sent', async () => {
|
||||
for (const role of [undefined, null]) {
|
||||
expect(roleConstraints(await validateAs(dto, { roleId: EMP, role }))).toEqual([]);
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -1,7 +1,7 @@
|
||||
import { IRelationalRole, IRole } from "@gauzy/contracts";
|
||||
import { ApiProperty } from "@nestjs/swagger";
|
||||
import { IsNotEmpty, ValidateIf } from "class-validator";
|
||||
import { IsRoleShouldExist } from "./../../shared/validators";
|
||||
import { IsRoleReference, IsRoleShouldExist } from "./../../shared/validators";
|
||||
|
||||
export class RoleFeatureDTO implements IRelationalRole {
|
||||
|
||||
@@ -13,9 +13,17 @@ export class RoleFeatureDTO implements IRelationalRole {
|
||||
})
|
||||
readonly roleId: string;
|
||||
|
||||
@ApiProperty({ type: () => String })
|
||||
@ValidateIf((it) => !it.roleId)
|
||||
/**
|
||||
* The role as an object carrying its `id` — never a bare id string (send `roleId` for that).
|
||||
*
|
||||
* It is validated whenever it is SENT, not only when `roleId` is absent: with the old
|
||||
* `ValidateIf(!roleId)` a body pairing a harmless `roleId` with any `role` value skipped every
|
||||
* validator on `role` (GHSA-x4mv-fhwj-g3rp).
|
||||
*/
|
||||
@ApiProperty({ type: () => Object, description: 'Role reference: an object with the role `id` (UUID).' })
|
||||
@ValidateIf((it) => !it.roleId || (it.role !== undefined && it.role !== null))
|
||||
@IsNotEmpty()
|
||||
@IsRoleReference()
|
||||
@IsRoleShouldExist({
|
||||
message: 'Role should be exist for this tenant.'
|
||||
})
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { ForbiddenException } from '@nestjs/common';
|
||||
import { BadRequestException, ForbiddenException } from '@nestjs/common';
|
||||
import { PermissionsEnum, RolesEnum } from '@gauzy/contracts';
|
||||
import { environment as env } from '@gauzy/config';
|
||||
import { sign } from 'jsonwebtoken';
|
||||
@@ -176,6 +176,69 @@ describe('RegisterAuthorizationGuard', () => {
|
||||
await expect(guard.canActivate(executionContext)).rejects.toBeInstanceOf(ForbiddenException);
|
||||
});
|
||||
|
||||
/**
|
||||
* GHSA-hjcg-633x-qq74 hardening: the tenant of EVERY role identifier in `body.user` is checked —
|
||||
* `roleId`, `role.id`, and `role` as a bare id string (GHSA-x4mv-fhwj-g3rp) — instead of the first
|
||||
* one picked with `roleId ?? role.id`.
|
||||
*/
|
||||
describe('role tenant isolation', () => {
|
||||
const OWN_ROLE = 'role-own-employee';
|
||||
const FOREIGN_ROLE = 'role-foreign-super-admin';
|
||||
|
||||
/** A role repository that only knows the caller tenant's role, as a tenant-scoped lookup would. */
|
||||
function buildWithRoles() {
|
||||
const built = build(activeSuperAdmin, superAdminState);
|
||||
const findOneByOrFail = jest.fn(async ({ id, tenantId }: { id: string; tenantId: string }) => {
|
||||
if (id === OWN_ROLE && tenantId === 'tenant-1') {
|
||||
return { id, tenantId };
|
||||
}
|
||||
throw new Error('EntityNotFound');
|
||||
});
|
||||
(built.guard as any).typeOrmRoleRepository = { findOneByOrFail };
|
||||
return { ...built, findOneByOrFail };
|
||||
}
|
||||
|
||||
it('CONTROL: the pre-fix `roleId ?? role.id` pick never looked at the foreign role', () => {
|
||||
const user: any = { roleId: OWN_ROLE, role: { id: FOREIGN_ROLE } };
|
||||
expect(user.roleId ?? user.role?.id).toBe(OWN_ROLE);
|
||||
});
|
||||
|
||||
it('refuses an own-tenant roleId paired with a foreign role.id', async () => {
|
||||
const { guard, findOneByOrFail } = buildWithRoles();
|
||||
const { executionContext } = context({
|
||||
user: { email: 'new@ever.co', roleId: OWN_ROLE, role: { id: FOREIGN_ROLE } }
|
||||
});
|
||||
|
||||
await expect(guard.canActivate(executionContext)).rejects.toBeInstanceOf(ForbiddenException);
|
||||
expect(findOneByOrFail).toHaveBeenCalledWith({ id: FOREIGN_ROLE, tenantId: 'tenant-1' });
|
||||
});
|
||||
|
||||
it('refuses a foreign role sent as a bare id string', async () => {
|
||||
const { guard } = buildWithRoles();
|
||||
const { executionContext } = context({ user: { email: 'new@ever.co', role: FOREIGN_ROLE } });
|
||||
|
||||
await expect(guard.canActivate(executionContext)).rejects.toBeInstanceOf(ForbiddenException);
|
||||
});
|
||||
|
||||
it('refuses a role key that references nothing', async () => {
|
||||
const { guard } = buildWithRoles();
|
||||
const { executionContext } = context({ user: { email: 'new@ever.co', role: { name: 'SUPER_ADMIN' } } });
|
||||
|
||||
await expect(guard.canActivate(executionContext)).rejects.toBeInstanceOf(BadRequestException);
|
||||
});
|
||||
|
||||
it.each([
|
||||
['roleId', { roleId: OWN_ROLE }],
|
||||
['a role object', { role: { id: OWN_ROLE, name: RolesEnum.EMPLOYEE } }],
|
||||
['both, agreeing', { roleId: OWN_ROLE, role: { id: OWN_ROLE } }]
|
||||
])('still admits an own-tenant role sent as %s', async (_label, fields) => {
|
||||
const { guard } = buildWithRoles();
|
||||
const { executionContext } = context({ user: { email: 'new@ever.co', ...fields } });
|
||||
|
||||
await expect(guard.canActivate(executionContext)).resolves.toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
it('still refuses an unauthenticated privileged registration', async () => {
|
||||
const { guard } = build(activeSuperAdmin, superAdminState);
|
||||
const { executionContext } = context({ user: { email: 'new@ever.co', roleId: 'role-target' } }, null);
|
||||
|
||||
@@ -10,6 +10,9 @@ import { RoleAuthorizationService } from '../../role/role-authorization.service'
|
||||
import { TypeOrmOrganizationRepository } from '../../organization/repository/type-orm-organization.repository';
|
||||
import { MikroOrmOrganizationRepository } from '../../organization/repository/mikro-orm-organization.repository';
|
||||
import { getORMType, MultiORMEnum } from '../../core/utils';
|
||||
// A dependency-free helper file (no entity imports), so this does not close the `user/` import cycle
|
||||
// described on `findCaller` below.
|
||||
import { extractRoleIds } from '../../user/role-assignment.helper';
|
||||
|
||||
/**
|
||||
* Minimal user shape set on the request by this guard when the register route
|
||||
@@ -189,9 +192,12 @@ export class RegisterAuthorizationGuard implements CanActivate {
|
||||
// Get ORM type from request context
|
||||
const ormType = getORMType();
|
||||
|
||||
// Validate tenant isolation for roleId (top-level or nested in user)
|
||||
const targetRoleId = body.user?.roleId ?? getIdFromRelation(body.user?.role);
|
||||
if (targetRoleId && typeof targetRoleId === 'string') {
|
||||
// Validate tenant isolation for EVERY role identifier nested in user — `roleId`, and `role` as an
|
||||
// object or a bare id string. Picking one with `roleId ?? role.id` left the other unchecked here,
|
||||
// so the guard's tenant isolation silently depended on the handler's second look
|
||||
// (GHSA-hjcg-633x-qq74 hardening; the string form is GHSA-x4mv-fhwj-g3rp). A role key that is
|
||||
// present but references nothing is refused with a 400 by `extractRoleIds`.
|
||||
for (const targetRoleId of extractRoleIds(body.user)) {
|
||||
try {
|
||||
const whereCondition = {
|
||||
id: targetRoleId,
|
||||
|
||||
@@ -5,6 +5,7 @@ import { RequestContext } from '../../../core/context';
|
||||
import { MultiORM, MultiORMEnum, getORMType } from '../../../core/utils';
|
||||
import { TypeOrmRoleRepository } from '../../../role/repository/type-orm-role.repository';
|
||||
import { MikroOrmRoleRepository } from '../../../role/repository/mikro-orm-role.repository';
|
||||
import { resolveRoleReference } from '../../../user/role-assignment.helper';
|
||||
|
||||
// Get the type of the Object-Relational Mapping (ORM) used in the application.
|
||||
const ormType: MultiORM = getORMType();
|
||||
@@ -30,9 +31,9 @@ export class RoleShouldExistConstraint implements ValidatorConstraintInterface {
|
||||
* @returns True if the role exists, false otherwise.
|
||||
*/
|
||||
async validate(role: string | IRole): Promise<boolean> {
|
||||
if (!role) return false;
|
||||
|
||||
const roleId: string = typeof role === 'string' ? role : role.id;
|
||||
// The same allowlisted reading the services use (GHSA-x4mv-fhwj-g3rp): an id string or an object
|
||||
// with a non-empty string `id`. Anything else (a number, an array, `{ id: 42 }`) is not a role.
|
||||
const roleId = resolveRoleReference(role);
|
||||
if (!roleId) return false;
|
||||
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
|
||||
@@ -5,6 +5,7 @@ export * from './is-employee-belongs-to-organization.decorator';
|
||||
export * from './is-expense-category-exist.decorator';
|
||||
export * from './is-organization-belongs-to-user.decorator';
|
||||
export * from './is-role-already-exist.decorator';
|
||||
export * from './is-role-reference.decorator';
|
||||
export * from './is-role-should-exist.decorator';
|
||||
export * from './is-team-already-exist.decorator';
|
||||
export * from './is-tenant-belongs-to-user.decorator';
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
import { buildMessage, isUUID, ValidateBy, ValidationOptions } from 'class-validator';
|
||||
|
||||
/**
|
||||
* Checks that a value is a role reference in OBJECT form: a plain object whose `id` is a UUID.
|
||||
*
|
||||
* @param value The value to check.
|
||||
* @returns True for `{ id: '<uuid>', ... }`, false for anything else (a bare id string included).
|
||||
*/
|
||||
export function isRoleReference(value: unknown): boolean {
|
||||
if (value === null || typeof value !== 'object' || Array.isArray(value)) {
|
||||
return false;
|
||||
}
|
||||
return isUUID((value as { id?: unknown }).id);
|
||||
}
|
||||
|
||||
/**
|
||||
* Accepts a `role` relation only as an object carrying a UUID `id` (extra fields, such as a full role
|
||||
* record the client loaded, are allowed; only the `id` is persisted).
|
||||
*
|
||||
* A bare id STRING used to pass validation (`IsRoleShouldExist` accepts one) and reach the services,
|
||||
* whose role checks read `role?.id` and so saw nothing, while TypeORM wrote the string as the foreign
|
||||
* key (GHSA-x4mv-fhwj-g3rp). The services now read every form themselves; this refuses the string form
|
||||
* at the edge as well. Clients send the flat `roleId` for a bare id.
|
||||
*
|
||||
* @param validationOptions - Validation options.
|
||||
* @returns {PropertyDecorator} - Decorator function.
|
||||
*/
|
||||
export const IsRoleReference = (validationOptions?: ValidationOptions): PropertyDecorator =>
|
||||
ValidateBy(
|
||||
{
|
||||
name: 'isRoleReference',
|
||||
validator: {
|
||||
validate: (value): boolean => isRoleReference(value),
|
||||
defaultMessage: buildMessage(
|
||||
(eachPrefix) => eachPrefix + '$property must be an object with a UUID id',
|
||||
validationOptions
|
||||
)
|
||||
}
|
||||
},
|
||||
validationOptions
|
||||
);
|
||||
@@ -2,6 +2,7 @@ import { CommandHandler, ICommandHandler } from '@nestjs/cqrs';
|
||||
import { IUser } from '@gauzy/contracts';
|
||||
import { UserCreateCommand } from '../user.create.command';
|
||||
import { UserService } from '../../user.service';
|
||||
import { normalizeRolePayload } from '../../role-assignment.helper';
|
||||
|
||||
@CommandHandler(UserCreateCommand)
|
||||
export class UserCreateHandler implements ICommandHandler<UserCreateCommand> {
|
||||
@@ -16,11 +17,16 @@ export class UserCreateHandler implements ICommandHandler<UserCreateCommand> {
|
||||
public async execute(command: UserCreateCommand): Promise<IUser> {
|
||||
const { input } = command;
|
||||
|
||||
// Every form of the role — `roleId`, `role` as a bare id string, `role: { id }` — is read, and
|
||||
// the payload is pinned to that single id so the role that is checked is the role that is
|
||||
// saved (GHSA-x4mv-fhwj-g3rp). A malformed role key, or a `role`/`roleId` pair that disagrees,
|
||||
// is a 400.
|
||||
normalizeRolePayload(input);
|
||||
|
||||
// Creating a SUPER_ADMIN is reserved to callers who may edit super admins — the same boundary
|
||||
// the register handler and invite creation enforce. Both the flat `roleId` and the `role`
|
||||
// relation are resolved from the database (the relation wins on persist), and an id that does
|
||||
// not belong to the caller's tenant is refused rather than ignored.
|
||||
await this.userService.assertCanAssignRoles([input?.roleId, input?.role?.id]);
|
||||
// the register handler and invite creation enforce. The role is resolved from the database, and
|
||||
// an id that does not belong to the caller's tenant is refused rather than ignored.
|
||||
await this.userService.assertCanAssignRoles(input);
|
||||
|
||||
return await this.userService.create(input);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,198 @@
|
||||
// The handlers are exercised with their collaborators stubbed; importing the real services drags in the
|
||||
// whole core entity graph (pre-existing circular import). Same technique as user.service.account-status.spec.ts.
|
||||
jest.mock('../auth/auth.service', () => ({ AuthService: class AuthService {} }));
|
||||
jest.mock('../user-organization/user-organization.services', () => ({
|
||||
UserOrganizationService: class UserOrganizationService {}
|
||||
}));
|
||||
jest.mock('../employee/employee.service', () => ({ EmployeeService: class EmployeeService {} }));
|
||||
jest.mock('../candidate/candidate.service', () => ({ CandidateService: class CandidateService {} }));
|
||||
jest.mock('../email-send/email.service', () => ({ EmailService: class EmailService {} }));
|
||||
jest.mock('../role/role.service', () => ({ RoleService: class RoleService {} }));
|
||||
jest.mock('./user.service', () => ({ UserService: class UserService {} }));
|
||||
jest.mock('../role/repository/type-orm-role.repository', () => ({
|
||||
TypeOrmRoleRepository: class TypeOrmRoleRepository {}
|
||||
}));
|
||||
jest.mock('../role/repository/mikro-orm-role.repository', () => ({
|
||||
MikroOrmRoleRepository: class MikroOrmRoleRepository {}
|
||||
}));
|
||||
jest.mock('../core/utils', () => ({
|
||||
getORMType: () => 'typeorm',
|
||||
MultiORMEnum: { TypeORM: 'typeorm', MikroORM: 'mikro-orm' }
|
||||
}));
|
||||
|
||||
import { BadRequestException, NotFoundException } from '@nestjs/common';
|
||||
import { PermissionsEnum, RolesEnum } from '@gauzy/contracts';
|
||||
import { RequestContext } from '../core/context';
|
||||
import { EmployeeCreateHandler } from '../employee/commands/handlers/employee.create.handler';
|
||||
import { EmployeeCreateCommand } from '../employee/commands/employee.create.command';
|
||||
import { CandidateCreateHandler } from '../candidate/commands/handlers/candidate.create.handler';
|
||||
import { CandidateCreateCommand } from '../candidate/commands/candidate.create.command';
|
||||
import { AuthRegisterHandler } from '../auth/commands/handlers/auth.register.handler';
|
||||
import { AuthRegisterCommand } from '../auth/commands/auth.register.command';
|
||||
import { UserCreateCommand } from './commands/user.create.command';
|
||||
|
||||
/**
|
||||
* GHSA-x4mv-fhwj-g3rp siblings: every other role-assignment site reads the role in all its forms and
|
||||
* persists only the role that was decided/checked.
|
||||
*/
|
||||
const TENANT = 'tenant-1';
|
||||
const EMPLOYEE_ROLE = { id: 'role-employee', name: RolesEnum.EMPLOYEE, tenantId: TENANT };
|
||||
const CANDIDATE_ROLE = { id: 'role-candidate', name: RolesEnum.CANDIDATE, tenantId: TENANT };
|
||||
const SA = 'role-super-admin';
|
||||
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
/** The user payload a handler sent to `UserCreateCommand`. */
|
||||
function userPayloadOf(commandBus: { execute: jest.Mock }): any {
|
||||
const call = commandBus.execute.mock.calls.find(([command]) => command instanceof UserCreateCommand);
|
||||
return call?.[0].input;
|
||||
}
|
||||
|
||||
describe('EmployeeCreateHandler / CandidateCreateHandler pin the trusted role', () => {
|
||||
beforeEach(() => {
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT);
|
||||
});
|
||||
|
||||
it('CONTROL: the pre-fix spread kept a body user.roleId next to the trusted role', () => {
|
||||
const body = { email: 'x@example.com', roleId: SA };
|
||||
const preFix: any = { ...body, role: EMPLOYEE_ROLE };
|
||||
expect(preFix.roleId).toBe(SA);
|
||||
expect(preFix.role.id).toBe(EMPLOYEE_ROLE.id);
|
||||
});
|
||||
|
||||
it.each([
|
||||
['a body roleId', { roleId: SA }],
|
||||
['a bare-string body role', { role: SA }]
|
||||
])('employee create drops %s', async (_label, fields) => {
|
||||
const commandBus = {
|
||||
execute: jest.fn(async (command: any) => ({ id: 'user-1', ...command.input }))
|
||||
};
|
||||
const handler = new EmployeeCreateHandler(
|
||||
commandBus as any,
|
||||
{ create: jest.fn(async (input: any) => ({ id: 'employee-1', ...input })) } as any,
|
||||
{ addUserToOrganization: jest.fn() } as any,
|
||||
{ getPasswordHash: jest.fn(async () => 'digest') } as any,
|
||||
{ welcomeUser: jest.fn() } as any,
|
||||
{ findOneByWhereOptions: jest.fn(async () => EMPLOYEE_ROLE) } as any,
|
||||
{
|
||||
findOneByOptions: jest.fn(async () => {
|
||||
throw new NotFoundException();
|
||||
})
|
||||
} as any
|
||||
);
|
||||
|
||||
await handler.execute(
|
||||
new EmployeeCreateCommand({
|
||||
organizationId: 'org-1',
|
||||
password: 'correct-horse',
|
||||
user: { email: 'x@example.com', ...fields }
|
||||
} as any)
|
||||
);
|
||||
|
||||
const user = userPayloadOf(commandBus);
|
||||
expect(user.role).toBe(EMPLOYEE_ROLE);
|
||||
expect(user.roleId).toBe(EMPLOYEE_ROLE.id);
|
||||
});
|
||||
|
||||
it.each([
|
||||
['a body roleId', { roleId: SA }],
|
||||
['a bare-string body role', { role: SA }]
|
||||
])('candidate create drops %s', async (_label, fields) => {
|
||||
const commandBus = {
|
||||
execute: jest.fn(async (command: any) => ({ id: 'user-1', ...command.input }))
|
||||
};
|
||||
const handler = new CandidateCreateHandler(
|
||||
commandBus as any,
|
||||
{ getPasswordHash: jest.fn(async () => 'digest') } as any,
|
||||
{ create: jest.fn(async (input: any) => ({ id: 'candidate-1', ...input })) } as any,
|
||||
{ findOneByWhereOptions: jest.fn(async () => CANDIDATE_ROLE) } as any,
|
||||
{ addUserToOrganization: jest.fn() } as any,
|
||||
{ welcomeUser: jest.fn() } as any
|
||||
);
|
||||
|
||||
await handler.execute(
|
||||
new CandidateCreateCommand({ password: 'correct-horse', user: { email: 'x@example.com', ...fields } } as any)
|
||||
);
|
||||
|
||||
const user = userPayloadOf(commandBus);
|
||||
expect(user.role).toBe(CANDIDATE_ROLE);
|
||||
expect(user.roleId).toBe(CANDIDATE_ROLE.id);
|
||||
});
|
||||
});
|
||||
|
||||
describe('AuthRegisterHandler reads every role form', () => {
|
||||
function build() {
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT);
|
||||
jest.spyOn(RequestContext, 'currentUserId').mockReturnValue('admin-1');
|
||||
jest.spyOn(RequestContext, 'hasPermission').mockImplementation(
|
||||
(permission: PermissionsEnum) => permission !== PermissionsEnum.SUPER_ADMIN_EDIT
|
||||
);
|
||||
const roles: Record<string, any> = {
|
||||
[EMPLOYEE_ROLE.id]: EMPLOYEE_ROLE,
|
||||
[SA]: { id: SA, name: RolesEnum.SUPER_ADMIN, tenantId: TENANT }
|
||||
};
|
||||
const authService = { register: jest.fn(async (input: any) => input.user) };
|
||||
const handler = new AuthRegisterHandler(
|
||||
authService as any,
|
||||
{ findOneByIdString: jest.fn(async () => ({ role: EMPLOYEE_ROLE })) } as any,
|
||||
{
|
||||
findOneByOrFail: jest.fn(async ({ id }: any) => {
|
||||
if (!roles[id]) throw new Error('EntityNotFound');
|
||||
return roles[id];
|
||||
})
|
||||
} as any,
|
||||
{ findOneOrFail: jest.fn() } as any
|
||||
);
|
||||
return { handler, authService };
|
||||
}
|
||||
|
||||
it('CONTROL: the pre-fix extraction read a harmless roleId and missed a string role beside it', () => {
|
||||
const user: any = { roleId: EMPLOYEE_ROLE.id, role: SA };
|
||||
expect([user.roleId, user.role?.id].filter((id) => !!id)).toEqual([EMPLOYEE_ROLE.id]);
|
||||
});
|
||||
|
||||
it('refuses a harmless roleId paired with a different (string) role', async () => {
|
||||
const { handler, authService } = build();
|
||||
|
||||
await expect(
|
||||
handler.execute(
|
||||
new AuthRegisterCommand({
|
||||
user: { email: 'x@example.com', roleId: EMPLOYEE_ROLE.id, role: SA },
|
||||
password: 'correct-horse',
|
||||
confirmPassword: 'correct-horse'
|
||||
} as any, 'en' as any)
|
||||
)
|
||||
).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(authService.register).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('still registers the role object the admin UI sends, with roleId pinned to it', async () => {
|
||||
const { handler, authService } = build();
|
||||
|
||||
await handler.execute(
|
||||
new AuthRegisterCommand({
|
||||
user: { email: 'x@example.com', role: EMPLOYEE_ROLE },
|
||||
password: 'correct-horse',
|
||||
confirmPassword: 'correct-horse'
|
||||
} as any, 'en' as any)
|
||||
);
|
||||
|
||||
const [input] = authService.register.mock.calls[0];
|
||||
expect(input.user.role).toBe(EMPLOYEE_ROLE);
|
||||
expect(input.user.roleId).toBe(EMPLOYEE_ROLE.id);
|
||||
});
|
||||
|
||||
it('still lets a plain self-registration (no role) through', async () => {
|
||||
const { handler, authService } = build();
|
||||
|
||||
await handler.execute(
|
||||
new AuthRegisterCommand({
|
||||
user: { email: 'x@example.com' },
|
||||
password: 'correct-horse',
|
||||
confirmPassword: 'correct-horse'
|
||||
} as any, 'en' as any)
|
||||
);
|
||||
|
||||
expect(authService.register).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,11 @@
|
||||
import { BadRequestException, ForbiddenException } from '@nestjs/common';
|
||||
import { RolesEnum } from '@gauzy/contracts';
|
||||
import { assertRoleAssignmentAllowed } from './role-assignment.helper';
|
||||
import {
|
||||
assertRoleAssignmentAllowed,
|
||||
extractRoleIds,
|
||||
normalizeRolePayload,
|
||||
resolveRoleReference
|
||||
} from './role-assignment.helper';
|
||||
|
||||
/**
|
||||
* Regression suite for the SUPER_ADMIN assignment boundary (GHSA-hjcg-633x-qq74 / GHSA-x4mv-fhwj-g3rp
|
||||
@@ -40,3 +45,114 @@ describe('assertRoleAssignmentAllowed', () => {
|
||||
expect(fixed).toThrow(ForbiddenException);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* GHSA-x4mv-fhwj-g3rp: the role checks read `role?.id` and `roleId`, while the request validator and
|
||||
* both ORMs also accept `role` as a bare id STRING. That form was invisible to every check.
|
||||
*/
|
||||
describe('role payload extraction (GHSA-x4mv-fhwj-g3rp)', () => {
|
||||
const SA = '55555555-5555-4555-8555-555555555555';
|
||||
const EMP = '44444444-4444-4444-8444-444444444444';
|
||||
|
||||
/** The exact pre-fix extraction used by updateProfile / UserCreateHandler / assertCanAssignRoles. */
|
||||
const preFixExtract = (payload: any) => [payload.role?.id, payload.roleId].filter((id) => !!id);
|
||||
|
||||
describe('resolveRoleReference', () => {
|
||||
it.each([
|
||||
[SA, SA],
|
||||
[{ id: SA }, SA],
|
||||
[{ id: SA, name: RolesEnum.SUPER_ADMIN }, SA]
|
||||
])('reads %p as %p', (value, expected) => {
|
||||
expect(resolveRoleReference(value)).toBe(expected);
|
||||
});
|
||||
|
||||
it.each([[undefined], [null], [''], [' '], [42], [[SA]], [{}], [{ id: '' }], [{ id: 42 }], [{ id: null }]])(
|
||||
'reads %p as no role',
|
||||
(value) => {
|
||||
expect(resolveRoleReference(value)).toBeUndefined();
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
describe('extractRoleIds', () => {
|
||||
it('CONTROL: the pre-fix extraction sees nothing in a bare-string role', () => {
|
||||
expect(preFixExtract({ role: SA })).toEqual([]);
|
||||
expect(preFixExtract({ roleId: EMP, role: SA })).toEqual([EMP]);
|
||||
});
|
||||
|
||||
it('reads a bare-string role', () => {
|
||||
expect(extractRoleIds({ role: SA })).toEqual([SA]);
|
||||
});
|
||||
|
||||
it('reads every form at once, roleId first and de-duplicated', () => {
|
||||
expect(extractRoleIds({ roleId: EMP, role: SA })).toEqual([EMP, SA]);
|
||||
expect(extractRoleIds({ roleId: EMP, role: { id: SA } })).toEqual([EMP, SA]);
|
||||
expect(extractRoleIds({ roleId: SA, role: { id: SA, name: 'x' } })).toEqual([SA]);
|
||||
});
|
||||
|
||||
it('treats absent, undefined and null as "not sent"', () => {
|
||||
expect(extractRoleIds(undefined)).toEqual([]);
|
||||
expect(extractRoleIds({})).toEqual([]);
|
||||
expect(extractRoleIds({ role: undefined, roleId: undefined })).toEqual([]);
|
||||
expect(extractRoleIds({ role: null, roleId: null })).toEqual([]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
[{ role: {} }],
|
||||
[{ role: { id: '' } }],
|
||||
[{ role: { name: RolesEnum.SUPER_ADMIN } }],
|
||||
[{ role: 42 }],
|
||||
[{ role: [SA] }],
|
||||
[{ roleId: '' }],
|
||||
[{ roleId: 42 }],
|
||||
// An empty `role` must not mask a privileged `roleId`, and vice versa: refused outright.
|
||||
[{ role: { id: '' }, roleId: SA }]
|
||||
])('refuses a present role key that references nothing: %p', (payload) => {
|
||||
expect(() => extractRoleIds(payload)).toThrow(BadRequestException);
|
||||
});
|
||||
});
|
||||
|
||||
describe('normalizeRolePayload', () => {
|
||||
it('turns a bare-string role into { id } and pins roleId to it', () => {
|
||||
const payload: any = { role: SA };
|
||||
expect(normalizeRolePayload(payload)).toBe(SA);
|
||||
expect(payload).toEqual({ role: { id: SA }, roleId: SA });
|
||||
});
|
||||
|
||||
it('keeps a role object as is (callers read role.name afterwards) and pins roleId', () => {
|
||||
const role = { id: EMP, name: RolesEnum.EMPLOYEE };
|
||||
const payload: any = { role };
|
||||
expect(normalizeRolePayload(payload)).toBe(EMP);
|
||||
expect(payload.role).toBe(role);
|
||||
expect(payload.roleId).toBe(EMP);
|
||||
});
|
||||
|
||||
it('keeps a lone roleId untouched', () => {
|
||||
const payload: any = { roleId: EMP };
|
||||
expect(normalizeRolePayload(payload)).toBe(EMP);
|
||||
expect(payload).toEqual({ roleId: EMP });
|
||||
});
|
||||
|
||||
it('refuses a role / roleId pair that disagrees', () => {
|
||||
expect(() => normalizeRolePayload({ roleId: EMP, role: SA })).toThrow(BadRequestException);
|
||||
expect(() => normalizeRolePayload({ roleId: EMP, role: { id: SA } })).toThrow(BadRequestException);
|
||||
});
|
||||
|
||||
it('strips a null role / roleId so it cannot clear the stored role', () => {
|
||||
const payload: any = { role: null, roleId: null, firstName: 'Ada' };
|
||||
expect(normalizeRolePayload(payload)).toBeUndefined();
|
||||
expect(payload).toEqual({ firstName: 'Ada' });
|
||||
|
||||
const withId: any = { role: null, roleId: EMP };
|
||||
expect(normalizeRolePayload(withId)).toBe(EMP);
|
||||
expect(withId).toEqual({ roleId: EMP });
|
||||
});
|
||||
|
||||
it('leaves a payload without role keys alone', () => {
|
||||
const payload: any = { firstName: 'Ada' };
|
||||
expect(normalizeRolePayload(payload)).toBeUndefined();
|
||||
expect(payload).toEqual({ firstName: 'Ada' });
|
||||
expect(normalizeRolePayload(undefined)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -29,3 +29,114 @@ export function assertRoleAssignmentAllowed(roleName: string | undefined, canEdi
|
||||
throw new ForbiddenException('Only a super admin may assign the super admin role.');
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The two fields through which a payload can assign a role: the `role` relation and the flat
|
||||
* `roleId` column.
|
||||
*/
|
||||
export interface IRoleAssignmentPayload {
|
||||
role?: unknown;
|
||||
roleId?: unknown;
|
||||
}
|
||||
|
||||
/**
|
||||
* Reads the role id out of ONE role reference, without throwing.
|
||||
*
|
||||
* A role reference is either a bare id string or an object carrying a non-empty string `id`. This is
|
||||
* an allowlist on purpose: the request validator (`IsRoleShouldExist`) and both ORMs also accept a
|
||||
* bare id string for the `role` relation, and TypeORM persists it as the foreign key — so a reader
|
||||
* that only looks at `role?.id` misses that form entirely (GHSA-x4mv-fhwj-g3rp).
|
||||
*
|
||||
* @param value A `role` or `roleId` value from a payload.
|
||||
* @returns The role id, or undefined when the value is not a usable reference.
|
||||
*/
|
||||
export function resolveRoleReference(value: unknown): string | undefined {
|
||||
if (typeof value === 'string') {
|
||||
return value.trim() !== '' ? value : undefined;
|
||||
}
|
||||
if (value !== null && typeof value === 'object' && !Array.isArray(value)) {
|
||||
const id = (value as { id?: unknown }).id;
|
||||
return typeof id === 'string' && id.trim() !== '' ? id : undefined;
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns EVERY role identifier a payload carries, whichever form it takes: `roleId`, `role` as a
|
||||
* bare id string, and `role` as an object with an `id`.
|
||||
*
|
||||
* Role checks must run on this list, never on `role?.id` / `roleId` picked by hand: each form the
|
||||
* check forgets is a form an attacker can use to assign a role nobody checked (GHSA-x4mv-fhwj-g3rp).
|
||||
* A key that is PRESENT but does not reference a role (`role: {}`, `role: { id: '' }`, `role: 42`,
|
||||
* `roleId: ''`, ...) is refused rather than skipped, so it can never leave the list empty and let
|
||||
* the check pass without checking anything. `undefined` and `null` count as "not sent";
|
||||
* {@link normalizeRolePayload} strips a `null` so it cannot clear the stored role either.
|
||||
*
|
||||
* @param payload The request payload (or its `user` part).
|
||||
* @returns The distinct role ids, in payload order (`roleId` first).
|
||||
* @throws BadRequestException When a role key is present but does not reference a role.
|
||||
*/
|
||||
export function extractRoleIds(payload: IRoleAssignmentPayload | null | undefined): string[] {
|
||||
if (!payload || typeof payload !== 'object') {
|
||||
return [];
|
||||
}
|
||||
|
||||
const ids: string[] = [];
|
||||
for (const key of ['roleId', 'role'] as const) {
|
||||
const value = payload[key];
|
||||
if (value === undefined || value === null) {
|
||||
continue;
|
||||
}
|
||||
const roleId = resolveRoleReference(value);
|
||||
if (!roleId) {
|
||||
throw new BadRequestException('The specified role does not reference a valid role.');
|
||||
}
|
||||
if (!ids.includes(roleId)) {
|
||||
ids.push(roleId);
|
||||
}
|
||||
}
|
||||
return ids;
|
||||
}
|
||||
|
||||
/**
|
||||
* Makes the payload persist exactly the role that was checked, and returns that role id.
|
||||
*
|
||||
* - Two DIFFERENT role ids (`roleId` vs `role`) are refused: which one the ORM writes is an ORM
|
||||
* detail, and the check must never validate one while the other is stored.
|
||||
* - A bare id string in `role` becomes `{ id }`, and `roleId` is pinned to the same id, so the
|
||||
* relation and the FK column always agree.
|
||||
* - A `null` role or roleId is removed, so it can neither clear the stored role nor bypass the check.
|
||||
*
|
||||
* Call it once the payload is authorized and before it is saved. A role object is kept as is (only
|
||||
* its `id` is persisted — the relation does not cascade), so callers that read `role.name` from the
|
||||
* saved entity keep working.
|
||||
*
|
||||
* @param payload The payload to normalize (mutated in place).
|
||||
* @returns The single role id the payload assigns, or undefined when it assigns none.
|
||||
* @throws BadRequestException When a role key is malformed or the two keys disagree.
|
||||
*/
|
||||
export function normalizeRolePayload(payload: IRoleAssignmentPayload | null | undefined): string | undefined {
|
||||
const ids = extractRoleIds(payload);
|
||||
if (ids.length > 1) {
|
||||
throw new BadRequestException('The role and roleId fields must reference the same role.');
|
||||
}
|
||||
if (!payload || typeof payload !== 'object') {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const [roleId] = ids;
|
||||
if (!roleId) {
|
||||
// Only `undefined` / `null` can reach here; never let a `null` through to the write.
|
||||
delete payload.role;
|
||||
delete payload.roleId;
|
||||
return undefined;
|
||||
}
|
||||
|
||||
payload.roleId = roleId;
|
||||
if (payload.role === null) {
|
||||
delete payload.role;
|
||||
} else if (typeof payload.role === 'string') {
|
||||
payload.role = { id: roleId };
|
||||
}
|
||||
return roleId;
|
||||
}
|
||||
|
||||
@@ -0,0 +1,136 @@
|
||||
import 'reflect-metadata';
|
||||
import { Column, DataSource, Entity, JoinColumn, ManyToOne, PrimaryColumn, RelationId, Repository } from 'typeorm';
|
||||
import { extractRoleIds, normalizeRolePayload } from './role-assignment.helper';
|
||||
|
||||
/**
|
||||
* GHSA-x4mv-fhwj-g3rp — "the role that is checked is the role that is persisted".
|
||||
*
|
||||
* The fixtures mirror `User`'s real TypeORM mapping: a `role` many-to-one relation whose join column
|
||||
* is `roleId`, plus `roleId` declared again as an explicit `@RelationId` + `@Column` (what
|
||||
* `@MultiORMColumn({ relationId: true })` emits). Saving goes through a real better-sqlite3 database,
|
||||
* so the assertions are about what TypeORM actually writes, not about what we think it writes.
|
||||
*
|
||||
* The CONTROL arm shows the pre-fix gap: the old check read `[role?.id, roleId]`, so a bare-string
|
||||
* `role` (and a `null` one) was checked as nothing while it still rewrote the stored FK; and when a
|
||||
* `role: { id }` sits next to a different `roleId`, TypeORM stores the RELATION — so a check that
|
||||
* picks either one alone is wrong. After `normalizeRolePayload`, every shape either stores exactly
|
||||
* the checked id or is refused.
|
||||
*/
|
||||
@Entity('role_payload_role')
|
||||
class FixtureRole {
|
||||
@PrimaryColumn('varchar')
|
||||
id: string;
|
||||
|
||||
@Column('varchar')
|
||||
name: string;
|
||||
}
|
||||
|
||||
@Entity('role_payload_user')
|
||||
class FixtureUser {
|
||||
@PrimaryColumn('varchar')
|
||||
id: string;
|
||||
|
||||
@Column('varchar')
|
||||
email: string;
|
||||
|
||||
@ManyToOne(() => FixtureRole, { nullable: true, onDelete: 'SET NULL' })
|
||||
@JoinColumn()
|
||||
role?: FixtureRole | string;
|
||||
|
||||
@RelationId((it: FixtureUser) => it.role)
|
||||
@Column({ nullable: true })
|
||||
roleId?: string;
|
||||
}
|
||||
|
||||
describe('role payload persistence (GHSA-x4mv-fhwj-g3rp)', () => {
|
||||
const EMP = 'role-employee';
|
||||
const SA = 'role-super-admin';
|
||||
const USER_ID = 'user-1';
|
||||
|
||||
let dataSource: DataSource;
|
||||
let users: Repository<FixtureUser>;
|
||||
|
||||
beforeAll(async () => {
|
||||
dataSource = new DataSource({
|
||||
type: 'better-sqlite3',
|
||||
database: ':memory:',
|
||||
entities: [FixtureRole, FixtureUser],
|
||||
synchronize: true,
|
||||
logging: false
|
||||
});
|
||||
await dataSource.initialize();
|
||||
await dataSource.getRepository(FixtureRole).save([
|
||||
{ id: EMP, name: 'EMPLOYEE' },
|
||||
{ id: SA, name: 'SUPER_ADMIN' }
|
||||
]);
|
||||
users = dataSource.getRepository(FixtureUser);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await dataSource?.destroy();
|
||||
});
|
||||
|
||||
/** Resets the user to EMPLOYEE, saves `body` over it, and returns the stored roleId. */
|
||||
async function persist(body: Record<string, unknown>): Promise<string | null> {
|
||||
await users.save({ id: USER_ID, email: 'ada@example.com', roleId: EMP });
|
||||
await users.save({ id: USER_ID, ...body } as any);
|
||||
const [row] = await dataSource.query('SELECT "roleId" FROM "role_payload_user" WHERE "id" = ?', [USER_ID]);
|
||||
return row.roleId;
|
||||
}
|
||||
|
||||
/** The exact pre-fix extraction (updateProfile / UserCreateHandler / assertCanAssignRoles). */
|
||||
const preFixCheckedIds = (body: any): string[] => [body.role?.id, body.roleId].filter((id) => !!id);
|
||||
|
||||
describe('CONTROL: pre-fix — what was checked is not what was stored', () => {
|
||||
it('a bare-string role is checked as nothing, yet rewrites the stored FK', async () => {
|
||||
const body = { role: SA };
|
||||
expect(preFixCheckedIds(body)).toEqual([]);
|
||||
// TypeORM does not write the string as the id here (MikroORM does, as a reference) — it
|
||||
// clears the FK. Either way the stored role changed without any role check running.
|
||||
await expect(persist(body)).resolves.not.toBe(EMP);
|
||||
});
|
||||
|
||||
it('a role object next to a different roleId stores the relation', async () => {
|
||||
const body = { roleId: EMP, role: { id: SA } };
|
||||
expect(preFixCheckedIds(body)).toContain(EMP);
|
||||
await expect(persist(body)).resolves.toBe(SA);
|
||||
});
|
||||
|
||||
it('a null role clears the stored role without any check', async () => {
|
||||
const body = { role: null };
|
||||
expect(preFixCheckedIds(body)).toEqual([]);
|
||||
await expect(persist(body)).resolves.toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('fixed — every accepted shape stores exactly the checked id', () => {
|
||||
it.each([
|
||||
[{ role: SA }],
|
||||
[{ role: { id: SA } }],
|
||||
[{ role: { id: SA, name: 'SUPER_ADMIN' } }],
|
||||
[{ roleId: SA }],
|
||||
[{ roleId: SA, role: SA }],
|
||||
[{ roleId: SA, role: { id: SA } }]
|
||||
])('%p', async (raw) => {
|
||||
const body: any = JSON.parse(JSON.stringify(raw));
|
||||
normalizeRolePayload(body);
|
||||
const checked = extractRoleIds(body);
|
||||
|
||||
expect(checked).toEqual([SA]);
|
||||
await expect(persist(body)).resolves.toBe(SA);
|
||||
});
|
||||
|
||||
it('a null role / roleId is stripped and the stored role is kept', async () => {
|
||||
const body: any = { role: null, roleId: null };
|
||||
normalizeRolePayload(body);
|
||||
|
||||
expect(extractRoleIds(body)).toEqual([]);
|
||||
await expect(persist(body)).resolves.toBe(EMP);
|
||||
});
|
||||
|
||||
it('a pair that disagrees is refused before anything is written', () => {
|
||||
expect(() => normalizeRolePayload({ roleId: EMP, role: { id: SA } })).toThrow();
|
||||
expect(() => normalizeRolePayload({ roleId: EMP, role: SA })).toThrow();
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -113,6 +113,13 @@ export class User extends TenantBaseEntity implements IUser {
|
||||
/**
|
||||
* bcrypt password digest. Blanked rather than masked on export: a trailing hint of a digest buys
|
||||
* an offline cracker free characters and buys an operator nothing.
|
||||
*
|
||||
* Not MikroORM `hidden` (unlike `refreshToken` / `code` / `codeExpireAt`): under DB_ORM=mikro-orm
|
||||
* every CrudService read returns `wrap(entity).toJSON()`, which drops hidden properties, and
|
||||
* `AuthService.login` / workspace sign-in read `hash` from exactly those results — hiding it would
|
||||
* break every password login. The same holds for `emailToken` (e-mail confirmation) and
|
||||
* `emailVerifiedAt`. Responses are still scrubbed of all six credential columns by
|
||||
* `TransformInterceptor` (GHSA-hh83-hq74-gh9f).
|
||||
*/
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@@ -128,7 +135,9 @@ export class User extends TenantBaseEntity implements IUser {
|
||||
@IsString()
|
||||
@ExportRedacted({ blank: true })
|
||||
@Exclude({ toPlainOnly: true })
|
||||
@MultiORMColumn({ insert: false, nullable: true })
|
||||
// MikroORM `hidden`: a prototype-less `toJSON()` user must not carry it (GHSA-hh83-hq74-gh9f). Only
|
||||
// read from a repository entity (`getUserIfRefreshTokenMatches`), which `hidden` does not affect.
|
||||
@MultiORMColumn({ insert: false, nullable: true, hidden: true })
|
||||
refreshToken?: string;
|
||||
|
||||
@ApiPropertyOptional({ type: () => String, maxLength: 500 })
|
||||
@@ -174,13 +183,15 @@ export class User extends TenantBaseEntity implements IUser {
|
||||
@IsString()
|
||||
@ExportRedacted()
|
||||
@Exclude({ toPlainOnly: true })
|
||||
@MultiORMColumn({ insert: false, nullable: true })
|
||||
// MikroORM `hidden` (GHSA-hh83-hq74-gh9f): the code is only ever matched in a WHERE clause, never
|
||||
// read back from a serialized user.
|
||||
@MultiORMColumn({ insert: false, nullable: true, hidden: true })
|
||||
code?: string;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Date })
|
||||
@IsOptional()
|
||||
@Exclude({ toPlainOnly: true })
|
||||
@MultiORMColumn({ insert: false, nullable: true })
|
||||
@MultiORMColumn({ insert: false, nullable: true, hidden: true })
|
||||
codeExpireAt?: Date;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Date })
|
||||
|
||||
@@ -0,0 +1,227 @@
|
||||
// Only the shape of UserService is needed here. Importing it for real pulls in the employee/task
|
||||
// services and the User entity, which drag the whole core entity graph in and hit the pre-existing
|
||||
// circular import between `core/entities/internal` and the custom validators. Same technique as
|
||||
// user.service.account-status.spec.ts.
|
||||
jest.mock('./user.entity', () => ({ User: class User {} }));
|
||||
jest.mock('./repository/type-orm-user.repository', () => ({ TypeOrmUserRepository: class TypeOrmUserRepository {} }));
|
||||
jest.mock('./repository/mikro-orm-user.repository', () => ({
|
||||
MikroOrmUserRepository: class MikroOrmUserRepository {}
|
||||
}));
|
||||
jest.mock('../employee/employee.service', () => ({ EmployeeService: class EmployeeService {} }));
|
||||
jest.mock('../tasks/task.service', () => ({ TaskService: class TaskService {} }));
|
||||
jest.mock('../password-hash/password-hash.service', () => ({ PasswordHashService: class PasswordHashService {} }));
|
||||
jest.mock('./../core/crud', () => ({ TenantAwareCrudService: class TenantAwareCrudService {} }));
|
||||
|
||||
import { BadRequestException, ForbiddenException } from '@nestjs/common';
|
||||
import { PermissionsEnum, RolesEnum } from '@gauzy/contracts';
|
||||
import { RequestContext } from '../core/context';
|
||||
import { UserService } from './user.service';
|
||||
import { UserCreateHandler } from './commands/handlers/user.create.handler';
|
||||
import { UserCreateCommand } from './commands/user.create.command';
|
||||
|
||||
/**
|
||||
* GHSA-x4mv-fhwj-g3rp — role privilege escalation through the alternative forms of the role field.
|
||||
*
|
||||
* `PUT /user/:id` (PROFILE_EDIT, held by every EMPLOYEE) and `POST /user` (ORG_USERS_EDIT, held by
|
||||
* ADMIN) checked the role the body assigns by reading `role?.id` and `roleId`. The DTO validator and
|
||||
* the ORMs also accept `role` as a bare id string, so `{ "role": "<SUPER_ADMIN role id>" }` was
|
||||
* checked as "no role change" and still reached `save()`.
|
||||
*/
|
||||
describe('UserService role forms (GHSA-x4mv-fhwj-g3rp)', () => {
|
||||
const TENANT = 'tenant-1';
|
||||
const EMP = '44444444-4444-4444-8444-444444444444';
|
||||
const ADMIN = '66666666-6666-4666-8666-666666666666';
|
||||
const SA = '55555555-5555-4555-8555-555555555555';
|
||||
const ROLE_NAMES: Record<string, RolesEnum> = { [EMP]: RolesEnum.EMPLOYEE, [ADMIN]: RolesEnum.ADMIN, [SA]: RolesEnum.SUPER_ADMIN };
|
||||
|
||||
const SELF = 'user-self';
|
||||
const OTHER = 'user-other';
|
||||
|
||||
interface Caller {
|
||||
userId: string;
|
||||
roleId: string;
|
||||
permissions: PermissionsEnum[];
|
||||
}
|
||||
const employee: Caller = { userId: SELF, roleId: EMP, permissions: [PermissionsEnum.PROFILE_EDIT] };
|
||||
const admin: Caller = {
|
||||
userId: SELF,
|
||||
roleId: ADMIN,
|
||||
permissions: [PermissionsEnum.PROFILE_EDIT, PermissionsEnum.ORG_USERS_EDIT]
|
||||
};
|
||||
|
||||
function build(caller: Caller) {
|
||||
jest.spyOn(RequestContext, 'currentUserId').mockReturnValue(caller.userId);
|
||||
jest.spyOn(RequestContext, 'currentRoleId').mockReturnValue(caller.roleId);
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT);
|
||||
jest.spyOn(RequestContext, 'hasPermission').mockImplementation((permission: PermissionsEnum) =>
|
||||
caller.permissions.includes(permission)
|
||||
);
|
||||
|
||||
// `resolveRoleName` really runs: it reads the role from the (fake) repository, tenant-scoped.
|
||||
const manager = {
|
||||
findOne: jest.fn(async (_entity: string, { where }: any) =>
|
||||
where.tenantId === TENANT && ROLE_NAMES[where.id] ? { id: where.id, name: ROLE_NAMES[where.id] } : null
|
||||
)
|
||||
};
|
||||
const service: UserService = Object.create(UserService.prototype);
|
||||
const save = jest.fn(async (entity: any) => entity);
|
||||
const create = jest.fn(async (entity: any) => entity);
|
||||
Object.assign(service, {
|
||||
ormType: 'typeorm',
|
||||
typeOrmRepository: { manager },
|
||||
// The target of the update currently holds EMPLOYEE (or, for OTHER, the role asked below).
|
||||
findOneByIdString: jest.fn(async (id: string) => ({ id, role: { id: EMP, name: RolesEnum.EMPLOYEE } })),
|
||||
findOneByWhereOptions: jest.fn(async (where: any) => ({ id: where.id })),
|
||||
save,
|
||||
create
|
||||
});
|
||||
return { service, save, create };
|
||||
}
|
||||
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
/** The pre-fix self check, verbatim: `[entity.role?.id, entity.roleId].filter(isNotEmpty)`. */
|
||||
const preFixSelfCheckPasses = (body: any, currentRoleId: string) =>
|
||||
![body.role?.id, body.roleId]
|
||||
.filter((id) => id !== undefined && id !== null && id !== '')
|
||||
.some((id) => String(id) !== String(currentRoleId));
|
||||
|
||||
describe('updateProfile — self', () => {
|
||||
it('CONTROL: the pre-fix self check let a bare-string SUPER_ADMIN role through', () => {
|
||||
expect(preFixSelfCheckPasses({ role: SA }, EMP)).toBe(true);
|
||||
expect(preFixSelfCheckPasses({ roleId: EMP, role: SA }, EMP)).toBe(true);
|
||||
});
|
||||
|
||||
it.each([
|
||||
['a bare-string role', { role: SA }],
|
||||
['a role object', { role: { id: SA } }],
|
||||
['a flat roleId', { roleId: SA }]
|
||||
])('refuses an employee escalating itself through %s', async (_label, body) => {
|
||||
const { service, save } = build(employee);
|
||||
|
||||
await expect(service.updateProfile(SELF, { ...body } as any)).rejects.toBeInstanceOf(ForbiddenException);
|
||||
expect(save).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([
|
||||
['a role key that references nothing', { role: { id: '' }, roleId: SA }],
|
||||
['a number', { role: 42 }],
|
||||
['a role / roleId pair that disagrees', { roleId: EMP, role: SA }]
|
||||
])('answers 400 for %s, before anything is saved', async (_label, body) => {
|
||||
const { service, save } = build(employee);
|
||||
|
||||
await expect(service.updateProfile(SELF, { ...body } as any)).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(save).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('never lets a null role clear the caller role', async () => {
|
||||
const { service, save } = build(employee);
|
||||
|
||||
await service.updateProfile(SELF, { role: null, roleId: null, firstName: 'Ada' } as any);
|
||||
|
||||
const saved = save.mock.calls[0][0];
|
||||
expect('role' in saved).toBe(false);
|
||||
expect('roleId' in saved).toBe(false);
|
||||
expect(saved.firstName).toBe('Ada');
|
||||
});
|
||||
|
||||
it.each([
|
||||
['a bare-string role', { role: EMP }],
|
||||
['the full role object the profile form sends', { role: { id: EMP, name: RolesEnum.EMPLOYEE } }],
|
||||
['no role at all', {}]
|
||||
])('still saves a profile carrying %s (own role unchanged)', async (_label, body) => {
|
||||
const { service, save } = build(employee);
|
||||
|
||||
await service.updateProfile(SELF, { ...body, firstName: 'Ada' } as any);
|
||||
|
||||
const saved = save.mock.calls[0][0];
|
||||
expect(saved.firstName).toBe('Ada');
|
||||
if ('role' in body) {
|
||||
// The value that was checked is the value persisted: a string became `{ id }`, and the FK
|
||||
// column is pinned to the same id.
|
||||
expect(saved.role.id).toBe(EMP);
|
||||
expect(saved.roleId).toBe(EMP);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('updateProfile — someone else', () => {
|
||||
it('CONTROL: the pre-fix other-user check had no candidate to check for a bare-string role', () => {
|
||||
const candidates = [({ role: SA } as any).role?.id, ({ role: SA } as any).roleId].filter(Boolean);
|
||||
expect(candidates).toEqual([]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
['a bare-string role', { role: SA }],
|
||||
['a role object', { role: { id: SA } }],
|
||||
['a flat roleId', { roleId: SA }]
|
||||
])('refuses an ADMIN (no SUPER_ADMIN_EDIT) granting SUPER_ADMIN through %s', async (_label, body) => {
|
||||
const { service, save } = build(admin);
|
||||
|
||||
await expect(service.updateProfile(OTHER, { ...body } as any)).rejects.toBeInstanceOf(ForbiddenException);
|
||||
expect(save).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('still lets an ADMIN assign an ordinary role, and persists exactly that id', async () => {
|
||||
const { service, save } = build(admin);
|
||||
|
||||
await service.updateProfile(OTHER, { role: ADMIN } as any);
|
||||
|
||||
expect(save.mock.calls[0][0]).toEqual(expect.objectContaining({ id: OTHER, role: { id: ADMIN }, roleId: ADMIN }));
|
||||
});
|
||||
});
|
||||
|
||||
describe('assertCanAssignRoles', () => {
|
||||
it('checks a bare-string role', async () => {
|
||||
const { service } = build(admin);
|
||||
await expect(service.assertCanAssignRoles({ role: SA })).rejects.toBeInstanceOf(ForbiddenException);
|
||||
});
|
||||
|
||||
it('refuses a present role key with no usable id instead of checking nothing', async () => {
|
||||
const { service } = build(admin);
|
||||
await expect(service.assertCanAssignRoles({ role: {} })).rejects.toBeInstanceOf(BadRequestException);
|
||||
await expect(service.assertCanAssignRoles({ roleId: '' })).rejects.toBeInstanceOf(BadRequestException);
|
||||
});
|
||||
|
||||
it('lets a payload without a role through (nothing is assigned)', async () => {
|
||||
const { service } = build(admin);
|
||||
await expect(service.assertCanAssignRoles({})).resolves.toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('UserCreateHandler (POST /user)', () => {
|
||||
it.each([
|
||||
['a bare-string role', { role: SA }],
|
||||
['a role object', { role: { id: SA } }],
|
||||
['a flat roleId', { roleId: SA }]
|
||||
])('refuses an ADMIN creating a SUPER_ADMIN through %s', async (_label, body) => {
|
||||
const { service, create } = build(admin);
|
||||
const handler = new UserCreateHandler(service);
|
||||
|
||||
await expect(handler.execute(new UserCreateCommand({ email: 'x@example.com', ...body } as any))).rejects.toBeInstanceOf(
|
||||
ForbiddenException
|
||||
);
|
||||
expect(create).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('refuses a role / roleId pair that disagrees', async () => {
|
||||
const { service, create } = build(admin);
|
||||
const handler = new UserCreateHandler(service);
|
||||
|
||||
await expect(
|
||||
handler.execute(new UserCreateCommand({ email: 'x@example.com', roleId: EMP, role: SA } as any))
|
||||
).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(create).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('keeps the role object the UI sends (callers read role.name) and pins roleId to it', async () => {
|
||||
const { service, create } = build(admin);
|
||||
const handler = new UserCreateHandler(service);
|
||||
const role = { id: EMP, name: RolesEnum.EMPLOYEE };
|
||||
|
||||
await handler.execute(new UserCreateCommand({ email: 'x@example.com', role } as any));
|
||||
|
||||
expect(create).toHaveBeenCalledWith(expect.objectContaining({ role, roleId: EMP }));
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -49,7 +49,12 @@ import { User } from './user.entity';
|
||||
import { validateUserDeletion } from './default-protected-users';
|
||||
import { assertUiPreferencesSize, mergeUiPreferences, sanitizeUiPreferencesPatch } from './ui-preferences.util';
|
||||
import { PasswordHashService } from '../password-hash/password-hash.service';
|
||||
import { assertRoleAssignmentAllowed } from './role-assignment.helper';
|
||||
import {
|
||||
assertRoleAssignmentAllowed,
|
||||
extractRoleIds,
|
||||
IRoleAssignmentPayload,
|
||||
normalizeRolePayload
|
||||
} from './role-assignment.helper';
|
||||
import {
|
||||
emailVerificationClaimWhere,
|
||||
emailVerificationClaimWhereMikroOrm,
|
||||
@@ -488,6 +493,13 @@ export class UserService extends TenantAwareCrudService<User> {
|
||||
}
|
||||
}
|
||||
|
||||
// Read the role the body assigns in EVERY form it can take — `roleId`, `role` as a bare id string,
|
||||
// `role: { id }` — and make the payload persist exactly that id (GHSA-x4mv-fhwj-g3rp). A string
|
||||
// `role` used to be invisible to the checks below while TypeORM still wrote it as the FK, and a
|
||||
// `null` role cleared the caller's own role. A malformed role key or a `role`/`roleId` pair that
|
||||
// disagrees is a 400; this runs before the `try`, which turns every error into a 403.
|
||||
normalizeRolePayload(entity);
|
||||
|
||||
let user: IUser;
|
||||
|
||||
try {
|
||||
@@ -511,15 +523,14 @@ export class UserService extends TenantAwareCrudService<User> {
|
||||
}
|
||||
|
||||
// Restrict users from updating their own role.
|
||||
// Check BOTH the nested `role` object and the flat `roleId` field INDEPENDENTLY, otherwise a
|
||||
// user could escalate their own privileges (e.g. to SUPER_ADMIN). `role?.id ?? roleId` is not
|
||||
// enough: a crafted body could send an empty `role: { id: '' }` (non-nullish) to mask a
|
||||
// privileged `roleId` and slip through. Reject if any provided role identifier differs from the
|
||||
// caller's current role.
|
||||
// Every role identifier the (normalized) payload carries is checked — `roleId`, `role` as an
|
||||
// id string and `role: { id }` alike — otherwise a user could escalate their own privileges
|
||||
// (e.g. to SUPER_ADMIN) through whichever form the check forgot. Reject if any of them differs
|
||||
// from the caller's current role.
|
||||
// Compare as strings: `id` is typed `ID | number`, so a numeric-equivalent value must not
|
||||
// slip past the self-update check on a strict `===`.
|
||||
if (String(currentUserId) === String(id)) {
|
||||
const requestedRoleIds = [entity.role?.id, entity.roleId].filter((roleId) => isNotEmpty(roleId));
|
||||
const requestedRoleIds = extractRoleIds(entity);
|
||||
if (requestedRoleIds.some((roleId) => String(roleId) !== String(currentRoleId))) {
|
||||
throw new ForbiddenException();
|
||||
}
|
||||
@@ -527,7 +538,7 @@ export class UserService extends TenantAwareCrudService<User> {
|
||||
// Updating SOMEONE ELSE: granting SUPER_ADMIN is reserved to callers who may edit super
|
||||
// admins (the same boundary the register handler and invite creation enforce). The role is
|
||||
// resolved from the database — never from a client-supplied role name.
|
||||
await this.assertCanAssignRoles([entity.role?.id, entity.roleId]);
|
||||
await this.assertCanAssignRoles(entity);
|
||||
}
|
||||
|
||||
// Update password hash if provided
|
||||
@@ -1005,15 +1016,18 @@ export class UserService extends TenantAwareCrudService<User> {
|
||||
/**
|
||||
* Refuses a payload that assigns a role the caller may not grant.
|
||||
*
|
||||
* @param roleIds Every role identifier in the payload — both the flat `roleId` and `role.id`.
|
||||
* @throws BadRequestException When an id does not resolve inside the caller's tenant.
|
||||
* @param payload The payload that assigns the role. Its identifiers are extracted here, in every
|
||||
* form (`roleId`, `role` as an id string, `role: { id }`), so a caller cannot forget one.
|
||||
* @throws BadRequestException When a role key does not reference a role, or an id does not resolve
|
||||
* inside the caller's tenant.
|
||||
* @throws ForbiddenException When SUPER_ADMIN is requested without `SUPER_ADMIN_EDIT`.
|
||||
*/
|
||||
public async assertCanAssignRoles(roleIds: Array<ID | undefined>): Promise<void> {
|
||||
public async assertCanAssignRoles(payload: IRoleAssignmentPayload): Promise<void> {
|
||||
// EVERY candidate is checked, not just the first: the entity carries both a `role` relation and a
|
||||
// flat `roleId` column, and the RELATION wins when the row is persisted — so a body sending a
|
||||
// harmless `roleId` next to a privileged `role: { id }` must not validate the harmless one.
|
||||
const candidates = roleIds.filter((roleId) => isNotEmpty(roleId)) as ID[];
|
||||
// flat `roleId` column, so a body sending a harmless `roleId` next to a privileged `role` must not
|
||||
// validate the harmless one. A role key that is present but references nothing throws inside
|
||||
// `extractRoleIds` rather than leaving an empty list that checks nothing (GHSA-x4mv-fhwj-g3rp).
|
||||
const candidates = extractRoleIds(payload);
|
||||
const canEditSuperAdmin = RequestContext.hasPermission(PermissionsEnum.SUPER_ADMIN_EDIT);
|
||||
for (const roleId of candidates) {
|
||||
assertRoleAssignmentAllowed(await this.resolveRoleName(roleId), canEditSuperAdmin);
|
||||
|
||||
Reference in New Issue
Block a user