Skip to content
This repository was archived by the owner on Feb 7, 2024. It is now read-only.

Fix signature validation #38

Merged
merged 8 commits into from
Jan 2, 2019
Merged

Fix signature validation #38

merged 8 commits into from
Jan 2, 2019

Conversation

stayallive
Copy link
Contributor

@stayallive stayallive commented Dec 7, 2018

I noticed that running this request to the websocket server yields an invalid signature error.

$pusher->get_channels(['filter_by_prefix' => 'presence-global']);

I noticed that the signatures are not generated as the Pusher lib and server expects/validates them.

So I fixed that and modified all tests to use the correct way (according to the Pusher library).


Edit: After more testing this is broken since the route parameters are added as query params causing it to mess up the signature creation...


Edit edit: This now works like it should and ready for review

@stayallive stayallive changed the title Fix signature validation [WIP] Fix signature validation Dec 7, 2018
@stayallive stayallive changed the title [WIP] Fix signature validation Fix signature validation Dec 7, 2018
@stayallive
Copy link
Contributor Author

@mpociot @freekmurze I would like to have your opinion.

To correctly validate the signature we cannot have appId, appKey & channelName in the query params, but I'm not seeing any way to have them being recognized as actual route params because Ratchet is not very flexible on how the route is matched and what happens after.

So to fix any extra parameter added to the request being ignored en thus rendering the signature invalid too I added those to a blacklist to be able to reconstruct the signature how it should be with params that are not route params.

However this feels a bit dirty... we could also special case filter_by_prefix since it's the only actual query param that is being used in the entire (tiny) REST api. However would be cool if we can build upon the Pusher compat with some extra endpoints possibly that will make it more flexible doing it the way it's now...

Let me know what you think on what route (heh, no pun intended) to take with this.

# Conflicts:
#	tests/HttpApi/FetchChannelsTest.php
@stayallive
Copy link
Contributor Author

Rebased with master to fix conflicts.

@mpociot
Copy link
Member

mpociot commented Dec 10, 2018

@stayallive You mention "After more testing this is broken since the route parameters are added as query params causing it to mess up the signature creation..."
So..is this broken?

@stayallive
Copy link
Contributor Author

Please read my update: #38 (comment)

It now works great, but not sure if there is a better way I'm missing.

This still feels a bit wrong to me to whitelist/blacklist the query parameter names but I can see no way to (without rewriting the router and possibly things in Ratchet) to know which query params are coming from the route parameters and which come from the user.

/*
* The `auth_signature` & `body_md5` parameters are not included when calculating the `auth_signature` value.
*
* The `appId`, `appKey` & `channelName` parameters are actually route paramaters and are never supplied by the client.
*/
$params = array_except($request->query(), ['auth_signature', 'body_md5', 'appId', 'appKey', 'channelName']);

@stayallive
Copy link
Contributor Author

@mpociot can you let me know what you plan is with this? 😄

Want to know if I need to merge 1.0.3 in my fork or if I can fix something to get this moving forward.

@stayallive
Copy link
Contributor Author

@mpociot can you let me know what you plan is with this? 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants