Invert the methodRewriting option logic

[cheeseandcereal]

Describe the bug

• Node.js version: 12.18.0
• OS & version: Linux 5.6

According to the HTTP spec in RFC2616, HTTP clients should NOT redirect on 3XX response codes UNLESS the original request was a GET or a HEAD.

Got's default behavior causes it to repeat the original request (even if it's something like a POST) to the redirect URL, breaking very common webserver upload patterns.

Actual behavior

Common webserver behavior is to accept a POST to create some sort of resource, then reply with a 302 to point the user to the newly creating resource.

Got's default redirection behavior is to simply perform the same exact request again on some redirect code, regardless of the initial request method. So a POST request with the same body was then performed to the new URL from the location header of the redirect, which almost always will cause an error, as the redirected location generally will not accept a POST request, and certainly not expecting the same body.

Expected behavior

HTTP spec indicates that you SHOULD NOT automatically redirect any request that is not originally a GET or a HEAD request.

As for this specific scenario, it's worth noting that every single web browser, and a vast majority of HTTP clients treat a 302 after a POST as a redirection to then perform a GET on the 302, even though the original request was a POST.

At the very least by default got should not repeat requests to a redirected location on 3XX if the original request was not a GET or a HEAD, to stay compliant with the http spec by default.

Code to reproduce

const got = require('got');
async function main() {
  await got('https://httpbin.org/status/302', {
    method: 'POST',
    body: 'some data',
    retry: 0,
  });
}

main().catch(console.error)

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[Giotino]

According to the HTTP spec in RFC2616

That spec is outdated, take a look at RFC7231 section 6.4

[cheeseandcereal]

Yeah, I agree that's a newer version of the spec, but it's saying the same thing as the spec I linked. In fact it even clarifies on what I was saying.

First of all, in the main part of the redirection section 6.4 it says:

Automatic redirection needs to done with
care for methods not known to be safe, as defined in Section 4.2.1,
since the user might not wish to redirect an unsafe request.

Where a 'safe' method is defined:

Request methods are considered "safe" if their defined semantics are
essentially read-only; i.e., the client does not request, and does
not expect, any state change on the origin server as a result of
applying a safe method to a target resource.

In other words, a POST is generally not considered 'safe' and should not be replayed on redirection.

Additionally in the notes for the redirection section, it goes on to clarify exactly what I pointed out with this issue:

Early user agents split on whether the
method applied to the redirect target would be the same as the
original request or would be rewritten as GET.  Although HTTP
originally defined the former semantics for 301 and 302 (to match
its original implementation at CERN), and defined 303 (See Other)
to match the latter semantics, prevailing practice gradually
converged on the latter semantics for 301 and 302 as well.  The
first revision of HTTP/1.1 added 307 (Temporary Redirect) to
indicate the former semantics without being impacted by divergent
practice.  Over 10 years later, most user agents still do method
rewriting for 301 and 302; therefore, this specification makes
that behavior conformant when the original request is POST.

So the newer RFC is explicitly pointing out that changing the method to a GET on the redirection URL after a 301 or 302 from a POST is the officially recognized and conformant behavior.

[proton-ab]

@Giotino Please also note that term methodRewriting used for configuration value is incorrect; method rewriting occurs whenever client changes method for subsequent request after receiving 3xx response.

Early user agents split on whether the method applied to the redirect target would be the same as the original request or would be rewritten as GET

---

I agree that this should be modified. The meaning of methodRewriting should be changed to:

• true - send GET for unsafe methods on 3xx
• false - keep unsafe method on new location on 3xx

The default should be kept as true, that is got should perform rewriting of unsafe methods to GET on 3xx.

---

Additionally, given #1271 it is clear that this is not behavior expected by people using got and should be explicitly documented that FormData can not be reused on redirects. The example given on https://github.com/sindresorhus/got#form-data has potential for crash that might leave developer clueless as to its reason.

[szmarczak]

If we were to rename this or change the behavior, we would do so in Got 12. Honestly I don't know what to do about this. We have provided an example in the docs.

@sindresorhus Have any ideas?

[sindresorhus]

I agree with @proton-ab's proposal. We should get this fixed in Got 12.

[cheeseandcereal]

It's worth noting that you do not want to rewrite unsafe methods by default for all 3XX responses. Specifically, 307 and 308 were explicitly created to indicate that the client should repeat the original (potentially unsafe) method on the new location.

You'll probably only want to rewrite 'unsafe' methods by default for 301, 302, and 303, as that most closely follows the HTTP spec as well as conventions set by most other HTTP clients and web browsers.

[szmarczak]

Fixed in 51d88a0