-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
Consider applying inMemoryBlockOnConsumed to RateLimiter.get(key) as well as consume(). #265
Comments
I guess an alternative could be to have an additional call, like isBlockedInMemoryForConsume(key) which could be checked/called in the example/use case before the more expensive (more DDoS risky) database requesting get() calls. Might that be better? |
Still thinking about this issue, I had another thought which I thought I would bounce here for the possibility of feedback. Maybe the login point protection shouldn't have the get(key) manual checks at the start, and just do the consume from the outset with inMemoryBlockOnConsumed set, and after a successful logins it would then delete the keys from all limiters instead of just the one for limiterConsecutiveFailsByUsernameAndIP. It looking at the source code, it does look like delete clears the key for the in memory storage created by inMemoryBlockOnConsumed. Is there a reason the example is written as is though that would make this hypothetical rewrite into a drawback I don't see atm? |
@jeremykentbgross Hi, thank you for sharing your thoughts.
It should be larger than points. Otherwise a limiter wouldn't count points correctly. I added this to docs. Thanks.
Good catch. Yes, the example could be rewritten to make two |
Closing this one for now. Feel free to re-open if you have more questions. |
Thanks for your feedback. I didn't see the reply from last week at the time, as I only seem to have gotten the notifications in my inbox when the ticket was closed. I think I understand what you are saying though. If I understand correctly, inMemoryBlockOnConsumed isn't useful in this use case because the read only checks at the beginning always need to check the DB (which is way way cheaper than consume because it lacks upsert), and given that check, it will fail before any inMemoryBlockOnConsumed protection would kick in during the consume call anyway, and thus be of no additional help here. Is my understanding correct? |
The example given for login protection at https://github.com/animir/node-rate-limiter-flexible/wiki/Overall-example#login-endpoint-protection does seem to require using the get(key) calls at the start, as it should probably not consume on the limiters until the the login attempt has failed (and users existence has been verified for user/ip combo).
However this means that using inMemoryBlockOnConsumed does not actually help reduce database traffic due to the need for the RateLimiter.get(key) call, which doesn't check the in memory version like consume does.
=>
What is worse, it causes the consumedPoints checks that depend on the get(key) call at the start (prior to trying to login the user) to never trigger properly (unless inMemoryBlockOnConsumed is larger than points) because it blocks on consume in memory only and never writes the failing point deduction to the database; but the database is what is being checked with the get(key) call that can then therefore never fail.
Maybe there is a way to refactor this example to make use of the inMemoryBlockOnConsumed optimization/protection, but at present I don't see a nice way to do that. Additionally since even trying to use the inMemoryBlockOnConsumed with the example seems to break the use case, it seems that the get(key) should probably use inMemoryBlockOnConsumed in addition to consume.
I assume that there is a reason inMemoryBlockOnConsumed is presently ONLY used in consume(), but it seems like if there was another place that it makes sense to use/check it, it would be for local read only (ie the get(key) call).
The text was updated successfully, but these errors were encountered: