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

Can't jsonify list object #673

Closed
CuriousDima opened this issue Feb 1, 2013 · 25 comments
Closed

Can't jsonify list object #673

CuriousDima opened this issue Feb 1, 2013 · 25 comments

Comments

@CuriousDima
Copy link

JSON standard (http://json.org/) say, that any entity can be object or array.
When I try:

jsonify(['a','b','c', 'd', 'e', 'f'])

I got an error:

ValueError: dictionary update sequence element #0 has length 8; 2 is required

I think it is not a correct answer from flask. It should returns a JSON-object with array.

@sigmavirus24
Copy link

This is a duplicate of #170. It's for security reasons as @mitsuhiko mentions. This is also documented. You can, however, make your own request, set the proper content-type header and use the json library to do this yourself if you're intent on by-passing it.

You could also do jsonify({'objects': ['a', 'b', 'c',]}) if you wanted to.

@apiguy
Copy link

apiguy commented Feb 4, 2013

you can also use the syntax jsonify(results=['a', 'b', 'c'])

@apiguy
Copy link

apiguy commented Feb 4, 2013

Also if you'd like to read more about the security issues with returning Arrays in a json response: http://flask.pocoo.org/docs/security/

@CuriousDima
Copy link
Author

Thank you, guys.

@Blewin
Copy link

Blewin commented Jul 17, 2014

If its intended, why isn't the error less cryptic?

@danielchatfield
Copy link

👍 on a more helpful error message

@sigmavirus24
Copy link

All pull requests are considered. The line you're most likely looking for is in json.py

@danielchatfield
Copy link

Also I would support the removal of this restriction, the attack vector was closed in ECMAScript 5 and doesn't affect any current browser.

@sigmavirus24
Copy link

@danielchatfield define "current". If you mean "most recently released version", then sure it's safe. If instead you mean "most widely used version" then you're likely wrong. I would expect this to affect a large number of IE users.

@danielchatfield
Copy link

Latest version of IE that is vulnerable is IE 5.

@danielchatfield
Copy link

Latest version of firefox that is vulnerable is 2.0.

@danielchatfield
Copy link

@DasIch
Copy link
Contributor

DasIch commented Jul 17, 2014

Given that it appears to be solved by browsers now it seems reasonable to allow lists now.

I still feel somewhat uncomfortable with the idea though, considering it's a security issue.

How are other frameworks handling this?

@sigmavirus24
Copy link

It's also important to point out that this is easily worked around if you like to ignore the best practices of the framework you chose.

@DasIch
Copy link
Contributor

DasIch commented Jul 17, 2014

While that may be possible, if you have to defend the API with "it can be worked around" you have unquestionably failed at its design.

We should strive to be ahead and not behind when it comes to defining "the right way to do it".

I'm not sure if this was your intention but your comment moved me more in the let's allow lists position.

If we do go ahead with this, we may want to consider officially defining which browsers we support. Having a clear definition would help in finding a solution for this issue and others like it, which will inevitably be raised.

@sigmavirus24
Copy link

I'm not sure if this was your intention but your comment moved me more in the let's allow lists position.

We'll never know ;)

@untitaker
Copy link
Contributor

I'm for allowing lists. Maybe we could mostly eliminate the security concerns by disallowing lists based on UA sniffing. It'd cause errors in production, but at least no security issues.

@danielchatfield
Copy link

I'm not in support of UA sniffing.

Lets be realistic about this, I don't know how many people still use IE 5
but we are talking minuscule amounts of people here. This in itself makes
the attack not really feasible as it requires a user visiting an "evil
site" which is targeting a specific website that the victim has to be
logged into and using IE 5. An attacker would be waiting a pretty long time
(indefinitely) for someone like that to come along.

In any case, IE 5 has unfixed Remote code execution bugs so this really
doesn't matter.

On 17 July 2014 20:14, Markus Unterwaditzer notifications@github.com
wrote:

I'm for allowing lists. Maybe we could mostly eliminate the security
concerns by disallowing lists based on UA sniffing. It'd cause errors in
production, but at least no security issues.

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

For contact info visit: www.danielchatfield.com

@untitaker
Copy link
Contributor

To clarify, i was not supporting the idea of UA sniffing, only suggesting it as a possible part of a compromise.

@danielchatfield i agree that IE5 is broken anyway, there are other browsers affected by this particular issue though.

@danielchatfield
Copy link

All affected browsers are equally as old/insignificant/broken.

@duaneking
Copy link

This is clearly a browser side defect, not something that should limit the features of the Flask Library itself. And lets face it, not every client of a flask based API will be a browser so the argument that its a browser issue and we must protect people is invalid, because at that point there is no browser to protect people from.

@untitaker
Copy link
Contributor

I think we already agreed that this should be removed, but i don't think the argument that this is a security issue somewhere else is a good one.

@duaneking
Copy link

The argument that its a security issue is actually nothing but Security Theater; nobody should care about internet explorer 5.5 anymore.

pallets-eco/flask-sqlalchemy#229

@untitaker
Copy link
Contributor

Yes we already had that argument. I think it's safe to remove that restriction for 1.0, but i'd like to get some kind of confirmation from @mitsuhiko. In any case PRs for a better error message are always welcome!

@untitaker
Copy link
Contributor

See #248, please continue discussion there.

@pallets pallets locked and limited conversation to collaborators Oct 19, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants