mirror of
https://github.com/ever-co/ever-gauzy.git
synced 2026-10-02 01:54:50 +08:00
fix(security): Nested-relation ownership checks (GHSA-jh6m, GHSA-gwpq, GHSA-6qvm) (#10238)
* fix(security): check ownership of nested relations, not just the root row GHSA-jh6m-9fxr-rx3c (high): TenantAwareCrudService verified the tenant of the row being written but not of the rows named inside it, so PUT /invoices/:id carrying invoiceItems with another tenant's item id re-parented or overwrote that row, and PUT /candidate/:id could cascade into another user's credentials. A new assertGraphNotForeign() walks the payload's relation graph (depth-bounded, one batched SELECT per relation), refuses foreign-tenant rows through any relation, refuses re-parenting a global NULL-tenant row, stamps the caller's tenant on cascaded inserts, and strips credential fields from a nested existing User. Candidate updates no longer cascade into User at all. Also closes the named siblings: request-approval's raw employee and team lookups are tenant-scoped (GHSA-gwpq-mmw7-vx85); UpdateTimeSlotHandler, the bulk activity save and the time-log routes are tenant-scoped, whitelisted and no longer accept a foreign employeeId, and the organization policy guard checks the target's tenant for SUPER_ADMIN too (GHSA-6qvm-3wg4-26w4). 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(security): link nested users instead of writing them, fail closed on the report queries Review follow-up on the nested-relation ownership work. Six findings held up when checked against the code; each protective line has a spec arm that fails when the line is reverted. - nested-graph: an EXISTING `User` reached through a cascade is now reduced to `{ id }`. Removing only the credential columns still let `user: { id, firstName }` rename any account of the caller's own tenant, which a tenant check cannot catch — the victim is a legitimate member of that tenant. A NEW user inserted through a cascade (candidate sign-up) is untouched. CandidateCreateHandler re-attaches the user it just created to its response, so POST /candidate still answers with the user the UI prints. - nested-graph: a payload that would still persist rows BELOW the walk limit is now refused (400) instead of being waved through. The row sitting at the limit was checked, but its own relations were not walked, and `save()` would still cascade or re-parent them with no ownership check at all. - user create: `UserCreateCommand` drops a body-supplied `id`. `CrudService.create()` upserts when the payload carries a primary key, so a body id turned "create a user" into "overwrite that user" — and POST /candidate / POST /employee hash the request's `password` into that payload, so `user: { id: <an admin of my tenant> }` reset that account's password and demoted its role. Same class as the candidate update cascade this branch already closed, through the create route instead. - time logs / time slots: `getTimeLogs()`, the report queries built on `getFilterTimeLogQuery()` / `buildMikroOrmTimeLogWhere()` and `getTimeSlots()` build their own queries, so the never-matching employee condition added to the CRUD reads never applied to them. Their employee predicate is only added when `employeeIds` is non-empty, and `employeeIds` is only narrowed for a caller who HAS an employee record — so a caller with neither CHANGE_SELECTED_EMPLOYEE nor an employee read the whole organization, or the employees they named in the body. They now match nothing, exactly as `findConditionsWithoutOwnEmployee` already does. - activities: `scopeActivitiesForWrite()` now drops a `projectId` / `taskId` (and folds a `project` / `task` object into its id) that does not name a row of the caller's tenant. These are plain foreign keys, so the nested-graph check never sees them, and the activity is written through the raw repository: a foreign projectId was stored verbatim and handed back with the victim tenant's project joined onto it. The bulk handler now applies its request-level `projectId` before that check rather than over it. - OrganizationPermissionGuard: the SUPER_ADMIN exemption resolves the tenant before the no-target shortcut. Not exploitable today (TenantBaseGuard / TenantPermissionGuard already refuse a tenant-less request on both controllers that apply this guard), but the guard no longer depends on a sibling guard for it. Also, for the two new SonarCloud complexity findings: the reference rules of `assertGraphNotForeign` move into `resolveReference()` / `isGlobalRowWritable()`, and `UpdateTimeSlotHandler.execute` into `resolveEmployeeScope()` / `collectChanges()` / `saveActivities()`. Behaviour is unchanged; the existing specs cover both. Two review findings were NOT applied, deliberately: - scoping the request-approval employee / team lookups by `RequestContext.currentOrganizationId()`: cross-organization references inside one tenant are a different boundary (the advisories are about cross-tenant writes), and the header-derived organization is absent on legitimate calls, which would silently drop approvers. - nothing was relaxed to silence a finding; no test was deleted or loosened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(time-tracking): drop the redundant optional chaining, keep a null employeeId meaning "no scope" Follow-up on the review fixes, for DeepScan's three new findings on the previous commit (all in the code it added). - `user` is dereferenced unguarded a few lines above each of the new fail-closed guards, so `user?.employeeId` in them was a null check that can never fire; it now reads `user.employeeId` like the surrounding code (time-log report filters, both ORM branches, and `getTimeSlots`). - `UpdateTimeSlotHandler.resolveEmployeeScope()` returned `ID | undefined | null`, where `null` meant "refuse" — but a caller holding CHANGE_SELECTED_EMPLOYEE may legitimately send `employeeId: null` (no employee filter), and that would have been read as a refusal. It now returns a scope object or `null`, so the two cases cannot be confused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(time-tracking): take the organization from the verified employee, not from the body Two more review findings, both real: the manual-time routes and the bulk activity save validated the employee against the caller's tenant but then persisted whatever `organizationId` the body carried. - `addManualTime` / `updateManualTime` already read the `futureDateAllowed` policy from `employee.organization`, so a body organizationId of the same tenant had the write judged by one organization's rules and then filed — time log, time slots and timesheet — under another's. On update it also decided which rows counted as conflicting, i.e. which sibling organization's time slots this call was allowed to delete. Both now derive the organization from the employee, with the body value left as a fallback for an employee that has none. - `BulkActivitiesSaveHandler` used the employee's organization only when the body omitted one. An Employee row belongs to exactly one organization, so the body value could only ever file that employee's activities under an organization they are not a member of; the employee's own organization now wins. Each is covered by a spec arm that fails when the line is reverted (verified by revert-and-run, files restored and hashed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(core): split the per-relation walk and the activity normalisation out SonarCloud's two remaining cognitive-complexity findings on this branch, both in code it added. Pure extraction, no behaviour change: - `assertGraphNotForeign()` keeps the level-by-level loop; the per-relation lookup and the per-reference rules move into `walkRelation()` (31 -> well under the threshold). - `scopeActivitiesForWrite()` hands the relation-object stripping and the `project` / `task` fold to `normalizeReferences()`. The nested-graph and activity specs (112 tests, real better-sqlite3 databases with their CONTROL arms) cover both and stay green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(user): align the create handler with the merged role normalization Merging develop brought in GHSA-x4mv's role handling: assertCanAssignRoles now takes the payload and extracts every role form itself, and a role/roleId pair naming two different roles is a 400. This spec still asserted the older two-argument contract with a contradicting pair, so the two changes were green apart and red together. It now covers both: the agreeing pair is validated, and the contradicting pair is refused before anything is persisted. Also replaces a stray console.log with the Nest logger. 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
3a62724c23
commit
2bc0b39cd0
@@ -52,12 +52,18 @@ export class CandidateCreateHandler implements ICommandHandler<CandidateCreateCo
|
||||
})
|
||||
);
|
||||
|
||||
// 2. Create candidate for specific user
|
||||
// 2. Create candidate for specific user.
|
||||
//
|
||||
// `Candidate.user` cascades, and the nested-graph check reduces an EXISTING user to `{ id }`
|
||||
// so a cascade can never write into an account (GHSA-jh6m-9fxr-rx3c). The user was just
|
||||
// created here, so the link is all this needs — but the response must still carry the user
|
||||
// the caller asked for (the UI reads `candidate.user.firstName`), hence the re-attach below.
|
||||
const candidate = await this._candidateService.create({
|
||||
...input,
|
||||
status: CandidateStatusEnum.APPLIED,
|
||||
user
|
||||
});
|
||||
candidate.user = user;
|
||||
|
||||
// 3. Assign organization to the candidate user
|
||||
if (candidate.organizationId) {
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
// Must stay first: loads the entity graph before the handler pulls an entity (see activity.controller.spec.ts).
|
||||
import '../../../core/entities/internal';
|
||||
|
||||
import { CandidateUpdateCommand } from '../candidate.update.command';
|
||||
import { CandidateUpdateHandler } from './candidate.update.handler';
|
||||
|
||||
/**
|
||||
* GHSA-jh6m-9fxr-rx3c — PUT /candidate/:id.
|
||||
*
|
||||
* `Candidate.user` is a cascading owner relation and the route does not whitelist its body, so a
|
||||
* nested `user: { id, hash }` reached `create({ ...input, id })` and rewrote that user's password — any
|
||||
* user's, a same-tenant super admin included. The candidate screens edit the user through PUT /user/:id,
|
||||
* never through this route.
|
||||
*/
|
||||
describe('CandidateUpdateHandler (GHSA-jh6m-9fxr-rx3c)', () => {
|
||||
const exploit = {
|
||||
id: 'candidate-1',
|
||||
appliedDate: new Date('2026-01-01'),
|
||||
user: { id: 'super-admin', hash: '$2b$10$attacker' },
|
||||
userId: 'super-admin'
|
||||
} as any;
|
||||
|
||||
it('CONTROL: the pre-fix payload hands the nested user to create()', () => {
|
||||
expect({ ...exploit, id: exploit.id }).toMatchObject({ user: { hash: '$2b$10$attacker' }, userId: 'super-admin' });
|
||||
});
|
||||
|
||||
it('never passes the user or userId on to create()', async () => {
|
||||
const create = jest.fn(async (entity: any) => entity);
|
||||
const handler = new CandidateUpdateHandler({ create } as any);
|
||||
|
||||
await handler.execute(new CandidateUpdateCommand(exploit));
|
||||
|
||||
const [persisted] = create.mock.calls[0];
|
||||
expect(persisted).toEqual({ id: 'candidate-1', appliedDate: exploit.appliedDate });
|
||||
// The command input is left as it was.
|
||||
expect(exploit.user).toBeDefined();
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,6 @@
|
||||
import { CommandHandler, ICommandHandler } from '@nestjs/cqrs';
|
||||
import { BadRequestException } from '@nestjs/common';
|
||||
import { ICandidate } from '@gauzy/contracts';
|
||||
import { ICandidate, ICandidateUpdateInput } from '@gauzy/contracts';
|
||||
import { CandidateService } from '../../candidate.service';
|
||||
import { CandidateUpdateCommand } from '../candidate.update.command';
|
||||
|
||||
@@ -15,10 +15,19 @@ export class CandidateUpdateHandler
|
||||
public async execute(command: CandidateUpdateCommand): Promise<ICandidate> {
|
||||
const { input } = command;
|
||||
const { id } = input;
|
||||
|
||||
// A candidate edit never writes the linked User. `Candidate.user` cascades, so a nested
|
||||
// `user: { id, hash, email }` overwrote ANY user's credentials (another tenant's, or a
|
||||
// super admin of the same tenant), and `userId` re-linked the candidate to another account
|
||||
// (GHSA-jh6m-9fxr-rx3c). The UI edits the candidate's user through PUT /user/:id instead.
|
||||
const candidate: ICandidateUpdateInput = { ...input };
|
||||
delete (candidate as any).user;
|
||||
delete (candidate as any).userId;
|
||||
|
||||
try {
|
||||
//We are using create here because create calls the method save()
|
||||
//We need save() to save ManyToMany relations
|
||||
return await this.candidateService.create({ ...input, id });
|
||||
return await this.candidateService.create({ ...candidate, id });
|
||||
} catch (error) {
|
||||
throw new BadRequestException(error);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,679 @@
|
||||
import '../entities/internal';
|
||||
|
||||
import { BadRequestException, ForbiddenException } from '@nestjs/common';
|
||||
import { DataSource, EntitySchema, In, Repository } from 'typeorm';
|
||||
import { PermissionsEnum } from '@gauzy/contracts';
|
||||
import { RequestContext } from '../context';
|
||||
import { MultiORMEnum } from '../utils';
|
||||
import { CrudService } from './crud.service';
|
||||
import { TenantAwareCrudService } from './tenant-aware-crud.service';
|
||||
import { assertGraphNotForeign, GRAPH_CHECK_MAX_DEPTH } from './nested-graph-ownership.helper';
|
||||
import { TimeLogService } from '../../time-tracking/time-log/time-log.service';
|
||||
import { TimeSlotService } from '../../time-tracking/time-slot/time-slot.service';
|
||||
|
||||
/**
|
||||
* GHSA-jh6m-9fxr-rx3c — nested-relation takeover through create({ ...body, id }) / save().
|
||||
*
|
||||
* `assertNotForeignRow` guards the ROOT id only. TypeORM resolves every nested object by primary key
|
||||
* alone, so a payload on the caller's OWN row could overwrite, re-parent or claim another tenant's
|
||||
* rows through its relations. Every "refused" case below has a CONTROL arm that runs the very same
|
||||
* payload through a plain `repository.save()` — the pre-fix persistence path — against a real
|
||||
* better-sqlite3 database and shows the exploit landing. A green run therefore proves the check is
|
||||
* what makes the difference, not a fixture that could never be exploited.
|
||||
*/
|
||||
|
||||
const TENANT_A = '11111111-1111-4111-8111-111111111111';
|
||||
const TENANT_B = '22222222-2222-4222-8222-222222222222';
|
||||
|
||||
const tenantColumns = {
|
||||
id: { primary: true, type: 'varchar', generated: 'uuid' },
|
||||
tenantId: { type: 'varchar', nullable: true }
|
||||
} as const;
|
||||
|
||||
const ParentSchema = new EntitySchema<any>({
|
||||
name: 'Parent',
|
||||
tableName: 'parent',
|
||||
columns: { ...tenantColumns, name: { type: 'varchar', nullable: true } },
|
||||
relations: {
|
||||
children: { type: 'one-to-many', target: 'Child', inverseSide: 'parent', cascade: true },
|
||||
plainChildren: { type: 'one-to-many', target: 'PlainChild', inverseSide: 'parent' },
|
||||
tags: { type: 'many-to-many', target: 'Tag', joinTable: { name: 'parent_tag' } },
|
||||
user: { type: 'one-to-one', target: 'User', joinColumn: { name: 'userId' }, cascade: true },
|
||||
kind: { type: 'many-to-one', target: 'Kind', joinColumn: { name: 'kindId' }, cascade: true }
|
||||
}
|
||||
});
|
||||
|
||||
const ChildSchema = new EntitySchema<any>({
|
||||
name: 'Child',
|
||||
tableName: 'child',
|
||||
columns: {
|
||||
...tenantColumns,
|
||||
price: { type: 'int', default: 0 },
|
||||
parentId: { type: 'varchar', nullable: true },
|
||||
deletedAt: { type: 'datetime', nullable: true, deleteDate: true }
|
||||
},
|
||||
relations: {
|
||||
parent: { type: 'many-to-one', target: 'Parent', inverseSide: 'children', joinColumn: { name: 'parentId' } },
|
||||
grandChildren: { type: 'one-to-many', target: 'GrandChild', inverseSide: 'child', cascade: true }
|
||||
}
|
||||
});
|
||||
|
||||
const GrandChildSchema = new EntitySchema<any>({
|
||||
name: 'GrandChild',
|
||||
tableName: 'grand_child',
|
||||
columns: {
|
||||
...tenantColumns,
|
||||
note: { type: 'varchar', nullable: true },
|
||||
childId: { type: 'varchar', nullable: true }
|
||||
},
|
||||
relations: {
|
||||
child: { type: 'many-to-one', target: 'Child', inverseSide: 'grandChildren', joinColumn: { name: 'childId' } }
|
||||
}
|
||||
});
|
||||
|
||||
const PlainChildSchema = new EntitySchema<any>({
|
||||
name: 'PlainChild',
|
||||
tableName: 'plain_child',
|
||||
columns: { ...tenantColumns, parentId: { type: 'varchar', nullable: true } },
|
||||
relations: {
|
||||
parent: { type: 'many-to-one', target: 'Parent', inverseSide: 'plainChildren', joinColumn: { name: 'parentId' } }
|
||||
}
|
||||
});
|
||||
|
||||
const TagSchema = new EntitySchema<any>({
|
||||
name: 'Tag',
|
||||
tableName: 'tag',
|
||||
columns: { ...tenantColumns, name: { type: 'varchar', nullable: true } }
|
||||
});
|
||||
|
||||
const KindSchema = new EntitySchema<any>({
|
||||
name: 'Kind',
|
||||
tableName: 'kind',
|
||||
columns: { ...tenantColumns, name: { type: 'varchar', nullable: true } }
|
||||
});
|
||||
|
||||
/** Self-referencing cascade, to nest a payload deeper than the walk follows. */
|
||||
const NodeSchema = new EntitySchema<any>({
|
||||
name: 'Node',
|
||||
tableName: 'node',
|
||||
columns: {
|
||||
...tenantColumns,
|
||||
name: { type: 'varchar', nullable: true },
|
||||
parentId: { type: 'varchar', nullable: true }
|
||||
},
|
||||
relations: {
|
||||
children: { type: 'one-to-many', target: 'Node', inverseSide: 'parent', cascade: true },
|
||||
parent: { type: 'many-to-one', target: 'Node', inverseSide: 'children', joinColumn: { name: 'parentId' } }
|
||||
}
|
||||
});
|
||||
|
||||
const UserSchema = new EntitySchema<any>({
|
||||
name: 'User',
|
||||
tableName: 'user',
|
||||
columns: {
|
||||
...tenantColumns,
|
||||
firstName: { type: 'varchar', nullable: true },
|
||||
email: { type: 'varchar', nullable: true },
|
||||
hash: { type: 'varchar', nullable: true }
|
||||
}
|
||||
});
|
||||
|
||||
describe('assertGraphNotForeign (GHSA-jh6m-9fxr-rx3c)', () => {
|
||||
let dataSource: DataSource;
|
||||
let parents: Repository<any>;
|
||||
let children: Repository<any>;
|
||||
let grandChildren: Repository<any>;
|
||||
let plainChildren: Repository<any>;
|
||||
let tags: Repository<any>;
|
||||
let kinds: Repository<any>;
|
||||
let users: Repository<any>;
|
||||
let nodes: Repository<any>;
|
||||
|
||||
/** The check, exactly as TenantAwareCrudService runs it, for tenant A. */
|
||||
const check = (payload: any, tenantId: string | null = TENANT_A) =>
|
||||
assertGraphNotForeign(dataSource.manager, parents.metadata, [payload], tenantId);
|
||||
|
||||
beforeEach(async () => {
|
||||
dataSource = new DataSource({
|
||||
type: 'better-sqlite3',
|
||||
database: ':memory:',
|
||||
entities: [
|
||||
ParentSchema,
|
||||
ChildSchema,
|
||||
GrandChildSchema,
|
||||
PlainChildSchema,
|
||||
TagSchema,
|
||||
KindSchema,
|
||||
UserSchema,
|
||||
NodeSchema
|
||||
],
|
||||
synchronize: true,
|
||||
logging: false
|
||||
});
|
||||
await dataSource.initialize();
|
||||
parents = dataSource.getRepository('Parent');
|
||||
children = dataSource.getRepository('Child');
|
||||
grandChildren = dataSource.getRepository('GrandChild');
|
||||
plainChildren = dataSource.getRepository('PlainChild');
|
||||
tags = dataSource.getRepository('Tag');
|
||||
kinds = dataSource.getRepository('Kind');
|
||||
users = dataSource.getRepository('User');
|
||||
nodes = dataSource.getRepository('Node');
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await dataSource.destroy();
|
||||
});
|
||||
|
||||
const seed = async () => {
|
||||
const ownParent = await parents.save({ tenantId: TENANT_A, name: 'own' });
|
||||
const foreignParent = await parents.save({ tenantId: TENANT_B, name: 'foreign' });
|
||||
const ownChild = await children.save({ tenantId: TENANT_A, price: 10, parentId: ownParent.id });
|
||||
const foreignChild = await children.save({ tenantId: TENANT_B, price: 20, parentId: foreignParent.id });
|
||||
return { ownParent, foreignParent, ownChild, foreignChild };
|
||||
};
|
||||
|
||||
describe('cascaded one-to-many (invoice.invoiceItems)', () => {
|
||||
it('CONTROL: a plain save overwrites and re-parents the foreign child', async () => {
|
||||
const { ownParent, foreignChild } = await seed();
|
||||
|
||||
await parents.save({ id: ownParent.id, children: [{ id: foreignChild.id, price: 999 }] });
|
||||
|
||||
const after = await children.findOneBy({ id: foreignChild.id });
|
||||
expect(after.price).toBe(999);
|
||||
expect(after.parentId).toBe(ownParent.id);
|
||||
});
|
||||
|
||||
it('refuses the foreign child and leaves it untouched', async () => {
|
||||
const { ownParent, foreignParent, foreignChild } = await seed();
|
||||
|
||||
await expect(check({ id: ownParent.id, children: [{ id: foreignChild.id, price: 999 }] })).rejects.toThrow(
|
||||
ForbiddenException
|
||||
);
|
||||
|
||||
const after = await children.findOneBy({ id: foreignChild.id });
|
||||
expect(after).toMatchObject({ price: 20, parentId: foreignParent.id, tenantId: TENANT_B });
|
||||
});
|
||||
|
||||
it('refuses a soft-deleted foreign child too (save() reaches those as well)', async () => {
|
||||
const { ownParent, foreignChild } = await seed();
|
||||
await children.softDelete({ id: foreignChild.id });
|
||||
|
||||
await expect(check({ id: ownParent.id, children: [{ id: foreignChild.id, price: 1 }] })).rejects.toThrow(
|
||||
ForbiddenException
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses a bare foreign id (a primitive is treated as a key)', async () => {
|
||||
const { ownParent, foreignChild } = await seed();
|
||||
|
||||
await expect(check({ id: ownParent.id, children: [foreignChild.id] })).rejects.toThrow(ForbiddenException);
|
||||
});
|
||||
|
||||
it('allows editing the own children and adding new ones', async () => {
|
||||
const { ownParent, ownChild } = await seed();
|
||||
const payload = { id: ownParent.id, children: [{ id: ownChild.id, price: 11 }, { price: 12 }] };
|
||||
|
||||
await expect(check(payload)).resolves.toBeUndefined();
|
||||
await parents.save(payload);
|
||||
|
||||
const rows = await children.find({ where: { parentId: ownParent.id }, order: { price: 'ASC' } });
|
||||
expect(rows.map((row) => [row.price, row.tenantId])).toEqual([
|
||||
[11, TENANT_A],
|
||||
[12, TENANT_A]
|
||||
]);
|
||||
});
|
||||
|
||||
it("CONTROL: a plain save inserts a new child into the tenant named in the body", async () => {
|
||||
const { ownParent } = await seed();
|
||||
|
||||
await parents.save({ id: ownParent.id, children: [{ price: 5, tenantId: TENANT_B }] });
|
||||
|
||||
expect((await children.findOneBy({ price: 5 })).tenantId).toBe(TENANT_B);
|
||||
});
|
||||
|
||||
it("stamps the caller's tenant onto a new nested child, whatever the body says", async () => {
|
||||
const { ownParent } = await seed();
|
||||
const payload = { id: ownParent.id, children: [{ price: 5, tenantId: TENANT_B }] };
|
||||
|
||||
await check(payload);
|
||||
await parents.save(payload);
|
||||
|
||||
expect((await children.findOneBy({ price: 5 })).tenantId).toBe(TENANT_A);
|
||||
});
|
||||
|
||||
it('follows cascades below the first level (child.grandChildren)', async () => {
|
||||
const { ownParent, ownChild } = await seed();
|
||||
const foreignGrandChild = await grandChildren.save({ tenantId: TENANT_B, note: 'theirs' });
|
||||
|
||||
await expect(
|
||||
check({
|
||||
id: ownParent.id,
|
||||
children: [{ id: ownChild.id, grandChildren: [{ id: foreignGrandChild.id, note: 'mine' }] }]
|
||||
})
|
||||
).rejects.toThrow(ForbiddenException);
|
||||
});
|
||||
});
|
||||
|
||||
describe('one-to-many WITHOUT cascade', () => {
|
||||
it('CONTROL: a plain save still re-parents the foreign row', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const foreignPlain = await plainChildren.save({ tenantId: TENANT_B });
|
||||
|
||||
await parents.save({ id: ownParent.id, plainChildren: [{ id: foreignPlain.id }] });
|
||||
|
||||
expect((await plainChildren.findOneBy({ id: foreignPlain.id })).parentId).toBe(ownParent.id);
|
||||
});
|
||||
|
||||
it('refuses the foreign row', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const foreignPlain = await plainChildren.save({ tenantId: TENANT_B });
|
||||
|
||||
await expect(check({ id: ownParent.id, plainChildren: [{ id: foreignPlain.id }] })).rejects.toThrow(
|
||||
ForbiddenException
|
||||
);
|
||||
});
|
||||
|
||||
it('allows a NULL-tenant row only when it already belongs to this parent', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const linkedLegacy = await plainChildren.save({ tenantId: null, parentId: ownParent.id });
|
||||
const unlinkedLegacy = await plainChildren.save({ tenantId: null, parentId: null });
|
||||
|
||||
await expect(check({ id: ownParent.id, plainChildren: [{ id: linkedLegacy.id }] })).resolves.toBeUndefined();
|
||||
await expect(check({ id: ownParent.id, plainChildren: [{ id: unlinkedLegacy.id }] })).rejects.toThrow(
|
||||
ForbiddenException
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('many-to-many links (tags)', () => {
|
||||
it('keeps global (NULL-tenant) and own tags linkable', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const globalTag = await tags.save({ tenantId: null, name: 'global' });
|
||||
const ownTag = await tags.save({ tenantId: TENANT_A, name: 'own' });
|
||||
|
||||
await expect(
|
||||
check({ id: ownParent.id, tags: [{ id: globalTag.id, name: 'global' }, { id: ownTag.id }] })
|
||||
).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it("refuses another tenant's tag", async () => {
|
||||
const { ownParent } = await seed();
|
||||
const foreignTag = await tags.save({ tenantId: TENANT_B, name: 'foreign' });
|
||||
|
||||
await expect(check({ id: ownParent.id, tags: [{ id: foreignTag.id }] })).rejects.toThrow(ForbiddenException);
|
||||
});
|
||||
});
|
||||
|
||||
describe('cascaded owner relation to a global row (parent.kind)', () => {
|
||||
it('CONTROL: a plain save writes into the global row', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const globalKind = await kinds.save({ tenantId: null, name: 'system' });
|
||||
|
||||
await parents.save({ id: ownParent.id, kind: { id: globalKind.id, name: 'hijacked' } });
|
||||
|
||||
expect((await kinds.findOneBy({ id: globalKind.id })).name).toBe('hijacked');
|
||||
});
|
||||
|
||||
it('keeps the link but never writes the global row', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const globalKind = await kinds.save({ tenantId: null, name: 'system' });
|
||||
const payload: any = { id: ownParent.id, kind: { id: globalKind.id, name: 'hijacked' } };
|
||||
|
||||
await check(payload);
|
||||
expect(payload.kind).toEqual({ id: globalKind.id });
|
||||
await parents.save(payload);
|
||||
|
||||
expect(await kinds.findOneBy({ id: globalKind.id })).toMatchObject({ name: 'system', tenantId: null });
|
||||
expect(await parents.findOne({ where: { id: ownParent.id }, relations: { kind: true } })).toMatchObject({
|
||||
kind: { id: globalKind.id }
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('cascaded user (candidate.user)', () => {
|
||||
it('CONTROL: a plain save rewrites the password hash of another user of the same tenant', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const admin = await users.save({ tenantId: TENANT_A, email: 'admin@a', hash: 'admin-hash' });
|
||||
|
||||
await parents.save({ id: ownParent.id, user: { id: admin.id, hash: 'attacker-hash', email: 'x@evil' } });
|
||||
|
||||
expect(await users.findOneBy({ id: admin.id })).toMatchObject({ hash: 'attacker-hash', email: 'x@evil' });
|
||||
});
|
||||
|
||||
it('CONTROL: a plain save also renames another user of the same tenant', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const admin = await users.save({ tenantId: TENANT_A, email: 'admin@a', firstName: 'Admin' });
|
||||
|
||||
await parents.save({ id: ownParent.id, user: { id: admin.id, firstName: 'Renamed' } });
|
||||
|
||||
expect(await users.findOneBy({ id: admin.id })).toMatchObject({ firstName: 'Renamed' });
|
||||
});
|
||||
|
||||
it('links an existing user but never writes into it, not even a profile field', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const admin = await users.save({
|
||||
tenantId: TENANT_A,
|
||||
email: 'admin@a',
|
||||
hash: 'admin-hash',
|
||||
firstName: 'Admin'
|
||||
});
|
||||
const original = { id: admin.id, hash: 'attacker-hash', email: 'x@evil', firstName: 'Renamed' };
|
||||
const payload: any = { id: ownParent.id, user: original };
|
||||
|
||||
await check(payload);
|
||||
await parents.save(payload);
|
||||
|
||||
expect(await users.findOneBy({ id: admin.id })).toMatchObject({
|
||||
hash: 'admin-hash',
|
||||
email: 'admin@a',
|
||||
firstName: 'Admin'
|
||||
});
|
||||
// The link itself still lands, and the caller's own object is not mutated.
|
||||
expect((await parents.findOne({ where: { id: ownParent.id }, relations: { user: true } })).user).toMatchObject(
|
||||
{ id: admin.id }
|
||||
);
|
||||
expect(original.hash).toBe('attacker-hash');
|
||||
});
|
||||
|
||||
it("refuses another tenant's user", async () => {
|
||||
const { ownParent } = await seed();
|
||||
const foreignUser = await users.save({ tenantId: TENANT_B, email: 'b@b', hash: 'b' });
|
||||
|
||||
await expect(check({ id: ownParent.id, user: { id: foreignUser.id, hash: 'x' } })).rejects.toThrow(
|
||||
ForbiddenException
|
||||
);
|
||||
});
|
||||
|
||||
it('lets a NEW user keep its credentials (candidate sign-up)', async () => {
|
||||
const payload: any = { tenantId: TENANT_A, user: { email: 'new@a', hash: 'new-hash' } };
|
||||
|
||||
await check(payload);
|
||||
const saved = await parents.save(payload);
|
||||
|
||||
expect(await users.findOneBy({ id: saved.user.id })).toMatchObject({
|
||||
hash: 'new-hash',
|
||||
email: 'new@a',
|
||||
tenantId: TENANT_A
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('payloads deeper than the walk follows', () => {
|
||||
/** A chain of `levels` nested cascading children below the root, each a distinct object. */
|
||||
const chain = (levels: number, deepest: Record<string, any> = { name: 'leaf' }): any => {
|
||||
let payload: any = deepest;
|
||||
for (let level = 0; level < levels; level++) {
|
||||
payload = { children: [payload] };
|
||||
}
|
||||
return { tenantId: TENANT_A, ...payload };
|
||||
};
|
||||
|
||||
const checkNode = (payload: any) =>
|
||||
assertGraphNotForeign(dataSource.manager, nodes.metadata, [payload], TENANT_A);
|
||||
|
||||
it('still checks the row sitting AT the limit', async () => {
|
||||
const foreign = await nodes.save({ tenantId: TENANT_B, name: 'theirs' });
|
||||
|
||||
await expect(checkNode(chain(GRAPH_CHECK_MAX_DEPTH, { id: foreign.id, name: 'mine' }))).rejects.toThrow(
|
||||
ForbiddenException
|
||||
);
|
||||
});
|
||||
|
||||
it('accepts a payload that stops at the limit', async () => {
|
||||
await expect(checkNode(chain(GRAPH_CHECK_MAX_DEPTH))).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it('refuses a payload that would still persist rows below the limit', async () => {
|
||||
// The row below the limit would be cascaded (or re-parented) with no ownership check at all,
|
||||
// so the payload is refused rather than waved through.
|
||||
await expect(checkNode(chain(GRAPH_CHECK_MAX_DEPTH + 1))).rejects.toThrow(BadRequestException);
|
||||
});
|
||||
});
|
||||
|
||||
describe('cost and scope', () => {
|
||||
it('runs one lookup per relation, not one per nested object', async () => {
|
||||
const { ownParent } = await seed();
|
||||
const own = await children.save([
|
||||
{ tenantId: TENANT_A, parentId: ownParent.id },
|
||||
{ tenantId: TENANT_A, parentId: ownParent.id },
|
||||
{ tenantId: TENANT_A, parentId: ownParent.id }
|
||||
]);
|
||||
const tag = await tags.save({ tenantId: TENANT_A });
|
||||
const spy = jest.spyOn(dataSource.manager, 'getRepository');
|
||||
|
||||
await check({ id: ownParent.id, children: own.map(({ id }) => ({ id })), tags: [{ id: tag.id }] });
|
||||
|
||||
expect(spy.mock.calls.map(([target]) => (target as any).options?.name ?? target)).toEqual(['Child', 'Tag']);
|
||||
});
|
||||
|
||||
it('checks nothing without a tenant (seeds, background jobs)', async () => {
|
||||
const { ownParent, foreignChild } = await seed();
|
||||
|
||||
await expect(
|
||||
check({ id: ownParent.id, children: [{ id: foreignChild.id }] }, null)
|
||||
).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it('turns an unreadable id into a 400 rather than letting it through', async () => {
|
||||
const { ownParent } = await seed();
|
||||
jest.spyOn(dataSource.manager, 'getRepository').mockImplementation(() => {
|
||||
throw new Error('invalid input syntax for type uuid');
|
||||
});
|
||||
|
||||
await expect(check({ id: ownParent.id, children: [{ id: 'not-a-uuid' }] })).rejects.toThrow(
|
||||
BadRequestException
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('through TenantAwareCrudService', () => {
|
||||
class ParentService extends TenantAwareCrudService<any> {
|
||||
constructor(repository: Repository<any>) {
|
||||
super(repository, {} as any);
|
||||
}
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
jest.spyOn(CrudService.prototype, 'ormType', 'get').mockReturnValue(MultiORMEnum.TypeORM);
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT_A);
|
||||
jest.spyOn(RequestContext, 'currentEmployeeId').mockReturnValue(null);
|
||||
jest.spyOn(RequestContext, 'hasPermission').mockImplementation(
|
||||
(permission) => permission === PermissionsEnum.CHANGE_SELECTED_EMPLOYEE
|
||||
);
|
||||
jest.spyOn(console, 'error').mockImplementation(() => undefined);
|
||||
});
|
||||
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
it('create({ ...body, id }) refuses a foreign nested child on the caller’s own row', async () => {
|
||||
const { ownParent, foreignChild } = await seed();
|
||||
const service = new ParentService(parents);
|
||||
|
||||
await expect(
|
||||
service.create({ id: ownParent.id, children: [{ id: foreignChild.id, price: 999 }] })
|
||||
).rejects.toThrow(ForbiddenException);
|
||||
expect((await children.findOneBy({ id: foreignChild.id })).price).toBe(20);
|
||||
});
|
||||
|
||||
it('saveMany() refuses it too, and still saves legitimate graphs', async () => {
|
||||
const { ownParent, ownChild, foreignChild } = await seed();
|
||||
const service = new ParentService(parents);
|
||||
|
||||
await expect(
|
||||
service.saveMany([{ id: ownParent.id, children: [{ id: foreignChild.id }] }])
|
||||
).rejects.toThrow(ForbiddenException);
|
||||
|
||||
await service.saveMany([{ id: ownParent.id, children: [{ id: ownChild.id, price: 42 }] }]);
|
||||
expect((await children.findOneBy({ id: ownChild.id })).price).toBe(42);
|
||||
});
|
||||
});
|
||||
|
||||
describe('ids that resolve case-insensitively (Postgres uuid / MySQL collation)', () => {
|
||||
/**
|
||||
* Postgres stores and renders `uuid` lower case, and MySQL's default collation ignores case, so
|
||||
* `AAAA-…` in the payload addresses the row stored as `aaaa-…` — both here and later, inside
|
||||
* `save()`. `COLLATE NOCASE` reproduces that on sqlite.
|
||||
*/
|
||||
const caseColumns = {
|
||||
id: { primary: true, type: 'varchar', collation: 'NOCASE' },
|
||||
tenantId: { type: 'varchar', nullable: true }
|
||||
} as const;
|
||||
|
||||
const CaseParentSchema = new EntitySchema<any>({
|
||||
name: 'CaseParent',
|
||||
tableName: 'case_parent',
|
||||
columns: { ...caseColumns },
|
||||
relations: {
|
||||
// No cascade: TypeORM re-parents the child by id alone, which is the write this addresses.
|
||||
children: { type: 'one-to-many', target: 'CaseChild', inverseSide: 'parent' }
|
||||
}
|
||||
});
|
||||
|
||||
const CaseChildSchema = new EntitySchema<any>({
|
||||
name: 'CaseChild',
|
||||
tableName: 'case_child',
|
||||
columns: {
|
||||
...caseColumns,
|
||||
price: { type: 'int', nullable: true },
|
||||
parentId: { type: 'varchar', nullable: true }
|
||||
},
|
||||
relations: {
|
||||
parent: {
|
||||
type: 'many-to-one',
|
||||
target: 'CaseParent',
|
||||
inverseSide: 'children',
|
||||
joinColumn: { name: 'parentId' }
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
const OWN_PARENT = 'cccccccc-1111-4111-8111-111111111111';
|
||||
const FOREIGN_CHILD = 'dddddddd-2222-4222-8222-222222222222';
|
||||
|
||||
let caseSource: DataSource;
|
||||
|
||||
beforeEach(async () => {
|
||||
caseSource = new DataSource({
|
||||
type: 'better-sqlite3',
|
||||
database: ':memory:',
|
||||
entities: [CaseParentSchema, CaseChildSchema],
|
||||
synchronize: true,
|
||||
logging: false
|
||||
});
|
||||
await caseSource.initialize();
|
||||
await caseSource.getRepository('CaseParent').save({ id: OWN_PARENT, tenantId: TENANT_A });
|
||||
await caseSource.getRepository('CaseChild').save({ id: FOREIGN_CHILD, tenantId: TENANT_B, price: 20 });
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await caseSource.destroy();
|
||||
});
|
||||
|
||||
const payload = () => ({
|
||||
id: OWN_PARENT,
|
||||
children: [{ id: FOREIGN_CHILD.toUpperCase() }]
|
||||
});
|
||||
|
||||
it('CONTROL: a plain save claims the foreign row through the upper-cased id', async () => {
|
||||
await caseSource.getRepository('CaseParent').save(payload());
|
||||
|
||||
const stored = await caseSource.getRepository('CaseChild').findOneBy({ id: FOREIGN_CHILD });
|
||||
expect(stored.parentId).toBe(OWN_PARENT);
|
||||
expect(stored.tenantId).toBe(TENANT_B);
|
||||
});
|
||||
|
||||
it('refuses it, instead of missing the row and waving the payload through', async () => {
|
||||
await expect(
|
||||
assertGraphNotForeign(
|
||||
caseSource.manager,
|
||||
caseSource.getMetadata('CaseParent'),
|
||||
[payload()],
|
||||
TENANT_A
|
||||
)
|
||||
).rejects.toThrow(ForbiddenException);
|
||||
|
||||
const stored = await caseSource.getRepository('CaseChild').findOneBy({ id: FOREIGN_CHILD });
|
||||
expect(stored.parentId).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('never-matching employee condition (GHSA-6qvm-3wg4-26w4, time logs / slots)', () => {
|
||||
const RowSchema = new EntitySchema<any>({
|
||||
name: 'Row',
|
||||
tableName: 'row',
|
||||
columns: { id: { primary: true, type: 'varchar', generated: 'uuid' }, employeeId: { type: 'varchar' } }
|
||||
});
|
||||
|
||||
it('`employeeId IN ()` matches nothing, where the old empty condition matched every row', async () => {
|
||||
const rowsSource = new DataSource({
|
||||
type: 'better-sqlite3',
|
||||
database: ':memory:',
|
||||
entities: [RowSchema],
|
||||
synchronize: true
|
||||
});
|
||||
await rowsSource.initialize();
|
||||
try {
|
||||
const rows = rowsSource.getRepository('Row');
|
||||
await rows.save([{ employeeId: 'e1' }, { employeeId: 'e2' }]);
|
||||
|
||||
// CONTROL: the historical fallback for a caller with no employee record.
|
||||
expect(await rows.count({ where: {} })).toBe(2);
|
||||
expect(await rows.count({ where: { employeeId: In([]) } })).toBe(0);
|
||||
} finally {
|
||||
await rowsSource.destroy();
|
||||
}
|
||||
});
|
||||
|
||||
it('the time log and time slot services replace the tenant-wide fallback', () => {
|
||||
const base = (TenantAwareCrudService.prototype as any).findConditionsWithoutOwnEmployee;
|
||||
|
||||
// CONTROL: everywhere else the fallback stays what it always was — tenant-wide.
|
||||
expect(base.call({})).toEqual({});
|
||||
|
||||
for (const service of [TimeLogService, TimeSlotService]) {
|
||||
const hook = (service.prototype as any).findConditionsWithoutOwnEmployee;
|
||||
expect(hook).not.toBe(base);
|
||||
expect(hook.call(service.prototype)).toEqual({ employeeId: In([]) });
|
||||
}
|
||||
});
|
||||
|
||||
it('a caller with neither CHANGE_SELECTED_EMPLOYEE nor an employee record reaches the fallback', async () => {
|
||||
const rowsSource = new DataSource({
|
||||
type: 'better-sqlite3',
|
||||
database: ':memory:',
|
||||
entities: [RowSchema],
|
||||
synchronize: true
|
||||
});
|
||||
await rowsSource.initialize();
|
||||
try {
|
||||
const rows = rowsSource.getRepository('Row');
|
||||
// No tenant column on this fixture: the case is about the EMPLOYEE half of the condition.
|
||||
await rows.save([{ employeeId: 'e1' }, { employeeId: 'e2' }]);
|
||||
|
||||
class RowService extends TenantAwareCrudService<any> {
|
||||
constructor() {
|
||||
super(rows as any, {} as any);
|
||||
}
|
||||
}
|
||||
const service = new RowService();
|
||||
|
||||
jest.spyOn(CrudService.prototype, 'ormType', 'get').mockReturnValue(MultiORMEnum.TypeORM);
|
||||
jest.spyOn(RequestContext, 'currentUser').mockReturnValue({ id: 'u1', tenantId: TENANT_A } as any);
|
||||
jest.spyOn(RequestContext, 'currentEmployeeId').mockReturnValue(null);
|
||||
jest.spyOn(RequestContext, 'hasPermission').mockReturnValue(false);
|
||||
|
||||
// CONTROL: the inherited fallback leaves the read tenant-wide.
|
||||
expect(await service.find()).toHaveLength(2);
|
||||
|
||||
jest.spyOn(RowService.prototype as any, 'findConditionsWithoutOwnEmployee').mockReturnValue({
|
||||
employeeId: In([])
|
||||
});
|
||||
expect(await service.find()).toHaveLength(0);
|
||||
} finally {
|
||||
jest.restoreAllMocks();
|
||||
await rowsSource.destroy();
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,367 @@
|
||||
// cspell:ignore reparents
|
||||
import { BadRequestException, ForbiddenException } from '@nestjs/common';
|
||||
import { EntityManager, EntityMetadata } from 'typeorm';
|
||||
import { RelationMetadata } from 'typeorm/metadata/RelationMetadata';
|
||||
import { ID } from '@gauzy/contracts';
|
||||
|
||||
/**
|
||||
* How many levels of nested objects below the root are walked. Real payloads nest two or three
|
||||
* levels deep (invoice → items, integration setting → tied entities); a payload that would still
|
||||
* persist rows below this depth is refused (400), because nothing down there can be checked.
|
||||
*/
|
||||
export const GRAPH_CHECK_MAX_DEPTH = 5;
|
||||
|
||||
interface IGraphNode {
|
||||
metadata: EntityMetadata;
|
||||
entity: Record<string, any>;
|
||||
depth: number;
|
||||
}
|
||||
|
||||
interface IGraphRef {
|
||||
relation: RelationMetadata;
|
||||
node: IGraphNode;
|
||||
/** The nested object, or the primitive id TypeORM treats as a foreign key. */
|
||||
value: unknown;
|
||||
/** Position in the relation array, or -1 for a single-valued relation. */
|
||||
index: number;
|
||||
id: ID | undefined;
|
||||
}
|
||||
|
||||
interface IStoredRow {
|
||||
tenantId: ID | null;
|
||||
parentId?: ID | null;
|
||||
}
|
||||
|
||||
const isPlainValue = (value: unknown): value is string | number => typeof value === 'string' || typeof value === 'number';
|
||||
|
||||
/**
|
||||
* The key a loaded row is stored and looked up under.
|
||||
*
|
||||
* Postgres `uuid` columns always render lower case and MySQL's default collation is case
|
||||
* insensitive, so `AAAA-…` in the payload SELECTs the row stored as `aaaa-…`. Keying the lookup by
|
||||
* the raw string would then miss the row, skip the ownership check and let the very payload this
|
||||
* helper exists to refuse reach `save()`, which resolves the id case-insensitively again
|
||||
* (GHSA-jh6m-9fxr-rx3c). Ids are uuids here — {@link isTenantScoped} requires a single `id` primary
|
||||
* key — so folding the case cannot merge two different rows.
|
||||
*/
|
||||
const rowKey = (id: ID | number): string => String(id).toLowerCase();
|
||||
|
||||
const isObject = (value: unknown): value is Record<string, any> =>
|
||||
!!value && typeof value === 'object' && !(value instanceof Date) && typeof (value as any).then !== 'function';
|
||||
|
||||
/**
|
||||
* TypeORM rewrites the CHILD row of these relations to point at the parent, cascade or not
|
||||
* (OneToManySubjectBuilder / OneToOneInverseSideSubjectBuilder), so referencing a row here is a write.
|
||||
*/
|
||||
const reparentsTarget = (relation: RelationMetadata): boolean => relation.isOneToMany || relation.isOneToOneNotOwner;
|
||||
|
||||
const cascadesInto = (relation: RelationMetadata): boolean => relation.isCascadeInsert || relation.isCascadeUpdate;
|
||||
|
||||
/** Only rows with a single `id` primary key and a `tenantId` column can be checked (and need to be). */
|
||||
const isTenantScoped = (metadata: EntityMetadata): boolean =>
|
||||
metadata.primaryColumns.length === 1 &&
|
||||
metadata.primaryColumns[0].propertyName === 'id' &&
|
||||
metadata.hasColumnWithPropertyPath('tenantId');
|
||||
|
||||
/**
|
||||
* Refuses a persistence payload whose NESTED objects or ids reach rows of another tenant.
|
||||
*
|
||||
* `assertNotForeignRow` only checks the root id. TypeORM's save() resolves every nested object by
|
||||
* primary key alone: cascaded relations are UPDATEd in place, one-to-many children are re-parented
|
||||
* even without cascade, and owner / many-to-many relations store whatever id they are given. A body
|
||||
* such as `{ invoiceItems: [{ id: <another tenant's item>, price: 1 }] }` on the caller's OWN invoice
|
||||
* therefore overwrote and claimed the other tenant's row (GHSA-jh6m-9fxr-rx3c).
|
||||
*
|
||||
* The walk follows the TypeORM relation metadata, one level at a time, with one SELECT per relation
|
||||
* per level (soft-deleted rows included, since save() reaches those too):
|
||||
*
|
||||
* - A row of ANOTHER tenant is refused through any relation.
|
||||
* - A row with a NULL tenant (global languages, system issue types / priorities, global tags):
|
||||
* - may be LINKED (owner many-to-one / one-to-one, many-to-many), as today; when the relation
|
||||
* cascades and the payload carries fields beyond `id`, the object is reduced to `{ id }` so the
|
||||
* global row is linked but never written;
|
||||
* - may not be re-parented (one-to-many / inverse one-to-one) unless it already belongs to this
|
||||
* parent — adopting it would be a write into a row no tenant owns.
|
||||
* - Objects persisted through a cascade are recursed into (visited set, and a payload that would
|
||||
* persist below {@link GRAPH_CHECK_MAX_DEPTH} is refused rather than left unchecked) and stamped
|
||||
* with the caller's tenant (a nested `tenantId` would otherwise place — or move — the row into
|
||||
* another tenant). An EXISTING `User` is reduced to `{ id }`: it is linked, never written.
|
||||
*
|
||||
* Nested objects are only replaced (with a copy) when fields have to be removed; tenant stamping is
|
||||
* done in place so the caller still sees ids TypeORM generates on save.
|
||||
*
|
||||
* @param manager - The TypeORM entity manager used for the lookups.
|
||||
* @param metadata - Metadata of the root entity.
|
||||
* @param entities - The root payloads about to be persisted.
|
||||
* @param tenantId - The caller's tenant; without one nothing can be judged and nothing is checked.
|
||||
*/
|
||||
export async function assertGraphNotForeign(
|
||||
manager: EntityManager,
|
||||
metadata: EntityMetadata | undefined,
|
||||
entities: unknown[],
|
||||
tenantId: ID | null | undefined
|
||||
): Promise<void> {
|
||||
if (!tenantId || !manager || !metadata?.relations) {
|
||||
return;
|
||||
}
|
||||
|
||||
const visited = new WeakSet<object>();
|
||||
let level: IGraphNode[] = [];
|
||||
for (const entity of entities ?? []) {
|
||||
if (isObject(entity) && !visited.has(entity)) {
|
||||
visited.add(entity);
|
||||
level.push({ metadata, entity, depth: 0 });
|
||||
}
|
||||
}
|
||||
|
||||
while (level.length) {
|
||||
const next: IGraphNode[] = [];
|
||||
|
||||
for (const [relation, refs] of collectReferences(level)) {
|
||||
next.push(...(await walkRelation(manager, relation, refs, tenantId, visited)));
|
||||
}
|
||||
|
||||
level = next;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks every reference carried by ONE relation (one batched lookup) and returns the nodes the next
|
||||
* level has to walk.
|
||||
*
|
||||
* @param manager - The TypeORM entity manager used for the lookup.
|
||||
* @param relation - The relation the references were collected from.
|
||||
* @param refs - The nested objects / ids that relation carries at this level.
|
||||
* @param tenantId - The caller's tenant.
|
||||
* @param visited - Objects already queued, so a cyclic payload is walked once.
|
||||
*/
|
||||
async function walkRelation(
|
||||
manager: EntityManager,
|
||||
relation: RelationMetadata,
|
||||
refs: IGraphRef[],
|
||||
tenantId: ID,
|
||||
visited: WeakSet<object>
|
||||
): Promise<IGraphNode[]> {
|
||||
const target = relation.inverseEntityMetadata;
|
||||
const scoped = isTenantScoped(target);
|
||||
const rows = scoped ? await loadStoredRows(manager, relation, refs) : new Map<string, IStoredRow>();
|
||||
const next: IGraphNode[] = [];
|
||||
|
||||
for (const ref of refs) {
|
||||
const row = ref.id !== undefined ? rows.get(rowKey(ref.id)) : undefined;
|
||||
const persisted = resolveReference(ref, row, scoped, tenantId);
|
||||
const depth = ref.node.depth + 1;
|
||||
|
||||
if (!persisted || visited.has(persisted)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (depth >= GRAPH_CHECK_MAX_DEPTH) {
|
||||
// Nothing below this level is walked, so nothing may be persisted below it either: save()
|
||||
// would cascade (or re-parent) those rows with no ownership check at all.
|
||||
if (hasRelationPayload(target, persisted)) {
|
||||
throw new BadRequestException(
|
||||
`Nested payload in "${relation.propertyPath}" is deeper than ${GRAPH_CHECK_MAX_DEPTH} levels`
|
||||
);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
visited.add(persisted);
|
||||
next.push({ metadata: target, entity: persisted, depth });
|
||||
}
|
||||
|
||||
return next;
|
||||
}
|
||||
|
||||
/**
|
||||
* Applies the ownership rules to ONE reference and returns the object the walk should descend into
|
||||
* — `undefined` when the reference is only a link (nothing of it is persisted) or had to be reduced
|
||||
* to one. Throws when the reference may not be persisted at all.
|
||||
*
|
||||
* @param ref - The nested object / id and the relation that carries it.
|
||||
* @param row - The stored row it names, when it names one.
|
||||
* @param scoped - Whether the target entity is tenant scoped (see {@link isTenantScoped}).
|
||||
* @param tenantId - The caller's tenant.
|
||||
*/
|
||||
function resolveReference(
|
||||
ref: IGraphRef,
|
||||
row: IStoredRow | undefined,
|
||||
scoped: boolean,
|
||||
tenantId: ID
|
||||
): Record<string, any> | undefined {
|
||||
const { relation } = ref;
|
||||
|
||||
if (row) {
|
||||
const rowTenantId = row.tenantId ?? null;
|
||||
|
||||
if (rowTenantId !== null && rowKey(rowTenantId) !== rowKey(tenantId)) {
|
||||
throw new ForbiddenException('A related record belongs to another tenant');
|
||||
}
|
||||
|
||||
if (rowTenantId === null && !isGlobalRowWritable(ref, row)) {
|
||||
// Keep the link to the global row, but never write into it.
|
||||
replaceValue(ref, { id: ref.id });
|
||||
return undefined;
|
||||
}
|
||||
}
|
||||
|
||||
if (!cascadesInto(relation) || !isObject(ref.value)) {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
// An EXISTING user is LINKED through another entity's cascade, never written into. Removing only
|
||||
// the credential columns still left `user: { id, firstName }` renaming any account of the tenant —
|
||||
// a tenant check alone cannot stop that, since the victim is a legitimate member of the caller's
|
||||
// tenant (GHSA-jh6m-9fxr-rx3c). A NEW user inserted through a cascade (candidate sign-up) is not
|
||||
// reduced and keeps every field.
|
||||
if (row && relation.inverseEntityMetadata.name === 'User') {
|
||||
if (hasFieldsBeyondId(ref.value)) {
|
||||
replaceValue(ref, { id: ref.id });
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const persisted = ref.value;
|
||||
|
||||
if (scoped && (!row || row.tenantId != null || 'tenantId' in persisted || 'tenant' in persisted)) {
|
||||
stampTenant(persisted, tenantId);
|
||||
}
|
||||
|
||||
return persisted;
|
||||
}
|
||||
|
||||
/**
|
||||
* Rules for a stored row that belongs to NO tenant (global languages, system issue types and
|
||||
* priorities, global tags): it may be linked, but re-parenting it is a write into a row no tenant
|
||||
* owns — refused unless it already hangs off this very parent.
|
||||
*
|
||||
* @returns false when a cascading payload has to be reduced to a plain link.
|
||||
*/
|
||||
function isGlobalRowWritable(ref: IGraphRef, row: IStoredRow): boolean {
|
||||
const { relation } = ref;
|
||||
|
||||
if (reparentsTarget(relation)) {
|
||||
const parentId = ref.node.entity?.id;
|
||||
if (!parentId || row.parentId == null || rowKey(row.parentId) !== rowKey(parentId)) {
|
||||
throw new ForbiddenException('A related record is not owned by this tenant');
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
return !(cascadesInto(relation) && isObject(ref.value) && hasFieldsBeyondId(ref.value));
|
||||
}
|
||||
|
||||
/**
|
||||
* Groups every nested object / id of the given nodes by the relation that carries it.
|
||||
*/
|
||||
function collectReferences(level: IGraphNode[]): Map<RelationMetadata, IGraphRef[]> {
|
||||
const refsByRelation = new Map<RelationMetadata, IGraphRef[]>();
|
||||
|
||||
for (const node of level) {
|
||||
for (const relation of node.metadata.relations) {
|
||||
const raw = node.entity[relation.propertyName];
|
||||
// Nothing to check, or a MikroORM Collection of loaded entities rather than request data.
|
||||
if (raw === undefined || raw === null || typeof raw.getItems === 'function') {
|
||||
continue;
|
||||
}
|
||||
const values: unknown[] = Array.isArray(raw) ? raw : [raw];
|
||||
values.forEach((value, index) => {
|
||||
let id: ID | undefined;
|
||||
if (isPlainValue(value)) {
|
||||
id = value as ID;
|
||||
} else if (isObject(value)) {
|
||||
id = isPlainValue(value.id) ? (value.id as ID) : undefined;
|
||||
} else {
|
||||
return;
|
||||
}
|
||||
const refs = refsByRelation.get(relation) ?? [];
|
||||
refs.push({ relation, node, value, index: Array.isArray(raw) ? index : -1, id });
|
||||
refsByRelation.set(relation, refs);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
return refsByRelation;
|
||||
}
|
||||
|
||||
/**
|
||||
* Loads `{ id, tenantId }` — plus the parent foreign key for re-parenting relations — of every row the
|
||||
* refs name, in one query.
|
||||
*/
|
||||
async function loadStoredRows(
|
||||
manager: EntityManager,
|
||||
relation: RelationMetadata,
|
||||
refs: IGraphRef[]
|
||||
): Promise<Map<string, IStoredRow>> {
|
||||
const ids = [...new Set(refs.map((ref) => ref.id).filter((id) => id !== undefined && id !== ''))];
|
||||
const rows = new Map<string, IStoredRow>();
|
||||
if (!ids.length) {
|
||||
return rows;
|
||||
}
|
||||
|
||||
const target = relation.inverseEntityMetadata;
|
||||
const inverse = relation.inverseRelation;
|
||||
const selectsParent = reparentsTarget(relation) && inverse?.joinColumns?.length === 1;
|
||||
|
||||
try {
|
||||
const query = manager
|
||||
.getRepository(target.target)
|
||||
.createQueryBuilder('graph_row')
|
||||
.withDeleted()
|
||||
.select('graph_row.id', 'id')
|
||||
.addSelect('graph_row.tenantId', 'tenantId');
|
||||
if (selectsParent) {
|
||||
query.addSelect(`graph_row.${inverse.propertyPath}`, 'parentId');
|
||||
}
|
||||
const raw: Array<{ id: ID; tenantId: ID | null; parentId?: ID | null }> = await query
|
||||
.whereInIds(ids)
|
||||
.getRawMany();
|
||||
|
||||
for (const row of raw) {
|
||||
rows.set(rowKey(row.id), { tenantId: row.tenantId ?? null, parentId: row.parentId ?? null });
|
||||
}
|
||||
} catch {
|
||||
// An id the database cannot even parse (e.g. not a UUID) cannot be proven harmless. The driver
|
||||
// error itself is deliberately not surfaced: it would echo the query back to the caller.
|
||||
throw new BadRequestException(`Invalid reference in "${relation.propertyPath}"`);
|
||||
}
|
||||
|
||||
return rows;
|
||||
}
|
||||
|
||||
function hasFieldsBeyondId(value: Record<string, any>): boolean {
|
||||
return Object.keys(value).some((key) => key !== 'id' && value[key] !== undefined);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the object carries request data for any relation of its entity — i.e. whether persisting
|
||||
* it would reach further rows. A loaded MikroORM Collection is not request data (see
|
||||
* {@link collectReferences}).
|
||||
*/
|
||||
function hasRelationPayload(metadata: EntityMetadata, entity: Record<string, any>): boolean {
|
||||
return metadata.relations.some((relation) => {
|
||||
const raw = entity[relation.propertyName];
|
||||
return raw !== undefined && raw !== null && typeof raw.getItems !== 'function';
|
||||
});
|
||||
}
|
||||
|
||||
function stampTenant(value: Record<string, any>, tenantId: ID): void {
|
||||
value.tenantId = tenantId;
|
||||
if ('tenant' in value) {
|
||||
value.tenant = { id: tenantId };
|
||||
}
|
||||
}
|
||||
|
||||
/** Swaps the nested value on its parent, without mutating the original object or array. */
|
||||
function replaceValue(ref: IGraphRef, replacement: unknown): void {
|
||||
const { entity } = ref.node;
|
||||
const property = ref.relation.propertyName;
|
||||
if (ref.index >= 0) {
|
||||
const copy = [...entity[property]];
|
||||
copy[ref.index] = replacement;
|
||||
entity[property] = copy;
|
||||
} else {
|
||||
entity[property] = replacement;
|
||||
}
|
||||
}
|
||||
@@ -9,6 +9,7 @@ import { RequestContext } from '../context';
|
||||
import { TenantBaseEntity } from '../entities/internal';
|
||||
import { CrudService } from './crud.service';
|
||||
import { assertCriteriaHasPredicate } from './criteria.helper';
|
||||
import { assertGraphNotForeign } from './nested-graph-ownership.helper';
|
||||
import { ICrudService, IPartialEntity } from './icrud.service';
|
||||
import { ITryRequest } from './try-request';
|
||||
|
||||
@@ -85,9 +86,32 @@ export abstract class TenantAwareCrudService<T extends TenantBaseEntity>
|
||||
} as unknown as FindOptionsWhere<T>;
|
||||
}
|
||||
|
||||
// A caller who may not act for other employees, but has no employee record of their own.
|
||||
if (!isNotEmpty(employeeId) && hasEmployeeColumn && !canChangeEmployee) {
|
||||
return this.findConditionsWithoutOwnEmployee();
|
||||
}
|
||||
|
||||
return {} as FindOptionsWhere<T>;
|
||||
}
|
||||
|
||||
/**
|
||||
* Conditions for a caller who lacks CHANGE_SELECTED_EMPLOYEE and has no employee record, on an
|
||||
* entity with an `employeeId` column.
|
||||
*
|
||||
* The default keeps the historical tenant-wide scope. A service whose rows are strictly personal
|
||||
* overrides this with {@link neverMatchingEmployeeCondition}.
|
||||
*/
|
||||
protected findConditionsWithoutOwnEmployee(): FindOptionsWhere<T> {
|
||||
return {} as FindOptionsWhere<T>;
|
||||
}
|
||||
|
||||
/**
|
||||
* A condition that matches no row (`employeeId IN ()` renders as `0=1`).
|
||||
*/
|
||||
protected neverMatchingEmployeeCondition(): FindOptionsWhere<T> {
|
||||
return { employeeId: In([]) } as unknown as FindOptionsWhere<T>;
|
||||
}
|
||||
|
||||
/**
|
||||
* Executes a callback without automatic employeeId filtering.
|
||||
* This is useful when you need to implement custom access control logic.
|
||||
@@ -457,6 +481,26 @@ export abstract class TenantAwareCrudService<T extends TenantBaseEntity>
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Extends the root-id check to the nested objects and ids of the payload (cascaded relations,
|
||||
* re-parented one-to-many children, owner / many-to-many links). See {@link assertGraphNotForeign}.
|
||||
*
|
||||
* The lookups go through TypeORM for both ORMs: both are initialised on the same database, and the
|
||||
* check only reads. For MikroORM the same payload shape reaches `assign()` / `em.create()`, which
|
||||
* resolve nested objects by primary key as well.
|
||||
*
|
||||
* @param entities - The payloads about to be persisted.
|
||||
* @param tenantId - The caller's tenant.
|
||||
*/
|
||||
protected async assertNestedGraphNotForeign(entities: IPartialEntity<T>[], tenantId: ID | null): Promise<void> {
|
||||
await assertGraphNotForeign(
|
||||
this.typeOrmRepository.manager,
|
||||
this.typeOrmRepository.metadata,
|
||||
entities as unknown[],
|
||||
tenantId
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a new entity instance and copies all entity properties from this object into a new entity.
|
||||
* Note that it copies only properties that are present in entity schema.
|
||||
@@ -468,6 +512,7 @@ export abstract class TenantAwareCrudService<T extends TenantBaseEntity>
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
const employeeId = RequestContext.currentEmployeeId();
|
||||
await this.assertNotForeignRow(entity, tenantId);
|
||||
await this.assertNestedGraphNotForeign([entity], tenantId);
|
||||
|
||||
const hasTenantColumn = this.typeOrmRepository.metadata?.hasColumnWithPropertyPath('tenantId');
|
||||
const hasEmployeeColumn = this.typeOrmRepository.metadata?.hasColumnWithPropertyPath('employeeId');
|
||||
@@ -500,6 +545,7 @@ export abstract class TenantAwareCrudService<T extends TenantBaseEntity>
|
||||
public async createMany(entities: IPartialEntity<T>[]): Promise<T[]> {
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
await this.assertNotForeignRows(entities, tenantId);
|
||||
await this.assertNestedGraphNotForeign(entities, tenantId);
|
||||
const employeeId = RequestContext.currentEmployeeId();
|
||||
|
||||
const hasTenantColumn = this.typeOrmRepository.metadata?.hasColumnWithPropertyPath('tenantId');
|
||||
@@ -528,6 +574,7 @@ export abstract class TenantAwareCrudService<T extends TenantBaseEntity>
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
const hasTenantColumn = this.typeOrmRepository.metadata?.hasColumnWithPropertyPath('tenantId');
|
||||
await this.assertNotForeignRow(entity, tenantId);
|
||||
await this.assertNestedGraphNotForeign([entity], tenantId);
|
||||
|
||||
return await super.save({
|
||||
...entity,
|
||||
@@ -564,6 +611,7 @@ export abstract class TenantAwareCrudService<T extends TenantBaseEntity>
|
||||
public async saveMany(entities: IPartialEntity<T>[]): Promise<T[]> {
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
await this.assertNotForeignRows(entities, tenantId);
|
||||
await this.assertNestedGraphNotForeign(entities, tenantId);
|
||||
const hasTenantColumn = this.typeOrmRepository.metadata?.hasColumnWithPropertyPath('tenantId');
|
||||
|
||||
const enriched = entities.map((entity) => ({
|
||||
|
||||
@@ -0,0 +1,214 @@
|
||||
// Must stay first: loads the entity graph before the DTOs pull an entity (see activity.controller.spec.ts).
|
||||
import '../entities/internal';
|
||||
|
||||
import { ArgumentMetadata, ValidationPipe } from '@nestjs/common';
|
||||
import { PIPES_METADATA, ROUTE_ARGS_METADATA } from '@nestjs/common/constants';
|
||||
import { InvoiceStatusTypesEnum, InvoiceTypeEnum, TimeLogSourceEnum, TimeLogType } from '@gauzy/contracts';
|
||||
import { UpdateInvoiceDTO } from '../../invoice/dto/update-invoice.dto';
|
||||
import { CreateManualTimeLogDTO } from '../../time-tracking/time-log/dto/create-time-log.dto';
|
||||
import { UpdateManualTimeLogDTO } from '../../time-tracking/time-log/dto/update-time-log.dto';
|
||||
import { UpdateTimeSlotDTO } from '../../time-tracking/time-slot/dto/update-time-slot.dto';
|
||||
import { InvoiceController } from '../../invoice/invoice.controller';
|
||||
import { TimeLogController } from '../../time-tracking/time-log/time-log.controller';
|
||||
import { TimeSlotController } from '../../time-tracking/time-slot/time-slot.controller';
|
||||
|
||||
/**
|
||||
* Update routes that feed create()/save()/update() with the body now run `whitelist: true`
|
||||
* (GHSA-jh6m-9fxr-rx3c: PUT /invoices/:id; GHSA-6qvm-3wg4-26w4: POST/PUT /timesheet/time-log and
|
||||
* PUT /timesheet/time-slot/:id). Whitelisting only helps if it does not silently drop what the Angular
|
||||
* and desktop clients send, so each case runs the REAL pipe over the payload those clients build and
|
||||
* checks both halves: every client field survives, and nothing else does.
|
||||
*
|
||||
* CONTROL: the same payload through the pre-fix pipe options keeps the smuggled fields.
|
||||
*
|
||||
* The payloads use `sentTo` instead of tenantId/organizationId: those two carry async ownership
|
||||
* validators that need a live request context, and are not what these cases are about.
|
||||
*/
|
||||
|
||||
/** The keys that carry a value (declared-but-absent class fields may exist as `undefined`). */
|
||||
const definedKeys = (value: Record<string, any>) =>
|
||||
Object.keys(value)
|
||||
.filter((key) => value[key] !== undefined)
|
||||
.sort();
|
||||
|
||||
const run = (options: ConstructorParameters<typeof ValidationPipe>[0], metatype: any, body: any) =>
|
||||
new ValidationPipe({
|
||||
...options,
|
||||
// Name the failing constraints instead of a bare "Bad Request Exception".
|
||||
exceptionFactory: (errors) =>
|
||||
new Error(JSON.stringify(errors.map(({ property, constraints, children }) => ({ property, constraints, children }))))
|
||||
}).transform(body, { type: 'body', metatype } as ArgumentMetadata);
|
||||
|
||||
describe('whitelisted update routes', () => {
|
||||
describe('PUT /invoices/:id — UpdateInvoiceDTO', () => {
|
||||
// What invoice-edit.component.ts sends (minus tenantId / organizationId, see above).
|
||||
const uiPayload = () => ({
|
||||
invoiceNumber: 7,
|
||||
invoiceDate: '2026-09-01T00:00:00.000Z',
|
||||
dueDate: '2026-09-30T23:59:59.999Z',
|
||||
currency: 'USD',
|
||||
discountValue: 0,
|
||||
discountType: 'PERCENT',
|
||||
tax: 0,
|
||||
tax2: 0,
|
||||
taxType: 'PERCENT',
|
||||
tax2Type: 'PERCENT',
|
||||
terms: 'net 30',
|
||||
totalValue: 100,
|
||||
invoiceType: InvoiceTypeEnum.BY_EMPLOYEE_HOURS,
|
||||
organizationContactId: 'contact-1',
|
||||
toContact: { id: 'contact-1', name: 'Client' },
|
||||
tags: [{ id: 'tag-1' }],
|
||||
status: InvoiceStatusTypesEnum.DRAFT,
|
||||
sentTo: 'org-a',
|
||||
hasRemainingAmountInvoiced: false,
|
||||
alreadyPaid: 0,
|
||||
amountDue: 100
|
||||
});
|
||||
|
||||
const smuggled = {
|
||||
token: 'public-link-token',
|
||||
invoiceItems: [
|
||||
{
|
||||
id: '6c1a3b7e-8a51-4c1e-9d55-6a0f2b1c3d4e',
|
||||
price: 1,
|
||||
quantity: 1,
|
||||
totalValue: 1,
|
||||
invoiceId: 'invoice-1',
|
||||
sentTo: 'org-a',
|
||||
hacked: true
|
||||
}
|
||||
]
|
||||
};
|
||||
|
||||
it('keeps every field the invoice edit screen sends', async () => {
|
||||
const result = await run({ transform: true, whitelist: true }, UpdateInvoiceDTO, uiPayload());
|
||||
|
||||
expect(definedKeys(result)).toEqual(definedKeys(uiPayload()));
|
||||
});
|
||||
|
||||
it('drops unknown fields, but keeps an in-place invoice item id', async () => {
|
||||
const result = await run({ transform: true, whitelist: true }, UpdateInvoiceDTO, {
|
||||
...uiPayload(),
|
||||
...smuggled
|
||||
});
|
||||
|
||||
expect(result.token).toBeUndefined();
|
||||
expect(result.invoiceItems[0].id).toBe(smuggled.invoiceItems[0].id);
|
||||
expect(result.invoiceItems[0].hacked).toBeUndefined();
|
||||
});
|
||||
|
||||
it('CONTROL: the pre-fix pipe keeps the smuggled fields', async () => {
|
||||
const result = await run({ transform: true }, UpdateInvoiceDTO, { ...uiPayload(), ...smuggled });
|
||||
|
||||
expect(result.token).toBe('public-link-token');
|
||||
expect(result.invoiceItems[0].hacked).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('POST / PUT /timesheet/time-log — manual time DTOs', () => {
|
||||
// edit-time-log-modal.component.ts: the form value plus dates, log type and source.
|
||||
const uiPayload = () => ({
|
||||
isBillable: true,
|
||||
employeeId: '2f0cbd4e-1c9d-4c34-8f7c-3c4b2f0e9a11',
|
||||
projectId: 'project-1',
|
||||
organizationContactId: 'contact-1',
|
||||
organizationTeamId: 'team-1',
|
||||
taskId: 'task-1',
|
||||
description: 'work',
|
||||
reason: 'forgot the timer',
|
||||
startedAt: new Date('2026-09-01T09:00:00Z'),
|
||||
stoppedAt: new Date('2026-09-01T10:00:00Z'),
|
||||
sentTo: 'org-a',
|
||||
logType: TimeLogType.MANUAL,
|
||||
source: TimeLogSourceEnum.WEB_TIMER
|
||||
});
|
||||
const smuggled = { isRunning: true, timesheetId: 'foreign-timesheet', id: 'some-log', timeSlots: [{ id: 'slot' }] };
|
||||
|
||||
it.each([
|
||||
['PUT', UpdateManualTimeLogDTO],
|
||||
['POST', CreateManualTimeLogDTO]
|
||||
])('%s keeps every field the edit modal sends and drops the rest', async (_method, dto) => {
|
||||
const result = await run({ transform: true, whitelist: true }, dto, { ...uiPayload(), ...smuggled });
|
||||
|
||||
expect(definedKeys(result)).toEqual(definedKeys(uiPayload()));
|
||||
expect(result).toMatchObject({ description: 'work', reason: 'forgot the timer', isBillable: true });
|
||||
});
|
||||
|
||||
it('keeps the timer config fields (tags, version) of the web timer', async () => {
|
||||
const result = await run({ transform: true, whitelist: true }, CreateManualTimeLogDTO, {
|
||||
...uiPayload(),
|
||||
tags: ['tag-1'],
|
||||
version: '1.0.0'
|
||||
});
|
||||
|
||||
expect(result).toMatchObject({ tags: ['tag-1'], version: '1.0.0' });
|
||||
});
|
||||
|
||||
it('CONTROL: the pre-fix pipe keeps the smuggled fields', async () => {
|
||||
const result = await run({ transform: true }, UpdateManualTimeLogDTO, { ...uiPayload(), ...smuggled });
|
||||
|
||||
expect(result).toMatchObject({ isRunning: true, timesheetId: 'foreign-timesheet', id: 'some-log' });
|
||||
});
|
||||
});
|
||||
|
||||
describe('PUT /timesheet/time-slot/:id — UpdateTimeSlotDTO', () => {
|
||||
// apps/desktop app.service.ts updateToTimeSlot().
|
||||
const desktopPayload = () => ({ duration: 600, keyboard: 10, mouse: 20, overall: 30, activities: [{ title: 'x' }] });
|
||||
|
||||
it('keeps what the desktop timer sends and drops tenant / organization', async () => {
|
||||
const result = await run({ whitelist: true }, UpdateTimeSlotDTO, {
|
||||
...desktopPayload(),
|
||||
tenantId: 'b',
|
||||
organizationId: 'org-b',
|
||||
timeLogs: [{ id: 'log' }]
|
||||
});
|
||||
|
||||
expect(definedKeys(result)).toEqual(definedKeys(desktopPayload()));
|
||||
expect(result).toMatchObject(desktopPayload());
|
||||
});
|
||||
|
||||
it('CONTROL: without whitelisting the body reaches the handler whole', async () => {
|
||||
const result = await run({}, UpdateTimeSlotDTO, { ...desktopPayload(), tenantId: 'b' });
|
||||
|
||||
expect(result.tenantId).toBe('b');
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* The DTO cases above only prove the DTOs are right. These prove the ROUTES actually hand their
|
||||
* body to a whitelisting pipe — the half a refactor can silently drop.
|
||||
*/
|
||||
describe('the routes run the whitelisting pipe', () => {
|
||||
const whitelistOf = (pipes: any[] = []) =>
|
||||
pipes.find((pipe) => pipe instanceof ValidationPipe)?.['validatorOptions']?.whitelist;
|
||||
|
||||
/** `@UseValidationPipe()` stores its pipe on the handler ... */
|
||||
const handlerWhitelist = (handler: (...args: any[]) => any) =>
|
||||
whitelistOf(Reflect.getMetadata(PIPES_METADATA, handler));
|
||||
|
||||
/** ... while `@Body(..., new ValidationPipe())` stores it on the route argument. */
|
||||
const bodyWhitelist = (controller: any, method: string) =>
|
||||
whitelistOf(
|
||||
Object.values<any>(Reflect.getMetadata(ROUTE_ARGS_METADATA, controller, method) ?? {}).flatMap(
|
||||
(argument) => argument?.pipes ?? []
|
||||
)
|
||||
);
|
||||
|
||||
it('PUT /invoices/:id and PUT /timesheet/time-slot/:id', () => {
|
||||
expect(handlerWhitelist(InvoiceController.prototype.update)).toBe(true);
|
||||
expect(handlerWhitelist(TimeSlotController.prototype.update)).toBe(true);
|
||||
|
||||
// CONTROL: the same read on a route that does not whitelist, so the assertion discriminates.
|
||||
expect(handlerWhitelist(InvoiceController.prototype.create)).toBeUndefined();
|
||||
});
|
||||
|
||||
it('POST and PUT /timesheet/time-log', () => {
|
||||
expect(bodyWhitelist(TimeLogController, 'addManualTime')).toBe(true);
|
||||
expect(bodyWhitelist(TimeLogController, 'updateManualTime')).toBe(true);
|
||||
|
||||
// CONTROL: the pre-fix pipe of those two routes, read the same way.
|
||||
expect(whitelistOf([new ValidationPipe({ transform: true })])).toBeUndefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
+1
-1
@@ -18,6 +18,6 @@ export class IntegrationEntitySettingTiedUpdateHandler
|
||||
const { input, integrationId } = command;
|
||||
|
||||
await this._integrationTenantService.findOneByIdString(integrationId);
|
||||
return await this._integrationEntitySettingTiedService.bulkUpdateOrCreate(input);
|
||||
return await this._integrationEntitySettingTiedService.bulkUpdateOrCreate(integrationId, input);
|
||||
}
|
||||
}
|
||||
|
||||
+46
-7
@@ -1,6 +1,9 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { IIntegrationEntitySettingTied } from '@gauzy/contracts';
|
||||
import { ForbiddenException, Injectable } from '@nestjs/common';
|
||||
import { In } from 'typeorm';
|
||||
import { ID, IIntegrationEntitySettingTied } from '@gauzy/contracts';
|
||||
import { RequestContext } from './../core/context';
|
||||
import { TenantAwareCrudService } from './../core/crud';
|
||||
import { IntegrationEntitySetting } from './../integration-entity-setting/integration-entity-setting.entity';
|
||||
import { IntegrationEntitySettingTied } from './integration-entity-setting-tied.entity';
|
||||
import { MikroOrmIntegrationEntitySettingTiedRepository } from './repository/mikro-orm-integration-entity-setting-tied.repository';
|
||||
import { TypeOrmIntegrationEntitySettingTiedRepository } from './repository/type-orm-integration-entity-setting-tied.repository';
|
||||
@@ -17,20 +20,56 @@ export class IntegrationEntitySettingTiedService extends TenantAwareCrudService<
|
||||
/**
|
||||
* Create or update bulk integration entity settings tied entities by integration.
|
||||
*
|
||||
* Goes through the tenant-aware `saveMany()` (tenant stamping + foreign-id refusal) instead of a raw
|
||||
* `repository.save(body)`, which upserted any tenant's row by the id in the body
|
||||
* (GHSA-jh6m-9fxr-rx3c). The parent setting each item names must be a setting of the route's
|
||||
* integration in the caller's tenant.
|
||||
*
|
||||
* @param integrationId - The integration (from the route) the tied entities belong to.
|
||||
* @param input - The integration entity setting tied input data, either a single entity or an array of entities.
|
||||
* @returns A promise that resolves to an array of created or updated IIntegrationEntitySettingTied instances.
|
||||
*/
|
||||
async bulkUpdateOrCreate(
|
||||
integrationId: ID,
|
||||
input: IIntegrationEntitySettingTied | IIntegrationEntitySettingTied[]
|
||||
): Promise<IIntegrationEntitySettingTied[]> {
|
||||
// Ensure that the input is always an array for consistency
|
||||
const settings: IIntegrationEntitySettingTied[] = Array.isArray(input) ? input : [input];
|
||||
const settings: IIntegrationEntitySettingTied[] = (Array.isArray(input) ? input : [input]).map((setting) => {
|
||||
// Reduce the parent relation object to its id, so it is validated below and cannot cascade.
|
||||
const { integrationEntitySetting, ...rest } = setting ?? ({} as IIntegrationEntitySettingTied);
|
||||
const integrationEntitySettingId = rest.integrationEntitySettingId ?? integrationEntitySetting?.id;
|
||||
return { ...rest, ...(integrationEntitySettingId ? { integrationEntitySettingId } : {}) };
|
||||
});
|
||||
|
||||
await this.assertParentSettingsBelongToIntegration(settings, integrationId);
|
||||
|
||||
// Save the array of integration entity settings to the database
|
||||
const savedSettings: IIntegrationEntitySettingTied[] =
|
||||
await this.typeOrmIntegrationEntitySettingTiedRepository.save(settings);
|
||||
return await this.saveMany(settings);
|
||||
}
|
||||
|
||||
// Return the array of created or updated integration entity settings
|
||||
return savedSettings;
|
||||
/**
|
||||
* Refuses a tied entity whose parent setting is not a setting of the given integration in the
|
||||
* caller's tenant.
|
||||
*
|
||||
* @param settings - The tied entities about to be saved.
|
||||
* @param integrationId - The integration of the route.
|
||||
*/
|
||||
private async assertParentSettingsBelongToIntegration(
|
||||
settings: IIntegrationEntitySettingTied[],
|
||||
integrationId: ID
|
||||
): Promise<void> {
|
||||
const parentIds = [...new Set(settings.map((setting) => setting.integrationEntitySettingId).filter(Boolean))];
|
||||
if (!parentIds.length) {
|
||||
return;
|
||||
}
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
const count = tenantId
|
||||
? await this.typeOrmRepository.manager.count(IntegrationEntitySetting, {
|
||||
where: { id: In(parentIds), integrationId, tenantId }
|
||||
})
|
||||
: 0;
|
||||
if (count !== parentIds.length) {
|
||||
throw new ForbiddenException('The integration entity setting does not belong to this integration');
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
+1
-1
@@ -22,6 +22,6 @@ export class IntegrationEntitySettingUpdateOrCreateHandler implements ICommandHa
|
||||
const { input, integrationId } = command;
|
||||
|
||||
await this._integrationTenantService.findOneByIdString(integrationId);
|
||||
return await this._integrationEntitySettingService.bulkUpdateOrCreate(input);
|
||||
return await this._integrationEntitySettingService.bulkUpdateOrCreate(integrationId, input);
|
||||
}
|
||||
}
|
||||
|
||||
+98
@@ -0,0 +1,98 @@
|
||||
// Must stay first: loads the entity graph before the services pull an entity (see activity.controller.spec.ts).
|
||||
import '../core/entities/internal';
|
||||
|
||||
import { ForbiddenException } from '@nestjs/common';
|
||||
import { RequestContext } from '../core/context';
|
||||
import { TenantAwareCrudService } from '../core/crud/tenant-aware-crud.service';
|
||||
import { IntegrationEntitySettingTiedService } from '../integration-entity-setting-tied/integration-entity-setting-tied.service';
|
||||
import { IntegrationEntitySettingService } from './integration-entity-setting.service';
|
||||
|
||||
/**
|
||||
* GHSA-jh6m-9fxr-rx3c — PUT /integration-entity-setting(-tied)/integration/:id.
|
||||
*
|
||||
* Both handlers only checked that the PATH integration was the caller's, then handed the raw body to
|
||||
* `typeOrmRepository.save()`: no tenant stamping, no foreign-id check, and the body could name any
|
||||
* integration. The services now go through TenantAwareCrudService.saveMany (tenant stamp + root and
|
||||
* nested foreign-id checks) and pin every item to the route's integration.
|
||||
*
|
||||
* CONTROL: the raw repository `save` is a spy that must never be called any more — it is exactly what
|
||||
* the pre-fix services called with the body.
|
||||
*/
|
||||
|
||||
const TENANT_A = 'tenant-a';
|
||||
|
||||
describe('integration entity settings bulk upsert (GHSA-jh6m-9fxr-rx3c)', () => {
|
||||
let saveMany: jest.SpyInstance;
|
||||
|
||||
beforeEach(() => {
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT_A);
|
||||
saveMany = jest
|
||||
.spyOn(TenantAwareCrudService.prototype, 'saveMany')
|
||||
.mockImplementation(async (entities: any[]) => entities);
|
||||
});
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
describe('IntegrationEntitySettingService', () => {
|
||||
it('pins every item to the route integration and saves through the tenant-aware saveMany', async () => {
|
||||
const repository = { save: jest.fn() };
|
||||
const service = new IntegrationEntitySettingService(repository as any, {} as any);
|
||||
|
||||
await service.bulkUpdateOrCreate('integration-a', [
|
||||
{ id: 'setting-1', entity: 'Project', sync: true, integrationId: 'integration-other' },
|
||||
{ entity: 'Task', sync: false, integration: { id: 'integration-other' } }
|
||||
] as any);
|
||||
|
||||
expect(repository.save).not.toHaveBeenCalled();
|
||||
const saved = saveMany.mock.calls[0][0];
|
||||
expect(saved.map((item: any) => item.integrationId)).toEqual(['integration-a', 'integration-a']);
|
||||
expect(saved.every((item: any) => !('integration' in item))).toBe(true);
|
||||
expect(saved[0]).toMatchObject({ id: 'setting-1', entity: 'Project', sync: true });
|
||||
});
|
||||
|
||||
it('accepts a single object as before', async () => {
|
||||
const service = new IntegrationEntitySettingService({ save: jest.fn() } as any, {} as any);
|
||||
|
||||
await service.bulkUpdateOrCreate('integration-a', { entity: 'Project', sync: true } as any);
|
||||
|
||||
expect(saveMany.mock.calls[0][0]).toEqual([{ entity: 'Project', sync: true, integrationId: 'integration-a' }]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('IntegrationEntitySettingTiedService', () => {
|
||||
const createTiedService = (parentsInIntegration: number) => {
|
||||
const count = jest.fn(async (..._args: any[]) => parentsInIntegration);
|
||||
const repository = { save: jest.fn(), manager: { count } };
|
||||
const service = new IntegrationEntitySettingTiedService(repository as any, {} as any);
|
||||
return { service, repository, count };
|
||||
};
|
||||
|
||||
it("refuses a parent setting that is not one of the route integration's settings", async () => {
|
||||
const { service, repository, count } = createTiedService(0);
|
||||
|
||||
await expect(
|
||||
service.bulkUpdateOrCreate('integration-a', [
|
||||
{ entity: 'Label', sync: true, integrationEntitySettingId: 'setting-of-another-integration' }
|
||||
] as any)
|
||||
).rejects.toThrow(ForbiddenException);
|
||||
|
||||
expect(count.mock.calls[0][1]).toMatchObject({
|
||||
where: { integrationId: 'integration-a', tenantId: TENANT_A }
|
||||
});
|
||||
expect(repository.save).not.toHaveBeenCalled();
|
||||
expect(saveMany).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('saves through the tenant-aware saveMany when every parent belongs to the integration', async () => {
|
||||
const { service, repository } = createTiedService(1);
|
||||
|
||||
await service.bulkUpdateOrCreate('integration-a', [
|
||||
{ entity: 'Label', sync: true, integrationEntitySetting: { id: 'setting-1', entity: 'Issue' } }
|
||||
] as any);
|
||||
|
||||
expect(repository.save).not.toHaveBeenCalled();
|
||||
expect(saveMany.mock.calls[0][0]).toEqual([
|
||||
{ entity: 'Label', sync: true, integrationEntitySettingId: 'setting-1' }
|
||||
]);
|
||||
});
|
||||
});
|
||||
});
|
||||
+13
-2
@@ -35,16 +35,27 @@ export class IntegrationEntitySettingService extends TenantAwareCrudService<Inte
|
||||
/**
|
||||
* Create or update integration entity settings in bulk by integration.
|
||||
*
|
||||
* Goes through the tenant-aware `saveMany()`, which stamps the caller's tenant and refuses ids
|
||||
* (and nested tied entities) of another tenant. The raw `repository.save(body)` it replaces upserted
|
||||
* any tenant's setting by the id in the body (GHSA-jh6m-9fxr-rx3c). Every item is pinned to the
|
||||
* integration of the route, which the caller has already been checked against.
|
||||
*
|
||||
* @param integrationId - The integration (from the route) the settings belong to.
|
||||
* @param input - An individual IIntegrationEntitySetting or an array of IIntegrationEntitySetting objects to be created or updated.
|
||||
* @returns A promise resolving to an array of created or updated IIntegrationEntitySetting objects.
|
||||
*/
|
||||
async bulkUpdateOrCreate(
|
||||
integrationId: ID,
|
||||
input: IIntegrationEntitySetting | IIntegrationEntitySetting[]
|
||||
): Promise<IIntegrationEntitySetting[]> {
|
||||
// Prepare an array of settings to be saved
|
||||
const settings: IIntegrationEntitySetting[] = Array.isArray(input) ? input : [input];
|
||||
const settings: IIntegrationEntitySetting[] = (Array.isArray(input) ? input : [input]).map((setting) => {
|
||||
// The relation object would take precedence over the pinned integrationId.
|
||||
const { integration, ...rest } = setting ?? ({} as IIntegrationEntitySetting);
|
||||
return { ...rest, integrationId };
|
||||
});
|
||||
|
||||
// Save the new settings to the database
|
||||
return await this.typeOrmIntegrationEntitySettingRepository.save(settings);
|
||||
return await this.saveMany(settings);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,9 +1,19 @@
|
||||
import { IInvoice, IUser } from '@gauzy/contracts';
|
||||
import { ApiProperty } from '@nestjs/swagger';
|
||||
import { IsNotEmpty, IsObject, IsOptional, IsString } from 'class-validator';
|
||||
import { ID, IInvoice, IUser } from '@gauzy/contracts';
|
||||
import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger';
|
||||
import { IsNotEmpty, IsObject, IsOptional, IsString, IsUUID } from 'class-validator';
|
||||
import { TenantOrganizationBaseDTO } from '../../core/dto';
|
||||
|
||||
export abstract class InvoiceEstimateHistoryDTO extends TenantOrganizationBaseDTO {
|
||||
/**
|
||||
* An existing history record sent back with its invoice. Declared so a whitelisted invoice update
|
||||
* keeps it linked instead of inserting a copy; ownership is checked by TenantAwareCrudService's
|
||||
* nested-graph check.
|
||||
*/
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsUUID()
|
||||
readonly id?: ID;
|
||||
|
||||
@ApiProperty({ type: () => String, readOnly: true })
|
||||
@IsNotEmpty()
|
||||
@IsString()
|
||||
|
||||
@@ -1,8 +1,19 @@
|
||||
import { ApiProperty } from '@nestjs/swagger';
|
||||
import { IsOptional, IsString, IsNumber, IsNotEmpty, IsBoolean } from 'class-validator';
|
||||
import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger';
|
||||
import { IsOptional, IsString, IsNumber, IsNotEmpty, IsBoolean, IsUUID } from 'class-validator';
|
||||
import { ID } from '@gauzy/contracts';
|
||||
import { TenantOrganizationBaseDTO } from '../../core/dto';
|
||||
|
||||
export abstract class InvoiceItemDTO extends TenantOrganizationBaseDTO {
|
||||
/**
|
||||
* An existing item edited in place through the invoice. Declared so a whitelisted invoice update
|
||||
* keeps it (a stripped id turns the edit into an insert and unlinks the original item). Whose item
|
||||
* it names is checked by TenantAwareCrudService's nested-graph ownership check.
|
||||
*/
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsUUID()
|
||||
readonly id?: ID;
|
||||
|
||||
@ApiProperty({ type: () => String, readOnly: true })
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
|
||||
@@ -1,13 +1,25 @@
|
||||
import { IInvoiceUpdateInput } from "@gauzy/contracts";
|
||||
import { IInvoiceUpdateInput, InvoiceTypeEnum } from "@gauzy/contracts";
|
||||
import { IntersectionType } from "@nestjs/mapped-types";
|
||||
import { ApiPropertyOptional } from "@nestjs/swagger";
|
||||
import { IsEnum, IsOptional } from "class-validator";
|
||||
import { RelationalTagDTO } from "./../../tags/dto";
|
||||
import { DiscountInvoiceDTO } from "./discount-invoice.dto";
|
||||
import { InvoiceDTO } from "./invoice.dto";
|
||||
import { TaxInvoiceDTO } from "./tax-invoice.dto";
|
||||
|
||||
/**
|
||||
* PUT /invoices/:id runs with `whitelist: true` (GHSA-jh6m-9fxr-rx3c), so every field the invoice
|
||||
* edit screen sends has to be declared here or on the DTOs it extends.
|
||||
*/
|
||||
export class UpdateInvoiceDTO extends IntersectionType(
|
||||
InvoiceDTO,
|
||||
TaxInvoiceDTO,
|
||||
RelationalTagDTO,
|
||||
DiscountInvoiceDTO
|
||||
) implements IInvoiceUpdateInput {}
|
||||
) implements IInvoiceUpdateInput {
|
||||
|
||||
@ApiPropertyOptional({ type: () => String, enum: InvoiceTypeEnum, readOnly: true })
|
||||
@IsOptional()
|
||||
@IsEnum(InvoiceTypeEnum)
|
||||
readonly invoiceType?: InvoiceTypeEnum;
|
||||
}
|
||||
|
||||
@@ -162,7 +162,7 @@ export class InvoiceController extends CrudController<Invoice> {
|
||||
})
|
||||
@HttpCode(HttpStatus.ACCEPTED)
|
||||
@Put(':id')
|
||||
@UseValidationPipe({ transform: true })
|
||||
@UseValidationPipe({ transform: true, whitelist: true })
|
||||
async update(
|
||||
@Param('id', UUIDValidationPipe) id: IInvoice['id'],
|
||||
@Body() entity: UpdateInvoiceDTO
|
||||
|
||||
@@ -0,0 +1,120 @@
|
||||
// Must stay first: loads the entity graph before the service pulls an entity (see activity.controller.spec.ts).
|
||||
import '../core/entities/internal';
|
||||
|
||||
import { FindOperator, In } from 'typeorm';
|
||||
import { RequestContext } from '../core/context';
|
||||
import { CrudService } from '../core/crud/crud.service';
|
||||
import { MultiORMEnum } from '../core/utils';
|
||||
import { RequestApprovalService } from './request-approval.service';
|
||||
|
||||
/**
|
||||
* GHSA-gwpq-mmw7-vx85 sibling — POST / PUT /request-approval resolved the body's approver employees
|
||||
* and teams through raw repositories with no tenant predicate, attached the loaded rows, and echoed
|
||||
* them back: a foreign-tenant employee or team was read and linked by its UUID.
|
||||
*
|
||||
* The fake repositories evaluate the where clause against fixtures. CONTROL arms replay the pre-fix
|
||||
* `{ id: In(ids) }` lookups and show the foreign rows coming back.
|
||||
*/
|
||||
|
||||
const TENANT_A = 'tenant-a';
|
||||
const TENANT_B = 'tenant-b';
|
||||
|
||||
const EMPLOYEES = [
|
||||
{ id: 'employee-own', tenantId: TENANT_A },
|
||||
{ id: 'employee-foreign', tenantId: TENANT_B }
|
||||
];
|
||||
const TEAMS = [
|
||||
{ id: 'team-own', tenantId: TENANT_A },
|
||||
{ id: 'team-foreign', tenantId: TENANT_B }
|
||||
];
|
||||
|
||||
const matches = (row: Record<string, any>, where: Record<string, any>) =>
|
||||
Object.entries(where).every(([key, value]) =>
|
||||
value === undefined ? true : value instanceof FindOperator ? (value.value as any[]).includes(row[key]) : row[key] === value
|
||||
);
|
||||
|
||||
const fakeRepository = (rows: any[]) => ({
|
||||
find: jest.fn(async ({ where }: any) => rows.filter((row) => matches(row, where)))
|
||||
});
|
||||
|
||||
function createService() {
|
||||
const employees = fakeRepository(EMPLOYEES);
|
||||
const teams = fakeRepository(TEAMS);
|
||||
// updateRequestApproval clears the previous approver rows through a query builder first.
|
||||
const requestApprovals = {
|
||||
createQueryBuilder: () => {
|
||||
const builder: any = {
|
||||
delete: () => builder,
|
||||
from: () => builder,
|
||||
where: () => builder,
|
||||
execute: async () => ({ affected: 0 })
|
||||
};
|
||||
return builder;
|
||||
}
|
||||
};
|
||||
const service = new RequestApprovalService(
|
||||
requestApprovals as any,
|
||||
{} as any,
|
||||
employees as any,
|
||||
{} as any,
|
||||
teams as any,
|
||||
{} as any
|
||||
);
|
||||
jest.spyOn(service, 'save').mockImplementation(async (entity: any) => entity);
|
||||
return { service, employees, teams };
|
||||
}
|
||||
|
||||
describe('RequestApprovalService approver lookups (GHSA-gwpq-mmw7-vx85 sibling)', () => {
|
||||
beforeEach(() => {
|
||||
jest.spyOn(CrudService.prototype, 'ormType', 'get').mockReturnValue(MultiORMEnum.TypeORM);
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT_A);
|
||||
});
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
const input: any = {
|
||||
name: 'approval',
|
||||
organizationId: 'org-a',
|
||||
employeeApprovals: ['employee-own', 'employee-foreign'],
|
||||
teams: ['team-own', 'team-foreign']
|
||||
};
|
||||
|
||||
it('CONTROL: the pre-fix lookups return the foreign employee and team', async () => {
|
||||
const { employees, teams } = createService();
|
||||
|
||||
expect((await employees.find({ where: { id: In(input.employeeApprovals) } })).map((e) => e.id)).toContain(
|
||||
'employee-foreign'
|
||||
);
|
||||
expect((await teams.find({ where: { id: In(input.teams) } })).map((t) => t.id)).toContain('team-foreign');
|
||||
});
|
||||
|
||||
it('links only approvers of the caller tenant on create', async () => {
|
||||
const { service } = createService();
|
||||
|
||||
const created: any = await service.createRequestApproval(input);
|
||||
|
||||
expect(created.employeeApprovals.map((row: any) => row.employeeId)).toEqual(['employee-own']);
|
||||
expect(created.teamApprovals.map((row: any) => row.teamId)).toEqual(['team-own']);
|
||||
expect(JSON.stringify(created)).not.toContain(TENANT_B);
|
||||
});
|
||||
|
||||
it('links only approvers of the caller tenant on update', async () => {
|
||||
const { service } = createService();
|
||||
jest.spyOn(service, 'findOneByIdString').mockResolvedValue({ id: 'approval-1', tenantId: TENANT_A } as any);
|
||||
|
||||
const updated: any = await service.updateRequestApproval('approval-1', input);
|
||||
|
||||
expect(updated.employeeApprovals.map((row: any) => row.employeeId)).toEqual(['employee-own']);
|
||||
expect(updated.teamApprovals.map((row: any) => row.teamId)).toEqual(['team-own']);
|
||||
expect(JSON.stringify(updated)).not.toContain(TENANT_B);
|
||||
});
|
||||
|
||||
it('matches nothing without a tenant', async () => {
|
||||
const { service, employees } = createService();
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(null);
|
||||
|
||||
const created: any = await service.createRequestApproval(input);
|
||||
|
||||
expect(created.employeeApprovals).toEqual([]);
|
||||
expect(employees.find).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -248,6 +248,49 @@ export class RequestApprovalService extends TenantAwareCrudService<RequestApprov
|
||||
return { items: result, total: result.length };
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves the approver employees named by a request approval, inside the caller's tenant only.
|
||||
*
|
||||
* The ids come from the request body and the raw repositories carry no tenant scoping, so an
|
||||
* unscoped lookup attached another tenant's employee (and echoed it back in the response)
|
||||
* (GHSA-gwpq-mmw7-vx85 sibling). Ids of other tenants are ignored; no tenant means no match.
|
||||
*
|
||||
* @param ids - The employee ids from the request.
|
||||
* @param tenantId - The caller's tenant.
|
||||
*/
|
||||
private async findEmployeesInTenant(ids: ID[], tenantId: ID): Promise<IEmployee[]> {
|
||||
if (!tenantId || !Array.isArray(ids) || !ids.length) {
|
||||
return [];
|
||||
}
|
||||
switch (this.ormType) {
|
||||
case MultiORMEnum.MikroORM:
|
||||
return await this.mikroOrmEmployeeRepository.find({ id: { $in: ids }, tenantId } as any);
|
||||
case MultiORMEnum.TypeORM:
|
||||
default:
|
||||
return await this.typeOrmEmployeeRepository.find({ where: { id: In(ids), tenantId } });
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves the approver teams named by a request approval, inside the caller's tenant only.
|
||||
* See {@link findEmployeesInTenant}.
|
||||
*
|
||||
* @param ids - The team ids from the request.
|
||||
* @param tenantId - The caller's tenant.
|
||||
*/
|
||||
private async findTeamsInTenant(ids: ID[], tenantId: ID): Promise<IOrganizationTeam[]> {
|
||||
if (!tenantId || !Array.isArray(ids) || !ids.length) {
|
||||
return [];
|
||||
}
|
||||
switch (this.ormType) {
|
||||
case MultiORMEnum.MikroORM:
|
||||
return await this.mikroOrmOrganizationTeamRepository.find({ id: { $in: ids }, tenantId } as any);
|
||||
case MultiORMEnum.TypeORM:
|
||||
default:
|
||||
return await this.typeOrmOrganizationTeamRepository.find({ where: { id: In(ids), tenantId } });
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a RequestApproval record.
|
||||
*
|
||||
@@ -268,20 +311,7 @@ export class RequestApprovalService extends TenantAwareCrudService<RequestApprov
|
||||
requestApproval.tenantId = tenantId;
|
||||
|
||||
if (entity.employeeApprovals?.length) {
|
||||
let employees: IEmployee[];
|
||||
switch (this.ormType) {
|
||||
case MultiORMEnum.MikroORM:
|
||||
employees = await this.mikroOrmEmployeeRepository.find({
|
||||
id: { $in: entity.employeeApprovals as any }
|
||||
});
|
||||
break;
|
||||
case MultiORMEnum.TypeORM:
|
||||
default:
|
||||
employees = await this.typeOrmEmployeeRepository.find({
|
||||
where: { id: In(entity.employeeApprovals as any) }
|
||||
});
|
||||
break;
|
||||
}
|
||||
const employees = await this.findEmployeesInTenant(entity.employeeApprovals as unknown as ID[], tenantId);
|
||||
|
||||
requestApproval.employeeApprovals = employees.map((employee) => {
|
||||
const requestApprovalEmployee = new RequestApprovalEmployee();
|
||||
@@ -294,18 +324,7 @@ export class RequestApprovalService extends TenantAwareCrudService<RequestApprov
|
||||
}
|
||||
|
||||
if (entity.teams?.length) {
|
||||
let teams: IOrganizationTeam[];
|
||||
switch (this.ormType) {
|
||||
case MultiORMEnum.MikroORM:
|
||||
teams = await this.mikroOrmOrganizationTeamRepository.find({ id: { $in: entity.teams as any } });
|
||||
break;
|
||||
case MultiORMEnum.TypeORM:
|
||||
default:
|
||||
teams = await this.typeOrmOrganizationTeamRepository.find({
|
||||
where: { id: In(entity.teams as any) }
|
||||
});
|
||||
break;
|
||||
}
|
||||
const teams = await this.findTeamsInTenant(entity.teams as unknown as ID[], tenantId);
|
||||
|
||||
requestApproval.teamApprovals = teams.map((team) => {
|
||||
const requestApprovalTeam = new RequestApprovalTeam();
|
||||
@@ -360,22 +379,7 @@ export class RequestApprovalService extends TenantAwareCrudService<RequestApprov
|
||||
}
|
||||
|
||||
if (entity.employeeApprovals) {
|
||||
let employees: IEmployee[];
|
||||
switch (this.ormType) {
|
||||
case MultiORMEnum.MikroORM:
|
||||
employees = await this.mikroOrmEmployeeRepository.find({
|
||||
id: { $in: entity.employeeApprovals as any }
|
||||
});
|
||||
break;
|
||||
case MultiORMEnum.TypeORM:
|
||||
default:
|
||||
employees = await this.typeOrmEmployeeRepository.find({
|
||||
where: {
|
||||
id: In(entity.employeeApprovals as any)
|
||||
}
|
||||
});
|
||||
break;
|
||||
}
|
||||
const employees = await this.findEmployeesInTenant(entity.employeeApprovals as unknown as ID[], tenantId);
|
||||
const requestApprovalEmployees: IRequestApprovalEmployee[] = [];
|
||||
employees.forEach((employee) => {
|
||||
const raEmployees = new RequestApprovalEmployee();
|
||||
@@ -390,20 +394,7 @@ export class RequestApprovalService extends TenantAwareCrudService<RequestApprov
|
||||
}
|
||||
|
||||
if (entity.teams) {
|
||||
let teams: IOrganizationTeam[];
|
||||
switch (this.ormType) {
|
||||
case MultiORMEnum.MikroORM:
|
||||
teams = await this.mikroOrmOrganizationTeamRepository.find({ id: { $in: entity.teams as any } });
|
||||
break;
|
||||
case MultiORMEnum.TypeORM:
|
||||
default:
|
||||
teams = await this.typeOrmOrganizationTeamRepository.find({
|
||||
where: {
|
||||
id: In(entity.teams as any)
|
||||
}
|
||||
});
|
||||
break;
|
||||
}
|
||||
const teams = await this.findTeamsInTenant(entity.teams as unknown as ID[], tenantId);
|
||||
const requestApprovalTeams: IRequestApprovalTeam[] = [];
|
||||
teams.forEach((team) => {
|
||||
const raTeam = new RequestApprovalTeam();
|
||||
|
||||
@@ -580,6 +580,92 @@ describe('OrganizationPermissionGuard', () => {
|
||||
}
|
||||
});
|
||||
|
||||
it('denies a tenant-less SUPER_ADMIN even on a route with no policy target', async () => {
|
||||
// The exemption is granted before any tenant-scoped lookup, so it must not be granted to a
|
||||
// request whose tenant cannot be resolved at all (nothing downstream can scope such a call).
|
||||
const previous = (env as any).allowSuperAdminRole;
|
||||
const { guard, createQueryBuilder } = createGuard();
|
||||
asCaller({ role: RolesEnum.SUPER_ADMIN, employeeId: null, isSuperAdmin: true, tenantId: null });
|
||||
(env as any).allowSuperAdminRole = true;
|
||||
|
||||
try {
|
||||
const context = createContext([PermissionsEnum.ALLOW_MANUAL_TIME], {
|
||||
body: { organizationId: 'org-allow' }
|
||||
});
|
||||
|
||||
await expect(guard.canActivate(context)).resolves.toBe(false);
|
||||
expect(createQueryBuilder).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
(env as any).allowSuperAdminRole = previous;
|
||||
}
|
||||
});
|
||||
|
||||
// GHSA-6qvm-3wg4-26w4: every tenant owner is a SUPER_ADMIN. The early return used to skip the
|
||||
// tenant-scoped target lookup, which is the only ownership check on PUT /timesheet/time-slot/:id.
|
||||
// CONTROL: with the pre-fix `return true` restored, the foreign-record arm resolves to `true`.
|
||||
describe('on a route that addresses a record by id', () => {
|
||||
const target = { entity: TimeLogStub, param: 'id' };
|
||||
|
||||
const asExemptSuperAdmin = () => {
|
||||
asCaller({ role: RolesEnum.SUPER_ADMIN, employeeId: null, isSuperAdmin: true });
|
||||
(env as any).allowSuperAdminRole = true;
|
||||
};
|
||||
|
||||
let previous: unknown;
|
||||
beforeEach(() => {
|
||||
previous = (env as any).allowSuperAdminRole;
|
||||
});
|
||||
afterEach(() => {
|
||||
(env as any).allowSuperAdminRole = previous;
|
||||
});
|
||||
|
||||
it('denies a record of another tenant', async () => {
|
||||
const { guard, findTarget } = createGuard();
|
||||
asExemptSuperAdmin();
|
||||
|
||||
const context = createContext(
|
||||
[PermissionsEnum.ALLOW_MODIFY_TIME],
|
||||
{ params: { id: 'log-foreign' }, body: {} },
|
||||
target
|
||||
);
|
||||
|
||||
await expect(guard.canActivate(context)).resolves.toBe(false);
|
||||
expect(findTarget).toHaveBeenCalledWith(TimeLogStub, {
|
||||
where: { id: 'log-foreign', tenantId: TENANT_ID },
|
||||
select: { id: true, organizationId: true }
|
||||
});
|
||||
});
|
||||
|
||||
it('still exempts a record of its own tenant from the organization policy', async () => {
|
||||
const { guard, createQueryBuilder } = createGuard();
|
||||
asExemptSuperAdmin();
|
||||
|
||||
// The record's organization has allowModifyTime off: the policy stays exempt.
|
||||
const context = createContext(
|
||||
[PermissionsEnum.ALLOW_MODIFY_TIME],
|
||||
{ params: { id: 'log-in-deny' }, body: {} },
|
||||
target
|
||||
);
|
||||
|
||||
await expect(guard.canActivate(context)).resolves.toBe(true);
|
||||
expect(createQueryBuilder).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('denies when the request has no tenant', async () => {
|
||||
const { guard } = createGuard();
|
||||
asCaller({ role: RolesEnum.SUPER_ADMIN, employeeId: null, isSuperAdmin: true, tenantId: null });
|
||||
(env as any).allowSuperAdminRole = true;
|
||||
|
||||
const context = createContext(
|
||||
[PermissionsEnum.ALLOW_MODIFY_TIME],
|
||||
{ params: { id: 'log-in-allow' }, body: {} },
|
||||
target
|
||||
);
|
||||
|
||||
await expect(guard.canActivate(context)).resolves.toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
it('enforces the policy for SUPER_ADMIN when allowSuperAdminRole is off', async () => {
|
||||
const previous = (env as any).allowSuperAdminRole;
|
||||
const { guard } = createGuard();
|
||||
|
||||
@@ -119,7 +119,11 @@ export class OrganizationPermissionGuard implements CanActivate {
|
||||
|
||||
// Check if super admin role is allowed from the .env file
|
||||
if (env.allowSuperAdminRole && RequestContext.hasRoles([RolesEnum.SUPER_ADMIN])) {
|
||||
return true;
|
||||
// The exemption covers the organization POLICY only. On a route that addresses a record by
|
||||
// id, the tenant-scoped target lookup is also the ownership check: every tenant owner is a
|
||||
// SUPER_ADMIN, so returning early here let one reach another tenant's record by its UUID
|
||||
// (GHSA-6qvm-3wg4-26w4). The record still has to exist in the caller's tenant.
|
||||
return await this.superAdminTargetIsInTenant(context);
|
||||
}
|
||||
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
@@ -162,6 +166,40 @@ export class OrganizationPermissionGuard implements CanActivate {
|
||||
return isAuthorized;
|
||||
}
|
||||
|
||||
/**
|
||||
* For an exempt SUPER_ADMIN: allows the request unless the route declares an
|
||||
* `@OrganizationPolicyTarget()` whose record is missing from the caller's tenant.
|
||||
*
|
||||
* @param context The execution context.
|
||||
* @returns true when the route has no policy target, or its record belongs to the caller's tenant.
|
||||
*/
|
||||
private async superAdminTargetIsInTenant(context: ExecutionContext): Promise<boolean> {
|
||||
// The tenant is resolved BEFORE the no-target shortcut: an exemption granted without one would
|
||||
// hand the route to a caller whose tenant scoping cannot be evaluated anywhere downstream.
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
|
||||
if (isEmpty(tenantId)) {
|
||||
console.log('OrganizationPermissionGuard: no tenant on the request, access denied');
|
||||
return false;
|
||||
}
|
||||
|
||||
const target = this._reflector.get<IOrganizationPolicyTarget | undefined>(
|
||||
ORGANIZATION_POLICY_TARGET_METADATA,
|
||||
context.getHandler()
|
||||
);
|
||||
|
||||
if (!target) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (!(await this.findTargetOrganizationId(context, tenantId, target))) {
|
||||
console.log('OrganizationPermissionGuard: the target record is not in the caller tenant, access denied');
|
||||
return false;
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Works out which organizations must allow the requested action.
|
||||
*
|
||||
|
||||
@@ -0,0 +1,187 @@
|
||||
import { DataSource, EntitySchema, Repository } from 'typeorm';
|
||||
import { scopeActivitiesForWrite } from './activity-write-scope.helper';
|
||||
|
||||
/**
|
||||
* GHSA-6qvm-3wg4-26w4 — body-supplied activity ids reached `repository.save()`, which upserts by
|
||||
* primary key alone. Real better-sqlite3 database; the CONTROL arm saves the same body the pre-fix
|
||||
* way and shows another tenant's activity being overwritten and moved.
|
||||
*/
|
||||
|
||||
const TENANT_A = 'tenant-a';
|
||||
const TENANT_B = 'tenant-b';
|
||||
|
||||
const TimeSlotSchema = new EntitySchema<any>({
|
||||
name: 'TimeSlot',
|
||||
tableName: 'time_slot',
|
||||
columns: {
|
||||
id: { primary: true, type: 'varchar', generated: 'uuid' },
|
||||
tenantId: { type: 'varchar', nullable: true },
|
||||
employeeId: { type: 'varchar', nullable: true }
|
||||
}
|
||||
});
|
||||
|
||||
const ProjectSchema = new EntitySchema<any>({
|
||||
name: 'OrganizationProject',
|
||||
tableName: 'organization_project',
|
||||
columns: {
|
||||
id: { primary: true, type: 'varchar', generated: 'uuid' },
|
||||
tenantId: { type: 'varchar', nullable: true },
|
||||
name: { type: 'varchar', nullable: true }
|
||||
}
|
||||
});
|
||||
|
||||
const TaskSchema = new EntitySchema<any>({
|
||||
name: 'Task',
|
||||
tableName: 'task',
|
||||
columns: {
|
||||
id: { primary: true, type: 'varchar', generated: 'uuid' },
|
||||
tenantId: { type: 'varchar', nullable: true },
|
||||
title: { type: 'varchar', nullable: true }
|
||||
}
|
||||
});
|
||||
|
||||
const ActivitySchema = new EntitySchema<any>({
|
||||
name: 'Activity',
|
||||
tableName: 'activity',
|
||||
columns: {
|
||||
id: { primary: true, type: 'varchar', generated: 'uuid' },
|
||||
tenantId: { type: 'varchar', nullable: true },
|
||||
employeeId: { type: 'varchar', nullable: true },
|
||||
title: { type: 'varchar', nullable: true },
|
||||
timeSlotId: { type: 'varchar', nullable: true },
|
||||
projectId: { type: 'varchar', nullable: true },
|
||||
taskId: { type: 'varchar', nullable: true },
|
||||
deletedAt: { type: 'datetime', nullable: true, deleteDate: true }
|
||||
},
|
||||
relations: {
|
||||
timeSlot: { type: 'many-to-one', target: 'TimeSlot', joinColumn: { name: 'timeSlotId' } },
|
||||
project: { type: 'many-to-one', target: 'OrganizationProject', joinColumn: { name: 'projectId' } },
|
||||
task: { type: 'many-to-one', target: 'Task', joinColumn: { name: 'taskId' } }
|
||||
}
|
||||
});
|
||||
|
||||
describe('scopeActivitiesForWrite (GHSA-6qvm-3wg4-26w4)', () => {
|
||||
let dataSource: DataSource;
|
||||
let activities: Repository<any>;
|
||||
let slots: Repository<any>;
|
||||
let foreignActivity: any;
|
||||
let ownActivity: any;
|
||||
let ownSlot: any;
|
||||
let foreignSlot: any;
|
||||
let ownProject: any;
|
||||
let foreignProject: any;
|
||||
let foreignTask: any;
|
||||
|
||||
const SCOPE = { tenantId: TENANT_A, employeeId: 'employee-a' };
|
||||
|
||||
beforeEach(async () => {
|
||||
dataSource = new DataSource({
|
||||
type: 'better-sqlite3',
|
||||
database: ':memory:',
|
||||
entities: [TimeSlotSchema, ProjectSchema, TaskSchema, ActivitySchema],
|
||||
synchronize: true,
|
||||
logging: false
|
||||
});
|
||||
await dataSource.initialize();
|
||||
activities = dataSource.getRepository('Activity');
|
||||
slots = dataSource.getRepository('TimeSlot');
|
||||
ownSlot = await slots.save({ tenantId: TENANT_A, employeeId: 'employee-a' });
|
||||
foreignSlot = await slots.save({ tenantId: TENANT_B, employeeId: 'employee-b' });
|
||||
ownActivity = await activities.save({ tenantId: TENANT_A, employeeId: 'employee-a', title: 'own' });
|
||||
foreignActivity = await activities.save({ tenantId: TENANT_B, employeeId: 'employee-b', title: 'theirs' });
|
||||
ownProject = await dataSource.getRepository('OrganizationProject').save({ tenantId: TENANT_A, name: 'ours' });
|
||||
foreignProject = await dataSource.getRepository('OrganizationProject').save({ tenantId: TENANT_B, name: 'theirs' });
|
||||
foreignTask = await dataSource.getRepository('Task').save({ tenantId: TENANT_B, title: 'theirs' });
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await dataSource.destroy();
|
||||
});
|
||||
|
||||
it('CONTROL: saving the body as-is overwrites and re-tenants the foreign activity', async () => {
|
||||
await activities.save([{ id: foreignActivity.id, title: 'stolen', ...SCOPE }]);
|
||||
|
||||
expect(await activities.findOneBy({ id: foreignActivity.id })).toMatchObject({
|
||||
title: 'stolen',
|
||||
tenantId: TENANT_A
|
||||
});
|
||||
});
|
||||
|
||||
it('drops the foreign id, so the foreign activity is left alone and a new row is inserted', async () => {
|
||||
const body = [{ id: foreignActivity.id, title: 'stolen' }];
|
||||
|
||||
const scoped = await scopeActivitiesForWrite(body as any[], activities, SCOPE);
|
||||
await activities.save(scoped.map((activity) => ({ ...activity, ...SCOPE })));
|
||||
|
||||
expect(await activities.findOneBy({ id: foreignActivity.id })).toMatchObject({
|
||||
title: 'theirs',
|
||||
tenantId: TENANT_B
|
||||
});
|
||||
expect(await activities.countBy({ title: 'stolen', tenantId: TENANT_A })).toBe(1);
|
||||
});
|
||||
|
||||
it("keeps the employee's own id (a re-sent activity still updates in place), soft-deleted ones included", async () => {
|
||||
await activities.softDelete({ id: ownActivity.id });
|
||||
|
||||
const [scoped] = await scopeActivitiesForWrite([{ id: ownActivity.id, title: 'again' }] as any[], activities, SCOPE);
|
||||
|
||||
expect(scoped.id).toBe(ownActivity.id);
|
||||
});
|
||||
|
||||
it("keeps an own time slot link and drops a foreign one", async () => {
|
||||
const scoped = await scopeActivitiesForWrite(
|
||||
[{ timeSlotId: ownSlot.id }, { timeSlotId: foreignSlot.id }] as any[],
|
||||
activities,
|
||||
SCOPE
|
||||
);
|
||||
|
||||
expect(scoped.map((activity) => activity.timeSlotId)).toEqual([ownSlot.id, undefined]);
|
||||
});
|
||||
|
||||
it('strips relation objects that would override the forced scope', async () => {
|
||||
const [scoped] = await scopeActivitiesForWrite(
|
||||
[{ title: 'x', tenant: { id: TENANT_B }, organization: { id: 'o' }, employee: { id: 'e' }, timeSlot: { id: 's' } }] as any[],
|
||||
activities,
|
||||
SCOPE
|
||||
);
|
||||
|
||||
expect(scoped).toEqual({ title: 'x' });
|
||||
});
|
||||
|
||||
it("keeps the tenant's own project and drops another tenant's project and task", async () => {
|
||||
// A foreign projectId is stored verbatim by the raw save() and joined back on read, which hands
|
||||
// the caller a project of the victim tenant; the nested-graph check never sees a plain FK.
|
||||
const scoped = await scopeActivitiesForWrite(
|
||||
[
|
||||
{ title: 'mine', projectId: ownProject.id },
|
||||
{ title: 'theirs', projectId: foreignProject.id, taskId: foreignTask.id }
|
||||
] as any[],
|
||||
activities,
|
||||
SCOPE
|
||||
);
|
||||
|
||||
expect(scoped).toEqual([{ title: 'mine', projectId: ownProject.id }, { title: 'theirs' }]);
|
||||
});
|
||||
|
||||
it('folds a project relation object into the id and scopes it the same way', async () => {
|
||||
const scoped = await scopeActivitiesForWrite(
|
||||
[
|
||||
{ title: 'mine', project: { id: ownProject.id } },
|
||||
{ title: 'theirs', project: { id: foreignProject.id } }
|
||||
] as any[],
|
||||
activities,
|
||||
SCOPE
|
||||
);
|
||||
|
||||
expect(scoped).toEqual([{ title: 'mine', projectId: ownProject.id }, { title: 'theirs' }]);
|
||||
});
|
||||
|
||||
it('keeps no id at all without an employee to scope by', async () => {
|
||||
const [scoped] = await scopeActivitiesForWrite([{ id: ownActivity.id }] as any[], activities, {
|
||||
tenantId: TENANT_A,
|
||||
employeeId: null
|
||||
});
|
||||
|
||||
expect(scoped.id).toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,175 @@
|
||||
import { In, Repository } from 'typeorm';
|
||||
import { IActivity, ID } from '@gauzy/contracts';
|
||||
|
||||
/**
|
||||
* Relation objects a client-supplied activity must not carry into a save: each one would let the
|
||||
* body re-point the row at another tenant, organization, employee or time slot, overriding the
|
||||
* scalar ids the handler forces.
|
||||
*/
|
||||
const FORCED_RELATION_KEYS = ['tenant', 'organization', 'employee', 'timeSlot'] as const;
|
||||
|
||||
/**
|
||||
* References a client-supplied activity carries as a plain foreign key. They are not relation
|
||||
* OBJECTS, so the nested-graph ownership check never sees them, and the activity is written through
|
||||
* the raw repository: a body `projectId` of another tenant was stored as-is and then read back with
|
||||
* its project joined, handing the caller that tenant's project. Each is kept only when it names a
|
||||
* row of the caller's tenant, and the relation object is folded into the id (the row stores the id).
|
||||
*/
|
||||
const SCOPED_REFERENCES = [
|
||||
{ relation: 'project', column: 'projectId' },
|
||||
{ relation: 'task', column: 'taskId' }
|
||||
] as const;
|
||||
|
||||
export interface IActivityWriteScope {
|
||||
tenantId: ID;
|
||||
employeeId: ID;
|
||||
}
|
||||
|
||||
/**
|
||||
* Prepares client-supplied activities for `repository.save()`.
|
||||
*
|
||||
* `save()` upserts by primary key alone, so an activity that kept a body-supplied `id` overwrote —
|
||||
* and re-parented — whatever Activity row carried that UUID, in any tenant (GHSA-6qvm-3wg4-26w4,
|
||||
* PUT /timesheet/time-slot/:id and POST /timesheet/activity/bulk). This keeps an `id` only when it
|
||||
* names an activity of the same tenant AND employee (a re-sent activity still updates in place); any
|
||||
* other id is dropped so the row is inserted as new. A `timeSlotId` that is not a slot of the same
|
||||
* tenant and employee is dropped as well, and a `projectId` / `taskId` (or a `project` / `task`
|
||||
* object) naming another tenant's row is dropped too.
|
||||
*
|
||||
* Mutates and returns the given activities. The caller still forces tenantId / organizationId /
|
||||
* employeeId.
|
||||
*
|
||||
* @param activities - The activities about to be saved.
|
||||
* @param repository - The TypeORM Activity repository.
|
||||
* @param scope - The tenant and employee the activities are written for.
|
||||
*/
|
||||
export async function scopeActivitiesForWrite<T extends IActivity>(
|
||||
activities: T[],
|
||||
repository: Repository<any>,
|
||||
scope: IActivityWriteScope
|
||||
): Promise<T[]> {
|
||||
const { tenantId, employeeId } = scope;
|
||||
|
||||
normalizeReferences(activities);
|
||||
|
||||
await dropForeignReferences(activities, repository, tenantId);
|
||||
|
||||
const ids = unique(activities.map((activity) => activity.id));
|
||||
const ownIds = await findOwnIds(repository, ids, tenantId, employeeId);
|
||||
|
||||
const timeSlotIds = unique(activities.map((activity) => activity.timeSlotId));
|
||||
const timeSlotTarget = repository.metadata?.findRelationWithPropertyPath('timeSlot')?.inverseEntityMetadata?.target;
|
||||
const ownTimeSlotIds =
|
||||
timeSlotTarget && timeSlotIds.length
|
||||
? await findOwnIds(repository.manager.getRepository(timeSlotTarget), timeSlotIds, tenantId, employeeId)
|
||||
: new Set<string>();
|
||||
|
||||
for (const activity of activities) {
|
||||
if (activity.id && !ownIds.has(idKey(activity.id))) {
|
||||
delete activity.id;
|
||||
}
|
||||
if (activity.timeSlotId && !ownTimeSlotIds.has(idKey(activity.timeSlotId))) {
|
||||
delete activity.timeSlotId;
|
||||
}
|
||||
}
|
||||
|
||||
return activities;
|
||||
}
|
||||
|
||||
/**
|
||||
* Drops the relation objects the caller forces anyway ({@link FORCED_RELATION_KEYS}) and folds a
|
||||
* {@link SCOPED_REFERENCES} relation object into the scalar id the row actually stores, so only the
|
||||
* id has to be scoped.
|
||||
*
|
||||
* @param activities - The activities about to be saved.
|
||||
*/
|
||||
function normalizeReferences<T extends IActivity>(activities: T[]): void {
|
||||
for (const activity of activities) {
|
||||
for (const key of FORCED_RELATION_KEYS) {
|
||||
delete (activity as any)[key];
|
||||
}
|
||||
|
||||
for (const { relation, column } of SCOPED_REFERENCES) {
|
||||
const nested = (activity as any)[relation];
|
||||
|
||||
if (!nested) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (!(activity as any)[column] && typeof nested === 'object' && nested.id) {
|
||||
(activity as any)[column] = nested.id;
|
||||
}
|
||||
|
||||
delete (activity as any)[relation];
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Drops every {@link SCOPED_REFERENCES} id that does not name a row of the caller's tenant, one
|
||||
* batched lookup per reference. Fails closed: without a tenant, or when the relation cannot be
|
||||
* resolved from the metadata, the id is removed rather than trusted.
|
||||
*/
|
||||
async function dropForeignReferences<T extends IActivity>(
|
||||
activities: T[],
|
||||
repository: Repository<any>,
|
||||
tenantId: ID
|
||||
): Promise<void> {
|
||||
for (const { relation, column } of SCOPED_REFERENCES) {
|
||||
const ids = unique(activities.map((activity) => (activity as any)[column]));
|
||||
if (!ids.length) {
|
||||
continue;
|
||||
}
|
||||
const target = repository.metadata?.findRelationWithPropertyPath(relation)?.inverseEntityMetadata?.target;
|
||||
const own = target
|
||||
? await findTenantIds(repository.manager.getRepository(target), ids, tenantId)
|
||||
: new Set<string>();
|
||||
|
||||
for (const activity of activities) {
|
||||
const id = (activity as any)[column];
|
||||
if (id && !own.has(idKey(id))) {
|
||||
delete (activity as any)[column];
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns which of the given ids name a row of the tenant (soft-deleted rows included). Fails
|
||||
* closed without a tenant.
|
||||
*/
|
||||
async function findTenantIds(repository: Repository<any>, ids: ID[], tenantId: ID): Promise<Set<string>> {
|
||||
if (!ids.length || !tenantId) {
|
||||
return new Set<string>();
|
||||
}
|
||||
const rows = await repository.find({ where: { id: In(ids), tenantId }, select: { id: true }, withDeleted: true });
|
||||
return new Set(rows.map((row: { id: ID }) => idKey(row.id)));
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns which of the given ids name a row of the tenant and employee (soft-deleted rows included,
|
||||
* since `save()` would reach those too). Fails closed without a tenant or an employee.
|
||||
*/
|
||||
async function findOwnIds(repository: Repository<any>, ids: ID[], tenantId: ID, employeeId: ID): Promise<Set<string>> {
|
||||
if (!ids.length || !tenantId || !employeeId) {
|
||||
return new Set<string>();
|
||||
}
|
||||
const rows = await repository.find({
|
||||
where: { id: In(ids), tenantId, employeeId },
|
||||
select: { id: true },
|
||||
withDeleted: true
|
||||
});
|
||||
return new Set(rows.map((row: { id: ID }) => idKey(row.id)));
|
||||
}
|
||||
|
||||
/**
|
||||
* Postgres renders `uuid` lower case and MySQL compares ids case-insensitively, so the row a
|
||||
* differently-cased id resolves to is the same row; compare the ids the same way.
|
||||
*/
|
||||
function idKey(id: ID): string {
|
||||
return String(id).toLowerCase();
|
||||
}
|
||||
|
||||
function unique(values: Array<ID | null | undefined>): ID[] {
|
||||
return [...new Set(values.filter((value): value is ID => !!value))];
|
||||
}
|
||||
+132
@@ -0,0 +1,132 @@
|
||||
// Must stay first: loads the entity graph before any handler pulls an entity (see activity.controller.spec.ts).
|
||||
import '../../../../core/entities/internal';
|
||||
|
||||
import { ForbiddenException } from '@nestjs/common';
|
||||
import { FindOperator } from 'typeorm';
|
||||
import { PermissionsEnum } from '@gauzy/contracts';
|
||||
import { RequestContext } from '../../../../core/context';
|
||||
import { BulkActivitiesSaveCommand } from '../bulk-activities-save.command';
|
||||
import { BulkActivitiesSaveHandler } from './bulk-activities-save.handler';
|
||||
|
||||
/**
|
||||
* GHSA-6qvm-3wg4-26w4 — POST /timesheet/activity/bulk.
|
||||
*
|
||||
* The handler resolved a body employeeId with `findOneBy({ id })` in any tenant, fell back to a body
|
||||
* tenantId, and saved body activities with their own ids (an upsert by primary key). The fakes below
|
||||
* evaluate the where clause against fixtures; CONTROL arms replay the pre-fix call shapes.
|
||||
*/
|
||||
|
||||
const TENANT_A = 'tenant-a';
|
||||
const TENANT_B = 'tenant-b';
|
||||
|
||||
const EMPLOYEES = [
|
||||
{ id: 'employee-a', tenantId: TENANT_A, organizationId: 'org-a' },
|
||||
{ id: 'employee-b', tenantId: TENANT_B, organizationId: 'org-b' }
|
||||
];
|
||||
|
||||
const ACTIVITIES = [
|
||||
{ id: 'activity-own', tenantId: TENANT_A, employeeId: 'employee-a' },
|
||||
{ id: 'activity-foreign', tenantId: TENANT_B, employeeId: 'employee-b' }
|
||||
];
|
||||
|
||||
const matches = (row: Record<string, any>, where: Record<string, any>) =>
|
||||
Object.entries(where).every(([key, value]) =>
|
||||
value === undefined ? true : value instanceof FindOperator ? (value.value as any[]).includes(row[key]) : row[key] === value
|
||||
);
|
||||
|
||||
function createHandler() {
|
||||
const employeeRepository = {
|
||||
findOneBy: jest.fn(async (where: any) => EMPLOYEES.find((employee) => matches(employee, where)) ?? null)
|
||||
};
|
||||
const activityRepository = {
|
||||
find: jest.fn(async ({ where }: any) => ACTIVITIES.filter((activity) => matches(activity, where))),
|
||||
save: jest.fn(async (activities: any[]) => activities),
|
||||
metadata: { findRelationWithPropertyPath: (): undefined => undefined },
|
||||
manager: {}
|
||||
};
|
||||
const handler = new BulkActivitiesSaveHandler(activityRepository as any, employeeRepository as any);
|
||||
return { handler, employeeRepository, activityRepository };
|
||||
}
|
||||
|
||||
function asCaller(options: { tenantId?: string | null; employeeId?: string | null; canChangeEmployee?: boolean }) {
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(
|
||||
(options.tenantId === undefined ? TENANT_A : options.tenantId) as any
|
||||
);
|
||||
jest.spyOn(RequestContext, 'currentEmployeeId').mockReturnValue((options.employeeId ?? null) as any);
|
||||
jest.spyOn(RequestContext, 'currentUser').mockReturnValue({ name: 'caller' } as any);
|
||||
jest.spyOn(RequestContext, 'hasPermission').mockImplementation(
|
||||
(permission) => !!options.canChangeEmployee && permission === PermissionsEnum.CHANGE_SELECTED_EMPLOYEE
|
||||
);
|
||||
}
|
||||
|
||||
describe('BulkActivitiesSaveHandler (GHSA-6qvm-3wg4-26w4)', () => {
|
||||
beforeEach(() => jest.spyOn(console, 'log').mockImplementation(() => undefined));
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
it('CONTROL: the pre-fix employee lookup resolves an employee of another tenant', async () => {
|
||||
const { employeeRepository } = createHandler();
|
||||
|
||||
expect(await employeeRepository.findOneBy({ id: 'employee-b' })).toMatchObject({ tenantId: TENANT_B });
|
||||
});
|
||||
|
||||
it('refuses an employee of another tenant named by a CHANGE_SELECTED_EMPLOYEE holder', async () => {
|
||||
const { handler, employeeRepository, activityRepository } = createHandler();
|
||||
asCaller({ canChangeEmployee: true });
|
||||
|
||||
await expect(
|
||||
handler.execute(
|
||||
new BulkActivitiesSaveCommand({ employeeId: 'employee-b', activities: [{ title: 'x' }] } as any)
|
||||
)
|
||||
).rejects.toThrow(ForbiddenException);
|
||||
|
||||
expect(employeeRepository.findOneBy).toHaveBeenCalledWith({ id: 'employee-b', tenantId: TENANT_A });
|
||||
expect(activityRepository.save).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('refuses to fall back to a body tenantId when the request has none', async () => {
|
||||
const { handler, activityRepository } = createHandler();
|
||||
asCaller({ tenantId: null, employeeId: 'employee-a' });
|
||||
|
||||
await expect(
|
||||
handler.execute(
|
||||
new BulkActivitiesSaveCommand({ tenantId: TENANT_B, activities: [{ title: 'x' }] } as any)
|
||||
)
|
||||
).rejects.toThrow(ForbiddenException);
|
||||
expect(activityRepository.save).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("files the activities under the employee's own organization, not a body-supplied one", async () => {
|
||||
// An Employee row belongs to exactly one organization; a sibling organization of the same tenant
|
||||
// would otherwise get this employee's activities filed under it.
|
||||
const { handler, activityRepository } = createHandler();
|
||||
asCaller({ employeeId: 'employee-a' });
|
||||
|
||||
await handler.execute(
|
||||
new BulkActivitiesSaveCommand({ organizationId: 'org-sibling', activities: [{ title: 'x' }] } as any)
|
||||
);
|
||||
|
||||
const [saved] = activityRepository.save.mock.calls[0][0];
|
||||
expect(saved).toMatchObject({ organizationId: 'org-a', employeeId: 'employee-a', tenantId: TENANT_A });
|
||||
});
|
||||
|
||||
it("never saves under another tenant's activity id, and keeps the employee's own", async () => {
|
||||
const { handler, activityRepository } = createHandler();
|
||||
asCaller({ employeeId: 'employee-a' });
|
||||
|
||||
await handler.execute(
|
||||
new BulkActivitiesSaveCommand({
|
||||
activities: [
|
||||
{ id: 'activity-foreign', title: 'steal', tenant: { id: TENANT_B } },
|
||||
{ id: 'activity-own', title: 'resend' }
|
||||
]
|
||||
} as any)
|
||||
);
|
||||
|
||||
const saved = activityRepository.save.mock.calls[0][0];
|
||||
expect(saved.map((activity: any) => activity.id)).toEqual([undefined, 'activity-own']);
|
||||
for (const activity of saved) {
|
||||
expect(activity).toMatchObject({ tenantId: TENANT_A, employeeId: 'employee-a', organizationId: 'org-a' });
|
||||
expect(activity.tenant).toBeUndefined();
|
||||
}
|
||||
});
|
||||
});
|
||||
+33
-8
@@ -1,7 +1,9 @@
|
||||
import { ForbiddenException } from '@nestjs/common';
|
||||
import { CommandHandler, ICommandHandler } from '@nestjs/cqrs';
|
||||
import { IActivity, PermissionsEnum } from '@gauzy/contracts';
|
||||
import { isEmpty, isNotEmpty } from '@gauzy/utils';
|
||||
import { Activity } from '../../activity.entity';
|
||||
import { scopeActivitiesForWrite } from '../../activity-write-scope.helper';
|
||||
import { BulkActivitiesSaveCommand } from '../bulk-activities-save.command';
|
||||
import { RequestContext } from '../../../../core/context';
|
||||
import { TypeOrmActivityRepository } from '../../repository/type-orm-activity.repository';
|
||||
@@ -26,7 +28,12 @@ export class BulkActivitiesSaveHandler implements ICommandHandler<BulkActivities
|
||||
let { employeeId, organizationId, activities = [], projectId } = input;
|
||||
|
||||
const user = RequestContext.currentUser();
|
||||
const tenantId = RequestContext.currentTenantId() ?? input.tenantId;
|
||||
|
||||
// Activities are written into the caller's tenant only; a body tenantId is never a fallback.
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
if (!tenantId) {
|
||||
throw new ForbiddenException('A tenant is required to save activities');
|
||||
}
|
||||
|
||||
// Check if the logged user has permission to change the selected employee
|
||||
const hasChangeEmployeePermission = RequestContext.hasPermission(PermissionsEnum.CHANGE_SELECTED_EMPLOYEE);
|
||||
@@ -36,10 +43,18 @@ export class BulkActivitiesSaveHandler implements ICommandHandler<BulkActivities
|
||||
employeeId = RequestContext.currentEmployeeId();
|
||||
}
|
||||
|
||||
// Assign the current user's organizationId if it's not provided
|
||||
if (isEmpty(organizationId) && employeeId) {
|
||||
const employee = await this.typeOrmEmployeeRepository.findOneBy({ id: employeeId });
|
||||
organizationId = employee ? employee.organizationId : null;
|
||||
// The employee must belong to the caller's tenant: CHANGE_SELECTED_EMPLOYEE holders may name any
|
||||
// employee in the body, and the lookup used to resolve it in any tenant (GHSA-6qvm-3wg4-26w4).
|
||||
if (employeeId) {
|
||||
const employee = await this.typeOrmEmployeeRepository.findOneBy({ id: employeeId, tenantId });
|
||||
if (!employee) {
|
||||
throw new ForbiddenException('The employee does not belong to this tenant');
|
||||
}
|
||||
// The employee's own organization wins over a body-supplied one: an Employee row belongs to
|
||||
// exactly one organization, so a different organizationId of the same tenant would file this
|
||||
// employee's activities under an organization they are not a member of. The body value stays
|
||||
// a fallback for an employee without an organization.
|
||||
organizationId = employee.organizationId || organizationId;
|
||||
}
|
||||
|
||||
// Log empty activities and filter out any invalid ones
|
||||
@@ -48,15 +63,25 @@ export class BulkActivitiesSaveHandler implements ICommandHandler<BulkActivities
|
||||
activities.filter((activity: IActivity) => Object.keys(activity).length === 0)
|
||||
);
|
||||
|
||||
activities = activities
|
||||
.filter((activity: IActivity) => Object.keys(activity).length !== 0)
|
||||
// Body-supplied activity ids / time slot ids are kept only when they are the employee's own:
|
||||
// save() upserts by primary key alone and would otherwise overwrite any tenant's activity. The
|
||||
// request-level `projectId` is applied BEFORE the check, so it is tenant-scoped like the
|
||||
// per-activity ones rather than overriding them unchecked.
|
||||
const validActivities = await scopeActivitiesForWrite(
|
||||
activities
|
||||
.filter((activity: IActivity) => Object.keys(activity).length !== 0)
|
||||
.map((activity: IActivity) => ({ ...activity, ...(projectId ? { projectId } : {}) })),
|
||||
this.typeOrmActivityRepository,
|
||||
{ tenantId, employeeId }
|
||||
);
|
||||
|
||||
activities = validActivities
|
||||
.map(
|
||||
(activity: IActivity) =>
|
||||
// `recordedAt` is guaranteed by `ActivitySubscriber.beforeEntityCreate`, which
|
||||
// runs for every Activity write path (bulk save, single create, imports).
|
||||
new Activity({
|
||||
...activity,
|
||||
...(projectId ? { projectId } : {}),
|
||||
employeeId,
|
||||
organizationId,
|
||||
tenantId
|
||||
|
||||
+82
@@ -0,0 +1,82 @@
|
||||
// Must stay first: loads the entity graph before any handler pulls an entity (see activity.controller.spec.ts).
|
||||
import '../../../../core/entities/internal';
|
||||
|
||||
import { NotFoundException } from '@nestjs/common';
|
||||
import { RequestContext } from '../../../../core/context';
|
||||
import { TimeLogUpdateCommand } from '../time-log-update.command';
|
||||
import { omitScopeFields, TimeLogUpdateHandler } from './time-log-update.handler';
|
||||
|
||||
/**
|
||||
* GHSA-6qvm-3wg4-26w4 — PUT /timesheet/time-log/:id.
|
||||
*
|
||||
* The handler spread its input straight into `update(timeLog.id, { ...input })`, and TenantBaseGuard
|
||||
* skips the body tenant check when a Tenant-Id header is present, so an employee could re-point their
|
||||
* own log at another tenant. Tenant and organization now come from the stored row.
|
||||
*/
|
||||
|
||||
const TENANT_A = 'tenant-a';
|
||||
const TENANT_B = 'tenant-b';
|
||||
|
||||
const LOGS = [
|
||||
{ id: 'log-own', tenantId: TENANT_A, organizationId: 'org-a', employeeId: 'employee-a' },
|
||||
{ id: 'log-foreign', tenantId: TENANT_B, organizationId: 'org-b', employeeId: 'employee-b' }
|
||||
];
|
||||
|
||||
function createHandler() {
|
||||
const timeLogRepository = {
|
||||
findOneBy: jest.fn(
|
||||
async (where: any) =>
|
||||
LOGS.find((log) => Object.entries(where).every(([key, value]) => (log as any)[key] === value)) ?? null
|
||||
),
|
||||
update: jest.fn(async () => ({ affected: 1 }))
|
||||
};
|
||||
const timeSlotService = { generateTimeSlots: jest.fn(() => []) };
|
||||
const handler = new TimeLogUpdateHandler(
|
||||
{ execute: jest.fn() } as any,
|
||||
timeLogRepository as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
timeSlotService as any
|
||||
);
|
||||
return { handler, timeLogRepository };
|
||||
}
|
||||
|
||||
describe('TimeLogUpdateHandler (GHSA-6qvm-3wg4-26w4)', () => {
|
||||
beforeEach(() => {
|
||||
jest.spyOn(console, 'log').mockImplementation(() => undefined);
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT_A);
|
||||
});
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
const body = { description: 'edited', isBillable: true, tenantId: TENANT_B, organizationId: 'org-b' } as any;
|
||||
|
||||
it('CONTROL: the pre-fix payload carries the body tenant and organization into the update', () => {
|
||||
expect({ ...body }).toMatchObject({ tenantId: TENANT_B, organizationId: 'org-b' });
|
||||
});
|
||||
|
||||
it('updates within the stored tenant, without the body tenant / organization', async () => {
|
||||
const { handler, timeLogRepository } = createHandler();
|
||||
|
||||
await handler.execute(new TimeLogUpdateCommand(body, 'log-own'));
|
||||
|
||||
expect(timeLogRepository.update).toHaveBeenCalledWith(
|
||||
{ id: 'log-own', tenantId: TENANT_A },
|
||||
{ description: 'edited', isBillable: true }
|
||||
);
|
||||
});
|
||||
|
||||
it('resolves an id inside the caller tenant only', async () => {
|
||||
const { handler, timeLogRepository } = createHandler();
|
||||
|
||||
await expect(handler.execute(new TimeLogUpdateCommand(body, 'log-foreign'))).rejects.toThrow(NotFoundException);
|
||||
expect(timeLogRepository.findOneBy).toHaveBeenCalledWith({ id: 'log-foreign', tenantId: TENANT_A });
|
||||
expect(timeLogRepository.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('keeps the fields internal callers rely on (the timer stop sends isRunning / stoppedAt)', () => {
|
||||
expect(omitScopeFields({ isRunning: false, stoppedAt: 'x', id: 'y', tenant: {}, organization: {}, employee: {} })).toEqual({
|
||||
isRunning: false,
|
||||
stoppedAt: 'x'
|
||||
});
|
||||
});
|
||||
});
|
||||
+44
-12
@@ -1,3 +1,4 @@
|
||||
import { NotFoundException } from '@nestjs/common';
|
||||
import { ICommandHandler, CommandBus, CommandHandler } from '@nestjs/cqrs';
|
||||
import * as moment from 'moment';
|
||||
import { ID, ITimeLog, ITimeSlot, ITimesheet, TimeLogSourceEnum } from '@gauzy/contracts';
|
||||
@@ -14,6 +15,24 @@ import { TypeOrmTimeLogRepository } from '../../repository/type-orm-time-log.rep
|
||||
import { TypeOrmTimeSlotRepository } from '../../../time-slot/repository/type-orm-time-slot.repository';
|
||||
import { MikroOrmTimeSlotRepository } from '../../../time-slot/repository/mikro-orm-time-slot.repository';
|
||||
|
||||
/**
|
||||
* Input keys that decide WHERE a time log lives. They are never taken from the input.
|
||||
*/
|
||||
const TIME_LOG_SCOPE_FIELDS = ['id', 'tenantId', 'tenant', 'organizationId', 'organization', 'employee'] as const;
|
||||
|
||||
/**
|
||||
* Returns a copy of the input without the scope fields.
|
||||
*
|
||||
* @param input - The requested time log changes.
|
||||
*/
|
||||
export function omitScopeFields<T extends object>(input: T): Partial<T> {
|
||||
const changes: Partial<T> = { ...input };
|
||||
for (const field of TIME_LOG_SCOPE_FIELDS) {
|
||||
delete (changes as any)[field];
|
||||
}
|
||||
return changes;
|
||||
}
|
||||
|
||||
@CommandHandler(TimeLogUpdateCommand)
|
||||
export class TimeLogUpdateHandler implements ICommandHandler<TimeLogUpdateCommand> {
|
||||
protected ormType: MultiORM = getORMType();
|
||||
@@ -41,14 +60,15 @@ export class TimeLogUpdateHandler implements ICommandHandler<TimeLogUpdateComman
|
||||
const { id, input, manualTimeSlot, forceDelete = false } = command;
|
||||
console.log('Executing TimeLogUpdateCommand:', { id, input, manualTimeSlot, forceDelete });
|
||||
|
||||
// Retrieve the tenant ID from the request context or the provided input
|
||||
const tenantId = RequestContext.currentTenantId() ?? input.tenantId;
|
||||
console.log('Tenant ID:', tenantId);
|
||||
|
||||
let timeLog: ITimeLog = await this.getTimeLogByIdOrInstance(id);
|
||||
console.log('Retrieved TimeLog:', timeLog);
|
||||
|
||||
const { employeeId, organizationId } = timeLog;
|
||||
// Tenant and organization always come from the stored row, never from the input: the input is
|
||||
// (partly) a request body, and spreading its tenantId/organizationId into the update re-pointed
|
||||
// the log at another tenant (GHSA-6qvm-3wg4-26w4).
|
||||
const { employeeId, organizationId, tenantId } = timeLog;
|
||||
console.log('Tenant ID:', tenantId);
|
||||
const changes = omitScopeFields(input);
|
||||
|
||||
let timesheet: ITimesheet;
|
||||
let updateTimeSlots: ITimeSlot[] = [];
|
||||
@@ -64,17 +84,20 @@ export class TimeLogUpdateHandler implements ICommandHandler<TimeLogUpdateComman
|
||||
console.log('Generated or retrieved Timesheet:', timesheet);
|
||||
|
||||
// Generate time slots based on the updated time log details
|
||||
const { startedAt, stoppedAt } = { ...timeLog, ...input };
|
||||
const { startedAt, stoppedAt } = { ...timeLog, ...changes };
|
||||
updateTimeSlots = this.timeSlotService.generateTimeSlots(startedAt, stoppedAt);
|
||||
console.log('Generated updated TimeSlots:', updateTimeSlots);
|
||||
}
|
||||
|
||||
// Update the time log in the repository
|
||||
await this.typeOrmTimeLogRepository.update(timeLog.id, {
|
||||
...input,
|
||||
...(timesheet ? { timesheetId: timesheet.id } : {})
|
||||
});
|
||||
console.log('Updated TimeLog in the repository:', { id: timeLog.id, input });
|
||||
await this.typeOrmTimeLogRepository.update(
|
||||
{ id: timeLog.id, tenantId },
|
||||
{
|
||||
...changes,
|
||||
...(timesheet ? { timesheetId: timesheet.id } : {})
|
||||
}
|
||||
);
|
||||
console.log('Updated TimeLog in the repository:', { id: timeLog.id, input: changes });
|
||||
|
||||
// Regenerate the existing time slots for the time log
|
||||
const timeSlots = this.timeSlotService.generateTimeSlots(timeLog.startedAt, timeLog.stoppedAt);
|
||||
@@ -127,7 +150,16 @@ export class TimeLogUpdateHandler implements ICommandHandler<TimeLogUpdateComman
|
||||
* @returns A promise that resolves to the `ITimeLog` instance.
|
||||
*/
|
||||
private async getTimeLogByIdOrInstance(id: ID | TimeLog): Promise<ITimeLog> {
|
||||
return id instanceof TimeLog ? id : this.typeOrmTimeLogRepository.findOneBy({ id });
|
||||
if (id instanceof TimeLog) {
|
||||
return id;
|
||||
}
|
||||
// An id is resolved inside the caller's tenant only (the raw repository has no tenant scoping).
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
const timeLog = tenantId ? await this.typeOrmTimeLogRepository.findOneBy({ id, tenantId }) : null;
|
||||
if (!timeLog) {
|
||||
throw new NotFoundException('The time log was not found');
|
||||
}
|
||||
return timeLog;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { ApiProperty } from "@nestjs/swagger";
|
||||
import { IsNotEmpty, IsUUID } from "class-validator";
|
||||
import { IEmployee, IManualTimeInput } from "@gauzy/contracts";
|
||||
import { ApiProperty, ApiPropertyOptional } from "@nestjs/swagger";
|
||||
import { IsArray, IsBoolean, IsNotEmpty, IsOptional, IsString, IsUUID } from "class-validator";
|
||||
import { ID, IEmployee, IManualTimeInput } from "@gauzy/contracts";
|
||||
import { IsBeforeDate } from "./../../../shared/validators";
|
||||
import { TenantOrganizationBaseDTO } from "./../../../core/dto";
|
||||
|
||||
@@ -33,4 +33,57 @@ export class ManualTimeLogDTO extends TenantOrganizationBaseDTO implements IManu
|
||||
@IsNotEmpty()
|
||||
@IsUUID()
|
||||
employeeId: IEmployee['id'];
|
||||
|
||||
/*
|
||||
* The fields below are what the web and desktop clients send besides the dates (the edit-time-log
|
||||
* modal and the timer's `timerConfig`). They are declared so the routes can run with
|
||||
* `whitelist: true`, which keeps everything else (id, isRunning, timesheetId, relation objects)
|
||||
* out of the time log (GHSA-6qvm-3wg4-26w4). Ids are validated as strings, not UUIDs, because the
|
||||
* clients send them as-is and an empty value must not start failing.
|
||||
*/
|
||||
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
projectId?: ID;
|
||||
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
taskId?: ID;
|
||||
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
organizationContactId?: ID;
|
||||
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
organizationTeamId?: ID;
|
||||
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
description?: string;
|
||||
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
reason?: string;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Boolean })
|
||||
@IsOptional()
|
||||
@IsBoolean()
|
||||
isBillable?: boolean;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Array, isArray: true })
|
||||
@IsOptional()
|
||||
@IsArray()
|
||||
tags?: string[];
|
||||
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
@IsString()
|
||||
version?: string;
|
||||
}
|
||||
|
||||
@@ -1,4 +1,19 @@
|
||||
import { IManualTimeInput } from "@gauzy/contracts";
|
||||
import { ApiPropertyOptional } from "@nestjs/swagger";
|
||||
import { IsEnum, IsOptional } from "class-validator";
|
||||
import { IManualTimeInput, TimeLogSourceEnum, TimeLogType } from "@gauzy/contracts";
|
||||
import { ManualTimeLogDTO } from "./manual-time-log.dto";
|
||||
|
||||
export class UpdateManualTimeLogDTO extends ManualTimeLogDTO implements IManualTimeInput { }
|
||||
export class UpdateManualTimeLogDTO extends ManualTimeLogDTO implements IManualTimeInput {
|
||||
/**
|
||||
* The edit-time-log modal sends the log type and source on update too.
|
||||
*/
|
||||
@ApiPropertyOptional({ type: () => String, enum: TimeLogType })
|
||||
@IsOptional()
|
||||
@IsEnum(TimeLogType)
|
||||
logType?: TimeLogType;
|
||||
|
||||
@ApiPropertyOptional({ type: () => String, enum: TimeLogSourceEnum })
|
||||
@IsOptional()
|
||||
@IsEnum(TimeLogSourceEnum)
|
||||
source?: TimeLogSourceEnum;
|
||||
}
|
||||
|
||||
@@ -278,7 +278,7 @@ export class TimeLogController {
|
||||
@UseGuards(OrganizationPermissionGuard)
|
||||
@Permissions(PermissionsEnum.ALLOW_MANUAL_TIME)
|
||||
async addManualTime(
|
||||
@Body(TimeLogBodyTransformPipe, new ValidationPipe({ transform: true })) entity: CreateManualTimeLogDTO
|
||||
@Body(TimeLogBodyTransformPipe, new ValidationPipe({ transform: true, whitelist: true })) entity: CreateManualTimeLogDTO
|
||||
): Promise<ITimeLog> {
|
||||
return await this._timeLogService.addManualTime(entity);
|
||||
}
|
||||
@@ -304,7 +304,7 @@ export class TimeLogController {
|
||||
@OrganizationPolicyTarget(TimeLog)
|
||||
async updateManualTime(
|
||||
@Param('id', UUIDValidationPipe) id: ID,
|
||||
@Body(TimeLogBodyTransformPipe, new ValidationPipe({ transform: true })) entity: UpdateManualTimeLogDTO
|
||||
@Body(TimeLogBodyTransformPipe, new ValidationPipe({ transform: true, whitelist: true })) entity: UpdateManualTimeLogDTO
|
||||
): Promise<ITimeLog> {
|
||||
return await this._timeLogService.updateManualTime(id, entity);
|
||||
}
|
||||
|
||||
@@ -12,7 +12,7 @@
|
||||
import '../../core/entities/internal';
|
||||
import { Test, TestingModule } from '@nestjs/testing';
|
||||
import { CommandBus } from '@nestjs/cqrs';
|
||||
import { IGetTimeLogReportInput } from '@gauzy/contracts';
|
||||
import { IGetTimeLogReportInput, IManualTimeInput } from '@gauzy/contracts';
|
||||
import { moment } from '../../core/moment-extend';
|
||||
import { getDateRangeFormat, MultiORMEnum } from '../../core/utils';
|
||||
import { ManagedEmployeeService } from '../../employee/managed-employee.service';
|
||||
@@ -22,7 +22,9 @@ import {
|
||||
nextMacrotask,
|
||||
RecordingQueryBuilder
|
||||
} from '../testing/recording-query-builder';
|
||||
import { TypeOrmEmployeeRepository } from '../../employee/repository/type-orm-employee.repository';
|
||||
import { TypeOrmTimeLogRepository } from './repository/type-orm-time-log.repository';
|
||||
import { TimeLogCreateCommand, IGetConflictTimeLogCommand } from './commands';
|
||||
import { TimeLogService } from './time-log.service';
|
||||
|
||||
const TENANT_ID = '5a1c2f0e-6d3b-4c8a-9e2f-1b7d4a6c8e90';
|
||||
@@ -163,6 +165,29 @@ describe('TimeLogService', () => {
|
||||
expect(parameters).toEqual(expect.objectContaining({ employeeIds: [TARGET_EMPLOYEE_ID] }));
|
||||
});
|
||||
|
||||
// GHSA-6qvm-3wg4-26w4: the employee predicate is only added when `employeeIds` is non-empty, and
|
||||
// the narrowing above only runs for a caller who HAS an employee record. A caller with neither
|
||||
// the permission nor an employee (a custom role holding TIME_TRACKER) therefore read the whole
|
||||
// organization, or the employees they named in the body. CONTROL: the two arms below, where the
|
||||
// same caller state with an employee record still produces the ordinary employee predicate.
|
||||
it.each<[string, (input: IGetTimeLogReportInput) => Promise<unknown>]>(reportMethods)(
|
||||
'%s matches nothing for a caller with neither CHANGE_SELECTED_EMPLOYEE nor an employee record',
|
||||
async (_name, run) => {
|
||||
mockRequestContext({
|
||||
tenantId: TENANT_ID,
|
||||
user: { id: USER_ID, employeeId: null },
|
||||
canChangeSelectedEmployee: false
|
||||
});
|
||||
|
||||
await run(request);
|
||||
|
||||
const { conditions, parameters } = executedFilters(builder);
|
||||
expect(conditions).toEqual(['1 = 0']);
|
||||
expect(parameters).not.toHaveProperty('employeeIds');
|
||||
expect(canManageEmployees).not.toHaveBeenCalled();
|
||||
}
|
||||
);
|
||||
|
||||
it('honours onlyMe without consulting the manager check', async () => {
|
||||
actAs({ canChangeSelectedEmployee: false });
|
||||
|
||||
@@ -174,4 +199,85 @@ describe('TimeLogService', () => {
|
||||
expect(parameters).toEqual(expect.objectContaining({ employeeIds: [CURRENT_EMPLOYEE_ID] }));
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* The organization the manual-time routes act under is the EMPLOYEE's, not the body's: the
|
||||
* `futureDateAllowed` policy they check belongs to `employee.organization`, so honouring a body
|
||||
* organizationId of the same tenant judged the write by one organization's rules and then
|
||||
* persisted it — log, slots and timesheet — under another's, and (on update) let a sibling
|
||||
* organization's time slots count as conflicting, i.e. be deleted.
|
||||
*/
|
||||
describe('manual time organization scope', () => {
|
||||
const EMPLOYEE_ORGANIZATION_ID = 'a7f1c6de-0f3e-4f4b-9b1a-2c4d6e8f0a1b';
|
||||
const BODY_ORGANIZATION_ID = 'b8e2d7cf-1a4f-4c5d-8e2b-3d5f7a9c1b2e';
|
||||
|
||||
let manualService: TimeLogService;
|
||||
let execute: jest.Mock;
|
||||
|
||||
const request = {
|
||||
employeeId: TARGET_EMPLOYEE_ID,
|
||||
organizationId: BODY_ORGANIZATION_ID,
|
||||
startedAt: new Date('2026-01-05T09:00:00.000Z'),
|
||||
stoppedAt: new Date('2026-01-05T10:00:00.000Z')
|
||||
} as IManualTimeInput;
|
||||
|
||||
const commandsOfType = <T>(type: new (...args: any[]) => T): T[] =>
|
||||
execute.mock.calls.map(([command]) => command).filter((command) => command instanceof type);
|
||||
|
||||
beforeEach(async () => {
|
||||
execute = jest.fn().mockResolvedValue([]);
|
||||
|
||||
const module: TestingModule = await Test.createTestingModule({ providers: [TimeLogService] })
|
||||
.useMocker((token) => {
|
||||
if (token === TypeOrmEmployeeRepository) {
|
||||
return {
|
||||
findOne: jest.fn().mockResolvedValue({
|
||||
id: TARGET_EMPLOYEE_ID,
|
||||
organizationId: EMPLOYEE_ORGANIZATION_ID,
|
||||
organization: { id: EMPLOYEE_ORGANIZATION_ID, futureDateAllowed: true }
|
||||
})
|
||||
};
|
||||
}
|
||||
if (token === CommandBus) {
|
||||
return { execute };
|
||||
}
|
||||
if (token === TypeOrmTimeLogRepository) {
|
||||
return { metadata: { tableName: 'time_log' }, createQueryBuilder: () => builder };
|
||||
}
|
||||
return {};
|
||||
})
|
||||
.compile();
|
||||
|
||||
manualService = module.get<TimeLogService>(TimeLogService);
|
||||
Object.defineProperty(manualService, 'ormType', { value: MultiORMEnum.TypeORM });
|
||||
mockRequestContext({
|
||||
tenantId: TENANT_ID,
|
||||
user: { id: USER_ID, employeeId: CURRENT_EMPLOYEE_ID },
|
||||
canChangeSelectedEmployee: true
|
||||
});
|
||||
});
|
||||
|
||||
it("addManualTime persists the employee's organization, not the body's", async () => {
|
||||
await manualService.addManualTime(request);
|
||||
|
||||
// CONTROL: the body named a different organization of the same tenant.
|
||||
expect(request.organizationId).toBe(BODY_ORGANIZATION_ID);
|
||||
expect(commandsOfType(IGetConflictTimeLogCommand)[0].input).toMatchObject({
|
||||
organizationId: EMPLOYEE_ORGANIZATION_ID
|
||||
});
|
||||
expect(commandsOfType(TimeLogCreateCommand)[0].input).toMatchObject({
|
||||
organizationId: EMPLOYEE_ORGANIZATION_ID
|
||||
});
|
||||
});
|
||||
|
||||
it("updateManualTime looks for conflicts in the employee's organization", async () => {
|
||||
jest.spyOn(manualService, 'findOneByIdString').mockResolvedValue({ id: 'log-1' } as any);
|
||||
|
||||
await manualService.updateManualTime('log-1', { ...request });
|
||||
|
||||
expect(commandsOfType(IGetConflictTimeLogCommand)[0].input).toMatchObject({
|
||||
organizationId: EMPLOYEE_ORGANIZATION_ID
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -3,10 +3,11 @@ import {
|
||||
BadRequestException,
|
||||
ForbiddenException,
|
||||
HttpException,
|
||||
NotAcceptableException
|
||||
NotAcceptableException,
|
||||
NotFoundException
|
||||
} from '@nestjs/common';
|
||||
import { CommandBus } from '@nestjs/cqrs';
|
||||
import { SelectQueryBuilder, Brackets, WhereExpressionBuilder, DeleteResult, UpdateResult } from 'typeorm';
|
||||
import { SelectQueryBuilder, Brackets, WhereExpressionBuilder, DeleteResult, UpdateResult, FindOptionsWhere } from 'typeorm';
|
||||
import { chain, pluck } from 'underscore';
|
||||
import {
|
||||
IManualTimeInput,
|
||||
@@ -71,6 +72,15 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
super(typeOrmTimeLogRepository, mikroOrmTimeLogRepository);
|
||||
}
|
||||
|
||||
/**
|
||||
* Time logs are personal: a caller without CHANGE_SELECTED_EMPLOYEE and without an employee record
|
||||
* of their own (a custom role holding TIME_TRACKER, say) must not fall back to the tenant-wide scope
|
||||
* of the CRUD reads and deletes (GHSA-6qvm-3wg4-26w4). They match nothing instead.
|
||||
*/
|
||||
protected findConditionsWithoutOwnEmployee(): FindOptionsWhere<TimeLog> {
|
||||
return this.neverMatchingEmployeeCondition();
|
||||
}
|
||||
|
||||
/**
|
||||
* Retrieves time logs based on the provided input.
|
||||
* @param request The input parameters for fetching time logs.
|
||||
@@ -1273,6 +1283,16 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
}
|
||||
}
|
||||
|
||||
// Fail closed for a caller who may not act for other employees and has no employee record of
|
||||
// their own: there is no personal scope to narrow to, and every filter below is optional, so the
|
||||
// query would return the whole organization (GHSA-6qvm-3wg4-26w4). The CRUD reads already match
|
||||
// nothing in that state (findConditionsWithoutOwnEmployee); these hand-built report queries
|
||||
// never reach that hook, so they carry the same rule here.
|
||||
if (!hasChangeSelectedEmployeePermission && !user.employeeId) {
|
||||
query.andWhere('1 = 0');
|
||||
return query;
|
||||
}
|
||||
|
||||
// Filters records based on the timesheetId.
|
||||
if (isNotEmpty(request.timesheetId)) {
|
||||
const { timesheetId } = request;
|
||||
@@ -1402,6 +1422,12 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
}
|
||||
}
|
||||
|
||||
// Fail closed for a caller with neither the permission nor an employee record. See the TypeORM
|
||||
// branch in getFilterTimeLogQuery.
|
||||
if (!hasChangeSelectedEmployeePermission && !user.employeeId) {
|
||||
return { id: { $in: [] } };
|
||||
}
|
||||
|
||||
const where: any = { tenantId, organizationId };
|
||||
|
||||
if (isNotEmpty(request.timesheetId)) {
|
||||
@@ -1491,6 +1517,31 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
return await this.commandBus.execute(new IGetConflictTimeLogCommand({ ...input, tenantId }));
|
||||
}
|
||||
|
||||
/**
|
||||
* Loads the employee a manual time log is written for, inside the caller's tenant.
|
||||
*
|
||||
* The raw repository has no tenant scoping, and an undefined id would be dropped from the where
|
||||
* clause and match an arbitrary employee, so both a missing id and a missing tenant fail closed
|
||||
* (GHSA-6qvm-3wg4-26w4).
|
||||
*
|
||||
* @param employeeId - The employee from the request.
|
||||
* @param tenantId - The caller's tenant.
|
||||
* @returns The employee, with its organization.
|
||||
*/
|
||||
private async findEmployeeInTenant(employeeId: ID, tenantId: ID): Promise<IEmployee> {
|
||||
const employee =
|
||||
employeeId && tenantId
|
||||
? await this.typeOrmEmployeeRepository.findOne({
|
||||
where: { id: employeeId, tenantId },
|
||||
relations: { organization: true }
|
||||
})
|
||||
: null;
|
||||
if (!employee) {
|
||||
throw new NotFoundException('The employee was not found');
|
||||
}
|
||||
return employee;
|
||||
}
|
||||
|
||||
/**
|
||||
* Adds a manual time log entry.
|
||||
*
|
||||
@@ -1499,8 +1550,8 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
*/
|
||||
async addManualTime(request: IManualTimeInput): Promise<ITimeLog> {
|
||||
try {
|
||||
const tenantId = RequestContext.currentTenantId() ?? request.tenantId;
|
||||
const { employeeId, startedAt, stoppedAt, organizationId } = request;
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
const { employeeId, startedAt, stoppedAt } = request;
|
||||
|
||||
// Validate input
|
||||
if (!startedAt || !stoppedAt) {
|
||||
@@ -1508,10 +1559,13 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
}
|
||||
|
||||
// Retrieve employee information
|
||||
const employee: IEmployee = await this.typeOrmEmployeeRepository.findOne({
|
||||
where: { id: employeeId },
|
||||
relations: { organization: true }
|
||||
});
|
||||
const employee: IEmployee = await this.findEmployeeInTenant(employeeId, tenantId);
|
||||
|
||||
// The organization is the EMPLOYEE's, not the body's. The policy consulted right below is
|
||||
// `employee.organization`'s, so honouring a different organizationId of the same tenant would
|
||||
// judge the write by one organization's rules and then persist it — log, slots and timesheet —
|
||||
// under another's. The body value is only a fallback for an employee without an organization.
|
||||
const organizationId = employee.organizationId ?? request.organizationId;
|
||||
|
||||
// Check if future dates are allowed for the organization
|
||||
const futureDateAllowed: IOrganization['futureDateAllowed'] = employee.organization.futureDateAllowed;
|
||||
@@ -1551,7 +1605,7 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
}
|
||||
|
||||
// Create the new time log entry
|
||||
return await this.commandBus.execute(new TimeLogCreateCommand(request));
|
||||
return await this.commandBus.execute(new TimeLogCreateCommand({ ...request, organizationId }));
|
||||
} catch (error) {
|
||||
// Never swallow the reason: a blanket message here hid a real database failure indefinitely.
|
||||
if (error instanceof HttpException) {
|
||||
@@ -1570,8 +1624,8 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
*/
|
||||
async updateManualTime(id: ID, request: IManualTimeInput): Promise<ITimeLog> {
|
||||
try {
|
||||
const tenantId = RequestContext.currentTenantId() ?? request.tenantId;
|
||||
const { startedAt, stoppedAt, employeeId, organizationId } = request;
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
const { startedAt, stoppedAt, employeeId } = request;
|
||||
|
||||
// Validate input
|
||||
if (!startedAt || !stoppedAt) {
|
||||
@@ -1579,10 +1633,11 @@ export class TimeLogService extends TenantAwareCrudService<TimeLog> {
|
||||
}
|
||||
|
||||
// Retrieve employee information
|
||||
const employee: IEmployee = await this.typeOrmEmployeeRepository.findOne({
|
||||
where: { id: employeeId },
|
||||
relations: { organization: true }
|
||||
});
|
||||
const employee: IEmployee = await this.findEmployeeInTenant(employeeId, tenantId);
|
||||
|
||||
// The employee's organization, never the body's — see `addManualTime`. Here it also decides
|
||||
// which rows count as conflicting, i.e. which time slots this call is allowed to delete.
|
||||
const organizationId = employee.organizationId ?? request.organizationId;
|
||||
|
||||
// Check if future dates are allowed for the organization
|
||||
const futureDateAllowed: IOrganization['futureDateAllowed'] = employee.organization.futureDateAllowed;
|
||||
|
||||
+180
@@ -0,0 +1,180 @@
|
||||
// Must stay first: loads the entity graph before any handler pulls an entity (see activity.controller.spec.ts).
|
||||
import '../../../../core/entities/internal';
|
||||
|
||||
import { FindOperator } from 'typeorm';
|
||||
import { PermissionsEnum } from '@gauzy/contracts';
|
||||
import { RequestContext } from '../../../../core/context';
|
||||
import { UpdateTimeSlotCommand } from '../update-time-slot.command';
|
||||
import { UpdateTimeSlotHandler, UPDATABLE_TIME_SLOT_FIELDS } from './update-time-slot.handler';
|
||||
|
||||
/**
|
||||
* GHSA-6qvm-3wg4-26w4 — PUT /timesheet/time-slot/:id.
|
||||
*
|
||||
* The handler looked the slot up with `findOne({ where: { id } })` on a raw repository, wrote
|
||||
* `update(id, input)` with the whole body, and saved body activities with their own ids. Every tenant
|
||||
* owner is a SUPER_ADMIN (CHANGE_SELECTED_EMPLOYEE, exempt from OrganizationPermissionGuard), so a
|
||||
* foreign slot UUID was enough to modify it and read its time logs back.
|
||||
*
|
||||
* The repositories below are fakes that EVALUATE the where clause against fixtures (undefined keys are
|
||||
* dropped, as with the shipped `invalidWhereValuesBehavior.undefined: 'ignore'`), so a missing tenant
|
||||
* predicate shows up as a match on the foreign row. CONTROL arms run the pre-fix where shape.
|
||||
*/
|
||||
|
||||
const TENANT_A = 'tenant-a';
|
||||
const TENANT_B = 'tenant-b';
|
||||
|
||||
const SLOTS = [
|
||||
{ id: 'slot-own', tenantId: TENANT_A, organizationId: 'org-a', employeeId: 'employee-a' },
|
||||
{ id: 'slot-foreign', tenantId: TENANT_B, organizationId: 'org-b', employeeId: 'employee-b' }
|
||||
];
|
||||
|
||||
const ACTIVITIES = [
|
||||
{ id: 'activity-own', tenantId: TENANT_A, employeeId: 'employee-a' },
|
||||
{ id: 'activity-foreign', tenantId: TENANT_B, employeeId: 'employee-b' }
|
||||
];
|
||||
|
||||
const matches = (row: Record<string, any>, where: Record<string, any>) =>
|
||||
Object.entries(where).every(([key, value]) => {
|
||||
if (value === undefined) {
|
||||
return true;
|
||||
}
|
||||
if (value instanceof FindOperator) {
|
||||
return (value.value as any[]).includes(row[key]);
|
||||
}
|
||||
return row[key] === value;
|
||||
});
|
||||
|
||||
function createHandler() {
|
||||
const timeSlotRepository = {
|
||||
findOne: jest.fn(async ({ where }: any) => SLOTS.find((slot) => matches(slot, where)) ?? null),
|
||||
update: jest.fn(async () => ({ affected: 1 }))
|
||||
};
|
||||
const activityRepository = {
|
||||
find: jest.fn(async ({ where }: any) => ACTIVITIES.filter((activity) => matches(activity, where))),
|
||||
save: jest.fn(async (activities: any[]) => activities),
|
||||
metadata: { findRelationWithPropertyPath: (): undefined => undefined },
|
||||
manager: {}
|
||||
};
|
||||
const handler = new UpdateTimeSlotHandler(timeSlotRepository as any, activityRepository as any);
|
||||
return { handler, timeSlotRepository, activityRepository };
|
||||
}
|
||||
|
||||
function asCaller(options: { tenantId?: string | null; employeeId?: string | null; canChangeEmployee?: boolean }) {
|
||||
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(
|
||||
(options.tenantId === undefined ? TENANT_A : options.tenantId) as any
|
||||
);
|
||||
jest.spyOn(RequestContext, 'currentUser').mockReturnValue({ employeeId: options.employeeId ?? null } as any);
|
||||
jest.spyOn(RequestContext, 'hasPermission').mockImplementation(
|
||||
(permission) => !!options.canChangeEmployee && permission === PermissionsEnum.CHANGE_SELECTED_EMPLOYEE
|
||||
);
|
||||
}
|
||||
|
||||
describe('UpdateTimeSlotHandler (GHSA-6qvm-3wg4-26w4)', () => {
|
||||
afterEach(() => jest.restoreAllMocks());
|
||||
|
||||
it('CONTROL: the pre-fix lookup of a super admin (no employee filter) finds the foreign slot', async () => {
|
||||
const { timeSlotRepository } = createHandler();
|
||||
const employeeId: string = undefined; // input.employeeId of a CHANGE_SELECTED_EMPLOYEE holder
|
||||
|
||||
const found = await timeSlotRepository.findOne({
|
||||
where: { ...(employeeId ? { employeeId } : {}), id: 'slot-foreign' }
|
||||
});
|
||||
|
||||
expect(found?.tenantId).toBe(TENANT_B);
|
||||
});
|
||||
|
||||
it("does not find, update or return another tenant's slot for a super admin", async () => {
|
||||
const { handler, timeSlotRepository, activityRepository } = createHandler();
|
||||
asCaller({ canChangeEmployee: true });
|
||||
|
||||
await expect(
|
||||
handler.execute(new UpdateTimeSlotCommand('slot-foreign', { overall: 0, activities: [{ title: 'x' } as any] }))
|
||||
).resolves.toBeNull();
|
||||
|
||||
expect(timeSlotRepository.findOne).toHaveBeenCalledWith({ where: { tenantId: TENANT_A, id: 'slot-foreign' } });
|
||||
expect(timeSlotRepository.update).not.toHaveBeenCalled();
|
||||
expect(activityRepository.save).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('updates the own slot with the allowed fields only, scoped by tenant', async () => {
|
||||
const { handler, timeSlotRepository } = createHandler();
|
||||
asCaller({ employeeId: 'employee-a' });
|
||||
|
||||
await handler.execute(
|
||||
new UpdateTimeSlotCommand('slot-own', {
|
||||
duration: 600,
|
||||
keyboard: 3,
|
||||
mouse: 4,
|
||||
overall: 5,
|
||||
tenantId: TENANT_B,
|
||||
organizationId: 'org-b',
|
||||
employeeId: 'employee-b',
|
||||
timeLogs: [{ id: 'log' } as any]
|
||||
} as any)
|
||||
);
|
||||
|
||||
expect(timeSlotRepository.update).toHaveBeenCalledWith(
|
||||
{ id: 'slot-own', tenantId: TENANT_A },
|
||||
{ duration: 600, keyboard: 3, mouse: 4, overall: 5 }
|
||||
);
|
||||
expect(UPDATABLE_TIME_SLOT_FIELDS).not.toEqual(
|
||||
expect.arrayContaining(['tenantId', 'organizationId', 'employeeId', 'activities'])
|
||||
);
|
||||
});
|
||||
|
||||
it('pins a caller without CHANGE_SELECTED_EMPLOYEE to their own employee, ignoring the body', async () => {
|
||||
const { handler, timeSlotRepository } = createHandler();
|
||||
asCaller({ employeeId: 'employee-a' });
|
||||
|
||||
await handler.execute(new UpdateTimeSlotCommand('slot-own', { employeeId: 'employee-b', overall: 1 } as any));
|
||||
|
||||
expect(timeSlotRepository.findOne).toHaveBeenCalledWith({
|
||||
where: { employeeId: 'employee-a', tenantId: TENANT_A, id: 'slot-own' }
|
||||
});
|
||||
});
|
||||
|
||||
it('fails closed for a caller with neither CHANGE_SELECTED_EMPLOYEE nor an employee record', async () => {
|
||||
const { handler, timeSlotRepository } = createHandler();
|
||||
asCaller({ employeeId: null });
|
||||
|
||||
await expect(handler.execute(new UpdateTimeSlotCommand('slot-own', { overall: 1 }))).resolves.toBeNull();
|
||||
expect(timeSlotRepository.findOne).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('fails closed without a tenant', async () => {
|
||||
const { handler, timeSlotRepository } = createHandler();
|
||||
asCaller({ tenantId: null, canChangeEmployee: true });
|
||||
|
||||
await expect(handler.execute(new UpdateTimeSlotCommand('slot-own', { overall: 1 }))).resolves.toBeNull();
|
||||
expect(timeSlotRepository.findOne).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("saves body activities on the slot's own scope, and never under a foreign activity id", async () => {
|
||||
const { handler, timeSlotRepository, activityRepository } = createHandler();
|
||||
asCaller({ employeeId: 'employee-a' });
|
||||
|
||||
await handler.execute(
|
||||
new UpdateTimeSlotCommand('slot-own', {
|
||||
activities: [
|
||||
{ id: 'activity-foreign', title: 'steal', tenantId: TENANT_B, employee: { id: 'employee-b' } },
|
||||
{ id: 'activity-own', title: 'resend' },
|
||||
{ title: 'new', organizationId: 'org-b', timeSlotId: 'slot-foreign' }
|
||||
] as any[]
|
||||
})
|
||||
);
|
||||
|
||||
const saved = activityRepository.save.mock.calls[0][0];
|
||||
expect(saved.map((activity: any) => activity.id)).toEqual([undefined, 'activity-own', undefined]);
|
||||
for (const activity of saved) {
|
||||
expect(activity).toMatchObject({
|
||||
tenantId: TENANT_A,
|
||||
organizationId: 'org-a',
|
||||
employeeId: 'employee-a',
|
||||
timeSlotId: 'slot-own'
|
||||
});
|
||||
expect(activity.employee).toBeUndefined();
|
||||
}
|
||||
// The relation array never reaches update() (TypeORM cannot update a one-to-many that way).
|
||||
expect(timeSlotRepository.update).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
+131
-48
@@ -1,13 +1,30 @@
|
||||
import { CommandHandler, ICommandHandler } from '@nestjs/cqrs';
|
||||
import * as moment from 'moment';
|
||||
import { PermissionsEnum } from '@gauzy/contracts';
|
||||
import { ID, ITimeSlot, PermissionsEnum } from '@gauzy/contracts';
|
||||
import { RequestContext } from '../../../../core/context';
|
||||
import { Activity } from '../../../activity/activity.entity';
|
||||
import { scopeActivitiesForWrite } from '../../../activity/activity-write-scope.helper';
|
||||
import { UpdateTimeSlotCommand } from '../update-time-slot.command';
|
||||
import { TimeSlot } from './../../time-slot.entity';
|
||||
import { TypeOrmTimeSlotRepository } from '../../repository/type-orm-time-slot.repository';
|
||||
import { TypeOrmActivityRepository } from '../../../activity/repository/type-orm-activity.repository';
|
||||
|
||||
/**
|
||||
* The only time slot columns a client may change through PUT /timesheet/time-slot/:id. Everything
|
||||
* else — tenantId, organizationId, employeeId, relation arrays — stays as stored (GHSA-6qvm-3wg4-26w4).
|
||||
*/
|
||||
export const UPDATABLE_TIME_SLOT_FIELDS = [
|
||||
'duration',
|
||||
'keyboard',
|
||||
'mouse',
|
||||
'overall',
|
||||
'location',
|
||||
'startedAt',
|
||||
'kbMouseActivity',
|
||||
'locationActivity',
|
||||
'customActivity'
|
||||
] as const;
|
||||
|
||||
@CommandHandler(UpdateTimeSlotCommand)
|
||||
export class UpdateTimeSlotHandler implements ICommandHandler<UpdateTimeSlotCommand> {
|
||||
constructor(
|
||||
@@ -18,54 +35,120 @@ export class UpdateTimeSlotHandler implements ICommandHandler<UpdateTimeSlotComm
|
||||
public async execute(command: UpdateTimeSlotCommand): Promise<TimeSlot> {
|
||||
const { input, id } = command;
|
||||
|
||||
let employeeId = input.employeeId;
|
||||
if (!RequestContext.hasPermission(PermissionsEnum.CHANGE_SELECTED_EMPLOYEE)) {
|
||||
const user = RequestContext.currentUser();
|
||||
employeeId = user.employeeId;
|
||||
}
|
||||
|
||||
let timeSlot = await this.typeOrmTimeSlotRepository.findOne({
|
||||
where: {
|
||||
...(employeeId ? { employeeId: employeeId } : {}),
|
||||
id: id
|
||||
}
|
||||
});
|
||||
|
||||
if (timeSlot) {
|
||||
if (input.startedAt) {
|
||||
input.startedAt = moment(input.startedAt)
|
||||
//.set('minute', 0)
|
||||
.set('millisecond', 0)
|
||||
.toDate();
|
||||
}
|
||||
|
||||
let newActivities = [];
|
||||
if (input.activities) {
|
||||
newActivities = input.activities.map((activity) => {
|
||||
activity = new Activity(activity);
|
||||
activity.employeeId = timeSlot.employeeId;
|
||||
activity.tenantId = RequestContext.currentTenantId();
|
||||
return activity;
|
||||
});
|
||||
await this.typeOrmActivityRepository.save(newActivities);
|
||||
input.activities = (timeSlot.activities || []).concat(newActivities);
|
||||
}
|
||||
await this.typeOrmTimeSlotRepository.update(id, input);
|
||||
|
||||
timeSlot = await this.typeOrmTimeSlotRepository.findOne({
|
||||
where: {
|
||||
...(employeeId ? { employeeId } : {}),
|
||||
id
|
||||
},
|
||||
relations: {
|
||||
timeLogs: true,
|
||||
screenshots: true,
|
||||
activities: true
|
||||
}
|
||||
});
|
||||
return timeSlot;
|
||||
} else {
|
||||
// The slot is looked up and written inside the caller's tenant only. The raw repository has no
|
||||
// tenant scoping of its own, and OrganizationPermissionGuard is not an ownership check for every
|
||||
// caller, so a missing tenant fails closed instead of matching any tenant's slot.
|
||||
const tenantId = RequestContext.currentTenantId();
|
||||
if (!tenantId) {
|
||||
return null;
|
||||
}
|
||||
|
||||
// A body employeeId only narrows the lookup, and only for callers who may act for any employee
|
||||
// of the tenant. Everyone else is pinned to their own employee; without one there is no slot
|
||||
// they may edit (an absent filter would match every employee's slot).
|
||||
const scope = this.resolveEmployeeScope(input.employeeId);
|
||||
if (!scope) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const where = {
|
||||
...(scope.employeeId ? { employeeId: scope.employeeId } : {}),
|
||||
tenantId,
|
||||
id
|
||||
};
|
||||
|
||||
const timeSlot = await this.typeOrmTimeSlotRepository.findOne({ where });
|
||||
|
||||
if (!timeSlot) {
|
||||
return null;
|
||||
}
|
||||
|
||||
if (Array.isArray(input.activities) && input.activities.length) {
|
||||
await this.saveActivities(input.activities, timeSlot, tenantId);
|
||||
}
|
||||
|
||||
const changes = this.collectChanges(input);
|
||||
|
||||
if (Object.keys(changes).length) {
|
||||
await this.typeOrmTimeSlotRepository.update({ id: timeSlot.id, tenantId }, changes);
|
||||
}
|
||||
|
||||
return await this.typeOrmTimeSlotRepository.findOne({
|
||||
where,
|
||||
relations: {
|
||||
timeLogs: true,
|
||||
screenshots: true,
|
||||
activities: true
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* The employee the lookup is narrowed to: the body's for a caller who may act for any employee of
|
||||
* the tenant, the caller's own otherwise. `null` means the request may not edit any slot at all —
|
||||
* as opposed to an absent `employeeId`, which a caller who may act for anyone is allowed to omit.
|
||||
*
|
||||
* @param requestedEmployeeId - The employeeId carried by the request body.
|
||||
*/
|
||||
private resolveEmployeeScope(requestedEmployeeId: ID | undefined): { employeeId?: ID } | null {
|
||||
if (RequestContext.hasPermission(PermissionsEnum.CHANGE_SELECTED_EMPLOYEE)) {
|
||||
return { employeeId: requestedEmployeeId ?? undefined };
|
||||
}
|
||||
|
||||
const own = RequestContext.currentUser()?.employeeId;
|
||||
|
||||
return own ? { employeeId: own } : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* The changes the body is allowed to make, taken from {@link UPDATABLE_TIME_SLOT_FIELDS} only, so
|
||||
* tenantId / organizationId / employeeId and the relation arrays can never be mass-assigned.
|
||||
*
|
||||
* @param input - The request body.
|
||||
*/
|
||||
private collectChanges(input: Partial<ITimeSlot>): Partial<ITimeSlot> {
|
||||
const changes: Partial<ITimeSlot> = {};
|
||||
|
||||
for (const field of UPDATABLE_TIME_SLOT_FIELDS) {
|
||||
if (input[field] !== undefined) {
|
||||
changes[field] = input[field];
|
||||
}
|
||||
}
|
||||
|
||||
if (changes.startedAt) {
|
||||
changes.startedAt = moment(changes.startedAt)
|
||||
//.set('minute', 0)
|
||||
.set('millisecond', 0)
|
||||
.toDate();
|
||||
}
|
||||
|
||||
return changes;
|
||||
}
|
||||
|
||||
/**
|
||||
* Saves the body's activities on their own and attaches them to THIS slot: their ids, relation
|
||||
* objects and scope columns come from the slot, never from the body.
|
||||
*
|
||||
* @param activities - The activities carried by the request body.
|
||||
* @param timeSlot - The slot they are attached to.
|
||||
* @param tenantId - The caller's tenant.
|
||||
*/
|
||||
private async saveActivities(activities: ITimeSlot['activities'], timeSlot: TimeSlot, tenantId: ID): Promise<void> {
|
||||
const scoped = await scopeActivitiesForWrite(
|
||||
activities.map((activity) => ({ ...activity })),
|
||||
this.typeOrmActivityRepository,
|
||||
{ tenantId, employeeId: timeSlot.employeeId }
|
||||
);
|
||||
|
||||
const entities = scoped.map((activity) => {
|
||||
const entity = new Activity(activity);
|
||||
entity.employeeId = timeSlot.employeeId;
|
||||
entity.organizationId = timeSlot.organizationId;
|
||||
entity.tenantId = tenantId;
|
||||
entity.timeSlotId = timeSlot.id;
|
||||
return entity;
|
||||
});
|
||||
|
||||
await this.typeOrmActivityRepository.save(entities);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,5 +4,5 @@ import { ID, ITimeSlot } from '@gauzy/contracts';
|
||||
export class UpdateTimeSlotCommand implements ICommand {
|
||||
static readonly type = '[TimeSlot] update';
|
||||
|
||||
constructor(public readonly id: ID, public readonly input: ITimeSlot) {}
|
||||
constructor(public readonly id: ID, public readonly input: Partial<ITimeSlot>) {}
|
||||
}
|
||||
|
||||
@@ -1,2 +1,3 @@
|
||||
export * from './time-slot-query.dto';
|
||||
export * from './delete-time-slot.dto';
|
||||
export * from './update-time-slot.dto';
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
import { ApiPropertyOptional } from '@nestjs/swagger';
|
||||
import { IsArray, IsOptional } from 'class-validator';
|
||||
import { IActivity, ID } from '@gauzy/contracts';
|
||||
|
||||
/**
|
||||
* Body of PUT /timesheet/time-slot/:id.
|
||||
*
|
||||
* Declares exactly what the timer clients send (the desktop timer sends duration, keyboard, mouse,
|
||||
* overall and activities), so `whitelist: true` drops everything else — tenantId, organizationId,
|
||||
* relation objects — before it can reach the update (GHSA-6qvm-3wg4-26w4). The value checks are left
|
||||
* loose on purpose: the endpoint accepted these fields unvalidated before, and old desktop builds
|
||||
* must keep working. The handler still picks its own allow-list.
|
||||
*/
|
||||
export class UpdateTimeSlotDTO {
|
||||
@ApiPropertyOptional({ type: () => Number })
|
||||
@IsOptional()
|
||||
readonly duration?: number;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Number })
|
||||
@IsOptional()
|
||||
readonly keyboard?: number;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Number })
|
||||
@IsOptional()
|
||||
readonly mouse?: number;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Number })
|
||||
@IsOptional()
|
||||
readonly overall?: number;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Number })
|
||||
@IsOptional()
|
||||
readonly location?: number;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Date })
|
||||
@IsOptional()
|
||||
startedAt?: Date;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Object })
|
||||
@IsOptional()
|
||||
readonly kbMouseActivity?: any;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Object })
|
||||
@IsOptional()
|
||||
readonly locationActivity?: any;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Object })
|
||||
@IsOptional()
|
||||
readonly customActivity?: any;
|
||||
|
||||
/**
|
||||
* Only narrows which time slot is addressed, and only for callers holding
|
||||
* CHANGE_SELECTED_EMPLOYEE. It is never written.
|
||||
*/
|
||||
@ApiPropertyOptional({ type: () => String })
|
||||
@IsOptional()
|
||||
readonly employeeId?: ID;
|
||||
|
||||
@ApiPropertyOptional({ type: () => Array, isArray: true })
|
||||
@IsOptional()
|
||||
@IsArray()
|
||||
activities?: IActivity[];
|
||||
}
|
||||
@@ -14,7 +14,7 @@ import { UUIDValidationPipe, UseValidationPipe } from './../../shared/pipes';
|
||||
import { CreateTimeSlotCommand, DeleteTimeSlotCommand, UpdateTimeSlotCommand } from './commands';
|
||||
import { TimeSlot } from './time-slot.entity';
|
||||
import { TimeSlotService } from './time-slot.service';
|
||||
import { DeleteTimeSlotDTO, TimeSlotQueryDTO } from './dto';
|
||||
import { DeleteTimeSlotDTO, TimeSlotQueryDTO, UpdateTimeSlotDTO } from './dto';
|
||||
|
||||
@ApiTags('TimeSlot')
|
||||
@UseGuards(TenantPermissionGuard, PermissionGuard)
|
||||
@@ -106,7 +106,8 @@ export class TimeSlotController {
|
||||
@Permissions(PermissionsEnum.ALLOW_MODIFY_TIME)
|
||||
@OrganizationPolicyTarget(TimeSlot)
|
||||
@Put('/:id')
|
||||
async update(@Param('id', UUIDValidationPipe) id: ID, @Body() request: ITimeSlot): Promise<ITimeSlot> {
|
||||
@UseValidationPipe({ whitelist: true })
|
||||
async update(@Param('id', UUIDValidationPipe) id: ID, @Body() request: UpdateTimeSlotDTO): Promise<ITimeSlot> {
|
||||
return await this._commandBus.execute(new UpdateTimeSlotCommand(id, request));
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,125 @@
|
||||
/**
|
||||
* 🛑 This import must stay FIRST — see the note in `time-log.service.spec.ts`: loading the entity
|
||||
* barrel before any core service keeps `@IsEmployeeBelongsToOrganization()` from resolving to
|
||||
* `undefined` while `dashboard.entity.ts` applies it.
|
||||
*/
|
||||
import '../../core/entities/internal';
|
||||
import { Test, TestingModule } from '@nestjs/testing';
|
||||
import { CommandBus } from '@nestjs/cqrs';
|
||||
import { IGetTimeSlotInput } from '@gauzy/contracts';
|
||||
import { MultiORMEnum } from '../../core/utils';
|
||||
import { mockRequestContext } from '../testing/recording-query-builder';
|
||||
import { TypeOrmTimeSlotRepository } from './repository/type-orm-time-slot.repository';
|
||||
import { TimeSlotService } from './time-slot.service';
|
||||
|
||||
const TENANT_ID = '5a1c2f0e-6d3b-4c8a-9e2f-1b7d4a6c8e90';
|
||||
const ORGANIZATION_ID = '0f9e8d7c-6b5a-4c3d-8e2f-1a0b9c8d7e6f';
|
||||
const USER_ID = 'c3b2a190-8f7e-4d6c-9b5a-4e3d2c1b0a9f';
|
||||
const OWN_EMPLOYEE_ID = '7e6d5c4b-3a29-4180-9f8e-7d6c5b4a3928';
|
||||
const TARGET_EMPLOYEE_ID = '1a2b3c4d-5e6f-4a7b-8c9d-0e1f2a3b4c5d';
|
||||
|
||||
/** The subset of the query builder `getTimeSlots()` drives; it only has to execute, not filter. */
|
||||
class TimeSlotQueryDouble {
|
||||
readonly alias = 'time_slot';
|
||||
leftJoin(): this {
|
||||
return this;
|
||||
}
|
||||
innerJoin(): this {
|
||||
return this;
|
||||
}
|
||||
setFindOptions(): this {
|
||||
return this;
|
||||
}
|
||||
where(factory: unknown): this {
|
||||
if (typeof factory === 'function') {
|
||||
factory(this);
|
||||
}
|
||||
return this;
|
||||
}
|
||||
andWhere(): this {
|
||||
return this;
|
||||
}
|
||||
addOrderBy(): this {
|
||||
return this;
|
||||
}
|
||||
async getMany(): Promise<never[]> {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* GHSA-6qvm-3wg4-26w4 — `getTimeSlots()` builds its own query, so the never-matching employee
|
||||
* condition of the CRUD reads (`findConditionsWithoutOwnEmployee`) never applies to it. The employee
|
||||
* predicate is only added when `employeeIds` is non-empty, and `employeeIds` is only narrowed for a
|
||||
* caller who HAS an employee record — so a caller with neither the permission nor an employee read
|
||||
* the whole organization's slots, or the ones they named in the body.
|
||||
*/
|
||||
describe('TimeSlotService.getTimeSlots employee scope', () => {
|
||||
let service: TimeSlotService;
|
||||
let createQueryBuilder: jest.Mock;
|
||||
|
||||
const request: IGetTimeSlotInput = {
|
||||
organizationId: ORGANIZATION_ID,
|
||||
employeeIds: [TARGET_EMPLOYEE_ID],
|
||||
startDate: '2026-01-05T00:00:00.000Z',
|
||||
endDate: '2026-01-12T00:00:00.000Z'
|
||||
} as IGetTimeSlotInput;
|
||||
|
||||
beforeEach(async () => {
|
||||
createQueryBuilder = jest.fn(() => new TimeSlotQueryDouble());
|
||||
|
||||
const module: TestingModule = await Test.createTestingModule({ providers: [TimeSlotService] })
|
||||
.useMocker((token) => {
|
||||
if (token === TypeOrmTimeSlotRepository) {
|
||||
return { metadata: { tableName: 'time_slot' }, createQueryBuilder };
|
||||
}
|
||||
if (token === CommandBus) {
|
||||
return { execute: jest.fn().mockResolvedValue([]) };
|
||||
}
|
||||
return {};
|
||||
})
|
||||
.compile();
|
||||
|
||||
service = module.get<TimeSlotService>(TimeSlotService);
|
||||
Object.defineProperty(service, 'ormType', { value: MultiORMEnum.TypeORM });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
jest.restoreAllMocks();
|
||||
});
|
||||
|
||||
it('returns nothing, without querying, for a caller with neither the permission nor an employee record', async () => {
|
||||
mockRequestContext({
|
||||
tenantId: TENANT_ID,
|
||||
user: { id: USER_ID, employeeId: null },
|
||||
canChangeSelectedEmployee: false
|
||||
});
|
||||
|
||||
await expect(service.getTimeSlots(request)).resolves.toEqual([]);
|
||||
expect(createQueryBuilder).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('CONTROL: the same caller state WITH an employee record still runs the query', async () => {
|
||||
mockRequestContext({
|
||||
tenantId: TENANT_ID,
|
||||
user: { id: USER_ID, employeeId: OWN_EMPLOYEE_ID },
|
||||
canChangeSelectedEmployee: false
|
||||
});
|
||||
|
||||
await service.getTimeSlots(request);
|
||||
|
||||
expect(createQueryBuilder).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('CONTROL: a caller holding CHANGE_SELECTED_EMPLOYEE still runs the query', async () => {
|
||||
mockRequestContext({
|
||||
tenantId: TENANT_ID,
|
||||
user: { id: USER_ID, employeeId: null },
|
||||
canChangeSelectedEmployee: true
|
||||
});
|
||||
|
||||
await service.getTimeSlots(request);
|
||||
|
||||
expect(createQueryBuilder).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,6 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { CommandBus } from '@nestjs/cqrs';
|
||||
import { SelectQueryBuilder } from 'typeorm';
|
||||
import { FindOptionsWhere, SelectQueryBuilder } from 'typeorm';
|
||||
import { PermissionsEnum, IGetTimeSlotInput, ID, ITimeSlot, ITimeSlotMinute } from '@gauzy/contracts';
|
||||
import { isEmpty, isNotEmpty } from '@gauzy/utils';
|
||||
import { RequestContext } from '../../core/context';
|
||||
@@ -29,6 +29,15 @@ export class TimeSlotService extends TenantAwareCrudService<TimeSlot> {
|
||||
super(typeOrmTimeSlotRepository, mikroOrmTimeSlotRepository);
|
||||
}
|
||||
|
||||
/**
|
||||
* Time slots are personal: a caller without CHANGE_SELECTED_EMPLOYEE and without an employee record
|
||||
* of their own (a custom role holding TIME_TRACKER, say) must not fall back to the tenant-wide scope
|
||||
* of the CRUD reads and deletes (GHSA-6qvm-3wg4-26w4). They match nothing instead.
|
||||
*/
|
||||
protected findConditionsWithoutOwnEmployee(): FindOptionsWhere<TimeSlot> {
|
||||
return this.neverMatchingEmployeeCondition();
|
||||
}
|
||||
|
||||
/**
|
||||
* Retrieves time slots based on the provided input parameters.
|
||||
*
|
||||
@@ -67,6 +76,15 @@ export class TimeSlotService extends TenantAwareCrudService<TimeSlot> {
|
||||
employeeIds = [user.employeeId];
|
||||
}
|
||||
|
||||
// Fail closed for a caller who may not act for other employees and has no employee record of
|
||||
// their own: the employee predicate below is only applied when `employeeIds` is non-empty, so
|
||||
// such a caller would read the whole organization's slots — or the body-supplied employees'
|
||||
// (GHSA-6qvm-3wg4-26w4). The CRUD reads already match nothing in that state
|
||||
// (findConditionsWithoutOwnEmployee); this hand-built query carries the same rule.
|
||||
if (!hasChangeSelectedEmployeePermission && !user.employeeId) {
|
||||
return [];
|
||||
}
|
||||
|
||||
// Calculate start and end dates using a utility function
|
||||
const { start, end } = getDateRangeFormat(
|
||||
moment.utc(startDate || moment().startOf('day')),
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
// Must stay first: loads the entity graph before the handler pulls an entity (see candidate.update.handler.spec.ts).
|
||||
import '../../../core/entities/internal';
|
||||
|
||||
import { UserCreateCommand } from '../user.create.command';
|
||||
import { UserCreateHandler } from './user.create.handler';
|
||||
|
||||
/**
|
||||
* GHSA-jh6m-9fxr-rx3c (same class as the candidate → user cascade).
|
||||
*
|
||||
* `CrudService.create()` upserts when the payload carries a primary key (TypeORM `save()`, and the
|
||||
* MikroORM branch loads the row and `assign()`s onto it). So a body id turned "create a user" into
|
||||
* "overwrite that user": POST /candidate and POST /employee hash the request's `password` into the
|
||||
* payload they hand this command, which meant `user: { id: <an admin of my own tenant> }` reset that
|
||||
* account's password and demoted its role. The tenant check cannot see it — the victim is a member
|
||||
* of the caller's own tenant — and no caller of this command wants an update.
|
||||
*/
|
||||
describe('UserCreateHandler (GHSA-jh6m-9fxr-rx3c)', () => {
|
||||
const build = () => {
|
||||
const create = jest.fn(async (input: any) => ({ id: 'new-user', ...input }));
|
||||
const assertCanAssignRoles = jest.fn(async () => undefined);
|
||||
const handler = new UserCreateHandler({ create, assertCanAssignRoles } as any);
|
||||
return { handler, create, assertCanAssignRoles };
|
||||
};
|
||||
|
||||
const takeover = {
|
||||
id: 'victim-super-admin',
|
||||
email: 'attacker@evil.test',
|
||||
hash: '$2b$10$attacker',
|
||||
roleId: 'candidate-role'
|
||||
} as any;
|
||||
|
||||
it('CONTROL: the pre-fix payload still names the victim row, which create() would upsert', () => {
|
||||
expect(takeover).toMatchObject({ id: 'victim-super-admin', hash: '$2b$10$attacker' });
|
||||
});
|
||||
|
||||
it('never passes a body-supplied id on to create()', async () => {
|
||||
const { handler, create } = build();
|
||||
|
||||
await handler.execute(new UserCreateCommand(takeover));
|
||||
|
||||
const [persisted] = create.mock.calls[0];
|
||||
expect(persisted).not.toHaveProperty('id');
|
||||
expect(persisted).toMatchObject({ email: 'attacker@evil.test', hash: '$2b$10$attacker' });
|
||||
// The command input is left as it was.
|
||||
expect(takeover.id).toBe('victim-super-admin');
|
||||
});
|
||||
|
||||
it('still validates the role being assigned, whichever form it arrives in', async () => {
|
||||
const { handler, assertCanAssignRoles } = build();
|
||||
|
||||
// The two forms agree, which is the only shape `normalizeRolePayload` lets through
|
||||
// (GHSA-x4mv-fhwj-g3rp rejects a `role`/`roleId` pair that names two different roles, so that
|
||||
// the id which is CHECKED is always the id that is persisted).
|
||||
await handler.execute(
|
||||
new UserCreateCommand({ email: 'new@test', roleId: 'the-role', role: { id: 'the-role' } } as any)
|
||||
);
|
||||
|
||||
// `assertCanAssignRoles` takes the whole payload and extracts every role form itself, so a
|
||||
// caller cannot forget one (GHSA-x4mv-fhwj-g3rp).
|
||||
expect(assertCanAssignRoles).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ roleId: 'the-role', role: { id: 'the-role' } })
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses a payload whose role and roleId name different roles', async () => {
|
||||
const { handler, create } = build();
|
||||
|
||||
await expect(
|
||||
handler.execute(
|
||||
new UserCreateCommand({ email: 'new@test', roleId: 'flat-role', role: { id: 'relation-role' } } as any)
|
||||
)
|
||||
).rejects.toThrow(/same role/i);
|
||||
|
||||
expect(create).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -1,11 +1,14 @@
|
||||
import { Logger } from '@nestjs/common';
|
||||
import { CommandHandler, ICommandHandler } from '@nestjs/cqrs';
|
||||
import { IUser } from '@gauzy/contracts';
|
||||
import { ID, IUser, IUserCreateInput } 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> {
|
||||
private readonly logger = new Logger(UserCreateHandler.name);
|
||||
|
||||
constructor(private readonly userService: UserService) {}
|
||||
|
||||
/**
|
||||
@@ -15,7 +18,17 @@ export class UserCreateHandler implements ICommandHandler<UserCreateCommand> {
|
||||
* @returns A Promise resolving to the created IUser object.
|
||||
*/
|
||||
public async execute(command: UserCreateCommand): Promise<IUser> {
|
||||
const { input } = command;
|
||||
const { id, ...input } = (command.input ?? {}) as IUserCreateInput & { id?: ID };
|
||||
|
||||
// A create never adopts an existing row. `CrudService.create()` upserts when the payload carries
|
||||
// a primary key, so a body id turned "create a user" into "overwrite that user": POST /candidate
|
||||
// and POST /employee hash the body `password` into the payload, so `user: { id: <an admin of my
|
||||
// tenant> }` reset that account's password and demoted its role — a takeover the tenant check
|
||||
// cannot see, since the victim is a member of the caller's own tenant (GHSA-jh6m-9fxr-rx3c).
|
||||
// Every caller of this command means to INSERT a user, so the id is dropped rather than refused.
|
||||
if (id) {
|
||||
this.logger.warn(`Ignoring the body-supplied user id on a create: ${id}`);
|
||||
}
|
||||
|
||||
// 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
|
||||
|
||||
Reference in New Issue
Block a user