Pass Uri.IdnHost to WinHttpConnect #28849

MarcoRossignoli · 2018-04-05T08:58:23Z

fixes #28722
replace PR #28818

cc: @davidsh @rmkerr @Caesar1995

MarcoRossignoli · 2018-04-05T09:03:03Z

src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

@@ -783,7 +783,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,


With regard to this i'm writing a manual test.

@davidsh @rmkerr @Caesar1995 without [] ipv6 http://[::1234]:8080 and http://[::1234] tests fails with ERROR_INTERNET_INVALID_URL.

@Caesar1995 related to https://github.com/dotnet/corefx/issues/28863#issue-311719169 if this PR will be merged i think we'll need to remove workaround also here.

Yes, thanks for bringing this up! I will update that issue with this PR.

MarcoRossignoli · 2018-04-05T13:49:21Z

src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

+                    Assert.NotNull(ex.InnerException);
+
+                    //ERROR_INTERNET_INVALID_URL https://msdn.microsoft.com/en-us/library/windows/desktop/aa385465(v=vs.85).aspx
+                    Assert.NotEqual(12005, (int)ex.InnerException.GetType().GetProperty("NativeErrorCode").GetValue(ex.InnerException));


I used reflection to avoid to add reference for WinHttpException

You don't need to use reflection.

While WinHttpException is not a public type, it is derived from Win32Exception which is a public type. And the NativeErrorCode property can be accessed directly.
https://msdn.microsoft.com/en-us/library/system.componentmodel.win32exception(v=vs.110).aspx

MarcoRossignoli · 2018-04-05T13:52:17Z

src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

+        [InlineData("http://[::1234]:8080", false, null)]
+        [InlineData("http://127.0.0.1", false, null)]
+        [InlineData("http://www.microsoft.com", false, "www.microsoft.com")]        
+        public async Task ManualTest_IdnHostName(string requestUri, bool codeBase64, string requestHost)


@davidsh @rmkerr @Caesar1995 i tried to create a manual test, get back to me.

davidsh · 2018-04-05T14:36:18Z

src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

+            Take internationalized domain name 'Bangladesh' sample url from http://www.i18nguy.com/markup/idna-examples.html 
+            Saved in codeBase64 to avoid Visual Studio complains about encoding
+        */
+        [InlineData("aHR0cDovL+CmrOCmvuCmguCmsuCmvuCmpuCnh+Cmti5pY29tLm11c2V1bQ==", true, "icom.museum")]


It isn't necessary to use Base64 encoding to create Unicode Uri literals. Just use "\uuuu" notation in the literals.

i.e. "http://\u09AC\u09BE\u0982\u09B2\u09BE\u09A6\u09C7\u09B6.icom.museum"

caesar-chen · 2018-04-05T17:11:28Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop OSX x64 Debug Build please
@dotnet-bot test Outerloop Windows x64 Debug Build please

MarcoRossignoli · 2018-04-05T18:01:06Z

@Caesar1995 seems unrelated issue, right?

davidsh · 2018-04-05T18:35:01Z

src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

+        [InlineData("http://[::1234]:8080", null)]
+        [InlineData("http://127.0.0.1", null)]
+        [InlineData("http://www.microsoft.com", "www.microsoft.com")]        
+        public async Task ManualTest_IdnHostName(string requestUri, string requestHost)


I think this manual test should be simplified. The only test we really need to add is an end-to-end test that validates that a request can be made to a server that uses Unicode characters in the DNS name. The other test cases are really already covered by other IPv4 and IPv6 literal test cases in System.Net.Http/tests/FunctionalTests.

Also, then, this test would be more valuable as a test case in the System.Net.Http/tests/FunctionalTests folder so that it can be run, manually, across the different handlers.

This test should also be marked in a way so that it doesn't get run all the time in CI because then it will be trying to connect all the time to a server that doesn't really exist.

The error handling then for HttpRequestException doesn't need to be present then since the test is only validating a successful operation when run.

Even better, would be if we could use a third-party server on the public Internet that uses Unicode characters in the DNS names. I'm sure that there are lots of them. Here is one that appears to be a international version of the Starbucks website.

Unicode: 스타벅스코리아.com
Punnycode: xn--oy2b35ckwhba574atvuzkc.com
\uuuu format: \uc2a4\ud0c0\ubc85\uc2a4\ucf54\ub9ac\uc544.com

So if you used that website, then the test could always be enabled and you would check for a successful connection.

@davidsh ok i'll move test to System.Net.Http/tests/FunctionalTests with simple successful operation check.

This test should also be marked in a way so that it doesn't get run all the time in CI because then it will be trying to connect all the time to a server that doesn't really exist

Do you mean [Outerloop] or do you have some test sample to take a cue?

If you use the website (international Starbucks) that I described above, then you can simply mark the test as [Outerloop]. It will still get run all the time during outerloop CI runs. By default, tests in CI only run innerloop.

However, if the website was something that can't normally be reached, then you would mark this test in some other way so that it never runs in innerloop nor outerloop.

I recommend you simply try to use the website I indicated and then the test can run in outerloop all the time.

MarcoRossignoli · 2018-04-06T13:24:34Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+                string idn = "\uc2a4\ud0c0\ubc85\uc2a4\ucf54\ub9ac\uc544.com";
+                using (HttpResponseMessage response = await client.GetAsync("http://" + idn))
+                {                    
+                    response.EnsureSuccessStatusCode();


@davidsh i added response.EnsureSuccessStatusCode(); as assertion.
Question on tests, i want to understand how do we test all handlers, I've noticed that test run two times, one for every handler, how it works?

In System.Net.Http.FunctionalTest project, we have abstract base test class, that's where you want to your test. The current location System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs is correct. Since the base class is abstract, those test cases will only be invoked by derived types (explained below).

There are two files which will actually invoke all the test cases. PlatformHandlerTest.cs, which will run the test against platform specific handlers (WinHttpHandler on Windows, and CurlHandler on Unix), and SocketsHttpHandler, which will run the test using our managed SocketsHttpHandler.

Even though you only modified the WinHttpHandler's code, we want to make sure the test you added is not specific to WinHttpHandler, and will pass with other handlers. To do so, you will help us detect/avoid platform specific behavior difference.

@Caesar1995 understood TYVM!

MarcoRossignoli · 2018-04-06T13:25:32Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

@@ -1659,7 +1659,7 @@ public void GetAsync_ServerNeedsAuthAndNoCredential_StatusCodeUnauthorized()
                }
            });
        }
-        
+


i don't know why...in my vs diff tool these changes weren't visible, should i remove?

Yes, please remove any changes that aren't part of the PR.

MarcoRossignoli · 2018-04-06T14:18:25Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop OSX x64 Debug Build please
@dotnet-bot test Outerloop Windows x64 Debug Build please

MarcoRossignoli · 2018-04-06T14:20:00Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+                string idn = "\uc2a4\ud0c0\ubc85\uc2a4\ucf54\ub9ac\uc544.com";
+                using (HttpResponseMessage response = await client.GetAsync("http://" + idn))
+                {
+                    response.EnsureSuccessStatusCode();


@davidsh i added response.EnsureSuccessStatusCode(); as assertion.
Question on tests, i want to understand how do we test all handlers, I've noticed that test run two times, one for every handler, how it works?

Explained in one of the comments above.

caesar-chen · 2018-04-06T17:26:27Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

@@ -2138,6 +2138,25 @@ public void GetAsync_ServerNeedsAuthAndNoCredential_StatusCodeUnauthorized()
            });
        }

+        [OuterLoop]
+        [Fact]
+        public async Task GetAsync_IdnHostName()


Usually the naming for the test function should be like: A_B_C, A is the attribute what we are testing about, B is the condition to trigger the attribute, and C is the behavior outcome.

You can change to something like GetAsync_IdnHostName_SuccessStatusCodeInResponse.

caesar-chen · 2018-04-06T17:28:38Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+                /*
+                    international version of the Starbucks website
+                    Punnycode: xn--oy2b35ckwhba574atvuzkc.com
+                */


In corefx, we usually do comment like this:

// Here is the comment.

MarcoRossignoli · 2018-04-06T18:12:31Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop OSX x64 Debug Build please
@dotnet-bot test Outerloop Windows x64 Debug Build please

davidsh · 2018-04-06T19:47:58Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+            HttpClientHandler handler = CreateHttpClientHandler();
+            using (var client = new HttpClient(handler))
+            {                
+                //international version of the Starbucks website


Add space character after "//" comment characters.

davidsh · 2018-04-06T19:50:16Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+            {                
+                //international version of the Starbucks website
+                //punnycode: xn--oy2b35ckwhba574atvuzkc.com                
+                string idn = "\uc2a4\ud0c0\ubc85\uc2a4\ucf54\ub9ac\uc544.com";


I would name this variable to something like "server". "idn" implies that this string consists of punnycode character which it doesn't. It is simply a hostname of a endpoint that just happens to have Unicode characters in the name.

And I would simplify this by bringing in the "http://" portion of the string here as well, i.e.

string server = "http://\uc2a4\ud0c0\ubc85\uc2a4\ucf54\ub9ac\uc544.com";

davidsh · 2018-04-06T19:50:52Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

@@ -2138,6 +2138,23 @@ public void GetAsync_ServerNeedsAuthAndNoCredential_StatusCodeUnauthorized()
            });
        }

+        [OuterLoop]
+        [Fact]
+        public async Task GetAsync_IdnHostName_SuccessStatusCodeInResponse()


I would change this name to: "GetAsync_UnicodeHostName_SuccessStatusCodeInResponse"

Also, did you confirm that this test will fail without the fix in WinHttpHandler (to use IdnHost instead of Host)?

actually no, i did only IdnHost square brackets tests. I'll do test.

Note that WinHttp does not accept international host names without converting them first to Punycode

@davidsh...weird...seems to work, i tried also others url to test different characters...

@davidsh...weird...seems to work, i tried also others url to test different characters...

Perhaps WinHTTP has changed behavior on recent Windows OS versions to allow Unicode characters to be used in the API calls. However, if you look at WireShark traces, you'll probably see IDN (punnycode) encoding actually used underneath. Did you observe that?

It's possible that the documentation is out of date and needs to be corrected. This needs to be researched. @Caesar1995 can you start an offline email with WinHTTP team and ask them about this behavior?

For now, it would be useful to update your PR to remove the product code change in WinHttpHandler but keep the new test you added. Then run all the tests against this PR and see what passes/fails. Perhaps older Windows OS versions like Windows 7 might fail. We need to understand this more. It's possible that we don't need a product code change at all but the new test is useful.

i did a test with fiddler and i saw punycode going out, i'll restore WinHttpHandler...i apologize i assumed that guide was up to date, must never do.

MarcoRossignoli · 2018-04-06T20:59:07Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop OSX x64 Debug Build please
@dotnet-bot test Outerloop Windows x64 Debug Build please

MarcoRossignoli · 2018-04-07T11:15:43Z

@dotnet-bot test Outerloop Windows x64 Debug Build please

MarcoRossignoli · 2018-04-09T19:42:18Z

@davidsh @Caesar1995 test fail on Windows.7.Amd64.Open-x64-Debug as you thought.
https://mc.dot.net/#/user/MarcoRossignoli/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/850743d62b18422f296b8ba028cfc9b640b56009/workItem/System.Net.Http.Functional.Tests/analysis/xunit/System.Net.Http.Functional.Tests.PlatformHandler_HttpClientHandlerTest~2FGetAsync_UnicodeHostName_SuccessStatusCodeInResponse

System.Net.Http.HttpRequestException : An error occurred while sending the request.
---- System.Net.Http.WinHttpException : Error 12005 calling WinHttpConnect, 'The URL is invalid'.

Maybe the guide is "conservative" for backward compatibility.
Do you agree if i re-add code change and re-run outerloop?

MarcoRossignoli · 2018-04-17T09:18:20Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop OSX x64 Debug Build please
@dotnet-bot test Outerloop Windows x64 Debug Build please

MarcoRossignoli · 2018-04-17T13:31:38Z

@davidsh @Caesar1995 i don't see issue on Win7 for System.Net.Http.Functional.Tests

old test without fix(test fail)
https://mc.dot.net/#/user/MarcoRossignoli/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/850743d62b18422f296b8ba028cfc9b640b56009

with fix
https://mc.dot.net/#/user/MarcoRossignoli/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/b3cdff8703f995f8c03cbfe8d523ec45eea1e3a6

stephentoub · 2018-04-17T13:34:01Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+            using (var client = new HttpClient(handler))
+            {
+                // international version of the Starbucks website
+                // punnycode: xn--oy2b35ckwhba574atvuzkc.com                


Nit: punnycode => punycode

davidsh

Thanks for the PR!

davidsh · 2018-04-17T15:58:38Z

@dotnet-bot Test Outerloop Windows x64 Debug Build

davidsh · 2018-04-17T15:59:54Z

CI failures in OSX x64 Debug Build are unrelated to PR.

Will merge this once Outerloop Windows tests completes

caesar-chen

Thanks for fixing this!

* pass IdnHost * add manual functional test * cleanup * fix comment * address PR feedback * address PR feedback * address PR feedback * address PR feedback * address PR feedback * address PR feedback * address PR feedback * fix IPv6 IdnHost * address PR feedback Commit migrated from dotnet/corefx@b2893f0

pass IdnHost

4f1a60d

MarcoRossignoli changed the title ~~Pass Uri.IdnHost to WinHttpConnect~~ [WIP]Pass Uri.IdnHost to WinHttpConnect Apr 5, 2018

MarcoRossignoli commented Apr 5, 2018

View reviewed changes

MarcoRossignoli added 2 commits April 5, 2018 15:44

add manual functional test

8a9cbee

cleanup

c5e4fdc

MarcoRossignoli commented Apr 5, 2018

View reviewed changes

MarcoRossignoli changed the title ~~[WIP]Pass Uri.IdnHost to WinHttpConnect~~ Pass Uri.IdnHost to WinHttpConnect Apr 5, 2018

fix comment

9009d2b

davidsh added the area-System.Net.Http label Apr 5, 2018

davidsh reviewed Apr 5, 2018

View reviewed changes

address PR feedback

940f497

davidsh reviewed Apr 5, 2018

View reviewed changes

MarcoRossignoli changed the title ~~Pass Uri.IdnHost to WinHttpConnect~~ [WIP]Pass Uri.IdnHost to WinHttpConnect Apr 5, 2018

address PR feedback

caed777

MarcoRossignoli commented Apr 6, 2018

View reviewed changes

MarcoRossignoli changed the title ~~[WIP]Pass Uri.IdnHost to WinHttpConnect~~ Pass Uri.IdnHost to WinHttpConnect Apr 6, 2018

MarcoRossignoli added 2 commits April 6, 2018 16:17

address PR feedback

3f21a59

address PR feedback

99648b0

MarcoRossignoli commented Apr 6, 2018

View reviewed changes

caesar-chen reviewed Apr 6, 2018

View reviewed changes

MarcoRossignoli added 2 commits April 6, 2018 19:51

address PR feedback

9c80ffe

address PR feedback

91ba27d

davidsh reviewed Apr 6, 2018

View reviewed changes

address PR feedback

850743d

stephentoub modified the milestones: Future, 2.2.0 Apr 11, 2018

karelz assigned MarcoRossignoli, rmkerr, davidsh and caesar-chen Apr 13, 2018

fix IPv6 IdnHost

b3cdff8

stephentoub reviewed Apr 17, 2018

View reviewed changes

stephentoub approved these changes Apr 17, 2018

View reviewed changes

address PR feedback

6973787

davidsh approved these changes Apr 17, 2018

View reviewed changes

caesar-chen approved these changes Apr 17, 2018

View reviewed changes

caesar-chen merged commit b2893f0 into dotnet:master Apr 17, 2018

MarcoRossignoli deleted the winhttp2 branch April 17, 2018 17:44

caesar-chen mentioned this pull request Jan 31, 2020

[Uri] Fix IPv6 address format dotnet/runtime#25782

Open

Pass Uri.IdnHost to WinHttpConnect #28849

Pass Uri.IdnHost to WinHttpConnect #28849

Conversation

MarcoRossignoli commented Apr 5, 2018

Choose a reason for hiding this comment

MarcoRossignoli Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Apr 5, 2018

MarcoRossignoli commented Apr 5, 2018

davidsh Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli commented Apr 6, 2018

MarcoRossignoli commented Apr 7, 2018

MarcoRossignoli commented Apr 9, 2018 • edited Loading

MarcoRossignoli commented Apr 17, 2018

MarcoRossignoli commented Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh left a comment

Choose a reason for hiding this comment

davidsh commented Apr 17, 2018

davidsh commented Apr 17, 2018

caesar-chen left a comment

Choose a reason for hiding this comment

MarcoRossignoli Apr 5, 2018 •

edited

Loading

MarcoRossignoli Apr 6, 2018 •

edited

Loading

davidsh Apr 5, 2018 •

edited

Loading

MarcoRossignoli Apr 5, 2018 •

edited

Loading

MarcoRossignoli Apr 6, 2018 •

edited

Loading

MarcoRossignoli Apr 6, 2018 •

edited

Loading

MarcoRossignoli Apr 6, 2018 •

edited

Loading

MarcoRossignoli Apr 6, 2018 •

edited

Loading

MarcoRossignoli commented Apr 9, 2018 •

edited

Loading

MarcoRossignoli commented Apr 17, 2018 •

edited

Loading