Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fix ipv6 address format in Host header #28578

Merged
merged 13 commits into from
Apr 2, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,8 @@ private async Task WriteHostHeaderAsync(Uri uri)
else
{
Debug.Assert(_pool.UsingProxy);
await WriteAsciiStringAsync(uri.IdnHost).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

What about the if block above? Don't we need to fix that case as well?

Choose a reason for hiding this comment

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

Yes, we need to fix that as well. The fix there is more involved because it needs to happen as part of the construction of the connection key -- basically we should detect that the hostname is an IPv6 address when constructing the key, and construct the hostname using [].

You won't be able to test this using a proxy as you're doing here. But you should at least be able to test against IPv6Loopback. There's already a test for IP address based Uris -- GetAsync_IPBasedUri_Success. Unfortunately it's not validating the Host header. It should be modified to validate the Host header. And it would be nice to add https cases to that test as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! I will make a fix and add tests to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix the above if by changing GetConnectionKey method.

We still need to special case this else statement for Proxy case, because for this case, we pass null as host, thus _pool.HostHeaderValueBytes == null, and here is the only place to append []

await WriteAsciiStringAsync(uri.HostNameType == UriHostNameType.IPv6 ?
"[" + uri.IdnHost + "]" : uri.IdnHost).ConfigureAwait(false);
Copy link

@akashkc akashkc Mar 29, 2018

Choose a reason for hiding this comment

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

How about using string interpolation instead of string concatenation manually i.e. $"[{uri.IdnHost}]"

Copy link
Member

Choose a reason for hiding this comment

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

String interpolation would be more expensive here.

Copy link

Choose a reason for hiding this comment

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

Does string interpolation has significant performance difference than string concatenation. I believe, string interpolation has better readability.

Copy link
Member

Choose a reason for hiding this comment

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

In this case it'll compile down to a call to string.Concat:

string.Concat("[", uri.IdnHost, "]");

which will incur just a single allocation for the destination string, with the data from each constituent string blitted into it. In contrast, the string interpolation would compile down to the equivalent of:

string.Format("[{0}]", new object[] { uri.IdnHost });

and would thus incur at least the destination string and an object array allocation, plus the work that has to be done inside of Format, which involves a StringBuilder.

Copy link

Choose a reason for hiding this comment

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

Another useful note on string.Concat vs. string.Format #8025 (comment) (I want "bookmark favorite comment" feature in GitHub) :octocat:

Copy link

Choose a reason for hiding this comment

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

@stephentoub : Thank you for explaining it with implementation side. @kasper3 : Thank you for commenting with interesting thread.

Choose a reason for hiding this comment

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

Since we're now using uri.Host in the GetConnectionKey path, seems like it would make sense to use it here as well for consistency and to avoid the extra alloc (maybe?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, will change and add comment as well.


if (!uri.IsDefaultPort)
{
Expand Down
26 changes: 26 additions & 0 deletions src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,32 @@ public async Task GetAsync_SetAutomaticDecompression_ContentDecompressed(Uri ser
}
}

[Theory]
[InlineData("[::1234]")]
[InlineData("[::1234]:8080")]
public async Task GetAsync_IPv6AddressInHostHeader_CorrectlyFormatted(string host)
{
string ipv6Address = "http://" + host;
bool connectionAccepted = false;

await LoopbackServer.CreateClientAndServerAsync(async proxyUri =>
{
using (HttpClientHandler handler = CreateHttpClientHandler())
using (var client = new HttpClient(handler))
{
handler.Proxy = new WebProxy(proxyUri);
Copy link
Member

Choose a reason for hiding this comment

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

Clever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you taught me this. : )

Copy link
Member

Choose a reason for hiding this comment

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

Heh, ok, at least I'm consistent :)

try { await client.GetAsync(ipv6Address); } catch { }
}
}, server => server.AcceptConnectionAsync(async connection =>
{
connectionAccepted = true;
List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();
Assert.Contains($"Host: {host}", headers);
}));

Assert.True(connectionAccepted);
}

[OuterLoop] // TODO: Issue #11345
[Theory, MemberData(nameof(CompressedServers))]
public async Task GetAsync_SetAutomaticDecompression_HeadersRemoved(Uri server)
Expand Down