should not process form data if it is a GET method

[catatsuy]

I would like to make another suggestion on this issue. #1787

The implementation of Go is a problem. But in reality there are such HTTP clients.

I think rack should ignore them.

sinatra has implemented the following response to this problem.

https://github.com/sinatra/sinatra/pull/1743/files

I think this sinatra change is terrible. However, due to the implementation of rack, sinatra has no choice but to do this.

I propose the following changes to this issue.

rack/lib/rack/request.rb

Lines 470 to 475 in a7d5649

	def form_data?
	type = media_type
	meth = get_header(RACK_METHODOVERRIDE_ORIGINAL_METHOD) || get_header(REQUEST_METHOD)
	(meth == POST && type.nil?) || FORM_DATA_MEDIA_TYPES.include?(type)
	end

We will change the implementation of this function so that form_data? is false for GET methods. For example:

-         (meth == POST && type.nil?) || FORM_DATA_MEDIA_TYPES.include?(type)
+         (meth == POST && type.nil?) || (meth != GET && FORM_DATA_MEDIA_TYPES.include?(type))

We rarely send form data with the GET method.

However, this implementation would cause multiple rack tests to fail. This should be corrected in the test itself.

[sorah]

The following both arguments and present behaviours seem we're falling into inambiguity of the HTTP spec.

• handle_empty_content! causes problems with Go's http.Client #1787 (comment)
• net/http: http.Client's auto-redirect causes errors in Rack on Ruby by RFC 2046 golang/go#52519 (comment)

Chaging form_data? definition sounds dangerous for me, this will involve unintentional incompatibility - to workaround this issue I propose handling this at Rack::Request#POST to discard error when empty body is present for non-POST/PUT/PATCH requests.

https://github.com/rack/rack/blob/a7d56490fd2fb41de8c94ead2f63fbad71c5c489/lib/rack/request.rb#LL504C58-L504C58

[ioquatix]

Thanks I've created an issue here for further discussion: sinatra/sinatra#1844.

I'm not against being "liberal in what you accept" but this is a clear violation of the RFC.

So, I think we need a clear problem statement, example reproduction, a discussion from the Go team that they understand their behaviour is (?) in violation of the spec and they don't plan to fix it.

We could make a specific error for this case in Rack (i.e. something that would return a bad request). I'm more in favour of being strict in that regard than silently ignoring the error/protocol violation.

[ioquatix]

Okay, I've got a repro:

https://github.com/ioquatix/rack-go-http-client-redirect-bug

This includes a Ruby server, and client, and a Go client.

It shows the desired behaviour and it shows the buggy go behaviour.

I think we should file a bug report to Go with this example, but maybe someone else can check it first.

Also, after digging into the details of the issue, it would make sense to return a 400 Bad Request status if the request is malformed. Right now it's an internal server error as it tries to read the non-existent body in Rack. I'll make a separate PR for that and we can discuss the exact semantics. However, this does not resolve the buggy behaviour in Go's HTTP client.

[sorah]

Thank you!

In this case, the OP's use case is straightforward - 302 redirect on successful POST request, frequent pattern for web browser based apps.

I was curious how browser is handling this case and found: https://fetch.spec.whatwg.org/#http-redirect-fetch , so it is imambiguous just within the HTTP RFC specs?

[ioquatix]

I think the fetch spec is a fine baseline for how a redirect handling should work. Specifically I like that they've enumerated which headers to delete when changing from a post request to a get request.

A request-body-header name is a header name that is a byte-case-insensitive match for one of:

Content-Encoding
Content-Language
Content-Location
Content-Type

[ioquatix]

@catatsuy I think your middleware is fine if you want a compatibility fix, but just to be clear on my opinion, I think it would be better to return 400 Bad Request in such a case, since IIUC, it's prohibited for a GET request to have an entity body.

[ioquatix]

I've created a new issue for go: golang/go#57273.

[ioquatix]

I spent some more time looking at the rack implementation, and noticed that env['rack.input'] is non-optional even if no entity body is provided.

I think this is a little odd.

If there is no entity body, I feel like this should be unset (effectively nil).

That's different from "the request had an empty entity body". In this case, calling read should give an empty string. Someone uploading an empty file is different from a missing file.

I've created #1995 and will propose a PR.

[ioquatix]

I've also proposed #1996 which can be used like so:

      def parse_multipart(env, params = Rack::Utils.default_query_parser)
        unless io = env[RACK_INPUT]
          raise BadRequest, "missing multipart input"
        end

This to me feels like the correct solution - if we are expecting to parse a multipart body, and no body is given, this is a bad request.

See

rack/lib/rack/multipart.rb

Lines 17 to 18 in a7d5649

	def parse_multipart(env, params = Rack::Utils.default_query_parser)
	io = env[RACK_INPUT]

for the current implementation.

[dentarg]

Sounds good to me @ioquatix

[ioquatix]

@catatsuy @sorah can you please see the above linked PR and give your feedback.