diff --git a/backend/src/users/dto/member-query.dto.ts b/backend/src/users/dto/member-query.dto.ts index 1259cafa..83e88511 100644 --- a/backend/src/users/dto/member-query.dto.ts +++ b/backend/src/users/dto/member-query.dto.ts @@ -1,11 +1,45 @@ import { ApiPropertyOptional } from '@nestjs/swagger'; -import { IsEnum, IsOptional, IsString } from 'class-validator'; +import { Transform, Type } from 'class-transformer'; +import { + IsEnum, + IsInt, + IsOptional, + IsString, + Max, + MaxLength, + Min, +} from 'class-validator'; import { MembershipStatus } from '../enums/membership-status.enum'; -import { PaginationDto } from '../../common/dto/pagination.dto'; +import { + DEFAULT_LIMIT, + DEFAULT_PAGE, + MAX_LIMIT, + PaginationDto, +} from '../../common/dto/pagination.dto'; + +/** + * BE-18 — Hard ceiling on the `page` query parameter. + * + * `PaginationDto` already bounds `limit` (see `MAX_LIMIT`) but leaves + * `page` unbounded, so `?page=999999999` produced a multi-billion row + * OFFSET that forced Postgres to scan and discard the whole table before + * returning nothing. Capping the page number keeps the worst case at + * `MAX_MEMBER_PAGE * MAX_LIMIT` rows scanned. + */ +export const MAX_MEMBER_PAGE = 10_000; + +/** + * BE-18 — Hard ceiling on the free-text `search` filter. Without a + * bound, a caller could push an arbitrarily long string into the + * `LIKE '%…%'` predicate for every member-list request. + */ +export const MAX_MEMBER_SEARCH_LENGTH = 100; /** * Query parameters for listing members (BE-15 acceptance). - * Inherits `page` / `limit` from the unified `PaginationDto`. + * Inherits `page` / `limit` from the unified `PaginationDto` and adds + * BE-18 bounds so a single request can never fan out into an + * unbounded table scan. */ export class MemberQueryDto extends PaginationDto { @ApiPropertyOptional({ enum: MembershipStatus }) @@ -13,8 +47,98 @@ export class MemberQueryDto extends PaginationDto { @IsEnum(MembershipStatus) status?: MembershipStatus; - @ApiPropertyOptional({ example: 'jane' }) + @ApiPropertyOptional({ + example: 'jane', + maxLength: MAX_MEMBER_SEARCH_LENGTH, + }) @IsOptional() @IsString() + @MaxLength(MAX_MEMBER_SEARCH_LENGTH, { + message: `search must be at most ${MAX_MEMBER_SEARCH_LENGTH} characters`, + }) + @Transform(({ value }) => (typeof value === 'string' ? value.trim() : value)) search?: string; + + /** + * Re-declared (rather than inherited) so the member endpoints can + * reject absurd page numbers while the rest of the API keeps the + * `PaginationDto` default of "any page >= 1". + */ + @ApiPropertyOptional({ + default: DEFAULT_PAGE, + minimum: 1, + maximum: MAX_MEMBER_PAGE, + }) + @IsOptional() + @Type(() => Number) + @IsInt({ message: 'page must be an integer' }) + @Min(1, { message: 'page must be greater than or equal to 1' }) + @Max(MAX_MEMBER_PAGE, { + message: `page must be less than or equal to ${MAX_MEMBER_PAGE}`, + }) + override page: number = DEFAULT_PAGE; +} + +/** + * BE-18 — Clamp a raw query value into the inclusive range + * `[min, max]`. Non-numeric / `NaN` / `Infinity` input falls back to + * `fallback` instead of leaking `NaN` into the SQL builder, and the + * value is truncated so a fractional page can never produce a + * fractional OFFSET. + */ +export function clampQueryNumber( + value: unknown, + { min, max, fallback }: { min: number; max: number; fallback: number }, +): number { + const parsed = typeof value === 'number' ? value : Number(value); + if (!Number.isFinite(parsed)) return fallback; + return Math.min(max, Math.max(min, Math.trunc(parsed))); +} + +/** + * BE-18 — Resolve the page size actually used for the query. Oversized + * requests are capped at `MAX_LIMIT` rather than rejected so the + * endpoint degrades predictably for callers that bypass the DTO layer + * (internal service calls, legacy admin tooling). + */ +export function resolveMemberLimit(limit: unknown): number { + return clampQueryNumber(limit, { + min: 1, + max: MAX_LIMIT, + fallback: DEFAULT_LIMIT, + }); +} + +/** + * BE-18 — Resolve the 1-indexed page actually used for the query. + */ +export function resolveMemberPage(page: unknown): number { + return clampQueryNumber(page, { + min: 1, + max: MAX_MEMBER_PAGE, + fallback: DEFAULT_PAGE, + }); +} + +/** + * BE-18 — Normalise the free-text search filter: trim, bound the + * length, and escape the `LIKE` wildcards (`%` and `_`) so a search + * term can never widen into a full-table match. Returns `undefined` + * when nothing searchable is left. + */ +export function resolveMemberSearch(search: unknown): string | undefined { + if (typeof search !== 'string') return undefined; + const trimmed = search.trim().slice(0, MAX_MEMBER_SEARCH_LENGTH); + if (!trimmed) return undefined; + return escapeLikePattern(trimmed.toLowerCase()); +} + +/** + * Escape the characters PostgreSQL treats as wildcards inside `LIKE` + * so a user-supplied term is matched literally. The default LIKE + * escape character in PostgreSQL is a backslash, so no extra `ESCAPE` + * clause is required. + */ +export function escapeLikePattern(value: string): string { + return value.replace(/[\\%_]/g, (match) => `\\${match}`); } diff --git a/backend/src/users/providers/get-members.provider.spec.ts b/backend/src/users/providers/get-members.provider.spec.ts new file mode 100644 index 00000000..dadd6a48 --- /dev/null +++ b/backend/src/users/providers/get-members.provider.spec.ts @@ -0,0 +1,331 @@ +import { Test } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { plainToInstance } from 'class-transformer'; +import { validate } from 'class-validator'; + +import { GetMembersProvider } from './get-members.provider'; +import { User } from '../entities/user.entity'; +import { MembershipStatus } from '../enums/membership-status.enum'; +import { + MAX_MEMBER_PAGE, + MAX_MEMBER_SEARCH_LENGTH, + MemberQueryDto, +} from '../dto/member-query.dto'; +import { + DEFAULT_LIMIT, + DEFAULT_PAGE, + MAX_LIMIT, +} from '../../common/dto/pagination.dto'; + +type QueryBuilderStub = Record; + +function buildQueryBuilderStub( + overrides: Partial> = {}, +): QueryBuilderStub { + const stub: QueryBuilderStub = { + select: jest.fn().mockReturnThis(), + withDeleted: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + skip: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + getCount: jest.fn().mockResolvedValue(0), + getMany: jest.fn().mockResolvedValue([]), + }; + Object.assign(stub, overrides); + return stub; +} + +/** Mirrors the options used by `CentralizedValidationPipe`. */ +const VALIDATION_OPTIONS = { + whitelist: true, + forbidNonWhitelisted: true, +} as const; + +async function validationErrorsFor( + query: Record, +): Promise { + const dto = plainToInstance(MemberQueryDto, query); + const errors = await validate(dto, VALIDATION_OPTIONS); + return errors.flatMap((error) => Object.values(error.constraints ?? {})); +} + +describe('GetMembersProvider (BE-18 — bounded member-list query parameters)', () => { + let provider: GetMembersProvider; + let qb: QueryBuilderStub; + let repository: { createQueryBuilder: jest.Mock }; + + beforeEach(async () => { + qb = buildQueryBuilderStub(); + repository = { createQueryBuilder: jest.fn().mockReturnValue(qb) }; + + const moduleRef = await Test.createTestingModule({ + providers: [ + GetMembersProvider, + { provide: getRepositoryToken(User), useValue: repository }, + ], + }).compile(); + + provider = moduleRef.get(GetMembersProvider); + }); + + describe('defaults', () => { + it('falls back to page 1 / limit 20 when no pagination is supplied', async () => { + qb.getCount.mockResolvedValue(42); + + const result = await provider.getMembers({} as MemberQueryDto); + + expect(qb.skip).toHaveBeenCalledWith(0); + expect(qb.take).toHaveBeenCalledWith(DEFAULT_LIMIT); + expect(result).toMatchObject({ + page: DEFAULT_PAGE, + limit: DEFAULT_LIMIT, + total: 42, + totalPages: 3, + }); + }); + + it('computes totalPages from the effective limit', async () => { + qb.getCount.mockResolvedValue(100); + + const result = await provider.getMembers({ + page: 1, + limit: 30, + } as MemberQueryDto); + + expect(result.totalPages).toBe(4); + }); + + it('translates the page number into the matching OFFSET', async () => { + await provider.getMembers({ page: 4, limit: 25 } as MemberQueryDto); + + expect(qb.skip).toHaveBeenCalledWith(75); + expect(qb.take).toHaveBeenCalledWith(25); + }); + }); + + describe('limits', () => { + it('caps an oversized page size at MAX_LIMIT', async () => { + const result = await provider.getMembers({ + page: 1, + limit: 100_000, + } as MemberQueryDto); + + expect(qb.take).toHaveBeenCalledWith(MAX_LIMIT); + expect(result.limit).toBe(MAX_LIMIT); + }); + + it('clamps a zero or negative page size up to 1', async () => { + for (const limit of [0, -5]) { + jest.clearAllMocks(); + qb = buildQueryBuilderStub(); + repository.createQueryBuilder.mockReturnValue(qb); + provider = new GetMembersProvider(repository as never); + + const result = await provider.getMembers({ limit } as MemberQueryDto); + + expect(result.limit).toBe(1); + expect(qb.take).toHaveBeenCalledWith(1); + } + }); + + it('truncates a fractional page size so take() always receives an integer', async () => { + const result = await provider.getMembers({ + limit: 7.9, + } as MemberQueryDto); + + expect(result.limit).toBe(7); + expect(qb.take).toHaveBeenCalledWith(7); + }); + + it('falls back to the default page size for non-numeric input', async () => { + for (const limit of ['abc', Number.NaN, Number.POSITIVE_INFINITY]) { + jest.clearAllMocks(); + qb = buildQueryBuilderStub(); + repository.createQueryBuilder.mockReturnValue(qb); + provider = new GetMembersProvider(repository as never); + + const result = await provider.getMembers({ + limit, + } as unknown as MemberQueryDto); + + expect(result.limit).toBe(DEFAULT_LIMIT); + } + }); + + it('never computes totalPages against a zero page size', async () => { + qb.getCount.mockResolvedValue(7); + + const result = await provider.getMembers({ limit: 0 } as MemberQueryDto); + + expect(result.totalPages).toBe(7); + expect(Number.isFinite(result.totalPages)).toBe(true); + }); + }); + + describe('page bounds', () => { + it('caps an absurd page number so OFFSET stays finite', async () => { + await provider.getMembers({ + page: 2_000_000_000, + limit: 10, + } as MemberQueryDto); + + expect(qb.skip).toHaveBeenCalledWith((MAX_MEMBER_PAGE - 1) * 10); + }); + + it('clamps a negative page number to the first page', async () => { + const result = await provider.getMembers({ page: -3 } as MemberQueryDto); + + expect(result.page).toBe(DEFAULT_PAGE); + expect(qb.skip).toHaveBeenCalledWith(0); + }); + }); + + describe('filters', () => { + it('applies the status filter', async () => { + await provider.getMembers({ + status: MembershipStatus.ACTIVE, + } as MemberQueryDto); + + expect(qb.andWhere).toHaveBeenCalledWith( + 'user.membershipStatus = :status', + { status: MembershipStatus.ACTIVE }, + ); + }); + + it('normalises the search term and matches it case-insensitively', async () => { + await provider.getMembers({ search: ' JaNe ' } as MemberQueryDto); + + expect(qb.andWhere).toHaveBeenCalledWith( + expect.stringContaining('LIKE'), + { + search: '%jane%', + }, + ); + }); + + it('escapes LIKE wildcards so the search cannot widen the match', async () => { + await provider.getMembers({ search: '100%_x' } as MemberQueryDto); + + expect(qb.andWhere).toHaveBeenCalledWith( + expect.stringContaining('LIKE'), + { + search: '%100\\%\\_x%', + }, + ); + }); + + it('drops a whitespace-only search instead of filtering on "%%"', async () => { + await provider.getMembers({ search: ' ' } as MemberQueryDto); + + expect(qb.andWhere).not.toHaveBeenCalled(); + }); + + it('ignores a non-string search term', async () => { + await provider.getMembers({ + search: { toString: () => 'x' }, + } as unknown as MemberQueryDto); + + expect(qb.andWhere).not.toHaveBeenCalled(); + }); + }); + + describe('soft-deleted users', () => { + it('excludes tombstones by default', async () => { + await provider.getMembers({} as MemberQueryDto); + + expect(qb.withDeleted).not.toHaveBeenCalled(); + expect(qb.andWhere).not.toHaveBeenCalledWith( + 'user.deletedAt IS NOT NULL', + ); + }); + + it('only surfaces tombstones when asked', async () => { + await provider.getMembers({} as MemberQueryDto, true); + + expect(qb.withDeleted).toHaveBeenCalled(); + expect(qb.andWhere).toHaveBeenCalledWith('user.deletedAt IS NOT NULL'); + }); + }); +}); + +describe('MemberQueryDto (BE-18 — validation)', () => { + it('accepts an empty query and applies the shared defaults', async () => { + expect(await validationErrorsFor({})).toEqual([]); + + const dto = plainToInstance(MemberQueryDto, {}); + expect(dto.page).toBe(DEFAULT_PAGE); + expect(dto.limit).toBe(DEFAULT_LIMIT); + }); + + it('accepts a well-formed query', async () => { + expect( + await validationErrorsFor({ + page: '2', + limit: '50', + status: MembershipStatus.ACTIVE, + search: 'jane', + }), + ).toEqual([]); + }); + + it('rejects a limit above the maximum page size', async () => { + const errors = await validationErrorsFor({ limit: String(MAX_LIMIT + 1) }); + + expect(errors).toContain('limit must be less than or equal to 100'); + }); + + it('rejects a zero, negative or non-integer limit', async () => { + expect(await validationErrorsFor({ limit: '0' })).toContain( + 'limit must be greater than or equal to 1', + ); + expect(await validationErrorsFor({ limit: '-1' })).toContain( + 'limit must be greater than or equal to 1', + ); + expect(await validationErrorsFor({ limit: 'abc' })).toContain( + 'limit must be an integer', + ); + }); + + it('rejects a page above the member page ceiling', async () => { + expect( + await validationErrorsFor({ page: String(MAX_MEMBER_PAGE + 1) }), + ).toContain(`page must be less than or equal to ${MAX_MEMBER_PAGE}`); + }); + + it('rejects a non-integer or out-of-range page', async () => { + expect(await validationErrorsFor({ page: '0' })).toContain( + 'page must be greater than or equal to 1', + ); + expect(await validationErrorsFor({ page: 'first' })).toContain( + 'page must be an integer', + ); + }); + + it('rejects an unknown membership status', async () => { + expect(await validationErrorsFor({ status: 'not-a-status' })).not.toEqual( + [], + ); + }); + + it('rejects a search term longer than the allowed maximum', async () => { + const errors = await validationErrorsFor({ + search: 'a'.repeat(MAX_MEMBER_SEARCH_LENGTH + 1), + }); + + expect(errors).toContain( + `search must be at most ${MAX_MEMBER_SEARCH_LENGTH} characters`, + ); + }); + + it('trims the search term before it reaches the provider', () => { + const dto = plainToInstance(MemberQueryDto, { search: ' jane ' }); + + expect(dto.search).toBe('jane'); + }); + + it('rejects unknown query parameters', async () => { + expect(await validationErrorsFor({ sortBy: 'password' })).not.toEqual([]); + }); +}); diff --git a/backend/src/users/providers/get-members.provider.ts b/backend/src/users/providers/get-members.provider.ts index 7c89b082..b890f807 100644 --- a/backend/src/users/providers/get-members.provider.ts +++ b/backend/src/users/providers/get-members.provider.ts @@ -2,7 +2,12 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository, SelectQueryBuilder } from 'typeorm'; import { User } from '../entities/user.entity'; -import { MemberQueryDto } from '../dto/member-query.dto'; +import { + MemberQueryDto, + resolveMemberLimit, + resolveMemberPage, + resolveMemberSearch, +} from '../dto/member-query.dto'; export interface PaginatedMembers { data: Partial[]; @@ -33,7 +38,16 @@ export class GetMembersProvider { query: MemberQueryDto, withDeleted = false, ): Promise { - const { page = 1, limit = 20, status, search } = query; + // BE-18 — Never trust the raw query object. The DTO validates and + // bounds the values that arrive over HTTP, but this provider is + // also called directly by the admin controller and by internal + // services, so the page size is capped here as well. A `limit` of + // 0 (or an omitted one) previously reached `take(0)` and made + // `totalPages` divide by zero. + const page = resolveMemberPage(query?.page); + const limit = resolveMemberLimit(query?.limit); + const status = query?.status; + const search = resolveMemberSearch(query?.search); const qb: SelectQueryBuilder = this.usersRepository .createQueryBuilder('user') @@ -77,8 +91,8 @@ export class GetMembersProvider { if (search) { qb.andWhere( - '(LOWER(user.firstname) LIKE :search OR LOWER(user.lastname) LIKE :search OR LOWER(user.email) LIKE :search)', - { search: `%${search.toLowerCase()}%` }, + "(LOWER(user.firstname) LIKE :search ESCAPE '\\' OR LOWER(user.lastname) LIKE :search ESCAPE '\\' OR LOWER(user.email) LIKE :search ESCAPE '\\')", + { search: `%${search}%` }, ); }