Multiple writes to Rack::Response result in incorrect content-length header in Rack 3

[mattbrictson]

Problem

After upgrading to Rack 3, I found that Rack::Response generates an incorrect content-length header when:

• it is constructed with an empty array for the body, and
• write is called multiple times

Here is a simple reproduction script:

require "rack"

status = 200
headers = {}
response = Rack::Response.new([], status, headers)

# Write 11 total bytes
response.write "hello"
response.write "world"
response.write "!"

# Resulting content-length should be 11, but is 26
response.headers
# => {"content-length"=>"26"}

Underlying cause

When Rack::Response is constructed with an empty array, it sets this internal state:

rack/lib/rack/response.rb

Line 82 in 0cd4d40

@buffered = nil # undetermined as of yet.

This causes the value of @length to accumulate on every call to Rack::Response#write:

rack/lib/rack/response.rb

Lines 317 to 323 in 0cd4d40

	if @buffered.nil?
	if @body.is_a?(Array)
	# The user supplied body was an array:
	@body = @body.compact
	@body.each do |part|
	@length += part.to_s.bytesize
	end

When chunked? is false, @length is incremented yet again, and then emitted as a content-length header:

rack/lib/rack/response.rb

Lines 348 to 350 in 0cd4d40

	unless chunked?
	@length += chunk.bytesize
	set_header(CONTENT_LENGTH, @length.to_s)

The result is that the content-length value is too large, and the error gets larger on every call to write.

[ioquatix]

Without digging into it, it appears that the latter @length += chunk.bytesize is incorrect. Do you think removing it is the correct course of action?

[mattbrictson]

I'm actually not sure I understand the intention of the existing code, in particular why @buffered is set to nil in the constructor and lazily initialized later. Since I may be missing the point of @buffered, take what follows with a grain of salt.

The problem I see is that there is a missing assignment of @buffered in one of the conditional branches within buffered_body!.

Abbreviated, the code currently looks like this:

if @buffered.nil?
  if @body.is_a?(Array)
    # ...
    # ⚠️ @buffered is not set in this branch
  elsif @body.respond_to?(:each)
    # ...
    @buffered = true
  else
    @buffered = false
  end
end

It seems like the point of this code was to lazily init @buffered to true or false, but currently that isn't happening in the first branch. So I think the fix is to add @buffered = true, like this:

--- lib/rack/response.rb
+++ lib/rack/response.rb
@@ -318,12 +318,14 @@ module Rack
           if @body.is_a?(Array)
             # The user supplied body was an array:
             @body = @body.compact
             @body.each do |part|
               @length += part.to_s.bytesize
             end
+
+            @buffered = true
           elsif @body.respond_to?(:each)
             # Turn the user supplied body into a buffered array:
             body = @body
             @body = Array.new
 
             body.each do |part|

With this one-line change, I get the correct content-length result when running my reproduction script.

[ioquatix]

Do you mind making a PR?

[mattbrictson]

OK, I've opened PR #2150