getServerSession mutates event argument

[j2L4e]

Environment

---

• Operating System: Linux
• Node Version: v18.16.0
• Nuxt Version: 3.7.0
• CLI Version: 3.7.3
• Nitro Version: 2.6.2
• Package Manager: [email protected]
• Builder: -
• User Config: css, ssr, devtools, modules, auth, vuetify, runtimeConfig
• Runtime Modules: @invictus.codes/[email protected], @sidebase/[email protected], @pinia/[email protected]
• Build Modules: -

---

Reproduction

Call getServerSession() multiple times from within any api route handler

// /server/api/bug.ts

import { getServerSession } from "#auth";

export default defineEventHandler(async (event) => {
    console.log(event.node.res.getHeaders())
    // [Object: null prototype] {}

    await getServerSession(event)
    console.log(event.node.res.getHeaders())
    // [Object: null prototype] { 'content-type': 'application/json' }

    await getServerSession(event)
    console.log(event.node.res.getHeaders())
    // [Object: null prototype] {
    //   'content-type': [ 'application/json', 'application/json' ]
    // }

    return []
})

Describe the bug

Calling getServerSession internally calls the auth handler. This has side-effects because cookies and headers are set.
In my case, having multiple Content-Type headers set, caused errors in client-side content type detection. $fetch wouldn't automatically parse application/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

[xak2000]

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).

Examples:

With no calls to 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.

With 1 call to 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.

With 2 calls to 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).

[xak2000]

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:

1. Handling a real GET /session request.
2. From calling 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.

[phoenix-ru]

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.

[xak2000]

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.

[j2L4e]

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

[xak2000]

@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!

[phoenix-ru]

@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.

[warflash]

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 💚

[phoenix-ru]

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

[warflash]

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!

[phoenix-ru]

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 🙂

[phoenix-ru]

@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 🙂

Thank you very much for doing investigations and providing a good reproduction

[warflash]

@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

[phoenix-ru]

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?

[warflash]

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.