Property 'hasOwnProperty' of object #<Object> is not a function

[defunctzombie]

https://github.com/hapijs/qs/blob/master/lib/parse.js#L32

When the object is created via Object.create(null) it does not have object properties. Instead, the following should be used:

Object.prototype.hasOwnProperty.call(obj, key)

[nlf]

In the context of this function, obj is created here https://github.com/hapijs/qs/blob/master/lib/parse.js#L18 so we know for certain it was not created via Object.create(null)

[defunctzombie]

Not if someone is trying to compromise your server via bad urls :) I will try to dig up the request that causes failure, but I will say that this should be fixed and is indeed broken ;)

I am using an up to date version of express 4.12 but obviously would be more idea if I reproduced against this module directly which I will try to do if you want proof.

[nlf]

If you can prove it with a test case I'll definitely fix it, but seeing as this method is what is creating the obj variable in a known fashion, I fail to see how it's possible that it could break in this particular line.

[nlf]

Have a test now, you were right @defunctzombie great find!

[nlf]

fixed in 2.4.1

[defunctzombie]

Thanks for the quick response!

@dougwilson ping to bump express qs usage

[dougwilson]

Thanks for the ping :) I was already planning to roll a debug bump tonight, so this just made it and I'll be pretty quick :D I appreciate the heads-up :)

[dougwilson]

The good news is it doesn't cause a crash in Express (phew), only an annoying 500 error.

[defunctzombie]

Yep :) caught it via our error logging

On Friday, March 13, 2015, Douglas Christopher Wilson <
notifications@github.com> wrote:

The good news is it doesn't cause a crash in Express (phew), only an
annoying 500 error.

—
Reply to this email directly or view it on GitHub
#73 (comment).

[dougwilson]

So apparently this is not a bug fix, but a complete behavior change. It basically can be described as "Ignore keys that override Object.prototype properties" and ignores anything attached to Object.prototype and not just hasOwnProperty. This means I cannot actually include this in Express until the next minor, not a patch.

[dougwilson]

It basically means that any random module you loaded in your code base can now affect your qs parse, even by accident. I jut noticed this because some module was doing Object.defineProperty(Object.prototype, find, {...}) and with this release, no longer can you have a query string parameter named find.

[dougwilson]

@defunctzombie let me know how you feel about this finding, but to me, this change is way too impactful to ever make it into Express 3/4 since this module is the default query string parser and it's basically asking people to restructure their entire application if they want to use certain query string parameter names + the enhanced syntax here.

Express currently handles any errors from this module (like the one you posted) in a way that does not impact the application, simply generating a 400 Bad Request error on requests that have a hasOwnProperty key.

[dougwilson]

At the most basic level, though, I feel like this library is very inconsistent, now:

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

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

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

[defunctzombie]

I am not sure what the exact fix should be here but I can say that it
caused a crash of the app server and not a 400 request because of the
hasOwnProperty function call when the function does not exist.

On Sunday, March 15, 2015, Douglas Christopher Wilson <
notifications@github.com> wrote:

At the most basic level, though, I feel like this library is very
inconsistent, now:

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

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

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

—
Reply to this email directly or view it on GitHub
#73 (comment).

[dougwilson]

@defunctzombie oh, I didn't realize, I though you were saying it was just an annoyance, otherwise I would have released a new Express days ago :( I'm going to add a try-catch to https://github.com/strongloop/express/blob/master/lib/middleware/query.js for now. Hopefully the issues I outlined above can be addressed by this module, otherwise I may need to seek an alternative for Express.

[nlf]

Hmmm.. I see the problem. I'll rethink this and see if I can come up with a better solution first thing tomorrow morning

[defunctzombie]

A try/catch would be good then maybe we can think about what the fix here
should be.

On one hand using reserved object properties as names isn't good but on the
other the object here should be treated like a hash map and not a
traditional js object so that any name could be used in the query string.

Maintainers, thoughts?

On Sunday, March 15, 2015, Douglas Christopher Wilson <
notifications@github.com> wrote:

@defunctzombie https://github.com/defunctzombie oh, I didn't realize, I
though you were saying it was just an annoyance, otherwise I would have
released a new Express days ago :( I'm going to add a try-catch to
https://github.com/strongloop/express/blob/master/lib/middleware/query.js
for now. Hopefully the issues I outlined above can be addressed by this
module, otherwise I may need to seek an alternative for Express.

—
Reply to this email directly or view it on GitHub
#73 (comment).

[dougwilson]

Yes, @defunctzombie I'm slapping out across-the-board try-catch patches right now :(

[dougwilson]

Wait a minute, lol, the Express server doesn't crash at all; it just sends back a 500 response:

$ curl -I http://127.0.0.1:3000/?hasOwnProperty=yes\&foo=bar
HTTP/1.1 500 Internal Server Error
X-Content-Type-Options: nosniff
Content-Type: text/html; charset=utf-8
Content-Length: 1298
Date: Mon, 16 Mar 2015 06:06:49 GMT
Connection: keep-alive

Which means there is nothing critical to take care of right now (at 2am Monday morning). Basically it's just an annoyance right now and not a DoS vector, say.

[defunctzombie]

Sorry, I did indeed mean to say it sends a 500. We log all 500s as app
level errors and thus the 500 was an annoyance and not a crash. Apologies
for not making that clear.

500 is still not ideal and we should think about the fix because a 500
usually means a dev needs to go see what is going on with the server.

On Sunday, March 15, 2015, Douglas Christopher Wilson <
notifications@github.com> wrote:

Wait a minute, lol, the Express server doesn't crash at all; it just sends
back a 500 response:

$ curl -I http://127.0.0.1:3000/?hasOwnProperty=yes\&foo=bar
HTTP/1.1 http://127.0.0.1:3000/?hasOwnProperty=yes%5C&foo=barHTTP/1.1 500 Internal Server Error
X-Content-Type-Options: nosniff
Content-Type: text/html; charset=utf-8
Content-Length: 1298
Date: Mon, 16 Mar 2015 06:06:49 GMT
Connection: keep-alive

Which means there is nothing critical to take care of right now.

—
Reply to this email directly or view it on GitHub
#73 (comment).

[dougwilson]

@defunctzombie I agree we need to take care of it. I was just working on pushing this stuff out, ran into the weird changes in the qs patch in the process and didn't want to risk it for the time being with the qs patch. Something like a DoS vector is critical and I would stay to all hours of the night to fix as soon as soon as I was made aware of such a thing, which is why I "panicked" :) Since this is just a 500, I'm going to leave this for tomorrow morning, and drop this work-around if you wanted to not hear from that error (if your logging system don't have a feature to suppress already known errors):

app.use(function (err, req, res, next) {
  if (err.name === 'TypeError'
    && typeof err.status !== 'number'
    && String(err.message).indexOf("'hasOwnProperty'") !== -1) {
    err.status = 400
  }
  next(err)
})

[dougwilson]

@defunctzombie while we wait for this issue to be resolved, I think I'm actually going to change the wrapper not to return a 400, but rather an empty object {}, as if there were no query string parameters? Right now this whole situation is a bit tricky. When we parse a query string, we should always be successful, because there is no input you can provide that is bad, really. Returning a 400 says "hey client, you made a mistake in your request" while the current behavior of 500 is "hey client, we f-ed up, because we failed to do something that should never fail".

[defunctzombie]

In the current case tho I think a 400 is correct, no? The client specified
a query string which the server seems to be "bad" because it messes with
some expectations we have outlined.

Failing silently will lead to users possibly complaining to app authors
that things are not working as expected.

On Monday, March 16, 2015, Douglas Christopher Wilson <
notifications@github.com> wrote:

@defunctzombie https://github.com/defunctzombie while we wait for this
issue to be resolved, I think I'm actually going to change the wrapper not
to return a 400, but rather an empty object {}, as if there were no query
string parameters? Right now this whole situation is a bit tricky. When we
parse a query string, we should always be successful, because there is
no input you can provide that is bad, really. Returning a 400 says "hey
client, you made a mistake in your request" while the current behavior of
500 is "hey client, we f-ed up, because we failed to do something that
should never fail".

—
Reply to this email directly or view it on GitHub
#73 (comment).

[nlf]

@dougwilson @defunctzombie so I'm trying to think through this.. I definitely feel like it's best to continue refusing to overwrite keys that belong to the object's prototype, since this is a security concern. After all, the reason you were seeing errors at all is because someone was attempting to write to the hasOwnProperty key, which means that if the client later tries to use hasOwnProperty as per the original prototype (as a method), they'll receive an error since it's now a string.

Yes, it does mean that users who are tainting Object.prototype will in turn not be able to use those same keys in their query strings, but for safety's sake, I feel like this is the most appropriate thing to do.

Thoughts?

[dougwilson]

In the end, this is why the return should be of Object.create(null), because qs.parse should be returning a "bag of keys" and server code should not be calling methods on anything in it (and Object.create(null) makes is extremely apparent that people should not be, since they won't even be able to). But even then, JSON.parse does not have any problem doing this and neither does querystring module.

[nlf]

I tend to agree. I'll look into shuffling things a bit.

[nlf]

Ok, so even with the change I made a bit ago you still effectively get keys that match prototype methods stripped due to additional checks.

What I'm going to do is shuffle things so that this module only deals with objects created via Object.create(null) so that we don't have to worry about overwriting prototypes. Then we can just document that the passed objects will not have prototypes associated with them.

This is definitely a major change though, and I have a couple other things that I want to land in 3.0.0, so give me a day or two to get that done.

[defunctzombie]

Awesome stuff! Great discussion and I think the end result will be a
cleaner interface and expectation of functionality.

On Monday, March 16, 2015, Nathan LaFreniere notifications@github.com
wrote:

Ok, so even with the change I made a bit ago you still effectively get
keys that match prototype methods stripped due to additional checks.

What I'm going to do is shuffle things so that this module only deals with
objects created via Object.create(null) so that we don't have to worry
about overwriting prototypes. Then we can just document that the passed
objects will not have prototypes associated with them.

This is definitely a major change though, and I have a couple other things
that I want to land in 3.0.0, so give me a day or two to get that done.

—
Reply to this email directly or view it on GitHub
#73 (comment).

[dougwilson]

In the meantime, I'm going to use whatever version of qs is published at the end of tonight in some Express/Connect updates and then change over later :)