feat: oauth server side and onfidential client #1366
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughCentralises OAuth client construction and session handling, migrating OAuth session and state storage from per-request/in-memory fields to a storage-backed, per-session scheme using a new OAUTH_CACHE_STORAGE_BASE. Adds JWK support (runtime config field, JWKS route, and a generation script), updates OAuth route handlers and metadata to load keys asynchronously, introduces getNodeOAuthClient and related helpers, changes session/state store APIs to key-scoped storage operations, and adds a client-side watcher to trigger reauthentication when the server session marks relogin. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
modules/cache.ts (1)
7-8: Centralise the OAuth storage key to avoid drift.The same literal now exists here and in
server/utils/atproto/storage.ts; consider moving it to a shared constant so updates stay in sync.server/utils/atproto/oauth-session-store.ts (1)
10-11: Remove the duplicate assignment in the constructor.Minor clarity win; the extra assignment is redundant.
Suggested tidy-up
constructor(session: SessionManager<UserServerSession>) { - this.serverSession = this.serverSession = session + this.serverSession = session }server/utils/atproto/oauth-state-store.ts (1)
28-34: Align error handling with the session store.If
setItemfails afteroauthStateIdis updated, the session will point to a missing state. Consider wrapping the update/write in a try/catch, logging the error, and rolling backoauthStateId.Example pattern
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) + const stateId = crypto.randomUUID() + try { + await this.serverSession.update({ oauthStateId: stateId }) + await this.storage.setItem<NodeSavedState>(this.createStorageKey(key, stateId), val) + } catch (error) { + console.error( + '[oauth state store] Failed to set state:', + error instanceof Error ? error.message : 'Unknown error', + ) + await this.serverSession.update({ oauthStateId: undefined }) + throw error + } }
| runtimeConfig: { | ||
| sessionPassword: '', | ||
| oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined, | ||
| // Upstash Redis for distributed OAuth token refresh locking in production |
There was a problem hiding this comment.
🧩 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 oauthJwkOne config reads from OAUTH_JWK_ONE, but Nuxt convention (as used for sessionPassword with NUXT_SESSION_PASSWORD) suggests it should be NUXT_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)
- oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined,
+ oauthJwkOne: process.env.NUXT_OAUTH_JWK_ONE || process.env.OAUTH_JWK_ONE || undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runtimeConfig: { | |
| sessionPassword: '', | |
| oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined, | |
| // Upstash Redis for distributed OAuth token refresh locking in production | |
| runtimeConfig: { | |
| sessionPassword: '', | |
| oauthJwkOne: process.env.NUXT_OAUTH_JWK_ONE || process.env.OAUTH_JWK_ONE || undefined, | |
| // Upstash Redis for distributed OAuth token refresh locking in production |
| 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' |
There was a problem hiding this comment.
Import getNodeOAuthClient where it is used.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { scope } from '#server/utils/atproto/oauth' | |
| import { getNodeOAuthClient, scope } from '#server/utils/atproto/oauth' |
| const keyset = await loadJWKs(config) | ||
| // @ts-expect-error Taken from statusphere-example-app. Throws a ts error | ||
| const pk = keyset?.findPrivateKey({ use: 'sig' }) |
There was a problem hiding this comment.
🧩 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:
@atproto/oauth-client-node Keyset type findPrivateKey method
💡 Result:
In @atproto/oauth-client-node (and the core @atproto/oauth-client), the keyset you pass is not a Keyset object with a findPrivateKey() method—it’s simply a list/array of private Key objects used for private_key_jwt client authentication. The README shows keyset: [ await JoseKey.fromImportable(...), ... ]. [1]
So there is no keyset.findPrivateKey(...) to call. If you need that behavior, implement it yourself by selecting from your array (usually by kid), e.g. keyset.find(k => k.kid === kid).
To construct keys for that array, use JoseKey.fromImportable(input, kid?) (optionally supplying a kid). [2]
Sources
[1] @atproto/oauth-client README example showing keyset as an array of keys: (npmjs.com)
[2] JoseKey.fromImportable(input, kid?) in @atproto/jwk-jose: (app.unpkg.com)
Citations:
- 1: https://www.npmjs.com/package/%40atproto/oauth-client?utm_source=openai
- 2: https://app.unpkg.com/%40atproto/jwk-jose%400.1.11/files/src/jose-key.ts
🏁 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:
@atproto/oauth-client-node 0.3.15 Keyset API documentation
💡 Result:
keyset (confidential-client signing keys)
In @atproto/oauth-client-node@0.3.15, the “Keyset API” is the keyset option you pass into the underlying OAuthClient configuration to enable confidential client auth (token_endpoint_auth_method=private_key_jwt). The type is:
keyset?: Keyset | Iterable<Key>(imported from@atproto/jwk) [2]
What it’s for:
- Provide one or more private keys used to sign the client assertion JWT for authenticated token requests.
- These must correspond to the public keys you publish in client metadata (
jwks/jwks_uri). [1][3] - The public JWKS corresponding to your configured private keys can be obtained via the
client.jwksgetter. [1]
Minimal example (Node / backend / BFF):
import { NodeOAuthClient } from '@atproto/oauth-client-node'
import { JoseKey } from '@atproto/jwk-jose'
const client = new NodeOAuthClient({
responseMode: 'query',
clientMetadata: {
client_id: 'https://example.com/oauth-client-metadata.json',
jwks_uri: 'https://example.com/jwks.json',
token_endpoint_auth_method: 'private_key_jwt',
},
// ...stateStore, sessionStore, etc...
keyset: [
await JoseKey.fromImportable(process.env.PRIVATE_KEY_1!),
await JoseKey.fromImportable(process.env.PRIVATE_KEY_2!),
],
})Notes:
- If you’re building a native/Electron app without a backend, you typically do not provide a
keyset(you can’t safely store the private key client-side). [4] - Atproto OAuth expects confidential clients to publish JWKS and rotate keys; authorization servers may re-fetch metadata/JWKS and can revoke sessions if keys disappear. [3]
Sources: @atproto/oauth-client config example showing keyset and client.jwks [1], OAuthClientOptions type including keyset?: Keyset | Iterable<Key> [2], atproto OAuth spec on jwks / jwks_uri and confidential-client key expectations [3], @atproto/oauth-client-node readme note about no keyset in app-only clients [4].
🌐 Web query:
statusphere-example-app oauth keyset findPrivateKey
💡 Result:
In bluesky-social/statusphere-example-app, the OAuth “private key / keyset” is configured via the PRIVATE_KEYS environment variable (a JSON array of JWKs). In production you’re expected to generate a JWK (private key) with ./bin/gen-jwk and paste the generated JWK JSON into PRIVATE_KEYS (example shown in the README). The app uses the first key in that array as the current signing key (older keys can remain for validating existing sessions). [1]
The OAuth client in the app is built on @atproto/oauth-client-node and is initialized in the app’s authentication code (src/auth/client.ts per the project docs). [2]
Sources: GitHub README for the repo (key generation + PRIVATE_KEYS behavior) [1]; DeepWiki “Authentication System” page (OAuth client package + file locations) [2].
[1] (github.com)
[2] (deepwiki.com)
Citations:
- 1: https://github.com/bluesky-social/statusphere-example-app
- 2: https://deepwiki.com/bluesky-social/statusphere-example-app/3.3-authentication-system
Replace the @ts-expect-error suppression with a typed helper in both locations.
The suppression weakens type safety and violates the "strictly type-safe" guideline. This pattern appears in both server/routes/oauth-client-metadata.json.get.ts (lines 3–5) and server/utils/atproto/oauth.ts (lines 80–81), indicating a systemic need for a typed solution. Extract a helper function (e.g., getPrivateSigningKey) in server/utils/atproto/oauth.ts to encapsulate the private key selection logic and eliminate the type suppression across both files.
Example change
- // `@ts-expect-error` Taken from statusphere-example-app. Throws a ts error
- const pk = keyset?.findPrivateKey({ use: 'sig' })
+ const pk = getPrivateSigningKey(keyset)🧰 Tools
🪛 GitHub Check: 💪 Type check
[failure] 3-3:
Argument of type 'RuntimeConfig' is not assignable to parameter of type 'NitroRuntimeConfig'.
📝 WalkthroughWalkthroughThe PR implements OAuth session refresh and state management improvements by introducing server-side storage for OAuth sessions and state. It adds a JWK generation script and exposes public keys via a new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
server/utils/atproto/oauth-state-store.ts (1)
28-34: UseconstforstateIdand consider adding error handling.The
stateIdvariable is never reassigned, so it should be declared withconst. Additionally, thesetmethod inOAuthSessionStoreincludes error handling with try/catch, but this method lacks it—consider adding consistent error handling for debugging purposes.Proposed fix
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) + const stateId = crypto.randomUUID() + try { + await this.serverSession.update({ + oauthStateId: stateId, + }) + await this.storage.setItem<NodeSavedState>(this.createStorageKey(key, stateId), val) + } catch (error) { + console.error( + '[oauth state store] Failed to set state:', + error instanceof Error ? error.message : 'Unknown error', + ) + throw error + } }server/utils/atproto/oauth-session-store.ts (2)
26-29: Useconstfor variables that are not reassigned.Both
session(line 26) andsessionId(line 33) are never reassigned and should useconstinstead oflet.Proposed fix for line 26
- let session = await this.storage.getItem<NodeSavedSession>( + const session = await this.storage.getItem<NodeSavedSession>( this.createStorageKey(key, serverSessionData.oauthSessionId), )
32-38: UseconstforsessionId.The
sessionIdvariable is never reassigned.Proposed fix
async set(key: string, val: NodeSavedSession) { - let sessionId = crypto.randomUUID() + const sessionId = crypto.randomUUID() try {
| runtimeConfig: { | ||
| sessionPassword: '', | ||
| oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined, | ||
| // Upstash Redis for distributed OAuth token refresh locking in production |
There was a problem hiding this comment.
🧩 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 runtimeConfig entries with NUXT_ when reading environment variables (as seen in CONTRIBUTING.md with NUXT_TEST_FIXTURES). The current code reads process.env.OAUTH_JWK_ONE directly, bypassing this auto-prefixing. In standard Nuxt deployments, the variable is set as NUXT_OAUTH_JWK_ONE, so the current code will always resolve to undefined.
Either allow Nuxt's auto-prefixing to work (remove process.env access), or explicitly support both the prefixed and non-prefixed variants:
🔧 Suggested adjustment
- oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined,
+ oauthJwkOne:
+ process.env.NUXT_OAUTH_JWK_ONE ??
+ process.env.OAUTH_JWK_ONE ??
+ undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runtimeConfig: { | |
| sessionPassword: '', | |
| oauthJwkOne: process.env.OAUTH_JWK_ONE || undefined, | |
| // Upstash Redis for distributed OAuth token refresh locking in production | |
| runtimeConfig: { | |
| sessionPassword: '', | |
| oauthJwkOne: | |
| process.env.NUXT_OAUTH_JWK_ONE ?? | |
| process.env.OAUTH_JWK_ONE ?? | |
| undefined, | |
| // Upstash Redis for distributed OAuth token refresh locking in production |
| 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' |
There was a problem hiding this comment.
Missing import for getNodeOAuthClient.
The function getNodeOAuthClient is called on line 29 but is not imported. This will cause a runtime error.
Proposed fix
-import { scope } from '#server/utils/atproto/oauth'
+import { scope, getNodeOAuthClient } from '#server/utils/atproto/oauth'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { scope } from '#server/utils/atproto/oauth' | |
| import { scope, getNodeOAuthClient } from '#server/utils/atproto/oauth' |
| if (!keys) { | ||
| console.error('Failed to load JWKs. May not be set') | ||
| return [] | ||
| } |
There was a problem hiding this comment.
Inconsistent JWKS response format on failure.
When JWKs fail to load, returning an empty array [] does not conform to the JWKS specification, which expects { "keys": [] }. OAuth clients parsing this endpoint may fail if they expect the standard JWKS structure.
Proposed fix
if (!keys) {
console.error('Failed to load JWKs. May not be set')
- return []
+ return { keys: [] }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!keys) { | |
| console.error('Failed to load JWKs. May not be set') | |
| return [] | |
| } | |
| if (!keys) { | |
| console.error('Failed to load JWKs. May not be set') | |
| return { keys: [] } | |
| } |
| 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' }) | ||
| return getOauthClientMetadata(pk?.alg) |
There was a problem hiding this comment.
🧩 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:
@atproto/oauth-client-node v0.3.15 Keyset findPrivateKey TypeScript types
💡 Result:
In @atproto/oauth-client-node@0.3.15, the keyset you pass in is typed by the core package as:
keyset?: Keyset | Iterable<Key | undefined | null | false>where Keyset and Key come from @atproto/jwk. (app.unpkg.com)
In current @atproto/jwk typings, Keyset is a class and it does not have a findPrivateKey method. Instead, it exposes methods like:
get(search: KeySearch): Klist(search: KeySearch): Generator<K>findKey(search: KeySearch): [key: Key, alg: string]
So keyset.findPrivateKey(...) will fail type-checking because it’s not part of the Keyset API. (app.unpkg.com)
If your goal is “get the signing key for a given kid”, you’d do something like:
const key = keyset.get({ kid, use: 'sig' }) // returns a Key
// or:
const [key, alg] = keyset.findKey({ kid, use: 'sig' })Citations:
- 1: https://app.unpkg.com/%40atproto/oauth-client%400.3.7/files/dist/oauth-client.d.ts
- 2: https://app.unpkg.com/%40atproto/jwk%400.1.1/files/dist/keyset.d.ts
Remove @ts-expect-error and use the correct Keyset API method.
The suppression masks a genuine type error: Keyset (from @atproto/jwk) does not provide a findPrivateKey() method. Use keyset.get({ use: 'sig' }) or keyset.findKey({ use: 'sig' }) instead to obtain the signing key with proper type safety.
🔧 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)
})
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/utils/atproto/oauth-session-store.ts (1)
26-29: Useconstinstead ofletfor non-reassigned variable.
sessionis never reassigned, so preferconstfor clarity.Proposed fix
- let session = await this.storage.getItem<NodeSavedSession>( + const session = await this.storage.getItem<NodeSavedSession>( this.createStorageKey(key, serverSessionData.oauthSessionId), )
| // These are optional and not expected or can be used easily in local development, only in production | ||
| const keyset = await loadJWKs(config) | ||
| // @ts-expect-error Taken from statusphere-example-app. Throws a ts error | ||
| const pk = keyset?.findPrivateKey({ use: 'sig' }) | ||
| const clientMetadata = getOauthClientMetadata(pk?.alg) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine the specific lines
cat -n server/utils/atproto/oauth.ts | sed -n '70,90p'Repository: npmx-dev/npmx.dev
Length of output: 905
🏁 Script executed:
# Check the imports at the top of the file
head -30 server/utils/atproto/oauth.tsRepository: npmx-dev/npmx.dev
Length of output: 1252
🏁 Script executed:
# Search for Keyset type definitions in the codebase
rg -t ts "type Keyset|interface Keyset|class Keyset" --max-count 20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Search for findPrivateKey usage
rg "findPrivateKey" --type ts --type tsxRepository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check dependencies related to jose
cat package.json | grep -A 20 '"dependencies"' | grep -i joseRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Search for findPrivateKey with corrected syntax
rg "findPrivateKey" --type tsRepository: npmx-dev/npmx.dev
Length of output: 242
🏁 Script executed:
# Search for loadJWKs function definition
rg "loadJWKs" -A 5 --type tsRepository: npmx-dev/npmx.dev
Length of output: 1418
🏁 Script executed:
# Check if there are node_modules or type definitions available
fd -t f "oauth-client-node" --max-depth 3Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Let's look for JoseKey which is also imported
rg "JoseKey" --type ts -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 1256
🏁 Script executed:
# Get full loadJWKs function
rg "export async function loadJWKs" -A 10 --type tsRepository: npmx-dev/npmx.dev
Length of output: 868
🏁 Script executed:
# Check the second location with the same pattern
cat -n server/routes/oauth-client-metadata.json.get.ts | sed -n '1,50p'Repository: npmx-dev/npmx.dev
Length of output: 413
🏁 Script executed:
# Check more context around the loadJWKs and Keyset creation
rg "new Keyset" -B 5 -A 2 --type tsRepository: npmx-dev/npmx.dev
Length of output: 499
🏁 Script executed:
# Let's verify that there are no other type-safe ways this is handled elsewhere
rg "keyset\?" --type ts -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 891
🏁 Script executed:
# Check if there's any type definition or augmentation for Keyset in the project
fd -t f "tsconfig\|types\|d.ts" | head -20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Look for any .d.ts files that might augment the Keyset type
fd "\.d\.ts" --type fRepository: npmx-dev/npmx.dev
Length of output: 119
Remove @ts-expect-error and add type safety for findPrivateKey access.
This suppression hides a real typing gap in the Keyset type from '@atproto/oauth-client-node'. The method exists at runtime but TypeScript doesn't recognise it. This pattern appears in two places (server/utils/atproto/oauth.ts and server/routes/oauth-client-metadata.json.get.ts) and violates the requirement for strictly type-safe code.
Implement a local type guard to safely cast the Keyset or type-assert the method's availability, then remove the suppression.
🔧 Suggested fix (local type guard, remove suppression)
const keyset = await loadJWKs(config)
- // `@ts-expect-error` Taken from statusphere-example-app. Throws a ts error
- const pk = keyset?.findPrivateKey({ use: 'sig' })
+ type KeysetWithFindPrivateKey = Keyset & {
+ findPrivateKey: (opts: { use: string }) => JoseKey | undefined
+ }
+ const pk =
+ keyset && 'findPrivateKey' in keyset
+ ? (keyset as KeysetWithFindPrivateKey).findPrivateKey({ use: 'sig' })
+ : undefined
There was a problem hiding this comment.
I'll wait for @matthieusieben's review, but in the mean time I've created that key and configured it on vercel so that it's good to go from my pov.
fyi, I created two keys, one for production + a different one for preview deployments. if we should get rid of preview deployment key (maybe allow nuxt to regen a key per-preview, as we do with session passwords?) then let me know
|
Hey @fatfingers23, While I wrote the First, I am wondering if the use of Secondly, by creating a new instance for every request, you are using the Thirdly, the implementation lacks a "cleanup" logic. That logic should ideally be implemented inside the Here is my recommendation (and again, this is based on my basic understanding of Nitro):
Here is a skeleton of what the plugin might look like: /// Plugin definition
export default defineNitroPlugin(nitroApp => {
// Store keeping pending OAuth authorization requests
const stateStore = new SimpleStoreStorage<NodeSavedStateStore>("oauth:state")
// Store keeping credentials from completed OAuth authorization requests
const sessionStore = new SimpleStoreStorage<NodeSavedSessionStore>("oauth:session")
const oauthClient = new NodeOAuthClient({
stateStore,
sessionStore,
clientMetadata,
requestLock: getOAuthLock(),
handleResolver,
})
oauthClient.addEventListener('updated', ({ details: { sub, tokenSet: { aud } } }) => {
// An OAuth session was refreshed. we might want to check if we need to update the
// profile info (e.g. if the `aud` changed) in the [Device ID => sub] storage.
})
oauthClient.addEventListener('deleted', ({ details: { sub } }) => {
// An OAuth session was deleted (for any reason: failure to refresh, user logout, etc.):
// TODO: remove the the "profile" from the "profile" storage
})
nitroApp.oauthClient = oauthClient
// Prevent the "oauth:state" store from growing indefinitely with un-completed oauth requests
// @NOTE: This should eventually be implemented by the NodeOAuthClient itself !
// Note that this can be implemented with a TTL on the store (if that is available)
const stopCleanup = startAsyncTask(async (signal: AbortSignal) => {
try {
for await (const [key, state] of stateStore.list()) {
if (signal.aborted) return
// Remove every state that was created more than 30 minutes ago
if (state.lastUpdatedAt < Date.now() - 30 * 60 * 1e3) {
stateStore.del(key)
}
}
} catch (err) {
// "list" triggered an error, ignore it
console.error('Failed to list active sessions', err)
}
})
nitroApp.hooks.hook('close', stopCleanup)
// Proactively keep OAuth sessions alive
// @NOTE: This should eventually be implemented by the NodeOAuthClient itself !
const stopRefresh = startAsyncTask(async (signal: AbortSignal) => {
try {
for await (const [sub, savedSession] of sessionStore.list()) {
if (signal.aborted) return
// Note that we can keep either one , or both, branches of the if/else-if bellow (depending on what we want)
if (wasLastRefreshedTooLongAgo(savedSession)) {
try {
await oauthClient.revoke(sub)
} catch (err) {
// No biggie
}
} else if (shouldRefresh(savedSession)) {
try {
await oauthClient.restore(key, true)
} catch (err) {
console.warn(`Refreshing session ${key} failed`, err);
// No need to remove from the store, the client will call sessionStore.del()
}
}
}
} catch (err) {
// "list" triggered an error, ignore it
console.error('Failed to list active sessions', err)
}
})
nitroApp.hooks.hook('close', stopRefresh)
})Implementation detailsfunction shouldRefresh({ tokenSet, lastUpdatedAt }: NodeSavedSession & { lastUpdatedAt: number }) {
if (tokenSet.expires_at) {
// If the server returned an expiration date, use that as signal to refresh
return new Date(tokenSet.expires_at).getTime() <= Date.now()
} else {
// Otherwise, refresh once a day (every 23.5H)
return lastUpdatedAt <= Date.now() - 23.5 * 60 * 60e3
}
}
export function startAsyncTask(
task: (signal: AbortSignal) => void | Promise<void>,
intervalMs = 60 * 60 * 1e3, // 1 hour
): () => Promise<void> {
const abortController = new AbortController()
let promise: Promise<void> | undefined
let timer: ReturnType<typeof setTimeout> | undefined
const runAndSchedule = async (signal: AbortSignal) => {
await (promise = (async () => task(signal))())
if (!signal.aborted) {
timer = setTimeout(runAndSchedule, intervalMs, signal)
}
}
runAndSchedule(abortController.signal)
return async () => {
abortController.abort()
clearTimeout(timer)
await promise // Await pending task run to finish
}
}Note, with this change, you can probably use a single implementation for the "store" instead of two: /**
* Wrapper around {@link Storage} that implements the {@link SimpleStore} interface, required by the AT Protocol OAuth client.
*/
class SimpleStoreStorage<T extends Record<string, unknown>> implements SimpleStore<string, T & { lastUpdatedAt: number }> {
private readonly storage: Storage<T>
constructor(storageName: string) {
this.storage = useStorage(storageName)
}
async get(key: string, options?: GetOptions): Promise<undefined | T> {
// TODO
}
async set(key: string, value: T): Promise<void> {
// TODO
const data = { ...value, lastUpdatedAt: Date.now() }
}
async del(key: string): Promise<void> {
// TODO
}
// Note: not part of the simple store interface
async *list(): AsyncGenerator<string, T> {
const keys = await this.storage.getKeys()
for (const key of keys) {
yield await this.storage.get(key)
}
}
} |
3 participants
This moves the OAuth session/state store back server-side. This should resolve #840, resolves #1322, and I think we can go ahead and close #870 and create new issues on new bugs
NUXT_OAUTH_JWK_ONEon production before we do the next release, or find a better home for it. This value can be gotten frompnpm run generate:jwkand is a secret. This will hopefully allow oauth sessions to last much longer than 2 weeks@matthieusieben was kind enough to offer a review of this PR, so I am going to tag him. Thanks again for looking this over and for the other atproto help!