pantry-for-good icon indicating copy to clipboard operation
pantry-for-good copied to clipboard

Published 20 hours ago • verified publisher icon freeCodeCamp

Api rate limiting

Open jspaine opened this issue 7 years ago • 13 comments

jspaine avatar Jul 05 '17 14:07 jspaine

@jspaine What's the issue. :)

vikramnr avatar Jul 26 '17 03:07 vikramnr

You can send as many requests as the network is capable of to the app and it'll try to process them all. Could be for denial of service, brute force logins. It should be limited to a reasonable number, say 20 for general api routes and 5 for auth related routes (requests per ip per second)

jspaine avatar Jul 26 '17 07:07 jspaine

@jspaine do you want to do something from scratch or would implementing something like express-rate-limit

Thirdoptics avatar Oct 19 '17 19:10 Thirdoptics

That library looks good.

jspaine avatar Oct 20 '17 20:10 jspaine

Sounds good, I'll see if I can get it implemented

Thirdoptics avatar Oct 23 '17 17:10 Thirdoptics

I haven't forgotten about this, I've been working on it, just taking longer than I thought to get the server side stuff figured out. Made good progress today and should have something to post tomorrow and probably some questions about implementation and specific behavior.

Thirdoptics avatar Oct 30 '17 23:10 Thirdoptics

After I'm sure too much overthinking, I settled on a fairly basic implementation that can be seen in the feature/api-rate-limit branch in my repo. I'm hoping you can review and let me know if I'm on the right track and if I've missed anything major. Right now it just sets response status to 429 with a message and blocks further requests.

a few questions about overall app behavior:

  • Do we want the client to handle this response with an error page or some other behavior or is the server response sufficient?
  • Should I keep this to the production environment only?
  • Do we want to log this or do more than just cut off responses?

Thirdoptics avatar Oct 31 '17 22:10 Thirdoptics

Good questions,

  • I don't think any user should be able to hit the limits manually in a browser, so I guess there's no need to show an error page or anything more than sending the status code.
  • You can do, probably a good idea to at least keep it out of the test environment, I doubt the tests would hit the limit but would be annoying if they did.
  • There's nowhere to log it really other than console, and I'm not sure it's a good idea without extra work to also limit the logs...

I don't see anything in that branch, did you push to it? Or I'm blind :smile:

jspaine avatar Nov 01 '17 11:11 jspaine

Oops, made the push, just forgot to make a commit first 🤦

Which would've shown the tests hitting the limiter, which was in fact very annoying. Got that all fixed, committed and pushed.

Thirdoptics avatar Nov 01 '17 15:11 Thirdoptics

Hehe :smiley: Looks good, I was thinking around 5 and 20 per second though, to start with, not per minute.

jspaine avatar Nov 02 '17 21:11 jspaine

Gah, thought I'd put that back to where it needed to be after my own testing, gotta get better at catching those little details.

Fixed and committed, couldn't find a whole lot of info on it so I was unsure if there was a better method for implementing something like this. I'll send a pull request if all looks good though.

Thirdoptics avatar Nov 02 '17 22:11 Thirdoptics

Checking the diff is pretty useful, either after making a pull request or just with git diff <hash_of_first_commit> <hash_of_last_commit>.

I haven't seen much about it either, just seemed an easy way to add a little extra defence.

jspaine avatar Nov 02 '17 22:11 jspaine

@Thirdoptics didn't you finish this? I thought it had been merged but doesn't seem so, want to make a PR?

jspaine avatar Dec 11 '17 23:12 jspaine