-
-
Notifications
You must be signed in to change notification settings - Fork 239
feat: oauth server side and onfidential 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
ebce9c9
d7b1bbf
cb4619f
fc88622
27e7b66
d5fe914
4b7ab3a
e93592b
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 | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,7 @@ export default defineNuxtConfig({ | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| runtimeConfig: { | ||||||||||||||||||||||||
| sessionPassword: '', | ||||||||||||||||||||||||
| oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined, | ||||||||||||||||||||||||
| // Upstash Redis for distributed OAuth token refresh locking in production | ||||||||||||||||||||||||
|
Comment on lines
35
to
38
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 || '', | ||||||||||||||||||||||||
|
|
@@ -119,6 +120,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 }, | ||||||||||||||||||||||||
| // never cache | ||||||||||||||||||||||||
| '/api/auth/**': { isr: false, cache: false }, | ||||||||||||||||||||||||
| '/api/social/**': { isr: false, cache: false }, | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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,8 @@ 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, | ||||||||||
| }) | ||||||||||
| const atclient = await getNodeOAuthClient(session, config) | ||||||||||
|
|
||||||||||
| if (query.handle) { | ||||||||||
| // Initiate auth flow | ||||||||||
|
|
@@ -69,7 +57,10 @@ export default defineEventHandler(async event => { | |||||||||
| const redirectUrl = await atclient.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 }), | ||||||||||
| }) | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,12 @@ | ||||||||||||||||||
| import { loadJWKs } from '#server/utils/atproto/oauth' | ||||||||||||||||||
|
|
||||||||||||||||||
| export default defineEventHandler(async event => { | ||||||||||||||||||
| const config = useRuntimeConfig(event) | ||||||||||||||||||
| const keys = await loadJWKs(config) | ||||||||||||||||||
| if (!keys) { | ||||||||||||||||||
| console.error('Failed to load JWKs. May not be set') | ||||||||||||||||||
| return [] | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+6
to
+9
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,7 @@ | ||
| export default defineEventHandler(() => { | ||
| return getOauthClientMetadata() | ||
| export default defineEventHandler(async event => { | ||
| const config = useRuntimeConfig(event) | ||
| const keyset = await loadJWKs(config) | ||
| // @ts-expect-error Taken from statusphere-example-app. Throws a ts error | ||
| const pk = keyset?.findPrivateKey({ use: 'sig' }) | ||
|
Comment on lines
+3
to
+5
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, locate and examine the file in question
find . -name "oauth-client-metadata.json.get.ts" -type fRepository: npmx-dev/npmx.dev Length of output: 110 🏁 Script executed: # Check for existing oauth utils
find . -path "*/server/utils/atproto/*" -type fRepository: npmx-dev/npmx.dev Length of output: 331 🏁 Script executed: # Search for Keyset imports and usage patterns
rg "import.*Keyset|from.*oauth-client" --type ts --type tsxRepository: npmx-dev/npmx.dev Length of output: 88 🏁 Script executed: # Check package.json for `@atproto/oauth-client-node` version
cat package.json | grep -A 2 -B 2 "@atproto/oauth-client"Repository: npmx-dev/npmx.dev Length of output: 235 🏁 Script executed: # Read the file with the issue
cat -n ./server/routes/oauth-client-metadata.json.get.tsRepository: npmx-dev/npmx.dev Length of output: 413 🏁 Script executed: # Read oauth.ts to check for existing helpers
cat -n ./server/utils/atproto/oauth.tsRepository: npmx-dev/npmx.dev Length of output: 7060 🏁 Script executed: # Search for Keyset type and how it's used elsewhere
rg "Keyset|findPrivateKey" --type ts -A 3 -B 1Repository: npmx-dev/npmx.dev Length of output: 2001 🌐 Web query:
💡 Result: In So there is no To construct keys for that array, use Sources Citations:
🏁 Script executed: # Check the `@atproto/oauth-client-node` types for Keyset
find node_modules/@atproto/oauth-client-node -name "*.d.ts" -type f | head -20Repository: npmx-dev/npmx.dev Length of output: 135 🏁 Script executed: # Look for Keyset type definition
rg "class Keyset|interface Keyset|type Keyset" --type tsRepository: npmx-dev/npmx.dev Length of output: 43 🏁 Script executed: # Check if there are any other implementations or references to findPrivateKey
rg "findPrivateKey" --type ts -B 2 -A 2Repository: npmx-dev/npmx.dev Length of output: 891 🌐 Web query:
💡 Result:
|
||
| return getOauthClientMetadata(pk?.alg) | ||
|
Comment on lines
+1
to
+6
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: # Find package.json to check `@atproto/oauth-client-node` version
find . -name "package.json" -type f | head -5Repository: npmx-dev/npmx.dev Length of output: 114 🏁 Script executed: # Look at the file in question to understand full context
cat -n server/routes/oauth-client-metadata.json.get.tsRepository: npmx-dev/npmx.dev Length of output: 413 🏁 Script executed: # Search for loadJWKs import to find its source
rg "loadJWKs" --type ts --type tsx -B2 -A2Repository: npmx-dev/npmx.dev Length of output: 88 🏁 Script executed: # Search for loadJWKs import more broadly
rg "loadJWKs" -B2 -A2Repository: npmx-dev/npmx.dev Length of output: 1356 🏁 Script executed: # Check main package.json for `@atproto` dependencies
cat package.json | grep -A5 -B5 "@atproto"Repository: npmx-dev/npmx.dev Length of output: 626 🏁 Script executed: # Read the oauth.ts file to understand Keyset type and loadJWKs implementation
cat -n server/utils/atproto/oauth.tsRepository: npmx-dev/npmx.dev Length of output: 7039 🌐 Web query:
💡 Result: In keyset?: Keyset | Iterable<Key | undefined | null | false>where In current
So If your goal is “get the signing key for a given const key = keyset.get({ kid, use: 'sig' }) // returns a Key
// or:
const [key, alg] = keyset.findKey({ kid, use: 'sig' })Citations:
Remove The suppression masks a genuine type error: 🔧 Suggested fix using Keyset.get() export default defineEventHandler(async event => {
const config = useRuntimeConfig(event)
const keyset = await loadJWKs(config)
- // `@ts-expect-error` Taken from statusphere-example-app. Throws a ts error
- const pk = keyset?.findPrivateKey({ use: 'sig' })
+ const pk = keyset?.get({ use: 'sig' })
return getOauthClientMetadata(pk?.alg)
}) |
||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,30 +1,45 @@ | ||
| 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 '#server/utils/atproto/storage' | ||
|
|
||
| export class OAuthStateStore implements NodeSavedStateStore { | ||
| private readonly session: SessionManager<UserServerSession> | ||
| private readonly serverSession: SessionManager<UserServerSession> | ||
| private readonly storage = useStorage(OAUTH_CACHE_STORAGE_BASE) | ||
|
|
||
| constructor(session: SessionManager<UserServerSession>) { | ||
| this.session = session | ||
| this.serverSession = session | ||
| } | ||
|
|
||
| async get(): Promise<NodeSavedState | undefined> { | ||
| const sessionData = this.session.data | ||
| if (!sessionData) return undefined | ||
| return sessionData.oauthState | ||
| private createStorageKey(did: string, sessionId: string) { | ||
| return `state:${did}:${sessionId}` | ||
| } | ||
|
|
||
| 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 get(key: string): Promise<NodeSavedState | undefined> { | ||
| const serverSessionData = this.serverSession.data | ||
| if (!serverSessionData) return undefined | ||
| if (!serverSessionData.oauthStateId) return undefined | ||
| const state = await this.storage.getItem<NodeSavedState>( | ||
| this.createStorageKey(key, serverSessionData.oauthStateId), | ||
| ) | ||
| return state ?? undefined | ||
| } | ||
|
|
||
| async set(key: string, val: NodeSavedState) { | ||
| let stateId = crypto.randomUUID() | ||
| await this.serverSession.update({ | ||
| oauthStateId: stateId, | ||
| }) | ||
| await this.storage.setItem<NodeSavedState>(this.createStorageKey(key, stateId), val) | ||
| } | ||
|
|
||
| async del() { | ||
| await this.session.update({ | ||
| oauthState: undefined, | ||
| async del(key: string) { | ||
| const serverSessionData = this.serverSession.data | ||
| if (!serverSessionData) return undefined | ||
| if (!serverSessionData.oauthStateId) return undefined | ||
| await this.storage.removeItem(this.createStorageKey(key, serverSessionData.oauthStateId)) | ||
| await this.serverSession.update({ | ||
| oauthStateId: undefined, | ||
| }) | ||
| } | ||
| } |
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: 450
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 135
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 644
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 231
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 1726
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 1138
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 231
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 91
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 132
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 345
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 1727
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 3401
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 96
🏁 Script executed:
Repository: npmx-dev/npmx.dev
Length of output: 445
Use standard Nuxt env var naming for consistency.
The
oauthJwkOneconfig reads fromOAUTH_JWK_ONE, but Nuxt convention (as used forsessionPasswordwithNUXT_SESSION_PASSWORD) suggests it should beNUXT_OAUTH_JWK_ONE. To avoid confusion and follow the established pattern, either rename to the prefixed variant or support both for backwards compatibility.💡 Suggested approach (support both names)
📝 Committable suggestion