search

ruby-grape / grape

An opinionated framework for creating REST-like APIs in Ruby.
http://www.ruby-grape.org
MIT License
9.88k stars 1.22k forks source link

Invalid % Encoding #2159

Open nab0310 opened 3 years ago

nab0310 commented 3 years ago

Hi!

So I've found what I think is a problem with how Grape passes query parameters to Rack. Using Grape 1.5.1, I made a simple Grape app running on Puma and Rack. I can get an error consistently when sending in the request /api/ping?test=%9g. This manifests itself as the following stack trace:

Rack::QueryParser::InvalidParameterError: invalid %-encoding (%9g)
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/2.7.0/uri/common.rb:387:in `decode_www_form_component'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/utils.rb:54:in `unescape'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/query_parser.rb:155:in `unescape'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/query_parser.rb:69:in `block (2 levels) in parse_nested_query'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/query_parser.rb:69:in `map!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/query_parser.rb:69:in `block in parse_nested_query'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/query_parser.rb:68:in `each'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/query_parser.rb:68:in `parse_nested_query'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/utils.rb:99:in `parse_nested_query'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/formatter.rb:145:in `format_from_params'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/formatter.rb:125:in `negotiate_content_type'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/formatter.rb:19:in `before'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/base.rb:34:in `call!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/base.rb:29:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/error.rb:39:in `block in call!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/error.rb:38:in `catch'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/error.rb:38:in `call!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/base.rb:29:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/head.rb:12:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/endpoint.rb:231:in `call!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/endpoint.rb:225:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/router/route.rb:58:in `exec'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/router.rb:116:in `process_route'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/router.rb:72:in `block in identity'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/router.rb:91:in `transaction'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/router.rb:70:in `identity'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/router.rb:55:in `block in call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/router.rb:132:in `with_optimization'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/router.rb:54:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/api/instance.rb:167:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/api/instance.rb:71:in `call!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/api/instance.rb:66:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/api.rb:68:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/tempfile_reaper.rb:15:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/lint.rb:50:in `_call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/lint.rb:38:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/show_exceptions.rb:23:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/common_logger.rb:38:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/content_length.rb:17:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/puma-5.1.1/lib/puma/configuration.rb:246:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/puma-5.1.1/lib/puma/request.rb:76:in `block in handle_request'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/puma-5.1.1/lib/puma/thread_pool.rb:337:in `with_force_shutdown'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/puma-5.1.1/lib/puma/request.rb:75:in `handle_request'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/puma-5.1.1/lib/puma/server.rb:431:in `process_client'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/puma-5.1.1/lib/puma/thread_pool.rb:145:in `block in spawn_thread'

The reason why I think it's Grapes responsibility to validate this is that according to this issue (https://github.com/rack/rack/issues/337) the Rack community believes it's on the application of framework code to catch this.

In my test project I created a middleware that catches the Rack::QueryParser::InvalidParameterError and maps it to a 400 rather naively. I believe the exception occurs on the first instance of the framework of trying to call Rack to parse the parameters.

Just wanted to start the conversation here because I haven't found it yet. Thanks!

dm1try commented 3 years ago

In my test project I created a middleware that catches the Rack::QueryParser::InvalidParameterError and maps it to a 400 rather naively.

This can be implemented in a simpler way by using rescue_from Rack::QueryParser::InvalidParameterError 

    rescue_from Rack::QueryParser::InvalidParameterError do |e|
      error! e, 400
    end

I believe the exception occurs on the first instance of the framework of trying to call Rack to parse the parameters.

The stakctrace above points to #format_from_params that uses #parse_nested_query underneath which can raise Rack::QueryParser::InvalidParameterError.

There is a little we can do here: wrap all calls to rack that can raise and re-raise a custom exception/reuse already existing. Meanwhile rescue_from can be used.

aside note, format_from_params implementation does not look optimal :)

nab0310 commented 3 years ago

That makes sense.

I believe the exception occurs on the first instance of the framework of trying to call Rack to parse the parameters.

What I really meant to say here is that if the mounting order of the included middleware changes, we might need to make the rescue_for fix more than once, but I suppose it's easier just to fix that if it ever does happen.

I can take a look at the #format_from_params method and raise a PR to try and fix the issue!

nab0310 commented 3 years ago

I did a little more investigation and found that if I send in the following request /api/ping.json?test=%9g, I get a different stack trace which validates my hunch that this happens the first time rack params is invoked.

The relevant bits of the stack trace:

/Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/request.rb:32:in `params'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/extensions/active_support/hash_with_indifferent_access.rb:19:in `build_params'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/request.rb:17:in `params'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/endpoint.rb:250:in `block in run'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/activesupport-6.0.3.4/lib/active_support/notifications.rb:182:in `instrument'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/endpoint.rb:247:in `run'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/endpoint.rb:322:in `block in build_stack'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/base.rb:36:in `call!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/base.rb:29:in `call'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/error.rb:39:in `block in call!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/error.rb:38:in `catch'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/error.rb:38:in `call!'
    /Users/nb051436/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/grape-1.5.1/lib/grape/middleware/base.rb:29:in `call'

Still trying to come up with a good solution, and it doesn't really feel like there's a good place to rescue all the instances of Rack::QueryParser::InvalidParameterError. And doing rescues at these specific points mean's I'm almost guaranteed to miss one. I'll keep looking though, I need a little bit to understand everything that's happening in here, its a big project 😄

dm1try commented 3 years ago

Still trying to come up with a good solution, and it doesn't really feel like there's a good place to rescue all the instances of Rack::QueryParser::InvalidParameterError. And doing rescues at these specific points mean's I'm almost guaranteed to miss one.

yeah, I agree.

Actually, my first feeling was that it should be caught on the layer above(web application server) but this comment proposes doing the validation in applications. I don't think this is a right solution though: such params does not follow HTTP specification. So proposing such solution is like arguing that any malformed HTTP request should be caught in your application. For example, my expectation that such error should be caught by Puma either with help from rack or without. After that Puma should return an error response with 400 status code to HTTP client. This requires pre-validation of HTTP data sent from clients. Optionally this behavior can be implemented as Rack middleware.