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

feat: expo auth #720

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

feat: expo auth #720

wants to merge 19 commits into from

Conversation

juliusmarminge
Copy link
Member

@juliusmarminge juliusmarminge commented Nov 2, 2023

Closes #486

@juliusmarminge
Copy link
Member Author

juliusmarminge commented Nov 2, 2023

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @juliusmarminge and the rest of your teammates on Graphite Graphite

@juliusmarminge juliusmarminge mentioned this pull request Nov 2, 2023
@juliusmarminge juliusmarminge changed the base branch from 11-01-nativewind_v4 to main November 2, 2023 11:26
@shiroyasha9
Copy link

CleanShot 2024-05-12 at 00 01 32

Merged some stuff in and verified it still works.

But I really wanna find a fix so you don't have to have AUTH_URL='http://192.168.10.xxx:3000' or similar in .env...

Something like this doesn't work?

const isDev = process.env.NODE_ENV === 'development';
const { expoConfig } = Constants;

const baseUrl = isDev
    ? `http://${expoConfig!.hostUri?.split(':').shift()}:3000` : `https://your_real_url.com`

@juliusmarminge
Copy link
Member Author

The problem isnt on the expo side, it's on the next side. The URL must match for csrf reasons and since the expo app hits using the LAN IP, the server must accept that URL 😵‍💫😵‍💫

@Azzerty23
Copy link

Is setting the AUTH_URL env variable in a script a proper solution? (in apps/nextjs/package.json)
"dev": "AUTH_URL=http://$(ipconfig getifaddr en0):3000 pnpm with-env next dev" appears to work equally well.

@Wundero
Copy link

Wundero commented May 14, 2024

Is setting the AUTH_URL env variable in a script a proper solution? (in apps/nextjs/package.json) "dev": "AUTH_URL=http://$(ipconfig getifaddr en0):3000 pnpm with-env next dev" appears to work equally well.

This likely won't work on all developer's machines (I believe ipconfig is windows/mac, while ifconfig is linux - might be wrong about this). Personally not a fan of using platform-specific commands. Additionally, this would then cause issues if you use web since I think AUTH_URL should be localhost or 127.0.0.1 instead.

I think a more comprehensive solution would be to support multiple URLs on nextauth's side, or try to hack into nextauth a bit to check where the request comes from.

Another possible solution is to disable CSRF checks entirely, and then use a different mechanism for CSRF (particularly, checking origin/host headers) in a middleware. See this section on the OWASP cheatsheet for details on why this works. Per Lucia's docs, it can be implemented as such:

// middleware.ts
import { verifyRequestOrigin } from "lucia";
import { NextResponse } from "next/server";
import type { NextRequest } from "next/server";

export async function middleware(request: NextRequest): Promise<NextResponse> {
	if (request.method === "GET") {
		return NextResponse.next();
	}
	const originHeader = request.headers.get("Origin");
	// NOTE: You may need to use `X-Forwarded-Host` instead
	const hostHeader = request.headers.get("Host");
	if (!originHeader || !hostHeader || !verifyRequestOrigin(originHeader, [hostHeader])) {
		return new NextResponse(null, {
			status: 403
		});
	}
	return NextResponse.next();
}

Obviously, this uses a lucia utility, but the premise remains that this could supplant CSRF and thus fix the need for AUTH_URL.

The trusted URLs section of the call (in this case, the host header) can also just use hard-coded values or check against loopback addresses.

(For reference, this is the impl from lucia (technically oslo)):

export function verifyRequestOrigin(origin: string, allowedDomains: string[]): boolean {
	if (!origin || allowedDomains.length === 0) return false;
	const originHost = safeURL(origin)?.host ?? null;
	if (!originHost) return false;
	for (const domain of allowedDomains) {
		let host: string | null;
		if (domain.startsWith("http://") || domain.startsWith("https://")) {
			host = safeURL(domain)?.host ?? null;
		} else {
			host = safeURL("https://" + domain)?.host ?? null;
		}
		if (originHost === host) return true;
	}
	return false;
}

function safeURL(url: URL | string): URL | null {
	try {
		return new URL(url);
	} catch {
		return null;
	}
}

@Wundero
Copy link

Wundero commented May 15, 2024

Is setting the AUTH_URL env variable in a script a proper solution? (in apps/nextjs/package.json) "dev": "AUTH_URL=http://$(ipconfig getifaddr en0):3000 pnpm with-env next dev" appears to work equally well.

This likely won't work on all developer's machines (I believe ipconfig is windows/mac, while ifconfig is linux - might be wrong about this). Personally not a fan of using platform-specific commands. Additionally, this would then cause issues if you use web since I think AUTH_URL should be localhost or 127.0.0.1 instead.

I think a more comprehensive solution would be to support multiple URLs on nextauth's side, or try to hack into nextauth a bit to check where the request comes from.

Another possible solution is to disable CSRF checks entirely, and then use a different mechanism for CSRF (particularly, checking origin/host headers) in a middleware. See this section on the OWASP cheatsheet for details on why this works. Per Lucia's docs, it can be implemented as such:

// middleware.ts
import { verifyRequestOrigin } from "lucia";
import { NextResponse } from "next/server";
import type { NextRequest } from "next/server";

export async function middleware(request: NextRequest): Promise<NextResponse> {
	if (request.method === "GET") {
		return NextResponse.next();
	}
	const originHeader = request.headers.get("Origin");
	// NOTE: You may need to use `X-Forwarded-Host` instead
	const hostHeader = request.headers.get("Host");
	if (!originHeader || !hostHeader || !verifyRequestOrigin(originHeader, [hostHeader])) {
		return new NextResponse(null, {
			status: 403
		});
	}
	return NextResponse.next();
}

Obviously, this uses a lucia utility, but the premise remains that this could supplant CSRF and thus fix the need for AUTH_URL.

The trusted URLs section of the call (in this case, the host header) can also just use hard-coded values or check against loopback addresses.

(For reference, this is the impl from lucia (technically oslo)):

export function verifyRequestOrigin(origin: string, allowedDomains: string[]): boolean {
	if (!origin || allowedDomains.length === 0) return false;
	const originHost = safeURL(origin)?.host ?? null;
	if (!originHost) return false;
	for (const domain of allowedDomains) {
		let host: string | null;
		if (domain.startsWith("http://") || domain.startsWith("https://")) {
			host = safeURL(domain)?.host ?? null;
		} else {
			host = safeURL("https://" + domain)?.host ?? null;
		}
		if (originHost === host) return true;
	}
	return false;
}

function safeURL(url: URL | string): URL | null {
	try {
		return new URL(url);
	} catch {
		return null;
	}
}

As a followup to this, unfortunately CSRF isn't the only thing that doesn't work when you don't set the AUTH_URL. A couple of points I found:

  • OAuth callbacks will still point to localhost, and I can't seem to get the auth proxy to work with this either.
  • NextJS seems to infer the request.url property as localhost even when your origin is the 192.168.x.y ip, despite the request being made to the proper location. You can introspect the real host using the host header, and set the request url to use that, but that seems hacky at best, and doesn't really seem to cover all urls anyways

Ultimately, I have been unable to get local auth to work without the env var, and I don't want to try to hack nextauth to get it to work even though that seems like the only way this could be made to work.

I have had more success using Lucia and integrating auth, however Lucia has its own caveats:

  • Lucia's auth is far more involved, requiring you to implement all of these features by yourself:
    • Pages (e.g. sign in pages, sign out pages, etc.)
    • Routing (handling OAuth routes, etc.) - this is especially noticeable when you are trying to implement multiple providers
    • Redirects (NextAuth protects redirects and handles next pages, callback urls, etc. for you, but you have to implement and secure these yourself in Lucia.)
    • Auth proxy for preview environments (Especially if you want to secure this, you have to implement some custom cryptographic logic, either signing or encrypting the state payload of an OAuth callback)
  • Lucia is focused on giving users good primitives + guides to implement secure apps using those primitives, but if CT3T (or CT3A by proxy) implements Lucia, it could lead less experienced developers to make mistakes if they want to change some auth stuff unless careful consideration into the code design is taken.

@juliusmarminge
Copy link
Member Author

juliusmarminge commented May 15, 2024

@ndom91 @balazsorban44 you have thoughts?

@ndom91
Copy link

ndom91 commented May 16, 2024

So I haven't read the whole thread, but based off of the last few comments my first suggestion would be to try the trustHost config option (or AUTH_TRUST_HOST env var). Which will tell nextauth to use the x-forwarded-for header for the host instead of the Host header, like what would be required for use-cases behind a reverse proxy.

Have you tried that? 🤔

@Wundero
Copy link

Wundero commented May 16, 2024

As a followup to this, unfortunately CSRF isn't the only thing that doesn't work when you don't set the AUTH_URL. A couple of points I found:

  • OAuth callbacks will still point to localhost, and I can't seem to get the auth proxy to work with this either.
  • NextJS seems to infer the request.url property as localhost even when your origin is the 192.168.x.y ip, despite the request being made to the proper location. You can introspect the real host using the host header, and set the request url to use that, but that seems hacky at best, and doesn't really seem to cover all urls anyways

Ultimately, I have been unable to get local auth to work without the env var, and I don't want to try to hack nextauth to get it to work even though that seems like the only way this could be made to work.

I have done some further experimentation, and figured out that the nextjs request.url problem seems to be the root cause of some of these issues. Adding this seems to fix the problem, though how safe this is is questionable:

function rewriteRequestUrl(req: NextRequest) {
  const host = req.headers.get("host");
  const newURL = new URL(req.url);
  newURL.host = host ?? req.nextUrl.host;
  return new NextRequest(newURL, req);
}

export const POST = async (req: NextRequest) => {
  req = rewriteRequestUrl(req);
  return DEFAULT_POST(req);
};

export const GET = async (
  req: NextRequest,
  props: { params: { nextauth: string[] } },
) => {
  req = rewriteRequestUrl(req);
  // ... rest
}

This will override the host of the URL to be the one which the request is targeting. This may or may not work in some cases with proxies, but this seems to work in dev in my experience. With this in place, I am able to get as far as the app redirecting to expo:///login?session_token=..., though for some reason my dev build doesn't recognize that URL.

As an aside, a couple of issues I have run into that seem unrelated:

  • Windows, turborepo and config = "edge" seem to hate each other, and I can only get the webapp to work by disabling edge runtimes on all trpc related endpoints. This seems to be specific to turborepo + local, so deployment would still be fine, but it is a minor annoyance
  • The auth proxy expects the URL to be of the form https://<domain>/auth, which isn't indicated in the documentation. This can be remedied by altering the basePath in the auth config on the proxy (perhaps to /api/auth for consistency with the main app), or just noting it in the readme of the proxy/main app
  • For some reason, the call NextAuth makes to setCookie when running callback doesn't have a consistent order. This breaks the code which tries to get cookie 0 and find the session token - instead, calling find might be better. For example, const setCookie = authResponse.headers.getSetCookie().find((cookie) => cookie.startsWith("authjs.session-token"));

I have a (mostly) working fork of this that handles not having the AUTH_URL, with the above changes applied, that I can push when I have a bit more time, but I am still not really satisfied with this solution because manually adjusting the request URL by pulling it from the host header feels wrong.

EDIT: Repo is here: https://github.com/Wundero/create-t3-turbo/tree/11-02-feat_expo_auth (Commit: 4072aa9). Feel free to play around with this and see if you can get it to work, but I think this is the last I will touch nextauth here since I have requirements that it can't fulfil (namely, password logins + session impersonation).

@dBianchii
Copy link
Contributor

Hi, someone asked before, but I'll ask again. Has someone managed to get it working with Google? My app only has email and google, but I can't get either to work

@ochicf
Copy link

ochicf commented May 19, 2024

From what I understand, the AUTH_URL is required in production so this is only something affecting development environments, right? Hence, I think the approach proposed in #720 (comment) seems fine to me, as there are no runtime overheads, just when running the command.

Here's my take on it:

  1. added file in /packages/auth/scripts/env-auth-url.js
// @ts-check
import { networkInterfaces } from "os";

// TODO: read port from env/command
const PORT = 3000;

process.stdout.write(process.env.AUTH_URL || getAuthUrlFallback());

function getAuthUrlFallback() {
  const interfaces = networkInterfaces();
  for (const name in interfaces) {
    const candidates = interfaces[name] || [];
    for (const candidate of candidates) {
      if (candidate.family === "IPv4" && !candidate.internal) {
        return `http://${candidate.address}:${PORT}`;
      }
    }
  }
  // this will not work in expo app, but trying anyway...
  return `http://localhost:${PORT}`;
}
  1. added script in /apps/nextjs/package.json:
    "with-dev-env": "AUTH_URL=$(node ../../packages/auth/scripts/env-auth-url.js) pnpm with-env",
  1. use the above script in the dev script:
    "dev": "pnpm with-dev-env next dev",

Some notes:

  • placement of script is completelly made up, I was trying to think of a consistent way that packages could do something similar (instead of cramping everything in the app/nextjs itself)
  • port could be read from env var or executed command, but if the port is being used nextjs will lift with another one, which cannot be known when setting the env var
  • I had never read results from JS files on package.json scripts, so maybe there's a better way of doing it
  • I have tried to encapsulate not just the value of the env var, but the name aswell, but did not manage to make it work:
