Skip to content
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

Lua script takes non integer argument as input which causes random error on user supplied input #221

Open
shameersheik opened this issue Sep 8, 2023 · 6 comments · May be fixed by #267
Labels
enhancement New feature or request

Comments

@shameersheik
Copy link

The argument 1 used in the INCRBY statement in the Lua script can be anything and not necessarily an integer.

local consumed = redis.call('incrby', KEYS[1], ARGV[1]) \

This will cause "“ERR value is not an integer or out of range script: , on @user_script:1. ”

The problems :

  1. No one knows this is a lua script excplicity.
  2. Which of the lua script commands causes this.
  3. Based on my understanding we set points to IRateLimiterOptions and this points value is divided by the allowedrequests. If points/allowedrequests gives a decimal number. This error will occur.
  4. This will occur solely based on the value of allowedrequests and will appear as a random issue.

Solution :

  1. Make sure to use a Math.floor option in consume function for pointstoconsume.
    async _upsert(rlKey, points, msDuration, forceExpire = false) {
@animir animir self-assigned this Sep 9, 2023
@animir
Copy link
Owner

animir commented Sep 9, 2023

@shameersheik Hi, this is interesting. Could you help to reproduce this error or provide a code snippet? Could you also provide rate-limiter-flexible version and redis client package name and version you have installed? Thank you.

@shameersheik
Copy link
Author

Yep.

Way to reproduce.

  1. Set your client budget as 10

`const CLIENT_BUDGET = 10; // Here client budget is 10

const RATE_LIMITER_OPTIONS: IRateLimiterOptions = {
duration: RATE_LIMITER_WINDOW,
points: CLIENT_BUDGET,
};

//create your Redis ratelimiter as neccessary
new RedisRateLimiter({
storeClient: redisClient,
...RATE_LIMITER_OPTIONS
});`

  1. Set your points to consume as "3.33" any non integer value to the consume function of RedisRateLimiter.

const pointsToConsume = (clientMaxTPSAllowed: number): number => CLIENT_BUDGET / clientMaxTPSAllowed; // Here the pointsthat are supposed to be consumed is calculated

// if clientMaxTPSAllowed =2 , pointsToConsume is 10/2 = 5 (no problem) , this is an integer
// if clientMaxTPSAllowed =3 , pointsToConsume is 10/3 = 3.33 (Yes problem), this is not an integer

// if we call consume like below here which will call RateLimiterRedis.consume pointsToConsume can be integer or a float.

RedisRateLimiter.consume("somekey",pointsToConsume); // here pointsToConsume can be 5 or 3.33

  1. This consume function will call the lua script with ARGV[1] as a non integer, which will give the error "This will cause "“ERR value is not an integer or out of range script: , on @user_script:1. ”" , So we will have to make sure that ARGV[1] is always an integer. INCRBY only accepts integer 64 bit signed integers. https://redis.io/commands/incrby/

local consumed = redis.call('incrby', KEYS[1], ARGV[1]) \

rate-limiter-flexible version : "^2.3.4"
ioredis : "^4.28.1"

  1. I believe this error can only been caught with reconnectonError Make it possible to log when an error occurs #205. I just connected with my Redis server and hit the below command, got the same error.
    "incrby" "somekey" "3.33"

@shameersheik
Copy link
Author

@animir Sorry Forgot to tag.

@shameersheik
Copy link
Author

Hi any update on this ?

@animir
Copy link
Owner

animir commented Sep 16, 2023

@shameersheik Hi, I looked into your issue.
When we use this package, we set number of points and it is kind of supposed that points are integers not decimal. If you need to consume 3.33 from 10 available it is always possible to consume 333 points from 1000. So the issue is not in decimals.
You have interesting example, as you didn't know you were going to consume decimal and that's the problem. So there should be a way to know, if somebody's code consume decimal number of points. If the package rounds that silently, there may be consequences for some package users who use this package to count money. Yes, it may be unexpected, but there are companies how do that for their pricing plans to limit number of actions per period and account, for example.
Considering that, we can't silently round that number. What we can do is throw an exception with an error, so it would be possible to understand the issue and decide what to do in this case.

In your case, you may round that number of points before points are consumed. Or, as mentioned before, you can increase the number of points in the limiter and consume 333 from 1000.

I hope this helps.

When I or somebody from the community have time to work on your issue, we'll do that. You can also contribute and add that additional check to consume method. The error would say something like Consuming decimal number of points is not supported by this package. That would be helpful in cases like yours.

Note, I also edited some docs on wiki and added integer where it was appropriate.

@animir animir removed their assignment Sep 16, 2023
@animir animir added enhancement New feature or request and removed investigation labels Sep 18, 2023
@shameersheik
Copy link
Author

When I or somebody from the community have time to work on your issue, we'll do that. You can also contribute and add that additional check to consume method. The error would say something like Consuming decimal number of points is not supported by this package. That would be helpful in cases like yours.

Thank you will take a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants