Skip to content

Comments

fix: small oauth fixes to extend sessions (hopefully)#905

Merged
danielroe merged 8 commits intonpmx-dev:mainfrom
fatfingers23:bug/smalll-oauth-fixes
Feb 5, 2026
Merged

fix: small oauth fixes to extend sessions (hopefully)#905
danielroe merged 8 commits intonpmx-dev:mainfrom
fatfingers23:bug/smalll-oauth-fixes

Conversation

@fatfingers23
Copy link
Contributor

@fatfingers23 fatfingers23 commented Feb 4, 2026

Working on #870. Takes an hour for each access token to expire. So it's slow going

  • Add fallbacks for if the call to the minidoc fails so we can depend on the did being there fully if an oauth session was successful
  • Moved the NodeClient to use handleResolver instead of depending on Bluesky's apis (default if not set)
  • Re worked some of the useSession to hopefully not leave any stale values while the OAuthClient updates the store
  • Return the error page with an error on OAuth redirect failures. I'd like to move this up to the AuthModal, but did not have luck with anything I tried. May be returning json for either a redirect client side or to show an error
  • moved session.get.ts to 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 else

The big error seems to be Refresh token replayed every 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

@vercel
Copy link

vercel bot commented Feb 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 4, 2026 6:51pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 4, 2026 6:51pm
npmx-lunaria Ignored Ignored Feb 4, 2026 6:51pm

Request Review

@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@fatfingers23
Copy link
Contributor Author

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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

Adds new lexicons for Bluesky actor profiles and AT Protocol label definitions and updates lexicons.json to include them. package.json gains an @atproto/syntax dependency. Significant OAuth and session plumbing changes: introduces a DoH-based handle resolver, restructures getOAuthSession to return both oauthSession and serverSession, adapts eventHandlerWithOAuthSession and related handlers, adds a resilient avatar fetch with HTTPS/SSRF protections, and adds error handling around session updates.

Possibly related PRs

Suggested reviewers

  • danielroe
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description directly addresses the changeset, detailing specific issues with OAuth session management, refresh token replay errors, and the implemented solutions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Guard against partially written session data before dereferencing public.did.

If serverSession.data exists but public (or public.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 between set() and del() methods.

The del() method lacks equivalent error handling. If error handling was added to set() for debugging purposes, it would be beneficial to apply the same pattern to del() for consistency.

server/api/auth/atproto.get.ts (3)

14-44: Good SSRF protection and error-tolerant design.

The getAvatar helper properly validates inputs with ensureValidAtIdentifier and restricts fetches to HTTPS endpoints. The silent catch block is appropriate since avatar retrieval is non-critical.

Minor suggestion: The avatar?.ref is 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:

  1. The hardcoded 'Not available' string (line 123) should ideally be a constant or localised string for consistency.
  2. 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()).aud
server/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.

@danielroe danielroe added this pull request to the merge queue Feb 5, 2026
Merged via the queue into npmx-dev:main with commit c8fcc6e Feb 5, 2026
17 checks passed
@fatfingers23 fatfingers23 deleted the bug/smalll-oauth-fixes branch February 5, 2026 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants