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

Should jsonify() throw an exception when provided a list? #1177

Closed
jgehrcke opened this issue Sep 16, 2014 · 8 comments
Closed

Should jsonify() throw an exception when provided a list? #1177

jgehrcke opened this issue Sep 16, 2014 · 8 comments

Comments

@jgehrcke
Copy link

Consider jsonify()cation of a dictionary contained in a list:

print flask.__version__
print repr(flask.jsonify([{"a": 1, "b": 2}]).data)

With the following output:

0.10.1
'{\n  "a": "b"\n}'

jsonify() has -- without complaining -- mangled the data into something really not useful: The original dictionary values are gone, and the "b" key suddenly is a value.

I appreciate that top-level array elements are not allowed to be created in Flask's jsonify() implementation (http://flask.pocoo.org/docs/0.10/security/#json-security). Also, there are for sure straight-forward strategies for solving this in application code, for example by wrapping the list in a dictionary: jsonify({"items": collection}).

However, in the Python world we do not find it very often that an entity accepts invalid input and produces garbled output without complaining. Also, it looks like jsonify() once raised an exception when provided a list (at least in 2011 it did raise a TypeError: #170).

Should we raise an exception again or is the current behavior carefully designed with a focus on performance? I mean, it is unlikely that this data mangling ends up in a production environment (a developer really should realize that jsonify() produces garbage in this case , like I did).

@danielchatfield
Copy link

#673

@danielchatfield
Copy link

As I stated in above issue I'm all for removing this restriction - the browsers affected by the security bug are now really really old (IE5, firefox 2 etc.) and all have unfixed remote code execution bugs anyway.

@hockeybuggy
Copy link

Using flask 0.10.1 I was able to raise the same TypeError as #170. So it still has the same behaveior as 2011.

TypeError: cannot convert dictionary update sequence element #0 to a sequence

To add garbleing behavior of jsonify:

a1 = [{"a":1}]
a2 = [{"a":1, "b":2}]
a3 = [{"a":1, "b":2, "c":3}]

only a2 produces the garbled you describe when passed to jsonify (regardless of the key value). a1 and a3 throw the same TypeError as above.

In the context of allowing top-level lists to jsonify I currently have no opinion.

@duaneking
Copy link

If I submit a patch that removes this restriction and allows the developer to be productive by accepting lists, will it be accepted?

@untitaker
Copy link
Contributor

@duaneking sure

@methane methane mentioned this issue Oct 19, 2014
6 tasks
@untitaker
Copy link
Contributor

Actually, i need to expand on this and paddle back: as mentioned in #673, any patches for improving the error message are always welcomed and will be merged. I think we agreed in that issue that we should remove the restriction, however, since @mitsuhiko introduced that restriction, i think you should negotiate this with him if this is going to land in 1.0

@untitaker
Copy link
Contributor

Hah, actually this seems to be a duplicate of #248.

@untitaker
Copy link
Contributor

Closing this in favor of #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

5 participants