server: rate limiter v1 #1708
server: rate limiter v1 #1708
Conversation
TBonnin
force-pushed
the
tbonnin/NAN-359/rate-limiting
branch
2 times, most recently
from
0086f2c
to
0eed9cb
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
TBonnin
force-pushed
the
tbonnin/NAN-359/rate-limiting
branch
from
0eed9cb
to
f6decbe
Compare
TBonnin
requested review from
khaliqgant and
bastienbeurier
and removed request for
khaliqgant
| const opts = { | ||
| keyPrefix: 'middleware', | ||
| points: 1200, | ||
| duration: 60, |
There was a problem hiding this comment.
I picked a pretty high global limit (20rps). The goal is for now to prevent an "infinite" number of requests to overload the server when users misbehave like it happened with gretel last month.
We can tweak the logic as much as we want later. Let me know what you think
There was a problem hiding this comment.
no need to change anything, just thinking out loud.
when I set a ratelimiter I always try to put a large window because it allows a few burst but not a constant one, so you don't end up locking the system constantly.
- in a 60s window you can burst 1200 http call every 60s
- in a 600s window you can burst once every 600s, but if you reach the limit sooner you are blocked for a long period of time
In other word, an attacker/bad script would only be block for a few seconds now.
There was a problem hiding this comment.
Setting the right limit is tricky indeed. A larger window with the same limit also means the avg rate per seconds is lower.
I am sure we will be tweaking the logic to handle more scenarii in the future but for now I would sleep better at night knowing that beyond a certain threshold, request will be discarded quickly and not hit the rest of the system, especially the db
| logger.info(`Rate limit exceeded for ${key}. Request: ${req.method} ${req.path})`); | ||
| next(); | ||
| // TODO: | ||
| // res.status(429).send('Too Many Requests'); |
There was a problem hiding this comment.
As a first step, we are just logging and not sending the 429. Just to make sure there is no obvious issue. I will let it run a few hours like this before to start sending 429
| const apiPublicAuth = authMiddleware.publicKeyAuth.bind(authMiddleware); | ||
|
|
||
| const apiAuth = [authMiddleware.secretKeyAuth, rateLimiterMiddleware]; | ||
| const apiPublicAuth = [authMiddleware.publicKeyAuth, rateLimiterMiddleware]; |
There was a problem hiding this comment.
@khaliqgant why were we using bind before?
There was a problem hiding this comment.
Legacy, I'm actually not sure why and not sure if it is needed.
TBonnin
requested review from
bodinsamuel and
khaliqgant
and removed request for
bastienbeurier
TBonnin
force-pushed
the
tbonnin/NAN-359/rate-limiting
branch
2 times, most recently
from
d01fb4c
to
0dbcd4d
Compare
| }; | ||
|
|
||
| function getKey(req: Request, res: Response): string { | ||
| try { |
There was a problem hiding this comment.
Clarification question: for api usage the key will be the account and for web it’ll be the user?
There was a problem hiding this comment.
yes with a fallback to IP for non-authenticated endpoints
TBonnin
force-pushed
the
tbonnin/NAN-359/rate-limiting
branch
from
0dbcd4d
to
733f8fc
Compare
| points: 1200, | ||
| duration: 60, |
There was a problem hiding this comment.
more actionable comment: can you make them env var so we don't need to deploy in case of urgent need?
There was a problem hiding this comment.
sure. I will do that when I actually enable 429s. Now it is just logging
| })(); | ||
|
|
||
| export const rateLimiterMiddleware = (req: Request, res: Response, next: NextFunction) => { | ||
| const setXRateLimitHeaders = (rateLimiterRes: RateLimiterRes) => { |
There was a problem hiding this comment.
Can we have a env var to entirely enable/disable ratelimit?
There was a problem hiding this comment.
will also do that when actually returning 429s
| const pointsToConsume = getPointsToConsume(req); | ||
| rateLimiter | ||
| .consume(key, pointsToConsume) | ||
| .then((rateLimiterRes) => { |
There was a problem hiding this comment.
any reason for not using try/catch await?
There was a problem hiding this comment.
no other reason than using the same style as the rate-limit-flexible documentation. Also found it easier to read
| if (req.user) { | ||
| return `user-${req.user.id}`; | ||
| } | ||
| return `ip-${req.ip}`; |
There was a problem hiding this comment.
I wonder if proxy are handled with this. I remember that old version of express would not display the right IP if you had proxy (always happen in cloud env)
There was a problem hiding this comment.
I am not 100% sure. That's the main reason why I prefer to first just log and not return 429 so I can check with real requests
## Describe your changes Enabling rate-limiting (was only logging so far). Since I pushed the memoization of getConnection yesterday, no rate limiting has been logged so it should not impact anybody. followup of #1708
Describe your changes
Add rate-limiting capabilities to server
Issue ticket number and link
https://linear.app/nango/issue/NAN-359/implement-rate-limiting
Checklist before requesting a review (skip if just adding/editing APIs & templates)