starlette
starlette copied to clipboard
encode
Add RequestSizeLimitMiddleware and RequestTimeoutMiddleware
A couple of things to consider:
- Dealing with SSE requests
- Overriding instead of adding. As this PR currently stands you could set a 10s timeout for your entire app and a stricter timeout for individual routes, but not the other way around (a 1s timeout for all routes except some specific routes which get not timeout, e.g. because they stream data). To get that working I think we'd need to do the enforcing at the route level and have a middleware that simply sets some markers on the
scopeobject to customize the limits. That would be more invasive of a change than this.
Both middlewares can be on a third party packages, and we can close the issue by just pointing at them.
Can we separate the two middlewares in different PRs? The initial discussion was about the request size one.
Both middlewares can be on a third party packages, and we can close the issue by just pointing at them.
I disagree. It's an important feature to have readily available to all Starlette users, it doesn't require 3rd party packages, the complexity isn't massive and I believe we should make some limits the default in 1.0 like Django and other more mature frameworks do.
Can we separate the two middlewares in different PRs? The initial discussion was about the request size one.
Yes I'm happy to do that if it'd make it easier to discuss.
for reference: my approach which works on per-route basis https://github.com/encode/starlette/pull/2175
My main concern about middleware based approach is that it is very limited. Say, your global middleware will always have a large value. Because if just one route needs to handle 1gb uploads, the global limit will be 1gb. Per route limits does not have this flaw, however it is still possible to read unlimited data in custom middlewares. But per-route approach solves the majority of cases.
I see it as app-level limit (the global one that covers all middlewares also) + option to override per a route. In result, we can get this example setup: 8mb global and 24mb for /user/photo-upload.
Yeah, I brought that up above.
I'll rework this to satisfy that use case, although it will be a good chunk more invasive as I said above.
Also sorry I did not consider your PR, I had lost track / forgotten about it.
I have a small question, why not just read the request content-length to limit the size.
Clients can easily lie about that
But proxy server or asgi server should not receive data more than content-length?
I'm not saying we shouldn't also limit the length to the reported content length, I'm just saying that it doesn't serve a security purpose.
I'm not saying we shouldn't also limit the length to the reported content length, I'm just saying that it doesn't serve a security purpose.
Maybe my previous words caused some misunderstanding. What I meant was: the forward reverse proxy server or ASGI server will not pass data to the ASGI application that exceeds the content-length length. Therefore, forging a content-length length on the client side will only result in truncation when the server receives data. For example, h11 used by Uvicorn will check whether the Content-Length in the Reader matches the actual size of the received data.
Then I’m confused as to what you are suggesting: are you saying that we should also enforce that limit, that that limit is already enforced elsewhere, and this PR (or #2174) is not needed, or something else?
What do we want to do for websockets?
- Tornado WebSockets have a limitation by default.
- https://www.tornadoweb.org/en/stable/releases/v4.5.0.html
- https://www.tornadoweb.org/en/stable/websocket.html#tornado.websocket.WebSocketHandler
- Uvicorn already limits the size of each WebSocket message via
—ws-max-sizeconfiguration - it only applies to websockets.- https://www.uvicorn.org/settings/#implementation.
I don't think WebSockets should be a point of concern here.
Lifespans?
Nothing to do for lifespans.
Dealing with SSE requests
No special logic is required for SSE.
Notes/Questions
- Is the argumentation of having resource limits on the ASGI application, and not ASGI server because you can apply on individual endpoints/have more granularity?
- We need to check the
Content-Length(single body event) on the requests that are notTransfer-Encoding: chunked(streaming).
I don't think WebSockets should be a point of concern here.
The configurations offered by Uvicorn don't cover all of the use cases here. You can't configure it on a per route basis for starters.
No special logic is required for SSE.
What I mean is that if you install a blanket timeout or request size limit it might make sense to say "SSE requests don't have a limit". That would require special logic.
Is the argumentation of having resource limits on the ASGI application, and not ASGI server because you can apply on individual endpoints/have more granularity?
Yes.
We need to check the Content-Length (single body event) on the requests that are not Transfer-Encoding: chunked (streaming).
Agreed, we can check it and if it is larger than the limit error immediately without streaming anything. But we can't "trust" it if it's smaller.
But Starlette doesn't depend on the server. We shouldn't make assumptions like this about the server's behavior unless it's part of the ASGI spec, which to my knowledge this is not. Besides, the only advantage of trusting the content-length header when it says it's smaller than the user defined limit is that we do one less ASGI middleware wrapping on receive. It's not expensive to keep track of the actual streamed size, I don't think it justifies the potential footgun.
@Kludex I’m not sure if it was on purpose or not but I noticed you closed #2175 and not this one. I don’t think there was any concensus on how to implement this feature, nor do I think this PR is intrinsically better in significant ways, so I just wanted to check what your intention was.