-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
AddFileTypeValidator (still) doesn't work correctly #15055
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
Comments
are you using commonjs or ESM in your app project? (the |
Currently type is not specified in package.json (so defaults to commonjs), and tsconfig module was set to commonjs |
that has to do with Jest as well. You got no errors when running that app outside of Jest, right? then try this:
|
Thanks @micalevisk - that didn't resolve the issue on its own but once setting that flag, I can now see the file type validation fails because the test files are zero bytes. This previously was allowed through though, so perhaps there was another breaking change in this area? |
@micalevisk we actually encountered this first when doing a minor upgrade on via npm audit fix ("10.3.7" -> "10.4.17" for nestjs common) and then subsequently just decided to go straight to 11.x), so it's possible the zero byte files no longer being accepted was an older change that was made on 10.x some time ago, as opposed to related to what was back ported, and was just then hidden by the new requirement for experimental-vm-modules. I'll have more of a dig. |
I wonder if we can replace the nest/packages/common/pipes/file/file-type.validator.ts Lines 75 to 77 in f2d8543
with the same approach shown at nest/sample/34-using-esm-packages/src/import-esm-package.ts Lines 5 to 10 in f2d8543
can you please try it out in your project? just edit your |
@micalevisk that importEsmPackage method still triggers a "A dynamic import callback was invoked without --experimental-vm-modules" error for me, at least when defined as this in the JS file:
|
I think, the starting point of this issue was CI pipeline warnings, as exemplified by discussions in #14876. From NestJS's perspective, the original FileTypeValidator (which primarily used string/regex matching for file extensions) might not have been considered a "security vulnerability" per se, but rather a simpler validation mechanism as per its initial design. However, CI/security scanning tools began flagging this approach as insufficient for robustly preventing spoofed file types, leading to these CI warnings. To resolve these CI warnings and generally enhance file validation robustness, NestJS adopted the file-type library. This library performs more reliable validation by checking the file's actual magic numbers, which is a significant step up. However, file-type is an ESM-only package. This shift has now led to the current set of problems for projects still using CommonJS (CJS), primarily the requirement to use Node.js flags like --experimental-vm-modules. This impacts developer experience, especially in testing environments (like Jest) and potentially in production. In summary, while integrating an external library like file-type as a hard dependency was a solution to meet CI recommendations, it unintentionally introduced ESM/CJS compatibility issues. We now need to find a way to resolve the original CI concerns without forcing CJS users into undesirable workarounds or Node.js flag configurations. |
Uh oh!
There was an error while loading. Please reload this page.
Is there an existing issue for this?
Current behavior
We've just hit the same problem as #14970 (which is closed) upgrading to the latest packages, but that issue says it's fixed - it doesn't seem to be.
If we log the error being swallowed by this try, catch:
https://github.com/nestjs/nest/blob/master/packages/common/pipes/file/file-type.validator.ts#L93-L95
thrown by the loadEsm call:
https://github.com/nestjs/nest/blob/master/packages/common/pipes/file/file-type.validator.ts#L75-L76
we get:
It seems to be a breaking change - I'm not yet clear if this is limited to just our jest config or would require a change to our production environment too?
Minimum reproduction code
empty
Steps to reproduce
No response
Expected behavior
Tests pass as previously.
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 package
No response
NestJS version
No response
Packages versions
Node.js version
20.16.0
In which operating systems have you tested?
Other
No response
The text was updated successfully, but these errors were encountered: