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

NEXTAUTH_URL gets overwritten on Vercel and it breaks the deployment #6763

Closed
maxprilutskiy opened this issue Feb 20, 2023 · 12 comments · Fixed by #6814
Closed

NEXTAUTH_URL gets overwritten on Vercel and it breaks the deployment #6763

maxprilutskiy opened this issue Feb 20, 2023 · 12 comments · Fixed by #6814
Labels
accepted The feature request is considered. If nobody works actively on it, feel free to.

Comments

@maxprilutskiy
Copy link

Environment

  System:
    OS: macOS 13.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 285.25 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.14.0 - ~/.n/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 9.3.1 - ~/.n/bin/npm
    Watchman: 2023.02.06.00 - /opt/homebrew/bin/watchman
  Browsers:
    Chrome: 110.0.5481.100
    Safari: 16.2

Reproduction URL

https://vercel.com

Describe the issue

I'm deploying nextjs app with nextauth to Vercel, and NEXTAUTH_URL gets overwritten. Usually, it wouldn't be a problem: according to the code I was able to find, next-auth gets the host value from the Vercel's system env variables, uses it, and everything works.

However... 👇

How to reproduce

In my case, I'm using zones i.e. proxies, to host one application (the landing) from /, and another application (the actual app with auth) from /app. And in this case, Vercel injects through its env variables not the host user requested, but rather the host the app is deployed at.

And it breaks the whole set up.

I was able to fix the issue locally, by using patch-package, here's the patch:

diff --git a/node_modules/next-auth/.DS_Store b/node_modules/next-auth/.DS_Store
new file mode 100644
index 0000000..e69de29
diff --git a/node_modules/next-auth/utils/detect-host.js b/node_modules/next-auth/utils/detect-host.js
index 59f70f4..c6351d4 100644
--- a/node_modules/next-auth/utils/detect-host.js
+++ b/node_modules/next-auth/utils/detect-host.js
@@ -8,6 +8,10 @@ exports.detectHost = detectHost;
 function detectHost(forwardedHost) {
   var _process$env$VERCEL;
 
-  if ((_process$env$VERCEL = process.env.VERCEL) !== null && _process$env$VERCEL !== void 0 ? _process$env$VERCEL : process.env.AUTH_TRUST_HOST) return forwardedHost;
+  if (!process.env.NEXTAUTH_URL) {
+    if ((_process$env$VERCEL = process.env.VERCEL) !== null && _process$env$VERCEL !== void 0 ? _process$env$VERCEL : process.env.AUTH_TRUST_HOST) {
+      return forwardedHost;
+    }
+  }
   return process.env.NEXTAUTH_URL;
 }
\ No newline at end of file

As you see, I'm adding a condition that prevents the host from being overwritten if the NEXTAUTH_URL was set by the user.

Expected behavior

I expect next-auth to not overwrite the host with value provided by Vercel (or whomever else), when NEXTAUTH_URL is explicitly set by the user.

@maxprilutskiy maxprilutskiy added the triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. label Feb 20, 2023
@maxprilutskiy maxprilutskiy changed the title NEXTAUTH gets overwritten on Vercel and it breaks the deployment NEXTAUTH_URL gets overwritten on Vercel and it breaks the deployment Feb 20, 2023
@balazsorban44
Copy link
Member

Thanks, this recently came up. See also: #6647 and #3944 (reply in thread)

You don't actually need patch-package, here is a cleaner solution:

#6526 (comment)

I'll keep this open until we decide if we want to prefer NEXTAUTH_URL again on Vercel, or have a different way to configure this. cc @ThangHuuVu for thoughts. 🙏

@balazsorban44 balazsorban44 added accepted The feature request is considered. If nobody works actively on it, feel free to. and removed triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Feb 22, 2023
@maxprilutskiy
Copy link
Author

Hi @balazsorban44 - thanks so much for the reply! Good to know I can get rid of patch-package here, thanks. 💯

I've checked the links you've shared, and I totally agree with you - it might break the already deployed apps. That's a very good point indeed, and we should avoid that as much as possible.

Even though in my opinion it was a bad decision in the first place, to make it behave in such a different way when deployed, and especially coupling the code with the vendor that code could be deployed to (Vercel) - we have what we have, and should focus on what it's best to do next.

Here are my observations.

If the majority benefits from the current setup, if for the most of the users it's more beneficial and effortless to allow Vercel config to take precedence over NEXTAUTH_URL - we could leave it as it is, at least for the time being until an ultimately better solution is found. However, I believe for advanced cases, there must be a way to make it work without the hacks like the one you've shared or my dirty approach with patch-package above. There're two options I can see possible here:

  • Introduce the NEXTAUTH_URL_OVERRIDE flag, a falsy value of which would allow NEXTAUTH_URL to have priority over configs of platforms like Vercel (or potentially others too). This solution is good as it solves the problem without requiring any hacks in the code, but I don't quite like it since it introduces yet another env variable.
  • Introduce the auth.config.js config file (cosmiconfig could fit very nicely here), which would keep this setting inside: const config = { urlOverride: false };. This solution is good as it solves the problem pretty elegantly and doesn't require any config vars (while requiring to introduce an auth config file though - but it most likely will be needed anyways in the future as the project becomes even more mature).

Let me know what you think.

@ThangHuuVu
Copy link
Member

Thanks for your agreeable suggestions @maxprilutskiy. My takes:

  • For next-auth let's not break existing deployments by changing the behavior.
    • I'm OK with adding a new environment variable to solve the issue. This will be added under our "Advanced Guide" and not all people will need to know about this variable so I think it's reasonable.
    • I don't think adding a new auth.config.js file is good, since next-auth is already a legacy package and we don't want to backport too many features to it, only the absolutely necessary parts like security updates etc.
  • Moving forward, in our new package @auth/nextjs we can flip the behavior to prioritize NEXTAUTH_URL when it's set and avoid having this configuration. We need to document the migration process properly so folks won't miss this bit. (Or we might not need NEXTAUTH_URL at all, I don't know yet)

If we go with the new environment variable, lets decide what we should call it:

  • NEXTAUTH_URL_INTERNAL maybe we can reuse this env? Currently, it's only used in the client code
  • NEXTAUTH_URL_OVERRIDE
  • NEXTAUTH_URL_CUSTOM
  • NEXTAUTH_URL_CANONICAL
  • ... others

@maxprilutskiy
Copy link
Author

@ThangHuuVu, that's great news you're considering flipping the behavior in the @auth/ versions!

For this legacy package, I agree env variable would be a good solution, given all the context. I think we could indeed reuse NEXTAUTH_URL_INTERNAL here, but I personally never used it / dug into the specifics of its implementation, so I might be missing the full picture here.

So if you folks believe it's gonna be fine - I'd say let's go for it! 👍

@balazsorban44
Copy link
Member

balazsorban44 commented Feb 23, 2023

I'm voting for NEXTAUTH_URL_INTERNAL too, so we don't introduce yet another environment variable.

And look! There's been a PR for it for a while 🙈 #5679 It has conflicts though and not opened against the v4 branch, but the approach could be similar.

Here is the relevant place to add this:

// If we detect a Vercel environment, we can trust the host
if (process.env.VERCEL ?? process.env.AUTH_TRUST_HOST)
return forwardedHost
// If `NEXTAUTH_URL` is `undefined` we fall back to "http://localhost:3000"
return process.env.NEXTAUTH_URL

@magicspon
Copy link

@balazsorban44 do you have any idea when this might be released/merged? I have potentially 1.7 million users that would benefit!! 😂

@balazsorban44
Copy link
Member

balazsorban44 commented Feb 25, 2023

@magicspon Sorry to be a blocker, but that's not how OSS works 🙂. You can either use patch-package, #6763 (comment) , or sponsor the project on behalf of your 1.7 million users... 😉

@fforootd
Copy link

fforootd commented Mar 3, 2023

This sounds a lot like #4507 😁

@balazsorban44
Copy link
Member

#6814 has been released, so you can set NEXTAUTH_URL_INTERNAL on Vercel now! Please upgrade to at least 4.20.1 and let us know. :blob_pray:

@fforootd
Copy link

fforootd commented Mar 6, 2023

#6814 has been released, so you can set NEXTAUTH_URL_INTERNAL on Vercel now! Please upgrade to at least 4.20.1 and let us know. :blob_pray:

I can confirm that this works 😁 Thank you 🥳

@maxprilutskiy
Copy link
Author

Great job everyone! 💪 💪 💪

@jvineveld
Copy link

Anybody aware of the issues this has created for allot of users?
#6949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted The feature request is considered. If nobody works actively on it, feel free to.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants