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

API: Implement rate limiting in the API #12

Open
rajdip-b opened this issue Dec 26, 2023 · 23 comments · May be fixed by #793
Open

API: Implement rate limiting in the API #12

rajdip-b opened this issue Dec 26, 2023 · 23 comments · May be fixed by #793
Assignees
Labels
difficulty: 4 FOSSHack2025 FOSS Hack is scheduled from Feb 22 to Feb 23. FOSS-accepted issues would have this tag good first issue Good for newcomers help wanted Extra attention is needed scope: api Everything related to the API type: enhancement New feature or request
Milestone

Comments

@rajdip-b
Copy link
Member

Is your feature request related to a problem? Please describe.
I would like the API to have rate limiting set up so that malicious actors can't spam our APIs.

Describe the solution you'd like
Use this document as a reference: https://docs.nestjs.com/security/rate-limiting

Suggestions for implementing this feature are most welcome!

@rajdip-b rajdip-b added type: enhancement New feature or request good first issue Good for newcomers question Further information is requested labels Dec 26, 2023
@rajdip-b rajdip-b added this to the Release v1 milestone Dec 26, 2023
@rajdip-b rajdip-b moved this to Approval required in keyshade-api Dec 26, 2023
@rajdip-b rajdip-b added the scope: api Everything related to the API label Dec 26, 2023
@Sachinsharma01
Copy link

Sachinsharma01 commented Dec 30, 2023

@rajdip-b picking this as this does not require major change, can work parallelly with create user #13
assign this to me

@rajdip-b
Copy link
Member Author

@Sachinsharma01 assigned to you. Do open a thread in discord regarding this issue since we still haven't fixed on what parameters we will do the rate limit. So you can get on with this after we have came to a decision!

@Sachinsharma01
Copy link

ok, got it

@Sachinsharma01
Copy link

As per discussion, we are going to have rate limiting as below

We would want to send OTP, validate OTP to be rate limited to 10 requests per minute for a single user
Upon multiple failures of validate OTP (20 consecutive times over any period), we want to send a mail to the user that their account failed authentication 20 times. They might want to take the necessary steps.
The rest of the endpoints will follow a standard 20-50 requests per minute based on the computation power it takes, but we will stall that for a later issue.

@rajdip-b
Copy link
Member Author

rajdip-b commented Jan 9, 2024

Yes, correct. That looks good! @Sachinsharma01

@Sachinsharma01
Copy link

yeah, picking this up for development

@rajdip-b rajdip-b moved this from Approval required to In progress in keyshade-api Jan 22, 2024
@bansal-harsh-2504
Copy link
Contributor

bansal-harsh-2504 commented Oct 23, 2024

can I work on this issue? @rajdip-b

@rajdip-b
Copy link
Member Author

Sure thing! Go ahead.

@rajdip-b
Copy link
Member Author

The requirements of this issue is still not clear, so please feel free to add a list of the implementations you would be making before you go ahead and code it.

@bansal-harsh-2504
Copy link
Contributor

But the next 10days are very hectic for me and I can only start solving after that. If that's okay with you

@rajdip-b
Copy link
Member Author

Yeah no problem with that

@bansal-harsh-2504 bansal-harsh-2504 removed their assignment Nov 6, 2024
@Dhanushranga1
Copy link

Hi there, My team and I are paricipating in FOSS Hack 2025 are interested in working on this issue. Could you please provide guidance and more information regarding this issue.

@rajdip-b
Copy link
Member Author

Howdy! yes, we will update the issue description with the details shortly. This issue has been present for a long time now.

@rajdip-b rajdip-b changed the title Implement rate limiting in the API API: Implement rate limiting in the API Feb 11, 2025
@rajdip-b rajdip-b added help wanted Extra attention is needed difficulty: 4 and removed question Further information is requested labels Feb 11, 2025
@rajdip-b rajdip-b added the FOSSHack2025 FOSS Hack is scheduled from Feb 22 to Feb 23. FOSS-accepted issues would have this tag label Feb 11, 2025
@Dhanushranga1
Copy link

Howdy, Thanks for assigning the issue, I'll review the details once updated and get started. Let me know if there's anything specific you'd like us to consider.

@Dhanushranga1
Copy link

Hey, Rajdip hope everything's going well, so fosshack's tmrw, can you please guide us with the details regarding the api rate limiting issue.

@rajdip-b
Copy link
Member Author

Oh boy, yes i absolutely forgot. I'm updating this right away. Thanks for dropping in a reminder

@rajdip-b
Copy link
Member Author

rajdip-b commented Feb 21, 2025

Okay I think as of now this is a pretty generalized issue. We don't have any specific use cases as of now. What we would ideally like to have is, all the endpoints should have a rate limit for the number of requests. For instance, we don't want a user to be able to spam our APIs with automated requests. Let's say, someone tries to create a DOS attack on our servers by making expensive requests.

The solution we would like to have is, put in a configurable number of requests per second that would be accepted by any users. We should be filtering this both based on IP Address and user ID.

@Dhanushranga1
Copy link

Dhanushranga1 commented Feb 23, 2025

Hey Rajdip, Good Morning and a Happy Sunday :)
I’ve set up global rate limiting that applies per user (if authenticated) or per IP (if not). Also fixed the OTP validation issues, so it’s workin now. Do you think we should set a specific lockout duration, and should users get notified when their account gets locked? currently in the api logs it would show the 429 too many request error.

So I was also thinking about future enhancements, like account lockout for too many failed OTP attempts, but I’m not exactly sure how I’d do it. Right now, I’m thinking of storing the lockout status in the database since I’m not familiar with it and don’t want to break anything.

@rajdip-b
Copy link
Member Author

I don't think an unlock feedback would be needed.

@rajdip-b
Copy link
Member Author

rajdip-b commented Feb 23, 2025

Also, we don't want the locks to be specific, but general to all the endpoints. But we should also have the ability to configure the max hits for each and every endpoint. I would suggest you to develop a custom decorator to override this. Something like @MaxRequestsPerUser(20). By default, the value can be 10 per second (or whatever you feel is okay) if the annotation is not specified. But if it is, we should accept the value as an override.

As for storing the status, i think the cache might be a better place to do it. A tuple like (userId, endpoint, lockedUntil) would be a good choice.

@Dhanushranga1
Copy link

oh ok, well i have already implemented a general lock out period for all endpoints, we will work on the overide usinng a decorator. might i push a pr so you can review my progress?

Copy link

@Dhanushranga1, please open a draft PR linking this issue!

anaypurohit0907 added a commit to anaypurohit0907/keyshade that referenced this issue Feb 23, 2025
Copy link

@Dhanushranga1, please open a draft PR linking this issue; otherwise you will be unassigned from this issue after Tue Feb 25 2025 10:02:30 GMT+0000 (Coordinated Universal Time)!

@Dhanushranga1 Dhanushranga1 linked a pull request Feb 23, 2025 that will close this issue
3 tasks
anaypurohit0907 added a commit to anaypurohit0907/keyshade that referenced this issue Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: 4 FOSSHack2025 FOSS Hack is scheduled from Feb 22 to Feb 23. FOSS-accepted issues would have this tag good first issue Good for newcomers help wanted Extra attention is needed scope: api Everything related to the API type: enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging a pull request may close this issue.

4 participants