From 78bb425a6790d3645f0a1b054d50557cc83647d9 Mon Sep 17 00:00:00 2001 From: amberly-d <294444926+amberly-d@users.noreply.github.com> Date: Fri, 16 Oct 2026 19:57:29 +0100 Subject: [PATCH] fix(assets): bound and sort list queries, escape search wildcards --- backend/src/assets/assets.controller.ts | 21 ++-------- backend/src/assets/assets.service.spec.ts | 41 +++++++++++++++++++ backend/src/assets/assets.service.ts | 28 +++++++++++-- .../assets/dto/list-assets-query.dto.spec.ts | 24 +++++++++++ .../src/assets/dto/list-assets-query.dto.ts | 29 +++++++++++++ backend/src/common/utils/list-query.spec.ts | 31 ++++++++++++++ backend/src/common/utils/list-query.ts | 26 ++++++++++++ ...0100000000-AddAssetSearchTrigramIndexes.ts | 35 ++++++++++++++++ backend/src/users/dto/list-users-query.dto.ts | 29 +++++++++++++ backend/src/users/users.controller.ts | 10 ++--- backend/src/users/users.service.ts | 24 +++++++++-- 11 files changed, 265 insertions(+), 33 deletions(-) create mode 100644 backend/src/assets/dto/list-assets-query.dto.spec.ts create mode 100644 backend/src/assets/dto/list-assets-query.dto.ts create mode 100644 backend/src/common/utils/list-query.spec.ts create mode 100644 backend/src/common/utils/list-query.ts create mode 100644 backend/src/migrations/1760100000000-AddAssetSearchTrigramIndexes.ts create mode 100644 backend/src/users/dto/list-users-query.dto.ts diff --git a/backend/src/assets/assets.controller.ts b/backend/src/assets/assets.controller.ts index 3c8cac60..f57f36fa 100644 --- a/backend/src/assets/assets.controller.ts +++ b/backend/src/assets/assets.controller.ts @@ -17,6 +17,7 @@ import { ApiResponse, } from '@nestjs/swagger'; import { AssetsService } from './assets.service'; +import { ListAssetsQueryDto } from './dto/list-assets-query.dto'; import { AssetHistoryService } from './asset-history.service'; import { ReservationsService } from '../reservations/reservations.service'; import { AssetHistoryAction } from './entities/asset-history-event.entity'; @@ -44,24 +45,8 @@ export class AssetsController { @Get() @ApiOperation({ summary: 'List all assets (paginated, filterable)' }) @ApiResponse({ status: 200, description: 'Paginated list of assets' }) - findAll( - @Query('search') search?: string, - @Query('categoryId') categoryId?: string, - @Query('departmentId') departmentId?: string, - @Query('locationId') locationId?: string, - @Query('status') status?: string, - @Query('page') page?: number, - @Query('limit') limit?: number, - ) { - return this.assetsService.findAll({ - search, - categoryId, - departmentId, - locationId, - status, - page, - limit, - }); + findAll(@Query() query: ListAssetsQueryDto) { + return this.assetsService.findAll(query); } @Post() diff --git a/backend/src/assets/assets.service.spec.ts b/backend/src/assets/assets.service.spec.ts index 8a54ce30..524fae59 100644 --- a/backend/src/assets/assets.service.spec.ts +++ b/backend/src/assets/assets.service.spec.ts @@ -86,4 +86,45 @@ describe('AssetsService', () => { expect(repository.softRemove).toHaveBeenCalledWith(mockAsset); }); + + describe('findAll', () => { + function mockQb() { + const qb: any = { + skip: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + addOrderBy: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + getManyAndCount: jest.fn().mockResolvedValue([[], 0]), + }; + jest.spyOn(repository, 'createQueryBuilder').mockReturnValue(qb); + return qb; + } + + it('escapes LIKE wildcards so "100%" matches literally', async () => { + const qb = mockQb(); + await service.findAll({ search: '100%' }); + const [sql, params] = qb.andWhere.mock.calls[0]; + expect(params).toEqual({ s: '%100\\%%' }); + expect(sql).toContain("ESCAPE '\\'"); + }); + + it('clamps limit to 100 and uses an id tiebreaker for stable pages', async () => { + const qb = mockQb(); + const result = await service.findAll({ page: 3, limit: 1_000_000 }); + expect(qb.take).toHaveBeenCalledWith(100); + expect(qb.skip).toHaveBeenCalledWith(200); + expect(qb.orderBy).toHaveBeenCalledWith('asset.createdAt', 'DESC'); + expect(qb.addOrderBy).toHaveBeenCalledWith('asset.id', 'ASC'); + expect(result.limit).toBe(100); + }); + + it('sorts by a whitelisted column and ignores unknown ones', async () => { + const qb = mockQb(); + await service.findAll({ sortBy: 'name', sortOrder: 'ASC' }); + expect(qb.orderBy).toHaveBeenCalledWith('asset.name', 'ASC'); + await service.findAll({ sortBy: 'passwordHash; --' }); + expect(qb.orderBy).toHaveBeenLastCalledWith('asset.createdAt', 'DESC'); + }); + }); }); diff --git a/backend/src/assets/assets.service.ts b/backend/src/assets/assets.service.ts index 7b2bf0fa..da57bca4 100644 --- a/backend/src/assets/assets.service.ts +++ b/backend/src/assets/assets.service.ts @@ -6,6 +6,12 @@ import { import { InjectRepository } from '@nestjs/typeorm'; import { DataSource, Repository } from 'typeorm'; import { EventEmitter2 } from '@nestjs/event-emitter'; +import { + LIKE_ESCAPE, + containsPattern, + resolveSort, +} from '../common/utils/list-query'; +import { ASSET_SORT_COLUMNS } from './dto/list-assets-query.dto'; import { Asset, ASSET_DETAIL_RELATIONS } from './entities/asset.entity'; import { AssetLifecycleService, AssetStatus } from './asset-lifecycle.service'; import { AuditLogsService } from '../audit-logs/audit-logs.service'; @@ -25,6 +31,9 @@ export interface BulkResult { skipped: string[]; } +/** Hard ceiling on list page size, enforced again here as defence in depth. */ +const MAX_PAGE_SIZE = 100; + @Injectable() export class AssetsService { constructor( @@ -45,18 +54,29 @@ export class AssetsService { status?: string; page?: number; limit?: number; + sortBy?: string; + sortOrder?: string; }) { const page = query?.page || 1; - const limit = query?.limit || 20; + const limit = Math.min(query?.limit || 20, MAX_PAGE_SIZE); + const sort = resolveSort( + query?.sortBy, + query?.sortOrder, + ASSET_SORT_COLUMNS, + 'createdAt', + ); const qb = this.assetRepo .createQueryBuilder('asset') .skip((page - 1) * limit) - .take(limit); + .take(limit) + // `id` breaks ties so rows never move between pages. + .orderBy(`asset.${sort.column}`, sort.order) + .addOrderBy('asset.id', 'ASC'); if (query?.search) { qb.andWhere( - '(asset.name ILIKE :s OR asset.assetTag ILIKE :s OR asset.serialNumber ILIKE :s)', - { s: `%${query.search}%` }, + `(asset.name ILIKE :s ${LIKE_ESCAPE} OR asset.assetTag ILIKE :s ${LIKE_ESCAPE} OR asset.serialNumber ILIKE :s ${LIKE_ESCAPE})`, + { s: containsPattern(query.search) }, ); } if (query?.categoryId) diff --git a/backend/src/assets/dto/list-assets-query.dto.spec.ts b/backend/src/assets/dto/list-assets-query.dto.spec.ts new file mode 100644 index 00000000..c7a35e52 --- /dev/null +++ b/backend/src/assets/dto/list-assets-query.dto.spec.ts @@ -0,0 +1,24 @@ +import { plainToInstance } from 'class-transformer'; +import { validate } from 'class-validator'; +import { ListAssetsQueryDto } from './list-assets-query.dto'; + +const check = async (input: Record) => + validate(plainToInstance(ListAssetsQueryDto, input)); + +describe('ListAssetsQueryDto', () => { + it('accepts a normal query', async () => { + expect( + await check({ page: '2', limit: '50', sortBy: 'name', sortOrder: 'ASC' }), + ).toEqual([]); + }); + + it('rejects limit above 100 and page below 1', async () => { + expect((await check({ limit: '101' })).length).toBeGreaterThan(0); + expect((await check({ page: '0' })).length).toBeGreaterThan(0); + }); + + it('rejects non-whitelisted sort columns and directions', async () => { + expect((await check({ sortBy: 'serialNumber; DROP' })).length).toBeGreaterThan(0); + expect((await check({ sortOrder: 'sideways' })).length).toBeGreaterThan(0); + }); +}); diff --git a/backend/src/assets/dto/list-assets-query.dto.ts b/backend/src/assets/dto/list-assets-query.dto.ts new file mode 100644 index 00000000..9385dd71 --- /dev/null +++ b/backend/src/assets/dto/list-assets-query.dto.ts @@ -0,0 +1,29 @@ +import { ApiPropertyOptional } from '@nestjs/swagger'; +import { IsIn, IsOptional, IsString } from 'class-validator'; +import { PaginationQueryDto } from '../../common/dto/pagination.dto'; + +export const ASSET_SORT_COLUMNS = [ + 'createdAt', + 'updatedAt', + 'name', + 'assetTag', + 'status', + 'purchaseDate', +] as const; + +export class ListAssetsQueryDto extends PaginationQueryDto { + @ApiPropertyOptional({ enum: ASSET_SORT_COLUMNS, default: 'createdAt' }) + @IsOptional() + @IsIn(ASSET_SORT_COLUMNS) + sortBy?: string; + + @ApiPropertyOptional({ enum: ['ASC', 'DESC'], default: 'DESC' }) + @IsOptional() + @IsIn(['ASC', 'DESC']) + sortOrder?: 'ASC' | 'DESC'; + + @ApiPropertyOptional() @IsOptional() @IsString() categoryId?: string; + @ApiPropertyOptional() @IsOptional() @IsString() departmentId?: string; + @ApiPropertyOptional() @IsOptional() @IsString() locationId?: string; + @ApiPropertyOptional() @IsOptional() @IsString() status?: string; +} diff --git a/backend/src/common/utils/list-query.spec.ts b/backend/src/common/utils/list-query.spec.ts new file mode 100644 index 00000000..25d85781 --- /dev/null +++ b/backend/src/common/utils/list-query.spec.ts @@ -0,0 +1,31 @@ +import { containsPattern, escapeLike, resolveSort } from './list-query'; + +describe('list-query helpers', () => { + it('escapes LIKE wildcards and backslashes', () => { + expect(escapeLike('100%')).toBe('100\\%'); + expect(escapeLike('a_b')).toBe('a\\_b'); + expect(escapeLike('c:\\x')).toBe('c:\\\\x'); + expect(containsPattern('100%')).toBe('%100\\%%'); + }); + + it('leaves ordinary text untouched', () => { + expect(escapeLike('laptop')).toBe('laptop'); + }); + + it('only sorts by whitelisted columns', () => { + const allowed = ['name', 'createdAt']; + expect(resolveSort('name', 'ASC', allowed, 'createdAt')).toEqual({ + column: 'name', + order: 'ASC', + }); + expect(resolveSort('id; DROP TABLE x', 'ASC', allowed, 'createdAt')).toEqual({ + column: 'createdAt', + order: 'ASC', + }); + }); + + it('defaults to DESC for anything but ASC', () => { + expect(resolveSort(undefined, 'sideways', ['a'], 'a').order).toBe('DESC'); + expect(resolveSort(undefined, 'asc', ['a'], 'a').order).toBe('ASC'); + }); +}); diff --git a/backend/src/common/utils/list-query.ts b/backend/src/common/utils/list-query.ts new file mode 100644 index 00000000..dae4ffb8 --- /dev/null +++ b/backend/src/common/utils/list-query.ts @@ -0,0 +1,26 @@ +/** Escapes `%`, `_` and `\` so user input is matched literally by LIKE/ILIKE. */ +export function escapeLike(term: string): string { + return term.replace(/[\\%_]/g, '\\$&'); +} + +/** `%term%` pattern with the term escaped; pair with `ESCAPE '\'` in SQL. */ +export function containsPattern(term: string): string { + return `%${escapeLike(term)}%`; +} + +export const LIKE_ESCAPE = "ESCAPE '\\'"; + +/** + * Picks a sortable column from a whitelist and a direction. Unknown input + * falls back to the default, so a client can never inject a column name. + */ +export function resolveSort( + sortBy: string | undefined, + sortOrder: string | undefined, + allowed: readonly string[], + defaultColumn: string, +): { column: string; order: 'ASC' | 'DESC' } { + const column = sortBy && allowed.includes(sortBy) ? sortBy : defaultColumn; + const order = String(sortOrder).toUpperCase() === 'ASC' ? 'ASC' : 'DESC'; + return { column, order }; +} diff --git a/backend/src/migrations/1760100000000-AddAssetSearchTrigramIndexes.ts b/backend/src/migrations/1760100000000-AddAssetSearchTrigramIndexes.ts new file mode 100644 index 00000000..df504fba --- /dev/null +++ b/backend/src/migrations/1760100000000-AddAssetSearchTrigramIndexes.ts @@ -0,0 +1,35 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * Trigram GIN indexes so `ILIKE '%term%'` searches on assets (#1585) don't + * scan the whole table. Requires the `pg_trgm` extension. + */ +export class AddAssetSearchTrigramIndexes1760100000000 + implements MigrationInterface +{ + name = 'AddAssetSearchTrigramIndexes1760100000000'; + + public async up(queryRunner: QueryRunner): Promise { + if (!(await queryRunner.hasTable('assets'))) return; + await queryRunner.query(`CREATE EXTENSION IF NOT EXISTS pg_trgm`); + for (const [index, column] of [ + ['IDX_assets_name_trgm', 'name'], + ['IDX_assets_asset_tag_trgm', '"assetTag"'], + ['IDX_assets_serial_number_trgm', '"serialNumber"'], + ]) { + await queryRunner.query( + `CREATE INDEX IF NOT EXISTS "${index}" ON assets USING gin (${column} gin_trgm_ops)`, + ); + } + } + + public async down(queryRunner: QueryRunner): Promise { + for (const index of [ + 'IDX_assets_name_trgm', + 'IDX_assets_asset_tag_trgm', + 'IDX_assets_serial_number_trgm', + ]) { + await queryRunner.query(`DROP INDEX IF EXISTS "${index}"`); + } + } +} diff --git a/backend/src/users/dto/list-users-query.dto.ts b/backend/src/users/dto/list-users-query.dto.ts new file mode 100644 index 00000000..4cc79f88 --- /dev/null +++ b/backend/src/users/dto/list-users-query.dto.ts @@ -0,0 +1,29 @@ +import { ApiPropertyOptional } from '@nestjs/swagger'; +import { IsEnum, IsIn, IsOptional } from 'class-validator'; +import { PaginationQueryDto } from '../../common/dto/pagination.dto'; +import { UserRole } from '../entities/user.entity'; + +export const USER_SORT_COLUMNS = [ + 'createdAt', + 'email', + 'firstName', + 'lastName', + 'role', +] as const; + +export class ListUsersQueryDto extends PaginationQueryDto { + @ApiPropertyOptional({ enum: USER_SORT_COLUMNS, default: 'createdAt' }) + @IsOptional() + @IsIn(USER_SORT_COLUMNS) + sortBy?: string; + + @ApiPropertyOptional({ enum: ['ASC', 'DESC'], default: 'DESC' }) + @IsOptional() + @IsIn(['ASC', 'DESC']) + sortOrder?: 'ASC' | 'DESC'; + + @ApiPropertyOptional({ enum: UserRole }) + @IsOptional() + @IsEnum(UserRole) + role?: UserRole; +} diff --git a/backend/src/users/users.controller.ts b/backend/src/users/users.controller.ts index d92596c0..57cc47c2 100644 --- a/backend/src/users/users.controller.ts +++ b/backend/src/users/users.controller.ts @@ -17,6 +17,7 @@ import { import { UsersService } from './users.service'; import { UserRole } from './entities/user.entity'; import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import { ListUsersQueryDto } from './dto/list-users-query.dto'; import { UpdateProfileDto } from './dto/update-profile.dto'; @ApiTags('users') @@ -28,13 +29,8 @@ export class UsersController { @Get() @ApiOperation({ summary: 'List users (paginated, searchable)' }) @ApiResponse({ status: 200, description: 'List of users' }) - findAll( - @Query('search') search?: string, - @Query('role') role?: UserRole, - @Query('page') page?: number, - @Query('limit') limit?: number, - ) { - return this.usersService.findAll({ search, role, page, limit }); + findAll(@Query() query: ListUsersQueryDto) { + return this.usersService.findAll(query); } @Get('me') diff --git a/backend/src/users/users.service.ts b/backend/src/users/users.service.ts index ae2b28f8..522a194f 100644 --- a/backend/src/users/users.service.ts +++ b/backend/src/users/users.service.ts @@ -9,6 +9,12 @@ import { import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import * as bcrypt from 'bcryptjs'; +import { + LIKE_ESCAPE, + containsPattern, + resolveSort, +} from '../common/utils/list-query'; +import { USER_SORT_COLUMNS } from './dto/list-users-query.dto'; import { User, UserRole } from './entities/user.entity'; import { UpdateProfileDto } from './dto/update-profile.dto'; import { AuditLogsService } from '../audit-logs/audit-logs.service'; @@ -26,18 +32,28 @@ export class UsersService { role?: UserRole; page?: number; limit?: number; + sortBy?: string; + sortOrder?: string; }) { const page = query?.page || 1; - const limit = query?.limit || 20; + const limit = Math.min(query?.limit || 20, 100); + const sort = resolveSort( + query?.sortBy, + query?.sortOrder, + USER_SORT_COLUMNS, + 'createdAt', + ); const qb = this.userRepository .createQueryBuilder('user') .skip((page - 1) * limit) - .take(limit); + .take(limit) + .orderBy(`user.${sort.column}`, sort.order) + .addOrderBy('user.id', 'ASC'); if (query?.search) { qb.andWhere( - '(user.email ILIKE :s OR user.firstName ILIKE :s OR user.lastName ILIKE :s)', - { s: `%${query.search}%` }, + `(user.email ILIKE :s ${LIKE_ESCAPE} OR user.firstName ILIKE :s ${LIKE_ESCAPE} OR user.lastName ILIKE :s ${LIKE_ESCAPE})`, + { s: containsPattern(query.search) }, ); } if (query?.role) {