Open j2L4e opened 1 year ago
I have a similar problem caused by this behavior.
I have a Nitro middleware that calls getServerSession(event)
. If an actual API handler also calls getServerSession(event)
, then useFetch()
doesn't read data properly when it is called from SSR. It decodes the data as Blob
instead of simple JS object
, so the client can't render the data.
So, calling getServerSession(event)
more than one time from anywhere (e.g. a handler, a middleware) causes this problem.
The problem is caused by modifying the response headers. Even calling getServerSession(event)
just once still modifies the headers, so this can still cause the same problem if the response headers already have had a content-type
header at the moment of the first call of getServerSession(event)
.
getServerSession(event)
:Headers in the middleware (event.node.res.getHeaders()):
{}
Headers that useFetch() sees (context.response.headers):
HeadersList {
cookies: null,
[Symbol(headers map)]: Map(1) {
'content-type' => { name: 'content-type', value: 'application/json' }
},
[Symbol(headers map sorted)]: null
}
Data that useFetch() sees:
{ ...normal JS object... }
It's OK.
getServerSession(event)
:Headers in the middleware before calling getServerSession(event):
{}
Headers in the middleware after calling getServerSession(event):
{ 'content-type': 'application/json' }
Headers that useFetch() sees:
HeadersList {
cookies: null,
[Symbol(headers map)]: Map(1) {
'content-type' => { name: 'content-type', value: 'application/json' }
},
[Symbol(headers map sorted)]: null
}
Data that useFetch() sees: { ...normal JS object... }
It's still OK in the content-type
header was empty at the moment of the getServerSession(event)
call.
getServerSession(event)
:Headers in the middleware before calling getServerSession(event):
{}
Headers in the middleware after calling getServerSession(event):
{ 'content-type': [ 'application/json', 'application/json' ] }
Headers that useFetch() sees:
HeadersList {
cookies: null,
[Symbol(headers map)]: Map(1) {
'content-type' => {
name: 'content-type',
value: 'application/json,application/json'
}
},
[Symbol(headers map sorted)]: null
}
Data that useFetch() sees: Blob { size: 1856, type: '' }
It's not OK, as useFetch()
cannot understand the content-type
header, so it considers the returned data as Blob
.
The workaround is to call event.node.res.removeHeader('content-type')
after calling getServerSession(event)
, but this workaround is fragile as it will remove any values, even if they are previously set intentially by user's middleware or api handler.
I think that getServerSession(event)
should just not modify the headers (or any other data of the H3Event
object).
I did a small research and discovered where this header is set.
This line createas a session response.
This line uses the response data to update the event
.
I don't know the internals of nuxt-auth
very well but probably the same handler is called for:
GET /session
request.getServerSession(event)
.In the first case, modifying the headers
is totally OK, as the HTTP response will return the session data. Basically, it's the headers of GET /session
response.
In the second case, the event
object is not the event
, that represents a GET /session
request, but the event
of the request, that was handled at the moment of calling getServerSession(event)
function. So, modifying of this event
doesn't make sense.
Looks like a flaw of the getServerSession(event)
implementation to me. It blindly calls eventHandler
that is not aware of such hacky usage (direct call instead of HTTP request). It should be either:
eventHandler
is aware of such call possibility and skip event
modification in such case.getServerSession(event)
doesn't call eventHandler
directly and uses HTTP call instead.Hi @xak2000, thanks for your investigation, it genuinely helped me a lot!
I linked a Pull Request above which fixes the header and cookie duplication when calling getServerSession
.
For a context: I was considering not modifying the event, however this is too much effort to properly test for me.
Hello @phoenix-ru, thank you for you response!
However, I reviewed your commits and I think that maybe you don't fully understand the problem.
The main problem is not headers/cookies duplication, but the fact that the getServerSession(event)
method modifies headers/cookies of totally unrelated response (a current event). Current request is not supposed to be modified at all by getServerSession(event)
call, right? The returned headers/cookies from /auth/session
handler have nothing to do with the response from current event handler. This modification of headers/cookies is intended to modify /auth/session
response, but not an arbitrary response from user-defined API!
Example:
Let's say the client calls /api/text
endpoint on the server. The handler of /api/text
is implemented like this:
import { getServerSession } from '#auth'
export default defineEventHandler(async (event) => {
setResponseHeader(event, 'Content-Type', 'text/plain')
console.log('On the server headers', event.node.res.getHeaders())
await getServerSession(event)
console.log('On the server headers', event.node.res.getHeaders())
return 'Hello, World!'
})
The client could look like this:
const { data } = await useFetch('/api/text', {
onResponse(context) {
console.log(`On the client content-type: ${context.response.headers.get('content-type')}`)
},
})
console.log(`On the client response: ${data.value}`)
In this situation, even after de-duplication, the getServerSession(event)
function will still add some headers/cookies to the response from /api/text
request. This behavior can have any possible consequences (including security vulnerabilities), as these headers/cookies were never intended to be added to this response. They are intended to be added only to GET /auth/session
response.
An example of console output from the example above (when the client-side is running on SSR):
On the server headers { 'content-type': 'text/plain' }
On the server headers {
'content-type': [ 'text/plain', 'application/json' ],
'set-cookie': [
'next-auth.csrf-token=1613ae7b232422a49bedbe0bbb55ce3a6177294b5605514557aa16b997137147%7Cee3d639c5fc039be5b449dc793f7a7f4ef81c6416fc43c3d09ad997d2955832c; Path=/; HttpOnly; SameSite=Lax',
'next-auth.callback-url=http%3A%2F%2Flocalhost%3A3000; Path=/; HttpOnly; SameSite=Lax'
]
}
On the client content-type: text/plain,application/json
On the client response: Hello, World!
Also, as the set-cookie
header was set, the Browser will set these cookies for the domain. Probably it is not that bad in this particular case, but I still don't think this behavior is intended and expected as these cookies should be set only from calls to nuxt-auth
APIs (e.g. /auth/session
) and not random user-defined API method.
So, I consider your implementation as a workaround, that is better than nothing, as it will still help to overcome the consequences of this getServerSession(event)
behavior in most cases (for JSON responses), but still think this issue should be kept opened as proper fix is required.
Without digging into this too much: What about cloning and restoring headers and cookies before / after this line? If not mutating the event isn't easily accomplished, this might work around the entire problem rather than parts of it.
edit: Don't get me wrong, thanks for working on this @phoenix-ru
@j2L4e Interesting idea, actually! I think both headers/cookies are just JS arrays? In this case cloning/restoring them should not cause any troubles.
And, of course, I agree, @phoenix-ru, thank you for your work!
@xak2000 and @j2L4e thank you for the context. I agree with you, we shouldn't be mutating the event
from a user-defined API.
I mainly provided the workaround so that at least your JSON endpoints do not fail. Re-opening for a further investigation.
Hey everyone! We've just noticed this after adding caching to our api routes where suddenly every api response is cached with someone elses Set-Cookie for entire session tokens, csrf tokens and callback urls. This seems extremely dangerous and I'm slightly confused why there hasn't been more reports from others honestly, but perhaps it's hard to notice? A fix/workaround/idea would be highly appreciated, thanks 💚
Hi @warflash , I am currently busy on #673. Seeing as it has high potential to resolve some of the other issues, I would do an investigation after that.
In the meantime, it would be great if you could provide a reproduction to what you're experiencing, as reproducing these edge-cases takes a noticeable effort from our side which could've been dedicated to fixing instead.
upd: I have found a way to fix it during the authjs migration. Not sure if we should back-port it or simply release after the migration is complete
Hey @phoenix-ru, first off, #673 sounds fantastic, so thanks for that! Regarding the repro, I wrongly assumed that one of the other comments already provided one. I've seen you already found the issue but for the sake of completionism, here's a quick repro
Not sure if we should back-port it or simply release after the migration is complete
I'd highly vote in favor of a backport, given that #673 will be a new major version with breaking changes. If for whatever reason people can not update to ^1.0.0 right away (or potentially never) they'll be stuck with a security issue later down the road. But if backporting takes too much time then that of course is totally understandable as well!
Thanks!
Giving an update - me and @zoey-kaiser decided to postpone #673 due to some stability issues with @auth/core
, so I am already back-porting the proper implementation to 0.8
:slightly_smiling_face:
@warflash I am getting Hello world!
(guest) or Hello J Smith!
(auth) in your reproduction repository on the PR above (#849) which is a first good sign.
To verify the fix, I tested headers on both server handle and client using method from @xak2000 :
// Client
const { data: statusData } = await useFetch('/api/status', {
onResponse (context) {
console.log(`On the client content-type: ${context.response.headers.get('content-type')}`)
}
})
// Server
const session = await getServerSession(event)
console.log('On the server content-type:', event.node.res.getHeader('content-type'))
And in both places content type is text/plain
, so this issue should get resolved after the PR is merged :slightly_smiling_face:
Thank you very much for doing investigations and providing a good reproduction
@phoenix-ru Nice! Just did a very quick test on the feature branch and it indeed looks like the api response sent by the server has the proper headers now and crucially also no longer includes the Set-Cookies header 🚀
Slightly unrelated, but just noticed while investigating this issue:
Setting auth.disableServerSideAuth: true
does not seem to affect getServerSession
, meaning in my repro the rendered html always displays the username.
I feel like getServerSession should return an unauthed state here as well, otherwise it's quite the footgun for everyone caching their html I think.
Happy to create a fresh issue for that if you want me to
Being fair, disableServerSideAuth
is still a very confusing feature for me - I don't understand the use-case of setting the option and still calling getServerSession
?
Hmm thinking more about it - it sort of needs to return the session as otherwise there's no way to do anything auth related on the server at all :/
My understanding of disableServerSideAuth
was that it enables "easy-ish" caching but as with my example, calling the status
endpoint will return the name of the user eventhough auth is disabled. Adding a cache rule as per https://auth.sidebase.io/guide/advanced/caching will leak the name of that user to all requests. Name being a tame example of course, could also be any sort of personal information.
If possible getServerSession
could perhaps detect whether it's called in an SSR/Prerendering context? Alternatively a big warning in the docs might be nice. CC: @KyleSmith0905 in case you have any thoughts here too.
Environment
Build Modules: -
Reproduction
Call getServerSession() multiple times from within any api route handler
Describe the bug
Calling
getServerSession
internally calls the auth handler. This has side-effects because cookies and headers are set. In my case, having multipleContent-Type
headers set, caused errors in client-side content type detection.$fetch
wouldn't automatically parseapplication/json
responses correctly, but return a blob instead.Some component of the nuxt dev setup seems to be deduping or resetting
Content-Type
headers, effectively mitigating this problem in development. an observable error only occurs in production builds.The cause of this can be observed in dev mode, though.
Additional context
No response
Logs
No response