-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Conversation
How did you verify this change? Did you do any manual testing and confirm what is being sent to WinHTTP APIs (via debugger) or on the wire (Wireshark etc.)? |
no @davidsh i read only docs and followed thread discussion. I put this PR in WIP and i'll do manual verify, thank you for you advice/review. |
Based on the documentation here, this does seem like the correct behavior:
@Caesar1995 has been looking into this though and can probably provide the best feedback. |
Thanks! In terms of the proposed fix, it is probably correct. But we would like to see some sort of validation even with a simulated end-to-end scenario that might be manually done due to the difficulties of doing an IDN related automated test in the CI system. |
Probably this PR will fail with the tests I added. You will need to append |
In terms of calling the WinHTTP API itself, I'm pretty sure it is ok to simply use Uri.IdnHost as input to the WinHTTP API call, even if it is an IPv6 literal wo/ enclosing brackets. In fact, adding brackets might generate an error with calling the WinHTTP API. |
Current implementation pass What error/concern we may have here? |
Normally, the brackets are not part of the actual IPv6 literal. They are used when IPv6 literals are part of a Uri which is something that is defined in the Uri RFC's. See:https://www.ietf.org/rfc/rfc2732.txt
But IPv6 literals in general are not defined as having brackets in them: See: https://tools.ietf.org/html/rfc5952 It's possible that WinHTTP APIs are resilient enough to ignore any brackets when an IPv6 literal is passed. Doing testing on this is recommended to validate our assumptions. But, therefore, I don't think we need to add any code to explicitly check for and add brackets if not present. |
I agree. I'm curious about what WinHttp will send if |
@davidsh @Caesar1995 [Fact]
[Trait("MyTrait", "MyTrait")]
public async Task ManualTest()
{
var idna = System.Text.Encoding.UTF8.GetString(Convert.FromBase64String("aHR0cDovL+CmrOCmvuCmguCmsuCmvuCmpuCnh+Cmti5pY29tLm11c2V1bQ=="));
var handler = new WinHttpHandler();
handler.Proxy = new WebProxy("127.0.0.1", 8888);
handler.WindowsProxyUsePolicy = WindowsProxyUsePolicy.UseCustomProxy;
using (HttpClient client = new HttpClient(handler))
{
var response = await client.GetAsync(idna);
}
} using codeBase64 to avoid VS ask to re-save file. |
You can use online tools like this one to decode the IDN and verify that it is equal. I think the important thing is that the IDN encoded string really is being sent on the network. Did you see the IDN encoded version in fiddler? |
@rmkerr sorry i replaced my post, if i setup Fiddler as proxy i can see tranlsated host. |
Awesome! Based on that, the IDN encoded host is getting sent. It looks like the tests are hanging though. @Caesar1995 do you think that could be related to the ipv6 bracket issue? |
@@ -785,7 +785,7 @@ private void EnsureSessionHandleExists(WinHttpRequestState state) | |||
// Specify an HTTP server. | |||
connectHandle = Interop.WinHttp.WinHttpConnect( | |||
_sessionHandle, | |||
state.RequestMessage.RequestUri.Host, | |||
state.RequestMessage.RequestUri.HostNameType == UriHostNameType.IPv6 ? $"[{state.RequestMessage.RequestUri.IdnHost}]" : state.RequestMessage.RequestUri.IdnHost, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Caesar1995 @davidsh @rmkerr i added []
for IPv6 because i get 'The URL is invalid'
for http://[::1234]
address. In case my idea is ok do you prefer "["+state.RequestMessage.RequestUri.IdnHost+"]"
(less allocations)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because i get 'The URL is invalid' for http://[::1234] address.
Could you elaborate on this? What part of the code will complain "The URL is invalid"? What test did you do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you get that error for http://[::1234]
or for http://::1234
?
@davidsh @rmkerr @Caesar1995 if i create a fake IWebProxy i could try to add a unit test to verify traslation(at the moment i don't know how write a web proxy but i can investigate). Is this reasonable? |
sorry @davidsh @rmkerr @Caesar1995 i did some mess with rebase(i wanted to integrate new tests on ipv6) i'll create a new PR, i don't know how to reopen a PR(but i think i can't do that). |
fixes #28722