feat: oauth server side and confidential client#1366
feat: oauth server side and confidential client#1366fatfingers23 wants to merge 9 commits intonpmx-dev:mainfrom
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; moves OAuth session and state storage to a storage-backed, per-session scheme using a new OAUTH_CACHE_STORAGE_BASE; adds JWK support (runtime config field, JWKS route, and a key-generation script); updates server-side OAuth handlers and metadata to load keys asynchronously and use getNodeOAuthClient/loadJWKs; changes OAuth session/state store APIs to key-scoped storage operations and per-session IDs; adds a client-side watcher to trigger reauthentication when the server session sets relogin. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![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?
// A better place to check this would probably be from the api/auth/session endpoint so that
// we only perform outbound requests when a user actually requests data.
})
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, { lastUpdatedAt }] of stateStore.list()) {
if (signal.aborted) return
// Remove every state that was created more than 30 minutes ago
if (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, {lastUpdatedAt, value: 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, lastUpdatedAt)) {
try {
await oauthClient.revoke(sub)
} catch (err) {
// No biggie
}
} else if (shouldRefresh(savedSession, lastUpdatedAt)) {
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 }: 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> {
private readonly storage: Storage<{value: T, lastUpdatedAt: number }>
constructor(storageName: string) {
this.storage = useStorage(storageName)
}
async get(key: string, options?: GetOptions): Promise<undefined | T> {
// TODO
return data.value
}
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, { value: T, lastUpdatedAt: number }> {
const keys = await this.storage.getKeys()
for (const key of keys) {
try {
yield await this.storage.get(key)
} catch (err) {
// the key was removed while async iterating
if (isKeyNotFoundError(err)) continue
throw err
}
}
}
} |
|
Thank you a ton for all the feedback @matthieusieben! Really appreciate it, and I think I'm on board with it all. It will probably be a bit before I can start digging in the code, but I did have a couple of things
|
I see no problem with caching the current "profile" in In that situation, any attempt to actually grab an oauth session (through Now, I still recommend to proactively attempt to refresh the oauth session & |
|
Now that I think about it, we could get away with storing the user did in a long lived cookie, instead of using a Device ID, and use Note that doing this ABSOLUTELY REQUIRES that the cookie is both long lived AND (very importantly) encrypted (to prevent stealing user sessions). Note also that since This means that whatever we do, we have to assume that the presence of both If creating an encrypted version of the DID in a cookie is too hard (why would it be?), a random "device id" will provide a good workaround. I guess the choice between "deviceID" vs. "DID cookie" boils down to how cleanly you want to model "active sessions" in you system. If you use "device ID", NPMX would be able to show "you have X active sessions (on using )", while the "DID cookie" approach prevents that. The "Device ID" approach also allows to better protect against session hijacking attacks (in particular if the device ID is rotated every now and then) |
|
a few quick notes:
|
We're golden then!
That's fine, the redis based lock should handle concurrency |
fatfingers23
force-pushed
the
feat/oauth-take-three
branch
from
e93592b to
6c5aa3e
Compare
|
Thank you for all the feedback @matthieusieben! Helped a lot and I learned some new things as well. Was a bit more afk yesterday and just got around to working on it. If this works for you @danielroe I'm good with going with this.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/atproto/oauth-state-store.ts (1)
1-6:⚠️ Potential issue | 🔴 CriticalMissing imports for cache adapter and constants.
CACHE_MAX_AGE_ONE_MINUTE,CacheAdapter,getCacheAdapter, andOAUTH_CACHE_STORAGE_BASEare used but not imported, which will fail compilation.Proposed fix
import type { NodeSavedState, NodeSavedStateStore } from '@atproto/oauth-client-node' import type { UserServerSession } from '#shared/types/userSession' import type { SessionManager } from 'h3' +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 './storage'
🧹 Nitpick comments (4)
modules/cache.ts (1)
7-46: Consider centralisingOAUTH_CACHE_STORAGE_BASEto avoid drift.
The literal is duplicated here and inserver/utils/atproto/storage.ts; a shared constant would prevent mismatches over time.server/api/auth/atproto.get.ts (1)
60-63: Track theui_localesTODO to avoid a permanent regression.
If locale handling is still needed, consider a follow‑up issue so it doesn’t get lost.If you want, I can draft a follow‑up issue or propose a guarded re‑enablement strategy.
server/utils/atproto/oauth-session-store.ts (1)
68-79: Usereturninstead ofreturn undefinedfor void-returning function.The
del()method implicitly returnsPromise<void>, but lines 70 and 73 explicitly returnundefined. Whilst functionally equivalent, using barereturnstatements is more idiomatic for void-returning functions.♻️ Proposed fix
async del(key: string) { const serverSessionData = this.serverSession.data - if (!serverSessionData) return undefined + if (!serverSessionData) return if (!serverSessionData.oauthSessionId) { console.warn('[oauth session store] No oauthSessionId found in session data') - return undefined + return } await this.cache.delete(this.createStorageKey(key, serverSessionData.oauthSessionId)) await this.serverSession.update({ oauthSessionId: undefined, }) }server/utils/atproto/oauth.ts (1)
107-135: Consider single NodeOAuthClient instance per PR reviewer recommendations.As noted in the PR comments by matthieusieben, creating a new
NodeOAuthClientper request (line 115) may be suboptimal. The recommendation was to use a single long-lived instance via a Nitro plugin to:
- Enable proactive session refresh
- Reduce concurrency risks
- Decouple cookie logic from backend data
This is a design consideration for future improvement rather than a blocking issue.
| '/oauth-client-metadata.json': { prerender: true }, | ||
| '/.well-known/jwks.json': { prerender: true }, |
There was a problem hiding this comment.
🧩 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:
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)- This is part of Nuxt “hybrid rendering” where different routes can be prerendered/SSR/CSR/ISR/SWR, etc. (nuxt.com)
runtimeConfig: build-time vs runtime (the key gotcha with prerender)
Nuxt has two “times” where code can run:
- Build time (prerender/generate time): your page’s server-render step is executed during the build.
- Runtime (request time): your server renders pages when a request comes in.
When a route is prerendered, the “server-side render” for that page happens at build time, so:
- Any
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) - Nuxt explicitly supports reading the resolved runtime config at build time (e.g., in modules) via
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)- But even
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:
- Don’t prerender that page’s HTML (use SSR/SWR/ISR instead), or
- Keep the page prerendered but fetch dynamic/runtime values at runtime (e.g., from an API endpoint) so they aren’t baked into the HTML. (Prerendered HTML itself will not change without a rebuild.) (nuxt.com)
Citations:
- 1: https://nuxt.com/docs/3.x/getting-started/prerendering?utm_source=openai
- 2: https://nuxt.com/docs/3.x/guide/concepts/rendering?utm_source=openai
- 3: https://nuxt.com/docs/3.x/getting-started/prerendering?utm_source=openai
- 4: https://nuxt.com/docs/api/kit/runtime-config?utm_source=openai
- 5: https://nuxt.com/docs/4.x/bridge/runtime-config?utm_source=openai
- 6: https://nuxt.com/docs/3.x/getting-started/prerendering?utm_source=openai
- 7: https://nuxt.com/docs/3.x/getting-started/prerendering?utm_source=openai
🌐 Web query:
Nuxt 3 useRuntimeConfig during prerender SSG build
💡 Result:
During prerender / SSG (nuxi generate or nuxi build --prerender), Nuxt boots a Nuxt/Nitro instance at build time, renders your pages, and writes out static HTML + payload.json into .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
- You can use
useRuntimeConfig()while prerendering (it’s server-side during the build). It will see full config on the server, including private keys. (dev.nuxt.com) - In the generated static site, there is no server runtime config. Only
runtimeConfig.publicis available client-side, and its values are effectively baked into the generated output/payload. (new.nuxtjs.cn) - If you were hoping to change
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 toruntimeConfig.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:
- 1: https://nuxt.com/docs/3.x/getting-started/prerendering?utm_source=openai
- 2: https://dev.nuxt.com/docs/3.x/guide/going-further/runtime-config?utm_source=openai
- 3: https://new.nuxtjs.cn/docs/guide/going-further/runtime-config?utm_source=openai
- 4: https://nuxt.com/docs/3.x/api/composables/use-runtime-config?utm_source=openai
- 5: https://new.nuxtjs.cn/docs/guide/going-further/runtime-config?utm_source=openai
- 6: https://nuxt.com/docs/api/kit/runtime-config?utm_source=openai
Prerendered JWKS endpoint will serve empty keys at runtime, breaking OAuth authentication.
The /.well-known/jwks.json endpoint calls useRuntimeConfig() to read oauthJwkOne from environment. With prerender: true, this evaluation happens at build time (where OAUTH_JWK_ONE is 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: 60 to allow server-side regeneration with access to runtime configuration:
Suggested change
- '/.well-known/jwks.json': { prerender: true },
+ '/.well-known/jwks.json': { isr: 60 },Note: /oauth-client-metadata.json has the same dependency on oauthJwkOne and should also be reconsidered for ISR.
📝 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.
| '/oauth-client-metadata.json': { prerender: true }, | |
| '/.well-known/jwks.json': { prerender: true }, | |
| '/oauth-client-metadata.json': { prerender: true }, | |
| '/.well-known/jwks.json': { isr: 60 }, |
| let oauthSessionId = serverSession.data.oauthSessionId | ||
|
|
||
| // User was authenticated at one point, but was not able to restore | ||
| // the session to the PDS | ||
| if (!oauthSession && oauthSessionId) { | ||
| // cleans up our server side session store | ||
| await serverSession.clear() | ||
| throw createError({ | ||
| status: 401, | ||
| message: 'User needs to re authenticate', | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add null check when accessing serverSession.data.oauthSessionId.
Line 159 accesses serverSession.data.oauthSessionId without guarding against serverSession.data being undefined. If the session data is empty or undefined, this will throw a TypeError.
🛡️ Proposed fix
const { oauthSession, serverSession } = await getOAuthSession(event)
- let oauthSessionId = serverSession.data.oauthSessionId
+ const oauthSessionId = serverSession.data?.oauthSessionId
// User was authenticated at one point, but was not able to restore
// the session to the PDSAs per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index" – the same principle applies to potentially undefined objects.
📝 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.
| let oauthSessionId = serverSession.data.oauthSessionId | |
| // User was authenticated at one point, but was not able to restore | |
| // the session to the PDS | |
| if (!oauthSession && oauthSessionId) { | |
| // cleans up our server side session store | |
| await serverSession.clear() | |
| throw createError({ | |
| status: 401, | |
| message: 'User needs to re authenticate', | |
| }) | |
| } | |
| const { oauthSession, serverSession } = await getOAuthSession(event) | |
| const oauthSessionId = serverSession.data?.oauthSessionId | |
| // User was authenticated at one point, but was not able to restore | |
| // the session to the PDS | |
| if (!oauthSession && oauthSessionId) { | |
| // cleans up our server side session store | |
| await serverSession.clear() | |
| throw createError({ | |
| status: 401, | |
| message: 'User needs to re authenticate', | |
| }) | |
| } |
| // These values are tied to the users browser session and used by atproto OAuth | ||
| oauthSessionId?: string | undefined | ||
| oauthStateId?: string | undefined | ||
| // Last time the oauth session was updated | ||
| lastUpdatedAt?: Date | undefined |
There was a problem hiding this comment.
Avoid Date in session payload; it serialises to string.
useSession storage is JSON‑based, so Date won’t round‑trip. Prefer a string/number and parse when needed.
Suggested adjustment
- lastUpdatedAt?: Date | undefined
+ lastUpdatedAt?: string | undefinedDoes h3 useSession serialise session data as JSON, and do Date objects round‑trip as strings?
shuuji3
changed the title
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!