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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 [Bug]: Inconsistency in Session and CSRF Middleware Handling of Timeout and Expiration #2741

Open
3 of 9 tasks
sixcolors opened this issue Nov 27, 2023 · 11 comments
Open
3 of 9 tasks

Comments

@sixcolors
Copy link
Member

Bug Description

Inconsistency in Session and CSRF Middleware Handling of Timeout and Expiration

The current behaviour of both the session and CSRF middlewares introduces a notable inconsistency in handling expiration, sometimes as a timeout, deviating from established standards such as those outlined in NIST Special Publication 800-63B.

TD;DR for NIST guidelines:

Expiration refers to the overall validity period of a session and may be extended by reauthentication, while timeout is associated with inactivity and requires user activity within a defined period to prevent session termination.

Session Middleware

The session middleware currently has a predetermined endpoint for session expiration. However, there is an unexpected side-effect when sess.Save() is called, which extends the session as if it were a timeout. This behaviour is not intuitive and needs to be reviewed to comply with NIST SP 800-63B guidelines. The guidelines make a clear distinction between timeout and expiration, and the session middleware should be updated accordingly to follow these guidelines. Alternatively, developers should have some mechanism to control which actions extend a session expiration and which do not.

CSRF Middleware

The CSRF middleware treats expiration as a timeout and extends it with every request. This behaviour aligns with a specific use case but deviates from the principles outlined in the NIST documentation, which separates Timeout and Expiration.

After every request, if this middleware is used with a session, the createOrExtendTokenInStorage() function is called. This function, in turn, calls sessionManager.setRaw() which calls sess.Save(), extending the session as if it too had a timeout and not an expiration.

Recommendation

We should evaluate how expiration is handled in middlewares, eliminate inconsistencies, and align with best practices.

  1. Session Middleware:
  • Evaluate the Session middleware and consider adopting a consistent and transparent mechanism for session management, adhering to the guidance provided by NIST on distinguishing between timeout and expiration.
  1. CSRF Middleware:
  • Evaluate the CSRF middleware to align its behaviour with the recognized standards outlined in NIST documentation. Ensure that the middleware distinguishes between Timeout and Expiration, providing a secure and predictable approach to CSRF token management.
  • Evaluate how using this middleware with the Session middleware affects expiry and timeout in sessions and consider any side effects.
  1. Documentation:
  • Update the middleware documentation to provide developers with clear guidance on how session expiration, timeout, and CSRF token management work.

Additional Considerations

  • Engage the community for feedback on the proposed changes and their alignment with NIST guidelines to ensure comprehensive coverage of developer concerns and use cases.
  • Conduct thorough testing to validate that any modifications made to the middleware do not introduce regressions or disrupt existing applications.

How to Reproduce

Use middleware

Expected Behavior

Expiration and Timeout act is described in issue description

Fiber Version

<= 2.51.0

Code Snippet (optional)

No response

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
@sixcolors sixcolors self-assigned this Nov 27, 2023
@sixcolors
Copy link
Member Author

@ReneWerner87 @gaby @efectn @nickajacks1 I would appreciate your input on this.

@sixcolors
Copy link
Member Author

sixcolors commented Nov 27, 2023

Inconsisties were noted when working on: https://github.com/sixcolors/gofiber-react-session-csrf-example

@gaby
Copy link
Member

gaby commented Nov 27, 2023

@sixcolors I totally agree, the inconsistency was made worst when the fixes for csrf were done. The session middleware needs a full refactoring. It would make sense to make these two consistent given how CSRF depends on Sessions as so do other middlewares.

@ReneWerner87
Copy link
Member

yes agree
expire and timeout are a bit mixed up here
or treated unexpectedly

@rngallen
Copy link

rngallen commented Jan 5, 2024

Is this fixed yet?

@sixcolors
Copy link
Member Author

No. V3 fix. Breaking change required.

@sixcolors sixcolors added the v3 label Jan 6, 2024
@sixcolors
Copy link
Member Author

@rngallen did you have a solution or input in how to address?

@efectn efectn added this to the v3 milestone Jan 6, 2024
@rngallen
Copy link

rngallen commented Jan 6, 2024

@sixcolors Currently no solution for this, I was trying to use session store with csrf, but I discovered session expiration and session id was updated on each request. I though session id should remain constant throughout the session (before expiration) but should have option on config if user want session expiration to be updated upon each request.

@sixcolors
Copy link
Member Author

sixcolors commented Jan 6, 2024

@rngallen Ah I see. Actually this issue is probably unrelated; I'll assume you're using EncryptCookie?

If So: it is technically possible (although not necessarily beneficial) to use Session and CSRF with encrypt cookie. It's just matters what order you use them in.

See: https://docs.gofiber.io/api/middleware/encryptcookie

@rngallen
Copy link

rngallen commented Jan 6, 2024

Encrypt cookie not in use,
On csrf if I use SessionStore I found that on each request session expiration is updated as well as sessionid` is changed upon each request.
I was expecting sessionid to remain the same throughout the session period before expiration.
Should I open new issue for this?

@sixcolors
Copy link
Member Author

sixcolors commented Jan 7, 2024

Encrypt cookie not in use, On csrf if I use SessionStore I found that on each request session expiration is updated as well as sessionid` is changed upon each request. I was expecting sessionid to remain the same throughout the session period before expiration. Should I open new issue for this?

Okay, let's move the discussion to #2793 as I think there is some config/setup/session use issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

5 participants