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

AsyncMiddleManServlet removes content-length header if the client doesn't send Expect: 100-Continue #9758

Open
mtheos opened this issue May 11, 2023 · 5 comments
Assignees
Labels

Comments

@mtheos
Copy link

mtheos commented May 11, 2023

Jetty version
10.0.15
Java version
17.0.3
Question

Hi Jetty,

The AsyncMiddleManServlet.ProxyReader removes the content-length header and switches to chunked encoding when proxying.

I initially thought this was because you don't know the length of the transformed request, but I noticed that it only does this when the client doesn't send Expect: 100-Continue and otherwise keeps the content length header.

Do you know why this difference in behaviour exists? It would be possible to change the headers in copyRequestHeaders myself if the content length is unknown without enforcing it in the reader.

I can extend the ProxyReader impl to change this behaviour, but it's a significant amount of code that needs to be copied to make a very small behaviour change.

@sbordet
Copy link
Contributor

sbordet commented Aug 1, 2023

It is a bug that we keep the Content-Length header when the client request has Expect: 100-Continue because we do not know the size of the transformed request content.

With that fixed, the behavior will be the same: if the proxy can read the whole client request content in one read, then the proxy request to the server will have Content-Length, otherwise it will be chunked.

Would that solve this issue?
I ask because I'm not sure I understand what you are asking in the last part of your comment above.

@mtheos
Copy link
Author

mtheos commented Aug 1, 2023

Hi Simone,

In our use case, we know the size of the transformed request up front since we're only adding metadata to the body, so we can add the content-length header before we've read the client request.

The issue we faced is that the downstream expects the content-length header so it can adjust how it handles the request based on the size.

We worked around the issue by copying the code and modifying it.

@sbordet
Copy link
Contributor

sbordet commented Aug 1, 2023

Can you detail what code you have modified?
It may be that we can add an overridable method that will make your life simpler rather than copying large chunks of code.

@mtheos
Copy link
Author

mtheos commented Aug 1, 2023

I copied the AsyncMiddleManServlet and removed that line. That was the only change needed.

We were looking at the middle man servlet as an option, but we've since moved to reactor netty with webflux so I wouldn't worry about changing this for us unless you think it's the right change.

Copy link

github-actions bot commented Aug 1, 2024

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label Aug 1, 2024
@sbordet sbordet removed the Stale For auto-closed stale issues and pull requests label Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants