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

Remove problematic customization of session callback #435

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

ulrikandersen
Copy link
Contributor

@ulrikandersen ulrikandersen commented Nov 6, 2024

Description

This PR fixes the customization on the Authjs session callback.

The customization was suppose to only add id to the user object of the session callback response (exposed via /api/auth/session), but when the previous customization, the callback also started returning otherwise secret information - namely the "sessionToken". This is really unexpected and poses a security risk.

It is especially a problem because the session token is suppose to only be stored in an HttpOnly cookie in the browser and on the server side (database), making it inaccessible to JavaScript. But with the /api/auth/session endpoint returning the session token it is easily accessible from JavaScript by doing a network request.

Reading the session token from JavaScript is problematic because it can be used as part of a session hijacking attack using XSS.

With this fix the session object is explicitly constructed to follow the DefaultSession (source) with only the user.id added.

Motivation and Context

With this seemingly innocent modification to the session callback (which is clearly documented in Authjs documentation)

callbacks: {
  session({ session, user }) {
    session.user.id = user.id
    return session
  },
}

the GET /api/auth/session endpoint starts returning a lot more session information:

{
    "id": 27,
    "userId": 5,
    "expires": "2024-12-06T04:43:00.371Z",
    "sessionToken": "abcdefg-4fb0-4145-9787-421e3465bc49", <---- !!!
    "user": {
        "id": 5,
        "name": "Firstname Lastname",
        "email": null,
        "emailVerified": null,
        "image": "https://avatars.githubusercontent.com/u/114411111?v=4"
    }
}

Without the modification the response looks like this (which is safe and expected):

{
    "user": {
        "name": "Firstname Lastname",
        "email": null,
        "image": "https://avatars.githubusercontent.com/u/114411111?v=4"
    },
    "expires": "2024-12-06T04:43:00.371Z"
}

The root cause is likely that with the modification the callback starts returning AdapterSession (source) instead of Default Session (source).

With this PR only the user.id and property is added:

{
    "user": {
        "id": 5,
        "email": null,
        "name": "Firstname Lastname",
        "image": "https://avatars.githubusercontent.com/u/114411111?v=4"
    },
    "expires": "2024-12-06T04:43:00.371Z"
}

Screenshots (if appropriate):

From the Authjs documentation (https://authjs.dev/guides/extending-the-session#with-database).
Screenshot 2024-11-06 at 11 07 38

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@ulrikandersen ulrikandersen marked this pull request as ready for review November 6, 2024 10:12
@simonbs
Copy link
Contributor

simonbs commented Nov 6, 2024

@ulrikandersen Do you know if this has any consequences? Will removing the user ID break anything?

@ulrikandersen
Copy link
Contributor Author

ulrikandersen commented Nov 6, 2024

@ulrikandersen Do you know if this has any consequences? Will removing the user ID break anything?

I have been testing locally with both an empty database and with existing users and I have not found it to break anything. Perhaps it is a leftover from when we were working on the separate guest flow.

We are accessing the session.user object in UserButton, but we do not use the id.

We do however have a getUserId method in AuthjsSession. I will look more into where it is being used.

@ulrikandersen ulrikandersen marked this pull request as draft November 6, 2024 10:35
@ulrikandersen ulrikandersen force-pushed the remove-session-customization branch 2 times, most recently from e70057f to 2ebae3a Compare November 6, 2024 10:50
The customization was suppose to only add "id" to the "user" object of the session callback response (exposed via /api/auth/session), but when this customization is added, the callback also starts returning otherwise secret information - namely the "sessionToken".

This is a problem because the session token is suppose to only be stored in an HttpOnly cookie in the browser and on the server side, making it inaccessible to JavaScript. But with the /api/auth/session endpoint returning the session token it is easily accessible from JavaScript by doing a network request.

With this change the session object is explicitly constructed.
@ulrikandersen ulrikandersen force-pushed the remove-session-customization branch from 2ebae3a to 7e7777d Compare November 6, 2024 10:52
@ulrikandersen ulrikandersen marked this pull request as ready for review November 6, 2024 10:55
@ulrikandersen
Copy link
Contributor Author

@simonbs We do have some functionality which depends on the user ID (PersistingOAuthTokenRefresher) so in order to not break functionality I now construct the session response to include the user ID (and only the user ID 😁).

@simonbs simonbs merged commit 2bfce0d into develop Nov 6, 2024
7 checks passed
@simonbs simonbs deleted the remove-session-customization branch November 6, 2024 11:44
@ulrikandersen ulrikandersen mentioned this pull request Nov 6, 2024
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

Successfully merging this pull request may close these issues.

2 participants