Skip to content

Conversation

@smaye81
Copy link
Contributor

@smaye81 smaye81 commented Apr 9, 2025

This ports the ipv6 fix from bufbuild/protovalidate-go#215.

For context see the description on the above PR. The summary is that this fixes the validation for some corner cases of IPv6 address validation. Namely:

  • Adds a check that an IPv6 address can't begin or end on a single colon.
  • Adds a check to fail-fast on invalid hextets.

@smaye81 smaye81 requested review from a user and timostamm April 9, 2025 15:04
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

This is a straight port of bufbuild/protovalidate-go#215, right?

Why aren't we removing failing cases from nonconforming.yaml?

@smaye81
Copy link
Contributor Author

smaye81 commented Apr 9, 2025

This is a straight port of bufbuild/protovalidate-go#215, right?

Why aren't we removing failing cases from nonconforming.yaml?

It is yeah. We didn't need to add any additional failing cases when we updated to v0.10.4 on main. I guess the validation logic on main correctly handled the new cases added in bufbuild/protovalidate#341.

I didn't add these to the nonconforming.yaml on the sayers/validation_upgrades branch because those new tests actually caused runtime / out-of-range errors when running against the new validation logic. So tl;dr - these tests were never added to nonconforming and this fixes them, so it just evens out.

@timostamm
Copy link
Member

those new tests actually caused runtime / out-of-range errors when running against the new validation logic

This means that there are two bugs in that branch:

  1. Access out of range on a string.
  2. Conformance executor crashing on this exception.

The second bug is fixed by #282.
The first bug is hopefully addressed by this PR.

@smaye81
Copy link
Contributor Author

smaye81 commented Apr 9, 2025

The second bug is fixed by #282. The first bug is hopefully addressed by this PR.

It is yeah. You can see the failures in #275, which is 0.10.4 with the ported validation logic but not the fix addressed by this PR.

@smaye81 smaye81 merged commit 9b4e69c into sayers/validation_upgrades Apr 9, 2025
1 check passed
@smaye81 smaye81 deleted the sayers/ipv6_fix branch April 9, 2025 16:25
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.

3 participants