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

maintain object prototype chain for req.body #318

Closed

Conversation

danielkrainas
Copy link

This changes req.body to include Object in the prototype.

The req.body object provided by expressjs has Object in the prototype chain but multer creates an object with no prototype. For code that expects hasOwnProperty or another Object member, I need to manually set the prototype myself or create a shallow copy to something with Object in its chain.

@LinusU
Copy link
Member

LinusU commented Feb 21, 2016

First of all, great commit message!

This was however done with purpose so I think that we need some discussion before merging this. The problem is that the client data should never be trusted, and thus we can't rely on properties existing up the prototype chain.

Consider what would happen if the client sent hasOwnProperty=nope or hasOwnProperty[call]=. This would break any script trying to use req.body.hasOwnProperty(_), and having client data break server code is never good.

To be honest, I would be most happy if we could use a Map to store the data, but I don't think that people would like there more complicated syntax (req.body.get('test') instead of req.body.test).

This exact change have actually been proposed before, and I think that my answer then is still relevant: #187

@danielkrainas
Copy link
Author

While I can respect the cautiousness taken in the design, I believe this is one time where it's better to be consistent as a community, regardless of whether that is right or wrong. Currently, all of bodyparser's parsers spit out an Object object:

This creates an expectation at the handler level, regardless of right or wrong, that then gets thrown out when multer is added to the pipeline. What's more is that this is not made clear and a reasonable person wouldn't expect a module like multer to assert an opinion like that, unrelated to its function and purpose.

@LinusU
Copy link
Member

LinusU commented Aug 14, 2016

We feel that keeping it with a null prototype is the way to go forward, since the alternative either introduces potential bugs that can be exploited by a client, or have to strip some keys which can be very confusing and hard to work around if it affects you.

If you have anything new to add to the discussion, please feel free to post a comment in the discussions issue: expressjs/discussions#25

Also, as someone pointed out in that issue, urlencoded returns a prototype-less object in Node.js 6.x.

Thank you for your time

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

Successfully merging this pull request may close these issues.

None yet

2 participants