fix: small oauth fixes to extend sessions (hopefully) #905
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
fatfingers23
force-pushed
the
bug/smalll-oauth-fixes
branch
from
f1fae5b to
0f87189
Compare
fatfingers23
force-pushed
the
bug/smalll-oauth-fixes
branch
from
868f8ed to
9a95462
Compare
|
I think this PR is good to go. I don't think it will close every thing out for #870, but hoping some changes help with the session refreshes. I've had better luck with it locally testing and I think it's worth submitting for that. |
📝 WalkthroughWalkthroughThis pull request introduces new lexicon definitions for Bluesky actor profiles and AT Protocol label semantics, adds a new dependency for AT Protocol syntax utilities, and significantly refactors the OAuth and session management infrastructure. Key changes include introducing a DoH-based handle resolver for DID resolution, restructuring getOAuthSession to return both OAuth and server session objects, modifying session endpoint handling, adding error handling and resilience to the authorization flow, and implementing a dedicated avatar retrieval function with SSRF protection. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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: 1
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.ts (1)
52-83:⚠️ Potential issue | 🟠 MajorGuard against partially written session data before dereferencing
public.did.If
serverSession.dataexists butpublic(orpublic.did) is absent due to a partial write, this will throw and abort the auth flow. Add a defensive guard and return{ oauthSession: undefined, serverSession }when the DID is missing.🔒 Proposed fix
- const currentSession = serverSession.data - if (!currentSession) return { oauthSession: undefined, serverSession } - - const oauthSession = await client.restore(currentSession.public.did) + const currentSession = serverSession.data + const did = currentSession?.public?.did + if (!did) return { oauthSession: undefined, serverSession } + + const oauthSession = await client.restore(did) return { oauthSession, serverSession }
🧹 Nitpick comments (5)
server/utils/atproto/oauth-session-store.ts (1)
33-37: Inconsistent error handling betweenset()anddel()methods.The
del()method lacks equivalent error handling. If error handling was added toset()for debugging purposes, it would be beneficial to apply the same pattern todel()for consistency.server/api/auth/atproto.get.ts (3)
14-44: Good SSRF protection and error-tolerant design.The
getAvatarhelper properly validates inputs withensureValidAtIdentifierand restricts fetches to HTTPS endpoints. The silent catch block is appropriate since avatar retrieval is non-critical.Minor suggestion: The
avatar?.refis accessed twice on lines 35 and 37 - consider extracting to a variable:♻️ Optional simplification
const validatedResponse = app.bsky.actor.profile.main.validate(profileResponse.value) - if (validatedResponse.avatar?.ref) { + const avatarRef = validatedResponse.avatar?.ref + if (avatarRef) { // Use Bluesky CDN for faster image loading - avatar = `https://cdn.bsky.app/img/feed_thumbnail/plain/${did}/${validatedResponse.avatar?.ref}@jpeg` + avatar = `https://cdn.bsky.app/img/feed_thumbnail/plain/${did}/${avatarRef}@jpeg` }
85-91: Error message may expose internal details to the client.The error message from the underlying exception is directly included in the response. Depending on the error source, this could leak implementation details (e.g., internal hostnames, stack traces in some libraries).
Consider using a generic message for the client while logging the full error server-side:
🛡️ Suggested approach
} catch (error) { const message = error instanceof Error ? error.message : 'Authentication failed.' + console.error('[atproto auth] Authorization failed:', message) return handleApiError(error, { statusCode: 401, - message: `${message}. Please login and try again.`, + message: 'Authentication failed. Please login and try again.', }) }
116-127: Good fallback behaviour, though the hardcoded string could be a concern.The fallback path correctly ensures essential session data (
did,pds,avatar) is available even when the external service fails. However:
- The hardcoded
'Not available'string (line 123) should ideally be a constant or localised string for consistency.- Consider logging when falling back to this path to aid debugging.
🔧 Optional: Add logging for fallback path
} else { //If slingshot fails we still want to set some key info we need. + console.warn('[atproto auth] Slingshot service unavailable, using fallback session data') const pdsBase = (await authSession.getTokenInfo()).audserver/utils/atproto/oauth.ts (1)
14-19: Make the DoH endpoint configurable to support diverse deployment environments.Hard-coding 'https://cloudflare-dns.com/dns-query' limits flexibility for self-hosted instances, region-specific requirements, or enterprise policies. Extract the endpoint to runtime configuration with Cloudflare as the sensible default, aligning with the codebase's existing approach to OAuth configuration.
| try { | ||
| await this.session.update({ | ||
| oauthSession: val, | ||
| }) | ||
| } catch (error) { | ||
| // Not sure if this has been happening. But helps with debugging | ||
| console.error( | ||
| '[oauth session store] Failed to set session:', | ||
| error instanceof Error ? error.message : 'Unknown error', | ||
| ) | ||
| } |
There was a problem hiding this comment.
Error is silently swallowed, which may mask session persistence failures.
The try-catch logs the error but doesn't propagate it, meaning callers won't know the session failed to persist. This could lead to subtle bugs where the OAuth client believes the session is stored when it isn't.
Consider either re-throwing after logging, or returning a boolean to indicate success:
🛠️ Option: Re-throw after logging
try {
await this.session.update({
oauthSession: val,
})
} catch (error) {
// Not sure if this has been happening. But helps with debugging
console.error(
'[oauth session store] Failed to set session:',
error instanceof Error ? error.message : 'Unknown error',
)
+ throw error
}1 participant
Working on #870. Takes an hour for each access token to expire. So it's slow going
session.get.tsto use a regular h3 event handler to see if there may be a race condition and an old refresh token is getting set and sent along. I did see on my last run that it had passed in a brand new session to the store (had a console log). But not sure if it's maybe throwing an expection there on that update or something elseThe big error seems to be
Refresh token replayedevery time the token refreshes (access token has 1 hour lifetime. From reading the atproto repo looks like it is sending a refresh token that does not match and my guess is it's sending the previous one and it is not being set or unset somehwere