fix: serialization and primitives validation (no. 1) #3312
Conversation
|
Amazing work! A small suggestion for future PR's is a one I learned from @LetItRock, is to create inline comments on the PR code diff to explain why a particular change is made. In the way you have written in the PR body, just makes it more contextual to the actual code change. Not a must, but just something I find makes easier to review bigger PR's 🚀 |
|
|
||
| export class ChangesRequestDto extends PaginationRequestDto(10, 100) { |
| @UserSession() user: IJwtPayload, | ||
| @Body() body: { color: string; logo: string; fontColor: string; contentBackground: string; fontFamily: string } | ||
| ) { | ||
| async updateBrandingDetails(@UserSession() user: IJwtPayload, @Body() body: UpdateBrandingDetailsDto) { |
| @@ -12,6 +12,8 @@ export class ChangeEntity { | |||
|
|
|||
| _environmentId: EnvironmentId; | |||
|
|
|||
| user: Pick<UserEntity, '_id' | 'firstName' | 'lastName' | 'profilePicture'>; | |||
There was a problem hiding this comment.
Should we add this one here only for the ChangeRepository.getList populate?
Maybe instead we can create a response type with it for getList, wdyt?
There was a problem hiding this comment.
I agree - I will create type for this particular method
| export const IsBooleanAny = () => | ||
| IsIn([true, false, 'true', 'false'], { | ||
| message: '$property must be a boolean value', | ||
| }); |
There was a problem hiding this comment.
Can we add a test in order to check this one?
There was a problem hiding this comment.
Sure. I made this validator because frontend sends "true"/"false" for some credentials fields - maybe we will change this behaviour to send only real boolean and use built-in @IsBoolean?
I can imagine scenario when we have something like this and someone will trust dto types:
class ExampleDto {
@IsBooleanAny()
isSample: boolean;
}
@Post()
createExample(@Body() body: ExampleDto) {
if (body.isSample) {
console.log(body.isSample) // 'false' is truthy value because it's string :D
// do something
}
}Our dtos are not reliable. For credentials it works because I guess mongoose can handle it with schema.
So I would change frontend code and just add @IsBoolean validator.
There was a problem hiding this comment.
but for example in some e2e tests for integration - regular boolean is sent.
|
Looking good! Let's finish with @djabarovgeorge suggestions and we can merge this :) |
|
@scopsy @djabarovgeorge new changes are ready to review |
There was a problem hiding this comment.
Loved your work here, and left a small suggestion.
P.S. I was inspired a bit by you and created a @isnotempty validator in one of my tasks, once we merge this one I will move it to your validators folder :)
| @@ -6,6 +6,10 @@ import { ChangeEntity, ChangeDBModel } from './change.entity'; | |||
| import { Change } from './change.schema'; | |||
| import { UserEntity } from '../user'; | |||
|
|
|||
| type ChangeEntityPopulated = ChangeEntity & { | |||
There was a problem hiding this comment.
Could we export it one level outside
as we have in environment types?
libs/dal/src/repositories/environment/types.ts
|
@djabarovgeorge @scopsy I think it's ready to merge - check out last small changes I made. |
What change does this PR introduce?
This is first PR in validation fix pull requests series.
It fixes some problems I mentioned in DMs to @scopsy :)
Affected modules (some of them only partially)
Changes:
@IsString,@IsEnum...) for affected modules to avoid data corruption and some other dangerous things.Changeentity because API returns whole user object with sensitive data (password hash, reset token hash). Now it returns only necessary for UI data.@IsMongoIdin some places, because API often returnsHTTP Status: 500when Mongo Id is expected but some random value is provided. During review pay attention to whether the right fields have been annotated as Mongo Ids.@Query('someQueryParam') param: stringwith@Query() query: QueryDtoto perform validation and improve Swagger documentation :) I fixed HTTP Parameter Pollution in affected modules.transformers.tsfile in shared modules. It actually will be used in future PRs in the series.validators.tsfile for custom class validatorsintegrationmodule. In most cases I try to fix my solutions first without changing existing tests - but in some tests "EMAIL" was used for channel insteadChannelTypeEnum.EMAIL. I checked frontend and some other tests (they rely on enum) - I decided to accept only ChannelTypeEnum as is without transformations to lowercase etc.Why was this change needed?
To improve security and prevent some crazy things with corrupted data to happen in the future.
Other information (Screenshots)