Add max response body size

[eric0fw]

This is to avoid eating all the memory when fetching from untrusted links.

[eric0fw]

@sneako Let me know if something is missing. I want to close this issue soon since we will be putting this in production.

[sneako]

Hi @eric0fw ! I'm back from a short vacation and looking over this again.

I am hesitant to move forward here for a few reasons, but the main reason being that if we aren't going to pass the number of bytes that we want to receive from the socket to Mint.HTTP.recv/3, then this solution is not any better than what could be achieved with a custom streaming function.

This PR also includes changes that are orthogonal to the issue of adding a max response body size, which I would prefer to be added and thoroughly tested in a separate PR. Adding [:flush] to the Process.demonitor/2 calls is one example, adding a call to close/1 in Conn.request/6 is another.

If you are under pressure at work to get this functionality today, I would suggest using a custom streaming function like this which should be very similar to the changes in this PR:

      acc = {nil, [], [], 0}

      fun = fn
        {:status, value}, {_, headers, body, body_size_acc} ->
          {value, headers, body, body_size_acc}

        {:headers, value}, {status, headers, body, body_size_acc} -> 
          if content_size_too_large?(headers) do
            throw "body too large"
          else
            {status, headers ++ value, body, body_size_acc}
          end

        {:data, value}, {status, headers, body, body_size_acc} ->
          total_body_size = body_size_acc + byte_size(size)

          if total_body_size > max_body_size do
            throw "body too large"
          else
            {status, headers, [value | body], total_body_size}
          end
      end

     Finch.stream(req, name, acc, fun)

[eric0fw]

Hi @eric0fw ! I'm back from a short vacation and looking over this again.

I am hesitant to move forward here for a few reasons, but the main reason being that if we aren't going to pass the number of bytes that we want to receive from the socket to Mint.HTTP.recv/3, then this solution is not any better than what could be achieved with a custom streaming function.

This PR also includes changes that are orthogonal to the issue of adding a max response body size, which I would prefer to be added and thoroughly tested in a separate PR. Adding [:flush] to the Process.demonitor/2 calls is one example, adding a call to close/1 in Conn.request/6 is another.

If you are under pressure at work to get this functionality today, I would suggest using a custom streaming function like this which should be very similar to the changes in this PR:

      acc = {nil, [], [], 0}

      fun = fn
        {:status, value}, {_, headers, body, body_size_acc} ->
          {value, headers, body, body_size_acc}

        {:headers, value}, {status, headers, body, body_size_acc} -> 
          if content_size_too_large?(headers) do
            throw "body too large"
          else
            {status, headers ++ value, body, body_size_acc}
          end

        {:data, value}, {status, headers, body, body_size_acc} ->
          total_body_size = body_size_acc + byte_size(size)

          if total_body_size > max_body_size do
            throw "body too large"
          else
            {status, headers, [value | body], total_body_size}
          end
      end

     Finch.stream(req, name, acc, fun)

If I include changes for the recv buffer and provide separate PRs for unrelated fixes. Will you take this PR?

[sneako]

Sure, I would still be willing to work together towards a solution that I would want to merge

[eric0fw]

@sneako I made the changes requested, including the recv optimization.

Please take a look. Once this is merged, I can provide the other changes as separate PRs.

[luiz-firework]

Hi, @sneako! I really think this is ready for merging.

Can you provide any updates?

Thanks!

[sneako]

Hi @luiz-firework, unfortunately there are still several issues I had commented on that have been 'marked as resolved' without any changes or comment...

Why do we need to add these additional calls to close/1 and Process.demonitor/1? I would feel much better about adding these if there were some test cases that show they are necessary. Our current implementation is very heavily used and seems to work quite well, so I am not sure that they are necessary. Of course I would be happy to see some proof to the contrary, but

Why are we only testing HTTP/2? We should definitely test both protocols.

I am also not such a fan of this particular usage of with, with the happy path happening in the else clause.

I would like to see the code to parse the content-length headers extracted in to a well named function, as well as the code that calculates the remaining max body length. Maybe this logic should be centralized in a new module so we can use it for both http/1 & 2?

The option name :max_response_body is long, but still feels a bit vague. Maybe it should be :max_response_body_size or _length?

[sneako]

Closing due to inactivity, and I have created https://github.com/sneako/finch/issues/224 to track the issue