Skip to content

make returning plain objects and allowing prototype overwriting properties optional#98

Merged
nlf merged 3 commits intomasterfrom
optional_prototypes
Jul 2, 2015
Merged

make returning plain objects and allowing prototype overwriting properties optional#98
nlf merged 3 commits intomasterfrom
optional_prototypes

Conversation

@nlf
Copy link
Copy Markdown
Collaborator

@nlf nlf commented Jul 1, 2015

this is a short term solution before 5.0.0 which will change everything. Essentially the default behavior will match the 2.x releases, with the difference that you may set the plainObjects option to true to get the behavior from the 3.x releases.

If you don't want plain objects but also don't want to lose keys that would overwrite prototype properties, you can set the prefixPrototypes option to true and those keys will be prefixed with an underscore '_' and passed along. It's not perfect, but at least the keys aren't silently ignored.

Would appreciate your feedback on this one @hueniverse @dougwilson

@nlf nlf self-assigned this Jul 1, 2015
@nlf nlf added this to the 4.0.0 milestone Jul 1, 2015
@dougwilson
Copy link
Copy Markdown
Contributor

To me, this seems to make the situation potentially worse, especially for user-confusion, because of the following not being the same:

$ node -pe 'JSON.parse('"'"'{"hasOwnProperty":"yes"}'"'"')'
{ hasOwnProperty: 'yes' }

$ node -pe 'require("querystring").parse("hasOwnProperty=yes")'
{ hasOwnProperty: 'yes' }

$ node -pe 'require("qs").parse("hasOwnProperty=yes")'
{}

Can there be an option to use the default prototype and just overwrite properties, even if it's something you have to opt into?

@dougwilson
Copy link
Copy Markdown
Contributor

lol, please ignore my above comment, as it was technically only based off reading the source code. I see there is prefixPrototypes, my bad!

@nlf
Copy link
Copy Markdown
Collaborator Author

nlf commented Jul 1, 2015

prefixPrototypes is what allows things that would overwrite a prototype property at all, by prefixing them with a string. if it's set to false then prototypes would be ignored like they were in 2.x. I can see your point about confusion though, so I can get rid of that option and add an allowPrototypes instead that will let people shoot themselves in the foot if they so wish.

@dougwilson
Copy link
Copy Markdown
Contributor

So I had made that comment in haste :) Let me really look through it to see what real comments I have :)

@hueniverse
Copy link
Copy Markdown
Contributor

Looks fine to me. Can't make everyone happy and I really don't care about prototype names in payload. This is JS. You are fucked anyway you look at it.

@dougwilson
Copy link
Copy Markdown
Contributor

Ok, so this PR seems fine in general. My only real comment is that I don't think the global environment should affect the functionality of the library:

$ node -pe 'require("qs").parse("hide=1")'
{ hide: '1' }

$ node -pe 'Object.defineProperty(Object.prototype,"hide",{value:function(){},writable:true});require("qs").parse("hide=1")'
{}

JSON.parse and the built-in query string does not have this issue, though:

$ node -pe 'Object.defineProperty(Object.prototype,"hide",{value:function(){},writable:true});JSON.parse('"'"'{"hide":"1"}'"'"')'
{ hide: '1' }

$ node -pe 'Object.defineProperty(Object.prototype,"hide",{value:function(){},writable:true});require("querystring").parse("hide=1")'
{ hide: '1' }

@nlf
Copy link
Copy Markdown
Collaborator Author

nlf commented Jul 1, 2015

That would be resolved by exchanging prefixPrototypes with allowPrototypes, the latter being an option to let you overwrite properties on Object.prototype.

@nlf
Copy link
Copy Markdown
Collaborator Author

nlf commented Jul 2, 2015

Ok, changed this around so instead of optionally adding a prefix to properties that would overwrite the object prototype it instead optionally lets you shoot yourself in the foot, just like JSON.parse. It's off by default, but it's there.

@nlf nlf changed the title make returning plain objects and prefixing prototype overwriting properties optional make returning plain objects and allowing prototype overwriting properties optional Jul 2, 2015
nlf added a commit that referenced this pull request Jul 2, 2015
make returning plain objects and allowing prototype overwriting properties optional
@nlf nlf merged commit bc803d3 into master Jul 2, 2015
@nlf nlf deleted the optional_prototypes branch July 2, 2015 18:33
@AdriVanHoudt
Copy link
Copy Markdown

This is only breaking when using { prototype: false } right?

@nlf
Copy link
Copy Markdown
Collaborator Author

nlf commented Jul 2, 2015

this reverts the breaking change made in 3.x, so it's in itself another breaking change, though odds are if you don't use any of the prototype stuff on parsed results you'll never notice.

@AdriVanHoudt
Copy link
Copy Markdown

ok, thanks for the explanation

Comment thread README.md
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be worth to explicitly note this is a security risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants