Skip to content

Commit 84f3748

Browse files
gr8mangr8man
committed
fix(HTTP): validate REMOTE_ADDR against proxyIPs in isSecure() and optimize tests
1 parent e7ee5b0 commit 84f3748

3 files changed

Lines changed: 176 additions & 76 deletions

File tree

‎system/HTTP/IncomingRequest.php‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -272,11 +272,31 @@ public function isSecure(): bool
272272
return true;
273273
}
274274

275-
if ($this->hasHeader('X-Forwarded-Proto') && $this->header('X-Forwarded-Proto')->getValue() === 'https') {
276-
return true;
275+
if ($this->hasHeader('X-Forwarded-Proto') || $this->hasHeader('Front-End-Https')) {
276+
$isFromTrustedProxy = false;
277+
$remoteAddr = $this->getServer('REMOTE_ADDR');
278+
279+
if ($remoteAddr !== null && is_array($this->config->proxyIPs)) {
280+
foreach (array_keys($this->config->proxyIPs) as $proxyIP) {
281+
if ($this->checkIPAgainstProxy($remoteAddr, (string) $proxyIP)) {
282+
$isFromTrustedProxy = true;
283+
break;
284+
}
285+
}
286+
}
287+
288+
if ($isFromTrustedProxy) {
289+
if ($this->hasHeader('X-Forwarded-Proto') && $this->header('X-Forwarded-Proto')->getValue() === 'https') {
290+
return true;
291+
}
292+
293+
if ($this->hasHeader('Front-End-Https') && ! empty($this->header('Front-End-Https')->getValue()) && strtolower($this->header('Front-End-Https')->getValue()) !== 'off') {
294+
return true;
295+
}
296+
}
277297
}
278298

279-
return $this->hasHeader('Front-End-Https') && ! empty($this->header('Front-End-Https')->getValue()) && strtolower($this->header('Front-End-Https')->getValue()) !== 'off';
299+
return false;
280300
}
281301

282302
/**

‎system/HTTP/RequestTrait.php‎

Lines changed: 31 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -85,69 +85,8 @@ public function getIPAddress(): string
8585
return $this->ipAddress = '0.0.0.0';
8686
}
8787

88-
// @TODO Extract all this IP address logic to another class.
8988
foreach ($proxyIPs as $proxyIP => $header) {
90-
// Check if we have an IP address or a subnet
91-
if (! str_contains($proxyIP, '/')) {
92-
// An IP address (and not a subnet) is specified.
93-
// We can compare right away.
94-
if ($proxyIP === $this->ipAddress) {
95-
$spoof = $this->getClientIP($header);
96-
97-
if ($spoof !== null) {
98-
$this->ipAddress = $spoof;
99-
break;
100-
}
101-
}
102-
103-
continue;
104-
}
105-
106-
// We have a subnet ... now the heavy lifting begins
107-
if (! isset($separator)) {
108-
$separator = $ipValidator($this->ipAddress, 'ipv6') ? ':' : '.';
109-
}
110-
111-
// If the proxy entry doesn't match the IP protocol - skip it
112-
if (! str_contains($proxyIP, $separator)) {
113-
continue;
114-
}
115-
116-
// Convert the REMOTE_ADDR IP address to binary, if needed
117-
if (! isset($ip, $sprintf)) {
118-
if ($separator === ':') {
119-
// Make sure we're having the "full" IPv6 format
120-
$ip = explode(':', str_replace('::', str_repeat(':', 9 - substr_count($this->ipAddress, ':')), $this->ipAddress));
121-
122-
for ($j = 0; $j < 8; $j++) {
123-
$ip[$j] = intval($ip[$j], 16);
124-
}
125-
126-
$sprintf = '%016b%016b%016b%016b%016b%016b%016b%016b';
127-
} else {
128-
$ip = explode('.', $this->ipAddress);
129-
$sprintf = '%08b%08b%08b%08b';
130-
}
131-
132-
$ip = vsprintf($sprintf, $ip);
133-
}
134-
135-
// Split the netmask length off the network address
136-
sscanf($proxyIP, '%[^/]/%d', $netaddr, $masklen);
137-
138-
// Again, an IPv6 address is most likely in a compressed form
139-
if ($separator === ':') {
140-
$netaddr = explode(':', str_replace('::', str_repeat(':', 9 - substr_count($netaddr, ':')), $netaddr));
141-
142-
for ($i = 0; $i < 8; $i++) {
143-
$netaddr[$i] = intval($netaddr[$i], 16);
144-
}
145-
} else {
146-
$netaddr = explode('.', $netaddr);
147-
}
148-
149-
// Convert to binary and finally compare
150-
if (strncmp($ip, vsprintf($sprintf, $netaddr), $masklen) === 0) {
89+
if ($this->checkIPAgainstProxy($this->ipAddress, (string) $proxyIP)) {
15190
$spoof = $this->getClientIP($header);
15291

15392
if ($spoof !== null) {
@@ -164,6 +103,36 @@ public function getIPAddress(): string
164103
return $this->ipAddress;
165104
}
166105

106+
/**
107+
* Checks if the given IP address matches the proxy IP/subnet.
108+
*/
109+
protected function checkIPAgainstProxy(string $ipAddress, string $proxyIP): bool
110+
{
111+
if (! str_contains($proxyIP, '/')) {
112+
return $proxyIP === $ipAddress;
113+
}
114+
115+
[$netAddr, $maskLen] = explode('/', $proxyIP, 2);
116+
117+
if (str_contains($ipAddress, ':') !== str_contains($netAddr, ':')) {
118+
return false;
119+
}
120+
121+
$ipPacked = inet_pton($ipAddress);
122+
$netPacked = inet_pton($netAddr);
123+
124+
if ($ipPacked === false || $netPacked === false) {
125+
return false;
126+
}
127+
128+
$toBits = static fn (string $packed) => implode('', array_map(
129+
static fn ($byte) => str_pad(decbin(ord($byte)), 8, '0', STR_PAD_LEFT),
130+
str_split($packed),
131+
));
132+
133+
return strncmp($toBits($ipPacked), $toBits($netPacked), (int) $maskLen) === 0;
134+
}
135+
167136
/**
168137
* Gets the client IP address from the HTTP header.
169138
*/

