unjs
fix: make session clearing resilient
This PR addresses an issue in the session manager utility which results in an already cleared session returning when useSession, session.update, getSession, updateSession, or sealSession are used after calling session.clear or clearSession.
This ensures that after clearing the session, a new blank session is initialized on the event context so that subsequent session mutation calls cause updates on the newly initialized session, rather than restoring the original session from the event context.
There are still edge-cases where the session can be restored, but they require subverting the use of the provided methods and session manager. This fix largely relies on the understanding that the event.context.sessions object is the source of truth for expected session state, but in the absence of a valid session in the context, the original session will still be reconstructed.
The documentation for the session management has also been updated to add a section on session rotation, which should lend in resolving issues with the downstream nuxt-auth-utils issues.
Closes #1004
Just out of curiosity: I imagine this will have a cost on memory 🤔
This is a very small object that is already instantiated on the event.context.sessions object in almost all other conditions, so it doesn't really use any more memory than before, though it might slightly reduce memory savings by clearing the session compared to the current implementation.
As I looked at the code, though, I did realize that it deletes a property (something that normally should be avoided) and then redeclares the same property for the new session object. As such, the https://github.com/unjs/h3/pull/1010/commits/370ce915e6fcf7271f5fdef8914ac4323374913a commit essentially combines the calls to make it a clobber operation. Ultimately, I expect it will have 0% impact on the final runtime performance, but it looks a little bit cleaner this way 😝.
Thanks for the well-written explanations.
For session rotation, I believe we might need a new util or at least flag on clear, normal clear() means logout and should actually clear all data. Also, by having an explicit way of calling rotate, we can make user usage easier to not make them cache previous value and restore with 3 lines of code but only renew createdAt + id
The refactors of this PR look nice addition, btw. Can you please convert this PR to a refactor only and then a subsequent to draft idea for explicit rotation?
@pi0, updates to remove documentation about session rotation have been applied. I agree with your point about clear() and for clarity this implementation does remove the data and does setup a new session stub to prevent session restoration by the next session call, but because it bypasses the cookie setting phase it won't actually propagate a new session until updateSession() is called.