qa: apply Psalm rules and update baseline

Corrected a number of issues flagged by Psalm, primarily around ensuring we have expected types. Most "new" errors involve: - Calling on PSR-7 mutator methods from a "pure" function (since the filter is marked immutable) - Inability to infer mixed type values (where we know they're mixed) Signed-off-by: Matthew Weier O'Phinney <[email protected]>
laminas · Jun 27, 2022 · 4b5d1ad · 4b5d1ad
1 parent 4d0cf3e
commit 4b5d1ad
Show file tree

Hide file tree

Showing 7 changed files with 82 additions and 21 deletions.
diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<files psalm-version="4.21.0@d8bec4c7aaee111a532daec32fb09de5687053d1">
+<files psalm-version="4.23.0@f1fe6ff483bf325c803df9f510d09a03fd796f88">
   <file src="src/CallbackStream.php">
     <ImplementedReturnTypeMismatch occurrences="1">
       <code>null|callable</code>
@@ -242,13 +242,45 @@
     </ParamNameMismatch>
   </file>
   <file src="src/ServerRequestFactory.php">
+    <LessSpecificReturnStatement occurrences="1"/>
     <MixedArgument occurrences="1">
       <code>$headers['cookie']</code>
     </MixedArgument>
+    <MixedAssignment occurrences="3">
+      <code>$iisUrlRewritten</code>
+      <code>$requestUri</code>
+      <code>$unencodedUrl</code>
+    </MixedAssignment>
+    <MixedInferredReturnType occurrences="1">
+      <code>array{string, int|null}</code>
+    </MixedInferredReturnType>
+    <MixedReturnStatement occurrences="1">
+      <code>$defaults</code>
+    </MixedReturnStatement>
+    <MoreSpecificReturnType occurrences="1">
+      <code>ServerRequest</code>
+    </MoreSpecificReturnType>
     <RedundantConditionGivenDocblockType occurrences="1">
       <code>is_callable(self::$apacheRequestHeaders)</code>
     </RedundantConditionGivenDocblockType>
   </file>
+  <file src="src/ServerRequestFilter/FilterUsingXForwardedHeaders.php">
+    <ImpureMethodCall occurrences="7">
+      <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">
     <InvalidArgument occurrences="1"/>
     <MixedInferredReturnType occurrences="1">

diff --git a/psalm.xml.dist b/psalm.xml.dist
@@ -15,11 +15,26 @@
     </projectFiles>
 
     <issueHandlers>
+        <DeprecatedFunction>
+            <errorLevel type="suppress">
+                <referencedFunction name="laminas\diactoros\marshalurifromsapi"/>
+            </errorLevel>
+        </DeprecatedFunction>
+
+        <InternalClass>
+            <errorLevel type="suppress">
+                <referencedClass name="Laminas\Diactoros\ServerRequestFilter\IPRange"/>
+            </errorLevel>
+        </InternalClass>
+
         <InternalMethod>
             <errorLevel type="suppress">
                 <referencedMethod name="PHPUnit\Framework\MockObject\Builder\InvocationMocker::method"/>
                 <referencedMethod name="PHPUnit\Framework\MockObject\Builder\InvocationMocker::willReturn"/>
                 <referencedMethod name="PHPUnit\Framework\MockObject\Builder\InvocationMocker::with"/>
+                <referencedMethod name="Laminas\Diactoros\ServerRequestFilter\IPRange::matches"/>
+                <referencedMethod name="Laminas\Diactoros\ServerRequestFilter\IPRange::matchesIPv4"/>
+                <referencedMethod name="Laminas\Diactoros\ServerRequestFilter\IPRange::matchesIPv6"/>
             </errorLevel>
         </InternalMethod>
 

diff --git a/src/Exception/InvalidForwardedHeaderNameException.php b/src/Exception/InvalidForwardedHeaderNameException.php
@@ -4,10 +4,11 @@
 
 namespace Laminas\Diactoros\Exception;
 
-use Laminas\Diactoros\ServerRequestFilter\XForwardedRequestFilter;
+use Laminas\Diactoros\ServerRequestFilter\FilterUsingXForwardedHeaders;
 
 class InvalidForwardedHeaderNameException extends RuntimeException implements ExceptionInterface
 {
+    /** @param mixed $name */
     public static function forHeader($name): self
     {
         if (! is_string($name)) {
@@ -17,7 +18,7 @@ public static function forHeader($name): self
         return new self(sprintf(
             'Invalid X-Forwarded-* header name "%s" provided to %s',
             $name,
-            XForwardedRequestFilter::class
+            FilterUsingXForwardedHeaders::class
         ));
     }
 }
diff --git a/src/ServerRequestFactory.php b/src/ServerRequestFactory.php
@@ -139,7 +139,7 @@ private static function marshalUriFromSapi(array $server) : Uri
         // URI query
         $query = '';
         if (isset($server['QUERY_STRING'])) {
-            $query = ltrim($server['QUERY_STRING'], '?');
+            $query = ltrim((string) $server['QUERY_STRING'], '?');
         }
 
         // URI fragment
@@ -157,7 +157,7 @@ private static function marshalUriFromSapi(array $server) : Uri
     /**
      * Marshal the host and port from the PHP environment.
      *
-     * @return array{string, numeric-string} Array of two items, host and port,
+     * @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 marshalHostAndPort(array $server) : array
@@ -168,7 +168,7 @@ private static function marshalHostAndPort(array $server) : array
             return $defaults;
         }
 
-        $host = $server['SERVER_NAME'];
+        $host = (string) $server['SERVER_NAME'];
         $port = isset($server['SERVER_PORT']) ? (int) $server['SERVER_PORT'] : null;
 
         if (! isset($server['SERVER_ADDR'])
@@ -183,14 +183,20 @@ private static function marshalHostAndPort(array $server) : array
     }
 
     /**
-     * @return array{string, numeric-string} Array of two items, host and port,
+     * @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 marshalIpv6HostAndPort(array $server, ?int $port) : array
     {
-        $host = '[' . $server['SERVER_ADDR'] . ']';
-        $port = $port ?: 80;
-        if ($port . ']' === substr($host, strrpos($host, ':') + 1)) {
+        $host             = '[' . (string) $server['SERVER_ADDR'] . ']';
+        $port             = $port ?: 80;
+        $portSeparatorPos = strrpos($host, ':');
+
+        if (false === $portSeparatorPos) {
+            return [$host, $port];
+        }
+
+        if ($port . ']' === substr($host, $portSeparatorPos + 1)) {
             // The last digit of the IPv6-Address has been taken as port
             // Unset the port so the default port can be used
             $port = null;
@@ -214,18 +220,18 @@ private static function marshalRequestPath(array $server) : string
         // (double slash problem).
         $iisUrlRewritten = $server['IIS_WasUrlRewritten'] ?? null;
         $unencodedUrl    = $server['UNENCODED_URL'] ?? '';
-        if ('1' === $iisUrlRewritten && ! empty($unencodedUrl)) {
+        if ('1' === $iisUrlRewritten && is_string($unencodedUrl) && '' !== $unencodedUrl) {
             return $unencodedUrl;
         }
 
         $requestUri = $server['REQUEST_URI'] ?? null;
 
-        if ($requestUri !== null) {
+        if (is_string($requestUri)) {
             return preg_replace('#^[^/:]+://[^/]+#', '', $requestUri);
         }
 
-        $origPathInfo = $server['ORIG_PATH_INFO'] ?? null;
-        if (empty($origPathInfo)) {
+        $origPathInfo = $server['ORIG_PATH_INFO'] ?? '';
+        if (! is_string($origPathInfo) || '' === $origPathInfo) {
             return '/';
         }
 

diff --git a/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php b/src/ServerRequestFilter/FilterUsingXForwardedHeaders.php
@@ -40,6 +40,9 @@ final class FilterUsingXForwardedHeaders implements FilterServerRequestInterface
 
     /**
      * Only allow construction via named constructors
+     *
+     * @param list<non-empty-string> $trustedProxies
+     * @param list<FilterUsingXForwardedHeaders::HEADER_*> $trustedHeaders
      */
     private function __construct(
         array $trustedProxies = [],
@@ -53,7 +56,7 @@ public function __invoke(ServerRequestInterface $request): ServerRequestInterfac
     {
         $remoteAddress = $request->getServerParams()['REMOTE_ADDR'] ?? '';
 
-        if ('' === $remoteAddress) {
+        if ('' === $remoteAddress || ! is_string($remoteAddress)) {
             // Should we trigger a warning here?
             return $request;
         }
@@ -77,13 +80,11 @@ public function __invoke(ServerRequestInterface $request): ServerRequestInterfac
                     $uri = $uri->withHost($header);
                     break;
                 case self::HEADER_PORT:
-                    $uri = $uri->withPort($header);
+                    $uri = $uri->withPort((int) $header);
                     break;
                 case self::HEADER_PROTO:
                     $uri = $uri->withScheme($header);
                     break;
-                default:
-                    break;
             }
         }
 
@@ -186,8 +187,8 @@ private static function validateTrustedHeaders(array $headers): void
     }
 
     /**
-     * @param non-empty-list<non-empty-string> $proxyCIDRList
-     * @return non-empty-list<non-empty-string>
+     * @param list<non-empty-string> $proxyCIDRList
+     * @return list<non-empty-string>
      * @throws InvalidProxyAddressException
      */
     private static function normalizeProxiesList(array $proxyCIDRList): array
@@ -219,7 +220,7 @@ private static function normalizeProxiesList(array $proxyCIDRList): array
      */
     private static function validateProxyCIDR($cidr): bool
     {
-        if (! is_string($cidr)) {
+        if (! is_string($cidr) || '' === $cidr) {
             return false;
         }
 

diff --git a/src/ServerRequestFilter/IPRange.php b/src/ServerRequestFilter/IPRange.php
@@ -14,6 +14,7 @@ private function __construct()
     {
     }
 
+    /** @psalm-pure */
     public static function matches(string $ip, string $cidr): bool
     {
         if (false !== strpos($ip, ':')) {
@@ -23,6 +24,7 @@ public static function matches(string $ip, string $cidr): bool
         return self::matchesIPv4($ip, $cidr);
     }
 
+    /** @psalm-pure */
     public static function matchesIPv4(string $ip, string $cidr): bool
     {
         $mask   = 32;
@@ -52,6 +54,7 @@ public static function matchesIPv4(string $ip, string $cidr): bool
         );
     }
 
+    /** @psalm-pure */
     public static function matchesIPv6(string $ip, string $cidr): bool
     {
         $mask   = 128;

diff --git a/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php b/test/ServerRequestFilter/FilterUsingXForwardedHeadersTest.php
@@ -193,6 +193,9 @@ public function testPassingInvalidAddressInProxyListRaisesException(): void
     public function testPassingInvalidForwardedHeaderNamesWhenTrustingProxyRaisesException(): void
     {
         $this->expectException(InvalidForwardedHeaderNameException::class);
+        /**
+         * @psalm-suppress InvalidArgument
+         */
         FilterUsingXForwardedHeaders::trustProxies(['192.168.1.0/24'], ['Host']);
     }