Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Middleware not executed when using exclude in setGlobalPrefix #13401

Open
3 of 15 tasks
xtrinch opened this issue Apr 5, 2024 · 11 comments
Open
3 of 15 tasks

Middleware not executed when using exclude in setGlobalPrefix #13401

xtrinch opened this issue Apr 5, 2024 · 11 comments

Comments

@xtrinch
Copy link

xtrinch commented Apr 5, 2024

Is there an existing issue for this?

  • 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

  1. Run yarn test, observe that the tests do not pass - middleware should attach a header to request
  2. Remove the exclude param from setGlobalPrefix
  3. 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
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • 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?

  • macOS
  • Windows
  • Linux

Other

No response

@Papooch
Copy link
Contributor

Papooch commented Apr 5, 2024

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: #13337

@CodyTseng
Copy link
Contributor

I think this is caused by #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
Copy link

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

@kamilmysliwiec
Copy link
Member

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)

@micalevisk micalevisk removed the needs triage This issue has not been looked into label May 16, 2024
@eric-deeporigin
Copy link

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/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/middleware/middleware-module.js
./node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/middleware/middleware-module.js
./node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected]_@nestjs+platform-expre_sn6fu2jetslishj5vuxdktzjei/node_modules/@nestjs/core/middleware/middleware-module.js
./node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected]_@nestjs+platform-expre_jpofokt5mcym25avs3ht3bnhbi/node_modules/@nestjs/core/middleware/middleware-module.js
./node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/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
Copy link

@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
Copy link
Member

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

@B4nan
Copy link

B4nan commented Jun 18, 2024

@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.

#11572 (comment)

@renanz
Copy link

renanz commented Jun 20, 2024

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

@Anhdao153
Copy link

@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

@CodyTseng
Copy link
Contributor

CodyTseng commented Aug 14, 2024

@eric-deeporigin @B4nan hi, I tried the reproduction @eric-deeporigin provided, and I found that adding the { exclude: ['/graphql'] } option to setGlobalPrefix resolves the issue.

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

This is because the GraphQL module bypasses the setGlobalPrefix, and NestJS is unaware of the /graphql path, resulting in the MikroOrmMiddleware not being mounted on /graphql. I’m not very familiar with mikro-orm, this is my guess after testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants