Created rate limit middleware #113
Conversation
There was a problem hiding this comment.
The WIP is looking great
Would this be better implemented else where (e.g. whoever hosts this might have their own rate limiting capabilities)?
I agree that it would be good to have some infrastructure handle this rather than a middleware, but I'm not yet sure about the setup of that
I think the last two questions would be better answered by someone who knows expected volumes of the app
| @@ -42,6 +42,7 @@ | |||
| "bcrypt": "^4.0.1", | |||
| "class-transformer": "^0.2.3", | |||
| "class-validator": "^0.11.1", | |||
| "express-rate-limit": "^5.1.1", | |||
There was a problem hiding this comment.
This module, by default uses an in-memory store for tracking the rate limits - this means that if the environment running the application has multiple processes running, the limits can be bypassed to a certain extent. I'm not yet familiar enough to comment whether or not that would be a large issue for the project, my guess is that it wouldn't be a big deal. Alternatively it could be passed an external postgres store to mitigate that, as the state would be shared across processes/instances
There was a problem hiding this comment.
Thanks a lot for the PR!
- Would this be better implemented else where (e.g. whoever hosts this might have their own rate limiting capabilities)?
Probably yes! But I do not think it harms for the moment to have the possibility on each instance directly.
We could default "off" the rate limiting by env variables.
- I've arbitrarily set it to default to 100 requests every 15 minutes and for auth 10 every 30 minutes. What actually would be the ideal config here?
I would put it a bit higher, such as 50 for the auth and 1000 requests every 15 minutes for the rest.
Also here, maybe changeable per environment variables?
- Finally, would this be better set as an environment variable or in some config somewhere?
Would make sense to me, this is something we might want to adjust. Could you check if there is a nice way to integrate it with the existing configuration module?
JackVarney
force-pushed
the
feature/rate-limiting
branch
from
c5712c6 to
65f54ae
Compare
|
@fanick-ber Is this more inline with what you was thinking? |
There was a problem hiding this comment.
Yes, exactly.
I think the configuration has be to ConfigurationService from the ConfigurationModule
I added the middleware to the app module in this way:
export class AppModule implements NestModule {
configure(consumer: MiddlewareConsumer) {
consumer.apply(DefaultRateLimitMiddleware).forRoutes('*');
}
}
It gets called on every call, but I do not get an error when going above the limits. Any idea what I am missing?
JackVarney
force-pushed
the
feature/rate-limiting
branch
from
65f54ae to
fe5db65
Compare
|
I think it was because I was creating a new instance of rate limit each time lol. Its using the same version everytime now if you'd like to give it another go. |
|
@fanick-ber |
|
Thanks @JackVarney , I will test it tomorrow! :-) |
| export class DefaultRateLimitMiddleware implements NestMiddleware { | ||
| private configuredMiddleware: RequestHandler; | ||
|
|
||
| constructor(private configService: ConfigService) { |
There was a problem hiding this comment.
It was failed for me, if I did not use the custom ConfigurationService instead of the nestjs configservice.
| export class AuthRateLimitMiddleware implements NestMiddleware { | ||
| private configuredMiddleware: RequestHandler; | ||
|
|
||
| constructor(private configService: ConfigService) { |
| private configuredMiddleware: RequestHandler; | ||
|
|
||
| constructor(private configService: ConfigService) { | ||
| const defaultWindowMs = this.configService.get( |
There was a problem hiding this comment.
The variable is named a bit weird, Min instead of Ms would be correct. (Same for the Auth limiter below)
GitHub issue
WIP...
Wanted to get feedback on a couple of issues