Added isset check for REMOTE_ADDR #5418

Closed
wants to merge 4 commits into
from

Projects

None yet

3 participants

Contributor
AxaliaN commented Nov 5, 2013

getIpAddressFromProxy failed while unit-testing, because there is no REMOTE_ADDR set. Now there is a check to see if it is set before trying to check if it is in the trustedProxies array.

@AxaliaN AxaliaN Added isset check for REMOTE_ADDR
getIpAddressFromProxy failed while unit-testing, because there is no REMOTE_ADDR set. Now there is a check to see if it is set before trying to check if it is in the trustedProxies array.
f3b0e99
Member
Maks3w commented Nov 5, 2013

Can you provide a unit test?

I don't see any unit test broken in Travis-CI due this thing.

Contributor
AxaliaN commented Nov 5, 2013

I will provide a unit test shortly, however, the reason that Travis-CI probably didn't break, is because they (probably) supply a REMOTE_ADDR in the $_SERVER global.

Owner

@AxaliaN Um... I know I do not set REMOTE_ADDR or any other $_SERVER globals when running unit tests, and the tests run fine for me. I can't say I'm convinced we need this...

Contributor
AxaliaN commented Nov 6, 2013

Try running it on Windows, our entire build failed because there was no check for this ;) I can reproduce it any time, with a simple call to getIpAddress.

The code checks if $_SERVER['REMOTE_ADDR'] is in an array, but just assumes this variable is set. Apparently, this isn't the case all of the time, so a check is in order imho.

Contributor
AxaliaN commented Nov 6, 2013

Just to make my case a little stronger: I just pushed a commit to remove trailing spaces from the unit test, and Travis fails with this error:

There was 1 error:

  1. ZendTest\Http\PhpEnvironment\RemoteAddressTest::testGetIpAddressReturnsEmptyStringOnNoRemoteAddr
    Undefined index: REMOTE_ADDR

/home/travis/build/zendframework/zf2/tests/ZendTest/Http/PhpEnvironment/RemoteAddressTest.php:140

Which is the exact same error RemoveAddress.php gives.

I will fix my unit test for this, but I hope you understand now how there needs to be a check to see if the REMOTE_ADDR key is available.

@AxaliaN AxaliaN Fixed unit test
Added a check to see if REMOTE_ADDR has been set.
5e8ad0f
Owner

@AxaliaN To be honest, I'm not terribly concerned about tests not running on Windows; Windows is a very rare platform when it comes to deployment. I'll merge anyways, but I don't see this as critical by any means.

@weierophinney weierophinney added a commit that referenced this pull request Nov 6, 2013
@weierophinney weierophinney Merge branch 'hotfix/5418' into develop
Forward port #5418
515063f
Contributor
AxaliaN commented Nov 6, 2013

Thanks. It didn't just failf or us on windows, but while building using Jenkins as well. That's why it was critical, at least in our case.

@weierophinney weierophinney was assigned Nov 6, 2013
@weierophinney weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#5418 from AxaliaN/patch-1
Added isset check for REMOTE_ADDR
e16deed
@weierophinney weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/5418' 94f241e
@weierophinney weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/5418' into develop 3919bc8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment