Alternator fails to reject oversized requests if not using Content-Length

[nyh]

Alternator places a limit on the request size, using Seastar's HTTP server feature of content_length_limit.

What the Seastar code does is just to check the Content-Length header... But a long request can be sent without such a header - using chunked-encoding or if the connection is to be closed after the request - even without any pre-specified length at all. Such requests are currently allowed at any length, which is wrong.

This issue can be reproduced by the test:

• test_manual_requests.py::test_too_large_request_chunked

We could add support in Seastar for this case as well: We have now a content_stream stream, and when the N+1th byte is read from the stream, or perhaps earlier when the next chunked-encoding chunk is known to go beyond position N, the read from the stream could throw an exception. This exception would cause the request handler to abort what it's doing. If the first Content-Length or chunk already go beyond N, we don't have to run the handler at all (this is what happens today).

Because the content_length_limit is a Seastar feature, we will need to open a corresponding issue for Seastar. We actually have two choices on what to do in Seastar:

1. Complete the Seastar feature - perhaps in the way suggested above.
2. Drop the incomplete feature from Seastar - and instead do the checking in Scylla: the JSON-parsing code already tracks the length of the parsing, and can throw when reaching the limit. Note that in Scylla, we can't report the error as soon as we see the Content-Length or chunk length and we can only report it once the Nth byte was actually read by the handler. I'm not sure this is bad.

[avikivity]

I don't think it merits a seastar feature. You should read from the content stream, and if you decide it's too long, close the stream and abort the request with an error. This way the condition (too long content) isn't hard-coded into Seastar but is left to the application.

[nyh]

I don't think it merits a seastar feature. You should read from the content stream, and if you decide it's too long, close the stream and abort the request with an error. This way the condition (too long content) isn't hard-coded into Seastar but is left to the application.

I tend to agree. For historical context on why we had this feature in Seastar: Until recently, we didn't have a content stream feature in Scylla's HTTP server, just a content string, so we needed this option to avoid having the server read a huge string. Now that we have (and use in Alternator since a recent patch) the content stream, we indeed don't need the content_length_limit inside Seastar.

One small benefit of being in the Seastar server that we'll lose if we switch to a Scylla-side use of the content stream: Inside the Seastar server, we have the advance knowledge of the size of the entire request (when Content-Length is used) or at least the next chunk (with chunked encoding), which can allow us to abort a request earlier - not just when we actually read the Nth byte. However, it's not clear to me that this is an important advantage. It probably isn't.