Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSRF error status code #116

Open
zisiszikos opened this issue Oct 31, 2017 · 3 comments
Open

CSRF error status code #116

zisiszikos opened this issue Oct 31, 2017 · 3 comments

Comments

@zisiszikos
Copy link

zisiszikos commented Oct 31, 2017

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
Copy link
Author

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
Copy link

jonathansamines commented Dec 21, 2017

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants