From 94f70180afa5ed76dcd3cc866e1437323b812253 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Wed, 5 Apr 2023 15:34:15 +0200 Subject: [PATCH 01/11] qa: add failing test regarding `X-Forwarded-Host` containing a port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- .../FilterUsingXForwardedHeadersTest.php | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php b/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php index ff677867..9af4093d 100644 --- a/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php +++ b/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php @@ -368,4 +368,40 @@ 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 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:80', + 'X-Forwarded-Port' => '443', + ] + ); + + $filter = FilterUsingXForwardedHeaders::trustAny(); + + $filteredRequest = $filter($request); + $uri = $filteredRequest->getUri(); + self::assertSame('example.org', $uri->getHost()); + } } From 2f64b1f0979201879ebc4fc382fc6071d2b3251d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Wed, 5 Apr 2023 15:42:40 +0200 Subject: [PATCH 02/11] bugfix: split host and port from `X-Forwarded-Host` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- .../FilterUsingXForwardedHeaders.php | 4 +++- src/UriFactory.php | 10 +++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php b/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php index 2327646d..88a5d6be 100644 --- 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,8 @@ public function __invoke(ServerRequestInterface $request): ServerRequestInterfac switch ($headerName) { case self::HEADER_HOST: - $uri = $uri->withHost($header); + [$host] = UriFactory::marshalHostAndPortFromHeader($header); + $uri = $uri->withHost($host); break; case self::HEADER_PORT: $uri = $uri->withPort((int) $header); diff --git a/src/UriFactory.php b/src/UriFactory.php index d0ce13af..49e9f709 100644 --- 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,13 @@ private static function marshalHttpsValue(mixed $https): bool } /** - * @param string|list $host + * @internal + * * @return array{string, int|null} Array of two items, host and port, in that order (can be * passed to a list() operation). */ - 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 From aaba858a2659d0951f66d94968495e07c292e252 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Wed, 5 Apr 2023 15:48:40 +0200 Subject: [PATCH 03/11] qa: mark `UriFactory#marshalHostAndPortFromHeader` as `mutation-free` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- src/UriFactory.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/UriFactory.php b/src/UriFactory.php index 49e9f709..ec7ccb9d 100644 --- a/src/UriFactory.php +++ b/src/UriFactory.php @@ -231,6 +231,7 @@ private static function marshalHttpsValue(mixed $https): bool * * @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 */ public static function marshalHostAndPortFromHeader(string $host): array { From 0e5f4ba81907486130942aea66d2f27665f5d82d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Thu, 6 Apr 2023 02:29:03 +0200 Subject: [PATCH 04/11] qa: add failing test to verify that the port from `X-Forwarded-Host` is only ignored when `X-Forwarded-Port` is available MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- .../FilterUsingXForwardedHeadersTest.php | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php b/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php index 9af4093d..7019e251 100644 --- a/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php +++ b/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php @@ -382,7 +382,7 @@ public function testOnlyHonorsXForwardedProtoIfValueResolvesToHTTPS( * @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 testWillFilterXForwardedHostPort(): void + public function testWillFilterXForwardedHostPortWithPreservingForwardedPort(): void { $request = new ServerRequest( ['REMOTE_ADDR' => '192.168.0.1'], @@ -403,5 +403,29 @@ public function testWillFilterXForwardedHostPort(): void $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()); } } From 441cd050a6c5d8467dfe1994ad514eea796f1b56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Thu, 6 Apr 2023 02:30:17 +0200 Subject: [PATCH 05/11] bugfix: preserve `port` from `X-Forwarded-Host` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- src/ServerRequestFilter/FilterUsingXForwardedHeaders.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php b/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php index 88a5d6be..00f7277b 100644 --- a/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php +++ b/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php @@ -77,8 +77,12 @@ public function __invoke(ServerRequestInterface $request): ServerRequestInterfac switch ($headerName) { case self::HEADER_HOST: - [$host] = UriFactory::marshalHostAndPortFromHeader($header); - $uri = $uri->withHost($host); + [$host, $port] = UriFactory::marshalHostAndPortFromHeader($header); + $uri = $uri + ->withHost($host); + if ($port !== null) { + $uri = $uri->withPort((int) $port); + } break; case self::HEADER_PORT: $uri = $uri->withPort((int) $header); From 36e7e71729dc4b81d2439e4cddf05984e3c920e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Thu, 6 Apr 2023 03:44:47 +0200 Subject: [PATCH 06/11] qa: introduce psalm stub for `psr/http-message` `UriInterface` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This also requires `Uri` to be `psalm-immutable`. Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- .gitattributes | 1 + psalm.xml.dist | 4 ++++ psalm/http-message-stubs/UriInterface.phpstub | 18 ++++++++++++++++++ src/Uri.php | 5 +++-- 4 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 psalm/http-message-stubs/UriInterface.phpstub diff --git a/.gitattributes b/.gitattributes index fae892b7..ed26f59d 100644 --- 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.xml.dist b/psalm.xml.dist index 0a854e54..82f531f3 100644 --- a/psalm.xml.dist +++ b/psalm.xml.dist @@ -14,6 +14,10 @@ + + + + diff --git a/psalm/http-message-stubs/UriInterface.phpstub b/psalm/http-message-stubs/UriInterface.phpstub new file mode 100644 index 00000000..79540212 --- /dev/null +++ b/psalm/http-message-stubs/UriInterface.phpstub @@ -0,0 +1,18 @@ + Date: Thu, 6 Apr 2023 03:44:59 +0200 Subject: [PATCH 07/11] qa: remove redundant integer cast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- src/ServerRequestFilter/FilterUsingXForwardedHeaders.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php b/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php index 00f7277b..d246cf01 100644 --- a/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php +++ b/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php @@ -81,7 +81,7 @@ public function __invoke(ServerRequestInterface $request): ServerRequestInterfac $uri = $uri ->withHost($host); if ($port !== null) { - $uri = $uri->withPort((int) $port); + $uri = $uri->withPort($port); } break; case self::HEADER_PORT: From 9626babe6116a4bb9419d5d5aa884f258ee31706 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Thu, 6 Apr 2023 03:54:23 +0200 Subject: [PATCH 08/11] qa: update psalm configuration and cleanup baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- psalm-baseline.xml | 22 ++-------------------- psalm.xml.dist | 2 ++ test/StreamTest.php | 2 +- test/UriTest.php | 2 +- 4 files changed, 6 insertions(+), 22 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 1741a4f9..6dada0f6 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,5 +1,5 @@ - + null|callable @@ -114,10 +114,6 @@ $requestTarget - - $this->uri->getPort() - $uri->getPort() - @@ -170,11 +166,6 @@ $hasContentType - - - json_encode - - gettype($uri) @@ -234,21 +225,12 @@ - + getHeaderLine getServerParams getUri - withHost - withPort - withScheme withUri - - $proxyCIDRList - - - list<non-empty-string> - diff --git a/psalm.xml.dist b/psalm.xml.dist index 82f531f3..3cf45c00 100644 --- 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" > diff --git a/test/StreamTest.php b/test/StreamTest.php index 1b72fb31..d156cd6a 100644 --- 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); } diff --git a/test/UriTest.php b/test/UriTest.php index 848241af..5c65b192 100644 --- 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:pass@local.example.com: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()); From dafe22c715d326b330969291758d320b08a2aca0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Thu, 6 Apr 2023 03:54:46 +0200 Subject: [PATCH 09/11] qa: ensure that immutability is given and/or possible mutations are suppressed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- src/Uri.php | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/Uri.php b/src/Uri.php index afd53ea9..59f776ee 100644 --- a/src/Uri.php +++ b/src/Uri.php @@ -111,6 +111,7 @@ public function __toString(): string return $this->uriString; } + /** @psalm-suppress ImpureMethodCall, InaccessibleProperty */ $this->uriString = static::createUriString( $this->scheme, $this->getAuthority(), @@ -443,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 { @@ -553,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'], @@ -569,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'], @@ -657,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'], From f0482862393c59309637edc6dd165faf1d6f08c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Thu, 6 Apr 2023 03:55:54 +0200 Subject: [PATCH 10/11] qa: add missing line-break to assertion message as it exceeded 120 characters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- .../FilterUsingXForwardedHeadersTest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php b/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php index 7019e251..e4bbd4a7 100644 --- a/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php +++ b/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php @@ -403,7 +403,11 @@ public function testWillFilterXForwardedHostPortWithPreservingForwardedPort(): v $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'); + 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 From ee734118bf6a2392154a93d19787b29b8d605ab8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Sat, 8 Apr 2023 01:32:41 +0200 Subject: [PATCH 11/11] qa: remove unused psalm suppressions reported by psalm itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- test/MessageTraitTest.php | 2 +- test/StreamTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/MessageTraitTest.php b/test/MessageTraitTest.php index 92c63f96..4c636926 100644 --- 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/StreamTest.php b/test/StreamTest.php index d156cd6a..e7460c8f 100644 --- a/test/StreamTest.php +++ b/test/StreamTest.php @@ -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); }