Check the content type before returning the json value. #2177

fduxiao · 2017-08-08T02:59:07Z

What do these changes do?

Check the content type before returning the json value.

Are there changes in behavior for the user?

When getting json requests one could directly await request.json().
If one'd like to ignore the Content-Type, he/she should specify if_check manually i.e. await request.json(if_check=False).

Related issue number

referencing #2174

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is Xiao Tan.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the changes folder
- name it 2174.bugfix for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

codecov-io · 2017-08-08T04:23:34Z

Codecov Report

Merging #2177 into master will decrease coverage by 0.04%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #2177      +/-   ##
==========================================
- Coverage    97.2%   97.15%   -0.05%     
==========================================
  Files          39       39              
  Lines        7945     7956      +11     
  Branches     1378     1382       +4     
==========================================
+ Hits         7723     7730       +7     
- Misses         98      100       +2     
- Partials      124      126       +2

Impacted Files	Coverage Δ
aiohttp/web_request.py	`98.46% <71.42%> (-1.22%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4c1237...99068d8. Read the comment docs.

kxepal · 2017-08-08T06:34:54Z

aiohttp/web_request.py

+        You can also handle it manually by passing the
+        `wrong_format` parameter.
+        """
+        if if_check and self.content_type != 'application/json':


How would I use, say, jsonapi if it will never match this hardcoded content-type?

Reasonable. I will add it.

kxepal · 2017-08-08T06:37:03Z

aiohttp/web_request.py

@@ -510,8 +510,19 @@ def text(self):
        return bytes_body.decode(encoding)

    @asyncio.coroutine
-    def json(self, *, loads=json.loads):
-        """Return BODY as JSON."""
+    def json(self, *, loads=json.loads, wrong_format=None, if_check=True):


I think signature is quite overloaded. I suggest just to allow to pass what exact content-type should be considered as valid. No match - throw an exception, nothing more. For BC, default value could be None until some release when we change it to 'application/json'. Thoughts?

kxepal · 2017-08-08T07:22:24Z

aiohttp/web_request.py

+        wrong_format_flag = False
+        if isinstance(allowed_types, str):
+            wrong_format_flag = allowed_types != self.content_type
+        elif isinstance(allowed_types, list):


What if allowed_types be a set or tuple or frosenset or whatever container?

So will iterable make sense? I mean how about hasattr(alowed_types, '__iter__')?

May be do like it was don't in ClientResponse.json() ? It's quite abstract from what kind of type was passed and the behaviour would be consistent between client and server.

kxepal · 2017-08-08T07:23:06Z

aiohttp/web_request.py

-        if if_check and self.content_type != 'application/json':
-            if wrong_format is None:
+        wrong_format_flag = False
+        if isinstance(allowed_types, str):


What if I receive application/json in legacy form when it will have a charset option defined explicitly like application/json;charset=utf-8?

How about just checking the type, subtype and suffix? And should json be decoded according to the charset?

I suggest just supporting a container (list, tuple, whatever) only and raising explicit TypeError on single string.
By default it should be None for backward compatibility. That's sad but we cannot change it.
Application might have a parameter for enabling checking json types in the application by default though.

How about just checking the type, subtype and suffix?

That's correct, but will make things too much complicated.

And should json be decoded according to the charset?

In modern RFC's it's mandatory should be UTF-8. However, there are lot of legacy around which forces charset via params. I don't thing we should care about non-UTF8 JSON, but if client asks about it explicitly, there is no reason to deny him, imho.

I suggest just supporting a container (list, tuple, whatever) only and raising explicit TypeError on single string.

It's good idea, but it conflicts with how client's json() acts. I don't think we should have different behaviour of a similar things - that would annoy people all around.

So I think we can use either Container or str, and None is for wildcard (or should I use a '*' as in add_route, since allowed_types=None looks weird for wildcard); otherwise a TypeError is raised

I don't like global switches. You have argument and it's default. None disables the check. For awhile it will be default to preserve BC. After some release it becomes application/json in defaults and may be we could prohibit None usage, but that's another story.

Could I suggest a function(mapping a string to boolean) be passed as an argument to filter the Content-Type and str, None are also accepted and nothing more is supported?

I would recommend to avoid callback style. This is the road to make aiohttp acts weird depending on what functions user passed around. There is pretty simple rules to find out if MIME type is json-like. In the rest cases user could override that behavior by explicit string value.

So that means the user won't want to behavior weirdly? Then I just need to check whether the type is str or tuple or list or set and compare them directly, or that it is something that means json-like or wildcard. Also I need to derive the charset from Content-type and pass it to loads.

Why he should? .json() provides stable interface and guarantee about what it does. User may customize default expected content-type via string argument. Why there is need to do more?

If user need to do more complex content type check, it should make it at middleware level, not in .json() call.

kxepal · 2017-08-08T07:23:39Z

aiohttp/web_request.py

                raise HTTPNotAcceptable()
-            elif callable(wrong_format):
-                yield from wrong_format()
+            elif callable(err_handler):


You expect callable, but below err_handler is a coroutine actually.

I can only use module inspect to decide whether an object is a coroutine. Should I decide whether it is a coroutine or not or even something else(says a string) and make calling correspondingly. I think more details should be given.

I would suggest to not pass exc_handler at all. No argument - no problems (:

kxepal · 2017-08-08T07:24:33Z

aiohttp/web_request.py

-            elif callable(wrong_format):
-                yield from wrong_format()
+            elif callable(err_handler):
+                yield from err_handler()


Why to pass here err_handler after all? You can wrap req.json() call with try/except and do the rest on user code side.

I meant to have reduced the use of try/except so one could get rid of the fact that if you want to handle the error yourself, you need the try/except block i.e. one doesn't need to raise the error, catch it and handle it since catch an error is costly.

No need to worry about try/except usage. If you do it, you explicitly sign about that you're aware about that things here could go wrong. Some custom argument that does the same makes it not so oblivious.

As about speed, you shouldn't worry about catch performance. I bet, your app business logic will be million times slower than that.

kxepal · 2017-08-08T07:25:09Z

aiohttp/web_request.py

        then the request is rejected.
-        You can also handle it manually by passing the
-        `wrong_format` parameter.
+        You can also handle it manually by passing the `err_handler` parameter.


I think there is need to note what will happens if content-type won't match.

socketpair · 2017-08-08T12:05:28Z

aiohttp/web_request.py

+        """
+        wrong_format_flag = False
+        if isinstance(allowed_types, str):
+            wrong_format_flag = allowed_types != self.content_type


The self.content_type may be application/json; charset=UTF-8. Am I right ? So, use startswith or handle that in a different way.

Yes. So how do you think of these codes:

content_type = self.content_type (mime_type, subtype, suffix, parameters) = parse_mimetype(content_type) content_type = mime_type + '/' + subtype if suffix != '': content_type += '+' + suffix charset = parameters.get('charset', None)

seems OK, I want to review this part in context of whole path.

socketpair · 2017-08-08T12:05:40Z

aiohttp/web_request.py

+        wrong_format_flag = False
+        if isinstance(allowed_types, str):
+            wrong_format_flag = allowed_types != self.content_type
+        elif isinstance(allowed_types, list):


what about tuple and set ?

Maybe all Containers should be supported, says

if isinstance(allowed_types, str): wrong_format_flag = allowed_types != self.content_type elif isinstance(allowed_types, collections.Container): wrong_format_flag = self.content_type not in allowed_types elif allowed_types is None: # do not check the content_type wrong_format_flag = False else: # for other type raise TypeError("Unsupported container of `Content-Type`")

-1 for any containers, though. Even list.

socketpair · 2017-08-08T12:07:15Z

aiohttp/web_request.py

@@ -510,8 +510,26 @@ def text(self):
        return bytes_body.decode(encoding)

    @asyncio.coroutine
-    def json(self, *, loads=json.loads):
-        """Return BODY as JSON."""
+    def json(self, *, loads=json.loads, allowed_types=None, err_handler=None):


I vote for allowed_types='application/json'

That would be breaking change. See previous discussions.

Shall we directly check if the mime type starts with 'application/' and ends with '+json' i.e. flask's way to deal with it? Hence no need to consider about how should allowed_type behavior

If we stay with string only argument as @socketpair suggested, that's would be friendly default behaviour with almost no cost.

socketpair · 2017-08-08T12:12:14Z

aiohttp/web_request.py

+            if err_handler is None:
+                raise HTTPNotAcceptable()
+            elif callable(err_handler):
+                yield from err_handler()


Please do not check if it is callable. Instead, check return value of call. if it is awaitable -- await it. (see signal handlers in aiohttp)

In current code, with err_handler=42 -- error will be just ignored, which is a bug.

errors must not be handled as callbacks or so. There are exceptions for that.

As we discussed above, there's no need to give callbacks. Instead, only the 406 error is raised.

socketpair · 2017-08-10T19:26:28Z

aiohttp/web_request.py

+        charset = parameters.get('charset', None)
+
+        if isinstance(allowed_types, str):
+            if allowed_types == 'json':


Useless. Users must supply full contenttype like xxxx/yyyyy

This means 'json-like'. View my code after that. I will then check whether the Content-Type is 'application/json' or starts with 'application' and ends with '+json'. If it is a string and is not 'json', I will compare it with Content-Type directly. This may be convenient i.e. one won't need to always specify the argument for different json format

socketpair · 2017-08-10T19:27:42Z

aiohttp/web_request.py

+                        raise HTTPNotAcceptable()
+            elif allowed_types != content_type:
+                raise HTTPNotAcceptable()
+        elif isinstance(allowed_types, set):


-2 for containers. Why they are intended for ? Which user-story (case) do you suggest ?

Previously, we discussed what if one sends 'application/json; charset=utf-8' and 'application/json' is hard-coded and now I strip the charset. My concern is that will there be something else the frontend will send fail to pass the examination, says the handler need to handle both application/json and application/javascript or some uncontrollable i.e. for compatibility of an old frontend framework.

We should keep code of aiohttp simple and clear. Simple thing should be done in a simple way, and difficult ones should be possible.

If an specific application needs to handle such very obscure situation, it may control content-type by it's own before decoding of json. And also, I think, situation you have provided is just an imagination, not the real one. If no, please describe deeper.

socketpair · 2017-08-10T19:29:30Z

aiohttp/web_request.py

        body = yield from self.text()
-        return loads(body)
+        return loads(body, encoding=charset)


The other arguments have the same meaning as in load(), except encoding which is ignored and deprecated.

Charset should go as additional argument of the function. And should be passed to .text() if defined.

What if the frontend sends a request specifying charset? And I think a json object should be parsed with loads, with encoding supported by python itself. Should .text() decode something like "\\u4e16\\u754c\\u4f60\\u597d"?

PLEASE look at the aiohttp sources before asking and before changing the code. .text() will analyze conten-type header (regarding charset) before converting bytes to text.

JSON-decoding is two-step process. First, it needs to convert from bytes to characters (symbols). Thi is done by UTF-8 (typically) decoder in .text(). Second-step -- is to decode slash-escapes according to JSON-format. this is done using loads(). So, encoding you are talking about is for first step only. As you can see, letter u after every slash signifies that every item is UNICODE symbol (as opposed to some byte value). And so encoding is not applicable.

Yes. I did a search and found it so I removed these codes.

socketpair

Please remove containers
Make Support of either None or full content type
Remove encoding from loads

socketpair · 2017-08-11T10:49:40Z

aiohttp/web_request.py

+        """ Return BODY as JSON. If the `Content-Type` is not in allowed_types,
+        then the request is rejected.
+        """
+        content_type = self.content_type


it's not required to parse self.content_type if allowed_type is None

socketpair · 2017-08-11T10:51:02Z

aiohttp/web_request.py

+        if suffix != '':
+            content_type += '+' + suffix
+
+        if isinstance(allowed_type, str):


Much simplier:

if allowed_type is not None: parse content type if parsed_ct != allowed_type: raise ...

without any else

socketpair · 2017-08-11T10:56:44Z

aiohttp/web_request.py

-    def json(self, *, loads=json.loads):
-        """Return BODY as JSON."""
+    def json(self, *, loads=json.loads, allowed_type=None):
+        """ Return BODY as JSON. If the `Content-Type` is not in allowed_types,


is not in is wrong now.

…ype`

socketpair · 2017-08-12T09:38:50Z

aiohttp/web_request.py

+        allowed_type, then the request is rejected.
+        """
+
+        if allowed_type is None:  # do not check the content_type


I vote for replacing this chain with only one condition like:

if allowed_type is not None: ...

socketpair · 2017-08-12T09:40:07Z

aiohttp/web_request.py

+                content_type += '+' + suffix
+
+            if allowed_type != content_type:
+                raise HTTPNotAcceptable()


Also, I'm not sure if this concrete exception should be raised. Also please provide an message in exception.

@asvetlov @fafhrd91

So, what do you guys think now? Should the message be passed as an argument? I think the error should be raised directly since that's exactly what I want for a JSON API in most cases and I think we've reached the agreement that for special requirements one should write a middleware or catch the exception as we discussed before.

What would be wrong with just

def json(self, *, loads=json.loads, allowed_type='application/json'): if allowed_type and self.content_type != allow_type: raise HTTPNotAcceptable(text='invalid content-type: "{}" not "{}"'.format(self.content_type, allowed_type)) ...

?

If users really want more sophisticated behaviour than this they can add their own logic, there's nothing very complicated here that couldn't be replicated in a custom coroutine.

samuelcolvin · 2017-09-09T19:02:14Z

also is 406 really the correct response?

from wikipedia:

The requested resource is capable of generating only content not acceptable according to the Accept headers sent in the request.

That doesn't sound quite right. 400 is very generic but is technically more correct I think.

asvetlov · 2019-07-24T16:06:49Z

Fixed by another PR #3889

fduxiao force-pushed the content_type_check branch 2 times, most recently from 88f254c to a4abca2 Compare August 8, 2017 03:54

Check the content type before returning the json value.

bd5a95f

fduxiao force-pushed the content_type_check branch from a4abca2 to bd5a95f Compare August 8, 2017 06:21

kxepal requested changes Aug 8, 2017

View reviewed changes

add a parameter to specify allowed content type

f2d2fd5

kxepal requested changes Aug 8, 2017

View reviewed changes

socketpair reviewed Aug 8, 2017

View reviewed changes

restrict type of allowed content type in BaseRequest.json

5ae894d

fduxiao force-pushed the content_type_check branch from 3b16898 to 5ae894d Compare August 10, 2017 09:53

socketpair reviewed Aug 10, 2017

View reviewed changes

socketpair requested changes Aug 10, 2017

View reviewed changes

support either None or full content type

3eb62fe

socketpair reviewed Aug 11, 2017

View reviewed changes

parse the content type when needing to compare it with the `allowed_t…

267cf5d

…ype`

socketpair reviewed Aug 12, 2017

View reviewed changes

Merge branch 'master' into content_type_check

99068d8

atemate mentioned this pull request Jul 5, 2019

Raise 400 Bad Request on server-side await request.json() if incorrect content-type received #3889

Merged

5 tasks

asvetlov closed this Jul 24, 2019

Check the content type before returning the json value. #2177

Check the content type before returning the json value. #2177

Conversation

fduxiao commented Aug 8, 2017 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov-io commented Aug 8, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fduxiao Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fduxiao Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

socketpair Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fduxiao Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair Aug 12, 2017 • edited Loading

Choose a reason for hiding this comment

socketpair Aug 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelcolvin Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

samuelcolvin commented Sep 9, 2017

asvetlov commented Jul 24, 2019

fduxiao commented Aug 8, 2017 •

edited

Loading

codecov-io commented Aug 8, 2017 •

edited

Loading

fduxiao Aug 9, 2017 •

edited

Loading

socketpair Aug 8, 2017 •

edited

Loading

socketpair Aug 8, 2017 •

edited

Loading

fduxiao Aug 11, 2017 •

edited

Loading

socketpair Aug 10, 2017 •

edited

Loading

fduxiao Aug 11, 2017 •

edited

Loading

socketpair Aug 12, 2017 •

edited

Loading

socketpair Aug 12, 2017 •

edited

Loading

samuelcolvin Sep 9, 2017 •

edited

Loading