rate limit cache expiration is not working as expect in CachedKeySet.php #543
Comments
chris-lee-lb
changed the title
chris-lee-lb
changed the title
|
Hello @chris-lee-lb ! Thank you for filing this issue. I see the problem in the first point, but in the second point, if you retrieve a cache item successfully (e.g. use Google\Auth\Cache\MemoryCacheItemPool;
$cache = new MemoryCacheItemPool();
$rateLimitCacheKey = 'foo123';
// This will never be a hit since this is an in-memory cache
$cacheItem = $cache->getItem($rateLimitCacheKey);
$cacheItem->expiresAfter(60); // # of calls are cached each minute
// simulate what happens at the end of "rateLimitExceeded"
$cacheItem->set(0);
$cache->save($cacheItem);
var_dump($cacheItem); // dump expiration
echo "\nSleep for 5 seconds...\n\n";
sleep(5);
// The claim is that this will overwrite expiresAfter above
$cacheItem = $cache->getItem($rateLimitCacheKey);
$cacheItem->set(1 + (int) $cacheItem->get());
$cache->save($cacheItem);
$cacheItem = $cache->getItem($rateLimitCacheKey);
var_dump($cacheItem); // dump expiration
printf("Calls per minute: %s\n", $cacheItem->get()); // should be "1"This uses the in-memory cache defined in object(Google\Auth\Cache\TypedItem)#2 (4) {
["value":"Google\Auth\Cache\TypedItem":private]=>
int(0)
["expiration":"Google\Auth\Cache\TypedItem":private]=>
object(DateTime)#4 (3) {
["date"]=>
string(26) "2023-12-01 16:59:26.045207"
["timezone_type"]=>
int(3)
["timezone"]=>
string(3) "UTC"
}
["isHit":"Google\Auth\Cache\TypedItem":private]=>
bool(true)
["key":"Google\Auth\Cache\TypedItem":private]=>
string(6) "foo123"
}
Sleep for 5 seconds...
object(Google\Auth\Cache\TypedItem)#2 (4) {
["value":"Google\Auth\Cache\TypedItem":private]=>
int(1)
["expiration":"Google\Auth\Cache\TypedItem":private]=>
object(DateTime)#4 (3) {
["date"]=>
string(26) "2023-12-01 16:59:26.045207"
["timezone_type"]=>
int(3)
["timezone"]=>
string(3) "UTC"
}
["isHit":"Google\Auth\Cache\TypedItem":private]=>
bool(true)
["key":"Google\Auth\Cache\TypedItem":private]=>
string(6) "foo123"
}
Calls per minute: 1As you can see, the cached expiration is the same in both cases, and the cached value is updated as expected. So I'm not quite sure how to replicate the second issue you've reported. The confusion might be that if you're using an in-memory cache, this would not persist between script executions. |
|
Hello @bshaffer I have the same issue using Symfony's Filesystem Cache Adapter. I was abble to reproduce it using your script: use \Symfony\Component\Cache\Adapter\FilesystemAdapter;
$cache = new FilesystemAdapter();
$rateLimitCacheKey = 'foo123';
// This will never be a hit since this is an in-memory cache
$cacheItem = $cache->getItem($rateLimitCacheKey);
$cacheItem->expiresAfter(60); // # of calls are cached each minute
// simulate what happens at the end of "rateLimitExceeded"
$cacheItem->set(0);
$cache->save($cacheItem);
var_dump($cacheItem); // dump expiration
echo "\nSleep for 5 seconds...\n\n";
sleep(5);
// The claim is that this will overwrite expiresAfter above
$cacheItem = $cache->getItem($rateLimitCacheKey);
$cacheItem->set(1 + (int) $cacheItem->get());
$cache->save($cacheItem);
$cacheItem = $cache->getItem($rateLimitCacheKey);
var_dump($cacheItem); // dump expiration
printf("Calls per minute: %s\n", $cacheItem->get()); // should be "1"The expiry is overwritten to unlimited: object(Symfony\Component\Cache\CacheItem)#5 (9) {
["key":protected]=>
string(6) "foo123"
["value":protected]=>
int(0)
["isHit":protected]=>
bool(true)
["expiry":protected]=>
float(1701695286.310535)
["metadata":protected]=>
array(0) {
}
["newMetadata":protected]=>
array(0) {
}
["innerItem":protected]=>
NULL
["poolHash":protected]=>
NULL
["isTaggable":protected]=>
bool(false)
}
Sleep for 5 seconds...
object(Symfony\Component\Cache\CacheItem)#5 (9) {
["key":protected]=>
string(6) "foo123"
["value":protected]=>
int(1)
["isHit":protected]=>
bool(true)
["expiry":protected]=>
NULL
["metadata":protected]=>
array(0) {
}
["newMetadata":protected]=>
array(0) {
}
["innerItem":protected]=>
NULL
["poolHash":protected]=>
NULL
["isTaggable":protected]=>
bool(false)
} |
|
How could this possibly be working as designed for the symfony file cache? Why would a retrieved item be given unlimited expiry by default? I'll need to consult the PSR for this because that doesn't seem right. The result would be forever extending the cache.... |
|
It seems like psr6 requires an expiration to be set each time you save an item:
Also, it seems there is no way to get the expiration, I found this discussion which conclusion was to remove a |
|
To me that seems to be referring to a new item, and not one that has been previously pulled from the cache. It seems odd to me to not be able to persist an expiry of a cache item. But if we really want to get around this, we could just set the expiry as part of the cached value. |
|
I really need the #556 to be merged, I lost hours in debugging this issue that caused downtime for my app as it couldn't refresh the jwks due to rate limit and my clients failed to be marked as logged in. |
This file https://github.com/firebase/php-jwt/blob/main/src/CachedKeySet.php#L216 and this function
private function rateLimitExceeded(): bool$cacheItem->expiresAfter(1); // # of calls are cached each minute, but according to PSR 6 spec (https://www.php-fig.org/psr/psr-6/),public function expiresAfter($time);=>An integer parameter is understood to be the time in seconds$this->cache->save($cacheItem);looks like will overwrite ttl each time. So when ratelimit cache key exists,$cacheItem->expiresAfter()will not be executed, and lifetime will be overwritten to unlimited.I thinks all two parts are not expect behaviors.
The text was updated successfully, but these errors were encountered: