Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix]: Fix IPv6 validation #1150

Merged
merged 2 commits into from
Nov 25, 2022

Conversation

daenney
Copy link
Member

@daenney daenney commented Nov 25, 2022

The current code considers ff00::/8 valid, but contrary to the comment that's not the global unicast range. ff-prefixes in IPv6 denote multicast.

This adapts the code to take the same approach as IPv4, explicitly blacklisting reserved internal/private ranges.

@daenney
Copy link
Member Author

daenney commented Nov 25, 2022

An alternative would be to whitelist the allocated ranges from https://www.iana.org/assignments/ipv6-unicast-address-assignments/ipv6-unicast-address-assignments.xhtml instead.

The current code considers ff00::/8 valid, but contrary to the comment
that's not the global unicast range. ff-prefixes in IPv6 denote
multicast.

This adapts the code to take the same approach as IPv4, explicitly
blacklisting reserved internal/private ranges.
@daenney
Copy link
Member Author

daenney commented Nov 25, 2022

If I'm reading the code right and seeing how this is always used through Sanitize on outgoing requests, then I think most outgoing communication with IPv6-enabled instances might be failing (assuming your own instance is reachable over IPv6 in the first place).

@tsmethurst
Copy link
Contributor

I will defer to @NyaaaWhatsUpDoc on this one since she authored the sanitize code :)

@daenney
Copy link
Member Author

daenney commented Nov 25, 2022

I double-checked this in the Go Playground with an IPv6 address of one of my boxes: https://go.dev/play/p/X-VMfnKuQkT.

That results in false, whereas it's very much a valid IPv6 unicast address.

@NyaaaWhatsUpDoc
Copy link
Member

Yes that is very much an oops on my behalf :')

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc merged commit e6cd81b into superseriousbusiness:main Nov 25, 2022
@daenney daenney deleted the ipv6-allowed branch November 25, 2022 23:28
@daenney
Copy link
Member Author

daenney commented Nov 25, 2022

Thanks for checking and merging this so fast!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants