org.springframework.web.client.HttpMessageConverterExtractor#extractData fails to detect empty body when content-length header is missing

[bruce-stewart]

spring-web 5.1.3.RELEASE:

The org.springframework.web.client.HttpMessageConverterExtractor#extractData results in a NullPointerException if:

1. The HttpStatus of the org.springframework.http.client.ClientHttpResponse is 401
2. The org.springframework.http.client.ClientHttpResponse#getBody is null.
3. The response does not contain a content-length header.

The following test can be added to org.springframework.web.client.HttpMessageConverterExtractorTest to verify this behaviour:

   @Test
    public void missingContentLengthHeader() throws IOException {
        HttpMessageConverter<?> converter = mock(HttpMessageConverter.class);
        HttpHeaders responseHeaders = new HttpHeaders();
        extractor = new HttpMessageConverterExtractor<>(String.class, createConverterList(converter));
        given(response.getRawStatusCode()).willReturn(HttpStatus.valueOf(401).value());
        given(response.getHeaders()).willReturn(responseHeaders);

        Object result = extractor.extractData(response);
        assertNull(result);
    }

The org.springframework.web.client.MessageBodyClientHttpResponseWrapper#hasMessageBody incorrectly returns true which causes org.springframework.web.client.MessageBodyClientHttpResponseWrapper#hasEmptyMessageBody to throw the NPE.

public T extractData(ClientHttpResponse response) throws IOException {
...
		if (!responseWrapper.hasMessageBody() || responseWrapper.hasEmptyMessageBody()) {
...

This can be fixed in org.springframework.web.client.MessageBodyClientHttpResponseWrapper#hasMessageBody by changing this:

if (getHeaders().getContentLength() == 0) {
   return false;
} 

to this:

if (getHeaders().getContentLength() <= 0) {
   return false;
} 

When the headers do not contain a content-length header, getHeaders().getContentLength() returns -1.

[bruce-stewart]

The line that requires chaging IMHO is here:

spring-framework/spring-web/src/main/java/org/springframework/web/client/MessageBodyClientHttpResponseWrapper.java

Line 66 in 0db4c22

if (getHeaders().getContentLength() == 0) {

[rstoyanchev]

A response can be set to chunked transfer-encoding, and does need have to have a Content-Length header to have a body. So a potential fix would to just check the body for null.

That said the getBody is not supposed to return null:

spring-framework/spring-web/src/main/java/org/springframework/http/HttpInputMessage.java

Lines 34 to 39 in 4fdfa42

	/**
	* Return the body of the message as an input stream.
	* @return the input stream body (never {@code null})
	* @throws IOException in case of I/O errors
	*/
	InputStream getBody() throws IOException;

What's the actual use case?

[bruce-stewart]

In the case of a 401 response, the response will not have a body and it will also not have a content length header. The org.springframework.web.client.MessageBodyClientHttpResponseWrapper#hasMessageBody method correctly checks the content-length header so that the getBody() method will NOT be called, but there's a mis-match in logic between the line of code that checks for 'contentLength == 0' and the line of code in the HttpHeaders class that returns a content-length value of -1 when the content-length header is missing. The mismatch can be rectified by changing the '== 0' to '<=0' so that a content-length value of either 0 or -1 causes the getBody() method to NOT be called, avoiding the NPE.

The unit test above demonstrates the issue.

Does that make sense?

[bruce-stewart]

Perhaps I should mention that I'm specifically developing a consumer of a rest api which uses a feign client. In a traditional web application, a 401 response might have a message body (and a corresponding content-length header), but this is not necessarily the case in a REST api.

[bruce-stewart]

A response can be set to chunked transfer-encoding, and does need have to have a Content-Length header to have a body. So a potential fix would to just check the body for null.

That said the getBody is not supposed to return null:
spring-framework/spring-web/src/main/java/org/springframework/http/HttpInputMessage.java

spring-framework/spring-web/src/main/java/org/springframework/http/HttpInputMessage.java

Lines 34 to 39 in 4fdfa42

	/**
	* Return the body of the message as an input stream.
	* @return the input stream body (never {@code null})
	* @throws IOException in case of I/O errors
	*/
	InputStream getBody() throws IOException;

What's the actual use case?

If the getBody() method is suppposed to return the InputStream or throw an IOException (instead of returning null), how would that work with chunked transfer-encoding and a missing content-length header? Are you saying that such responses rely on the fact that getHeaders().getContentLength() returns -1 which allows the getBody() method to be invoked.

[rstoyanchev]

I'm specifically developing a consumer of a rest api which uses a feign client.

Right so it is the ClientHttpResponse adapter for Feign that may return null:
https://github.com/spring-cloud/spring-cloud-openfeign/blob/6d4686624e732247cab4ede05ef453783fd6f257/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringDecoder.java#L101

This can be considered a bug in Spring Cloud since the contract for getBody clearly says it should never return null. That said we should make MessageBodyClientHttpResponseWrapper#hasEmptyMessageBody more defensive based on this feedback.

A check for value 0 or -1 is not a solution in this because -1 means there is no Content-Length which does not mean there is no body.

[bruce-stewart]

I'm specifically developing a consumer of a rest api which uses a feign client.

Right so it is the ClientHttpResponse adapter for Feign that may return null:
https://github.com/spring-cloud/spring-cloud-openfeign/blob/6d4686624e732247cab4ede05ef453783fd6f257/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringDecoder.java#L101

Ok - I see that now. The adapter class could/should check the response.body() before invoking the body#asInputStream() method and should either throw an IOException if the body is null or return an empty stream which would be picked up by the MessageBodyClientHttpResponseWrapper#hasEmptyMessageBody method. Is that more-or-less correct?

This can be considered a bug in Spring Cloud since the contract for getBody clearly says it should never return null. That said we should make MessageBodyClientHttpResponseWrapper#hasEmptyMessageBody more defensive based on this feedback.

A check for value 0 or -1 is not a solution in this because -1 means there is no Content-Length which does not mean there is no body.

That makes sense. The return value of MessageBodyClientHttpResponseWrapper#hasMessageBody is basically unreliable at this stage, but I'm afraid I'm not sure what the best answer or solution is.