Open Prakhargarg-2010196 opened 2 weeks ago
β±οΈ Estimated effort to review: 3 π΅π΅π΅βͺβͺ |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Key issues to review Configuration Concern The ThrottlerModule is configured with environment variables, but there's no validation or default values set. This could lead to runtime errors if the variables are not properly set. Error Handling The `resendOtp` method catches all errors and only logs them. This could hide important errors and make debugging difficult. Rate Limiting Configuration The `resendOtp` endpoint has a hard-coded rate limit that might not be flexible for different environments or use cases. |
Category | Suggestion | Score |
Input validation |
Add input validation for the email parameter in the resendOtp method___ **Consider adding input validation for the email parameter in theresendOtp method. This can help prevent unnecessary processing and improve security by ensuring only valid email addresses are processed.** [apps/api/src/auth/controller/auth.controller.ts [49-58]](https://github.com/keyshade-xyz/keyshade/pull/445/files#diff-ca5c58065b10468a07314a5a9224336ac37df84a4a0e769babb2ed040cf8c82aR49-R58) ```diff @Public() @Post('resend-otp/:email') @UseGuards(ThrottlerGuard) @Throttle({ default: { ttl: seconds(1), limit: 2 } }) async resendOtp( - @Param('email') + @Param('email', new ParseEmailPipe()) email: string ): Promise Suggestion importance[1-10]: 9Why: Adding input validation for the email parameter enhances security and prevents unnecessary processing of invalid data, which is crucial for maintaining the integrity of the application. | 9 |
Error handling |
Implement more specific error handling in the catch block___ **Consider handling specific error types separately in the catch block of theresendOtp method. This would allow for more precise error handling and logging, potentially improving the ability to debug and respond to different types of errors.** [apps/api/src/auth/service/auth.service.ts [68-74]](https://github.com/keyshade-xyz/keyshade/pull/445/files#diff-b133cdd3d1772bc73698061218512fd024ce1ed7cd9eaeb27858e6acb444b90bR68-R74) ```diff try { const user = await getUserByEmailOrId(email, this.prisma) const otp = await generateOtp(email, user.id, this.prisma) await this.mailService.sendOtp(email, otp.code) } catch (e) { - this.logger.error(`Resending OTP failed ${e}`) + if (e instanceof PrismaClientKnownRequestError) { + this.logger.error(`Database error while resending OTP: ${e.message}`) + } else if (e instanceof MailServiceError) { + this.logger.error(`Mail service error while resending OTP: ${e.message}`) + } else { + this.logger.error(`Unexpected error while resending OTP: ${e}`) + } + throw new InternalServerErrorException('Failed to resend OTP') } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: This suggestion improves error handling by distinguishing between different error types, which enhances debugging and provides more informative logging. It also introduces an InternalServerErrorException, which is a good practice for handling unexpected errors. | 8 |
Enhancement |
β Provide default values for throttling configuration___ **Add default values for the THROTTLE_TTL and THROTTLE_LIMIT variables to provideguidance for users setting up the environment.** [.env.example [54-56]](https://github.com/keyshade-xyz/keyshade/pull/445/files#diff-a3046da0d15a27e89f2afe639b25748a7ad4d9290af3e7b1b6c1a5533c8f0a8cR54-R56) ```diff # Enter time in secs -THROTTLE_TTL= -THROTTLE_LIMIT= +THROTTLE_TTL=60 +THROTTLE_LIMIT=10 ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 8Why: Adding default values for environment variables can guide users in setting up their environment correctly and ensure that the application has sensible defaults, improving usability and reducing setup errors. | 8 |
Code cleanup |
β Remove TODO comments and rely on controller-level throttling___ **Remove the TODO comments and implement the actual throttling logic in theresendOtp method. The throttling should be handled at the controller level using the @Throttle decorator, as shown in the auth.controller.ts file.**
[apps/api/src/auth/service/auth.service.ts [64-67]](https://github.com/keyshade-xyz/keyshade/pull/445/files#diff-b133cdd3d1772bc73698061218512fd024ce1ed7cd9eaeb27858e6acb444b90bR64-R67)
```diff
-/**
- *TODO Resend the otp based on another function send otp but
- *TODO with a throttler to rate limit the api
- */
+// Throttling is handled at the controller level
```
`[Suggestion has been applied]`
Suggestion importance[1-10]: 7Why: Removing the TODO comments clarifies that throttling is already handled at the controller level, which improves code readability and reduces confusion about where throttling logic should be implemented. | 7 |
Best practice |
Specify exact version for "@nestjs/throttler" package___ **Consider adding a specific version for the "@nestjs/throttler" package instead ofusing the caret (^) to ensure consistency across environments.** [apps/api/package.json [35]](https://github.com/keyshade-xyz/keyshade/pull/445/files#diff-2c40985d6d91eed8ae85ec1c8e754a85984ee32e156a600d2b7a467423d7e338R35-R35) ```diff -"@nestjs/throttler": "^6.2.1", +"@nestjs/throttler": "6.2.1", ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Specifying an exact version can help ensure consistency across different environments, reducing the risk of unexpected behavior due to version changes. However, it may limit flexibility in receiving minor updates and patches. | 7 |
Configuration management |
β Use type-safe configuration retrieval for throttle settings___ **Consider using environment variables or a configuration file for the throttle limitand TTL values instead of hardcoding them. This would make the application more flexible and easier to configure in different environments.** [apps/api/src/app/app.module.ts [41-50]](https://github.com/keyshade-xyz/keyshade/pull/445/files#diff-edcd2071cfcb26ce4d2c9195a095fd4a86461ee5a66ceceb1d441eaaef6efec9R41-R50) ```diff ThrottlerModule.forRootAsync({ imports: [ConfigModule], inject: [ConfigService], useFactory: (config: ConfigService) => [ { - ttl: seconds(config.get('THROTTLE_TTL')), - limit: config.get('THROTTLE_LIMIT') + ttl: seconds(config.get Suggestion importance[1-10]: 6Why: Using type-safe configuration retrieval is a minor improvement that enhances code safety and maintainability, ensuring that the configuration values are of the expected type. However, the impact is relatively small compared to other suggestions. | 6 |
π‘ Need additional feedback ? start a PR chat
User description
Description
Base implemented by using present functions to resend otp to the user. Tried using the already present sendotp function. But as the errors returned are different in the resend otp took few other functions and called those instead of calling sendOtp().
Fixes #326 (pending)
Dependencies
Mention any dependencies/packages used
Future Improvements
Developer's checklist
If changes are made in the code:
Documentation Update
PR Type
enhancement, dependencies
Description
resendOtp
endpoint in theAuthController
with throttling to limit the number of OTP resend requests.ThrottlerModule
andThrottlerGuard
to the application for rate limiting API requests.AuthService
to include aresendOtp
method with error handling.THROTTLE_TTL
andTHROTTLE_LIMIT
for configuring throttling behavior.@nestjs/throttler
to the project dependencies and updated the lock file accordingly.Changes walkthrough π
app.module.ts
Add throttling configuration and guard to AppModule
apps/api/src/app/app.module.ts
ThrottlerModule
configuration for rate limiting.ThrottlerGuard
as a global guard.auth.controller.ts
Implement resend OTP endpoint with throttling
apps/api/src/auth/controller/auth.controller.ts
resendOtp
endpoint with throttling.ThrottlerGuard
toresendOtp
endpoint.auth.service.ts
Add resend OTP functionality to AuthService
apps/api/src/auth/service/auth.service.ts
resendOtp
method inAuthService
..env.example
Add throttling configuration variables to .env.example
.env.example - Added `THROTTLE_TTL` and `THROTTLE_LIMIT` environment variables.
package.json
Update package.json with throttler dependency
apps/api/package.json
@nestjs/throttler
as a dependency.reflect-metadata
in devDependencies.pnpm-lock.yaml
Update pnpm-lock.yaml for new dependencies
pnpm-lock.yaml
@nestjs/throttler
.