amphp / websocket-client

Async WebSocket client for PHP based on Amp.
https://amphp.org/websocket-client
MIT License
147 stars 17 forks source link

Issue with handshake regex #16

Closed dotstormz closed 5 years ago

dotstormz commented 5 years ago

I'm having an issue getting a legitimate Websocket handshake to be recognised. Initially I thought it was the AWS ALB implementation and how is handled headers however debugging suggests its the library. I have looked at the regex in Handshake.php to try and work out the problem. After running the regex through a regex tool I noticed there were some errors in the regex.

The HTTP1.1 Response looks like this:

HTTP/1.1 101 Switching Protocols
upgrade: websocket
connection: upgrade
sec-websocket-accept: iekWUM343JKJJKY/SRcjycD9l8UsRX2iU=
keep-alive: timeout=60, max=999
date: Wed, 30 Jan 2019 04:48:08 GMT

The regex I am referring to is here: https://github.com/amphp/websocket-client/blob/master/lib/Handshake.php#L84

I was getting the error 'Missing "Upgrade: websocket" header.'. I changed it the following and it now works as expected. Can someone please let me know why the additional slashes were in the original regex as they are not valid? Happy to change it back if required but it would be awesome if anyone can identify any problems with my response.

\preg_match_all("((?P<field>[^()<>@,;:\"\/[\]?={}\x01-\x20\x7F]+):[\x20\x09]*(?P<value>[^\x01-\x08\x0A-\x1F\x7F]*)\x0D?[\x20\x09]*\r?\n)", $headerBuffer, $responseHeaders);
trowski commented 5 years ago

Fixed by using our header parser in amphp/http. Please try the new tagged version and let us know if you find any issues.

I'd like to rework this library in the coming months, hopefully splitting some of the common parts shared by websocket-server and this library into a shared library. PRs to help split or update are always welcome.

dotstormz commented 5 years ago

@trowski Thanks for providing that reference however it's still an issue as I connect using:

$handshake = new Websocket\Handshake('someSuperCoolUrl');
$connection = yield Websocket\connect($handshake);

Are you suggesting I fork the library implement the header parser and submit a PR? Happy to do so if you are happy include the ampphp/http dependency in websocket-client ....

dotstormz commented 5 years ago

@trowski Scrap that, I can see the fixes in 0.2.3. I misunderstood your explanation above. Cheers :)

trowski commented 5 years ago

@dotstormz Sorry that I was confusing. Hopefully it's working for you.

I decided to start working on splitting the common parts of this library up into amphp/websocket, so you'll probably see some more changes coming soon!