Skip to content

Commit 2515f41

Browse files
XerkusXerkus
authored
Merge pull request #157 from weierophinney/feature/filter-integer-header-names
Filter integer header names during SAPI discovery
2 parents 9bca291 + 5535fe5 commit 2515f41

File tree

8 files changed

+119
-66
lines changed

8 files changed

+119
-66
lines changed

‎docs/book/v3/forward-migration.md

Lines changed: 0 additions & 20 deletions
This file was deleted.

‎docs/book/v3/migration.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ For consumers, usage should be completely backwards compatible, however.
2424
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.
2525
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).
2626

27+
### marshalHeadersFromSapi
28+
29+
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.
30+
Please see the [Security Features](security-features.md) document for more details.
31+
2732
## Removed
2833

2934
The following features were removed for version 3.

‎docs/book/v3/security.md

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
# Security Features
2+
3+
## ServerRequestFilterInterface defaults
4+
5+
`Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface` is used by `ServerRequestFactory::fromGlobals()` to allow modifying the generated `ServerRequest` instance prior to returning it.
6+
The primary use case is to allow modifying the generated URI based on the presence of headers such as `X-Forwarded-Host`.
7+
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.
8+
We also similarly examine the `X-Forwarded-Port` header.
9+
10+
To accommodate this use case, we provide `Laminas\Diactoros\ServerRequestFilter\FilterUsingXForwardedHeaders`.
11+
12+
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.
13+
(This value is found in `$_SERVER['REMOTE_ADDR']`, which is present as `$request->getServerParams()['REMOTE_ADDR']` within PSR-7 implementations.)
14+
`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.
15+
We use this filter by default, marked to trust **only proxies on private subnets**.
16+
17+
If you **do not** need the functionality, we recommend specifying `Laminas\Diactoros\ServerRequestFilter\DoNotFilter` as the configured `FilterServerRequestInterface` in your application.
18+
19+
## Filtering of integer header names
20+
21+
[PSR-7](https://www.php-fig.org/psr/psr-7/) targets [RFC 7230](https://www.rfc-editor.org/rfc/rfc7230).
22+
RFC-7230 defines an ABNF pattern for header field names that allows the possibility of using an integer as a header field; e.g.,
23+
24+
```http
25+
1234: header value
26+
```
27+
28+
The PSR-7, `Psr\Http\MessageInterface::getHeaders()` method requires implementations to return an associative array, where the key is the header field name.
29+
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).
30+
This presents several issues:
31+
32+
- First, it means that consumers cannot depend on the header field name returned being a string.
33+
- Second, our own validation of header field name will fail, as it will not see a string.
34+
35+
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.
36+
However, when using `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, it can present a problem if any discovered headers have field names that evaluate to integers.
37+
38+
To prevent issues, as of version 3.0.0, the `ServerRequestFactory` implementation in Diactoros filters out any headers that evaluate to integers.
39+
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`).
40+
41+
> NOTE: **Integer keys can still be returned from getHeaders()**
42+
> While `withHeader()` and `withHeaderLine()` require string name values, please be aware that these can be presented as string integers.
43+
> These names will be considered valid, and that means that when you call `getHeaders()`, any such names will become integers at this time.

‎mkdocs.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ nav:
1717
- Factories: v3/factories.md
1818
- "Server Request Filters": v3/server-request-filters.md
1919
- "Custom Responses": v3/custom-responses.md
20+
- "Security Features": v3/security.md
2021
- Serialization: v3/serialization.md
2122
- API: v3/api.md
2223
- Migration:

‎psalm-baseline.xml

Lines changed: 49 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<?xml version="1.0" encoding="UTF-8"?>
2-
<files psalm-version="5.9.0@8b9ad1eb9e8b7d3101f949291da2b9f7767cd163">
2+
<files psalm-version="5.10.0@a5effd2d2dddd1a7ea7a0f6a051ce63ff979e356">
33
<file src="src/CallbackStream.php">
44
<ImplementedReturnTypeMismatch>
55
<code>null|callable</code>
@@ -80,7 +80,7 @@
8080
<code>$protocolVersion</code>
8181
<code>$requestTarget</code>
8282
<code>$uri</code>
83-
<code><![CDATA[self::getValueFromKey($serializedRequest, 'body')]]></code>
83+
<code>self::getValueFromKey($serializedRequest, 'body')</code>
8484
</MixedArgument>
8585
<MixedAssignment>
8686
<code>$headers</code>
@@ -114,7 +114,7 @@
114114
<code>$protocolVersion</code>
115115
<code>$reasonPhrase</code>
116116
<code>$statusCode</code>
117-
<code><![CDATA[self::getValueFromKey($serializedResponse, 'body')]]></code>
117+
<code>self::getValueFromKey($serializedResponse, 'body')</code>
118118
</MixedArgument>
119119
<MixedAssignment>
120120
<code>$headers</code>
@@ -186,7 +186,7 @@
186186
</file>
187187
<file src="src/ServerRequestFactory.php">
188188
<MixedArgument>
189-
<code><![CDATA[$headers['cookie']]]></code>
189+
<code>$headers['cookie']</code>
190190
</MixedArgument>
191191
<MixedArgumentTypeCoercion>
192192
<code>$headers</code>
@@ -250,42 +250,48 @@
250250
</file>
251251
<file src="src/functions/create_uploaded_file.php">
252252
<MixedArgument>
253-
<code><![CDATA[$spec['error']]]></code>
254-
<code><![CDATA[$spec['name'] ?? null]]></code>
255-
<code><![CDATA[$spec['tmp_name']]]></code>
256-
<code><![CDATA[$spec['type'] ?? null]]></code>
253+
<code>$spec['error']</code>
254+
<code>$spec['name'] ?? null</code>
255+
<code>$spec['tmp_name']</code>
256+
<code>$spec['type'] ?? null</code>
257257
</MixedArgument>
258258
</file>
259259
<file src="src/functions/marshal_headers_from_sapi.php">
260+
<LessSpecificReturnStatement>
261+
<code><![CDATA[array_filter($headers, fn(string|int $key): bool => is_string($key), ARRAY_FILTER_USE_KEY)]]></code>
262+
</LessSpecificReturnStatement>
260263
<MixedAssignment>
261264
<code>$headers[$name]</code>
262265
<code>$headers[$name]</code>
263266
<code>$value</code>
264267
</MixedAssignment>
268+
<MoreSpecificReturnType>
269+
<code><![CDATA[array<non-empty-string, mixed>]]></code>
270+
</MoreSpecificReturnType>
265271
</file>
266272
<file src="src/functions/marshal_method_from_sapi.php">
267273
<MixedInferredReturnType>
268274
<code>string</code>
269275
</MixedInferredReturnType>
270276
<MixedReturnStatement>
271-
<code><![CDATA[$server['REQUEST_METHOD'] ?? 'GET']]></code>
272-
<code><![CDATA[$server['REQUEST_METHOD'] ?? 'GET']]></code>
277+
<code>$server['REQUEST_METHOD'] ?? 'GET'</code>
278+
<code>$server['REQUEST_METHOD'] ?? 'GET'</code>
273279
</MixedReturnStatement>
274280
</file>
275281
<file src="src/functions/marshal_protocol_version_from_sapi.php">
276282
<MixedArgument>
277-
<code><![CDATA[$server['SERVER_PROTOCOL']]]></code>
283+
<code>$server['SERVER_PROTOCOL']</code>
278284
</MixedArgument>
279285
</file>
280286
<file src="src/functions/normalize_server.php">
281287
<MixedArrayAccess>
282-
<code><![CDATA[$apacheRequestHeaders['Authorization']]]></code>
283-
<code><![CDATA[$apacheRequestHeaders['authorization']]]></code>
288+
<code>$apacheRequestHeaders['Authorization']</code>
289+
<code>$apacheRequestHeaders['authorization']</code>
284290
</MixedArrayAccess>
285291
<MixedAssignment>
286292
<code>$apacheRequestHeaders</code>
287-
<code><![CDATA[$server['HTTP_AUTHORIZATION']]]></code>
288-
<code><![CDATA[$server['HTTP_AUTHORIZATION']]]></code>
293+
<code>$server['HTTP_AUTHORIZATION']</code>
294+
<code>$server['HTTP_AUTHORIZATION']</code>
289295
</MixedAssignment>
290296
</file>
291297
<file src="src/functions/normalize_uploaded_files.php">
@@ -308,25 +314,25 @@
308314
$nameTree[$key] ?? null,
309315
$typeTree[$key] ?? null
310316
)</code>
311-
<code><![CDATA[$recursiveNormalize(
317+
<code>$recursiveNormalize(
312318
$files['tmp_name'],
313319
$files['size'],
314320
$files['error'],
315321
$files['name'] ?? null,
316322
$files['type'] ?? null
317-
)]]></code>
323+
)</code>
318324
</MixedFunctionCall>
319325
<MixedInferredReturnType>
320326
<code>array</code>
321327
</MixedInferredReturnType>
322328
<MixedReturnStatement>
323-
<code><![CDATA[$recursiveNormalize(
329+
<code>$recursiveNormalize(
324330
$files['tmp_name'],
325331
$files['size'],
326332
$files['error'],
327333
$files['name'] ?? null,
328334
$files['type'] ?? null
329-
)]]></code>
335+
)</code>
330336
</MixedReturnStatement>
331337
</file>
332338
<file src="src/functions/parse_cookie_header.php">
@@ -386,7 +392,7 @@
386392
</file>
387393
<file src="test/ServerRequestFactoryTest.php">
388394
<InvalidArgument>
389-
<code><![CDATA[$normalizedFiles['fooFiles']]]></code>
395+
<code>$normalizedFiles['fooFiles']</code>
390396
</InvalidArgument>
391397
</file>
392398
<file src="test/ServerRequestTest.php">
@@ -434,23 +440,23 @@
434440
</file>
435441
<file src="test/functions/NormalizeUploadedFilesTest.php">
436442
<MixedArgument>
437-
<code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
438-
<code><![CDATA[$normalised['slide-shows'][0]['slides']]]></code>
443+
<code>$normalised['my-form']['details']['avatars']</code>
444+
<code>$normalised['slide-shows'][0]['slides']</code>
439445
</MixedArgument>
440446
<MixedArrayAccess>
441-
<code><![CDATA[$normalised['my-form']['details']['avatar']]]></code>
442-
<code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
443-
<code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
444-
<code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
445-
<code><![CDATA[$normalised['my-form']['details']['avatars']]]></code>
446-
<code><![CDATA[$normalised['my-form']['details']['avatars'][0]]]></code>
447-
<code><![CDATA[$normalised['my-form']['details']['avatars'][1]]]></code>
448-
<code><![CDATA[$normalised['my-form']['details']['avatars'][2]]]></code>
449-
<code><![CDATA[$normalised['slide-shows'][0]['slides']]]></code>
450-
<code><![CDATA[$normalised['slide-shows'][0]['slides']]]></code>
451-
<code><![CDATA[$normalised['slide-shows'][0]['slides']]]></code>
452-
<code><![CDATA[$normalised['slide-shows'][0]['slides'][0]]]></code>
453-
<code><![CDATA[$normalised['slide-shows'][0]['slides'][1]]]></code>
447+
<code>$normalised['my-form']['details']['avatar']</code>
448+
<code>$normalised['my-form']['details']['avatars']</code>
449+
<code>$normalised['my-form']['details']['avatars']</code>
450+
<code>$normalised['my-form']['details']['avatars']</code>
451+
<code>$normalised['my-form']['details']['avatars']</code>
452+
<code>$normalised['my-form']['details']['avatars'][0]</code>
453+
<code>$normalised['my-form']['details']['avatars'][1]</code>
454+
<code>$normalised['my-form']['details']['avatars'][2]</code>
455+
<code>$normalised['slide-shows'][0]['slides']</code>
456+
<code>$normalised['slide-shows'][0]['slides']</code>
457+
<code>$normalised['slide-shows'][0]['slides']</code>
458+
<code>$normalised['slide-shows'][0]['slides'][0]</code>
459+
<code>$normalised['slide-shows'][0]['slides'][1]</code>
454460
</MixedArrayAccess>
455461
<MixedMethodCall>
456462
<code>getClientFilename</code>
@@ -461,14 +467,14 @@
461467
<code>getClientFilename</code>
462468
</MixedMethodCall>
463469
<UndefinedInterfaceMethod>
464-
<code><![CDATA[$normalised['my-form']]]></code>
465-
<code><![CDATA[$normalised['my-form']]]></code>
466-
<code><![CDATA[$normalised['my-form']]]></code>
467-
<code><![CDATA[$normalised['my-form']]]></code>
468-
<code><![CDATA[$normalised['my-form']]]></code>
469-
<code><![CDATA[$normalised['slide-shows']]]></code>
470-
<code><![CDATA[$normalised['slide-shows']]]></code>
471-
<code><![CDATA[$normalised['slide-shows']]]></code>
470+
<code>$normalised['my-form']</code>
471+
<code>$normalised['my-form']</code>
472+
<code>$normalised['my-form']</code>
473+
<code>$normalised['my-form']</code>
474+
<code>$normalised['my-form']</code>
475+
<code>$normalised['slide-shows']</code>
476+
<code>$normalised['slide-shows']</code>
477+
<code>$normalised['slide-shows']</code>
472478
</UndefinedInterfaceMethod>
473479
</file>
474480
</files>

‎src/functions/marshal_headers_from_sapi.php

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,19 @@
44

55
namespace Laminas\Diactoros;
66

7+
use function array_filter;
78
use function array_key_exists;
89
use function is_string;
910
use function str_starts_with;
1011
use function strtolower;
1112
use function strtr;
1213
use function substr;
1314

15+
use const ARRAY_FILTER_USE_KEY;
16+
1417
/**
1518
* @param array $server Values obtained from the SAPI (generally `$_SERVER`).
16-
* @return array Header/value pairs
19+
* @return array<non-empty-string, mixed> Header/value pairs
1720
*/
1821
function marshalHeadersFromSapi(array $server): array
1922
{
@@ -30,7 +33,7 @@ function marshalHeadersFromSapi(array $server): array
3033

3134
$headers = [];
3235
foreach ($server as $key => $value) {
33-
if (! is_string($key)) {
36+
if (! is_string($key) || $key === '') {
3437
continue;
3538
}
3639

@@ -63,5 +66,9 @@ function marshalHeadersFromSapi(array $server): array
6366
}
6467
}
6568

66-
return $headers;
69+
// Filter out integer keys.
70+
// These can occur if the translated header name is a string integer.
71+
// PHP will cast those to integers when assigned to an array.
72+
// This filters them out.
73+
return array_filter($headers, fn(string|int $key): bool => is_string($key), ARRAY_FILTER_USE_KEY);
6774
}

‎test/ServerRequestFactoryTest.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ public function testMarshalsExpectedHeadersFromServerArray(): void
4343
'HTTP_X_FOO_BAR' => 'FOOBAR',
4444
'CONTENT_MD5' => 'CONTENT-MD5',
4545
'CONTENT_LENGTH' => 'UNSPECIFIED',
46+
123 => 'integer',
47+
'1' => 'string-integer',
48+
'0' => 'string-zero',
49+
'-1' => 'string-negative-integer',
4650
];
4751

4852
$expected = [
@@ -65,6 +69,10 @@ public function testMarshalInvalidHeadersStrippedFromServerArray(): void
6569
'HTTP_AUTHORIZATION' => 'token',
6670
'MD5' => 'CONTENT-MD5',
6771
'CONTENT_LENGTH' => 'UNSPECIFIED',
72+
123 => 'integer',
73+
'1' => 'string-integer',
74+
'0' => 'string-zero',
75+
'-1' => 'string-negative-integer',
6876
];
6977

7078
//Headers that don't begin with HTTP_ or CONTENT_ will not be returned

‎test/functions/MarshalHeadersFromSapiTest.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ public function testReturnsHeaders(): void
3434
'CONTENT_TEST_XY' => '',
3535
'CONTENT_TEST_ZZ' => null,
3636
123 => 'integer',
37+
'1' => 'string-integer',
38+
'0' => 'string-zero',
39+
'-1' => 'string-negative-integer',
3740
];
3841

3942
$expectedHeaders = [

0 commit comments

Comments
 (0)