artisansdk / ratelimiter Goto Github PK

Hey, we really enjoy using your library for ratelimiting in Larvel. However a small issue we encounter is that this package does not have any tagged releases, thus dev-master is required in our composer.json. This is bad practice and it's prefered to use semver instead. That way no backwards incompatible changes should be made when only updating minor versions. How do you feel about this? Would it be possible for this package to adhere to semver standards?

This is not a bug, I think I just I do not understand how to set the limits on custom resolvers.

I created a custom resolver for ratings that uses path + IP as the key. Basically what I need to do is to allow all IPs to only vote once per day per game.
So every IP can vote multiple games, but only once per day. I set my resolver like this but is not working as it should

`<?php
namespace App\Http\RateResolvers;

use ArtisanSdk\RateLimiter\Resolvers\User as Resolver;
use Illuminate\Http\Request;

class RatingResolver extends Resolver {
protected $max = '1';
protected $rate = '0.00001157';
protected $duration = 3600*24;

public function __construct(Request $request)
{
	parent::__construct($request, $this->max, $this->rate, $this->duration);
}

/**
 * Get the resolver key used by the rate limiter for the unique request.
 */
public function key(): string
{
	$prefix = parent::key() . $this->request->ip();

	$route = $this->request->route();

	if ($path = $this->request->path() ) {
		return $prefix.':'.sha1($path);
	}

	if ($name = $route->getName()) {
		return $prefix.':'.sha1($name);
	}

	$class = $route->getAction('uses');
	if (! is_null($class) && is_string($class)) {
		return $prefix.':'.sha1($class);
	}

	return $prefix.':'.sha1($route->uri());
}

}`

Despite the bursting issue being one of the problems you're solving here, is there a way to configure a limit based on requests per minute instead of second, but still using your rolling window for allowing requests again?

Hello!

