jsonrpc(fix): set host header to URL hostname instead of resolved IP#578
Open
lorenzolfm wants to merge 1 commit intorust-bitcoin:masterfrom
Open
jsonrpc(fix): set host header to URL hostname instead of resolved IP#578lorenzolfm wants to merge 1 commit intorust-bitcoin:masterfrom
lorenzolfm wants to merge 1 commit intorust-bitcoin:masterfrom
Conversation
Member
|
Appreciate the contribution. Is this LLM generated? The PR description smells a bit like it. Can you state what you wrote and what is bot written please. From your GitHub account it looks like you know what you are doing but I'd just like an idea so I can guide my review. Thanks and beleza. |
Author
|
The fix and The approach was first adding a test the failed. This is the LLM generated test |
jamillambert
approved these changes
May 4, 2026
Collaborator
jamillambert
left a comment
There was a problem hiding this comment.
ACK 622faed
This all looks good to me. Thanks for the issue and the fix.
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
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 and Notes
Fixes rust-bitcoin/corepc#577.
The
simple_httpwrites the resolved socket address to the outgoingHostHTTP header instead of the URL's authority component, violating RFC 9112 §3.2:The fix
The bug:
SimpleHttpTransporthad no field for the URL hostname, socheck_urlcouldn't preserve it across DNS resolution, so the request serializer had nothing but the resolvedSocketAddrto put in theHostheader. The fix updates all three.Struct (
SimpleHttpTransport, line 40): add ahost_header: Stringfield. Stores the URL's authority component verbatimhostorhost:port.URL parser (
check_url, line 282): now returns(SocketAddr, host_header, path)instead of(SocketAddr, path). Capturesafter_auth.to_owned()before callingto_socket_addrs().Wire serializer (
try_request, line 152-153): writesself.host_headerto theHost:header instead ofself.addr.to_string().set_urlis updated to thread the new tuple. TheDefaultimpl initializeshost_headerconsistently with the defaultaddr(127.0.0.1:DEFAULT_PORT).The tests
Two new tests, plus an existing test extended.
request_uses_url_host_in_host_header: the regression test.Binds a
TcpListeneronlocalhost:0, points the client athttp://localhost:<port>/, captures the outgoing request bytes viastream.read(), and asserts thehost:line of the request equalslocalhost:<port>. Against unfixed code it sees[::1]:<port>or127.0.0.1:<port>(whichever the system resolveslocalhostto first)check_url_host_header: unit test for the parser.Table over hostname / IP / IPv6-bracketed / userinfo URL forms. Asserts the URL authority is preserved in
host_headerverbatim:The userinfo case (
me:weak@) verifies credentials are stripped; the IPv6 case verifies brackets round-trip; the no-port and explicit-port cases verify both default-port branches.test_urls: existing test, extended.Already exercised the parser for all valid URLs. Updated to destructure the new 3-tuple and assert that
host_headerround-trips throughBuilder::url()alongside the existingaddrandpathchecks. Same coverage as before plus the new field.