Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Added isset check for REMOTE_ADDR #5418

Closed
wants to merge 4 commits into from
Closed

Added isset check for REMOTE_ADDR #5418

wants to merge 4 commits into from

Conversation

AxaliaN
Copy link
Contributor

@AxaliaN 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.

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.
@Maks3w
Copy link
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.

@AxaliaN
Copy link
Contributor Author

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.

@weierophinney
Copy link
Member

@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...

@AxaliaN
Copy link
Contributor Author

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.

@AxaliaN
Copy link
Contributor Author

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.

Added a check to see if REMOTE_ADDR has been set.
@weierophinney
Copy link
Member

@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 added a commit that referenced this pull request Nov 6, 2013
Added isset check for REMOTE_ADDR
weierophinney added a commit that referenced this pull request Nov 6, 2013
@AxaliaN
Copy link
Contributor Author

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.

@ghost ghost assigned weierophinney Nov 6, 2013
weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-http that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants