Exception when JSON body contains %

[Jammjammjamm]

One of our users reported an error which occurred when they were submitting an AJAX-based form. After some investigation, we discovered that the error was caused by the % character in the JSON body of the POST.

Here's a basic spec to demonstrate, it occurs with or without the json body parser:

require 'hanami/middleware/body_parser'
require 'hanami/router'
require 'rack/test'
require 'json'

RSpec.describe 'routing wildcard behavior' do
  include Rack::Test::Methods

  let(:app) do
    router =
      Hanami::Router.new do
        post '/abc', to: ->(env) { [200, {}, ['resources/:id']]}
      end
    Hanami::Middleware::BodyParser.new(router, :json)
  end

  it 'responds to /abc' do
    post '/abc', JSON.generate('foo' => '%'), 'CONTENT_TYPE' => 'application/json'
    expect(last_response.status).to eq(200)
  end
end

The result:

ᐅ be rspec router_spec.rb
F

Failures:

  1) routing wildcard behavior responds to /abc
     Failure/Error: post '/abc', JSON.generate('foo' => '%'), 'CONTENT_TYPE' => 'application/json'

     Rack::QueryParser::InvalidParameterError:
       invalid %-encoding ({"foo":"%"})
     # ./router_spec.rb:18:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # ArgumentError:
     #   invalid %-encoding ({"foo":"%"})
     #   ./router_spec.rb:18:in `block (2 levels) in <top (required)>'

Finished in 0.01218 seconds (files took 0.09398 seconds to load)
1 example, 1 failure

Stack trace:

~/.asdf/installs/ruby/3.1.2/lib/ruby/3.1.0/uri/common.rb:341:in `decode_www_form_component': invalid %-encoding ({"foo":"%"}) (Rack::QueryParser::InvalidParameterError)
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/utils.rb:57:in `unescape'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/query_parser.rb:159:in `unescape'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/query_parser.rb:73:in `block (2 levels) in parse_nested_query'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/query_parser.rb:73:in `map!'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/query_parser.rb:73:in `block in parse_nested_query'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/query_parser.rb:72:in `each'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/query_parser.rb:72:in `parse_nested_query'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/utils.rb:102:in `parse_nested_query'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/hanami-router-2.0.0.rc1/lib/hanami/router.rb:923:in `_params'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/hanami-router-2.0.0.rc1/lib/hanami/router.rb:108:in `call'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/hanami-router-2.0.0.rc1/lib/hanami/middleware/body_parser.rb:54:in `call'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-test-2.0.2/lib/rack/test.rb:358:in `process_request'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-test-2.0.2/lib/rack/test.rb:165:in `custom_request'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/rack-test-2.0.2/lib/rack/test.rb:114:in `post'
	from ~/.asdf/installs/ruby/3.1.2/lib/ruby/3.1.0/forwardable.rb:238:in `post'
	from ~/code/testing/router_spec.rb:20:in `block (2 levels) in <top (required)>'

[jodosha]

@Jammjammjamm Sorry but there is nothing that we can do.
The bug is on Rack side.

Steps to isolate the problem in an IRb session:

irb(main):001:0> ::Rack::Utils.parse_nested_query(%({"foo": "%"}))
/Users/jodosha/.rubies/ruby-3.1.0/lib/ruby/3.1.0/uri/common.rb:341:in `decode_www_form_component': invalid %-encoding ({"foo": "%"}) (Rack::QueryParser::InvalidParameterError)
# ...

[jodosha]

@Jammjammjamm About the format of the spec above, I suggest to use proper Rack tooling to build your app:

# frozen_string_literal: true

require "hanami/middleware/body_parser"
require "hanami/router"
require "rack/builder"
require "rack/test"
require "json"

RSpec.describe "routing wildcard behavior" do
  include Rack::Test::Methods
  let(:app) do
    r = router
    Rack::Builder.new do
      use Hanami::Middleware::BodyParser, :json
      run r
    end
  end

  let(:router) do
    Hanami::Router.new do
      post "/abc", to: ->(_env) { [200, {}, ["resources/:id"]] }
    end
  end

  it "responds to /abc" do
    post "/abc", JSON.generate("foo" => "%"), "CONTENT_TYPE" => "application/json"
    expect(last_response.status).to eq(200)
  end
end

[Jammjammjamm]

I don't think this is a rack problem. The documentation for Rack::Utils.parse_nested_query says:

parse_nested_query expands a query string into structural types.

router.rb is explicitly calling it on things which aren't query strings, a JSON payload in this example. I've tried this with a bunch of different rack versions going back to 2.0.1, and they all exhibit the same behavior. The thing that broke this behavior is #217. If you point hanami-router at the previous commit (a5b6a4417a3692b47bf1ae855dedebdd9175a793), the spec passes.

This completely breaks non-form-encoded requests and seems like a critical bug.

[wuarmin]

@Jammjammjamm thanks for pointing this out. Can you reopen the issue?

[Jammjammjamm]

It doesn't look like I have permission to reopen issues.

[jodosha]

@Jammjammjamm @wuarmin Could you please try with the patch here? #240

[Jammjammjamm]

That seems to have resolved the issue for us.