Skip to content

Use irc-socket Tls fix from silverbucket/irc-socket #322

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

Merged
merged 5 commits into from
Feb 15, 2021

Conversation

silverbucket
Copy link
Member

This PR uses the Tls irc-socket fix in https://github.com/silverbucket/irc-socket/tree/v4-alpha
Tested on my local machine and it works at least with both secure and non-secure connection sand send/receiving messages.

@silverbucket silverbucket added this to the IRC Platform milestone Feb 11, 2021
@silverbucket silverbucket requested a review from galfert February 11, 2021 17:25
@silverbucket silverbucket self-assigned this Feb 11, 2021
@raucao
Copy link
Contributor

raucao commented Feb 15, 2021

I just tried to validate this fix. But both on the master branch, as well as this one, I get CORS errors from trying to connect to Sockethub from Hyperchannel (master) in the first place:

Screenshot from 2021-02-15 16-30-21

I think it shouldn't even use HTTP polling in the first place, so there seems to be something wrong with the connection itself.

Copy link
Contributor

@galfert galfert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that this fix works for me using Hyperchannel master.

@raucao
Copy link
Contributor

raucao commented Feb 15, 2021

Same in both Firefox and Chromium for me. Sockethub client doesn't try to connect via Websockets at all, and fails to connect due to missing CORS headers when falling back to HTTP polling.

Edit: I also have a massive diff on yarn lock files (4553 lines) after running npm i (i.e. lerna bootstrap). But it looks like it's just re-ordering properties in the objects of all package dependencies' metadata...

@galfert
Copy link
Contributor

galfert commented Feb 15, 2021

I just ran yarn install and then DEBUG=sockethub* yarn run dev from the project root.

I also see large diffs for yarn lock files in the root and in packages/sockethub-platform-irc/.

But everything works for me with both Chrome and Firefox.

When I quit the Sockethub server while Hyperchannel is still running, I can see the same error messages as you. So my guess would be that Sockethub didn't start properly for you.

@raucao
Copy link
Contributor

raucao commented Feb 15, 2021

So my guess would be that Sockethub didn't start properly for you.

Looks alright to me:

Screenshot from 2021-02-15 18-06-10

@galfert
Copy link
Contributor

galfert commented Feb 15, 2021

Yes, that looks pretty much the same as here.

Can you open http://localhost:10550/ in the browser?

@raucao
Copy link
Contributor

raucao commented Feb 15, 2021

Can you open http://localhost:10550/ in the browser?

Aha: those examples work just fine, using Websockets immediately! Anything not on he same origin doesn't seem to attempt Websockets and fails at HTTP polling with the CORS errors.

@silverbucket
Copy link
Member Author

@raucao Hmm.. Are both Hyperchannel and Sockethub running on localhost? Becuase if I do that they work fine, but I do get weirdness if they are running on different "hosts" eg local.dev and localhost or some variant like that. localhost + CORS is treated with suspicion by the browser in some cases that wouldn't ordinarily be the case on two different hosts neither of which are localhost..

@silverbucket
Copy link
Member Author

Merging now and will followup with @raucao as it's not related to this PR.

@silverbucket silverbucket merged commit 4f97e41 into master Feb 15, 2021
@silverbucket silverbucket deleted the irc-socket-fix branch February 15, 2021 20:28
silverbucket added a commit that referenced this pull request Mar 16, 2021
* use irc-socket fix from github repo

* use base socket packages, not the socket objects themselves

* update example to default to secure, and only use tls options when tls is used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants