Expose signer

[budarin]

In this PR, I propose to re-export signerFactory, sign and unsign utilities to expand the capabilities of manual sign/unsign cookies

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[mcollina]

I think it might be more idiomatic to expose it as app.signCookie as well, similar to how unsignCookie is defined in

fastify-cookie/plugin.js

Line 61 in dffa0ea

fastify.decorate('unsignCookie', unsignCookie)

.

Wdyt?

[budarin]

In this case, it seems to me, there will be 2 problems

• name conflict
• sign and unsign are independent tools for custom actions - although maybe I'm wrong

As for me the main problem is naming in this case

How do you suggest it looks like?

[mcollina]

I'm essentially ok with this change, so we can land this.

[mcollina]

lgtm

[budarin]

In principle, sign and unsign can be integrated into fastify, but I have no idea what to name them

[mcollina]

Given we already have app.unsignCookie and app.parseCookie, I would go for app.signCookie:

fastify-cookie/plugin.js

Lines 60 to 61 in dffa0ea

	fastify.decorate('parseCookie', parseCookie)
	fastify.decorate('unsignCookie', unsignCookie)

[budarin]

in this case, how should the unsign method be named?
app.unsignCookie?

[mcollina]

I think there is already app.unsignCookie(), isn't that enough?

[budarin]

I want to clarify: in our project, some cookies are also signed/unsigned with so called "dynamic secret" - user specific info, so now we use sign and unsign along with res.setCookie(..., {signed: true}) and req.unsing

Therefore, if we add signCookie(value, secret) to the plugin, then we would like to have a method something like unsignCookie(value, secret)

that's what I was talking about - the conflict of names...

[mcollina]

I want to clarify: in our project, some cookies are also signed/unsigned with so called "dynamic secret" - user specific info, so now we use sign and unsign along with res.setCookie(..., {signed: true}) and req.unsing

How are you passing these through? I don't know how somebody could do dynamic/user specific cookie signatures with this module. Have you got a reduced example for this? How about adding a test with the full usage you are envisioning those new exposed functions?

This plugin's most basic use case has one (or more) secrets for all users. In this case providing app.signCookie(value) would be more than enough.

[budarin]

How are you passing these through? I don't know how somebody could do dynamic/user specific cookie signatures with this module. Have you got a reduced example for this? How about adding a test with the full usage you are envisioning those new exposed functions?

I think these are private cases of our architects and they are not widely used, so I agree app.signCookie will be quite enough

This plugin's most basic use case has one (or more) secrets for all users. In this case providing app.signCookie(value) would be more than enough.

After I added the signCookie to fastify instance decorator in the code as

fastify.decorate('signCookie', signCookie)

ran into a problem in the test

test('create signed cookie manually using signCookie decorator', (t) => {
    const fastify = Fastify()
    fastify.register(plugin, { secret: 'secret' })

    fastify.get('/test1', (req, reply) => {
      reply.send({
        unsigned: req.unsignCookie(req.cookies.foo)
      })
    })

    fastify.inject({
      method: 'GET',
      url: '/test1',
      headers: { cookie: `foo=${fastify.signCookie('bar')}` }
    }, (err, res) => {
      t.error(err)
      t.equal(res.statusCode, 200)
      t.same(JSON.parse(res.body), { unsigned: { value: 'bar', renew: false, valid: false } })
    })
})

fastify.signCookie in this context is undefined (

[mcollina]

fastify.signCookie in this context is undefined (

What stacktrace are you getting?

[budarin]

[mcollina]

I don't see that exposed anywhere.

[budarin]

I don't see that exposed anywhere.

I can not push the code changes due to failed tests

[mcollina]

git commit -n

[mcollina]

Basically you need await app.register(...)

[mcollina]

lgtm

[budarin]

Basically you need await app.register(...)

I got it - I relied on the style and code of existing tests, was surprised, but copied the code as it is)

[mcollina]

I landed #194 and now this conflicts. Coul you move these exports to the plugin.js file instead?

[budarin]

Ок

[budarin]

How then should the import of utilities look like?

const cookie = require('@fastify/cookie');
const { signerFactory , sign, unsign } = cookie;

[mcollina]

that works

[budarin]

Do I understand correctly that exporting to plugin.js should it look like this?

/**
 * These export configurations enable JS and TS developers
 * to consume fastify-cookie in whatever way best suits their needs.
 * Some examples of supported import syntax includes:
 * - `const fastifyCookie = require('fastify-cookie')`
 * - `const { fastifyCookie } = require('fastify-cookie')`
 * - `import * as fastifyCookie from 'fastify-cookie'`
 * - `import { fastifyCookie } from 'fastify-cookie'`
 * - `import fastifyCookie from 'fastify-cookie'`
 */
fastifyCookie.fastifyCookie = fastifyCookie
fastifyCookie.default = fastifyCookie
module.exports = fastifyCookie

fastifyCookie.fastifyCookie.signerFactory = signerFactory;
fastifyCookie.fastifyCookie.sign = sign;
fastifyCookie.fastifyCookie.unsign = unsign;

module.exports.signerFactory = signerFactory;
module.exports.sign = sign;
module.exports.unsign = unsign;

[climba03003]

Either

fastifyCookie.signerFactory = signerFactory;
fastifyCookie.sign = sign;
fastifyCookie.unsign = unsign;

or

plugin.signerFactory = signerFactory;
plugin.sign = sign;
plugin.unsign = unsign;

[Uzlopak]

Can we actually integrate cookie-signer into our codebase? I mean the code is trivial.
https://github.com/tj/node-cookie-signature/blob/master/index.js

Also why do they replace the characters when signing? \ is not part of base64?
https://github.com/tj/node-cookie-signature/blob/7deca8b38110a3bd65841c34359794706cc7c60f/index.js#L23

[Uzlopak]

We could theoretically also allow to set the algorithms of the signer, if we integrate it into this package

[climba03003]

Can we actually integrate cookie-signer into our codebase? I mean the code is trivial. https://github.com/tj/node-cookie-signature/blob/master/index.js

Also why do they replace the characters when signing? \ is not part of base64? https://github.com/tj/node-cookie-signature/blob/7deca8b38110a3bd65841c34359794706cc7c60f/index.js#L23

I believe they are stripping = only. \= is = in RegExp.

[Uzlopak]

But = isnt a special character in regex. So there is no escaping necessary. I have the feeling that they wanted to remove the / character from base64.

[climba03003]

But = isnt a special character in regex. So there is no escaping necessary. I have the feeling that they wanted to remove the / character from base64.

It will works with or without the \. But it didn't hurt for explicit escaping.
= should be escaped since it affect the header. / doesn't.

If you believe it is wrong, I suggest you to open a issue in their repo for clarification.

[mcollina]

lgtm