Handle a single unusual argument in jsonify #1209
Conversation
If you're using the JSON Encoder in Overholt, you may notice that passing one of your models to jsonify does not work because "object is not iterable", even though your custom json encoder will turn it into an iterable in a moment. This is because Flask is trying to make it into a dict before that can happen. This fixes that case.
| @@ -234,7 +234,13 @@ def get_current_user(): | |||
| if current_app.config['JSONIFY_PRETTYPRINT_REGULAR'] \ | |||
| and not request.is_xhr: | |||
| indent = 2 | |||
| return current_app.response_class(dumps(dict(*args, **kwargs), | |||
|
|
|||
| if len(args) == 1 and len(kwargs) == 0: | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
This is actually intended, see http://flask.pocoo.org/docs/0.10/security/#json-security. Your particular case would be pallets-eco/flask-sqlalchemy#229. However, we're considering removing that restriction, see #248, so don't close this yet. |
|
@untitaker I have read that page. While my patch would allow you to serialize arrays, that's not the point of my patch. In my case, I am returning an object that my JSON encoder is going to turn into a dict. A much better way to prevent you from serializing arrays would be to check after serialization if the first character is an open bracket ([). There is no point in breaking other legitimate uses of jsonify just to prevent that. |
|
I see. Would you be willing to maintain the PR regarding #248 though? |
|
@untitaker What do you mean? |
|
Nevermind. I'll leave this PR open, because your particular issue is different from the linked one. The current patch is unlikely to land in its current form, but i expect that we will fix both issues at once. |
This makes the JSON security check more reliable and adds a configuration option that allows you to disable it if you aren't targeting old browsers.
|
I updated it to improve the check for arrays, and added a configuration option. I'd like to run flask's test suite on this, but when I run py.test it says |
|
I think the new system requires you to install flask in developer mode: |
|
I installed it with |
I don't think this is the way to go. I actually think we should completely remove all checks, and also clarify in the docs that this portion is basically obsolete. @DasIch also suggested to specify a list of officially supported browsers. Remember, this is 1.0, we can break compat all the way we want. If the user still wants to support Internet Explorer 5, they can just avoid serializing arrays.
It seems to run fine for me without that.
I don't see that message. Travis can be glitchy sometimes in that regard. |
| serialized = dumps(data, indent=indent) | ||
|
|
||
| if current_app.config['JSONIFY_ARRAY_SECURITY'] and serialized[0] == '[': | ||
| raise ValueError(ARRAY_SECURITY_MESSAGE) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
My understanding is that the previous behavior (always converting the args to a dict) was sufficient to always prevent a top level json list, but blocked the legitimate use case of jsonify'ing an object for which one has written a custom JSONEncoder. This PR seems to enable that ability, while still protecting against top level json lists. Whether json lists are considered a vulnerability Flask should handle, whether a config value allows top level lists to be enabled/disabled, and which browsers have been patched or not all seem to be concerns that can be handled in a separate PR(s). The ability to disable array security is not needed to solve the problem. If this PR did not attempt to fix the jsonify(object) issue and introduce a JSONIFY_ARRAY_SECURITY config flag (contentious) I imagine it could be merged without much debate? |
I actually think that this is a definitive bug that can be fixed as done in this PR without much discussion.
Actually this is the controversial part for me: I think we should completely remove any security checks (as argued in #248) and extensively document why the user might want to take manual precautions against this problem. |
|
No longer merges cleanly. |
|
Yes, that too. I'll close this for now. |
|
I'm sad to see this hasn't merged, though I appreciate the responsibly cautious approach in #248 |
I ran into this issue today, googled it, and found someone else having the same issue.
If you're using the JSON Encoder in Overholt, you may notice that passing one of your models to jsonify does not work. It says "object is not iterable", even though your custom json encoder will turn it into an iterable in a moment. This is because Flask is trying to make it into a dict before that can happen.
This patch fixes this case. Also it allows you to return other legitimate singular json values such as integers and strings. Hopefully this doesn't raise any security issues. If it does, someone else may wish to handle this more carefully.