Skip to content

fix: preserve multi-instance session data on login/logout with tests …#1394

Open
Tony133 wants to merge 2 commits intomainfrom
fix/session-persistence-multi-instance-and-type-safe-test
Open

fix: preserve multi-instance session data on login/logout with tests …#1394
Tony133 wants to merge 2 commits intomainfrom
fix/session-persistence-multi-instance-and-type-safe-test

Conversation

@Tony133
Copy link
Copy Markdown
Member

@Tony133 Tony133 commented Apr 7, 2026

Proposal:

When using multiple Authenticator instances with clearSessionOnLogin: false, logging in or out with one instance would inadvertently destroy the session data of other instances.

This happened because request.session.regenerate() was called without arguments in both logIn and logOut, which wipes the entire session — including passport keys belonging to other instances (e.g. passport-a being lost when logging in via passport-b).

Changes Made:

  • In SecureSessionManager:

    • logIn: the else branch (triggered when clearSessionOnLogin: false) called request.session.regenerate() with no arguments, destroying all existing session data.

    • logOut: similarly called request.session.regenerate() unconditionally, logging out all instances instead of just the current one.

    • When clearSessionOnLogin is false, instead of calling regenerate() blindly:

      • Read the existing session data via (request.session as any).data?.() before regenerating
      • Call request.session.regenerate() to get a new secure session ID
      • Restore the saved data with request.session.set(), omitting only the key being logged out (in logOut)
  • Updated tests with better type checking

  • Update @fastify/secure-session dependency in the package.json

Note:

  • session.data() is used because Object.keys(request.session) only returns internal metadata fields (changed, deleted), not the actual session data. The method is available on both @fastify/session and @fastify/secure-session, though only declared in the types of the latter.

This PR should close this PRs:

@Tony133 Tony133 marked this pull request as ready for review April 7, 2026 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant