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

waiProxy: don't strip Content-Length in response for HTTP/2 #45

Merged
merged 2 commits into from
Dec 12, 2023
Merged

waiProxy: don't strip Content-Length in response for HTTP/2 #45

merged 2 commits into from
Dec 12, 2023

Conversation

bjin
Copy link
Contributor

@bjin bjin commented Dec 9, 2023

HTTP/2 (and newer HTTP/3) don't use chunked encoding so it's fine to keep Content-Length header in responses.

Also include cases for HEAD requests and empty-body responses, because in both cases, chunked encoding won't be used.

Copy link
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have enough experience with proxying and HTTP/2-3 to approve this. @kazu-yamamoto any thoughts?

@kazu-yamamoto
Copy link

The explanation by @bjin is correct, I think.

@bjin
Copy link
Contributor Author

bjin commented Dec 12, 2023

I would like to emphasize that this change is crucial for reverse proxying the Docker registry (ghcr.io in my case). The docker pull operation will simply fail without the Content-Length header. In my case, the request involves both HTTP/2 and a HEAD request, so the Content-Length can be safely retained.

@snoyberg
Copy link
Member

In that case could you add a minor version bump and changelog entry for this?

HTTP/2 (and newer HTTP/3) don't use chunked encoding so it's fine to
preserve 'Content-Length' in proxied response header.

Also do the same to HEAD requests and empty-body responses, because in
both cases, chunked encoding won't be used.
@bjin
Copy link
Contributor Author

bjin commented Dec 12, 2023

In that case could you add a minor version bump and changelog entry for this?

Pushed a new commit bumping minor package version (and Changelog change), also edited commit message to reflect HEAD request changes.

@snoyberg snoyberg merged commit 4e076eb into fpco:master Dec 12, 2023
10 checks passed
bjin added a commit to bjin/http-reverse-proxy that referenced this pull request Jun 6, 2024
This is a proper fix to fpco#45.

In previous merge request (fpco#45), for HTTP/2 connections, Content-Length
header is retained in order to avoid problems with Docker client rejecting
chunked encoding responses from Docker registry.

However, this approach failed when the upstream response was encoded
(e.g. with 'Content-Encoding: gzip' in header). The retained
'Content-Length' header then mismatched the actual content size after
decoding.

To fix this, we now strip the Content-Length header if Content-Encoding
header is present, along with our existing checks for cases where chunked
encoding is not used (HTTP/2 and HEAD requests).
bjin added a commit to bjin/http-reverse-proxy that referenced this pull request Jun 6, 2024
This is a proper fix to fpco#45.

In previous merge request (fpco#45), for HTTP/2 connections, Content-Length
header is retained in order to avoid problems with Docker client rejecting
chunked encoding responses from Docker registry.

However, this approach failed when the upstream response was encoded
(e.g. with 'Content-Encoding: gzip' in header). The retained
'Content-Length' header then mismatched the actual content size after
decoding.

To fix this, we now strip the Content-Length header if Content-Encoding
header is present, along with our existing checks for cases where chunked
encoding is not used (HTTP/2 and HEAD requests).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants