Merge pull request #135 from boesing/bugfix/x-forwarded-host-contains…

…-port Strip port from `X-Forwarded-Host`
laminas · Apr 7, 2023 · a7a7f72 · a7a7f72
2 parents 6028af6 + ee73411
commit a7a7f72
Show file tree

Hide file tree

Showing 11 changed files with 127 additions and 36 deletions.
diff --git a/.gitattributes b/.gitattributes
@@ -12,3 +12,4 @@
 /psalm.xml.dist export-ignore
 /renovate.json export-ignore
 /test/ export-ignore
+/psalm/ export-ignore
diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<files psalm-version="5.3.0@b6faa3e96b8eb50ec71384c53799b8a107236bb6">
+<files psalm-version="5.4.0@62db5d4f6a7ae0a20f7cc5a4952d730272fc0863">
   <file src="src/CallbackStream.php">
     <ImplementedReturnTypeMismatch occurrences="1">
       <code>null|callable</code>
@@ -114,10 +114,6 @@
     <MoreSpecificImplementedParamType occurrences="1">
       <code>$requestTarget</code>
     </MoreSpecificImplementedParamType>
-    <PossiblyNullOperand occurrences="2">
-      <code>$this-&gt;uri-&gt;getPort()</code>
-      <code>$uri-&gt;getPort()</code>
-    </PossiblyNullOperand>
   </file>
   <file src="src/Response.php">
     <DocblockTypeContradiction occurrences="5">
@@ -170,11 +166,6 @@
       <code>$hasContentType</code>
     </MixedAssignment>
   </file>
-  <file src="src/Response/JsonResponse.php">
-    <UnusedFunctionCall occurrences="1">
-      <code>json_encode</code>
-    </UnusedFunctionCall>
-  </file>
   <file src="src/Response/RedirectResponse.php">
     <DocblockTypeContradiction occurrences="1">
       <code>gettype($uri)</code>
@@ -234,21 +225,12 @@
     </RedundantConditionGivenDocblockType>
   </file>
   <file src="src/ServerRequestFilter/FilterUsingXForwardedHeaders.php">
-    <ImpureMethodCall occurrences="7">
+    <ImpureMethodCall occurrences="4">
       <code>getHeaderLine</code>
       <code>getServerParams</code>
       <code>getUri</code>
-      <code>withHost</code>
-      <code>withPort</code>
-      <code>withScheme</code>
       <code>withUri</code>
     </ImpureMethodCall>
-    <LessSpecificReturnStatement occurrences="1">
-      <code>$proxyCIDRList</code>
-    </LessSpecificReturnStatement>
-    <MoreSpecificReturnType occurrences="1">
-      <code>list&lt;non-empty-string&gt;</code>
-    </MoreSpecificReturnType>
   </file>
   <file src="src/Stream.php">
     <PossiblyNullArgument occurrences="4">

diff --git a/psalm.xml.dist b/psalm.xml.dist
@@ -5,6 +5,8 @@
     xmlns="https://getpsalm.org/schema/config"
     xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
     errorBaseline="psalm-baseline.xml"
+    findUnusedCode="false"
+    findUnusedPsalmSuppress="true"
 >
     <projectFiles>
         <directory name="src"/>
@@ -14,6 +16,10 @@
         </ignoreFiles>
     </projectFiles>
 
+    <stubs>
+        <file name="psalm/http-message-stubs/UriInterface.phpstub"/>
+    </stubs>
+
     <issueHandlers>
         <DeprecatedFunction>
             <errorLevel type="suppress">

diff --git a/psalm/http-message-stubs/UriInterface.phpstub b/psalm/http-message-stubs/UriInterface.phpstub
@@ -0,0 +1,18 @@
+<?php
+declare(strict_types=1);
+
+namespace Psr\Http\Message;
+
+/**
+ * [...]
+ * Instances of this interface are considered immutable; all methods that
+ * might change state MUST be implemented such that they retain the internal
+ * state of the current instance and return an instance that contains the
+ * changed state.
+ * [...]
+ * @psalm-immutable
+ */
+interface UriInterface
+{
+
+}
diff --git a/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php b/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php
@@ -6,6 +6,7 @@
 
 use Laminas\Diactoros\Exception\InvalidForwardedHeaderNameException;
 use Laminas\Diactoros\Exception\InvalidProxyAddressException;
+use Laminas\Diactoros\UriFactory;
 use Psr\Http\Message\ServerRequestInterface;
 
 use function explode;
@@ -76,7 +77,12 @@ public function __invoke(ServerRequestInterface $request): ServerRequestInterfac
 
             switch ($headerName) {
                 case self::HEADER_HOST:
-                    $uri = $uri->withHost($header);
+                    [$host, $port] = UriFactory::marshalHostAndPortFromHeader($header);
+                    $uri           = $uri
+                        ->withHost($host);
+                    if ($port !== null) {
+                        $uri = $uri->withPort($port);
+                    }
                     break;
                 case self::HEADER_PORT:
                     $uri = $uri->withPort((int) $header);

diff --git a/src/Uri.php b/src/Uri.php
@@ -38,6 +38,8 @@
  * might change state are implemented such that they retain the internal
  * state of the current instance and return a new instance that contains the
  * changed state.
+ *
+ * @psalm-immutable
  */
 class Uri implements UriInterface, Stringable
 {
@@ -67,8 +69,7 @@ class Uri implements UriInterface, Stringable
 
     private string $host = '';
 
-    /** @var int|null */
-    private $port;
+    private ?int $port = null;
 
     private string $path = '';
 
@@ -110,6 +111,7 @@ public function __toString(): string
             return $this->uriString;
         }
 
+        /** @psalm-suppress ImpureMethodCall, InaccessibleProperty */
         $this->uriString = static::createUriString(
             $this->scheme,
             $this->getAuthority(),
@@ -442,6 +444,9 @@ public function withFragment($fragment): UriInterface
 
     /**
      * Parse a URI into its parts, and set the properties
+     *
+     * @psalm-suppress InaccessibleProperty Method is only called in {@see Uri::__construct} and thus immutability is
+     *                                      still given.
      */
     private function parseUri(string $uri): void
     {
@@ -552,8 +557,12 @@ private function filterUserInfoPart(string $part): string
     {
         $part = $this->filterInvalidUtf8($part);
 
-        // Note the addition of `%` to initial charset; this allows `|` portion
-        // to match and thus prevent double-encoding.
+        /**
+         * @psalm-suppress ImpureFunctionCall Even tho the callback targets this immutable class,
+         *                                    psalm reports an issue here.
+         * Note the addition of `%` to initial charset; this allows `|` portion
+         * to match and thus prevent double-encoding.
+         */
         return preg_replace_callback(
             '/(?:[^%' . self::CHAR_UNRESERVED . self::CHAR_SUB_DELIMS . ']+|%(?![A-Fa-f0-9]{2}))/u',
             [$this, 'urlEncodeChar'],
@@ -568,6 +577,10 @@ private function filterPath(string $path): string
     {
         $path = $this->filterInvalidUtf8($path);
 
+        /**
+         * @psalm-suppress ImpureFunctionCall Even tho the callback targets this immutable class,
+         *                                    psalm reports an issue here.
+         */
         return preg_replace_callback(
             '/(?:[^' . self::CHAR_UNRESERVED . ')(:@&=\+\$,\/;%]+|%(?![A-Fa-f0-9]{2}))/u',
             [$this, 'urlEncodeChar'],
@@ -656,6 +669,10 @@ private function filterQueryOrFragment(string $value): string
     {
         $value = $this->filterInvalidUtf8($value);
 
+        /**
+         * @psalm-suppress ImpureFunctionCall Even tho the callback targets this immutable class,
+         *                                    psalm reports an issue here.
+         */
         return preg_replace_callback(
             '/(?:[^' . self::CHAR_UNRESERVED . self::CHAR_SUB_DELIMS . '%:@\/\?]+|%(?![A-Fa-f0-9]{2}))/u',
             [$this, 'urlEncodeChar'],

diff --git a/src/UriFactory.php b/src/UriFactory.php
@@ -12,7 +12,6 @@
 use function explode;
 use function gettype;
 use function implode;
-use function is_array;
 use function is_bool;
 use function is_scalar;
 use function is_string;
@@ -228,16 +227,14 @@ private static function marshalHttpsValue(mixed $https): bool
     }
 
     /**
-     * @param string|list<string> $host
+     * @internal
+     *
      * @return array{string, int|null} Array of two items, host and port, in that order (can be
      *     passed to a list() operation).
+     * @psalm-mutation-free
      */
-    private static function marshalHostAndPortFromHeader($host): array
+    public static function marshalHostAndPortFromHeader(string $host): array
     {
-        if (is_array($host)) {
-            $host = implode(', ', $host);
-        }
-
         $port = null;
 
         // works for regname, IPv4 & IPv6

diff --git a/test/MessageTraitTest.php b/test/MessageTraitTest.php
@@ -382,7 +382,7 @@ public function numericHeaderValuesProvider(): array
     /**
      * @dataProvider numericHeaderValuesProvider
      * @group 99
-     * @psalm-suppress InvalidArgument,InvalidScalarArgument this test
+     * @psalm-suppress InvalidArgument this test
      *     explicitly verifies that pre-type-declaration implicit type
      *     conversion semantics still apply, for BC Compliance
      */

diff --git a/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php b/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php
@@ -368,4 +368,68 @@ public function testOnlyHonorsXForwardedProtoIfValueResolvesToHTTPS(
         $uri             = $filteredRequest->getUri();
         $this->assertSame($expectedScheme, $uri->getScheme());
     }
+
+    /**
+     * Caddy server and NGINX seem to strip ports from `X-Forwarded-Host` as it should only contain the `host` which
+     * was initially requested. Due to Mozilla, the `Host` header is allowed to contain a port. Apache2 does pass the
+     * `Host` header via `X-Forwarded-Host` and thus, it should be stripped. We should only trust the `X-Forwarded-Port`
+     * header which is provided by the proxy since the `Host` header could contain port `80` while the initial request
+     * was still sent to port `443`.
+     *
+     * @link https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host
+     * @link https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host
+     * @link https://www.nginx.com/resources/wiki/start/topics/examples/forwarded/
+     * @link https://caddyserver.com/docs/caddyfile/directives/reverse_proxy#defaults
+     * @link https://httpd.apache.org/docs/2.4/en/mod/mod_proxy.html#x-headers
+     */
+    public function testWillFilterXForwardedHostPortWithPreservingForwardedPort(): void
+    {
+        $request = new ServerRequest(
+            ['REMOTE_ADDR' => '192.168.0.1'],
+            [],
+            'http://localhost:80/foo/bar',
+            'GET',
+            'php://temp',
+            [
+                'Host'              => 'localhost',
+                'X-Forwarded-Proto' => 'https',
+                'X-Forwarded-Host'  => 'example.org:80',
+                'X-Forwarded-Port'  => '443',
+            ]
+        );
+
+        $filter = FilterUsingXForwardedHeaders::trustAny();
+
+        $filteredRequest = $filter($request);
+        $uri             = $filteredRequest->getUri();
+        self::assertSame('example.org', $uri->getHost());
+        self::assertNull(
+            $uri->getPort(),
+            'Port is omitted due to the fact that `https` protocol was used and port 80 is being ignored due'
+            . ' to the availability of `X-Forwarded-Port'
+        );
+    }
+
+    public function testWillFilterXForwardedHostPort(): void
+    {
+        $request = new ServerRequest(
+            ['REMOTE_ADDR' => '192.168.0.1'],
+            [],
+            'http://localhost:80/foo/bar',
+            'GET',
+            'php://temp',
+            [
+                'Host'              => 'localhost',
+                'X-Forwarded-Proto' => 'https',
+                'X-Forwarded-Host'  => 'example.org:8080',
+            ]
+        );
+
+        $filter = FilterUsingXForwardedHeaders::trustAny();
+
+        $filteredRequest = $filter($request);
+        $uri             = $filteredRequest->getUri();
+        self::assertSame('example.org', $uri->getHost());
+        self::assertSame(8080, $uri->getPort());
+    }
 }
diff --git a/test/StreamTest.php b/test/StreamTest.php
@@ -613,7 +613,7 @@ public function testRaisesExceptionOnConstructionForNonStreamResources(): void
         $this->expectException(InvalidArgumentException::class);
         $this->expectExceptionMessage('stream');
 
-        /** @psalm-suppress ImplicitToStringCast, PossiblyInvalidArgument */
+        /** @psalm-suppress PossiblyInvalidArgument */
         new Stream($resource);
     }
 
@@ -632,7 +632,7 @@ public function testRaisesExceptionOnAttachForNonStreamResources(): void
         $this->expectException(InvalidArgumentException::class);
         $this->expectExceptionMessage('stream');
 
-        /** @psalm-suppress ImplicitToStringCast, PossiblyInvalidArgument */
+        /** @psalm-suppress  PossiblyInvalidArgument */
         $stream->attach($resource);
     }
 

diff --git a/test/UriTest.php b/test/UriTest.php
@@ -166,7 +166,7 @@ public function testWithPortReturnsNewInstanceWithProvidedPort($port): void
     public function testWithPortReturnsSameInstanceWithProvidedPortIsSameAsBefore(): void
     {
         $uri = new Uri('https://user:[email protected]:3001/foo?bar=baz#quz');
-        /** @psalm-suppress PossiblyInvalidArgument,InvalidArgument */
+        /** @psalm-suppress InvalidArgument */
         $new = $uri->withPort('3001');
         $this->assertSame($uri, $new);
         $this->assertSame(3001, $new->getPort());