Allow connection upgrades #159
Conversation
|
@mbonneau thanks for filing this PR! 👍 Unfortunately, the PR does not currently contain any documentation, examples or tests, so it's a bit unclear how this can be used. May I ask you provide a simple gist of what you're trying to achieve and how this looks from a consumer perspective? Thanks! |
|
Here is a simple (maybe?) example of a WebSocket echo server: https://gist.github.com/mbonneau/821096940a0f32091f6475c404697193 I wanted to file this PR with tests and more info - but ran out of time at the moment. If this is something you want to move forward with, let me know and I can get testing, docs and look around the edges a little more. I know it is not too close on the horizon, but HTTP2 cleartext uses the upgrade mechanism also. React/http is currently used in here: https://github.com/RxPHP/RxWebsocket/blob/fc60831de1863ed0469f97a1735f6ba07e3b572e/src/Server.php#L44 And is used in testing here: https://github.com/ratchetphp/RFC6455/blob/56aecde679d72d7b1087bbe1d6f7d96e123d396a/tests/ab/startServer.php#L11 Both with upgrades for WebSockets. Upgrade spec for HTTP/1.1 is here: https://tools.ietf.org/html/rfc2616#section-14.42 |
|
Added the |
src/Server.php
Outdated
| 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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.).
|
@mbonneau For the reference, that RFC has been superseded by https://tools.ietf.org/html/rfc7230#section-6.7 and https://tools.ietf.org/html/rfc7231#section-6.2.2 @WyriHaximus I'm okay with getting this in for |
|
@clue Most certainly, it's more something I want to keep supporting. Preferable in |
|
I added some tests and moved I also added quite a bit of validation code to the promise resolution callback. Is there a better place for this? |
Don't close the connection when
Connection: Upgraderequest and101 Switching Protocolsresponse are encountered.This allows other protocols (WebSocket) to use react/http to start up the connection.