Passing `signed: true` in options breaks session decoding.

[SkeLLLa]

🐛 Bug Report

Passing signed: true (I know that this makes not too much sense) to underlying fastify-cookie breaks session decoding.

To Reproduce

Steps to reproduce the behavior:

await app.register(fastifySecureSession, {
    // the name of the session cookie, defaults to 'session'
    cookieName: 'ses',
    // adapt this to point to the directory where secret-key is located
    key: await fs.readFile('key-file'),
    cookie: {
      signed: true,
    },
  });

In this case fastify-cookie signature will be counted as a part of nonce https://github.com/fastify/fastify-secure-session/blob/master/index.js#L96 and decoding will fail

Expected behaviour

Maybe it worth to mention this in docs or somewhere else until signing will be fully implemented. Or signature(all that goes after the .) could be removed from nonce part

[mcollina]

I think this should be properly fixed - or maybe we should throw.

As far as I understand the code, this should work - there might be something odd at play - maybe some incompatibilities between cookie-signature and this module.

[SkeLLLa]

@mcollina

Just to make it clear what fastify-cookie does in case of signed=true:

E.g. if we have cookie foo=bar it adds signature foo=bar.<signature>.

In case of fastify-secure-session it splits cookie https://github.com/fastify/fastify-secure-session/blob/master/index.js#L77 by ;. First part is cypher text, rest - is considered as nonce. But if we add signed=true in "rest" part we receive not only nonce, but signature as well: foo=<cypher_text>;<nonce>.<signature> So this module considers that nonce part is <nonce>.<signature> and checks it length that should be 24. But obviously it would be longer because of .<signature>.

So maybe it just worth to check if there's a dot symbol in nonce part and either ignore the signature. Or maybe first - validate signature, then split to cypher and nonce ignoring symbols after .

[mcollina]

What I do not understand is why the .signature part is still there. We are getting it from fastify-cookie and cookie-signature.. and the signature part should have already be extracted.

(FYI, using cookie-signature with this module does not add any security benefits).

[mariusa]

Just found this issue after researching why the session doesn't work. signed: false makes it work.

FYI express cookie stores the signature in a different cookie with same options, named cookie.name.sig. Maybe this is more clear and avoids bugs like above.

[mcollina]

https://github.com/fastify/fastify-cookie/blob/master/package.json#L51 depends on cookie-signature, which introduce this problem. I think it would be better to solve this problem there.

[bcomnes]

What I do not understand is why the .signature part is still there. We are getting it from fastify-cookie and cookie-signature.. and the signature part should have already be extracted.

(FYI, using cookie-signature with this module does not add any security benefits).

https://github.com/fastify/fastify-cookie/blob/master/package.json#L51 depends on cookie-signature, which introduce this problem. I think it would be better to solve this problem there.

Just to understand what you think the solution to the problem is (roughly):

https://github.com/fastify/fastify-secure-session/blob/9e723b30eb46d9fd874b6c7e79c03dd31b0b7df0/index.js#L145-L151

The cookie should not contain the .<signature> suffix added by fastify-cookie when signing is enabled at that point?

[mcollina]

I think so, yes. A lot of time has passed, my memory is a bit blurry on this detail.