Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore obviously malformed host headers when constructing a ServerRequest #97

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/ServerRequestFactory.php
Expand Up @@ -168,7 +168,15 @@ private static function marshalHostAndPort(array $server, array $headers) : arra

$host = self::getHeaderFromArray('host', $headers, false);
if ($host !== false) {
return self::marshalHostAndPortFromHeader($host);
// Ignore obviously malformed host headers:
// - Whitespace is invalid within a hostname and break the URI representation within HTTP.
// non-printable characters other than SPACE and TAB are already rejected by HeaderSecurity.
// - A comma indicates that multiple host headers have been sent which is not legal
// and might be used in an attack where a load balancer sees a different host header
// than Diactoros.
if (! \preg_match('/[\\t ,]/', $host)) {
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
return self::marshalHostAndPortFromHeader($host);
}
}

if (! isset($server['SERVER_NAME'])) {
Expand Down Expand Up @@ -289,9 +297,10 @@ private static function marshalHostAndPortFromHeader($host): array
/**
* Retrieve a header value from an array of headers using a case-insensitive lookup.
*
* @template T
* @param array<string, string|list<string>> $headers Key/value header pairs
* @param mixed $default Default value to return if header not found
* @return mixed
* @param T $default Default value to return if header not found
* @return string|T
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement!

*/
private static function getHeaderFromArray(string $name, array $headers, $default = null)
{
Expand Down
36 changes: 36 additions & 0 deletions test/ServerRequestFactoryTest.php
Expand Up @@ -786,4 +786,40 @@ public function testHonorsHostHeaderOverServerNameWhenMarshalingUrl(): void
$uri = $request->getUri();
$this->assertSame('example.com', $uri->getHost());
}

/**
* @psalm-return iterable<string, array{
* 0: string
* }>
*/
public function invalidHostHeaders(): iterable
{
return [
'comma' => ['example.com,example.net'],
'space' => ['example com'],
'tab' => ["example\tcom"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add another case here for entries containing newlines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are already rejected by HeaderSecurity and thus the request completely fails to construct. See: https://github.com/laminas/laminas-diactoros/runs/7151517319?check_suite_focus=true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the usage.md be updated to explain that ServerRequestFactory may throw based on user input and any exceptions need to be gracefully handled: https://github.com/laminas/laminas-diactoros/blob/2.12.x/docs/book/v2/usage.md#marshaling-an-incoming-request?

see WoltLab/WCF#4888

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about it, but this discussion can be deferred to a different issue

];
}

/**
* @dataProvider invalidHostHeaders
*/
public function testRejectsDuplicatedHostHeader(string $host): void
{
$server = [
'HTTP_HOST' => $host,
];

$request = ServerRequestFactory::fromGlobals(
$server,
null,
null,
null,
null,
new DoNotFilter()
);

$uri = $request->getUri();
$this->assertSame('', $uri->getHost());
}
}