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

ValidationPipe still uses class-validator package #8562

Closed
5 of 15 tasks
IanMoroney opened this issue Nov 11, 2021 · 19 comments
Closed
5 of 15 tasks

ValidationPipe still uses class-validator package #8562

IanMoroney opened this issue Nov 11, 2021 · 19 comments
Labels
needs triage This issue has not been looked into

Comments

@IanMoroney
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

When changing the class-validator package to instead use @nestjs\class-validator, I found that ValidationPipe still requires the class-validator package.
This is also verified by this page:
https://docs.nestjs.com/techniques/validationUsePipes(new ValidationPipe

[Nest] 150  - 11/11/2021, 11:00:13 AM   ERROR [PackageLoader] The "class-validator" package is missing. Please, make sure to install this library ($ npm install class-validator) to take advantage of ValidationPipe.
#14 77.71   ●  process.exit called with "1"
#14 77.71 
#14 77.71       59 | 
#14 77.71       60 |   @Post()
#14 77.71     > 61 |   @UsePipes(new ValidationPipe({ transform: true }))

Minimum reproduction code

none yet

Steps to reproduce

npm ci
npm test
npm run build

Expected behavior

I was hoping the class-validator package would have been detected after installing @nestjs\class-validator

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

@nestjs/class-validator

NestJS version

8.2.0

Packages versions

"@azure/service-bus": "^7.0.2",
"@nestjs/azure-database": "^2.1.0",
"@nestjs/class-validator": "^0.13.3",
"@nestjs/common": "^8.2.0",
"@nestjs/config": "^1.1.0",
"@nestjs/core": "^8.2.0",
"@nestjs/platform-express": "^8.2.0",

Node.js version

14.18.1

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@IanMoroney IanMoroney added the needs triage This issue has not been looked into label Nov 11, 2021
@micalevisk
Copy link
Member

I was hoping the class-validator package would have been detected after installing @nestjs/class-validator

I'm pretty sure it will soon.

To avoid breaking changes, I think we can still use class-validator as a fallback

@IanMoroney
Copy link
Author

I was hoping the class-validator package would have been detected after installing @nestjs/class-validator

I'm pretty sure it will soon.

To avoid breaking changes, I think we can still use class-validator as a fallback

sadly the class-validator package isn't being maintained and has a high vulnerability in it which is not being addressed. Many issues raised (and PRs too) over it.
I think the lack of updates and required patches is what drove the fork in the first place.

@micalevisk
Copy link
Member

micalevisk commented Nov 11, 2021

My point is that changing this will be a breaking change as Nest will try to load @nestjs/class-validator and so will break folks out there that didn't installed this package yet, right?

Nest can release a patch that still loads class-validator if @nestjs/class-validator was not found. And drops class-validator on v9

@micalevisk
Copy link
Member

nevermind.

Since class-validator ins't mandatory, replacing it with @nestjs/class-validator isn't a breaking a change. And also Nestjs throws errors due to missing packages, which helps people to figure out that they can uninstall class-validator

@jmcdo29
Copy link
Member

jmcdo29 commented Nov 11, 2021

It would be a breaking change to just swap out class-validator for @nestjs/class-valdator because there's nothing in the update that would tell npm/yarn/pnpm that a new package would be necessary to download, so people updating package versions would now get this new error seemingly out of nowhere on a minor update. We could do a multi-step try/catch, i.e. try for @nestjs/class-validator catch try class-validator catch throw error, but the less optional package requires we have, usually the better (it'll also mean needing to update our webpack config in nest-cli to exclude the @nestjs/class-validator and @nestjs/class-transformer packages).

My plans would be to hold out until Nest v9 and have the forked package ready and maintained, and give instructions for how people could override the default if they wanted to, essentially making it opt-in until v9. Something like

@Injectable()
export class NestValidationPipe extends ValidationPipe {
  loadValidator() {
    return require('@nestjs/class-validator');
  }

  loadTransformer() {
    return require('@nestjs/class-transformer');
  }
}

Would be all that's necessary to use this package (if it's been published of course, I haven't checked that part yet)

@FelipeEmerim
Copy link

This is also a problem with class-transformer and ClassSerializerInterceptor, you can patch it using the following code:

/* eslint-disable global-require */
/* eslint-disable @typescript-eslint/no-var-requires */
import {
  ClassSerializerInterceptor as NestClassSerializerInterceptor,
  ClassSerializerInterceptorOptions,
  Injectable,
  Optional,
} from '@nestjs/common';
import { Reflector } from '@nestjs/core';

@Injectable()
export class ClassSerializerInterceptor extends NestClassSerializerInterceptor {
  constructor(
    protected readonly reflector: Reflector,
    @Optional()
    protected readonly defaultOptions: ClassSerializerInterceptorOptions = {},
  ) {
    super(reflector, {
      ...defaultOptions,
      transformerPackage: require('@nestjs/class-transformer'),
    });
  }
}

@shaunek
Copy link

shaunek commented Nov 13, 2021

FYI that if you are using below v8.2, FelipeEmerim's code sample won't work since before v8.2 that class had a less flexible constructor. I'm guessing that if you use a previous nest version you couldn't extend ClassSerializerInterceptor easily and instead would just want to copy/paste that class into your project if this matters enough to you. Maybe there is a better way?

In any case, thanks for your code snippets @FelipeEmerim and @jmcdo29

@micalevisk
Copy link
Member

micalevisk commented Nov 13, 2021

Maybe there is a better way?

I guess using https://www.npmjs.com/package/patch-package will be better as you can just replace class-transformer string with @nestjs/class-transformer

@kamilmysliwiec
Copy link
Member

You can start using forks now in the following way:

app.useGlobalPipes(new ValidationPipe(
  {
    validatorPackage: require('@nestjs/class-validator'),
    transformerPackage: require('@nestjs/class-transformer')
  }
));

We'll keep using the original packages for the time being till we figure out what's the best approach to migrate.

@sszczep
Copy link

sszczep commented Aug 19, 2022

Hey, It's been almost a year now. Any update on this?

@KikoCosmetics
Copy link

KikoCosmetics commented Aug 22, 2022

Same here.

Doesn't work using

app.useGlobalPipes(new ValidationPipe({
    transform: true
}));

or even

app.useGlobalPipes(new ValidationPipe({
    validatorPackage: require('@nestjs/class-validator'),
    transformerPackage: require('@nestjs/class-transformer')
}));

But works using

@UsePipes(new ValidationPipe({transform: true}))
@Controller()
export class MyFooController {

}

I was looking for a way of using the Config Module to apply this, but still no joy.
If it helps, I was debugging the useGlobalPipes function this morning and after being called I actually see the ValidationPipe inside the app instance.

Cheers

@jmcdo29
Copy link
Member

jmcdo29 commented Aug 22, 2022

@KikoCosmetics can you create a minimum reproduction and a new issue if the useGlobalPipes() method is not working? I'm not sure that's related specifically to the ValidationPipe or this issue. However, if the method isn't working, we need to get that fixed

@KikoCosmetics
Copy link

@jmcdo29 ok I'll try to setup a webcontainer on Stackblitz...see what happens!
Give me a few hours ;)

@KikoCosmetics
Copy link

@jmcdo29 Done 😁

https://stackblitz.com/edit/node-toz46t

If you uncomment the @UsePipes in app.controller
you'll get a 400 response in the textarea.
The webcontainer doesn't work very well and when you save it breaks...
But if you save and reload the whole page, it works...
Here are the behaviours, just in case.

just the global:

image

in the controller:

image

@jmcdo29
Copy link
Member

jmcdo29 commented Aug 22, 2022

@KikoCosmetics in your main.ts you have app.useGlobalPipes after the app.listen() meaning the pipe can't be bound before all the route handlers are. Move it before and it will work. If you're still having trouble with that, let's move over to our Discord for support

@KikoCosmetics
Copy link

KikoCosmetics commented Aug 22, 2022

You're right it works now!
Maybe I would make it more clear in the docs, that await app.listen(3000); should be the last instruction in the bootstrap.
I honestly didn't think about that.
Thanks a lot for the support!

For anyone reading this in the future, if you actually go further in the "Global scoped pipes" section in the docs, there's a cooler way of achieving the same result, which is using providers.
So this is what I've got in my AppModule and I was able to remove the app.globalPipes (at least for now).

import {APP_PIPE} from "@nestjs/core";
import {Module, ValidationPipe} from "@nestjs/common";

// ...
 providers: [
        {
            provide: APP_PIPE,
            useFactory: function (): ValidationPipe {
                return new ValidationPipe({
                    transform: true,
                    whitelist: true, // remove extra props
                })
            }
        }
    ]
// ...

@joshuawwright
Copy link

joshuawwright commented Aug 27, 2022

It appears the latest versions of class-transformer and class-validator are no longer flagged for the critical security vulnerability. So maybe nestjs/class-transformer and nestjs/class-validator should be marked as deprecated?

@sszczep
Copy link

sszczep commented Aug 27, 2022

It appears the latest versions of class-transformer and class-validator are no longer flagged for the critical security vulnerability. So maybe nestjs/class-transformer and nestjs/class-validator should be marked as deprecated?

There haven't been any releases lately so I doubt the vulnerability has been fixed. Can't run the test right now though. Also @nestjs packages introduced more than that fix, there were many other resolved issues.

@sszczep
Copy link

sszczep commented Aug 27, 2022

Regarding the original question, I didn't like patch-package nor specifying packages manually in every pipe, we have decided to go with package aliasing:

"dependencies": {
    "class-transformer": "npm:@nestjs/[email protected]",
    "class-validator": "npm:@nestjs/[email protected]",
}

Works like a charm and didn't require any code to set it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

9 participants