feature: make sure csurf work correctly when user repeatedly call `cs…

[Yu-Jack]

Introduction

Recently, I answered a question in the stackoverflow. The user repeatedly call csurf then put it into express middleware. It result in a error message "invalid csrf token" when user make form post at first time.

It only happen in cookie mode, not session mode.

Reason

This is a sample code make this happen.

Reproduced Way:

1. Open a new incognito window (it must be a new window which doesn't have cookie of localhost:8080)
2. Go to http://localhost:8080/settings
3. Press the submit button

// my server.js
const cookieParser = require('cookie-parser')
const csrf = require('csurf')
const bodyParser = require('body-parser')
const express = require('express')
const app = express()
app.set("view engine", "ejs")
app.use(bodyParser.urlencoded({ extended: false }))
app.use(cookieParser())
// ---> mark1
app.use(csrf({ cookie: true }))
app.use((req, res, next) => {
    res.locals.token = req.csrfToken();
    next();
});

// ---> mark2
const csrfProtection = csrf({ cookie: true })
app.get('/settings', csrfProtection, function (req, res) {
    res.render('send')
})
app.post('/settings', csrfProtection, function (req, res) {
    res.send('data is being processed')
})
app.listen(8080)

<!-- my send.ejs -->
<form method="POST" action="/settings">
  <input type="hidden" name="_csrf" value="<%= locals.token %>">
  <button class="btn btn-primary">Submit</button>
</form>

We could there are two line called csurf() and put it into middleware of express in mark1 and mark2. In mark1, csurf will generate a secret1 and token1 to give ejs template. But, in mark2, csurf is re-initialized, secret and token1 is changed. That means csurf will validate next request with new secret2 and token2.

But, it only happen at the first time validation. Subsequently, the request will be validated correctly.

Solution

So, I think I could return secret from set-cookie header if user have called csurf in cookie mode. It only happen in cookie mode, not session mode.

Discussion

Hi @Yu-Jack that may work, but there are a few issues with your code there:

1. You should not try reading from the cookie if this module is not in the cookie mode.

I've tested it, it'll not happen in session mode. Because csurf assign secret into req so it could get correct secret in second csurf. But for more protection, I also add if (cookie) to prevent.

2. cookie.parse cannot parse a set-cookie syntax; you need to use an appropriate parser for that header.

res.getHeader("set-cookie") is returning an array, so I use for-loop and Cookie.parse. It works correctly, is there other case I may not consider it?

3. The value of the cookie may not be the secret directly, as users can enable signing, so it would need to be unsinged as well

I've tested it when user use singed and unsigned mode, it works.

4. When reading a set-cookie header, you need to also check that it will come back to the relevant path and domain, as the cookie name is not unique enough to know that would be the cookie the browser would send back to you

I added the check about path and domain.
About "the browser would send back to you", it's interesting. Because express and server in test case, they act different behavior.

This is express will do.

1. csurf will give two set-cookie headers, I assume A set-cookie and B set-cookie
2. csurf give the ejs A set-cookie
3. express only get the later set-cookie which is B set-cookie, and send B back to me
4. csurf get B to do validation

This is server in test case will do

1. csurf will give two set-cookie headers, I assume A set-cookie and B set-cookie
2. csurf give the ejs A set-cookie
3. server send both cookie back to me
4. cusrf get A to do validation

So, it will make two situation.

// situation A
app.use(csurf({cookie: true})
app.use((req, res, next) => {
  req.token1 = req.csrfToken()
  next()
})
app.use(csurf({cookie: true})
app.use((req, res) => { res.end(req.token1) })

// situation B
app.use(csurf({cookie: true})
app.use(csurf({cookie: true})
app.use((req, res) => { res.end(req.csrfToken()) })

express will succeed in situation B, but failed in situation A. server in test case will succeed in situation A, but failed situation B So, I wrote these two test cases to test situation A and B.

Thank you!

[dougwilson]

res.getHeader("set-cookie") is returning an array, so I use for-loop and Cookie.parse. It works correctly, is there other case I may not consider it?

It looks like due to your usage, this is incorrectly treating the attributes like path and domain as case-sensitive. These keys are not case-sensitive and you should never use an API for something it was not designed to do. It may "seem" to work today, but that API will not guarantee it will work in the future, since that is not what it was designed for. Especially to land in such a popular module as this, we should never abuse any APIs and use them to do what they were designed for.

I've tested it when user use singed and unsigned mode, it works.

Can you add a test to demonstrate this? I just tested this and a signed cookie will fail with the provided code. I have marked this as "needs tests" to indicate you need to at least add a test that shows it working with signed cookies.

I added the check about path and domain. About "the browser would send back to you", it's interesting. Because express and server in test case, they act different behavior.

So these checks you implemented are not accurate. Please make sure the mechanism you are matching is not a simple string comparison, but actually matches the cookie specification: https://tools.ietf.org/html/rfc6265

For example if you first declare csurf({ cookie: { path: '/' }}) and then a csurf({ cookie: { path: '/foo' }}), the second csurf should be using the cookie set by the first, but not vice-versa. Domains are similar, in that a cookie declared for .domain.com should load for .foo.domain.com, but not the other way around.

[Yu-Jack]

HI @dougwilson , thank you for your comments.

But, I think It's difficult to use the first setting. Because sometime some use cases are allowed use different setting in different path or router.

The restriction should be not allowed duplicated called in same middleware. Below situation A is allowed.

const csrfProtection1 = csrf({ cookie: {signed: false, key: "key1"} })
app.get('/path1', csrfProtection1, function (req, res) {
    res.render('send', {token: req.csrfToken()})
})
app.post('/path1', csrfProtection1, function (req, res) {
    res.send('data is being processed')
})

const csrfProtection2 = csrf({ cookie: {signed: true, key: "key1"} })
app.get('/path2', csrfProtection2, function (req, res) {
    res.render('send', {token: req.csrfToken()})
})
app.post('/path2', csrfProtection2, function (req, res) {
    res.send('data is being processed')
})

Below situation B should be not allowed even use same settings.

const csrfProtection1 = csrf({ cookie: {signed: false, key: "key1"} })
app.use(csrfProtection1)

const csrfProtection2 = csrf({ cookie: {signed: true, key: "key1"} })
app.get('/path2', csrfProtection2, function (req, res) {
    res.render('send', {token: req.csrfToken()})
})
app.post('/path2', csrfProtection2, function (req, res) {
    res.send('data is being processed')
})

So, I'm thinking a new way to prevent it, like this (just an example). It make situation A is allowed, but throw an error in situation B.

return function csrf (req, res, next) {
    if (cookie) {
      if (req.isCsurflnitialized) {
        throw new Error("duplicated used in same middleware")
      }
      if (!req.isCsurflnitialized) {
        req.isCsurflnitialized = true
      }
    }
    // .... other code
}

Solution

I'm not sure which one is better for this module.

1. Prevent user from using the above situation B.
2. Make situation B work correctly by using the first setting when called csurf() This implementation may put config into req to remain first config

Personally, I prefer solution 1. Thank you for helping!