-
-
Notifications
You must be signed in to change notification settings - Fork 343
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
base: main
Are you sure you want to change the base?
feat: expo auth #720
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @juliusmarminge and the rest of your teammates on Graphite |
14ec1c6
to
cfc5645
Compare
cfc5645
to
267720f
Compare
34f4f80
to
bcf02b6
Compare
267720f
to
7f3fa76
Compare
bcf02b6
to
b81139e
Compare
7f3fa76
to
e2336e2
Compare
c4bac86
to
fb5bc06
Compare
e2336e2
to
eac0abc
Compare
eac0abc
to
ae25ddf
Compare
3917c17
to
8a7c70e
Compare
8a7c70e
to
31fc2dc
Compare
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` |
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 😵💫😵💫 |
Is setting the |
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 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:
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:
|
@ndom91 @balazsorban44 you have thoughts? |
So I haven't read the whole thread, but based off of the last few comments my first suggestion would be to try the Have you tried that? 🤔 |
I have done some further experimentation, and figured out that the nextjs 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 As an aside, a couple of issues I have run into that seem unrelated:
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). |
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 |
From what I understand, the Here's my take on it:
// @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}`;
}
"with-dev-env": "AUTH_URL=$(node ../../packages/auth/scripts/env-auth-url.js) pnpm with-env",
"dev": "pnpm with-dev-env next dev", Some notes:
// 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 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. |
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:
This has the following benefits:
Drawbacks:
Now, I'm adding the code and more details inside an expansion panel to avoid cluttering (more) the MR. Code snippets
// @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);
});
}
// @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);
}
});
"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: Here are some screenshots of the outcome (note that the Maybe I went too far with this and overcomplicated it... looking for feedback and thoughts. |
@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 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);
}); |
Created #1045 with a proposal for this solution. Some notes:
|
const setCookie = authResponse.headers.getSetCookie()[0]; | ||
const setCookie = authResponse.headers | ||
.getSetCookie() | ||
.find((cookie) => cookie.startsWith("authjs.session-token")); |
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 exportedauthConfig
does not have thecookies
and the nested properties, so TS coplains of theauthConfig.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
There was a problem hiding this comment.
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']
There was a problem hiding this comment.
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.
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 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. |
@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.movScreen.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 |
@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 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. |
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 @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. |
Closes #486