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

Jump to bottom

Issue with handshake regex #16

Closed
dotstormz opened this issue Jan 30, 2019 · 4 comments
Closed

Issue with handshake regex #16

dotstormz opened this issue Jan 30, 2019 · 4 comments

Comments

@dotstormz
Copy link

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
Copy link
Member

trowski commented Jan 30, 2019

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
Copy link
Author

@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
Copy link
Author

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

@trowski
Copy link
Member

trowski commented Jan 31, 2019 •
edited
Loading

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@trowski @dotstormz