-
-
Notifications
You must be signed in to change notification settings - Fork 265
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
base: main
Are you sure you want to change the base?
Changes from all commits
ef06c80
67e5787
b4a028b
626e1c7
e3981f0
e711766
7dbc12a
5857d99
b2e887e
a9ab4f9
bea9ce4
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 |
|---|---|---|
| @@ -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.
🧩 Analysis chain
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 99
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 991
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 1504
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 470
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 1545
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 352
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 1998
🏁 Script executed:
Repository: 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:
Repository: npmx-dev/npmx.dev
Length of output: 1834
🌐 Web query:
Nuxt 3 prerender routeRules runtimeConfig access build time vs runtime💡 Result:
What happens when you set
routeRules: { '/some/**': { prerender: true } }prerender: truemeans Nuxt/Nitro will render that route at build time and write a static HTML file into.output/public/...(or equivalent) which is then served as a file, not computed per request. (nuxt.com)runtimeConfig: build-time vs runtime (the key gotcha with prerender)Nuxt has two “times” where code can run:
When a route is prerendered, the “server-side render” for that page happens at build time, so:
useRuntimeConfig()usage that influences HTML for prerendered pages will be evaluated at build time, and the resulting HTML is frozen until the next build. (Because the output is a static file.) (nuxt.com)useRuntimeConfigfrom Nuxt Kit. (nuxt.com)Public vs private config and what you can “access”
runtimeConfig.publicis intended to be exposed to client-side code; non-public keys are server-only. (nuxt.com)publicruntimeConfig values used during prerender become baked into the generated output, because prerender output is static HTML generated at build time. (nuxt.com)Practical rule of thumb
If a value must change without rebuilding (per environment, per deployment, per request), then:
Citations:
🌐 Web query:
Nuxt 3 useRuntimeConfig during prerender SSG build💡 Result:
During prerender / SSG (
nuxi generateornuxi build --prerender), Nuxt boots a Nuxt/Nitro instance at build time, renders your pages, and writes out static HTML +payload.jsoninto.output/public. (nuxt.com)So
useRuntimeConfig()is evaluated with whatever config/env vars exist at build time—not at “runtime on the static host”.What you can (and can’t) do
useRuntimeConfig()while prerendering (it’s server-side during the build). It will see full config on the server, including private keys. (dev.nuxt.com)runtimeConfig.publicis available client-side, and its values are effectively baked into the generated output/payload. (new.nuxtjs.cn)useRuntimeConfig()values after deployment (without rebuilding), that won’t work with pure static hosting—you need an SSR/server deployment (or rebuild per environment).Common gotchas / fixes
Set env vars for the build step (CI/CD), not just on the hosting platform
Nuxt notes
.env/process.envare used during development and build/generate, while production runtime should use platform env vars (which won’t apply to already-generated static files). (nuxt.com)Don’t read private config in client code
Client-side only has access to
runtimeConfig.public. (new.nuxtjs.cn)If you need config at Nuxt “build time” (module/hooks), use Nuxt Kit
In modules/build tooling,
@nuxt/kitexposesuseRuntimeConfig()to access the resolved runtime config at build time. (nuxt.com)If you share what you’re trying to accomplish (e.g., “different API base URL per environment without rebuild”, or “use secret to fetch data during prerender”), I can suggest the cleanest Nuxt pattern for that setup.
Citations:
Prerendered JWKS endpoint will serve empty keys at runtime, breaking OAuth authentication.
The
/.well-known/jwks.jsonendpoint callsuseRuntimeConfig()to readoauthJwkOnefrom environment. Withprerender: true, this evaluation happens at build time (whereOAUTH_JWK_ONEis typically unavailable in CI/CD), causing the endpoint to return an empty array[]. This empty response gets baked into the prerendered HTML file and is served to all requests, rendering OAuth inoperative regardless of whether the environment variable exists at runtime.Change to
isr: 60to allow server-side regeneration with access to runtime configuration:Suggested change
Note:
/oauth-client-metadata.jsonhas the same dependency onoauthJwkOneand should also be reconsidered for ISR.📝 Committable suggestion