Merge pull request #125 from phenixphp/feature/improve-rate-limit-with-forwarded-ips

Improve rate limit with forwarded ips

Author: barbosa89

src/Http/Ip.php

@@ -61,42 +61,44 @@ public function forwardingAddress(): string|null
 
     public function hash(): string
     {
-        return hash('sha256', $this->host);
+        [$host] = $this->parseAddress($this->forwardingAddress ?? $this->host);
+
+        return hash('sha256', $host);
     }
 
     protected function parse(): void
     {
-        $address = trim($this->address);
+        [$this->host, $this->port] = $this->parseAddress($this->address);
+    }
 
-        if (preg_match('/^\[(?<addr>[^\]]+)\](?::(?<port>\d+))?$/', $address, $m) === 1) {
-            $this->host = $m['addr'];
-            $this->port = isset($m['port']) ? (int) $m['port'] : null;
+    /**
+     * @return array{0: string, 1: int|null}
+     */
+    protected function parseAddress(string $address): array
+    {
+        $address = trim($address);
 
-            return;
+        if (preg_match('/^\[(?<addr>[^\]]+)\](?::(?<port>\d+))?$/', $address, $m) === 1) {
+            return [$m['addr'], isset($m['port']) ? (int) $m['port'] : null];
         }
 
         if (filter_var($address, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) {
-            $this->host = $address;
-            $this->port = null;
-
-            return;
+            return [$address, null];
         }
 
+        $result = [$address, null];
+
         if (str_contains($address, ':')) {
             [$maybeHost, $maybePort] = explode(':', $address, 2);
 
             if (
                 filter_var($maybeHost, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) ||
                 filter_var($maybeHost, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME)
             ) {
-                $this->host = $maybeHost;
-                $this->port = is_numeric($maybePort) ? (int) $maybePort : null;
-
-                return;
+                $result = [$maybeHost, is_numeric($maybePort) ? (int) $maybePort : null];
             }
         }
 
-        $this->host = $address;
-        $this->port = null;
+        return $result;
     }
 }

src/Testing/Concerns/InteractWithResponses.php

@@ -24,6 +24,15 @@
 
 trait InteractWithResponses
 {
+    protected int $redirectsFollowed = 0;
+
+    protected function followingRedirects(): static
+    {
+        $this->redirectsFollowed = 10;
+
+        return $this;
+    }
+
     public function call(
         HttpMethod $method,
         string $path,
@@ -61,6 +70,7 @@ public function connect(
         };
 
         $client = (new HttpClientBuilder())
+            ->followRedirects($this->redirectsFollowed)
             ->usingPool(new UnlimitedConnectionPool(new DefaultConnectionFactory($connector)))
             ->build();
 

tests/Feature/AuthenticationTest.php

@@ -12,6 +12,7 @@
 use Phenix\Auth\Middlewares\Guest;
 use Phenix\Auth\PersonalAccessToken;
 use Phenix\Auth\User;
+use Phenix\Constants\AppMode;
 use Phenix\Database\Constants\Connection;
 use Phenix\Facades\Config;
 use Phenix\Facades\Crypto;
@@ -389,6 +390,42 @@
     ])->assertStatusCode(HttpStatus::TOO_MANY_REQUESTS)->assertHeaders(['Retry-After' => '300']);
 });
 
+it('tracks failed token validation attempts by forwarded client ip behind a trusted proxy', function (): void {
+    Config::set('app.app_mode', AppMode::PROXIED->value);
+    Config::set('app.trusted_proxies', ['127.0.0.1/32', '127.0.0.1']);
+
+    $connection = $this->getMockBuilder(MysqlConnectionPool::class)->getMock();
+    $connection->expects($this->any())
+        ->method('prepare')
+        ->willReturn(
+            new Statement(new Result()),
+        );
+
+    $this->app->swap(Connection::default(), $connection);
+
+    Route::get('/limited-proxy', fn (): Response => response()->plain('Never reached'))
+        ->middleware(Authenticated::class);
+
+    $this->app->run();
+
+    for ($i = 0; $i < 5; $i++) {
+        $this->get('/limited-proxy', headers: [
+            'Authorization' => 'Bearer invalid-token',
+            'X-Forwarded-For' => '203.0.113.10',
+        ])->assertUnauthorized();
+    }
+
+    $this->get('/limited-proxy', headers: [
+        'Authorization' => 'Bearer invalid-token',
+        'X-Forwarded-For' => '203.0.113.10',
+    ])->assertStatusCode(HttpStatus::TOO_MANY_REQUESTS)->assertHeaders(['Retry-After' => '300']);
+
+    $this->get('/limited-proxy', headers: [
+        'Authorization' => 'Bearer invalid-token',
+        'X-Forwarded-For' => '203.0.113.11',
+    ])->assertUnauthorized();
+});
+
 it('resets rate limit counter on successful authentication', function (): void {
     $user = new User();
     $user->id = 1;

tests/Feature/Cache/LocalRateLimitTest.php

@@ -2,6 +2,7 @@
 
 declare(strict_types=1);
 
+use Phenix\Constants\AppMode;
 use Phenix\Facades\Config;
 use Phenix\Facades\Crypto;
 use Phenix\Facades\Route;
@@ -65,3 +66,25 @@
     $this->get(path: '/')
         ->assertOk();
 });
+
+it('uses forwarded client ip for rate limiting behind a trusted proxy', function (): void {
+    Config::set('app.app_mode', AppMode::PROXIED->value);
+    Config::set('app.trusted_proxies', ['127.0.0.1/32', '127.0.0.1']);
+    Config::set('cache.rate_limit.per_minute', 1);
+
+    Route::get('/', fn (): Response => response()->plain('Ok'));
+
+    $this->app->run();
+
+    $this->get(path: '/', headers: [
+        'X-Forwarded-For' => '203.0.113.10',
+    ])->assertOk();
+
+    $this->get(path: '/', headers: [
+        'X-Forwarded-For' => '203.0.113.10',
+    ])->assertStatusCode(HttpStatus::TOO_MANY_REQUESTS);
+
+    $this->get(path: '/', headers: [
+        'X-Forwarded-For' => '203.0.113.11',
+    ])->assertOk();
+});

tests/Feature/RequestTest.php

@@ -509,3 +509,21 @@
             'Cross-Origin-Opener-Policy',
         ]);
 });
+
+it('can follow redirects when enabled', function (): void {
+    Route::get('/redirect-follow', fn (): Response => response()->redirect('/redirect-target'));
+    Route::get('/redirect-target', fn (): Response => response()->json([
+        'message' => 'Redirect followed',
+        'redirected' => true,
+    ]));
+
+    $this->app->run();
+
+    $this->followingRedirects()
+        ->get('/redirect-follow')
+        ->assertOk()
+        ->assertIsJson()
+        ->assertJsonPath('message', 'Redirect followed')
+        ->assertJsonPath('redirected', true)
+        ->assertHeaderIsMissing('Location');
+});

tests/Unit/Http/IpAddressTest.php

@@ -269,4 +269,50 @@ public function __toString(): string
 
     expect($ip->isForwarded())->toBeTrue();
     expect($ip->forwardingAddress())->toBe('203.0.113.1:4711');
+    expect($ip->hash())->toBe(hash('sha256', '203.0.113.1'));
+});
+
+it('uses forwarded ipv6 host for hash when request is behind a trusted proxy', function (): void {
+    $client = $this->createMock(Client::class);
+    $client->method('getRemoteAddress')->willReturn(
+        new class ('10.0.0.1:1234') implements SocketAddress {
+            public function __construct(private string $address)
+            {
+            }
+
+            public function toString(): string
+            {
+                return $this->address;
+            }
+
+            public function getType(): SocketAddressType
+            {
+                return SocketAddressType::Internet;
+            }
+
+            public function __toString(): string
+            {
+                return $this->address;
+            }
+        }
+    );
+
+    $request = new ServerRequest($client, HttpMethod::GET->value, Http::new(Url::to('/')));
+    $request->setHeader('X-Forwarded-For', '2001:db8::10');
+    $request->setAttribute(
+        Forwarded::class,
+        new Forwarded(
+            new InternetAddress('2001:db8::10', 4711),
+            [
+                'for' => '[2001:db8::10]:4711',
+            ]
+        )
+    );
+
+    $ip = Ip::make($request);
+
+    expect($ip->isForwarded())->toBeTrue();
+    expect($ip->forwardingAddress())->toBe('[2001:db8::10]:4711');
+    expect($ip->host())->toBe('10.0.0.1');
+    expect($ip->hash())->toBe(hash('sha256', '2001:db8::10'));
 });