New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post request containing % breaks Rack::Test #1378

Open
marcusmichaely opened this Issue Jan 8, 2018 · 4 comments

Comments

Projects
None yet
5 participants
@marcusmichaely

marcusmichaely commented Jan 8, 2018

Here is a minimal example to reproduce bug. It doesnt matter if the % sign is part of a bigger payload or a single sign:

require 'sinatra'

class MyApp < Sinatra::Base
  post '/foo/bar' do
  end
end

require 'rspec'
require 'rack/test'

RSpec.describe 'percent_bug' do
  include Rack::Test::Methods

  def app
    MyApp.new
  end

  shared_examples :returns_200 do
    it 'should return 200' do
      post '/foo/bar', payload
      expect(last_response.status).to eq(200)
    end
  end

  context 'non-problematic payload' do
    let(:payload) { 'foobar' }
    include_examples :returns_200
  end

  context 'problematic payload' do
    let(:payload) { '%' }
    include_examples :returns_200
  end
end
@rdalverny

This comment has been minimized.

Show comment
Hide comment
@rdalverny

rdalverny Jan 8, 2018

It's triggered by URI.decode_www_form_component, through Rack::Utils.unescape, Rack::QueryParser#parse_nested_query:

if your payloads contains a %, the parser validates for a valid percent encoded string.
So would you need a percent sign in the payload, you should encode it, as %25.

rdalverny commented Jan 8, 2018

It's triggered by URI.decode_www_form_component, through Rack::Utils.unescape, Rack::QueryParser#parse_nested_query:

if your payloads contains a %, the parser validates for a valid percent encoded string.
So would you need a percent sign in the payload, you should encode it, as %25.

@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Feb 4, 2018

Member

Can we close this issue?

Member

jkowens commented Feb 4, 2018

Can we close this issue?

@jreinert

This comment has been minimized.

Show comment
Hide comment
@jreinert

jreinert Feb 5, 2018

I think the issue is rather that there is an inconsistency between Rack::Test and normal execution. Running the example above one can curl the route with --data-binary '%' and it processes without errors. Also I don't see the point in using the query parser at all with request bodies that aren't application/www-form-urlencoded.

jreinert commented Feb 5, 2018

I think the issue is rather that there is an inconsistency between Rack::Test and normal execution. Running the example above one can curl the route with --data-binary '%' and it processes without errors. Also I don't see the point in using the query parser at all with request bodies that aren't application/www-form-urlencoded.

@rdalverny

This comment has been minimized.

Show comment
Hide comment
@rdalverny

rdalverny Feb 5, 2018

@jreinert good point, I had not seen it that way.

If you specify the content type to skip this parser, it works:

     it 'should return 200' do
+      header 'Content-Type', 'application/octet-stream'
       post '/foo/bar', payload

But, from my tests, curl defaults to sending application/x-www-form-urlencoded (and so does Rack::Test):

$ curl -v -XPOST --data-binary '%' localhost:4567/
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 4567 (#0)
> POST / HTTP/1.1
> Host: localhost:4567
> User-Agent: curl/7.47.0
> Accept: */*
> Content-Length: 1
> Content-Type: application/x-www-form-urlencoded
...

Removing the content type sent ($ curl -v -XPOST -d '%' -H'Content-Type:' localhost:4567/foo/bar/) produces the same result (error 400, and verifying, that means Rack defaults to parsing the request as a www-form-urlencoded).

So that would be 2 issues actually:

  • Rack sending application/x-www-form-urlencoded when no default type is provided (so above rspec test fails);
  • Rack assuming request content type to be application/x-www-form-urlencoded when not explicitly set (so Sinatra failing to parse above request, if there is no specified content type).

Or did I miss something?

Quoting https://tools.ietf.org/html/rfc7231#section-3.1.1.5:

If a Content-Type header field is not present, the recipient
MAY either assume a media type of "application/octet-stream"
([RFC2046], Section 4.5.1) or examine the data to determine its type.

So, that points back to Rack - not sure its helps much, but here's a sample of similar issues:

And so far, it seems it's up to the app/tests to manage this:

  • server-side with a middleware (see #804 (comment) for instance)
  • client-side, by specifying the right content type header.

I'd close the issue, but maybe that could be a case for an addition to the README, in the Mime Types section? (not sure)

rdalverny commented Feb 5, 2018

@jreinert good point, I had not seen it that way.

If you specify the content type to skip this parser, it works:

     it 'should return 200' do
+      header 'Content-Type', 'application/octet-stream'
       post '/foo/bar', payload

But, from my tests, curl defaults to sending application/x-www-form-urlencoded (and so does Rack::Test):

$ curl -v -XPOST --data-binary '%' localhost:4567/
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 4567 (#0)
> POST / HTTP/1.1
> Host: localhost:4567
> User-Agent: curl/7.47.0
> Accept: */*
> Content-Length: 1
> Content-Type: application/x-www-form-urlencoded
...

Removing the content type sent ($ curl -v -XPOST -d '%' -H'Content-Type:' localhost:4567/foo/bar/) produces the same result (error 400, and verifying, that means Rack defaults to parsing the request as a www-form-urlencoded).

So that would be 2 issues actually:

  • Rack sending application/x-www-form-urlencoded when no default type is provided (so above rspec test fails);
  • Rack assuming request content type to be application/x-www-form-urlencoded when not explicitly set (so Sinatra failing to parse above request, if there is no specified content type).

Or did I miss something?

Quoting https://tools.ietf.org/html/rfc7231#section-3.1.1.5:

If a Content-Type header field is not present, the recipient
MAY either assume a media type of "application/octet-stream"
([RFC2046], Section 4.5.1) or examine the data to determine its type.

So, that points back to Rack - not sure its helps much, but here's a sample of similar issues:

And so far, it seems it's up to the app/tests to manage this:

  • server-side with a middleware (see #804 (comment) for instance)
  • client-side, by specifying the right content type header.

I'd close the issue, but maybe that could be a case for an addition to the README, in the Mime Types section? (not sure)

@namusyaka namusyaka added this to the v2.0.2 milestone Feb 6, 2018

@namusyaka namusyaka modified the milestones: v2.0.2, v2.0.3, v2.0.4 Jun 5, 2018

@namusyaka namusyaka modified the milestones: v2.0.4, v2.0.5 Sep 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment