[BUG] return an Error will not stop middleware chain.Still call route handler!

[SaltFish001]

Use async middleware, retuen an Error. Still call route handler.
But in middleware docs said it will trigger global error handler as we are returning an Error,Docs

[kartikk221]

Could you provide a reproducible example? I believe the example snippets already have tests for them which would fail If they were not behaving properly?

[SaltFish001]

Could you provide a reproducible example? I believe the example snippets already have tests for them which would fail If they were not behaving properly?

https://github.com/SaltFish001/hyperexpress_exm
here

[BolverBlitz]

I´ve now tested all the version between, its from 6.16.4 -> 6.17.2
On 6.16.4 status 500 with the error is returned
From version 6.17.2 status 200 with the secret message is returned.

This bug originates from this commit: e521785

I don´t know if im the only one who returns a error in the middleware if a session is rejected, but if thats commen this might cause security issues for projects using hyper-express. On my application it gives everyone full admin permissions.

Code i used for testing:

const HyperExpress = require('hyper-express');
const app = new HyperExpress.Server();

const port = process.env.PORT || 80;
const bindip = process.env.BINDIP || '0.0.0.0'

const verifyRequest = () => {
    return async (req, res) => {
        try {
            console.log("Yes")
            throw new Error("No")
        } catch (error) {
            return error; // This will trigger global error handler as we are returning an Error
        }
    };
};

app.get('/', verifyRequest(), async (req, res) => {
    res.status(200);
    res.json({
        message: `Secret`
    });
});

app.set_error_handler((req, res, error) => {
    res.status(500);
    res.json({
        message: error.message
    });
});

app.listen(port, bindip)
    .then((socket) => console.log(`Listening on port: ${port}`))
    .catch((error) => console.error(`Failed to start webserver on: ${port}\nError: ${error}`));

[zeta-squared]

This isn't actually a bug in the code but rather a problem in the documentation. The problem is using a return statement doesn't actually pass control to the error handler, and I don't believe it should either from a design philosophy. If you change your verifyRequest middleware to only throw your error, rather than throw -> catch -> return, you will get the expected behaviour.

I will note that I believe there is another issue with how errors are handled in hyper-express. I have mentioned this in PR #328. This was an oversight on my part due to a tsc option.

EDIT: To clarify the current documentation states "Throwing or iterating next with an Error object will trigger the global error handler" which will work, it's unfortunate though that the provided example snippet just returns the error.

[BolverBlitz]

Thanks for your quick response pointing this out @zeta-squared. I've tryed it like you said and it works in the new version again.
However using it without the try catch resutls in a exit using the previus version.

throw new Error("No")
              ^

Error: No
    at Object.handler (C:\Users\1515\Desktop\hypertest\index.js:10:15)
    at Route.handle (C:\Users\1515\Desktop\hypertest\node_modules\hyper-express\src\components\router\Route.js:112:37)
    at Server._handle_uws_request (C:\Users\1515\Desktop\hypertest\node_modules\hyper-express\src\components\Server.js:527:19)

In gerneral there should be a test to catch when any behaviour with error handling changes like this. At least i do not expect such significant change from such a smal version jump.

[zeta-squared]

That's strange I copied your snippet from above and tested with my suggestions and they work. I've included it below.

const HyperExpress = require('hyper-express');
const app = new HyperExpress.Server();

const port = process.env.PORT || 5000;
const bindip = process.env.BINDIP || '0.0.0.0'

const verifyRequest = () => {
    return async (req, res, next) => {
    /** 
     * Optionally replace the try-catch block with:
     * console.log("Yes")
     * throw new Error("No")
     */
        try {
            console.log("Yes")
            throw new Error("No")
        } catch (error) {
            next(error); // This will trigger global error handler as we are returning an Error
        }
    };
};

app.get('/', verifyRequest(), async (req, res) => {
    res.status(200);
    res.json({
        message: `Secret`
    });
});

app.set_error_handler((req, res, error) => {
    res.status(500);
    res.json({
        message: error.message
    });
});

app.listen(port, bindip)
    .then((socket) => console.log(`Listening on port: ${port}`))
    .catch((error) => console.error(`Failed to start webserver on: ${port}\nError: ${error}`));

[BolverBlitz]

Make sure npm i realy installs 6.16.4 or ^6.17.2
It works on ^6.17.2
It does not work 6.16.4

I wrote this middleware ~2 years ago, reading whatever documentation there was back then and endet up with the try & catch version. Im fine with change to this, but i would like to read about this change before it happens and not by my entire codebase breaeking from a "smal" version jump, i hope you understand.

[BolverBlitz]

I´ve looked at @zeta-squared #328 and your solution there with next(error) works for both versions, but you left the command with return or throw it. I've tryed throw in #329 and that just dosn't work for me at all :/

const verifyRequest = () => {
    return async (req, res, next) => {
        try {
            console.log("Yes")
            throw new Error("No")
        } catch (error) {
            next(error);
        };
    };
};

[zeta-squared]

@BolverBlitz

I have left the throw in my fork because that works in the current (6.17.3) version of hyper-express so it should be included. Just to help me understand the situation a bit more, are you trying to transition to the current version of hyper-express or are you wanting to stay on 6.16.4 and add additional features to your application but are running into issues because the current documentation is not relevant for 6.16.4?

[BolverBlitz]

I work on this application for over a year and from time to time i update my packages with npx npm-check-updates -u when there is a (large) jump from for example 1.0.0 to 2.0.0 or 1.11 - 1.86 then i check the docs of the module if there where any breaking changes.
I did this a few days ago, i was on 6.14.12 and updated to 6.17.3. For me thats a smal update so i did not check docs on breaking changes and since it seemd to work at first glance i just continue to work on the project.

Spoiler: My spesific story, not relevant to this issue at all just for your question

Now yesterday i wrote a API route to allow the user to view and remove all of his current sessions, when testing this i got the error that the user was not defind when the UI updated after deletion was completed. My `verifyRequest` middleware validates the session and then adds users permissions and the user itself to `req.user` which i use in my route to save a database transaction. At first glance, this makes sense because the session got deleted by the previus request. However later i realized that it should not even go into the route because the session was invalid but because "bug" (or whatever you wanna call it) the returned error did not matter anymore. Most routes perform actions by session context so `db.user.updateName(req.user.id)` so a user can only ever update his own data, but admin routes use `db.user.updateName(param.id)` (param refering to `/editname/:id`. So on 6.17.3 every user can just send any token, or even just not include a token at all and can edit any user with those routes because the middleware won´t stop it anymore.

Edit: If you wanna look at the code, its there:
https://github.com/EBG-PW/EBG-WEB-Core/blob/7c62ebacfb0d74d19b75ff7780017c659daa1a00/middleware/verifyRequest.js#L14-L85
https://github.com/EBG-PW/EBG-WEB-Core/blob/7c62ebacfb0d74d19b75ff7780017c659daa1a00/api/user.js#L76-L90
https://github.com/EBG-PW/EBG-WEB-Core/blob/b5bae22f588d2cdc19a830d0ddfd2d23bf8546a9/api/admin_user.js#L59-L64

[zeta-squared]

@BolverBlitz Ok I understand now. Going back to:

I´ve looked at @zeta-squared #328 and your solution there with next(error) works for both versions, but you left the command with return or throw it. I've tryed throw in #329 and that just dosn't work for me at all :/

const verifyRequest = () => {
    return async (req, res, next) => {
        try {
            console.log("Yes")
            throw new Error("No")
        } catch (error) {
            next(error);
        };
    };
};

Are you saying that just using throw instead of a try-catch in 6.17.3 does not work either?

[BolverBlitz]

Good call, looks like this works with the smal code example here just fine.
It dosn´t work in my project and i have no idear why. I can´t write a smal example to reproduce it, sorry.

Its caused either by a throw new Error in the middleware, or by a req.accepts('html')

Edit: So i personaly will only use next(error) in my middlewares and req.headers['accept'] !== 'application/json' then it seems to work reliable for my project with the current version - Plus i´ll for sure write tests to see when it breaks again.

The debugger makes it seem everything is fine until this point.

Then in the next step the errow was thrown

[zeta-squared]

@BolverBlitz is your application written in JavaScript or TypeScript?

[BolverBlitz]

@zeta-squared Pure JS, i did link to it there in the spoiler.

I work on this application for over a year and from time to time i update my packages with npx npm-check-updates -u when there is a (large) jump from for example 1.0.0 to 2.0.0 or 1.11 - 1.86 then i check the docs of the module if there where any breaking changes. I did this a few days ago, i was on 6.14.12 and updated to 6.17.3. For me thats a smal update so i did not check docs on breaking changes and since it seemd to work at first glance i just continue to work on the project.
Spoiler: My spesific story, not relevant to this issue at all just for your question
Now yesterday i wrote a API route to allow the user to view and remove all of his current sessions, when testing this i got the error that the user was not defind when the UI updated after deletion was completed. My verifyRequest middleware validates the session and then adds users permissions and the user itself to req.user which i use in my route to save a database transaction. At first glance, this makes sense because the session got deleted by the previus request. However later i realized that it should not even go into the route because the session was invalid but because "bug" (or whatever you wanna call it) the returned error did not matter anymore. Most routes perform actions by session context so db.user.updateName(req.user.id) so a user can only ever update his own data, but admin routes use db.user.updateName(param.id) (param refering to /editname/:id. So on 6.17.3 every user can just send any token, or even just not include a token at all and can edit any user with those routes because the middleware won´t stop it anymore.

Edit: If you wanna look at the code, its there: https://github.com/EBG-PW/EBG-WEB-Core/blob/7c62ebacfb0d74d19b75ff7780017c659daa1a00/middleware/verifyRequest.js#L14-L85 https://github.com/EBG-PW/EBG-WEB-Core/blob/7c62ebacfb0d74d19b75ff7780017c659daa1a00/api/user.js#L76-L90 https://github.com/EBG-PW/EBG-WEB-Core/blob/b5bae22f588d2cdc19a830d0ddfd2d23bf8546a9/api/admin_user.js#L59-L64

[zeta-squared]

@BolverBlitz I'm sorry I couldn't be anymore help. I have looked at the code you linked and there are many try-catch blocks which will catch then return the error. It's possible that these are causing the conflict. At least for now you have a working solution.

When you have had a chance to write some tests and reproduce the bug can you please let me know. I need to make sure that if this is indeed an unintended bug that I account for it as well with the router specific error handlers in my fork - and of course if they're accepted into the main repo.