feature: prevent user call `csurf()` repeatedly

[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.

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

I think I could add a flag to decide the csurf is initialized or not.
It's default is false, it will be changed to true when user call csurf(). Then It will throw error when initialized is already true.

I'm not sure is it a good idea to prevent user from using csurf() Please let me know, is there any problem will make a new problem? Thanks!

[dougwilson]

Thank you for this, but the issue is that users are 100% allowed to call this module twice, and this will break that. Here is a quick scenario that will break with this change: you want csurf on two parts of your application but not the entire application. You just app.use('/admin', csurf()) and then app.use('/forum', csurf()) for example.

[Yu-Jack]

Okay, thank you!

[Yu-Jack]

@dougwilson

What if I return secret from "set-cookie" response headers when bag[key] is null, is it okay? This is my modified code, then the test case is all passed. And it will let the above case passed.

function getSecret (req, res, sessionKey, cookie) {
  // get the bag & key
  var bag = getSecretBag(req, sessionKey, cookie)
  var key = cookie ? cookie.key : 'csrfSecret'

  if (!bag) {
    throw new Error('misconfigured csrf')
  }

  if (!bag[key]) {
    let setCookieHeaders = res.getHeader("set-cookie")
    if (setCookieHeaders) {
      for (const header of setCookieHeaders) {
        let cookie = Cookie.parse(header)
        if (cookie[key]) {
          // return secert from sent response headers
          return cookie[key]
        }
      }
    }
  }

  // return secret from bag
  return bag[key]
}

[dougwilson]

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.
2. cookie.parse cannot parse a set-cookie syntax; you need to use an appropriate parser for that header.
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
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