Merge pull request #157 from weierophinney/feature/filter-integer-hea…

…der-names Filter integer header names during SAPI discovery
laminas · May 4, 2023 · 2515f41 · 2515f41
2 parents 9bca291 + 5535fe5
commit 2515f41
Show file tree

Hide file tree

Showing 8 changed files with 119 additions and 66 deletions.
diff --git a/docs/book/v3/forward-migration.md b/docs/book/v3/forward-migration.md
diff --git a/docs/book/v3/migration.md b/docs/book/v3/migration.md
@@ -24,6 +24,11 @@ For consumers, usage should be completely backwards compatible, however.
 The factory `Laminas\Diactoros\ServerRequestFactory::fromGlobals()` was modified such that passing empty array values for arguments that accept `null` or an array now will not use the associated superglobal in that scenario.
 Previously, an empty array value was treated as identical to `null`, and would cause the factory to fallback to superglobals; now, this is a way to provide an empty set for the associated value(s).
 
+### marshalHeadersFromSapi
+
+The function `Laminas\Diactoros\marshalHeadersFromSapi()`, which is consumed by `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, was modified such that it will now filter out header field names that evaluate to integers.
+Please see the [Security Features](security-features.md) document for more details.
+
 ## Removed
 
 The following features were removed for version 3.

diff --git a/docs/book/v3/security.md b/docs/book/v3/security.md
@@ -0,0 +1,43 @@
+# Security Features
+
+## ServerRequestFilterInterface defaults
+
+`Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface` is used by `ServerRequestFactory::fromGlobals()` to allow modifying the generated `ServerRequest` instance prior to returning it.
+The primary use case is to allow modifying the generated URI based on the presence of headers such as `X-Forwarded-Host`.
+When operating behind a reverse proxy, the `Host` header is often rewritten to the name of the node to which the request is being forwarded, and an `X-Forwarded-Host` header is generated with the original `Host` value to allow the server to determine the original host the request was intended for.
+We also similarly examine the `X-Forwarded-Port` header.
+
+To accommodate this use case, we provide `Laminas\Diactoros\ServerRequestFilter\FilterUsingXForwardedHeaders`.
+
+Due to potential security issues, it is generally best to only accept these headers if you trust the reverse proxy that has initiated the request.
+(This value is found in `$_SERVER['REMOTE_ADDR']`, which is present as `$request->getServerParams()['REMOTE_ADDR']` within PSR-7 implementations.)
+`FilterUsingXForwardedHeaders` provides named constructors to allow you to trust these headers from any source (which has been the default behavior of Diactoros since the beginning), or to specify specific IP addresses or CIDR subnets to trust, along with which headers are trusted.
+We use this filter by default, marked to trust **only proxies on private subnets**.
+
+If you **do not** need the functionality, we recommend specifying `Laminas\Diactoros\ServerRequestFilter\DoNotFilter` as the configured `FilterServerRequestInterface` in your application.
+
+## Filtering of integer header names
+
+[PSR-7](https://www.php-fig.org/psr/psr-7/) targets [RFC 7230](https://www.rfc-editor.org/rfc/rfc7230).
+RFC-7230 defines an ABNF pattern for header field names that allows the possibility of using an integer as a header field; e.g.,
+
+```http
+1234: header value
+```
+
+The PSR-7, `Psr\Http\MessageInterface::getHeaders()` method requires implementations to return an associative array, where the key is the header field name.
+This triggers an interesting quirk in PHP: when adding a value to an array using a string that consists of an integer value, PHP will convert this value to an integer (see [PHP bug 80309](https://bugs.php.net/bug.php?id=80309) for more details).
+This presents several issues:
+
+- First, it means that consumers cannot depend on the header field name returned being a string.
+- Second, our own validation of header field name will fail, as it will not see a string.
+
+Normally, this will not present an issue, as the way to add headers to a message is via the `MessageInterface::withHeader()` and `MessageInterface::withAddedHeader()` methods, which both require a `string` name argument.
+However, when using `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, it can present a problem if any discovered headers have field names that evaluate to integers.
+
+To prevent issues, as of version 3.0.0, the `ServerRequestFactory` implementation in Diactoros filters out any headers that evaluate to integers.
+If you wish to accept these anyways, we strongly recommend that you modify your web server to rewrite the incoming header field name to add a prefix or suffix string (e.g., `X-Digit-1`, `1-Digit`).
+
+> NOTE: **Integer keys can still be returned from getHeaders()**
+> While `withHeader()` and `withHeaderLine()` require string name values, please be aware that these can be presented as string integers.
+> These names will be considered valid, and that means that when you call `getHeaders()`, any such names will become integers at this time.
diff --git a/mkdocs.yml b/mkdocs.yml
@@ -17,6 +17,7 @@ nav:
  - Factories: v3/factories.md
  - "Server Request Filters": v3/server-request-filters.md
  - "Custom Responses": v3/custom-responses.md
+ - "Security Features": v3/security.md
  - Serialization: v3/serialization.md
  - API: v3/api.md
  - Migration:

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.9.0@8b9ad1eb9e8b7d3101f949291da2b9f7767cd163">
+<files psalm-version="5.10.0@a5effd2d2dddd1a7ea7a0f6a051ce63ff979e356">
  <file src="src/CallbackStream.php">
  <ImplementedReturnTypeMismatch>
  <code>null|callable</code>
@@ -80,7 +80,7 @@
  <code>$protocolVersion</code>
  <code>$requestTarget</code>
  <code>$uri</code>
- <code><![CDATA[self::getValueFromKey($serializedRequest, 'body')]]></code>
+ <code>self::getValueFromKey($serializedRequest, 'body')</code>
  </MixedArgument>
  <MixedAssignment>
  <code>$headers</code>
@@ -114,7 +114,7 @@
  <code>$protocolVersion</code>
  <code>$reasonPhrase</code>
  <code>$statusCode</code>
- <code><![CDATA[self::getValueFromKey($serializedResponse, 'body')]]></code>
+ <code>self::getValueFromKey($serializedResponse, 'body')</code>
  </MixedArgument>
  <MixedAssignment>
  <code>$headers</code>
@@ -186,7 +186,7 @@
  </file>
  <file src="src/ServerRequestFactory.php">
  <MixedArgument>
- <code><![CDATA[$headers['cookie']]]></code>
+ <code>$headers['cookie']</code>
  </MixedArgument>
  <MixedArgumentTypeCoercion>
  <code>$headers</code>
@@ -250,42 +250,48 @@
  </file>
  <file src="src/functions/create_uploaded_file.php">
  <MixedArgument>
- <code><![CDATA[$spec['error']]]></code>
- <code><![CDATA[$spec['name'] ?? null]]></code>
- <code><![CDATA[$spec['tmp_name']]]></code>
- <code><![CDATA[$spec['type'] ?? null]]></code>
+ <code>$spec['error']</code>
+ <code>$spec['name'] ?? null</code>
+ <code>$spec['tmp_name']</code>
+ <code>$spec['type'] ?? null</code>
  </MixedArgument>
  </file>
  <file src="src/functions/marshal_headers_from_sapi.php">
+ <LessSpecificReturnStatement>
+ <code><![CDATA[array_filter($headers, fn(string|int $key): bool => is_string($key), ARRAY_FILTER_USE_KEY)]]></code>
+ </LessSpecificReturnStatement>
  <MixedAssignment>
  <code>$headers[$name]</code>
  <code>$headers[$name]</code>
  <code>$value</code>
  </MixedAssignment>
+ <MoreSpecificReturnType>
+ <code><![CDATA[array<non-empty-string, mixed>]]></code>
+ </MoreSpecificReturnType>
  </file>
  <file src="src/functions/marshal_method_from_sapi.php">
  <MixedInferredReturnType>
  <code>string</code>
  </MixedInferredReturnType>
  <MixedReturnStatement>
- <code><![CDATA[$server['REQUEST_METHOD'] ?? 'GET']]></code>
- <code><![CDATA[$server['REQUEST_METHOD'] ?? 'GET']]></code>
+ <code>$server['REQUEST_METHOD'] ?? 'GET'</code>
+ <code>$server['REQUEST_METHOD'] ?? 'GET'</code>
  </MixedReturnStatement>
  </file>
  <file src="src/functions/marshal_protocol_version_from_sapi.php">
  <MixedArgument>
- <code><![CDATA[$server['SERVER_PROTOCOL']]]></code>
+ <code>$server['SERVER_PROTOCOL']</code>
  </MixedArgument>
  </file>
  <file src="src/functions/normalize_server.php">
  <MixedArrayAccess>
- <code><![CDATA[$apacheRequestHeaders['Authorization']]]></code>
- <code><![CDATA[$apacheRequestHeaders['authorization']]]></code>
+ <code>$apacheRequestHeaders['Authorization']</code>
+ <code>$apacheRequestHeaders['authorization']</code>
  </MixedArrayAccess>
  <MixedAssignment>
  <code>$apacheRequestHeaders</code>
- <code><![CDATA[$server['HTTP_AUTHORIZATION']]]></code>
- <code><![CDATA[$server['HTTP_AUTHORIZATION']]]></code>
+ <code>$server['HTTP_AUTHORIZATION']</code>
+ <code>$server['HTTP_AUTHORIZATION']</code>
  </MixedAssignment>
  </file>
  <file src="src/functions/normalize_uploaded_files.php">
@@ -308,25 +314,25 @@
  $nameTree[$key] ?? null,
  $typeTree[$key] ?? null
  )</code>
- <code><![CDATA[$recursiveNormalize(
+ <code>$recursiveNormalize(
  $files['tmp_name'],
  $files['size'],
  $files['error'],
  $files['name'] ?? null,
  $files['type'] ?? null
- )]]></code>
+ )</code>
  </MixedFunctionCall>
  <MixedInferredReturnType>
  <code>array</code>
  </MixedInferredReturnType>
  <MixedReturnStatement>
- <code><![CDATA[$recursiveNormalize(
+ <code>$recursiveNormalize(
  $files['tmp_name'],
  $files['size'],
  $files['error'],
  $files['name'] ?? null,
  $files['type'] ?? null
- )]]></code>
+ )</code>
  </MixedReturnStatement>
  </file>
  <file src="src/functions/parse_cookie_header.php">
@@ -386,7 +392,7 @@
  </file>
  <file src="test/ServerRequestFactoryTest.php">
  <InvalidArgument>
- <code><![CDATA[$normalizedFiles['fooFiles']]]></code>
+ <code>$normalizedFiles['fooFiles']</code>
  </InvalidArgument>
  </file>
  <file src="test/ServerRequestTest.php">
@@ -434,23 +440,23 @@
  </file>
  <file src="test/functions/NormalizeUploadedFilesTest.php">
  <MixedArgument>
- <code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
- <code><![CDATA[$normalised['slide-shows'][0]['slides']]]></code>
+ <code>$normalised['my-form']['details']['avatars']</code>
+ <code>$normalised['slide-shows'][0]['slides']</code>
  </MixedArgument>
  <MixedArrayAccess>
- <code><![CDATA[$normalised['my-form']['details']['avatar']]]></code>
- <code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
- <code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
- <code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
- <code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
- <code><![CDATA[$normalised['my-form']['details']['avatars'][0]]]></code>
- <code><![CDATA[$normalised['my-form']['details']['avatars'][1]]]></code>
- <code><![CDATA[$normalised['my-form']['details']['avatars'][2]]]></code>
- <code><![CDATA[$normalised['slide-shows'][0]['slides']]]></code>
- <code><![CDATA[$normalised['slide-shows'][0]['slides']]]></code>
- <code><![CDATA[$normalised['slide-shows'][0]['slides']]]></code>
- <code><![CDATA[$normalised['slide-shows'][0]['slides'][0]]]></code>
- <code><![CDATA[$normalised['slide-shows'][0]['slides'][1]]]></code>
+ <code>$normalised['my-form']['details']['avatar']</code>
+ <code>$normalised['my-form']['details']['avatars']</code>
+ <code>$normalised['my-form']['details']['avatars']</code>
+ <code>$normalised['my-form']['details']['avatars']</code>
+ <code>$normalised['my-form']['details']['avatars']</code>
+ <code>$normalised['my-form']['details']['avatars'][0]</code>
+ <code>$normalised['my-form']['details']['avatars'][1]</code>
+ <code>$normalised['my-form']['details']['avatars'][2]</code>
+ <code>$normalised['slide-shows'][0]['slides']</code>
+ <code>$normalised['slide-shows'][0]['slides']</code>
+ <code>$normalised['slide-shows'][0]['slides']</code>
+ <code>$normalised['slide-shows'][0]['slides'][0]</code>
+ <code>$normalised['slide-shows'][0]['slides'][1]</code>
  </MixedArrayAccess>
  <MixedMethodCall>
  <code>getClientFilename</code>
@@ -461,14 +467,14 @@
  <code>getClientFilename</code>
  </MixedMethodCall>
  <UndefinedInterfaceMethod>
- <code><![CDATA[$normalised['my-form']]]></code>
- <code><![CDATA[$normalised['my-form']]]></code>
- <code><![CDATA[$normalised['my-form']]]></code>
- <code><![CDATA[$normalised['my-form']]]></code>
- <code><![CDATA[$normalised['my-form']]]></code>
- <code><![CDATA[$normalised['slide-shows']]]></code>
- <code><![CDATA[$normalised['slide-shows']]]></code>
- <code><![CDATA[$normalised['slide-shows']]]></code>
+ <code>$normalised['my-form']</code>
+ <code>$normalised['my-form']</code>
+ <code>$normalised['my-form']</code>
+ <code>$normalised['my-form']</code>
+ <code>$normalised['my-form']</code>
+ <code>$normalised['slide-shows']</code>
+ <code>$normalised['slide-shows']</code>
+ <code>$normalised['slide-shows']</code>
  </UndefinedInterfaceMethod>
  </file>
 </files>
diff --git a/src/functions/marshal_headers_from_sapi.php b/src/functions/marshal_headers_from_sapi.php
@@ -4,16 +4,19 @@
 
 namespace Laminas\Diactoros;
 
+use function array_filter;
 use function array_key_exists;
 use function is_string;
 use function str_starts_with;
 use function strtolower;
 use function strtr;
 use function substr;
 
+use const ARRAY_FILTER_USE_KEY;
+
 /**
  * @param array $server Values obtained from the SAPI (generally `$_SERVER`).
- * @return array Header/value pairs
+ * @return array<non-empty-string, mixed> Header/value pairs
  */
 function marshalHeadersFromSapi(array $server): array
 {
@@ -30,7 +33,7 @@ function marshalHeadersFromSapi(array $server): array
 
  $headers = [];
  foreach ($server as $key => $value) {
- if (! is_string($key)) {
+ if (! is_string($key) || $key === '') {
  continue;
  }
 
@@ -63,5 +66,9 @@ function marshalHeadersFromSapi(array $server): array
  }
  }
 
- return $headers;
+ // Filter out integer keys.
+ // These can occur if the translated header name is a string integer.
+ // PHP will cast those to integers when assigned to an array.
+ // This filters them out.
+ return array_filter($headers, fn(string|int $key): bool => is_string($key), ARRAY_FILTER_USE_KEY);
 }
diff --git a/test/ServerRequestFactoryTest.php b/test/ServerRequestFactoryTest.php
@@ -43,6 +43,10 @@ public function testMarshalsExpectedHeadersFromServerArray(): void
  'HTTP_X_FOO_BAR' => 'FOOBAR',
  'CONTENT_MD5' => 'CONTENT-MD5',
  'CONTENT_LENGTH' => 'UNSPECIFIED',
+ 123 => 'integer',
+ '1' => 'string-integer',
+ '0' => 'string-zero',
+ '-1' => 'string-negative-integer',
  ];
 
  $expected = [
@@ -65,6 +69,10 @@ public function testMarshalInvalidHeadersStrippedFromServerArray(): void
  'HTTP_AUTHORIZATION' => 'token',
  'MD5' => 'CONTENT-MD5',
  'CONTENT_LENGTH' => 'UNSPECIFIED',
+ 123 => 'integer',
+ '1' => 'string-integer',
+ '0' => 'string-zero',
+ '-1' => 'string-negative-integer',
  ];
 
  //Headers that don't begin with HTTP_ or CONTENT_ will not be returned

diff --git a/test/functions/MarshalHeadersFromSapiTest.php b/test/functions/MarshalHeadersFromSapiTest.php
@@ -34,6 +34,9 @@ public function testReturnsHeaders(): void
  'CONTENT_TEST_XY' => '',
  'CONTENT_TEST_ZZ' => null,
  123 => 'integer',
+ '1' => 'string-integer',
+ '0' => 'string-zero',
+ '-1' => 'string-negative-integer',
  ];
 
  $expectedHeaders = [