Middleware not working with setGlobalPrefix's exclude list

toonpang commented 2 years ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

Middlewares are not applied to routes (that are excluded by setGlobalPrefix) when using Middlewares with app.setGlobalPrefix.

Minimum reproduction code

https://stackblitz.com/edit/nestjs-typescript-starter-nf5fjs?file=src%2Fmain.ts

Steps to reproduce

npm start
go to https://nestjs-typescript-starter-nf5fjs--3000.local.webcontainer.io/api/test and ######################## THIS IS CUSTOM MIDDLEWARE is printed out in the console
go to https://nestjs-typescript-starter-nf5fjs--3000.local.webcontainer.io/health-check and ######################## THIS IS CUSTOM MIDDLEWARE is NOT printed out in the console

Expected behavior

The middleware should be applied to all routes, including those excluded by global prefix.

Package

[ ] I don't know. Or some 3rd-party package
[X] code>@nestjs/common</code
[X] code>@nestjs/core</code
[ ] code>@nestjs/microservices</code
[X] code>@nestjs/platform-express</code
[ ] code>@nestjs/platform-fastify</code
[ ] code>@nestjs/platform-socket.io</code
[ ] code>@nestjs/platform-ws</code
[ ] code>@nestjs/testing</code
[ ] code>@nestjs/websockets</code
[ ] Other (see below)

Other package

No response

NestJS version

8.2.6

Packages versions

{
  "dependencies": {
    "@nestjs/common": "^8.1.1",
    "@nestjs/core": "^8.1.1",
    "@nestjs/platform-express": "^8.1.1",
    "reflect-metadata": "^0.1.13",
    "rimraf": "^3.0.2",
    "rxjs": "^7.4.0"
  },
  "devDependencies": {
    "@nestjs/cli": "^8.1.3",
    "@nestjs/schematics": "^8.0.4",
    "@nestjs/testing": "^8.1.1",
    "@types/express": "^4.17.13",
    "@types/jest": "^27.0.2",
    "@types/node": "^16.11.1",
    "@types/supertest": "^2.0.11",
    "@typescript-eslint/eslint-plugin": "^4.29.2",
    "@typescript-eslint/parser": "^4.29.2",
    "eslint": "^7.32.0",
    "eslint-config-prettier": "^8.3.0",
    "eslint-plugin-prettier": "^3.4.1",
    "jest": "^27.3.0",
    "prettier": "^2.4.1",
    "source-map-support": "^0.5.20",
    "supertest": "^6.1.6",
    "ts-jest": "^27.0.7",
    "ts-loader": "^9.2.6",
    "ts-node": "^10.3.0",
    "tsconfig-paths": "^3.11.0",
    "typescript": "^4.4.4"
  }
}

Node.js version

16.14.0

In which operating systems have you tested?

[X] macOS
[ ] Windows
[ ] Linux

Other

No response

Zclhlmgqzc commented 2 years ago

🤔

https://github.com/nestjs/nest/blob/6e1a5d2559fbfcbf55f66a4995df7121bb5dd791/packages/core/middleware/middleware-module.ts#L276-L279

=>

if (
  Array.isArray(excludedRoutes) && 
  (isRouteExcluded(excludedRoutes, path, method) || 
  // or ['*', '/*', '(.*)', '/(.*)'].includes(path)
  ['*', '/*', '(.*)', '/(.*)'].includes(path))
)

toonpang commented 2 years ago

do you mind giving a bit more explanation on your reply? thanks!

yn4v4s commented 2 years ago

Using .forRoutes(AppController, HealthCheckController) instead of .forRoutes('*') works.

toonpang commented 2 years ago

Thanks @yn4v4s for you reply. Yup it do work if we add in the controllers manually. However, having a .forRoutes('*') will be much more convenient as there is no need to add in every time a new controller is created.

kamilmysliwiec commented 2 years ago

Would you like to create a PR to address this issue?

yn4v4s commented 2 years ago

Thanks @yn4v4s for you reply. Yup it do work if we add in the controllers manually. However, having a .forRoutes('*') will be much more convenient as there is no need to add in every time a new controller is created.

As an alternative (if your middleware doesn't need any dependencies) you can use a functional middleware and bind it globally in your main.ts using app.use(customMiddleware)

toonpang commented 2 years ago

Would you like to create a PR to address this issue?

let me give it a try

BPerry24 commented 2 years ago

@toonpang have you made any progress?

by the way - this also appears to get the job done .forRoutes(AppController, '*'), where in my case AppController is the controller for my health check route and excluded from the global prefix. This way you don't need to add every new controller to this list, only those which are explicitly excluded.

kamilmysliwiec commented 2 years ago

Let's track this here https://github.com/nestjs/nest/pull/9332

mcworox commented 1 year ago

Hey @kamilmysliwiec, I think this one #9332 introduced a new bug. Considering the middleware in all routes (including the excluded from the prefix) causes parallel consequences like applying the middleware when using @nestjs/serve-static. Now everyone that access my website gets 401.

In middlewareConsumer.forRoutes(), exist a way to consider using the api prefix as well? Because here we can not just .forRoutes('api/controller/(.*)'), only .forRoutes('controller/(.*)'). Probably may solve all the related problems

linkFly6 commented 1 year ago

嘿@kamilmysliwiec，我认为这个#9332引入了一个新错误。考虑所有路由中的中间件（包括从前缀中排除的）会导致并行结果，例如在使用时应用中间件@nestjs/serve-static。现在每个访问我网站的人都会收到 401。

在中middlewareConsumer.forRoutes()，是否也存在一种考虑使用 api 前缀的方法？因为在这里我们不能只是.forRoutes('api/controller/(.*)')，只能.forRoutes('controller/(.*)')。大概可以解决所有相关的问题

This problem still exists.

nestjs / nest