Skip to content
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

Only expose public data part of session #47

Open
pi0 opened this issue Feb 5, 2024 · 6 comments
Open

Only expose public data part of session #47

pi0 opened this issue Feb 5, 2024 · 6 comments

Comments

@pi0
Copy link

pi0 commented Feb 5, 2024

(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:

  • Only server can mutate session so it's data is reliable
  • Only server can read/decrypt session so it's data is private

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
Copy link
Contributor

harlan-zw commented Feb 5, 2024

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:

  • data.public is a great idea and solves the original issue.
  • we need a dedicated docs section explaining how session data is stored / exposed given how sensitive it is

@Atinux
Copy link
Owner

Atinux commented Feb 6, 2024

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
Copy link
Contributor

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.

Copy link
Owner

Atinux commented Mar 11, 2024

Can you open a PR with your solution @Gerbuuun ?

@septatrix
Copy link

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.

Copy link
Owner

Atinux commented Jun 5, 2024

I agree @septatrix

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants