-
-
Notifications
You must be signed in to change notification settings - Fork 438
feat: oauth server side and confidential client #1366
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 2 commits
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,8 @@ import { provider } from 'std-env' | |||||||||||
|
|
||||||||||||
| // Storage key for fetch cache - must match shared/utils/fetch-cache-config.ts | ||||||||||||
| const FETCH_CACHE_STORAGE_BASE = 'fetch-cache' | ||||||||||||
| // Storage key for OAuth cache - must match server/utils/atproto/storage.ts | ||||||||||||
| const OAUTH_CACHE_STORAGE_BASE = 'atproto:oauth' | ||||||||||||
|
|
||||||||||||
| export default defineNuxtModule({ | ||||||||||||
| meta: { | ||||||||||||
|
|
@@ -37,6 +39,11 @@ export default defineNuxtModule({ | |||||||||||
| ...nitroConfig.storage[FETCH_CACHE_STORAGE_BASE], | ||||||||||||
| driver: 'vercel-runtime-cache', | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| nitroConfig.storage[OAUTH_CACHE_STORAGE_BASE] = { | ||||||||||||
| ...nitroConfig.storage[OAUTH_CACHE_STORAGE_BASE], | ||||||||||||
| driver: 'vercel-runtime-cache', | ||||||||||||
| } | ||||||||||||
|
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. I don't think this is needed as we have L50-52
Suggested change
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const env = process.env.VERCEL_ENV | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,6 +37,7 @@ export default defineNuxtConfig({ | |||||||||||||||||||||||||||||||||||||||
| github: { | ||||||||||||||||||||||||||||||||||||||||
| orgToken: '', | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined, | ||||||||||||||||||||||||||||||||||||||||
| // Upstash Redis for distributed OAuth token refresh locking in production | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
35
to
41
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check where each env var name is referenced
rg -n "OAUTH_JWK_ONE|NUXT_OAUTH_JWK_ONE" --type-listRepository: npmx-dev/npmx.dev Length of output: 450 🏁 Script executed: #!/bin/bash
# Search for both env var references across the codebase
rg -n "OAUTH_JWK_ONE|NUXT_OAUTH_JWK_ONE"Repository: npmx-dev/npmx.dev Length of output: 135 🏁 Script executed: #!/bin/bash
# Search for other NUXT_ prefixed env vars in the codebase
rg -n "process\.env\.NUXT_"Repository: npmx-dev/npmx.dev Length of output: 644 🏁 Script executed: #!/bin/bash
# Look for .env files and environment documentation
fd -type f "\.env" -o "\.env\.*" -o "*env*example*" -o "*README*" -o "*ENV*"Repository: npmx-dev/npmx.dev Length of output: 231 🏁 Script executed: #!/bin/bash
# Check the nuxt.config.ts file to see the full context around line 37
cat -n nuxt.config.ts | head -50Repository: npmx-dev/npmx.dev Length of output: 1726 🏁 Script executed: #!/bin/bash
# Search for sessionPassword references
rg -n "sessionPassword|SESSION_PASSWORD"Repository: npmx-dev/npmx.dev Length of output: 1138 🏁 Script executed: #!/bin/bash
# Look for documentation or config files mentioning env vars
fd -type f -name "README*" -o -name "*.md" | head -10Repository: npmx-dev/npmx.dev Length of output: 231 🏁 Script executed: #!/bin/bash
# Check if there's a .env.example or similar
find . -maxdepth 2 -type f \( -name ".env*" -o -name "env.*" \) 2>/dev/nullRepository: npmx-dev/npmx.dev Length of output: 91 🏁 Script executed: #!/bin/bash
# Read the .env.example file
cat .env.exampleRepository: npmx-dev/npmx.dev Length of output: 132 🏁 Script executed: #!/bin/bash
# Search for OAUTH_JWK in documentation or comments
rg -n "OAUTH_JWK" -B 2 -A 2Repository: npmx-dev/npmx.dev Length of output: 345 🏁 Script executed: #!/bin/bash
# Check modules/oauth.ts to understand the OAuth configuration pattern
cat -n modules/oauth.ts | head -50Repository: npmx-dev/npmx.dev Length of output: 1727 🏁 Script executed: #!/bin/bash
# Search for JWKS or jwks references
rg -n "jwks|JWKS|JWK" -iRepository: npmx-dev/npmx.dev Length of output: 3401 🏁 Script executed: #!/bin/bash
# Check git log for context around this change
git log --oneline -20 -- nuxt.config.tsRepository: npmx-dev/npmx.dev Length of output: 96 🏁 Script executed: #!/bin/bash
# Look for any documentation about OAuth setup
fd -type f -name "*.md" | xargs rg -l "oauth|OAUTH" -i 2>/dev/null | head -5Repository: npmx-dev/npmx.dev Length of output: 445 Use standard Nuxt env var naming for consistency. The 💡 Suggested approach (support both names)- oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined,
+ oauthJwkOne: process.env.NUXT_OAUTH_JWK_ONE || process.env.OAUTH_JWK_ONE || undefined,📝 Committable suggestion
Suggested change
Comment on lines
35
to
41
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. 🧩 Analysis chain🏁 Script executed: rg -n "OAUTH_JWK_ONE|NUXT_OAUTH_JWK_ONE"Repository: npmx-dev/npmx.dev Length of output: 135 🏁 Script executed: # Check for environment variable references in deployment or config documentation
fd -e md -e yml -e yaml -e json | head -20 | xargs rg -l "OAUTH_JWK|env var|environment" 2>/dev/null || trueRepository: npmx-dev/npmx.dev Length of output: 140 🏁 Script executed: # Examine other runtimeConfig entries to understand naming pattern
cat -n nuxt.config.ts | head -50Repository: npmx-dev/npmx.dev Length of output: 1726 🏁 Script executed: rg -A 3 -B 3 "OAUTH_JWK|oauth" docs/ README.md CONTRIBUTING.md 2>/dev/null || trueRepository: npmx-dev/npmx.dev Length of output: 43 🏁 Script executed: # Check for deployment or environment setup documentation
rg -i "env.*var|environment.*setup|deployment" docs/ README.md --max-count=5Repository: npmx-dev/npmx.dev Length of output: 43 🏁 Script executed: # Search for any reference to how runtimeConfig env vars should be named
rg -i "nuxt.*env.*prefix|NUXT_|runtimeConfig" README.md docs/ CONTRIBUTING.md --max-count=10Repository: npmx-dev/npmx.dev Length of output: 316 Align the environment variable name with Nuxt 3 conventions. Nuxt 3 automatically prefixes Either allow Nuxt's auto-prefixing to work (remove 🔧 Suggested adjustment- oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined,
+ oauthJwkOne:
+ process.env.NUXT_OAUTH_JWK_ONE ??
+ process.env.OAUTH_JWK_ONE ??
+ undefined,📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
| upstash: { | ||||||||||||||||||||||||||||||||||||||||
| redisRestUrl: process.env.UPSTASH_KV_REST_API_URL || process.env.KV_REST_API_URL || '', | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -122,6 +123,7 @@ export default defineNuxtConfig({ | |||||||||||||||||||||||||||||||||||||||
| '/_avatar/**': { isr: 3600, proxy: 'https://www.gravatar.com/avatar/**' }, | ||||||||||||||||||||||||||||||||||||||||
| '/opensearch.xml': { isr: true }, | ||||||||||||||||||||||||||||||||||||||||
| '/oauth-client-metadata.json': { prerender: true }, | ||||||||||||||||||||||||||||||||||||||||
| '/.well-known/jwks.json': { prerender: true }, | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
125
to
+126
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. 🧩 Analysis chain🏁 Script executed: # First, find and examine the nuxt.config.ts file
find . -name "nuxt.config.ts" -type fRepository: npmx-dev/npmx.dev Length of output: 99 🏁 Script executed: # Search for references to oauthJwkOne to understand how it's used
rg "oauthJwkOne" -A 3 -B 3Repository: npmx-dev/npmx.dev Length of output: 991 🏁 Script executed: # Search for JWKS endpoint implementation
rg "well-known/jwks" -A 5 -B 5Repository: npmx-dev/npmx.dev Length of output: 1504 🏁 Script executed: # Look for runtimeConfig configuration in the project
rg "runtimeConfig" -A 3 -B 1Repository: npmx-dev/npmx.dev Length of output: 470 🏁 Script executed: # Search for the actual JWKS endpoint implementation
rg "jwks\.json" -A 10 -B 2Repository: npmx-dev/npmx.dev Length of output: 1545 🏁 Script executed: # Look for server routes
find . -path "./server/routes/**" -type f | head -20Repository: npmx-dev/npmx.dev Length of output: 352 🏁 Script executed: # Search for any route handling JWKS
rg "\.well-known" -A 10 -B 2Repository: npmx-dev/npmx.dev Length of output: 1998 🏁 Script executed: # Look at the full nuxt.config.ts to understand the routeRules context
sed -n '115,140p' ./nuxt.config.tsRepository: npmx-dev/npmx.dev Length of output: 1334 🏁 Script executed: # Read the actual JWKS endpoint implementation
cat -n ./server/routes/.well-known/jwks.json.get.tsRepository: npmx-dev/npmx.dev Length of output: 449 🏁 Script executed: # Also check the oauth-client-metadata endpoint for comparison
cat -n ./server/routes/oauth-client-metadata.json.get.tsRepository: npmx-dev/npmx.dev Length of output: 413 🏁 Script executed: # Search for how loadJWKs is used in the codebase
rg "loadJWKs" -A 5 -B 2Repository: npmx-dev/npmx.dev Length of output: 1834 🌐 Web query:
💡 Result: What happens when you set
|
||||||||||||||||||||||||||||||||||||||||
| '/oauth-client-metadata.json': { prerender: true }, | |
| '/.well-known/jwks.json': { prerender: true }, | |
| '/oauth-client-metadata.json': { prerender: true }, | |
| '/.well-known/jwks.json': { isr: 60 }, |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { JoseKey } from '@atproto/oauth-client-node' | ||
|
|
||
| async function run() { | ||
| const kid = Date.now().toString() | ||
| const key = await JoseKey.generate(['ES256'], kid) | ||
| const jwk = key.privateJwk | ||
|
|
||
| console.log(JSON.stringify(jwk)) | ||
| } | ||
|
|
||
| await run() |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,19 +1,16 @@ | ||||||||||
| import type { OAuthSession } from '@atproto/oauth-client-node' | ||||||||||
| import { NodeOAuthClient, OAuthCallbackError } from '@atproto/oauth-client-node' | ||||||||||
| import { OAuthCallbackError } from '@atproto/oauth-client-node' | ||||||||||
| import { createError, getQuery, sendRedirect, setCookie, getCookie, deleteCookie } from 'h3' | ||||||||||
| import type { H3Event } from 'h3' | ||||||||||
| import { getOAuthLock } from '#server/utils/atproto/lock' | ||||||||||
| import { useOAuthStorage } from '#server/utils/atproto/storage' | ||||||||||
| import { SLINGSHOT_HOST } from '#shared/utils/constants' | ||||||||||
| import { useServerSession } from '#server/utils/server-session' | ||||||||||
| import { handleResolver } from '#server/utils/atproto/oauth' | ||||||||||
| import { handleApiError } from '#server/utils/error-handler' | ||||||||||
| import type { DidString } from '@atproto/lex' | ||||||||||
| import { Client } from '@atproto/lex' | ||||||||||
| import * as com from '#shared/types/lexicons/com' | ||||||||||
| import * as app from '#shared/types/lexicons/app' | ||||||||||
| import { isAtIdentifierString } from '@atproto/lex' | ||||||||||
| import { scope, getOauthClientMetadata } from '#server/utils/atproto/oauth' | ||||||||||
| import { scope } from '#server/utils/atproto/oauth' | ||||||||||
|
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. Import It is referenced on Line 29 but not imported, so this will throw at runtime. 🔧 Proposed fix-import { scope } from '#server/utils/atproto/oauth'
+import { getNodeOAuthClient, scope } from '#server/utils/atproto/oauth'📝 Committable suggestion
Suggested change
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. Missing import for The function Proposed fix-import { scope } from '#server/utils/atproto/oauth'
+import { scope, getNodeOAuthClient } from '#server/utils/atproto/oauth'📝 Committable suggestion
Suggested change
|
||||||||||
| import { UNSET_NUXT_SESSION_PASSWORD } from '#shared/utils/constants' | ||||||||||
| // @ts-expect-error virtual file from oauth module | ||||||||||
| import { clientUri } from '#oauth/config' | ||||||||||
|
|
@@ -28,17 +25,7 @@ export default defineEventHandler(async event => { | |||||||||
| } | ||||||||||
|
|
||||||||||
| const query = getQuery(event) | ||||||||||
| const clientMetadata = getOauthClientMetadata() | ||||||||||
| const session = await useServerSession(event) | ||||||||||
| const { stateStore, sessionStore } = useOAuthStorage(session) | ||||||||||
|
|
||||||||||
| const atclient = new NodeOAuthClient({ | ||||||||||
| stateStore, | ||||||||||
| sessionStore, | ||||||||||
| clientMetadata, | ||||||||||
| requestLock: getOAuthLock(), | ||||||||||
| handleResolver, | ||||||||||
| }) | ||||||||||
|
|
||||||||||
| if (query.handle) { | ||||||||||
| // Initiate auth flow | ||||||||||
|
|
@@ -66,10 +53,13 @@ export default defineEventHandler(async event => { | |||||||||
| } | ||||||||||
|
|
||||||||||
| try { | ||||||||||
| const redirectUrl = await atclient.authorize(query.handle, { | ||||||||||
| const redirectUrl = await event.context.oauthClient.authorize(query.handle, { | ||||||||||
| scope, | ||||||||||
| prompt: query.create ? 'create' : undefined, | ||||||||||
| ui_locales: query.locale?.toString(), | ||||||||||
| // TODO: I do not beleive this is working as expected on | ||||||||||
| // a unsupported locale on the PDS. Gives Invalid at body.ui_locales | ||||||||||
| // Commenting out for now | ||||||||||
| // ui_locales: query.locale?.toString(), | ||||||||||
| state: encodeOAuthState(event, { redirectPath }), | ||||||||||
| }) | ||||||||||
|
|
||||||||||
|
|
@@ -87,7 +77,7 @@ export default defineEventHandler(async event => { | |||||||||
| // Handle callback | ||||||||||
| try { | ||||||||||
| const params = new URLSearchParams(query as Record<string, string>) | ||||||||||
| const result = await atclient.callback(params) | ||||||||||
| const result = await event.context.oauthClient.callback(params) | ||||||||||
| try { | ||||||||||
| const state = decodeOAuthState(event, result.state) | ||||||||||
| const profile = await getMiniProfile(result.session) | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,23 @@ | ||
| import { PublicUserSessionSchema } from '#shared/schemas/publicUserSession' | ||
| import { safeParse } from 'valibot' | ||
|
|
||
| export default defineEventHandler(async event => { | ||
| const serverSession = await useServerSession(event) | ||
| export default eventHandlerWithOAuthSession(async (event, _, serverSession) => { | ||
| const result = safeParse(PublicUserSessionSchema, serverSession.data.public) | ||
| if (!result.success) { | ||
| return null | ||
| } | ||
|
|
||
| // A one time redirect to upgrade the previous sessions. | ||
| // Can remove in 2 weeks from merge if we'd like | ||
| if (serverSession.data.oauthSession && serverSession.data?.public?.did) { | ||
| await serverSession.update({ | ||
| oauthSession: undefined, | ||
| }) | ||
| return { | ||
| ...result.output, | ||
| relogin: true, | ||
| } | ||
| } | ||
|
Comment on lines
+4
to
+20
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. Time-relative migration comment is likely to go stale; consider a dated TODO/issue reference. The one-time migration behaviour is reasonable, but the comment on Line 11-12 (“remove in 2 weeks from merge”) is hard to track over time. Consider replacing it with a dated TODO (or a tracked issue ID) so it doesn’t linger indefinitely. As per coding guidelines, “Add comments only to explain complex logic or non-obvious implementations”. |
||
|
|
||
| return result.output | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import type { NodeOAuthClient } from '@atproto/oauth-client-node' | ||
|
|
||
| /** | ||
| * Creates a long living instance of the NodeOAuthClient. | ||
| */ | ||
| export default defineNitroPlugin(async nitroApp => { | ||
| const oauthClient = await getNodeOAuthClient() | ||
|
|
||
| // Attach to event context for access in composables via useRequestEvent() | ||
| nitroApp.hooks.hook('request', event => { | ||
| event.context.oauthClient = oauthClient | ||
| }) | ||
| }) | ||
|
|
||
| // Extend the H3EventContext type | ||
| declare module 'h3' { | ||
| interface H3EventContext { | ||
| oauthClient: NodeOAuthClient | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,11 @@ | ||||||||||||||||||
| import { loadJWKs } from '#server/utils/atproto/oauth' | ||||||||||||||||||
|
|
||||||||||||||||||
| export default defineEventHandler(async _ => { | ||||||||||||||||||
| const keys = await loadJWKs() | ||||||||||||||||||
| if (!keys) { | ||||||||||||||||||
| console.error('Failed to load JWKs. May not be set') | ||||||||||||||||||
| return [] | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+5
to
+8
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. Inconsistent JWKS response format on failure. When JWKs fail to load, returning an empty array Proposed fix if (!keys) {
console.error('Failed to load JWKs. May not be set')
- return []
+ return { keys: [] }
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| return keys.publicJwks | ||||||||||||||||||
| }) | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| export default defineEventHandler(() => { | ||
| return getOauthClientMetadata() | ||
| export default defineEventHandler(async _ => { | ||
| const keyset = await loadJWKs() | ||
| const pk = keyset?.findPrivateKey({ usage: 'sign' }) | ||
| return getOauthClientMetadata(pk?.alg) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,39 +1,32 @@ | ||
| import type { NodeSavedSession, NodeSavedSessionStore } from '@atproto/oauth-client-node' | ||
| import type { UserServerSession } from '#shared/types/userSession' | ||
| import type { SessionManager } from 'h3' | ||
| import { OAUTH_CACHE_STORAGE_BASE } from '#server/utils/atproto/storage' | ||
|
|
||
| // Refresh tokens from a confidential client should last for 180 days, each new refresh of access token resets | ||
| // the expiration with the new refresh token. Shorting to 179 days to keep it a bit simpler since we rely on redis to clear sessions | ||
| // Note: This expiration only lasts this long in production. Local dev is 2 weeks | ||
| const SESSION_EXPIRATION = CACHE_MAX_AGE_ONE_DAY * 179 | ||
|
|
||
| export class OAuthSessionStore implements NodeSavedSessionStore { | ||
| private readonly session: SessionManager<UserServerSession> | ||
| private readonly cache: CacheAdapter | ||
|
|
||
| constructor() { | ||
| this.cache = getCacheAdapter(OAUTH_CACHE_STORAGE_BASE) | ||
| } | ||
|
|
||
| constructor(session: SessionManager<UserServerSession>) { | ||
| this.session = session | ||
| private createStorageKey(did: string) { | ||
| return `sessions:${did}` | ||
| } | ||
|
|
||
| async get(): Promise<NodeSavedSession | undefined> { | ||
| const sessionData = this.session.data | ||
| if (!sessionData) return undefined | ||
| return sessionData.oauthSession | ||
| async get(key: string): Promise<NodeSavedSession | undefined> { | ||
| let session = await this.cache.get<NodeSavedSession>(this.createStorageKey(key)) | ||
| return session ?? undefined | ||
| } | ||
|
|
||
| async set(_key: string, val: NodeSavedSession) { | ||
| // We are ignoring the key since the mapping is already done in the session | ||
| try { | ||
| await this.session.update({ | ||
| oauthSession: val, | ||
| }) | ||
| } catch (error) { | ||
| // Not sure if this has been happening. But helps with debugging | ||
| console.error( | ||
| '[oauth session store] Failed to set session:', | ||
| error instanceof Error ? error.message : 'Unknown error', | ||
| ) | ||
| throw error | ||
| } | ||
| async set(key: string, val: NodeSavedSession) { | ||
| await this.cache.set<NodeSavedSession>(this.createStorageKey(key), val, SESSION_EXPIRATION) | ||
| } | ||
|
|
||
| async del() { | ||
| await this.session.update({ | ||
| oauthSession: undefined, | ||
| }) | ||
| async del(key: string) { | ||
| await this.cache.delete(this.createStorageKey(key)) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,30 +1,30 @@ | ||
| import type { NodeSavedState, NodeSavedStateStore } from '@atproto/oauth-client-node' | ||
| import type { UserServerSession } from '#shared/types/userSession' | ||
| import type { SessionManager } from 'h3' | ||
| import { OAUTH_CACHE_STORAGE_BASE } from './storage' | ||
|
|
||
| // It is recommended that oauth state is only saved for 30 minutes | ||
| const STATE_EXPIRATION = CACHE_MAX_AGE_ONE_MINUTE * 30 | ||
|
|
||
| export class OAuthStateStore implements NodeSavedStateStore { | ||
| private readonly session: SessionManager<UserServerSession> | ||
| private readonly cache: CacheAdapter | ||
|
|
||
| constructor() { | ||
| this.cache = getCacheAdapter(OAUTH_CACHE_STORAGE_BASE) | ||
| } | ||
|
|
||
|
Comment on lines
+4
to
+13
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. 🧩 Analysis chain🏁 Script executed: cat -n server/utils/atproto/oauth-state-store.tsRepository: npmx-dev/npmx.dev Length of output: 2150 🌐 Web query:
💡 Result: In Practical implications:
Citations:
Add missing imports and fix concurrent OAuth state handling.
Either reuse the same Example fix (one-time session identifier)+import { CACHE_MAX_AGE_ONE_MINUTE } from '#shared/utils/constants'
+import type { CacheAdapter } from '#server/utils/cache/shared'
+import { getCacheAdapter } from '#server/utils/cache/adapter'
+import { OAUTH_CACHE_STORAGE_BASE } from '#server/utils/atproto/storage'
// It is recommended that oauth state is only saved for 30 minutes
const STATE_EXPIRATION = CACHE_MAX_AGE_ONE_MINUTE * 30
export class OAuthStateStore implements NodeSavedStateStore {
private readonly serverSession: SessionManager<UserServerSession>
private readonly cache: CacheAdapter
constructor(session: SessionManager<UserServerSession>) {
this.serverSession = session
this.cache = getCacheAdapter(OAUTH_CACHE_STORAGE_BASE)
}
private createStorageKey(stateKey: string, browserStateId: string) {
- return `state:${did}:${sessionId}`
+ return `state:${browserStateId}:${stateKey}`
}
async get(key: string): Promise<NodeSavedState | undefined> {
- const serverSessionData = this.serverSession.data
- if (!serverSessionData) return undefined
- if (!serverSessionData.oauthStateId) return undefined
+ const browserStateId = this.serverSession.data?.oauthStateId
+ if (!browserStateId) return undefined
const state = await this.cache.get<NodeSavedState>(
- this.createStorageKey(key, serverSessionData.oauthStateId),
+ this.createStorageKey(key, browserStateId),
)
return state ?? undefined
}
async set(key: string, val: NodeSavedState) {
- let stateId = crypto.randomUUID()
- await this.serverSession.update({
- oauthStateId: stateId,
- })
- await this.cache.set<NodeSavedState>(this.createStorageKey(key, stateId), val, STATE_EXPIRATION)
+ const existing = this.serverSession.data?.oauthStateId
+ const browserStateId = existing ?? crypto.randomUUID()
+ if (!existing) {
+ await this.serverSession.update({ oauthStateId: browserStateId })
+ }
+ await this.cache.set<NodeSavedState>(
+ this.createStorageKey(key, browserStateId),
+ val,
+ STATE_EXPIRATION,
+ )
}
async del(key: string) {
- const serverSessionData = this.serverSession.data
- if (!serverSessionData) return undefined
- if (!serverSessionData.oauthStateId) return undefined
+ const browserStateId = this.serverSession.data?.oauthStateId
+ if (!browserStateId) return
await this.cache.delete(this.createStorageKey(key, serverSessionData.oauthStateId))
- await this.serverSession.update({
- oauthStateId: undefined,
- })
}
} |
||
| constructor(session: SessionManager<UserServerSession>) { | ||
| this.session = session | ||
| private createStorageKey(key: string) { | ||
| return `state:${key}` | ||
| } | ||
|
|
||
| async get(): Promise<NodeSavedState | undefined> { | ||
| const sessionData = this.session.data | ||
| if (!sessionData) return undefined | ||
| return sessionData.oauthState | ||
| async get(key: string): Promise<NodeSavedState | undefined> { | ||
| const state = await this.cache.get<NodeSavedState>(this.createStorageKey(key)) | ||
| return state ?? undefined | ||
| } | ||
|
|
||
| async set(_key: string, val: NodeSavedState) { | ||
| // We are ignoring the key since the mapping is already done in the session | ||
| await this.session.update({ | ||
| oauthState: val, | ||
| }) | ||
| async set(key: string, val: NodeSavedState) { | ||
| await this.cache.set<NodeSavedState>(this.createStorageKey(key), val, STATE_EXPIRATION) | ||
| } | ||
|
|
||
| async del() { | ||
| await this.session.update({ | ||
| oauthState: undefined, | ||
| }) | ||
| async del(key: string) { | ||
| await this.cache.delete(this.createStorageKey(key)) | ||
| } | ||
| } | ||
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.