-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Changes from all commits
3babc5f
0b3e8ec
57e2138
40b4dca
71ba2b2
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 |
|---|---|---|
|
|
@@ -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); | ||
|
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.
Rule Used: JWTs should always be validated before use and the... (source)
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. Thanks — this is a real gap but it's pre-existing on
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. Bots, I hear you. But since it's flagged, and since it's security related... it would be more than nice if this validation occurred. 😅
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. Confirmed with Jonah that we're keeping this PR caching-only — none of the other WorkOS SDKs validate Happy to file a cross-SDK tracking issue if useful — just let me know.
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, sure, turn it into an issue.
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. Filed: #69 — scoped it as a cross-SDK tracking issue (since the same gap exists in authkit-nextjs / authkit-remix / authkit-astro and needs a coordinated rollout + a pinned-down |
||
| 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.