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

fix(tests): update IPv6 test to align with PHP 8.4.3 behavior #885

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

xHeaven
Copy link
Contributor

@xHeaven xHeaven commented Jan 18, 2025

This PR aims to fix the issue that came with PHP 8.4.3 regarding reserved IPv6 address handling. From PHP 8.4.3, you need to add an additional flag to allow checking for global reserved IPv6 addresses, otherwise only local ones will be taken into account (for example ::1).

@xHeaven
Copy link
Contributor Author

xHeaven commented Jan 18, 2025

Reverted my changes here but kept the skipping removed to see whether it's actually just Windows failing, or it's a global issue. Apparently, it affects Linux as well.

Weird, to say the least. I'll mark this PR as draft, I'm confident this is a GH Actions issue, let's not merge this one, at least until we get more information on this. Feel free to close if you wish.

@xHeaven xHeaven marked this pull request as draft January 18, 2025 13:59
@xHeaven xHeaven changed the title fix(ci): use manual check for reserved IPv6 range to fix Windows refactor(ipv6): apply proper flag to check for reserved IPv6 ranges since PHP 8.4.3 Jan 18, 2025
@xHeaven
Copy link
Contributor Author

xHeaven commented Jan 18, 2025

Since PHP 8.4.3, you need an additional flag to check global reserved IPv6 addresses.

See these for more details:
php/php-src#16944
php/php-src@d25aac2
https://www.php.net/manual/en/filter.constants.php#constant.filter-flag-no-res-range
https://www.php.net/manual/en/filter.constants.php#constant.filter-flag-global-range

With this in mind, one question remains, though. Do we want to add a switch for this flag? What if someone only wants to restrict locally reserved ranges but not globally reserved ones? Seems like an extreme edge-case to me, but hey.

@xHeaven xHeaven marked this pull request as ready for review January 18, 2025 18:48
@aidan-casey aidan-casey self-requested a review January 18, 2025 19:03
@aidan-casey
Copy link
Member

@xHeaven let me take a look at this please. The test case is testing an address that falls in 2001:db8::/32 which should be reserved for documentation I believe, but would like to confirm.

@xHeaven
Copy link
Contributor Author

xHeaven commented Jan 18, 2025

@xHeaven let me take a look at this please. The test case is testing an address that falls in 2001:db8::/32 which should be reserved for documentation I believe, but would like to confirm.

That's right - according to RFC 3849 - however, FILTER_FLAG_NO_RES_RANGE adheres to RFC 6890.

Since 2001:db8::/32 is not Reserved-by-Protocol, it passes when you only use FILTER_FLAG_NO_RES_RANGE.
::1 is marked Reserved-by-Protocol, so it fails under FILTER_FLAG_NO_RES_RANGE.

So with only FILTER_FLAG_NO_RES_RANGE:

  • ::1 → blocked (because it’s reserved by protocol).
  • 2001:db8:: → not blocked (because it’s not reserved by protocol).

With the FILTER_FLAG_GLOBAL_RANGE, 2001:db8::/32 is not global (Global = False), so it fails. Consequently, 2001:db8:: is considered "reserved" (or simply "blocked") for purposes of the global range filter, even though it’s not Reserved-by-Protocol.

So with the FILTER_FLAG_GLOBAL_RANGE flag applied:

  • ::1 → blocked anyway (reserved-by-protocol, also not global).
  • 2001:db8:: → also blocked now (because it isn’t global).

@xHeaven
Copy link
Contributor Author

xHeaven commented Jan 19, 2025

FYI, src/Tempest/Validation/src/Rules/IP.php also has to be modified in order to work as expected, since it also accepts IPv6 addresses and has the isValid() method that can decide whether a reserved address is valid or not.

... or we can close this entire PR and change the test to check ::1 instead of the current IPv6 addy.

@aidan-casey
Copy link
Member

Okay, here's what I think we should do here...

Let's defer to PHP on the behavior for allowReservedRange and update the test IP address accordingly. I don't think we need any other changes.

We should probably also add an option to this rule regarding the global range, but that probably deserves a different PR.

Thanks for your research and detail here, @xHeaven!

@xHeaven xHeaven changed the title refactor(ipv6): apply proper flag to check for reserved IPv6 ranges since PHP 8.4.3 refactor(tests): change IPv6 test to follow PHP 8.4.3's behavior Jan 23, 2025
@xHeaven
Copy link
Contributor Author

xHeaven commented Jan 23, 2025

Okay, here's what I think we should do here...

Let's defer to PHP on the behavior for allowReservedRange and update the test IP address accordingly. I don't think we need any other changes.

We should probably also add an option to this rule regarding the global range, but that probably deserves a different PR.

Thanks for your research and detail here, @xHeaven!

Sounds good to me and honestly, makes more sense than the original approach of changing the code.
I've updated the PR accordingly. There is one failing test, perhaps you could retry it? It has nothing to do with the PR itself; it looks like there was a momentary error in the PHP setup action.

@aidan-casey aidan-casey changed the title refactor(tests): change IPv6 test to follow PHP 8.4.3's behavior fix(tests): update IPv6 test to align with PHP 8.4.3 behavior Jan 23, 2025
@aidan-casey aidan-casey merged commit dec5c2f into tempestphp:main Jan 23, 2025
67 checks passed
@aidan-casey
Copy link
Member

Thanks!

@xHeaven xHeaven deleted the patch-1 branch January 25, 2025 01:15
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.

2 participants