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

Patch fixes prototype pollution issue #17 #19

Closed
wants to merge 5 commits into from

Conversation

tomharrisonjr
Copy link

Solution as proposed by Johns Hopkins researchers.

If querystring key contains strings that can cause prototype pollution in the browser, e.g. __proto__, transform to uppercase.

fixes #17

Copy link
Contributor

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch! This is definitely worth fixing, but I don't think we've gone about it the right way here.

With this code change, we'd potentially be returning incorrect data (see comment left inline). Instead, we should either error when given a restricted keyword or return undefined.

To avoid breaking existing apps as much as possible, IMO the best solution is to return undefined.

Example:

var restricted = ['__proto__', 'prototype', 'constructor'];
var isRestricted = function(key){
  for (var i = 0; i < restricted.length; i++) {
    if (key == restricted[i]) {
      return true
    }
  }
  return false
}

exports.parse = function(str){
  if ('string' != typeof str) return {};

  str = trim(str);
  if ('' == str) return {};
  if ('?' == str.charAt(0)) str = str.slice(1);

  var obj = {};
  var pairs = str.split('&');
  for (var i = 0; i < pairs.length; i++) {
    var parts = pairs[i].split('=');
    var key = decode(parts[0]);
    var m;

    if (isRestricted(key)) {
      continue
    }

    if (m = pattern.exec(key)) {
      obj[m[1]] = obj[m[1]] || [];
      obj[m[1]][m[2]] = decode(parts[1]);
      continue;
    }

    obj[parts[0]] = null == parts[1]
      ? ''
      : decode(parts[1]);
  }

  return obj;
};

*
* @param {String} str
* @return {String}
* @api public
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not part of the public API.

Suggested change
* @api public
* @api private


var sanitizeObjKey = function(str) {
if (str && ["__proto__", "constructor", "prototype"].indexOf(str.toLowerCase()) > -1) {
return str.toUpperCase()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why convert to uppercase? If the user (for some weird reason) wanted the value of prototype, they'd get PROTOTYPE instead, which would either return unexpected (incorrect) results or undefined. Consider the following:

var q = querystring.parse('prototype=hi&PROTOTYPE=bye')
console.log(q.prototype)
// => bye
console.log(q.PROTOTYPE)
// => bye

IMO we should either error or return undefined.

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.

Prototype Pollution
3 participants