Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signed cookies are not supported #13

Closed
jurca opened this issue Feb 11, 2020 · 5 comments · Fixed by #14
Closed

Signed cookies are not supported #13

jurca opened this issue Feb 11, 2020 · 5 comments · Fixed by #14

Comments

@jurca
Copy link
Contributor

jurca commented Feb 11, 2020

Hello again,

setting the cookie.signed flag true in the options results in fastify-cookie signing the cookies being set by this middleware, however, fastify-csrf does not properly handle reading signed cookies, since they need to be "unsigned" to get to the raw value.

I recommmend adding an if (cookie.signed) { ... } check to the getSecret() function to make this work.

I am willing to submit a PR, if you'd prefer me to do so.

Thank you in advance.

@Tarang11
Copy link
Member

Tarang11 commented Feb 12, 2020

Can you please give an example.

@jurca
Copy link
Contributor Author

jurca commented Feb 12, 2020

Quite busy now, I'll try to get to it on Sunday evening. Thank you for your patience.

@jurca
Copy link
Contributor Author

jurca commented Feb 16, 2020

Here's an example code that results in HTTP 500 'invalid csrf token' error when the form is submitted:

const fastify = require('fastify')()
const fastifyCookie = require('fastify-cookie')
const fastifyFormBody = require('fastify-formbody')
const fastifyCSRF = require('fastify-csrf')

fastify.register(fastifyCookie, {
  secret: 'my-secret',
})
fastify.register(fastifyFormBody)
fastify.register(fastifyCSRF, {
  cookie: {
    signed: true,
  },
})

fastify.get('/', (request, reply) => {
  reply.type('text/html; charset=utf-8')
  reply.send(`
    <!DOCTYPE html>
    <html>
      <head><meta charset="UTF-8"><title>Signed cookie test</title></head>
      <body>
        <form action="/" method="post">
          <input type="hidden" name="_csrf" value="${request.csrfToken()}">
          <button>Test signed CSRF cookie</button>
        </form>
      </body>
    </html>
  `)
})

fastify.post('/', (request, reply) => {
  reply.type('text/plain; charset=UTF-8').send('CSRF OK')
})

fastify.listen(3000)

Removing the signed: true, line, making the CSRF cookie not signed, resolves the issue.

Now, I know it's not really needed security-wise to have the cookie signed, however, it would be nice to either support it, or drop the flag with a warning to the console.

@Tarang11
Copy link
Member

Confirmed. Pls submit PR

@jurca
Copy link
Contributor Author

jurca commented Feb 19, 2020

Sorry for the delay, the PR is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants