Splits token generation and validation

[arcanis]

I'm currently using this branch one a personal project and it seems to work fine. Tests are (mostly) passing, check the following for more infos.

It splits the token generation and validation into two different steps, so one can return the csrf token without having to actually validate it. Cf #10.

Additions

• csurf.generator() is a middleware adding the following to the request object :
  • csrfToken() returns the current session token (as before)
  • checkCsrf() returns a boolean : true if the csrf token is correctly set (new)
• csurf.validator() is a middleware automatically calling the checkCsrf() method from the request object, and returning a 403 error if the token does not match. Note that this middleware depends on csurf.generator.

Changes

• One of the test expected every requests to crash when there was !cookie && !req.session (bad configuration settings), even when the request method was supposed to be ignored. I fixed it. The un-fix is pretty simple, so tell me if you want to revert it, but I think it should be mostly safe, since it's a bit impossible to run a website with a bad configuration anyway.
• I've added my name to the credit list since it doesn't really fit the short patch pattern. Tell me if I should not !

WIP

• One test is failing (csurf should error without cookieParser secret and signed cookie storage), because validating a csrf does not cause one to be created. I think it is correct too, but your call.

[dougwilson]

Sorry for all these comments, but because you changes all the styling, your PR is hard to follow since it basically rewrite the file by altering existing function names, adding semicolons when we don't use them, etc.

[arcanis]

Np, I fixed them

[dougwilson]

BTW, I don't want this to get lost in the style fixes: I really appreciate this PR!

[arcanis]

You're welcome!

I've fixed style issues, what's left is the tests behavior.

[arcanis]

Ping ?

[dougwilson]

Pong :) This isn't forgotten; I know I wish I moved on this quicker, but it's just a large change and I usually try to rest at the end of the year, haha. I was just wanting this myself in a project, so I think this has just moved up a lot in priority. I also cannot thank you enough for putting this together.

[arcanis]

Ah ah, np, take your time :) In the meantime, it's simple enough to do a "csurf" : "arcanis/csurf" in the package.json dependencies if one really need this feature

[kmanzana]

:+1: on this. Csurf is unusable without being able to split these 2 out because all routes need to have access to the csrfToken but only certain routes need to verify (specifically our API shouldn't verify).

[kmanzana]

Just going to use the fork for now, thanks for the tip on that @arcanis.

[kmanzana]

@arcanis, any reason there aren't added tests to show your added functionality is working and how to use it?

[dougwilson]

You also don't need to app.use() the middleware on all your routes--you can simply only use it on a subset of them, which is how a lot of people are using it.

Very simple example splitting your API and web routes:

// install all your api routes
app.use('/api', api)

// install your web routes second
app.use(csurf(), web)

[kmanzana]

@dougwilson I have tried that, might not be doing it correctly. I have

# routes
app.get '*', require('csurf')()

# auth-routes
app.post '/logout', require('csurf')()

I hit '/' and use that token to post to /logout. What ends up happening though is that the secret has changed by the time it gets to /logout so it says the token is invalid. If I just do app.use() this works so I know it is set up correctly.

Is there something I'm missing?

[dougwilson]

Is there something I'm missing?

Your csurf is never invoked in your code above; you only setup csurf for GET requests, so a POST to log out won't invoke csurf. Besides, app.get('*', require('csurf')()) doesn't give you anything--the * means you're still adding it to all your routes, though you have it attached to the wrong method. You need to post a lot more code than that for help (and please open a new issue, rather than cluttering this PR).

[arcanis]

Ping ? :smile:

[dougwilson]

The 2.0 is just waiting for Express 3.x to drop dead so we can untie these modules from it. The timeline is July 2015, otherwise this work is great and I hope to get it incorporated.

[glenjamin]

Is this still alive?

I've just run into the scenario where I want to only verify the token conditionally - so this would be just what I need!

Happy to help if there's work to be done.

[strawbrary]

@glenjamin I ended up needing this so I rebased @arcanis's changes against the latest from master and pushed that into my fork at strawbrary/csurf. I believe it still needs tests for the new functionality and updates to the docs before it's ready to be merged though.

[arcanis]

I do not "champion" this issue anymore - my thoughts on how this feature could be implemented API-wise are still there if you need them, and the current implementation will remain available for some time, but I'm afraid I now have to move on other things.

[dougwilson]

Hi @arcanis, I'm sorry to see you close your pull request, especially since now this module is part of the Node.js foundation, and should be getting attention soon after getting everything setup with the foundation. I have copied your commit to a branch in this repository: https://github.com/expressjs/csurf/tree/split-module

[arcanis]

@dougwilson Sorry about the passive-agressive tone, I was a bit tired last night - I'm happy to see that you're still invested on Express after these last weeks :)

For what it worth, I've been using my PR in production for about a year now, and it seems to work quite fine. I haven't had any reported issue yet.