Automatic decoding of JSON fields doesn't work well with browser requests

[cyantree]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

3.22.1

Plugin version

5.1.0

Node.js version

16.3.0

Operating system

Windows

Operating system version (i.e. 20.04, 11.3, 10)

20H2

Description

fastify-multipart uses busboy for multipart parsing. busboy has a logic to decide whether a part is a field or file by checking the filename:
https://github.com/mscdex/busboy/blob/master/lib/types/multipart.js#L174

However this doesn't work well with browser generated requests as it doesn't seem possible to leave out the filename when specifying a content type. The best I've achieved is passing an empty filename.

Steps to Reproduce

Setup the server:

await server.register(fastifyMultipart, {
    attachFieldsToBody: true,
});

Define the route:

server.post('/test', async (req, rep) => {
    console.log(req.body.json.value);
    rep.send('ok');
});

Send a browser request:

const body = new FormData();
body.append('json', new Blob(['{"foo":"bar"}'], {type: 'application/json'}), '');
fetch('/test', {method: 'POST', body});

Expected Behavior

The expected behaviour is getting a console output of the json payload.

However because of busboy's behaviour onField won't be triggered and therefore json parsing not started:
https://github.com/fastify/fastify-multipart/blob/master/index.js#L358

A possible solution could be checking detected files whether they have an empty filename and then deciding whether to parse them as json.

Another (hacky) solution would be allowing to configure a naming schema for part names that would be used to decide whether to handle a part as file or json field. Part names could look like json.foo.

However the best solution would be to adjust busboy. However I'm not sure whether this is realistic as it hasn't been updated in years.

[cyantree]

I've opened the following busboy issues:
mscdex/busboy#258

[radomird]

Hi @cyantree - I was trying to reproduce your use case. If you look closely to the documentation you'll see that the way you are trying to get the contents of the body is wrong.

I wrote a simple example on how you should access the field(s) (json field in your case):

const fastify = require('fastify')()

fastify.register(require('./index', {
    attachFieldsToBody: true,
}))

fastify.post('/test', async (req, rep) => {
    const data = await req.file()
    rep.send(data.fields.json.file) // will return to the browser a FileStream containing: {"foo": "bar"}
});

fastify.listen(3000, err => {
  if (err) throw err
  console.log(`server listening on ${fastify.server.address().port}`)
})

Since all the fields are considered "files" you have to read them as such, in your server.

I hope this helps.

[cyantree]

Hi @radomird,
thanks for taking the time analyzing my problem and crafting some example. 👍
The approach you're suggesting is correct, thanks for that. However it's not what I wanted.

• In your case the JSON field is handled as file but I specifically want it to be a non-file field.
• In your case the data needs to be parsed and validated manually but I want to facilitate the functionality provided by fastify-multipart.

In my usecase I want to use the functionality described here:

• https://github.com/fastify/fastify-multipart#json-schema-non-file-field
• https://github.com/fastify/fastify-multipart#json-non-file-fields

So since 5.1 it should be possible to pass data as a non-file field that will be parsed and validated automatically.

The following request would be processed correctly - but only because the part doesn't contain a filename:

POST http://localhost:3000/test
Content-Type: multipart/form-data; boundary=----WebKitFormBoundary9slwAmq4P8O9POU7

------WebKitFormBoundary9slwAmq4P8O9POU7
Content-Disposition: form-data; name="json"
Content-Type: application/json

{"foo":"bar"}
------WebKitFormBoundary9slwAmq4P8O9POU7--

In this case req.body.json.value represents the parsed JSON data.

A full working example is:

const fastify = require('fastify')();

fastify.register(require('fastify-multipart'), {
    attachFieldsToBody: true,
});

fastify.post('/test', {
    schema: {
        body: {
            type: 'object',
            required: ['json'],
            additionalProperties: false,
            properties: {
                json: {
                    type: 'object',
                    required: ['value'],
                    properties: {
                        value: {
                            type: 'object',
                            required: ['foo'],
                            additionalProperties: false,
                            properties: {
                                foo: {
                                    type: 'string',
                                },
                            },
                        },
                    },
                },
            },
        },
    },
}, async (req, rep) => {
    rep.send(req.body.json.value.foo);
});

fastify.listen(3000, err => {
  if (err) throw err
  console.log(`server listening on ${fastify.server.address().port}`)
});

However I found a new workaround that doesn't add too much overhead to get it running:

fastify.register(require('fastify-multipart'), {
    attachFieldsToBody: true,
    onFile: async (file, req) => {
        if (file.mimetype === 'application/json' && !file.filename) {
            // Use `secure-json-parse` for more safety
            file.value = JSON.parse((await file.toBuffer()).toString('utf8'));
        } else {
            await file.toBuffer();
        }
    },
});

By manually converting the JSON-data into a non-file field the data still will be validated through the configured schema.

[Eomm]

Could @cyantree explain why you need to use the Blob object instead of the string?

const body = new FormData();
body.append('json', JSON.stringify({"foo":"bar"}), { contentType: 'application/json' })
fetch('/test', {method: 'POST', body});

I don't think busboy will accept the request because the Blob object maps files

[cyantree]

Hi @Eomm,
thanks for your response.

My use case is a browser upload form which also passes some additional data. I want to use the functionality provided by this package for file and data validation.

I'm using a Blob because this allows me setting the Content-Type which is required to use JSON parsing and validation as described in https://github.com/fastify/fastify-multipart#json-non-file-fields
Please also see https://developer.mozilla.org/en-US/docs/Web/API/FormData/append#parameters
Is there any other way to do this in the browser?

Does your example work for you? It just throws an error for me:

Uncaught TypeError: Failed to execute 'append' on 'FormData': parameter 2 is not of type 'Blob'.

I assume you're running your example on Node.js with form-data. I've seen that fastify-multipart uses it for its tests. However unfortunately that's not applicable for my case and it's not compatible to the browser API.

Yes, busboy treats the part as file because of the sent filename. But unfortunately I can't force browsers to remove the filename that's why I opened an issue for busboy too. In my understanding it's acceptable to treat an empty filename as "no filename".

[radomird]

@cyantree After looking into this issue once more, the conclusion is that the workaround that you posted earlier is great, if it solves your problem then I think we are good.

Despite the issue with busboy, I've tried to come up with a workaround solution - just to see if we could do an easy detection and handle the json form data like a field - but there is no easy solution here, unfortunately, without seriously changing some internal handling.

This can be solved in two ways:

• it has to be fixed on the busboy side - Handle empty filenames as fields instead of files mscdex/busboy#258
  OR
• move away from using busboy

[mcollina]

An alternative idea is to remove the automatic decoding of JSON fields because we can't support it.

I don't think there is a brtter alternative than busboy.

[cyantree]

Thanks @radomird and @mcollina for your time.

@radomird
The workaround I posted earlier has one drawback. It doesn't check for max field size as would do the native approach. So I would need to copy/paste this too. However that's not a huge issue. So yeah, I think I'll go with the workaround for now.

@mcollina
Removing the feature depends on the use cases you want to support. As seen in the examples it's usable with Node.js as a client (and probably others) but not with the browsers. So maybe a hint in the docs would be a good starting point. (And maybe with a link to this issue and the workaround). I don't know how many people rely on this (quite new) feature.

Regarding switching libraries:
Have you checked out formidable? It looks quite promising and seems quite popular and active too. However I don't have any experience with it and don't know whether this issue would be solved by using it. And maybe if this is the first time busboy is causing any issues, it might be too much switching libraries a this point.

[kibertoad]

@mcollina Why not fork busboy to fastify org and do fixes there? Isn't that the route we've taken in the past?

[climba03003]

Regarding switching libraries: Have you checked out formidable? It looks quite promising and seems quite popular and active too. However I don't have any experience with it and don't know whether this issue would be solved by using it. And maybe if this is the first time busboy is causing any issues, it might be too much switching libraries a this point.

formidable isn't an option in my opinion, the stream support is the core of this lib. I do not see any other lib which provide stream support for multipart parsing atm.

[mcollina]

I propose we revert #288 as it was a recent addition.

This feature was asked recently I think it requires too much work to support.

(I'm ok in forking, but I think it's a stretch here.)

[kibertoad]

@mcollina Considering that busboy is pretty much unsupported right now, wouldn't that make sense long-term? And it would allow fixing this particular issue properly.

[mcollina]

I'm ok if somebody else would help but I have no bandwidth atm.

[kibertoad]

I can do this.
@cyantree Could you lend a hand with implementing needed changes inside a fork after I create it?

[kibertoad]

@mcollina I've created https://github.com/fastify/busboy and will work over weekend to update it a bit and merge whatever outstanding PRs make sense. After first forked version is ready to go, I'll open PR in fastify-multipart to do the switch.
Will also evaluate how difficult mscdex/busboy#258 will be to address, any PRs from anyone doing that would be most welcome.

[cyantree]

@kibertoad Yep, sure. Thanks!
Which approach do you think is better?

1. Handling parts with empty filename as non-file field. I think this is the most straightforward option. It might be seen as a breaking change as it changes some behaviour and I'm not sure whether a file might have no name at all. (But I doubt it can). So in my opinion the risk of breaking anything is fairly small.
2. Add a configuration for filtering parts as file or field. That would be backward compatible and shouldn't be too complicated either.

[kibertoad]

@cyantree Let's continue discussing in fastify/busboy repo (probably we need to create a new issue there), but I would go with approach 1 - I don't think we need to maintain backwards compatibility in this case, I would treat first release of fastify/busboy as a breaking change either way.
I'll work on moving tests over to mocha now, so feel free to use mocha for writing new tests.

[kibertoad]

After #300 lands, we should revisit this issue and check if fastify-multipart exposes enough of busboy configuration to make use of new isPartFile config option, or if some extra changes would be needed.

[kibertoad]

@cyantree Can you check if this is possible on latest master without any additional changes to fastify-multipart?

[cyantree]

@kibertoad
For Non-TS it works without any changes.
For my usecase I just needed to add

isPartAFile: (fieldName, contentType, fileName) => !!fileName,

However with TS it doesn't work without fixing the types as isPartAFile currently isn't known.

[kibertoad]

@cyantree Don't we just proxy BusboyConfig types in fastify-multipart? which part misses the type?

[Uzlopak]

I tested it and I see isPartAFile?!

[cyantree]

I'm using it like this:

await server.register(fastifyMultipart, {
        isPartAFile: (fieldName, contentType, fileName) => {
            console.log(fieldName, contentType, fileName);
            return !!fileName;
        },
        // Other configs ...
    });

In my installed version BusboyConfig is only passed to various methods in FastifyRequest but not to the constructor signature:

declare const fastifyMultipart: FastifyPluginCallback<FastifyMultipartOptions | FastifyMultipartAttactFieldsToBodyOptions>;

I've installed it in package.json like this: "fastify-multipart": "fastify/fastify-multipart#master",

I can confirm that it works within a handler like this:

req.files({isPartAFile: () => true});

However here I'm expected to provide headers too which I find quite confusing. Shouldn't this be omitted?

[kibertoad]

I'll check if having a subset of busboy config is ok there.

[kibertoad]

@cyantree Can you check with 5.2.1? I think I added missing type in the right place :)

[cyantree]

Thanks @kibertoad . It works for me. :)