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.

Sign up for GitHub

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

Allow connection upgrades #159

Closed
wants to merge 3 commits into from
Closed

Allow connection upgrades #159

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5205bc7
Allow connection upgrades
mbonneau Mar 31, 2017
96d8d26
Add tests and validation
mbonneau Apr 9, 2017
079a20c
Support PHP 5.3
mbonneau Apr 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,9 @@ function ($error) use ($that, $conn, $request) {
}
);

if ($contentLength === 0) {
$upgradeConnection = $request->hasHeader('Connection') && $request->getHeaderLine('Connection') === 'Upgrade';

if (!$upgradeConnection && $contentLength === 0) {
// If Body is empty or Content-Length is 0 and won't emit further data,
// 'data' events from other streams won't be called anymore
$stream->emit('end');
Copy link
Member

Choose a reason for hiding this comment

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

This implies that the request body will never end for connection upgrade with an empty body.

Given that supporting the connection upgrades is optional as per the RFCs, this in turn implies that this creates potential for DOS attack if the consumer expects this end event.

For example, sending an upgrade request against example #4 will never return a result.

What do you think about this?

Afaict this is nothing that can be fixed with this design, so we may want to consider implementing #98 through a dedicated callback, as originally suggested?

Copy link
Author

Choose a reason for hiding this comment

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

I do see the issue that this causes. To support upgrades, the 'end' event could be emitted after the response is constructed and available.

As far as implementing as a separate callback, I am not sure that that makes sense as per the spec, Upgrade can be ignored, or other normal HTTP things can happen (like redirect, 100-continue, etc.).

Expand Down Expand Up @@ -349,7 +351,7 @@ public function handleResponse(ConnectionInterface $connection, RequestInterface

// HTTP/1.1 assumes persistent connection support by default
// we do not support persistent connections, so let the client know
if ($request->getProtocolVersion() === '1.1') {
if ($request->getProtocolVersion() === '1.1' && $response->getStatusCode() !== 101) {
$response = $response->withHeader('Connection', 'close');
}

Expand All @@ -359,9 +361,12 @@ public function handleResponse(ConnectionInterface $connection, RequestInterface
$response = $response->withoutHeader('Content-Length')->withoutHeader('Transfer-Encoding');
}

// response to HEAD and 1xx, 204 and 304 responses MUST NOT include a body
if ($request->getMethod() === 'HEAD' || ($code >= 100 && $code < 200) || $code === 204 || $code === 304) {
$response = $response->withBody(Psr7Implementation\stream_for(''));
// 101 response (Upgrade) should hold onto the body
if ($code !== 101) {
// response to HEAD and 1xx, 204 and 304 responses MUST NOT include a body
if ($request->getMethod() === 'HEAD' || ($code >= 100 && $code < 200) || $code === 204 || $code === 304) {
$response = $response->withBody(Psr7Implementation\stream_for(''));
}
}

$this->handleResponseBody($response, $connection);
Expand Down