Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(next-auth): redirecting to /undefined when signIn method throws an error #11010

Conversation

andresgutgon
Copy link
Contributor

@andresgutgon andresgutgon commented May 25, 2024

☕️ Reasoning

When using a custom signin page in a NextJS app and using a server action as explained in the docs:
https://authjs.dev/guides/pages/signin

When the auth fails. For example error=ConfigurationError it doesn't redirect to error page. It redirects to /undefined

The problem

I think that @auth/core Auth function can return an InternalResponse or a standard web Response. Internal response has a redirect attribute. The problem is that the logic inside Auth method when there is an error returns a Standard web Response that doesn't have redirect attribute but we can get a redirecting url by using headers.get('Location').

What changes are being made? What feature/bug is being fixed here?

We check if the returned response is a next auth InternalResponse or a standard web Response. This way we always get redirecting URL.

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

Fixes: #11008

Copy link

vercel bot commented May 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 1, 2024 4:35am
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Jun 1, 2024 4:35am
proxy ⬜️ Ignored (Inspect) Visit Preview Jun 1, 2024 4:35am

Copy link

vercel bot commented May 25, 2024

@andresgutgon is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.

@ThangHuuVu
Copy link
Member

thanks @andresgutgon , could you help to add a test for this so that we don't regress? the change LGTM otherwise.

Copy link

socket-security bot commented May 26, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@andresgutgon
Copy link
Contributor Author

andresgutgon commented May 26, 2024

What did I do to get mad at the bot? 😂 . Also why tests are not running?

@andresgutgon andresgutgon force-pushed the fix/redirect-to-undefined-using-next-auth-with-server-actions branch from fb1b9f9 to d74e166 Compare May 26, 2024 15:11
@github-actions github-actions bot added adapters Changes related to the core code concerning database adapters core Refers to `@auth/core` labels May 26, 2024
@andresgutgon andresgutgon force-pushed the fix/redirect-to-undefined-using-next-auth-with-server-actions branch from d74e166 to f9b1a41 Compare May 26, 2024 15:19
@andresgutgon andresgutgon force-pushed the fix/redirect-to-undefined-using-next-auth-with-server-actions branch from f9b1a41 to 744ad4f Compare May 26, 2024 15:49
@andresgutgon andresgutgon force-pushed the fix/redirect-to-undefined-using-next-auth-with-server-actions branch from 744ad4f to cef0f19 Compare May 26, 2024 16:05
error
@auth/core `Auth` function can return an internalResponse or a Standard
Response. Internal response has a `redirect` attribute. The problem is
that the logic inside `Auth` method when there is an error is returning
a Standard web Response which doesn't have `redirect` attribute but we
can get redirecting url by using `headers.get('Origin')`.

I think this fix this issue:
nextauthjs#11008
@andresgutgon andresgutgon force-pushed the fix/redirect-to-undefined-using-next-auth-with-server-actions branch from cef0f19 to 5cebb45 Compare May 26, 2024 16:07
@github-actions github-actions bot removed the core Refers to `@auth/core` label May 26, 2024
@andresgutgon
Copy link
Contributor Author

@ThangHuuVu I think I did tests and docs. I can see docs in my machine working and the tests I did are passing too.

Thanks for this software and glad to help!

@andresgutgon
Copy link
Contributor Author

andresgutgon commented May 27, 2024

@ThangHuuVu I think I reviewed all the comments. Let me know anything I need to change

@andresgutgon andresgutgon force-pushed the fix/redirect-to-undefined-using-next-auth-with-server-actions branch from 61abbe5 to 997aa02 Compare May 27, 2024 17:39
@andresgutgon andresgutgon force-pushed the fix/redirect-to-undefined-using-next-auth-with-server-actions branch from 997aa02 to 0d804ea Compare May 27, 2024 17:47
config
)
expect(redirectTo).toEqual(
"http://localhost/api/auth/signin/nodemailer?"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ThangHuuVu I made a test for that case. Is this what you meant?

@ndom91 ndom91 changed the title Fix NextAuth redirecting to /undefined when signIn method throws an error fix(next-auth): redirecting to /undefined when signIn method throws an error May 30, 2024
@andresgutgon
Copy link
Contributor Author

@ThangHuuVu please let me know if there's anything else I can do to move forward with these changes?

@ThangHuuVu ThangHuuVu self-requested a review June 1, 2024 04:06
Copy link

codecov bot commented Jun 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.85%. Comparing base (f6dbcd2) to head (1a4b78b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11010      +/-   ##
==========================================
+ Coverage   38.74%   40.85%   +2.11%     
==========================================
  Files         176      176              
  Lines       27887    27893       +6     
  Branches     1217     1242      +25     
==========================================
+ Hits        10804    11397     +593     
+ Misses      17083    16496     -587     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ThangHuuVu
Copy link
Member

thanks @andresgutgon , sorry I was absent for the week. I pushed a commit to simplify the tests.
IMO, we don't have to mock the Auth response with an empty location headers - the core tests should already ensure this case isn't happening. I'll merge this once the CI passes 🤞

@ThangHuuVu ThangHuuVu merged commit 8332c6f into nextauthjs:main Jun 1, 2024
14 of 15 checks passed
@andresgutgon andresgutgon deleted the fix/redirect-to-undefined-using-next-auth-with-server-actions branch June 1, 2024 07:48
hillac pushed a commit to hillac/next-auth that referenced this pull request Jun 19, 2024
…rows an error (nextauthjs#11010)

* Fix NextAuth redirecting to /undefined when signIn method throws an
error
@auth/core `Auth` function can return an internalResponse or a Standard
Response. Internal response has a `redirect` attribute. The problem is
that the logic inside `Auth` method when there is an error is returning
a Standard web Response which doesn't have `redirect` attribute but we
can get redirecting url by using `headers.get('Origin')`.

I think this fix this issue:
nextauthjs#11008

* Move action test to tests folder

* Remove test-adapter and add reference to docs

* Defend from undefined responseUrl

* Fix linter

* chore(test): simplify tests

---------

Co-authored-by: Thang Vu <hi@thvu.dev>
k3k8 pushed a commit to k3k8/next-auth that referenced this pull request Aug 1, 2024
…rows an error (nextauthjs#11010)

* Fix NextAuth redirecting to /undefined when signIn method throws an
error
@auth/core `Auth` function can return an internalResponse or a Standard
Response. Internal response has a `redirect` attribute. The problem is
that the logic inside `Auth` method when there is an error is returning
a Standard web Response which doesn't have `redirect` attribute but we
can get redirecting url by using `headers.get('Origin')`.

I think this fix this issue:
nextauthjs#11008

* Move action test to tests folder

* Remove test-adapter and add reference to docs

* Defend from undefined responseUrl

* Fix linter

* chore(test): simplify tests

---------

Co-authored-by: Thang Vu <hi@thvu.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v5] Throwing error with custom sign in page leads to undefined route
2 participants