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

ipv6 validation allows invalid ipv6 addresses #206

Open
kurtextrem opened this issue Oct 8, 2023 · 5 comments
Open

ipv6 validation allows invalid ipv6 addresses #206

kurtextrem opened this issue Oct 8, 2023 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@kurtextrem
Copy link
Contributor

The current regexp used for checking ipv6 in valibot might not be correct, as it allows, e.g.:
fe80::3:bEFf:5b:%3NG which is not valid.

!/^(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))$/.test(

As per garycourt/uri-js#40 (comment), it looks like other regexps can not be used to fully make sure an ipv6 address is valid and the regexp used in uri-js seems quite expensive (although the issue is from 2019, so it might have changed nowadays).
In any case, the Fastify team is now using a different approach to parse ipv6 addresses: fastify/fast-uri@main/lib/utils.js#L27 (the mentioned "read buffer" approach is used there).

The buffer code is faster, but also increases bundle size a lot more compared to the regex. What do we prefer?

@fabian-hiller
Copy link
Owner

Fastify's code seems to be quite long. What if we use a mix of regex and JS code? The regex checks the basic structure and the JS code checks the details. This is what I found on Medium: https://medium.com/@stheodorejohn/validating-ipv4-and-ipv6-addresses-with-ease-unveiling-the-power-of-validation-in-javascript-2af04ee065c5

@fabian-hiller fabian-hiller self-assigned this Oct 8, 2023
@fabian-hiller fabian-hiller added the enhancement New feature or request label Oct 8, 2023
@kurtextrem
Copy link
Contributor Author

kurtextrem commented Oct 15, 2023

The Medium approach sounds good to me. Do you plan to make the change?

As alternative, https://github.com/colinhacks/zod/pull/2849/files also switched to a new ipv6 checking approach, maybe worth comparing the two (bundle size vs runtime perf)

@fabian-hiller
Copy link
Owner

Yes, I plan to improve that in the long run. However, I will not have the time in the next few weeks for it.

@langscot
Copy link

I believe the IPV6_CIDR_REGEX added in #367 could be modified and replace the current IPV6_REGEX, as it seems to be more correct.

fe80::3:bEFf:5b:%3NG/24 for example is flagged as invalid.

Down the line, we could investigate a faster solution, but for the scope of this issue I think this would be sufficient. Perhaps we can add a warning to the docblock + docs about execution time.

@fabian-hiller
Copy link
Owner

Thank you for the tip. Currently my focus is on other areas of the library but I will get back to this in the long run.

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

No branches or pull requests

3 participants