-
Notifications
You must be signed in to change notification settings - Fork 3
fix: cache JWKS instance across verifyAccessToken calls #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
3babc5f
0b3e8ec
57e2138
40b4dca
71ba2b2
4cce369
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -819,6 +819,139 @@ describe('session', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('JWKS caching', () => { | ||
| const createLoaderArgs = (request: Request): LoaderFunctionArgs => | ||
| ({ | ||
| request, | ||
| params: {}, | ||
| context: {}, | ||
| }) as LoaderFunctionArgs; | ||
|
|
||
| const mockSessionData = { | ||
| accessToken: 'valid.jwt.token', | ||
| refreshToken: 'refresh.token', | ||
| user: { | ||
| id: 'user-1', | ||
| email: 'test@example.com', | ||
| }, | ||
| impersonator: null, | ||
| }; | ||
|
|
||
| type IsolatedModules = { | ||
| authkitLoader: typeof authkitLoader; | ||
| createRemoteJWKSet: jest.Mock; | ||
| jwtVerify: jest.Mock; | ||
| getJwksUrl: jest.Mock; | ||
| }; | ||
|
|
||
| // Each test gets its own freshly-loaded copy of ./session.js so the | ||
| // module-level JWKS cache never leaks across tests (or out of this | ||
| // describe block). This guards against subtle ordering bugs where a later | ||
| // test would depend on cache state set up here. | ||
| function loadIsolated(): IsolatedModules { | ||
| let isolated!: IsolatedModules; | ||
| jest.isolateModules(() => { | ||
| /* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the code be written such that these disables are not necessary?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 71ba2b2 — swapped the bare |
||
| const joseModule = require('jose') as typeof import('jose'); | ||
| const workosModule = require('./workos.js') as typeof import('./workos.js'); | ||
| const sessionStorageModule = require('./sessionStorage.js') as typeof import('./sessionStorage.js'); | ||
| const ironSessionModule = require('iron-session') as typeof import('iron-session'); | ||
| const sessionModule = require('./session.js') as typeof import('./session.js'); | ||
| /* eslint-enable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */ | ||
|
|
||
| const wos = workosModule.getWorkOS(); | ||
| const getJwksUrlMock = wos.userManagement.getJwksUrl as jest.Mock; | ||
| const createRemoteJWKSetMock = joseModule.createRemoteJWKSet as jest.Mock; | ||
| const jwtVerifyMock = joseModule.jwtVerify as jest.Mock; | ||
| const decodeJwtMock = joseModule.decodeJwt as jest.Mock; | ||
| const getSessionStorageMock = sessionStorageModule.getSessionStorage as jest.Mock; | ||
| const unsealDataMock = ironSessionModule.unsealData as jest.Mock; | ||
|
|
||
| const isolatedGetSession = jest.fn().mockResolvedValue( | ||
| createMockSession({ | ||
| has: jest.fn().mockReturnValue(true), | ||
| get: jest.fn().mockReturnValue('encrypted-jwt'), | ||
| set: jest.fn(), | ||
| }), | ||
| ); | ||
| getSessionStorageMock.mockResolvedValue({ | ||
| cookieName: 'wos-cookie', | ||
| getSession: isolatedGetSession, | ||
| destroySession: jest.fn().mockResolvedValue('destroyed-session-cookie'), | ||
| commitSession: jest.fn(), | ||
| }); | ||
| unsealDataMock.mockResolvedValue({ | ||
| ...mockSessionData, | ||
| headers: { 'Set-Cookie': 'session-cookie' }, | ||
| }); | ||
| getJwksUrlMock.mockImplementation((clientId: string) => `https://auth.workos.com/oauth/jwks/${clientId}`); | ||
| // Real createRemoteJWKSet returns a getKey function used by jwtVerify. | ||
| // The mock needs to return a truthy value so the module-level cache | ||
| // check in session.ts treats it as populated. | ||
| createRemoteJWKSetMock.mockReturnValue(jest.fn()); | ||
| jwtVerifyMock.mockResolvedValue({ | ||
| payload: {}, | ||
| protectedHeader: {}, | ||
| key: new TextEncoder().encode('test-key'), | ||
| }); | ||
| decodeJwtMock.mockReturnValue({ | ||
| sid: 'test-session-id', | ||
| org_id: 'org-123', | ||
| role: 'admin', | ||
| roles: ['admin'], | ||
| permissions: ['read', 'write'], | ||
| entitlements: ['premium'], | ||
| feature_flags: [], | ||
| }); | ||
|
|
||
| isolated = { | ||
| authkitLoader: sessionModule.authkitLoader, | ||
| createRemoteJWKSet: createRemoteJWKSetMock, | ||
| jwtVerify: jwtVerifyMock, | ||
| getJwksUrl: getJwksUrlMock, | ||
| }; | ||
| }); | ||
| return isolated; | ||
| } | ||
|
|
||
| it('reuses the cached JWKS instance across multiple verifyAccessToken calls', async () => { | ||
| const { authkitLoader, createRemoteJWKSet, jwtVerify } = loadIsolated(); | ||
|
|
||
| // Prime the module-scoped cache. | ||
| await authkitLoader(createLoaderArgs(new Request('http://example.com/a', { headers: { Cookie: 'cookie' } }))); | ||
| createRemoteJWKSet.mockClear(); | ||
|
|
||
| await authkitLoader(createLoaderArgs(new Request('http://example.com/b', { headers: { Cookie: 'cookie' } }))); | ||
| await authkitLoader(createLoaderArgs(new Request('http://example.com/c', { headers: { Cookie: 'cookie' } }))); | ||
| await authkitLoader(createLoaderArgs(new Request('http://example.com/d', { headers: { Cookie: 'cookie' } }))); | ||
|
|
||
| expect(jwtVerify).toHaveBeenCalled(); | ||
| expect(createRemoteJWKSet).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('rebuilds the JWKS instance when the JWKS URL changes', async () => { | ||
| const { authkitLoader, createRemoteJWKSet, getJwksUrl } = loadIsolated(); | ||
|
|
||
| // Populate the cache with the default URL. | ||
| await authkitLoader(createLoaderArgs(new Request('http://example.com/a', { headers: { Cookie: 'cookie' } }))); | ||
| createRemoteJWKSet.mockClear(); | ||
|
|
||
| // Same URL → no rebuild. | ||
| await authkitLoader(createLoaderArgs(new Request('http://example.com/b', { headers: { Cookie: 'cookie' } }))); | ||
| expect(createRemoteJWKSet).not.toHaveBeenCalled(); | ||
|
|
||
| // URL changes (e.g. consumer re-configures with a different clientId) → | ||
| // the cache must be invalidated and a new JWKS instance created. | ||
| getJwksUrl.mockImplementation(() => 'https://auth.workos.com/oauth/jwks/other-client'); | ||
| await authkitLoader(createLoaderArgs(new Request('http://example.com/c', { headers: { Cookie: 'cookie' } }))); | ||
| expect(createRemoteJWKSet).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Still the same new URL → still cached. | ||
| await authkitLoader(createLoaderArgs(new Request('http://example.com/d', { headers: { Cookie: 'cookie' } }))); | ||
| expect(createRemoteJWKSet).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe('saveSession', () => { | ||
| const sessionData = { | ||
| accessToken: 'new.valid.token', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -595,8 +595,20 @@ export async function getSessionFromCookie(cookie: string, session?: SessionData | |
| } | ||
| } | ||
|
|
||
| let cachedJWKS: ReturnType<typeof createRemoteJWKSet> | undefined; | ||
| let cachedJWKSUrl: string | undefined; | ||
|
|
||
| function getJWKS(): ReturnType<typeof createRemoteJWKSet> { | ||
| const jwksUrl = getWorkOS().userManagement.getJwksUrl(getConfig('clientId')); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good instinct to check, but getJwksUrl(clientId) {
if (!clientId) {
throw TypeError('clientId must be a valid clientId');
}
return `${this.workos.baseURL}/sso/jwks/${clientId}`;
}So the "fetch once per clientId" goal is already what this cache achieves: same
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, sounds good! Thanks clanker. |
||
| if (!cachedJWKS || cachedJWKSUrl !== jwksUrl) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In src/session.ts:603,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, but Within a single process the failure mode is the trivial one: if two calls genuinely arrive in the same tick before either has populated the cache (only possible if a prior microtask scheduled them both), each would compute True parallelism only shows up across separate workers / isolates, and those don't share module state at all — each gets its own |
||
| cachedJWKS = createRemoteJWKSet(new URL(jwksUrl)); | ||
| cachedJWKSUrl = jwksUrl; | ||
| } | ||
| return cachedJWKS; | ||
| } | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
|
|
||
| async function verifyAccessToken(accessToken: string) { | ||
| const JWKS = createRemoteJWKSet(new URL(getWorkOS().userManagement.getJwksUrl(getConfig('clientId')))); | ||
| const JWKS = getJWKS(); | ||
| try { | ||
| await jwtVerify(accessToken, JWKS); | ||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
|
||
| return true; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're dealing with a cache, and the cache is at the module-level, I think we should sandbox these tests within
jest.isolateModules()to ensure that anything going on here doesn't affect any other tests. It would be an unlikely ordering bug should such an issue occur, but a subtle and annoying one, to be sure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 57e2138 — each test now loads a fresh copy of
./session.jsviajest.isolateModules(...)and re-wires the mocks on the isolatedjose/workos/sessionStorage/iron-session. That also kills the module-scoped JWKS cache at the end of every test, so no state leaks out of thisdescribe.