Skip to content

AddFileTypeValidator doesn't work correctly #14970

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

Closed
3 of 15 tasks
NirZamirGuardz opened this issue Apr 15, 2025 · 25 comments
Closed
3 of 15 tasks

AddFileTypeValidator doesn't work correctly #14970

NirZamirGuardz opened this issue Apr 15, 2025 · 25 comments
Labels
needs triage This issue has not been looked into

Comments

@NirZamirGuardz
Copy link

NirZamirGuardz commented Apr 15, 2025

Is there an existing issue for this?

  • I have searched the existing issues

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

  1. npm ci
  2. npm run start: dev / starting command
    We haven't done anything special, and we noticed it when several tests failed and blocked the pipe.

Expected behavior

To work correctly as it worked in the past
In the past, the following worked
.addFileTypeValidator({ fileType: /jpeg|png/, })

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

  • macOS
  • Windows
  • Linux

Other

No response

@NirZamirGuardz NirZamirGuardz added the needs triage This issue has not been looked into label Apr 15, 2025
@NirZamirGuardz NirZamirGuardz changed the title FileTypeValidator doesn't work correctly AddFileTypeValidator doesn't work correctly Apr 15, 2025
@tizianozonta
Copy link

I think the problem is related to this issue #14881
@Chathula may be possible?

@Chathula
Copy link
Contributor

I think the problem is related to this issue #14881
@Chathula may be possible?

I think yes. I will check the implementation and i will cover these cases in test as well.

@tizianozonta
Copy link

I think the problem is related to this issue #14881
@Chathula may be possible?

I think yes. I will check the implementation and i will cover these cases in test as well.

thank you very much!

@mariusrubei
Copy link

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

@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

@francescosalvi
Copy link

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

@phlogisticfugu
Copy link

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

@Chathula
Copy link
Contributor

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: Borewit/strtok3#1224
file-type: sindresorhus/file-type#745

@Chathula
Copy link
Contributor

@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

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.

@Chathula
Copy link
Contributor

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,
});

@phlogisticfugu
Copy link

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

@Borewit
Copy link
Contributor

Borewit commented Apr 15, 2025

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.

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Apr 15, 2025

@Chathula
Copy link
Contributor

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,
      ]);

@kamilmysliwiec
Copy link
Member

Fixed in v11.0.19 (v11 users) and v10.4.17 (v10 users)

(file-type added as a required dependency)

@Borewit
Copy link
Contributor

Borewit commented Apr 15, 2025

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

@Chathula
Copy link
Contributor

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

@Borewit
Copy link
Contributor

Borewit commented Apr 15, 2025

I did create a PR replacing the eval with load-esm: #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:

  1. The dynamically importing the file-type as an ESM module
  2. The fileTypeFromBuffer method

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

@kamilmysliwiec
Copy link
Member

#14970 (comment)

@boobo94
Copy link

boobo94 commented Apr 16, 2025

#14970 (comment)

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?

@Chathula
Copy link
Contributor

#14970 (comment)

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.

sindresorhus/file-type#264 (comment)

@boobo94
Copy link

boobo94 commented Apr 16, 2025

#14970 (comment)

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.

sindresorhus/file-type#264 (comment)

thanks. got the point 🙏

@Borewit
Copy link
Contributor

Borewit commented Apr 16, 2025

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.

@boobo94
Copy link

boobo94 commented Apr 16, 2025

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

@francescosalvi
Copy link

francescosalvi commented Apr 23, 2025

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.

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