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

RateLimit is not working as expected #20

Closed
rohitcoder opened this issue Sep 22, 2019 · 8 comments
Closed

RateLimit is not working as expected #20

rohitcoder opened this issue Sep 22, 2019 · 8 comments

Comments

@rohitcoder
Copy link

Hi @touhonoob ,
Great work! I am just having a problem while setting this up. I am using this configuration and soemtime it's working and sometimes it not.

$limit_adapter = new PredisAdapter((new \Predis\Client('localhost')));
$rateLimit = new RateLimit("limit2", 100, 360, $limit_adapter); 
if(!empty($_SESSION['admin_id'])){
	$id = $_SESSION['admin_id']; 
}else{
	$id = $_SERVER['REMOTE_ADDR'];
}  
if($rateLimit->check($id)) {
	// passed
}else{ 
	 header('HTTP/1.1 429 Too many requests', true, 429);
}

When a user is blocked and he keeps reloading that tab. Then sometimes user gets passed and sometimes not passed even if the user is blocked. I don't know why this is happening can you please help me with that?

Thanks,
Rohit Kumar

@DavidGoodwin
Copy link
Contributor

A blocked user will eventually become unblocked if they do nothing.

Is the session expiring?

I'll assume you're aware you need to exit() or similar after the http 429 header to stop any other code from executing in the script.

@rohitcoder
Copy link
Author

Hi @DavidGoodwin ,

Yes, i am doing exit() after header('HTTP/1.1 429 Too many requests', true, 429); }

The session is not expiring (i am echoing $_SESSION['user_id'] in both cases the user is blocked or not blocked and its printing same user-id in both cases, so its working as expected)

A blocked user will eventually become unblocked if they do nothing.

Actually, this works randomly I don't think a user is getting blocked/unblocked in this scenario. It's something else.

Like suddenly i get 429 error (intended behavior) and when i refresh the page within few seconds it gets unblocked and again after 2 or 3 refreshes, it gets blocked again.

Should I post a screen recorded video? So, that you can reproduce it properly.

@DavidGoodwin
Copy link
Contributor

That sounds like expected behaviour to me, if my memory serves me correctly ....

If your rate limiter is setup to allow e.g. 4 requests in a 60 second window, if you refresh / retry e.g. every 15 seconds, I'd expect one request to get through.

https://github.com/touhonoob/RateLimit/blob/master/src/RateLimit.php#L57

@rohitcoder
Copy link
Author

Hi All,

Sorry for late reply, here update on this thread.

I noticed this tool isn't working sometimes, and the user can still send a malicious request and it will be passed without checking block cases.

Have a look at the attached video:

gif

Let me know if you need more information on this.

@rohitcoder
Copy link
Author

@DavidGoodwin @touhonoob Can you please take a look on this issue?

@DavidGoodwin
Copy link
Contributor

Test code -

<?php
require_once('vendor/autoload.php');

$server = [
    'host' => '192.168.0.1',
    'port' => 6379,
    'database' => 15,
];

$adapter = new \PalePurple\RateLimit\Adapter\Predis(new \Predis\Client($server));
$rateLimiter = new \PalePurple\RateLimit\RateLimit('login', 5, 60, $adapter);
$count = $rateLimiter->getAllowance('test');

if ($rateLimiter->check('test')) {
    http_response_code(200);
    echo date('c') . " - OK - {$count} for 5/60\n";
} else {
    http_response_code(429);
    echo date('c') . " - Blocked; {$count} for 5/60.\n";
    exit(0);
}

Running this in a

while true
do
    php test.php
    sleep 1
done

Output is a bit like :

2020-03-03T19:50:14+00:00 - OK - 5 for 5/60
2020-03-03T19:50:15+00:00 - OK - 4 for 5/60
2020-03-03T19:50:16+00:00 - OK - 3 for 5/60
2020-03-03T19:50:17+00:00 - OK - 2 for 5/60
2020-03-03T19:50:18+00:00 - OK - 1 for 5/60
2020-03-03T19:50:19+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:20+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:21+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:23+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:24+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:25+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:26+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:27+00:00 - OK - 1 for 5/60
2020-03-03T19:50:28+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:29+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:30+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:31+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:33+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:34+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:35+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:36+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:37+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:38+00:00 - Blocked; 0 for 5/60.
2020-03-03T19:50:39+00:00 - OK - 1 for 5/60

Which looks correct to me from my hazy memory of how it works....

Namely the bucket fills up, and we then get a 'go' every 5/60 (12 seconds).

@rohitcoder
Copy link
Author

This issue not resolved for me + i also faced this same issue #17 for Race Conditions

@rohitcoder
Copy link
Author

Moved my integration from this project to https://github.com/nikolaposa/rate-limit . I also implemented exponential backoff.

Thanks for your awesome open-source contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants