-
Notifications
You must be signed in to change notification settings - Fork 0
Add: added middleware to limit request per IP #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/email/email.controller.ts
Outdated
| @HttpCode(HttpStatus.ACCEPTED) | ||
| @Post('resend-verification') | ||
| async reSendVerification(@Body('email') email: string) { | ||
| if (!email) throw new BadRequestException('Email is required'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont do this in controller , business logics should be in service , you can add validation by creating a dto I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/email/email.controller.ts
Outdated
| @HttpCode(HttpStatus.OK) | ||
| @Get('verify-email') | ||
| async verifyEmail(@Query('token') token: string) { | ||
| if (!token) throw new BadRequestException('Token is required'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated here too!
|
|
||
| const ipMap = new Map<string, RateLimitInfo>(); | ||
|
|
||
| @Injectable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Nest throttler might be useful
Feat analytics filter
…hortner into feat-event-emitter
src/auth/auth.service.ts
Outdated
| throw new BadRequestException({ | ||
| message: 'Something went wrong during signup', | ||
| error: (error as Error)?.message, | ||
| message: (error as Error)?.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: I don't think this might be a good approach . Error can be of different types. Even if it is query failed error or some errors that are coming from server it will throw BadReqauestException. Also in line 140 , that may be a Not Found Exception Error
| message: (error as Error)?.message, | |
| In line 140 | |
| const user = await this.userRepository.findOne({ | |
| where: { | |
| email: payload.email, | |
| }, | |
| }); | |
| if (!user) throw newNotFoundException('User not found'); |
Better do not use try catch until its an external service we are using , but if you have to handle error properly .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve already removed the try-catch blocks in the previous branch, so this will be resolved once that branch is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
pratham-outside
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added
Uh oh!
There was an error while loading. Please reload this page.