Movatterモバイル変換

carsonbot added this to the6.3 milestone

GromNaN added the HttpFoundation label

carsonbot changed the title~~Add IpUtils::PRIVATE_SUBNETS constant~~[HttpFoundation] Add IpUtils::PRIVATE_SUBNETS constant

GromNaN reviewed

Copy link

Member

GromNaN left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Yes, providing a way to check if an IP address is public or private is a good idea and should be included in the framework. You would use the constant like this?

IpUtils::checkIp($ip, IpUtils::PRIVATE_SUBNETS);

I wonder if we shouldn't add a new public method instead: it would be more intuitive for users. This would allow for more logic in the future if needed and prevent potential confusion. Developers who are not familiar with network ranges may not understand the purpose of the constant or how it should be used, which could lead to security errors in their code.

src/Symfony/Component/HttpClient/NoPrivateNetworkHttpClient.phpShow resolvedHide resolved

Copy link

ContributorAuthor

danielburger1337 commentedMar 18, 2023•
edited
Loading

I wonder if we shouldn't add a new public method instead: it would be more intuitive for users. This would allow for more logic in the future if needed and prevent potential confusion. Developers who are not familiar with network ranges may not understand the purpose of the constant or how it should be used, which could lead to security errors in their code.

Something along the lines of the following could be added:

IpUtils::isPrivateIp(string$requestIp)

If we can agree that the method should be added, I will add it to the PR. I don't know too much about network ranges but I should be able to reuse the tests from theNoPrivateNetworkHttpClient where this logic originates.

danielburger1337 changed the title~~[HttpFoundation] Add IpUtils::PRIVATE_SUBNETS constant~~[HttpFoundation] Add IpUtils::isPrivateIp

GromNaN reviewed

Copy link

Member

GromNaN left a comment

Choose a reason for hiding this comment

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

Thanks.

src/Symfony/Component/HttpClient/NoPrivateNetworkHttpClient.phpShow resolvedHide resolved

src/Symfony/Component/HttpFoundation/IpUtils.phpShow resolvedHide resolved

GromNaN reviewed

src/Symfony/Component/HttpFoundation/CHANGELOG.mdShow resolvedHide resolved

danielburger1337 mentioned this pull request

Add IpUtils::isPrivateIp docssymfony/symfony-docs#18102

Merged

GromNaN approved these changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

[HttpFoundation] Add IpUtils::isPrivateIp

6471582

Copy link

Member

nicolas-grekas commentedMar 20, 2023

Thank you@danielburger1337.

nicolas-grekas merged commit38b5992 intosymfony:6.3