Ignore obviously malformed host headers when constructing a ServerRequest

[TimWolla]

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	yes
RFC	no
QA	no

Description

The comment within the code should be self-explanatory. This adds some very basic checks to the host header as a hardening measure.

[weierophinney]

Quick question: does the Uri class not already disallow these values? Or is the change here primarily to just reject the Host header as a source for creating a Uri instance if it is malformed?

In other words, I'm wondering if we don't already get an exception that occurs for malformed Host headers.

[TimWolla]

Quick question: does the Uri class not already disallow these values?

No, the Uri class accepts arbitrary garbage as the host:

laminas-diactoros/src/Uri.php

Lines 297 to 316 in c272a93

	public function withHost($host) : UriInterface
	{
	if (! is_string($host)) {
	throw new Exception\InvalidArgumentException(sprintf(
	'%s expects a string argument; received %s',
	__METHOD__,
	is_object($host) ? get_class($host) : gettype($host)
	));
	}
	if ($host === $this->host) {
	// Do nothing if no change was made.
	return $this;
	}
	$new = clone $this;
	$new->host = strtolower($host);
	return $new;
	}

In other words, I'm wondering if we don't already get an exception that occurs for malformed Host headers.

No, we don't. We likely should, but that would probably be a BC break. With this PR I wanted to add some minimally invasive checks to reject/ignore the most egregious violations that might actually cause harm, as the host header is possibly untrusted.

[weierophinney]

Add another case here for entries containing newlines.

[TimWolla]

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

[TimWolla]

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

[Ocramius]

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

[Ocramius]

Should this go against 2.11.x, being a bugfix?

[TimWolla]

Should this go against 2.11.x, being a bugfix?

This technically has a (tiny) chance for breakage and is intended as a defense in depth measure. The web server / load balancer in front should already prevent those requests from reaching Diactoros. IMO 2.12 is fine.

[Ocramius]

Nice improvement!

[Ocramius]

Rebased + manually merged in 54d4682

That allowed shrinking of the baseline too.

Thanks @TimWolla!