Skip to content
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

feat(api): implement rate limiting in the api #793

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

Dhanushranga1
Copy link

@Dhanushranga1 Dhanushranga1 commented Feb 23, 2025

User description

Description

Implements rate limiting functionality to protect API endpoints from abuse. This change:

  • Adds ThrottlerModule configuration with a limit of 10 requests per 60 seconds window
  • Implements RateLimitGuard after authentication and API key validation
  • Ensures proper guard execution order (Auth → ApiKey → RateLimit)

Fixes #12

Dependencies

  • @nestjs/throttler

Future Improvements

  1. Endpoint-Specific Rate Limiting

    • Implement a custom @MaxRequestsPerUser(limit: number) decorator
    • Allow overriding default rate limits per endpoint
  2. Cache-Based Rate Limit Storage

    • Replace in-memory storage with Redis/cache-based solution
    • Store rate limit data as tuples: (userId, endpoint, lockedUntil)

Team name for fossHack20225: BunLock

Team Members:
@anaypurohit0907
@Dhanushranga1
@hanish-rishen

Thanks @rajdip-b for guiding us and answering all of our questions.

Screenshots

image

image

Developer's checklist

  • [] My PR follows the style guidelines of this project
  • [] I have performed a self-check on my work
  • [] My changes in code generate no new warnings
  • [] My changes follow proper guard execution order
  • [] Rate limiting configuration is properly tested
  • I have added test cases to show that the rate limiting works
  • Documentation has been updated to reflect rate limiting changes

Documentation Update

  • [] This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation

PR Type

Enhancement


Description

  • Implemented rate limiting for API requests using ThrottlerModule.

  • Added RateLimitGuard to enforce rate limits per user or IP.

  • Configured guard execution order: Auth → API Key → Rate Limit.

  • Improved OTP validation and cleanup logic in AuthService.


Changes walkthrough 📝

Relevant files
Enhancement
auth.service.ts
Refactored OTP validation and cleanup logic                           

apps/api/src/auth/service/auth.service.ts

  • Improved OTP validation logic with Prisma query fixes.
  • Enhanced OTP cleanup logic for expired entries.
  • Simplified and clarified method documentation.
  • Removed redundant comments and unused exception annotations.
  • +15/-54 
    app.module.ts
    Integrated rate limiting module and guard                               

    apps/api/src/app/app.module.ts

  • Integrated ThrottlerModule for rate limiting configuration.
  • Added RateLimitGuard to enforce rate limits.
  • Adjusted guard execution order for proper functionality.
  • +18/-2   
    rate-limit-guard.ts
    Added custom RateLimitGuard for API requests                         

    apps/api/src/common/rate-limit-guard.ts

  • Created RateLimitGuard extending ThrottlerGuard.
  • Implemented custom logic for user/IP-based rate limiting.
  • Added logging for debugging rate limit enforcement.
  • +54/-0   
    Additional files
    pnpm-lock.yaml +634/-230

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Dhanushranga1 and others added 5 commits February 23, 2025 02:12
    Changes Made:
    
        Added a RateLimitGuard that extends ThrottlerGuard to enforce API rate limits.
        Configured ThrottlerModule with a 60-second window allowing 10 requests per IP.
        Fixed type issues (remainingPoints and retryAfter do not exist in ThrottlerLimitDetail).
        Ensured the guard correctly handles excessive requests by throwing 429 Too Many Requests.
    
    Testing & Verification:
    
        Successfully blocked more than 10 requests within 60 seconds.
        Requests resumed normally after the time window expired.
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    12 - PR Code Verified

    Compliant requirements:

    • Implement rate limiting for the API to prevent abuse
    • Follow NestJS rate limiting documentation

    Requires further human verification:

    • Protect API endpoints from malicious actors - needs load testing to verify effectiveness
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Rate Limit Config

    The rate limit of 10 requests per 60 seconds may be too restrictive for legitimate API usage. Consider adjusting these values based on expected usage patterns.

    ttl: 60, // Time window in seconds
    limit: 10 // Max requests per user per window
    Potential Bug

    The rate limit key uses raw IP address without sanitization. This could lead to issues with IPv6 addresses or proxies. Consider normalizing the IP address format.

    const ip = request.ip
    
    // Use user ID if available; otherwise, use IP for rate limiting
    const key = userId ? `user-${userId}` : `ip-${ip}`

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Feb 23, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Limit failed OTP attempts

    Add a maximum retry count check before validating OTP to prevent brute force
    attacks. Track failed attempts and temporarily block after exceeding limit.

    apps/api/src/auth/service/auth.service.ts [79-87]

    +const failedAttempts = await this.prisma.otpAttempt.count({
    +  where: { 
    +    userId: user.id,
    +    createdAt: { gt: new Date(Date.now() - 15 * 60 * 1000) }
    +  }
    +});
    +if (failedAttempts >= 5) {
    +  throw new UnauthorizedException('Too many failed attempts. Please try again later.');
    +}
     const isOtpValid = await this.prisma.otp.findFirst({
       where: {
         userId: user.id,
         code: otp,
         expiresAt: {
           gt: new Date()
         }
       }
     })
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    __

    Why: Critical security feature that prevents brute force attacks on OTP validation by implementing a maximum retry limit with a cooldown period.

    High
    Mask sensitive data in logs

    Avoid logging sensitive information like email and OTP code in error logs, as
    this could expose user data. Use a redacted or masked version of the data
    instead.

    apps/api/src/auth/service/auth.service.ts [89-94]

     if (!isOtpValid) {
    -  this.logger.error(`Invalid login code for ${email}: ${otp}`)
    +  this.logger.error(`Invalid login code for ${email.slice(0,3)}***@${email.split('@')[1]}`);
       throw new UnauthorizedException(
         constructErrorBody('Invalid OTP', 'Please enter a valid 6-digit OTP.')
       )
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This is a critical security enhancement that prevents sensitive user data (email and OTP) from being exposed in logs, which could be a significant security risk if logs are compromised.

    High
    Sanitize rate limit key inputs

    The rate limit key should be sanitized to prevent injection attacks through
    malicious IPs or user IDs that could contain special characters.

    apps/api/src/common/rate-limit-guard.ts [46]

    -const key = userId ? `user-${userId}` : `ip-${ip}`
    +const sanitizedId = (id: string) => id.replace(/[^a-zA-Z0-9-]/g, '')
    +const key = userId ? `user-${sanitizedId(userId)}` : `ip-${sanitizedId(ip)}`
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Important security improvement that prevents potential injection attacks through unsanitized user IDs or IP addresses in rate limiting keys.

    Medium
    • Update

    keyshade.json
    pnpm-lock.yaml
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    this shouldnt go in gitignore

    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    can you revert the changes in this entire file?


    throw new HttpException(
    {
    message: 'Too many requests. Please try again later.'
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Return a constructErrorBody


    // Logging for debugging purposes
    this.logger.debug(`Rate limiting applied to key: ${key}`)

    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I don't think you have done the implementation of cache to actually make the logic work

    Copy link

    @Dhanushranga1, please resolve all open reviews!

    Copy link

    @Dhanushranga1, please resolve all open reviews; otherwise this PR will be closed after Sun Mar 02 2025 13:10:59 GMT+0000 (Coordinated Universal Time)!

    @Dhanushranga1
    Copy link
    Author

    Hey rajdip, sorry man, got caught up with some work, will start as soon as i can

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    API: Implement rate limiting in the API
    3 participants