Fix Uri.Host for IPv6 Link-local address #29769

MarcoRossignoli · 2018-05-17T20:16:15Z

contributes to #28863

this PR address the second part of issue:

Uri.Host LLA (Link-local address) IPv6 address doesn't contain %number part.

Currently it returns [fe80::e077:c9a3:eeba:b8e9], it should return [fe80::e077:c9a3:eeba:b8e9%18].
Note: Uri.IdnHost correctly contains the %number part.

cc: @rmkerr @Caesar1995 @davidsh

davidsh · 2018-05-17T20:19:29Z

Thanks for the PR.

contributes to #28863

In the future, it would be better if the commit description were clearer about what this does exactly. That issue has multiple parts to it. So, perhaps this particular PR is addressing the enclosing brackets ('[', ']') for IPv6 link-local addresses or the scope-id w/ percent character? Either way, it would be good to add more details for commits and the PR description.

davidsh · 2018-05-17T20:23:24Z

src/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs

+        [InlineData("FE80::e077:c9a3:eeba:b8e9", "%eth10", true)]
+        [InlineData("fe81::e077:c9a3:eeba:b8e9", "%18", false)]        
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
+        public void UriIPv6Host_LinkLocalAddress(string address, string zoneIndex, bool isValidLinkLocalAddress)


The test name should be 3 parts, so it should probably be named like this: "Host_IPv6LinkLocalAddress_HasScopeId"

First part describes the method or property under test. In this case, the test is validating the .Host property.

Second part describes the inputs to the test.

Third part describes the expected result.

Also, you should separate out the valid and invalid cases into separate tests. The invalid one could be named, "Host_NonIPv6LinkLocalAddress_NoScopeId"

MarcoRossignoli · 2018-05-17T20:35:30Z

src/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs

+        [InlineData("fE80::e077:c9a3:eeba:b8e9", "%18")]
+        [InlineData("FE80::e077:c9a3:eeba:b8e9", "%18")]
+        [InlineData("FE80::e077:c9a3:eeba:b8e9", "%eth10")]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]


@davidsh do you consider porting on netfx?

I will mark it as "consider". But those things have longer lead times for review and it might not qualify due to strict app-compat guidelines.

MarcoRossignoli · 2018-05-17T20:44:47Z

Either way, it would be good to add more details for commits and the PR description.

you're right sorry, updated.

rmkerr · 2018-05-17T20:50:30Z

src/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs

+        [InlineData("Fe80::e077:c9a3:eeba:b8e9", "%18")]
+        [InlineData("fE80::e077:c9a3:eeba:b8e9", "%18")]
+        [InlineData("FE80::e077:c9a3:eeba:b8e9", "%18")]
+        [InlineData("FE80::e077:c9a3:eeba:b8e9", "%eth10")]


I think it would be nice to add some additional test cases here for more interesting scope IDs. What happens if we include an empty scope? Or scopes containing unusual characters?

What happens if we include an empty scope?

This test pass.
In this case what we want?Do we need to throw invalid address exception?Can we "parse" LLA without zone index?
IMHO if an address is LLA(start with fe80) we should throw exception, if i'm not mistaken LLA MUST have a zone index, what do you think?

Or scopes containing unusual characters?

With special chars like [InlineData("FE80::e077:c9a3:eeba:b8e9", "\u30AF\u20E7")] we get exception Invalid URI: The hostname could not be parsed. before reaching Ipv6 parser.

From rfc specification:

A <zone_id> SHOULD contain only ASCII characters classified as "unreserved" for use in URIs [RFC3986]. This excludes characters such as "]" or even "%" that would complicate parsing. However, the syntax described below does allow such characters to be percent- encoded, for compatibility with existing devices that use them.

here behaviour seems correct.

Throwing an exception with special characters seems like the right behavior, especially considering we do the same thing before the change.

On the other hand, throwing a new exception for an empty scope is risky. I think we could justify it if creating an LLA without a zone id really isn't a meaningful operation, but otherwise it's probably better to be more accepting of potentially valid addresses. As far as I can tell, the spec implies that the zone id should be non-null, but never says it explicitly. From RFC 4007 11.2:

An implementation MAY support other kinds of non-null strings as <zone_id>.

So without the spec explicitly forbidding null zone ids, I'd be inclined to stick with the behavior you have in the PR now. @davidsh, any thoughts on this?

@davidsh, any thoughts on this?

This PR should be focussed on one thing. And changing behavior for something else shouldn't be part of that.

So, no, don't start throwing a new exception for empty scope-id.

@rmkerr @davidsh i added empty scope-id test case, would you like also a test with "special characters" for zone id(throwing exception assert)?

It'd be nice to have that behavior pinned, so yes! That should be easy to implement using Assert.Throws. Also, could you try the case where we have the zone ID "%" with no scope ID following. It's another unusual case, but I would hope we get the same behavior as when we provide a totally empty zone ID.

MarcoRossignoli · 2018-05-18T19:14:40Z

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

rmkerr · 2018-05-18T20:09:27Z

@MarcoRossignoli The test failures you're seeing in Win10 Nano are a consequence of #29757 and are not related. I'm not sure about the OSX run, but that leg does tend to be fairly unstable.

MarcoRossignoli · 2018-05-18T21:03:00Z

src/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs

+        [InlineData("FE80::e077:c9a3:eeba:b8e9", "%eth10")]
+        [InlineData("FE80::e077:c9a3:eeba:b8e9", "")]
+        [InlineData("FE80::e077:c9a3:eeba:b8e9", "%")]
+        [InlineData("fe80::e077:c9a3:eeba:b8e9", "%\u30AF\u20E7")]


@rmkerr actually special characters works well...i forget % escape char in zone-id test data, however this behaviour is conform to rfc SHOULD contain only ASCII is not a MUST.

That sounds fine to me, if we allowed Unicode characters before we should continue to allow them. Thanks for updating the tests.

rmkerr

Assuming the tests pass in CI this looks good to me.

davidsh · 2018-05-19T00:49:40Z

CI failures in Windows are only from Nano: https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/12510/

stephentoub · 2018-05-21T13:21:45Z

Were outer loop tests run on this? Many outerloop Windows and Linux runs are failing, and it appears to be due to this, with errors from GetAsync_IPv6LinkLocalAddressUri_Success like "URL using bad/illegal format or missing URL" and "No such device or address"

This reverts commit e1ded5a.

Contributes to #28863 Replace PR #29769 after revert #29818 for Outerloop CI fail This PR address the second part of issue: Uri.Host LLA (Link-local address) IPv6 address doesn't contain %number part. Currently it returns [fe80::e077:c9a3:eeba:b8e9], it should return [fe80::e077:c9a3:eeba:b8e9%18]. Note: Uri.IdnHost correctly contains the %number part.

Contributes to dotnet/corefx#28863 This PR address the second part of issue: Uri.Host LLA (Link-local address) IPv6 address doesn't contain %number part. Currently it returns [fe80::e077:c9a3:eeba:b8e9], it should return [fe80::e077:c9a3:eeba:b8e9%18]. Note: Uri.IdnHost correctly contains the %number part. Commit migrated from dotnet/corefx@e1ded5a

…" (dotnet/corefx#29818) This reverts commit dotnet/corefx@e1ded5a. Commit migrated from dotnet/corefx@18a84ea

Contributes to dotnet/corefx#28863 Replace PR dotnet/corefx#29769 after revert dotnet/corefx#29818 for Outerloop CI fail This PR address the second part of issue: Uri.Host LLA (Link-local address) IPv6 address doesn't contain %number part. Currently it returns [fe80::e077:c9a3:eeba:b8e9], it should return [fe80::e077:c9a3:eeba:b8e9%18]. Note: Uri.IdnHost correctly contains the %number part. Commit migrated from dotnet/corefx@f7a8cf1

MarcoRossignoli added 5 commits May 16, 2018 22:49

wip

a3a8429

wip

e6319a2

wip

baa30aa

nit: spaces

a2454a0

nit: spaces

d6d3b7d

rmkerr added the area-System.Net label May 17, 2018

davidsh reviewed May 17, 2018

View reviewed changes

MarcoRossignoli added 2 commits May 17, 2018 22:27

address PR feedback

7a6270c

address PR feedback

5d017be

MarcoRossignoli commented May 17, 2018

View reviewed changes

davidsh added the netfx-port-consider label May 17, 2018

rmkerr reviewed May 17, 2018

View reviewed changes

MarcoRossignoli added 2 commits May 18, 2018 22:36

address PR feedback

5f0a0df

address PR feedback

a17309c

MarcoRossignoli commented May 18, 2018

View reviewed changes

rmkerr approved these changes May 18, 2018

View reviewed changes

davidsh added this to the 2.2.0 milestone May 19, 2018

davidsh approved these changes May 19, 2018

View reviewed changes

davidsh merged commit e1ded5a into dotnet:master May 19, 2018

MarcoRossignoli deleted the ipv6lla branch May 19, 2018 05:26

stephentoub added a commit that referenced this pull request May 21, 2018

Revert "Fix Uri.Host for IPv6 Link-local address (#29769)"

db7482e

This reverts commit e1ded5a.

stephentoub added a commit to stephentoub/corefx that referenced this pull request May 21, 2018

Revert "Fix Uri.Host for IPv6 Link-local address (dotnet#29769)"

9401171

This reverts commit e1ded5a.

stephentoub mentioned this pull request May 21, 2018

Revert "Fix Uri.Host for IPv6 Link-local address (#29769)" #29818

Merged

stephentoub added a commit that referenced this pull request May 21, 2018

Revert "Fix Uri.Host for IPv6 Link-local address (#29769)" (#29818)

18a84ea

This reverts commit e1ded5a.

MarcoRossignoli mentioned this pull request May 21, 2018

Fix Uri.Host for IPv6 Link-local address #29829

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Revert "Fix Uri.Host for IPv6 Link-local address (dotnet/corefx#29769)…

5623812

…" (dotnet/corefx#29818) This reverts commit dotnet/corefx@e1ded5a. Commit migrated from dotnet/corefx@18a84ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Uri.Host for IPv6 Link-local address #29769

Fix Uri.Host for IPv6 Link-local address #29769

MarcoRossignoli commented May 17, 2018 •

edited

Loading

davidsh commented May 17, 2018 •

edited

Loading

davidsh May 17, 2018

davidsh May 17, 2018 •

edited

Loading

MarcoRossignoli May 17, 2018

davidsh May 17, 2018

MarcoRossignoli commented May 17, 2018

rmkerr May 17, 2018

MarcoRossignoli May 17, 2018 •

edited

Loading

MarcoRossignoli May 17, 2018 •

edited

Loading

rmkerr May 18, 2018

davidsh May 18, 2018 •

edited

Loading

MarcoRossignoli May 18, 2018

rmkerr May 18, 2018

MarcoRossignoli commented May 18, 2018

rmkerr commented May 18, 2018

MarcoRossignoli May 18, 2018 •

edited

Loading

rmkerr May 18, 2018

rmkerr left a comment

davidsh commented May 19, 2018

stephentoub commented May 21, 2018

Fix Uri.Host for IPv6 Link-local address #29769

Fix Uri.Host for IPv6 Link-local address #29769

Conversation

MarcoRossignoli commented May 17, 2018 • edited Loading

davidsh commented May 17, 2018 • edited Loading

Choose a reason for hiding this comment

davidsh May 17, 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 May 17, 2018

Choose a reason for hiding this comment

MarcoRossignoli May 17, 2018 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh May 18, 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 May 18, 2018

rmkerr commented May 18, 2018

MarcoRossignoli May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmkerr left a comment

Choose a reason for hiding this comment

davidsh commented May 19, 2018

stephentoub commented May 21, 2018

MarcoRossignoli commented May 17, 2018 •

edited

Loading

davidsh commented May 17, 2018 •

edited

Loading

davidsh May 17, 2018 •

edited

Loading

MarcoRossignoli May 17, 2018 •

edited

Loading

MarcoRossignoli May 17, 2018 •

edited

Loading

davidsh May 18, 2018 •

edited

Loading

MarcoRossignoli May 18, 2018 •

edited

Loading