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

Node 14.18 with http2: Uploading of files produces exception in deepmerge #274

Closed
2 tasks done
mccare opened this issue Sep 29, 2021 · 8 comments
Closed
2 tasks done

Comments

@mccare
Copy link
Contributor

mccare commented Sep 29, 2021

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

Plugin version

5.0.1

Node.js version

14.18

Operating system

Linux

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

cloud

Description

Since node 14.18 I get an exception when uploading a file. This is on Google Cloud Run with http2 enabled.
The stack trace shows:

TypeError: target.propertyIsEnumerable is not a function
    at /usr/src/app/node_modules/deepmerge/dist/cjs.js:55:18
    at Array.filter (<anonymous>)
    at getEnumerableOwnPropertySymbols (/usr/src/app/node_modules/deepmerge/dist/cjs.js:54:42)
    at getKeys (/usr/src/app/node_modules/deepmerge/dist/cjs.js:61:36)
    at mergeObject (/usr/src/app/node_modules/deepmerge/dist/cjs.js:86:2)
    at deepmerge (/usr/src/app/node_modules/deepmerge/dist/cjs.js:117:10)
    at cloneUnlessOtherwiseSpecified (/usr/src/app/node_modules/deepmerge/dist/cjs.js:34:5)
    at /usr/src/app/node_modules/deepmerge/dist/cjs.js:94:23
    at Array.forEach (<anonymous>)
    at mergeObject (/usr/src/app/node_modules/deepmerge/dist/cjs.js:86:18)

I've verified this by creating a sample project pushing it to google cloud run and uploading a file. It could be related to nodejs/node#34145 (comment) where the user reports errors when iterating of the headers.

Steps to Reproduce

I've created a sample project here and deployed this version on cloud run.

Run for the Cloud run version
curl -v -F key1=value1 -F upload=@./package.json https://multipart-5knuaozepq-uc.a.run.app/upload

Expected Behavior

The server should just return "ok".

@mcollina
Copy link
Member

Would you like to send a PR to fix this? This looks some bug you are the best equipped to fix.

@climba03003
Copy link
Member

climba03003 commented Sep 30, 2021

It is like a bug due to prototype of object is polluted.
or the option created by Object.create(null).

Strange, it should not fail.

const deepmerge = require("deepmerge");
const http2 = require("http2");

const headers = {
  [http2.sensitiveHeaders]: ["abc", "def"],
};

// success
deepmerge.all([{ headers: headers }, {}, {}]);
// success
deepmerge.all([{ headers: headers }, Object.create(null), {}]);

@mccare
Copy link
Contributor Author

mccare commented Sep 30, 2021

So narrowed it down a bit. With http2 on, neither the request.headers nor request.raw.headers have the propertyIsEnumerable function defined. In dev mode both objects have this function. This function is then missing and needed by deepmerge.

@climba03003
Copy link
Member

climba03003 commented Sep 30, 2021

So narrowed it down a bit. With http2 on, neither the request.headers nor request.raw.headers have the propertyIsEnumerable function defined. In dev mode both objects have this function. This function is then missing and needed by deepmerge.

This method is extends from Object.prototype. I can't find out where it is missing, even Object.create(null) can run without problem.

https://github.com/fastify/fastify/blob/4b389207765ac2c746725041dca0043b9c2aaf01/lib/request.js#L176-L179

I am thinking if it is the problem of node core.

@mccare
Copy link
Contributor Author

mccare commented Sep 30, 2021

Agreed, will try to reproduce it with a sample node server then. Filed a bug here nodejs/node#40263

@climba03003
Copy link
Member

Agreed, will try to reproduce it with a sample node server then. Filed a bug here nodejs/node#40263

@mccare I have check the code and document, it is intentional behavior.

nodejs/node#40263 (comment)
https://nodejs.org/api/http2.html#http2_headers_object

We may need a ugly fix on our side or deepmerge side.

const busboyOptions = deepmerge.all([{ headers: Object.assign({}, req.headers) }, options || {}, opts || {}])

in L226 and L318-L322

@mccare
Copy link
Contributor Author

mccare commented Sep 30, 2021

Thanks @climba03003 , I've implemented your suggestion and created a PR. Don't really know how to write a test for it though.

@mccare
Copy link
Contributor Author

mccare commented Oct 8, 2021

Fixed with release 5.0.2

@mccare mccare closed this as completed Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants