Skip to content

Conversation

imzhoukunqiang
Copy link
Contributor

@imzhoukunqiang imzhoukunqiang commented Sep 1, 2024

Hi maintainers, while working with the framework, I discovered a bug that I believe warrants attention.

In the org.springframework.http.client.InterceptingClientHttpRequest , the request body may be changed by interceptors, but Content-Length in the request headers didn't. This may cause the server receive the incorrect request body.
I created a simple demo to show this bug . DEMO

The fix for this is to reset the Content-Length before sending the request at org.springframework.http.client.InterceptingClientHttpRequest.InterceptingRequestExecution#execute

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 1, 2024
@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Sep 1, 2024
@bclozel
Copy link
Member

bclozel commented Sep 1, 2024

I am not sure we should do this. Do we really set the content length header in all cases and for all converters? If an interceptor is changing the request body I would argue it needs to change the content type as well as the content length of it was set already.
I think some http clients can send bodies with chunked transfer encoding.

@imzhoukunqiang
Copy link
Contributor Author

I am not sure we should do this. Do we really set the content length header in all cases and for all converters? If an interceptor is changing the request body I would argue it needs to change the content type as well as the content length of it was set already. I think some http clients can send bodies with chunked transfer encoding.

I agree with your point of view. I'm not sure whether setting Content-Length is the responsibility of the user or the framework. However, typically when we use RestTemplate, we do not set Content-Length, and it is handled by the framework.

And if after executing the interceptor chain, the Content-Length exists in the request header and does not equal body.length, should it be corrected to the correct value?

@bclozel
Copy link
Member

bclozel commented Sep 1, 2024

Your description makes sense. However, isn't your PR setting it in all cases?

@imzhoukunqiang
Copy link
Contributor Author

Your description makes sense. However, isn't your PR setting it in all cases?

Updated.

@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 2, 2024
@bclozel bclozel self-assigned this Sep 2, 2024
@bclozel bclozel added this to the 6.2.0-RC1 milestone Sep 2, 2024
@bclozel bclozel closed this in aca2649 Sep 2, 2024
@bclozel
Copy link
Member

bclozel commented Sep 2, 2024

Thanks for your contribution @imzhoukunqiang , this is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants