supertokens / supertokens-node

Node SDK for SuperTokens core
https://supertokens.com
Other
292 stars 79 forks source link

fix: duplicate session token issue when cookieDomain is changed #813

Closed anku255 closed 5 months ago

anku255 commented 6 months ago

Summary of change

On changing cookieSameSite or cookieSecure config

Changing these values simply overwrites the existing cookies with updated values.

On request spike

If an authenticated API is called after cookie domain has changed and token has expired then it would call the authenticated API 3 times while refresh API 2 times to get back to the normal state. Here is a screenshot -

image

On decision to clear cookies from refresh api layer

Originally, we planned to remove duplicate cookies from all session functions, including getSession, createSession, and refreshSession. However, some JavaScript frameworks don't permit reading the response cookie header, preventing us from setting multiple cookies with the same name and removing duplicates effectively. This approach would have led to inconsistency. Therefore, we decided to clear cookies only from the refresh API endpoint.

There was also a discussion regarding clearing cookies from getSession / verifySession along with refresh endpoint API but since this would have led to inconsistent behaviour among different JS frameworks, we decided against it. The saving in API call wasn't worth the inconsistency.

Related issues

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

Documentation changes

(If relevant, please create a PR in our docs repo, or create a checklist here highlighting the necessary changes)

Checklist for important updates


Ellipsis :rocket: This PR description was created by Ellipsis for commit 771782a80c44697e8e43d354e3547382dead7e50.

Summary:

This PR adds a new olderCookieDomain config option to handle cases where the cookieDomain is changed and fixes an issue where the access token wasn't cleared if the refresh token API was called without a refresh token.

Key points:


Generated with :heart: by ellipsis.dev

rishabhpoddar commented 6 months ago

Different cases to consider:

Case 1

Behaviour of API call:

Case 2

Behaviour of API call:

Case 3

Behaviour of API call:

Case 4

Behaviour of API call:

Case 5

Behaviour of API call:

Case 6

Behaviour of API call:

Case 7

Behaviour of API call:

Case 8

Behaviour of API call:

Case 9

Behaviour of API call:

Case 10

Behaviour of API call:

Case 11

Behaviour of API call:

Case 12

Behaviour of API call:

Case 13

Behaviour of API call:

Case 14

Behaviour of API call:

Case 15

Behaviour of API call:

Case 16

Behaviour of API call:

anku255 commented 6 months ago

I tested all the cases and the behavior is quite uniform for all of them.

The gist is that the olderCookieDomain needs to be exactly same as the older cookie domain or "" if it wasn't set.

Once the cookie domain is changed, we will end up setting session cookies with the newer cookie domain while the older ones will be removed only after refresh endpoint is called. The refresh endpoint will be called the moment we get a 401 and this would happen once the older tokens expire.

All the cases are listed below:

Case 1

Behaviour of API call:

Case 2

Behaviour of API call:

Case 3

Behaviour of API call:

Case 4

Behaviour of API call:

Case 5

Behaviour of API call:

Case 6

Behaviour of API call:

Case 7

Behaviour of API call:

Case 8

Behaviour of API call:

Case 9

Behaviour of API call:

Case 10

Behaviour of API call:

The cookies set by example.com with cookieDomain < not set > won't be sent to the request when the domain changes to api.example.com but it would be stored in the browser until it expires. The user would get logged out and after sign in, all the APIs would behave as they do normally.

Case 11

Behaviour of API call:

The cookies set by example.com with cookieDomain < not set > won't be sent to the request when the domain changes to api.example.com but it would be stored in the browser until it expires. The user would get logged out and after sign in, all the APIs would behave as they do normally.

Case 12

Behaviour of API call:

Case 13

Behaviour of API call:

The cookie domain change from .example.com to example.com wouldn't have any effect. The older cookies will get replaced by new ones as they would normally.

Case 14

Behaviour of API call:

The cookies set by api.example.com with cookieDomain "api.example.com" won't be sent to the request when the domain changes to example.com but it would be stored in the browser until it expires. The user would get logged out and after sign in, all the APIs would behave as they do normally.

Case 15

Behaviour of API call:

The cookies set by api.example.com with cookieDomain < not set > won't be sent to the request when the domain changes to example.com but it would be stored in the browser until it expires. The user would get logged out and after sign in, all the APIs would behave as they do normally.

Case 16

Behaviour of API call:

The cookies set by api.example.com with cookieDomain < not set > won't be sent to the request when the domain changes to example.com but it would be stored in the browser until it expires. The user would get logged out and after sign in, all the APIs would behave as they do normally.

rishabhpoddar commented 5 months ago

Hey @ellipsis, give me a code reivew

ellipsis-dev[bot] commented 5 months ago

OK! Reviewing this PR...


Responding to this comment by @rishabhpoddar. For more information about Ellipsis, check the documentation.

porcellus commented 5 months ago

Please explore/define the behaviors if:

porcellus commented 5 months ago

Please consider what happens if the user calls mergeIntoAccessTokenPayload (or tries to verify their email) in a case similar to Case 5 above.

anku255 commented 5 months ago

@porcellus Thanks for coming up with these cases.

After testing these cases, I have found two breaking cases:

  1. After the cookieDomain change, the updated token payload value won't be used until the older token expires. If the token payload doesn't change then its not an issue but if something like mergeIntoAccessTokenPayload is called then the updated value won't be used until the older token expires (and ultimately gets cleared by refresh call).

  2. If there are > 1 session cookies and the olderCookieDomain is set, we will clear the cookies for the older domain and return 200. If the olderCookieDomain is incorrect then it will cause an infinite loop. This can only be fixed by setting olderCookieDomain to the correct value.

NOTE:

I tested the case with multiple API domains. There are no additional issue with this setup. The issue arises when api2.example.com updates the token payload. api.example.com will still use the old token payload until the token expires while api2.example.com will use the updated token payload.

ellipsis-dev[bot] commented 5 months ago

:warning: This PR is too large for Ellipsis, but support for larger PRs is coming soon.


Generated with :heart: by ellipsis.dev

ellipsis-dev[bot] commented 5 months ago

Sorry, Ellipsis encountered a problem while reviewing this PR at commit 6482c7e28a4f540a19bed7dbe2b00b895369dd09. Our team has been alerted and is investigating. (wflow_A6zrl1XD5puDh9rY) :robot: