Middleware not executed when using `exclude` in `setGlobalPrefix`

xtrinch commented 3 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

In versions 10.3.4+, if using exclude in setGlobalPrefix, e.g.

    app.setGlobalPrefix('api', { exclude: ['/'] });

Then middleware stops being executed at all (any sort of middleware).

For reproduction see tests in https://github.com/xtrinch/nestjs-middleware-issue-demo

Minimum reproduction code

https://github.com/xtrinch/nestjs-middleware-issue-demo

Steps to reproduce

Run yarn test, observe that the tests do not pass - middleware should attach a header to request
Remove the exclude param from setGlobalPrefix
Run tests again and observe that the tests now pass - middleware attaches a header to request

Expected behavior

Middleware should run regardless of exclude in setGlobalPrefix

Package

[ ] I don't know. Or some 3rd-party package
[ ] code>@nestjs/common</code
[X] code>@nestjs/core</code
[ ] code>@nestjs/microservices</code
[ ] 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

10.3.4+

Packages versions

{
  "name": "test-project",
  "version": "0.0.1",
  "description": "",
  "author": "",
  "private": true,
  "license": "UNLICENSED",
  "scripts": {
    "build": "nest build",
    "format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\"",
    "start": "nest start",
    "start:dev": "nest start --watch",
    "start:debug": "nest start --debug --watch",
    "start:prod": "node dist/main",
    "lint": "eslint \"{src,apps,libs,test}/**/*.ts\" --fix",
    "test": "jest",
    "test:watch": "jest --watch",
    "test:cov": "jest --coverage",
    "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand",
    "test:e2e": "jest --config ./test/jest-e2e.json"
  },
  "dependencies": {
    "@nestjs/common": "^10.0.0",
    "@nestjs/core": "10.3.7",
    "@nestjs/platform-fastify": "^10.3.7",
    "reflect-metadata": "^0.2.0",
    "rxjs": "^7.8.1"
  },
  "devDependencies": {
    "@nestjs/cli": "^10.0.0",
    "@nestjs/schematics": "^10.0.0",
    "@nestjs/testing": "^10.0.0",
    "@types/express": "^4.17.17",
    "@types/jest": "^29.5.2",
    "@types/node": "^20.3.1",
    "@types/supertest": "^6.0.0",
    "@typescript-eslint/eslint-plugin": "^6.0.0",
    "@typescript-eslint/parser": "^6.0.0",
    "eslint": "^8.42.0",
    "eslint-config-prettier": "^9.0.0",
    "eslint-plugin-prettier": "^5.0.0",
    "jest": "^29.5.0",
    "prettier": "^3.0.0",
    "source-map-support": "^0.5.21",
    "supertest": "^6.3.4",
    "ts-jest": "^29.1.0",
    "ts-loader": "^9.4.3",
    "ts-node": "^10.9.1",
    "tsconfig-paths": "^4.2.0",
    "typescript": "^5.1.3"
  },
  "jest": {
    "moduleFileExtensions": [
      "js",
      "json",
      "ts"
    ],
    "rootDir": "src",
    "testRegex": ".*\\.spec\\.ts$",
    "transform": {
      "^.+\\.(t|j)s$": "ts-jest"
    },
    "collectCoverageFrom": [
      "**/*.(t|j)s"
    ],
    "coverageDirectory": "../coverage",
    "testEnvironment": "node"
  }
}

Node.js version

18

In which operating systems have you tested?

[X] macOS
[ ] Windows
[ ] Linux

Other

No response

Papooch commented 3 months ago

I have tested the reproduction and confirmed that the issue is only present with FastifyAdapter.

So the issue is most likely related to this fix: https://github.com/nestjs/nest/pull/13337

CodyTseng commented 3 months ago

I think this is caused by https://github.com/nestjs/nest/pull/11832, which was intended to address the issue of middleware being called multiple times. However, it filters out some paths. This is effective for Express, but not for Fastify

eric-deeporigin commented 2 months ago

Currently still affected by this even with latest nestjs (10.3.8) when using the mikro-orm module

kamilmysliwiec commented 2 months ago

I wonder if reverting this PR #11832 would fix your issue @eric-deeporigin

