Fix race condition for concurrent /auth/refresh calls

[hanneskuettner]

Scope

In #22360 we uncovered that concurrent requests to the /auth/refresh endpoint end up causing a race condition where the user get's logged out if following happens:

Scenario 1:

• Window A requests /auth/refresh with a valid session cookie and session identifier (which is called refreshToken in the code) token_1
• Window B requests /auth/refresh with a the same session cookie and session identifier token_1
• Window A passes the valid session check since token_1 is in the directus_sessions table
• Window A receives a new token token_2, updates that in the directus_sessions table, and receives a new session cookie with session: token_2
• Window B does not pass valid session check since token_1 is not in the directus_sessions table anymore
• Window B gets logged out

Scenario 2:

Similar two Scenario 1, but the verification are more interleaved due to the async nature of the database / auth provider / hook calls we're making.

• Window A & B both pass the valid session check since token_1 is in the directus_sessions table
• Window A updates the token to token_2 and sets the session cookie
• Window B updates the token to token_3 and sets the session cookie
• Window A does another request with while the session cookie from Window B is not yet received, and is being logged out because it still has the now invalid token_2 set.

---

Since we're using the refreshToken as a session identifier, wrapped in the session cookie, in the session mode, it should safe to not update it in the DB. The signed session cookie with the expiry time coded in takes care of invalidating the session after SESSION_COOKIE_TTL and REFRESH_TOKEN_TTL takes care of invalidating the session stored in the DB.
Not rotating the refreshToken (really the session identifier) should not impact the security as far as I

What's changed:

• Don't refresh the session identifying token when using session mode

Potential Risks / Drawbacks

Should be none

Review Notes / Questions

• Is not rotating the session identifier a problem I can't think of?

---

Fixes #22360

[changeset-bot]

🦋 Changeset detected

Latest commit: 0fd5293

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@directus/api	Patch
directus	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[br41nslug]

Since this PR prevents the session id (aka refresh token) from changing for session mode authentication we should prevent people from extracting the "session id / refresh token" from the session JWT and refreshing that like a regular json refresh token

As this will result in the same "invalid token" situation as the race condition currently does

[paescuj]

Since this PR prevents the session id (aka refresh token) from changing for session mode authentication we should prevent people from extracting the "session id / refresh token" from the session JWT and refreshing that like a regular json refresh token

Could you help me understand why this would be necessary? If a user should do so, they certainly know what they are doing.

[br41nslug]

Could you help me understand why this would be necessary?

One being functional, this is an "unintended feature" refreshing as session token as json breaks the session potentially leading to unexpected / unwanted results like getting logged out everywhere if using SSO. Imo if someone wants to use json/cookie mode you should log in using that mode not try to convert the stateful session token to a stateless access/refresh token set.
The purpose of this change is for the session ID to not change when using a stateful session and having a "workaround" that does explicitly change this undermines this fix.

So from a functional / footgun pov I believe we should be intentional with authentication and only be allowing this if there is a valid use-case to support that which outweighs the potential risks.

If this is a feature we want to support I'd be more in favor of intentionally implementing this in the login endpoint returning new credentials using a valid access_token and add proper documentation that this is possible.

If a user should do so, they certainly know what they are doing.

True but we also have to consider malicious actors, while this is not directly exploitable out of the box however having an unused / undocumented feature in authentication inherently adds risk of abuse.

A basic example would be if any token during the session gets leaked (expired or not) can then be used to create a new session, locking out the current user and persisting access to the platform indefinitely.

[paescuj]

Looks good to me so far:

• I'd even say it kinda makes more sense to not change the session identifier
• The session is still invalidated if the user logs-out
• As you mentioned, the session/refresh token will still expire when the user doesn't refresh

However, to @br41nslug's point, I agree we should make it more secure, by preventing usage of the session refresh token via other modes, ensuring the JWT expiration (SESSION_COOKIE_TTL) still takes place 👍
This would probably mean to add a new field (type or similar) to the directus_sessions collection, which we can then compare against. This would go well with the middleware changes in #22327.

[br41nslug]

This would probably mean to add a new field (type or similar) to the directus_sessions collection

I was considering something in that direction too 🤔 as i dont see many other options of telling if the specific session getting refreshed was intended to be stateless or stateful.

[paescuj]

This would probably mean to add a new field (type or similar) to the directus_sessions collection

I was considering something in that direction too 🤔 as i dont see many other options of telling if the specific session getting refreshed was intended to be stateless or stateful.

Yeah, another option could be to simply prefix session refresh token by something like s_ but then we would have to make sure the other generated tokens never receive that prefix or additionally compare the length of the token, but this feels a bit hacky.

[br41nslug]

Interesting idea 🤔 that does indeed feel risky as the tokens are auto-generated and could theoretically generate one with that exact prefix (or any other prefix that consists of characters that are used by the nanoid generator)

[br41nslug]

After some discussion it looks like this frontend solution #22457 will take priority over the authentication change done here.