transport/http: Include non-default ports in Host header#4882
Conversation
ethomson
left a comment
ethomson
left a comment
There was a problem hiding this comment.
I'm not sure why the CI builds didn't start here... but there are a few typos (getno should be gitno, which is an abbreviation for git network operations, not an accessor). This is a bit confusing since we use git_ as a prefix everywhere except this one place. Alas.
| const char *url, | ||
| const char *default_port); | ||
|
|
||
| const char *getno__default_port(gitno_connection_data *data); |
There was a problem hiding this comment.
This should be gitno, not getno...
| static const char *default_port_http = "80"; | ||
| static const char *default_port_https = "443"; | ||
|
|
||
| const char *getno__default_port( |
There was a problem hiding this comment.
This should be gitno, not getno...
| data->use_ssl = true; | ||
| } else if (url[0] == '/') | ||
| default_port = data->use_ssl ? "443" : "80"; | ||
| default_port = getno__default_port(data); |
There was a problem hiding this comment.
This should be gitno, not getno...
Constant strings and logic for HTTP(S) default ports were starting to be
spread throughout netops.c. Instead of duplicating this again to
determine if a Host header should include the port, move the default
port constants and logic into an internal method in netops.{c,h}.
|
I'm puzzled by how this even compiled. I definitely built it using CMake and msbuild on Windows 10 and did not see a warning or error even though src/transports/http.c is very clearly referencing an undefined symbol. More puzzling is that I build Rust's Cargo using this version and it worked correctly. |
When the port is omitted, the server assumes the default port for the service is used (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host). In cases where the client provided a non-default port, it should be passed along. This hasn't been an issue so far as the git protocol doesn't include server-generated URIs. I encountered this when implementing Rust registry support for Sonatype Nexus. Rust's registry uses a git repository for the package index. Clients look at a file in the root of the package index to find the base URL for downloading the packages. Sonatype Nexus looks at the incoming HTTP request (Host header and URL) to determine the client-facing URL base as it may be running behind a load balancer or reverse proxy. This client-facing URL base is then used to construct the package download base URL. When libgit2 fetches the index from Nexus on a non-default port, Nexus trusts the incorrect Host header and generates an incorrect package download base URL.
|
Aha! When I first encountered the bug this PR fixes, I was using a Linux machine. Since then I switched to a Windows machine and didn't notice that libgit2 uses WinHTTP by default instead of src/transports/http.c. So my builds were succeeding because the code I was changing wasn't even being built and I saw successful test results because it was using WinHTTP which doesn't have the bug. CI builds now pass and I'll test with a Linux VM later today. |
|
Thanks for the PR! |
2 participants
When the port is omitted, the server assumes the default port for the
service is used (see
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host). In
cases where the client provided a non-default port, it should be passed
along.
This hasn't been an issue so far as the git protocol doesn't include
server-generated URIs. I encountered this when implementing Rust
registry support for Sonatype Nexus. Rust's registry uses a git
repository for the package index. Clients look at a file in the root of
the package index to find the base URL for downloading the packages.
Sonatype Nexus looks at the incoming HTTP request (Host header and URL)
to determine the client-facing URL base as it may be running behind a
load balancer or reverse proxy. This client-facing URL base is then used
to construct the package download base URL. When libgit2 fetches the
index from Nexus on a non-default port, Nexus trusts the incorrect Host
header and generates an incorrect package download base URL.