Skip to content

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Nov 5, 2025

This PR blocks the use of IPv6 length prefixes (the /64 in "http://[AB::/64]/path").
These aren't allowed by the RFC3986 syntax and their usefulness is questionable, but they represent potential problems due to delimiter characters appearing inside the host.

I also restricted the characters allowed in a scope ID/zone ID to just unreserved or %, making it closer to RFC 6874 ('%' or unreserved), the concerns here being similar re: allowing reserved characters.
Note that RFC 6874 (from 2013) has been obsoleted recently (Aug 2025) by RFC 9844, which avoids recommending a specific syntax. This draft document also has more context on the use of link-local with Uri: https://datatracker.ietf.org/doc/html/draft-schinazi-httpbis-link-local-uri-bcp-03

While touching the code, I also removed the unused validateStrictAddress argument, and changed the method to accept the span including the leading '[', simplifying callers that had to do the +1 -1 dance.

@MihaZupan MihaZupan added this to the 11.0.0 milestone Nov 5, 2025
@MihaZupan MihaZupan self-assigned this Nov 5, 2025
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Do we have any negative tests for the previously accepted ipv6/number?

@MihaZupan MihaZupan marked this pull request as ready for review November 6, 2025 13:40
Copilot AI review requested due to automatic review settings November 6, 2025 13:40
@MihaZupan
Copy link
Member Author

We previously had two Uri test cases (http://[1111:2222:3333::431%16/20] and http://[1111:2222:3333::431/20) that I moved to the failing inputs

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes support for IPv6 prefix length notation (e.g., /20 suffix) in URI IPv6 addresses, as this notation is not part of standard URI specifications. The changes simplify the IPv6 address validation logic and ensure stricter adherence to URI standards.

Key changes:

  • Removed prefix length parsing logic from IPv6AddressHelper.IsValid()
  • Updated method to expect brackets in input and adjusted index calculations accordingly
  • Restricted allowed characters in IPv6 zone IDs to RFC3986 unreserved characters
  • Updated test cases to reflect that prefix notation is now invalid

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/libraries/System.Private.Uri/src/System/IPv6AddressHelper.cs Removed prefix length parsing logic, simplified validation, added zone ID character restrictions, and improved documentation
src/libraries/System.Private.Uri/src/System/Uri.cs Updated calls to IPv6AddressHelper.IsValid() to pass brackets and adjusted index calculations
src/libraries/System.Private.Uri/tests/FunctionalTests/UriEscapingTest.cs Made RFC3986Unreserved constant public for use in other tests
src/libraries/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs Added comprehensive zone ID character validation test, consolidated test methods, and removed framework-specific test
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Uri.CreateStringTests.cs Moved test cases with prefix notation from valid to invalid categories

@MihaZupan
Copy link
Member Author

/ba-g Failures are android timeouts dotnet/dnceng#6408

@MihaZupan MihaZupan merged commit 2e85cbd into dotnet:main Nov 6, 2025
84 of 96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants