Api rate limiting

[jspaine]

No description provided.

[vikramnr]

@jspaine What's the issue. :)

[jspaine]

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)

[Thirdoptics]

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

[jspaine]

That library looks good.

[Thirdoptics]

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

[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]

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?

[jspaine]

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 😄

[Thirdoptics]

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.

[jspaine]

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

[Thirdoptics]

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.

[jspaine]

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]

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