feat: lock_to_ip custom function

[aheissenberger]

The current version 4.x with checking for other environment variables is a creat start but still does not capture alle environments. I need the option to merge some IP ranges to one IP and optional offer to add a getIP as a callable function would offer a solution to any environment.

[aheissenberger]

1. I suggest to remove the latest general enhancement to get the ip of the client and only check for REMOTE_ADDR in the default setup with $this->lock_to_ip===true to avoid security holes. Implementations should only check for the environment specific header.
2. As the result is only used as part of the hash and never as an IP address, the function also provides the possibility to return any other identity to lock the session to a specific client
3. optional setting a function could become a configuration option by its own in the constructor

Here is how the symphony framework handles this and their warnings when only trusting some headers: https://symfony.com/doc/current/deployment/proxies.html

[aheissenberger]

This change will also fix issue #7

[stefangabos]

this looks good! i will be at my desk later today and I will look more into it but 100% this will be merged into master! thank you!

[aheissenberger]

this looks good! i will be at my desk later today and I will look more into it but 100% this will be merged into master! thank you!

please also consider my suggestions related to revert the existing get_ip_address to this only:

isset($_SERVER['REMOTE_ADDR'])

This will be a less breaking change for people upgrading from version 3.x and all of the requirements of the new function get_ip_address in version 4.1 will be possible with my suggested change. My current code checks for $this->lock_to_ip===true and $this->lock_to_ip===false to ensure that the new feature is not breaking any code with relies on the old behavior.

If you agree I would adapt my pull request to remove the new function get_ip_address

p.s.: thank you for this great library 😃

[stefangabos]

the last commit for removing get_ip_address is a no-go because I'd like this to work for most users - even those behind proxies - without having to do much work, preferably none at all.

i will do a commit in a few minutes with your idea but with minor changes here and there (i used a slightly different logic because in reality lock_to_ip can be truthy - both 1 and boolean true and everything in between) and with better docs for the lock_to_ip argument of the constructor.

[stefangabos]

I just realised that your name is in the source code at line 534! :) So you've been using this for a LONG time now! Thank you for that and for your contributions!

[stefangabos]

ok, regarding the removing the get_ip_address function I propose we release 4.1.0 and see if it affects people negatively and if it does we'll go your way

[aheissenberger]

ok, regarding the removing the get_ip_address function I propose we release 4.1.0 and see if it affects people negatively and if it does we'll go your way

I can live with not removing the get_ip_address but the current implementation is not a good implementation for AWS and not secure by looking into multiple values. I you read the link I posted from the symphony team you see their warning regarding only looking into some values and not checking some other values. I believe, this is not something this library should handle and my implementation gives everyman the freedom to fine tune to their environment and does not change anything for existing users.

[stefangabos]

allright, done. I went your way. docs were updated, get_ip_address was removed and instead used in the example for the lock_to_ip argument

[stefangabos]

4.1.0 was released - thanks a lot for your contribution, I really appreciate it!