fix: cache confusion risk with composite-key approach#587
Conversation
There was a problem hiding this comment.
Pull request overview
This PR mitigates a security issue where enabling cache with a collision-prone custom cacheKeyBuilder could cause the verifier to return cached claims for a different token (cache confusion / identity mix-up).
Changes:
- Compose custom
cacheKeyBuilderoutput withhashToken(token)to make cache keys collision-safe. - Update existing custom cache-key test expectations to match the new effective key format.
- Add regression tests covering multiple collision-prone
cacheKeyBuilderpatterns from the advisory.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/verifier.js |
Wraps custom cache key builders by appending a token hash to prevent cache confusion via key collisions. |
test/verifier.spec.js |
Updates cache key assertions and adds regression tests for common collision patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
We shouldn't be changing the user supplied mechanisms for building cache keys, including adding some kind of hashing. Many people will provide their own builder for either performance benefits or for domain specific needs, so adding/changing the hash doesn't work. I see two options here:
The former will add overhead to execution time so probably would negate the 'performance benefits' reasoning for people using their own builder, and given the shape of the payload could be anything it would be hard to do this. In this case I would suggest we update the related documentation to highlight the risk involved in using the non-standard key builder. Sometimes the best solutions involve no code at all, and this is probably one of those cases. |
…key approach" This reverts commit e12fe37.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
fix: prevent cache confusion via custom
cacheKeyBuildercollisionsWhen
cacheis enabled with a customcacheKeyBuilderthat produces colliding keys,the verifier could return a cached payload from a different token — leading to identity
confusion, privilege escalation, or cross-tenant data access.
Fix
When a custom
cacheKeyBuilderis provided a process warning is emitted with code FAST_JWT_CACHE_KEY_BUILDER_SECURITY_RISK.This warns users that a poorly implemented key builder can lead to cache collisions and identity/authorization bypass.
The README Caching section already had a [!WARNING] callout.
Tests added
caching - sync - custom cacheKeyBuilder emits security warning— asserts the warning is emitted (with the correct code and message) when a customcacheKeyBuilderis provided alongsidecache: true.caching - sync - default cacheKeyBuilder does not emit security warning— asserts no warning is emitted when the defaulthashTokenbuilder is used.FIXES #588