Runtime: [Uri] Incorrect port parsed when space at the end of URI

[karelz]

When space is at the end of URL with port number, we parse incorrect port:

c# string url = "http://a.b:1"; Console.WriteLine($"No space: {new Uri(url).Port}"); // 1 Console.WriteLine($"Space: {new Uri(url + " ").Port}"); // 65530

Reproduced on .NET Framework 4.7.2 and .NET Core 2.1.

Originally filed here: https://developercommunity.visualstudio.com/content/problem/368745/uriport-returns-a-different-port-number-due-to-a-t.html

[stephentoub]

What is the expected behavior here? Throw? Ignore the whitespace?

[karelz]

Good question - @rmkerr can you suggest the right course of action? (based on other Uri behavior)

[rmkerr]

Our current behavior for handling unencoded spaces depends on context. For example, if we find a space in the middle of a path segment:

Uri test = new Uri("http://localhost:1/test test/");

We'll encode the space as %20 in the absolute URI:

Absolute URI : "http://localhost:1/test%20test/"

On the other hand, if we find a space at the end of the URI, we'll drop it:

Uri test = new Uri("http://localhost:1/test ");

Results in:

Absolute URI : "http://localhost:1/test/"

Now, the behavior for the port segment is a little different. A space isn't legal there even if it is encoded. So, both of the following (correctly) throw an exception:

Uri test = new Uri("http://localhost:1 /test");
Uri test = new Uri("http://localhost:1%20/test");

To be consistent, I think we should enforce the following behavior:
(1) if we find a space in the port, but that space is at the end of the URI, drop the space.
(2) if we find a space in the port, and a path, query, or scheme follows, throw an exception.

Behavior (2) is already the case, so we just need to fix behavior (1) to strip the space and return the correct port.

[rmkerr]

The bug is in this code:
https://github.com/dotnet/corefx/blob/684704de20c28103ea8e0063bda7ed98f0912c58/src/System.Private.Uri/src/System/Uri.cs#L2410-L2433
Basically this code is doing string -> int conversion with no validation that the characters it's operating on are actually in the range of '0' to '9'. Previous validation will prevent most characters from getting through, but our rules on trailing whitespace allow spaces, tabs, and newlines.

The fix here is going to be one of two things: (1) make a targeted change to line 2425 to end parsing when we see any non-digit character rather than just the ones listed there, or (2) cut this whole block of code out and rewrite it to be more readable.

I'm inclined to do (1) to minimize the chance of regressions, but I want to know what others think (@karelz, @davidsh)

[stephentoub]

I agree on (1) for now. An overhaul could be done subsequently.