Skip to content

Commit 4f4375e

Browse files
brkalowbrkalow
authored
chore(backend): Fix JWKS cache (clerk#3321)
* chore(backend): Fix JWKS cache * chore(backend): Finish implementation * chore(backend): Fix keys tests * fix(backend): Add test to assert the cache doesn't get wiped out * fix(backend): Simplify test * chore(backend): Remove unnecessary comment * chore(backend): Add @deprecated comment * chore(backend): Remove unnecessary error * fix(backend): Ensure fetch if kid isn't found in cache * chore(backend): Re-add exported errors to avoid a breaking change * Update integration/tests/handshake.test.ts
1 parent 7c6146e commit 4f4375e

4 files changed

Lines changed: 67 additions & 27 deletions

File tree

‎.changeset/chatty-cooks-notice.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@clerk/backend': patch
3+
---
4+
5+
Fix bug in JWKS cache logic that caused a race condition resulting in no JWK being available.

‎packages/backend/src/errors.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ export const TokenVerificationErrorReason = {
2020
RemoteJWKInvalid: 'jwk-remote-invalid',
2121
RemoteJWKMissing: 'jwk-remote-missing',
2222
JWKFailedToResolve: 'jwk-failed-to-resolve',
23+
JWKKidMismatch: 'jwk-kid-mismatch',
2324
};
2425

2526
export type TokenVerificationErrorReason =

‎packages/backend/src/tokens/__tests__/keys.test.ts‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,9 @@ export default (QUnit: QUnit) => {
172172
} catch (err) {
173173
if (err instanceof Error) {
174174
assert.propEqual(err, {
175-
reason: 'jwk-remote-missing',
176-
action: 'Contact support@clerk.com',
175+
reason: 'jwk-kid-mismatch',
176+
action:
177+
'Go to your Dashboard and validate your secret and public keys are correct. Contact support@clerk.com if the issue persists.',
177178
});
178179
assert.propContains(err, {
179180
message: `Unable to find a signing key in JWKS that matches the kid='${kid}' of the provided session token. Please make sure that the __session cookie or the HTTP authorization header contain a Clerk-generated session JWT. The following kid is available: ${mockRsaJwkKid}, local`,
@@ -184,5 +185,38 @@ export default (QUnit: QUnit) => {
184185
}
185186
}
186187
});
188+
189+
test('cache TTLs do not conflict', async assert => {
190+
fakeClock.runAll();
191+
192+
fakeFetch.onCall(0).returns(jsonOk(mockJwks));
193+
let jwk = await loadClerkJWKFromRemote({
194+
secretKey: 'deadbeef',
195+
kid: mockRsaJwkKid,
196+
skipJwksCache: true,
197+
});
198+
assert.propEqual(jwk, mockRsaJwk);
199+
200+
// just less than an hour, the cache TTL
201+
fakeClock.tick(60 * 60 * 1000 - 5);
202+
203+
// re-fetch, 5m cache is expired
204+
fakeFetch.onCall(1).returns(jsonOk(mockJwks));
205+
jwk = await loadClerkJWKFromRemote({
206+
secretKey: 'deadbeef',
207+
kid: mockRsaJwkKid,
208+
});
209+
assert.propEqual(jwk, mockRsaJwk);
210+
211+
// cache should be cleared, but 5m ttl is still valid
212+
fakeClock.next();
213+
214+
// re-fetch, 5m cache is expired
215+
jwk = await loadClerkJWKFromRemote({
216+
secretKey: 'deadbeef',
217+
kid: mockRsaJwkKid,
218+
});
219+
assert.propEqual(jwk, mockRsaJwk);
220+
});
187221
});
188222
};

‎packages/backend/src/tokens/keys.ts‎

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { API_URL, API_VERSION, JWKS_CACHE_TTL_MS, MAX_CACHE_LAST_UPDATED_AT_SECONDS } from '../constants';
1+
import { API_URL, API_VERSION, MAX_CACHE_LAST_UPDATED_AT_SECONDS } from '../constants';
22
import {
33
TokenVerificationError,
44
TokenVerificationErrorAction,
@@ -26,19 +26,9 @@ function getCacheValues() {
2626
return Object.values(cache);
2727
}
2828

29-
function setInCache(jwk: JsonWebKeyWithKid, jwksCacheTtlInMs: number) {
29+
function setInCache(jwk: JsonWebKeyWithKid, shouldExpire = true) {
3030
cache[jwk.kid] = jwk;
31-
lastUpdatedAt = Date.now();
32-
33-
if (jwksCacheTtlInMs >= 0) {
34-
setTimeout(() => {
35-
if (jwk) {
36-
delete cache[jwk.kid];
37-
} else {
38-
cache = {};
39-
}
40-
}, jwksCacheTtlInMs);
41-
}
31+
lastUpdatedAt = shouldExpire ? Date.now() : -1;
4232
}
4333

4434
const LocalJwkKid = 'local';
@@ -83,7 +73,7 @@ export function loadClerkJWKFromLocal(localKey?: string): JsonWebKey {
8373
n: modulus,
8474
e: 'AQAB',
8575
},
86-
-1, // local key never expires in cache
76+
false, // local key never expires in cache
8777
);
8878
}
8979

@@ -92,6 +82,9 @@ export function loadClerkJWKFromLocal(localKey?: string): JsonWebKey {
9282

9383
export type LoadClerkJWKFromRemoteOptions = {
9484
kid: string;
85+
/**
86+
* @deprecated This cache TTL is deprecated and will be removed in the next major version. Specifying a cache TTL is now a no-op.
87+
*/
9588
jwksCacheTtlInMs?: number;
9689
skipJwksCache?: boolean;
9790
secretKey?: string;
@@ -108,19 +101,16 @@ export type LoadClerkJWKFromRemoteOptions = {
108101
* @param {Object} options
109102
* @param {string} options.kid - The id of the key that the JWT was signed with
110103
* @param {string} options.alg - The algorithm of the JWT
111-
* @param {number} options.jwksCacheTtlInMs - The TTL of the jwks cache (defaults to 1 hour)
112104
* @returns {JsonWebKey} key
113105
*/
114106
export async function loadClerkJWKFromRemote({
115107
secretKey,
116108
apiUrl = API_URL,
117109
apiVersion = API_VERSION,
118110
kid,
119-
jwksCacheTtlInMs = JWKS_CACHE_TTL_MS,
120111
skipJwksCache,
121112
}: LoadClerkJWKFromRemoteOptions): Promise<JsonWebKey> {
122-
const needsFetch = !getFromCache(kid) || cacheHasExpired();
123-
if (skipJwksCache || needsFetch) {
113+
if (skipJwksCache || cacheHasExpired() || !getFromCache(kid)) {
124114
if (!secretKey) {
125115
throw new TokenVerificationError({
126116
action: TokenVerificationErrorAction.ContactSupport,
@@ -139,7 +129,7 @@ export async function loadClerkJWKFromRemote({
139129
});
140130
}
141131

142-
keys.forEach(key => setInCache(key, jwksCacheTtlInMs));
132+
keys.forEach(key => setInCache(key));
143133
}
144134

145135
const jwk = getFromCache(kid);
@@ -152,11 +142,9 @@ export async function loadClerkJWKFromRemote({
152142
.join(', ');
153143

154144
throw new TokenVerificationError({
155-
action: TokenVerificationErrorAction.ContactSupport,
156-
message: `Unable to find a signing key in JWKS that matches the kid='${kid}' of the provided session token. Please make sure that the __session cookie or the HTTP authorization header contain a Clerk-generated session JWT.${
157-
jwkKeys ? ` The following kid is available: ${jwkKeys}` : ''
158-
}`,
159-
reason: TokenVerificationErrorReason.RemoteJWKMissing,
145+
action: `Go to your Dashboard and validate your secret and public keys are correct. ${TokenVerificationErrorAction.ContactSupport} if the issue persists.`,
146+
message: `Unable to find a signing key in JWKS that matches the kid='${kid}' of the provided session token. Please make sure that the __session cookie or the HTTP authorization header contain a Clerk-generated session JWT. The following kid is available: ${jwkKeys}`,
147+
reason: TokenVerificationErrorReason.JWKKidMismatch,
160148
});
161149
}
162150

@@ -208,7 +196,19 @@ async function fetchJWKSFromBAPI(apiUrl: string, key: string, apiVersion: string
208196
}
209197

210198
function cacheHasExpired() {
211-
return Date.now() - lastUpdatedAt >= MAX_CACHE_LAST_UPDATED_AT_SECONDS * 1000;
199+
// If lastUpdatedAt is -1, it means that we're using a local JWKS and it never expires
200+
if (lastUpdatedAt === -1) {
201+
return false;
202+
}
203+
204+
// If the cache has expired, clear the value so we don't attempt to make decisions based on stale data
205+
const isExpired = Date.now() - lastUpdatedAt >= MAX_CACHE_LAST_UPDATED_AT_SECONDS * 1000;
206+
207+
if (isExpired) {
208+
cache = {};
209+
}
210+
211+
return isExpired;
212212
}
213213

214214
type ErrorFields = {

0 commit comments

Comments
 (0)