Comments (29)
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)
from qs.
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.
from qs.
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.
from qs.
Have a test now, you were right @defunctzombie great find!
from qs.
fixed in 2.4.1
from qs.
Thanks for the quick response!
@dougwilson ping to bump express qs usage
from qs.
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 :)
from qs.
The good news is it doesn't cause a crash in Express (phew), only an annoying 500 error.
from qs.
Yep :) caught it via our error logging
On Friday, March 13, 2015, Douglas Christopher Wilson <
[email protected]> 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).
from qs.
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.
from qs.
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
.
from qs.
@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.
from qs.
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")'
{}
from qs.
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 <
[email protected]> 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).
from qs.
@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.
from qs.
Hmmm.. I see the problem. I'll rethink this and see if I can come up with a better solution first thing tomorrow morning
from qs.
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 <
[email protected]> 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).
from qs.
Yes, @defunctzombie I'm slapping out across-the-board try
-catch
patches right now :(
from qs.
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.
from qs.
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 <
[email protected]> 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-aliveWhich means there is nothing critical to take care of right now.
—
Reply to this email directly or view it on GitHub
#73 (comment).
from qs.
@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)
})
from qs.
@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".
from qs.
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 <
[email protected]> 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).
from qs.
@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?
from qs.
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.
from qs.
I tend to agree. I'll look into shuffling things a bit.
from qs.
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.
from qs.
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 [email protected]
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).
from qs.
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 :)
from qs.
Related Issues (20)
- Keeping nesting with custom decoder ? HOT 1
- qs.parse decoder option doesn't work on IOS web view environment HOT 1
- Unable to parse back a stringified object when too nested HOT 3
- `stringify` with `arrayFormat: 'comma'` does not encode commas in strings. HOT 13
- [spam]
- ESM & CJS - Built HOT 9
- Stringify null values HOT 9
- None. HOT 1
- Serialization for `Date` is not working when using `filter` option. HOT 3
- Issue using qs while using Express@5 HOT 7
- qs.stringify(json) and qs.parse(json) Results are inconsistent with expectations HOT 4
- Parsing the stringify result would lead to a different object HOT 1
- qs parse is not letting app load on production when imported from index.tsx HOT 1
- Feature request: export another endpoint that doesn't depend on function-bind and possibly other polyfills HOT 10
- Does not work in NextJS edge middleware HOT 3
- Stringify method with format: "RFC1738" does not encode parentheses -> ( ) HOT 3
- I'm appreciate with this library, however my new project use python. Are there any same packages in Python? HOT 1
- How can you keep the square brackets when using arrayFormat: comma HOT 4
- Add required key and type checking HOT 2
- qs.parse return different while the query is in different position HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.