Skip to content

Conversation

@NandkishorJadoun
Copy link
Contributor

Description

This pr improves the authentication experience by handling cases where a user explicitly cancels the authorization request on the Bluesky/ATProto login page.

Previously, clicking "Cancel" would result in 401 "Handle not provided in query" error. This change introduces a check for the access_denied error and gracefully redirects the user back to their original location using the validated path stored in the session cookie.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

This change adds error handling to the OAuth callback endpoint for the atproto authentication flow. When the OAuth provider returns an access_denied error via query parameter, the endpoint now redirects the user to the URL stored in the auth_return_to cookie, defaulting to "/" if the cookie is absent. The cookie is cleared before the redirect occurs. This logic is executed as an early return prior to the existing code that processes the authorisation code.

Possibly related PRs

Suggested reviewers

  • danielroe
  • wojtekmaj
  • fatfingers23
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description accurately describes the changeset, explaining the OAuth cancellation handling and the graceful redirect mechanism implemented in the code changes.

✏️ 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

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
server/api/auth/atproto.get.ts (3)

73-80: Type safety: query.error could be an array.

The getQuery function from h3 can return arrays for repeated query parameters. While comparing an array to a string will safely return false, explicitly handling the type makes the code more robust and aligns with the coding guidelines for strict type safety.

Additionally, there's an inconsistency: deleteCookie here specifies { path: '/' } but the same deletion at line 169 omits this option. For the cookie to be properly cleared, both calls should use matching options.

♻️ Suggested improvement
- const error = query.error
-
- // user cancelled explicitly
- if (error === 'access_denied') {
-   const returnToURL = getCookie(event, 'auth_return_to') || '/'
-   deleteCookie(event, 'auth_return_to', { path: '/' })
-   return sendRedirect(event, returnToURL)
- }
+ const error = query.error?.toString()
+
+ // user cancelled explicitly
+ if (error === 'access_denied') {
+   const returnToURL = getCookie(event, 'auth_return_to') || '/'
+   deleteCookie(event, 'auth_return_to')
+   return sendRedirect(event, returnToURL)
+ }

96-101: Consider setting an explicit path when creating the cookie.

The auth_return_to cookie is set here without an explicit path option (which typically defaults to the current request path), but it is deleted at line 78 with { path: '/' } and at line 169 without a path. For consistent behaviour, consider explicitly setting path: '/' when creating the cookie, ensuring it can be reliably read and deleted regardless of which endpoint handles it.

♻️ Suggested improvement
   setCookie(event, 'auth_return_to', redirectPath, {
     maxAge: 60 * 5,
     httpOnly: true,
     // secure only if NOT in dev mode
     secure: !import.meta.dev,
+    path: '/',
   })

168-169: Ensure deleteCookie options match the cookie creation.

If the cookie was set with a specific path (or if you adopt the suggestion to explicitly set path: '/'), this deletion should include the same path option to guarantee the cookie is cleared correctly.

♻️ Suggested improvement
  const returnToURL = getCookie(event, 'auth_return_to') || '/'
- deleteCookie(event, 'auth_return_to')
+ deleteCookie(event, 'auth_return_to', { path: '/' })

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

3 similar comments
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

This change adds error handling to the OAuth callback endpoint for the atproto authentication flow. When the OAuth provider returns an access_denied error via query parameter, the endpoint now redirects the user to the URL stored in the auth_return_to cookie, defaulting to "/" if the cookie is absent. The cookie is cleared before the redirect occurs. This logic is executed as an early return prior to the existing code that processes the authorisation code.

Possibly related PRs

Suggested reviewers

  • danielroe
  • wojtekmaj
  • fatfingers23
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description accurately describes the changeset, explaining the OAuth cancellation handling and the graceful redirect mechanism implemented in the code changes.

✏️ 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

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
server/api/auth/atproto.get.ts (3)

73-80: Type safety: query.error could be an array.

The getQuery function from h3 can return arrays for repeated query parameters. While comparing an array to a string will safely return false, explicitly handling the type makes the code more robust and aligns with the coding guidelines for strict type safety.

Additionally, there's an inconsistency: deleteCookie here specifies { path: '/' } but the same deletion at line 169 omits this option. For the cookie to be properly cleared, both calls should use matching options.

♻️ Suggested improvement
- const error = query.error
-
- // user cancelled explicitly
- if (error === 'access_denied') {
-   const returnToURL = getCookie(event, 'auth_return_to') || '/'
-   deleteCookie(event, 'auth_return_to', { path: '/' })
-   return sendRedirect(event, returnToURL)
- }
+ const error = query.error?.toString()
+
+ // user cancelled explicitly
+ if (error === 'access_denied') {
+   const returnToURL = getCookie(event, 'auth_return_to') || '/'
+   deleteCookie(event, 'auth_return_to')
+   return sendRedirect(event, returnToURL)
+ }

96-101: Consider setting an explicit path when creating the cookie.

The auth_return_to cookie is set here without an explicit path option (which typically defaults to the current request path), but it is deleted at line 78 with { path: '/' } and at line 169 without a path. For consistent behaviour, consider explicitly setting path: '/' when creating the cookie, ensuring it can be reliably read and deleted regardless of which endpoint handles it.

♻️ Suggested improvement
   setCookie(event, 'auth_return_to', redirectPath, {
     maxAge: 60 * 5,
     httpOnly: true,
     // secure only if NOT in dev mode
     secure: !import.meta.dev,
+    path: '/',
   })

168-169: Ensure deleteCookie options match the cookie creation.

If the cookie was set with a specific path (or if you adopt the suggestion to explicitly set path: '/'), this deletion should include the same path option to guarantee the cookie is cleared correctly.

♻️ Suggested improvement
  const returnToURL = getCookie(event, 'auth_return_to') || '/'
- deleteCookie(event, 'auth_return_to')
+ deleteCookie(event, 'auth_return_to', { path: '/' })

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

This change adds error handling to the OAuth callback endpoint for the atproto authentication flow. When the OAuth provider returns an access_denied error via query parameter, the endpoint now redirects the user to the URL stored in the auth_return_to cookie, defaulting to "/" if the cookie is absent. The cookie is cleared before the redirect occurs. This logic is executed as an early return prior to the existing code that processes the authorisation code.

Possibly related PRs

Suggested reviewers

  • danielroe
  • wojtekmaj
  • fatfingers23
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description accurately describes the changeset, explaining the OAuth cancellation handling and the graceful redirect mechanism implemented in the code changes.

✏️ 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

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
server/api/auth/atproto.get.ts (3)

73-80: Type safety: query.error could be an array.

The getQuery function from h3 can return arrays for repeated query parameters. While comparing an array to a string will safely return false, explicitly handling the type makes the code more robust and aligns with the coding guidelines for strict type safety.

Additionally, there's an inconsistency: deleteCookie here specifies { path: '/' } but the same deletion at line 169 omits this option. For the cookie to be properly cleared, both calls should use matching options.

♻️ Suggested improvement
- const error = query.error
-
- // user cancelled explicitly
- if (error === 'access_denied') {
-   const returnToURL = getCookie(event, 'auth_return_to') || '/'
-   deleteCookie(event, 'auth_return_to', { path: '/' })
-   return sendRedirect(event, returnToURL)
- }
+ const error = query.error?.toString()
+
+ // user cancelled explicitly
+ if (error === 'access_denied') {
+   const returnToURL = getCookie(event, 'auth_return_to') || '/'
+   deleteCookie(event, 'auth_return_to')
+   return sendRedirect(event, returnToURL)
+ }

96-101: Consider setting an explicit path when creating the cookie.

The auth_return_to cookie is set here without an explicit path option (which typically defaults to the current request path), but it is deleted at line 78 with { path: '/' } and at line 169 without a path. For consistent behaviour, consider explicitly setting path: '/' when creating the cookie, ensuring it can be reliably read and deleted regardless of which endpoint handles it.

♻️ Suggested improvement
   setCookie(event, 'auth_return_to', redirectPath, {
     maxAge: 60 * 5,
     httpOnly: true,
     // secure only if NOT in dev mode
     secure: !import.meta.dev,
+    path: '/',
   })

168-169: Ensure deleteCookie options match the cookie creation.

If the cookie was set with a specific path (or if you adopt the suggestion to explicitly set path: '/'), this deletion should include the same path option to guarantee the cookie is cleared correctly.

♻️ Suggested improvement
  const returnToURL = getCookie(event, 'auth_return_to') || '/'
- deleteCookie(event, 'auth_return_to')
+ deleteCookie(event, 'auth_return_to', { path: '/' })

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

This change adds error handling to the OAuth callback endpoint for the atproto authentication flow. When the OAuth provider returns an access_denied error via query parameter, the endpoint now redirects the user to the URL stored in the auth_return_to cookie, defaulting to "/" if the cookie is absent. The cookie is cleared before the redirect occurs. This logic is executed as an early return prior to the existing code that processes the authorisation code.

Possibly related PRs

Suggested reviewers

  • danielroe
  • wojtekmaj
  • fatfingers23
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description accurately describes the changeset, explaining the OAuth cancellation handling and the graceful redirect mechanism implemented in the code changes.

✏️ 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

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
server/api/auth/atproto.get.ts (3)

73-80: Type safety: query.error could be an array.

The getQuery function from h3 can return arrays for repeated query parameters. While comparing an array to a string will safely return false, explicitly handling the type makes the code more robust and aligns with the coding guidelines for strict type safety.

Additionally, there's an inconsistency: deleteCookie here specifies { path: '/' } but the same deletion at line 169 omits this option. For the cookie to be properly cleared, both calls should use matching options.

♻️ Suggested improvement
- const error = query.error
-
- // user cancelled explicitly
- if (error === 'access_denied') {
-   const returnToURL = getCookie(event, 'auth_return_to') || '/'
-   deleteCookie(event, 'auth_return_to', { path: '/' })
-   return sendRedirect(event, returnToURL)
- }
+ const error = query.error?.toString()
+
+ // user cancelled explicitly
+ if (error === 'access_denied') {
+   const returnToURL = getCookie(event, 'auth_return_to') || '/'
+   deleteCookie(event, 'auth_return_to')
+   return sendRedirect(event, returnToURL)
+ }

96-101: Consider setting an explicit path when creating the cookie.

The auth_return_to cookie is set here without an explicit path option (which typically defaults to the current request path), but it is deleted at line 78 with { path: '/' } and at line 169 without a path. For consistent behaviour, consider explicitly setting path: '/' when creating the cookie, ensuring it can be reliably read and deleted regardless of which endpoint handles it.

♻️ Suggested improvement
   setCookie(event, 'auth_return_to', redirectPath, {
     maxAge: 60 * 5,
     httpOnly: true,
     // secure only if NOT in dev mode
     secure: !import.meta.dev,
+    path: '/',
   })

168-169: Ensure deleteCookie options match the cookie creation.

If the cookie was set with a specific path (or if you adopt the suggestion to explicitly set path: '/'), this deletion should include the same path option to guarantee the cookie is cleared correctly.

♻️ Suggested improvement
  const returnToURL = getCookie(event, 'auth_return_to') || '/'
- deleteCookie(event, 'auth_return_to')
+ deleteCookie(event, 'auth_return_to', { path: '/' })

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

@codecov
Copy link

codecov bot commented Feb 9, 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!

@vercel
Copy link

vercel bot commented Feb 9, 2026

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

Project Deployment Actions Updated (UTC)
docs.npmx.dev Error Error Feb 9, 2026 5:00pm
npmx.dev Error Error Feb 9, 2026 5:00pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
npmx-lunaria Ignored Ignored Feb 9, 2026 5:00pm

Request Review

@codecov
Copy link

codecov bot commented Feb 9, 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!

@danielroe danielroe enabled auto-merge February 9, 2026 16:44
Comment on lines +73 to +76
const error = query.error

// user cancelled explicitly
if (error === 'access_denied') {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const error = query.error
// user cancelled explicitly
if (error === 'access_denied') {
// user cancelled explicitly
if (query.error === 'access_denied') {

@danielroe danielroe added this pull request to the merge queue Feb 9, 2026
Merged via the queue into npmx-dev:main with commit 8252800 Feb 9, 2026
14 of 27 checks passed
@NandkishorJadoun NandkishorJadoun deleted the fix/handle-oauth-cancel branch February 10, 2026 01:28
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