Skip to content

Commit

Permalink
Do not allow BodyProxy to respond to to_ary or to_str
Browse files Browse the repository at this point in the history
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.

If body passed to BodyProxy responds to to_path, have the to_path
method close the body proxy.  Without this, use of Rack::Lock
with a body that responds to to_path would result in the mutex
remaining locked if the server calls to_path and does not iterate
over the body (which is allowed by SPEC).
  • Loading branch information
jeremyevans committed Mar 19, 2023
1 parent 8483b10 commit 703ba00
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 7 deletions.
20 changes: 18 additions & 2 deletions lib/rack/body_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ def initialize(body, &block)

# Return whether the wrapped body responds to the method.
def respond_to_missing?(method_name, include_all = false)
super or @body.respond_to?(method_name, include_all)
case method_name
when :to_str, :to_ary
false
else
super or @body.respond_to?(method_name, include_all)
end
end

# If not already closed, close the wrapped body and
Expand All @@ -38,7 +43,18 @@ def closed?

# Delegate missing methods to the wrapped body.
def method_missing(method_name, *args, &block)
@body.__send__(method_name, *args, &block)
case method_name
when :to_str, :to_ary
super
when :to_path
begin
@body.__send__(method_name, *args, &block)
ensure
close
end
else
@body.__send__(method_name, *args, &block)
end
end
# :nocov:
ruby2_keywords(:method_missing) if respond_to?(:ruby2_keywords, true)
Expand Down
43 changes: 38 additions & 5 deletions test/spec_body_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,45 @@ def body.banana(foo: nil); foo end
end

it 'not respond to :to_ary' do
body = Object.new.tap { |o| def o.to_ary() end }
body.respond_to?(:to_ary).must_equal true
proxy = Rack::BodyProxy.new([]) { }
proxy.respond_to?(:to_ary).must_equal false
proc do
proxy.to_ary
end.must_raise NoMethodError
end

proxy = Rack::BodyProxy.new(body) { }
x = [proxy]
assert_equal x, x.flatten
it 'not respond to :to_str' do
proxy = Rack::BodyProxy.new("string body") { }
proxy.respond_to?(:to_str).must_equal false
proc do
proxy.to_str
end.must_raise NoMethodError
end

it 'not respond to :to_path if body does not respond to it' do
proxy = Rack::BodyProxy.new("string body") { }
proxy.respond_to?(:to_path).must_equal false
proc do
proxy.to_path
end.must_raise NoMethodError
end

it 'should close body when calling to_path if body responds to it' do
body_closed = false
proxy_block_called = false
o = Object.new
o.define_singleton_method(:close) { body_closed = true }
def o.to_path; '/foo' end
def o.each; end
proxy = Rack::BodyProxy.new(o) { proxy_block_called = true }

proxy.respond_to?(:to_path).must_equal true
body_closed.must_equal false
proxy_block_called.must_equal false

proxy.to_path.must_equal '/foo'
body_closed.must_equal true
proxy_block_called.must_equal true
end

it 'not close more than one time' do
Expand Down
6 changes: 6 additions & 0 deletions test/spec_lock.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ def lock
def unlock
@synchronized = false
end

def locked?
@synchronized
end
end

module LockHelpers
Expand Down Expand Up @@ -67,7 +71,9 @@ def res.to_path ; "/tmp/hello.txt" ; end
body = app.call(env)[2]

body.must_respond_to :to_path
lock.locked?.must_equal true
body.to_path.must_equal "/tmp/hello.txt"
lock.locked?.must_equal false
end

it 'not delegate to_path if body does not implement it' do
Expand Down

0 comments on commit 703ba00

Please sign in to comment.