Add request_max_size option

[alex-oleshkevich]

This PR adds an option to limit request body size. This is a mitigation of some DoS attacks.

How it works
There are two places where you can define the limit: app instance and route. When defined on the app, the limit becomes global and guards all endpoints. The developer still can overwrite this value on per-route bases (for example on file upload routes).

app = Starlette(
    request_max_size=5,  # global
    routes=[Route("/", endpoint=read_view, methods=["POST"], request_max_size=24)], # route level
)

response = client.post("/", data=SOME_LARGE_PAYLOAD) 
assert response.status_code == 413

Refs #2155

[alex-oleshkevich]

This can be a base for the next guard - request timeout.

[alex-oleshkevich]

It is still possible to read unlimited data in mounted apps and in the middleware. But, I think, it is a developer's responsibility in this case.

[alex-oleshkevich]

Like in Django

[adriangb]

Can you permalink where you got this from in a comment below here just for the record? I'd also be okay with a code comment.

[alex-oleshkevich]

https://github.com/django/django/blob/main/django/conf/global_settings.py#L310

[Kludex]

What does flask do?

[alex-oleshkevich]

@Kludex Flask depends on the configuration option or on 'content-length' header. Otherwise, reads nothing.
https://flask.palletsprojects.com/en/2.3.x/config/#MAX_CONTENT_LENGTH
https://flask.palletsprojects.com/en/2.3.x/patterns/fileuploads/#improving-uploads

[alex-oleshkevich]

PHP has 8mb
https://github.com/php/php-src/blob/165a4e53da819d377734eb2286df1b54709d18f2/php.ini-production#L713

[alex-oleshkevich]

Is the exception good enough?

[adriangb]

Yeah fine by me

[Kludex]

Please follow the same logic as above.

        if "app" in scope:
            raise HTTPException(status_code=413, "Request Entity Too Large")
        else:
            response = PlainTextResponse("Request Entity Too Large", status_code=404)

[adriangb]

It might be a good idea to start making these attributes private.

[alex-oleshkevich]

Currently, all members of Starlette are public. I would not like to introduce a new style here.

[adriangb]

Not true:

starlette/starlette/datastructures.py

Line 61 in 2168e47

self._url = url

[alex-oleshkevich]

I am referring to the Starlette class itself
https://github.com/encode/starlette/blob/master/starlette/applications.py#L71

[adriangb]

Fair enough. I still think a lot of those shouldn't have been public in the first place and just because they are doesn't mean we should make this public as well.

[adriangb]

Yeah fine by me

[adriangb]

A couple things to consider @alex-oleshkevich:

• Can we make the configurator a middleware? That way it can be applied to Mount, etc. It makes sense to keep the parameters to the application object since I imagine a global and a per route override like you laid out here is going to be the most common approach, but it’d be nice to make it a bit more flexible. Also so that it can be used with current versions of FastAPI.
• Can we make the per-route enforcement a middleware that gets applied automatically? My main reasoning for this is to be able to use it in existing FastAPI applications, but I imagine other use cases like enforcing at the router level or on a mounted app may make sense

[alex-oleshkevich]

It can be a middleware from #2328 that automatically gets applied globally and per-route. And be reused anywhere what can accept a middleware (like Mount).

[adriangb]

That sounds good to me. It’ll be two middleware’s right? One to set the values (or override them), and another to enforce them.

[alex-oleshkevich]

Yes, like that. One middleware in the Starlette.middleware_stack and another one in the Route.handle. However, I am aware that middleware may add a method-call overhead compared to middlewareless implementation.

[alex-oleshkevich]

It may be worth to have a shared code to use in app, route and middleware.

[adriangb]

Yeah I was thinking we make two middlewares, let's call them RequestSizeLimitConfigurationMiddleware and RequestSizeLimitEnforcementMiddleware. In Starlette we'd do something like:

def __init__(self, ..., request_max_size_bytes: int | None = 1234) -> None:
    if request_max_size_bytes:
        self.app = RequestSizeLimitConfigurationMiddleware(self.app, request_max_size_bytes=request_max_size_bytes)

And then in Route (or maybe rather in the request_response function as pointed out above) we'd do something like:

def request_response(
    func: typing.Callable[[Request], typing.Union[typing.Awaitable[Response], Response]],
    max_request_size: int | None = 1234,
) -> ASGIApp:
    """
    Takes a function or coroutine `func(request) -> response`,
    and returns an ASGI application.
    """

    async def app(scope: Scope, receive: Receive, send: Send) -> None:
        request = Request(scope, receive, send)

        async def app(scope: Scope, receive: Receive, send: Send) -> None:
            if is_async_callable(func):
                response = await func(request)
            else:
                response = await run_in_threadpool(func, request)
            await response(scope, receive, send)

        await wrap_app_handling_exceptions(app, request)(scope, receive, send)
    if max_request_size is not None:
        app = RequestSizeLimitEnforcementMiddleware(app, max_request_size=max_request_size)
    return app

[alex-oleshkevich]

@Kludex what should we do with this ticket? It is stale and can be closed.

[Kludex]

@Kludex what should we do with this ticket? It is stale and can be closed.

Let's close this, and proceed with the subject on the issue, or on #2328.

Thanks Alex.