RI-7200 add request metadata to session metadata and exclude it from … #4771
Conversation
ArtemHoruzhenko
requested review from
KIvanow,
KrumTy,
dantovska,
pawelangelow,
pd-redis and
valkirilov
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR adds request metadata to session metadata while ensuring it's excluded from logs for security purposes. The implementation uses class-transformer groups to control when sensitive request metadata is serialized.
- Adds
requestMetadatafield toSessionMetadatawith security group exposure control - Implements
logDataToPlainfunction to sanitize log data and exclude sensitive metadata - Updates logging infrastructure to use the new sanitization function
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| redisinsight/api/src/common/models/session.ts | Adds requestMetadata field with security group exposure |
| redisinsight/api/src/utils/logsFormatter.ts | Implements logDataToPlain function for log sanitization |
| redisinsight/api/src/common/logger/app-logger.ts | Updates logging to use new sanitization functions |
| redisinsight/api/src/common/decorators/session/session-metadata.decorator.ts | Extracts and includes requestMetadata in session creation |
| redisinsight/api/src/common/decorators/client-metadata/client-metadata.decorator.ts | Adds security groups to client metadata transformation |
| redisinsight/api/src/common/models/client-metadata.ts | Updates type reference from Session to SessionMetadata |
| seen.add(value); | ||
|
|
||
| if (!isPlainObject(value)) { | ||
| return instanceToPlain(value); |
There was a problem hiding this comment.
The instanceToPlain call doesn't specify transformation groups, which could expose sensitive data. Since this function is used for logging, it should exclude security-sensitive fields by not including the 'security' group in the transformation options.
| return instanceToPlain(value); | |
| return instanceToPlain(value, { groups: [] }); |
| return logData.join(separator); | ||
| }); | ||
|
|
||
| const MAX_DEPTH = 10; |
There was a problem hiding this comment.
[nitpick] The MAX_DEPTH constant should be moved to the top of the file with other constants or configuration values for better maintainability and consistency with the existing LOGGER_CONFIG pattern.
| const MAX_DEPTH = 10; |
| @IsOptional() | ||
| requestMetadata?: Record<string, any> = {}; | ||
|
|
||
| @Expose() |
There was a problem hiding this comment.
Should we expose all of those? For example sessionId?
| @IsObject() | ||
| data?: Record<string, any> = {}; | ||
|
|
||
| @Expose({ groups: ['security'] }) |
There was a problem hiding this comment.
Nit: this should become Groups.Security constant as it's on a lot of places
There was a problem hiding this comment.
Correct but it is not a single place. Didn't want to introduce more changes in bunch of not related to this PR files
ArtemHoruzhenko
deleted the
be/feature/RI-7200-add-request-meta-to-session-metadata
branch
…logs