v5.3.0 breaks with `Cannot read properties of undefined (reading 'sign')"`

[Tirke]

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

4.9.2

Plugin version

5.3.0

Node.js version

16.10.0

Operating system

macOS

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

12.6

Description

We bumped the package to 5.3.0 and immediately experienced the Cannot read properties of undefined (reading 'sign')" error.

I then investigated this issue. It comes from changes in @fastify/cookie and the fact that @fastify/secure-session now relies on @fastify/cookie@8.3.0 whereas it was using @fastify/cookie@7.4.0 before.

The error comes from the fastifyCookieSetCookie method especially those lines https://github.com/fastify/fastify-cookie/blob/8d3feb35ab82016554d718fc4a450bd828227bcf/plugin.js#L14-L16

if (opts.signed) {
    value = signer.sign(value)
  }

Obviously the error comes from the fact that signer is undefined. Why is that?

It's because fastifyCookieSetCookie is called with signer as undefined whereas in 5.2.0 it was not. And it is undefined because when the plugin is initialized signer is never set due to changes in @fastify/cookie@8.3.0.

We are going to analyse the plugin function of @fastify/cookie which is always initialized with an empty options params in the context of @fastify/secure-session (at least in our case). (options = {})

In @fastify/cookie@7.4.0 the plugin function starts like this

function plugin (fastify, options, next) {
  const secret = options.secret || ''
  const enableRotation = Array.isArray(secret)
  const algorithm = options.algorithm || 'sha256'
  const signer = typeof secret === 'string' || enableRotation ? new Signer(secret, algorithm) : secret

So, options is empty and the || case will assign empty string '' to secret and signer will become new Signer(secret, algorithm), we can sign cookies!

In @fastify/cookie@8.3.0 the plugin function starts like this

function plugin (fastify, options, next) {
  const secret = options.secret
  const hook = getHook(options.hook)
  if (hook === undefined) {
    return next(new Error('@fastify/cookie: Invalid value provided for the hook-option. You can set the hook-option only to false, \'onRequest\' , \'preParsing\' , \'preValidation\' or \'preHandler\''))
  }
  const isSigner = !secret || (typeof secret.sign === 'function' && typeof secret.unsign === 'function')
  const algorithm = options.algorithm || 'sha256'
  const signer = isSigner ? secret : new Signer(secret, algorithm)

options is empty so secret is undefined. isSigner is true because of !undefined. signer is undefined because isSigner is true and secret is undefined.

That's how we end up with an undefined signer and a crashing lib :)

Steps to Reproduce

Use latest version of the package and try to sign a cookie. It will crash. We bootstrap secureSession like that (NestJS app)

await app.register(secureSession, {
    secret: 'secret-string',
    salt: 'salt',
  })

Expected Behavior

I don't know if it's on the cookie lib or the fact that options are always empty but the release is not working for us :( I'm wondering if the issue is not more important than that because even on 5.2.0 it looks like we sign cookie with an empty string as the secret which is...bad? Regards :)

[mcollina]

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

[wilkmaia]

@Tirke I've tried with the following snippet and couldn't reproduce the issue you mentioned. Could you maybe show me where I got it wrong?

Environment

Windows host - NodeJS v16.10.0

Snippet

const Fastify = require('fastify')
const secureSession = require('@fastify/secure-session')

const run = async () => {
  const app = Fastify()

  await app.register(secureSession, {
    secret: 'averylogphrasebiggerthanthirtytwochars',
    salt: 'mq9hDxBVDbspDR6n',
  })

  app.post('/', (request, reply) => {
    request.session.set('data', request.body)
    reply.send('session set')
  })

  app.get('/', (request, reply) => {
    const data = request.session.get('data')
    if (!data) {
      reply.code(404).send()
      return
    }
    reply.send(data)
  })

  app.get('/all', (request, reply) => {
    // get all data from session
    const data = request.session.data()
    if (!data) {
      reply.code(404).send()
      return
    }
    reply.send(data)
  })

  app.listen({ port: 3000 })
}

run()

package.json

{
  "dependencies": {
    "@fastify/secure-session": "5.3.0",
    "fastify": "4.9.2"
  }
}

---

For the sake of completeness I've also run the same snippet using the current versions (NodeJS v18.14.0, fastify v4.12.0, @fastify/secure-session v6.0.0) and got the same result.

There's the remote possibility of it being an OS-related issue but, for the reason I expain below, I find it unlikely at this point.

---

Also, by taking a look at the code of the plugins I couldn't find any piece of code setting the signed flag in the options object. With that flag not being set the code shouldn't reach the line where signer is undefined. With the default cookie settings (i.e. not passing cookie in the plugin's options) an empty object is passed to setCookie. The signed option should always be undefined in that scenario.

Maybe I'm missing something but I'm not being able to either identify the issue or reproduce it in order to work on a fix.

[simoneb]

I can confirm this is also working for me, with an independent repro. @Tirke thanks for the analysis nonetheless, feel free to reopen this issue if for any reason you still cannot workaround the problem you reported.

[Tirke]

@simoneb I still have this problem, but I realise that I haven't provided you with a minimal reproduction, so I'm opening a new issue with a minimal reproduction to show that the problem does exist.