Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ProxyExchange fails with "IOException: insufficient data written" with boot 3.2 #3154

Open
jkuipers opened this issue Nov 30, 2023 · 17 comments · May be fixed by #3313
Open

ProxyExchange fails with "IOException: insufficient data written" with boot 3.2 #3154

jkuipers opened this issue Nov 30, 2023 · 17 comments · May be fixed by #3313
Labels

Comments

@jkuipers
Copy link
Contributor

jkuipers commented Nov 30, 2023

The ProxyExchange type in spring-cloud-gateway-mvc copies over the headers from the incoming request, unless they're marked as sensitive. In addition, it also uses the HTTP message converters to unmarshal the payload of the incoming request and marshal it again when sending.
What we found is that in the header copying, it also includes the Content-Length header of the incoming request. However, b/o the unmarshalling and then marshalling the content length of the proxied request can be different: especially in the case of JSON where whitespace is removed, it can become less.

This causes problems with some downstream servers, as they keep waiting for additional request contents based on the erroneous Content-Length header. When not using smth like Apache Component HTTP client but using the default Java client instead, this even leads to errors in the client itself (showcased in attached sample).

This behavior was not there in previous versions: actually, with Spring Boot 3.1.6 and the underlying Spring web version this works. When you check what's being sent then you can see that a Content-Length header is always set, with the correct value.
It looks like the latest Spring web code (Jackson HTTP message converter?) no longer sets its own Content-Length header when marshalling to JSON. Therefore, spring-cloud-gateway-mvc should never copy over this header from the incoming request, but should rather leave it up to the RestTemplate to either leave it out or set this to the correct value for the outgoing request.

BTW, although this isn't strictly related to this issue, I believe that the Host header of the incoming request shouldn't be copied either by default: this can also lead to issues.

Sample
I've attached a project with two integration tests that showcase the problem. By configuring the content-length to be a sensitive header you can work around the issue, but users of the framework should not be required to do this.
gateway-mvc-contentlength.zip

@jkuipers jkuipers changed the title spring-cloud-gateway-mvc spring-cloud-gateway-mvc's ProxyExchange not compatible with recent Spring web versions Nov 30, 2023
@spencergibb
Copy link
Member

Posting for those encountering, the workaround is to set

spring.cloud.gateway.proxy.sensitive=content-length

@spencergibb spencergibb changed the title spring-cloud-gateway-mvc's ProxyExchange not compatible with recent Spring web versions ProxyExchange fails with "IOException: insufficient data written" with boot 3.2 Dec 5, 2023
@spencergibb spencergibb added this to the 4.1.0 milestone Dec 5, 2023
@spencergibb
Copy link
Member

Why don't you pass the body in the ProxyController?

@jkuipers
Copy link
Contributor Author

jkuipers commented Dec 6, 2023

The body isn’t modified by my code, why would I pass it myself? This is done automatically.
The way that we use this, we just want to pass the request with some modified headers and path to the downstream service, which I think is the typical use case for a proxy solution. The controller doesn't care about the type of a potential payload of the request, nor about the response payload type: that's why we use ProxyExchange<byte[]>.

We could explicitly add an @RequestBody byte[] body parameter to the controller to force the conversion to not use the Jackson message converter, but to me that feels like just another workaround: I think it's confusing to explicitly ask for input that you don't actually process in your controller method just to prevent the ProxyExchange from producing an invalid HTTP request (invalid in the sense that its Content-Length value is too low).
In our current use case we don't really mind the fact that the JSON message conversion is done: it also provides an additional layer of validation to ensure that the payload is in fact valid JSON. In general, since this is all just Spring MVC, other uses might involve unmarshalling the request body to some type (even outside of the controller) without the proxying code being interested in the body, and you'd expect that to work without having to compensate for headers with invalid values I think.

@estigma88

This comment was marked as off-topic.

@spencergibb

This comment was marked as off-topic.

@estigma88

This comment was marked as off-topic.

@spencergibb

This comment was marked as off-topic.

@jkuipers
Copy link
Contributor Author

Would it help to create a pull request to address this? I didn't consider that when I reported the issue as it seems trivial to fix, but given that it's been open for 4 months already and that various people have encountered issues b/o this bug I think it would be good to have a released solution ASAP.

@spencergibb
Copy link
Member

Yes, PRs welcome. We have an upcoming release in a week

jkuipers added a commit to jkuipers/spring-cloud-gateway that referenced this issue Mar 21, 2024
Also remove the bogus spring.cloud.gateway.proxy.auto-forward setting
jkuipers added a commit to jkuipers/spring-cloud-gateway that referenced this issue Mar 21, 2024
… copied

Also remove the bogus spring.cloud.gateway.proxy.auto-forward setting from the tests
jkuipers added a commit to jkuipers/spring-cloud-gateway that referenced this issue Mar 21, 2024
Also remove the bogus spring.cloud.gateway.proxy.auto-forward setting from the tests.
Fixes spring-cloudgh-3154
@jkuipers
Copy link
Contributor Author

jkuipers commented Mar 21, 2024

OK, I've created #3313 to address the issue for both the MVC and Webflux version.

jkuipers added a commit to jkuipers/spring-cloud-gateway that referenced this issue Mar 21, 2024
Also remove the bogus spring.cloud.gateway.proxy.auto-forward setting from the tests.
Fixes spring-cloudgh-3154
jkuipers added a commit to jkuipers/spring-cloud-gateway that referenced this issue Mar 21, 2024
Also remove the bogus spring.cloud.gateway.proxy.auto-forward setting from the tests.
Fixes spring-cloudgh-3154
@jkuipers
Copy link
Contributor Author

I thought the idea was that a PR could be considered for this week's release, but I see that the release was already cut without any feedback on the PR that I created last week... Any chance to get this into the next release, at least? Or any feedback on what might still be needed for that to happen?

@spencergibb
Copy link
Member

Sorry about that. I'll look at it again soon

@jkuipers
Copy link
Contributor Author

jkuipers commented Apr 7, 2024

Posting for those encountering, the workaround is to set

spring.cloud.gateway.proxy.sensitive=content-length

Note that setting it like this removes all the default sensitive headers and therefore is likely to let you expose secrets to downstream services that you’re proxying, so please don’t copy this as-is.

@jkuipers
Copy link
Contributor Author

Hey, it's been 5 five months since reporting this and four weeks since providing a suggested fix: any chance of getting a review on the PR? Right now both ProxyExchange modules are broken as shipped in the latest releases...

@larkioking
Copy link

@spencergibb is there any updates about bug status?

@jkuipers
Copy link
Contributor Author

jkuipers commented May 7, 2024

Missed yet another release, I see. I'm going to give up on chasing this: it's very puzzling to me how an official Spring project simply ignores shipping two broken modules for half a year already when it's clear what the problem is and there's a proposed solution.

@spencergibb
Copy link
Member

It'll be in the May release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants