Skip to content
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

net/http: http.Client's auto-redirect causes errors in Rack on Ruby by RFC 2046 #52519

Closed
catatsuy opened this issue Apr 24, 2022 · 6 comments
Closed

Comments

@catatsuy
Copy link

According to RFC 2046, the Body of a multipart request must not be empty. However, http.Client's auto-redirect uses the same Content-Type, which violates this rule.

Of course we can avoid the error by using http.ErrUseLastResponse. But this default behavior results in an error with Rack, which is very widely used in Ruby. Rack does not provide a workaround.

What is the correct solution?

refs: rack/rack#1787

@gopherbot gopherbot added this to the Proposal milestone Apr 24, 2022
@seankhliao
Copy link
Member

rack should use 307 if they want to maintain proper compatibility with user agents, see RFC 7231 Section 6.4.3.

@seankhliao seankhliao changed the title proposal: net/http: http.Client's auto-redirect causes errors in Rack on Ruby by RFC 2046 net/http: http.Client's auto-redirect causes errors in Rack on Ruby by RFC 2046 Apr 24, 2022
@seankhliao seankhliao removed this from the Proposal milestone Apr 24, 2022
@ioquatix
Copy link

@seankhliao Rack is a middleware for applications and does not force any redirect behaviour, it's up to applications to choose what status code to use.

Go is violating RFC 2046 section 5.1, which states "The body must then contain one or more body parts" (i.e. empty bodies are not allowed), assuming it is submitting an empty body with a multipart/form-data content type.

So, in any case, it's not Rack that is wrong here, it's following the RFC in terms of the error being generated. Either the app is wrong (should use 307?) or Go is wrong (should not send empty request with content type multipart/form-data).

@ioquatix
Copy link

@seankhliao quoting the section you reference:

       Note: For historical reasons, a user agent MAY change the request
      method from POST to GET for the subsequent request.  If this
      behavior is undesired, the 307 (Temporary Redirect) status code
      can be used instead.

It seems like either you should:

  1. Avoid doing an internal redirect and instead pass the result back to the user, or
  2. Perform a GET request for the subsequent request, or
  3. Perform the post to the updated URL with he same body and content-type.

Not sure what is best, but 307 doesn't seem like the right option, or at least from reading the RFC it looks like a 307 forces option 3 above? Interested to hear your interpretation.

@seankhliao
Copy link
Member

  1. is possible via http.Client.CheckRedirect
  2. is what is currently happening by default if CheckRedirect is unset
  3. is what happens if the server replies with 307

It's useful to consider an HTTP request as 2 layers: the transport protocol and the application protocol it carries. Here, Go follows the redirect according to the transport protocol, which Rack later marks as invalid at the application level. Based on your description of what Rack is, I think neither are wrong here.

The server has generated a response, that when followed will create an invalid request.
It sounds like you want the behaviour of 3, follow redirects, preserving the method and body, and the standard for that within HTTP (transport) is a 307.

@ioquatix
Copy link

I think the problem here is that the client is doing something invalid. Yes, the server might be sending the wrong status code, but if that's going to cause the client to do something incorrect, maybe it should be a warning/exception on the client side? As someone who has been on both sides of this, I guess it's best to follow the RFCs - i.e. the server shouldn't send invalid/impractical response codes and the client shouldn't redirect incorrectly.

@ioquatix
Copy link

Just FYI: "which Rack later marks as invalid at the application level" - rack is at the protocol level too in this case, it's totally up to the application to do the right thing here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants