Closed NirZamirGuardz closed 3 months ago
I think the problem is related to this issue https://github.com/nestjs/nest/pull/14881 @Chathula may be possible?
I think the problem is related to this issue https://github.com/nestjs/nest/pull/14881 @Chathula may be possible?
I think yes. I will check the implementation and i will cover these cases in test as well.
@Chathula I think an unintended breaking change was added when you made isValid
as async.
I used to do custom validators using FileTypeValidator
import { FileTypeValidator } from '@nestjs/common';
...
const validator = new FileTypeValidator({ fileType: expectedMimeType });
if(!validator.isValid(file))
....
Now since the method is async, it will break a lot of custom validators.
@Chathula I think an unintended breaking change was added when you made
isValid
as async.I used to do custom validators using
FileTypeValidator
import { FileTypeValidator } from '@nestjs/common'; ... const validator = new FileTypeValidator({ fileType: expectedMimeType }); if(!validator.isValid(file)) ....
Now since the method is async, it will break a lot of custom validators.
I will consider this as well
Just wondering, how come this (kinda serious) breaking change wasn't intercepted by CI/tests ?
I was upgrading due to npm audit
reporting the following, not thinking much of 1/2 patch revision changes, thank god the project had integration tests covering FileTypeValidator
usage cases...
@nestjs/common
ββ ID: 1103892
ββ Issue: nest allows a remote attacker to execute arbitrary code via the Content-Type header
ββ URL: https://github.com/advisories/GHSA-cj7v-w2c7-cp7c
ββ Severity: moderate
ββ Vulnerable Versions: <11.0.16
β
ββ Tree Versions
β ββ 11.0.15
we just updated as well - and are getting similar issues where a previously working file upload is now failing.
suspect that it may be because the file-type
package is now being used, but when the import of it fails (silently) - the validation returns false
https://github.com/nestjs/nest/blob/master/packages/common/pipes/file/file-type.validator.ts#L52-L64
propose: adding file-type as a dependency
or being more noisy about a failed import
One of the dependencies in the file-type package has an issue detecting a PNG file buffer and throws an error. I have already created tickets on that.
strtok3: https://github.com/Borewit/strtok3/issues/1224 file-type: https://github.com/sindresorhus/file-type/issues/745
@Chathula I think an unintended breaking change was added when you made
isValid
as async. I used to do custom validators usingFileTypeValidator
import { FileTypeValidator } from '@nestjs/common'; ... const validator = new FileTypeValidator({ fileType: expectedMimeType }); if(!validator.isValid(file)) ....
Now since the method is async, it will break a lot of custom validators.
I will consider this as well
Making this function synchronous will not be easy as we are using the file-type
package with dynamic import through eval.
Then we have to use a package like this, or we have to implement our own helper to detect the file type based on the buffer. I used this package earlier https://github.com/nir11/file-type-checker. But @kamilmysliwiec mentioned that it doesn't have enough popularity to use it.
For now, you can use below skipMagicNumbersValidation
parameter to skip the magic numbers validation. This will use old validation(which is vulnerble according to the Github advisory). This has been added as we can't detect text-based formats(files like txt, json etc) through magic numbers. So we have to detect the MIME type the way we used to.
const fileTypeValidator = new FileTypeValidator({
fileType: 'image/jpeg',
skipMagicNumbersValidation: true,
});
mmmm thats unfortunate that PNG's dont work - its one of our primary use-cases (to allow image uploads, but not malicious files)
we'd be willing to use file-type-checker
https://snyk.io/advisor/npm-package/file-type-checker if it will result in a better file check
Making this function synchronous will not be easy as we are using the file-type package with dynamic import through eval.
I assume you try to load the file-type, which is an ESM module, in a TypeScript CommonJS project. Then you run into the issue, the TypeScript compiler converts the dynamic import, which you need to load the ESM module, to require
. Does not make much sense, but that is what the compiler does.
Better to use load-esm to load the ESM module, rather then eval
.
More detailed explenation: https://stackoverflow.com/a/79265806/28701779
One of the dependencies in the file-type package has an issue detecting a PNG file buffer and throws an error. I have already created tickets on that.
PNG detection certainly works in file-type, but you need to provide a bigger sample (not the first 8 bytes). For the best results, better to provide the actual file.
Making this function synchronous will not be easy as we are using the file-type package with dynamic import through eval.
I assume you try to load the file-type, which is an ESM module, in a TypeScript CommonJS project. Then you run into the issue, the TypeScript compiler converts the dynamic import, which you need to load the ESM module, to
require
. Does not make much sense, but that is what the compiler does.Better to use load-esm to load the ESM module, rather then
eval
.More detailed explenation: https://stackoverflow.com/a/79265806/28701779
One of the dependencies in the file-type package has an issue detecting a PNG file buffer and throws an error. I have already created tickets on that.
PNG detection certainly works in file-type, but you need to provide a bigger sample (not the first 8 bytes). For the best results, better to provide the actual file.
Thanks for update. it worked fine with bigger buffer like this
const pngBuffer = Buffer.from([
0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x0a, 0x00, 0x0d, 0x4a,
0x46, 0x49, 0x46, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae,
0x42, 0x60, 0x82, 0x00, 0x00, 0x00, 0x00, 0x43, 0x52, 0x49, 0x43, 0x41,
]);
Fixed in v11.0.19 (v11 users) and v10.4.17 (v10 users)
(file-type added as a required dependency)
The reason it does need more then 8 bytes (as 8 bytes is indeed the PNG magic file signature), is that it will read deeper into the PNG to determine it is a Animated PNG, a subtype of the PNG file..
Fixed in v11.0.19 (v11 users) and v10.4.17 (v10 users)
(file-type added as a required dependency)
How can we support the people who use this package now without async
? this is a breaking change for them
I did create a PR replacing the eval
with load-esm
: https://github.com/nestjs/nest/pull/14974
How can we support the people who use this package now without async ? this is a breaking change for them
There are 2 async portions involved:
fileTypeFromBuffer
methodLoading an ESM module via a dynamic import is an async
operations by design. Using require (synchronous) which supports loading ESM, is only available in Node.js engine 22.
See:
I am not aware of any other workaround.
fileTypeFromBuffer()
is defined as async
. And yes in theory it could be synchronous, and resolves instantly, but JavaScript has no way to convert it back into synchronous.
The reason it has to be async, is because the file-type supports different ways of reading from a "file", For example a file stream. And because that read can be async, the abstract logic has to be defined as async
.
So sorry for that.
Hei, thanks for your support.
I may need some help, I have upgraded to v11.0.20, and it is still failing for me:
new FileTypeValidator({
fileType:
/^(text\/(plain|csv|comma-separated-values|xml))|(application\/vnd.(ms-excel|openxmlformats-officedocument.spreadsheetml.sheet))|(application\/zip|application\/octet-stream|multipart\/x-zip|application\/zip-compressed|application\/x-zip-compressed|application\/x-zip)|(application\/pdf)$/,
}),
where the file is:
{
fieldname: 'file',
originalname: 'file.csv',
encoding: '7bit',
mimetype: 'text/csv',
buffer: <Buffer ...>,
size: 2281
}
using skipMagicNumbersValidation: true
does the trick
anything else I can do?
Hei, thanks for your support.
I may need some help, I have upgraded to v11.0.20, and it is still failing for me:
new FileTypeValidator({ fileType: /^(text\/(plain|csv|comma-separated-values|xml))|(application\/vnd.(ms-excel|openxmlformats-officedocument.spreadsheetml.sheet))|(application\/zip|application\/octet-stream|multipart\/x-zip|application\/zip-compressed|application\/x-zip-compressed|application\/x-zip)|(application\/pdf)$/, }), where the file is:
{ fieldname: 'file', originalname: 'file.csv', encoding: '7bit', mimetype: 'text/csv', buffer: <Buffer ...>, size: 2281 } using
skipMagicNumbersValidation: true
does the trickanything else I can do?
CSV is a text-based format. So you have to skip the magic numbers validation.
https://github.com/sindresorhus/file-type/issues/264#issuecomment-568439196
Hei, thanks for your support. I may need some help, I have upgraded to v11.0.20, and it is still failing for me: new FileTypeValidator({ fileType: /^(text/(plain|csv|comma-separated-values|xml))|(application/vnd.(ms-excel|openxmlformats-officedocument.spreadsheetml.sheet))|(application/zip|application/octet-stream|multipart/x-zip|application/zip-compressed|application/x-zip-compressed|application/x-zip)|(application/pdf)$/, }), where the file is: { fieldname: 'file', originalname: 'file.csv', encoding: '7bit', mimetype: 'text/csv', buffer: <Buffer ...>, size: 2281 } using
skipMagicNumbersValidation: true
does the trick anything else I can do?CSV is a text-based format. So you have to skip the magic numbers validation.
thanks. got the point π
You can extend file-type detection, in addition to the default binary formats, with most common XML formats, using the @file-type/xml plugin .
For CSV there is to my knowledge no implementation. This is no coincidence as it is very tricky to reliably detect a CSV from its file content.
You can extend file-type detection, in addition to the default binary formats, with most common XML formats, using the @file-type/xml plugin .
For CSV there is to my knowledge no implementation. This is no coincidence as it is very tricky to reliably detect a CSV from its file content.
Perfect. Checking the mime type is enough for me. But I had a breaking change after upgrading to the latest version. And justΔ wanted to double confirm thatβs the only way of doing it, with skipMagicNumbersValidation.
Thanks
I tried upgrading to the latest version (11.0.15
-> 11.0.21
) and my file-upload-related integration tests were still failing.
I started debugging and found that FileTypeValidator.isValid
was early returning false
at this line:
if (!isFileValid || !file.buffer) return false;
the reason being file.buffer
was undefined
.
Now, file
is a Express.Multer.File
, and as per documentation, its buffer
property only gets populated in case of MemoryStorage
.
In fact, as it turned out, in the Nest controller I had the dest
option, which selects DiskStorage
instead.
@UseInterceptors(FileInterceptor('file', { dest: './uploads' }))
Considering that FileTypeValidator
apparently requires MemoryStorage
, I think Nest documentation should very clearly warn the users that in fact they should not use FileInterceptor...dest
in conjunction with FileTypeValidator
.
In addition, I think failure of asserting the expectation that the multer-file was already read into a buffer should make FileTypeValidator
fail with a readable error... I can imagine many more dev scratching their heads, this scenario is literally inexplicable without debugging.
Is there an existing issue for this?
Current behavior
We had this in production for a while, and it worked well until a few hours ago
.addFileTypeValidator({ fileType: /jpeg|png/, })
We are getting the following error: "Validation failed (current file type is image/png, expected type is /jpeg|png/)"
Then we tried setting the meme type explicitly with a string instead of a regex
.addFileTypeValidator({ fileType: 'image/png', })
And got the following error: "Validation failed (current file type is image/png, expected type is image/png)"And at last, we tried the same as in the example in the docs
new ParseFilePipeBuilder() .addFileTypeValidator({ fileType: 'png', })
"Validation failed (current file type is image/png, expected type is png)"Minimum reproduction code
empty
Steps to reproduce
Expected behavior
To work correctly as it worked in the past In the past, the following worked
.addFileTypeValidator({ fileType: /jpeg|png/, })
Package
Other package
No response
NestJS version
10.4.16
Packages versions
[System Information] OS Version : macOS 24.3.0 NodeJS Version : v22.14.0 NPM Version : 10.9.2
[Nest CLI] Nest CLI Version : 10.4.9
[Nest Platform Information] platform-express version : 10.4.16 elasticsearch version : 10.0.2 schematics version : 10.2.3 passport version : 10.0.3 schedule version : 4.1.2 terminus version : 10.3.0 swagger version : 7.4.2 testing version : 10.4.16 bullmq version : 10.2.3 common version : 10.4.16 config version : 3.3.0 axios version : 3.1.3 core version : 10.4.16 jwt version : 10.2.0 cli version : 10.4.9
Node.js version
v22.14.0
In which operating systems have you tested?
Other
No response