separating token creation and checking

[mscdex]

Currently csrf secret creation and token checking are done "together." The problem is that I may want to put the csrf secret creation early on in the middleware stack, but then check the token some time later in the stack.

My use case is that I only parse incoming form submissions in routes where I expect them instead of parsing them near the top of the stack for every request. The form is where I'd usually have my csrf token for verification.

[jonathanong]

the correct way imo would be to make it a constructor/prototype and expose each part as a method. then people could also do stuff like change the salt function and length and stuff. but then it would be more difficult to use as middleware, which 99% of people use this library for.

maybe make it a different lib and make this lib sugar on top of it?

[mscdex]

Right now I've modified csurf locally to have an autoCheck option that is the default and performs the way csurf currently does. However if autoCheck is falsey, then it adds a checkToken function to the request object instead of immediately checking the token.

[dougwilson]

We could attach two functions to the main export to split the two operations, while keeping the current behavior intact. Like

app.use(csurf())

would still work, but if you wanted to split it up (shouldn't be the default because people don't seem to understand how things work):

app.use(csurf.generate())

app.post('/save', bodyParser.json(), csurf.check(), function(req, res, next){
  // ...
})

I am impartial to those function names, haha

[sjlu]

+1

[dantman]

+1 Sometimes you don't want to CSRF token (maybe you're doing an API with its own tokens or OAuth) and the generic error also makes it hard to display custom errors for CSRF errors – which sometime are a result of accidental session data destruction and warrant a much more polite and helpful error that doesn't go through the normal handler.

[dougwilson]

hard to display custom errors for CSRF errors

the errors generated here are forwarded to your error-handler. you can if (err.message.indexOf('csrf') !== -1) {} right now in your error handler to display a different message for the csrf, but, really, the error could use it's own property or a code to make the detection easier.

app.use(function(err, req, res, next){
  if (err.message.indexOf('csrf') === -1) return next(err)
  // display your csrf custom messages here
})

[dantman]

@dougwilson Testing the message for the text 'csrf' isn't the proper way to do that. A custom property, name, or custom error type is what I was getting at.

However in reality most csrf errors shouldn't even hit the error handler. The correct behaviour for csrf issues is not to display a generic csrf error but instead to display the original form that lead to the csrf (pre-filled) with a small form error. I can't do that if I can't either let the route go through to the normal handler or autoCheck: false and do the csrf check directly from the form.

[dougwilson]

Testing the message for the text 'csrf' isn't the proper way to do that. A custom property, name, or custom error type is what I was getting at.

I know that, which is why I said that in my message. Error handling is a completely different issue from this. Can you please open a new issue, rather than diverting this issue re: error handling?

[jonathanong]

If you're feeling crazy you could build your own middleware with https://github.com/expressjs/csrf-tokens. If you could think of any more ways to be generic let us know!

[jonathanong]

how about changing the module like this:

// only generates the secret if it doesn't exist
app.use(csrf()) 

// create a csrf token like it does right now
app.use(function (req, res) {
  var csrf = req.csrfToken()
})

// verify a CSRF token, optionally on a body or a string
app.use(function (req, res) {
  var err = req.csrfVerify(req.body)
  if (err) return next(err)
})

maybe making the last one another middleware as convenience.

the only problem with this is that CSRF verification is now opt-in, so it's going to open up a lot of security holes for users that don't pay attention to this change. maybe make the whole second part optional. but the idea that CSRF token verification is required on ALL routes is ridiculous, especially when you have an app + an API

[dougwilson]

but the idea that CSRF token verification is required on ALL routes is ridiculous, especially when you have an app + an API

it is only valid on the routes you mount your middleware to. You would split up your API like so:

app.use('/api', apiRouter)
app.use(csrf())
// now all routes

[jonathanong]

i hate sites that do that. just let me append .json to the URL and grab the data as JSON!

[dougwilson]

just let me append .json to the URL and grab the data as JSON!

hm, I cannot think of a solution to that with the current workings :)

anyway, yes, this should be possible to split up, but i think we should keep the bare export function as-is and just add a generate and verify to the exports or something like that. almost all the people using csrf here are just front-end developers who just want to slap things together, so not giving them a good default everything protected is probably not a good idea.

[dougwilson]

I've just finished refactoring the internals in preparation for this.

[arcanis]

We could attach two functions to the main export to split the two operations, while keeping the current behavior intact. Like

app.use(csurf())

would still work, but if you wanted to split it up (shouldn't be the default because people don't seem to understand how things work):

app.use(csurf.generate())

app.post('/save', bodyParser.json(), csurf.check(), function(req, res, next){
  // ...
})

I am impartial to those function names, haha

I really like this idea; is it still planned? Would you merge a PR?

[dougwilson]

It sure is. I would be happy to look at a PR.

[mickeyckm]

+1

For FB Canvas, this is actually useful because it will POST to a URL and it require us to disable the CSRF for that URL. However, I would like to have a CSRF token generated on that page so that, the subsequent POST call, I know it is only coming from within the domain and not external.

[ohmyhusky]

I have a question about how to prevent CSRF attack for 'GET' request by using this middleware, but I don't know what is the proper place to ask this question, would you mind if I post to this issue?

I'm looking for a solution to prevent XSS and CSRF attack, and I've got some ideas but not sure if they are good ideas, would you mind give me some advises? Thank you very much in advance!

