Multipart flatten includes properties in prototype

Open
#438 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
45/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Stale
Tech stack
javascript
Domain
api, backend

Research direction

Search the multipart implementation for the referenced flatten function and inspect how it builds the multipart body. Add a regression test showing that inherited properties are excluded, then run the existing multipart tests to confirm that own properties and supported file or buffer values still work.

Written by the indexing model from the issue text.

Description

The multipart library uses the flatten function, that i've included here for reference:

// flattens nested objects for multipart body
function flatten(object, into, prefix) {
  into = into || {};

  for(var key in object) {
    var prefix_key = prefix ? prefix + '[' + key + ']' : key;
    var prop = object[key];

    if (prop && typeof prop === 'object' && !(prop.buffer || prop.file || prop.content_type))
      flatten(prop, into, prefix_key)
    else
      into[prefix_key] = prop;
  }

  return into;
}

The function uses a for ... in loop to iterate through the properties to add them into the into object. However, a for ... in loop iterates through object prototypes as well. This is not normally a bad thing unless users have defined something in the object prototype that could cause some troubles:

var a = {};
a.__proto__.filename = "/etc/passwd"
// Below code is taken from one of the examples on needle's npm page
var data = {
  buffer: '/home/johnlennon/walrus.png',
  content_type: 'image/png'
};

// the callback is optional, and needle returns a `readableStream` object
// that triggers a 'done' event when the request/response process is complete.
needle
  .post('https://my.server.com/foo', data, { multipart: true })
  .on('readable', function() { /* eat your chunks */ })
  .on('done', function(err) {
    console.log('Ready-o!');
  })

The resultant into object will look like so:
Screenshot 2024-09-09 at 1 51 38 PM

This normally wouldn't be a problem if a user were to stipulate all their properties in their data object. Prototypal lookups will stop if they see that a property is defined at the object level, and will not go up the prototype chain further. However, in cases where those properties are not included when multipart is set to true, could lead to some issues. If a user were to include this library and their application using your library had a vulnerability known as prototype pollution, then a bad user could inject arbitrary properties and wreak havoc. (to be clear, proto pollution is not something inherent to your library, but is posited as a theoretical situation where properties hiding in the object prototype could be found if another user used your library in an application with prototype pollution).

A way to solve this would be to swap out the for ... in loop with Object.entries(), as Object.entries doesn't go up the prototype chain. Therefore, any properties hiding in the prototype will be ignored. I can submit a PR to suggest this change if ignoring prototype-based properties is ideal behaviour.

Dominant language
JavaScript
Stars
1.6k
Forks
237
PR merge metrics
No merged PRs in 30d

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from tomas/needle

All issues in tomas/needle

Similar issues

More JavaScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.