Routes and middleware not run in defined order

[alexwhitman]

Bug Report

Restify Version

v7.2.1

Node.js Version

v10.5.0

Expected behaviour

In restify v4, using the serveStatic plugin before including any middleware would not run the middleware when serving a file.

Actual behaviour

Middleware is now run prior to serving files.

Repro case

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

const server = restify.createServer();

server.get('/', restify.plugins.serveStatic({
	directory : './public/',
	default   : 'index.html'
}));

server.use((request, response, next) => {
	return next(new errors.ForbiddenError('Nope'));
});

server.get('/api/test', (request, response, next) => {
	response.send({});
	return next();
});

server.listen(8000);

Expected: Request to / serves index.html, request to /api/test gives a ForbiddenError
Actual: Request to both / and /api/test gives a ForbiddenError

Cause

Changes somewhere between v4 and v7, presumably the change in routing/middleware handling.

Are you willing and able to fix this?

No, there appears to be other issues with serveStatic in general so this should be tackled as part of sorting the overall problem out.

[alexwhitman]

Actually, this isn't specific to serveStatic. It's now seemingly impossible to only run middleware for specific routes by using .use() after defining other other routes. I have a /health route which indicates server status to load balancers but with the upgrade to v7 the /health route (along with the static files) now check for authentication and rate limiting.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[alexwhitman]

This is still an issue.

[santino]

+1 This is a real issue and a regression which goes against the specs and documentation.

restify runs handlers in the order they are registered

This is no longer happening which obviously is causing problems.

As an additional note I only upgraded from v6 to v7 so this got broken in the last major version release and I don't believe this is an intentional breaking change since I can't seem to find warnings anywhere and the docs (as per quote above) still seems to state what the intended behaviour should be.

Can't believe this didn't get any attention in 3 months!

[ghermeto]

@alexwhitman,

apparently until Restify v6 that was the expected behavior and properly documented here.

However, Restify v7 introduces a new middleware chain which is invoked at request time, instead of built for each route at registration time. This seems like a deliberate decision rather than a bug, but the documentation wasn't properly updated for v7 and this breaking change wasn't reflected on the CHANGELOG or the migration guide...

@hekike could you give context on this change?

[avimar]

Yikes, so this is the issue I just hit.

I tried upgrading from 4 to 7. I read the upgrade guides, removed RegExp from my routes, and now my auth handler is running even for things that shouldn't require auth.

So, if this feature is no longer available, how do I set up such an option to only require authentication (my middleware) on specific routes??

[avimar]

@alexwhitman @santino @ghermeto has anyone figured out a work-around? I'm afraid I'll have to undo my migrations...

Thanks!

[avimar]

I've downgraded to 6.4, where the only change is that there's full regex support available, instead of wildcards in strings.

I just had to convert '*' back to /.*/

[vsrajmohan]

If any of you have a solution for this issue with 7+ please let me know.

Thanks!

[sgilroy]

We also ran into this when attempting to upgrade from an older version of restify. We use the following pattern (which is working effectively in restify 6.4.0) to register routes:

1. Register all unauthenticated routes (server.get(), etc)
2. Add authentication handler (server.use(authentication.apiMiddleware()))
3. Register authenticated routes (more calls to server.get(), etc)

@hekike (or @rajatkumar) Can we please at least get some guidance on what the new recommended approach is for dealing with authentication in an app like ours where there is a mix of authenticated and unauthenticated routes?

Ideally, we would like to be able to opt-in to the old behavior of the middleware/handlers chain being built for each route when the route is added. However, if this is not possible then please at least document one or more viable approaches. This undocumented breaking change seems like a pretty major shift in how restify works, as @ghermeto pointed out #1685 (comment).

Speculation on my part: the authentication handler could detect the current route at run time and detect some flag that was included on the route when it was registered. Can we include arbitrary attributes in the methodOpts and then access the route attributes from a handler? How? What if we are trying to apply someone else's handler (such passport)? Do we need to create a wrapper around the handler to handle the decision at the time of the request?

For reference, we are using passport for authentication and our apiMiddleware generating function currently looks like this:

    apiMiddleware: function authenticateBearer() {
        return passport.authenticate('bearer', { session: false });
    },

Another alternative would be to explicitly include the desired authentication middleware/handler in the middleware chain of each route, but that would be rather cumbersome and seems to defeat the purpose of being able to apply handlers across multiple routes. This is not a very appealing solution for our application.

[rajatkumar]

@sgilroy I will look into this. Thanks for a detailed writeup here, let me see what is happening here. I am still catching up with all the issues.

[DonutEspresso]

Hi all - this was a deliberate change, one unfortunately poorly documented. We definitely could have done a better job on this front :( The intention was to get away from unintentional bugs as a result of shifting use/pre/{verb} statements around in the code, and moving towards explicit ordering specified by the API. For example:

const restify = require('restify');
const server = restify.createServer();

server.use(function(req, res, next) {
    console.warn('use1');
    return next();
});

server.pre(function(req, res, next) {
    console.warn('pre1');
    return next();
});

server.get('/', [
    function myGet(req, res, next) {
        console.warn('get1');
        return next();
    },
    function myGet(req, res, next) {
        console.warn('get2');
        res.end();
        return next();
    }
]);

server.use([
    function(req, res, next) {
        console.warn('use2');
        return next();
    },
    function(req, res, next) {
        console.warn('use3');
        return next();
    },
]);

server.pre([
    function(req, res, next) {
        console.warn('pre2');
        return next();
    },
    function(req, res, next) {
        console.warn('pre3');
        return next();
    },
]);

server.listen(3000);

Would result in:

pre1
pre2
pre3
use1
use2
use3
get1
get2

Moving the statements around would not change the output. Since pre, use, and {verb} all take arrays, existing behavior is possible by leveraging factories of some sort to "bind" common middlewares for use cases like auth, e.g.:

server.get(prependWithCommon(myHandler()));
server.get(prependWithAuth(myHandler()));

We've also seen teams build out a JSON config mapping for their URLs to handlers, and use that to programmatically install routes and bind auth only when the route specifies it.

Ultimately, the behavior is more consistent, although at the cost of verbosity. We'll definitely need to update the docs to reflect this change more explicitly. We understand this might not be to everyone's liking; we should definitely continue the conversation if there are use cases we can better support.