Use originalMaxAge in sessionData to implement `rolling`

[rclmenezes]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have searched existing issues to ensure the bug has not already been reported

Fastify version

9.0.0

Plugin version

No response

Node.js version

16.6.0

Operating system

macOS

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

12.4

Description

Quoth the documentation for rolling:

Forces the session identifier cookie to be set on every response. The expiration is reset to the original maxAge - effectively resetting the cookie lifetime.

[I added this documentation myself[(https://github.com/fastify/session/commit/3c13194d4f2568cebd53bde2eca3a0a7ca58ba4e) based on the comments in the typing.

However, we don't actually follow this behavior! The Cookie class doesn't take any of the sessionData as a parameter, only cookieOpts: https://github.com/fastify/session/blob/master/lib/cookie.js#L4

This means that it will use the maxAge passed in cookieOpts instead.

This actually creates another compatibility issue with express-session. express-session stores originalMaxAge in the session, which looks something like this in Redis:

"{\"cookie\":{\"originalMaxAge\":900000,\"expires\":\"2022-08-14T18:31:01.402Z\",\"httpOnly\":true,\"path\":\"/\"},\"csrfSecret\":\"dL8EJsNJ4TzlRNjyjztfnnSX\"}"

@fastify/session will read this session and overwrite originalMaxAge as maxAge (not that it ever reads it):

"{\"cookie\":{\"maxAge\":900000,\"path\":\"/\",\"httpOnly\":true,\"secure\":\"auto\",\"expires\":\"2022-08-14T18:31:24.145Z\",\"sameSite\":null,\"domain\":null},\"csrfSecret\":\"dL8EJsNJ4TzlRNjyjztfnnSX\"}"

Finally, when express-session reads that session, it will conclude that there was no originalMaxAge and it will remove maxAge entirely:

"{\"cookie\":{\"path\":\"/\",\"httpOnly\":true,\"secure\":\"auto\",\"expires\":\"2022-08-14T18:31:24.145Z\",\"sameSite\":null,\"domain\":null},\"csrfSecret\":\"dL8EJsNJ4TzlRNjyjztfnnSX\"}"

Since originalMaxAge is missing, this will also mean that rolling will stop working within express-session.

Steps to Reproduce

• Instantiate with cookie.maxAge set to 1000
• Start a session
• Change cookie.maxAge to 2000
• Observe that cookie.maxAge is changed to 2000. Also observe that cookie.maxAge is saved in session instead of cookie.originalMaxAge

Expected Behavior

No response

[Uzlopak]

@rclmenezes

Please review first #125, then we can touch more regarding the cookie.

[Uzlopak]

Can you provide a unit test for it, please?

[Uzlopak]

@rclmenezes

See #131 kind of removed rolling and basically nothing breaks lol.

[Uzlopak]

@rclmenezes

You still active or am I alone in this struggle :D?

[rclmenezes]

Sorry my dude! Been really busy the past few days.

Yes, I can make a unit test for it. Honestly there are two todos:

1. Store maxAge as originalMaxAge so we can interact safely with express-session
2. Actually use the values in session.cookie when recreating the cookie to adhere to the intended behavior in the documentation

Anywho, I'll make a unit test for both, gimme a beat

Also -- thank you for removing Session.restore in that PR. Totally agree with that direction

[rclmenezes]

This test ensures that we save maxAge as session.cookie.originalMaxAge, like express-session:

test('stores maxAge as originalMaxAge, just like express-session', async (t) => {
  t.plan(2)

  const fastify = await buildFastify((request, reply) => {
    // We should see cookie.originalMaxAge as soon as the session is created
    t.equal(request.session.cookie.originalMaxAge, 42)
    reply.send(200)
  }, {
    secret: DEFAULT_SECRET,
    maxAge: 42,
  }, plugin)
  t.teardown(() => fastify.close())

  const response = await fastify.inject({
    url: '/',
    headers: { 'x-forwarded-proto': 'https' }
  })
  t.equal(response.statusCode, 200)
})

This next test ensures that we continue using originalMaxAge when rolling, not the options passed in cookie:

test('continues to use originalMaxAge when rolling, not the new maxAge', async (t) => {
  t.plan(4)

  const oldMaxAge = 1000
  const newMaxAge = 42

  const plugin = fastifyPlugin(async (fastify) => {
    fastify.get('/check', (request, reply) => {
      // originalMaxAge will stay the same, even if maxAge is different
      t.equal(request.session.cookie.originalMaxAge, oldMaxAge)
  
      // expires gets reset to the maxAge, which is higher than newMaxAge
      t.ok(request.session.cookie.expires > Date.now() + newMaxAge)

      reply.send(200)
    })
  })
  
  const fastify = await buildFastify((request, reply) => {
    // Set the session store with the oldMaxAge value
    request.sessionStore.set(DEFAULT_SESSION_ID, {
      cookie: { secure: false, httpOnly: true, path: '/', originalMaxAge: oldMaxAge, expires: new Date(Date.now() + 10), }
    }, () => {
      reply.send(200)
    })
  }, {
    secret: DEFAULT_SECRET,
    rolling: true,
    cookie: {
      maxAge: newMaxAge,
      secure: false,
    }
  }, plugin)
  t.teardown(() => fastify.close())

  const response1 = await fastify.inject({
    url: '/',
    headers: { Cookie: DEFAULT_COOKIE }
  })
  t.equal(response1.statusCode, 200)

  const response2 = await fastify.inject({
    url: '/check',
    headers: { Cookie: response1.headers['set-cookie'] }
  })
  t.equal(response2.statusCode, 200)
})

[Uzlopak]

I was wondering if the unit test of #162 was enough, So I checked your provided unit tests here and can confirm that the implemented unit test is sufficient and the feature works as expected.

Thanks again :)

Closing because it is resolved.