Skip to content

Commit

Permalink
#169: fix returning 'ws' for location even when the client connects v…
Browse files Browse the repository at this point in the history
…ia 'wss'
  • Loading branch information
Justin Randell committed Mar 31, 2011
1 parent 5c4c681 commit ce88922
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/socket.io/transports/websocket.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ WebSocket.prototype._onConnect = function(req, socket){
}

var origin = this.request.headers.origin,
location = (origin && origin.substr(0, 5) == 'https' ? 'wss' : 'ws')
location = (this.request.socket.encrypted ? 'wss' : 'ws')
+ '://' + this.request.headers.host + this.request.url;

this.waitingForNonce = false;
Expand Down

9 comments on commit ce88922

@dvv
Copy link
Contributor

@dvv dvv commented on ce88922 May 19, 2011

Choose a reason for hiding this comment

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

If an external proxy which uncyphers SSL (say, stunnel) sits between the client and the server, this.request.socket.encrypted is false and the client won't be able to connect to wss://*. So the best is to or both versions -- this.request.socket.encrypted || origin && origin.substr(0, 6) === 'https:'. Tested on my setup. Works. TIA.

@madari
Copy link

@madari madari commented on ce88922 May 19, 2011

Choose a reason for hiding this comment

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

@dvv: If I'm not wrong, your suggestion would break https -> ws connections

@madari
Copy link

@madari madari commented on ce88922 May 19, 2011

Choose a reason for hiding this comment

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

Maybe something like this.request.socket.encrypted || this.request.headers['x-forwarded-proto'] === 'https' would work. I'm not sure if stunnel passes such information, but in general... just maybe...

See: https://github.com/jcoglan/faye/pull/40

@dvv
Copy link
Contributor

@dvv dvv commented on ce88922 May 20, 2011

Choose a reason for hiding this comment

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

Well, stunnel is TCP proxy, and in its pristine unpatched state knowing nothing of HTTP and its headers. Plus, externally set non-standard headers (x-forwarded-proto) are to be considered tainted, imho.

I think it'd be better to extract current protocol determining logic out of _onConnect into a prototype method which can be overridden by authors.

@dvv
Copy link
Contributor

@dvv dvv commented on ce88922 May 20, 2011

Choose a reason for hiding this comment

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

@guille: Please, consider applying https://gist.github.com/982523 -- this would retain the current behavior still allowing to override the logic. TIA

@tj
Copy link
Contributor

@tj tj commented on ce88922 May 20, 2011

Choose a reason for hiding this comment

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

@dvv imo node needs a public method and/or prop for that as well, i dont think .encrypted is even documented so it's likely volatile (might be wrong)

@macros
Copy link

@macros macros commented on ce88922 Nov 21, 2011

Choose a reason for hiding this comment

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

Either an origin check or a way to force an override via config is needed. The current behavior is a regression for setups that are not doing the encryption inside the node.js process which is a valid deployment strategy.

Safari is the only browser we have come across so far that will not function if ws:// is used on a ssl wrapped conneciton.

@rauchg
Copy link
Contributor

@rauchg rauchg commented on ce88922 Nov 21, 2011

Choose a reason for hiding this comment

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

@macros you're right, we need to add this option. I'll try to get this into the next minor release

@meelash
Copy link

Choose a reason for hiding this comment

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

Any news on this?

Please sign in to comment.