expressjs / csurf

CSRF token middleware
MIT License
2.3k stars 217 forks source link

EBADCSRFTOKEN after session expirery #62

Closed borisdiakur closed 9 years ago

borisdiakur commented 9 years ago

Please note in the docs, that if using the cookie-session module, it must be required before requiring csurf.

My code looked like this:

....
var cookieParser = require('cookie-parser');
var csrf = require('csurf');
var bodyParser = require('body-parser');
var express = require('express');
var cookieSession = require('cookie-session'); // <- should be required before csurf!
...
app.use(bodyParser.urlencoded({ extended: true }));
app.use(cookieParser());
app.use(cookieSession({
    secret: conf.session.secret,
    expires: new Date(Date.now() + sessionMaxAge),
    key: 'sessionId'
}));
app.use(csrf({ cookie: true}));
...

I passed the csrf token to the view:

res.render('index', { csrf: req.csrfToken() });

In the view I use the csrf token in a hidden input element (Jade code):

input(type='hidden', name='_csrf', value='#{csrf}')

Now if I waited until the session had expired and tried to submit the form I got the error EBADCSRFTOKEN no matter what.

Requiring cookie-session before csurf solved the problem for me.

dougwilson commented 9 years ago

Hi! It's the very first sentence in the README already:

Requires [...] a session middleware [..] to be initialized first

borisdiakur commented 9 years ago

It's funny that you just replaced the words "either" and "or" with "[...]" and "[...]". It is definitely not clear, that you must require both or at least the cookie-parser module before requiring csurf. As you can see in my code sample I had required the cookie-parser module before csurf and still ran into that nasty issue.

dougwilson commented 9 years ago

You do not need cookie-parser at all. You need either a session middleware or cookie-parser, not both.

dougwilson commented 9 years ago

Basically, you need cookie-parser if you set cookie: true in your settings here, and do not need cookie-session. If you remove cookie: true, then you do not need cookie-parser and do need cookie-session.

dougwilson commented 9 years ago

If you have better wording that will help, please make a PR. The wording that is there makes complete sense to me and many other people, so I don't know how to improve it without direct input from you :)

borisdiakur commented 9 years ago

Aaaah, ok! Actually your wording above is what I've been missing. I'll send you a pull request tomorrow (it's already late here in Germany... maybe the only problem was that english is not my first language). Thanks a lot!

dougwilson commented 9 years ago

It's no problem, I look forward to the PR :) I can always tweak it. It's not always easy for us to write docs that are clear to new users, since we already have an understanding of how it works, so I can certainly add more text to the README, but it's always best when it's basically from the mouths of the users :)

borisdiakur commented 9 years ago

@dougwilson don't forget -> https://github.com/expressjs/csurf/pull/63 : ]

dougwilson commented 9 years ago

I haven't forgotten :)