search

sidebase / nuxt-auth

Authentication built for Nuxt 3! Easily add authentication via OAuth providers, credentials or Email Magic URLs!
https://auth.sidebase.io
MIT License
1.32k stars 164 forks source link

perf: server side fetch requests made to various endpoints #742

Closed warflash closed 6 months ago

warflash commented 7 months ago

Environment

Reproduction

https://github.com/warflash/nuxt-auth-debug

Using the module's playground-authjs dir.

Describe the bug

In our production env we noticed a ton of /api/auth/session?callbackUrl=http:%2F%2Flocalhost:3333%2Falways-unprotected requests coming through our proxy layer, all with user agent set to "node".

To debug this I spun up a local version of the playground of the repo and attached a proxy, -> see repro above.

I am not logged in on the local playground, depite that:

In the logs below you can see me calling for /always-unprotected with my browsers user agent in the first line. Then 3 requests are fired for /api/auth/session?callbackUrl=http:%2F%2Flocalhost:3333%2Falways-unprotected (which is what we've been seeing in our prod proxy logs as well, followed by /api/auth/providers and /api/auth/csrf.

Additional context

Wanted to perhaps spark a discussion or question on whether these calls can be avoided or made internally using $fetch or at least limited to users that are actually logged in or pages that are protected. Sending out a full http request for all of these adds a considerable amount of overhead and delay to the rendered SSR page.

Logs

Received request for /always-unprotected
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /api/auth/session?callbackUrl=http:%2F%2Flocalhost:3333%2Falways-unprotected
"node"
Received request for /api/auth/providers
"node"
Received request for /api/auth/csrf
"node"
Received request for /_nuxt/DdW6MrNW.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/CiQvDwpd.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/DZ4PCLmw.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/ChRk-71c.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/DlAUqK2U.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/CuxXRkfr.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/builds/meta/28820aa0-4610-4b7e-b225-6a545c117d71.json
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /favicon.ico
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /api/auth/providers
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /api/auth/csrf
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /api/token
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
phoenix-ru commented 7 months ago

I absolutely agree with you, it is quite wasteful to make full-blown http requests in Node side. What I don't understand, however, is that we are already using $fetch to avoid that: https://github.com/sidebase/nuxt-auth/blob/adfe97ee22890934acb3e179651ef72bc4ade6dd/src/runtime/utils/fetch.ts#L5-L24

I guess we'd have to refer to the $fetch docs and probably check our passed options. Will look into it later (possibly this week)

warflash commented 7 months ago

Awesome, thank you @phoenix-ru!

While looking into this I came up with two possible reasons. Please do note however that that's just possible explanations I had, not based on much. But figured sharing can't hurt.

  1. Those calls are made by authjs itself somewhere internally and are not using $fetch

  2. Prepending the auth.baseURL prop to fetch calls makes $fetch unable to resolve the fact that the calls are local. With a full featured domain it has no way of knowing those are supposed to be internal calls. If that's the case then the easiest solution might be to have a auth.baseUrl as well as auth.serverBaseUrl that's used in import.meta.server contexts.

warflash commented 6 months ago

So after looking into this it turns out the fetch implementation does indeed prepend the entire host section of the URL. This causes $fetch to not resolve calls internally.

I've played with it for a bit and technically all that's needed is a change in the fetch constructor. -> https://github.com/warflash/nuxt-auth-debug/commit/746cfd8f1c8b5262f301f6278f012fb497163a5f

To show the differences and debug I went ahead and created cpu flamegraphs for both versions.

Default/Current version:

image

Notice the fetch call and http dispatch.

Full cpuprofile: no-local-fetch.cpuprofile

Patched version based on repo linked:

image

Notice the localFetch working and only calling callHandle

Full cpuprofile: local-fetch-ufo.cpuprofile

Obviously on localhost those changes only shave off .2ms, however in a real env they save an entire roundtrip that would otherwise have been routed out to the public internet and back to the nuxt app.

phoenix-ru commented 6 months ago

Hi @warflash, thank you a lot for a thorough investigation, I can indeed confirm the issue exists. Moreover, it is caused by this particular line in Nuxt's $fetch implementation (tldr request must start with /): https://github.com/nuxt/nuxt/blob/572c36745597ee45e9b57f8f32a24dd99a5b9d42/packages/nuxt/src/app/composables/fetch.ts#L172

I checked your linked repo, however, I don't think I can use it, since the issue is really in _fetch implementation I already linked here.

It uses joinedPath, which is a concatenation of baseURL + path: https://github.com/sidebase/nuxt-auth/blob/949e556f0ed71ea273e20f236b80748831e8b338/src/runtime/utils/url.ts#L8

baseURL comes from computed state: https://github.com/sidebase/nuxt-auth/blob/949e556f0ed71ea273e20f236b80748831e8b338/src/runtime/composables/commonAuthState.ts#L35-L38

and, finally, computed state fullBaseUrl comes from user's baseURL: https://github.com/sidebase/nuxt-auth/blob/949e556f0ed71ea273e20f236b80748831e8b338/src/module.ts#L117-L119 https://github.com/sidebase/nuxt-auth/blob/949e556f0ed71ea273e20f236b80748831e8b338/src/module.ts#L128

So, going back to the $fetch, it is always being called with http://localhost:3000/api/auth/..., which triggers the usual HTTP fetch rather than a server procedure call. The correct way would be to determine if the request is actually local or not. I would assume using baseURL from ofetch and selectively providing/omitting it.

But there may be much better solutions, so all the ideas are welcome.

I have seen quite a lot of confusion when it comes to origin, baseURL, pathname and other related things:

Even though there was origin prior to v0.6, I think the whole design needs to be adjusted

warflash commented 6 months ago

Oh I apologize, for some reason my linked commit was faulty and didn't include the actual "fix"/workaround. Added that here: https://github.com/warflash/nuxt-auth-debug/commit/746cfd8f1c8b5262f301f6278f012fb497163a5f

But yeah, $fetch indeed only works if it's a relative path that's passed into it. Prepending the origin breaks that behavior.

And I highly agree with your concerns regarding baseURL and origin and pathname and whatnot. That combined with the fact that there's also a bunch of env vars floating around can get very tricky. In this context and in the context of the workaround I linked I'd also like to point out that not leveraging nuxt's app.baseURL seems like a weird design decision to me.

I feel like it should for sure be included somewhere in the path building logic -> https://github.com/sidebase/nuxt-auth/issues/340

Just from an minimal implementation perspective I feel like this logic: https://github.com/warflash/nuxt-auth-debug/commit/746cfd8f1c8b5262f301f6278f012fb497163a5f should already work for most things. In fact we've been using a patched version with that logic for 2 weeks now on our clusters and everything is smooth so far.

It still requires the possibility the overwrite the hardcoded /api/auth path if users set their api in a different path + a way to set the auth domain, not for fetching but for next auth to validate redirects iirc.

phoenix-ru commented 6 months ago

Okay, I see it now:

  1. Next Auth requires a fully-specified auth URL, such as NEXTAUTH_URL=https://example.com/custom-route/api/auth;
  2. $fetch can work with fully-specified URLs, but should do this only for external requests;
  3. $fetch for internal functionality should always receive pathnames instead of full URLs;
  4. _fetch is used in both authjs and local providers;

@warflash Regarding app.baseURL: I agree that it needs to be taken into account, however, the patch is not fully sufficient. I will try to come up with a patch which fits all 4 points above.

phoenix-ru commented 6 months ago

@warflash wow, thanks for reporting this issue! I can already see 50% improvement on req/sec when stress-tested on a local machine (and similar flamegraph results as yours). I am committing the patch, would you be able to test it on your loads and whether it satisfies your usecases?

Before: image

After: image

warflash commented 6 months ago

Great you're seeing the same sort of improvements, nice!

I am committing the patch, would you be able to test it on your loads and whether it satisfies your usecases?

Would there be a way for me to install a prerelease of that PR or should I go ahead and create a pnpm patch myself?

phoenix-ru commented 6 months ago

Would there be a way for me to install a prerelease of that PR

I will ask @zoey-kaiser to do a prerelease as a part of her functional review.

zoey-kaiser commented 6 months ago

I will ask @zoey-kaiser to do a prerelease as a part of her functional review.

I will do it right now! ❤️

zoey-kaiser commented 6 months ago

Just released the changes in a next release tag: https://www.npmjs.com/package/@sidebase/nuxt-auth/v/0.8.0-alpha.1

We will also begin testing the improvements internally and include it in the next full 0.8.0 release! Thank you so much for bringing this issue to our attention and the amazing investigation! We really appreciate it ❤️

spras commented 4 months ago

@zoey-kaiser I'm currently testing upgrade from 0.7.2 to 0.8.0-rc

This break my config : local provider and auth.baseURL defined to a custom base url ex : https://example.com/

Login is posted to the nuxt base URL , not to my custom base url

I have to redefine all the endpoints and removing the / prefix . Perhaps it has to become the default values ? 

endpoints: { 
       signIn: { path: 'login', method: 'post' },        
       signOut: { path: 'logout', method: 'post' },        
       signUp: { path: 'register', method: 'post' },        
       getSession: { path: 'session', method: 'get' }    
}
zoey-kaiser commented 4 months ago

Hi @spras 👋

Could you please open a new issue for this! I will look into it tomorrow! 😊