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

Fixed Http\Response::setCode producing invalid HTTP header #113

Merged
merged 1 commit into from Dec 19, 2016

Conversation

Projects
None yet
4 participants
@ondrejmirtes
Copy link
Contributor

commented Dec 16, 2016

According to RFC 7230 (HTTP/1.1), the first line of a HTTP response must be:

 The first line of a response message is the status-line, consisting
   of the protocol version, a space (SP), the status code, another
   space, a possibly empty textual phrase describing the status code,
   and ending with CRLF.

     status-line = HTTP-version SP status-code SP reason-phrase CRLF

When setting a custom code through the setCode method, Nette omits this part:

another space, a possibly empty textual phrase describing the status code

This is easily fixed by calling the http_response_code function that's in PHP.

We bumped into this issue when trying to call our own application with Guzzle which has a strict parsing rules of HTTP responses. It requires the space to be there:

/**
 * Parses a response message string into a response object.
 *
 * @param string $message Response message string.
 *
 * @return Response
 */
function parse_response($message)
{
    $data = _parse_message($message);
    if (!preg_match('/^HTTP\/.* [0-9]{3} .*/', $data['start-line'])) {
        throw new \InvalidArgumentException('Invalid response string');
    }
    $parts = explode(' ', $data['start-line'], 3);

    return new Response(
        $parts[1],
        $data['headers'],
        $data['body'],
        explode('/', $parts[0])[1],
        isset($parts[2]) ? $parts[2] : null
    );
}
@JanTvrdik

This comment has been minimized.

@Majkl578

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2016

I'd rather use default reason for each code or empty string for unknown codes.
And maybe also add 2nd optional parameter $reason to allow custom reason override.
That'd match what Symfony does by the way.

@dg

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

It seems that it is related to webserver. It works with Apache or CGI and doesn't work with nginx.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2016

Yep, it doesn't work in nginx + php-fpm, I don't know about Apache. But the code is obviously missing the additional required space:

header($protocol . ' ' . $code, TRUE, $code);

Maybe Apache appends it automatically and nginx doesn't, but I don't expect my webserver to do that.

@dg

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

Yes, because it was completed automatically, I've used this solution. I didn't know it doesn't work with nginx. Thanks for the fix.

@dg dg merged commit 71602f2 into nette:master Dec 19, 2016

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.03%) to 79.167%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

dg added a commit that referenced this pull request Dec 19, 2016

dg added a commit to nette/latte that referenced this pull request Dec 19, 2016

dg added a commit to nette/tracy that referenced this pull request Dec 19, 2016

dg added a commit to nette/tracy that referenced this pull request Dec 19, 2016

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2016

Great, thanks! 👍

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2016

@dg @ondrejmirtes This change is a BC break for people running PHP as an Apache module. See https://secure.php.net/manual/en/function.http-response-code.php#114996 for details.


Edit: So, it turns out it is probably not a BC break, because $httpResponse->setCode(418) never worked with apache2handler SAPI.

The apache2handler SAPI converts header('HTTP/1.1 404') to HTTP/1.1 404 Not Found, however header('HTTP/1.1 418') does not work, you need to write header('HTTP/1.1 418 I\'m A Teapot').

So I think that best we can do, is to change the setCode() methods to sth like

public function setCode($code, $reasonPhrase = NULL)
{
	$code = (int) $code;
	if ($code < 100 || $code > 599) {
		throw new Nette\InvalidArgumentException("Bad HTTP response '$code'.");
	}
	self::checkHeaders();
	$this->code = $code;

	if ($reasonPhrase !== NULL) {
		$protocol = isset($_SERVER['SERVER_PROTOCOL']) ? $_SERVER['SERVER_PROTOCOL'] : 'HTTP/1.1';
		header("$protocol $code $reasonPhrase", TRUE, $code);
	
	} else {
		http_response_code($code);	
	}
	
	return $this;
}
@dg

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

@JanTvrdik http_response_code() works the same way as header() worked.

@dg

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

@JanTvrdik I have added something similar.

dg added a commit to nette/tracy that referenced this pull request Dec 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.