Repository navigation
Als ontwikkelaar wil ik logging van beveiligingsrelevante gebeurtenissen toevoegen, zodat verdachte activiteiten traceerbaar zijn. - #20
Conversation
…hanced request and error logging
There was a problem hiding this comment.
Pull request overview
This PR implements a centralized logging infrastructure to improve observability across the application. It introduces a custom LoggerService that provides structured JSON logging and integrates it with NestJS framework logs, HTTP requests, database operations, and authentication flows.
Key Changes:
- Custom
LoggerServicewith structured JSON output for consistent logging format - Global request logging middleware tracking HTTP method, URL, status, and response time
- Enhanced database connection logging with retry logic and Prisma event forwarding
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/common/logger.service.ts |
New centralized logger service implementing NestJS LoggerService interface with structured JSON output |
src/presentation/middleware/request-logging.middleware.ts |
New middleware for logging all HTTP requests with timing information |
src/app.module.ts |
Registers LoggerService and applies RequestLoggingMiddleware globally |
src/main.ts |
Configures application to use custom logger for framework logs |
src/infrastructure/database/prisma.ts |
Adds database connection retry logic and forwards Prisma events to logger |
src/application/services/auth.service.ts |
Integrates logging for failed login attempts while avoiding user enumeration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| constructor(@Inject('REPO.USER') _userRepository: IUserRepository) { | ||
| constructor( | ||
| @Inject('REPO.USER') _userRepository: IUserRepository, | ||
| private readonly logger?: LoggerService, |
There was a problem hiding this comment.
The logger parameter is marked as optional, but the code uses optional chaining throughout. Consider making it required to ensure consistent logging behavior across all authentication operations.
| const logMsg = | ||
| repoMsg === 'User not found' ? 'Credentials not correct' : repoMsg; | ||
| this.logger?.warn( | ||
| `Failed login attempt for ${email}: ${logMsg}`, |
There was a problem hiding this comment.
Logging user email addresses in failed login attempts may expose sensitive information in logs. Consider hashing or redacting email addresses, or logging only a session/request identifier for correlation while keeping PII out of logs.
| const isMatch = await verify(user.hashedPassword, password); | ||
| if (!isMatch) return fail(new Error('Invalid credentials')); | ||
| if (!isMatch) { | ||
| this.logger?.warn(`Invalid credentials for ${email}`, 'AuthService'); |
There was a problem hiding this comment.
Logging user email addresses in failed login attempts may expose sensitive information in logs. Consider hashing or redacting email addresses, or logging only a session/request identifier for correlation while keeping PII out of logs.
| this.logger?.warn(`Invalid credentials for ${email}`, 'AuthService'); | |
| this.logger?.warn('Invalid credentials', 'AuthService'); |
| private name = 'App'; | ||
|
|
||
| setContext(name: string) { | ||
| this.name = name; | ||
| } | ||
|
|
||
| private format(level: LogLevel, message: any, context?: string, meta?: any) { | ||
| const payload: Record<string, any> = { | ||
| timestamp: new Date().toISOString(), | ||
| level, | ||
| context: context ?? this.name, |
There was a problem hiding this comment.
The default context name 'App' is generic and may not be helpful for distinguishing log sources. Consider using a more descriptive default or requiring context to be set explicitly during initialization.
| private name = 'App'; | |
| setContext(name: string) { | |
| this.name = name; | |
| } | |
| private format(level: LogLevel, message: any, context?: string, meta?: any) { | |
| const payload: Record<string, any> = { | |
| timestamp: new Date().toISOString(), | |
| level, | |
| context: context ?? this.name, | |
| private name?: string; | |
| setContext(name: string) { | |
| this.name = name; | |
| } | |
| private format(level: LogLevel, message: any, context?: string, meta?: any) { | |
| const resolvedContext = context ?? this.name; | |
| if (!resolvedContext) { | |
| throw new Error('LoggerService context is not set. Please set a context using setContext() or provide one when logging.'); | |
| } | |
| const payload: Record<string, any> = { | |
| timestamp: new Date().toISOString(), | |
| level, | |
| context: resolvedContext, |
| // Prisma client's $on typing may be strict depending on generated client. | ||
| // Cast to `any` to attach listeners at runtime while keeping TypeScript happy. | ||
| (this as any).$on('info', (e: any) => { | ||
| this.logger.log(`${e.message}`, 'Prisma'); | ||
| }); | ||
|
|
||
| (this as any).$on('warn', (e: any) => { | ||
| this.logger.warn(`${e.message}`, 'Prisma'); | ||
| }); | ||
|
|
||
| (this as any).$on('error', (e: any) => { | ||
| // e is an object with message and perhaps stack | ||
| this.logger.error( | ||
| `${e.message}`, | ||
| (e as any).stack ?? undefined, |
There was a problem hiding this comment.
Multiple type assertions to any reduce type safety. Consider defining an interface for the event object structure or using a type guard to safely access the Prisma event properties.
| // Prisma client's $on typing may be strict depending on generated client. | |
| // Cast to `any` to attach listeners at runtime while keeping TypeScript happy. | |
| (this as any).$on('info', (e: any) => { | |
| this.logger.log(`${e.message}`, 'Prisma'); | |
| }); | |
| (this as any).$on('warn', (e: any) => { | |
| this.logger.warn(`${e.message}`, 'Prisma'); | |
| }); | |
| (this as any).$on('error', (e: any) => { | |
| // e is an object with message and perhaps stack | |
| this.logger.error( | |
| `${e.message}`, | |
| (e as any).stack ?? undefined, | |
| // Define event interfaces for Prisma events | |
| interface PrismaLogEvent { | |
| message: string; | |
| target?: string; | |
| timestamp?: Date; | |
| } | |
| interface PrismaErrorEvent extends PrismaLogEvent { | |
| stack?: string; | |
| } | |
| // Cast only `this` to any if needed, but type event parameter | |
| (this as any).$on('info', (e: PrismaLogEvent) => { | |
| this.logger.log(`${e.message}`, 'Prisma'); | |
| }); | |
| (this as any).$on('warn', (e: PrismaLogEvent) => { | |
| this.logger.warn(`${e.message}`, 'Prisma'); | |
| }); | |
| (this as any).$on('error', (e: PrismaErrorEvent) => { | |
| // e is an object with message and perhaps stack | |
| this.logger.error( | |
| `${e.message}`, | |
| e.stack ?? undefined, |
| this.logger.log(`${e.message}`, 'Prisma'); | ||
| }); | ||
|
|
||
| (this as any).$on('warn', (e: any) => { |
There was a problem hiding this comment.
Multiple type assertions to any reduce type safety. Consider defining an interface for the event object structure or using a type guard to safely access the Prisma event properties.
| this.logger.warn(`${e.message}`, 'Prisma'); | ||
| }); | ||
|
|
||
| (this as any).$on('error', (e: any) => { |
There was a problem hiding this comment.
Multiple type assertions to any reduce type safety. Consider defining an interface for the event object structure or using a type guard to safely access the Prisma event properties.
| ); | ||
| }); | ||
| } catch (e) { | ||
| // Listening may fail in some runtime builds; safely ignore |
There was a problem hiding this comment.
Silently catching and ignoring exceptions without logging makes debugging difficult if event registration actually fails. Consider logging this failure at debug level to aid troubleshooting.
| // Listening may fail in some runtime builds; safely ignore | |
| // Listening may fail in some runtime builds; log at debug level for troubleshooting | |
| this.logger.debug( | |
| `Failed to register Prisma event listeners: ${e instanceof Error ? e.message : String(e)}`, | |
| (e instanceof Error && e.stack) ? e.stack : undefined, | |
| 'PrismaService', | |
| ); |
This pull request introduces a new centralized logging system with a custom
LoggerService, integrates request and database logging, and improves error visibility across the application. The changes enhance observability, especially for authentication and database interactions, and standardize log output for easier debugging and monitoring.Logging Infrastructure and Integration:
LoggerServiceimplementing NestJS'sLoggerServiceinterface, providing structured logging methods (log,warn,error, etc.) and context support.LoggerServiceas a provider inAppModuleand configured the application to use it for all framework and custom logs. [1] [2]HTTP and Middleware Logging:
RequestLoggingMiddlewarethat logs all incoming HTTP requests with method, URL, status code, and response time. This middleware is now applied globally alongsideSessionActivityMiddleware. [1] [2]Database Logging and Robustness:
PrismaServiceto:LoggerServicefor logging database connection, disconnection, and Prisma client events (info,warn,error).DATABASE_URLfor connection configuration and implement retry logic for database connection with detailed logging on failures and disconnects.Authentication and Security Logging:
AuthServiceto log failed login attempts and invalid credentials using the new logger, while avoiding leaking sensitive user existence information. [1] [2]Application Bootstrap Logging:
main.tsto useLoggerServicefor application-level logs, including startup messages and Swagger UI initialization, ensuring consistent logging format across the app. [1] [2]…hanced request and error logging