Skip to content

fix: Request::getIPAddress() #6820

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Nov 7, 2022
Merged

fix: Request::getIPAddress() #6820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b3cf77
docs: fix typo
kenjis Nov 6, 2022
be79c1c
style: add empty lines
kenjis Nov 6, 2022
a48e27b
test: remove unneeded code
kenjis Nov 6, 2022
799832d
test: change the order of codes
kenjis Nov 6, 2022
0225b3e
fix: use header() instead of getServer() for header values
kenjis Nov 6, 2022
0580ece
test: update test code
kenjis Nov 6, 2022
ead7757
test: refactor test code
kenjis Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions system/HTTP/RequestTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,13 @@ public function getIPAddress(): string
$this->ipAddress = $this->getServer('REMOTE_ADDR');

if ($proxyIPs) {
foreach (['HTTP_X_FORWARDED_FOR', 'HTTP_CLIENT_IP', 'HTTP_X_CLIENT_IP', 'HTTP_X_CLUSTER_CLIENT_IP'] as $header) {
if (($spoof = $this->getServer($header)) !== null) {
foreach (['x-forwarded-for', 'client-ip', 'x-client-ip', 'x-cluster-client-ip'] as $header) {
$spoof = null;
$headerObj = $this->header($header);

if ($headerObj !== null) {
$spoof = $headerObj->getValue();

// Some proxies typically list the whole chain of IP
// addresses through which the client has reached us.
// e.g. client_ip, proxy_ip1, proxy_ip2, etc.
Expand Down
50 changes: 31 additions & 19 deletions tests/system/HTTP/IncomingRequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,10 @@ public function testGetIPAddressNormal()
{
$expected = '123.123.123.123';
$_SERVER['REMOTE_ADDR'] = $expected;
$this->request = new Request(new App());

$this->request = new Request(new App());
$this->request->populateHeaders();

$this->assertSame($expected, $this->request->getIPAddress());
// call a second time to exercise the initial conditional block in getIPAddress()
$this->assertSame($expected, $this->request->getIPAddress());
Expand All @@ -709,66 +712,75 @@ public function testGetIPAddressNormal()
public function testGetIPAddressThruProxy()
{
$expected = '123.123.123.123';
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$_SERVER['REMOTE_ADDR'] = '10.0.1.200';
$config = new App();
$config->proxyIPs = '10.0.1.200,192.168.5.0/24';
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);

$this->request = new Request($config);
$this->request->populateHeaders();

// we should see the original forwarded address
$this->assertSame($expected, $this->request->getIPAddress());
}

public function testGetIPAddressThruProxyInvalid()
{
$expected = '123.456.23.123';
$_SERVER['REMOTE_ADDR'] = '10.0.1.200';
$_SERVER['HTTP_X_FORWARDED_FOR'] = '123.456.23.123';
$expected = '10.0.1.200';
$_SERVER['REMOTE_ADDR'] = $expected;
$config = new App();
$config->proxyIPs = '10.0.1.200,192.168.5.0/24';
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);

$this->request = new Request($config);
$this->request->populateHeaders();

// spoofed address invalid
$this->assertSame('10.0.1.200', $this->request->getIPAddress());
$this->assertSame($expected, $this->request->getIPAddress());
}

public function testGetIPAddressThruProxyNotWhitelisted()
{
$expected = '123.456.23.123';
$_SERVER['REMOTE_ADDR'] = '10.10.1.200';
$expected = '10.10.1.200';
$_SERVER['REMOTE_ADDR'] = $expected;
$_SERVER['HTTP_X_FORWARDED_FOR'] = '123.456.23.123';
$config = new App();
$config->proxyIPs = '10.0.1.200,192.168.5.0/24';
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);

$this->request = new Request($config);
$this->request->populateHeaders();

// spoofed address invalid
$this->assertSame('10.10.1.200', $this->request->getIPAddress());
$this->assertSame($expected, $this->request->getIPAddress());
}

public function testGetIPAddressThruProxySubnet()
{
$expected = '123.123.123.123';
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$_SERVER['REMOTE_ADDR'] = '192.168.5.21';
$config = new App();
$config->proxyIPs = ['192.168.5.0/24'];
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);

$this->request = new Request($config);
$this->request->populateHeaders();

// we should see the original forwarded address
$this->assertSame($expected, $this->request->getIPAddress());
}

public function testGetIPAddressThruProxyOutofSubnet()
{
$expected = '123.123.123.123';
$_SERVER['REMOTE_ADDR'] = '192.168.5.21';
$expected = '192.168.5.21';
$_SERVER['REMOTE_ADDR'] = $expected;
$config = new App();
$config->proxyIPs = ['192.168.5.0/28'];
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$_SERVER['HTTP_X_FORWARDED_FOR'] = '123.123.123.123';
$this->request = new Request($config);
$this->request->populateHeaders();

// we should see the original forwarded address
$this->assertSame('192.168.5.21', $this->request->getIPAddress());
$this->assertSame($expected, $this->request->getIPAddress());
}

// @TODO getIPAddress should have more testing, to 100% code coverage
Expand Down
21 changes: 8 additions & 13 deletions tests/system/HTTP/MessageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,6 @@ protected function setUp(): void
$this->message = new Message();
}

protected function tearDown(): void
{
$this->message = null;
unset($this->message);
}

// We can only test the headers retrieved from $_SERVER
// This test might fail under apache.
public function testHeadersRetrievesHeaders()
Expand Down Expand Up @@ -246,23 +240,24 @@ public function testSetHeaderWithExistingArrayValuesAppendNullValue()

public function testPopulateHeadersWithoutContentType()
{
// fail path, if the CONTENT_TYPE doesn't exist
$original = $_SERVER;
$_SERVER = ['HTTP_ACCEPT_LANGUAGE' => 'en-us,en;q=0.50'];
$originalEnv = getenv('CONTENT_TYPE');

// fail path, if the CONTENT_TYPE doesn't exist
$_SERVER = ['HTTP_ACCEPT_LANGUAGE' => 'en-us,en;q=0.50'];
putenv('CONTENT_TYPE');

$this->message->populateHeaders();

$this->assertNull($this->message->header('content-type'));

putenv("CONTENT_TYPE={$originalEnv}");
$this->message->removeHeader('accept-language');
$_SERVER = $original; // restore so code coverage doesn't break
}

public function testPopulateHeadersWithoutHTTP()
{
// fail path, if arguement does't have the HTTP_*
// fail path, if argument doesn't have the HTTP_*
$original = $_SERVER;
$_SERVER = [
'USER_AGENT' => 'Mozilla/5.0 (iPad; U; CPU OS 3_2_1 like Mac OS X; en-us) AppleWebKit/531.21.10 (KHTML, like Gecko) Mobile/7B405',
Expand All @@ -273,6 +268,7 @@ public function testPopulateHeadersWithoutHTTP()

$this->assertNull($this->message->header('user-agent'));
$this->assertNull($this->message->header('request-method'));

$_SERVER = $original; // restore so code coverage doesn't break
}

Expand All @@ -288,7 +284,7 @@ public function testPopulateHeadersKeyNotExists()
$this->message->populateHeaders();

$this->assertSame('', $this->message->header('accept-charset')->getValue());
$this->message->removeHeader('accept-charset');

$_SERVER = $original; // restore so code coverage doesn't break
}

Expand All @@ -305,8 +301,7 @@ public function testPopulateHeaders()

$this->assertSame('text/html; charset=utf-8', $this->message->header('content-type')->getValue());
$this->assertSame('en-us,en;q=0.50', $this->message->header('accept-language')->getValue());
$this->message->removeHeader('content-type');
$this->message->removeHeader('accept-language');

$_SERVER = $original; // restore so code coverage doesn't break
}
}
5 changes: 5 additions & 0 deletions tests/system/HTTP/RequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,7 @@ public function testGetIPAddressThruProxy()
$config->proxyIPs = '10.0.1.200,192.168.5.0/24';
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);
$this->request->populateHeaders();

// we should see the original forwarded address
$this->assertSame($expected, $this->request->getIPAddress());
Expand All @@ -626,6 +627,7 @@ public function testGetIPAddressThruProxyInvalid()
$config->proxyIPs = '10.0.1.200,192.168.5.0/24';
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);
$this->request->populateHeaders();

// spoofed address invalid
$this->assertSame('10.0.1.200', $this->request->getIPAddress());
Expand All @@ -639,6 +641,7 @@ public function testGetIPAddressThruProxyNotWhitelisted()
$config->proxyIPs = '10.0.1.200,192.168.5.0/24';
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);
$this->request->populateHeaders();

// spoofed address invalid
$this->assertSame('10.10.1.200', $this->request->getIPAddress());
Expand All @@ -652,6 +655,7 @@ public function testGetIPAddressThruProxySubnet()
$config->proxyIPs = ['192.168.5.0/24'];
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);
$this->request->populateHeaders();

// we should see the original forwarded address
$this->assertSame($expected, $this->request->getIPAddress());
Expand All @@ -665,6 +669,7 @@ public function testGetIPAddressThruProxyOutofSubnet()
$config->proxyIPs = ['192.168.5.0/28'];
$_SERVER['HTTP_X_FORWARDED_FOR'] = $expected;
$this->request = new Request($config);
$this->request->populateHeaders();

// we should see the original forwarded address
$this->assertSame('192.168.5.21', $this->request->getIPAddress());
Expand Down