Suppose there are some url I want them away from XSS and CSRF, let's use the 'api/v1/accounts' as an example, if we don't want only the not-admin user access (GET) this resources, how do we implement it? My idea is to store the authorization token in the cookie with httpOnly flag (and maybe sometimes with secure flag as well) to prevent XSS attack. About the CSRF, I think we can avoid it by issuing a xsrfToken to the authorized user (jump to a admin page with a xsrfToken store in the cookie). For example, if you want to access 'api/v1/accounts', you must login with username and password, after login, you will be redirected to page '/page/admin', this page with give you the required xsrfToken in the cookie, every time you access 'api/v1/accounts', you have set the X-XSRF-TOKEN header with this xsrfToken. Since the cookie is accessible by only our domain, I can keep the 'api/v1/accounts' away from CSRF attack. Is this a proper way? If not, how to prevent CSRF attack to 'GET' 'api/v1/accounts'?

The csurf middleware's default configuration is ignore the ['GET', 'HEAD', 'OPTIONS'] method, So when I tried to 'GET' the 'api/v1/accounts' from other domain with CSRF attack, it can access the resources successfully. What should I do to validate such 'GET' request? Just simply remove the 'GET' from the ignoring list? Or I should use two middleware with one ignoring the 'GET' request (/page/admin) for issuing xsrfToken and another do NOT ignore 'GET' that keep the resources (api/v1/accounts) away from CSRF attack? I think my idea is a little bit awkward, what do you usually do for protecting resources? Did I go to a wrong way? Because I don't know if using the X-XSRF-TOKEN way to prevent CSRF attack should care about the 'GET' request.

[dougwilson]

Hi @cyl19910101, I'm sorry, this is not the correct place to ask that question. I would suggest either opening a new issue (https://help.github.com/articles/creating-an-issue/) or posting on stackoverflow.com

[ohmyhusky]

@dougwilson Thanks a lot, I was thinking maybe I should do that by separating the token creation and validation, sorry about that. I will post such question in stack overflow later, thank you

[Wtower]

As @mickeyckm suggested, I am too affected by this facebook canvas nonsense. The first request to the index / is a POST request, without getting the chance to receive a CSRF token. Of course I would happily exempt the particular route from csrf, and only include other routes that in fact do data alteration.

But here is the problem: having ruled out the first route, I am not able to generate a CSRF token to have for the next requests that do alter data.

So I believe by separating the token generation from check might help this situation.

Until then, as a workaround, is there something that you would kindly recommend? I mean apart from the obvious exclude-all from csrf. Would a subsequent ajax get request to obtain a csrf make sense, or it contradicts the very reason to have csrf?

EDIT: As an additional security layer (hopefully), in a second check post (no data alteration) I record the fbId in session and require it in subsequent data-altering posts. Not csrf protection I understand.

[rjerue]

Was this ever completed? There's no documentation.

+1

[tinder-qhan]

It is 2018, is this happening?

[SzaboMate90]

Any news about separation?

[VincentCATILLON]

Even if the thread is old (but not resolved so), so you can do something like this (to separate generation and check):

// src/middlewares/csrf.js

const csrf = require('csurf');

const middleware = csrf();

const _nextWithoutCheck = function(_next) {
  return function(e) {
    // Triggered by check method automatically (there is to bypass it)
    // (Source: https://github.com/expressjs/csurf/blob/master/index.js#L113)
    if (e && e.code !== 'EBADCSRFTOKEN') {
      return _next(e);
    }

    return _next();
  };
};

module.exports.generate = function() {
  return function(req, res, next) {
    const _middleware = middleware(req, res, _nextWithoutCheck(next));

    res.cookie('XSRF-TOKEN', req.csrfToken(), {
      secure: req.secure
    });

    return _middleware;
  };
};

module.exports.check = function() {
  return middleware;
};

And use it to return XSRF token for every request:

// src/index.js

const express = require('express');

const csrfMiddleware = require('./middlewares/csrf');

const app = express();
app.use(csrf.generate());

And protect only some routes like this:

// src/routes/user.js

const csrf = require('../middlewares/csrf');

server.put('/', csrf.check(), function(req, res, next) {
  ...
});

Hope this helps.

[ivansieder]

@dougwilson I've read through this issue right now, wondering if it's still something that should/could be discussed.

I've got the following use case:

• SPA, which does a POST to log in
• Creation works for 'GET', 'HEAD', 'OPTIONS' by default, so on a POST I'll get a 'EBADCSRFTOKEN' error, which makes sense
• Excluding POST requests isn't an option either, not even for this one request, as that doesn't seem quiet clean to me
• Could it be an option to have something like the following, where csrf({}) exposes the middleware as usual to keep the funcitonality consistent but additionally exposes something like init to register the csrfToken() function and check to validate the token?

// sets csrf up
var csrfProtection = csrf({ cookie: true })

// adds functionality to the request object for csrf token creation
app.use(csrfProtection.init())

// this way, this route doesn't check for the csrf token, even if it's a post request
app.post("/login", (req, res) => {
  res.render('send', { csrfToken: req.csrfToken() });
});

// csrfProtection.check() basically is the same as csrfProtection, which additionally does the check.
app.post("/secret_route", csrfProtection.check(), (req, res) => {
  res.status(200).json({});
});

[naufalkhalid]

I would love to see this happen