Nodejs Express multiple callback error

So let's start with the error:

Can't set headers after they are sent.

Nine times out of ten this is caused by trying to send two responses to the same request. The reference to headers is a little misleading, though technically true. The first thing the second response will try to do is set some headers, which will fail because the first response has already sent them back to the client.

The stack trace gives you a clear indication of where the second response originates, including filenames and line numbers. More difficult is to track down the first response, generally I'd just throw in some extra console logging to figure it out.

In the question you mention that you believe you've found the source of the problem but it looks to me like it might just be the tip of the iceberg. You've repeatedly used the same pattern and even if you fix it in one place that may not be sufficient.

Before I get into that, let's start with this:

return next();

For the purposes of this example it doesn't really matter whether you pass an error, e.g. return next(err);, the point is the same. First it invokes next(), which returns undefined. It then returns undefined from the surrounding function. In other words, it's just a convenient shorthand for this:

next();
return;

The reason why we return is to ensure that nothing else happens after we've called next(), we always try to ensure that calling next() is the last thing we do in our handler, not least because otherwise we can have bugs where we try to send the response twice.

The (anti-)pattern you've used looks a bit like this:

obj.doSomething(function() {
    return next(); // next 1
});

return next(); // next 2

Again, it doesn't really matter whether you're calling next() or next(err), it all works out much the same. The key thing to note is that the return for next 1 is just returning from the function passed to doSomething. It isn't doing anything to prevent next 2 from being hit. Both next 1 and next 2 will be called.

In places your code seems unclear about whether it's trying to be synchronous or asynchronous, using callbacks and return values at the same time. It makes it a little bit difficult to say for sure what the 'correct' code should look like. Specifically, is the value of can supposed to be returned synchronously or passed to the callback asynchronously? I suspect it's the latter but the current code seems torn between the two. It's important to ensure that you don't call next() until you're ready for the next thing to happen, so if you're waiting on a DB query you mustn't call next until that comes back.

Personally I'd rename your callbacks so they aren't all called next, I find that really confusing. When I see next I'm expecting it to be an Express next function, not just an arbitrary callback.

It's a bit of a guess but I'd suggest your middleware should look something like this:

exports.can = function (resource, action) {
    return function (request, response, next) {
        action = action || request.method;

        if (!request.user) {
            return next(new errors.UnauthorizedError());
        }

        request.user.can(request.user.role, request.user.currentPack, resource, action, function (can, error) {
            if (error) {
                next(error);
            }
            else if (can) {
                next();
            }
            else {
                next(new errors.UnauthorizedError());
            }
        });

        // Do not call next() here
    };
};

The relevant section of the User model would then be:

if (can) {
    pack.can(userPack, resource, action, function (can) {
        next(can);
    });
}
else {
    next(can);
}