Atinux / nuxt-auth-utils

Minimal Auth module for Nuxt 3.
MIT License
657 stars 61 forks source link

Only expose public data part of session #47

Open pi0 opened 5 months ago

pi0 commented 5 months ago

(context: question by @harlan-zw in nuxt discord regarding reliablilty of sessions and weather he should use storage to keep private session data)

H3 sessions are encrypted and only readable by server-side. This can guarantee two things:

Auth-utils, exposes an edpoint (session.get) that server-side decrypts the session for user. It takes away the second benefit of session encoding which can guarantee data remains secret and private.

While there must be good benefits of this, it is something IMO insecure to do by default and developers might wrongly put sensitive data based on encryption guarantee that will be exposed again.

I would highly recommend (as a breaking change) to only expose data.public part of the session.

harlan-zw commented 5 months ago

For context, the reason why this came up is that I was trying to store private data into the users sessions. Knowing how sessions work, I figured it would be safe to add it there. After all it was only data I needed within a Nitro context.

I missed the comments in the code example from the documentation explaining that the actual logic is that nothing attached to the user sessions is safe. I found it later via the jsdocs while debugging some code. In hindsight it is kind of obvious that this is the only way it could work.

So in reality there is no way to attach private data to the users session (without some workarounds?), which seems like a missed opportunity.

I think:

Atinux commented 5 months ago

To avoid breaking changes, I am thinking of having:

setUserSession(event, publicData, privateData)

And only expose the publicData to the endpoint. I need to think about the types though

Gerbuuun commented 4 months ago

Do you already have something in mind? I can have a go at this as I have good use for it.

An easy way is to just have two types and merge them into a single object:

export interface PublicSessionData {
  user?: User
}

export interface PrivateSessionData {
}

// Like this?
export interface SessionData {
  public?: PublicSessionData
  private?: PrivateSessionData
}
// Or this?
export interface SessionData extends PrivateSessionData {
  public?: PublicSessionData
}

and then /api/session.get:

export default eventHandler(async (event) => {
  const session = await requireUserSession(event)

  await sessionHooks.callHookParallel('fetch', session, event)

  return session.public
})

Or we could do some crazy key manipulation in a way that private properties can be filtered out recursively. But that does not sound like the way to go.

Atinux commented 4 months ago

Can you open a PR with your solution @Gerbuuun ?

septatrix commented 2 months ago

To avoid breaking changes, I am thinking of having:

setUserSession(event, publicData, privateData)

This project is still quite fresh so I would rather vote for some small backwards incompatible changes which can quickly be adjusted by a find+replace instead of resorting to signature trickery just to preserve backwards compatibility.

Atinux commented 1 month ago

I agree @septatrix

I am thinking of a hard breaking change with a nicer API at the end