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.

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

Problems with secure websockets #7

Merged
merged 9 commits into from Aug 19, 2014
Merged

Problems with secure websockets #7

merged 9 commits into from Aug 19, 2014

Conversation

asciimike
Copy link
Contributor

Changed default ports to be 443 for SSL and 80 for non-SSL (they were switched). Passed the whole URL to the connection, since it needed both the host and the path. Updated the package by bumping the version number. Updated the README to make ws.connect()'s callback non-optional, since it wasn't optional (at least, not in my experience).

Changed default ports to be 443 for SSL and 80 for non-SSL (they were switched).
…ath. Also fixed the handshaking code since it never seemed to hit.
Bumped package version
Callback in ws.connect() didn't feel very optional, it failed with a hard error when I didn't include it.
this.socket = socket
this.server = typeof parent == "string" ? null : parent
this.server = typeof url.path == "string" ? null : url.path
Copy link
Owner

Choose a reason for hiding this comment

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

The parent parameter has two different meanings, depending on whether this connection is server-side or client-side;

  • For client-side connections, parent is the path string. The host was really missing and you patch fixes that, great!
  • For server-side, parent is a reference to the Server instance that accepted the connection. For this case, your patch has broken previous behavior.

I suggest something like (parentOrUrl is the second parameter):

if (parentOrUrl instanceof Server) {
    // Server-side connection
    this.server = parentOrUrl
    this.path = null
} else {
    // Client-side
    this.host = parentOrUrl.host
    this.server = null
    this.path = parentOrUrl.path
}

Don't forget to add var Server = require("./Server.js") to the require list near line 70

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment on Server. I was so totally focused on changing the client side code that I completely ignored the server. I'll go ahead and toss this in and make sure it works.

@sitegui
Copy link
Owner

sitegui commented Aug 19, 2014

@mcdonamp, thank you for the explanation. But I confess I'm a little confused here :)

The faye connection code is, in essence:

self._driver.start = function() {
    self._write(this._handshakeRequest())
}

var onConnect = function() { self._driver.start() }
tls.connect(this._uri.port, this._uri.hostname, tlsOptions, onConnect)

And the connection code for nodejs-websocket is:

socket = tls.connect(options)
socket.on("connect", function () {
    that.startHandshake()
})

In both cases the logic is the same, call tls.connect, then attach a 'connect' listener. When the 'connect' event is fired by the tls, do a write with the handshake.

What can be a little confusing is that there are two different 'connect' events:

  1. the 'connect' event fired by the tls, when the socket connection is established securely.
  2. the 'connect' event fired by Connection class, when the other side answers correctly the handshake.

The Connection class listens for the first (tls#event:connect) and it should not depend on whether you're using the nodejs-websocket Server or not.

@asciimike
Copy link
Contributor Author

I think I found out the issue, and we're both right (and both wrong). TLS.connect doesn't emit a 'connect' event, it emits a 'secureConnect' event, so we need to just check if we're a TLS socket or a regular socket and add the appropriate event, then it works.

@sitegui
Copy link
Owner

sitegui commented Aug 19, 2014

Ha! Now I see... :)



if (!this.server) {
if (socket instanceof tls.constructor) {
Copy link
Owner

Choose a reason for hiding this comment

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

tls.constructor is the same as Object, but I got the idea. I'll fix this with socket.constructor.name === 'CleartextStream

@sitegui sitegui changed the title Update index.js Problems with secure websockets Aug 19, 2014
sitegui added a commit that referenced this pull request Aug 19, 2014
Problems with secure websockets
@sitegui sitegui merged commit 3a22d64 into sitegui:master Aug 19, 2014
@sitegui sitegui mentioned this pull request Aug 19, 2014
@sitegui
Copy link
Owner

sitegui commented Aug 19, 2014

@mcdonamp Thank you for all assistance and time!

@asciimike
Copy link
Contributor Author

Thanks for making this in the first place. I know that the folks from Tessel are pretty excited.

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

Successfully merging this pull request may close these issues.

None yet

2 participants