I'm getting the error
"sha1() expects parameter 1 to be string, int given"
in the Resolvers\User.php file at sha1($user->getAuthIdentifier());

    public function key(): string
    {
        if ($user = $this->resolveUser()) {
            return sha1($user->getAuthIdentifier());
        }

The result of getAuthIdentifier() is the id of the user table for me. Due to the new introduced declare(strict_types=1); at the top and that sha1 doesn't allow an int, it fails :(
According to the documentation the getAuthIdentifier should actually return an email, maybe a wrong configuration from me?

I copied the file and changed it now to the following.

        if ($user = $this->resolveUser()) {
            return sha1((string) $user->getAuthIdentifier());
        }

But I think this should be changed.

I would create a pull request but I've absolutely no idea how to. 😅

This is related to #4 and #5 .

This is a little more difficult for me to figure out this late at night, but it looks like the TTL that's set in Limiter::hit() is far too low. In my simple tests, it's expiring super fast.

Here's ArtisanSdk\RateLimiter\Limiter::hit():

    public function hit(): int
    {
        foreach ($this->buckets as $bucket) {
            $bucket->fill();
            $this->cache->put(
                $bucket->key(),
                $bucket->toArray(),
                ceil($bucket->duration() / 60)
            );
        }

The TTL is set to the number of seconds divided by 60....giving minutes. Again, unless I'm wrong, the TTL should be in seconds.

I'll make a quick PR for this, too, but there may be other minutes/seconds issues floating about. 😳 Or....I could be wrong.... 😬

Hi, this is a question.

The redis official website has a reference implementation Sliding Window Rate Limiting to showcase the use case of sorted set

After comparing the their Sliding Window Rate Limiting and your Leaky Bucket Algorithm implementation I feel they are not much difference although they use different metaphors. The main difference is Leaky Bucket leaks out at a constant rate while the sliding window does not slide at a constant rate but only slide when there is a new request and the time gap is larger than the window size. I feel sliding window seems to have more flexibility in managing bursts of traffic.

And the redis's implementation is rather simple, basically 3 commands for sorted set, zremrangebyscore, zcard, zadd

Do you consider add sliding window to your rate limiter ?

I want to configure a global limit and i want to configure Route limits.

I think my problem is about the understanding of:
https://github.com/artisansdk/ratelimiter#how-multiple-buckets-work

So i have setup a default in the kernel:

protected $middlewareGroups = [
        'web' => [
            'throttle:10,1,30',
            \App\Http\Middleware\EncryptCookies::class,
            \Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse::class,
            \Illuminate\Session\Middleware\StartSession::class,
            \Illuminate\View\Middleware\ShareErrorsFromSession::class,
            \App\Http\Middleware\VerifyCsrfToken::class,
            \Illuminate\Routing\Middleware\SubstituteBindings::class,
        ],

        'api' => [
            //default throttle | allow 600 requests | every second you gain 2 more requests | on exceeding limit, wait for 300 seconds
            //translates to: the full 600 requests are backfilled in 5 minutes you can make up to 1200 requests in 5 minutes
            'throttle:600,2,300',
            // \Laravel\Sanctum\Http\Middleware\EnsureFrontendRequestsAreStateful::class,
            \Illuminate\Routing\Middleware\SubstituteBindings::class,
            
        ],
    ];

And configured:

use Illuminate\Support\Facades\Route;
use ArtisanSdk\RateLimiter\Resolvers\Route as Limiter;

Route::group([
    'prefix' => 'v1',
    'namespace' => 'App\Http\Controllers\Api\V1',
    //'middleware' => ['auth:api'],
    'middleware' => 'throttle:' . Limiter::class . ',5,1,30',
], function () {
    Route::apiResource('tasks', TaskController::class, [
        
    ]);
});

So 5 requests at start, 1/s drain, 30 sec penalty.

The rate limits are enforced correctly 👍
But the returned headers are always the headers, from the global default and not from Route Limit.
That makes it impossible to know for the clients, what limits are on a route.

Response while not limited: (request on /api/v1/tasks)

HTTP/1.1 200 OK
Host: localhost:8400
Date: Fri, 29 Sep 2023 11:32:52 GMT
Connection: close
X-Powered-By: PHP/8.2.10
Content-Type: text/html; charset=UTF-8
Cache-Control: no-cache, private
Date: Fri, 29 Sep 2023 11:32:52 GMT
X-RateLimit-Limit: 600
X-RateLimit-Remaining: 599
Access-Control-Allow-Origin: *

While limited: (request on /api/v1/tasks)

HTTP/1.1 429 Too Many Requests
Host: localhost:8400
Date: Fri, 29 Sep 2023 11:33:46 GMT
Connection: close
X-Powered-By: PHP/8.2.10
X-RateLimit-Limit: 600
X-RateLimit-Remaining: 594
retry-after: 30
x-ratelimit-reset: 1695987256
Cache-Control: no-cache, private
date: Fri, 29 Sep 2023 11:33:46 GMT
Content-Type: text/html; charset=UTF-8
Access-Control-Allow-Origin: *

The retry-after seems to be correct but not Limit and Remaining
So i am unsure if its a bug or i misunderstand the README.
Thanks in advance.

Here expiration time is passed as a float: https://github.com/artisansdk/ratelimiter/blob/master/src/Limiter.php#L167
Laravel does not allow that: https://github.com/illuminate/contracts/blob/master/Cache/Repository.php#L24

This still works with, for example, MySQL (I tested v8.0), due to a less strict input rules, which allow string with decimal inside as an input for INTEGER column, truncating decimal part.

However, Postgres (v12.1) does not allow that, which results in error Invalid text representation: 7 ERROR: invalid input syntax for type integer: "1667989363.0001" when numbers align in a way that creates a decimal part.

I suggest simply rounding max(1, $bucket->duration()) to an integer before passing to put()

Hi. I am a graphic designer. I volunteer to design a logo for open source projects. I can design it for you to use it in the text file. What dou you say?

I'm new to this package, so take what I say with a grain of salt. I could be wrong. 🤓

I installed the latest/greatest today and tried it out a bit in the hopes of getting a better overall understanding of the leaky bucket and how it would look/feel for my particular use case.

Basically, I was finding that whenever I would seem to fill my bucket, it would be empty again...right away. 😳

It took a while of debugging/troubleshooting, which helped me learn a bit more of the ins/outs of the package and the concept in general, but I believe I found a problem.

There appears to be an issue in ArtisanSdk\RateLimiter\Limiter:

    public function timeout(int $duration = 1): void
    {
        if ($this->hasTimeout()) {
            return;
        }

        $this->cache->put(
            $this->getTimeoutKey(),
            (int) $this->lastBucket()->timer() + ($duration * 60),
            $duration
        );
    }

The problem appears to be in the call to put(). The ttl is never multiplied by 60, so it expires long before the timeout would expire. I believe $duration should be multiplied by 60 for the ttl as well.

I'll try to do a quick PR and relate it to this issue. I'll test it locally first, of course. 🤓

Please let me know if you have any questions.

Installed into laravel 9, used this class in one route group, where I used these middlewares. After the setup, I ran my own node js instance to spam my website, then at one moment 2 requests passed through in 1 second eventhough there is a limit of 1 request per 10 seconds like so :

use ArtisanSdk\RateLimiter\Resolvers\User;
Route::group(['middleware' => ['auth:sanctum','throttle:'.User::class.',1,0.1,10']], function () {
   // my routes
});

and the strange behaviour started... Now every user and every route is getting rate limited for no reason... eventhough only 1 user (me) was spamming.
Not even command : php artisan cache:clear is working