fix(backend): Scope the JWKS cache per Clerk instance - #9394
fix(backend): Scope the JWKS cache per Clerk instance#9394dominic-clerk wants to merge 1 commit into
Conversation
The module-level JWKS cache was keyed on the bare `kid`. Because a Clerk `kid` is the instance id, a key cached for one instance was a direct hit for another instance's verification in the same process, and the lookup short-circuits before `secretKey` is consulted. Since `verifyJwt` never asserts `iss`, a session token minted by instance B authenticated against instance A in any process serving both — the documented Dynamic Keys / multi-tenant pattern. The same cache backs the M2M and OAuth sinks via `resolveKeyAndVerifyJwt`. Remote keys are now cached per `(apiUrl, apiVersion, secretKey)`, each namespace carrying its own TTL, so a cross-instance lookup misses and forces the secret-key-authenticated fetch. Local PEM keys move to their own store, which also stops a local `jwtKey` from disabling the remote TTL process-wide. The `jwk-kid-mismatch` message no longer enumerates cached kids, which disclosed the instance ids warm in a shared process. SDK-148
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 7cc23a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
📝 WalkthroughWalkthroughThe backend now stores local PEM keys separately from remote JWKS keys. Remote JWKS caches are scoped by API URL, API version, and secret key. Each scoped cache has independent expiration. Missing-key errors no longer list cached key IDs. Tests cover cache isolation, independent TTL behavior, and error-message output. Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/src/tokens/__tests__/keys.test.ts`:
- Around line 229-266: Extend the cache-isolation tests in keys.test.ts to cover
apiUrl and apiVersion without using skipJwksCache. For each field, keep
secretKey and kid identical across scopes, configure distinct values, assert the
second request performs an independent fetch, and verify it returns the key from
its own JWKS response.
In `@packages/backend/src/tokens/keys.ts`:
- Around line 34-47: Update getRemoteCache and the related cache-expiration
paths around cacheHasExpired to evict expired remote-cache namespaces from
remoteCaches, not merely clear their keys. Implement a bounded LRU or idle-entry
policy so unused scopes are removed while preserving reuse of active scopes and
existing key-expiration behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 190d1fcf-c228-426b-b194-97a49d8ee5d3
📒 Files selected for processing (3)
.changeset/scope-jwks-cache-per-instance.mdpackages/backend/src/tokens/__tests__/keys.test.tspackages/backend/src/tokens/keys.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/cli(auto-detected)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)
| it('does not serve a cached key to a different secretKey', async () => { | ||
| const instanceAKid = 'ins_tenant_a'; | ||
| let secretKeysUsed: string[] = []; | ||
|
|
||
| server.use( | ||
| http.get( | ||
| 'https://api.clerk.com/v1/jwks', | ||
| validateHeaders(({ request }) => { | ||
| secretKeysUsed.push((request.headers.get('Authorization') ?? '').replace('Bearer ', '')); | ||
| // Each instance's JWKS contains only its own signing key. | ||
| return HttpResponse.json({ keys: [{ ...mockRsaJwk, kid: instanceAKid }] }); | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| // Instance A warms the cache with its own key. | ||
| const jwk = await loadClerkJWKFromRemote({ secretKey: 'sk_test_a', kid: instanceAKid }); | ||
| expect(jwk).toMatchObject({ kid: instanceAKid }); | ||
| expect(secretKeysUsed).toEqual(['sk_test_a']); | ||
|
|
||
| // Instance B asking for instance A's kid must miss the cache and fetch under its | ||
| // own secretKey. | ||
| secretKeysUsed = []; | ||
| server.use( | ||
| http.get( | ||
| 'https://api.clerk.com/v1/jwks', | ||
| validateHeaders(({ request }) => { | ||
| secretKeysUsed.push((request.headers.get('Authorization') ?? '').replace('Bearer ', '')); | ||
| return HttpResponse.json({ keys: [{ ...mockRsaJwk, kid: 'ins_tenant_b' }] }); | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| await expect(() => loadClerkJWKFromRemote({ secretKey: 'sk_test_b', kid: instanceAKid })).rejects.toThrowError( | ||
| TokenVerificationError, | ||
| ); | ||
| expect(secretKeysUsed).toEqual(['sk_test_b']); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/backend/src/tokens/__tests__/keys.test.ts --items all --type function
rg -n -C 6 \
'loadClerkJWKFromRemote|apiUrl|apiVersion|skipJwksCache' \
packages/backend/src/tokens/__tests__/keys.test.tsRepository: clerk/javascript
Length of output: 7239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cache implementation and callers ---'
rg -n -C 12 'getRemoteCache|apiVersion|apiUrl' packages/backend/src/tokens packages/backend/src -g '*.ts' -g '*.tsx' | head -n 320
printf '%s\n' '--- all cache-related tests ---'
rg -n -C 8 'getRemoteCache|apiVersion|apiUrl|separate cache|cache.*instance|secretKey.*kid|kid.*secretKey' packages -g '*.{test,spec}.{ts,tsx,js,jsx}' | head -n 400Repository: clerk/javascript
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant file outline ---'
ast-grep outline packages/backend/src/tokens/keys.ts --items all --type function || true
printf '%s\n' '--- implementation ---'
cat -n packages/backend/src/tokens/keys.ts | sed -n '1,280p'Repository: clerk/javascript
Length of output: 10885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
implementation = Path("packages/backend/src/tokens/keys.ts").read_text()
tests = Path("packages/backend/src/tokens/__tests__/keys.test.ts").read_text()
scope = re.search(r"getRemoteCache\(([^)]*)\)", implementation)
print("scope_expression:", scope.group(1) if scope else "MISSING")
print("scope_fields:", {
field: bool(re.search(rf"\b{field}\b", scope.group(1) if scope else ""))
for field in ("apiUrl", "apiVersion", "secretKey")
})
for field in ("apiUrl", "apiVersion"):
matches = list(re.finditer(rf"\b{field}\s*:", tests))
print(f"{field}_test_occurrences:", len(matches))
for match in matches:
start = max(0, tests.rfind("it(", 0, match.start()))
end = tests.find("\n });", match.start())
block = tests[start:end if end != -1 else match.end()]
print(" has_skipJwksCache:", "skipJwksCache" in block)
PYRepository: clerk/javascript
Length of output: 343
Add cache-isolation tests for apiUrl and apiVersion.
The existing apiUrl test bypasses the cache with skipJwksCache: true, and no apiVersion test exists. For each field, keep secretKey and kid unchanged, then assert that the new scope performs an independent fetch and returns its own key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/tokens/__tests__/keys.test.ts` around lines 229 - 266,
Extend the cache-isolation tests in keys.test.ts to cover apiUrl and apiVersion
without using skipJwksCache. For each field, keep secretKey and kid identical
across scopes, configure distinct values, assert the second request performs an
independent fetch, and verify it returns the key from its own JWKS response.
Source: Coding guidelines
| const remoteCaches = new Map<string, RemoteJwksCache>(); | ||
|
|
||
| function getCacheValues() { | ||
| return Object.values(cache); | ||
| } | ||
| /** Local PEM keys are not tied to a secret key, and never expire. */ | ||
| const localCache: JsonWebKeyCache = {}; | ||
|
|
||
| function setInCache(cacheKey: string, jwk: JsonWebKeyWithKid, shouldExpire = true) { | ||
| cache[cacheKey] = jwk; | ||
| lastUpdatedAt = shouldExpire ? Date.now() : -1; | ||
| /** | ||
| * The scope is held in memory only as a Map key. It is never logged or surfaced in errors. | ||
| */ | ||
| function getRemoteCache(scope: string): RemoteJwksCache { | ||
| let cache = remoteCaches.get(scope); | ||
| if (!cache) { | ||
| cache = { keys: {}, lastUpdatedAt: 0 }; | ||
| remoteCaches.set(scope, cache); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Evict unused remote-cache namespaces.
Every distinct scope remains in remoteCaches for the process lifetime. cacheHasExpired only clears keys after the same scope is used again. It never removes the Map entry. A multi-tenant service can therefore grow remoteCaches without a bound.
Add a bounded LRU policy or idle-entry eviction that removes expired cache namespaces.
Also applies to: 147-147, 235-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/tokens/keys.ts` around lines 34 - 47, Update
getRemoteCache and the related cache-expiration paths around cacheHasExpired to
evict expired remote-cache namespaces from remoteCaches, not merely clear their
keys. Implement a bounded LRU or idle-entry policy so unused scopes are removed
while preserving reuse of active scopes and existing key-expiration behavior.
| // are called with the same kid | ||
| const prefixedKid = `local-${kid}`; | ||
| const cachedJwk = getFromCache(prefixedKid); | ||
| const cachedJwk = localCache[prefixedKid]; |
There was a problem hiding this comment.
[MEDIUM] Local PEM (jwtKey) cache still shared across instances by kid alone
The remote cache is now correctly scoped by apiUrl|apiVersion|secretKey, but loadClerkJwkFromPem still keys localCache on local-${kid} only — global across every instance in the process, with no scoping by the supplied pem and no TTL. On a cache hit it returns the cached JWK and ignores the passed pem. Since verifyJwt never asserts iss and verifyToken prefers options.jwtKey, an app doing networkless multi-tenant verification is exposed: (1) an instance-A token (kid=insA) presented to instance B (jwtKey=pemB) gets a warm hit returning instance A's key and authenticates cross-tenant; (2) an attacker can poison localCache['local-insA'] with pemB, permanently breaking instance-A verification until restart. Fix: scope the localCache key by a hash of the pem/instance (as the remote cache was fixed) and add a TTL.
— Comment generated with Claude with @dominic-clerk's supervision
1 participant
Description
The module-level JWKS cache was keyed on the bare
kid. Because a Clerkkidis the instance id, a key cached for one instance was a direct hit for another instance's verification in the same process, and the lookup short-circuits beforesecretKeyis consulted. SinceverifyJwtnever assertsiss, a session token minted by instance B authenticated against instance A in any process serving both — the documented Dynamic Keys / multi-tenant pattern. The same cache backs the M2M and OAuth sinks viaresolveKeyAndVerifyJwt.Remote keys are now cached per
(apiUrl, apiVersion, secretKey), each namespace carrying its own TTL, so a cross-instance lookup misses and forces the secret-key-authenticated fetch. Local PEM keys move to their own store, which also stops a localjwtKeyfrom disabling the remote TTL process-wide.The
jwk-kid-mismatchmessage no longer enumerates cached kids, which disclosed the instance ids warm in a shared process.Fixes SDK-148
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change