‎tests/system/HTTP/IncomingRequestTest.php‎

Lines changed: 122 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -750,22 +750,133 @@ public function testIsAJAX(): void
750750
$this->assertTrue($this->request->isAJAX());
751751
}
752752

753-
public function testIsSecure(): void
753+
#[DataProvider('provideIsSecure')]
754+
public function testIsSecure(array $server, array $proxyIPs, array $headers, bool $expected): void
754755
{
755-
service('superglobals')->setServer('HTTPS', 'on');
756-
$this->assertTrue($this->request->isSecure());
757-
}
756+
$superglobals = new Superglobals();
758757

759-
public function testIsSecureFrontEnd(): void
760-
{
761-
$this->request->appendHeader('Front-End-Https', 'on');
762-
$this->assertTrue($this->request->isSecure());
758+
foreach ($server as $key => $value) {
759+
$superglobals->setServer($key, $value);
760+
}
761+
Services::injectMock('superglobals', $superglobals);
762+
763+
$config = new App();
764+
$config->proxyIPs = $proxyIPs;
765+
$request = $this->createRequest($config);
766+
767+
foreach ($headers as $name => $value) {
768+
$request->appendHeader($name, $value);
769+
}
770+
771+
$this->assertSame($expected, $request->isSecure());
763772
}
764773

