[native] Fix blank HTTP Host header in requests #24929
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZacBlanco
changed the title
pramodsatya
previously approved these changes
Apr 16, 2025
ZacBlanco
force-pushed
the
upstream-native-host-header
branch
from
c669bee to
06fdfa6
Compare
prestodb-ci
requested review from
a team,
nmahadevuni and
namya28
and removed request for
a team
BryanCutler
reviewed
Apr 17, 2025
| @@ -244,7 +244,6 @@ class RequestBuilder { | |||
| send(HttpClient* client, const std::string& body = "", int64_t delayMs = 0) { | |||
| addJwtIfConfigured(); | |||
| header(proxygen::HTTP_HEADER_CONTENT_LENGTH, std::to_string(body.size())); | |||
| headers_.ensureHostHeader(); | |||
There was a problem hiding this comment.
I think send is used other places too, can this be left in?
There was a problem hiding this comment.
The problem is this will always set the Host header as an empty string because the HttpMessage doesn't have a dstAddress set. It isn't possible to get the dstAddress except for when the client actually sends the request. If the Host header gets set to empty, then subsequent calls to ensureHostHeader are a no-op
ZacBlanco
force-pushed
the
upstream-native-host-header
branch
from
06fdfa6 to
f08764e
Compare
Because the request's destination address wasn't set inside of the RequestBuilder object, the Host was populated with a blank value. On older HTTP versions this is permissible, but on newer ones (such as Jetty 12), this is a hard error and results in the request being rejected with a 400 error. This change properly sets the request destination address so that ensureHostHeader sets the correct value.
ZacBlanco
force-pushed
the
upstream-native-host-header
branch
from
f08764e to
7a445d8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change properly sets the request destination address so that ensureHostHeader sets the correct value for the HTTP
Hostheader.Motivation and Context
Because the request's destination address wasn't set inside of the RequestBuilder object, the Host was populated with a blank value. On older HTTP servers this is permissible, but on newer ones (such as the incoming Jetty 12 upgrade), this is a hard error and results in the request being rejected with a 400 error.
Impact
N/A
Test Plan
Existing tests
Contributor checklist
Release Notes