Repository: aio-libs/aiohttp
Pull request: Add an argument to override request body size
<!-- Thank you for your contribution! -->

## What do these changes do?

<!-- Please give a short brief about these changes. -->
A new argument `client_max_size` was added to `BaseRequest.clone` method to allow overriding the request body size.

## Are there changes in behavior for the user?

<!-- Outline any notable behaviour for the end users. -->
The new argument adds the ability to customize the request body size for each request.

## Related issue number

<!-- Are there any issues opened that will be resolved by merging this change? -->
Resolves #5704

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist
- [x] Documentation reflects the changes
- [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt`
  * The format is &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [x] Add a new news fragment into the `CHANGES` folder
  * name it `<issue_id>.<type>` for example (588.bugfix)
  * 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."


Problem statement:
Customising the maximum size of request payload
🐣 **Is your feature request related to a problem? Please describe.**
<!-- A clear and concise description of what the problem is. Ex. I'm always frustrated when [...] -->

I would like to explicitly limit the maximum size of an incoming json body for a POST request. 


💡 **Describe the solution you'd like**
<!-- A clear and concise description of what you want to happen. -->

Currently I see there is a private `_client_max_size` property for such purpose, which equals to 4MB by default. Although, I don't see a clear way to, for example, limit the maximum size of a json object that one retrieves from `await request.json()` call.

❓ **Describe alternatives you've considered**
<!-- A clear and concise description of any alternative solutions or features you've considered. -->

Currently the following workaround is used:

```python
        default_client_max_size = request._client_max_size
        request._client_max_size = MAX_PAYLOAD_SIZE
        try:
            req_json = await request.json()
        except web_exceptions.HTTPClientError:
            raise
        except Exception:
            raise web.HTTPBadRequest(reason='The request body is expected to be a valid JSON')
        finally:
            request._client_max_size = default_client_max_size
```

Although, it would be nice if `_client_max_size ` would be public/had a setter, so one could limit the body size in `request.text()` or `request.json()` APIs without changing values of private properties.

📋 **Additional context**
<!-- Add any other context or screenshots about the feature request here. -->

Hints from issue discussion:
@mykola-mokhnach there's a way to change the maximum size of request payload when initializing `web.Application`:
`web.Application(client_max_size=1024 ** 2)`, the specified size will be used for all requests.
Thanks @anesabml My feature request is about customizing it for a single route rather than for the whole webapp
@mykola-mokhnach I think it is possible to add a feature to specify the maximum size per request. but for now, you can specify it globally or using your suggested workaround.
It sounds like you'd want to have a check after receiving an HTTP request. I do not think it's reasonable for it to be present on the HTTP protocol level because route handlers are matched on a higher level than HTTP. For per-handler/route checks, I imagine that the best solution is to make a decorator or a middleware that would look at the `Content-Length` header and raise an error if it's not satisfactory. You could also use lower level methods to read only a part of the payload to cover the case without the length header.

P.S. Your workaround looks dangerously unpredictable, race-condition-wise: you have `await` which pauses the coroutine execution and let's other coroutines on the loop get executed, meaning that another request handler may mess with that attribute concurrently.
> Your workaround looks dangerously unpredictable, race-condition-wise: you have await which pauses the coroutine execution and let's other coroutines on the loop get executed, meaning that another request handler may mess with that attribute concurrently.

Isn't this property set per request instance? I don't think it's shared with anything else.

> For per-handler/route checks, I imagine that the best solution is to make a decorator or a middleware that would look at the Content-Length header and raise an error if it's not satisfactory. You could also use lower level methods to read only a part of the payload to cover the case without the length header.

Why one should trust Content-Length? Nothing prevents an attacher to manipulate its value while providing a different length of the body payload.

> You could also use lower level methods to read only a part of the payload to cover the case without the length header.

Yes, I could use read(n_bytes) like it's done in request.json() or request.text() helpers themselves. But then what is the point in having these shortcuts at all?
> Isn't this property set per request instance?

Ah. I missed that.


> Why one should trust Content-Length? Nothing prevents an attacher to manipulate its value while providing a different length of the body payload.

Why would you perceive this as a security feature?


> Yes, I could use read(n_bytes) like it's done in request.json() or request.text() helpers themselves. But then what is the point in having these shortcuts at all?

There are always common utility helpers that most people need and then there are features that only one user on the internet will ever use. The latter ain't worthy of being shipped to every single user as a part of the common framework, they should be provided via third-party libs or even embedded into one's project.

That said, for this to be considered for inclusion, we need someone to write up a comparison of how and if other web frameworks provide such an API.
This is, for example, what Django has:
https://docs.djangoproject.com/en/1.11/ref/settings/#data-upload-max-memory-size
https://docs.djangoproject.com/en/1.11/ref/settings/#std:setting-FILE_UPLOAD_MAX_MEMORY_SIZE
https://github.com/django/channels/issues/1240
> This is, for example, what Django has:
> [docs.djangoproject.com/en/1.11/ref/settings/#data-upload-max-memory-size](https://docs.djangoproject.com/en/1.11/ref/settings/#data-upload-max-memory-size)
> [docs.djangoproject.com/en/1.11/ref/settings/#std:setting-FILE_UPLOAD_MAX_MEMORY_SIZE](https://docs.djangoproject.com/en/1.11/ref/settings/#std:setting-FILE_UPLOAD_MAX_MEMORY_SIZE)
> [django/channels#1240](https://github.com/django/channels/issues/1240)

Those settings look global, not per-handler.
We have the` request.clone()` method to change some request properties.

I can accept a pull request that 
1. adds `client_max_size` to a list of accepted arguments
2. adds `BaseRequest.client_max_size` read-only property
3. reflects changes in docs
4. has some tests to cover new functionality

Commit message: Fix mypy issue

Patch to review:
diff --git a/CHANGES/5704.feature b/CHANGES/5704.feature
new file mode 100644
index 00000000000..51d5d0ee1e6
--- /dev/null
+++ b/CHANGES/5704.feature
@@ -0,0 +1 @@
+A new argument `client_max_size` was added to `BaseRequest.clone` method to allow overriding the request body size -- :user:`anesabml`.
diff --git a/aiohttp/web_request.py b/aiohttp/web_request.py
index 6646a82f6ca..30b2e8d3fc4 100644
--- a/aiohttp/web_request.py
+++ b/aiohttp/web_request.py
@@ -205,6 +205,7 @@ def clone(
         scheme: Union[str, _SENTINEL] = sentinel,
         host: Union[str, _SENTINEL] = sentinel,
         remote: Union[str, _SENTINEL] = sentinel,
+        client_max_size: int = 1024 ** 2,
     ) -> "BaseRequest":
         """Clone itself with replacement some attributes.
 
@@ -247,7 +248,7 @@ def clone(
             self._payload_writer,
             self._task,
             self._loop,
-            client_max_size=self._client_max_size,
+            client_max_size=client_max_size,
             state=self._state.copy(),
             **kwargs,
         )
@@ -270,6 +271,10 @@ def transport(self) -> Optional[asyncio.Transport]:
     def writer(self) -> AbstractStreamWriter:
         return self._payload_writer
 
+    @property
+    def client_max_size(self) -> int:
+        return self._client_max_size
+
     @reify
     def rel_url(self) -> URL:
         return self._rel_url
@@ -852,6 +857,7 @@ def clone(
         scheme: Union[str, _SENTINEL] = sentinel,
         host: Union[str, _SENTINEL] = sentinel,
         remote: Union[str, _SENTINEL] = sentinel,
+        client_max_size: int = sentinel,
     ) -> "Request":
         ret = super().clone(
             method=method,
@@ -860,6 +866,7 @@ def clone(
             scheme=scheme,
             host=host,
             remote=remote,
+            client_max_size=client_max_size,
         )
         new_ret = cast(Request, ret)
         new_ret._match_info = self._match_info
diff --git a/docs/web_advanced.rst b/docs/web_advanced.rst
index 480becf97c7..ab44078c3ad 100644
--- a/docs/web_advanced.rst
+++ b/docs/web_advanced.rst
@@ -974,9 +974,9 @@ headers too, pushing non-trusted data values.
 That's why *aiohttp server* should setup *forwarded* headers in custom
 middleware in tight conjunction with *reverse proxy configuration*.
 
-For changing :attr:`BaseRequest.scheme` :attr:`BaseRequest.host` and
-:attr:`BaseRequest.remote` the middleware might use
-:meth:`BaseRequest.clone`.
+For changing :attr:`BaseRequest.scheme` :attr:`BaseRequest.host`
+:attr:`BaseRequest.remote` and :attr:`BaseRequest.client_max_size`
+the middleware might use :meth:`BaseRequest.clone`.
 
 .. seealso::
 
diff --git a/docs/web_reference.rst b/docs/web_reference.rst
index bc60bcabd28..9ca36d72aac 100644
--- a/docs/web_reference.rst
+++ b/docs/web_reference.rst
@@ -149,6 +149,23 @@ and :ref:`aiohttp-web-signals` handlers.
 
       .. seealso:: :ref:`aiohttp-web-forwarded-support`
 
+   .. attribute:: client_max_size
+
+      The maximum size of the request body.
+
+      The value could be overridden by :meth:`~BaseRequest.clone`.
+
+      Read-only :class:`int` property.
+
+      .. versionchanged:: 4.0
+
+         *Forwarded* and *X-Forwarded-Proto* are not used anymore.
+
+         Call ``.clone(client_max_size=new_size)`` for setting up the value
+         explicitly.
+
+      .. seealso:: :ref:`aiohttp-web-forwarded-support`
+
    .. attribute:: path_qs
 
       The URL including PATH_INFO and the query string. e.g.,
diff --git a/tests/test_web_request.py b/tests/test_web_request.py
index b4d6514d16a..60c34319ddc 100644
--- a/tests/test_web_request.py
+++ b/tests/test_web_request.py
@@ -513,6 +513,12 @@ def test_clone_client_max_size() -> None:
     assert req2._client_max_size == 1024
 
 
+def test_clone_override_client_max_size() -> None:
+    req = make_mocked_request("GET", "/path", client_max_size=1024)
+    req2 = req.clone(client_max_size=2048)
+    assert req2.client_max_size == 2048
+
+
 def test_clone_method() -> None:
     req = make_mocked_request("GET", "/path")
     req2 = req.clone(method="POST")


Code review comments:
diff_hunk: @@ -0,0 +1 @@
+A new argument `client_max_size` was added to `BaseRequest.clone` method to allow overriding the request body size -- :user:`anesabml`.
path: CHANGES/5704.feature
text: @user1:
This should use RST, not Markdown inline code.

diff_hunk: @@ -205,6 +205,7 @@ def clone(
         scheme: Union[str, _SENTINEL] = sentinel,
         host: Union[str, _SENTINEL] = sentinel,
         remote: Union[str, _SENTINEL] = sentinel,
+        client_max_size: int = 1024 ** 2,
path: aiohttp/web_request.py
text: @user1:
Why not just use a `Union` as all the above arguments do?