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

improved client IP detection #65

Merged
merged 1 commit into from Jan 30, 2017

Conversation

Projects
None yet
2 participants
@Overtorment
Contributor

Overtorment commented Jan 27, 2017

On Heroku ip wasnt detected correctly. Thats a fix.

@oscarotero

This comment has been minimized.

Show comment
Hide comment
@oscarotero

oscarotero Jan 28, 2017

Owner

Hi, @Overtorment thanks for your contribution.
I think this change is unnecesary, because these headers are checked. The values of $server['HTTP_X_FORWARDED_FOR'] should be checked in the header X-Forwarded-For.

Owner

oscarotero commented Jan 28, 2017

Hi, @Overtorment thanks for your contribution.
I think this change is unnecesary, because these headers are checked. The values of $server['HTTP_X_FORWARDED_FOR'] should be checked in the header X-Forwarded-For.

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Jan 28, 2017

Contributor

@oscarotero thats what I thought too, but I deployed to https://heroku.com/ and got the wrong IP

Contributor

Overtorment commented Jan 28, 2017

@oscarotero thats what I thought too, but I deployed to https://heroku.com/ and got the wrong IP

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Jan 28, 2017

Contributor

hmmm. worth doublechecking, maybe smth to do with the order of headers...

Contributor

Overtorment commented Jan 28, 2017

hmmm. worth doublechecking, maybe smth to do with the order of headers...

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Jan 28, 2017

Contributor

image
@oscarotero am I doing smth wrong? ^^^
shows internal ip starting with 10.

Contributor

Overtorment commented Jan 28, 2017

image
@oscarotero am I doing smth wrong? ^^^
shows internal ip starting with 10.

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Jan 28, 2017

Contributor

ok, found the issue:

$server['REMOTE_ADDR'] takes priority since its always first in the array, and its a reverse proxy address on heroku
image

@oscarotero how would you advise to fix that? Ill prepare a PR

Contributor

Overtorment commented Jan 28, 2017

ok, found the issue:

$server['REMOTE_ADDR'] takes priority since its always first in the array, and its a reverse proxy address on heroku
image

@oscarotero how would you advise to fix that? Ill prepare a PR

@oscarotero

This comment has been minimized.

Show comment
Hide comment
@oscarotero

oscarotero Jan 28, 2017

Owner

Ok, I see.
Maybe an option to use REMOTE_ADDR, or change the priority.
A PR would be great. Thank you.

Owner

oscarotero commented Jan 28, 2017

Ok, I see.
Maybe an option to use REMOTE_ADDR, or change the priority.
A PR would be great. Thank you.

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Jan 28, 2017

Contributor

@oscarotero I have just rearranged code so that HTTP_X_FORWARDED_FOR is before REMOTE_ADDR

Contributor

Overtorment commented Jan 28, 2017

@oscarotero I have just rearranged code so that HTTP_X_FORWARDED_FOR is before REMOTE_ADDR

@oscarotero

This comment has been minimized.

Show comment
Hide comment
@oscarotero

oscarotero Jan 28, 2017

Owner

Your solution is not valid, because only cover your needs.
It's better to keep the code as was, just with the addition to an option to use or not remote_addr.

Owner

oscarotero commented Jan 28, 2017

Your solution is not valid, because only cover your needs.
It's better to keep the code as was, just with the addition to an option to use or not remote_addr.

@oscarotero

This comment has been minimized.

Show comment
Hide comment
@oscarotero

oscarotero Jan 28, 2017

Owner

I preffer to create a new PR for this.

Owner

oscarotero commented Jan 28, 2017

I preffer to create a new PR for this.

@wolfy-j wolfy-j referenced this pull request Jan 28, 2017

Closed

Ip detect options #66

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Jan 28, 2017

Contributor

Okay, I think its the best and simplest solution. Litterally no code written, just moved a bit.
Now headers (if any) take priority over $server['REMOTE_ADDR']
In case you explicitly need $server['REMOTE_ADDR'] you can disable headers via configuration.

Contributor

Overtorment commented Jan 28, 2017

Okay, I think its the best and simplest solution. Litterally no code written, just moved a bit.
Now headers (if any) take priority over $server['REMOTE_ADDR']
In case you explicitly need $server['REMOTE_ADDR'] you can disable headers via configuration.

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Jan 28, 2017

Contributor

@oscarotero I rebased it to 1 commit to keep git history tidy

Contributor

Overtorment commented Jan 28, 2017

@oscarotero I rebased it to 1 commit to keep git history tidy

@oscarotero

This comment has been minimized.

Show comment
Hide comment
@oscarotero

oscarotero Jan 29, 2017

Owner

Ok, I'm ok with this but there's one more thing:
The default config shoud be trust in REMOTE_ADDR, instead the headers, for security reasons and to keep backward compatibility, so the headers array should be empty by default. To ease the configuration of headers, the argument of headers() should have a default value:

Middlewares::clientIp(); //trust in REMOTE_ADDR
Middlewares::clientIp()->headers(); //Use the headers before
Middlewares::clientIp()->headers(['X-Forwarded']); //Use just this header before, instead all

The only change is empty the headers array and add it as the default value of headers method:

public function headers (array $headers = ['Forwarded', 'Forwarded-For', 'Client-Ip', 'X-Forwarded', 'X-Forwarded-For', 'X-Cluster-Client-Ip']) {
        $this->headers = $headers;

        return $this;
}
Owner

oscarotero commented Jan 29, 2017

Ok, I'm ok with this but there's one more thing:
The default config shoud be trust in REMOTE_ADDR, instead the headers, for security reasons and to keep backward compatibility, so the headers array should be empty by default. To ease the configuration of headers, the argument of headers() should have a default value:

Middlewares::clientIp(); //trust in REMOTE_ADDR
Middlewares::clientIp()->headers(); //Use the headers before
Middlewares::clientIp()->headers(['X-Forwarded']); //Use just this header before, instead all

The only change is empty the headers array and add it as the default value of headers method:

public function headers (array $headers = ['Forwarded', 'Forwarded-For', 'Client-Ip', 'X-Forwarded', 'X-Forwarded-For', 'X-Cluster-Client-Ip']) {
        $this->headers = $headers;

        return $this;
}
@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Jan 30, 2017

Contributor

@oscarotero fixed. Unit test as well

Contributor

Overtorment commented Jan 30, 2017

@oscarotero fixed. Unit test as well

@oscarotero oscarotero merged commit 10b9f92 into oscarotero:master Jan 30, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Scrutinizer No new issues
Details
@oscarotero

This comment has been minimized.

Show comment
Hide comment
@oscarotero

oscarotero Jan 30, 2017

Owner

Thank you

Owner

oscarotero commented Jan 30, 2017

Thank you

@Overtorment Overtorment deleted the Overtorment:client-ip-fix branch Jan 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment