Limit max request size

[aviramha]

As discussed in the Gitter, my opinion is that starlette should provide a default limit for request size.
The main reason is that without it, any Starlette application is vulnerable to very easy DoS.
For example, newbie me can write a program as follows:

from starlette.requests import Request
from starlette.responses import Response


async def app(scope, receive, send):
    assert scope['type'] == 'http'
    request = Request(scope, receive)
    body = b''
    json = await request.json()
    response = Response(body, media_type='text/plain')
    await response(scope, receive, send)

As a malicious user, I could send a 30GB sized JSON and cause the memory to go OOM.
Other frameworks support this also - Django, Quart.
My proposal is to add a default limit which can be overrided in the app configuration.

[florimondmanca]

As far as I intuitively agree with the relevance of supporting this in Starlette core, my two cents here...

My proposal is to add a default limit which can be overrided in the app configuration.

Would this be a kwarg on the Starlette class? I don’t think there’s precedent for this - everything else is provided as middleware (even security features such as CORS or trusted hosts). Also not sure this would be the most reusable API (whereas other frameworks would benefit from a public ASGI middleware).

So personally not sure we want a new config option, or just a middleware that’s documented as « you should really turn this on for production » (both in the middleware docs and any « deployment » docs).

[aviramha]

As far as I intuitively agree with the relevance of supporting this in Starlette core, my two cents here...

My proposal is to add a default limit which can be overrided in the app configuration.

Would this be a kwarg on the Starlette class? I don’t think there’s precedent for this - everything else is provided as middleware (even security features such as CORS or trusted hosts). Also not sure this would be the most reusable API (whereas other frameworks would benefit from a public ASGI middleware).

So personally not sure we want a new config option, or just a middleware that’s documented as « you should really turn this on for production » (both in the middleware docs and any « deployment » docs).

What about a middleware that's used by default? For example there's already:

middleware - A list of middleware to run for every request. A starlette application will always automatically include two middleware classes. ServerErrorMiddleware is added the very outermost middleware, to handle any uncaught errors occuring anywhere in the entire stack. ExceptionMiddleware is added as the very innermost middleware, to deal with handled exception cases occuring in the routing or endpoints.

Add to this list the request size middleware. Another option is that the middleware parameter has default value that has the middleware.

[florimondmanca]

Including the middlware by default leads to the question of how to leave it out in case users want to specify it explicitly… ServerErrorMiddleware/ExceptionMiddleware work well that way because they don't have any associated config (apart from exception_handlers).

I don't think it'd be terribly awful for Starlette to not have everything turned-on-and-secure-by-default, i.e. if there are small steps required for the developer to make their app production-ready. (The philosophy is that of an ASGI toolkit and lightweight framework, so that's how I interpret it.)

There's been related discussion on FastAPI too, eg tiangolo/fastapi#362. Conversation was resolved as "you can implement this as middleware", and that feature was not provided in core. (Always a good idea to go see how things are going there, as a lot of FastAPI users want to see stuff in FastAPI whereas it would really benefit from being in Starlette or separate ASGI components…)

So I'm still inclined to think that "here's an upload limit ASGI middleware, please turn it on for prod" (i.e. no default, but easy to add) would be an okay approach for Starlette and the wider ASGI ecosystem.

[aviramha]

What about optional argument to the json, body and form functions with default values?
i.e

    async def json(self, max_size: int = 64000) -> typing.Any:
        if not hasattr(self, "_json"):
            body = await self.body(max_size)
            self._json = json.loads(body)
        return self._json

[rafalp]

Wouldn't it be more reliable for this limit to live on edge server, such as NGINX?

[aviramha]

Wouldn't it be more reliable for this limit to live on edge server, such as NGINX?

It is possible to limit it using NGINX, but you don't want to couple Starlette's resilience with an edge server.

[erewok]

I always have these limits in Nginx. It would not occur to me to have these limits in the framework but others may need this if they're exposing services without a reverse proxy in front? I'd just want a way to turn it off or not have it enabled by default.

[aviramha]

I always have these limits in Nginx. It would not occur to me to have these limits in the framework but others may need this if they're exposing services without a reverse proxy in front? I'd just want a way to turn it off or not have it enabled by default.

Because if Starlette doesn't need a reverse proxy, why add it? if Starlette relies on a reverse proxy to be protected it needs to be well documented and IMO it's a bad decision, as each layer need to protect itself.
Layering features is smart and OK, but layering protection is not really smart.. (Especially when the cost of adding this protection is so trivial and minimal)

[erewok]

Because if Starlette doesn't need a reverse proxy, why add it?

I deploy everything behind a reverse proxy because my services require load balancing and redundancy.

In fairness, however, I was trying to remember what Django does for this, and so I went looking for it. I honestly can't ever remember setting this DATA_UPLOAD_MAX_MEMORY_SIZE parameter in the past but I must have because I've had apps that accepted large file uploads, much larger than the defaulf of 2.5MB described in their docs.

There's a similar discussion on this issue in Gunicorn where in addition to talking about DDOS attacks, they also talk about what Werkzeug (the library underpinning Flask) does.

Further, it appears that Tornado also offers a max_buffer_size argument to their HTTPServer class, which closes the connection when a request exceeds that size (by default, it's set to 100MB).

In summary, it seems like lots of Python web frameworks (although I tend of think of Tornado as more of a server) implement something like this, but there's some concern about trending too far into full-blown server territory because there are could be many strategies to mitigate DDOS attacks that belong elsewhere than the web framework. Thus, it's probably not an uncontroversial thing to add if all the established libraries already offer it.

[gnat]

IMO there really should be a configurable limit and reasonable default if its not too expensive to do a simple check. This is an attack vector everyone has by default.

Right now I feel stuff like this keeps Starlette as more of a "pro only" framework rather than something usable by any level of developer.

[Chaostheorie]

Has there been any progress so far on this? Or should I configure my webserver or uvicorn to do this?

[gnat]

For those following this, even behind nginx, I've realised there's a number of ways to run denial of service on Starlette which can be mitigated with a simple timeout middleware. I recommend a dual solution of setting up nginx client_max_body_size, and using a Starlette middleware.

1. For nginx, you need to set client_max_body_size to limit total body size (and in turn, file uploads). For example, to limit to 100 MB, your core http config block should look like:

http {
    client_max_body_size 100m;
    # ... other stuff ...
}

2. As for Starlette itself, a timeout middleware can serve as a "catch all" solution for large file uploads and long running queries. Sample middleware here: How does one setup a global timeout to all requests? tiangolo/fastapi#1752 (comment)

from starlette.middleware.base import BaseHTTPMiddleware
from starlette.responses import HTMLResponse
import asyncio

class TimeoutMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request, call_next):
        try:
            response = await asyncio.wait_for(call_next(request), timeout=30)
        except asyncio.TimeoutError:
            return HTMLResponse("Request reached timeout.", status_code=504)
        return response

Timeout middleware seems to free the memory used by the attempted file upload as well.

Any additional thoughts, suggestions appreciated! We all want to achieve the goal of making our Starlette Apps more robust.

[gnat]

Note: By default PHP sets a request time limit of 30 seconds since version 4.0, which undoubtedly handles a lot of DOS sidecases.

https://www.php.net/manual/en/function.set-time-limit.php

[gnat]

2023 note: Sanic Server has these safeties out of the box, if you're looking for something that just works.

Still no sane default in the Uvicorn / Starlette stack, even with nginx in front- You must use the middleware or risk a DOS.