Would you like to try applying an inline patch just to test it out locally? (open node_modules and revert that change to see if it's causing issues for your project)

eric-deeporigin commented 1 month ago

I wonder if reverting this PR #11832 would fix your issue @eric-deeporigin

Would you like to try applying an inline patch just to test it out locally? (open node_modules and revert that change to see if it's causing issues for your project)

@kamilmysliwiec Sorry for the late reply. I did test your changes out, but unfortunately it did not work.

My change looks like this in the core/middleware/middleware-module.js file in my node_modules folder of the app

Because we are in a monorepo, I had to find all the places where the middleware-module.js was defined and make sure all of them were replaced with the below changes. These are the files I had to update:

./node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.1_rxjs@7.8.1/node_modules/@nestjs/core/middleware/middleware-module.js
./node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+platform-express@10.3.8_reflect-metadata@0.2.1_rxjs@7.8.0/node_modules/@nestjs/core/middleware/middleware-module.js
./node_modules/.pnpm/@nestjs+core@10.3.0_@nestjs+common@10.3.0_@nestjs+microservices@10.3.9_@nestjs+platform-expre_sn6fu2jetslishj5vuxdktzjei/node_modules/@nestjs/core/middleware/middleware-module.js
./node_modules/.pnpm/@nestjs+core@10.3.8_@nestjs+common@10.3.8_@nestjs+microservices@10.3.9_@nestjs+platform-expre_jpofokt5mcym25avs3ht3bnhbi/node_modules/@nestjs/core/middleware/middleware-module.js
./node_modules/.pnpm/@nestjs+core@10.3.0_@nestjs+common@10.3.0_@nestjs+platform-express@10.3.8_reflect-metadata@0.1.13_rxjs@7.8.1/node_modules/@nestjs/core/middleware/middleware-module.js

(we use NX with pnpm)

The change:

            paths.forEach(path => router(path, middlewareFunction));
            // const pathsToApplyMiddleware = [];
            // paths.some(path => path.match(/^\/?$/))
            //     ? pathsToApplyMiddleware.push('/')
            //     : pathsToApplyMiddleware.push(...paths);
            // pathsToApplyMiddleware.forEach(path => router(path, middlewareFunction));

Since the rest of the changes were testing/integration changes, I did not revert them.

I will work on a minimal repro shortly

eric-deeporigin commented 1 month ago

@kamilmysliwiec Here is a reproduction of the bug. It uses MikroORM and GraphQL.

https://github.com/eric-deeporigin/nestjs-global-prefix-bug

To be clear, the error that is seen is this:

[Nest] 18210  - 06/17/2024, 7:15:25 PM   ERROR [ExceptionsHandler] Using global EntityManager instance methods for context specific actions is disallowed. If you need to work with the global instance's identity map, use `allowGlobalContext` configuration option or `fork()` instead.
ValidationError: Using global EntityManager instance methods for context specific actions is disallowed. If you need to work with the global instance's identity map, use `allowGlobalContext` configuration option or `fork()` instead.

Any endpoint that hits a GQL resolver and uses mikro-orm will throw this error.

If a GQL resolver does not use mikro-orm, the error will not be present.

Any endpoint that does not hit a GQL resolver (ie, an HTTP/Rest endpoint) does not have this issue.

I have followed the instructions specified here on mikro-orm's documentation when using gql + nest + mikro-orm: https://mikro-orm.io/docs/usage-with-nestjs#request-scoping-when-using-graphql

The issue goes away when not using app.setGlobalPrefix('api')

micalevisk commented 1 month ago

@eric-deeporigin I'd suggest you to report this to MikroORM repo as well.

B4nan commented 1 month ago

@eric-deeporigin I'd suggest you to report this to MikroORM repo as well.

This has been reported on our end many times, no need for yet another report I can't do anything about, it needs to be fixed in nest.

https://github.com/nestjs/nest/issues/11572#issuecomment-1873927604

renanz commented 1 month ago

@kamilmysliwiec any update on this? This is also happening with Express adapter.

Anhdao153 commented 2 weeks ago

@xtrinch

i changed pattern of middleware .forRoutes({ path: '/', method: RequestMethod.ALL });

main.ts

 app.setGlobalPrefix('/api/v1', {
    exclude: [
      { path: '/', method: RequestMethod.GET },
      { path: '/api', method: RequestMethod.GET },
       .....
    ],
  });

my app.module.ts

export class AppModule implements NestModule {
  configure(consumer: MiddlewareConsumer) {
    consumer
      .apply(RequestMiddleware)
      .forRoutes({ path: '*/*', method: RequestMethod.ALL });
}

I think it still a bug, there will be a vul, which i cannot test. maybe help

nestjs / nest