Using AdminBro alongside NestJS breaks payload validation

[pbn4]

When the AdminModule is registered into root AppModule the validation errors are triggered in every other place, all the time as If the request contained no payload. I created a minimal reproduction repository https://github.com/pbn4/adminbro-nestjs-validation-bug/blob/master/src/app.module.ts and here is an example request:

curl -X POST http://localhost:3000/ -d '{"hello": "hello"}' -H "Content-Type: application/json"

Comment out AdminModule import in AppModule and see it working again.

[SimonB407]

Thank you for your repo with steps to reproduce! Probably it's related to @admin-bro/express using formidable under the hood which is causing issues with body-parser that is used by default in Nest. I'll look into solution how to avoid that.

[pablosproject]

Same happening to me here, all the request came with an empty body.

[niksoc]

@SimonB407 this issue occurs because admin-bro-nestjs pulls out jsonParser and urlencoded parser from their original position in the stack and pushes it back at the end of the stack. Unfortunately, this may be after all the route handling in nestjs. I've wrapped around the admin module to fix this temporarily.

import { AdminModule } from '@admin-bro/nestjs'
import { Module, OnApplicationBootstrap } from '@nestjs/common'
import { HttpAdapterHost } from '@nestjs/core'

import { adminPanelConfig } from './config'

/**
 * Wraps AdminBro AdminModule and reorders express middleware
 * See: https://github.com/SoftwareBrothers/admin-bro-nestjs/blob/e73598c65b5109d0086a566b3aa48ff475117929/src/loaders/express.loader.ts#L29
 * admin-bro uses express formidable which conflicts with NestJs's body parser
 * So admin-bro-nestjs removes body parser from the express middleware stack,
 * adds the admin middleware and then pushes back body parser. But the body parser
 * needs to come before all the other routes in the application. So in this module
 * we pick up admin and body parser middleware and put them before all the other
 * routes
 */
@Module({
  imports: [AdminModule.createAdminAsync(adminPanelConfig)],
})
export class AdminPanelModule implements OnApplicationBootstrap {
  constructor(private readonly _httpAdapterHost: HttpAdapterHost) {}

  onApplicationBootstrap() {
    const { httpAdapter } = this._httpAdapterHost
    const app = httpAdapter.getInstance()

    const jsonParserIndex = app._router.stack.findIndex(
      (layer: { name: string }) => layer.name === 'jsonParser',
    )
    const jsonParser = app._router.stack.splice(jsonParserIndex, 1)

    const urlencodedParserIndex = app._router.stack.findIndex(
      (layer: { name: string }) => layer.name === 'urlencodedParser',
    )
    const urlencodedParser = app._router.stack.splice(urlencodedParserIndex, 1)

    const adminIndex = app._router.stack.findIndex((layer: { regexp: RegExp }) =>
      layer.regexp.toString().includes('admin'),
    )
    const admin = app._router.stack.splice(adminIndex, 1)

    // if admin-bro-nestjs didn't reorder the middleware
    // the body parser would have come after corsMiddleware
    const corsIndex = app._router.stack.findIndex(
      (layer: { name: string }) => layer.name === 'corsMiddleware',
    )

    app._router.stack.splice(
      corsIndex + 1,
      0,
      ...admin,
      ...jsonParser,
      ...urlencodedParser,
    )
  }
}

[github-actions[bot]]

:tada: This issue has been resolved in version 1.1.0-beta.1 :tada:

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[hra4h03]

@niksoc after adding this module it throws an error

TypeError: res.redirect is not a function at node_modules/@admin-bro/express/plugin.js:216

"@admin-bro/express": "^3.0.1", "@admin-bro/mongoose": "^1.1.0", "@admin-bro/nestjs": "^1.0.0",

[niksoc]

@Hrach2003 the fix for this seems to have been released on beta so I think you should try that instead of my workaround. https://www.npmjs.com/package/@admin-bro/nestjs/v/1.1.0-beta.1

[SimonB407]

