rate limit cache expiration is not working as expect in CachedKeySet.php

[chris-lee-lb]

This file https://github.com/firebase/php-jwt/blob/main/src/CachedKeySet.php#L216 and this function private function rateLimitExceeded(): bool

1. $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
2. $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.

[bshaffer]

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. isHit is true), it shouldn't overwrite the TTL. I coded up a quick test to prove this (please tell me if I'm missing something):

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 google/auth, and outputs the following:

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: 1

As 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.

[mattsax]

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)
}

[bshaffer]

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....

[mattsax]

It seems like psr6 requires an expiration to be set each time you save an item:

If a calling library asks for an item to be saved but does not specify an expiration time, or specifies a null expiration time or TTL, an Implementing Library MAY use a configured default duration. If no default duration has been set, the Implementing Library MUST interpret that as a request to cache the item forever, or for as long as the underlying implementation supports.

Also, it seems there is no way to get the expiration, I found this discussion which conclusion was to remove a getExpiration() method: https://groups.google.com/g/php-fig/c/vbG1DcchdjI/m/Y3b546PoCwAJ

[bshaffer]

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.

[mihaileu]

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.

[bshaffer]

@mihaileu we are sorry to hear this caused you and your customers difficulty. Please update your library to v6.10.1, and this should now be fixed.