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

[Feature Request]: Allow signing of sessions #1156

Open
tobiasdiez opened this issue Sep 24, 2023 · 4 comments
Open

[Feature Request]: Allow signing of sessions #1156

tobiasdiez opened this issue Sep 24, 2023 · 4 comments
Labels
feature request New feature requests

Comments

@tobiasdiez
Copy link
Contributor

tobiasdiez commented Sep 24, 2023

Description

While it is not a huge increase of security, it is generally advised to hash/sign session ids before sending them out to the user. The main advantages are:

  • Using a predetermined secret, it is possible to detect if someone tampered with the session id (ie its possible to detect the difference between an expired and an invalid session).
  • Might hide possible security vulnerabilities coming from the session id generation
  • It is not possible for an attacker to impersonate users even if they got access to the session database
  • If you store the expire date of the session as well, one might be able to save a database request (for expired sessions)

Implementation: See e.g. https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L541-L550 in express-session or unjs/h3#315 in h3.

Discussions:

@pilcrowOnPaper
Copy link
Member

The benefit of signing cookies is minimal so I'm not sure if it's worth supporting it. I'm not a very big fan of using callback functions either:

lucia({
  sessionCookie: {
    parseSignedCookie: (cookie: string) => {
      const parsedCookie = parse(cookie);
      // I really don't like requiring `if` statements inside options
      if (parsedCookie === null) return null;
      if (parsedCookie.expires > Date.now()) return null;
      return parsedCookie.sessionId;
    },
    signCookie: (session: Session) => {
      return signCookie(session.sessionId, session.idleExpires);
    }
  }
})

Lucia already exposes low level APIs (e.g Auth.validateSession()), and I'd rather have users build on top of it instead of hacking into Lucia's internals.

@tobiasdiez
Copy link
Contributor Author

I think it would already be sufficient to expose config option secrets (an string array). If this is passed, lucia internally signs the session id in the cookie.

In your example, in the case if (parsedCookie === null) return null; you probably also want to log the request - since it means someone tampered with the cookie.

@pilcrowOnPaper
Copy link
Member

I'm going to keep this open since I think it's worth having the discussion (for v4), but it won't be implemented in the near feature (within v3.x) since we'll have to introduce breaking changes.

@jckw
Copy link

jckw commented Apr 21, 2024

I just contributed to the bounty on this issue.

Each contribution to this bounty has an expiry time and will be auto-refunded to the contributor if the issue is not solved before then.

Current bounty reward

To make this a public bounty or have a reward split, the maintainer can reply to this comment.

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

No branches or pull requests

3 participants