kartikk221 / hyper-express

High performance Node.js webserver with a simple-to-use API powered by uWebsockets.js under the hood.
MIT License
1.58k stars 83 forks source link

Throwing error in Middleware causes app to crash #272

Open Miggets7 opened 1 month ago

Miggets7 commented 1 month ago

I've got this middleware function:

export async function authJwtMiddleware(req: Request, res: Response, next: MiddlewareNext) {
    const authHeader = req.get("Authorization");
    if (!authHeader) {
        throw new HttpException(401, "No authorization header");
    }

    if (!authHeader.startsWith("Bearer ")) {
        throw new HttpException(401, "Invalid authorization header")
    }

    const token = authHeader.split(" ")[1];
    let session: JWTPayload;
    try {
        session = await DependencyProviderService.getImpl<JwtSessionService>(JwtSessionServiceDi).verifySession(token);
    } catch (err) {
        throw new HttpException(401, err.message);
    }

    req["session"] = session;
    return next();
}

And this error handler:

export function errorMiddleware(
  request: Request,
  response: Response,
  error: Error
) {
  let status = 500;
  if (error instanceof HttpException) {
    status = error.status;
  }
  const message = error.message || "Something went wrong.";

  let data = undefined;
  if (error instanceof ZodError) {
    data = error.errors;
  }

  logger.error(`${status} - ${message} - ${request.originalUrl} - ${request.method} - ${data}: ${error.stack || error.message}`);

  response.status(status).json({ message });
}

The error handler code is called, but still the application crashes:

node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

HttpException [Error]: No authorization header
    at Object.authJwtMiddleware (/Users/michael/Documents/Projects/Stofloos/rondom-ewd/src/middlewares/auth.middleware.ts:10:15)
    at Route.handle (/Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/hyper-express/src/components/router/Route.js:112:37)
    at iterator (/Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/hyper-express/src/components/router/Route.js:102:22)
    at cors (/Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/cors/lib/index.js:188:7)
    at /Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/cors/lib/index.js:224:17
    at originCallback (/Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/cors/lib/index.js:214:15)
    at /Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/cors/lib/index.js:219:13
    at optionsCallback (/Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/cors/lib/index.js:199:9)
    at Object.corsMiddleware [as handler] (/Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/cors/lib/index.js:204:7)
    at Route.handle (/Users/michael/Documents/Projects/Stofloos/rondom-ewd/node_modules/hyper-express/src/components/router/Route.js:112:37) {
  status: 401
}

It however doesn't crash when writing the middleware like this:

export async function authJwtMiddleware(req: Request, res: Response, next: MiddlewareNext) {
    const authHeader = req.get("Authorization");
    if (!authHeader) {
        return next(new HttpException(401, "No authorization header"));
    }

    if (!authHeader.startsWith("Bearer ")) {
        return next(new HttpException(401, "Invalid authorization header"));
    }

    const token = authHeader.split(" ")[1];
    let session: JWTPayload;
    try {
        session = await DependencyProviderService.getImpl<JwtSessionService>(JwtSessionServiceDi).verifySession(token);
    } catch (err) {
        return next(new HttpException(401, err.message));
    }

    req["session"] = session;
    return next();
}

Is this intended to use return next(error) or is it a bug?

sudipzoom commented 1 month ago

Authorization header is a part of the http request header, make sure you are getting it from request headers in your middleware function. Also make sure that you have sent the auth token(base64-encoded) to the server with a subsequent request. I mean you must check that authorization header is present in your http request header. const authorization = request.headers.authorization Also I suggest you to use next() instead of return next() since it is a middleware function. Also make sure that you are correctly extracting the token from the authorization headers. It is very hard to guess the exact error because we can not reproduce the exact error at our end. Therefore follow above checks and reproduce the exact error.

Miggets7 commented 1 month ago

It's not about extracting the token. It's about throwing an Error instead of using next(Error). It's seems throwing causes the app to crash whereas using next(Error) doesn't. But both still reach the global error handler.

kartikk221 commented 1 month ago

It's not about extracting the token. It's about throwing an Error instead of using next(Error). It's seems throwing causes the app to crash whereas using next(Error) doesn't. But both still reach the global error handler.

If both throwing and next(Error) trigger the global error handler then what is the issue?

Also, by app crashing do you mean one of your snippets above somehow misses the global error handler and crashes the program due to an uncaught exception?

Miggets7 commented 1 month ago

What I mean is throwing causes the crash. So the program exits.

When using Next(error) the program doesn't crash, so it doesn't exit.

kartikk221 commented 2 weeks ago

Hey, I am looking into this now. It seems that the error may be escaping the global handler when not explicitly passed to Next while this should not be the case, it is likely a bug.