// in the JS file that reads the auth url
process.stdout.write(`AUTH_URL=${process.env.AUTH_URL || getAuthUrlFallback()}`);
// in package.json
"with-dev-env": "$(node ../../packages/auth/scripts/env-auth-url.js) pnpm with-env",
# getting this error
> $(node ../../packages/auth/scripts/env-auth-url.js) pnpm with-env "next" "dev"

sh: AUTH_URL=http://192.168.0.118:3000: No such file or directory

Let me know if this approach is worth considering and could work, I would try to prepare it a bit better.

@Wundero
Copy link

Wundero commented May 19, 2024

From what I understand, the AUTH_URL is required in production so this is only something affecting development environments, right? Hence, I think the approach proposed in #720 (comment) seems fine to me, as there are no runtime overheads, just when running the command.

Here's my take on it:

  1. added file in /packages/auth/scripts/env-auth-url.js
// @ts-check
import { networkInterfaces } from "os";

// TODO: read port from env/command
const PORT = 3000;

process.stdout.write(process.env.AUTH_URL || getAuthUrlFallback());

function getAuthUrlFallback() {
  const interfaces = networkInterfaces();
  for (const name in interfaces) {
    const candidates = interfaces[name] || [];
    for (const candidate of candidates) {
      if (candidate.family === "IPv4" && !candidate.internal) {
        return `http://${candidate.address}:${PORT}`;
      }
    }
  }
  // this will not work in expo app, but trying anyway...
  return `http://localhost:${PORT}`;
}
  1. added script in /apps/nextjs/package.json:
    "with-dev-env": "AUTH_URL=$(node ../../packages/auth/scripts/env-auth-url.js) pnpm with-env",
  1. use the above script in the dev script:
    "dev": "pnpm with-dev-env next dev",

Some notes:

  • placement of script is completelly made up, I was trying to think of a consistent way that packages could do something similar (instead of cramping everything in the app/nextjs itself)
  • port could be read from env var or executed command, but if the port is being used nextjs will lift with another one, which cannot be known when setting the env var
  • I had never read results from JS files on package.json scripts, so maybe there's a better way of doing it
  • I have tried to encapsulate not just the value of the env var, but the name aswell, but did not manage to make it work:
// in the JS file that reads the auth url
process.stdout.write(`AUTH_URL=${process.env.AUTH_URL || getAuthUrlFallback()}`);
// in package.json
"with-dev-env": "$(node ../../packages/auth/scripts/env-auth-url.js) pnpm with-env",
# getting this error
> $(node ../../packages/auth/scripts/env-auth-url.js) pnpm with-env "next" "dev"

sh: AUTH_URL=http://192.168.0.118:3000: No such file or directory

Let me know if this approach is worth considering and could work, I would try to prepare it a bit better.

You might have better success (in unix envs) with the latter approach by using the export command:

process.stdout.write(`AUTH_URL=${process.env.AUTH_URL ?? getAuthUrlFallback()}`);
"with-dev-env": "export $(node ../../packages/auth/scripts/env-auth-url.js); pnpm with-env",

But I think the first approach is mostly fine, barring the open port issue. What you could maybe do is scan for open ports on the system, and then something like this:

let port = 2999;
while (isInUse(++port));
// Port should be whatever nextjs will use

But I think this is also likely not ideal, since (a) it likely has to try opening a server to check, which is slow, and (b) port 3000 could be used between the time that this script runs and the nextjs server binds to a port (unlikely but still). Perhaps a build step in the next.config.js could work, but that seems hacky as well. It might just be worth accepting the risk that it doesn't work as expected if nextjs binds to any other port.

I think this approach could work, but it might be a bit fragile, but then again so is a lot of the current use of packages because of beta versions (authjs v5 is in beta, trpc 11 is also, etc.), so maybe its worth it.

@juliusmarminge What are your thoughts on this? Do you prefer the script to set the env var or the lucia approach to CSRF + the hacking of the next request? I could flesh out my approach a bit more to try to improve its security if you think that is the best one to go with.

@ochicf
Copy link

ochicf commented May 20, 2024

Ok so I've iterated a bit on the "generating the AUTH_URL variable dynamically", I'll try to put the highlights in a short/simple manner:

  1. auth package exposes /scripts/dev-env.js file. It's a regular JS module, can export functions
  2. nextjs app implements /scripts/write-dev-env.js file. It's a file intented to be run during the dev script, before the dotenv CLI call. It writes the /apps/nextjs/.env.local file (will replace it), and uses the auth package exported module
  3. when the project is run with dotenv -e ../../.env -- next dev, the .env.local will be read internally by nextjs, so it will have LESS priority than the variable already set

This has the following benefits:

  1. order of env vars is respected, so the developer will not have to think about this (set the variable as you would normally do will take priority, leave it blank will be set automatically)
  2. it should be system agnostic:
  • since the logic is done in JS modules, there's is nothing that relies in any specific OS/shell
  • only requirement would be node, more specific fs and http packages (should not be an issue, as we are running this on the developer machine)
  1. there's a defined way of doing this kind of things, in case other packages need it

Drawbacks:

  1. relies writing to .env.local, so there will only be one (not being able to run multiple instances... if that's ever needed?)
  2. a bit convoluted/complex?
  3. too much code for something that's just CLI? It pollutes the initialised workspace code...

Now, I'm adding the code and more details inside an expansion panel to avoid cluttering (more) the MR.

Code snippets

  • /packages/auth/scripts/dev-env.js
// @ts-check
import { createServer } from "http";
import { networkInterfaces } from "os";

// TODO: read port from env/command
const INITIAL_PORT = 3000;

export const getDevEnv = async () => ({
  ...(await prepareEnvVar("AUTH_URL", getAuthUrlFallback)),
});

/**
 * TODO: encapsulate this in a separate module
 * @param {string} name
 * @param {(() => string | Promise<string>)} fallbackFn
 */
async function prepareEnvVar(name, fallbackFn) {
  return { [name]: await getEnvVarValue(name, fallbackFn) };
}

/**
 * TODO: encapsulate this in a separate module
 * @param {string} name
 * @param {(() => string | Promise<string>)} fallbackFn
 * @returns {Promise<string>}
 */
async function getEnvVarValue(name, fallbackFn) {
  // eslint-disable-next-line no-restricted-properties
  return process.env[name] ?? (await fallbackFn());
}

async function getAuthUrlFallback() {
  const port = await getNextjsPort();
  const interfaces = networkInterfaces();
  for (const name in interfaces) {
    const candidates = interfaces[name] ?? [];
    for (const candidate of candidates) {
      if (candidate.family === "IPv4" && !candidate.internal) {
        return `http://${candidate.address}:${port}`;
      }
    }
  }
  // this will not work in expo app, but trying anyway...
  return `http://localhost:${port}`;
}

/**
 * TODO: encapsulate this in a separate module
 * @param {number | undefined} initialPort
 */
async function getNextjsPort(initialPort = INITIAL_PORT) {
  let port = initialPort;
  while (await isPortInUse(port)) {
    port += 1;
  }
  return port;
}

/**
 * TODO: encapsulate this in a separate module
 * @param {number} port
 */
function isPortInUse(port) {
  return new Promise((resolve) => {
    const server = createServer();
    server.on("error", (err) => {
      if ("code" in err && err.code === "EADDRINUSE") {
        resolve(true);
      } else {
        resolve(false);
      }
    });
    server.on("listening", () => {
      server.close();
      resolve(false);
    });
    server.listen(port);
  });
}
  • /apps/nextjs/scripts/write-dev-env.js
// @ts-check

import { writeFile } from "fs";

import * as auth from "@acme/auth/scripts/dev-env.js";

const devEnv = {
  ...(await auth.getDevEnv()),
};

const contents = `
# AUTOGENERATED BY /apps/nextjs/scripts/with-dev-env.js
${Object.entries(devEnv)
  .map(([key, value]) => `${key}=${value}`)
  .join("\n")}
`;

writeFile(".env.local", contents, "utf8", (err) => {
  if (err) {
    console.error(err);
    process.exit(1);
  }
});
  • /apps/nextjs/package.json
"scripts": {
    "dev": "pnpm with-dev-env next dev",
    "with-dev-env": "$(node ./scripts/write-dev-env.js) pnpm with-env",
    "with-env": "dotenv -e ../../.env --"
}

Note that there's some functions/logic that should be encapsulated to a different package (ex: /packages/scripts) so they can be used in multiple packages, apps...

Here are some screenshots of the outcome (note that the AUTH_URL is being logged in /packages/auth/env.ts

Generating AUTH_URL with default port

Screenshot 2024-05-20 at 12 51 47

Generating AUTH_URL when the port is being used

Screenshot 2024-05-20 at 12 52 49

Overriding the AUTH_URL in `.env`

Screenshot 2024-05-20 at 12 53 18

Maybe I went too far with this and overcomplicated it... looking for feedback and thoughts.

@Wundero
Copy link

Wundero commented May 20, 2024

@ochicf I think that works pretty well - one thing I might suggest is to append to the file instead of just overwriting it, in case a user is using .env.local manually. Something like this:

Code snippet
// @ts-check
import fs from "fs/promises";

import * as auth from "@acme/auth/scripts/dev-env.js";

const devEnv = {
  ...(await auth.getDevEnv()),
};

const PREFIX = "# AUTOGENERATED BY /apps/nextjs/scripts/with-dev-env.js";

const contents = `
${PREFIX}
${Object.entries(devEnv)
  .map(([key, value]) => `${key}=${value}`)
  .join("\n")}
`;

async function addEnvToFile() {
  let file;
  try {
    file = await fs.open('.env.local', fs.constants.O_RDWR | fs.constants.O_CREAT);
    const previousContents = (await file.readFile('utf8')).replace(/\r/g, '');
    let prefix = previousContents;
    if (previousContents.includes(PREFIX)) {
      prefix = previousContents.slice(0, previousContents.indexOf(PREFIX) - 1);
    }
    const output = `${prefix}${contents}`;
    const written = await file.write(output, 0, 'utf8');
    await file.truncate(written.bytesWritten);
  } finally {
    await file?.close();
  }
}

addEnvToFile().catch((err) => {
  console.error("Failed to write AUTH_URL to env file", err);
  process.exit(1);
});

@ochicf ochicf mentioned this pull request May 20, 2024
1 task
@ochicf
Copy link

ochicf commented May 20, 2024

Created #1045 with a proposal for this solution. Some notes:

  • encapsulated all the logic I could into the newly created packages/scripts
  • better handling of .env.local file as recommended by @Wundero (thanks for your snippet!). Also adding the file to blame for the generation to make it easier to understand
    Screenshot 2024-05-21 at 00 34 33
  • inferred vars will be logged to the console, also with the file that's generating them, and possibility of adding a link to readme
    Screenshot 2024-05-21 at 00 30 38
  • simplified the root readme with a link to a newly added readme for the auth package with the explanation of the AUTH_URL situation
  • had to write a priority function to sort valid IP addresses, not sure if this is the best TBH:
    Screenshot 2024-05-21 at 00 38 30

const setCookie = authResponse.headers.getSetCookie()[0];
const setCookie = authResponse.headers
.getSetCookie()
.find((cookie) => cookie.startsWith("authjs.session-token"));
Copy link

@ochicf ochicf May 22, 2024

Choose a reason for hiding this comment

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

This doesn't work for me, as the cookie starts differently and does not find any, throwing the error just below. Here's an example of my value:

authis.pkce.code verifier=; Max-Age=0; Path=/: HttpOnly: SameSite=Lax, authis.session-token=270ca062-6d2e-4f78-82c9-7690aaff8a6f: Path=/: Expires=Fri, 21 Jun 2024 07:20:50 GMT; HttpOnly; SameSite=Lax

Since the cookie pattern is defined as a regex in this file, could we just use it here to match the cookie?

const setCookie = authResponse.headers
  .getSetCookie()
  .find((cookie) => AUTH_COOKIE_PATTERN.test(cookie));

Copy link

Choose a reason for hiding this comment

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

The pattern itself is hardcoded as well - if you change the authjs cookie name, this will have issues no matter what. Perhaps it might make more sense to customize the cookie to be acme.session-token instead, and that way users of the app can just find+replace acme and will cover the cookie as well?

An alternative would be to just ignore the prefix with something like this:

const AUTH_COOKIE_PATTERN = /(?:\w+)\.session-token=([^;]+)/;

And then apply your suggestion to test the cookie.

Copy link

Choose a reason for hiding this comment

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

@Wundero I just realised that I misscopied the contents of the cookie 🤦

The part of the authjs.session-token does not change (at least on my case), I just wanted to highlight that the cookie contains another starting part (the authjs.pkce.code_verifier=... which caused that the .startsWith(...) condition to not comply.

authjs.pkce.code_verifier=; Max-Age=0; Path=/; HttpOnly; SameSite=Lax, authjs.session-token=e9bf1a44-44f1-453f-b492-fc26adec22ef; Path=/; Expires=Sat, 22 Jun 2024 07:33:02 GMT; HttpOnly; SameSite=Lax

With that being clarified, your point is still valid as NextAuth allows to configure cookies (including changing its names), so even the .session-token part could be changed. Is this something that needs to be supported? IMHO it's too of an edge case and not worth it, but still, adding a possible way below.

Details

I guess the regex could be created from exported authConfig's cookie configuration (if any), defaulting to the current one:

const AUTH_COOKIE_NAME =
  authConfig.cookies?.sessionToken?.name || "authjs.session-token";
const AUTH_COOKIE_PATTERN = new RegExp(
  `${AUTH_COOKIE_NAME.replace(/\./g, "\\.")}=([^;]+)`,
);

Notes:

  • because of the satisfies NextAuthConfig: the actual exported authConfig does not have the cookies and the nested properties, so TS coplains of the authConfig.cookies?.sessionToken?.name access, so this would need to be handled
  • the authConfig object needs to be exported from @acme/auth
  • there's some string manipulation things going on (replacing . with \\. so the regexp is properly created... there could be more edge cases so things could break

Copy link

Choose a reason for hiding this comment

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

The startsWith should still work, since the getSetCookie will return a list of all named cookies - the authjs.pkce.code_verifier (or any other authjs cookie) is a separate entry in the list, and should fail that check. In your example, it would split the cookies at the , and return these:

['authjs.pkce.code_verifier=; Max-Age=0; Path=/; HttpOnly; SameSite=Lax', 'authjs.session-token=e9bf1a44-44f1-453f-b492-fc26adec22ef; Path=/; Expires=Sat, 22 Jun 2024 07:33:02 GMT; HttpOnly; SameSite=Lax']

Copy link

@ochicf ochicf May 28, 2024

Choose a reason for hiding this comment

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

I'm not sure why (maybe different system, code or package versions?), but In my case it does not return the same as you.

What you provided:

// outer array
[
  // item 1
  'authjs.pkce.code_verifier=; Max-Age=0; Path=/; HttpOnly; SameSite=Lax',
  // item 2
  'authjs.session-token=e9bf1a44-44f1-453f-b492-fc26adec22ef; Path=/; Expires=Sat, 22 Jun 2024 07:33:02 GMT; HttpOnly; SameSite=Lax'
]

What I'm getting:

// outer array
[
  // item 1 - the comma is inside the string -------------------------- v
  'authjs.pkce.code_verifier=; Max-Age=0; Path=/; HttpOnly; SameSite=Lax, authjs.session-token=e9bf1a44-44f1-453f-b492-fc26adec22ef; Path=/; Expires=Sat, 22 Jun 2024 07:33:02 GMT; HttpOnly; SameSite=Lax'
]

So, in my case there's a single item that does not start with authjs.session-token.

To add a bit more into the mix, I'm doing some tests with HTTPS and the name of the cookie changes, which would make the startsWith to not work even if your case (note the __Secure-authjs.session-token names). TBH I'm trying with nextjs --experimental-https, haven't tried this in a production site or anything, so not sure if that would affect:

[
  '__Secure-authjs.pkce.code_verifier=; Max-Age=0; Path=/; HttpOnly; Secure; SameSite=Lax, __Secure-authjs.session-token=d99b92b9-acf2-4f01-b1bc-91cff0bff535; Path=/; Expires=Thu, 27 Jun 2024 07:33:38 GMT; HttpOnly; Secure; SameSite=Lax'
]

In any case, IMHO it feels more consistent and resilient doing the AUTH_COOKIE_PATTERN.test(...), as just below it's doing a setCookie?.match(AUTH_COOKIE_PATTERN) which also allows the cookie to be in any place of the string.

@juliusmarminge
Copy link
Member Author

Hey guys,

Sorry for not keeping up here, been busy.

You seem to have done a lot of exploration into getting the IP to work, I wonder if it wouldn't be easier to work around the csrf stuff in dev?

Have you tried what Nico suggested with the trust host thing?

@Wundero
Copy link

Wundero commented May 22, 2024

Hey guys,

Sorry for not keeping up here, been busy.

You seem to have done a lot of exploration into getting the IP to work, I wonder if it wouldn't be easier to work around the csrf stuff in dev?

Have you tried what Nico suggested with the trust host thing?

The branch I shared does this - afaik my branch works, and the checks can probably be turned back on for production, so I think it's a viable option. I can make a PR into this branch (like what ochicf has done) for you to review it a bit more, if that helps, but there's more than just CSRF that's a problem.

The big one is that NextJS sets request.url to localhost:3000 even if you make requests from other IPs, and that becomes a problem for callbacks. You need to patch it to be the Host header they requested the API from, as per my comment before.

I think both of these routes can work, and each one has pros and cons, so I think we can decide how we want to go forward once you have taken a look into both optins.

@ochicf
Copy link

ochicf commented May 28, 2024

@juliusmarminge I've been fiddling a bit more on the environment variables approach and pushed some new code to #1045. I've updated the description there writing the highlights, pros and cons, so you can have more info and evaluate whether this is a valid approach. I've also added some screenshots and a screen capture (which I'm adding here to make this comment sexier)

Screen.Recording.2024-05-28.at.16.47.51.mov
Screen.Recording.2024-05-28.at.16.29.45.mov

@Wundero would love your inputs aswell. Also I'll try to look deeper on your code and do some attempts aswell (probably to EOW/during weekend). If you push your working version in a MR would be great ❤️

@Wundero
Copy link

Wundero commented May 28, 2024

@juliusmarminge I've been fiddling a bit more on the environment variables approach and pushed some new code to #1045. I've updated the description there writing the highlights, pros and cons, so you can have more info and evaluate whether this is a valid approach. I've also added some screenshots and a screen capture (which I'm adding here to make this comment sexier)

Screen.Recording.2024-05-28.at.16.47.51.mov
Screen.Recording.2024-05-28.at.16.29.45.mov
@Wundero would love your inputs aswell. Also I'll try to look deeper on your code and do some attempts aswell (probably to EOW/during weekend). If you push your working version in a MR would be great ❤️

I've gone ahead and filed the MR here: #1054. I am gonna continue trying to get the stuff to work for me at some point, but for now I believe this is sufficient to get the app working.

I haven't verified that the login step completes at the end because I ran into linking issues, and I have recently reinstalled my boot drive and thus don't have the software setup (namely android studio) and haven't had the time to get back around to that yet.

As far as input goes, I think your method works quite well (as demonstrated), but there is definitely a lot of complexity as well. I think your implementation does the best with the concept of changing the env vars, but to me that feels fundamentally finicky (not that my approach of rewriting requests is much better haha).

Let me know what you think of my PR and whether you could get it working, since I got 99% of the way there and the roadblock I hit was that expo wasn't processing the acme://... links (i changed the slug on my end to ct3t since i called the project that on expo.dev, but the core issue of ct3t://login?session_token=... not being a valid link was the crux of the issue). Any advice on how to resolve that issue would be appreciated, since that's really my final blocker for validation.

@ochicf
Copy link

ochicf commented May 28, 2024

@Wundero thanks for this. I've tried your MR and worked fine for me with Discord on first try with my existing project, and also from a clean project. Definitely much more simpler and less code, so I think it's a better take!

I still had to add the metro IP in the allowed list from Discord app to be able to complete login from Expo, which maybe would not be super clear if I didn't faced with this issue already, so it could be worth adding this in the FAQ instead of the current AUTH_URL explanation?

BTW I added a question on your MR.

@Wundero
Copy link

Wundero commented May 28, 2024

@Wundero thanks for this. I've tried your MR and worked fine for me with Discord on first try with my existing project, and also from a clean project. Definitely much more simpler and less code, so I think it's a better take!

I still had to add the metro IP in the allowed list from Discord app to be able to complete login from Expo, which maybe would not be super clear if I didn't faced with this issue already, so it could be worth adding this in the FAQ instead of the current AUTH_URL explanation?

BTW I added a question on your MR.

I actually only tested this when using the deployed auth proxy nitro app, so that was what I was using, but adding the IP to discord or the auth proxy to discord are both valid (but at least one needs to be done). Glad to hear it works though. I will update the FAQ/etc. sometime in the coming day or two, when I have a bit more time.

@Wundero
Copy link

Wundero commented May 29, 2024

I've fixed all the issues I was having with my PR and it should be good to go now. Tested the auth on a real android device and it works seamlessly, and all security concerns should be alleviated since I only make the security relaxations in dev mode. Works well with the auth proxy, and supports localhost:3000 for the web without issue as well.

@juliusmarminge I think #1054 is now a feature-complete solution that doesn't really have compromises, so I would appreciate if you could take some time to review the PR and we could maybe merge it or come up with next steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expo authentication using next-auth