@Hrach2003 I recommend using 1.1.0-beta.3. Beta 1 had some bugs.

[dschoeni]

I'm currently trying out 1.1.0-beta.3, but I'm somehow missing sth:

express-session deprecated undefined resave option; provide resave option node_modules/@admin-bro/express/lib/buildAuthenticatedRouter.js:56:41
express-session deprecated undefined saveUninitialized option; provide saveUninitialized option node_modules/@admin-bro/express/lib/buildAuthenticatedRouter.js:56:41
TypeError: admin is not iterable
    at ExpressLoader.reorderRoutes (/api/node_modules/@admin-bro/nestjs/build/loaders/express.loader.js:52:55)
    at ExpressLoader.register (/api/node_modules/@admin-bro/nestjs/build/loaders/express.loader.js:30:14)

In my package.json:

    "@admin-bro/express": "^3.0.1",
    "@admin-bro/nestjs": "1.1.0-beta.3",
    "@admin-bro/typeorm": "^1.4.0",
    "admin-bro": "^3.3.1",

I use a custom ExpressAdapter in my NestJS Project:

  const expressApp = express()
  const adapter = new ExpressAdapter(expressApp)
  ...
  const app = await NestFactory.create(AppModule, adapter)

Edit: Seems to be related to my Sentry Integration that also accesses the ExpressApp (new Tracing.Integrations.Express({ app: expressApp })). I'll investigate this ;)

[SimonB407]

Probably this custom adapter is causing some issues with default ExpressLoader. You can write custom loader for plugin and pass it in options (it was introduced in those betas that you can pass your own loader and don't have to rely on default one).

[SimonB407]

Here is a bit of documentation that could help with custom loaders. https://github.com/SoftwareBrothers/admin-bro-nestjs/blob/f8fdfbca8fdc92221dadc1b98aa4bb90e605d6a5/src/index.ts#L168

[dschoeni]

Jepp, I'm trying that out right now. I have some type-conflicts related to AbstractHttpAdapter though, seems as @admin-bro/nestjs has nestjs/core & nestjs/common bundled as dependency with >7, I'll probably have to upgrade my NestJS version first.

[dschoeni]

There seem to by some conflicts around the typings, even after updating NestJS:

Types of parameters 'httpAdapter' and 'httpAdapter' are incompatible.
      Type 
      'import("~/api/node_modules/@admin-bro/nestjs/node_modules/@nestjs/core/adapters/http-adapter").AbstractHttpAdapter<any, any, any>' 
      is not assignable to type 
      'import("~/api/node_modules/@nestjs/core/adapters/http-adapter").AbstractHttpAdapter<any, any, any>'.

...

Property 'instance' is protected but type 'AbstractHttpAdapter<TServer, TRequest, TResponse>' is not a class derived from 'AbstractHttpAdapter<TServer, TRequest, TResponse>'.

Shouldn't NestJS Core + Common be declared as PeerDeps, or am I missing sth?

Edit: This issue probably isn't the right channel to discuss this specific problem, and I might be able to figure it out somehow from here - So just let me know if we should take the discussion elsewhere or its a non-issue for you guys to fix ;)

[SimonB407]

Yeah, you right, it should be peer dependency, will be fixed.

[SimonB407]

@dschoeni Fix should be released as beta.4 in couple minutes. Thanks for spotting an error!

[dschoeni]

Thanks for the quick heads up! I did some more investigation regarding the first mentioned issue, and found that the sentryjs Tracing handler for Express is messing with the routes and their "names", which breaks the ExpressAdapter you had written that used these to filter out the bodyParser and json middleware of express.

I'll file a separate issue for this, although I'm not sure its entirely fixable given the current approach.

[mamal72]

Any news on the proper fix for this? I wasn't able to use it in a normal way with the latest published package (1.1.0-beta.3).

[jonatasfernandespimenta]

Tried 1.1.0-beta.3 but didn't work too... When I make a POST request using insomnia for example, it loads forever :c

[github-actions[bot]]

:tada: This issue has been resolved in version 1.1.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: