Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Steve H August 10th, 2009 @ 06:50 AM

  Lighthouse ate my URL, here it is again:

  https://rails.lighthouseapp.com/dashboard?x[y]=1&x[y][][w]=2
  

• 

  David Trasbo June 23rd, 2010 @ 02:13 PM

  • Assigned user set to “Rohit Arondekar”

  Needs more info. Also, Lighthouse seems to have eaten the URL in second attempt too.

• You flagged this item as spam.
  

  Steve Hoeksema June 23rd, 2010 @ 09:11 PM

  If you copy and paste the second URL into your address bar you will get an internal server error from Lighthouse, because Lighthouse is running Rails and this affects every Rails installation. I was just using it to conveniently illustrate a point.

  Have a look at the parameters:

  x[y]=1
  x[y][][w]=2

  This is because you're assigning x[y] to an integer, and then using [] to add another value to it as if x[y] was an array.

  Rack correctly raises a TypeError for this, but I feel Rails should catch this and return a 400 Bad Request as it is an error on the clients part, and not a 500 Internal Server Error.

  Rack already has a test case for this (see the example from the test suite).

• 

  Rohit Arondekar June 24th, 2010 @ 01:34 AM

  • State changed from “new” to “open”

  Confirmed that Rails does indeed throw 500 Internal Server Error.

  Can you write a failing test and a patch?

• 

  Rohit Arondekar July 5th, 2010 @ 11:29 AM

  • Assigned user changed from “Rohit Arondekar” to “José Valim”
  • Importance changed from “” to “Low”

  A erroring test for this issue. Not a great test but I'm open to suggestions :)

  • test_malformed_query_string.diff 1.4 KB

• You flagged this item as spam.
  

  Miles Egan July 10th, 2010 @ 07:49 PM

  This patch seems to fix this. It may look a little heavy-handed but as far as I can tell the only reason that query_parameters should return nil, as it does in this case, is that there was some kind of parameter parsing error and I imagine that a lot of code expects that params should always return at least an empty hash, so it doesn't seem too unreasonable a fix.

  • query_parameters.patch 688 Bytes
• 

  José Valim July 10th, 2010 @ 08:35 PM

  -1 for simply discarding the error. The client that send such requests should be aware that he sent an invalid one.

• You flagged this item as spam.
  

  Miles Egan July 10th, 2010 @ 08:49 PM

  This patch doesn't discard the error, it just allows rails to display the error to the client instead of issuing a 500.

  The code accessing query_parameters is used by the error page to display the error. It's not the source of the error.

• 

  José Valim July 10th, 2010 @ 09:24 PM

  Ah my bad, great! Could you please then provide a patch following the conventions here? http://rails.lighthouseapp.com/projects/8994/sending-patches

  Thanks!

• You flagged this item as spam.
  

  Miles Egan July 10th, 2010 @ 10:32 PM

  Ok I've followed the instructions on that page and added a test case that triggers the error and verifies that it's handled correctly. Please let me know if this is acceptable or if I missed something. Thanks.

  • query_parse_error_fix.patch 1.9 KB

• You flagged this item as spam.
  

  Miles Egan July 11th, 2010 @ 03:01 AM

  Here's an alternative solution, if you think it's reasonable to assert that request_parameters and query_parameters should never be nil.

  • query_parse_error_fix2.patch 2.1 KB
• 

  José Valim July 18th, 2010 @ 10:39 AM

  Miles, your patches accept that the error is raised, which is great. But the proper behavior would be rails to rescue such error and return the proper http status.

• You flagged this item as spam.
  

  Miles Egan July 18th, 2010 @ 03:41 PM

  José here is the chain of events as I see it:

  1. rails tries to handle the request, which triggers the param parsing code in rack
     
  2. rack fails to parse the query string and throws a type error
     
  3. rails catches the type error and tries to display an error page
     
  4. the error page tries to display the params of the request but fails because rack didn't set the environment variables it expects to contain the request params
     
  5. this generates the 500

  My patch fixes #4 so that the default rails exception handling does the right thing. The only remaining problem is the HTTP error code. This is generated in this code:

        def rescue_action_locally(request, exception)
          template = ActionView::Base.new([RESCUES_TEMPLATE_PATH],
            :request => request,
            :exception => exception,
            :application_trace => application_trace(exception),
            :framework_trace => framework_trace(exception),
            :full_trace => full_trace(exception)
          )
          file = "rescues/#{@@rescue_templates[exception.class.name]}.erb"
          body = template.render(:file => file, :layout => 'rescues/layout.erb')
          render(status_code(exception), body)
        end
  

  Which calls this code:

     def status_code(exception)
        Rack::Utils.status_code(@@rescue_responses[exception.class.name])
     end
  

  So rails is letting rack choose the error code. Unfortunately, rack defaults to a 500 for anything that doesn't map to the textual equivalent of a standard http result code (mappings in lib/rack/utils.rb in rack). So in order to return a 400 instead of a 500 in this case we'd either need to override the rack status code for this case or change the exception that is thrown in rack on a parse error or make the rack error code mapping smarter.

  In either case it seems like a rack issue not a rails issue.

  Edited by Rohit Arondekar for formating.

• 

  José Valim July 18th, 2010 @ 04:17 PM

  Oh yes, confirm! :) So I can apply this initial patch, I just have two small suggestions:

  1) The second patch looks good but we should probably move the logic to normalize_params.

  2) The test name could be better to reveal what we are really testing, something like "parameters can be accessed even after Rack::TypeError is raised" would better express their intent.

  Thanks!

• You flagged this item as spam.
  

  Miles Egan July 18th, 2010 @ 04:35 PM

  All that our normalize_parameters function does is recursively convert the params hash to a hash_with_indifferent_access, so it doesn't seem to me that it really should know about possible exceptions from rack since it's just a simple utility function.

  Another possibility would be to explicitly rescue the exception in the GET function that we override from the Rack::Request base class but we'd have to re-throw it to propagate the error correctly. That is, explicitly do a begin/rescue in this block:

      # Override Rack's GET method to support indifferent access                                                                         
      def GET
        @env["action_dispatch.request.query_parameters"] ||= normalize_parameters(super)
      end
      alias :query_parameters :GET
  

  I think you're right that the test could use a better name. I'll fix that.

  The more I think about this the more I think that rack should handle this a little differently. If it can't parse a query string it should return or raise an error that it maps back to a 400, not a 500.

• 

  José Valim July 18th, 2010 @ 04:41 PM

  You are correct, so the duplication in both GET and POST is ok. Indeed, Rack errors could be more http-ish instead of delegating such stuff to the framework.

• You flagged this item as spam.
  

  Miles Egan July 18th, 2010 @ 05:11 PM

  Attached is a new patch with a better test.

  • query_parse_error_fix3.patch 2.3 KB

• You flagged this item as spam.
  

  Miles Egan August 3rd, 2010 @ 08:46 PM

  Any further thoughts on this? Is the patch ok or should we take a different approach?

• 

  Repository October 11th, 2010 @ 11:56 PM

  • State changed from “open” to “resolved”

  (from [3eff729079a1a9e717e7872bdae19c8703b280ac]) make sure request parameters are accessible after rack throws an exception parsing the query string [#3030 state:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/3eff729079a1a9e717e7872bdae19c...

• You flagged this item as spam.
  

  Bodaniel Jeanes October 22nd, 2010 @ 08:48 PM

  I just came here to post an error this as I found what I think is the same bug as this one.

  I've boiled this down to the smallest params segment that still raised this exception:

  ?a[b]&a[]
  

  I looked over the patch and it seems that it would cover this scenario, but I thought I'd add that this was reproducible in Rails 1, 2, and 3 and Ruby 1.8.7, 1.9.2, and REE.