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

Button to invalidate all existing session #1017

Open
ekzyis opened this issue Apr 4, 2024 · 11 comments
Open

Button to invalidate all existing session #1017

ekzyis opened this issue Apr 4, 2024 · 11 comments
Labels
feature new product features that weren't there before needs specification question further information is required

Comments

@ekzyis
Copy link
Member

ekzyis commented Apr 4, 2024

Is your feature request related to a problem? Please describe.

We don't have a button to invalidate all existing sessions.

Describe the solution you'd like

Button to invalidate all existing sessions. Could be in the settings.

Github for reference:

2024-04-04-172226_1920x1080_scrot

Describe alternatives you've considered

n/a

Additional context

https://stacker.news/items/493900

Since we don't want to store any IPs or other identifiable information along session (like user agent), I don't think we can show geolocation data etc. alongside a session.

But as a MVP, a simple button that kills all sessions is imo enough anyway (every except the current one?). No need to show and kill active individual session.

Sats should go to @frstdrgn

@ekzyis ekzyis added the feature new product features that weren't there before label Apr 4, 2024
@huumn
Copy link
Member

huumn commented Apr 4, 2024

We don't store sessions so I'm not sure how we'll manage to do this. It's all stateless.

@huumn huumn added the question further information is required label Apr 4, 2024
@SatsAllDay
Copy link
Contributor

Just thinking out loud here...

  1. Add a button in settings that sets a invalidateSessions timestamp in the DB somewhere, maybe an SessionInvalidationRequest table.
  2. Add middleware to nextjs that checks the DB for such an entry on requests when the user already has a session. If an entry is found, respond with a Set-Cookie header that clears the existing session cookie (the equivalent of a logout operation, I presume).
  3. Somehow we'd have to only clear the session if it was created before the latest SessionInvalidationRequest entry. Otherwise, you'd end up in an infinite loop where sessions are constantly revoked.

The first two steps seem reasonable, but the third I am not sure about.

@frstdrgn
Copy link

frstdrgn commented Apr 9, 2024

I didn't realize the sessions were stateless, I'll take a look at this and try to come up with some ideas.

Add middleware to nextjs that checks the DB for such an entry on requests when the user already has a session. If an entry is found, respond with a Set-Cookie header that clears the existing session cookie (the equivalent of a logout operation, I presume).

Doesn't sound like this actually invalidates the cookie? Which in my mind is the security concern...

This actually leaves me with a question - do sessions expire at all?

@frstdrgn
Copy link

frstdrgn commented Apr 9, 2024

@ekzyis if sessions don't expire at all, I think that should be separate security issue fwiw

@ekzyis
Copy link
Member Author

ekzyis commented Apr 9, 2024

Doesn't sound like this actually invalidates the cookie? Which in my mind is the security concern...

It would since any JWT that was created before user hit "invalidate all sessions" will no longer be accepted. So it's effectively invalidated, no?

This actually leaves with a question - do sessions expire at all?

Default expiration is 30 days but they are regular refreshed while you're logged in and on the site.

@frstdrgn
Copy link

frstdrgn commented Apr 9, 2024

@ekzyis , clearing the session cookie via a Set-Cookie header doesn't invalidate the session, it simply removes it from the browser. See the screenshots, showing the "me" GraphQL query returning my user data while I'm logged in, the sign out request setting the cookie to be empty, and the same GraphQL query returning my user data again even AFTER the sign out request. You'll have to take my word for it that it's the same cookie (obviously don't want to send that in plain view), and take a look at the time stamps. You can also try this on your own by grabbing the cookie from your browser and placing it back in after logging out, or using Burp Suite repeater (which is free, but takes a few minutes to set up if you haven't used it before).
logged_in
signout
post-signout

Other thing worth noting here is that the refresh request is being sent every second, which means every second a new valid session cookie is being generated and won't expire for 30 days.

Personally I would recommend that be sent significantly less often, but these are all low or informational level risks.

@SatsAllDay
Copy link
Contributor

Clearing the cookie via Set-Cookie alone doesn't invalidate the cookie. In my proposal, there would be additional requirements to track when sessions were requested to be invalidated, then some additional logic server-side to consult when the incoming jwt/cookie was generated, compare to the request to invalidate sessions, and conditionally allow/deny access accordingly. This approach does not work in today's codebase without modifications being made.

@ekzyis
Copy link
Member Author

ekzyis commented Apr 9, 2024

This sounds interesting:

A common approach for invalidating tokens when a user changes their password is to sign the token with a hash of their password. Thus if the password changes, any previous tokens automatically fail to verify. You can extend this to logout by including a last-logout-time in the user's record and using a combination of the last-logout-time and password hash to sign the token. This requires a DB lookup each time you need to verify the token signature, but presumably you're looking up the user anyway.

-- stackoverflow.com, Invalidating JSON Web Tokens

Instead of storing a date and having additional custom logic to check if a JWT was revoked, we could sign JWTs with unique keys per user. For example, we could generate some random data when a user is created and derive a new secret key from this random data + our secret in NEXTAUTH_SECRET. When they click on "invalidate all sessions", this secret would be updated.

If we would provide this derived secret here:

export const getAuthOptions = req => ({
callbacks: getCallbacks(req),
providers,
adapter: PrismaAdapter(prisma),
session: {
strategy: 'jwt'
},
pages: {
signIn: '/login',
verifyRequest: '/email',
error: '/auth/error'
},
events: getEventCallbacks()
})
export default async (req, res) => {
await NextAuth(req, res, getAuthOptions(req))
}

This "more native solution" could "magically" "just work" then (assuming it's really that easy and there are no pitfalls).

Maybe we could leverage overriding encode and decode for this:

jwt: {
  async encode(params: {
    token: JWT
    secret: string
    maxAge: number
  }): Promise<string> {
    // return a custom encoded JWT string
    return "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c"
  },
  async decode(params: {
    token: string
    secret: string
  }): Promise<JWT | null> {
    // return a `JWT` object, or `null` if decoding failed
    return {}
  },
}

@frstdrgn
Copy link

Yeah I think this is the simplest solution, and it sounds way easier than keeping a database of session tokens. This also gives you the option of resetting the key automatically every X days, so that a user can't refresh their session indefinitely. Can't see any meaningful risks with that idea as long as the user keys are generated via a CSPRNG and aren't exposed to the user.

@frstdrgn
Copy link

Would you need to override encode/decode if all you're doing is providing a different secret?

@ekzyis
Copy link
Member Author

ekzyis commented Apr 10, 2024

Would you need to override encode/decode if all you're doing is providing a different secret?

You are right, we don't afaict. No longer know what I was thinking in my previous comment lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new product features that weren't there before needs specification question further information is required
Projects
None yet
Development

No branches or pull requests

4 participants