-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
WIP: Make trusted_proxies configuration IPv6 compatible #13379
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
WIP: Make trusted_proxies configuration IPv6 compatible #13379
Conversation
This is based on the 3rdparty library rlanvin/php-ip. This library requires installation/activation of the GMP extension. Signed-off-by: Johan Fleury <[email protected]>
This must be removed when 3rparty changes are merged. Signed-off-by: Johan Fleury <[email protected]>
|
Thank you for your contribution 🎉 Unfortunately there is another pull request for ipv6 and trusted_proxies: #12535 😟
https://github.com/rlanvin/php-ip/ not sure if this is a good choice 🤔 |
|
Oh crap. I did a search before making this PR but for some reason I didn't find this one. It's your decision either you want to close this PR or not, but in my opinion, after taking a look at #12535, is that mine is simpler and does not rely on regex to parse IP addresses. There's still a lot of work to do to improve this PR. Tell me if I should keep working on it. |
|
Lets ask @nextcloud/server-triage |
|
Well your solution depends on a 3rdparty lib instead or regex, im not sure which is better and also the lib might use regex too. But I think the GMP dependency is the real killer here. But there is help: server/apps/workflowengine/lib/Check/RequestRemoteAddress.php Lines 121 to 153 in 954da26
We already have code that matches IPv4 and IPv6 addresses. I guess you can extract that to a new method which is then used by the workflow engine and the trusted domain code? |
That makes most sense. Thus I'm closing this PR. |
This is based on the 3rdparty library rlanvin/php-ip. This library
requires installation/activation of the GMP extension.
Warning: this might break existing installation.
Requires nextcloud/3rdparty#198.