Skip to content

Commit ecbf044

Browse files
michalsnmichalsn
authored
Merge commit from fork
* fix: validate forwarded HTTPS headers against trusted proxies * apply note about dual-stack servers * apply method visibility change
1 parent 72476b3 commit ecbf044

9 files changed

Lines changed: 333 additions & 148 deletions

File tree

‎system/HTTP/IncomingRequest.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,10 @@ public function isSecure(): bool
272272
return true;
273273
}
274274

275+
if (! $this->isFromTrustedProxy()) {
276+
return false;
277+
}
278+
275279
if ($this->hasHeader('X-Forwarded-Proto') && $this->header('X-Forwarded-Proto')->getValue() === 'https') {
276280
return true;
277281
}

‎system/HTTP/RequestTrait.php‎

Lines changed: 87 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -87,67 +87,7 @@ public function getIPAddress(): string
8787

8888
// @TODO Extract all this IP address logic to another class.
8989
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) {
90+
if ($this->checkIPAgainstProxy($this->ipAddress, (string) $proxyIP)) {
15191
$spoof = $this->getClientIP($header);
15292

15393
if ($spoof !== null) {
@@ -192,6 +132,92 @@ private function getClientIP(string $header): ?string
192132
return $spoof;
193133
}
194134

135+
/**
136+
* Checks if the request comes from one of the trusted proxies
137+
* configured in Config\App::$proxyIPs.
138+
*/
139+
protected function isFromTrustedProxy(): bool
140+
{
141+
$proxyIPs = $this->config->proxyIPs;
142+
143+
if (! is_array($proxyIPs) || $proxyIPs === []) {
144+
return false;
145+
}
146+
147+
$remoteAddr = $this->getServer('REMOTE_ADDR');
148+
149+
if (! is_string($remoteAddr) || $remoteAddr === '') {
150+
return false;
151+
}
152+
153+
foreach (array_keys($proxyIPs) as $proxyIP) {
154+
if ($this->checkIPAgainstProxy($remoteAddr, (string) $proxyIP)) {
155+
return true;
156+
}
157+
}
158+
159+
return false;
160+
}
161+
162+
/**
163+
* Checks if the given IP address matches the trusted proxy entry,
164+
* which may be a single IP address or a subnet in CIDR notation.
165+
* Supports both IPv4 and IPv6.
166+
*/
167+
private function checkIPAgainstProxy(string $ip, string $proxyIP): bool
168+
{
169+
$maskLength = null;
170+
171+
if (str_contains($proxyIP, '/')) {
172+
[$proxyIP, $mask] = explode('/', $proxyIP, 2);
173+
174+
if ($mask === '' || ! ctype_digit($mask)) {
175+
return false;
176+
}
177+
178+
$maskLength = (int) $mask;
179+
}
180+
181+
$binaryIP = inet_pton($ip);
182+
$binaryProxy = inet_pton($proxyIP);
183+
184+
if ($binaryIP === false || $binaryProxy === false) {
185+
return false;
186+
}
187+
188+
// If the proxy entry doesn't match the IP protocol - no match
189+
if (strlen($binaryIP) !== strlen($binaryProxy)) {
190+
return false;
191+
}
192+
193+
if ($maskLength === null) {
194+
return $binaryIP === $binaryProxy;
195+
}
196+
197+
if ($maskLength > strlen($binaryIP) * 8) {
198+
return false;
199+
}
200+
201+
if ($maskLength === 0) {
202+
return true;
203+
}
204+
205+
$fullBytes = intdiv($maskLength, 8);
206+
$remainingBits = $maskLength % 8;
207+
208+
if ($fullBytes > 0 && strncmp($binaryIP, $binaryProxy, $fullBytes) !== 0) {
209+
return false;
210+
}
211+
212+
if ($remainingBits > 0) {
213+
$bitmask = 0xFF & (0xFF << (8 - $remainingBits));
214+
215+
return (ord($binaryIP[$fullBytes]) & $bitmask) === (ord($binaryProxy[$fullBytes]) & $bitmask);
216+
}
217+
218+
return true;
219+
}
220+
195221
/**
196222
* Fetch an item from the $_SERVER array.
197223
*

‎tests/system/HTTP/IncomingRequestTest.php‎

Lines changed: 77 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -762,16 +762,86 @@ public function testIsSecure(): void
762762
$this->assertTrue($this->request->isSecure());
763763
}
764764

765-
public function testIsSecureFrontEnd(): void
766-
{
767-
$this->request->appendHeader('Front-End-Https', 'on');
768-
$this->assertTrue($this->request->isSecure());
765+
/**
766+
* @param array<string, string> $proxyIPs
767+
*/
768+
#[DataProvider('provideIsSecureWithForwardedHeaders')]
769+
public function testIsSecureWithForwardedHeaders(
770+
string $header,
771+
string $value,
772+
string $remoteAddr,
773+
array $proxyIPs,
774+
bool $expected,
775+
): void {
776+
service('superglobals')->setServer('REMOTE_ADDR', $remoteAddr);
777+
778+
$config = new App();
779+
$config->proxyIPs = $proxyIPs;
780+
781+
$request = $this->createRequest($config);
782+
$request->appendHeader($header, $value);
783+
784+
$this->assertSame($expected, $request->isSecure());
769785
}
770786

771-
public function testIsSecureForwarded(): void
787+
/**
788+
* @return iterable<string, array{string, string, string, array<string, string>, bool}>
789+
*/
790+
public static function provideIsSecureWithForwardedHeaders(): iterable
772791
{
773-
$this->request->appendHeader('X-Forwarded-Proto', 'https');
774-
$this->assertTrue($this->request->isSecure());
792+
yield from [
793+
'X-Forwarded-Proto trusted proxy IP' => [
794+
'X-Forwarded-Proto', 'https', '10.0.1.200', ['10.0.1.200' => 'X-Forwarded-For'], true,
795+
],
796+
'X-Forwarded-Proto no trusted proxies' => [
797+
'X-Forwarded-Proto', 'https', '10.0.1.200', [], false,
798+
],
799+
'X-Forwarded-Proto untrusted proxy IP' => [
800+
'X-Forwarded-Proto', 'https', '10.0.1.201', ['10.0.1.200' => 'X-Forwarded-For'], false,
801+
],
802+
'X-Forwarded-Proto trusted subnet' => [
803+
'X-Forwarded-Proto', 'https', '192.168.5.21', ['192.168.5.0/24' => 'X-Forwarded-For'], true,
804+
],
805+
'X-Forwarded-Proto out of trusted subnet' => [
806+
'X-Forwarded-Proto', 'https', '192.168.6.21', ['192.168.5.0/24' => 'X-Forwarded-For'], false,
807+
],
808+
'X-Forwarded-Proto trusted IPv6 subnet' => [
809+
'X-Forwarded-Proto', 'https', '2001:db8::5', ['2001:db8::/32' => 'X-Forwarded-For'], true,
810+
],
811+
'X-Forwarded-Proto out of trusted IPv6 subnet' => [
812+
'X-Forwarded-Proto', 'https', '2001:db9::5', ['2001:db8::/32' => 'X-Forwarded-For'], false,
813+
],
814+
'X-Forwarded-Proto trusted proxy but http' => [
815+
'X-Forwarded-Proto', 'http', '10.0.1.200', ['10.0.1.200' => 'X-Forwarded-For'], false,
816+
],
817+
'Front-End-Https trusted proxy IP' => [
818+
'Front-End-Https', 'on', '10.0.1.200', ['10.0.1.200' => 'X-Forwarded-For'], true,
819+
],
820+
'Front-End-Https no trusted proxies' => [
821+
'Front-End-Https', 'on', '10.0.1.200', [], false,
822+
],
823+
'Front-End-Https trusted proxy but off' => [
824+
'Front-End-Https', 'off', '10.0.1.200', ['10.0.1.200' => 'X-Forwarded-For'], false,
825+
],
826+
'invalid proxy IP string' => [
827+
'X-Forwarded-Proto', 'https', '10.0.1.200', ['not an ip' => 'X-Forwarded-For'], false,
828+
],
829+
'invalid proxy CIDR mask' => [
830+
'X-Forwarded-Proto', 'https', '192.168.5.21', ['192.168.5.0/foo' => 'X-Forwarded-For'], false,
831+
],
832+
'empty proxy CIDR mask' => [
833+
'X-Forwarded-Proto', 'https', '192.168.5.21', ['192.168.5.0/' => 'X-Forwarded-For'], false,
834+
],
835+
'negative proxy CIDR mask' => [
836+
'X-Forwarded-Proto', 'https', '192.168.5.21', ['192.168.5.0/-1' => 'X-Forwarded-For'], false,
837+
],
838+
'out of range IPv4 proxy CIDR mask' => [
839+
'X-Forwarded-Proto', 'https', '192.168.5.21', ['192.168.5.0/33' => 'X-Forwarded-For'], false,
840+
],
841+
'out of range IPv6 proxy CIDR mask' => [
842+
'X-Forwarded-Proto', 'https', '2001:db8::5', ['2001:db8::/129' => 'X-Forwarded-For'], false,
843+
],
844+
];
775845
}
776846

777847
public function testUserAgent(): void

0 commit comments

Comments
 (0)