hyper adds suprise Content-Length header

[bagder]

I did this

Build curl with current hyper, run test suite. The first merged commit with this failed CI seems to be dde4b38. The CI job gets the current git master from Hyper so it seems reasonable to assume that Hyper has changed in this time period.

Well established test cases fail.

TESTFAIL: These test cases failed: 13 17 1299 1613 

For example test 13:

 13: protocol FAILED:
--- log/check-expected	2024-04-15 14:52:05.976356850 +0000
+++ log/check-generated	2024-04-15 14:52:05.976356850 +0000
@@ -2,4 +2,5 @@
 Host: 127.0.0.1:44381[CR][LF]
 User-Agent: curl/8.8.0-DEV[CR][LF]
 Accept: */*[CR][LF]
+content-length: 0[CR][LF]
 [CR][LF]

### I expected the following

They should work

### curl/libcurl version

current git

### operating system

independent

[bagder]

Most probably caused by this: hyperium/hyper@172fdfa

[ckcr4lyf]

The initial issue reporter in reqwest (seanmonstar/reqwest#2240) raises a fair point, that as per the RFC a user-agent SHOULD include the Content-Length header. However, they also note, as per the definition of SHOULD, it is not a requirement (i.e. MUST).

This may be an intentional decision to honor the SHOULD by hyper, which would make it incompatible (unless cURL were to change this behavior).

[icing]

The initial issue reporter in reqwest (seanmonstar/reqwest#2240) raises a fair point, that as per the RFC a user-agent SHOULD include the Content-Length header. However, they also note, as per the definition of SHOULD, it is not a requirement (i.e. MUST).

This may be an intentional decision to honor the SHOULD by hyper, which would make it incompatible (unless cURL were to change this behavior).

It should "when the method defines a meaning for enclosed content". This is not the case for GET/HEAD/DELETE/CONNECT.

[ckcr4lyf]

It should "when the method defines a meaning for enclosed content". This is not the case for GET/HEAD/DELETE/CONNECT.

Right, hyper seems to have addressed it only for the methods where it SHOULD in hyperium/hyper#3630 (well looks like they forgot DELETE).

cURL (at least on CLI) does not include it, even for POST:

$ curl -v -X POST 127.0.0.1:6969
*   Trying 127.0.0.1:6969...
* Connected to 127.0.0.1 (127.0.0.1) port 6969
> POST / HTTP/1.1
> Host: 127.0.0.1:6969
> User-Agent: curl/8.7.1
> Accept: */*
>
* Request completely sent off

And on the receiver:

$ ncat -l 6969
POST / HTTP/1.1
Host: 127.0.0.1:6969
User-Agent: curl/8.7.1
Accept: */*

(or alternatively, hexdump):

$ ncat -l 6969 | xxd
00000000: 504f 5354 202f 2048 5454 502f 312e 310d  POST / HTTP/1.1.
00000010: 0a48 6f73 743a 2031 3237 2e30 2e30 2e31  .Host: 127.0.0.1
00000020: 3a36 3936 390d 0a55 7365 722d 4167 656e  :6969..User-Agen
00000030: 743a 2063 7572 6c2f 382e 372e 310d 0a41  t: curl/8.7.1..A
00000040: 6363 6570 743a 202a 2f2a 0d0a 0d0a       ccept: */*....

[icing]

-X POST is special in that this tells curl to forget all about the HTTP semantics and do a GET request with another method name. This is dangerous and highly discouraged and we politely beat people on the head where ever we encounter it.

[ckcr4lyf]

Looking at the failed tests, three of them are due to hyper including Content-Length for methods which should not expect a body (DELETE / OPTIONS)

• curl/tests/data/test13

  Line 28 in a1ecd0b

  http://%HOSTIP:%HTTPPORT/want/%TESTNUMBER -X DELETE

• curl/tests/data/test1299

  Line 37 in a1ecd0b

  --request-target '*' -X OPTIONS http://%HOSTIP:%HTTPPORT/ -H "Testno: %TESTNUMBER"

• curl/tests/data/test1613

  Line 38 in a1ecd0b

  --request-target '*' -X OPTIONS --proxy http://%HOSTIP:%HTTPPORT/ -H "Testno: %TESTNUMBER" http://www.example.org/

I guess hyper should not be applying the header for these methods.

The other failure is for a "custom" method (MOO):

curl/tests/data/test17

Line 47 in a1ecd0b

MOOO /that.site.com/%TESTNUMBER HTTP/1.1

, which is a bit more of a grey area.

[seanmonstar]

This is dangerous and highly discouraged and we politely beat people on the head where ever we encounter it.

But it sounds like being able to have that special behavior is on purpose, and curl would rather keep it. Right?

I can revert it in hyper, and say that "user agents" should do that themselves. Or I could leave it in by default with an option "I know what I'm doing". I suspect most want the behavior, so that's what I'm leaning towards.

[bagder]

@seanmonstar I think that's up for you and other hyper maintainers to decide. For curl use, I prefer as little "intelligence" as possible from hyper on matters such as this.

[seanmonstar]

This was reverted in hyper: hyperium/hyper#3633

A re-run of the curl CI should get the fix.

[bagder]

Thanks @seanmonstar !