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(core): cannot parse action at /session #10094

Merged
merged 9 commits into from Mar 14, 2024

Conversation

bwallberg
Copy link
Contributor

@bwallberg bwallberg commented Feb 21, 2024

☕️ Reasoning

According to the comment for createActionURL() it's supposed to build an URL in the same way for both with or withot ENV variables but currently it doesn't use the basePath provided.

This causes the createActionURL() to create an invalid action url for /session when one of the ENV variables are set.

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

Fixes #10081

Copy link

vercel bot commented Feb 21, 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 Mar 14, 2024 1:34am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Mar 14, 2024 1:34am

Copy link

vercel bot commented Feb 21, 2024

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

A member of the Team first needs to authorize it.

@bestickley
Copy link

Thank you for this fix, @bwallberg! I'm using it via PNPM patch for now.

@bestickley
Copy link

@oscarmylla, looking at the error message, I don't this is PR is related. It works without this PR?

@ThangHuuVu
Copy link
Member

hmm, I'm thinking about using createActionURL from the core and remove the function in next-auth instead - as the core function is intended to be used in framework packages. The fix should therefore be in the core's function. Would you be able to update the PR with my suggestion? 🙏

@ndom91
Copy link
Member

ndom91 commented Mar 2, 2024

Also @bwallberg regarding your comment here, the goal is not to require the basePath on the AUTH_URL (like http://localhost:3000/api/auth), quite the contrary, we'd like to continue supporting the previous env vars everyone has already set that consist of only the host (i.e. AUTH_URL=http://localhost:3000).

That being said, what we're all getting at, I think, is just to expand this fix to the other fraemwork libraries as well (like @auth/sveltekit) 😊

So long story short, that'd look like this imo:

  1. There's a "sister" createActionURL in core at packages/core/src/lib/utils/env.ts:47 that's meant to replace the framework specific ones, but just hasn't been done yet. Could you apply your change there instead?
  2. Then we can replace the next-auth and @auth/sveltekit's own createActionURL with this updated core one. Basically these two need replaced:
    • SvelteKit version: packages/frameworks-sveltekit/src/lib/actions.ts:135)
    • next-auth version: packages/next-auth/src/lib/actions.ts:118 (this is the one you updated already)

@bwallberg
Copy link
Contributor Author

Also @bwallberg regarding your comment here, the goal is not to require the basePath on the AUTH_URL (like http://localhost:3000/api/auth), quite the contrary, we'd like to continue supporting the previous env vars everyone has already set that consist of only the host (i.e. AUTH_URL=http://localhost:3000).

That being said, what we're all getting at, I think, is just to expand this fix to the other fraemwork libraries as well (like @auth/sveltekit) 😊

So long story short, that'd look like this imo:

1. There's a "sister" `createActionURL` in `core` at `packages/core/src/lib/utils/env.ts:47` that's meant to replace the framework specific ones, but just hasn't been done yet. Could you apply your change there instead?

2. Then we can replace the `next-auth` and `@auth/sveltekit`'s own `createActionURL` with this updated `core` one. Basically these two need replaced:
   
   * SvelteKit version: `packages/frameworks-sveltekit/src/lib/actions.ts:135`)
   * next-auth version: `packages/next-auth/src/lib/actions.ts:118` (this is the one you updated already)

Thanks for clarifying. Sounds very reasonable :)

@bwallberg
Copy link
Contributor Author

@ThangHuuVu @ndom91

I believe I've updated the PR as requested. I've also updated the unit-test to reflect the expectations, which is that the basePath should be set in the config and not in the env variable. I believe setting a custom one only in AUTH_URL would break everything regardless.

I've tested the solution in with next.js but not sveltekit, if you approve of the solution I'll setup the local dev app to test it out.

@ndom91
Copy link
Member

ndom91 commented Mar 4, 2024

Awesome, thanks a lot! I'll take a look shortly.

Regarding the basePath, yeah it's a bit tricky with SvelteKit because the svelte.config.js basePath is designed to be the basePath for your entire application, right. Like if your app was at a sub path of your entire organizations site (like https://company.com/projectMgmtApp/*). And the auth.ts config basePath / AUTH_URL url path is for your auth route only 🤔

So long short short, as a user, if i had set svelte.config.js basePath: "/app" and the auth.ts config basePath: "/auth", I'd expect my whole app to be at company.com/app/* and my auth URLs to be at company.com/app/auth/*, i.e. the auto-generated signin page at compny.com/app/auth/signin

@bwallberg
Copy link
Contributor Author

Awesome, thanks a lot! I'll take a look shortly.

Regarding the basePath, yeah it's a bit tricky with SvelteKit because the svelte.config.js basePath is designed to be the basePath for your entire application, right. Like if your app was at a sub path of your entire organizations site (like https://company.com/projectMgmtApp/*). And the auth.ts config basePath / AUTH_URL url path is for your auth route only 🤔

So long short short, as a user, if i had set svelte.config.js basePath: "/app" and the auth.ts config basePath: "/auth", I'd expect my whole app to be at company.com/app/* and my auth URLs to be at company.com/app/auth/*, i.e. the auto-generated signin page at compny.com/app/auth/signin

Yeah I see, that makes sense 🤔

@bwallberg
Copy link
Contributor Author

@ndom91 I believe I've solved the use-case you've described as well now.

@ThangHuuVu
Copy link
Member

@bwallberg could you separate the package changes in different PRs - one for core, one for next-auth, and one for sveltekit? just to keep things tidy and the git history clean. Sorry I should've mentioned this earlier. You can keep this PR for core and create new PRs for framework packages. Many thanks!

@ndom91
Copy link
Member

ndom91 commented Mar 4, 2024

@bwallberg thanks, changes in general look good so far! 👍

Was just talking with Thang and thinking about how to avoid foot guns of people misconfiguring this since there are quite a few different permutations that are supported now..

Note that basePath will default to /api/auth in next-auth and /auth with all other frameworks if its not set explicitly in the config.

We basically have the following variations we currently support / see used:

  1. AUTH_URL with and without an actual application base-path (i.e. /my-app)
  2. AUTH_URL with and without the auth.js path attached (i.e. /api/auth)
  3. Then both of those with basePath set to various things in the configuration

Written out, some of those options look like this:

  1. AUTH_URL=http://localhost:3000 & basePath: '/api/auth'
  2. AUTH_URL=http://localhost:3000/api/auth & basePath: '/api/auth'
  3. AUTH_URL=http://localhost:3000/my-app/api/auth & basePath: '/api/auth'
  4. AUTH_URL=http://localhost:3000/ & basePath: '/my-app/api/auth'

One idea - we should encourage AUTH_URL to be only the root domain and/or root domain + application base path (/my-app) if applicable, and then the configuration basePath is the route to your [...nextauth]/route.ts file/path. That's pretty easy to explain and reason about too imo. Also AUTH_URL sounds kinda like its the URL to the auth path, but actually its just URL since we prefixed everything with AUTH_, so if yuo think about it, its just the URL env var which should point at the root of your application

I know many people will probably have something else already configured though because we support / supported many things.

Imo to avoid more complexity here, we should pick a "golden path", whatever that looks like, and encourage that everywhere and document it well, and then throw errors / log warnings when folks are setting them to something unsupported. What do you all think?

@bwallberg
Copy link
Contributor Author

@bwallberg thanks, changes in general look good so far! 👍

Was just talking with Thang and thinking about how to avoid foot guns of people misconfiguring this since there are quite a few different permutations that are supported now..

Note that basePath will default to /api/auth in next-auth and /auth with all other frameworks if its not set explicitly in the config.

We basically have the following variations we currently support / see used:

1. `AUTH_URL` with and without an actual application base-path (i.e. `/my-app`)

2. `AUTH_URL` with and without the auth.js path attached (i.e. `/api/auth`)

3. Then both of those with `basePath` set to various things in the configuration

Written out, some of those options look like this:

1. `AUTH_URL=http://localhost:3000` & `basePath: '/api/auth'`

2. `AUTH_URL=http://localhost:3000/api/auth` & `basePath: '/api/auth'`

3. `AUTH_URL=http://localhost:3000/my-app/api/auth` & `basePath: '/api/auth'`

4. `AUTH_URL=http://localhost:3000/` & `basePath: '/my-app/api/auth'`

One idea - we should encourage AUTH_URL to be only the root domain and/or root domain + application base path (/my-app) if applicable, and then the configuration basePath is the route to your [...nextauth]/route.ts file/path. That's pretty easy to explain and reason about too imo. Also AUTH_URL sounds kinda like its the URL to the auth path, but actually its just URL since we prefixed everything with AUTH_, so if yuo think about it, its just the URL env var which should point at the root of your application

I know many people will probably have something else already configured though because we support / supported many things.

Imo to avoid more complexity here, we should pick a "golden path", whatever that looks like, and encourage that everywhere and document it well, and then throw errors / log warnings when folks are setting them to something unsupported. What do you all think?

I agree to me that felt intuitive that AUTH_URL represented the root of the app itself and config.basePath the route of the auth.

I think it's best to avoid multiple sources of truths that needs to align ( eg. AUTH_URL=http://localhost:3000/api/auth & basePath: '/api/auth' what if basePath was configured to /auth/api ) and keep one config for the root of the app and one for the basePath of the api.

There is a caveat here though that if AUTH_URL is not provided the basePath must be configured with the application base-path as well. Which of course is how it already works, but good to keep in mind when documenting.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 97.22222% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 39.36%. Comparing base (bd57d22) to head (b23bee9).

Files Patch % Lines
packages/core/src/lib/utils/env.ts 96.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10094      +/-   ##
==========================================
+ Coverage   39.29%   39.36%   +0.06%     
==========================================
  Files         171      171              
  Lines       27260    27288      +28     
  Branches     1154     1165      +11     
==========================================
+ Hits        10712    10742      +30     
+ Misses      16548    16546       -2     

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

@KaygNas
Copy link

KaygNas commented Mar 7, 2024

I self host my website behind a reverse proxy

When I go to mydomain.com/api/auth/providers all callback urls point to localhost:3000

@DuarteMartinho I met the similar situation when using 127.0.0.1:3000 to access my website. In my situation, Next always replace 127.0.0.1 to localhost, not sure whether it's caused by the same reason, but may you can checkout this.

https://github.com/vercel/next.js/blob/5740ef3e1e53c04231f8996ab737738e9b02e4b4/packages/next/src/server/web/next-url.ts#L25

@CyanFlare
Copy link

Hello,

I've recently upgraded from next-auth@5.0.0-beta.4 to next-auth@5.0.0-beta.15 and have ran into some issues that I think is related to this and I'm hoping someone can assist me with solving.

In the next-auth@5.0.0-beta.6 release notes, there was a line that says next-auth basePath should be set to /api/auth by default.
I believe this may be the cause of the issues I'm currently facing.

The first issue I'm having is with the NEXTAUTH_URL env variable and the basePath auth.ts config option.
Setting NEXTAUTH_URL to http://localhost:3000 and basePath to /api/auth would result in a Cannot parse action at /session error. But setting NEXTAUTH_URL to http://localhost:3000/api/auth and leaving basePath unset would work perfectly fine but I think this is what lead to the second issue below.

The second issue I'm having is with the pages auth.ts config option.
In next-auth@5.0.0-beta.4 setting the pages error option to /auth/error would redirect to http://localhost:3000/auth/error but when I upgraded to next-auth@5.0.0-beta.15 and applied the fix in the first issue it would redirect to http://localhost:3000/api/auth/auth/error.

@DuarteMartinho
Copy link

DuarteMartinho commented Mar 8, 2024

@KaygNas @CyanFlare

Have a look at this I made some comments showing my errors and getting it fixed

Turns out for me the problem was my next was not updated

@CyanFlare
Copy link

@KaygNas @CyanFlare

Have a look at this I made some comments showing my errors and getting it fixed

Turns out for me the problem was my next was not updated

My next version is 14.1.3.
I solved the first issue by removing NEXTAUTH_URL env variable completely as apparently it is no longer needed. But I am still encountering the second issue.

packages/core/src/lib/utils/env.ts Outdated Show resolved Hide resolved
packages/core/src/lib/utils/env.ts Outdated Show resolved Hide resolved
packages/core/test/env.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@ThangHuuVu ThangHuuVu left a comment

Choose a reason for hiding this comment

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

great work @bwallberg ! I just push a commit to refactor the warning to a code, using the logger instance. I will also follow up with a doc PR to explain these code. LGTM

@ThangHuuVu ThangHuuVu merged commit 4a2d511 into nextauthjs:main Mar 14, 2024
13 of 15 checks passed
JipSterk pushed a commit to JipSterk/next-auth that referenced this pull request Apr 3, 2024
* fix(core): cannot parse action at /session

fix: support apps hosted on subpaths

* refactor(core): use const instead of let for detected host & protocol

* fix(core): warn if basePath & AUTH_URL path are both provided

* refactor: update logger

---------

Co-authored-by: Nico Domino <yo@ndo.dev>
Co-authored-by: Thang Vu <hi@thvu.dev>
JipSterk pushed a commit to JipSterk/next-auth that referenced this pull request Apr 3, 2024
* fix(core): cannot parse action at /session

fix: support apps hosted on subpaths

* refactor(core): use const instead of let for detected host & protocol

* fix(core): warn if basePath & AUTH_URL path are both provided

* refactor: update logger

---------

Co-authored-by: Nico Domino <yo@ndo.dev>
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
Labels
core Refers to `@auth/core`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autodetect protocol+host behind proxy without AUTH_URL env variable
7 participants