Merge pull request from GHSA-c8rp-cgf4-937w

X-Forwarded header mitigation for 3.x series
mezzio · Jul 25, 2022 · fbaea3e · fbaea3e
2 parents 55461bd + 3b00374
commit fbaea3e
Show file tree

Hide file tree

Showing 10 changed files with 578 additions and 446 deletions.
diff --git a/composer.json b/composer.json
@@ -23,7 +23,10 @@
         "forum": "https://discourse.laminas.dev"
     },
     "config": {
-        "sort-packages": true
+        "sort-packages": true,
+        "allow-plugins": {
+            "dealerdirect/phpcodesniffer-composer-installer": true
+        }
     },
     "extra": {
     },
@@ -32,7 +35,7 @@
         "composer/package-versions-deprecated": "^1.11",
         "dflydev/fig-cookies": "^2.0.1 || ^3.0",
         "laminas/laminas-cli": "^0.1.5 || ^1.0",
-        "laminas/laminas-diactoros": "^1.8 || ^2.0",
+        "laminas/laminas-diactoros": "^2.11.2",
         "laminas/laminas-httphandlerrunner": "^1.0.1",
         "mezzio/mezzio": "^3.0.2",
         "psr/container": "^1.0",

diff --git a/composer.lock b/composer.lock
diff --git a/docs/book/v3/migration.md b/docs/book/v3/migration.md
@@ -71,3 +71,27 @@ $ ./vendor/bin/laminas mezzio:swoole:start
 ```
 
 Usage of other commands will change similarly.
+
+## ServerRequestSwooleFactory
+
+INFO: Since 3.7.0
+
+Starting in version 3.7.0, the behavior of `Mezzio\Swoole\ServerRequestSwooleFactory` changes slightly with regards to how it handles the various `X-Forwarded-*` headers.
+These headers are conventionally used when a server is behind a load balancer or reverse proxy in order to present to the application the URL that initiated a request.
+Starting in version 3.7.0, by default, these headers are only honored if the request received originates from a reserved subnet (e.g., localhost; class A, B, and C subnets; IPv6 private and local-link subnets).
+
+If you want to honor these headers from any source, or if you never want to allow them, you can provide an alternate implementation via the `Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface` service.
+As an example, you could use the following to dis-allow them:
+
+```php
+return [
+    'dependencies' => [
+        'invokables' => [
+            \Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface::class =>
+                \Laminas\Diactoros\ServerRequestFilter\DoNotFilter::class,
+        ],
+    ],
+];
+```
+
+If you are using Mezzio 3.11.0 or later, you can also use its `Mezzio\Container\FilterUsingXForwardedHeadersFactory` and related configuration to fine-tune which sources may be considered for usage of these headers.
diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<files psalm-version="v4.15.0@a1b5e489e6fcebe40cb804793d964e99fc347820">
+<files psalm-version="4.24.0@06dd975cb55d36af80f242561738f16c5f58264f">
   <file src="src/AbstractStaticResourceHandlerFactory.php">
     <MixedArgument occurrences="3">
       <code>$compressionLevel</code>
@@ -214,15 +214,6 @@
       <code>'-'</code>
       <code>'-'</code>
     </InvalidScalarArgument>
-    <MixedArgument occurrences="7">
-      <code>$matches[1]</code>
-      <code>$matches[1]</code>
-      <code>$matches[1]</code>
-      <code>$matches[1]</code>
-      <code>$matches[1]</code>
-      <code>$matches[1]</code>
-      <code>$matches[1]</code>
-    </MixedArgument>
   </file>
   <file src="src/Log/LoggerResolvingTrait.php">
     <MixedInferredReturnType occurrences="1">
@@ -262,6 +253,9 @@
     </MixedAssignment>
   </file>
   <file src="src/ServerRequestSwooleFactory.php">
+    <DeprecatedFunction occurrences="1">
+      <code>marshalUriFromSapi($server, $stripXForwardedHeaders($headers))</code>
+    </DeprecatedFunction>
     <MixedArgument occurrences="6">
       <code>$cookie</code>
       <code>$files</code>
@@ -270,12 +264,13 @@
       <code>$post</code>
       <code>$server</code>
     </MixedArgument>
-    <MixedAssignment occurrences="6">
+    <MixedAssignment occurrences="7">
       <code>$cookie</code>
       <code>$files</code>
       <code>$get</code>
       <code>$headers</code>
       <code>$post</code>
+      <code>$requestFilter</code>
       <code>$server</code>
     </MixedAssignment>
   </file>
@@ -562,9 +557,6 @@
     </MixedReturnStatement>
   </file>
   <file src="src/StaticResourceHandler/ValidateRegexTrait.php">
-    <MissingClosureParamType occurrences="1">
-      <code>$errno</code>
-    </MissingClosureParamType>
     <MixedArgument occurrences="1">
       <code>$regex</code>
     </MixedArgument>
@@ -682,6 +674,10 @@
       <code>$command-&gt;killProcess</code>
       <code>$command-&gt;waitThreshold</code>
     </InternalProperty>
+    <TypeDoesNotContainType occurrences="2">
+      <code>assertTrue</code>
+      <code>assertTrue</code>
+    </TypeDoesNotContainType>
   </file>
   <file src="test/Command/TestAsset/config/pipeline.php">
     <UnusedClosureParam occurrences="3">

diff --git a/psalm.xml.dist b/psalm.xml.dist
@@ -1,9 +1,9 @@
 <?xml version="1.0"?>
 <psalm
-    totallyTyped="true"
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xmlns="https://getpsalm.org/schema/config"
     xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
+    errorLevel="1"
     errorBaseline="psalm-baseline.xml"
 >
     <projectFiles>

diff --git a/src/ServerRequestSwooleFactory.php b/src/ServerRequestSwooleFactory.php
@@ -1,4 +1,4 @@
-<?php
+<?php // phpcs:disable WebimpressCodingStandard.NamingConventions.ValidVariableName.NotCamelCaps
 
 /**
  * @see       https://github.com/mezzio/mezzio-swoole for the canonical source repository
@@ -9,10 +9,14 @@
 namespace Mezzio\Swoole;
 
 use Laminas\Diactoros\ServerRequest;
+use Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface;
+use Laminas\Diactoros\ServerRequestFilter\FilterUsingXForwardedHeaders;
 use Psr\Container\ContainerInterface;
+use Psr\Http\Message\ServerRequestInterface;
 use Swoole\Http\Request as SwooleHttpRequest;
 
 use function array_change_key_case;
+use function array_key_exists;
 use function Laminas\Diactoros\marshalMethodFromSapi;
 use function Laminas\Diactoros\marshalProtocolVersionFromSapi;
 use function Laminas\Diactoros\marshalUriFromSapi;
@@ -27,7 +31,31 @@ class ServerRequestSwooleFactory
 {
     public function __invoke(ContainerInterface $container): callable
     {
-        return static function (SwooleHttpRequest $request) {
+        $requestFilter = $container->has(FilterServerRequestInterface::class)
+            ? $container->get(FilterServerRequestInterface::class)
+            : FilterUsingXForwardedHeaders::trustReservedSubnets();
+
+        $stripXForwardedHeaders = function (array $headers): array {
+            /** @psalm-var list<string> */
+            static $disallowedHeaders = [
+                'X-FORWARDED-FOR',
+                'X-FORWARDED-HOST',
+                'X-FORWARDED-PORT',
+                'X-FORWARDED-PROTO',
+            ];
+
+            $headers = array_change_key_case($headers, CASE_UPPER);
+            foreach ($disallowedHeaders as $name) {
+                if (array_key_exists($name, $headers)) {
+                    unset($headers[$name]);
+                }
+            }
+
+            return $headers;
+        };
+
+        // phpcs:disable Generic.Files.LineLength.TooLong
+        return static function (SwooleHttpRequest $request) use ($requestFilter, $stripXForwardedHeaders): ServerRequestInterface {
             // Aggregate values from Swoole request object
             $get     = $request->get ?? [];
             $post    = $request->post ?? [];
@@ -39,10 +67,10 @@ public function __invoke(ContainerInterface $container): callable
             // Normalize SAPI params
             $server = array_change_key_case($server, CASE_UPPER);
 
-            return new ServerRequest(
+            $request = new ServerRequest(
                 $server,
                 normalizeUploadedFiles($files),
-                marshalUriFromSapi($server, $headers),
+                marshalUriFromSapi($server, $stripXForwardedHeaders($headers)),
                 marshalMethodFromSapi($server),
                 new SwooleStream($request),
                 $headers,
@@ -51,6 +79,11 @@ public function __invoke(ContainerInterface $container): callable
                 $post,
                 marshalProtocolVersionFromSapi($server)
             );
+
+            return $requestFilter instanceof FilterServerRequestInterface
+                ? $requestFilter($request)
+                : $request;
         };
+        // phpcs:enable Generic.Files.LineLength.TooLong
     }
 }
diff --git a/test/ServerRequestSwooleFactoryTest.php b/test/ServerRequestSwooleFactoryTest.php
@@ -1,13 +1,11 @@
-<?php
+<?php // phpcs:disable Generic.Files.LineLength.TooLong
 
-/**
- * @see       https://github.com/mezzio/mezzio-swoole for the canonical source repository
- */
 
 declare(strict_types=1);
 
 namespace MezzioTest\Swoole;
 
+use Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface;
 use Mezzio\Swoole\ServerRequestSwooleFactory;
 use Mezzio\Swoole\SwooleStream;
 use PHPUnit\Framework\TestCase;
@@ -113,4 +111,104 @@ public function testInvoke(): void
         $this->assertInstanceOf(SwooleStream::class, $body);
         $this->assertEquals('this is the content', (string) $body);
     }
+
+    /** @psalm-return iterable<string, array{0: string, 1: string}> */
+    public function provideRemoteAddressAndExpectedUrl(): iterable
+    {
+        yield 'localhost'      => ['127.0.0.1', 'https://example.com/foo/bar'];
+        yield 'class-a-subnet' => ['10.0.0.1', 'https://example.com/foo/bar'];
+        yield 'class-b-subnet' => ['172.16.16.16', 'https://example.com/foo/bar'];
+        yield 'class-c-subnet' => ['192.168.1.1', 'https://example.com/foo/bar'];
+        yield 'public-ip'      => ['1.2.3.4', 'http://localhost:8080/foo/bar'];
+    }
+
+    /** @dataProvider provideRemoteAddressAndExpectedUrl */
+    public function testRequestGeneratedFromReturnedFactoryContainsUriBasedOnXForwardedHeadersBasedOnSubnetOfRequestingServer(
+        string $remoteAddr,
+        string $expectedUrl
+    ): void {
+        $swooleRequest         = $this->createMock(SwooleHttpRequest::class);
+        $swooleRequest->server = [
+            'path_info'       => '/',
+            'remote_addr'     => $remoteAddr,
+            'remote_port'     => 8080,
+            'REQUEST_METHOD'  => 'GET',
+            'REQUEST_TIME'    => time(),
+            'REQUEST_URI'     => '/foo/bar',
+            'server_port'     => 9501,
+            'server_protocol' => 'HTTP/1.1',
+        ];
+        $swooleRequest->get    = [];
+        $swooleRequest->post   = [];
+        $swooleRequest->cookie = [];
+        $swooleRequest->files  = [];
+        $swooleRequest->header = [
+            'host'              => 'localhost:8080',
+            'x-forwarded-host'  => 'example.com',
+            'x-forwarded-proto' => 'https',
+            'x-forwarded-port'  => '443',
+        ];
+
+        $swooleRequest->method('rawContent')->willReturn('');
+
+        $factory = new ServerRequestSwooleFactory();
+
+        $requestFactory = $factory($this->createMock(ContainerInterface::class));
+        $request        = $requestFactory($swooleRequest);
+
+        $this->assertInstanceOf(ServerRequestInterface::class, $request);
+        $this->assertSame($expectedUrl, $request->getUri()->__toString());
+    }
+
+    public function testFactoryUsesARequestFilterWhenPresentInTheContainer(): void
+    {
+        $swooleRequest         = $this->createMock(SwooleHttpRequest::class);
+        $swooleRequest->server = [
+            'path_info'       => '/',
+            'remote_addr'     => '1.2.3.4',
+            'remote_port'     => 8080,
+            'REQUEST_METHOD'  => 'GET',
+            'REQUEST_TIME'    => time(),
+            'REQUEST_URI'     => '/foo/bar',
+            'server_port'     => 9501,
+            'server_protocol' => 'HTTP/1.1',
+        ];
+        $swooleRequest->get    = [];
+        $swooleRequest->post   = [];
+        $swooleRequest->cookie = [];
+        $swooleRequest->files  = [];
+        $swooleRequest->header = [
+            'host'              => 'localhost:8080',
+            'x-forwarded-host'  => 'example.com',
+            'x-forwarded-proto' => 'https',
+            'x-forwarded-port'  => '443',
+        ];
+
+        $swooleRequest->method('rawContent')->willReturn('');
+
+        $serverRequest = $this->createMock(ServerRequestInterface::class);
+        $filter        = $this->createMock(FilterServerRequestInterface::class);
+        $filter
+            ->expects($this->once())
+            ->method('__invoke')
+            ->with($this->isInstanceOf(ServerRequestInterface::class))
+            ->willReturn($serverRequest);
+
+        $container = $this->createMock(ContainerInterface::class);
+        $container
+            ->expects($this->once())
+            ->method('has')
+            ->with(FilterServerRequestInterface::class)
+            ->willReturn(true);
+        $container
+            ->expects($this->once())
+            ->method('get')
+            ->with(FilterServerRequestInterface::class)
+            ->willReturn($filter);
+
+        $factory        = new ServerRequestSwooleFactory();
+        $requestFactory = $factory($container);
+
+        $this->assertSame($serverRequest, $requestFactory($swooleRequest));
+    }
 }
diff --git a/test/Task/DeferredServiceListenerDelegatorTest.php b/test/Task/DeferredServiceListenerDelegatorTest.php
@@ -8,11 +8,11 @@
 
 namespace MezzioTest\Swoole\Task;
 
-use Interop\Container\ContainerInterface;
 use Mezzio\Swoole\Task\DeferredServiceListener;
 use Mezzio\Swoole\Task\DeferredServiceListenerDelegator;
 use MezzioTest\Swoole\TestAsset\CallableObject;
 use PHPUnit\Framework\TestCase;
+use Psr\Container\ContainerInterface;
 use ReflectionProperty;
 use stdClass;
 use Swoole\Http\Server as SwooleHttpServer;

diff --git a/test/Task/TaskEventDispatchListenerFactoryTest.php b/test/Task/TaskEventDispatchListenerFactoryTest.php
@@ -8,11 +8,11 @@
 
 namespace MezzioTest\Swoole\Task;
 
-use Interop\Container\ContainerInterface;
 use Mezzio\Swoole\Event\EventDispatcherInterface as MezzioEventDispatcherInterface;
 use Mezzio\Swoole\Task\TaskEventDispatchListenerFactory;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use Psr\Container\ContainerInterface;
 use Psr\EventDispatcher\EventDispatcherInterface;
 use Psr\Log\LoggerInterface;
 use ReflectionProperty;

diff --git a/test/Task/TaskInvokerListenerTest.php b/test/Task/TaskInvokerListenerTest.php
@@ -8,12 +8,12 @@
 
 namespace MezzioTest\Swoole\Task;
 
-use Interop\Container\ContainerInterface;
 use Mezzio\Swoole\Event\TaskEvent;
 use Mezzio\Swoole\Task\TaskInterface;
 use Mezzio\Swoole\Task\TaskInvokerListener;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
 use Psr\Log\LogLevel;
 use RuntimeException;