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

Session token "safety window" when refreshing #22503

Merged
merged 23 commits into from
May 21, 2024
Merged

Conversation

br41nslug
Copy link
Member

@br41nslug br41nslug commented May 15, 2024

Implement a "safety window" keeping the old token active for a short amount of time to resolve in-flight requests using the previous token.

As described here https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renewal-timeout

Fixes #22360

Scope

What's changed:

  • Session IDs no longer get replaced on refresh but a new session ID is saved while the old one is kept for a short duration of time, the so called "safety timeout" or "grace period".
  • On refresh the expired sessions for the current user will be deleted
  • Adds a new column to the directus_sessions table used to link refreshed sessions to their successor.

Potential Risks / Drawbacks

  • Potential performance impact on the refresh endpoint where deleting the sessions
  • Can impact the security if a long grace period is set

Review Notes / Questions

  • I would like to lorem ipsum
  • Do we want to call the environment variable SESSION_REFRESH_GRACE_PERIOD?
  • Is a grace period of 10seconds a good default?

This comment was marked as resolved.

@br41nslug br41nslug changed the title [WIP] Session token "safety window" when refreshing Session token "safety window" when refreshing May 15, 2024
@br41nslug br41nslug marked this pull request as ready for review May 15, 2024 12:57
@br41nslug br41nslug removed the request for review from licitdev May 15, 2024 14:01
@br41nslug br41nslug requested a review from paescuj May 15, 2024 14:42
Co-authored-by: Hannes Küttner <4376726+hanneskuettner@users.noreply.github.com>
@ComfortablyCoding ComfortablyCoding self-requested a review May 15, 2024 16:23
@br41nslug br41nslug self-assigned this May 15, 2024
licitdev

This comment was marked as resolved.

joselcvarela

This comment was marked as resolved.

Copy link
Member

@joselcvarela joselcvarela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me ✅

@hanneskuettner

This comment was marked as resolved.

@hanneskuettner hanneskuettner self-requested a review May 17, 2024 08:59
@alexchopin alexchopin added this to the Next Patch Release milestone May 17, 2024
@br41nslug br41nslug requested a review from licitdev May 17, 2024 16:43
@paescuj paescuj enabled auto-merge (squash) May 21, 2024 11:44
@paescuj paescuj merged commit 9335664 into main May 21, 2024
5 checks passed
@paescuj paescuj deleted the session-sliding-window branch May 21, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Race condition on refresh with multiple tabs
7 participants