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(libs): Add API key rate limit middleware #8850

Merged
merged 8 commits into from
Dec 3, 2024

Conversation

aarushik93
Copy link
Contributor

@aarushik93 aarushik93 commented Dec 1, 2024

Once we release api key feature, we will want to be able to rate limit as well. This is the foundation for that.
For now it is a blanket rate limit, later we will be able to add tiered rate limits

Changes 🏗️

Added new middleware libary in autogpt_libs which contains the logic for getting the api key, storing it's details in redis and checking how many requests it's done, how many are left and what the reset time is.

@github-actions github-actions bot added platform/backend AutoGPT Platform - Back end size/m labels Dec 1, 2024
Copy link

netlify bot commented Dec 1, 2024

Deploy Preview for auto-gpt-docs-dev canceled.

Name Link
🔨 Latest commit f36be90
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs-dev/deploys/674ece36a5b8510008303b4d

Copy link

netlify bot commented Dec 1, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit f36be90
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/674ece363c7a9300083f987d

@aarushik93 aarushik93 marked this pull request as ready for review December 1, 2024 15:26
@aarushik93 aarushik93 requested a review from a team as a code owner December 1, 2024 15:26
@aarushik93 aarushik93 requested review from kcze, majdyz, ntindle and Swiftyos and removed request for a team December 1, 2024 15:26
Copy link

qodo-merge-pro bot commented Dec 1, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

Redis Security:
The default Redis configuration uses localhost without authentication or TLS. In production, this could expose the rate limiting data if Redis is accessible externally. Should enforce secure Redis connection configuration with authentication and TLS.

⚡ Recommended focus areas for review

Security Issue
The middleware accepts any API key format and doesn't validate the key format or authenticity before rate limiting. Should verify API key is valid before applying rate limits.

Race Condition
The Redis operations are not atomic which could lead to race conditions under high concurrency. Consider using Redis MULTI/EXEC or Lua scripting.

Configuration Risk
Default Redis URL uses localhost without authentication, which could be a security risk if accidentally used in production.

@Pwuts Pwuts changed the title feat(platform/libs) Add rate limt middleware feat(platform/libs): Add rate limt middleware Dec 2, 2024
@Pwuts Pwuts changed the title feat(platform/libs): Add rate limt middleware feat(platform/libs): Add rate limot middleware Dec 2, 2024
@Pwuts Pwuts changed the title feat(platform/libs): Add rate limot middleware feat(platform/libs): Add rate limit middleware Dec 2, 2024
@Pwuts Pwuts changed the title feat(platform/libs): Add rate limit middleware feat(platform/libs): Add API key rate limit middleware Dec 2, 2024
@Pwuts Pwuts changed the title feat(platform/libs): Add API key rate limit middleware feat(libs): Add API key rate limit middleware Dec 2, 2024
@aarushik93 aarushik93 merged commit 3bca279 into dev Dec 3, 2024
18 checks passed
@aarushik93 aarushik93 deleted the aarushikansal/api-rate-lmiting branch December 3, 2024 09:25
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.

3 participants