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

[HttpFoundation] Add IpUtils::isPrivateIp #49726

Merged
merged 1 commit into from
Mar 20, 2023
Merged

[HttpFoundation] Add IpUtils::isPrivateIp #49726

merged 1 commit into from
Mar 20, 2023

Conversation

danielburger1337
Copy link
Contributor

@danielburger1337 danielburger1337 commented Mar 18, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR symfony/symfony-docs#18102

This is only my second PR for this project, so I hope I followed all the guidelines correctly.
Recently I had more and more use cases where I had to make exceptions (mostly rate limiting) for private IP ranges.

Symfony currently does not provide an easy way to check if an IP is private or public but implements such logic internally (private constant) for the NoPrivateNetworkHttpClient.

This PR intents to make the private subnet list reusable by adding it to IpUtils.

In the original PR of the NoPrivateNetworkHttpClient it was also briefly mentioned that this constant may have value when made public. #35566 (comment)

I think symfony should and always should have exposed this constant.

@carsonbot carsonbot added this to the 6.3 milestone Mar 18, 2023
@carsonbot carsonbot changed the title Add IpUtils::PRIVATE_SUBNETS constant [HttpFoundation] Add IpUtils::PRIVATE_SUBNETS constant Mar 18, 2023
Copy link
Member

@GromNaN GromNaN left a comment

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.

@danielburger1337
Copy link
Contributor Author

danielburger1337 commented Mar 18, 2023

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 the NoPrivateNetworkHttpClient where this logic originates.

@danielburger1337 danielburger1337 changed the title [HttpFoundation] Add IpUtils::PRIVATE_SUBNETS constant [HttpFoundation] Add IpUtils::isPrivateIp Mar 20, 2023
Copy link
Member

@GromNaN 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.

@nicolas-grekas
Copy link
Member

Thank you @danielburger1337.

@nicolas-grekas nicolas-grekas merged commit 38b5992 into symfony:6.3 Mar 20, 2023
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Mar 21, 2023
This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

Add IpUtils::isPrivateIp docs

References symfony/symfony#49726

Commits
-------

990596e Add IpUtils::isPrivateIp docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants