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

Add ip() helper to request #523

Merged

Conversation

girardinsamuel
Copy link
Contributor

Allow to retrieve client IP easily from request instance.

Copy link
Member

@josephmancuso josephmancuso left a comment

Choose a reason for hiding this comment

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

Getting the IP Address is tricky I think we need to take into consideration HTTP_X_FORWARDED_FOR https://stackoverflow.com/questions/7835030/obtaining-client-ip-address-from-a-wsgi-app-using-eventlet

@girardinsamuel
Copy link
Contributor Author

girardinsamuel commented Feb 6, 2022

Getting the IP Address is tricky I think we need to take into consideration HTTP_X_FORWARDED_FOR https://stackoverflow.com/questions/7835030/obtaining-client-ip-address-from-a-wsgi-app-using-eventlet

Hmm you're right !
Laravel is using Symfony function to handle it and here is how Symfony is handling it:
https://github.com/symfony/symfony/blob/6.1/src/Symfony/Component/HttpFoundation/Request.php#L758

Interesting ! They indeed use HTTP_X_FORWARDED_FOR

@josephmancuso
Copy link
Member

josephmancuso commented Feb 6, 2022

It might be good to maybe somehow take into account a user defined order of getting the IP? Thinking in the event that some Azure or AWS setup has custom proxy or something.

Maybe make this a middleware?

See this answer:

public function getIp(){
    foreach (array('HTTP_CLIENT_IP', 'HTTP_X_FORWARDED_FOR', 'HTTP_X_FORWARDED', 'HTTP_X_CLUSTER_CLIENT_IP', 'HTTP_FORWARDED_FOR', 'HTTP_FORWARDED', 'REMOTE_ADDR') as $key){
        if (array_key_exists($key, $_SERVER) === true){
            foreach (explode(',', $_SERVER[$key]) as $ip){
                $ip = trim($ip); // just to be safe
                if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE) !== false){
                    return $ip;
                }
            }
        }
    }
    return request()->ip(); // it will return server ip when no client ip found
}

https://stackoverflow.com/questions/33268683/how-to-get-client-ip-address-in-laravel-5

@josephmancuso
Copy link
Member

josephmancuso commented Feb 6, 2022

Maybe we make it a middleware like this:

class IPMiddleware(Middleware):

    ___headers__ = [
         'HTTP_CLIENT_IP', 
         'HTTP_X_FORWARDED_FOR', 
         'HTTP_X_FORWARDED', 
         'HTTP_X_CLUSTER_CLIENT_IP', 
         'HTTP_FORWARDED_FOR', 
         'HTTP_FORWARDED', 
         'REMOTE_ADDR'
    ]

    #..

and you can inherit the middleware and overwrite this list if you needed a custom order lookup?

@girardinsamuel
Copy link
Contributor Author

Maybe we make it a middleware like this:

class IPMiddleware(Middleware):

    ___headers__ = [
         'HTTP_CLIENT_IP', 
         'HTTP_X_FORWARDED_FOR', 
         'HTTP_X_FORWARDED', 
         'HTTP_X_CLUSTER_CLIENT_IP', 
         'HTTP_FORWARDED_FOR', 
         'HTTP_FORWARDED', 
         'REMOTE_ADDR'
    ]

    #..

and you can inherit the middleware and overwrite this list if you needed a custom order lookup?

That's smart I like it ! So the middleware would add the ip in the request ? How would you add request.ip() from the middleware ?

@josephmancuso
Copy link
Member

umm. we can add an ip method to the request that returns request._ip and the middleware would just populate the _ip attribute

@girardinsamuel
Copy link
Contributor Author

What do you think of this ?

@girardinsamuel girardinsamuel self-assigned this Feb 7, 2022
@josephmancuso
Copy link
Member

Looks good. Ill take a better look tonight

@josephmancuso josephmancuso merged commit d088a5a into MasoniteFramework:4.0 Feb 12, 2022
@girardinsamuel girardinsamuel deleted the feat/add-request-ip branch February 14, 2022 09:13
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.

None yet

2 participants