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

Dynamic NEXTAUTH_URL #969

Closed
jasonkuhrt opened this issue Dec 18, 2020 · 26 comments
Closed

Dynamic NEXTAUTH_URL #969

jasonkuhrt opened this issue Dec 18, 2020 · 26 comments
Labels
enhancement New feature or request stale Did not receive any activity for 60 days

Comments

@jasonkuhrt
Copy link

Summary of proposed feature

Allow callback URL (NEXTAUTH_URL) to be set dynamically

Purpose of proposed feature
A clear and concise description description of why this feature is necessary and what problems it solves.

A static NEXTAUTH_URL is problematic for preview deployments like that offered by Vercel or any other platform/setup with similar concept.

It is problematic because a preview deployment URL will be, by definition, dynamic.

There are actually two sides to the problem:

  1. Callback URL used by Nextauth
  2. Callback URLs registered by Provider (e.g. GitHub App)

This issue is only about Problem 1 above.

To give more context, I tried mapping VERCEL_URL to NEXTAUTH_URL but that failed because it used the unique preview domains, not the ones we registered in our GitHub app callback configuration. Similar report of this problem can be found here vercel/vercel#5230.

not the ones we registered in our GitHub app callback configuration

In our case, our solution is to have extra domains like <project name>-branch-<#>.<org>.vercal.app. All of these have been registered in our GitHub app. When a team member is working on a branch of the app, they one of those domains in Vercel to point to their branch (yes, potentially requires some coordination with team, minor though).

Hope that makes sense :)

Detail about proposed feature

Our solution currently looks like as follows.

Before invoking NextAuth we dynamically set the NEXTAUTH_URL based on the request host. The protocol is https in production, otherwise http (e.g. on developer's machine).

import { NextApiRequest, NextApiResponse } from "next";
import NextAuth, { InitOptions } from "next-auth";
import Providers from "next-auth/providers";

const options: InitOptions = {
  providers: [
    Providers.GitHub({
      clientId: process.env.AUTH_GITHUB_CLIENT_ID!,
      clientSecret: process.env.AUTH_GITHUB_CLIENT_SECRET!,
    }),
  ],
};

export default (req: NextApiRequest, res: NextApiResponse) => {
  setNextAuthUrl(req);
  NextAuth(req, res, options);
};

/**
 * Setup the NEXTAUTH_URL envar based on the request header.
 */
function setNextAuthUrl(req: NextApiRequest) {
  const protocol = process.env.NODE_ENV === "production" ? "https" : "http";
  const host = req.headers["host"];

  if (!host) {
    throw new Error(
      `The request has no host header which breaks authentication and authorization.`
    );
  }

  process.env.NEXTAUTH_URL = `${protocol}://${host}`;

  console.log("set envar NEXTAUTH_URL=%s", process.env.NEXTAUTH_URL);
}

What I would like to propose is that NextAuth allow callbackURL to be passed as an optional configuration so that:

  1. NextAuth doesn't have to warn about missing envar
  2. User gets an explicit contract to work with. For example our current solution would break if the NextAuth codebase changed to read the envar eagerly instead of lazily like it does now.

Like so:

import { NextApiRequest, NextApiResponse } from "next";
import NextAuth, { InitOptions } from "next-auth";
import Providers from "next-auth/providers";

const options: InitOptions = {
  providers: [
    Providers.GitHub({
      clientId: process.env.AUTH_GITHUB_CLIENT_ID!,
      clientSecret: process.env.AUTH_GITHUB_CLIENT_SECRET!,
    }),
  ],
};

export default (req: NextApiRequest, res: NextApiResponse) => {
  const callbackURL = getCallbackUrl(req);
  NextAuth(req, res, { ...options, callbackURL });
};

/**
 * Setup the NEXTAUTH_URL envar based on the request header.
 */
function getCallbackUrl(req: NextApiRequest) {
  let protocol = process.env.NODE_ENV === "production" ? "https" : "http";
  const host = req.headers["host"];

  if (!host) {
    throw new Error(
      `The request has no host header which breaks authentication and authorization.`
    );
  }

  return `${protocol}://${host}`;
}

Another proposal I would like to suggest is a new default when no environment variable has been set AND no config has been passed. Then the default should be:

  1. use https if production otherwise http
  2. get the host from the request
  3. concat together to form the callback url

In many cases I think this default will be right, which is what a good default should be.

Then we can reduce our implementation back to just this:

import { NextApiRequest, NextApiResponse } from "next";
import NextAuth, { InitOptions } from "next-auth";
import Providers from "next-auth/providers";

const options: InitOptions = {
  providers: [
    Providers.GitHub({
      clientId: process.env.AUTH_GITHUB_CLIENT_ID!,
      clientSecret: process.env.AUTH_GITHUB_CLIENT_SECRET!,
    }),
  ],
};

export default (req: NextApiRequest, res: NextApiResponse) => {
  NextAuth(req, res, options);
};

Potential problems

The behaviour becomes a bit more variable so good docs would be needed (website and IMO JSDoc too).

Some kind of logging/debug logging might be worth considering.

Describe any alternatives you've considered
None

Please indicate if you are willing and able to help implement the proposed feature.
I am willing to submit a complete PR for all of this if agreed to.

@jasonkuhrt
Copy link
Author

@balazsorban44 I think this should be easy to implement. Can I get your blessing to take this on?

@iaincollins
Copy link
Member

Hi there!

Just jumping into say we shouldn't use req.headers["host"] as it's insecure unless it's being checked against a whitelist.

This was actually how v1 worked, as it simplified configuration greatly. There problem is little obtuse (and apologies for not having time to dive into it) but it can be used, in some fairly particular circumstances, to facilitate a Denial of Service attack.

It's a bit long winded to explain, but there should be some background on this in old closed issues if you feel like digging around.

HOWEVER! Using the Host header in combination with something like domain option (which could be a string, or an array) and to check it is a whitelisted option is secure and could be relevant to @grikomsn RE: #1168.

@balazsorban44
Copy link
Member

The exact point I was going to make @iaincollins! Your comment popped up while I was writing almost exactly the same thing. 😅

@balazsorban44
Copy link
Member

balazsorban44 commented Feb 1, 2021

I believe #1123 is also trying to solve this in a different way? cc @gergelyke

Corresponsing issue: Allow server-side calls to use a different URL other than NEXTAUTH_URL #1121

@iaincollins
Copy link
Member

iaincollins commented Feb 1, 2021

@balazsorban44 Thanks! Was just writing a comment on that issue :D

I think there is some overlap, if folks could check out that PR and the issues linked from it and share thoughts on where they intersect that would be great.

We may not want to solve them all in one PR, but they do seem to overlap.

For consideration:

Having an option like domain: "example.com" would also be potentially really interesting for the future as we could leverage for cookie policy and/or CORS too, to make it easier to sign in and create a session that works on subdomains at the same domain, which I think a lot of folk would find helpful.

Beyond that, we also might want something like an explicit list of domains - domains: ["example.com", "www.example.com", "www.example.org"] - to support cross domain sign in. Given that's a larger bit of work it's seems reasonable to not worry about multiple top level domains the short term, but just mention it in case it helps folks coming up with ideas.

@jasonkuhrt
Copy link
Author

Thanks for the feedback @iaincollins & @balazsorban44!

@Robert-OP
Copy link

Robert-OP commented Mar 12, 2021

Hey guys, thank you for the support on this issue and hope you are well!

I forked the latest main and started building on top of it by adding parts from this discussion and issue #600 for adding multi-tenancy to the library (also related with #1156). Still need to add the actual whitelisting of domains as @iaincollins suggested.

Let me know what you think about it and if I am on a right path. Cheers

main...Robert-OP:feature/multi-tenant-ropo

UPDATE: posted a multi-tenant version (incomplete, work in progress) on npm that seems to work for my use case to some extent, still working on it and will improve. With this you need to set MULTI_TENANT="true" in environment variables (.env) such that it's picked up by the library and goes into multi-tenant mode. NOTE that the domains whitelisting still need to be included. Also, the following behaviors are not as intended and need more work:

  • user needs to press LogIn/SignIn again after switching domain
  • SignOut doesn't happen on all domains

(if you guys have any idea how to trigger an event that would SignIn or SignOut users from all domains)

@Robert-OP
Copy link

Robert-OP commented Mar 15, 2021

To continue on my last comment and coming back with some questions, any thoughts about:

  1. How to share the session across multiple domains after a user has SignedIn into one domain?
    (linked to this stack overflow question which @iaincollins responded to)

  2. How to SignOut and delete the session for all domains?

  3. Where in the next-auth codebase could 1. and 2. be solved? 🤔

Cheers

@dackers86
Copy link

dackers86 commented Mar 17, 2021

Thanks @Robert-OP.

I have a similar issue. We currently use NextJS for multiple domains + caching + SSR.
Credentials for each provider are fetched based on the request headers and then added as provider ids and secrets.

Currently, on the main branch and updating my callback redirect, this will not work as for the four requests I will receive something similar to...

mydomain.com //NEXTAUTH_URL. <---- Providers are set here and do not update on subsequent requests
otherDomain.com
otherDomain.com
otherDomain.com

callbacks: {
     async redirect(url, baseUrl) {
       return `${hostname}`;
     },
   },

Is there a possible short term / incremental solution to this

And also, should this be a PR on the repository?

Edit: Apologies, noticed it is a PR / forked repo already!

@ramiel
Copy link
Contributor

ramiel commented Mar 27, 2021

I think the domain whitelist is not a good solution because in vercel the dynamic urls are in the form "XYZ.vercel.app" so the only common part is vercel.app which is in common with any other application of all users on vercel. A different proposal can be: setup an env variable that allow to use req['host'], so everything will work in preview on vercel but not set it in production where the normal VERCEL_URL or the domain whitelist would work

@thulstrup
Copy link

The Vercel urls are using the format [app-id]-[random-id]-[team-id].vercel.app.
So wouldn't it be possible to create a domain whitelist like this https://myapp-*-myteam.vercel.app?

@ramiel
Copy link
Contributor

ramiel commented Mar 27, 2021

Yes, indeed, unless that domain whitelist should be used for the cookie as well, in that case it can be something like ".vercel.app". But probably it's not the case

@stale stale bot added the stale Did not receive any activity for 60 days label May 27, 2021
@nextauthjs nextauthjs deleted a comment from stale bot May 28, 2021
@stale
Copy link

stale bot commented Jun 4, 2021

Hi there! It looks like this issue hasn't had any activity for a while. To keep things tidy, I am going to close this issue for now. If you think your issue is still relevant, just leave a comment and I will reopen it. (Read more at #912) Thanks!

@stale stale bot closed this as completed Jun 4, 2021
@thachp
Copy link

thachp commented Jun 26, 2021

@jasonkuhrt I try implementing your solution .. it works to a point. I keep getting ERR_JWS_VERIFICATION_FAILED after successful sign-in at a provider. How did you resolve the error? Thx

image

@jasonkuhrt
Copy link
Author

jasonkuhrt commented Jun 26, 2021

Hey @thachp We don't use next-auth in the app this came from anymore so haven't stayed on top of this. Good luck!

@iaincollins
Copy link
Member

I think the domain whitelist is not a good solution because in vercel the dynamic urls are in the form "XYZ.vercel.app" so the only common part is vercel.app which is in common with any other application of all users on Vercel.

Having a domain is a reasonable baseline assumption here.

A different proposal can be: setup an env variable that allow to use req['host'], so everything will work in preview on vercel but not set it in production where the normal VERCEL_URL or the domain whitelist would work

Using host in this way is potential vector for DDOS attacks. There is some previous discussion of this in older issues.

The Vercel urls are using the format [app-id]-[random-id]-[team-id].vercel.app.
So wouldn't it be possible to create a domain whitelist like this https://myapp-*-myteam.vercel.app?

Technically yes but I don't think it makes sense for us to get into domain specific regexes. I think we should avoid esoteric solutions (which can introduce new factors) when simple solutions like having a domain are a cheap and secure option.

@ericvanular
Copy link

Custom domain multi-tenancy is a current use case for us, curious if any progress has been made to support cross-domain sessions?

@viperfx
Copy link

viperfx commented Dec 31, 2021

@jasonkuhrt would you be able to comment on which auth library you used for supporting your use cases? and how well has that worked?

@jasonkuhrt
Copy link
Author

@viperfx We built our own solution on top of PassportJS.

@herleraja
Copy link

PassportJS

jasonkuhrt any way can you share the passport solution? I was not able to make it work end to end.

@jasonkuhrt
Copy link
Author

Company has moved on yet again now to Auth0 (we also looked at Clerk but top down decision that it wasn't enterprise ready enough).

@herleraja
Copy link

Company has moved on yet again now to Auth0 (we also looked at Clerk but top down decision that it wasn't enterprise ready enough).

Oh okay. Let me give a try for auth0 too. According to you which is the best Library that support multi tenancy ?

@jasonkuhrt
Copy link
Author

Not sure which is better for that. Clerk was simpler we felt.

@vicmosin
Copy link

vicmosin commented Aug 26, 2023

@iaincollins I came to the same issue now and wanted to check if it's indeed possible to use header's host value and list of trusted domains proposed here: #969 (comment)
I also checked the changed proposed here, though they are too old: main...Robert-OP:feature/multi-tenant-ropo

So basically it seems like the main issue now that both signIn and signOut should have a baseUrl to perform requests to, but both of the methods have no access to request context meaning we can't really get the header's value. Any ideas how can we workaround it?

@vicmosin
Copy link

My another idea would be.. what if we still keep NEXTAUTH_URL as a default url, but each provider would provide a list of trusted URLs can be used in case of making calls to app itself. It will kinda similar to the oauth's redirectionUrl where each client can define the url, server must redirect to after success/failure

@MatteoGauthier
Copy link

Hey everyone, what is the status of this feature ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale Did not receive any activity for 60 days
Projects
None yet
Development

No branches or pull requests