Added 'ECSRFTOKENINVALID' to indicate validation failure

[valango]

My intention was to implement an easy way to process possible attack attempts without a need for error handling - which would be a bit tricky due to their asynchronous nature.

[dougwilson]

This whole change seems too convoluted. Why can't we just set the name property of the error to like CSRFError or something and you just handle it in the standard error handlers?

[valango]

Hey Doug,

I think you are right - I was too keyboard-happy with all that. Error handler is definitely the place for such things.

In fact I was using the error handler before, looking for 'csrf' substring in message text, but this seemed a bit unsure technique to me. Your CSRFError idea is definitely the best solution. So I'll roll back most of my changes and push again.

I am sorry for the semicolon thing!

On Wed, Aug 13, 2014 at 9:12 PM, Douglas Christopher Wilson < notifications@github.com> wrote:

This whole change seems too convoluted. Why can't we just set the name property of the error to like CSRFError or something and you just handle it in the standard error handlers?

— Reply to this email directly or view it on GitHub https://github.com/expressjs/csurf/pull/24#issuecomment-52087521.

[dougwilson]

no problem :) fyi i'm not dismissing your need, just trying to find the best approach to solve the need :)

[valango]

I hope you haven't used my commit in any way yet - I'l use "git reset --hard" if it's ok w you (I'm new to git).

On Wed, Aug 13, 2014 at 10:21 PM, Douglas Christopher Wilson < notifications@github.com> wrote:

no problem :) fyi i'm not dismissing your need, just trying to find the best approach to solve the need :)

— Reply to this email directly or view it on GitHub https://github.com/expressjs/csurf/pull/24#issuecomment-52097198.

[dougwilson]

Feel free to do whatever you want with the commits :)

[jonathanong]

what's wrong with passing it to the error handler?

[valango]

When dealing with possible attacks (what csurf is used for), we need to tell CSRF validation failures clearly from all other error conditions. So far, the status 403 and specific message text were the only clues. But as doug already pointed out, a special option/handler would be an overkill and he advised a different approach. :)

Please see the new commit.

On Wed, Aug 13, 2014 at 11:24 PM, Jonathan Ong notifications@github.com wrote:

what's wrong with passing it to the error handler?

— Reply to this email directly or view it on GitHub https://github.com/expressjs/csurf/pull/24#issuecomment-52105382.

[jonathanong]

Ohhhh I like this new commit. Though I think it should be called CSRFError because error names in JS don't tend to have _. Though all those caps bug me...

[jonathanong]

We can also use node-like err.codes

[dougwilson]

Oh, I forgot all about err.code. Yea, let's use that here. Like

err.code = 'ECSRFTOKENINVALID'

or something

[valango]

Hi everybody - thank you for your hints and feedback so far - it has been both exciting and enlightening experience for me! :)

Now, just one more idea - if I get it right, the most common use case of csurf is probably something like:

app.use(require('csurf')());
app.use(function (req, res, next) {
    res.locals._csrfToken = req.csrfToken();
    next();
});

If this is almost always the same pattern, then I would like to make it shorter - something like this:

app.use(require('csurf')({res:'locals._csrfToken'}));

Any thoughts?

[dougwilson]

Well, it's only common because people are lazy :) You should really only be calling req.csrfToken(); in your routes that actually render a view, rather than every single request.

[dougwilson]

That's actually why it's a function you have to cal--because you're only supposed to call it when you actually need it, especially since it does a bunch of work.

[jonathanong]

can you remove the gitignore change and squash?

[charlie-s]

What are the merge conflicts holding this back? Need any help?

Might I suggest building out the custom error type directly so that it can be thrown without having to explicitly set the error code? Something like:

var CsrfTokenInvalid = function(message) {
    this.name = 'CsrfTokenInvalid';
    this.code = 'ECSRFTOKENINVALID';
    this.message = message || "Invalid CSRF token.";
};
CsrfTokenInvalid.prototype = new Error();
CsrfTokenInvalid.prototype.constructor = CsrfTokenInvalid;

and then

throw new InvalidCsrfToken();

[dougwilson]

What are the merge conflicts holding this back? Need any help?

Mainly once we implement something, it'll be hard to change, so just waiting to do it right.

Might I suggest building out the custom error type directly so that it can be thrown without having to explicitly set the error code?

Only if Object.prototype.toString.call(err) === '[object Error]' on the resulting object, which your implementation there doesn't do.

[charlie-s]

It shouldn't be a custom error type, you're saying?

[dougwilson]

It shouldn't be a custom error type, you're saying?

It can, as long as the internal JS [[Class]] is correctly set to Error in the custom error implementation. I was just saying your example did not inherit from error correctly, which is why the internal class was wrong :)

[charlie-s]

Ah, I understand. So how would the code change? I pulled mine from how I have seen other node modules define new error types, e.g.:

function MissingHeaderError(message) {
    this.name = 'MissingHeaderError';
    this.message = message;
    this.stack = (new Error()).stack;
}
MissingHeaderError.prototype = new Error();

which Object.prototype.toString.call() then reports as [object Object].

[dougwilson]

is is very, very verbose to do it. but it's necessary for people using node.js core util.isError: https://github.com/joyent/node/blob/v0.10.24/lib/util.js#L454

[jonathanong]

i like the code as it is. it just needs a rebase, remove the .gitignore changes, and a squash

[dougwilson]

i'm working on merging it now. the tests are implemented poorly, haha

[charlie-s]

Wow, really good reference and thanks for teaching me some new things today.

[dougwilson]

OK, let me know what you think of what was just committed to master. You can test it as well with npm install expressjs/csurf