Bad curl debug logs with POST request and --location

[jcamiel]

With this file (follow redirection from a POST request)

POST https://foo.com
[Options]
location: true
HTTP 200
[Asserts]
url == "https://bar.com"

Hurl 4.3.0 implements curl's behaviour:

From https://curl.se/docs/manpage.html#-L:

When curl follows a redirect and if the request is a POST, it sends the following request with a GET if the HTTP response was 301, 302, or 303. If the response code was any other 3xx code, curl resends the following request using the same unmodified method.

In Hurl debugs logs, the curl command is:

* Request can be run with the following curl command:
* curl --request POST --location 'https://foo.com'

This command will NOT make a POST request followed by a GET redirection

This command will force all methods to be POST and so does not corresponds to the behavior of Hurl file.

As https://curl.se/docs/manpage.html#-X, --location and --request/-X does not well play together:

The method string you set with -X, --request is used for all requests, which if you for example use -L, --location may cause unintended side-effects when curl does not change request method according to the HTTP 30x response codes - and similar.

In https://daniel.haxx.se/blog/2015/09/11/unnecessary-use-of-curl-x/ the "right" curl command in this case should be:

* Request can be run with the following curl command:
* curl --data '' --location 'https://foo.com'

[ashishajr]

Hey @jcamiel, I'd love to take this up, I've raised a PR which should hopefully address this

[jcamiel]

Hi @ashishajr thanks for the PR, for the moment we have no time to work on contributor PRs. We're going to update our contributing guide to reflect this.

[ashishajr]

Hi @ashishajr thanks for the PR, for the moment we have no time to work on contributor PRs. We're going to update our contributing guide to reflect this.

Ah okay, my bad, I misunderstood you comment earlier that only bug related PRs would be considered. I'll close my PR for now then. Would this be till 6.1.0 is released? Hope you guys aren't too overworked, will have to go find some other project till I can contribute to hurl again 😅

[ashishajr]

Hey @jcamiel, I'd like to take this up next.

I'd initially looked into this bug and the solution I arrived at that time was to add a check in this part of the code

hurl/packages/hurl/src/http/curl_cmd.rs

Lines 281 to 303 in 566c454

	impl Method {
	/// Returns the curl args for HTTP method, given the request has a body or not.
	fn curl_args(&self, has_body: bool) -> Vec<String> {
	match self.0.as_str() {
	"GET" => {
	if has_body {
	vec!["--request".to_string(), "GET".to_string()]
	} else {
	vec![]
	}
	}
	"HEAD" => vec!["--head".to_string()],
	"POST" => {
	if has_body {
	vec![]
	} else {
	vec!["--request".to_string(), "POST".to_string()]
	}
	}
	s => vec!["--request".to_string(), s.to_string()],
	}
	}
	}

From what I understood there could be these cases for a POST followed by GET redirection

• POST with body -> --data gets added so no need to add explicit options to curl
• POST with no body -> --data '' needs to be explicitly added as first request is a POST but subsequent redirections are GET

impl Method {
    /// Returns the curl args for HTTP method, given the request has a body or not.
    fn curl_args(&self, has_body: bool, follow_location: bool) -> Vec<String> {
        match self.0.as_str() {
            "GET" => {
                if has_body {
                    vec!["--request".to_string(), "GET".to_string()]
                } else {
                    vec![]
                }
            }
            "HEAD" => vec!["--head".to_string()],
            "POST" => {
                if has_body {
                    vec![]
                } else if follow_location {
                    vec!["--data".to_string(), "''".to_string()]
                } else {
                    vec!["--request".to_string(), "POST".to_string()]
                }
            }
            s => vec!["--request".to_string(), s.to_string()],
        }
    }
}

Does this implementation work or is there a better way to go about this?

[jcamiel]

Hi @ashishajr yes it seems OK to me we have to make a particular case for POST + --location. We can also add this comment because it's not really trivial why we're going to do this:

            "POST" => {
              // In <https://curl.se/docs/manpage.html#-X>, `--location` and `--request/-X` does not well play together:
              //
              // > The method string you set with -X, --request is used for all requests, which if you for example use -L, 
              // > --location may cause unintended side-effects when curl does not change request method according to the 
              // > HTTP 30x response codes - and similar.
              //
              // When we use `--request POST` with curl, we're telling curl to make POST requests for every request. This
              // can interfere with `--location` option that can make GET requests following the initial POST. So, in the 
              // case of a POST request with `--location` option, we don't force the method to let curl decides the right 
              // method of the redirection steps.
                if has_body {
                    vec![]
                } else if follow_location {
                    vec!["--data".to_string(), "''".to_string()]
                } else {
                    vec!["--request".to_string(), "POST".to_string()]
                }
            }

[ashishajr]

Sounds good, @jcamiel! I'll add the comment in this and raise PR with the modified integration tests