765-
public function testIsSecureForwarded(): void
774+
public static function provideIsSecure(): iterable
766775
{
767-
$this->request->appendHeader('X-Forwarded-Proto', 'https');
768-
$this->assertTrue($this->request->isSecure());
776+
yield from [
777+
'HTTPS on' => [
778+
'server' => ['HTTPS' => 'on'],
779+
'proxyIPs' => [],
780+
'headers' => [],
781+
'expected' => true,
782+
],
783+
'HTTPS ON case insensitive' => [
784+
'server' => ['HTTPS' => 'ON'],
785+
'proxyIPs' => [],
786+
'headers' => [],
787+
'expected' => true,
788+
],
789+
'HTTPS 1' => [
790+
'server' => ['HTTPS' => '1'],
791+
'proxyIPs' => [],
792+
'headers' => [],
793+
'expected' => true,
794+
],
795+
'HTTPS off' => [
796+
'server' => ['HTTPS' => 'off'],
797+
'proxyIPs' => [],
798+
'headers' => [],
799+
'expected' => false,
800+
],
801+
'HTTPS not set' => [
802+
'server' => [],
803+
'proxyIPs' => [],
804+
'headers' => [],
805+
'expected' => false,
806+
],
807+
'Front-End-Https on with trusted proxy' => [
808+
'server' => ['REMOTE_ADDR' => '10.0.1.200'],
809+
'proxyIPs' => ['10.0.1.200' => 'Front-End-Https'],
810+
'headers' => ['Front-End-Https' => 'on'],
811+
'expected' => true,
812+
],
813+
'Front-End-Https off with trusted proxy' => [
814+
'server' => ['REMOTE_ADDR' => '10.0.1.200'],
815+
'proxyIPs' => ['10.0.1.200' => 'Front-End-Https'],
816+
'headers' => ['Front-End-Https' => 'off'],
817+
'expected' => false,
818+
],
819+
'Front-End-Https on with untrusted proxy' => [
820+
'server' => ['REMOTE_ADDR' => '10.0.1.201'],
821+
'proxyIPs' => ['10.0.1.200' => 'Front-End-Https'],
822+
'headers' => ['Front-End-Https' => 'on'],
823+
'expected' => false,
824+
],
825+
'X-Forwarded-Proto https with trusted proxy' => [
826+
'server' => ['REMOTE_ADDR' => '10.0.1.200'],
827+
'proxyIPs' => ['10.0.1.200' => 'X-Forwarded-Proto'],
828+
'headers' => ['X-Forwarded-Proto' => 'https'],
829+
'expected' => true,
830+
],
831+
'X-Forwarded-Proto http with trusted proxy' => [
832+
'server' => ['REMOTE_ADDR' => '10.0.1.200'],
833+
'proxyIPs' => ['10.0.1.200' => 'X-Forwarded-Proto'],
834+
'headers' => ['X-Forwarded-Proto' => 'http'],
835+
'expected' => false,
836+
],
837+
'X-Forwarded-Proto https with untrusted proxy' => [
838+
'server' => ['REMOTE_ADDR' => '10.0.1.201'],
839+
'proxyIPs' => ['10.0.1.200' => 'X-Forwarded-Proto'],
840+
'headers' => ['X-Forwarded-Proto' => 'https'],
841+
'expected' => false,
842+
],
843+
'Front-End-Https on with trusted proxy subnet IPv4' => [
844+
'server' => ['REMOTE_ADDR' => '192.168.5.25'],
845+
'proxyIPs' => ['192.168.5.0/24' => 'Front-End-Https'],
846+
'headers' => ['Front-End-Https' => 'on'],
847+
'expected' => true,
848+
],
849+
'Front-End-Https on with untrusted proxy subnet IPv4' => [
850+
'server' => ['REMOTE_ADDR' => '192.168.6.25'],
851+
'proxyIPs' => ['192.168.5.0/24' => 'Front-End-Https'],
852+
'headers' => ['Front-End-Https' => 'on'],
853+
'expected' => false,
854+
],
855+
'X-Forwarded-Proto https with trusted proxy subnet IPv6' => [
856+
'server' => ['REMOTE_ADDR' => '2001:db8:1234::1'],
857+
'proxyIPs' => ['2001:db8:1234::/48' => 'X-Forwarded-Proto'],
858+
'headers' => ['X-Forwarded-Proto' => 'https'],
859+
'expected' => true,
860+
],
861+
'X-Forwarded-Proto https with untrusted proxy subnet IPv6' => [
862+
'server' => ['REMOTE_ADDR' => '2001:db8:1235::1'],
863+
'proxyIPs' => ['2001:db8:1234::/48' => 'X-Forwarded-Proto'],
864+
'headers' => ['X-Forwarded-Proto' => 'https'],
865+
'expected' => false,
866+
],
867+
'IPv4 client IP against IPv6 proxy subnet' => [
868+
'server' => ['REMOTE_ADDR' => '192.168.5.25'],
869+
'proxyIPs' => ['2001:db8:1234::/48' => 'X-Forwarded-Proto'],
870+
'headers' => ['X-Forwarded-Proto' => 'https'],
871+
'expected' => false,
872+
],
873+
'IPv6 client IP against IPv4 proxy subnet' => [
874+
'server' => ['REMOTE_ADDR' => '2001:db8:1234::1'],
875+
'proxyIPs' => ['192.168.5.0/24' => 'X-Forwarded-Proto'],
876+
'headers' => ['X-Forwarded-Proto' => 'https'],
877+
'expected' => false,
878+
],
879+
];
769880
}
770881

771882
public function testUserAgent(): void

0 commit comments

Comments
 (0)