Do not allow BodyProxy to respond to to_ary or to_str

[jeremyevans]

This methods could trigger different behavior in rack that is undesired when using BodyProxy. When using BodyProxy, you always want the caller to iterate through the body using each.

See rack/rack-test#335 for an example where allowing BodyProxy to respond to to_str (when provided an invalid rack body) complicated debugging.

BodyProxy already had a spec with a description "not respond to :to_ary". While you would assume that this checked whether the body actually responded to to_ary, it did not. This fixes that, making sure that respond_to?(:to_ary) is false, and calling to_ary raises a NoMethodError. It adds a similar spec for to_str.

[MSP-Greg]

These methods could trigger different behavior in rack that is undesired when using BodyProxy. When using BodyProxy, you always want the caller to iterate through the body using each.

Why? First of all, I'll divide bodies into four categories: array, enum, call, and file. You feel that each should be called for either an array or enum body. I'll be happy to regenerate data for it, but:

1. I've seen enum bodies that were 'time' based, what I might called a streaming body. A server cannot optimize delivery of an enum body.

2. Let's say we have a 10kB body. With tcp & unix socket connections, there isn't a significant time difference between putting 10k or 10 x 1k on the wire. But, when using ssl connections, that time difference is much more pronounced. 10 x 1k is much slower.

So, a server should be able to optimize delivery if a body is known to be an Array, which should also respond to to_ary.

From what I've seen, the main use for BodyProxy is the callback.

I'm not concerned about to_str, nor do I see a need for its use, but to_ary can be very helpful.

[jeremyevans]

@MSP-Greg The original impetus for this PR was to_str, as that led to the confusion resulting in the filing of the rack-test issue referenced. I added to_ary handling as the current BodyProxy behavior is broken in regards to to_ary, because calling to_ary on a BodyProxy will not call the block passed when creating the BodyProxy. Not supporting to_ary is one way to fix it. The other way is to support to_ary if the body responds to it, but also wrap calls to to_ary so calling BodyProxy#to_ary also calls BodyProxy#close. Otherwise, it's in violation of SPEC, which states:

If the Body responds to both
+to_ary+ and +close+, its implementation of +to_ary+ must call
+close+.

I did some research, and this PR restores BodyProxy's historical behavior, introduced in 5b251a9. the not respond to :to_ary spec was originally added in a9c0e35. The BodyProxy handling of to_ary was removed in 72959eb, when Response#to_ary was removed, but the spec description was not updated.

I'm fine with updating the PR to support to_ary and have it call close if the underlying body responds to it, if that's what other maintainers prefer.

[ioquatix]

I'm fine with updating the PR to support to_ary and have it call close if the underlying body responds to it, if that's what other maintainers prefer.

It seems more compatible and transparent to me.

[MSP-Greg]

support to_ary and have it call close if the underlying body responds to it

Isn't 'closing' the responsibility of the consumer of the Rack::BodyProxy?

In Puma, if we call to_ary, we maintain a reference to the original body, and call close on it.

JFYI, I think current behavior is just fine, as noted below:

require 'rack/body_proxy'

ary = [1,2,3,4,5,6]
enum = ary.to_enum

a_closed = false
e_closed = false

array_body = Rack::BodyProxy.new(ary)  { a_closed = true }
enum_body  = Rack::BodyProxy.new(enum) { e_closed = true }

puts "array_body.respond_to? :to_ary  #{array_body.respond_to? :to_ary}"
puts "enum_body.respond_to?  :to_ary  #{enum_body.respond_to? :to_ary}"

array_body.close
enum_body.close

puts "a_closed = #{a_closed}", "e_closed = #{e_closed}"

[ioquatix]

@MSP-Greg this is what is in the spec:

      ##
      ## If the Body responds to +to_ary+, it must return an +Array+ whose
      ## contents are identical to that produced by calling +each+.
      ## Middleware may call +to_ary+ directly on the Body and return a new
      ## Body in its place. In other words, middleware can only process the
      ## Body directly if it responds to +to_ary+. If the Body responds to both
      ## +to_ary+ and +close+, its implementation of +to_ary+ must call
      ## +close+.

Calling close a 2nd time shouldn't be an issue but also shouldn't be necessary.

[jeremyevans]

OK, I pushed a commit to support BodyProxy#to_ary if the body supports to_ary.

[MSP-Greg]

@ioquatix

Thanks. Sorry, I've read the spec several times, but didn't recall that info. A link would be the spec's Enumerable Body section (sorry, a little promotion).

For now, I think Puma should continue to call close. As I recall, there is some Rails response that responds to to_ary , but calling to_ary returns nil. Need to check...

[ioquatix]

For now, I think Puma should continue to call close. As I recall, there is some Rails response that responds to to_ary , but calling to_ary returns nil. Need to check...

That would definitely be against the spec and I'd suggest emitting a warning.

[ioquatix]

LGTM! Thanks for your patience with my questions.

[casperisfine]

👋 Could we get a release for that? I got someone reporting this on Pitchfork: Shopify/pitchfork#117.

This bug can be quite catastrophic (not with modern Rails fortunately I we always assume BodyProxy#close may not be called for safety, but some other middlewares may rely on it always being called.

[ioquatix]

We are planning to discuss Rack 3.1 release at RubyKaigi, will you be there?

[ioquatix]

@jeremyevans if this is a bug what about backporting it to 3.0?

[casperisfine]

will you be there?

I will yes.

if this is a bug what about backporting it to 3.0?

IMO it's a bug, can even be a security vulnerability in some case by causing state leak between requests if the application rely on close being called to reset state.

[jeremyevans]

I'm OK with backporting to 3.0.

[Earlopain]

Sounds good, I openen #2176 for this

[ioquatix]

Thanks for the backport, @Earlopain

[Wryte]

Hey folks, trying to upgrade to 3.0.11 but running into problems when trying to test our middleware that rescues ActionDispatch::Http::Parameters::ParseError.

We used to be able to test it like this:

post '/endpoint.json', params: '{"malformed: *}}', headers: { 'CONTENT_TYPE' => 'application/json' }

and our middleware:

class RescueInvalidJson
  class MalformedJson < StandardError; end

  def initialize(app)
    @app = app
  end

  def call(env)
    @app.call(env)
  rescue ActionDispatch::Http::Parameters::ParseError => e
    error = MalformedJson.new("[FILTERED]")
    error.set_backtrace(e.backtrace)
    Thingy::Exception.submit(error)
    [400, { "Content-Type" => "application/json" }, "Invalid JSON params"]
  end
end

would allow us to assert the 400 response. This is the error we are getting now though:

Minitest::UnexpectedError:         NoMethodError: undefined method `each' for an instance of String
            /Users/thinger/.gem/ruby/3.3.1/gems/rack-3.0.11/lib/rack/body_proxy.rb:56:in `method_missing'
            /Users/thinger/.gem/ruby/3.3.1/gems/rack-3.0.11/lib/rack/body_proxy.rb:56:in `method_missing'
            /Users/thinger/.gem/ruby/3.3.1/gems/rack-3.0.11/lib/rack/mock_response.rb:64:in `body'
            /Users/thinger/.gem/ruby/3.3.1/gems/actionpack-7.1.3.4/lib/action_dispatch/testing/test_response.rb:14:in `from_response'
            /Users/thinger/.gem/ruby/3.3.1/gems/actionpack-7.1.3.4/lib/action_dispatch/testing/integration.rb:293:in `process'
            /Users/thinger/.gem/ruby/3.3.1/gems/actionpack-7.1.3.4/lib/action_dispatch/testing/integration.rb:22:in `post'
            /Users/thinger/.gem/ruby/3.3.1/gems/actionpack-7.1.3.4/lib/action_dispatch/testing/integration.rb:379:in `post'
            /Users/thinger/.gem/ruby/3.3.1/gems/rails-controller-testing-1.0.5/lib/rails/controller/testing/integration.rb:16:in `block (2 levels) in <module:Integration>'

Is there a better way now to test a malformed payload or a use case that was overlooked that will need changes for?

(Feel free to direct me to an issue if this isn't an appropriate place to reach out)

[ioquatix]

Are you able to show us your test app? It looks like it’s returnin a string rather than an array of strings for the body.