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

Add Rate Limiting to API Endpoints #76

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

Conversation

Advaitgaur004
Copy link
Contributor

@Advaitgaur004 Advaitgaur004 commented Jan 26, 2025

Overview

Implemented IP-based rate limiting for sensitive API endpoints to protect against abuse and ensure service stability.

Changes

  • Added middleware package with sliding window rate limiter
  • Applied rate limiting to auth and API endpoints
  • Set default limits to 50 requests per 30 seconds per IP
  • Added proper HTTP 429 responses with Retry-After headers

Implementation Details

  • Uses in-memory sliding window algorithm for accurate rate tracking
  • Thread-safe implementation with proper mutex locking
  • Efficient memory usage with automatic cleanup of expired records
  • Handles both direct IP and X-Forwarded-For header
  • Configurable window size and request limits per endpoint

Protected Endpoints

  • /auth/oauth
  • /auth/callback
  • /api/user
  • /auth/logout

Rate Limit Response

When limit is exceeded:

  • Status: 429 Too Many Requests
  • Header: Retry-After: <window-size>
  • Body: "Rate limit exceeded. Please try again later."

Testing

Tested with concurrent requests:

  • Successfully allows first 50 requests
  • Rate limits subsequent requests with proper 429 responses
  • Correctly tracks and resets quotas after window expiration
  • To test, you can create a scripts as follows
package main

import (
	"fmt"
	"net/http"
	"sync"
	"time"
)

func main() {
	url := "http://localhost:8000/auth/oauth"
	
	numRequests := 120 // More than our limit of 100 per minute
	
	var wg sync.WaitGroup
	results := make(chan string, numRequests)
	
	fmt.Printf("Making %d requests to %s\n", numRequests, url)
	
	for i := 0; i < numRequests; i++ {
		wg.Add(1)
		go func(requestNum int) {
			defer wg.Done()
			
			client := &http.Client{
				CheckRedirect: func(req *http.Request, via []*http.Request) error {
					return http.ErrUseLastResponse // Don't follow redirects
				},
			}
			
			resp, err := client.Get(url)
			if err != nil {
				results <- fmt.Sprintf("Request %d: Error: %v", requestNum, err)
				return
			}
			defer resp.Body.Close()
			
			status := resp.StatusCode
			rateLimit := resp.Header.Get("Retry-After")
			
			if status == http.StatusTooManyRequests {
				results <- fmt.Sprintf("Request %d: Rate Limited (429) - Retry After: %s", requestNum, rateLimit)
			} else {
				results <- fmt.Sprintf("Request %d: Status %d", requestNum, status)
			}
		}(i)
		
		time.Sleep(10 * time.Millisecond)
	}
	
	go func() {
		wg.Wait()
		close(results)
	}()
	
	successCount := 0
	rateLimitCount := 0
	
	for result := range results {
		if result[len(result)-3:] == "302" { // OAuth endpoint returns 302 redirect
			successCount++
		} else if result[len(result)-3:] == "429" {
			rateLimitCount++
		}
	}
}

Security Impact

  • Protects against brute force attacks
  • Prevents API abuse and DoS attempts
  • Ensures fair resource distribution

@Advaitgaur004
Copy link
Contributor Author

@its-me-abhishek added as Carlos said, actually both #76 and #77 added simultaneously, so can you review them both

@its-me-abhishek
Copy link
Collaborator

you seem to have implemented the rate limiter on Oauth handlers, but not the others, what was the thought process behind this one?

@Advaitgaur004
Copy link
Contributor Author

Implemented on all apis, but for testing Oauth doesnt required authentication, so to test the rate limit working, i'll use Oauth

@its-me-abhishek
Copy link
Collaborator

please elaborate, i am unable to understand :/

backend/main.go Outdated Show resolved Hide resolved
@its-me-abhishek
Copy link
Collaborator

imo, as i had been planning to implement rate-limiting, it should have been implemented on the JobQueue or Task based endpoints, specifically, based on IP, as these can still be pinged over and over. The app does not contain any auth so its almost useless to implement this on only auth ones, provided the backend is meant only (for most part) for the app

@Advaitgaur004
Copy link
Contributor Author

imo, as i had been planning to implement rate-limiting, it should have been implemented on the JobQueue or Task based endpoints, specifically, based on IP, as these can still be pinged over and over. The app does not contain any auth so its almost useless to implement this on only auth ones, provided the backend is meant only (for most part) for the app

Lol, my bad.....What I intended to say is:

I would like someone to review whether the rate limiting is functioning correctly. If it is, I will then apply it to the necessary APIs, such as the job queue, add task API and all. After this review, I will implement rate limiting on all other APIs as well.

@its-me-abhishek
Copy link
Collaborator

I would like someone to review whether the rate limiting is functioning correctly. If it is, I will then apply it to the necessary APIs, such as the job queue, add task API and all. After this review, I will implement rate limiting on all other APIs as well.

Please implement that on all Endpoints, I will test that using a Postman simulator on my local machine after some time

@its-me-abhishek
Copy link
Collaborator

Also, please open relevant issues for the same, so that tracking things becomes easy in future

@Advaitgaur004
Copy link
Contributor Author

Test also Added.
Testing can be done from the backend go test -v ./middleware -run TestRateLimiter

Clients will receive these headers when rate limited:

HTTP/1.1 429 Too Many Requests
X-RateLimit-Limit: 50
X-RateLimit-Reset: Mon, 02 Jan 2023 15:04:05 MST
Retry-After: 30s

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

Successfully merging this pull request may close these issues.

2 participants