-
Notifications
You must be signed in to change notification settings - Fork 242
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.
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
Add TLS/SSL Support #304
Add TLS/SSL Support #304
Conversation
At a glance there is one or two API changes I might make, but this is amazing. I'll do a review shortly and fix any API changes in post. |
Do provide the NodeJs script as a gist link if possible |
This passed the http tests without code changes which is good enough for merge. Only API change for now was to add default values to 0c6d390. Will adjust API more at a later point. |
Again awesome work, this has been the longest and most prominent missing feature of the plugin! NB: If you do share the gist, I'll be able to add the https tests in my rounds. |
Minor note, plugin has been using v4 lib since plugin v1.6.0. Updated tables in readme to reflect this. |
Added a quick documentation snippet at https://github.com/getnamo/SocketIOClient-Unreal#httpsssl, you can doublecheck that section has accurate information on your current API. |
Release incorporating this made here: https://github.com/getnamo/SocketIOClient-Unreal/releases/tag/v2.1.0 |
@seesemichaelj it looks very good 🤗 Thank you for your incredible work. |
Had to modify the gist a tiny bit (resolve/reject didn't work on my node, likely need to update it), but it was enough to verify TLS functionality, stellar work. Fixed defaults (0d9b549) and expanded documentation a bit (https://github.com/getnamo/SocketIOClient-Unreal#httpstlsssl-support). Btw I really appreciated the comments on the public functions and properties for the new API. |
API modified in v2.3.0, it now supports TLS based purely on URL. Changed SSL toggle to optional forcing parameter. |
This PR is a combination of work from @dobby5, @joelnordell, @getnamo, and myself to add TLS/SSL support. Thanks everyone for their work and support! Unfortunately due to a lot of whitespace changes and merge conflicts, this PR removed individual commits in favor of a simple squashed commit without the majority of the whitespace changes.
With this PR:
SIO_TLS=1
(enabled with this PR), and users can easily enable/disable the TLS functionality with a boolean flagbShouldUseTlsLibraries
on theSocketIOClientComponent
. It was tricky to implement a smart detection based off the host's scheme (http
vshttps
/ws
vswss
) due to the class initialization and connection flexibility.1.4+
to3.0+
. A compatibility table was added to properly document what versions of the plugin work with which socket.io server versions. @getnamo should updateREADME.md
, line 48 to reflect the actual version this change gets published in.If requested, I can provide a NodeJS script that can be used for testing purposes.