diff --git a/docs/api/properties-health.md b/docs/api/properties-health.md new file mode 100644 index 00000000..cec6baa7 --- /dev/null +++ b/docs/api/properties-health.md @@ -0,0 +1,14 @@ +# Property API health check + +`GET /properties/health` (no authentication) reports whether the Property API can do its job. + +- `200` with `{ status: "up", checkedAt, checks: { database: { status, latencyMs } } }` +- `503` with the same shape and `status: "down"` when a read of the `properties` table fails or takes longer than 2 seconds. + +The probe is a `findFirst({ select: { id: true } })`, so it is the cheapest read available and still proves the connection, the pool and the table are all usable. The timeout is raced with `Promise.race` and its timer is always cleared, so a slow database cannot leak a handle per probe. + +`PropertiesHealthController` is registered **before** `PropertiesController` in `PropertiesModule`, so `GET /properties/health` is matched by the probe and not swallowed by `GET /properties/:id` — the same ordering issue the Users API had to solve. + +Error details are never returned, so the probe cannot leak connection information. + +This is separate from the platform-wide `/healthz` and `/readyz` probes: those report the process as a whole, this one reports specifically whether the database the Property API depends on is answering. diff --git a/docs/api/properties-injection-hardening.md b/docs/api/properties-injection-hardening.md new file mode 100644 index 00000000..7339d12a --- /dev/null +++ b/docs/api/properties-injection-hardening.md @@ -0,0 +1,18 @@ +# Property API injection hardening + +`PropertyInjectionGuardPipe` screens the bodies of property create, update, status-transition, bulk, agent-assignment and amenity requests, plus the `GET /properties/search` query. It answers `400` for: + +- keys starting with `$` or containing `.` at any depth (MongoDB operator injection and path traversal shapes), +- NUL bytes in any string, +- SQL comment or stacked-statement markers (`--`, `/*`, `; drop|delete|update|insert|alter|truncate …`) in the free-text fields `location`, `title`, `description`, `address`, `city`, `state`, `zipCode`, `country`, `propertyType`, `category`, `tag`, `filter`, `reason`, `hoaName` and `hoaContactInfo`, +- payloads nested deeper than 8 levels. + +## Why this is not already covered + +The global `ValidationPipe` runs with `whitelist: true, forbidNonWhitelisted: true`, which rejects unknown keys **at the top level of a body**. That check does not descend into values: a `$`-prefixed key inside a nested object, or a dotted key inside an array element, is not what `whitelist` inspects. The pipe walks the whole structure, including array elements (`propertyIds[0]`, `tags[2]`), so those shapes are caught before they reach the service. + +Search is the highest-value target here because it is the one **unauthenticated** route whose values are used to build predicates, and because `location`, `category` and `tag` all end up in `contains`/`has` filters. + +## Defence in depth + +Prisma parameterises every query, so a payload that got through would be bound rather than concatenated. This pipe is the earlier layer: it rejects hostile input outright instead of relying on the ORM being careful. Neither layer replaces the other. diff --git a/docs/api/properties-pagination.md b/docs/api/properties-pagination.md new file mode 100644 index 00000000..a0aa6fd2 --- /dev/null +++ b/docs/api/properties-pagination.md @@ -0,0 +1,18 @@ +# Property API pagination + +`GET /properties` accepts `page` (>= 1) and `limit` (1–100, default 20). + +- **With neither parameter** the response is the plain array it has always been, so existing clients are unaffected. +- **With either**, the response is `{ data, meta: { page, limit, total, totalPages, hasNext } }`. + +`total` comes from a `count()` over the same `where` clause as the page, issued in the same `$transaction` as the rows, so the count and the page cannot disagree. `findAll` is unchanged and still available for callers that just want the rows; `findAllPaginated` is the new path. + +Out-of-range values return `400`. Note that because the handler now takes a validated query DTO, an **unknown** query parameter on `GET /properties` is now rejected too — consistent with `GET /properties/search` and with the global `forbidNonWhitelisted: true`. + +## Search + +`GET /properties/search` already paged via `page`/`limit` and already capped `limit` at 100, so it is unchanged. It returns its own shape (`items` + `total` + `totalPages`), which is left alone so existing search clients are not broken. + +## Query efficiency + +The owner and assigned-agent payload that both listing reads attach was identical and is now defined once (`listingRelations`), and the "active and not deleted" filter once (`buildListingWhere`), so the list and the paginated list cannot drift apart. diff --git a/docs/api/properties-tracing.md b/docs/api/properties-tracing.md new file mode 100644 index 00000000..0e1326fd --- /dev/null +++ b/docs/api/properties-tracing.md @@ -0,0 +1,16 @@ +# Property API tracing + +`PropertiesTraceInterceptor` is applied to every `PropertiesController` handler. Each call logs exactly one line: + +``` +[1f0c…] properties.create ok 143ms +[1f0c…] properties.transitionStatus failed status=400 BadRequestException 8ms +``` + +The trace id is the one set by the platform `TraceInterceptor` when present, otherwise the inbound `x-request-id`, otherwise the literal `untraced`. That makes a single listing call followable across the log stream without any client-side correlation plumbing. + +Outcomes are split by log level — `log` for success, `warn` for failure with the status and error class — so a dashboard can alert on failures without parsing messages. + +## What is deliberately not logged + +Request bodies, headers, addresses and any other personal data are never included. A listing create carries an owner's identity and a full address; logging it would put both into log aggregation, which is exactly where they should not end up. The `AppLogger` used elsewhere in the module additionally redacts fields such as `password`, `token` and `secret`. diff --git a/src/properties/dto/pagination-query.dto.ts b/src/properties/dto/pagination-query.dto.ts new file mode 100644 index 00000000..9f80880f --- /dev/null +++ b/src/properties/dto/pagination-query.dto.ts @@ -0,0 +1,25 @@ +import { Type } from 'class-transformer'; +import { IsInt, IsOptional, Max, Min } from 'class-validator'; + +export const PROPERTIES_DEFAULT_PAGE_SIZE = 20; +export const PROPERTIES_MAX_PAGE_SIZE = 100; + +export class PropertiesPaginationQueryDto { + @IsOptional() + @Type(() => Number) + @IsInt() + @Min(1) + page?: number; + + @IsOptional() + @Type(() => Number) + @IsInt() + @Min(1) + @Max(PROPERTIES_MAX_PAGE_SIZE) + limit?: number; +} + +export function buildPropertiesPageMeta(page: number, limit: number, total: number) { + const totalPages = Math.max(1, Math.ceil(total / limit)); + return { page, limit, total, totalPages, hasNext: page < totalPages }; +} diff --git a/src/properties/interceptors/properties-trace.interceptor.ts b/src/properties/interceptors/properties-trace.interceptor.ts new file mode 100644 index 00000000..8cae119a --- /dev/null +++ b/src/properties/interceptors/properties-trace.interceptor.ts @@ -0,0 +1,30 @@ +import { CallHandler, ExecutionContext, Injectable, Logger, NestInterceptor } from '@nestjs/common'; +import { Observable } from 'rxjs'; +import { tap } from 'rxjs/operators'; + +/** + * Traces each Property API handler: one log line per call with the trace id set + * by `TraceInterceptor` (or the request id), handler name, outcome and duration. + * Bodies, headers and personal data are never logged. + */ +@Injectable() +export class PropertiesTraceInterceptor implements NestInterceptor { + private readonly logger = new Logger('PropertiesTrace'); + + intercept(context: ExecutionContext, next: CallHandler): Observable { + const request = context.switchToHttp().getRequest(); + const span = `properties.${context.getHandler().name}`; + const traceId: string = request.traceId ?? request.headers?.['x-request-id'] ?? 'untraced'; + const started = Date.now(); + + return next.handle().pipe( + tap({ + next: () => this.logger.log(`[${traceId}] ${span} ok ${Date.now() - started}ms`), + error: (err: { status?: number; name?: string }) => + this.logger.warn( + `[${traceId}] ${span} failed status=${err?.status ?? 500} ${err?.name ?? 'Error'} ${Date.now() - started}ms`, + ), + }), + ); + } +} diff --git a/src/properties/pipes/injection-guard.pipe.ts b/src/properties/pipes/injection-guard.pipe.ts new file mode 100644 index 00000000..3f2a36a4 --- /dev/null +++ b/src/properties/pipes/injection-guard.pipe.ts @@ -0,0 +1,50 @@ +import { BadRequestException, Injectable, PipeTransform } from '@nestjs/common'; + +const MAX_DEPTH = 8; +const SQL_MARKERS = /(--|\/\*|;\s*(drop|delete|update|insert|alter|truncate)\b)/i; +const FREE_TEXT_FIELDS = /^(location|title|description|address|city|state|zipCode|country|propertyType|category|tag|filter|reason|hoaName|hoaContactInfo)$/; + +/** + * Screens Property API payloads and queries for injection shapes before they + * reach the service. Query objects and array bodies are nested, so the walk is + * recursive rather than flat: + * - keys starting with `$` or containing `.` (operator / path injection), which + * `whitelist` does not catch because they are looked up inside a value rather + * than at the top level of the body, + * - NUL bytes in any string, + * - SQL comment / stacked-statement markers in the free-text fields, + * - payloads nested deeper than 8 levels. + * + * Prisma parameterises every query and the DTOs bound the values anyway; this + * rejects hostile input early, before it can reach either. + */ +@Injectable() +export class PropertyInjectionGuardPipe implements PipeTransform { + transform(value: unknown): unknown { + if (value !== null && typeof value === 'object') this.inspect(value, 0, ''); + return value; + } + + private inspect(node: unknown, depth: number, path: string): void { + if (depth > MAX_DEPTH) throw new BadRequestException('Request is nested too deeply'); + if (typeof node === 'string') { + if (node.includes('\0')) throw new BadRequestException(`Invalid characters in ${path || 'value'}`); + if (FREE_TEXT_FIELDS.test(path) && SQL_MARKERS.test(node)) { + throw new BadRequestException(`Invalid characters in ${path}`); + } + return; + } + if (Array.isArray(node)) { + node.forEach((item, i) => this.inspect(item, depth + 1, `${path}[${i}]`)); + return; + } + if (node !== null && typeof node === 'object') { + for (const [key, val] of Object.entries(node as Record)) { + if (key.startsWith('$') || key.includes('.')) { + throw new BadRequestException('Invalid field name in request'); + } + this.inspect(val, depth + 1, key); + } + } + } +} diff --git a/src/properties/properties-health.controller.ts b/src/properties/properties-health.controller.ts new file mode 100644 index 00000000..806fba47 --- /dev/null +++ b/src/properties/properties-health.controller.ts @@ -0,0 +1,28 @@ +import { Controller, Get, ServiceUnavailableException } from '@nestjs/common'; +import { ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger'; +import { PropertiesHealthIndicator, PropertiesHealthResult } from './properties-health.indicator'; + +@ApiTags('Properties') +@Controller('properties/health') +export class PropertiesHealthController { + constructor(private readonly indicator: PropertiesHealthIndicator) {} + + /** + * Unauthenticated probe: 200 when the Property API's dependencies are up, 503 otherwise. + * Registered before PropertiesController so `GET /properties/health` is not + * captured by `GET /properties/:id`. + */ + @Get() + @ApiOperation({ + summary: 'Property API health probe', + description: 'Reports whether the database backing the Property API is reachable.', + operationId: 'getPropertiesHealth', + }) + @ApiResponse({ status: 200, description: 'The Property API can serve requests' }) + @ApiResponse({ status: 503, description: 'A dependency is down' }) + async health(): Promise { + const result = await this.indicator.check(); + if (result.status === 'down') throw new ServiceUnavailableException(result); + return result; + } +} diff --git a/src/properties/properties-health.indicator.ts b/src/properties/properties-health.indicator.ts new file mode 100644 index 00000000..9fbb66c9 --- /dev/null +++ b/src/properties/properties-health.indicator.ts @@ -0,0 +1,45 @@ +import { Injectable } from '@nestjs/common'; +import { PrismaService } from '../database/prisma.service'; + +export interface PropertiesHealthResult { + status: 'up' | 'down'; + checkedAt: string; + checks: { database: { status: 'up' | 'down'; latencyMs: number } }; +} + +const DB_CHECK_TIMEOUT_MS = 2_000; + +/** + * Health of what the Property API cannot work without: a database that can read + * the `properties` table. Failure details are never returned, so a probe cannot + * leak connection information. + */ +@Injectable() +export class PropertiesHealthIndicator { + constructor(private readonly prisma: PrismaService) {} + + async check(): Promise { + const started = Date.now(); + let databaseUp = true; + let timer: NodeJS.Timeout | undefined; + try { + await Promise.race([ + // `select: { id: true }` keeps the probe to the cheapest possible read. + this.prisma.property.findFirst({ select: { id: true } }), + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error('timeout')), DB_CHECK_TIMEOUT_MS); + }), + ]); + } catch { + databaseUp = false; + } finally { + if (timer) clearTimeout(timer); + } + + return { + status: databaseUp ? 'up' : 'down', + checkedAt: new Date().toISOString(), + checks: { database: { status: databaseUp ? 'up' : 'down', latencyMs: Date.now() - started } }, + }; + } +} diff --git a/src/properties/properties.controller.ts b/src/properties/properties.controller.ts index bb7755f1..b4aa3eb7 100644 --- a/src/properties/properties.controller.ts +++ b/src/properties/properties.controller.ts @@ -9,6 +9,7 @@ import { Patch, Query, UseGuards, + UsePipes, UseInterceptors, Res, // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -35,6 +36,17 @@ import { import { CreateAmenityDto, UpdateAmenityDto } from './dto/amenity.dto'; import { PropertyReportService } from './report/property-report.service'; import { Response } from 'express'; +import { ApiBearerAuth, ApiOperation, ApiQuery, ApiResponse, ApiTags } from '@nestjs/swagger'; +import { PropertiesTraceInterceptor } from './interceptors/properties-trace.interceptor'; +import { PropertyInjectionGuardPipe } from './pipes/injection-guard.pipe'; +import { + PROPERTIES_DEFAULT_PAGE_SIZE, + PropertiesPaginationQueryDto, + buildPropertiesPageMeta, +} from './dto/pagination-query.dto'; + +@ApiTags('Properties') +@UseInterceptors(PropertiesTraceInterceptor) import { ApiBearerAuth, @@ -65,11 +77,42 @@ export class PropertiesController { @UseGuards(JwtAuthGuard, PropertiesRouteRateLimitGuard) @PropertiesRouteLimit({ max: 30, windowMs: 60 * 60 * 1000 }) @Post() + @UsePipes(PropertyInjectionGuardPipe) create(@Body() createPropertyDto: CreatePropertyDto, @CurrentUser() user: AuthUserPayload) { return this.propertiesService.create(createPropertyDto, user.sub); } + /** + * Public listing feed. + * + * Without `page`/`limit` this returns the plain array it has always returned, + * so existing clients are unaffected. With either parameter the response becomes + * `{ data, meta: { page, limit, total, totalPages, hasNext } }`. + */ @Get() + @ApiOperation({ summary: 'List published property listings', operationId: 'listProperties' }) + @ApiQuery({ name: 'page', required: false, minimum: 1, description: '1-based page number.' }) + @ApiQuery({ + name: 'limit', + required: false, + minimum: 1, + maximum: 100, + description: `Rows per page (default ${PROPERTIES_DEFAULT_PAGE_SIZE}).`, + }) + @ApiResponse({ + status: 200, + description: 'The listings, or a page of them when page/limit is supplied', + }) + @ApiResponse({ status: 400, description: 'page or limit outside the accepted range' }) + async findAll(@Query() query: PropertiesPaginationQueryDto = {}) { + const wantsPage = query.page !== undefined || query.limit !== undefined; + if (!wantsPage) return this.propertiesService.findAll(); + + const page = query.page ?? 1; + const limit = query.limit ?? PROPERTIES_DEFAULT_PAGE_SIZE; + const { items, total } = await this.propertiesService.findAllPaginated(page, limit); + + return { data: items, meta: buildPropertiesPageMeta(page, limit, total) }; @ApiOperation({ summary: 'List published property listings', description: @@ -94,6 +137,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response * Defined before `:id` so the static path is matched first. */ @Get('search') + @UsePipes(PropertyInjectionGuardPipe) @ApiOperation({ summary: 'Search property listings', description: @@ -201,6 +245,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @UseGuards(JwtAuthGuard, RolesGuard) @Roles(UserRole.AGENT, UserRole.ADMIN) @Put(':id') + @UsePipes(PropertyInjectionGuardPipe) update( @Param('id') id: string, @Body() updatePropertyDto: UpdatePropertyDto, @@ -269,6 +314,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @ApiBearerAuth('access-token') @UseGuards(JwtAuthGuard) @Patch(':id/status') + @UsePipes(PropertyInjectionGuardPipe) transitionStatus( @Param('id') id: string, @Body() dto: TransitionPropertyStatusDto, @@ -302,6 +348,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @UseGuards(JwtAuthGuard, RolesGuard) @Roles(UserRole.ADMIN) @Post('bulk/status') + @UsePipes(PropertyInjectionGuardPipe) @UseGuards(PropertiesRouteRateLimitGuard) @PropertiesRouteLimit({ max: 20, windowMs: 60 * 60 * 1000 }) async bulkUpdatePropertyStatus( @@ -316,6 +363,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @UseGuards(JwtAuthGuard, RolesGuard) @Roles(UserRole.ADMIN) @Post('bulk/delete') + @UsePipes(PropertyInjectionGuardPipe) @UseGuards(PropertiesRouteRateLimitGuard) @PropertiesRouteLimit({ max: 20, windowMs: 60 * 60 * 1000 }) async bulkDeleteProperties( @@ -330,6 +378,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @UseGuards(JwtAuthGuard, RolesGuard) @Roles(UserRole.ADMIN) @Post('bulk/export') + @UsePipes(PropertyInjectionGuardPipe) @UseGuards(PropertiesRouteRateLimitGuard) @PropertiesRouteLimit({ max: 10, windowMs: 60 * 60 * 1000 }) async bulkExportProperties( @@ -343,6 +392,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @ApiBearerAuth('access-token') @UseGuards(JwtAuthGuard) @Post(':id/agents') + @UsePipes(PropertyInjectionGuardPipe) async assignAgent( @Param('id') propertyId: string, @Body() dto: AssignAgentDto, @@ -354,6 +404,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @ApiBearerAuth('access-token') @UseGuards(JwtAuthGuard) @Put(':id/agents/:agentId') + @UsePipes(PropertyInjectionGuardPipe) async updateAgentAssignment( @Param('id') propertyId: string, @Param('agentId') agentId: string, @@ -384,6 +435,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @ApiBearerAuth('access-token') @UseGuards(JwtAuthGuard) @Post(':id/amenities') + @UsePipes(PropertyInjectionGuardPipe) async addAmenity(@Param('id') propertyId: string, @Body() dto: CreateAmenityDto) { return this.propertiesService.addAmenity(propertyId, dto); } @@ -396,6 +448,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response @ApiBearerAuth('access-token') @UseGuards(JwtAuthGuard) @Patch(':id/amenities/:amenityId') + @UsePipes(PropertyInjectionGuardPipe) async updateAmenity( @Param('id') propertyId: string, @Param('amenityId') amenityId: string, diff --git a/src/properties/properties.module.ts b/src/properties/properties.module.ts index c6c98bf9..ec98b159 100644 --- a/src/properties/properties.module.ts +++ b/src/properties/properties.module.ts @@ -14,6 +14,9 @@ import { FraudModule } from '../fraud/fraud.module'; import { DocumentsModule } from '../documents/documents.module'; import { PropertyReportService } from './report/property-report.service'; import { CacheModuleConfig } from '../cache/cache.module'; +import { PropertiesHealthController } from './properties-health.controller'; +import { PropertiesHealthIndicator } from './properties-health.indicator'; +import { PropertyInjectionGuardPipe } from './pipes/injection-guard.pipe'; import { PropertiesCacheService } from './properties-cache.service'; import { PropertiesRouteRateLimitGuard } from './guards/properties-route-rate-limit.guard'; import { RateLimitService } from '../auth/rate-limit.service'; @@ -28,13 +31,21 @@ import { RateLimitService } from '../auth/rate-limit.service'; DocumentsModule, NotificationsModule, ], - controllers: [PropertiesController, PropertyImagesController], + controllers: [ + // Before PropertiesController: `GET /properties/health` must not match + // `GET /properties/:id`. + PropertiesHealthController, + PropertiesController, + PropertyImagesController, + ], providers: [ PropertiesService, PropertyImagesService, GeocodingService, PropertyExpiryService, PropertyReportService, + PropertiesHealthIndicator, + PropertyInjectionGuardPipe, PropertiesCacheService, RateLimitService, PropertiesRouteRateLimitGuard, diff --git a/src/properties/properties.service.ts b/src/properties/properties.service.ts index d5c6c13c..cd6c999b 100644 --- a/src/properties/properties.service.ts +++ b/src/properties/properties.service.ts @@ -261,6 +261,58 @@ export class PropertiesService { async findAll(params?: FindAllParams) { const { skip, take, where, orderBy } = params || {}; + return (this.prisma.property as any).findMany({ + skip, + take, + where: this.buildListingWhere(where), + orderBy, + include: this.listingRelations, + }); + } + + /** + * `GET /properties` with paging. Returns the rows and the total that matches + * the same filter, so a client can render "page 2 of 7" without a second + * request. `findAll` is left as-is for callers that just want the rows. + */ + async findAllPaginated( + page: number, + limit: number, + params?: Omit, + ): Promise<{ items: unknown[]; total: number }> { + const where = this.buildListingWhere(params?.where); + const skip = (page - 1) * limit; + + const [items, total] = await this.prisma.$transaction([ + (this.prisma.property as any).findMany({ + skip, + take: limit, + where, + orderBy: params?.orderBy, + include: this.listingRelations, + }), + this.prisma.property.count({ where }), + ]); + + return { items, total }; + } + + /** The public listing filter: never soft-deleted, and ACTIVE unless overridden. */ + private buildListingWhere(where?: Record): Record { + return where + ? { ...where, status: where.status ?? PropertyStatus.ACTIVE, deleted: false } + : { status: PropertyStatus.ACTIVE, deleted: false }; + } + + /** Owner and assigned-agent payload shared by every listing read. */ + private get listingRelations() { + return { + owner: { + select: { + id: true, + firstName: true, + lastName: true, + email: true, const finalWhere = where ? { ...where, status: where.status ?? PropertyStatus.ACTIVE, deleted: false } : { status: PropertyStatus.ACTIVE, deleted: false }; @@ -284,21 +336,21 @@ export class PropertiesService { email: true, }, }, - agents: { - include: { - agent: { - select: { - id: true, - firstName: true, - lastName: true, - email: true, - phone: true, - }, + }, + agents: { + include: { + agent: { + select: { + id: true, + firstName: true, + lastName: true, + email: true, + phone: true, }, }, }, }, - }); + }; } async findOne(id: string) {