guimap01/review-nestjs

NestJS Code Review

Targets NestJS with Prisma in a multi-tenant SaaS context. Work through the five areas in order. For the full pass/fail checklist, see checklist.md.

1. Guards vs Interceptors — never mix concerns

• Guards decide whether a request proceeds (authentication, authorization, RBAC). They run before the route handler and must return true/false or throw.
• Interceptors shape or decorate the request/response (logging, caching, response transformation). They never make authorization decisions.
• Mixing these means auth logic can silently not run — treat any mixed usage as a high bug.

RBAC guard chain pattern (two guards, not one):

@UseGuards(JwtAuthGuard, RolesGuard)   // auth first, roles second
@Roles(Role.ADMIN)
@Get('admin-data')

Never collapse token validation and role checking into a single guard.

2. Tenant isolation — the highest-risk area

Every query on a tenant-scoped resource must filter by tenantId sourced from the JWT/session, not from the request body or URL params.

// BAD — tenantId from request body; attacker can spoof it
async getNotes(@Body() dto: GetNotesDto) {
  return this.notesService.findAll({ tenantId: dto.tenantId });
}

// GOOD — tenantId from validated JWT payload
async getNotes(@CurrentUser() user: JwtPayload) {
  return this.notesService.findAll({ tenantId: user.tenantId });
}

Use Prisma middleware to inject tenantId automatically as a safety net — but do not rely on it as the only enforcement layer.

Background jobs and raw queries bypass Prisma middleware: every one must explicitly set tenant context.

3. Validation pipes

Apply ValidationPipe globally with strict settings:

app.useGlobalPipes(
  new ValidationPipe({
    whitelist: true, // strip unknown properties
    forbidNonWhitelisted: true, // throw on unknown properties
    transform: true, // auto-transform to DTO types
  }),
);

Without whitelist: true, unvalidated properties reach service logic. Every incoming DTO must use class-validator decorators.

4. Module boundaries

• Providers should be exported from a module only when another module genuinely needs them
• Never export a service that contains security logic (guards, token validation) broadly
• Shared utilities belong in a SharedModule imported explicitly where needed

5. Logging and observability

• Use an interceptor (not console.log) for request/response logging
• Log auth events (login, logout, token refresh, permission denial) via a dedicated AuditService
• Permission denials must be logged at warn level with user ID, resource, and action — never swallowed silently
• AI request/response pairs should be logged with latency and any error codes

Additional resources

• Full pass/fail checklist → checklist.md