BufferingClientHttpRequestFactory should not set content length for empty request body about spring-framework

[bclozel]

@nhmarujo it's fixed now in 6.1.7-SNAPSHOTs and it will be released with next month's maintenance release. Thanks for your report!

[onjik]

Hmm....
The RFC document says.

Any Content-Length greater than or equal to zero is a valid value.
Section 4.4 describes how to determine the length of a message-body
if a Content-Length is not given.
https://datatracker.ietf.org/doc/html/rfc2616#section-14.13

So I'm not sure if it's right to change the preferences like this. Would it be too complicated to eventually give you an interceptor to tamper with a request, or a way to customize a policy?

[nhmarujo]

Hi @onjik , thank you for your prompt answer.

I do understand your point of view that Content-Length: 0 is compliant with the RFC. But I also think that the RFC doesn't state that Content-Length is mandatory for POST. I think there is a conceptual difference between a POST with no body and a POST with an empty body. I would expect the former to have neither Content-Type or Content-Length, while the latter I would expect to have a Content-Type and either a Content-Length or a Transfer-Encoding (assuming HTTP 1.1 as minimum for reference).

I have to say that I'm far from an expert on the matter, but I don't get why Spring is enforcing that default when clearly no body was set (not even Content-Type is being sent). What is the issue that default is trying to address? And why is this default not present in WebClient behaviour?

Last - yes I can absolutely add an interceptor as a workaround. In fact this is what I'm actually targeting as a temporary fix:

@Bean
public RestClientCustomizer restClientCustomizer() {
    return restClientBuilder -> restClientBuilder.requestInterceptor((request, body, execution) -> {
        if (body.length == 0 && request.getHeaders().getContentType() == null) {
            request.getHeaders().remove(CONTENT_LENGTH);
        }
        return execution.execute(request, body);
    });
}

That said, it feels wrong to me to do this. I was hoping that the fix I was proposing (or following similar idea) could be done at the framework level (assuming we reach a conclusion that it is ok and makes sense).

Let me know your thoughts.

Thank you 🙂

[onjik]

Thank you for your kind and detailed response! @nhmarujo

I think you're right.

There's no provision that you must attach a content-length header when sending a POST request. I don't think it's the right direction to enforce it at the framework level.
Rather, if the framework doesn't force it, the user is given the option to add that header if they want, and not add it if they don't.

[nhmarujo]

Hi @onjik . Thank you!

If we followed something like my suggestion I think it would be the best option, because:

1. if the user didn't set the Content-Length but there is a body with size greater than 0, the header will be added with proper value
2. if the user did set the Content-Length explicitly, that one will be respected
3. if the user didn't set the Content-Length and body as not length, no header will be added

I'm not sure if a variation of this would make more sense in regards of use case 3. to actually check if Content-Type is set to take that decision in a more clear way (i.e. Content-Type presence would imply that a body is expected or not). So that actually would look like:

if (headers.getContentType() != null && headers.getContentLength() < 0) {
	headers.setContentLength(bytes.length);
}

What you think?

[onjik]

@nhmarujo

As you mentioned in the first comment, I think following code is already checking if there is explicit content-length header.

	/**
	 * Return the length of the body in bytes, as specified by the
	 * {@code Content-Length} header.
	 * <p>Returns -1 when the content-length is unknown.
	 */
	public long getContentLength() {
		String value = getFirst(CONTENT_LENGTH);
		return (value != null ? Long.parseLong(value) : -1);
	}

If there is no explicit content-length header, it returns -1. So following code checks if there is explicit content-length header.

if (headers.getContentLength() < 0) {
	headers.setContentLength(bytes.length);
}

How about this code

if (headers.getContentLength() == -1 && bytes.length > 0) {
	headers.setContentLength(bytes.length);
}

It's similar to the code you provided for the first time, but if threre is explicit content-length header, don't check bytes.length. (There's no big difference in performance 🤣)

Not sure if == -1 is better.

What you think?

[nhmarujo]

Hi @onjik.

Yes, it is basically the same as my first proposal. The reason why I suggested < 0 instead of == -1 is because I saw a commit on which it seems to be the preferred way to check it within the framework and they actually wanted to make it consistent (I take it was a team decision).

I think both proposals I made are valid, but the latter one might be more accurate (not sure), as the assumption is that if a Content-Type is set, then there is the expectation to have a body and therefore the Content-Length must be set if absent (even if the body is empty and in that case it would be set to 0).

Any concerns about this last proposal or any reason why you would prefer the first one?

Thanks

[onjik]

@nhmarujo
You're very meticulous! < 0 is better

I've been thinking about counter examples a lot, and I think this is the case.

HTTP/1.1 200 OK
Content-Type: text/plain
Transfer-Encoding: chunked

7\r\n
Mozilla\r\n
11\r\n
Developer Network\r\n
0\r\n
\r\n

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding

If you use such a chunked way, Content-Length is not used even if there is a Content-Type.

So I think it's bad to guess based on the content-type header.

Let me know your thoughts.

Thanks!!

[nhmarujo]

Hi @onjik .

Thank you, I think that is a very good point!

I think the possible options are:

• No body -> No Content-Type, Content-Length or Transfer-Encoding
• With body:
  • Content-Type and Content-Length
  • Content-Type and Transfer-Encoding

With this in mind, right now I don't even know what the right approach is anymore. I mean, it does seem that what the framework is doing right now is not the best approach, but I also think what we discussed so far doesn't cover all cases.

I was thinking about something like this:

f (headers.getContentType() != null && headers.getTransferEncoding() == null && headers.getContentLength() < 0) {
	headers.setContentLength(bytes.length);
}

But honestly I'm not sure if the Transfer-Encoding was determined at that stage (that seems more like a lower-level decision).

Any thoughts? 😅

[bclozel]

The "Content-Length" header is not set by Spring, but actually by the JDK HTTP client itself.

Executing the following code

RestClient restClient = RestClient.create();
ResponseEntity<String> response = restClient.post().uri("http://localhost:8080/test").retrieve().toEntity(String.class);

Is equivalent to selecting the JdkClientHttpRequestFactory, using the HttpClient introduced in Java 11:

RestClient restClient = RestClient.builder().requestFactory(new JdkClientHttpRequestFactory()).build();

Using another client, like the httpcomponents one, does not send a Content-Length request header in this case:

RestClient restClient = RestClient.builder().requestFactory(new HttpComponentsClientHttpRequestFactory()).build();

I guess here the difference is that the JdkClientHttpRequestFactory is selected by default, whereas RestTemplate was using the SimpleClientHttpRequestFactory (which is using Java's HttpURLConnection). If you believe the behavior is invalid, please raise an issue to OpenJDK.

Thanks!

[nhmarujo]

Hi @bclozel . Thank you for your reply. I do understand that different client implementations underneath makes a difference and in fact on our case we are using Apache Http Components, not the JDK one that you guessed 🙂

But on the other hand I think you are missing something - if you check in this line (just like I shared on the issue description), that is actually being set there by the framework and I did actually debug and validated it was being added there.

Furthermore I can remove it with an interceptor later, which I think I wouldn't be able to if it was the underlying http client (whichever it was) doing it.

Please let me know your thougths 🙂

Thanks

[bclozel]

Thanks for your comment @nhmarujo , I guess I got lost in the discussion around HttpHeaders, which now has its own issue with #32660.

[nhmarujo]

You're welcome @bclozel and thank you for reopening the issue. Do you have any view on the matter? 🙂

P.S.: Thank you for also editing the title to something much clearer!

[nhmarujo]

Thank you so much @bclozel !

[Mario-Eis]

This broke our whole system and we will not be able to upgrade spring from now on. Because our central NoSql database system expects a content-length set, even when there is no body present. Thats a critical issue for us right now.

[bclozel]

After discussing this more in-depth, the team decided to reconsider this issue and maybe revert this change.
See #32678 for more details.

@nhmarujo Can you elaborate on your use case? Is the server a well-known web server? Is this a custom implementation that is setting this header?
Also, the error message is a bit strange HTTP Error 411. The request must be chunked or have a content length. This error code says the opposite: the server requires a "Transfer-Encoding" or "Content-Length" header. Here, setting "Content-Length: 0" is somehow raising this error but not when it's missing?

@Mario-Eis Thanks for your feedback. This means you have tested this change in production or in a testing environment? Can you share the name of the database product and the error that's raised?

[nhmarujo]

Hi @bclozel. Hard for me to say exactly what the provider is using, but looking at our access logs I don't see the header Server: "cloudflare" on the response for that failed request. I also see another header Report-To: "{"endpoints":[{"url":"https:\/\/a.nel.cloudflare.com\/report\/v4?s=eSJjE3hVNEyXU9lMO9i9pFeNrJSSSMZ2KQSSl0ksgIdJIKygolfg5vUTdoprUUkd8GYFw7vJScF4fzt5blewS6s4fBFqBiL2BqLafUKe7Qrl%2BOZV4itD2V0wtFum2HamY6L%2FQ6JCntYvBA%3D%3D"}],"group":"cf-nel","max_age":604800}".

Hard to say much more.

And yes, this error was not being raised when Content-Length is absent, which is what was happening when we had WebClient

[bclozel]

While the discussion around "no message body / empty message body" is relevant to the RFC, it seems that most clients send this "Content-Length: 0" request header for POST requests and that we're not in a position to consistently make the difference between those cases at the Framework level.

On top of that, the use case reported here is contradictory: the server complains about a missing "Content-Length" header when it's actually present. After discussing with the team, we're going to revert this change.

[bclozel]

Reverted in 64b0283.