ruby-grape / ruby-grape/grape

Invalid % Encoding

Open
#2,159 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug?
Dominant language
Ruby
Stars
10k
Forks
1.2k
Avg merge
14h 38m
Merged PRs (30d)
92

Description

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!

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the request /api/ping?test=%9g using the linked minimal Grape app, then inspect grape/middleware/formatter.rb where the stack trace shows query parsing begins. Determine the expected handling for Rack::QueryParser::InvalidParameterError and verify that the completed behavior returns a 400 response rather than an uncaught exception.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
api
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.