Fix #1051: Replace lru_cache with TTL-based key caching - #1070
ArvinAlizadehGitHub wants to merge 3 commits into
Conversation
4eaa549 to
11b357a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #1051 by replacing the permanent lru_cache with a TTL-aware cache for individual keys, ensuring that revoked keys are no longer served indefinitely.
- Replaces lru_cache with a TTL-based caching mechanism in jwt/jwks_client.py.
- Adds tests in tests/test_jwks_client.py to verify key expiration and proper cache eviction.
- Updates the CHANGELOG to document the change.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_jwks_client.py | Adds new tests verifying that expired keys are rejected and cache eviction works correctly. |
| jwt/jwks_client.py | Refactors caching logic by adding TTL-based caching methods and removing lru_cache. |
| CHANGELOG.rst | Updates changelog to capture the new TTL-aware key caching fix. |
Comments suppressed due to low confidence (1)
CHANGELOG.rst:12
- The issue reference in the changelog appears to be inconsistent with the PR title (#1051). Please update the issue reference to #1051 if that is the correct identifier.
- Fix indefinite key caching in PyJWKClient by replacing lru_cache with TTL-aware cache in `#1070 <https://github.com/jpadilla/pyjwt/pull/1070>`__
|
Hi @auvipy (or @jpadilla) My changes only touch Could you advise if I should:
The actual functionality tests are all passing - it's just the linting that's catching this pre-existing issue. Would appreciate any feedback on next steps to move this forward. Happy to make any adjustments needed! |
auvipy
left a comment
There was a problem hiding this comment.
please fix the merge conflicts
|
This is PR addresses a very important problem, looking forward to it's review/merge/release. |
|
there are merge conflicts to fix |
|
@auvipy Let's close this so someone else might consider making a PR to fix the issue. |
|
@thomasuster @auvipy sorry ill take a look and get to fixing the errors |
sure please |
a00aaff to
b55a383
Compare
|
@thomasuster @auvipy should be updated to resolve conflicts and staleness, pls have a look / run workflows |
5808196 to
465fe2b
Compare
for more information, see https://pre-commit.ci
|
@auvipy @thomasuster pls have a look and rerun the workflows thanks |
|
@auvipy Are we good to go? |
|
Hi @auvipy , any update on this PR? Thanks! |
|
@jpadilla Are we good to go on this one? |
|
lets do another round of review |
There was a problem hiding this comment.
🟡 Changes recommended
Cache invalidation, thread safety, capacity edge cases, and backward-compatible LRU behavior remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Balanced
| if not self._key_cache_enabled or kid not in self._key_cache: | ||
| return None | ||
|
|
||
| key, timestamp = self._key_cache[kid] |
| cached_key = self._get_cached_key(kid) | ||
| if cached_key is not None: | ||
| return cached_key |
| str, tuple[PyJWK, float] | ||
| ] = {} # kid -> (key, timestamp) | ||
| self._max_cached_keys = max_cached_keys | ||
| self._key_cache_ttl = lifespan # Use same TTL as JWKSetCache |
| # Evict oldest if at capacity | ||
| if len(self._key_cache) >= self._max_cached_keys and kid not in self._key_cache: | ||
| # Simple eviction: remove oldest timestamp | ||
| oldest_kid = min( | ||
| self._key_cache.keys(), key=lambda k: self._key_cache[k][1] | ||
| ) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
This PR fixes issue #1051 where PyJWKClient with
cache_keys=Trueserves potentially revoked keys indefinitely.Problem
When
cache_keys=Trueis enabled, PyJWKClient applies@lru_cacheto theget_signing_keymethod. This caches keys permanently until LRU eviction or process restart. If an identity provider removes a key from their JWKS, applications continue accepting tokens signed with that key.Solution
lru_cachewith TTL-aware cachinglifespan(default 300 seconds)Changes
Testing
All existing tests pass. New test verifies that:
Backward Compatibility
No breaking changes. All existing parameters work identically:
cache_keys=Truestill enables individual key cachingmax_cached_keysstill limits cache sizeFixes #1051