feat: update throttle limits for company and user endpoints#1583
feat: update throttle limits for company and user endpoints#1583
Conversation
There was a problem hiding this comment.
Pull request overview
Updates API rate-limiting on selected user and company endpoints to better align per-route throttle limits with expected usage patterns, overriding the app-wide default throttler settings.
Changes:
- Increased throttle limits for
/user/login/,/user/otp/verify/,/user/otp/login/, and/company/my/full. - Increased throttle limit for
/company/user/:companyId(invite user). - Added throttling to several user verification flows (email verify request/verify, password reset verify, email change request/verify) and company invite verification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
backend/src/entities/user/user.controller.ts |
Adjusts login/OTP throttle limits and adds throttling to multiple email/password verification endpoints. |
backend/src/entities/company-info/company-info.controller.ts |
Tunes throttling for “my full” company info, inviting users, and adds throttling to invite verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Throttle({ default: { limit: isTest() ? 200 : 5, ttl: 60000 } }) | ||
| @Get('user/email/verify/:verificationString') | ||
| async verifyEmail( |
There was a problem hiding this comment.
The throttle configuration object (ttl 60000, isTest() ? 200 : ...) is duplicated across multiple handlers in this controller, which increases the chance of inconsistent limits when changes are needed. Consider extracting shared throttle presets/helpers (e.g., 5/min, 10/min) into a constant or small factory to keep these values centralized.
| type: OperationResultMessageDs, | ||
| }) | ||
| @Throttle({ default: { limit: isTest() ? 200 : 5, ttl: 60000 } }) | ||
| @Get('user/email/change/request/') |
There was a problem hiding this comment.
GET user/email/change/request/ appears to trigger a side effect (sending an email change verification). Using GET for state-changing operations can lead to unintended requests (prefetching/crawlers) and makes caching semantics unsafe. Consider switching this endpoint to POST (or otherwise ensuring it is truly safe/idempotent and not cacheable).
| @Get('user/email/change/request/') | |
| @Post('user/email/change/request/') |
| @Throttle({ default: { limit: isTest() ? 200 : 10, ttl: 60000 } }) | ||
| @Get('my/full') | ||
| async getUserCompanies(@UserId() userId: string): Promise<FoundUserCompanyInfoDs | FoundUserFullCompanyInfoDs> { |
There was a problem hiding this comment.
The same inline throttle configuration pattern (ttl 60000, isTest() ? 200 : ...) is repeated for multiple endpoints. Consider centralizing these throttle presets (e.g., constants/factory) to avoid divergence when limits are tuned in the future.
| type: OperationResultMessageDs, | ||
| }) | ||
| @Throttle({ default: { limit: isTest() ? 200 : 5, ttl: 60000 } }) | ||
| @Get('user/email/verify/request') |
There was a problem hiding this comment.
GET user/email/verify/request appears to trigger a side effect (sending an email verification). Using GET for state-changing operations can lead to unintended requests (e.g., prefetching/crawlers) and makes caching semantics unsafe. Consider switching this endpoint to POST (or otherwise ensuring it is truly safe/idempotent and not cacheable).
| @Get('user/email/verify/request') | |
| @Post('user/email/verify/request') |
1 participant
No description provided.