search

fastify / fastify-secure-session

Create a secure stateless cookie session for Fastify
MIT License
208 stars 48 forks source link

New cookie is made on every request, even if session is unchanged #144

Closed fa-sharp closed 2 years ago

fa-sharp commented 2 years ago

Prerequisites

Fastify version

3.27.4

Plugin version

4.1.1

Node.js version

16.x

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Ubuntu 20.04.4 LTS

Description

I recently updated from fastify-secure-session v3.0.0 to v4.1.1. I'm now finding that even if there are no changes made to the Session data, a new cookie/session is being issued on every request.

With some debugging that I did, I suspect the issue is in the Proxy wrapper around the Session object, and how it interacts with the Session's changed and deleted properties. For example, when calling session.changed = signingKeyRotated (line 150 below), the Proxy wrapper calls the 'set' method on the Session, and the changed property is added to the actual Session data. And therefore the library thinks that the Session has been changed on every request, and issues a new cookie. https://github.com/fastify/fastify-secure-session/blob/e7b326983529882a4fb5dfd358c24c46d5b1f4f9/index.js#L149-L151

This might happen in other places where session.changed or session.deleted is modified as well. Unfortunately I don't know enough about how Proxy works to know how to fix this (I would make a PR if I did!)

Steps to Reproduce

  1. Enable 'trace' logging for fastify
  2. Register fastify-secure-session plugin
  3. Create a new session and get the cookie
  4. Make a request using the cookie, but don't make any changes to the session in the route handler (don't call session.set(), or session.foo = bar, etc.
  5. fastify-secure-session reports that changes have been made to the session, and a new cookie is issued and sent to the user.

Expected Behavior

A new cookie/session should not be issued if there are no changes made to the session data. Also, the changed property should not be added to the session data itself.

mcollina commented 2 years ago

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

fa-sharp commented 2 years ago

OK, I think I figured out where the issue is. I'll be making a PR shortly