CSRF error status code - Githubissues

zisiszikos commented 6 years ago

Documentation says:

If enabled, the CSRF token must be in the payload when modifying data or you will receive a 403 Forbidden

But the error that is thrown from lusca doesn't have a status code. Does the documentation implies that the users should finally send a 403 error? And how do the users know that the error that was thrown, came from lusca CSRF? Just by checking the error message?

Example error stack print:

Error: CSRF token mismatch
    at checkCsrf (/home/user/Projects/my-project/node_modules/lusca/lib/csrf.js:112:18)
    at Layer.handle [as handle_request] (/home/user/Projects/my-project/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/home/user/Projects/my-project/node_modules/express/lib/router/index.js:317:13)
    at /home/user/Projects/my-project/node_modules/express/lib/router/index.js:284:7
    at Function.process_params (/home/user/Projects/my-project/node_modules/express/lib/router/index.js:335:12)
    at next (/home/user/Projects/my-project/node_modules/express/lib/router/index.js:275:10)
    at urlencodedParser (/home/user/Projects/my-project/node_modules/body-parser/lib/types/urlencoded.js:100:7)
    at Layer.handle [as handle_request] (/home/user/Projects/my-project/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/home/user/Projects/my-project/node_modules/express/lib/router/index.js:317:13)
    at /home/user/Projects/my-project/node_modules/express/lib/router/index.js:284:7

zisiszikos commented 6 years ago

Ok, I just show that the error code is passed to res.statusCode. Whouldn't be better if a proper error was thrown? Like using the http-errors library?

Example:

var createError = require('http-errors');
...
next(new createError.Forbidden(errmsg));

jonathansamines commented 6 years ago

I would also like to get this change. We usually define application-wide error handlers, which rely on the error "shape" to determine how to respond.

In case a generic error is returned, like the one provided by lusca, then a default response with 500 is returned. What I mean by this, is that at the end, the statusCode is easily overridable without knowing about it. This is something I just realised when I saw this issue.

Having the status error available at the error instance would help on this scenario. Another option would be to attach another kind of "code" or property to the error instance. I have seen other libraries like boom or celebrate do just that.

EDIT I recently saw #63 in which a solution to verify this, is explained, however seems like an unreliable method to perform the error verification.

zisiszikos commented 6 years ago

The status code of the response of a server (and generally all the content of a server response) should be the decision of the server itself and it's not the job of a middleware library. In my opinion the best would be not to throw an error at all but just inform the server that lusca csrf validation didn't pass. This could be done for example by passing an option at res.locals object and the server must check this option through a middleware and respond accordingly.

const errors = require('http-errors');

app.use('/api', (req, res, next) => {
    if (!res.locals.csrf_pass) {
        throw new errors.Forbidden('CSRF validation failed.');
        // or do whatever you want
    } else {
        next();
    }
});

The request/response lifecycle is now handled totally by the server and its not restricted by an Error thrown by the library.

I know this is a breaking change and that could only be implemented at a major release, but I think this is the correct strategy.

krakenjs / lusca

CSRF error status code #116