MethodOverride middleware silently eats invalid parameters.

[ioquatix]

See also #2006. cc @byroot @jeremyevans

The current implementation of method_override_param effectively eats invalid parameters.

    def method_override_param(req)
      req.POST[METHOD_OVERRIDE_PARAM_KEY] if req.form_data? || req.parseable_data?
    rescue Utils::InvalidParameterError, Utils::ParameterTypeError
      req.get_header(RACK_ERRORS).puts "Invalid or incomplete POST params"
    rescue EOFError
      req.get_header(RACK_ERRORS).puts "Bad request content body"
    end

The first time this is called, an exception is raised and caught. However, the params is cached, and so no follow-up call to params doesn't appear to fail with the same error. This can change the behaviour of web applications which expect Rack::Request#POST or #params to raise an exception on invalid input.

I'm not sure what the ideal behaviour should be. Maybe we need to cache the exception and raise it in subsequent calls?

Example reproduction:

#!/usr/bin/env ruby

require 'stringio'
require 'rack/request'

env = {
  'REQUEST_METHOD' => 'POST',
  'PATH_INFO' => '/foo',
  'rack.input' => StringIO.new('invalid=bar&invalid[foo]=bar'),
  'HTTP_CONTENT_TYPE' => "application/x-www-form-urlencoded",
}

2.times do
  begin
    pp params: Rack::Request.new(env).POST
  rescue => error
    pp error: error
  end
end

Output:

["/home/samuel/.gem/ruby/3.2.0/gems/rack-3.0.3/lib/rack/query_parser.rb:79:in `block in parse_nested_query'",
 "/home/samuel/.gem/ruby/3.2.0/gems/rack-3.0.3/lib/rack/query_parser.rb:76:in `each'",
 "/home/samuel/.gem/ruby/3.2.0/gems/rack-3.0.3/lib/rack/query_parser.rb:76:in `parse_nested_query'",
 "/home/samuel/.gem/ruby/3.2.0/gems/rack-3.0.3/lib/rack/request.rb:648:in `parse_query'",
 "/home/samuel/.gem/ruby/3.2.0/gems/rack-3.0.3/lib/rack/request.rb:512:in `POST'",
 "/home/samuel/Desktop/request.rb:15:in `block in <main>'",
 "/home/samuel/Desktop/request.rb:13:in `times'",
 "/home/samuel/Desktop/request.rb:13:in `<main>'"]
{:error=>
  #<Rack::QueryParser::ParameterTypeError: expected Hash (got String) for param `invalid'>}
{:params=>{}}

[ioquatix]

Part of the problem here is probably to do with the fact that the input body is no longer rewindable. If it was, it would probably be rewound and on parsing a 2nd time, the exception would be raised. I think the correct solution here is to cache the exception.

[byroot]

I think the correct solution here is to cache the exception.

Yes.