Race condition in Redis backend / design issue with the Backend type

[mbg]

Initially reported for wai-rate-limit-postgres by @chshersh and then @donatello for wai-rate-limit-redis over in donatello/wai-rate-limit-postgres#7 (comment). The problem for wai-rate-limit-redis specifically is that EXPIRE will not create the key if it doesn't already exist, which might be the case if it has expired between the calls to backendIncAndGetUsage and backendExpireIn.

More generally, the problem extends to other backends, such as wai-rate-limit-postgres, since there is no way to run the two transactionally.

[qwbarch]

Is this library safe to use while this race condition exists?
I love the design of this library! Just want to make sure it's good for production use

[mbg]

Hi @qwbarch, sorry for the slight delay in getting back to you -- I somehow missed the notification for your reply here -- and sorry for not fixing this bug yet.

To answer your question, it somewhat depends on which rate limiting strategy you plan to use and how much you care about strict enforcement of the policies. Generally speaking, I'd think that the library is safe to use until this is fixed.

For the fixed window strategy, the race condition doesn't matter, because it will only call backendIncAndGetUsage once initially.

For the sliding window strategy, the race conditions comes into play if:

• The key happens to expire exactly between the calls to backendIncAndGetUsage and backendExpireIn.
• If that happens, the key expires and the request count is therefore effectively reset to 0. This effectively turns the sliding window strategy into a fixed window strategy if an attacker managed to get lucky and cause this to happen at the end of each window.
• The particular request during which this happens will fail since trying to change the expiry time of a non-existent key will cause an exception.

In short, it is unlikely to be exploitable by a malicious user and in the worst case would still rate limit calls within a given window.

[qwbarch]

Hi @qwbarch, sorry for the slight delay in getting back to you -- I somehow missed the notification for your reply here -- and sorry for not fixing this bug yet.

To answer your question, it somewhat depends on which rate limiting strategy you plan to use and how much you care about strict enforcement of the policies. Generally speaking, I'd think that the library is safe to use until this is fixed.

For the fixed window strategy, the race condition doesn't matter, because it will only call backendIncAndGetUsage once initially.

For the sliding window strategy, the race conditions comes into play if:

• The key happens to expire exactly between the calls to backendIncAndGetUsage and backendExpireIn.
• If that happens, the key expires and the request count is therefore effectively reset to 0. This effectively turns the sliding window strategy into a fixed window strategy if an attacker managed to get lucky and cause this to happen at the end of each window.
• The particular request during which this happens will fail since trying to change the expiry time of a non-existent key will cause an exception.

In short, it is unlikely to be exploitable by a malicious user and in the worst case would still rate limit calls within a given window.

Doesn't seem too bad to stick with the sliding window strategy then. Thanks a lot for the detailed reply!